From: Michal Kubecek <mkubecek@suse.cz>
To: netdev@vger.kernel.org
Cc: Jiri Pirko <jiri@resnulli.us>, David Miller <davem@davemloft.net>,
Jakub Kicinski <jakub.kicinski@netronome.com>,
Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
John Linville <linville@tuxdriver.com>,
Stephen Hemminger <stephen@networkplumber.org>,
Johannes Berg <johannes@sipsolutions.net>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v7 06/17] ethtool: netlink bitset handling
Date: Mon, 21 Oct 2019 09:18:15 +0200 [thread overview]
Message-ID: <20191021071815.GE27784@unicorn.suse.cz> (raw)
In-Reply-To: <20191014130205.GA2314@nanopsycho>
On Mon, Oct 14, 2019 at 03:02:05PM +0200, Jiri Pirko wrote:
> Mon, Oct 14, 2019 at 01:18:47PM CEST, mkubecek@suse.cz wrote:
> >On Fri, Oct 11, 2019 at 03:34:29PM +0200, Jiri Pirko wrote:
> >> Wed, Oct 09, 2019 at 10:59:18PM CEST, mkubecek@suse.cz wrote:
> >> >+Bit sets
> >> >+========
> >> >+
> >> >+For short bitmaps of (reasonably) fixed length, standard ``NLA_BITFIELD32``
> >> >+type is used. For arbitrary length bitmaps, ethtool netlink uses a nested
> >> >+attribute with contents of one of two forms: compact (two binary bitmaps
> >> >+representing bit values and mask of affected bits) and bit-by-bit (list of
> >> >+bits identified by either index or name).
> >> >+
> >> >+Compact form: nested (bitset) atrribute contents:
> >> >+
> >> >+ ============================ ====== ============================
> >> >+ ``ETHTOOL_A_BITSET_LIST`` flag no mask, only a list
> >>
> >> I find "list" a bit confusing name of a flag. Perhaps better to stick
> >> with the "compact" terminology and make this "ETHTOOL_A_BITSET_COMPACT"?
> >> Then in the code you can have var "is_compact", which makes the code a
> >> bit easier to read I believe.
> >
> >This is not the same as "compact", "list" flag means that the bit set
> >does not represent a value/mask pair but only a single bitmap (which can
> >be understood as a list or subset of possible values).
>
> Okay, this is confusing. So you say that the "LIST" may be on and
> ETHTOOL_A_BITSET_VALUE present, but ETHTOOL_A_BITSET_MASK not?
> I thought that whtn "LIST" is on, no "VALUE" nor "MASK" should be here.
>
> >This saves some space in kernel replies where there is no natural mask
> >so that we would have to invent one (usually all possible bits) but it
>
> Do you have an example?
E.g. peer advertised link modes or the four bitmaps returned in reply to
query for netdev features (replacement for ETHTOOL_GFEATURES).
> >is more important in request where some request want to modify a subset
> >of bits (set some, unset some) while some requests pass a list of bits
> >to be set after the operation (i.e. "I want exactly these to be
> >enabled").
>
> Hmm, it's a different type of bitset then. Wouldn't it be better to have
> ETHTOOL_A_BITSET_TYPE
> and enum:
> ETHTOOL_A_BITSET_TYPE_LIST
> ETHTOOL_A_BITSET_TYPE_MASKED
> or something like that?
> Or maybe just NLA_FLAG called "MASKED". I don't know, "list" has a
> specific meaning and this isn't that...
"MASKED" sounds fine to me. After all, there is a good chance there will
be more cases when bitset without mask will be returned so that it would
be natural to see unmasked bitmaps as default and value/mask pairs as
something special.
> >> B) Why don't you do bitmap_to_arr32 conversion in this function just
> >> before val/mask put. Then you can use normal test_bit() here.
> >
> >This relates to the question (below) why we need two versions of the
> >functions, one for unsigned long based bitmaps, one for u32 based ones.
> >The reason is that both are used internally by existing code. So if we
> >had only one set of bitset functions, callers using the other format
> >would have to do the wrapping themselves.
> >
> >There are two reasons why u32 versions are implemented directly and
> >usingned long ones as wrappers. First, u32 based bitmaps are more
> >frequent in existing code. Second, when we can get away with a cast
> >(i.e. anywhere exect 64-bit big endian), unsigned long based bitmap can
> >be always interpreted as u32 based bitmap but if we tried it the other
> >way, we would need a special handling of the last word when the number
> >of 32-bit words is odd.
>
> Okay. Perhaps you can add it as a comment so it is clear what is going
> on?
OK
Michal
next prev parent reply other threads:[~2019-10-21 7:18 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 20:59 [PATCH net-next v7 00/17] ethtool netlink interface, part 1 Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 01/17] rtnetlink: provide permanent hardware address in RTM_NEWLINK Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 02/17] netlink: rename nl80211_validate_nested() to nla_validate_nested() Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 03/17] ethtool: move to its own directory Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 04/17] ethtool: introduce ethtool netlink interface Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 05/17] ethtool: helper functions for " Michal Kubecek
2019-10-10 13:42 ` Jiri Pirko
2019-10-10 17:13 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 06/17] ethtool: netlink bitset handling Michal Kubecek
2019-10-11 13:34 ` Jiri Pirko
2019-10-14 11:18 ` Michal Kubecek
2019-10-14 13:02 ` Jiri Pirko
2019-10-21 7:18 ` Michal Kubecek [this message]
2019-10-09 20:59 ` [PATCH net-next v7 07/17] ethtool: support for netlink notifications Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 08/17] ethtool: move string arrays into common file Michal Kubecek
2019-10-10 13:27 ` Jiri Pirko
2019-10-09 20:59 ` [PATCH net-next v7 09/17] ethtool: generic handlers for GET requests Michal Kubecek
2019-10-10 13:56 ` Jiri Pirko
2019-10-10 18:04 ` Michal Kubecek
2019-10-10 18:18 ` Johannes Berg
2019-10-10 20:00 ` Michal Kubecek
2019-10-11 8:08 ` Johannes Berg
2019-10-11 6:06 ` Jiri Pirko
2019-10-10 15:23 ` Jiri Pirko
2019-10-09 20:59 ` [PATCH net-next v7 10/17] ethtool: provide string sets with STRSET_GET request Michal Kubecek
2019-10-10 13:59 ` Jiri Pirko
2019-10-10 18:05 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 11/17] ethtool: provide link mode names as a string set Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 12/17] ethtool: provide link settings with LINKINFO_GET request Michal Kubecek
2019-10-10 15:59 ` Jiri Pirko
2019-10-10 20:15 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 13/17] ethtool: add standard notification handler Michal Kubecek
2019-10-10 15:25 ` Jiri Pirko
2019-10-10 18:17 ` Michal Kubecek
2019-10-11 5:56 ` Jiri Pirko
2019-10-11 5:59 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 14/17] ethtool: set link settings with LINKINFO_SET request Michal Kubecek
2019-10-10 15:37 ` Jiri Pirko
2019-10-10 19:30 ` Michal Kubecek
2019-10-11 5:59 ` Jiri Pirko
2019-10-12 16:33 ` Jiri Pirko
2019-10-14 8:48 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 15/17] ethtool: provide link mode information with LINKMODES_GET request Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 16/17] ethtool: set link modes related data with LINKMODES_SET request Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 17/17] ethtool: provide link state with LINKSTATE_GET request Michal Kubecek
2019-10-11 0:48 ` [PATCH net-next v7 00/17] ethtool netlink interface, part 1 Jakub Kicinski
2019-10-11 6:46 ` Johannes Berg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191021071815.GE27784@unicorn.suse.cz \
--to=mkubecek@suse.cz \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=jiri@resnulli.us \
--cc=johannes@sipsolutions.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).