Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430198 > unrolled thread

Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Tejun Heo <tj@kernel.org> - 2016-06-23 23:40 +0200
    Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Peter Zijlstra <peterz@infradead.org> - 2016-06-24 09:10 +0200
      Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Petr Mladek <pmladek@suse.com> - 2016-06-24 11:10 +0200
      Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Tejun Heo <tj@kernel.org> - 2016-06-24 18:00 +0200
        Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Petr Mladek <pmladek@suse.com> - 2016-06-27 16:40 +0200
          Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Tejun Heo <tj@kernel.org> - 2016-06-28 19:10 +0200
            Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Petr Mladek <pmladek@suse.com> - 2016-06-29 10:20 +0200
              Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker() Tejun Heo <tj@kernel.org> - 2016-06-29 15:20 +0200

#1430198 — Re: [PATCH v9 06/12] kthread: Add kthread_drain_worker()

Hello,

On Wed, Jun 22, 2016 at 10:54:45PM +0200, Peter Zijlstra wrote:
> > + * The caller is responsible for blocking all users of this kthread
> > + * worker from queuing new works. Also it is responsible for blocking
> > + * the already queued works from an infinite re-queuing!
> 
> This, I really dislike that. And it makes the kthread_destroy_worker()
> from the next patch unnecessarily fragile.
> 
> Why not add a kthread_worker::blocked flag somewhere and refuse/WARN
> kthread_queue_work() when that is set.

It's the same logic from workqueue counterpart.  For workqueue,
nothing can make it less fragile as the workqueue struct itself is
freed on destruction.  If its users fail to stop issuing work items,
it'll lead to use-after-free.

IIRC, the draining of self-requeueing work items is a specific
requirement from some edge use case which used workqueue to implement
multi-step state machine.  Given how rare that is and the extra
complexity of identifying self-requeueing cases, let's forget about
draining and on destruction clear the worker pointer to block further
queueing and then flush whatever is in flight.

Thanks.

-- 
tejun

[toc] | [next] | [standalone]

#1430412

On Thu, Jun 23, 2016 at 05:32:58PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jun 22, 2016 at 10:54:45PM +0200, Peter Zijlstra wrote:
> > > + * The caller is responsible for blocking all users of this kthread
> > > + * worker from queuing new works. Also it is responsible for blocking
> > > + * the already queued works from an infinite re-queuing!
> > 
> > This, I really dislike that. And it makes the kthread_destroy_worker()
> > from the next patch unnecessarily fragile.
> > 
> > Why not add a kthread_worker::blocked flag somewhere and refuse/WARN
> > kthread_queue_work() when that is set.
> 
> It's the same logic from workqueue counterpart.

So ? Clearly it (the kthread workqueue) can be improved here.

> For workqueue, nothing can make it less fragile as the workqueue
> struct itself is freed on destruction.  If its users fail to stop
> issuing work items, it'll lead to use-after-free.

Right, but this kthread thingy does not, so why not add a failsafe?

> IIRC, the draining of self-requeueing work items is a specific
> requirement from some edge use case which used workqueue to implement
> multi-step state machine. 

Right, that might be an issue,

> Given how rare that is 

Could you then not remove/rework these few cases for workqueue as well
and make that 'better' too?

> and the extra
> complexity of identifying self-requeueing cases, let's forget about
> draining and on destruction clear the worker pointer to block further
> queueing and then flush whatever is in flight.

You're talking about regular workqueues here?

[toc] | [prev] | [next] | [standalone]

#1430489

On Fri 2016-06-24 09:05:15, Peter Zijlstra wrote:
> On Thu, Jun 23, 2016 at 05:32:58PM -0400, Tejun Heo wrote:
> > Hello,
> > 
> > On Wed, Jun 22, 2016 at 10:54:45PM +0200, Peter Zijlstra wrote:
> > > > + * The caller is responsible for blocking all users of this kthread
> > > > + * worker from queuing new works. Also it is responsible for blocking
> > > > + * the already queued works from an infinite re-queuing!
> > > 
> > > This, I really dislike that. And it makes the kthread_destroy_worker()
> > > from the next patch unnecessarily fragile.
> > > 
> > > Why not add a kthread_worker::blocked flag somewhere and refuse/WARN
> > > kthread_queue_work() when that is set.
> > 
> > It's the same logic from workqueue counterpart.
> 
> So ? Clearly it (the kthread workqueue) can be improved here.
> 
> > For workqueue, nothing can make it less fragile as the workqueue
> > struct itself is freed on destruction.  If its users fail to stop
> > issuing work items, it'll lead to use-after-free.
> 
> Right, but this kthread thingy does not, so why not add a failsafe?

The struct kthread_worker is freed in kthread_destroy_worker().
So kthread_worker is the same situation as workqueues.

The allocation/freeing has been added in v2. It helped
to make it clear when the structure was initialized. Note that we
still need the crate/destroy functions to start/stop the kthread.
See the discussion at
https://lkml.kernel.org/g/20150728172657.GC5322@mtj.duckdns.org

I personally do not have strong opinion about it.

