Re: [net-next PATCH v2 02/15] net: rcu-ify tcf_proto

[John Fastabend <john.fastabend@gmail.com>]

On 09/02/2014 01:52 PM, David Miller wrote:
> From: John Fastabend <john.fastabend@gmail.com>
> Date: Mon, 01 Sep 2014 18:39:13 -0700
>
>> On 08/24/2014 10:31 PM, David Miller wrote:
>>> From: John Fastabend <john.fastabend@gmail.com>
>>> Date: Sun, 24 Aug 2014 17:48:31 -0700
>>>
>>>> @@ -722,8 +724,9 @@ static void sfq_free(void *addr)
>>>>    static void sfq_destroy(struct Qdisc *sch)
>>>>    {
>>>>    	struct sfq_sched_data *q = qdisc_priv(sch);
>>>> +	struct tcf_proto *fl = rtnl_dereference(q->filter_list);
>>>>
>>>> -	tcf_destroy_chain(&q->filter_list);
>>>> +	tcf_destroy_chain(&fl);
>>
>> Sorry for the delayed reply...
>>
>>>
>>> This will cause tcf_destroy_chain() to set the local variable
>>> 'fl' to NULL rather than q->filter_list.
>>>
>>> I don't see how this can be correct at all.
>>
>> Right now (without these patches) nothing sets q->filter_list
>> to NULL and we only call this when the qdisc is being destroyed.
>
> Yes, it does set it to NULL, by virtue of how the loop iterates.
>

yes of course its obvious now that you spell it out for me.

> It iterates by setting the "*fl" to tp->next until that evaluates to
> NULL.  Thereby setting *fl to NULL.
>
> So the old code would set ->filter_list to NULL, your code will not.
>

right I will fix this in the morning along with the sparse
warnings related to the tcf_find signatures.

Thanks
John


-- 
John Fastabend         Intel Corporation