Messages in this thread | | | Date | Mon, 2 Jul 2007 15:45:02 +0200 | From | Jarek Poplawski <> | Subject | Re: [PATCH 1/3] rename cancel_rearming_delayed_work() to cancel_delayed_work_sync() |
| |
On Mon, Jul 02, 2007 at 04:14:47PM +0400, Oleg Nesterov wrote: > On 07/02, Jarek Poplawski wrote: > > > > On Sun, Jul 01, 2007 at 07:36:29PM +0400, Oleg Nesterov wrote: > > > Imho, the current naming of cancel_xxx workqueue functions is very confusing. > > > > > > cancel_delayed_work() > > > cancel_rearming_delayed_work() > > > cancel_rearming_delayed_workqueue() // obsolete > > > > > > cancel_work_sync() > > > > > > This looks as if the first 2 functions differ in "type" of their argument which > > > is not true any longer, nowadays the difference is the behaviour. > > > > > > The semantics of cancel_rearming_delayed_work(dwork) was changed significantly, > > > it doesn't require that dwork rearms itself, and cancels dwork synchronously. > > > > > > Rename it to cancel_delayed_work_sync(). This matches cancel_delayed_work() and > > > cancel_work_sync(). Re-create cancel_rearming_delayed_work() as a simple inline > > > obsolete wrapper, like cancel_rearming_delayed_workqueue(). > > > > I like the idea of this change, but have some doubt: "_sync" > > usually suggests the main difference from "" (or _nosync) is: > > _sync waits for something, while _nosync doesn't wait and > > instantly returns. > > Yes, but we already have cancel_work_sync().
And it's OK because it actually can block. Some confusion could appear (maybe to me only) if we would add cancel_work() which would also ... block.
> > > Here it's a bit complicated: cancel_delayed_work() (so nosync), > > actually can wait a little too (on del_timer_sync). And > > cancel_rearming_delayed_work() is really more universal now, > > but still the main difference is this should be used with works > > that rearm (at least sometimes). If there is no rearming - no > > reason for this function (of course not forbidden too) - and > > maybe it better helps to remember the difference? > > There is a reason even if no rearming. We have a lot of > > cancel_delayed_work(); > flush_workqueue(); > > This should be converted to use cancel_delayed_work_sync(). > > Note also that both cancel_work_sync() and cancel_rearming_delayed_work() > can be used on any work (rearming or not) and both imply "flush". > I think the "_rearming" part of the name is very confusing, and a "good" > name should be consistent with cancel_work_sync() and cancel_delayed_work() > which we already have.
I know, but I'm a little afraid of "overloading" the "_sync": here it's kind of double sync (can block plus really finish something). I wonder if this way is used elsewhere in linux.
But I see the more serious reason for it is: it's in your next patches and we don't want to waste time. So, I'll try to check this in the evening, and if nothing strange - I'll send acks tomorrow (of course - not that I think they are needed...).
Thanks, Jarek P. - 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/
| |