Stefano Brivio <sbrivio@xxxxxxxxxx> wrote:
> On Fri, 12 Sep 2025 15:19:59 +0200
> Florian Westphal <fw@xxxxxxxxx> wrote:
> By the way, you're referring to 'rp' here, and nothing else, right?
Yes, sorry.
> Could you mention that explicitly if that's the case? I have to say
> that "data (...) has already been incremented" took me a while to
> understand.
Apologies, I will rephrase it.
> > The restart logic in AVX2 is different compared to the plain C
> > implementation, but both should follow the same logic.
>
> That's because I wanted to avoid calling pipapo_refill() from the AVX2
> lookup path, as it was significantly slower than re-doing the full
> lookup, at least for "net, port" sets which I assumed would be the most
> common.
>
> On the other hand, it should always be a corner case (right?), so I
> guess simplicity / consistency should prevail.
Yes, I don't think we ever have a duplicate to begin with, insert path
returns -EEXIST or ignores the insertion request.
They exist in the clone/copy for the case when one transaction asked for
remoal of "e" (and marks it as invalid in new generation) followed by
a re-insert of "e".
In that case, The "e" re-insert should find and skip the old result
and continue to search for a matching result.
Thats also what the test case does:
insert e
<end of transaction>
flush set x <mark e invalid>
insert e <find e-old, continue search, find no result>, insert ok
insert e <find e-old, continue search, find e-new, fail transaction
I think I'll extend the test case to also check that "flush set x;
insert e" works on a set that already contained e before.
> > The C implementation just calls pipapo_refill() again to check the next
> > entry. Do the same in the AVX2 implementation.
>
> An alternative would be to reset rp = key, but maybe what you're doing
> is actually saner, see above.
Nope, that doesn't work. If you reset rp = key, it fixes the splat
but results in infinite loop.
> > Note that with this change, due to implementation differences of
> > pipapo_refill vs. nft_pipapo_avx2_refill, the refill call will return
> > the same element again,
> > then, on the next call it will move to the next
> > entry as expected. This is because avx2_refill doesn't clear the bitmap
> > in the 'last' conditional.
Note last sentence, its important the bitmap is toggled to off in the
"last" case so the next attempt won't result in rediscovery of that
element.
After this patch, the sequence is: (in case you have key matching both e1
and e2):
1. find e1, test, expired -> call pipapo_refill
2. find e1 again, expired -> call pipapo_refill
3. find e2, test, valid -> return e2 as matching entry
without this patch, its:
find e1, test, expired -> goto next (restart loop)
find no matching entry (rp not reset so its searching for
something else)
-> duplicate insertion passes as e2 not be found
with full restart but fixed rp assignment:
1) find e1, test, expired -> reset key
2) goto 1
... so i opted for the first solution.
For the packet path, the "pipapo_refill" call will happen when
the element is expired (timed out).
I don't see many users mixing pipapo entries with timeouts.
> > - !nft_set_elem_active(&e->ext, genmask)))
> > - goto next_match;
> > + !nft_set_elem_active(&e->ext, genmask))) {
> > + ret = pipapo_refill(res, f->bsize, f->rules, fill, f->mt, last);
>
> It could be wrapped like this:
>
> ret = pipapo_refill(res, f->bsize, f->rules,
> fill, f->mt, last);
I have to resend anyway so I will fix this up, thanks.
> Reviewed-by: Stefano Brivio <sbrivio@xxxxxxxxxx>
Thanks for reviewing!
