Re: Getting the new RxRPC patches upstream

From: David Howells
Date: Tue Apr 24 2007 - 14:23:37 EST

Oleg Nesterov <oleg@xxxxxxxxxx> wrote:

> Sure, I'll grep for cancel_delayed_work(). But unless I missed something,
> this change should be completely transparent for all users. Otherwise, it
> is buggy.

I guess you will have to make sure that cancel_delayed_work() is always
followed by a flush of the workqueue, otherwise you might get this situation:

=============================== =======================
<timer expires>
cancel_delayed_work(x) == 0 -->delayed_work_timer_fn(x)
kfree(x); -->do_IRQ()
y = kmalloc(); // reuses x
--- OOPS ---

That's my main concern. If you are certain that can't happen, then fair

Note that although you can call cancel_delayed_work() from within a work item
handler, you can't then follow it up with a flush as it's very likely to

> > Because calling schedule_delayed_work() is a waste of CPU if the timer
> > expiry handler is currently running at this time as *that* is going to
> > also schedule the delayed work item.
> Yes. But otoh, try_to_del_timer_sync() is a waste of CPU compared to
> del_timer(), when the timer is not pending.

I suppose that's true. As previously stated, my main objection to del_timer()
is the fact that it doesn't tell you if the timer expiry function is still

Can you show me a patch illustrating exactly how you want to change
cancel_delayed_work()? I can't remember whether you've done so already, but
if you have, I can't find it. Is it basically this?:

static inline int cancel_delayed_work(struct delayed_work *work)
int ret;

- ret = del_timer_sync(&work->timer);
+ ret = del_timer(&work->timer);
if (ret)
return ret;

I was thinking this situation might be a problem:

=============================== =======================
<timer expires>
cancel_delayed_work(x) == 0 -->delayed_work_timer_fn(x)
schedule_delayed_work(x,0) -->do_IRQ()
<keventd scheduled>

But it won't, will it?

> > Ah, but the timer routine may try to set the work item pending flag
> > *after* the work_pending() check you have here.
> No, delayed_work_timer_fn() doesn't set the _PENDING flag.

Good point. I don't think that's a problem because cancel_delayed_work()
won't clear the pending flag if it didn't remove a timer.

> First, this is very unlikely event, delayed_work_timer_fn() is very fast
> unless interrupted.

Yeah, I guess so.

Okay, you've convinced me, I think - provided you consider the case I
outlinded at the top of this email.

If you give me a patch to alter cancel_delayed_work(), I'll substitute it for
mine and use that that instead. Dave Miller will just have to live with that
patch being there:-)

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at