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

Jesse Gross jesse at nicira.com
Thu Jan 30 03:18:35 UTC 2014


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>



More information about the dev mailing list