Message-ID: <20191014130205.GA2314@nanopsycho>
Date:   Mon, 14 Oct 2019 15:02:05 +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 v7 06/17] ethtool: netlink bitset handling

Mon, Oct 14, 2019 at 01:18:47PM CEST, mkubecek@...e.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@...e.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?


>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...


>
>The flag could be omitted for compact form where we could simply say
>that if there is no mask, it's a list, but we need it for verbose form.
>
>> >+  ``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
>> 
>> Couple of times the NLA_BITFIELD32 limitation was discussed, so isn't
>> this the time to introduce generic NLA_BITFIELD with variable len and
>> use it here? This is exactly job for it. As this is UAPI, I believe it
>> should be done now cause later won't work.
>
>As discussed before, we would lose the option to omit mask when it's not
>needed.

Sorry, it's been couple of months already :/


>
>> >+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" does not exist in the code. I believe you ment
>> "ETHTOOL_A_BITSET_BITS"
>> 
>> 
>> >+ +-+-------------------------------+--------+-----------------------------+
>> >+ |   ``ETHTOOL_A_BITSET_BIT+``     | nested | one bit                     |
>> 
>> You seem to be missing "|" here.
>> Also "ETHTOOL_A_BITSET_BIT" does not exist. I believe you ment
>> "ETHTOOL_A_BITS_BIT"
>
>Yes on both, thanks.
>
>> >+/* bit sets */
>> >+
>> >+enum {
>> >+	ETHTOOL_A_BIT_UNSPEC,
>> >+	ETHTOOL_A_BIT_INDEX,			/* u32 */
>> >+	ETHTOOL_A_BIT_NAME,			/* string */
>> >+	ETHTOOL_A_BIT_VALUE,			/* flag */
>> >+
>> >+	/* add new constants above here */
>> >+	__ETHTOOL_A_BIT_CNT,
>> >+	ETHTOOL_A_BIT_MAX = __ETHTOOL_A_BIT_CNT - 1
>> >+};
>> >+
>> >+enum {
>> >+	ETHTOOL_A_BITS_UNSPEC,
>> >+	ETHTOOL_A_BITS_BIT,
>> >+
>> >+	/* add new constants above here */
>> >+	__ETHTOOL_A_BITS_CNT,
>> >+	ETHTOOL_A_BITS_MAX = __ETHTOOL_A_BITS_CNT - 1
>> >+};
>> 
>> I think it would be good to have this named with "_BITSET_" in it so it
>> is crystal clear this is part of the bitset UAPI.
>
>I guess we can add "_BITSET" (e.g. ETHTOOL_A_BITSET_BIT_VALUE). These
>constants shouldn't be used outside bitset.c (and some isolated part of
>the userspace code) so the length is not so much of an issue.

Great.


