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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM0EoM=7EgtOWtBFM7JDfr5E8FukoNRjEVFfJawkdfhSG63h7w@mail.gmail.com>
Date:   Sun, 12 Feb 2023 10:12:31 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Oz Shlomo <ozsh@...dia.com>
Cc:     netdev@...r.kernel.org, Saeed Mahameed <saeedm@...dia.com>,
        Roi Dayan <roid@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Marcelo Ricardo Leitner <mleitner@...hat.com>,
        Simon Horman <simon.horman@...igine.com>,
        Baowen Zheng <baowen.zheng@...igine.com>,
        Edward Cree <ecree.xilinx@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net-next v4 5/9] net/sched: support per action hw stats

On Sun, Feb 12, 2023 at 8:26 AM Oz Shlomo <ozsh@...dia.com> wrote:
>
> There are currently two mechanisms for populating hardware stats:
> 1. Using flow_offload api to query the flow's statistics.
>    The api assumes that the same stats values apply to all
>    the flow's actions.
>    This assumption breaks when action drops or jumps over following
>    actions.
> 2. Using hw_action api to query specific action stats via a driver
>    callback method. This api assures the correct action stats for
>    the offloaded action, however, it does not apply to the rest of the
>    actions in the flow's actions array.
>
> Extend the flow_offload stats callback to indicate that a per action
> stats update is required.
> Use the existing flow_offload_action api to query the action's hw stats.
> In addition, currently the tc action stats utility only updates hw actions.
> Reuse the existing action stats cb infrastructure to query any action
> stats.
>
> Signed-off-by: Oz Shlomo <ozsh@...dia.com>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>
> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>

I just looked for functional equivalency and it LGTM.

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

cheers,
jamal

> ---
> Change log:
>
> V1 -> V2
>     - Fix static function without inline keyword in header file
>     - Rearrange flow_cls_offload members such that stats and use_act stats
>       will be on the same cache line
>     - Fall-through to flow stats when hw_stats update returns an error
>       (this aligns with current behavior).
> ---
>  include/net/flow_offload.h |  1 +
>  include/net/pkt_cls.h      | 29 +++++++++++++++++++----------
>  net/sched/act_api.c        |  8 --------
>  net/sched/cls_flower.c     |  2 +-
>  net/sched/cls_matchall.c   |  2 +-
>  5 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index d177bf5f0e1a..8c05455b1e34 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -594,6 +594,7 @@ struct flow_cls_common_offload {
>  struct flow_cls_offload {
>         struct flow_cls_common_offload common;
>         enum flow_cls_command command;
> +       bool use_act_stats;
>         unsigned long cookie;
>         struct flow_rule *rule;
>         struct flow_stats stats;
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index bf50829d9255..ace437c6754b 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -292,9 +292,15 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>  #define tcf_act_for_each_action(i, a, actions) \
>         for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
>
> +static inline bool tc_act_in_hw(struct tc_action *act)
> +{
> +       return !!act->in_hw_count;
> +}
> +
>  static inline void
>  tcf_exts_hw_stats_update(const struct tcf_exts *exts,
> -                        struct flow_stats *stats)
> +                        struct flow_stats *stats,
> +                        bool use_act_stats)
>  {
>  #ifdef CONFIG_NET_CLS_ACT
>         int i;
> @@ -302,16 +308,18 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>         for (i = 0; i < exts->nr_actions; i++) {
>                 struct tc_action *a = exts->actions[i];
>
> -               /* if stats from hw, just skip */
> -               if (tcf_action_update_hw_stats(a)) {
> -                       preempt_disable();
> -                       tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
> -                                               stats->lastused, true);
> -                       preempt_enable();
> -
> -                       a->used_hw_stats = stats->used_hw_stats;
> -                       a->used_hw_stats_valid = stats->used_hw_stats_valid;
> +               if (use_act_stats || tc_act_in_hw(a)) {
> +                       if (!tcf_action_update_hw_stats(a))
> +                               continue;
>                 }
> +
> +               preempt_disable();
> +               tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
> +                                       stats->lastused, true);
> +               preempt_enable();
> +
> +               a->used_hw_stats = stats->used_hw_stats;
> +               a->used_hw_stats_valid = stats->used_hw_stats_valid;
>         }
>  #endif
>  }
> @@ -769,6 +777,7 @@ struct tc_cls_matchall_offload {
>         enum tc_matchall_command command;
>         struct flow_rule *rule;
>         struct flow_stats stats;
> +       bool use_act_stats;
>         unsigned long cookie;
>  };
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 917827199102..eda58b78da13 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -169,11 +169,6 @@ static bool tc_act_skip_sw(u32 flags)
>         return (flags & TCA_ACT_FLAGS_SKIP_SW) ? true : false;
>  }
>
> -static bool tc_act_in_hw(struct tc_action *act)
> -{
> -       return !!act->in_hw_count;
> -}
> -
>  /* SKIP_HW and SKIP_SW are mutually exclusive flags. */
>  static bool tc_act_flags_valid(u32 flags)
>  {
> @@ -308,9 +303,6 @@ int tcf_action_update_hw_stats(struct tc_action *action)
>         struct flow_offload_action fl_act = {};
>         int err;
>
> -       if (!tc_act_in_hw(action))
> -               return -EOPNOTSUPP;
> -
>         err = offload_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
>         if (err)
>                 return err;
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index cb04739a13ce..885c95191ccf 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -502,7 +502,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
>         tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
>                          rtnl_held);
>
> -       tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
> +       tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats, cls_flower.use_act_stats);
>  }
>
>  static void __fl_put(struct cls_fl_filter *f)
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index b3883d3d4dbd..fa3bbd187eb9 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -331,7 +331,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
>
>         tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
>
> -       tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
> +       tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, cls_mall.use_act_stats);
>  }
>
>  static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
> --
> 1.8.3.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