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

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]

Date:   Sat, 1 Dec 2018 10:55:15 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        thomas.lendacky@....com, f.fainelli@...il.com,
        ariel.elior@...ium.com, michael.chan@...adcom.com,
        santosh@...lsio.com, madalin.bucur@....com,
        yisen.zhuang@...wei.com, salil.mehta@...wei.com,
        jeffrey.t.kirsher@...el.com, tariqt@...lanox.com,
        saeedm@...lanox.com, jiri@...lanox.com, idosch@...lanox.com,
        peppe.cavallaro@...com, grygorii.strashko@...com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, alexandre.torgue@...com,
        joabreu@...opsys.com, linux-net-drivers@...arflare.com,
        ganeshgr@...lsio.com, ogerlitz@...lanox.com,
        Manish.Chopra@...ium.com, marcelo.leitner@...il.com
Subject: Re: [PATCH net-next,v4 05/12] flow_offload: add statistics retrieval
 infrastructure and use it

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 <pablo@...filter.org>
> 
> > 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.