Re: [PATCH nf] netfilter: ipset: remove need to allocate memory on delete operations

Jozsef Kadlecsik <[email protected]> wrote:
> >  /* Book-keeping of the prefixes added to the set */
> >  struct net_prefix {
> > -	u8 cidr;			/* the cidr value */
> > -	u32 count;			/* number of elements of this cidr */
> > +	u32 cidr:8;
> > +	u32 count:24;
> >  };
> > +#define CIDR_MAX_COUNT ((1 << 24) - 1)
> 
> Could it be changed to
> 
> struct net_prefix {
>      u64 cidr:8;
>      u64 count:24;
> };
> #define CIDR_MAX_COUNT ((1 << 56) - 1)
> 
> (and the other required changes below)?

Only for 64bit builds, because WRITE_ONCE() is not atomic on 32bit.
If we do it for 32 bit builds, all read side paths would need to
switch to write seqcount use (including the normal case, i.e.
nets[i]->count++ / count.  ATM only the "it became 0 and has
to be moved) uses  the sequence write lock, so read side never
retries for typical updates.

> 2**24 is a lot but still a reachable limit.

Hmm. 16m entries...  I guess its possible for very small prefix
lengths?  We could use BITS_PER_LONG == 64 but it gets more ugly then.

> > +	if (np.count) {
> > +		WRITE_ONCE(nets->nets[found], np);
> >  		goto unlock;

This should be the typical case, which doesn't change the
write seq side, hence no retry on reader side.

This would have to be changed if we no longer rely
on READ/WRITE_ONCE.