[ovs-dev] [PATCH net v2] openvswitch: fix a possible deadlock and lockdep warning

Jesse Gross jesse at nicira.com
Thu Mar 27 18:32:13 UTC 2014


On Thu, Mar 27, 2014 at 10:44 AM, Flavio Leitner <fbl at redhat.com> wrote:
> On Thu, Mar 27, 2014 at 10:37:32AM -0700, Jesse Gross wrote:
>> On Thu, Mar 27, 2014 at 10:33 AM, Flavio Leitner <fbl at redhat.com> wrote:
>> > On Thu, Mar 27, 2014 at 10:19:23AM -0700, Pravin Shelar wrote:
>> >> On Thu, Mar 27, 2014 at 7:05 AM, Flavio Leitner <fbl at redhat.com> wrote:
>> >> > There are two problematic situations.
>> >> >
>> >> > A deadlock can happen when is_percpu is false because it can get
>> >> > interrupted while holding the spinlock. Then it executes
>> >> > ovs_flow_stats_update() in softirq context which tries to get
>> >> > the same lock.
>> >> >
>> >> > The second sitation is that when is_percpu is true, the code
>> >> > correctly disables BH but only for the local CPU, so the
>> >> > following can happen when locking the remote CPU without
>> >> > disabling BH:
>> >> >
>> >> >        CPU#0                            CPU#1
>> >> >   ovs_flow_stats_get()
>> >> >    stats_read()
>> >> >  +->spin_lock remote CPU#1        ovs_flow_stats_get()
>> >> >  |  <interrupted>                  stats_read()
>> >> >  |  ...                       +-->  spin_lock remote CPU#0
>> >> >  |                            |     <interrupted>
>> >> >  |  ovs_flow_stats_update()   |     ...
>> >> >  |   spin_lock local CPU#0 <--+     ovs_flow_stats_update()
>> >> >  +---------------------------------- spin_lock local CPU#1
>> >> >
>> >> > This patch disables BH for both cases fixing the deadlocks.
>> >>
>> >> This bug is already fixed in OVS.
>> >
>> > Could you point me to the commit? I am not finding anything
>> > recent.
>>
>> This is the commit:
>>
>> commit 9d73c9cac76ba557fdac4a89c1b7eafe132b85a3
>> Author: Pravin B Shelar <pshelar at nicira.com>
>> Date:   Tue Dec 17 15:43:30 2013 -0800
>>
>>     datapath: Fix deadlock during stats update.
>>
>> I thought that I had sent it in the most recent batch of changes for
>> net but it looks like I missed it.
>
> That commit is incomplete. Look at the scenario #2 which I explain
> why it is needed to disable bh for all cpus and not just local ones.

OK, I understand the second problem now. OVS master (which I am
currently working to cross-port to net-next) uses a different strategy
that also always disables bottom halves for a different reason. Since
I forgot to send the original patch, maybe we can just apply this one
to net instead and use the new stuff directly everywhere else.

Pravin, any objections?



More information about the dev mailing list