[PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

[PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

[Eric Dumazet]

tc_action_net_exit() got an rtnl exclusion in commit
a159d3c4b829 ("net_sched: acquire RTNL in tc_action_net_exit()")

Since then, commit 16af6067392c ("net: sched: implement reference
counted action release") made this RTNL exclusion obsolete.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Vlad Buslov <vladbu@nvidia.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/act_api.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 404df8557f6a13420b18d9c52b9710fe86d084aa..04781c92b43d6ab9cc6c81a88d5c6fe8c282c590 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -170,14 +170,12 @@ static inline void tc_action_net_exit(struct list_head *net_list,
 {
 	struct net *net;
 
-	rtnl_lock();
 	list_for_each_entry(net, net_list, exit_list) {
 		struct tc_action_net *tn = net_generic(net, id);
 
 		tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
 		kfree(tn->idrinfo);
 	}
-	rtnl_unlock();
 }
 
 int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

[Kuniyuki Iwashima]

On Tue, Jul 1, 2025 at 6:30 AM Eric Dumazet <edumazet@google.com> wrote:
>
> tc_action_net_exit() got an rtnl exclusion in commit
> a159d3c4b829 ("net_sched: acquire RTNL in tc_action_net_exit()")
>
> Since then, commit 16af6067392c ("net: sched: implement reference
> counted action release") made this RTNL exclusion obsolete.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Thank you!

Re: [PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

[Cong Wang]

On Tue, Jul 01, 2025 at 01:30:06PM +0000, Eric Dumazet wrote:
> tc_action_net_exit() got an rtnl exclusion in commit
> a159d3c4b829 ("net_sched: acquire RTNL in tc_action_net_exit()")
> 
> Since then, commit 16af6067392c ("net: sched: implement reference
> counted action release") made this RTNL exclusion obsolete.

I am not sure removing RTNL is safe even we have action refcnt.

For example, are you sure tcf_action_offload_del() is safe to call
without RTNL?

What are you trying to improve here?

Thanks.

Re: [PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

[Eric Dumazet]

On Tue, Jul 1, 2025 at 7:35 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, Jul 01, 2025 at 01:30:06PM +0000, Eric Dumazet wrote:
> > tc_action_net_exit() got an rtnl exclusion in commit
> > a159d3c4b829 ("net_sched: acquire RTNL in tc_action_net_exit()")
> >
> > Since then, commit 16af6067392c ("net: sched: implement reference
> > counted action release") made this RTNL exclusion obsolete.
>
> I am not sure removing RTNL is safe even we have action refcnt.
>
> For example, are you sure tcf_action_offload_del() is safe to call
> without RTNL?

My thinking was that at the time of these calls, devices were already
gone from the dismantling netns, but this might be wrong.

We can conditionally acquire rtnl from tcf_idrinfo_destroy() when
there is at least one offloaded action in the idr.

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 057e20cef3754f33357c4c1e30034f6b9b872d91..9e468e46346710c85c3a85b905d27dfe3972916a
100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -933,18 +933,25 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
                         struct tcf_idrinfo *idrinfo)
 {
        struct idr *idr = &idrinfo->action_idr;
+       bool mutex_taken = false;
        struct tc_action *p;
-       int ret;
        unsigned long id = 1;
        unsigned long tmp;
+       int ret;

        idr_for_each_entry_ul(idr, p, tmp, id) {
+               if (tc_act_in_hw(p) && !mutex_taken) {
+                       rtnl_lock();
+                       mutex_taken = true;
+               }
                ret = __tcf_idr_release(p, false, true);
                if (ret == ACT_P_DELETED)
                        module_put(ops->owner);
                else if (ret < 0)
                        return;
        }
+       if (mutex_taken)
+               rtnl_unlock();
        idr_destroy(&idrinfo->action_idr);
 }
 EXPORT_SYMBOL(tcf_idrinfo_destroy);


>
> What are you trying to improve here?

Yeah, some of us are spending months of work to improve the RTNL
situation, and we do not copy/paste why on every single patch :)

I will capture the following in V2, thanks !

Most netns do not have actions, yet deleting them is adding a lot of
pressure on RTNL, which is for us the most contended mutex in the
kernel.

We are moving to a per-netns 'rtnl', so tc_action_net_exit() will not
be able to grab 'rtnl' a single time for a batch of netns.


Before the patch:

perf probe -a rtnl_lock  # Note: This does not capture all calls, some
of them might be inlined in net/core/rtnetlink.c

perf record -e probe:rtnl_lock -a /bin/bash -c 'unshare -n "/bin/true"; sleep 1'
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.305 MB perf.data (25 samples) ]

After the patch:

perf record -e probe:rtnl_lock -a /bin/bash -c 'unshare -n "/bin/true"; sleep 1'
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.304 MB perf.data (9 samples) ]

Re: [PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

[Kuniyuki Iwashima]

On Wed, Jul 2, 2025 at 12:08 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jul 1, 2025 at 7:35 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Jul 01, 2025 at 01:30:06PM +0000, Eric Dumazet wrote:
> > > tc_action_net_exit() got an rtnl exclusion in commit
> > > a159d3c4b829 ("net_sched: acquire RTNL in tc_action_net_exit()")
> > >
> > > Since then, commit 16af6067392c ("net: sched: implement reference
> > > counted action release") made this RTNL exclusion obsolete.
> >
> > I am not sure removing RTNL is safe even we have action refcnt.
> >
> > For example, are you sure tcf_action_offload_del() is safe to call
> > without RTNL?
>
> My thinking was that at the time of these calls, devices were already
> gone from the dismantling netns,

I thought the same because tcf_register_action() uses
register_pernet_subsys(), and subsys_ops->exit() is always executed
after other device_ops including default_device_exit_batch() that should
wait for all dev to go away.


> but this might be wrong.
>
> We can conditionally acquire rtnl from tcf_idrinfo_destroy() when
> there is at least one offloaded action in the idr.
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 057e20cef3754f33357c4c1e30034f6b9b872d91..9e468e46346710c85c3a85b905d27dfe3972916a
> 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -933,18 +933,25 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
>                          struct tcf_idrinfo *idrinfo)
>  {
>         struct idr *idr = &idrinfo->action_idr;
> +       bool mutex_taken = false;
>         struct tc_action *p;
> -       int ret;
>         unsigned long id = 1;
>         unsigned long tmp;
> +       int ret;
>
>         idr_for_each_entry_ul(idr, p, tmp, id) {
> +               if (tc_act_in_hw(p) && !mutex_taken) {
> +                       rtnl_lock();
> +                       mutex_taken = true;
> +               }
>                 ret = __tcf_idr_release(p, false, true);
>                 if (ret == ACT_P_DELETED)
>                         module_put(ops->owner);
>                 else if (ret < 0)
>                         return;
>         }
> +       if (mutex_taken)
> +               rtnl_unlock();
>         idr_destroy(&idrinfo->action_idr);
>  }
>  EXPORT_SYMBOL(tcf_idrinfo_destroy);
>
>
> >
> > What are you trying to improve here?
>
> Yeah, some of us are spending months of work to improve the RTNL
> situation, and we do not copy/paste why on every single patch :)
>
> I will capture the following in V2, thanks !
>
> Most netns do not have actions, yet deleting them is adding a lot of
> pressure on RTNL, which is for us the most contended mutex in the
> kernel.
>
> We are moving to a per-netns 'rtnl', so tc_action_net_exit() will not
> be able to grab 'rtnl' a single time for a batch of netns.
>
>
> Before the patch:
>
> perf probe -a rtnl_lock  # Note: This does not capture all calls, some
> of them might be inlined in net/core/rtnetlink.c
>
> perf record -e probe:rtnl_lock -a /bin/bash -c 'unshare -n "/bin/true"; sleep 1'
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.305 MB perf.data (25 samples) ]
>
> After the patch:
>
> perf record -e probe:rtnl_lock -a /bin/bash -c 'unshare -n "/bin/true"; sleep 1'
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.304 MB perf.data (9 samples) ]