holding a spin lock across a (potential) sleep is a bug - it can
lead to deadlock.
jan-simon.
Jakub Jelinek wrote:
>
> On Thu, Oct 12, 2000 at 11:38:11AM -0400, Yong Chi wrote:
> > Hopefully this will do for SMP locks. =)
>
> Holding a spinlock for this long (especially when you might sleep there in two
> places (interruptible_sleep_on, put_user)) is basically a bad idea.
> spinlocks are designed to be holded only for short time.
> Either protect just a small critical section with a spinlock, or use
> semaphores.
>
> > --- ds.c.bak Wed Oct 11 13:05:16 2000
> > +++ ds.c Thu Oct 12 11:25:20 2000
> > @@ -95,6 +95,7 @@
> > u_int user_magic;
> > int event_head, event_tail;
> > event_t event[MAX_EVENTS];
> > + spinlock_t lock;
> > struct user_info_t *next;
> > } user_info_t;
> >
> > @@ -567,6 +568,7 @@
> > user->event_tail = user->event_head = 0;
> > user->next = s->user;
> > user->user_magic = USER_MAGIC;
> > + spin_lock_init(&user->lock);
> > s->user = user;
> > file->private_data = user;
> >
> > @@ -616,6 +618,7 @@
> > socket_t i = MINOR(file->f_dentry->d_inode->i_rdev);
> > socket_info_t *s;
> > user_info_t *user;
> > + ssize_t retval=4;
> >
> > DEBUG(2, "ds_read(socket %d)\n", i);
> >
> > @@ -625,16 +628,23 @@
> > return -EINVAL;
> > s = &socket_table[i];
> > user = file->private_data;
> > - if (CHECK_USER(user))
> > - return -EIO;
> > -
> > + spin_lock(&user->lock);
> > + if (CHECK_USER(user)) {
> > + retval= -EIO;
> > + goto read_out;
> > + }
> > +
> > if (queue_empty(user)) {
> > interruptible_sleep_on(&s->queue);
> > if (signal_pending(current))
> > - return -EINTR;
> > + retval= -EINTR;
> > + goto read_out;
> > }
> > put_user(get_queued_event(user), (int *)buf);
> > - return 4;
> > +
> > +read_out:
> > + spin_unlock(&user->lock);
> > + return retval;
> > } /* ds_read */
> >
> > /*====================================================================*/
> > @@ -645,6 +655,7 @@
> > socket_t i = MINOR(file->f_dentry->d_inode->i_rdev);
> > socket_info_t *s;
> > user_info_t *user;
> > + ssize_t retval=4;
> >
> > DEBUG(2, "ds_write(socket %d)\n", i);
> >
> > @@ -656,18 +667,25 @@
> > return -EBADF;
> > s = &socket_table[i];
> > user = file->private_data;
> > - if (CHECK_USER(user))
> > - return -EIO;
> > + spin_lock(&user->lock);
> > + if (CHECK_USER(user)) {
> > + retval= -EIO;
> > + goto write_out;
> > + }
> >
> > if (s->req_pending) {
> > s->req_pending--;
> > get_user(s->req_result, (int *)buf);
> > if ((s->req_result != 0) || (s->req_pending == 0))
> > wake_up_interruptible(&s->request);
> > - } else
> > - return -EIO;
> > + } else {
> > + retval= -EIO;
> > + goto write_out;
> > + }
> >
> > - return 4;
> > +write_out:
> > + spin_unlock(&user->lock);
> > + return retval;
> > } /* ds_write */
> >
> > /*====================================================================*/
>
> Jakub
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Oct 15 2000 - 21:00:23 EST