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

[Joe Damato <jdamato@fastly.com>]

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 ;)