From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pablo Neira Ayuso Subject: Re: [PATCH net-next,v4 05/12] flow_offload: add statistics retrieval infrastructure and use it Date: Sat, 1 Dec 2018 10:55:15 +0100 Message-ID: <20181201095515.j2gmk7ixqww7exi4@salvia> References: <20181129022231.2740-1-pablo@netfilter.org> <20181129022231.2740-6-pablo@netfilter.org> <20181129124822.178e17a6@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, 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, 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 To: Jakub Kicinski Return-path: Received: from mail.us.es ([193.147.175.20]:55360 "EHLO mail.us.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726342AbeLAVHj (ORCPT ); Sat, 1 Dec 2018 16:07:39 -0500 Received: from antivirus1-rhel7.int (unknown [192.168.2.11]) by mail.us.es (Postfix) with ESMTP id A96B1D28F3 for ; Sat, 1 Dec 2018 10:55:27 +0100 (CET) Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id 98BE0DA79D for ; Sat, 1 Dec 2018 10:55:27 +0100 (CET) Content-Disposition: inline In-Reply-To: <20181129124822.178e17a6@cakuba.netronome.com> Sender: netdev-owner@vger.kernel.org List-ID: Hi Jakub, On Thu, Nov 29, 2018 at 12:48:22PM -0800, Jakub Kicinski wrote: > On Thu, 29 Nov 2018 03:22:24 +0100, Pablo Neira Ayuso wrote: > > 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 > > > 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; > > +} > > + > > #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, > > Apart from the bug Venkat mentioned I think this patch exposes a > potentially strange and unexpected TC-ism through to the abstracted API. > The stats for TC store the increment in update cycle, so flow->stats > will be equal to the diff between TC_CLSFLOWER_STATS calls. Goal of this patch is to stop exposing tcf_exts to drivers, so it is a preparation patch to remove a dependency. This patch introduces infrastructure to wheelbarrow the counters back to TC, as a result we a have centralized tcf_exts_stats_update() call and TC actions are not exposed anymore. Ethtool does not seem to expose counters, so it does not need this infrastructure. So you are right, the statistics interface is very much TC specific at this stage.