Re: [RFC] netlink: add variable-length / auto integers

[Jiri Pirko <jiri@resnulli.us>]

Wed, Oct 11, 2023 at 02:33:13AM CEST, kuba@kernel.org wrote:
>We currently push everyone to use padding to align 64b values in netlink.
>I'm not sure what the story behind this is. I found this:
>https://lore.kernel.org/all/1461339084-3849-1-git-send-email-nicolas.dichtel@6wind.com/#t
>but it doesn't go into details WRT the motivation.
>Even for arches which don't have good unaligned access - I'd think
>that access aligned to 4B *is* pretty efficient, and that's all
>we need. Plus kernel deals with unaligned input. Why can't user space?
>
>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 space,
>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>
>---
>Thoughts?

Hmm, I assume that genetlink.yaml schema should only allow uint and sint
to be defined after this, so new genetlink implementations use just uint
and sint, correct?

Than we have genetlink.yaml genetlink-legacy.yaml genetlink-legacy2.yaml
?
I guess in the future there might be other changes to require new
implemetation not to use legacy things. How does this scale?

+ 2 nits below



>
>This is completely untested. YNL to follow.
>---
> include/net/netlink.h        | 62 ++++++++++++++++++++++++++++++++++--
> include/uapi/linux/netlink.h |  5 +++
> lib/nlattr.c                 |  9 ++++++
> net/netlink/policy.c         | 14 ++++++--
> 4 files changed, 85 insertions(+), 5 deletions(-)
>
>diff --git a/include/net/netlink.h b/include/net/netlink.h
>index 8a7cd1170e1f..523486dfe4f3 100644
>--- a/include/net/netlink.h
>+++ b/include/net/netlink.h
>@@ -183,6 +183,8 @@ enum {
> 	NLA_REJECT,
> 	NLA_BE16,
> 	NLA_BE32,
>+	NLA_SINT,

Why not just NLA_INT?


>+	NLA_UINT,
> 	__NLA_TYPE_MAX,
> };
> 
>@@ -377,9 +379,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 +1361,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);

It's a bit confusing, perheps better just to use nla_put() here as well?

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

[...]