Re: [PATCH] kthread: add kthread_mod_pending_delayed_work api

[Yiwei Zhang‎]

On Mon, Feb 15, 2021 at 5:28 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Sun 2021-02-14 00:06:11, Yiwei Zhang wrote:
> > The existing kthread_mod_delayed_work api will queue a new work if
> > failing to cancel the current work due to no longer being pending.
> > However, there's a case that the same work can be enqueued from both
> > an async request and a delayed work, and a racing could happen if the
> > async request comes right after the timeout delayed work gets
> > scheduled,
>
> By other words, you want to modify the delayed work only when
> it is still waiting in the queue. You do not want to queue new
> work when it has not been already queued. Do I get it correctly?
>
Yes, you are correct.

> Could you please provide a patch where the new API is used?
>
Currently it will only get used in a downstream gpu driver.

> > because the clean up work may not be safe to run twice.
>
> This looks like a bad design of the code. There is likely
> another race that might break it. You should ask the following
> questions:
>
> Why anyone tries to modify the clean up work when it has been already
> queued? There should be only one location/caller that triggers the clean up.
>
The clean up work was initially queued as a safe timeout work just in
case the userspace doesn't queue the cleanup work(e.g. process
crashing), which leaves the global driver in an incorrect driver
state(e.g. power state due to some hinting). In addition, the cleanup
work will also have to clean the cache allocated work struct as well
in the racing scenario.

> Could anyone queue any work to the workqueue after the clean up
> work was queued? The cleanup work should be the last queued one.
> The workqueue user must inform all other users that the queue
> is being destroyed and nobody is allowed to queue any work
> any longer.
>
User can queue the initial work(internally it queues a cleanup work
with a big timeout in case user doesn't queue it later). Then after
user has done stuff within the scope, the user will queue the cleanup
work again to cancel out the effect, which is when it may race with
the underlying timeout'ed cleanup work.

> Best Regards,
> Petr

On Tue, Feb 16, 2021 at 1:12 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> On Sun, Feb 14, 2021 at 12:06:11AM +0000, Yiwei Zhang wrote:
> > The existing kthread_mod_delayed_work api will queue a new work if
> > failing to cancel the current work due to no longer being pending.
> > However, there's a case that the same work can be enqueued from both
> > an async request and a delayed work, and a racing could happen if the
> > async request comes right after the timeout delayed work gets scheduled,
> > because the clean up work may not be safe to run twice.
>
> Who is going to use this?  Please submit it together with the actual
> user.

I'm not sure what I should do if there's no users in the main kernel
tree. Currently it's only potentially used in a downstream gpu driver.
I was proposing here to see if this behavior is reasonable for this
mod function.(My guts feel, when folks are calling this mod function,
they do want to modify "the" exact pending work instead of requeuing
another if the pending one already gets scheduled...)

Best regards,
Yiwei