Date:   Thu, 4 Jul 2019 10:04:35 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 06/15] ethtool: netlink bitset handling

Wed, Jul 03, 2019 at 08:18:51PM CEST, mkubecek@...e.cz wrote:
>On Wed, Jul 03, 2019 at 01:49:33PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 01:50:09PM CEST, mkubecek@...e.cz wrote:
>> >diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
>> >index 97c369aa290b..4636682c551f 100644
>> >--- a/Documentation/networking/ethtool-netlink.txt
>> >+++ b/Documentation/networking/ethtool-netlink.txt
>> >@@ -73,6 +73,67 @@ set, the behaviour is the same as (or closer to) the behaviour before it was
>> > introduced.
>> > 
>> > 
>> >+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
>> >+    ETHTOOL_A_BITSET_SIZE	(u32)		number of significant bits
>> >+    ETHTOOL_A_BITSET_VALUE	(binary)	bitmap of bit values
>> >+    ETHTOOL_A_BITSET_MASK	(binary)	bitmap of valid bits
>> >+
>> >+Value and mask must have length at least ETHTOOL_A_BITSET_SIZE bits rounded up
>> >+to a multiple of 32 bits. They consist of 32-bit words in host byte order,
>> 
>> Looks like the blocks are similar to NLA_BITFIELD32. Why don't you user
>> nested array of NLA_BITFIELD32 instead?
>
>That would mean a layout like
>
>  4 bytes of attr header
>  4 bytes of value
>  4 bytes of mask
>  4 bytes of attr header
>  4 bytes of value
>  4 bytes of mask
>  ...
>
>i.e. interleaved headers, words of value and words of mask. Having value
>and mask contiguous looks cleaner to me. Also, I can quickly check the
>sizes without iterating through a (potentially long) array.

Yeah, if you are not happy with this, I suggest to introduce
NLA_BITFIELD with arbitrary size. That would be probably cleanest.


>
>> >+words ordered from least significant to most significant (i.e. the same way as
>> >+bitmaps are passed with ioctl interface).
>> >+
>> >+For compact form, ETHTOOL_A_BITSET_SIZE and ETHTOOL_A_BITSET_VALUE are
>> >+mandatory.  Similar to BITFIELD32, a compact form bit set requests to set bits
>> 
>> Double space^^
>
>Hm, I have to learn how to tell vim not to do that with "gq".
>
>> >+in the mask to 1 (if the bit is set in value) or 0 (if not) and preserve the
>> >+rest. If ETHTOOL_A_BITSET_LIST is present, there is no mask and bitset
>> >+represents a simple list of bits.
>> 
>> Okay, that is a bit confusing. Why not to rename to something like:
>> ETHTOOL_A_BITSET_NO_MASK (flag)
>> ?
>
>>From the logical point of view, it's used for lists - list of link
>modes, list of netdev features, list of timestamping modes etc.
>
>The point is that in userspace requests, we sometimes want to change
>some values (enable A, disable B), sometimes to define the list of
>values to be set (I want (only) A, C and E to be enabled). In kernel
>replies, sometimes there is a natural value/mask pairing (e.g.
>advertised and supported link modes, enabled and supported WoL modes)
>but often there is just one bitmap.
>
>> >+Kernel bit set length may differ from userspace length if older application is
>> >+used on newer kernel or vice versa. If userspace bitmap is longer, an error is
>> >+issued only if the request actually tries to set values of some bits not
>> >+recognized by kernel.
>> >+
>> >+Bit-by-bit form: nested (bitset) attribute contents:
>> >+
>> >+    ETHTOOL_A_BITSET_LIST	(flag)		no mask, only a list
>> >+    ETHTOOL_A_BITSET_SIZE	(u32)		number of significant bits
>> >+    ETHTOOL_A_BITSET_BIT	(nested)	array of bits
>> >+	ETHTOOL_A_BITSET_BIT+   (nested)	one bit
>> >+	    ETHTOOL_A_BIT_INDEX	(u32)		bit index (0 for LSB)
>> >+	    ETHTOOL_A_BIT_NAME	(string)	bit name
>> >+	    ETHTOOL_A_BIT_VALUE	(flag)		present if bit is set
>> >+
>> >+Bit size is optional for bit-by-bit form. ETHTOOL_A_BITSET_BITS nest can only
>> >+contain ETHTOOL_A_BITS_BIT attributes but there can be an arbitrary number of
>> >+them.  A bit may be identified by its index or by its name. When used in
>> >+requests, listed bits are set to 0 or 1 according to ETHTOOL_A_BIT_VALUE, the
>> >+rest is preserved. A request fails if index exceeds kernel bit length or if
>> >+name is not recognized.
>> >+
>> >+When ETHTOOL_A_BITSET_LIST flag is present, bitset is interpreted as a simple
>> >+bit list. ETHTOOL_A_BIT_VALUE attributes are not used in such case. Bit list
>> >+represents a bitmap with listed bits set and the rest zero.
>> >+
>> >+In requests, application can use either form. Form used by kernel in reply is
>> >+determined by a flag in flags field of request header. Semantics of value and
>> >+mask depends on the attribute. General idea is that flags control request
>> >+processing, info_mask control which parts of the information are returned in
>> >+"get" request and index identifies a particular subcommand or an object to
>> >+which the request applies.
>> 
>> This is quite complex and confusing. Having the same API for 2 APIs is
>> odd. The API should be crystal clear, easy to use.
>> 
>> Why can't you have 2 commands, one working with bit arrays only, one
>> working with strings? Something like:
>> X_GET
>>    ETHTOOL_A_BITS (nested)
>>       ETHTOOL_A_BIT_ARRAY (BITFIELD32)
>> X_NAMES_GET
>>    ETHTOOL_A_BIT_NAMES (nested)
>> 	ETHTOOL_A_BIT_INDEX
>> 	ETHTOOL_A_BIT_NAME
>> 
>> For set, you can also have multiple cmds:
>> X_SET  - to set many at once, by bit index
>>    ETHTOOL_A_BITS (nested)
>>       ETHTOOL_A_BIT_ARRAY (BITFIELD32)
>> X_ONE_SET   - to set one, by bit index
>>    ETHTOOL_A_BIT_INDEX
>>    ETHTOOL_A_BIT_VALUE
>> X_ONE_SET   - to set one, by name
>>    ETHTOOL_A_BIT_NAME
>>    ETHTOOL_A_BIT_VALUE
>
>This looks as if you assume there is nothing except the bitset in the
>message but that is not true. Even with your proposed breaking of
>current groups, you would still have e.g. 4 bitsets in reply to netdev
>features query, 3 in timestamping info GET request and often bitsets
>combined with other data (e.g. WoL modes and optional WoL password).
>If you wanted to further refine the message granularity to the level of
>single parameters, we might be out of message type ids already.

You can still have multiple bitsets(bitfields) in single message and
have separate cmd/cmds to get string-bit mapping. No need to mangle it.


>
>Unless you want to forget about structured data completely and turn
>everything into tunables - but that's rather scary idea.
>
>Michal

Powered by blists - more mailing lists