Re: [PATCH net-next v7 06/17] ethtool: netlink bitset handling

[Michal Kubecek]

On Mon, Oct 14, 2019 at 03:02:05PM +0200, Jiri Pirko wrote:
> Mon, Oct 14, 2019 at 01:18:47PM CEST, mkubecek@xxxxxxx wrote:
> >On Fri, Oct 11, 2019 at 03:34:29PM +0200, Jiri Pirko wrote:
> >> Wed, Oct 09, 2019 at 10:59:18PM CEST, mkubecek@xxxxxxx 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