Re: [PATCH nf-next 4/4] netfilter: nft_set_pipapo: speed up bulk element insertions

[Florian Westphal <fw@strlen.de>]

Stefano Brivio <sbrivio@redhat.com> wrote:
> /* pipapo_realloc_mt() - Reallocate mapping table if needed upon resize
>  * @f:		Field containing mapping table
>  * @old_rules:	Amount of existing mapped rules
>  * @rules:	Amount of new rules to map
>  *
>  * Return: 0 on success, negative error code on failure.
>  */

Thanks, I'll steal this for v2.

> static int pipapo_realloc_mt(struct nft_pipapo_field *f,
> 			     unsigned int old_rules, unsigned int rules)
> 
> > +{
> > +	union nft_pipapo_map_bucket *new_mt = NULL, *old_mt = f->mt;
> > +	unsigned int extra = 4096 / sizeof(*new_mt);
> 
> Shouldn't we actually use PAGE_SIZE? I think the one-page limit is
> somewhat arbitrary but makes sense, so it should be 64k on e.g.
> CONFIG_PPC_64K_PAGES=y.

I wasn't sure if it would make sense to to use 64k for this batching,
I guess kvmalloc will use vmalloc anyway for such huge sets so I'll
change it back to PAGE_SIZE.

> > +	BUILD_BUG_ON(extra < 32);
> 
> I'm not entirely sure why this would be a problem. I mean, 'extra' at
> this point is the number of extra rules, not the amount of extra
> bytes, right?

Its a leftover from a version where this was bytes. I'll remove it.

> > +	/* small sets get precise count, else add extra slack
> > +	 * to avoid frequent reallocations.  Extra slack is
> > +	 * currently one 4k page worth of rules.
> > +	 *
> > +	 * Use no slack if the set only has a small number
> > +	 * of rules.
> 
> This isn't always true: if we slightly decrease the size of a small
> mapping table, we might leave some slack, because we might hit the
> (remove < (2u * extra)) condition above. Is that intended? It doesn't
> look problematic to me, by the way.

Ok. I'll ammend the comment then.

> > +	if (old_mt)
> > +		memcpy(new_mt, old_mt, min(old_rules, rules) * sizeof(*new_mt));
> > +
> > +	if (rules > old_rules)
> 
> Nit: curly braces around multi-line block (for consistency).

Oh?  Guess I'll see if checkpatch complains...

> >  		if (src->rules > 0) {
> > -			dst->mt = kvmalloc_array(src->rules, sizeof(*src->mt), GFP_KERNEL);
> > +			dst->mt = kvmalloc_array(src->rules_alloc, sizeof(*src->mt), GFP_KERNEL);
> >  			if (!dst->mt)
> >  				goto out_mt;
> >  
> >  			memcpy(dst->mt, src->mt, src->rules * sizeof(*src->mt));
> > +			dst->rules_alloc = src->rules_alloc;
> > +			dst->rules = src->rules;
> 
> These two, and setting rules_alloc below, shouldn't be needed, because we
> already copy everything in the source field before the lookup table, above.

Ah you mean:
    memcpy(dst, src, offsetof(struct nft_pipapo_field, lt));

.. above.  Ok, I'll remove those lines then.

Thanks for reviewing!