Re: [PATCH nf-next 1/2] netfilter: nft_set_pipapo_avx2: split lookup function in two parts

[Florian Westphal <fw@strlen.de>]

Stefano Brivio <sbrivio@redhat.com> wrote:
> > - * nft_pipapo_avx2_lookup() - Lookup function for AVX2 implementation
> > - * @net:	Network namespace
> > - * @set:	nftables API set representation
> > - * @key:	nftables API element representation containing key data
> > + * pipapo_get_avx2() - Lookup function for AVX2 implementation
> > + * @m:		storage containing the set elements
> > + * @data:	Key data to be matched against existing elements
> > + * @genmask:	If set, check that element is active in given genmask
> > + * @tstamp:	timestamp to check for expired elements
> 
> Nits: Storage, Timestamp (or all lowercase, for consistency with the
> other ones).

Note that there is no consistency whatsoever in the kernel.
Some use upper case, some lower, some indent on same level (like done
here), some don't.

So, I don't care anymore since it will never be right.

In case i have to mangle it anyway i will "fix" it.

> > +			e = f->mt[ret].e;
> > +			if (unlikely(__nft_set_elem_expired(&e->ext, tstamp) ||
> 
> Here's the actual concern, even if I haven't tested this: I guess you now
> pass the timestamp to this function instead of getting it with each
> nft_set_elem_expired() call for either correctness (it should be done at
> the beginning of the insertion?) or as an optimisation (if BITS_PER_LONG < 64
> the overhead isn't necessarily trivial).

Its done because during insertion time should be frozen to avoid
elements timing out while transaction is in progress.
(this is unrelated to this patchset).

But in order to use this snippet from both control and data path
this has to be passed in so it can either be 'now' or 'time at
start of transaction'.

> But with 2/2, you need to call get_jiffies_64() as a result, from non-AVX2
> code, even for sets without a timeout (without NFT_SET_EXT_TIMEOUT
> extension).
> 
> Does that risk causing a regression on non-AVX2? If it's for correctness,
> I think we shouldn't care, but if it's done as an optimisation, perhaps
> it's not a universal one.

Its not an optimisation.  I could pass a 'is_control_plane' or
'is_packetpath' but I considered it too verbose and not needed for
correctness.