diff mbox

Possible regression with cgroups in 3.11

Message ID 20131118181440.GA2996@google.com
State Accepted
Headers show

Commit Message

Bjorn Helgaas Nov. 18, 2013, 6:14 p.m. UTC 
On Sat, Nov 16, 2013 at 01:53:56PM +0900, Tejun Heo wrote:
> Hello, Bjorn.
> 
> On Fri, Nov 15, 2013 at 05:28:20PM -0700, Bjorn Helgaas wrote:
> > It would be better to fix PCI so we don't call VF driver .probe() methods
> > from inside a PF driver .probe() method, but that's a bigger project.
> 
> Yeah, if pci doesn't need the recursion, we can simply revert restore
> the lockdep annoation on work_on_cpu().
> 
> > @@ -293,7 +293,9 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
> >  	   its local memory on the right node without any need to
> >  	   change it. */
> >  	node = dev_to_node(&dev->dev);
> > -	if (node >= 0) {
> > +	preempt_disable();
> > +
> > +	if (node >= 0 && node != numa_node_id()) {
> 
> A bit of comment here would be nice but yeah I think this should work.
> Can you please also queue the revert of c2fda509667b ("workqueue:
> allow work_on_cpu() to be called recursively") after this patch?
> Please feel free to add my acked-by.

OK, below are the two patches (Alex's fix + the revert) I propose to
merge.  Unless there are objections, I'll ask Linus to pull these
before v3.13-rc1.

Bjorn



commit 84f23f99b507c2c9247f47d3db0f71a3fd65e3a3
Author: Alexander Duyck <alexander.h.duyck@intel.com>
Date:   Mon Nov 18 10:59:59 2013 -0700

    PCI: Avoid unnecessary CPU switch when calling driver .probe() method
    
    If we are already on a CPU local to the device, call the driver .probe()
    method directly without using work_on_cpu().
    
    This is a workaround for a lockdep warning in the following scenario:
    
      pci_call_probe
        work_on_cpu(cpu, local_pci_probe, ...)
          driver .probe
            pci_enable_sriov
              ...
                pci_bus_add_device
                  ...
                    pci_call_probe
                      work_on_cpu(cpu, local_pci_probe, ...)
    
    It would be better to fix PCI so we don't call VF driver .probe() methods
    from inside a PF driver .probe() method, but that's a bigger project.
    
    [bhelgaas: disable preemption, open bugzilla, rework comments & changelog]
    Link: https://bugzilla.kernel.org/show_bug.cgi?id=65071
    Link: http://lkml.kernel.org/r/CAE9FiQXYQEAZ=0sG6+2OdffBqfLS9MpoN1xviRR9aDbxPxcKxQ@mail.gmail.com
    Link: http://lkml.kernel.org/r/20130624195942.40795.27292.stgit@ahduyck-cp1.jf.intel.com
    Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Tejun Heo <tj@kernel.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yinghai Lu Nov. 18, 2013, 7:29 p.m. UTC | #1 
On Mon, Nov 18, 2013 at 10:14 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> A bit of comment here would be nice but yeah I think this should work.
>> Can you please also queue the revert of c2fda509667b ("workqueue:
>> allow work_on_cpu() to be called recursively") after this patch?
>> Please feel free to add my acked-by.
>
> OK, below are the two patches (Alex's fix + the revert) I propose to
> merge.  Unless there are objections, I'll ask Linus to pull these
> before v3.13-rc1.
>
>
>
> commit 84f23f99b507c2c9247f47d3db0f71a3fd65e3a3
> Author: Alexander Duyck <alexander.h.duyck@intel.com>
> Date:   Mon Nov 18 10:59:59 2013 -0700
>
>     PCI: Avoid unnecessary CPU switch when calling driver .probe() method
>
>     If we are already on a CPU local to the device, call the driver .probe()
>     method directly without using work_on_cpu().
>
>     This is a workaround for a lockdep warning in the following scenario:
>
>       pci_call_probe
>         work_on_cpu(cpu, local_pci_probe, ...)
>           driver .probe
>             pci_enable_sriov
>               ...
>                 pci_bus_add_device
>                   ...
>                     pci_call_probe
>                       work_on_cpu(cpu, local_pci_probe, ...)
>
>     It would be better to fix PCI so we don't call VF driver .probe() methods
>     from inside a PF driver .probe() method, but that's a bigger project.
>
>     [bhelgaas: disable preemption, open bugzilla, rework comments & changelog]
>     Link: https://bugzilla.kernel.org/show_bug.cgi?id=65071
>     Link: http://lkml.kernel.org/r/CAE9FiQXYQEAZ=0sG6+2OdffBqfLS9MpoN1xviRR9aDbxPxcKxQ@mail.gmail.com
>     Link: http://lkml.kernel.org/r/20130624195942.40795.27292.stgit@ahduyck-cp1.jf.intel.com
>     Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Acked-by: Tejun Heo <tj@kernel.org>

Tested-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Yinghai Lu <yinghai@kernel.org>

>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 9042fdbd7244..add04e70ac2a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -288,12 +288,24 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
>         int error, node;
>         struct drv_dev_and_id ddi = { drv, dev, id };
>
> -       /* Execute driver initialization on node where the device's
> -          bus is attached to.  This way the driver likely allocates
> -          its local memory on the right node without any need to
> -          change it. */
> +       /*
> +        * Execute driver initialization on node where the device is
> +        * attached.  This way the driver likely allocates its local memory
> +        * on the right node.
> +        */
>         node = dev_to_node(&dev->dev);
> -       if (node >= 0) {
> +       preempt_disable();
> +
> +       /*
> +        * On NUMA systems, we are likely to call a PF probe function using
> +        * work_on_cpu().  If that probe calls pci_enable_sriov() (which
> +        * adds the VF devices via pci_bus_add_device()), we may re-enter
> +        * this function to call the VF probe function.  Calling
> +        * work_on_cpu() again will cause a lockdep warning.  Since VFs are
> +        * always on the same node as the PF, we can work around this by
> +        * avoiding work_on_cpu() when we're already on the correct node.
> +        */
> +       if (node >= 0 && node != numa_node_id()) {
>                 int cpu;
>
>                 get_online_cpus();
> @@ -305,6 +317,8 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
>                 put_online_cpus();
>         } else
>                 error = local_pci_probe(&ddi);
> +
> +       preempt_enable();
>         return error;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Nov. 18, 2013, 8:39 p.m. UTC | #2 
On Mon, Nov 18, 2013 at 11:29:32AM -0800, Yinghai Lu wrote:
> On Mon, Nov 18, 2013 at 10:14 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> A bit of comment here would be nice but yeah I think this should work.
> >> Can you please also queue the revert of c2fda509667b ("workqueue:
> >> allow work_on_cpu() to be called recursively") after this patch?
> >> Please feel free to add my acked-by.
> >
> > OK, below are the two patches (Alex's fix + the revert) I propose to
> > merge.  Unless there are objections, I'll ask Linus to pull these
> > before v3.13-rc1.
> >
> >
> >
> > commit 84f23f99b507c2c9247f47d3db0f71a3fd65e3a3
> > Author: Alexander Duyck <alexander.h.duyck@intel.com>
> > Date:   Mon Nov 18 10:59:59 2013 -0700
> >
> >     PCI: Avoid unnecessary CPU switch when calling driver .probe() method
> >
> >     If we are already on a CPU local to the device, call the driver .probe()
> >     method directly without using work_on_cpu().
> >
> >     This is a workaround for a lockdep warning in the following scenario:
> >
> >       pci_call_probe
> >         work_on_cpu(cpu, local_pci_probe, ...)
> >           driver .probe
> >             pci_enable_sriov
> >               ...
> >                 pci_bus_add_device
> >                   ...
> >                     pci_call_probe
> >                       work_on_cpu(cpu, local_pci_probe, ...)
> >
> >     It would be better to fix PCI so we don't call VF driver .probe() methods
> >     from inside a PF driver .probe() method, but that's a bigger project.
> >
> >     [bhelgaas: disable preemption, open bugzilla, rework comments & changelog]
> >     Link: https://bugzilla.kernel.org/show_bug.cgi?id=65071
> >     Link: http://lkml.kernel.org/r/CAE9FiQXYQEAZ=0sG6+2OdffBqfLS9MpoN1xviRR9aDbxPxcKxQ@mail.gmail.com
> >     Link: http://lkml.kernel.org/r/20130624195942.40795.27292.stgit@ahduyck-cp1.jf.intel.com
> >     Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >     Acked-by: Tejun Heo <tj@kernel.org>
> 
> Tested-by: Yinghai Lu <yinghai@kernel.org>
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Thanks, I added these and pushed my for-linus branch for -next to
pick up before I ask Linus to pull them.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin Nov. 21, 2013, 4:26 a.m. UTC | #3 
On 11/18/2013 03:39 PM, Bjorn Helgaas wrote:
> On Mon, Nov 18, 2013 at 11:29:32AM -0800, Yinghai Lu wrote:
>> On Mon, Nov 18, 2013 at 10:14 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> A bit of comment here would be nice but yeah I think this should work.
>>>> Can you please also queue the revert of c2fda509667b ("workqueue:
>>>> allow work_on_cpu() to be called recursively") after this patch?
>>>> Please feel free to add my acked-by.
>>>
>>> OK, below are the two patches (Alex's fix + the revert) I propose to
>>> merge.  Unless there are objections, I'll ask Linus to pull these
>>> before v3.13-rc1.
>>>
>>>
>>>
>>> commit 84f23f99b507c2c9247f47d3db0f71a3fd65e3a3
>>> Author: Alexander Duyck <alexander.h.duyck@intel.com>
>>> Date:   Mon Nov 18 10:59:59 2013 -0700
>>>
>>>      PCI: Avoid unnecessary CPU switch when calling driver .probe() method
>>>
>>>      If we are already on a CPU local to the device, call the driver .probe()
>>>      method directly without using work_on_cpu().
>>>
>>>      This is a workaround for a lockdep warning in the following scenario:
>>>
>>>        pci_call_probe
>>>          work_on_cpu(cpu, local_pci_probe, ...)
>>>            driver .probe
>>>              pci_enable_sriov
>>>                ...
>>>                  pci_bus_add_device
>>>                    ...
>>>                      pci_call_probe
>>>                        work_on_cpu(cpu, local_pci_probe, ...)
>>>
>>>      It would be better to fix PCI so we don't call VF driver .probe() methods
>>>      from inside a PF driver .probe() method, but that's a bigger project.
>>>
>>>      [bhelgaas: disable preemption, open bugzilla, rework comments & changelog]
>>>      Link: https://bugzilla.kernel.org/show_bug.cgi?id=65071
>>>      Link: http://lkml.kernel.org/r/CAE9FiQXYQEAZ=0sG6+2OdffBqfLS9MpoN1xviRR9aDbxPxcKxQ@mail.gmail.com
>>>      Link: http://lkml.kernel.org/r/20130624195942.40795.27292.stgit@ahduyck-cp1.jf.intel.com
>>>      Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>      Acked-by: Tejun Heo <tj@kernel.org>
>>
>> Tested-by: Yinghai Lu <yinghai@kernel.org>
>> Acked-by: Yinghai Lu <yinghai@kernel.org>
>
> Thanks, I added these and pushed my for-linus branch for -next to
> pick up before I ask Linus to pull them.

Hi guys,

This patch seems to be causing virtio (wouldn't it happen with any other driver too?) to give
the following spew:

[   11.966381] virtio-pci 0000:00:00.0: enabling device (0000 -> 0003)
[   11.968306] BUG: scheduling while atomic: swapper/0/1/0x00000002
[   11.968616] 2 locks held by swapper/0/1:
[   11.969144]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff820162e8>] 
__driver_attach+0x48/0xa0
[   11.969720]  #1:  (&__lockdep_no_validate__){......}, at: [<ffffffff820162f9>] 
__driver_attach+0x59/0xa0
[   11.971519] Modules linked in:
[   11.971519] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W 
3.12.0-next-20131120-sasha-00002-gf582b19 #4023
[   11.972293]  0000000000000003 ffff880fced736c8 ffffffff8429caa2 0000000000000003
[   11.973145]  ffff880fce820000 ffff880fced736e8 ffffffff8115b67b 0000000000000003
[   11.973952]  ffff880fe5dd7880 ffff880fced73768 ffffffff8429d463 ffff880fced73708
[   11.974881] Call Trace:
[   11.975233]  [<ffffffff8429caa2>] dump_stack+0x52/0x7f
[   11.975786]  [<ffffffff8115b67b>] __schedule_bug+0x6b/0x90
[   11.976411]  [<ffffffff8429d463>] __schedule+0x93/0x760
[   11.976971]  [<ffffffff810adfe4>] ? kvm_clock_read+0x24/0x50
[   11.977646]  [<ffffffff8429dde5>] schedule+0x65/0x70
[   11.978223]  [<ffffffff8429cb8d>] schedule_timeout+0x3d/0x260
[   11.978821]  [<ffffffff8117c8ce>] ? put_lock_stats+0xe/0x30
[   11.979595]  [<ffffffff8429eea7>] ? wait_for_completion+0xb7/0x120
[   11.980324]  [<ffffffff8117fd2a>] ? __lock_release+0x1da/0x1f0
[   11.981554]  [<ffffffff8429eea7>] ? wait_for_completion+0xb7/0x120
[   11.981664]  [<ffffffff8429eeaf>] wait_for_completion+0xbf/0x120
[   11.982266]  [<ffffffff81163880>] ? try_to_wake_up+0x2a0/0x2a0
[   11.982891]  [<ffffffff811421a8>] call_usermodehelper_exec+0x198/0x240
[   11.983552]  [<ffffffff811758e8>] ? complete+0x28/0x60
[   11.984053]  [<ffffffff81142385>] call_usermodehelper+0x45/0x50
[   11.984660]  [<ffffffff81a51d64>] kobject_uevent_env+0x594/0x600
[   11.985254]  [<ffffffff81a51ddb>] kobject_uevent+0xb/0x10
[   11.985855]  [<ffffffff82013635>] device_add+0x2b5/0x4a0
[   11.986495]  [<ffffffff8201383e>] device_register+0x1e/0x30
[   11.987051]  [<ffffffff81c59837>] register_virtio_device+0x87/0xb0
[   11.987760]  [<ffffffff81ac36a3>] ? pci_set_master+0x23/0x30
[   11.988410]  [<ffffffff81c5c3f2>] virtio_pci_probe+0x162/0x1c0
[   11.989000]  [<ffffffff81ac725c>] local_pci_probe+0x4c/0xb0
[   11.989683]  [<ffffffff81ac7361>] pci_call_probe+0xa1/0xd0
[   11.990359]  [<ffffffff81ac7643>] pci_device_probe+0x63/0xa0
[   11.991829]  [<ffffffff82015ce3>] ? driver_sysfs_add+0x73/0xb0
[   11.991829]  [<ffffffff8201601f>] really_probe+0x11f/0x2f0
[   11.992234]  [<ffffffff82016273>] driver_probe_device+0x83/0xb0
[   11.992847]  [<ffffffff8201630e>] __driver_attach+0x6e/0xa0
[   11.993407]  [<ffffffff820162a0>] ? driver_probe_device+0xb0/0xb0
[   11.994020]  [<ffffffff820162a0>] ? driver_probe_device+0xb0/0xb0
[   11.994719]  [<ffffffff82014066>] bus_for_each_dev+0x66/0xc0
[   11.995272]  [<ffffffff82015c1e>] driver_attach+0x1e/0x20
[   11.995829]  [<ffffffff8201552e>] bus_add_driver+0x11e/0x240
[   11.996411]  [<ffffffff870d3600>] ? virtio_mmio_init+0x14/0x14
[   11.996996]  [<ffffffff82016958>] driver_register+0xa8/0xf0
[   11.997628]  [<ffffffff870d3600>] ? virtio_mmio_init+0x14/0x14
[   11.998196]  [<ffffffff81ac7774>] __pci_register_driver+0x64/0x70
[   11.998798]  [<ffffffff870d3619>] virtio_pci_driver_init+0x19/0x1b
[   11.999421]  [<ffffffff810020ca>] do_one_initcall+0xca/0x1d0
[   12.000109]  [<ffffffff8114cf0b>] ? parse_args+0x1cb/0x310
[   12.000666]  [<ffffffff87065d76>] ? kernel_init_freeable+0x339/0x339
[   12.001364]  [<ffffffff87065a1a>] do_basic_setup+0x9c/0xbf
[   12.001903]  [<ffffffff87065d76>] ? kernel_init_freeable+0x339/0x339
[   12.002542]  [<ffffffff8708e894>] ? sched_init_smp+0x13f/0x141
[   12.003202]  [<ffffffff87065cf3>] kernel_init_freeable+0x2b6/0x339
[   12.003815]  [<ffffffff84292d4e>] ? kernel_init+0xe/0x130
[   12.004475]  [<ffffffff84292d40>] ? rest_init+0xd0/0xd0
[   12.005011]  [<ffffffff84292d4e>] kernel_init+0xe/0x130
[   12.005541]  [<ffffffff842ac9fc>] ret_from_fork+0x7c/0xb0
[   12.006068]  [<ffffffff84292d40>] ? rest_init+0xd0/0xd0


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Nov. 21, 2013, 4:47 a.m. UTC | #4 
[+cc Jiri]

On Wed, Nov 20, 2013 at 9:26 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> On 11/18/2013 03:39 PM, Bjorn Helgaas wrote:
>>
>> On Mon, Nov 18, 2013 at 11:29:32AM -0800, Yinghai Lu wrote:
>>>
>>> On Mon, Nov 18, 2013 at 10:14 AM, Bjorn Helgaas <bhelgaas@google.com>
>>> wrote:
>>>>>
>>>>> A bit of comment here would be nice but yeah I think this should work.
>>>>> Can you please also queue the revert of c2fda509667b ("workqueue:
>>>>> allow work_on_cpu() to be called recursively") after this patch?
>>>>> Please feel free to add my acked-by.
>>>>
>>>>
>>>> OK, below are the two patches (Alex's fix + the revert) I propose to
>>>> merge.  Unless there are objections, I'll ask Linus to pull these
>>>> before v3.13-rc1.
>>>>
>>>>
>>>>
>>>> commit 84f23f99b507c2c9247f47d3db0f71a3fd65e3a3
>>>> Author: Alexander Duyck <alexander.h.duyck@intel.com>
>>>> Date:   Mon Nov 18 10:59:59 2013 -0700
>>>>
>>>>      PCI: Avoid unnecessary CPU switch when calling driver .probe()
>>>> method
>>>>
>>>>      If we are already on a CPU local to the device, call the driver
>>>> .probe()
>>>>      method directly without using work_on_cpu().
>>>>
>>>>      This is a workaround for a lockdep warning in the following
>>>> scenario:
>>>>
>>>>        pci_call_probe
>>>>          work_on_cpu(cpu, local_pci_probe, ...)
>>>>            driver .probe
>>>>              pci_enable_sriov
>>>>                ...
>>>>                  pci_bus_add_device
>>>>                    ...
>>>>                      pci_call_probe
>>>>                        work_on_cpu(cpu, local_pci_probe, ...)
>>>>
>>>>      It would be better to fix PCI so we don't call VF driver .probe()
>>>> methods
>>>>      from inside a PF driver .probe() method, but that's a bigger
>>>> project.
>>>>
>>>>      [bhelgaas: disable preemption, open bugzilla, rework comments &
>>>> changelog]
>>>>      Link: https://bugzilla.kernel.org/show_bug.cgi?id=65071
>>>>      Link:
>>>> http://lkml.kernel.org/r/CAE9FiQXYQEAZ=0sG6+2OdffBqfLS9MpoN1xviRR9aDbxPxcKxQ@mail.gmail.com
>>>>      Link:
>>>> http://lkml.kernel.org/r/20130624195942.40795.27292.stgit@ahduyck-cp1.jf.intel.com
>>>>      Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>>>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>      Acked-by: Tejun Heo <tj@kernel.org>
>>>
>>>
>>> Tested-by: Yinghai Lu <yinghai@kernel.org>
>>> Acked-by: Yinghai Lu <yinghai@kernel.org>
>>
>>
>> Thanks, I added these and pushed my for-linus branch for -next to
>> pick up before I ask Linus to pull them.
>
>
> Hi guys,
>
> This patch seems to be causing virtio (wouldn't it happen with any other
> driver too?) to give
> the following spew:

Yep, Jiri Slaby reported this earlier.  I dropped those patches for
now.  Yinghai and I both tested this without incident, but we must
have tested quite the same scenario you did.

I'll look at this more tomorrow.  My first thought is that it's
probably silly to worry about preemption when checking the node.  It's
unlikely that we'd be preempted (probably not even possible except at
hot add-time), and the worst that can happen is we run the .probe()
method on the wrong node, which means worse performance but correct
functionality.

Bjorn

> [   11.966381] virtio-pci 0000:00:00.0: enabling device (0000 -> 0003)
> [   11.968306] BUG: scheduling while atomic: swapper/0/1/0x00000002
> [   11.968616] 2 locks held by swapper/0/1:
> [   11.969144]  #0:  (&__lockdep_no_validate__){......}, at:
> [<ffffffff820162e8>] __driver_attach+0x48/0xa0
> [   11.969720]  #1:  (&__lockdep_no_validate__){......}, at:
> [<ffffffff820162f9>] __driver_attach+0x59/0xa0
> [   11.971519] Modules linked in:
> [   11.971519] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W
> 3.12.0-next-20131120-sasha-00002-gf582b19 #4023
> [   11.972293]  0000000000000003 ffff880fced736c8 ffffffff8429caa2
> 0000000000000003
> [   11.973145]  ffff880fce820000 ffff880fced736e8 ffffffff8115b67b
> 0000000000000003
> [   11.973952]  ffff880fe5dd7880 ffff880fced73768 ffffffff8429d463
> ffff880fced73708
> [   11.974881] Call Trace:
> [   11.975233]  [<ffffffff8429caa2>] dump_stack+0x52/0x7f
> [   11.975786]  [<ffffffff8115b67b>] __schedule_bug+0x6b/0x90
> [   11.976411]  [<ffffffff8429d463>] __schedule+0x93/0x760
> [   11.976971]  [<ffffffff810adfe4>] ? kvm_clock_read+0x24/0x50
> [   11.977646]  [<ffffffff8429dde5>] schedule+0x65/0x70
> [   11.978223]  [<ffffffff8429cb8d>] schedule_timeout+0x3d/0x260
> [   11.978821]  [<ffffffff8117c8ce>] ? put_lock_stats+0xe/0x30
> [   11.979595]  [<ffffffff8429eea7>] ? wait_for_completion+0xb7/0x120
> [   11.980324]  [<ffffffff8117fd2a>] ? __lock_release+0x1da/0x1f0
> [   11.981554]  [<ffffffff8429eea7>] ? wait_for_completion+0xb7/0x120
> [   11.981664]  [<ffffffff8429eeaf>] wait_for_completion+0xbf/0x120
> [   11.982266]  [<ffffffff81163880>] ? try_to_wake_up+0x2a0/0x2a0
> [   11.982891]  [<ffffffff811421a8>] call_usermodehelper_exec+0x198/0x240
> [   11.983552]  [<ffffffff811758e8>] ? complete+0x28/0x60
> [   11.984053]  [<ffffffff81142385>] call_usermodehelper+0x45/0x50
> [   11.984660]  [<ffffffff81a51d64>] kobject_uevent_env+0x594/0x600
> [   11.985254]  [<ffffffff81a51ddb>] kobject_uevent+0xb/0x10
> [   11.985855]  [<ffffffff82013635>] device_add+0x2b5/0x4a0
> [   11.986495]  [<ffffffff8201383e>] device_register+0x1e/0x30
> [   11.987051]  [<ffffffff81c59837>] register_virtio_device+0x87/0xb0
> [   11.987760]  [<ffffffff81ac36a3>] ? pci_set_master+0x23/0x30
> [   11.988410]  [<ffffffff81c5c3f2>] virtio_pci_probe+0x162/0x1c0
> [   11.989000]  [<ffffffff81ac725c>] local_pci_probe+0x4c/0xb0
> [   11.989683]  [<ffffffff81ac7361>] pci_call_probe+0xa1/0xd0
> [   11.990359]  [<ffffffff81ac7643>] pci_device_probe+0x63/0xa0
> [   11.991829]  [<ffffffff82015ce3>] ? driver_sysfs_add+0x73/0xb0
> [   11.991829]  [<ffffffff8201601f>] really_probe+0x11f/0x2f0
> [   11.992234]  [<ffffffff82016273>] driver_probe_device+0x83/0xb0
> [   11.992847]  [<ffffffff8201630e>] __driver_attach+0x6e/0xa0
> [   11.993407]  [<ffffffff820162a0>] ? driver_probe_device+0xb0/0xb0
> [   11.994020]  [<ffffffff820162a0>] ? driver_probe_device+0xb0/0xb0
> [   11.994719]  [<ffffffff82014066>] bus_for_each_dev+0x66/0xc0
> [   11.995272]  [<ffffffff82015c1e>] driver_attach+0x1e/0x20
> [   11.995829]  [<ffffffff8201552e>] bus_add_driver+0x11e/0x240
> [   11.996411]  [<ffffffff870d3600>] ? virtio_mmio_init+0x14/0x14
> [   11.996996]  [<ffffffff82016958>] driver_register+0xa8/0xf0
> [   11.997628]  [<ffffffff870d3600>] ? virtio_mmio_init+0x14/0x14
> [   11.998196]  [<ffffffff81ac7774>] __pci_register_driver+0x64/0x70
> [   11.998798]  [<ffffffff870d3619>] virtio_pci_driver_init+0x19/0x1b
> [   11.999421]  [<ffffffff810020ca>] do_one_initcall+0xca/0x1d0
> [   12.000109]  [<ffffffff8114cf0b>] ? parse_args+0x1cb/0x310
> [   12.000666]  [<ffffffff87065d76>] ? kernel_init_freeable+0x339/0x339
> [   12.001364]  [<ffffffff87065a1a>] do_basic_setup+0x9c/0xbf
> [   12.001903]  [<ffffffff87065d76>] ? kernel_init_freeable+0x339/0x339
> [   12.002542]  [<ffffffff8708e894>] ? sched_init_smp+0x13f/0x141
> [   12.003202]  [<ffffffff87065cf3>] kernel_init_freeable+0x2b6/0x339
> [   12.003815]  [<ffffffff84292d4e>] ? kernel_init+0xe/0x130
> [   12.004475]  [<ffffffff84292d40>] ? rest_init+0xd0/0xd0
> [   12.005011]  [<ffffffff84292d4e>] kernel_init+0xe/0x130
> [   12.005541]  [<ffffffff842ac9fc>] ret_from_fork+0x7c/0xb0
> [   12.006068]  [<ffffffff84292d40>] ? rest_init+0xd0/0xd0
>
>
> Thanks,
> Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Nov. 25, 2013, 9:57 p.m. UTC | #5 
On Wed, Nov 20, 2013 at 9:47 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Jiri]
>
> On Wed, Nov 20, 2013 at 9:26 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
>> On 11/18/2013 03:39 PM, Bjorn Helgaas wrote:
>>>
>>> On Mon, Nov 18, 2013 at 11:29:32AM -0800, Yinghai Lu wrote:
>>>>
>>>> On Mon, Nov 18, 2013 at 10:14 AM, Bjorn Helgaas <bhelgaas@google.com>
>>>> wrote:
>>>>>>
>>>>>> A bit of comment here would be nice but yeah I think this should work.
>>>>>> Can you please also queue the revert of c2fda509667b ("workqueue:
>>>>>> allow work_on_cpu() to be called recursively") after this patch?
>>>>>> Please feel free to add my acked-by.
>>>>>
>>>>>
>>>>> OK, below are the two patches (Alex's fix + the revert) I propose to
>>>>> merge.  Unless there are objections, I'll ask Linus to pull these
>>>>> before v3.13-rc1.
>>>>>
>>>>>
>>>>>
>>>>> commit 84f23f99b507c2c9247f47d3db0f71a3fd65e3a3
>>>>> Author: Alexander Duyck <alexander.h.duyck@intel.com>
>>>>> Date:   Mon Nov 18 10:59:59 2013 -0700
>>>>>
>>>>>      PCI: Avoid unnecessary CPU switch when calling driver .probe()
>>>>> method
>>>>>
>>>>>      If we are already on a CPU local to the device, call the driver
>>>>> .probe()
>>>>>      method directly without using work_on_cpu().
>>>>>
>>>>>      This is a workaround for a lockdep warning in the following
>>>>> scenario:
>>>>>
>>>>>        pci_call_probe
>>>>>          work_on_cpu(cpu, local_pci_probe, ...)
>>>>>            driver .probe
>>>>>              pci_enable_sriov
>>>>>                ...
>>>>>                  pci_bus_add_device
>>>>>                    ...
>>>>>                      pci_call_probe
>>>>>                        work_on_cpu(cpu, local_pci_probe, ...)
>>>>>
>>>>>      It would be better to fix PCI so we don't call VF driver .probe()
>>>>> methods
>>>>>      from inside a PF driver .probe() method, but that's a bigger
>>>>> project.
>>>>>
>>>>>      [bhelgaas: disable preemption, open bugzilla, rework comments &
>>>>> changelog]
>>>>>      Link: https://bugzilla.kernel.org/show_bug.cgi?id=65071
>>>>>      Link:
>>>>> http://lkml.kernel.org/r/CAE9FiQXYQEAZ=0sG6+2OdffBqfLS9MpoN1xviRR9aDbxPxcKxQ@mail.gmail.com
>>>>>      Link:
>>>>> http://lkml.kernel.org/r/20130624195942.40795.27292.stgit@ahduyck-cp1.jf.intel.com
>>>>>      Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>>>>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>>      Acked-by: Tejun Heo <tj@kernel.org>
>>>>
>>>>
>>>> Tested-by: Yinghai Lu <yinghai@kernel.org>
>>>> Acked-by: Yinghai Lu <yinghai@kernel.org>
>>>
>>>
>>> Thanks, I added these and pushed my for-linus branch for -next to
>>> pick up before I ask Linus to pull them.
>>
>>
>> Hi guys,
>>
>> This patch seems to be causing virtio (wouldn't it happen with any other
>> driver too?) to give
>> the following spew:
>
> Yep, Jiri Slaby reported this earlier.  I dropped those patches for
> now.  Yinghai and I both tested this without incident, but we must
> have tested quite the same scenario you did.
>
> I'll look at this more tomorrow.  My first thought is that it's
> probably silly to worry about preemption when checking the node.  It's
> unlikely that we'd be preempted (probably not even possible except at
> hot add-time), and the worst that can happen is we run the .probe()
> method on the wrong node, which means worse performance but correct
> functionality.

I dropped the preempt_disable() and re-added this to my for-linus
branch.  Let me know if you see any more issues.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 9042fdbd7244..add04e70ac2a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -288,12 +288,24 @@  static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
 	int error, node;
 	struct drv_dev_and_id ddi = { drv, dev, id };
 
-	/* Execute driver initialization on node where the device's
-	   bus is attached to.  This way the driver likely allocates
-	   its local memory on the right node without any need to
-	   change it. */
+	/*
+	 * Execute driver initialization on node where the device is
+	 * attached.  This way the driver likely allocates its local memory
+	 * on the right node.
+	 */
 	node = dev_to_node(&dev->dev);
-	if (node >= 0) {
+	preempt_disable();
+
+	/*
+	 * On NUMA systems, we are likely to call a PF probe function using
+	 * work_on_cpu().  If that probe calls pci_enable_sriov() (which
+	 * adds the VF devices via pci_bus_add_device()), we may re-enter
+	 * this function to call the VF probe function.  Calling
+	 * work_on_cpu() again will cause a lockdep warning.  Since VFs are
+	 * always on the same node as the PF, we can work around this by
+	 * avoiding work_on_cpu() when we're already on the correct node.
+	 */
+	if (node >= 0 && node != numa_node_id()) {
 		int cpu;
 
 		get_online_cpus();
@@ -305,6 +317,8 @@  static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
 		put_online_cpus();
 	} else
 		error = local_pci_probe(&ddi);
+
+	preempt_enable();
 	return error;
 }
 
commit 2dde5285d06370b2004613ee4fd253e95622af20
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Mon Nov 18 11:00:29 2013 -0700

    Revert "workqueue: allow work_on_cpu() to be called recursively"
    
    This reverts commit c2fda509667b0fda4372a237f5a59ea4570b1627.
    
    c2fda509667b removed lockdep annotation from work_on_cpu() to work around
    the PCI path that calls work_on_cpu() from within a work_on_cpu() work item
    (PF driver .probe() method -> pci_enable_sriov() -> add VFs -> VF driver
    .probe method).
    
    84f23f99b507 ("PCI: Avoid unnecessary CPU switch when calling driver
    .probe() method) avoids that recursive work_on_cpu() use in a different
    way, so this revert restores the work_on_cpu() lockdep annotation.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Tejun Heo <tj@kernel.org>

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 987293d03ebc..5690b8eabfbc 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2840,19 +2840,6 @@  already_gone:
 	return false;
 }
 
-static bool __flush_work(struct work_struct *work)
-{
-	struct wq_barrier barr;
-
-	if (start_flush_work(work, &barr)) {
-		wait_for_completion(&barr.done);
-		destroy_work_on_stack(&barr.work);
-		return true;
-	} else {
-		return false;
-	}
-}
-
 /**
  * flush_work - wait for a work to finish executing the last queueing instance
  * @work: the work to flush
@@ -2866,10 +2853,18 @@  static bool __flush_work(struct work_struct *work)
  */
 bool flush_work(struct work_struct *work)
 {
+	struct wq_barrier barr;
+
 	lock_map_acquire(&work->lockdep_map);
 	lock_map_release(&work->lockdep_map);
 
-	return __flush_work(work);
+	if (start_flush_work(work, &barr)) {
+		wait_for_completion(&barr.done);
+		destroy_work_on_stack(&barr.work);
+		return true;
+	} else {
+		return false;
+	}
 }
 EXPORT_SYMBOL_GPL(flush_work);
 
@@ -4814,14 +4809,7 @@  long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
 	schedule_work_on(cpu, &wfc.work);
-
-	/*
-	 * The work item is on-stack and can't lead to deadlock through
-	 * flushing.  Use __flush_work() to avoid spurious lockdep warnings
-	 * when work_on_cpu()s are nested.
-	 */
-	__flush_work(&wfc.work);
-
+	flush_work(&wfc.work);
 	return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);