On Thu, May 04, 2000 at 11:24:05AM +0200, Manfred Spraul wrote:
> The kernel contains 2 fasync implementations:
> * net/socket.c
> * drivers/char/tty_io.c
>
> The code in tty_io.c uses cli() for synchronization, and cli() +
> tasklets are not SMP safe.
>
> I've written a patch that uses one rwlock for the synchronization, any
> comments?
you haven't actually changed socket.c to use the new methods.
> +/* SMP safe fasync helpers: */
> +extern int generic_fasync_helper(int, struct file *, int, struct fasync_struct **);
> +/* can be called from interrupts */
> +extern void generic_kill_fasync(struct fasync_struct **, int, int);
> +/* only for net: no internal synchronization */
> +extern void kill_fasync(struct fasync_struct *, int, int);
> +
I'd be tempted to change the names of these functions to:
fasync_helper
kill_fasync
__kill_fasync
> --- 2.3/fs/fcntl.c Sun Feb 27 08:57:11 2000
> +++ build-2.3/fs/fcntl.c Thu May 4 11:09:25 2000
> @@ -7,6 +7,7 @@
> #include <linux/mm.h>
> #include <linux/file.h>
> #include <linux/smp_lock.h>
> +#include <linux/slab.h>
you add the slab.h header, but haven't actually slabified the fasync_struct
allocation. I thnk this would be a worthwhile addition; do you agree?
> #include <asm/poll.h>
> #include <asm/siginfo.h>
> @@ -326,6 +327,51 @@
> read_unlock(&tasklist_lock);
> }
this is actually a very bad function. it would be much better to split it
into two, one for on, one for off. also, it's not necessary to take the
write lock for insertions, only deletions (the list isn't ever inconsistent
while adding elements).
> +/*
> + * generic_fasync_helper() is used by some character device drivers (mainly mice)
> + * to set up the fasync queue. It returns negative on error, 0 if it did
> + * no changes and positive if it added/deleted the entry.
> + */
> +static rwlock_t fasync_lock = RW_LOCK_UNLOCKED;
> +int generic_fasync_helper(int fd, struct file * filp, int on, struct fasync_struct **fapp)
> +{
> + struct fasync_struct *fa, **fp;
> + struct fasync_struct *new = NULL;
> + int result = 0;
> +
> + if (on) {
> + new = (struct fasync_struct *)kmalloc(sizeof(struct fasync_struct), GFP_KERNEL);
don't need a cast here.
> + if (!new)
> + return -ENOMEM;
> + }
> + write_lock_irq(&fasync_lock);
> + for (fp = fapp; (fa = *fp) != NULL; fp = &fa->fa_next) {
> + if (fa->fa_file == filp) {
> + if(on) {
> + fa->fa_fd = fd;
> + kfree(new);
> + } else {
> + *fp = fa->fa_next;
> + kfree(fa);
> + result = 1;
> + }
> + goto out;
> + }
> + }
> +
> + if (on) {
> + new->magic = FASYNC_MAGIC;
> + new->fa_file = filp;
> + new->fa_fd = fd;
> + new->fa_next = *fapp;
> + *fapp = new;
> + result = 1;
> + }
> +out:
> + write_unlock_irq(&fasync_lock);
> + return result;
> +}
> +
> void kill_fasync(struct fasync_struct *fa, int sig, int band)
> {
> while (fa) {
> @@ -343,4 +389,11 @@
> send_sigio(fown, fa, band);
> fa = fa->fa_next;
> }
> +}
> +
> +void generic_kill_fasync(struct fasync_struct **fp, int sig, int band)
> +{
> + read_lock(&fasync_lock);
> + kill_fasync(*fp, sig, band);
> + read_unlock(&fasync_lock);
> }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Mon May 15 2000 - 21:00:16 EST