On Wed, Mar 12, 2025 at 07:28:38PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > - struct ct_iter_state *st = seq->private;
> > > + hlist_nulls_for_each_entry(h, n, &st->hash[i], hnnode) {
> >
> > hlist_nulls_for_each_entry_rcu ?
>
> Yes.
>
> > > - if (likely(get_nulls_value(head) == st->bucket)) {
> > > - if (++st->bucket >= st->htable_size)
> > > - return NULL;
> > > + ++skip;
> > > }
> > > - head = rcu_dereference(
> > > - hlist_nulls_first_rcu(&st->hash[st->bucket]));
> >
> > This does not rewind if get_nulls_value(head) != st->bucket),
> > not needed anymore?
>
> There are only two choices:
> 1. rewind and (possibly) dump entries more than once
> 2. skip to next and miss an entry
I think we can still display duplicates in 2. too since nulls check if
the iteration finished on another bucket? Then 2. means skipped
entries and duplicates.
> I'm not sure whats worse/better.
Skipping looks simpler, it is less code. But if entries are duplicated
then userspace has a chance to deduplicate?
I am not sure how many entries could be skipped without the nulls
check in practise TBH.
If you prefer to simple and skip entries, I suggest to add this to the
patch description, this is a change in the behaviour that is worth
documenting IMO.
Thanks.
