Re: [PATCH nf v3 3/5] netfilter: ipset: replace internal hash table with rhashtable

Hi Florian,

On Wed, 2 Sep 2026, Florian Westphal wrote:

> Florian Westphal <[email protected]> wrote:
>> Assisted-by: Claude:claude-opus-4-6
>> Signed-off-by: Florian Westphal <[email protected]>
>> ---
>>  v3:
>>  serialize rhltable insertion with set->lock, else we can inject identical elements.
>>  nit: also rename mtype_rht_size toke-replace
>>  add kdoc comment to say that mtype_flush cannot guarantee set is empty
>>  after flush (parallel re-add).
>>  I ignored the LLM review wrt 'skipped elements' on set walks, with
>>  parallel mutations (internal resizes, deletes) a stable walk would
>>  require significantly more work, such as keeping elements on a
>>  dedicated list.  It should be ok as-is.
>
> [..]
>
>> -/* Flush a hash type of set: destroy all elements */
>> +/**
>> + * mtype_flush() - Flush a hash set type by destroying all elements.
>> + * @set: Pointer to the ip_set.
>> + *
>> + * Because other CPUs may concurrently insert new entries into the table
>> + * while flush is in progress, there is no guarantee that the table will
>> + * be empty upon return.
>> + */
>>  static void
>>  mtype_flush(struct ip_set *set)
>>  {
>>  	struct htype *h = set->data;
>> -#ifdef IP_SET_HASH_WITH_NETS
>> -	struct net_prefixes *nets;
>> -#endif
>> -	struct htable *t;
>> -	struct hbucket *n;
>> -	u32 r, i;
>> -
>> -	t = ipset_dereference_nfnl(h->table);
>> -	for (r = 0; r < ahash_numof_locks(t->htable_bits); r++) {
>> -		spin_lock_bh(&t->hregion[r].lock);
>> -		for (i = ahash_bucket_start(r, t->htable_bits);
>> -		     i < ahash_bucket_end(r, t->htable_bits); i++) {
>> -			n = __ipset_dereference(hbucket(t, i));
>> -			if (!n)
>> +	struct rhashtable_iter hti;
>> +	struct mtype_rht_elem *e;
>> +	unsigned int dropped;
>> +
>> +	ipset_hash_walk_enter(h, &hti);
>> +restart:
>> +	dropped = 0;
>> +	rhashtable_walk_start(&hti);
>> +
>> +	while ((e = rhashtable_walk_next(&hti))) {
>> +		if (IS_ERR(e)) {
>> +			if (PTR_ERR(e) == -EAGAIN)
>>  				continue;
>> -			if (set->extensions & IPSET_EXT_DESTROY)
>> -				mtype_ext_cleanup(set, n);
>> -			/* FIXME: use slab cache */
>> -			rcu_assign_pointer(hbucket(t, i), NULL);
>> -			kfree_rcu(n, rcu);
>> +			break;
>> +		}
>> +		if (ipset_hash_remove(h, e))
>> +			continue; /* Concurrent delete? skip */
>> +		mtype_del_cidr_all(set, h, &e->elem);
>> +		ip_set_ext_destroy(set, &e->elem);
>> +		kfree_rcu(e, rcu);
>> +
>> +		if (dropped++ > 128 && need_resched()) {
>> +			rhashtable_walk_stop(&hti);
>> +			cond_resched();
>> +			goto restart;
>
> As expected this triggers reject patterns in LLM.
>
> I can go back to v1, which had no restart, which in turn triggered
> 'softirq lockup with large sets' reject patterns, but doesn't have
> the "may skip elements" "problem".
>
> Or we delay this while I work on an rhashtable_flush() that can
> detach internal hash memory from the rhashtable set to avoid this.

Ignore my previous mail. Better delay it if it can be solved in 
rhashtable_flush(). That'd be a nice solution.

> I have no idea what the best course of action is here, but adding
> an extra data structure (list for instance) is something I want to
> avoid.

Yes, I agree completely.

Best regards,
Jozsef