On Tue, May 08, 2007 at 04:31:02PM +0400, Oleg Nesterov wrote:
> On 05/08, Jarek Poplawski wrote:
> >
> > On Fri, May 04, 2007 at 12:42:26AM +0400, Oleg Nesterov wrote:
> > ...
> > > +static int try_to_grab_pending(struct work_struct *work)
> > > +{
> > > + struct cpu_workqueue_struct *cwq;
> > > + int ret = 0;
> > > +
> > > + if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work)))
> > > + return 1;
> >
> > Previous version used this check to run del_timer, and I think, it
> > was good idea. So, maybe, try something like this:
> >
> > - run once del_timer before the loop (in cancel_rearming_ only),
>
> hmm, cancel_rearming_ does del_timer() before try_to_grab_pending().
>
> > - add a parmeter to try_to_grab_pending, e.g. "rearming",
> > - add here something like this:
> >
> > else if (rearming && del_timer(&work->timer)
> > return 1;
>
> I thought about adding such a parameter, and I don't like this. This is
> a matter of taste, of course, but _imho_ this uglifies the code.
>
> In any case, unless we do completely different patch, the sequence should be
>
> del_timer() - a pending timer is the most common case
>
> test_and_set_bit(WORK_STRUCT_PENDING) - the work is idle
>
> try-to-steal-the-queued-work
>
> This is what we are doing now.
I simply don't like to call del_timer(), where not needed, but maybe
it's not so expensive and we can afford it...
>
> > > +
> > > + /*
> > > + * The queueing is in progress, or it is already queued. Try to
> > > + * steal it from ->worklist without clearing WORK_STRUCT_PENDING.
> > > + */
> > > +
> > > + cwq = get_wq_data(work);
> > > + if (!cwq)
> > > + return ret;
> >
> > Probably you meant:
> > return 1;
>
> No, we should return 0. This can happen if the queueing of the freshly-
> initialized @dwork is in progress.
>
> NOTE: right now try_to_grab_pending() is called from cancel_xxx() only, so
> this can't happen (it would be meaningless to do cancel_xxx if somebody else
> can queue this work or start the timer), but I'd like try_to_grab_pending()
> to be as generic as possible.
>
> So, we should either return 0, or add BUG_ON(!cwq).
...And you prefer endless loop. Seems brave!
...
> Yes, please, and thank you very much for review!
You welcome & my pleasure,
Bye,
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/
