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

[Vlad Buslov <vladbu@mellanox.com>]

On Fri 13 Jul 2018 at 03:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 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?

Yes, it is.

>
>>
>> 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.

Could you please explain your concern in more details? Similar pattern
is already widely used in kernel for re-assigning rcu pointers. For
example, Eric Dumazet uses it in 1c0d32fde5bd ("net_sched:
gen_estimator: complete rewrite of rate estimators"):

void gen_kill_estimator(struct net_rate_estimator __rcu **rate_est)
{
	struct net_rate_estimator *est;

	est = xchg((__force struct net_rate_estimator **)rate_est, NULL);
	if (est) {
		del_timer_sync(&est->timer);
		kfree_rcu(est, rcu);
	}
}

Tom Herbert uses same idiom in a8c5f90fb59a ("ip_tunnel: Ops
registration for secondary encap (fou, gue)"):

int ip_tunnel_encap_add_ops(const struct ip_tunnel_encap_ops *ops,
			    unsigned int num)
{
	if (num >= MAX_IPTUN_ENCAP_OPS)
		return -ERANGE;

	return !cmpxchg((const struct ip_tunnel_encap_ops **)
			&iptun_encaps[num],
			NULL, ops) ? 0 : -1;
}

Again, Eric uses xchg to re-assign rcu pointer in 45f6fad84cc3 ("ipv6:
add complete rcu protection around np->opt"):

struct ipv6_txoptions *ipv6_update_options(struct sock *sk,
					   struct ipv6_txoptions *opt)
{
	if (inet_sk(sk)->is_icsk) {
		if (opt &&
		    !((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
		    inet_sk(sk)->inet_daddr != LOOPBACK4_IPV6) {
			struct inet_connection_sock *icsk = inet_csk(sk);
			icsk->icsk_ext_hdr_len = opt->opt_flen + opt->opt_nflen;
			icsk->icsk_sync_mss(sk, icsk->icsk_pmtu_cookie);
		}
	}
	opt = xchg((__force struct ipv6_txoptions **)&inet6_sk(sk)->opt,
		   opt);
	sk_dst_reset(sk);

	return opt;
}

>
>
>> @@ -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.

It is not necessary here, I just used tcf_set_action_cookie() that
already implements cookie pointer cleanup to prevent code duplication.
I'm open to changing it, if you concerned with performance impact of
using atomic operation for re-assigning cookie pointer.

>
> Marcelo, Vlad, Jiri, please explain.
>
> Thanks!

Thank you for reviewing my code!