On one hand, it makes the code more complex because we need strong
synchronization between queueing/canceling/destroying. There are cases
where it is not that important, for example the hugepage daemon or
hung task. It does not matter if the next round will be done or not.
Well, it is strange if someting gets queued and it is not proceed.

On the other hand, there are situations where the work must be
done, e.g. some I/O operation. They need the strong syncronization.
We could print a warning when queueing a work for a destroyed
(stoped) kthread_worker to catch potential problems. But then we will need
the strong synchronization in all cases to avoid "false" alarms.

After all, the blocked flag will not necessarily make the usage
less hairy. Or did I miss something?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1430777

Hello,

On Fri, Jun 24, 2016 at 09:05:15AM +0200, Peter Zijlstra wrote:
> > Given how rare that is 
> 
> Could you then not remove/rework these few cases for workqueue as well
> and make that 'better' too?

Usage of draining is rare for workqueue but that still means several
legitimate users.  With draining there, it's logical to use it during
shutdown.  I don't think it makes sense to change it on workqueue
side.

> > and the extra
> > complexity of identifying self-requeueing cases, let's forget about
> > draining and on destruction clear the worker pointer to block further
> > queueing and then flush whatever is in flight.
> 
> You're talking about regular workqueues here?

No, kthread worker.  It's unlikely that kthread worker is gonna need
chained draining especially given that most of its usages are gonna be
conversions from raw kthread usages.  We won't lose much if anything
by just ignoring draining and making the code simpler.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1432086

Hi,

On Fri 2016-06-24 11:54:47, Tejun Heo wrote:
> On Fri, Jun 24, 2016 at 09:05:15AM +0200, Peter Zijlstra wrote:
> > > Given how rare that is 
> > 
> > Could you then not remove/rework these few cases for workqueue as well
> > and make that 'better' too?
> 
> Usage of draining is rare for workqueue but that still means several
> legitimate users.  With draining there, it's logical to use it during
> shutdown.  I don't think it makes sense to change it on workqueue
> side.
> 
> > > and the extra
> > > complexity of identifying self-requeueing cases, let's forget about
> > > draining and on destruction clear the worker pointer to block further
> > > queueing and then flush whatever is in flight.
> > 
> > You're talking about regular workqueues here?
> 
> No, kthread worker.  It's unlikely that kthread worker is gonna need
> chained draining especially given that most of its usages are gonna be
> conversions from raw kthread usages.  We won't lose much if anything
> by just ignoring draining and making the code simpler.

OK, so you suggest to do the following:

  1. Add a flag into struct kthread_worker that will prevent
     from further queuing.

  2. kthread_create_worker()/kthread_destroy_worker() will
     not longer dynamically allocate struct kthread_worker.
     They will just start/stop the kthread.


The result will be:

  a. User will not need the strict synchronization between
     the queue and create/destroy operations.

  b. We could get rid of drain_kthread_worker() because
     flush_kthread_worker() will be enough.


IMHO, the 1st change does not make sense without the 2nd one.
Otherwise, users could do an out-of-memory access when testing
the freed kthread_worker flag.

Do I get this correctly please?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1433132

Hello,

On Mon, Jun 27, 2016 at 04:33:50PM +0200, Petr Mladek wrote:
> OK, so you suggest to do the following:
> 
>   1. Add a flag into struct kthread_worker that will prevent
>      from further queuing.

This doesn't add any protection, right?  It's getting freed anyway.

>   2. kthread_create_worker()/kthread_destroy_worker() will
>      not longer dynamically allocate struct kthread_worker.
>      They will just start/stop the kthread.

Ah, okay, I don't think we need to change this.  I was suggesting to
simplify it by dropping the draining and just do flush from destroy.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1433542

On Tue 2016-06-28 13:04:47, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jun 27, 2016 at 04:33:50PM +0200, Petr Mladek wrote:
> > OK, so you suggest to do the following:
> > 
> >   1. Add a flag into struct kthread_worker that will prevent
> >      from further queuing.
> 
> This doesn't add any protection, right?  It's getting freed anyway.
> 
> >   2. kthread_create_worker()/kthread_destroy_worker() will
> >      not longer dynamically allocate struct kthread_worker.
> >      They will just start/stop the kthread.
> 
> Ah, okay, I don't think we need to change this.  I was suggesting to
> simplify it by dropping the draining and just do flush from destroy.

I see. But then it does not address the original concern from Peter
Zijlstra. He did not like that the caller was responsible for blocking
further queueing. It still will be needed. Or did I miss something,
please?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1433723

Hello,

On Wed, Jun 29, 2016 at 10:17:48AM +0200, Petr Mladek wrote:
> > Ah, okay, I don't think we need to change this.  I was suggesting to
> > simplify it by dropping the draining and just do flush from destroy.
> 
> I see. But then it does not address the original concern from Peter
> Zijlstra. He did not like that the caller was responsible for blocking
> further queueing. It still will be needed. Or did I miss something,
> please?

You can only protect against so much.  Let's say we make the worker
struct to be allocated by the user, what then prevents it prematurely
from user side?  Use-after-free is use-after-free.  If we can trivally
add some protection against it, great, but no need to contort the
design to add marginal protection.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web