Re: [PATCH v3 07/22] kthread: Detect when a kthread work is used by more workers

From: Petr Mladek <pmladek@suse.com>
To: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>, Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Josh Triplett <josh@joshtriplett.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Jiri Kosina <jkosina@suse.cz>, Borislav Petkov <bp@suse.de>,
	Michal Hocko <mhocko@suse.cz>,
	linux-mm@kvack.org, Vlastimil Babka <vbabka@suse.cz>,
	linux-api@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 07/22] kthread: Detect when a kthread work is used by more workers
Date: Tue, 24 Nov 2015 17:28:23 +0100	[thread overview]
Message-ID: <20151124162822.GJ10750@pathway.suse.cz> (raw)
In-Reply-To: <20151124144942.GC17033@mtj.duckdns.org>

On Tue 2015-11-24 09:49:42, Tejun Heo wrote:
> Hello, Petr.
> 
> On Tue, Nov 24, 2015 at 11:06:50AM +0100, Petr Mladek wrote:
> > > > @@ -610,6 +625,12 @@ repeat:
> > > >  	if (work) {
> > > >  		__set_current_state(TASK_RUNNING);
> > > >  		work->func(work);
> > > > +
> > > > +		spin_lock_irq(&worker->lock);
> > > > +		/* Allow to queue the work into another worker */
> > > > +		if (!kthread_work_pending(work))
> > > > +			work->worker = NULL;
> > > > +		spin_unlock_irq(&worker->lock);
> > > 
> > > Doesn't this mean that the work item can't be freed from its callback?
> > > That pattern tends to happen regularly.
> > 
> > I am not sure if I understand your question. Do you mean switching
> > work->func during the life time of the struct kthread_work? This
> > should not be affected by the above code.
> 
>IOW, you can't expect the work
> item to remain accessible once the work function starts executing.

I see, I was not aware of this pattern.

> > The above code allows to queue an _unused_ kthread_work into any
> > kthread_worker. For example, it is needed for khugepaged,
> > see http://marc.info/?l=linux-kernel&m=144785344924871&w=2
> > The work is static but the worker can be started/stopped
> > (allocated/freed) repeatedly. It means that the work need
> > to be usable with many workers. But it is associated only
> > with one worker when being used.
>
> It can just re-init work items when it restarts workers, right?

Yes, this would work. It might be slightly inconvenient but
it looks like a good compromise. It helps to keep the API
implementation rather simple and rather secure.

Alternatively, we could allow to queue the work on another worker
if it is not pending. But then we would need to check the pending
status without the worker->lock because work->worker might point
to an already freed worker. We need to check the pending
status in many situations. It might open a can of worms that
I probably do not want to catch.

Thank you and PeterZ for explanation,
Petr