* [PATCH net-next 1/3] tools: ynl-gen: make the mnl_type() method public
2023-10-18 21:39 [PATCH net-next 0/3] netlink: add variable-length / auto integers Jakub Kicinski
@ 2023-10-18 21:39 ` Jakub Kicinski
2023-10-18 21:39 ` [PATCH net-next 2/3] netlink: add variable-length / auto integers Jakub Kicinski
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2023-10-18 21:39 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, Jakub Kicinski
uint/sint support will add more logic to mnl_type(),
deduplicate it and make it more accessible.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
tools/net/ynl/ynl-gen-c.py | 38 ++++++++++++++++++--------------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 552ba49a444c..6f4c538bda9a 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -159,6 +159,15 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
spec = self._attr_policy(policy)
cw.p(f"\t[{self.enum_name}] = {spec},")
+ def _mnl_type(self):
+ # mnl does not have helpers for signed integer types
+ # turn signed type into unsigned
+ # this only makes sense for scalar types
+ t = self.type
+ if t[0] == 's':
+ t = 'u' + t[1:]
+ return t
+
def _attr_typol(self):
raise Exception(f"Type policy not implemented for class type {self.type}")
@@ -329,12 +338,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
else:
self.type_name = '__' + self.type
- def _mnl_type(self):
- t = self.type
- # mnl does not have a helper for signed types
- if t[0] == 's':
- t = 'u' + t[1:]
- return t
+ def mnl_type(self):
+ return self._mnl_type()
def _attr_policy(self, policy):
if 'flags-mask' in self.checks or self.is_bitfield:
@@ -363,10 +368,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
return [f'{self.type_name} {self.c_name}{self.byte_order_comment}']
def attr_put(self, ri, var):
- self._attr_put_simple(ri, var, self._mnl_type())
+ self._attr_put_simple(ri, var, self.mnl_type())
def _attr_get(self, ri, var):
- return f"{var}->{self.c_name} = mnl_attr_get_{self._mnl_type()}(attr);", None, None
+ return f"{var}->{self.c_name} = mnl_attr_get_{self.mnl_type()}(attr);", None, None
def _setter_lines(self, ri, member, presence):
return [f"{member} = {self.c_name};"]
@@ -524,12 +529,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
def presence_type(self):
return 'count'
- def _mnl_type(self):
- t = self.type
- # mnl does not have a helper for signed types
- if t[0] == 's':
- t = 'u' + t[1:]
- return t
+ def mnl_type(self):
+ return self._mnl_type()
def _complex_member_type(self, ri):
if 'type' not in self.attr or self.attr['type'] == 'nest':
@@ -564,7 +565,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
def attr_put(self, ri, var):
if self.attr['type'] in scalars:
- put_type = self._mnl_type()
+ put_type = self.mnl_type()
ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
ri.cw.p(f"mnl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
elif 'type' not in self.attr or self.attr['type'] == 'nest':
@@ -1580,11 +1581,8 @@ _C_KW = {
ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr))")
ri.cw.p('return MNL_CB_ERROR;')
- elif aspec['type'] in scalars:
- t = aspec['type']
- if t[0] == 's':
- t = 'u' + t[1:]
- ri.cw.p(f"dst->{aspec.c_name}[i] = mnl_attr_get_{t}(attr);")
+ elif aspec.type in scalars:
+ ri.cw.p(f"dst->{aspec.c_name}[i] = mnl_attr_get_{aspec.mnl_type()}(attr);")
else:
raise Exception('Nest parsing type not supported yet')
ri.cw.p('i++;')
--
2.41.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH net-next 2/3] netlink: add variable-length / auto integers
2023-10-18 21:39 [PATCH net-next 0/3] netlink: add variable-length / auto integers Jakub Kicinski
2023-10-18 21:39 ` [PATCH net-next 1/3] tools: ynl-gen: make the mnl_type() method public Jakub Kicinski
@ 2023-10-18 21:39 ` Jakub Kicinski
2023-10-20 7:24 ` Nicolas Dichtel
2023-10-18 21:39 ` [PATCH net-next 3/3] netlink: specs: add support for auto-sized scalars Jakub Kicinski
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2023-10-18 21:39 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, Jakub Kicinski, johannes,
nicolas.dichtel, stephen, jiri
We currently push everyone to use padding to align 64b values
in netlink. Un-padded nla_put_u64() doesn't even exist any more.
The story behind this possibly start with this thread:
https://lore.kernel.org/netdev/20121204.130914.1457976839967676240.davem@davemloft.net/
where DaveM was concerned about the alignment of a structure
containing 64b stats. If user space tries to access such struct
directly:
struct some_stats *stats = nla_data(attr);
printf("A: %llu", stats->a);
lack of alignment may become problematic for some architectures.
These days we most often put every single member in a separate
attribute, meaning that the code above would use a helper like
nla_get_u64(), which can deal with alignment internally.
Even for arches which don't have good unaligned access - access
aligned to 4B should be pretty efficient.
Kernel and well known libraries deal with unaligned input already.
Padded 64b is quite space-inefficient (64b + pad means at worst 16B
per attr vs 32b which takes 8B). It is also more typing:
if (nla_put_u64_pad(rsp, NETDEV_A_SOMETHING_SOMETHING,
value, NETDEV_A_SOMETHING_PAD))
Create a new attribute type which will use 32 bits at netlink
level if value is small enough (probably most of the time?),
and (4B-aligned) 64 bits otherwise. Kernel API is just:
if (nla_put_uint(rsp, NETDEV_A_SOMETHING_SOMETHING, value))
Calling this new type "just" sint / uint with no specific size
will hopefully also make people more comfortable with using it.
Currently telling people "don't use u8, you may need the bits,
and netlink will round up to 4B, anyway" is the #1 comment
we give to newcomers.
In terms of netlink layout it looks like this:
0 4 8 12 16
32b: [nlattr][ u32 ]
64b: [ pad ][nlattr][ u64 ]
uint(32) [nlattr][ u32 ]
uint(64) [nlattr][ u64 ]
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v1:
- fill in missed nlattr support
- add docs
- fix build (missed commit -a --amend..)
RFC: https://lore.kernel.org/all/20231011003313.105315-1-kuba@kernel.org/
CC: johannes@sipsolutions.net
CC: nicolas.dichtel@6wind.com
CC: stephen@networkplumber.org
CC: jiri@resnulli.us
---
Documentation/userspace-api/netlink/specs.rst | 18 ++++-
include/net/netlink.h | 69 ++++++++++++++++++-
include/uapi/linux/netlink.h | 5 ++
lib/nlattr.c | 22 ++++++
net/netlink/policy.c | 14 +++-
5 files changed, 121 insertions(+), 7 deletions(-)
diff --git a/Documentation/userspace-api/netlink/specs.rst b/Documentation/userspace-api/netlink/specs.rst
index 40dd7442d2c3..c1b951649113 100644
--- a/Documentation/userspace-api/netlink/specs.rst
+++ b/Documentation/userspace-api/netlink/specs.rst
@@ -403,10 +403,21 @@ This section describes the attribute types supported by the ``genetlink``
compatibility level. Refer to documentation of different levels for additional
attribute types.
-Scalar integer types
+Common integer types
--------------------
-Fixed-width integer types:
+``sint`` and ``uint`` represent signed and unsigned 64 bit integers.
+If the value can fit on 32 bits only 32 bits are carried in netlink
+messages, otherwise full 64 bits are carried. Note that the payload
+is only aligned to 4B, so the full 64 bit value may be unaligned!
+
+Common integer types should be preferred over fix-width types in majority
+of cases.
+
+Fix-width integer types
+-----------------------
+
+Fixed-width integer types include:
``u8``, ``u16``, ``u32``, ``u64``, ``s8``, ``s16``, ``s32``, ``s64``.
Note that types smaller than 32 bit should be avoided as using them
@@ -416,6 +427,9 @@ See :ref:`pad_type` for padding of 64 bit attributes.
The payload of the attribute is the integer in host order unless ``byte-order``
specifies otherwise.
+64 bit values are usually aligned by the kernel but it is recommended
+that the user space is able to deal with unaligned values.
+
.. _pad_type:
pad
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 8a7cd1170e1f..aba2b162a226 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -128,6 +128,8 @@
* nla_len(nla) length of attribute payload
*
* Attribute Payload Access for Basic Types:
+ * nla_get_uint(nla) get payload for a uint attribute
+ * nla_get_sint(nla) get payload for a sint attribute
* nla_get_u8(nla) get payload for a u8 attribute
* nla_get_u16(nla) get payload for a u16 attribute
* nla_get_u32(nla) get payload for a u32 attribute
@@ -183,6 +185,8 @@ enum {
NLA_REJECT,
NLA_BE16,
NLA_BE32,
+ NLA_SINT,
+ NLA_UINT,
__NLA_TYPE_MAX,
};
@@ -229,6 +233,7 @@ enum nla_policy_validation {
* nested header (or empty); len field is used if
* nested_policy is also used, for the max attr
* number in the nested policy.
+ * NLA_SINT, NLA_UINT,
* NLA_U8, NLA_U16,
* NLA_U32, NLA_U64,
* NLA_S8, NLA_S16,
@@ -260,12 +265,14 @@ enum nla_policy_validation {
* while an array has the nested attributes at another
* level down and the attribute types directly in the
* nesting don't matter.
+ * NLA_UINT,
* NLA_U8,
* NLA_U16,
* NLA_U32,
* NLA_U64,
* NLA_BE16,
* NLA_BE32,
+ * NLA_SINT,
* NLA_S8,
* NLA_S16,
* NLA_S32,
@@ -280,6 +287,7 @@ enum nla_policy_validation {
* or NLA_POLICY_FULL_RANGE_SIGNED() macros instead.
* Use the NLA_POLICY_MIN(), NLA_POLICY_MAX() and
* NLA_POLICY_RANGE() macros.
+ * NLA_UINT,
* NLA_U8,
* NLA_U16,
* NLA_U32,
@@ -288,6 +296,7 @@ enum nla_policy_validation {
* to a struct netlink_range_validation that indicates
* the min/max values.
* Use NLA_POLICY_FULL_RANGE().
+ * NLA_SINT,
* NLA_S8,
* NLA_S16,
* NLA_S32,
@@ -377,9 +386,11 @@ struct nla_policy {
#define __NLA_IS_UINT_TYPE(tp) \
(tp == NLA_U8 || tp == NLA_U16 || tp == NLA_U32 || \
- tp == NLA_U64 || tp == NLA_BE16 || tp == NLA_BE32)
+ tp == NLA_U64 || tp == NLA_UINT || \
+ tp == NLA_BE16 || tp == NLA_BE32)
#define __NLA_IS_SINT_TYPE(tp) \
- (tp == NLA_S8 || tp == NLA_S16 || tp == NLA_S32 || tp == NLA_S64)
+ (tp == NLA_S8 || tp == NLA_S16 || tp == NLA_S32 || tp == NLA_S64 || \
+ tp == NLA_SINT)
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
#define NLA_ENSURE_UINT_TYPE(tp) \
@@ -1357,6 +1368,22 @@ static inline int nla_put_u32(struct sk_buff *skb, int attrtype, u32 value)
return nla_put(skb, attrtype, sizeof(u32), &tmp);
}
+/**
+ * nla_put_uint - Add a variable-size unsigned int to a socket buffer
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @value: numeric value
+ */
+static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
+{
+ u64 tmp64 = value;
+ u32 tmp32 = value;
+
+ if (tmp64 == tmp32)
+ return nla_put_u32(skb, attrtype, tmp32);
+ return nla_put(skb, attrtype, sizeof(u64), &tmp64);
+}
+
/**
* nla_put_be32 - Add a __be32 netlink attribute to a socket buffer
* @skb: socket buffer to add attribute to
@@ -1511,6 +1538,22 @@ static inline int nla_put_s64(struct sk_buff *skb, int attrtype, s64 value,
return nla_put_64bit(skb, attrtype, sizeof(s64), &tmp, padattr);
}
+/**
+ * nla_put_sint - Add a variable-size signed int to a socket buffer
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @value: numeric value
+ */
+static inline int nla_put_sint(struct sk_buff *skb, int attrtype, s64 value)
+{
+ s64 tmp64 = value;
+ s32 tmp32 = value;
+
+ if (tmp64 == tmp32)
+ return nla_put_s32(skb, attrtype, tmp32);
+ return nla_put(skb, attrtype, sizeof(s64), &tmp64);
+}
+
/**
* nla_put_string - Add a string netlink attribute to a socket buffer
* @skb: socket buffer to add attribute to
@@ -1667,6 +1710,17 @@ static inline u64 nla_get_u64(const struct nlattr *nla)
return tmp;
}
+/**
+ * nla_get_uint - return payload of uint attribute
+ * @nla: uint netlink attribute
+ */
+static inline u64 nla_get_uint(const struct nlattr *nla)
+{
+ if (nla_len(nla) == sizeof(u32))
+ return nla_get_u32(nla);
+ return nla_get_u64(nla);
+}
+
/**
* nla_get_be64 - return payload of __be64 attribute
* @nla: __be64 netlink attribute
@@ -1729,6 +1783,17 @@ static inline s64 nla_get_s64(const struct nlattr *nla)
return tmp;
}
+/**
+ * nla_get_sint - return payload of uint attribute
+ * @nla: uint netlink attribute
+ */
+static inline s64 nla_get_sint(const struct nlattr *nla)
+{
+ if (nla_len(nla) == sizeof(s32))
+ return nla_get_s32(nla);
+ return nla_get_s64(nla);
+}
+
/**
* nla_get_flag - return payload of flag attribute
* @nla: flag netlink attribute
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index e2ae82e3f9f7..f87aaf28a649 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -298,6 +298,8 @@ struct nla_bitfield32 {
* entry has attributes again, the policy for those inner ones
* and the corresponding maxtype may be specified.
* @NL_ATTR_TYPE_BITFIELD32: &struct nla_bitfield32 attribute
+ * @NL_ATTR_TYPE_SINT: 32-bit or 64-bit signed attribute, aligned to 4B
+ * @NL_ATTR_TYPE_UINT: 32-bit or 64-bit unsigned attribute, aligned to 4B
*/
enum netlink_attribute_type {
NL_ATTR_TYPE_INVALID,
@@ -322,6 +324,9 @@ enum netlink_attribute_type {
NL_ATTR_TYPE_NESTED_ARRAY,
NL_ATTR_TYPE_BITFIELD32,
+
+ NL_ATTR_TYPE_SINT,
+ NL_ATTR_TYPE_UINT,
};
/**
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 7a2b6c38fd59..dc15e7888fc1 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -134,6 +134,7 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
range->max = U32_MAX;
break;
case NLA_U64:
+ case NLA_UINT:
case NLA_MSECS:
range->max = U64_MAX;
break;
@@ -183,6 +184,9 @@ static int nla_validate_range_unsigned(const struct nla_policy *pt,
case NLA_U64:
value = nla_get_u64(nla);
break;
+ case NLA_UINT:
+ value = nla_get_uint(nla);
+ break;
case NLA_MSECS:
value = nla_get_u64(nla);
break;
@@ -248,6 +252,7 @@ void nla_get_range_signed(const struct nla_policy *pt,
range->max = S32_MAX;
break;
case NLA_S64:
+ case NLA_SINT:
range->min = S64_MIN;
range->max = S64_MAX;
break;
@@ -295,6 +300,9 @@ static int nla_validate_int_range_signed(const struct nla_policy *pt,
case NLA_S64:
value = nla_get_s64(nla);
break;
+ case NLA_SINT:
+ value = nla_get_sint(nla);
+ break;
default:
return -EINVAL;
}
@@ -320,6 +328,7 @@ static int nla_validate_int_range(const struct nla_policy *pt,
case NLA_U16:
case NLA_U32:
case NLA_U64:
+ case NLA_UINT:
case NLA_MSECS:
case NLA_BINARY:
case NLA_BE16:
@@ -329,6 +338,7 @@ static int nla_validate_int_range(const struct nla_policy *pt,
case NLA_S16:
case NLA_S32:
case NLA_S64:
+ case NLA_SINT:
return nla_validate_int_range_signed(pt, nla, extack);
default:
WARN_ON(1);
@@ -355,6 +365,9 @@ static int nla_validate_mask(const struct nla_policy *pt,
case NLA_U64:
value = nla_get_u64(nla);
break;
+ case NLA_UINT:
+ value = nla_get_uint(nla);
+ break;
case NLA_BE16:
value = ntohs(nla_get_be16(nla));
break;
@@ -433,6 +446,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
goto out_err;
break;
+ case NLA_SINT:
+ case NLA_UINT:
+ if (attrlen != sizeof(u32) && attrlen != sizeof(u64)) {
+ NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+ "invalid attribute length");
+ return -EINVAL;
+ }
+ break;
+
case NLA_BITFIELD32:
if (attrlen != sizeof(struct nla_bitfield32))
goto out_err;
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index e2f111edf66c..1f8909c16f14 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -230,6 +230,8 @@ int netlink_policy_dump_attr_size_estimate(const struct nla_policy *pt)
case NLA_S16:
case NLA_S32:
case NLA_S64:
+ case NLA_SINT:
+ case NLA_UINT:
/* maximum is common, u64 min/max with padding */
return common +
2 * (nla_attr_size(0) + nla_attr_size(sizeof(u64)));
@@ -288,6 +290,7 @@ __netlink_policy_dump_write_attr(struct netlink_policy_dump_state *state,
case NLA_U16:
case NLA_U32:
case NLA_U64:
+ case NLA_UINT:
case NLA_MSECS: {
struct netlink_range_validation range;
@@ -297,8 +300,10 @@ __netlink_policy_dump_write_attr(struct netlink_policy_dump_state *state,
type = NL_ATTR_TYPE_U16;
else if (pt->type == NLA_U32)
type = NL_ATTR_TYPE_U32;
- else
+ else if (pt->type == NLA_U64)
type = NL_ATTR_TYPE_U64;
+ else
+ type = NL_ATTR_TYPE_UINT;
if (pt->validation_type == NLA_VALIDATE_MASK) {
if (nla_put_u64_64bit(skb, NL_POLICY_TYPE_ATTR_MASK,
@@ -320,7 +325,8 @@ __netlink_policy_dump_write_attr(struct netlink_policy_dump_state *state,
case NLA_S8:
case NLA_S16:
case NLA_S32:
- case NLA_S64: {
+ case NLA_S64:
+ case NLA_SINT: {
struct netlink_range_validation_signed range;
if (pt->type == NLA_S8)
@@ -329,8 +335,10 @@ __netlink_policy_dump_write_attr(struct netlink_policy_dump_state *state,
type = NL_ATTR_TYPE_S16;
else if (pt->type == NLA_S32)
type = NL_ATTR_TYPE_S32;
- else
+ else if (pt->type == NLA_S64)
type = NL_ATTR_TYPE_S64;
+ else
+ type = NL_ATTR_TYPE_SINT;
nla_get_range_signed(pt, &range);
--
2.41.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net-next 2/3] netlink: add variable-length / auto integers
2023-10-18 21:39 ` [PATCH net-next 2/3] netlink: add variable-length / auto integers Jakub Kicinski
@ 2023-10-20 7:24 ` Nicolas Dichtel
0 siblings, 0 replies; 8+ messages in thread
From: Nicolas Dichtel @ 2023-10-20 7:24 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, edumazet, pabeni, johannes, stephen, jiri
Le 18/10/2023 à 23:39, Jakub Kicinski a écrit :
> We currently push everyone to use padding to align 64b values
> in netlink. Un-padded nla_put_u64() doesn't even exist any more.
>
> The story behind this possibly start with this thread:
> https://lore.kernel.org/netdev/20121204.130914.1457976839967676240.davem@davemloft.net/
> where DaveM was concerned about the alignment of a structure
> containing 64b stats. If user space tries to access such struct
> directly:
>
> struct some_stats *stats = nla_data(attr);
> printf("A: %llu", stats->a);
>
> lack of alignment may become problematic for some architectures.
> These days we most often put every single member in a separate
> attribute, meaning that the code above would use a helper like
> nla_get_u64(), which can deal with alignment internally.
> Even for arches which don't have good unaligned access - access
> aligned to 4B should be pretty efficient.
> Kernel and well known libraries deal with unaligned input already.
>
> Padded 64b is quite space-inefficient (64b + pad means at worst 16B
> per attr vs 32b which takes 8B). It is also more typing:
>
> if (nla_put_u64_pad(rsp, NETDEV_A_SOMETHING_SOMETHING,
> value, NETDEV_A_SOMETHING_PAD))
>
> Create a new attribute type which will use 32 bits at netlink
> level if value is small enough (probably most of the time?),
> and (4B-aligned) 64 bits otherwise. Kernel API is just:
>
> if (nla_put_uint(rsp, NETDEV_A_SOMETHING_SOMETHING, value))
>
> Calling this new type "just" sint / uint with no specific size
> will hopefully also make people more comfortable with using it.
> Currently telling people "don't use u8, you may need the bits,
> and netlink will round up to 4B, anyway" is the #1 comment
> we give to newcomers.
>
> In terms of netlink layout it looks like this:
>
> 0 4 8 12 16
> 32b: [nlattr][ u32 ]
> 64b: [ pad ][nlattr][ u64 ]
> uint(32) [nlattr][ u32 ]
> uint(64) [nlattr][ u64 ]
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 3/3] netlink: specs: add support for auto-sized scalars
2023-10-18 21:39 [PATCH net-next 0/3] netlink: add variable-length / auto integers Jakub Kicinski
2023-10-18 21:39 ` [PATCH net-next 1/3] tools: ynl-gen: make the mnl_type() method public Jakub Kicinski
2023-10-18 21:39 ` [PATCH net-next 2/3] netlink: add variable-length / auto integers Jakub Kicinski
@ 2023-10-18 21:39 ` Jakub Kicinski
2023-10-20 7:44 ` Nicolas Dichtel
2023-10-20 7:24 ` [PATCH net-next 0/3] netlink: add variable-length / auto integers Nicolas Dichtel
2023-10-20 22:35 ` Jakub Kicinski
4 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2023-10-18 21:39 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, Jakub Kicinski
Support uint / sint types in specs and YNL.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Documentation/netlink/genetlink-c.yaml | 3 ++-
Documentation/netlink/genetlink-legacy.yaml | 3 ++-
Documentation/netlink/genetlink.yaml | 3 ++-
tools/net/ynl/lib/nlspec.py | 6 ++++++
tools/net/ynl/lib/ynl.c | 6 ++++++
tools/net/ynl/lib/ynl.h | 17 +++++++++++++++++
tools/net/ynl/lib/ynl.py | 14 ++++++++++++++
tools/net/ynl/ynl-gen-c.py | 6 ++++--
8 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml
index dee11c514896..c72c8a428911 100644
--- a/Documentation/netlink/genetlink-c.yaml
+++ b/Documentation/netlink/genetlink-c.yaml
@@ -149,7 +149,8 @@ additionalProperties: False
name:
type: string
type: &attr-type
- enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
+ enum: [ unused, pad, flag, binary,
+ uint, sint, u8, u16, u32, u64, s32, s64,
string, nest, array-nest, nest-type-value ]
doc:
description: Documentation of the attribute.
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 9194f3e223ef..923de0ff1a9e 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -192,7 +192,8 @@ additionalProperties: False
type: string
type: &attr-type
description: The netlink attribute type
- enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
+ enum: [ unused, pad, flag, binary,
+ uint, sint, u8, u16, u32, u64, s32, s64,
string, nest, array-nest, nest-type-value ]
doc:
description: Documentation of the attribute.
diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml
index 0a4ae861d011..9ceb096b2df2 100644
--- a/Documentation/netlink/genetlink.yaml
+++ b/Documentation/netlink/genetlink.yaml
@@ -122,7 +122,8 @@ additionalProperties: False
name:
type: string
type: &attr-type
- enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
+ enum: [ unused, pad, flag, binary,
+ uint, sint, u8, u16, u32, u64, s32, s64,
string, nest, array-nest, nest-type-value ]
doc:
description: Documentation of the attribute.
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 37bcb4d8b37b..92889298b197 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -149,6 +149,7 @@ jsonschema = None
Represents a single attribute type within an attr space.
Attributes:
+ type string, attribute type
value numerical ID when serialized
attr_set Attribute Set containing this attr
is_multi bool, attr may repeat multiple times
@@ -157,10 +158,13 @@ jsonschema = None
len integer, optional byte length of binary types
display_hint string, hint to help choose format specifier
when displaying the value
+
+ is_auto_scalar bool, attr is a variable-size scalar
"""
def __init__(self, family, attr_set, yaml, value):
super().__init__(family, yaml)
+ self.type = yaml['type']
self.value = value
self.attr_set = attr_set
self.is_multi = yaml.get('multi-attr', False)
@@ -170,6 +174,8 @@ jsonschema = None
self.len = yaml.get('len')
self.display_hint = yaml.get('display-hint')
+ self.is_auto_scalar = self.type == "sint" or self.type == "uint"
+
class SpecAttrSet(SpecElement):
""" Netlink Attribute Set class.
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 514e0d69e731..350ddc247450 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -352,6 +352,12 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
yerr(yarg->ys, YNL_ERROR_ATTR_INVALID,
"Invalid attribute (u64 %s)", policy->name);
return -1;
+ case YNL_PT_UINT:
+ if (len == sizeof(__u32) || len == sizeof(__u64))
+ break;
+ yerr(yarg->ys, YNL_ERROR_ATTR_INVALID,
+ "Invalid attribute (uint %s)", policy->name);
+ return -1;
case YNL_PT_FLAG:
/* Let flags grow into real attrs, why not.. */
break;
diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
index 9eafa3552c16..87b4dad832f0 100644
--- a/tools/net/ynl/lib/ynl.h
+++ b/tools/net/ynl/lib/ynl.h
@@ -133,6 +133,7 @@ enum ynl_policy_type {
YNL_PT_U16,
YNL_PT_U32,
YNL_PT_U64,
+ YNL_PT_UINT,
YNL_PT_NUL_STR,
};
@@ -234,4 +235,20 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
void ynl_error_unknown_notification(struct ynl_sock *ys, __u8 cmd);
int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
+#ifndef MNL_HAS_AUTO_SCALARS
+static inline uint64_t mnl_attr_get_uint(const struct nlattr *attr)
+{
+ if (mnl_attr_get_len(attr) == 4)
+ return mnl_attr_get_u32(attr);
+ return mnl_attr_get_u64(attr);
+}
+
+static inline void
+mnl_attr_put_uint(struct nlmsghdr *nlh, uint16_t type, uint64_t data)
+{
+ if ((uint32_t)data == (uint64_t)data)
+ return mnl_attr_put_u32(nlh, type, data);
+ return mnl_attr_put_u64(nlh, type, data);
+}
+#endif
#endif
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 28ac35008e65..3b36553a66cc 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -130,6 +130,13 @@ from .nlspec import SpecFamily
format = self.get_format(attr_type, byte_order)
return format.unpack(self.raw)[0]
+ def as_auto_scalar(self, attr_type, byte_order=None):
+ if len(self.raw) != 4 and len(self.raw) != 8:
+ raise Exception(f"Auto-scalar len payload be 4 or 8 bytes, got {len(self.raw)}")
+ real_type = attr_type[0] + str(len(self.raw) * 8)
+ format = self.get_format(real_type, byte_order)
+ return format.unpack(self.raw)[0]
+
def as_strz(self):
return self.raw.decode('ascii')[:-1]
@@ -463,6 +470,11 @@ genl_family_name_to_id = None
attr_payload = bytes.fromhex(value)
else:
raise Exception(f'Unknown type for binary attribute, value: {value}')
+ elif attr.is_auto_scalar:
+ scalar = int(value)
+ real_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
+ format = NlAttr.get_format(real_type, attr.byte_order)
+ attr_payload = format.pack(int(value))
elif attr['type'] in NlAttr.type_formats:
format = NlAttr.get_format(attr['type'], attr.byte_order)
attr_payload = format.pack(int(value))
@@ -529,6 +541,8 @@ genl_family_name_to_id = None
decoded = self._decode_binary(attr, attr_spec)
elif attr_spec["type"] == 'flag':
decoded = True
+ elif attr_spec.is_auto_scalar:
+ decoded = attr.as_auto_scalar(attr_spec['type'], attr_spec.byte_order)
elif attr_spec["type"] in NlAttr.type_formats:
decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
elif attr_spec["type"] == 'array-nest':
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 6f4c538bda9a..a9e8898c9386 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -335,6 +335,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
maybe_enum = not self.is_bitfield and 'enum' in self.attr
if maybe_enum and self.family.consts[self.attr['enum']].enum_name:
self.type_name = f"enum {self.family.name}_{c_lower(self.attr['enum'])}"
+ elif self.is_auto_scalar:
+ self.type_name = '__' + self.type[0] + '64'
else:
self.type_name = '__' + self.type
@@ -362,7 +364,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
return super()._attr_policy(policy)
def _attr_typol(self):
- return f'.type = YNL_PT_U{self.type[1:]}, '
+ return f'.type = YNL_PT_U{c_upper(self.type[1:])}, '
def arg_member(self, ri):
return [f'{self.type_name} {self.c_name}{self.byte_order_comment}']
@@ -1291,7 +1293,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
self.p(line)
-scalars = {'u8', 'u16', 'u32', 'u64', 's32', 's64'}
+scalars = {'u8', 'u16', 'u32', 'u64', 's32', 's64', 'uint', 'sint'}
direction_to_suffix = {
'reply': '_rsp',
--
2.41.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net-next 3/3] netlink: specs: add support for auto-sized scalars
2023-10-18 21:39 ` [PATCH net-next 3/3] netlink: specs: add support for auto-sized scalars Jakub Kicinski
@ 2023-10-20 7:44 ` Nicolas Dichtel
0 siblings, 0 replies; 8+ messages in thread
From: Nicolas Dichtel @ 2023-10-20 7:44 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, edumazet, pabeni
Le 18/10/2023 à 23:39, Jakub Kicinski a écrit :
> Support uint / sint types in specs and YNL.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
I'm not a ynl expert, but FWIW:
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
[snip]
> +#ifndef MNL_HAS_AUTO_SCALARS
Is there a libmnl patch in flight?
> +static inline uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> +{
> + if (mnl_attr_get_len(attr) == 4)
> + return mnl_attr_get_u32(attr);
> + return mnl_attr_get_u64(attr);
> +}
> +
> +static inline void
> +mnl_attr_put_uint(struct nlmsghdr *nlh, uint16_t type, uint64_t data)
> +{
> + if ((uint32_t)data == (uint64_t)data)
> + return mnl_attr_put_u32(nlh, type, data);
> + return mnl_attr_put_u64(nlh, type, data);
> +}
> +#endif
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 0/3] netlink: add variable-length / auto integers
2023-10-18 21:39 [PATCH net-next 0/3] netlink: add variable-length / auto integers Jakub Kicinski
` (2 preceding siblings ...)
2023-10-18 21:39 ` [PATCH net-next 3/3] netlink: specs: add support for auto-sized scalars Jakub Kicinski
@ 2023-10-20 7:24 ` Nicolas Dichtel
2023-10-20 22:35 ` Jakub Kicinski
4 siblings, 0 replies; 8+ messages in thread
From: Nicolas Dichtel @ 2023-10-20 7:24 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, edumazet, pabeni
Le 18/10/2023 à 23:39, Jakub Kicinski a écrit :
> Add netlink support for "common" / variable-length / auto integers
> which are carried at the message level as either 4B or 8B depending
> on the exact value. This saves space and will hopefully decrease
> the number of instances where we realize that we needed more bits
> after uAPI is set is stone. It also loosens the alignment requirements,
> avoiding the need for padding.
>
> This mini-series is a fuller version of the previous RFC:
> https://lore.kernel.org/netdev/20121204.130914.1457976839967676240.davem@davemloft.net/
Probably https://lore.kernel.org/all/20231011003313.105315-1-kuba@kernel.org/ ;-)
Nicolas
> No user included here. I have tested (and will use) it
> in the upcoming page pool API but the assumption is that
> it will be widely applicable. So sending without a user.
>
> Jakub Kicinski (3):
> tools: ynl-gen: make the mnl_type() method public
> netlink: add variable-length / auto integers
> netlink: specs: add support for auto-sized scalars
>
> Documentation/netlink/genetlink-c.yaml | 3 +-
> Documentation/netlink/genetlink-legacy.yaml | 3 +-
> Documentation/netlink/genetlink.yaml | 3 +-
> Documentation/userspace-api/netlink/specs.rst | 18 ++++-
> include/net/netlink.h | 69 ++++++++++++++++++-
> include/uapi/linux/netlink.h | 5 ++
> lib/nlattr.c | 22 ++++++
> net/netlink/policy.c | 14 +++-
> tools/net/ynl/lib/nlspec.py | 6 ++
> tools/net/ynl/lib/ynl.c | 6 ++
> tools/net/ynl/lib/ynl.h | 17 +++++
> tools/net/ynl/lib/ynl.py | 14 ++++
> tools/net/ynl/ynl-gen-c.py | 44 ++++++------
> 13 files changed, 192 insertions(+), 32 deletions(-)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next 0/3] netlink: add variable-length / auto integers
2023-10-18 21:39 [PATCH net-next 0/3] netlink: add variable-length / auto integers Jakub Kicinski
` (3 preceding siblings ...)
2023-10-20 7:24 ` [PATCH net-next 0/3] netlink: add variable-length / auto integers Nicolas Dichtel
@ 2023-10-20 22:35 ` Jakub Kicinski
4 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2023-10-20 22:35 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, pabeni
On Wed, 18 Oct 2023 14:39:18 -0700 Jakub Kicinski wrote:
> Add netlink support for "common" / variable-length / auto integers
> which are carried at the message level as either 4B or 8B depending
> on the exact value. This saves space and will hopefully decrease
> the number of instances where we realize that we needed more bits
> after uAPI is set is stone. It also loosens the alignment requirements,
> avoiding the need for padding.
>
> This mini-series is a fuller version of the previous RFC:
> https://lore.kernel.org/netdev/20121204.130914.1457976839967676240.davem@davemloft.net/
> No user included here. I have tested (and will use) it
> in the upcoming page pool API but the assumption is that
> it will be widely applicable. So sending without a user.
For the record looks like DaveM merged this last night.
^ permalink raw reply [flat|nested] 8+ messages in thread