From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock Date: Wed, 8 Aug 2018 10:54:08 +0200 Message-ID: <20180808085408.GD1998@nanopsycho> References: <1533538465-23199-1-git-send-email-vladbu@mellanox.com> <1533538465-23199-13-git-send-email-vladbu@mellanox.com> <20180807163630.GD2890@nanopsycho> <20180808080308.GA1998@nanopsycho> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, davem@davemloft.net, jhs@mojatatu.com, xiyou.wangcong@gmail.com, pablo@netfilter.org, kadlec@blackhole.kfki.hu, fw@strlen.de, ast@kernel.org, daniel@iogearbox.net, edumazet@google.com, keescook@chromium.org, marcelo.leitner@gmail.com To: Vlad Buslov Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:32951 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727083AbeHHLPv (ORCPT ); Wed, 8 Aug 2018 07:15:51 -0400 Received: by mail-wm0-f68.google.com with SMTP id r24-v6so27466wmh.0 for ; Wed, 08 Aug 2018 01:57:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Wed, Aug 08, 2018 at 10:47:04AM CEST, vladbu@mellanox.com wrote: > >On Wed 08 Aug 2018 at 08:03, Jiri Pirko wrote: >> Wed, Aug 08, 2018 at 09:40:35AM CEST, vladbu@mellanox.com wrote: >>> >>>On Tue 07 Aug 2018 at 16:36, Jiri Pirko wrote: >>>> Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote: >>>> >>>> [...] >>>> >>>>>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h >>>>>index 46b8c7f1c8d5..e6e475d788c6 100644 >>>>>--- a/include/net/tc_act/tc_tunnel_key.h >>>>>+++ b/include/net/tc_act/tc_tunnel_key.h >>>>>@@ -30,26 +30,47 @@ struct tcf_tunnel_key { >>>>> >>>>> static inline bool is_tcf_tunnel_set(const struct tc_action *a) >>>>> { >>>>>+ bool ret = false; >>>>> #ifdef CONFIG_NET_CLS_ACT >>>>> struct tcf_tunnel_key *t = to_tunnel_key(a); >>>>>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params); >>>>>+ struct tcf_tunnel_key_params *params; >>>>> >>>>>+ rcu_read_lock(); >>>>>+ params = rcu_dereference(t->params); >>>>> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY) >>>>>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET; >>>>>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET; >>>>>+ rcu_read_unlock(); >>>>> #endif >>>>>- return false; >>>>>+ return ret; >>>>> } >>>>> >>>>> static inline bool is_tcf_tunnel_release(const struct tc_action *a) >>>> >>>> Why are these tunnel things in a mirred patch? >>> >>>Mistake during re-slit. Will move those to tunnel_key patch. >> >> Are you sure that the changes are safe? I just quickly looked over it >> and it smells: >> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: >> if (is_tcf_tunnel_set(a)) { >> info = tcf_tunnel_info(a); >> >> Why the "t->params" can't be nulled in the middle? > >First of all, no API is actually "unlocked" with this patch. It is a >preparation, rtnl mutex is still in use. > >Callers of these functions will have to be updated, for example, to use >their _rcu version while holding rcu_read_lock. I don't see any rcu version of these. I think that it would be good to convert the callers to rcu and you can avoid these changes. > > > >