Re: [PATCH net v2] openvswitch: fix a possible deadlock and lockdep warning

On Thu, Mar 27, 2014 at 11:32 AM, Jesse Gross [off-list ref] wrote:

On Thu, Mar 27, 2014 at 10:44 AM, Flavio Leitner [off-list ref] wrote:

quoted

On Thu, Mar 27, 2014 at 10:37:32AM -0700, Jesse Gross wrote:

quoted

On Thu, Mar 27, 2014 at 10:33 AM, Flavio Leitner [off-list ref] wrote:

quoted

On Thu, Mar 27, 2014 at 10:19:23AM -0700, Pravin Shelar wrote:

quoted

On Thu, Mar 27, 2014 at 7:05 AM, Flavio Leitner [off-list ref] wrote:

quoted

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 [off-list ref]
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.

The ovs patch fixes locking issue.

local cpu check is optimization for better latency which can be
significant on large SMP system. I think we should fix lockdep rather
than adding latency for ovs packet processing. But all this is going
to change with NUMA stats anyways, so I do not have any problem
pushing this new patch.