Wed, Jul 03, 2019 at 08:18:51PM CEST, mkube...@suse.cz wrote: >On Wed, Jul 03, 2019 at 01:49:33PM +0200, Jiri Pirko wrote: >> Tue, Jul 02, 2019 at 01:50:09PM CEST, mkube...@suse.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
