netdev - Re: [PATCH net-next] net_sched: act: remove tcfa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAM0EoM=KPSO6aA+iSZ+LdgAg34sEs9yPnJtKLKVT7jji5EQcsA@mail.gmail.com>
Date: Tue, 2 Sep 2025 07:57:32 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org, 
	eric.dumazet@...il.com
Subject: Re: [PATCH net-next] net_sched: act: remove tcfa_qstats

On Mon, Sep 1, 2025 at 5:31 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> tcfa_qstats is currently only used to hold drops and overlimits counters.
>
> tcf_action_inc_drop_qstats() and tcf_action_inc_overlimit_qstats()
> currently acquire a->tcfa_lock to increment these counters.
>
> Switch to two atomic_t to get lock-free accounting.
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>

Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal
> ---
>  include/net/act_api.h | 14 ++++++--------
>  net/sched/act_api.c   | 12 ++++++++----
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 2894cfff2da3..91a24b5e0b93 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -33,7 +33,10 @@ struct tc_action {
>         struct tcf_t                    tcfa_tm;
>         struct gnet_stats_basic_sync    tcfa_bstats;
>         struct gnet_stats_basic_sync    tcfa_bstats_hw;
> -       struct gnet_stats_queue         tcfa_qstats;
> +
> +       atomic_t                        tcfa_drops;
> +       atomic_t                        tcfa_overlimits;
> +
>         struct net_rate_estimator __rcu *tcfa_rate_est;
>         spinlock_t                      tcfa_lock;
>         struct gnet_stats_basic_sync __percpu *cpu_bstats;
> @@ -53,7 +56,6 @@ struct tc_action {
>  #define tcf_action     common.tcfa_action
>  #define tcf_tm         common.tcfa_tm
>  #define tcf_bstats     common.tcfa_bstats
> -#define tcf_qstats     common.tcfa_qstats
>  #define tcf_rate_est   common.tcfa_rate_est
>  #define tcf_lock       common.tcfa_lock
>
> @@ -241,9 +243,7 @@ static inline void tcf_action_inc_drop_qstats(struct tc_action *a)
>                 qstats_drop_inc(this_cpu_ptr(a->cpu_qstats));
>                 return;
>         }
> -       spin_lock(&a->tcfa_lock);
> -       qstats_drop_inc(&a->tcfa_qstats);
> -       spin_unlock(&a->tcfa_lock);
> +       atomic_inc(&a->tcfa_drops);
>  }
>
>  static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
> @@ -252,9 +252,7 @@ static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
>                 qstats_overlimit_inc(this_cpu_ptr(a->cpu_qstats));
>                 return;
>         }
> -       spin_lock(&a->tcfa_lock);
> -       qstats_overlimit_inc(&a->tcfa_qstats);
> -       spin_unlock(&a->tcfa_lock);
> +       atomic_inc(&a->tcfa_overlimits);
>  }
>
>  void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 9e468e463467..ff6be5cfe2b0 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1585,7 +1585,7 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
>         }
>
>         _bstats_update(&a->tcfa_bstats, bytes, packets);
> -       a->tcfa_qstats.drops += drops;
> +       atomic_add(drops, &a->tcfa_drops);
>         if (hw)
>                 _bstats_update(&a->tcfa_bstats_hw, bytes, packets);
>  }
> @@ -1594,8 +1594,9 @@ EXPORT_SYMBOL(tcf_action_update_stats);
>  int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
>                           int compat_mode)
>  {
> -       int err = 0;
> +       struct gnet_stats_queue qstats = {0};
>         struct gnet_dump d;
> +       int err = 0;
>
>         if (p == NULL)
>                 goto errout;
> @@ -1619,14 +1620,17 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
>         if (err < 0)
>                 goto errout;
>
> +       qstats.drops = atomic_read(&p->tcfa_drops);
> +       qstats.overlimits = atomic_read(&p->tcfa_overlimits);
> +
>         if (gnet_stats_copy_basic(&d, p->cpu_bstats,
>                                   &p->tcfa_bstats, false) < 0 ||
>             gnet_stats_copy_basic_hw(&d, p->cpu_bstats_hw,
>                                      &p->tcfa_bstats_hw, false) < 0 ||
>             gnet_stats_copy_rate_est(&d, &p->tcfa_rate_est) < 0 ||
>             gnet_stats_copy_queue(&d, p->cpu_qstats,
> -                                 &p->tcfa_qstats,
> -                                 p->tcfa_qstats.qlen) < 0)
> +                                 &qstats,
> +                                 qstats.qlen) < 0)
>                 goto errout;
>
>         if (gnet_stats_finish_copy(&d) < 0)
> --
> 2.51.0.318.gd7df087d1a-goog
>