On Mon, 18 Aug 2025 20:23:49 +0200
Florian Westphal <fw@xxxxxxxxx> wrote:
> Stefano Brivio <sbrivio@xxxxxxxxxx> 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.
Yeah, I just meant for consistency with the other parameter descriptions
of this function itself ("the other ones") and of these files.
> So, I don't care anymore since it will never be right.
>
> In case i have to mangle it anyway i will "fix" it.
Either way,
Reviewed-by: Stefano Brivio <sbrivio@xxxxxxxxxx>
> > > + 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'.
Ah, I see now, thanks for explaining!
> > 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.
...and it wouldn't help with my potential concern either as
is_packetpath would always be set anyway on lookups, with or without
timeout. Never mind then.
--
Stefano
