Re: [PATCH bpf-next 1/7] netdev-genl: create a simple family for netdev stuff

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5083 bytes --]

> On Fri, 20 Jan 2023 18:16:50 +0100 Lorenzo Bianconi wrote:
> > From: Jakub Kicinski <kuba@kernel.org>
> > 
> > Add a Netlink spec-compatible family for netdevs.
> > This is a very simple implementation without much
> > thought going into it.
> > 
> > It allows us to reap all the benefits of Netlink specs,
> > one can use the generic client to issue the commands:
> > 
> >   $ ./gen.py --spec netdev.yaml --do dev_get --json='{"ifindex": 2}'
> >   {'ifindex': 2, 'xdp-features': 31}
> > 
> >   $ ./gen.py --spec netdev.yaml --dump dev_get
> >   [{'ifindex': 1, 'xdp-features': 0}, {'ifindex': 2, 'xdp-features': 31}]
> 
> In the meantime I added support for rendering enums in Python.
> So you can show names in the example. eg:
> 
> $ ./cli.py --spec netdev.yaml --dump dev_get 
> [{'ifindex': 1, 'xdp-features': set()},
>  {'ifindex': 2,
>   'xdp-features': {'ndo-xmit', 'pass', 'redirect', 'aborted', 'drop'}},
>  {'ifindex': 3, 'xdp-features': {'rx-sg'}}]
> 
> > the generic python library does not have flags-by-name
> > support, yet, but we also don't have to carry strings
> > in the messages, as user space can get the names from
> > the spec.
> > 
> > Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > Co-developed-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > Co-developed-by: Marek Majtyka <alardam@gmail.com>
> > Signed-off-by: Marek Majtyka <alardam@gmail.com>
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  Documentation/netlink/specs/netdev.yaml |  72 ++++++++++
> 
> FWIW I'm not 100% sure if we should scope the family to all of netdev
> or just xdp. Same for the name of the op, should we call the op dev_get
> or dev_xdp_get..

is it likely we are going to add non-xdp info here in the near future? If not
I would say we can target just xdp for the moment.

> 
> > diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> > new file mode 100644
> > index 000000000000..254fc336d469
> > --- /dev/null
> > +++ b/include/uapi/linux/netdev.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/* Do not edit directly, auto-generated from: */
> 
> Like this line says, you can't hand edit this file.
> Next time someone adds an attribute all your changes will be wiped.

ack, right.

> 
> > +/*	Documentation/netlink/specs/netdev.yaml */
> > +/* YNL-GEN uapi header */
> > +
> > +#ifndef _UAPI_LINUX_NETDEV_H
> > +#define _UAPI_LINUX_NETDEV_H
> > +
> > +#define NETDEV_FAMILY_NAME	"netdev"
> > +#define NETDEV_FAMILY_VERSION	1
> > +
> > +enum netdev_xdp_act {
> > +	NETDEV_XDP_ACT_ABORTED_BIT,
> > +	NETDEV_XDP_ACT_DROP_BIT,
> > +	NETDEV_XDP_ACT_PASS_BIT,
> > +	NETDEV_XDP_ACT_TX_BIT,
> > +	NETDEV_XDP_ACT_REDIRECT_BIT,
> > +	NETDEV_XDP_ACT_NDO_XMIT_BIT,
> > +	NETDEV_XDP_ACT_XSK_ZEROCOPY_BIT,
> > +	NETDEV_XDP_ACT_HW_OFFLOAD_BIT,
> > +	NETDEV_XDP_ACT_RX_SG_BIT,
> > +	NETDEV_XDP_ACT_NDO_XMIT_SG_BIT
> 
> You need to add -bit to all the enum names in the yaml if you want 
> to have _BIT in the name here.

ack, I do not think it is needed (according to the comment below).

> 
> > +};
> > +
> > +#define NETDEV_XDP_ACT_ABORTED		BIT(NETDEV_XDP_ACT_ABORTED_BIT)
> > +#define NETDEV_XDP_ACT_DROP		BIT(NETDEV_XDP_ACT_DROP_BIT)
> > +#define NETDEV_XDP_ACT_PASS		BIT(NETDEV_XDP_ACT_PASS_BIT)
> > +#define NETDEV_XDP_ACT_TX		BIT(NETDEV_XDP_ACT_TX_BIT)
> > +#define NETDEV_XDP_ACT_REDIRECT		BIT(NETDEV_XDP_ACT_REDIRECT_BIT)
> > +#define NETDEV_XDP_ACT_NDO_XMIT		BIT(NETDEV_XDP_ACT_NDO_XMIT_BIT)
> > +#define NETDEV_XDP_ACT_XSK_ZEROCOPY	BIT(NETDEV_XDP_ACT_XSK_ZEROCOPY_BIT)
> > +#define NETDEV_XDP_ACT_HW_OFFLOAD	BIT(NETDEV_XDP_ACT_HW_OFFLOAD_BIT)
> > +#define NETDEV_XDP_ACT_RX_SG		BIT(NETDEV_XDP_ACT_RX_SG_BIT)
> > +#define NETDEV_XDP_ACT_NDO_XMIT_SG	BIT(NETDEV_XDP_ACT_NDO_XMIT_SG_BIT)
> > +
> > +#define NETDEV_XDP_ACT_BASIC		(NETDEV_XDP_ACT_DROP |	\
> > +					 NETDEV_XDP_ACT_PASS |	\
> > +					 NETDEV_XDP_ACT_TX |	\
> > +					 NETDEV_XDP_ACT_ABORTED)
> > +#define NETDEV_XDP_ACT_FULL		(NETDEV_XDP_ACT_BASIC |	\
> > +					 NETDEV_XDP_ACT_REDIRECT)
> > +#define NETDEV_XDP_ACT_ZC		(NETDEV_XDP_ACT_FULL |	\
> > +					 NETDEV_XDP_ACT_XSK_ZEROCOPY)
> 
> These defines don't belong in uAPI. Especially the use of BIT().

since netdev xdp_features is a bitmask, can we use 'flags' as type for definitions in
netdev.yaml so we can get rid of this BIT() definitions for both user and
kernel space?

> 
> > +			if (err < 0)
> > +				break;
> > +cont:
> > +			idx++;
> > +		}
> > +	}
> > +
> > +	rtnl_unlock();
> > +
> > +	if (err != -EMSGSIZE)
> > +		return err;
> > +
> > +	cb->args[1] = idx;
> > +	cb->args[0] = h;
> > +	cb->seq = net->dev_base_seq;
> > +	nl_dump_check_consistent(cb, nlmsg_hdr(skb));
> 
> I think that this line can be dropped.

ack, I will fix it.

Regards,
Lorenzo

> 
> > +	return skb->len;
> > +}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]