RE: [PATCH net-next,v4 05/12] flow_offload: add statistics retrieval infrastructure and use it

[Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Pablo Neira Ayuso
> Sent: Thursday, November 29, 2018 7:52 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; thomas.lendacky@amd.com;
> f.fainelli@gmail.com; ariel.elior@cavium.com; michael.chan@broadcom.com;
> santosh@chelsio.com; madalin.bucur@nxp.com;
> yisen.zhuang@huawei.com; salil.mehta@huawei.com;
> jeffrey.t.kirsher@intel.com; tariqt@mellanox.com; saeedm@mellanox.com;
> jiri@mellanox.com; idosch@mellanox.com; jakub.kicinski@netronome.com;
> peppe.cavallaro@st.com; grygorii.strashko@ti.com; andrew@lunn.ch;
> vivien.didelot@savoirfairelinux.com; alexandre.torgue@st.com;
> joabreu@synopsys.com; linux-net-drivers@solarflare.com;
> ganeshgr@chelsio.com; ogerlitz@mellanox.com;
> Manish.Chopra@cavium.com; marcelo.leitner@gmail.com
> Subject: [PATCH net-next,v4 05/12] flow_offload: add statistics
retrieval
> infrastructure and use it
>
> This patch provides the flow_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 anymore.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v4: rebase.
>
>  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   |  5 ++---
>  include/net/flow_offload.h                            | 14
++++++++++++++
>  include/net/pkt_cls.h                                 |  1 +
>  net/sched/cls_flower.c                                |  4 ++++
>  8 files changed, 28 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..09cd75f54eba 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);
> +	flow_stats_update(&tc_flow_cmd->stats, 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..8a2d66ee1d7b 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);
> +		flow_stats_update(&cls->stats, 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 afabd5e530f0..6bc5c57bda9d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -3225,7 +3225,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);
> +	flow_stats_update(&f->stats, 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 e6c4c672b1ca..60900e53243b 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);
> +	flow_stats_update(&f->stats, 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..524b9ae1a639 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -532,9 +532,8 @@ 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);
> +	flow_stats_update(&flow->stats, 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/flow_offload.h b/include/net/flow_offload.h
> index dabc819b6cc9..040c092c000a 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -179,4 +179,18 @@ static inline bool flow_rule_match_key(const struct
> flow_rule *rule,
>  	return dissector_uses_key(rule->match.dissector, key);
>  }
>
> +struct flow_stats {
> +	u64	pkts;
> +	u64	bytes;
> +	u64	lastused;
> +};
> +
> +static inline void flow_stats_update(struct flow_stats *flow_stats,
> +				     u64 pkts, u64 bytes, u64 lastused)
> +{
> +	flow_stats->pkts	= pkts;
> +	flow_stats->bytes	= bytes;
> +	flow_stats->lastused	= lastused;
> +}
This API is taking packets as 2nd parameter and bytes as 3rd parameter.
However, all the callers are passing them in reverse.
> +
>  #endif /* _NET_FLOW_OFFLOAD_H */
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index abb035f84321..a08c06e383db 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -767,6 +767,7 @@ struct tc_cls_flower_offload {
>  	enum tc_fl_command command;
>  	unsigned long cookie;
>  	struct flow_rule *rule;
> +	struct flow_stats stats;
>  	struct tcf_exts *exts;
>  	u32 classid;
>  };
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 8898943b8ee6..b88cf29aff7b 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -432,6 +432,10 @@ static void fl_hw_update_stats(struct tcf_proto
*tp,
> struct cls_fl_filter *f)
>
>  	tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
>  			 &cls_flower, false);
> +
> +	tcf_exts_stats_update(&f->exts, cls_flower.stats.bytes,
> +			      cls_flower.stats.pkts,
> +			      cls_flower.stats.lastused);
>  }
>
>  static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
> --
> 2.11.0