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

[Pravin Shelar]

I updated commit and added comment and pushed patch to master and 2.1.

Thanks,
Pravin.

On Thu, Jan 30, 2014 at 6:13 PM, Jesse Gross <jesse at nicira.com> wrote:
> 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.