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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
