linux-kernel - Re: [PATCH 2/8] sched: __wake_up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 7 Apr 2010 17:28:29 -0700
From:	Greg KH <greg@...ah.com>
To:	Michał Nazarewicz <m.nazarewicz@...sung.com>
Cc:	linux-usb@...r.kernel.org, Peter Korsgaard <jacmet@...site.dk>,
	Rupesh Gujare <rupeshgujare@...il.com>,
	linux-kernel@...r.kernel.org,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH 2/8] sched: __wake_up_locked() exported

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@...ah.com> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/