Re: [PATCH nf-next 1/3] netfilter: nf_set_pipapo_avx2: fix initial map fill

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +	/* Starting map doesn't need to be set to all-ones for this implementation,
> > +	 * but we do need to zero the remaining bits, if any.
> > +	 */
> > +	for (i = f->bsize; i < m->bsize_max; i++)
> > +		res_map[i] = 0ul;
> > +}
> > +
> >  /**
> >   * nft_pipapo_avx2_lookup() - Lookup function for AVX2 implementation
> >   * @net:	Network namespace
> > @@ -1171,7 +1190,7 @@ bool nft_pipapo_avx2_lookup(const struct net *net, const struct nft_set *set,
> >  	res  = scratch->map + (map_index ? m->bsize_max : 0);
> >  	fill = scratch->map + (map_index ? 0 : m->bsize_max);
> >  
> > -	/* Starting map doesn't need to be set for this implementation */
> > +	pipapo_resmap_init_avx2(m, res);
> 
> nitpick:
> 
> nft_pipapo_avx2_lookup_slow() calls pipapo_resmap_init() for
> non-optimized fields, eg. 8 bytes, which is unlikely to be seen.
> IIUC this resets it again.

Yes.  We have no test case for this function.
Can you come up with an example that would exercise this function?
It would be good to cover it in selftests.

> Maybe revisit this in nf-next? Would be worth to cover this avx2 path
> with 8 bytes in tests?

Not sure its worth it.  pipapo_resmap_init_avx2(), in most cases, is
a no-op as usually m->bsize_max is the same as f->bsize.

But yes, we could add yet another version of pipapo_resmap_init()
that only has the one-fill part.