Stefano Brivio <sbrivio@xxxxxxxxxx> 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!
