Message-ID: <20201005083829.GA3673@alley>
Date:   Mon, 5 Oct 2020 10:38:29 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
        tj@...nel.org, akpm@...ux-foundation.org,
        Peter Zijlstra <peterz@...radead.org>,
        Zqiang <qiang.zhang@...driver.com>
Subject: Re: [RFC PATCH] kthread: do not modify running work

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

Powered by blists - more mailing lists