Re: [PATCH net-next v15 02/14] net: netdev netlink api to bind dma-buf to a net device

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 28 Jun 2024 00:32:39 +0000 Mina Almasry wrote:
> API takes the dma-buf fd as input, and binds it to the netdevice. The
> user can specify the rx queues to bind the dma-buf to.
> 
> Suggested-by: Stanislav Fomichev <sdf@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>

> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> index 959755be4d7f9..899ac0882a098 100644
> --- a/Documentation/netlink/specs/netdev.yaml
> +++ b/Documentation/netlink/specs/netdev.yaml
> @@ -268,6 +268,45 @@ attribute-sets:
>          name: napi-id
>          doc: ID of the NAPI instance which services this queue.
>          type: u32
> +  -
> +    name: queue-dmabuf
> +    attributes:
> +      -
> +        name: type
> +        doc: rx or tx queue
> +        type: u8
> +        enum: queue-type
> +      -
> +        name: idx
> +        doc: queue index
> +        type: u32

u8 is a waste of space, since attrs are rounded up to 4B
and we don't use "idx"

How about we use a subset of queue attrs?

	name: queue-id
	subset-of: queue
	attributes:
	  -
	    name: id
	  -
	    name: type

> +  -
> +    name: bind-dmabuf

The naming is a bit too command specific, how about pp-buf ?
Or just dmabuf ?

> +    attributes:
> +      -
> +        name: ifindex
> +        doc: netdev ifindex to bind the dma-buf to.
> +        type: u32
> +        checks:
> +          min: 1
> +      -
> +        name: queues
> +        doc: receive queues to bind the dma-buf to.
> +        type: nest
> +        nested-attributes: queue-dmabuf
> +        multi-attr: true
> +      -
> +        name: dmabuf-fd
> +        doc: dmabuf file descriptor to bind.
> +        type: u32
> +      -
> +        name: dmabuf-id
> +        doc: id of the dmabuf binding
> +        type: u32
> +        checks:
> +          min: 1
> +

We need some form of introspection. Can we add both in the queue dump
and page pool dump some info (dmabuf-id?) to indicate there is a DMABUF
bound to the queue / page pool?

>    -
>      name: qstats
> @@ -579,6 +618,20 @@ operations:
>            attributes:
>              - ifindex
>          reply: *queue-get-op
> +    -
> +      name: bind-rx
> +      doc: Bind dmabuf to netdev
> +      attribute-set: bind-dmabuf
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - ifindex
> +            - dmabuf-fd
> +            - queues
> +        reply:
> +          attributes:
> +            - dmabuf-id

The ops end up getting rendered as an enum, so the ordering matters.
You can't insert in the middle without breaking uAPI.
For attribute sets (which you also added before qstat) it technically
doesn't matter but would be good to have them in order to match ops.

> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> index 43742ac5b00da..190a504a62358 100644
> --- a/include/uapi/linux/netdev.h
> +++ b/include/uapi/linux/netdev.h
> @@ -136,6 +136,24 @@ enum {
>  	NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
>  };
>  
> +enum {
> +	NETDEV_A_QUEUE_DMABUF_TYPE = 1,
> +	NETDEV_A_QUEUE_DMABUF_IDX,
> +
> +	__NETDEV_A_QUEUE_DMABUF_MAX,
> +	NETDEV_A_QUEUE_DMABUF_MAX = (__NETDEV_A_QUEUE_DMABUF_MAX - 1)
> +};
> +
> +enum {
> +	NETDEV_A_BIND_DMABUF_IFINDEX = 1,
> +	NETDEV_A_BIND_DMABUF_QUEUES,
> +	NETDEV_A_BIND_DMABUF_DMABUF_FD,
> +	NETDEV_A_BIND_DMABUF_DMABUF_ID,

This does look kinda repetitive, maybe let's drop the dmabuf from attr
names?

> +	__NETDEV_A_BIND_DMABUF_MAX,
> +	NETDEV_A_BIND_DMABUF_MAX = (__NETDEV_A_BIND_DMABUF_MAX - 1)
> +};