Messages in this thread | | | Date | Mon, 21 Oct 2019 09:18:15 +0200 | From | Michal Kubecek <> | Subject | Re: [PATCH net-next v7 06/17] ethtool: netlink bitset handling |
| |
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
| |