On 22/11/2018 17:35, Andrew Lunn wrote: > On Thu, Nov 22, 2018 at 06:29:24AM +0200, Nikolay Aleksandrov wrote: >> We have been adding many new bridge options, a big number of which are >> boolean but still take up netlink attribute ids and waste space in the skb. >> Recently we discussed learning from link-local packets[1] and decided >> yet another new boolean option will be needed, thus introducing this API >> to save some bridge nl space. >> The API supports changing the value of multiple boolean options at once >> via the br_boolopt_multi struct which has an optmask (which options to >> set, bit per opt) and optval (options' new values). Future boolean >> options will only be added to the br_boolopt_id enum and then will have >> to be handled in br_boolopt_toggle/get. The API will automatically >> add the ability to change and export them via netlink, sysfs can use the >> single boolopt function versions to do the same. The behaviour with >> failing/succeeding is the same as with normal netlink option changing. >> >> If an option requires mapping to internal kernel flag or needs special >> configuration to be enabled then it should be handled in >> br_boolopt_toggle. It should also be able to retrieve an option's current >> state via br_boolopt_get. >> >> [1] https://www.spinics.net/lists/netdev/msg532698.html >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> >> --- >> include/uapi/linux/if_bridge.h | 18 +++++++++ >> include/uapi/linux/if_link.h | 1 + >> net/bridge/br.c | 68 ++++++++++++++++++++++++++++++++++ >> net/bridge/br_netlink.c | 17 ++++++++- >> net/bridge/br_private.h | 6 +++ >> net/core/rtnetlink.c | 2 +- >> 6 files changed, 110 insertions(+), 2 deletions(-) >> >> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h >> index e41eda3c71f1..6dc02c03bdf8 100644 >> --- a/include/uapi/linux/if_bridge.h >> +++ b/include/uapi/linux/if_bridge.h >> @@ -292,4 +292,22 @@ struct br_mcast_stats { >> __u64 mcast_bytes[BR_MCAST_DIR_SIZE]; >> __u64 mcast_packets[BR_MCAST_DIR_SIZE]; >> }; >> + >> +/* bridge boolean options >> + * IMPORTANT: if adding a new option do not forget to handle >> + * it in br_boolopt_toggle/get and bridge sysfs >> + */ >> +enum br_boolopt_id { >> + BR_BOOLOPT_MAX >> +}; >> + >> +/* struct br_boolopt_multi - change multiple bridge boolean options >> + * >> + * @optval: new option values (bit per option) >> + * @optmask: options to change (bit per option) >> + */ >> +struct br_boolopt_multi { >> + __u32 optval; >> + __u32 optmask; >> +}; > > Hi Nikolay > > Thanks for handling this. > > How many boolean options do we already have? What it the likelihood a > u32 is going to be too small, in a couple of years time? > It would mean doubling the number of current options and this is only for boolean options so I think we're safe. > I recently went through the pain of converting the u32 for > representing link modes in the phylib API to a linux bitmap. I'm just > wondering if in the long run, using a linux bitmap right from the > beginning would be better? > >> +int br_boolopt_multi_toggle(struct net_bridge *br, >> + struct br_boolopt_multi *bm) >> +{ >> + unsigned long bitmap = bm->optmask; >> + int err = 0; >> + int opt_id; >> + >> + for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) { >> + bool on = !!(bm->optval & BIT(opt_id)); >> + >> + err = br_boolopt_toggle(br, opt_id, on); >> + if (err) { >> + br_debug(br, "boolopt multi-toggle error: option: %d current: %d new: %d error: %d\n", >> + opt_id, br_boolopt_get(br, opt_id), on, err); > > Would it be possible to return that to userspace using the extended > error infrastructure? > No, it doesn't support dynamic messages AFAIK. > Andrew >
