On Wed, Apr 07, 2010 at 07:11:05PM +0200, Michał Nazarewicz wrote:
> >On Wed, Apr 07, 2010 at 03:41:29PM +0200, Michal Nazarewicz wrote:
> >>The __wake_up_locked() function has been exported in case modules need it.
>
> On Wed, 07 Apr 2010 17:29:22 +0200, Greg KH <greg@xxxxxxxxx> wrote:
> >What module needs it?
>
> FunctionFS (f_fs) uses it (thus FunctionFS Gadget (g_ffs) uses it).
>
> >Why is it needed?
>
> The FunctionFS uses wait_queue_head_t's spinlock to protect a data
> structure (a queue) used by FunctionFS.
>
> In an earlier version of the code there was another spinlock for
> the queue alone thus when waiting for an event the following had
> to be used:
>
> #v+
> spin_lock(&queue.lock);
> while (queue.empty) {
> spin_unlock(&queue.lock);
> wait_event(&queue.wait_queue, !queue.empty);
> spin_lock(&queue.lock);
> }
> ...
> spin_unlock(&queue.lock);
> #v-
>
> I disliked this code very much and at first hoped that there's some
> "wait_event_holding_lock()" macro which would define the loop shown
> above (similar to user-space condition variables; see
> pthread_cond_wait(3) <http://linux.die.net/man/3/pthread_cond_wait>).
>
> What makes matter worse is that wait_event() calls prepare_to_wait()
> which locks the wait_queue_head_t's spinlock so in the end we have
> unlock one spinlock prior to locking another in situation where one
> spinlock would suffice.
>
> In searching for a better solution I stumbled across fs/timerfd.c which
> used the wait_queue_head_t's spinlock to lock it's own structures:
>
> * http://lxr.linux.no/#linux+v2.6.33/fs/timerfd.c#L39
> * http://lxr.linux.no/#linux+v2.6.33/fs/timerfd.c#L106
>
> So, in the end, I decided to use the same approach in FunctionFS and
> hence by using wait_queue_head_t's spinlock I removed one (unneeded)
> spinlock and decreased number of spin_lock and spin_unlock operations,
> ie. to something like (simplified code):
>
> #v+
> spin_lock(&queue.wait_queue.lock);
> my_wait_event_locked(&queue.wait_queue, !queue.empty);
> ...
> spin_unlock(&queue.lock);
> #v-
>
> where my_wait_event_locked() is a function that does what wait_event()
> does but assumes the wait_queue_head_t's lock is held when entering
> and leaves it held when exiting.
>
> >Are you sure that you really need it?
>
> I could live without it but I strongly believe the code is somehow
> cleaner and more optimised when __wake_up_locked() is used.
Ok, thanks for the detailed description, that makes more sense.
Perhaps you should put this into the patch description next time :)
Oh, and maybe we should move this type of functionality into the core
kernel so that the two users don't have to open-code it both times? If
there are 2 users, odds are someone else will want to also do the same
thing in the near future.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
