From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH net-next 1/2] net: bridge: add support for user-controlled bool options Date: Thu, 22 Nov 2018 11:37:17 -0800 Message-ID: <20181122113717.5c88bdb4@xeon-e3> References: <20181122042925.8878-1-nikolay@cumulusnetworks.com> <20181122042925.8878-2-nikolay@cumulusnetworks.com> <20181122153500.GF15403@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Andrew Lunn , netdev@vger.kernel.org, roopa@cumulusnetworks.com, davem@davemloft.net, bridge@lists.linux-foundation.org To: Nikolay Aleksandrov Return-path: Received: from mail-pg1-f195.google.com ([209.85.215.195]:42145 "EHLO mail-pg1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731412AbeKWGSI (ORCPT ); Fri, 23 Nov 2018 01:18:08 -0500 Received: by mail-pg1-f195.google.com with SMTP id d72so2080057pga.9 for ; Thu, 22 Nov 2018 11:37:20 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 22 Nov 2018 18:01:29 +0200 Nikolay Aleksandrov wrote: > 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 > >> --- > >> 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 > > > My concern is about backwards compatibility. What about old userspace and new userspace tools with old kernels. Having multiple bits does allow handling cases where certain combos won't work. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=6/3aDn/NzPWOWN7NHfqgkjGzvcpg9OLP54S7+u7dqaU=; b=coQxMorDLWPEWcBKPvap1OuLxs6LcNy7FOqTadeV9sN6Aj1nfxhsZsXb6AmHy9Y0kT TodDTLgVEw1s+nTdMELZLqR5MsNkUoqQYo0mt/xo13SgQVwpzvKMDkn1p3rmmqtiaiJi NZAfHsbGbqHynZLq2E8ux2xXZ9GJGpUEoiFEixkFomnGlSaKdUJWeiQ7AkeG8pzZOtiV aFJ+rpcNUxXNurrQSWYNbFYgeVNZ6A8ygRObg9JA4BXIuDgS1/BdCISQJ0/YxIVi+5sh sS/q9n8gmoSwXIjS6P+hOqhSOumYWhk8zlGSTSmJ3VQPFGjriuwZMTFUFmUy7uYYmj5t yUug== Date: Thu, 22 Nov 2018 11:37:17 -0800 From: Stephen Hemminger Message-ID: <20181122113717.5c88bdb4@xeon-e3> In-Reply-To: References: <20181122042925.8878-1-nikolay@cumulusnetworks.com> <20181122042925.8878-2-nikolay@cumulusnetworks.com> <20181122153500.GF15403@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for user-controlled bool options List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikolay Aleksandrov Cc: Andrew Lunn , roopa@cumulusnetworks.com, bridge@lists.linux-foundation.org, davem@davemloft.net, netdev@vger.kernel.org On Thu, 22 Nov 2018 18:01:29 +0200 Nikolay Aleksandrov wrote: > 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 > >> --- > >> 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 > > > My concern is about backwards compatibility. What about old userspace and new userspace tools with old kernels. Having multiple bits does allow handling cases where certain combos won't work.