On Fri 13 Jul 2018 at 03:52, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vla...@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.leit...@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!
