Re: [PATCH net-next v6 01/11] net: sched: use rcu for action cookie update

[Cong Wang <xiyou.wangcong@gmail.com>]

On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> Implement functions to atomically update and free action cookie
> using rcu mechanism.

Without stating any reason..... Is this even a changelog?

>
> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Dear Marcelo, how did it pass your review? See below:


> +static void tcf_set_action_cookie(struct tc_cookie __rcu **old_cookie,
> +                                 struct tc_cookie *new_cookie)
> +{
> +       struct tc_cookie *old;
> +
> +       old = xchg(old_cookie, new_cookie);


This is an incorrect use of RCU, obviously should be rcu_assign_pointer()
here.


> @@ -65,10 +83,7 @@ static void free_tcf(struct tc_action *p)
>         free_percpu(p->cpu_bstats);
>         free_percpu(p->cpu_qstats);
>
> -       if (p->act_cookie) {
> -               kfree(p->act_cookie->data);
> -               kfree(p->act_cookie);
> -       }
> +       tcf_set_action_cookie(&p->act_cookie, NULL);

So, this is called in free_tcf(), where the action is already
invisible from readers so it is ready to be freed.

The question is:

If the action itself is already ready to be freed, why do you
need RCU here? What could still read 'act->act_cookie'
while 'act' is already invisible?

Its last refcnt is already gone, the fast path RCU readers
are gone too given filters use rcu work already.

Standalone action dump? Again, the last refcnt is already
gone.

Marcelo, Vlad, Jiri, please explain.

Thanks!