Re: [RFC PATCH] kthread: do not modify running work

Re: [RFC PATCH] kthread: do not modify running work

[Petr Mladek]

On Sat 2020-09-26 12:04:26, Hillf Danton wrote:
> 
> It does not make much sense to rearm timer for the delayed work if
> it is worker's current work atm because it's good to do work only
> once.

Quite typical scenario is to queue delayed work from its own callback.
It allows to do the work in regular intervals.

This patch would break it. Or do I miss anything?

Best Regards,
Petr

Re: [RFC PATCH] kthread: do not modify running work

[Thomas Gleixner]

On Thu, Oct 01 2020 at 17:51, Hillf Danton wrote:
> On Wed, 30 Sep 2020 17:01:09 +0200 Petr Mladek wrote:
>> On Sat 2020-09-26 12:04:26, Hillf Danton wrote:
>> > 
>> > It does not make much sense to rearm timer for the delayed work if
>> > it is worker's current work atm because it's good to do work only
>> > once.
>> 
>> Quite typical scenario is to queue delayed work from its own callback.
>> It allows to do the work in regular intervals.
>> 
>> This patch would break it. Or do I miss anything?
>
> It took more than 30 minutes to search kthread_mod_delayed_work for a
> callback at lore.kernel.org and failed, so can you point me to such an
> use case?

Why searching lore.kernel.org and not using git grep?

Aside of that it's pretty irrelevant whether there is a user at the
moment which reschedules work from the callback or not.

It's something which needs to work because its possible from regular
work queues as well and makes a lot of sense.

Thanks,

        tglx

Re: [RFC PATCH] kthread: do not modify running work

[Thomas Gleixner]

On Fri, Oct 02 2020 at 10:34, Hillf Danton wrote:
> On Thu, 01 Oct 2020 15:59:38 +0200 Thomas Gleixner wrote:
>> On Thu, Oct 01 2020 at 17:51, Hillf Danton wrote:
>> Aside of that it's pretty irrelevant whether there is a user at the
>> moment which reschedules work from the callback or not.
>> 
>> It's something which needs to work because its possible from regular
>> work queues as well and makes a lot of sense.
>
> https://lore.kernel.org/lkml/87eemheay8.fsf@nanos.tec.linutronix.de/

That's a completely different thing, really. This adds new functionality
without users and exports it.

kthread work is modeled after workqueue to address specific
needs. delayed work can be rescheduled from the callback and all
variants of timers support rearming the timer from the callback as well.

So having a consistent behaviour accross all these facilities makes
absolutely sense and I don't agree with your sentiment in the changelog
at all.

Just because it does not make sense to you is not a justification for
making stuff inconsistent. You still have not provided a technical
reason why this change is needed.

Thanks,

        tglx

Re: [RFC PATCH] kthread: do not modify running work

[Petr Mladek]

On Sun 2020-10-04 10:12:13, Hillf Danton wrote:
> 
> On Fri, 02 Oct 2020 10:32:32 Thomas Gleixner wrote:
> > On Fri, Oct 02 2020 at 10:34, Hillf Danton wrote:
> > > On Thu, 01 Oct 2020 15:59:38 +0200 Thomas Gleixner wrote:
> > >> On Thu, Oct 01 2020 at 17:51, Hillf Danton wrote:
> > >> Aside of that it's pretty irrelevant whether there is a user at the
> > >> moment which reschedules work from the callback or not.
> > >> 
> > >> It's something which needs to work because its possible from regular
> > >> work queues as well and makes a lot of sense.
> > >
> > > https://lore.kernel.org/lkml/87eemheay8.fsf@nanos.tec.linutronix.de/
> > 
> > That's a completely different thing, really. This adds new functionality
> > without users and exports it.
> > 
> > kthread work is modeled after workqueue to address specific
> > needs.

Exactly.

> > delayed work can be rescheduled from the callback and all
> > variants of timers support rearming the timer from the callback as well.

This might be a bit confusing here. The timer callback just moves
the work to the list of works that are going to be proceed by the
kthread. It neither runs the work nor rearms the timer.

But the timer can be set again by any parallel
kthread_queue_delayed_work() or kthread_mod_delayed_work() call
even the the timer callback is still running. Module that some code
is serialized by the lock.


> What is the difference of invoking kthread_queue_delayed_work() from the
> callback from kthread_mod_delayed_work()?

kthread_queue_delayed_work() does nothing when the work is already
queued. kthread_mod_delayed_work() removes the work from the queue
if it is there and queue it again with the newly requested delay.


