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 _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 _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 _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(>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, , , );
>
>- 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(>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 {
>+
[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(>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, , , );
- 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(>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 {
struct
