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

Groups > linux.kernel > #1424489 > unrolled thread

Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock

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 v2] cgroup: disable irqs while holding css_set_lock Daniel Bristot de Oliveira <daolivei@redhat.com> - 2016-06-16 23:50 +0200
    Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock Tejun Heo <tj@kernel.org> - 2016-06-17 00:10 +0200
      Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock Tejun Heo <tj@kernel.org> - 2016-06-17 00:20 +0200
        Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock Daniel Bristot de Oliveira <daolivei@redhat.com> - 2016-06-17 02:20 +0200
          Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock Tejun Heo <tj@kernel.org> - 2016-06-17 07:40 +0200
            Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock Daniel Bristot de Oliveira <daolivei@redhat.com> - 2016-06-17 22:00 +0200
              Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock Daniel Bristot de Oliveira <daolivei@redhat.com> - 2016-06-22 22:10 +0200

#1424489 — Re: [PATCH v2] cgroup: disable irqs while holding css_set_lock

On 06/07/2016 05:05 PM, Daniel Bristot de Oliveira wrote:
> On 06/07/2016 04:30 PM, Tejun Heo wrote:
>> Is this something in mainline?  This forces all task free path to be
>> irq-safe, which *could* be fine but it's weird to make cgroup free
>> path irq-safe for something which isn't in mainline.
> 
> you mean, mainline linux kernel? if so, yes it is. I was running the
> v4.7-rc2 from Linus, as is.... no external patches applied.
> 

is there any other question/objection about this patch?

-- Daniel

[toc] | [next] | [standalone]

#1424499

On Thu, Jun 16, 2016 at 06:48:56PM -0300, Daniel Bristot de Oliveira wrote:
> On 06/07/2016 05:05 PM, Daniel Bristot de Oliveira wrote:
> > On 06/07/2016 04:30 PM, Tejun Heo wrote:
> >> Is this something in mainline?  This forces all task free path to be
> >> irq-safe, which *could* be fine but it's weird to make cgroup free
> >> path irq-safe for something which isn't in mainline.
> > 
> > you mean, mainline linux kernel? if so, yes it is. I was running the
> > v4.7-rc2 from Linus, as is.... no external patches applied.
> > 
> 
> is there any other question/objection about this patch?

Nope, I was just on vacation.  Applied to cgroup/for-4.7-fixes.

Thanks!

-- 
tejun

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

#1424500

On Thu, Jun 16, 2016 at 06:03:15PM -0400, Tejun Heo wrote:
> On Thu, Jun 16, 2016 at 06:48:56PM -0300, Daniel Bristot de Oliveira wrote:
> > On 06/07/2016 05:05 PM, Daniel Bristot de Oliveira wrote:
> > > On 06/07/2016 04:30 PM, Tejun Heo wrote:
> > >> Is this something in mainline?  This forces all task free path to be
> > >> irq-safe, which *could* be fine but it's weird to make cgroup free
> > >> path irq-safe for something which isn't in mainline.
> > > 
> > > you mean, mainline linux kernel? if so, yes it is. I was running the
> > > v4.7-rc2 from Linus, as is.... no external patches applied.
> > > 
> > 
> > is there any other question/objection about this patch?
> 
> Nope, I was just on vacation.  Applied to cgroup/for-4.7-fixes.

Except that the patch seems to use irqsave/restore instead of plain
irq ones in places.  Care to update those?

Thanks!

-- 
tejun

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

#1424552

On 06/16/2016 07:14 PM, Tejun Heo wrote:
> Except that the patch seems to use irqsave/restore instead of plain
> irq ones in places.  Care to update those?

Hi Tejun,

The use of the irq spin_(un)lock_irq() assumes that the code is always
called with IRQs enabled. But that is not always true in this case, as
we call cgroup_free() in the hard IRQ context, and unconditionally
enable IRQ in this context is a problem. So we need to use irqsave/restore.

Discussing with rostedt, we figured that this needs to be IRQ safe
(using irqsave/restore) in the PREEMPT RT too, so I need to code a v3 of
this patch using raw_spin_*() functions to avoid this problem in the -rt
kernel as well.

Do you see any problems on this?

Thanks! -- Daniel

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

#1424677

Hello,

On Thu, Jun 16, 2016 at 09:12:32PM -0300, Daniel Bristot de Oliveira wrote:
> The use of the irq spin_(un)lock_irq() assumes that the code is always
> called with IRQs enabled. But that is not always true in this case, as
> we call cgroup_free() in the hard IRQ context, and unconditionally
> enable IRQ in this context is a problem. So we need to use irqsave/restore.
> 
> Discussing with rostedt, we figured that this needs to be IRQ safe
> (using irqsave/restore) in the PREEMPT RT too, so I need to code a v3 of
> this patch using raw_spin_*() functions to avoid this problem in the -rt
> kernel as well.
> 
> Do you see any problems on this?

Use of raw_spin is fine but I don't see how, say, rebind_subsystems()
or cgroup_setup_root() can ever be called with irq disabled given that
they assume sleepable context.  Please use _irq and _irqsave
appropriately depending on the circumstances.

Thanks.

-- 
tejun

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

#1425408

Hi Tejun,

On 06/17/2016 02:36 AM, Tejun Heo wrote:
> Please use _irq and _irqsave
> appropriately depending on the circumstances.

ack! I will do it!

Cooking a v3:
- using _irq and _irqsave appropriately, and
- using raw_spin locks functions.

Thanks! -- Daniel

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

#1429085

Hi

On 06/17/2016 04:59 PM, Daniel Bristot de Oliveira wrote:
> - using _irq and _irqsave appropriately, and
> - using raw_spin locks functions.

After some patches/tests on -rt, I figured that there is a -rt specific
patch that moves cgroup_free() calls to the non-atomic context (in the
-rt kernel). So we do not need to convert spinlocks to raw_spinlocks.

I am sending the v3 just to use _irq and _irqsave appropriately.

-- Daniel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web