[ovs-dev] [PATCH] dpif-netdev: fix dp_netdev_free()

[Daniele Di Proietto]

On 9/2/14, 3:28 PM, "Pravin Shelar" <pshelar at nicira.com> wrote:

>On Tue, Sep 2, 2014 at 2:44 PM, Daniele Di Proietto
><ddiproietto at vmware.com> wrote:
>> On 9/2/14, 1:39 PM, "Pravin Shelar" <pshelar at nicira.com> wrote:
>>
>>>On Fri, Aug 29, 2014 at 4:52 PM, Daniele Di Proietto
>>><ddiproietto at vmware.com> wrote:
>>>> dp_netdev_free() must free 'dp->upcall_rwlock', but when upcalls are
>>>>disabled
>>>> (if the datapath is being freed upcalls should be disabled)
>>>>'dp->upcall_rwlock'
>>>> is taken and freeing it causes an assertion to fail.
>>>>
>>>> This commit takes makes sure that the upcalls are disabled and
>>>>releases
>>>> 'dp->upcall_rwlock' before freeing it. A simple testcase is added to
>>>>detect the
>>>> failure.
>>>> ---
>>>>  lib/dpif-netdev.c    | 8 ++++++++
>>>>  tests/dpif-netdev.at | 8 ++++++++
>>>>  2 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>>> index 3d09326..6db325f 100644
>>>> --- a/lib/dpif-netdev.c
>>>> +++ b/lib/dpif-netdev.c
>>>> @@ -513,6 +513,7 @@ dpif_netdev_open(const struct dpif_class *class,
>>>>const char *name,
>>>>  static void
>>>>  dp_netdev_free(struct dp_netdev *dp)
>>>>      OVS_REQUIRES(dp_netdev_mutex)
>>>> +    OVS_NO_THREAD_SAFETY_ANALYSIS
>>>I am not sure about these two annotations, how does it work in this
>>>case?
>>
>> If I understand your concern properly, the two annotations work
>> independently, i.e. (at least on my system, with clang 3.4) the compiler
>> still complains if I try to call dp_netdev_free() without holding the
>> Œdp_netdev_mutex¹, but it doesn¹t try to analyze what¹s inside the
>> function.
>>
>> I do not particularly like having to add OVS_NO_THREAD_SAFETY_ANALYSIS
>> annotations, but, given the way we use Œupcall_rwlock¹, I do not always
>> find a way to avoid it.
>>
>ok.
>
>> Would you prefer having a separate Œupcall_rwlock_destroy()¹ function
>> (marked with OVS_NO_THREAD_SAFETY_ANALYSIS), perhaps?
>>
>This sounds better.

Thanks for the feedback,

I sent a v2