> > So having a consistent behaviour accross all these facilities makes
> > absolutely sense and I don't agree with your sentiment in the changelog
> > at all.
> > 
> > Just because it does not make sense to you is not a justification for
> > making stuff inconsistent. You still have not provided a technical
> > reason why this change is needed.
> 
> Given the queue method, it is no win to modify delayed work from callback
> in any case because "we are not adding interfaces just because we can."

What about ipmi_kthread_worker_func()? It is delayed work that
queues itself.


> Nor does it help much in regard of a running work that is delayed two
> minutes because for instance it is not clear that it is a one-off work
> with resources released in the callback.

This is up to the other API user to use the API the right way.

As it has already been mentioned, kthread_worker() API should behave
the same as work_queue API. It is needed for kthreads that have special
needs, for example, real time priority. It should be easy to migrate
between the two APIs. Different behavior would be just a call for
problems.

The dream is that all custom kthreads are converted into either
the classic work_queues or kthread_worker API. People are really
creative when doing the main loop and it is easy to do it wrong.
It causes then problems, for example, with suspend/resume,
livepatching.

What is the motivation for this patch, please?
Does it solve some real problem?

Best Regards,
Petr

Re: [RFC PATCH] kthread: do not modify running work

[Petr Mladek]

On Mon 2020-10-05 10:38:29, Petr Mladek wrote:
> On Sun 2020-10-04 10:12:13, Hillf Danton wrote:
> > On Fri, 02 Oct 2020 10:32:32 Thomas Gleixner wrote:
> > > So having a consistent behaviour accross all these facilities makes
> > > absolutely sense and I don't agree with your sentiment in the changelog
> > > at all.
> > > 
> > > Just because it does not make sense to you is not a justification for
> > > making stuff inconsistent. You still have not provided a technical
> > > reason why this change is needed.
> > 
> > Given the queue method, it is no win to modify delayed work from callback
> > in any case because "we are not adding interfaces just because we can."
> 
> What about ipmi_kthread_worker_func()? It is delayed work that
> queues itself.

The function name is actually mv88e6xxx_irq_poll() in upstream.

The wrong name came from a patch when I worked on the API and
tried to switch some kthreads to it.

Best Regards,
Petr

Re: [RFC PATCH] kthread: do not modify running work

[Petr Mladek]

On Mon 2020-10-05 18:21:05, Hillf Danton wrote:
> On Mon, 5 Oct 2020 10:38:29 Petr Mladek wrote:
> > On Sun 2020-10-04 10:12:13, Hillf Danton wrote:
> > > What is the difference of invoking kthread_queue_delayed_work() from the
> > > callback from kthread_mod_delayed_work()?
> > 
> > kthread_queue_delayed_work() does nothing when the work is already
> > queued. kthread_mod_delayed_work() removes the work from the queue
> > if it is there and queue it again with the newly requested delay.
> 
> Can you let us know the reasons why we need to remove the work from
> queue in callback?

Each work could get queued only once at the same time. It can be either
in work_list or in delayed_work_list. But it must never be in both at
the same time.

Canceling the work solves race with the timer callback. We must be
sure that is not running in parallel and will not try to shuffle
the worker lists.

Note that there is a difference between queued work and running work.
Queued work is either in worker->work_list or in
worker->delayed_work_list. While worker->current_work points
to the currently running work.

Now, the work can be running and queued at the same time be design.
It is quite common usecase.

> > > Given the queue method, it is no win to modify delayed work from callback
> > > in any case because "we are not adding interfaces just because we can."
> > 
> > What about ipmi_kthread_worker_func()? It is delayed work that
> > queues itself.
> 
> Can you shed some light on where I can find ipmi_kthread_worker_func()
> in the mainline?

Ah, the right name is mv88e6xxx_irq_poll().

> > What is the motivation for this patch, please?
> 
> See the subject line.
> 
> > Does it solve some real problem?
> 
> Once more
> 1) avoid running a delayed work more than once if it is one-ff.
>
> 2) cut the risk of modifying a running one-off delayed work with
> resources released in the callback.

Do you have any real bug where this happen, please?

The API works this way be design. It makes it generic and usable
for different use-cases. It is up to the caller to use it the correct
way:

   + queue the work only once when the work should be done only once
   + make sure that the work is not queued when the service is being
     stopped

Note that it is not only about queuing the work from its own
callback. It is also about races between queuing and running.

There is no guarantee when the work will be running. It happens
when the worker thread gets scheduled and the work is the first
on the list. It is up to the API user to make sure that the work
will get queued when needed and vice versa.

It is possible that some other behavior might be more practical
in some scenarios but it would complicate situation in others.


If you need more details, please google the discussion when the API was
developed.

Best Regards,
Petr