Messages in this thread | | | Date | Mon, 14 Feb 2005 17:04:35 -0800 | From | Nishanth Aravamudan <> | Subject | Re: [RFC UPDATE PATCH] add wait_event_*_lock() functions and comments |
| |
On Sat, Feb 12, 2005 at 09:00:52PM -0800, Nish Aravamudan wrote: > On Sun, 13 Feb 2005 03:41:01 +0100, Arnd Bergmann <arnd@arndb.de> wrote: > > On Sünnavend 12 Februar 2005 14:28, Sergey Vlasov wrote: > > > On Sat, 12 Feb 2005 12:38:26 +0100 Arnd Bergmann wrote: > > > > #define __wait_event_lock(wq, condition, lock, flags) \ > > > > do { \ > > > > DEFINE_WAIT(__wait); \ > > > > \ > > > > for (;;) { \ > > > > prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE); \ > > > > spin_lock_irqsave(lock, flags); \ > > > > if (condition) \ > > > > break; \ > > > > spin_unlock_irqrestore(lock, flags); \ > > > > schedule(); \ > > > > } \ > > > > spin_unlock_irqrestore(lock, flags); \ > > > > finish_wait(&wq, &__wait); \ > > > > } while (0) > > > > > > But in this case the result of testing the condition becomes useless > > > after spin_unlock_irqrestore - someone might grab the lock and change > > > things. Therefore the calling code would need to add a loop around > > > wait_event_lock - and the wait_event_* macros were added precisely to > > > encapsulate such a loop and avoid the need to code it manually. > > > > Ok, i understand now what the patch really wants to achieve. However, > > I'm not convinced it's a good idea. In the usb/gadget/serial.c driver, > > this appears to work only because an unconventional locking scheme is > > used, i.e. there is an extra flag (port->port_in_use) that is set to > > tell other functions about the state of the lock in case the lock holder > > wants to sleep. > > > > Is there any place in the kernel that would benefit of the > > wait_event_lock() macro family while using locks without such > > special magic? > > Sorry for replying from a different account, but it's the best I can > do right now. I know while I was scanning the whole kernel for other > wait_event*() replacements, I thought at least a handful of times, > "ugh, I could replace this whole block of code, except for that lock!" > I will try to get you a more concrete example on Monday. Thanks for > the feedback & patience!
Here's at least one example:
drivers/ieee1394/video1394.c:__video1394_ioctl()
I'm having trouble finding more (maybe I already fixed some of them via the existing macros in different ways -- or maybe my memory is just acting up...).
I think this patch/macro can be useful for wait-queues where the same lock is used to protect the sleeper and the sleeper's data?
Any further feedback would be appreciated, or any recommendations for better ways of doing things. I really would just like to have one consistent interface for all wait-queue usage :) The fact that was is nearly (but not quite) done by wait_event*() has to be defined somewhere else just to get that functionality, when it costs little to add it to a common header, makes this a pretty small change to me.
But, Arnd, I understand your concern. It would not be good if we had a bunch of lock-holding sleepers pop up now! I will try to think of a better solution.
Thanks, Nish - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |