Re: [PATCH v3 13/13] flow: Avoid moving flow entries to compact table

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Jan 02, 2024 at 07:13:48PM +0100, Stefano Brivio wrote:
> On Mon, 1 Jan 2024 21:44:54 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Dec 28, 2023 at 07:25:25PM +0100, Stefano Brivio wrote:
> > > On Thu, 21 Dec 2023 17:15:49 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > [...]
> > >
> > > >  void flow_defer_handler(const struct ctx *c, const struct timespec *now)
> > > >  {
> > > > +	struct flow_free_block *free_head = NULL;
> > > > +	unsigned *last_next = &flow_first_free;
> > > >  	bool timer = false;
> > > > -	union flow *flow;
> > > > +	unsigned idx;
> > > >  
> > > >  	if (timespec_diff_ms(now, &flow_timer_run) >= FLOW_TIMER_INTERVAL) {
> > > >  		timer = true;
> > > >  		flow_timer_run = *now;
> > > >  	}
> > > >  
> > > > -	for (flow = flowtab + flow_count - 1; flow >= flowtab; flow--) {
> > > > +	for (idx = 0; idx < FLOW_MAX; idx++) {
> > > > +		union flow *flow = &flowtab[idx];
> > > >  		bool closed = false;
> > > >  
> > > > +		if (flow->f.type == FLOW_TYPE_NONE) {
> > > > +			/* Start of a free block */
> > > > +			free_head = &flow->free;
> > > > +			*last_next = idx;
> > > > +			last_next = &free_head->next;
> > > > +			/* Skip the rest of the block */
> > > > +			idx += free_head->n - 1;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		  
> > > 
> > > Stray tabs.

Fixed.

> > > >  		switch (flow->f.type) {
> > > > +		case FLOW_TYPE_NONE:
> > > > +			closed = true;
> > > > +			break;
> 
> ...more important than stray tabs, I noticed only now: how would we ever
> hit this switch case, if you're checking for this in the if clause just
> before?

Ah.. it can't.. but fixing it is a bit more complex.  The intention
here is that if there are multiple contiguous free clusters we'll
merge them together, which this currently won't do.  I'll have to take
another look at that.

> 
> > > >  		case FLOW_TCP:
> > > >  			closed = tcp_flow_defer(flow);
> > > >  			break;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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