>
>> >+/**
>> >+ * ethnl_put_bitset32() - Put a bitset nest into a message
>> >+ * @skb:      skb with the message
>> >+ * @attrtype: attribute type for the bitset nest
>> >+ * @val:      value bitmap (u32 based)
>> >+ * @mask:     mask bitmap (u32 based, optional)
>> >+ * @nbits:    bit length of the bitset
>> >+ * @names:    array of bit names (optional)
>> >+ * @compact:  use compact format for the output
>> >+ *
>> >+ * Compose a nested attribute representing a bitset. If @mask is null, simple
>> >+ * bitmap (bit list) is created, if @mask is provided, represent a value/mask
>> >+ * pair. Bit names are only used in verbose mode and when provided by calller.
>> >+ *
>> >+ * Return:    0 on success, negative error value on error
>> 
>> Remove the spaces.
>
>OK
>
>> >+ */
>> >+int ethnl_put_bitset32(struct sk_buff *skb, int attrtype, const u32 *val,
>> >+		       const u32 *mask, unsigned int nbits,
>> >+		       ethnl_string_array_t names, bool compact)
>> >+{
>> >+	struct nlattr *nest;
>> >+	struct nlattr *attr;
>> >+
>> >+	nest = nla_nest_start(skb, attrtype);
>> >+	if (!nest)
>> >+		return -EMSGSIZE;
>> >+
>> >+	if (!mask && nla_put_flag(skb, ETHTOOL_A_BITSET_LIST))
>> 
>> Wait, shouldn't you rather check "!compact" ?
>> 
>> and WARN_ON in case compact == true && mask == NULL?
>
>The "compact" and "list" flags are orthogonal. In this function, caller
>passes null @mask if it wants to generated a list (as documented in the
>function description above). In some older version I had "bool is_list"
>which was set to "!mask" but I felt it didn't really make the code any
>simpler; I can return to that if you think it will make the code easier
>to read.
>
>> 
>> 
>> >+		goto nla_put_failure;
>> >+	if (nla_put_u32(skb, ETHTOOL_A_BITSET_SIZE, nbits))
>> >+		goto nla_put_failure;
>> >+	if (compact) {
>> >+		unsigned int nwords = DIV_ROUND_UP(nbits, 32);
>> >+		unsigned int nbytes = nwords * sizeof(u32);
>> >+		u32 *dst;
>> >+
>> >+		attr = nla_reserve(skb, ETHTOOL_A_BITSET_VALUE, nbytes);
>> >+		if (!attr)
>> >+			goto nla_put_failure;
>> >+		dst = nla_data(attr);
>> >+		memcpy(dst, val, nbytes);
>> >+		if (nbits % 32)
>> >+			dst[nwords - 1] &= __lower_bits(nbits);
>> >+
>> >+		if (mask) {
>> >+			attr = nla_reserve(skb, ETHTOOL_A_BITSET_MASK, nbytes);
>> >+			if (!attr)
>> >+				goto nla_put_failure;
>> >+			dst = nla_data(attr);
>> >+			memcpy(dst, mask, nbytes);
>> >+			if (nbits % 32)
>> >+				dst[nwords - 1] &= __lower_bits(nbits);
>> >+		}
>> >+	} else {
>> >+		struct nlattr *bits;
>> >+		unsigned int i;
>> >+
>> >+		bits = nla_nest_start(skb, ETHTOOL_A_BITSET_BITS);
>> >+		if (!bits)
>> >+			goto nla_put_failure;
>> >+		for (i = 0; i < nbits; i++) {
>> >+			const char *name = names ? names[i] : NULL;
>> >+
>> >+			if (!__bitmap32_test_bit(mask ?: val, i))
>> 
>> A) this __bitmap32_test_bit() looks like something generic, yet it is
>>    not. Perhaps you would want to add this helper to
>>    include/linux/bitmap.h?
>
>I'm not sure it would be appreciated there as the whole header file is
>for functions handling unsigned long based bitmaps. I'll rename it to
>make it obvious it's a local helper.
>
>> 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?


>
>> >+				continue;
>> >+			attr = nla_nest_start(skb, ETHTOOL_A_BITS_BIT);
>> >+			if (!attr ||
>> >+			    nla_put_u32(skb, ETHTOOL_A_BIT_INDEX, i))
>> 
>> You mix these 2 in 1 if which are not related. Better keep them separate
>> in two ifs.
>> Or you can put the rest of the puts in the same if too.
>
>OK
>
>> >+				goto nla_put_failure;
>> >+			if (name &&
>> >+			    ethnl_put_strz(skb, ETHTOOL_A_BIT_NAME, name))
>> >+				goto nla_put_failure;
>> >+			if (mask && __bitmap32_test_bit(val, i) &&
>> >+			    nla_put_flag(skb, ETHTOOL_A_BIT_VALUE))
>> >+				goto nla_put_failure;
>> >+			nla_nest_end(skb, attr);
>> >+		}
>> >+		nla_nest_end(skb, bits);
>> >+	}
>> >+
>> >+	nla_nest_end(skb, nest);
>> >+	return 0;
>> >+
>> >+nla_put_failure:
>> >+	nla_nest_cancel(skb, nest);
>> >+	return -EMSGSIZE;
>> >+}
>> >+
>> >+static const struct nla_policy bitset_policy[ETHTOOL_A_BITSET_MAX + 1] = {
>> >+	[ETHTOOL_A_BITSET_UNSPEC]	= { .type = NLA_REJECT },
>> >+	[ETHTOOL_A_BITSET_LIST]		= { .type = NLA_FLAG },
>> >+	[ETHTOOL_A_BITSET_SIZE]		= { .type = NLA_U32 },
>> >+	[ETHTOOL_A_BITSET_BITS]		= { .type = NLA_NESTED },
>> >+	[ETHTOOL_A_BITSET_VALUE]	= { .type = NLA_BINARY },
>> >+	[ETHTOOL_A_BITSET_MASK]		= { .type = NLA_BINARY },
>> >+};
>> >+
>> >+static const struct nla_policy bit_policy[ETHTOOL_A_BIT_MAX + 1] = {
>> >+	[ETHTOOL_A_BIT_UNSPEC]		= { .type = NLA_REJECT },
>> >+	[ETHTOOL_A_BIT_INDEX]		= { .type = NLA_U32 },
>> >+	[ETHTOOL_A_BIT_NAME]		= { .type = NLA_NUL_STRING },
>> >+	[ETHTOOL_A_BIT_VALUE]		= { .type = NLA_FLAG },
>> >+};
>> >+
>> >+/**
>> >+ * ethnl_bitset_is_compact() - check if bitset attribute represents a compact
>> >+ *			       bitset
>> >+ * @bitset  - nested attribute representing a bitset
>> >+ * @compact - pointer for return value
>> 
>> In the rest of the code, you use
>> @name: description
>
>Right, I'll fix this.
>
>> >+ *
>> >+ * Return: 0 on success, negative error code on failure
>> >+ */
>> >+int ethnl_bitset_is_compact(const struct nlattr *bitset, bool *compact)
>> >+{
>> >+	struct nlattr *tb[ETHTOOL_A_BITSET_MAX + 1];
>> >+	int ret;
>> >+
>> >+	ret = nla_parse_nested(tb, ETHTOOL_A_BITSET_MAX, bitset,
>> >+			       bitset_policy, NULL);
>> >+	if (ret < 0)
>> >+		return ret;
>> >+
>> >+	if (tb[ETHTOOL_A_BITSET_BITS]) {
>> >+		if (tb[ETHTOOL_A_BITSET_VALUE] || tb[ETHTOOL_A_BITSET_MASK])
>> >+			return -EINVAL;
>> >+		*compact = false;
>> >+		return 0;
>> >+	}
>> >+	if (!tb[ETHTOOL_A_BITSET_SIZE] || !tb[ETHTOOL_A_BITSET_VALUE])
>> >+		return -EINVAL;
>> >+
>> >+	*compact = true;
>> >+	return 0;
>> >+}
>> >+
>> >+static int ethnl_name_to_idx(ethnl_string_array_t names, unsigned int n_names,
>> >+			     const char *name, unsigned int name_len)
>> >+{
>> >+	unsigned int i;
>> >+
>> >+	if (!names)
>> >+		return n_names;
>> >+
>> >+	for (i = 0; i < n_names; i++) {
>> >+		const char *bname = names[i];
>> >+
>> >+		if (!strncmp(bname, name, name_len) &&
>> >+		    strlen(bname) <= name_len)
>> >+			return i;
>> >+	}
>> >+
>> >+	return n_names;
>> 
>> Maybe bettet to stick with -ERRNO?
>
>OK
>
>> >+}
>> >+
>> >+static int ethnl_parse_bit(unsigned int *index, bool *val, unsigned int nbits,
>> >+			   const struct nlattr *bit_attr, bool is_list,
>> >+			   ethnl_string_array_t names,
>> >+			   struct netlink_ext_ack *extack)
>> >+{
>> >+	struct nlattr *tb[ETHTOOL_A_BIT_MAX + 1];
>> >+	int ret, idx;
>> >+
>> >+	if (nla_type(bit_attr) != ETHTOOL_A_BITS_BIT) {
>> >+		NL_SET_ERR_MSG_ATTR(extack, bit_attr,
>> >+				    "only ETHTOOL_A_BITS_BIT allowed in ETHTOOL_A_BITSET_BITS");
>> >+		return -EINVAL;
>> >+	}
>> 
>> Probably it makes sense the caller does this check. Later on, if there
>> is another possible value, the check would have to go there anyway.
>
>OK
>
>> >+	ret = nla_parse_nested(tb, ETHTOOL_A_BIT_MAX, bit_attr, bit_policy,
>> >+			       extack);
>> >+	if (ret < 0)
>> >+		return ret;
>> >+
>> >+	if (tb[ETHTOOL_A_BIT_INDEX]) {
>> >+		const char *name;
>> >+
>> >+		idx = nla_get_u32(tb[ETHTOOL_A_BIT_INDEX]);
>> >+		if (idx >= nbits) {
>> >+			NL_SET_ERR_MSG_ATTR(extack,
>> >+					    tb[ETHTOOL_A_BIT_INDEX],
>> >+					    "bit index too high");
>> >+			return -EOPNOTSUPP;
>> >+		}
>> >+		name = names ? names[idx] : NULL;
>> >+		if (tb[ETHTOOL_A_BIT_NAME] && name &&
>> >+		    strncmp(nla_data(tb[ETHTOOL_A_BIT_NAME]), name,
>> >+			    nla_len(tb[ETHTOOL_A_BIT_NAME]))) {
>> >+			NL_SET_ERR_MSG_ATTR(extack, bit_attr,
>> >+					    "bit index and name mismatch");
>> >+			return -EINVAL;
>> >+		}
>> >+	} else if (tb[ETHTOOL_A_BIT_NAME]) {
>> >+		idx = ethnl_name_to_idx(names, nbits,
>> >+					nla_data(tb[ETHTOOL_A_BIT_NAME]),
>> >+					nla_len(tb[ETHTOOL_A_BIT_NAME]));
>> 
>> It's a string? Policy validation should take care if it is correctly
>> terminated by '\0'. Then you don't need to pass len down. Anyone who is
>> interested in length can use strlen().
>
>OK
>
>> >+	is_list = (tb[ETHTOOL_A_BITSET_LIST] != NULL);
>> 
>> just:
>> 	is_list = tb[ETHTOOL_A_BITSET_LIST]
>> is enough.
>
>Assignment from pointer to a bool felt a bit weird but if you find it
>acceptable, I have no problem with it.
>
>> >+int ethnl_bitset_is_compact(const struct nlattr *bitset, bool *compact);
>> >+int ethnl_bitset_size(const unsigned long *val, const unsigned long *mask,
>> >+		      unsigned int nbits, ethnl_string_array_t names,
>> >+		      bool compact);
>> >+int ethnl_bitset32_size(const u32 *val, const u32 *mask, unsigned int nbits,
>> >+			ethnl_string_array_t names, bool compact);
>> >+int ethnl_put_bitset(struct sk_buff *skb, int attrtype,
>> >+		     const unsigned long *val, const unsigned long *mask,
>> >+		     unsigned int nbits, ethnl_string_array_t names,
>> >+		     bool compact);
>> >+int ethnl_put_bitset32(struct sk_buff *skb, int attrtype, const u32 *val,
>> >+		       const u32 *mask, unsigned int nbits,
>> >+		       ethnl_string_array_t names, bool compact);
>> >+int ethnl_update_bitset(unsigned long *bitmap, unsigned int nbits,
>> >+			const struct nlattr *attr, ethnl_string_array_t names,
>> >+			struct netlink_ext_ack *extack, bool *mod);
>> >+int ethnl_update_bitset32(u32 *bitmap, unsigned int nbits,
>> >+			  const struct nlattr *attr, ethnl_string_array_t names,
>> >+			  struct netlink_ext_ack *extack, bool *mod);
>> 
>> Hmm, I wonder why user needs to work with the 32 variants..
>
>See above.
>
>Michal

Powered by blists - more mailing lists