Re: Fw: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

On Thu, Apr 26, 2007 at 08:34:06PM +0400, Oleg Nesterov wrote:
On 04/26, Jarek Poplawski wrote:

void cancel_rearming_delayed_work(struct delayed_work *dwork)
{
struct work_struct *work = &dwork->work;
struct cpu_workqueue_struct *cwq = get_wq_data(work);
int done;

I don't understand, why you think cwq cannot be NULL here.

sure it can, this is just a template.


do {
done = 1;
spin_lock_irq(&cwq->lock);

if (!list_empty(&work->entry))
list_del_init(&work->entry);

BTW, isn't needs_a_good_name needles after this and after del_timer positive?

no, we still need it. work->func() may be running on another CPU as well.


else if (test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work)))
done = del_timer(&dwork->timer)

If this runs while a work function is fired in run_workqueue,
it sets _PENDING bit, but if the work skips rearming, we have probably
endless loop, again.

No, if the work skips rearming (or didn't yet), we set WORK_STRUCT_PENDING
successfully.

Sorry! Should be:
"If this runs while a work function is fired in run_workqueue,
it sets _PENDING bit, but if the work skips rearming, I have probably
endless loop, again."


It is something alike to the current
way, with some added measures: you try to shoot a work on the run,
while queued or timer_pending, plus the _PENDING flag set, so it seems,
there is some risk of longer than planed looping.

Sorry, can't understand. done == 0 means that the queueing in progress,
this work should be placed on cwq->worklist very soon, most probably
right after we drop cwq->lock.

I think, theoretically, probably, maybe, there is possible some strange
case, this function gets spin_lock only when: list_empty(&work->entry) == 1
&& _PENDING == 1 && del_timer(&dwork->timer) == 0.


I have to look at this more, at home and, if something new, I'll write
tomorrow. So, the good news, is you should have enough sleep this time!

Thanks for review!

OK. Here is the review:

It looks great!!! I cannot believe, it could be so "easy"!

Regards,
Jarek P.

PS: probably unusable, but for my own satisfaction:

Acked-by: Jarek Poplawski <jarkao2@xxxxx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/