Tue, Jul 02, 2019 at 01:50:09PM CEST, mkube...@suse.cz wrote: >The ethtool netlink code uses common framework for passing arbitrary >length bit sets to allow future extensions. A bitset can be a list (only >one bitmap) or can consist of value and mask pair (used e.g. when client >want to modify only some bits). A bitset can use one of two formats: >verbose (bit by bit) or compact. > >Verbose format consists of bitset size (number of bits), list flag and >an array of bit nests, telling which bits are part of the list or which >bits are in the mask and which of them are to be set. In requests, bits >can be identified by index (position) or by name. In replies, kernel >provides both index and name. Verbose format is suitable for "one shot" >applications like standard ethtool command as it avoids the need to >either keep bit names (e.g. link modes) in sync with kernel or having to >add an extra roundtrip for string set request (e.g. for private flags). > >Compact format uses one (list) or two (value/mask) arrays of 32-bit >words to store the bitmap(s). It is more suitable for long running >applications (ethtool in monitor mode or network management daemons) >which can retrieve the names once and then pass only compact bitmaps to >save space. > >Userspace requests can use either format and ETHTOOL_RF_COMPACT flag in >request header tells kernel which format to use in reply. Notifications >always use compact format. > >Signed-off-by: Michal Kubecek <mkube...@suse.cz> >--- > Documentation/networking/ethtool-netlink.txt | 61 ++ > include/uapi/linux/ethtool_netlink.h | 35 ++ > net/ethtool/Makefile | 2 +- > net/ethtool/bitset.c | 606 +++++++++++++++++++ > net/ethtool/bitset.h | 40 ++ > net/ethtool/netlink.h | 9 + > 6 files changed, 752 insertions(+), 1 deletion(-) > create mode 100644 net/ethtool/bitset.c > create mode 100644 net/ethtool/bitset.h > >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? >+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^^ >+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) ? >+ >+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 [...]