Messages in this thread | | | Date | Mon, 14 Oct 2019 15:02:05 +0200 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v7 06/17] ethtool: netlink bitset handling |
| |
Mon, Oct 14, 2019 at 01:18:47PM CEST, mkubecek@suse.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@suse.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
| |