Re: [PATCH RFC nf-next 08/12] netfilter: ipset: replace internal hash table with rhashtable

Jozsef Kadlecsik <[email protected]> wrote:
> >   This adds full set->lock serialization, meaning no parallel
> >   insertion for elements with comment extension.
> 
> The comment extension is restricted to add from userspace which is
> serialized by the mutex anyway.

Ah.  I was worried wrt. FORCEADD and the like evicting this and
messing the counter up.

I will look at this again, maybe this accounting can be switched
to atomic_add/sub to avoid the lock.

> > 	struct net_prefixes __rcu *rnets[IPSET_NET_COUNT]; /* cidr prefixes */
> > 	struct htable_gc gc;	/* gc workqueue */
> > -	u32 maxelem;		/* max elements in the hash */
> > -	u32 initval;		/* random jhash init value */
> 
> Please do not remove initval: the sole reason to keep it is to maintain full
> userspace compatibility. The "ipset" tool supports to specify initval and
> when listing/saving a set, initval is included in the output. Already
> existing setups may specify the value and we must be able to return it.

Fair, I will keep it to dump the value back to userspace.

> > +			pr_warn("Set %s is full, maxelem %u reached\n",
> > +				set->name, h->maxelem);
> > +		mtype_data_next(&h->next, d);
> 
> Why the mtype_data_next() call is needed here?

It's not, merge gone wrong during rebase :-/
Plan was this:

Call mtype_data_next under if (0) guard so compiler does not
complain.

Then figure out if we can remove it together with .resize callback.
But I forgot about that which is why its not mentioned in the commit
message.

Is that feasible?  (removal I mean).

> > -	if (tb[IPSET_ATTR_INITVAL])
> > -		h->initval = ntohl(nla_get_be32(tb[IPSET_ATTR_INITVAL]));
> > -	else
> > -		get_random_bytes(&h->initval, sizeof(h->initval));
> 
> When initval restored, the else branch is still not needed anymore.

Ok, thanks Jozsef, I was about to ask.  I restored the h->initval =
line.