On Wed, Oct 19, 2022 at 04:28:23PM +0300, Nikolay Aleksandrov wrote:
> On 18/10/2022 15:04, Ido Schimmel wrote:
> > +static int br_mdb_config_src_list_init(struct nlattr *src_list,
> > + struct br_mdb_config *cfg,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_mdb_src_entry *src, *tmp;
> > + struct nlattr *src_entry;
> > + int rem, err;
> > +
> > + nla_for_each_nested(src_entry, src_list, rem) {
> > + err = br_mdb_config_src_entry_init(src_entry, cfg, extack);
>
> Hmm, since we know the exact number of these (due to attr embedding) can't we allocate
> all at once and drop the list? They should not be more than 32 (PG_SRC_ENT_LIMIT) IIRC,
> which makes it at most 1152 bytes. Might simplify the code a bit and reduce allocations.
I didn't see how I can reliably determine the exact number of source
entries without going all the 'MDBE_SRC_LIST_ENTRY' attributes. I mean,
the entries can have varying sizes in case user space provided mixed
IPv4/IPv6 sources (which will be rejected later on) and in the future we
might have more attributes per-source entry other than the address
(e.g., source timer), which might be specified only for a subset of the
entries.
So, I did end up converting to an array like you suggested, but I'm
going over the entries twice. Once to understand how large the array
should be and again to initialize it. It's control path so it should be
fine. The advantages are that the number of allocations are reduced and
that I can reject a too long source list before doing any processing:
if (cfg->num_src_entries >= PG_SRC_ENT_LIMIT) {
NL_SET_ERR_MSG_FMT_MOD(extack, "Exceeded maximum number of source entries (%u)",
PG_SRC_ENT_LIMIT);
return -EINVAL;
}
[...]
> > +static void br_mdb_config_fini(struct br_mdb_config *cfg)
> > +{
> > + br_mdb_config_attrs_fini(cfg);
> > +}
> > +
>
> Is there more coming to these two _fini helpers? If not, I think one would be enough, i.e.
> just call br_mdb_config_src_list_fini() from br_mdb_config_fini()
Done.
Thanks!
