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

[Stanislav Fomichev <stfomichev@gmail.com>]

On 02/10, Joe Damato wrote:
> 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 ;)

Might be too much boilerplate for the helpers? (assuming you
want empty helpers for #else case). The only other place that tests
rxq->pool is devmem (net/core/devmem.c) and it uses simple ifdef
in place.