Re: [PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
On Mon, Nov 19, 2018 at 04:04:16PM +0100, Jiri Pirko wrote: > Mon, Nov 19, 2018 at 03:48:50PM CET, pa...@netfilter.org wrote: > >On Mon, Nov 19, 2018 at 02:57:05PM +0100, Jiri Pirko wrote: > >> Mon, Nov 19, 2018 at 01:15:12AM CET, pa...@netfilter.org wrote: > >[...] > >> >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > >> >index 7d7aefa5fcd2..7f9a8d5ca945 100644 > >> >--- a/include/net/pkt_cls.h > >> >+++ b/include/net/pkt_cls.h > >> >@@ -758,6 +758,12 @@ enum tc_fl_command { > >> > TC_CLSFLOWER_TMPLT_DESTROY, > >> > }; > >> > > >> >+struct tc_cls_flower_stats { > >> >+ u64 pkts; > >> >+ u64 bytes; > >> >+ u64 lastused; > >> >+}; > >> >+ > >> > struct tc_cls_flower_offload { > >> > struct tc_cls_common_offload common; > >> > enum tc_fl_command command; > >> >@@ -765,6 +771,7 @@ struct tc_cls_flower_offload { > >> > struct flow_rule rule; > >> > struct tcf_exts *exts; > >> > u32 classid; > >> >+ struct tc_cls_flower_stats stats; > >> > }; > >> > > >> > static inline struct flow_rule * > >> >@@ -773,6 +780,14 @@ tc_cls_flower_offload_flow_rule(struct > >> >tc_cls_flower_offload *tc_flow_cmd) > >> > return &tc_flow_cmd->rule; > >> > } > >> > > >> >+static inline void tc_cls_flower_stats_update(struct > >> >tc_cls_flower_offload *cls_flower, > >> >+ u64 pkts, u64 bytes, u64 lastused) > >> >+{ > >> >+ cls_flower->stats.pkts = pkts; > >> >+ cls_flower->stats.bytes = bytes; > >> >+ cls_flower->stats.lastused = lastused; > >> > >> Why do you need to store the values here in struct tc_cls_flower_offload? > >> Why don't you just call tcf_exts_stats_update()? Basically, > >> tc_cls_flower_stats_update() should be just wrapper around > >> tcf_exts_stats_update() so that drivers wouldn't use ->exts directly, as > >> you will remove them in follow-up patches, no? > > > >Patch 07/12 stops exposing tc action exts to drivers, so we need a > >structure (struct tc_cls_flower_stats) to convey this statistics back > >to the cls_flower frontend. > > Hmm, shouldn't these stats be rather flow_rule related than flower > related? I can rename tc_cls_flower_stats to struct flow_stats and place this in include/net/flow.h, given flow_rule is unset / not present from the TC_CLSFLOWER_STATS path. Thanks for reviewing!
Re: [PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
Mon, Nov 19, 2018 at 03:48:50PM CET, pa...@netfilter.org wrote: >On Mon, Nov 19, 2018 at 02:57:05PM +0100, Jiri Pirko wrote: >> Mon, Nov 19, 2018 at 01:15:12AM CET, pa...@netfilter.org wrote: >[...] >> >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >> >index 7d7aefa5fcd2..7f9a8d5ca945 100644 >> >--- a/include/net/pkt_cls.h >> >+++ b/include/net/pkt_cls.h >> >@@ -758,6 +758,12 @@ enum tc_fl_command { >> >TC_CLSFLOWER_TMPLT_DESTROY, >> > }; >> > >> >+struct tc_cls_flower_stats { >> >+ u64 pkts; >> >+ u64 bytes; >> >+ u64 lastused; >> >+}; >> >+ >> > struct tc_cls_flower_offload { >> >struct tc_cls_common_offload common; >> >enum tc_fl_command command; >> >@@ -765,6 +771,7 @@ struct tc_cls_flower_offload { >> >struct flow_rule rule; >> >struct tcf_exts *exts; >> >u32 classid; >> >+ struct tc_cls_flower_stats stats; >> > }; >> > >> > static inline struct flow_rule * >> >@@ -773,6 +780,14 @@ tc_cls_flower_offload_flow_rule(struct >> >tc_cls_flower_offload *tc_flow_cmd) >> >return &tc_flow_cmd->rule; >> > } >> > >> >+static inline void tc_cls_flower_stats_update(struct tc_cls_flower_offload >> >*cls_flower, >> >+ u64 pkts, u64 bytes, u64 lastused) >> >+{ >> >+ cls_flower->stats.pkts = pkts; >> >+ cls_flower->stats.bytes = bytes; >> >+ cls_flower->stats.lastused = lastused; >> >> Why do you need to store the values here in struct tc_cls_flower_offload? >> Why don't you just call tcf_exts_stats_update()? Basically, >> tc_cls_flower_stats_update() should be just wrapper around >> tcf_exts_stats_update() so that drivers wouldn't use ->exts directly, as >> you will remove them in follow-up patches, no? > >Patch 07/12 stops exposing tc action exts to drivers, so we need a >structure (struct tc_cls_flower_stats) to convey this statistics back >to the cls_flower frontend. Hmm, shouldn't these stats be rather flow_rule related than flower related?
Re: [PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
On Mon, Nov 19, 2018 at 02:57:05PM +0100, Jiri Pirko wrote: > Mon, Nov 19, 2018 at 01:15:12AM CET, pa...@netfilter.org wrote: [...] > >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > >index 7d7aefa5fcd2..7f9a8d5ca945 100644 > >--- a/include/net/pkt_cls.h > >+++ b/include/net/pkt_cls.h > >@@ -758,6 +758,12 @@ enum tc_fl_command { > > TC_CLSFLOWER_TMPLT_DESTROY, > > }; > > > >+struct tc_cls_flower_stats { > >+u64 pkts; > >+u64 bytes; > >+u64 lastused; > >+}; > >+ > > struct tc_cls_flower_offload { > > struct tc_cls_common_offload common; > > enum tc_fl_command command; > >@@ -765,6 +771,7 @@ struct tc_cls_flower_offload { > > struct flow_rule rule; > > struct tcf_exts *exts; > > u32 classid; > >+struct tc_cls_flower_stats stats; > > }; > > > > static inline struct flow_rule * > >@@ -773,6 +780,14 @@ tc_cls_flower_offload_flow_rule(struct > >tc_cls_flower_offload *tc_flow_cmd) > > return &tc_flow_cmd->rule; > > } > > > >+static inline void tc_cls_flower_stats_update(struct tc_cls_flower_offload > >*cls_flower, > >+ u64 pkts, u64 bytes, u64 lastused) > >+{ > >+cls_flower->stats.pkts = pkts; > >+cls_flower->stats.bytes = bytes; > >+cls_flower->stats.lastused = lastused; > > Why do you need to store the values here in struct tc_cls_flower_offload? > Why don't you just call tcf_exts_stats_update()? Basically, > tc_cls_flower_stats_update() should be just wrapper around > tcf_exts_stats_update() so that drivers wouldn't use ->exts directly, as > you will remove them in follow-up patches, no? Patch 07/12 stops exposing tc action exts to drivers, so we need a structure (struct tc_cls_flower_stats) to convey this statistics back to the cls_flower frontend.
Re: [PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
Mon, Nov 19, 2018 at 01:15:12AM CET, pa...@netfilter.org wrote: >This patch provides a tc_cls_flower_stats structure that acts as >container for tc_cls_flower_offload, then we can use to restore the >statistics on the existing TC actions. Hence, tcf_exts_stats_update() is >not used from drivers. > >Signed-off-by: Pablo Neira Ayuso >--- >v2: no changes. > > drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 4 ++-- > drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 6 +++--- > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +- > drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +- > drivers/net/ethernet/netronome/nfp/flower/offload.c | 6 +++--- > include/net/pkt_cls.h | 15 +++ > net/sched/cls_flower.c| 4 > 7 files changed, 29 insertions(+), 10 deletions(-) > >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c >b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c >index b82143d6cdde..3d71b2530d67 100644 >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c >@@ -1366,8 +1366,8 @@ static int bnxt_tc_get_flow_stats(struct bnxt *bp, > lastused = flow->lastused; > spin_unlock(&flow->stats_lock); > >- tcf_exts_stats_update(tc_flow_cmd->exts, stats.bytes, stats.packets, >-lastused); >+ tc_cls_flower_stats_update(tc_flow_cmd, stats.bytes, stats.packets, >+ lastused); > return 0; > } > >diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c >b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c >index 39c5af5dad3d..2c7d1aebe214 100644 >--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c >+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c >@@ -807,9 +807,9 @@ int cxgb4_tc_flower_stats(struct net_device *dev, > if (ofld_stats->packet_count != packets) { > if (ofld_stats->prev_packet_count != packets) > ofld_stats->last_used = jiffies; >- tcf_exts_stats_update(cls->exts, bytes - ofld_stats->byte_count, >-packets - ofld_stats->packet_count, >-ofld_stats->last_used); >+ tc_cls_flower_stats_update(cls, bytes - ofld_stats->byte_count, >+ packets - ofld_stats->packet_count, >+ ofld_stats->last_used); > > ofld_stats->packet_count = packets; > ofld_stats->byte_count = bytes; >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >index 2645e5d1e790..c5f0b826fa91 100644 >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >@@ -3224,7 +3224,7 @@ int mlx5e_stats_flower(struct mlx5e_priv *priv, > > mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); > >- tcf_exts_stats_update(f->exts, bytes, packets, lastuse); >+ tc_cls_flower_stats_update(f, bytes, packets, lastuse); > > return 0; > } >diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >index 193a6f9acf79..3398984ffb2a 100644 >--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >@@ -460,7 +460,7 @@ int mlxsw_sp_flower_stats(struct mlxsw_sp *mlxsw_sp, > if (err) > goto err_rule_get_stats; > >- tcf_exts_stats_update(f->exts, bytes, packets, lastuse); >+ tc_cls_flower_stats_update(f, bytes, packets, lastuse); > > mlxsw_sp_acl_ruleset_put(mlxsw_sp, ruleset); > return 0; >diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c >b/drivers/net/ethernet/netronome/nfp/flower/offload.c >index 708331234908..bec74d84756c 100644 >--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c >+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c >@@ -532,9 +532,9 @@ nfp_flower_get_stats(struct nfp_app *app, struct >net_device *netdev, > ctx_id = be32_to_cpu(nfp_flow->meta.host_ctx_id); > > spin_lock_bh(&priv->stats_lock); >- tcf_exts_stats_update(flow->exts, priv->stats[ctx_id].bytes, >-priv->stats[ctx_id].pkts, >-priv->stats[ctx_id].used); >+ tc_cls_flower_stats_update(flow, priv->stats[ctx_id].bytes, >+ priv->stats[ctx_id].pkts, >+ priv->stats[ctx_id].used); > > priv->stats[ctx_id].pkts = 0; > priv->stats[ctx_id].bytes = 0; >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >index 7d7aefa5fcd2..7f9a8d5ca945 100644 >--- a/include/net/pkt_cls.h >+++ b/include/net/pkt_cls.h >@@ -758,6 +758,12 @@ enum tc_fl_command { > TC_CLSFLOWER_TMPLT_DESTROY, > }; > >+struc
[PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
This patch provides a tc_cls_flower_stats structure that acts as container for tc_cls_flower_offload, then we can use to restore the statistics on the existing TC actions. Hence, tcf_exts_stats_update() is not used from drivers. Signed-off-by: Pablo Neira Ayuso --- v2: no changes. drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 4 ++-- drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 6 +++--- drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +- drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +- drivers/net/ethernet/netronome/nfp/flower/offload.c | 6 +++--- include/net/pkt_cls.h | 15 +++ net/sched/cls_flower.c| 4 7 files changed, 29 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c index b82143d6cdde..3d71b2530d67 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c @@ -1366,8 +1366,8 @@ static int bnxt_tc_get_flow_stats(struct bnxt *bp, lastused = flow->lastused; spin_unlock(&flow->stats_lock); - tcf_exts_stats_update(tc_flow_cmd->exts, stats.bytes, stats.packets, - lastused); + tc_cls_flower_stats_update(tc_flow_cmd, stats.bytes, stats.packets, + lastused); return 0; } diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c index 39c5af5dad3d..2c7d1aebe214 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c @@ -807,9 +807,9 @@ int cxgb4_tc_flower_stats(struct net_device *dev, if (ofld_stats->packet_count != packets) { if (ofld_stats->prev_packet_count != packets) ofld_stats->last_used = jiffies; - tcf_exts_stats_update(cls->exts, bytes - ofld_stats->byte_count, - packets - ofld_stats->packet_count, - ofld_stats->last_used); + tc_cls_flower_stats_update(cls, bytes - ofld_stats->byte_count, + packets - ofld_stats->packet_count, + ofld_stats->last_used); ofld_stats->packet_count = packets; ofld_stats->byte_count = bytes; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c index 2645e5d1e790..c5f0b826fa91 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c @@ -3224,7 +3224,7 @@ int mlx5e_stats_flower(struct mlx5e_priv *priv, mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); - tcf_exts_stats_update(f->exts, bytes, packets, lastuse); + tc_cls_flower_stats_update(f, bytes, packets, lastuse); return 0; } diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c index 193a6f9acf79..3398984ffb2a 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c @@ -460,7 +460,7 @@ int mlxsw_sp_flower_stats(struct mlxsw_sp *mlxsw_sp, if (err) goto err_rule_get_stats; - tcf_exts_stats_update(f->exts, bytes, packets, lastuse); + tc_cls_flower_stats_update(f, bytes, packets, lastuse); mlxsw_sp_acl_ruleset_put(mlxsw_sp, ruleset); return 0; diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c index 708331234908..bec74d84756c 100644 --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c @@ -532,9 +532,9 @@ nfp_flower_get_stats(struct nfp_app *app, struct net_device *netdev, ctx_id = be32_to_cpu(nfp_flow->meta.host_ctx_id); spin_lock_bh(&priv->stats_lock); - tcf_exts_stats_update(flow->exts, priv->stats[ctx_id].bytes, - priv->stats[ctx_id].pkts, - priv->stats[ctx_id].used); + tc_cls_flower_stats_update(flow, priv->stats[ctx_id].bytes, + priv->stats[ctx_id].pkts, + priv->stats[ctx_id].used); priv->stats[ctx_id].pkts = 0; priv->stats[ctx_id].bytes = 0; diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 7d7aefa5fcd2..7f9a8d5ca945 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -758,6 +758,12 @@ enum tc_fl_command { TC_CLSFLOWER_TMPLT_DESTROY, }; +struct tc_cls_flower_stats { + u64 pkts; + u64 bytes; + u64 lastused; +}; + struct tc_cls_flower_offload {