Re: [PATCH nf-next 1/2] netfilter: flowtable: retrieve stats on flow deletion

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]

On Oct 29, Lorenzo Bianconi wrote:
> On Oct 29, Pablo Neira Ayuso wrote:
> > On Thu, Oct 29, 2026 at 12:25:21PM +0100, Lorenzo Bianconi wrote:
> > > > Retrieve stats too when deleting a flow from hardware. Apparently, the
> > > > existing approach is missing last stats sync with the conntrack entry.
> > > > Pass the stats object to the driver so it has a chance to populate it
> > > > at deletion time. Drivers need to be updated to use this new approach.
> > > > 
> > > > Reported-by: Valentin Berezhnoy <dreyksmail@gmail.com>
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > ---
> > > >  net/netfilter/nf_flow_table_offload.c | 43 ++++++++++++++++-----------
> > > >  1 file changed, 26 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> > > > index 4365859220e6..53182323f73c 100644
> > > > --- a/net/netfilter/nf_flow_table_offload.c
> > > > +++ b/net/netfilter/nf_flow_table_offload.c
> > > > @@ -934,6 +934,27 @@ static int nf_flow_offload_tuple(struct nf_flowtable *flowtable,
> > > >  	return i;
> > > >  }
> > > >  
> > > > +static void flow_offload_update_stats(struct flow_offload_work *offload,
> > > > +				      struct flow_stats *stats)
> > > > +{
> > > > +	u64 lastused;
> > > > +
> > > > +	lastused = max_t(u64, stats[0].lastused, stats[1].lastused);
> > > > +	offload->flow->timeout = max_t(u64, offload->flow->timeout,
> > > > +				       lastused + flow_offload_get_timeout(offload->flow));
> > > > +
> > > > +	if (offload->flowtable->flags & NF_FLOWTABLE_COUNTER) {
> > > > +		if (stats[0].pkts)
> > > > +			nf_ct_acct_add(offload->flow->ct,
> > > > +				       FLOW_OFFLOAD_DIR_ORIGINAL,
> > > > +				       stats[0].pkts, stats[0].bytes);
> > > > +		if (stats[1].pkts)
> > > > +			nf_ct_acct_add(offload->flow->ct,
> > > > +				       FLOW_OFFLOAD_DIR_REPLY,
> > > > +				       stats[1].pkts, stats[1].bytes);
> > > > +	}
> > > > +}
> > > > +
> > > >  static int flow_offload_tuple_add(struct flow_offload_work *offload,
> > > >  				  struct nf_flow_rule *flow_rule,
> > > >  				  enum flow_offload_tuple_dir dir)
> > > > @@ -948,10 +969,13 @@ static int flow_offload_tuple_add(struct flow_offload_work *offload,
> > > >  static void flow_offload_tuple_del(struct flow_offload_work *offload,
> > > >  				   enum flow_offload_tuple_dir dir)
> > > >  {
> > > > +	struct flow_stats stats[FLOW_OFFLOAD_DIR_MAX] = {};
> > > > +
> > > >  	nf_flow_offload_tuple(offload->flowtable, offload->flow, NULL, dir,
> > > >  			      offload->flowtable->priority,
> > > > -			      FLOW_CLS_DESTROY, NULL,
> > > > +			      FLOW_CLS_DESTROY, stats,
> > > >  			      &offload->flowtable->flow_block.cb_list);
> > > 
> > > according to my understanding nf_flow_offload_tuple() copies the stats just if
> > > cmd is set to FLOW_CLS_STATS, right? Should we just check if stats is not NULL
> > > in nf_flow_offload_tuple() instead?
> > 
> > I posted a v2 with:
> > 
> >         if (cmd == FLOW_CLS_STATS ||
> >             cmd == FLOW_CLS_DESTROY)
> > 
> > but I can post a v3 with.
> > 
> >         if (stats)
> > 
> > instead as you suggest.
> 
> up to you, same for me. I guess you can add even add the support for airoha.
> 
> Regards,
> Lorenzo

off-topic:

Can I post new revision for flowtable sit support for nf-next even if net-next is
closed at the moment?

Regards,
Lorenzo

> 
> > 
> > > Regards,
> > > Lorenzo
> > > 
> > > > +	flow_offload_update_stats(offload, stats);
> > > >  }
> > > >  
> > > >  static int flow_offload_rule_add(struct flow_offload_work *offload,
> > > > @@ -1011,27 +1035,12 @@ static void flow_offload_tuple_stats(struct flow_offload_work *offload,
> > > >  static void flow_offload_work_stats(struct flow_offload_work *offload)
> > > >  {
> > > >  	struct flow_stats stats[FLOW_OFFLOAD_DIR_MAX] = {};
> > > > -	u64 lastused;
> > > >  
> > > >  	flow_offload_tuple_stats(offload, FLOW_OFFLOAD_DIR_ORIGINAL, &stats[0]);
> > > >  	if (test_bit(NF_FLOW_HW_BIDIRECTIONAL, &offload->flow->flags))
> > > >  		flow_offload_tuple_stats(offload, FLOW_OFFLOAD_DIR_REPLY,
> > > >  					 &stats[1]);
> > > > -
> > > > -	lastused = max_t(u64, stats[0].lastused, stats[1].lastused);
> > > > -	offload->flow->timeout = max_t(u64, offload->flow->timeout,
> > > > -				       lastused + flow_offload_get_timeout(offload->flow));
> > > > -
> > > > -	if (offload->flowtable->flags & NF_FLOWTABLE_COUNTER) {
> > > > -		if (stats[0].pkts)
> > > > -			nf_ct_acct_add(offload->flow->ct,
> > > > -				       FLOW_OFFLOAD_DIR_ORIGINAL,
> > > > -				       stats[0].pkts, stats[0].bytes);
> > > > -		if (stats[1].pkts)
> > > > -			nf_ct_acct_add(offload->flow->ct,
> > > > -				       FLOW_OFFLOAD_DIR_REPLY,
> > > > -				       stats[1].pkts, stats[1].bytes);
> > > > -	}
> > > > +	flow_offload_update_stats(offload, stats);
> > > >  }
> > > >  
> > > >  static void flow_offload_work_handler(struct work_struct *work)
> > > > -- 
> > > > 2.47.3
> > > > 
> > 
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]