[ovs-dev] [PATCH] datapath: Avoid system freeze due to ovs-flow-rehash softlockup.

[Jesse Gross]

On Wed, May 23, 2012 at 5:11 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Wed, May 23, 2012 at 5:03 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Wed, May 23, 2012 at 10:36 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
>>> OVS datapath does periodic flow table rehash which takes genl_lock
>>> in workq context.
>>> In some cases, like ports add or delete, genl_lock can cause softlockup
>>> as vswitchd would take and succeed with genl_lock and rehash workq
>>> would block on the lock. Eventually rehash will proceed, flow rehash
>>> is low priority task so this is not problem for rehashing.
>>> But it is blocking workq thread; some other workq item from other
>>> kernel subsystem would be blocked and can cause system freeze.
>>> To avoid workq blocking and system freeze, we can use OVS compat workq.
>>> It runs in separate kernel thread thus does not block any non-ovs
>>> deferred workq work item.
>>>
>>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>>
>> Can you add a note in the commit message about how we're planning on
>> addressing this in the future?
>>
> ok
>
>> I got a whitespace error:
>>
>> Applying: datapath: Avoid system freeze due to ovs-flow-rehash softlockup.
>> /home/jesse/openvswitch/.git/rebase-apply/patch:64: new blank line at EOF.
>> +
>> warning: 1 line adds whitespace errors.
>>
>>> diff --git a/datapath/linux/compat/include/linux/workqueue.h b/datapath/linux/compat/include/linux/workqueue.h
>>> index 919afe3..e5e2178 100644
>>> --- a/datapath/linux/compat/include/linux/workqueue.h
>>> +++ b/datapath/linux/compat/include/linux/workqueue.h
>>> @@ -68,6 +64,7 @@ int cancel_delayed_work_sync(struct delayed_work *dwork);
>>>                (_work)->func = (_func);                        \
>>>        } while (0)
>>>
>>> -#endif /* kernel version < 2.6.23 */
>>> +
>>> +extern void flush_scheduled_work(void);
>>
>> Is this declaration necessary?  I didn't find any other references to it.
>
> In new kernel it is declared in workqueue.h and used in interrupt.h,
> thats why we just need declaration.

OK, thanks.

Acked-by: Jesse Gross <jesse at nicira.com>