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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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@netfilter.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.