Re: [PATCH nf-next 3/5] nft_set_pipapo: Prepare for vectorised implementation: alignment

[Stefano Brivio <sbrivio@redhat.com>]

On Sun, 23 Feb 2020 23:14:35 +0100
Florian Westphal <fw@strlen.de> wrote:

> Stefano Brivio <sbrivio@redhat.com> wrote:
> >  struct nft_pipapo_field {
> > @@ -439,6 +456,9 @@ struct nft_pipapo_field {
> >  	unsigned long rules;
> >  	size_t bsize;
> >  	int bb;
> > +#ifdef NFT_PIPAPO_ALIGN
> > +	unsigned long *lt_aligned;
> > +#endif
> >  	unsigned long *lt;
> >  	union nft_pipapo_map_bucket *mt;
> >  };  
> 
> I wonder if these structs can be compressed.
> AFAICS bsize is in sizes of longs, so when this number is
> large then we also need to kvmalloc a large blob of memory.
> 
> I think u32 would be enough?

Hm, yes, it is. I just thought it was a "good idea" to use size_t for
"sizes", but this is so implementation-specific that u32 would make
total sense (it's enough for 32GiB), and avoid holes on 64-bit archs.

> nft_pipapo_field is probably the most relevant one wrt. to size.

Right. I forgot about that when I added 'bb'.

> >  struct nft_pipapo_match {
> >  	int field_count;
> > +#ifdef NFT_PIPAPO_ALIGN
> > +	unsigned long * __percpu *scratch_aligned;
> > +#endif
> >  	unsigned long * __percpu *scratch;
> >  	size_t bsize_max;  
> 
> Same here (bsize_max -- could fit with hole after field_count)?

Yes, makes sense.

> Also, since you know the size of nft_pipapo_match (including the
> dynamically allocated array at the end), you could store the
> original memory (*scratch) and the rcu_head at the end, since
> they are not needed at lookup time and a little overhead to calculate
> their storage offset is fine.
> 
> Not sure its worth it, just an idea.

'*scratch' is actually needed at lookup time for implementations that
don't need stricter alignment than natural one, but I could probably
use some macro trickery and "move" it as needed.

I'm not sure how to deal with fields after f[0], syntactically. Do you
have some, er, pointers?

-- 
Stefano