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

Florian Westphal <[email protected]> wrote:
> +		write_seqcount_end(&nets->seq);
> +		spin_unlock_bh(&set->lock);
>  	}
>  #endif
>  }
> @@ -1253,31 +1310,41 @@ mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
>  #if IPSET_NET_COUNT == 2
>  	struct net_prefixes *nets1;
>  	struct mtype_elem orig = *d;
> +	unsigned int seq1;
>  	int ret, i, j, k;
>  #else
>  	int ret, i, j;
>  #endif
> -	u32 key, multi = 0;
> +	unsigned int seq0;
> +	u32 key, multi;
>  	u8 pos;
>  
>  	pr_debug("test by nets\n");
>  	rcu_read_lock_bh();
> +retry:
> +	multi = 0;
>  	nets0 = rcu_dereference_bh(h->rnets[0]);
> +	seq0 = read_seqcount_begin(&nets0->seq);
[..]
>  unlock:
> +	if (read_seqcount_retry(&nets0->seq, seq0))
> +		goto retry;

Sashiko now says this retry loop causes double-count
on match, yet v3 said not doing this causes possible
miss of a nomatch entry.

I think missing nomatch entry is worse and doublecount
is ok.

The restart is rare enough anyway because write_seqcount_begin()
is only called when set is flushed or a element deletion brings
a cidr count value down to 0.

Another option is to prellocate all possible CIDRs but I'd
like to avoid it, this means checking up to 128 possible CIDRs
in ipv6 case.

Pre-sampling all values into local on-stack array is a non-starter
too due to stack cost.

What one COULD do is to use pcpu stash area for this, needs
local bh / local_lock.

But I think its a bit "too much" for this problem.