Re: [PATCH net-next v5 2/3] netdev-genl: Add an XSK attribute to queues

Joe Damato
2025-02-10T18:03:23Z

On Sat, Feb 08, 2025 at 05:43:47PM -0800, Stanislav Fomichev wrote:
> On 02/08, Joe Damato wrote:
> > Expose a new per-queue nest attribute, xsk, which will be present for
> > queues that are being used for AF_XDP. If the queue is not being used for
> > AF_XDP, the nest will not be present.
> >
> > In the future, this attribute can be extended to include more data about
> > XSK as it is needed.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > v5:
> > - Removed unused variable, ret, from netdev_nl_queue_fill_one.
> >
> > v4:
> > - Updated netdev_nl_queue_fill_one to use the empty nest helper added
> > in patch 1.
> >
> > v2:
> > - Patch adjusted to include an attribute, xsk, which is an empty nest
> > and exposed for queues which have a pool.
> >
> > Documentation/netlink/specs/netdev.yaml | 13 ++++++++++++-
> > include/uapi/linux/netdev.h | 6 ++++++
> > net/core/netdev-genl.c | 11 +++++++++++
> > tools/include/uapi/linux/netdev.h | 6 ++++++
> > 4 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> > index 288923e965ae..85402a2e289c 100644
> > --- a/Documentation/netlink/specs/netdev.yaml
> > +++ b/Documentation/netlink/specs/netdev.yaml
> > @@ -276,6 +276,9 @@ attribute-sets:
> > doc: The timeout, in nanoseconds, of how long to suspend irq
> > processing, if event polling finds events
> > type: uint
> > + -
> > + name: xsk-info
> > + attributes: []
> > -
> > name: queue
> > attributes:
> > @@ -294,6 +297,9 @@ attribute-sets:
> > -
> > name: type
> > doc: Queue type as rx, tx. Each queue type defines a separate ID space.
> > + XDP TX queues allocated in the kernel are not linked to NAPIs and
> > + thus not listed. AF_XDP queues will have more information set in
> > + the xsk attribute.
> > type: u32
> > enum: queue-type
> > -
> > @@ -309,7 +315,11 @@ attribute-sets:
> > doc: io_uring memory provider information.
> > type: nest
> > nested-attributes: io-uring-provider-info
> > -
> > + -
> > + name: xsk
> > + doc: XSK information for this queue, if any.
> > + type: nest
> > + nested-attributes: xsk-info
> > -
> > name: qstats
> > doc: |
> > @@ -652,6 +662,7 @@ operations:
> > - ifindex
> > - dmabuf
> > - io-uring
> > + - xsk
> > dump:
> > request:
> > attributes:
> > diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> > index 6c6ee183802d..4e82f3871473 100644
> > --- a/include/uapi/linux/netdev.h
> > +++ b/include/uapi/linux/netdev.h
> > @@ -136,6 +136,11 @@ enum {
> > NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
> > };
> >
> > +enum {
> > + __NETDEV_A_XSK_INFO_MAX,
> > + NETDEV_A_XSK_INFO_MAX = (__NETDEV_A_XSK_INFO_MAX - 1)
> > +};
> > +
> > enum {
> > NETDEV_A_QUEUE_ID = 1,
> > NETDEV_A_QUEUE_IFINDEX,
> > @@ -143,6 +148,7 @@ enum {
> > NETDEV_A_QUEUE_NAPI_ID,
> > NETDEV_A_QUEUE_DMABUF,
> > NETDEV_A_QUEUE_IO_URING,
> > + NETDEV_A_QUEUE_XSK,
> >
> > __NETDEV_A_QUEUE_MAX,
> > NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
> > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > index 0dcd4faefd8d..b5a93a449af9 100644
> > --- a/net/core/netdev-genl.c
> > +++ b/net/core/netdev-genl.c
> > @@ -400,11 +400,22 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
> > if (params->mp_ops &&
> > params->mp_ops->nl_fill(params->mp_priv, rsp, rxq))
> > goto nla_put_failure;
> > +
> > + if (rxq->pool)
> > + if (nla_put_empty_nest(rsp, NETDEV_A_QUEUE_XSK))
> > + goto nla_put_failure;
>
> Needs to be guarded by ifdef CONFIG_XDP_SOCKETS?
>
>
> net/core/netdev-genl.c: In function `netdev_nl_queue_fill_one´:
> net/core/netdev-genl.c:404:24: error: `struct netdev_rx_queue´ has no member named `pool´
> 404 | if (rxq->pool)
> | ^~
> net/core/netdev-genl.c:414:24: error: `struct netdev_queue´ has no member named `pool´
> 414 | if (txq->pool)
> | ^~

Ah, thanks.

I'm trying to decide if it'll look better factored out into helpers
vs just dropping the #ifdefs in netdev_nl_queue_fill_one.

Open to opinions so that hopefully v6 will be the last one ;)