[ovs-dev] [PATCH] datapath: Fix ovs_flow_free() ovs-lock assert.

[Jesse Gross]

On Wed, Jan 29, 2014 at 7:18 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Wed, Jan 29, 2014 at 4:42 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>> On Wed, Jan 29, 2014 at 4:38 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> On Wed, Jan 29, 2014 at 4:35 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>>>> On Wed, Jan 29, 2014 at 4:21 PM, Jesse Gross <jesse at nicira.com> wrote:
>>>>> On Wed, Jan 29, 2014 at 11:22 AM, Pravin Shelar <pshelar at nicira.com> wrote:
>>>>>> On Wed, Jan 29, 2014 at 10:44 AM, Jesse Gross <jesse at nicira.com> wrote:
>>>>>>> On Wed, Jan 29, 2014 at 9:20 AM,  <pshelar at nicira.com> wrote:
>>>>>>>> From: Pravin <pshelar at nicira.com>
>>>>>>>>
>>>>>>>> ovs_flow_free() is not called under ovs-lock during packet
>>>>>>>> execute path. Since packet execute does not touch flow->mask,
>>>>>>>> there is no need to take that lock either. So move assert in
>>>>>>>> case where flow->mask is checked.
>>>>>>>>
>>>>>>>> Found by code inspection.
>>>>>>>>
>>>>>>>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>>>>>>>
>>>>>>> The idea for putting it here is that callers should always hold OVS
>>>>>>> mutex and by putting the check earlier it increases the chances that
>>>>>>> problems will be caught sooner.
>>>>>>
>>>>>> But it is not protecting any data outside of that flow->mask case, so
>>>>>> it is not required.
>>>>>
>>>>> I agree that it's not required but I don't understand the benefit to
>>>>> removing it. Are there cases where we want to call this function where
>>>>> OVS mutex isn't held because there is no flow mask?
>>>>
>>>> I kept it because it was very recently added. I am ok with removing
>>>> it. I will send patch.
>>>
>>> I think we are saying the opposite - I don't understand why we want to
>>> remove it/make it trigger less often.
>>
>> yes, from ovs_packet_cmd_execute().
>
> Ah, OK. When I read packet execute in the commit message, I thought
> that you were referring to general packet processing. I understand the
> problem now - can you update the commit message with the specific
> function name?
>
> Acked-by: Jesse Gross <jesse at nicira.com>

If this is all set, would you mind pushing it? Thomas asked that I
send a pull request with fixes for net-next and I would like to
include this at the same time.