From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [PATCH net-next v6 01/11] net: sched: use rcu for action cookie update Date: Fri, 13 Jul 2018 16:30:25 +0300 Message-ID: References: <1530800673-12280-1-git-send-email-vladbu@mellanox.com> <1530800673-12280-2-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Linux Kernel Network Developers , David Miller , Jamal Hadi Salim , Jiri Pirko , Alexei Starovoitov , Daniel Borkmann , Yevgeny Kliteynik , Jiri Pirko To: Cong Wang Return-path: Received: from mail-eopbgr70084.outbound.protection.outlook.com ([40.107.7.84]:15056 "EHLO EUR04-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1729627AbeGMNpX (ORCPT ); Fri, 13 Jul 2018 09:45:23 -0400 In-reply-to: Sender: netdev-owner@vger.kernel.org List-ID: On Fri 13 Jul 2018 at 03:52, Cong Wang wrote: > On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov 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 > > 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!