diff mbox series

[RFC,net-next,v6,05/15] netdev: support binding dma-buf to netdevice

Message ID 20240305020153.2787423-6-almasrymina@google.com (mailing list archive)
State Superseded
Headers show
Series Device Memory TCP | expand

Commit Message

Mina Almasry March 5, 2024, 2:01 a.m. UTC
Add a netdev_dmabuf_binding struct which represents the
dma-buf-to-netdevice binding. The netlink API will bind the dma-buf to
rx queues on the netdevice. On the binding, the dma_buf_attach
& dma_buf_map_attachment will occur. The entries in the sg_table from
mapping will be inserted into a genpool to make it ready
for allocation.

The chunks in the genpool are owned by a dmabuf_chunk_owner struct which
holds the dma-buf offset of the base of the chunk and the dma_addr of
the chunk. Both are needed to use allocations that come from this chunk.

We create a new type that represents an allocation from the genpool:
net_iov. We setup the net_iov allocation size in the
genpool to PAGE_SIZE for simplicity: to match the PAGE_SIZE normally
allocated by the page pool and given to the drivers.

The user can unbind the dmabuf from the netdevice by closing the netlink
socket that established the binding. We do this so that the binding is
automatically unbound even if the userspace process crashes.

The binding and unbinding leaves an indicator in struct netdev_rx_queue
that the given queue is bound, but the binding doesn't take effect until
the driver actually reconfigures its queues, and re-initializes its page
pool.

The netdev_dmabuf_binding struct is refcounted, and releases its
resources only when all the refs are released.

Signed-off-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>

---

RFC v6:
- Validate rx queue index
- Refactor new functions into devmem.c (Pavel)

RFC v5:
- Renamed page_pool_iov to net_iov, and moved that support to devmem.h
  or netmem.h.

v1:

- Introduce devmem.h instead of bloating netdevice.h (Jakub)
- ENOTSUPP -> EOPNOTSUPP (checkpatch.pl I think)
- Remove unneeded rcu protection for binding->list (rtnl protected)
- Removed extraneous err_binding_put: label.
- Removed dma_addr += len (Paolo).
- Don't override err on netdev_bind_dmabuf_to_queue failure.
- Rename devmem -> dmabuf (David).
- Add id to dmabuf binding (David/Stan).
- Fix missing xa_destroy bound_rq_list.
- Use queue api to reset bound RX queues (Jakub).
- Update netlink API for rx-queue type (tx/re) (Jakub).

RFC v3:
- Support multi rx-queue binding

---
 include/net/devmem.h          | 115 +++++++++++++
 include/net/netdev_rx_queue.h |   1 +
 include/net/netmem.h          |  10 ++
 net/core/Makefile             |   2 +-
 net/core/dev.c                |   3 +
 net/core/devmem.c             | 293 ++++++++++++++++++++++++++++++++++
 net/core/netdev-genl.c        | 121 +++++++++++++-
 7 files changed, 542 insertions(+), 3 deletions(-)
 create mode 100644 include/net/devmem.h
 create mode 100644 net/core/devmem.c

Comments

Arnd Bergmann March 5, 2024, 9:04 a.m. UTC | #1
On Tue, Mar 5, 2024, at 03:01, Mina Almasry wrote:

> +int netdev_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
> +		       struct netdev_dmabuf_binding **out)
> +{
> +	struct netdev_dmabuf_binding *binding;
> +	static u32 id_alloc_next;
> +	struct scatterlist *sg;
> +	struct dma_buf *dmabuf;
> +	unsigned int sg_idx, i;
> +	unsigned long virtual;
> +	int err;
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	dmabuf = dma_buf_get(dmabuf_fd);
> +	if (IS_ERR_OR_NULL(dmabuf))
> +		return -EBADFD;

You should never need to use IS_ERR_OR_NULL() for a properly
defined kernel interface. This one should always return an
error or a valid pointer, so don't check for NULL.

> +	binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent);
> +	if (IS_ERR(binding->attachment)) {
> +		err = PTR_ERR(binding->attachment);
> +		goto err_free_id;
> +	}
> +
> +	binding->sgt =
> +		dma_buf_map_attachment(binding->attachment, DMA_BIDIRECTIONAL);
> +	if (IS_ERR(binding->sgt)) {
> +		err = PTR_ERR(binding->sgt);
> +		goto err_detach;
> +	}

Should there be a check to verify that this buffer
is suitable for network data?

In general, dmabuf allows buffers that are uncached or reside
in MMIO space of another device, but I think this would break
when you get an skb with those buffers and try to parse the
data inside of the kernel on architectures where MMIO space
is not a normal pointer or unaligned access is disallowed on
uncached data.

        Arnd
Yunsheng Lin March 5, 2024, 12:55 p.m. UTC | #2
On 2024/3/5 10:01, Mina Almasry wrote:

...

> 
> The netdev_dmabuf_binding struct is refcounted, and releases its
> resources only when all the refs are released.
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> 
> ---
> 
> RFC v6:
> - Validate rx queue index
> - Refactor new functions into devmem.c (Pavel)

It seems odd that the functions or stucts in a file called devmem.c
are named after 'dmabuf' instead of 'devmem'.

> 

...

> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index d8b810245c1d..72e932a1a948 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -8,6 +8,16 @@
>  #ifndef _NET_NETMEM_H
>  #define _NET_NETMEM_H
>  
> +#include <net/devmem.h>
> +
> +/* net_iov */
> +
> +struct net_iov {
> +	struct dmabuf_genpool_chunk_owner *owner;
> +};
> +
> +/* netmem */
> +
>  /**
>   * typedef netmem_ref - a nonexistent type marking a reference to generic
>   * network memory.
> diff --git a/net/core/Makefile b/net/core/Makefile
> index 821aec06abf1..592f955c1241 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -13,7 +13,7 @@ obj-y		     += dev.o dev_addr_lists.o dst.o netevent.o \
>  			neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
>  			sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
>  			fib_notifier.o xdp.o flow_offload.o gro.o \
> -			netdev-genl.o netdev-genl-gen.o gso.o
> +			netdev-genl.o netdev-genl-gen.o gso.o devmem.o
>  
>  obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fe054cbd41e9..bbea1b252529 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -155,6 +155,9 @@
>  #include <net/netdev_rx_queue.h>
>  #include <net/page_pool/types.h>
>  #include <net/page_pool/helpers.h>
> +#include <linux/genalloc.h>
> +#include <linux/dma-buf.h>
> +#include <net/devmem.h>
>  
>  #include "dev.h"
>  #include "net-sysfs.h"
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> new file mode 100644
> index 000000000000..779ad990971e
> --- /dev/null
> +++ b/net/core/devmem.c
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *      Devmem TCP
> + *
> + *      Authors:	Mina Almasry <almasrymina@google.com>
> + *			Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> + *			Kaiyuan Zhang <kaiyuanz@google.com
> + */
> +
> +#include <linux/types.h>
> +#include <linux/mm.h>
> +#include <linux/netdevice.h>
> +#include <trace/events/page_pool.h>
> +#include <net/netdev_rx_queue.h>
> +#include <net/page_pool/types.h>
> +#include <net/page_pool/helpers.h>
> +#include <linux/genalloc.h>
> +#include <linux/dma-buf.h>
> +#include <net/devmem.h>
> +
> +/* Device memory support */
> +
> +#ifdef CONFIG_DMA_SHARED_BUFFER

I still think it is worth adding its own config for devmem or dma-buf
for networking, thinking about the embeded system.

> +static void netdev_dmabuf_free_chunk_owner(struct gen_pool *genpool,
> +					   struct gen_pool_chunk *chunk,
> +					   void *not_used)

It seems odd to still keep the netdev_ prefix as it is not really related
to netdev, perhaps use 'net_' or something better.

> +{
> +	struct dmabuf_genpool_chunk_owner *owner = chunk->owner;
> +
> +	kvfree(owner->niovs);
> +	kfree(owner);
> +}
> +
> +void __netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding)
> +{
> +	size_t size, avail;
> +
> +	gen_pool_for_each_chunk(binding->chunk_pool,
> +				netdev_dmabuf_free_chunk_owner, NULL);
> +
> +	size = gen_pool_size(binding->chunk_pool);
> +	avail = gen_pool_avail(binding->chunk_pool);
> +
> +	if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu",
> +		  size, avail))
> +		gen_pool_destroy(binding->chunk_pool);
> +
> +	dma_buf_unmap_attachment(binding->attachment, binding->sgt,
> +				 DMA_BIDIRECTIONAL);

For now DMA_FROM_DEVICE seems enough as tx is not supported yet.

> +	dma_buf_detach(binding->dmabuf, binding->attachment);
> +	dma_buf_put(binding->dmabuf);
> +	xa_destroy(&binding->bound_rxq_list);
> +	kfree(binding);
> +}
> +
> +static int netdev_restart_rx_queue(struct net_device *dev, int rxq_idx)
> +{
> +	void *new_mem;
> +	void *old_mem;
> +	int err;
> +
> +	if (!dev || !dev->netdev_ops)
> +		return -EINVAL;
> +
> +	if (!dev->netdev_ops->ndo_queue_stop ||
> +	    !dev->netdev_ops->ndo_queue_mem_free ||
> +	    !dev->netdev_ops->ndo_queue_mem_alloc ||
> +	    !dev->netdev_ops->ndo_queue_start)
> +		return -EOPNOTSUPP;
> +
> +	new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
> +	if (!new_mem)
> +		return -ENOMEM;
> +
> +	err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
> +	if (err)
> +		goto err_free_new_mem;
> +
> +	err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
> +	if (err)
> +		goto err_start_queue;
> +
> +	dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);
> +
> +	return 0;
> +
> +err_start_queue:
> +	dev->netdev_ops->ndo_queue_start(dev, rxq_idx, old_mem);

It might worth mentioning why queue start with old_mem will always
success here as the return value seems to be ignored here.

> +
> +err_free_new_mem:
> +	dev->netdev_ops->ndo_queue_mem_free(dev, new_mem);
> +
> +	return err;
> +}
> +
> +/* Protected by rtnl_lock() */
> +static DEFINE_XARRAY_FLAGS(netdev_dmabuf_bindings, XA_FLAGS_ALLOC1);
> +
> +void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding)
> +{
> +	struct netdev_rx_queue *rxq;
> +	unsigned long xa_idx;
> +	unsigned int rxq_idx;
> +
> +	if (!binding)
> +		return;
> +
> +	if (binding->list.next)
> +		list_del(&binding->list);

The above does not seems to be a good pattern to delete a entry, is
there any reason having a checking before the list_del()? seems like
defensive programming?

> +
> +	xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) {
> +		if (rxq->binding == binding) {

It seems like defensive programming here too?

> +			/* We hold the rtnl_lock while binding/unbinding
> +			 * dma-buf, so we can't race with another thread that
> +			 * is also modifying this value. However, the driver
> +			 * may read this config while it's creating its
> +			 * rx-queues. WRITE_ONCE() here to match the
> +			 * READ_ONCE() in the driver.
> +			 */
> +			WRITE_ONCE(rxq->binding, NULL);
> +
> +			rxq_idx = get_netdev_rx_queue_index(rxq);
> +
> +			netdev_restart_rx_queue(binding->dev, rxq_idx);
> +		}
> +	}
> +
> +	xa_erase(&netdev_dmabuf_bindings, binding->id);
> +
> +	netdev_dmabuf_binding_put(binding);
> +}
> +
Mina Almasry March 5, 2024, 8 p.m. UTC | #3
On Tue, Mar 5, 2024 at 1:05 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Mar 5, 2024, at 03:01, Mina Almasry wrote:
>
> > +int netdev_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
> > +                    struct netdev_dmabuf_binding **out)
> > +{
> > +     struct netdev_dmabuf_binding *binding;
> > +     static u32 id_alloc_next;
> > +     struct scatterlist *sg;
> > +     struct dma_buf *dmabuf;
> > +     unsigned int sg_idx, i;
> > +     unsigned long virtual;
> > +     int err;
> > +
> > +     if (!capable(CAP_NET_ADMIN))
> > +             return -EPERM;
> > +
> > +     dmabuf = dma_buf_get(dmabuf_fd);
> > +     if (IS_ERR_OR_NULL(dmabuf))
> > +             return -EBADFD;
>
> You should never need to use IS_ERR_OR_NULL() for a properly
> defined kernel interface. This one should always return an
> error or a valid pointer, so don't check for NULL.
>

Thanks for clarifying. I will convert to IS_ERR().

> > +     binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent);
> > +     if (IS_ERR(binding->attachment)) {
> > +             err = PTR_ERR(binding->attachment);
> > +             goto err_free_id;
> > +     }
> > +
> > +     binding->sgt =
> > +             dma_buf_map_attachment(binding->attachment, DMA_BIDIRECTIONAL);
> > +     if (IS_ERR(binding->sgt)) {
> > +             err = PTR_ERR(binding->sgt);
> > +             goto err_detach;
> > +     }
>
> Should there be a check to verify that this buffer
> is suitable for network data?
>
> In general, dmabuf allows buffers that are uncached or reside
> in MMIO space of another device, but I think this would break
> when you get an skb with those buffers and try to parse the
> data inside of the kernel on architectures where MMIO space
> is not a normal pointer or unaligned access is disallowed on
> uncached data.
>
>         Arnd

A key goal of this patch series is that the kernel does not try to
parse the skb frags that reside in the dma-buf for that precise
reason. This is achieved using patch "net: add support for skbs with
unreadable frags" which disables the kernel touching the payload in
these skbs, and "tcp: RX path for devmem TCP" which implements a uapi
where the kernel hands the data in the dmabuf to the userspace via a
cmsg that gives the user a pointer to the data in the dmabuf (offset +
size).

So really AFACT the only restriction here is that the NIC should be
able to DMA into the dmabuf that we're attaching, and dma_buf_attach()
fails in this scenario so we're covered there.
Mina Almasry March 5, 2024, 9:17 p.m. UTC | #4
On Tue, Mar 5, 2024 at 4:55 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/3/5 10:01, Mina Almasry wrote:
>
> ...
>
> >
> > The netdev_dmabuf_binding struct is refcounted, and releases its
> > resources only when all the refs are released.
> >
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
> > Signed-off-by: Mina Almasry <almasrymina@google.com>
> >
> > ---
> >
> > RFC v6:
> > - Validate rx queue index
> > - Refactor new functions into devmem.c (Pavel)
>
> It seems odd that the functions or stucts in a file called devmem.c
> are named after 'dmabuf' instead of 'devmem'.
>

So my intention with this naming that devmem.c contains all the
functions for all devmem tcp specific support. Currently the only
devmem we support is dmabuf. In the future, other devmem may be
supported and it can fit nicely in devmem.c. For example, if we want
to extend devmem TCP to support NVMe devices, we need to add support
for p2pdma, maybe, and we can add that support under the devmem.c
umbrella rather than add new files.

But I can rename to dmabuf.c if there is strong objection to the current name.

> >
>
> ...
>
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index d8b810245c1d..72e932a1a948 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -8,6 +8,16 @@
> >  #ifndef _NET_NETMEM_H
> >  #define _NET_NETMEM_H
> >
> > +#include <net/devmem.h>
> > +
> > +/* net_iov */
> > +
> > +struct net_iov {
> > +     struct dmabuf_genpool_chunk_owner *owner;
> > +};
> > +
> > +/* netmem */
> > +
> >  /**
> >   * typedef netmem_ref - a nonexistent type marking a reference to generic
> >   * network memory.
> > diff --git a/net/core/Makefile b/net/core/Makefile
> > index 821aec06abf1..592f955c1241 100644
> > --- a/net/core/Makefile
> > +++ b/net/core/Makefile
> > @@ -13,7 +13,7 @@ obj-y                    += dev.o dev_addr_lists.o dst.o netevent.o \
> >                       neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
> >                       sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
> >                       fib_notifier.o xdp.o flow_offload.o gro.o \
> > -                     netdev-genl.o netdev-genl-gen.o gso.o
> > +                     netdev-genl.o netdev-genl-gen.o gso.o devmem.o
> >
> >  obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index fe054cbd41e9..bbea1b252529 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -155,6 +155,9 @@
> >  #include <net/netdev_rx_queue.h>
> >  #include <net/page_pool/types.h>
> >  #include <net/page_pool/helpers.h>
> > +#include <linux/genalloc.h>
> > +#include <linux/dma-buf.h>
> > +#include <net/devmem.h>
> >
> >  #include "dev.h"
> >  #include "net-sysfs.h"
> > diff --git a/net/core/devmem.c b/net/core/devmem.c
> > new file mode 100644
> > index 000000000000..779ad990971e
> > --- /dev/null
> > +++ b/net/core/devmem.c
> > @@ -0,0 +1,293 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      Devmem TCP
> > + *
> > + *      Authors:     Mina Almasry <almasrymina@google.com>
> > + *                   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > + *                   Kaiyuan Zhang <kaiyuanz@google.com
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/mm.h>
> > +#include <linux/netdevice.h>
> > +#include <trace/events/page_pool.h>
> > +#include <net/netdev_rx_queue.h>
> > +#include <net/page_pool/types.h>
> > +#include <net/page_pool/helpers.h>
> > +#include <linux/genalloc.h>
> > +#include <linux/dma-buf.h>
> > +#include <net/devmem.h>
> > +
> > +/* Device memory support */
> > +
> > +#ifdef CONFIG_DMA_SHARED_BUFFER
>
> I still think it is worth adding its own config for devmem or dma-buf
> for networking, thinking about the embeded system.
>

FWIW Willem did weigh on this previously and said he prefers to have
it unguarded by a CONFIG, but I will submit to whatever the consensus
here. It shouldn't be a huge deal to add a CONFIG technically
speaking.

> > +static void netdev_dmabuf_free_chunk_owner(struct gen_pool *genpool,
> > +                                        struct gen_pool_chunk *chunk,
> > +                                        void *not_used)
>
> It seems odd to still keep the netdev_ prefix as it is not really related
> to netdev, perhaps use 'net_' or something better.
>

Yes, thanks for catching. I can change to net_devmem_ maybe or net_dmabuf_*.

> > +{
> > +     struct dmabuf_genpool_chunk_owner *owner = chunk->owner;
> > +
> > +     kvfree(owner->niovs);
> > +     kfree(owner);
> > +}
> > +
> > +void __netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding)
> > +{
> > +     size_t size, avail;
> > +
> > +     gen_pool_for_each_chunk(binding->chunk_pool,
> > +                             netdev_dmabuf_free_chunk_owner, NULL);
> > +
> > +     size = gen_pool_size(binding->chunk_pool);
> > +     avail = gen_pool_avail(binding->chunk_pool);
> > +
> > +     if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu",
> > +               size, avail))
> > +             gen_pool_destroy(binding->chunk_pool);
> > +
> > +     dma_buf_unmap_attachment(binding->attachment, binding->sgt,
> > +                              DMA_BIDIRECTIONAL);
>
> For now DMA_FROM_DEVICE seems enough as tx is not supported yet.
>

Yes, good catch. I suspect we want to reuse this code for TX path. But
for now, I'll test with DMA_FROM_DEVICE and if I see no issues I'll
apply this change.

> > +     dma_buf_detach(binding->dmabuf, binding->attachment);
> > +     dma_buf_put(binding->dmabuf);
> > +     xa_destroy(&binding->bound_rxq_list);
> > +     kfree(binding);
> > +}
> > +
> > +static int netdev_restart_rx_queue(struct net_device *dev, int rxq_idx)
> > +{
> > +     void *new_mem;
> > +     void *old_mem;
> > +     int err;
> > +
> > +     if (!dev || !dev->netdev_ops)
> > +             return -EINVAL;
> > +
> > +     if (!dev->netdev_ops->ndo_queue_stop ||
> > +         !dev->netdev_ops->ndo_queue_mem_free ||
> > +         !dev->netdev_ops->ndo_queue_mem_alloc ||
> > +         !dev->netdev_ops->ndo_queue_start)
> > +             return -EOPNOTSUPP;
> > +
> > +     new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
> > +     if (!new_mem)
> > +             return -ENOMEM;
> > +
> > +     err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
> > +     if (err)
> > +             goto err_free_new_mem;
> > +
> > +     err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
> > +     if (err)
> > +             goto err_start_queue;
> > +
> > +     dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);
> > +
> > +     return 0;
> > +
> > +err_start_queue:
> > +     dev->netdev_ops->ndo_queue_start(dev, rxq_idx, old_mem);
>
> It might worth mentioning why queue start with old_mem will always
> success here as the return value seems to be ignored here.
>

So the old queue, we stopped it, and if we fail to bring up the new
queue, then we want to start the old queue back up to get the queue
back to a workable state.

I don't see what we can do to recover if restarting the old queue
fails. Seems like it should be a requirement that the driver tries as
much as possible to keep the old queue restartable.

I can improve this by at least logging or warning if restarting the
old queue fails.

> > +
> > +err_free_new_mem:
> > +     dev->netdev_ops->ndo_queue_mem_free(dev, new_mem);
> > +
> > +     return err;
> > +}
> > +
> > +/* Protected by rtnl_lock() */
> > +static DEFINE_XARRAY_FLAGS(netdev_dmabuf_bindings, XA_FLAGS_ALLOC1);
> > +
> > +void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding)
> > +{
> > +     struct netdev_rx_queue *rxq;
> > +     unsigned long xa_idx;
> > +     unsigned int rxq_idx;
> > +
> > +     if (!binding)
> > +             return;
> > +
> > +     if (binding->list.next)
> > +             list_del(&binding->list);
>
> The above does not seems to be a good pattern to delete a entry, is
> there any reason having a checking before the list_del()? seems like
> defensive programming?
>

I think I needed to apply this condition to handle the case where
netdev_unbind_dmabuf() is called when binding->list is not initialized
or is empty.

netdev_nl_bind_rx_doit() will call unbind to free a partially
allocated binding in error paths, so, netdev_unbind_dmabuf() may be
called with a partially initialized binding. This is why we check for
binding->list is initialized here and check that rxq->binding ==
binding below. The main point is that netdev_unbind_dmabuf() may be
asked to unbind a partially bound dmabuf due to error paths.

Maybe a comment here will test this better. I will double confirm the
check is needed for the error paths in netdev_nl_bind_rx_doit().

> > +
> > +     xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) {
> > +             if (rxq->binding == binding) {
>
> It seems like defensive programming here too?
>
> > +                     /* We hold the rtnl_lock while binding/unbinding
> > +                      * dma-buf, so we can't race with another thread that
> > +                      * is also modifying this value. However, the driver
> > +                      * may read this config while it's creating its
> > +                      * rx-queues. WRITE_ONCE() here to match the
> > +                      * READ_ONCE() in the driver.
> > +                      */
> > +                     WRITE_ONCE(rxq->binding, NULL);
> > +
> > +                     rxq_idx = get_netdev_rx_queue_index(rxq);
> > +
> > +                     netdev_restart_rx_queue(binding->dev, rxq_idx);
> > +             }
> > +     }
> > +
> > +     xa_erase(&netdev_dmabuf_bindings, binding->id);
> > +
> > +     netdev_dmabuf_binding_put(binding);
> > +}
> > +
>
Arnd Bergmann March 5, 2024, 9:42 p.m. UTC | #5
On Tue, Mar 5, 2024, at 21:00, Mina Almasry wrote:
> On Tue, Mar 5, 2024 at 1:05 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Mar 5, 2024, at 03:01, Mina Almasry wrote:
>
> A key goal of this patch series is that the kernel does not try to
> parse the skb frags that reside in the dma-buf for that precise
> reason. This is achieved using patch "net: add support for skbs with
> unreadable frags" which disables the kernel touching the payload in
> these skbs, and "tcp: RX path for devmem TCP" which implements a uapi
> where the kernel hands the data in the dmabuf to the userspace via a
> cmsg that gives the user a pointer to the data in the dmabuf (offset +
> size).
>
> So really AFACT the only restriction here is that the NIC should be
> able to DMA into the dmabuf that we're attaching, and dma_buf_attach()
> fails in this scenario so we're covered there.

Ok, makes sense. Thanks for the clarification.

     Arnd
Yunsheng Lin March 6, 2024, 12:38 p.m. UTC | #6
On 2024/3/6 5:17, Mina Almasry wrote:
> On Tue, Mar 5, 2024 at 4:55 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2024/3/5 10:01, Mina Almasry wrote:
>>
>> ...
>>
>>>
>>> The netdev_dmabuf_binding struct is refcounted, and releases its
>>> resources only when all the refs are released.
>>>
>>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>>> Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
>>> Signed-off-by: Mina Almasry <almasrymina@google.com>
>>>
>>> ---
>>>
>>> RFC v6:
>>> - Validate rx queue index
>>> - Refactor new functions into devmem.c (Pavel)
>>
>> It seems odd that the functions or stucts in a file called devmem.c
>> are named after 'dmabuf' instead of 'devmem'.
>>
> 
> So my intention with this naming that devmem.c contains all the
> functions for all devmem tcp specific support. Currently the only
> devmem we support is dmabuf. In the future, other devmem may be
> supported and it can fit nicely in devmem.c. For example, if we want
> to extend devmem TCP to support NVMe devices, we need to add support
> for p2pdma, maybe, and we can add that support under the devmem.c
> umbrella rather than add new files.
> 
> But I can rename to dmabuf.c if there is strong objection to the current name.

Grepping 'dmabuf' seems to show that it may be common rename it to
something as *_dmabuf.c.

> 
>>>
>>
>> ...
>>
>>> diff --git a/include/net/netmem.h b/include/net/netmem.h
>>> index d8b810245c1d..72e932a1a948 100644
>>> --- a/include/net/netmem.h
>>> +++ b/include/net/netmem.h
>>> @@ -8,6 +8,16 @@
>>>  #ifndef _NET_NETMEM_H
>>>  #define _NET_NETMEM_H
>>>
>>> +#include <net/devmem.h>
>>> +
>>> +/* net_iov */
>>> +
>>> +struct net_iov {
>>> +     struct dmabuf_genpool_chunk_owner *owner;
>>> +};
>>> +
>>> +/* netmem */
>>> +
>>>  /**
>>>   * typedef netmem_ref - a nonexistent type marking a reference to generic
>>>   * network memory.
>>> diff --git a/net/core/Makefile b/net/core/Makefile
>>> index 821aec06abf1..592f955c1241 100644
>>> --- a/net/core/Makefile
>>> +++ b/net/core/Makefile
>>> @@ -13,7 +13,7 @@ obj-y                    += dev.o dev_addr_lists.o dst.o netevent.o \
>>>                       neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
>>>                       sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
>>>                       fib_notifier.o xdp.o flow_offload.o gro.o \
>>> -                     netdev-genl.o netdev-genl-gen.o gso.o
>>> +                     netdev-genl.o netdev-genl-gen.o gso.o devmem.o
>>>
>>>  obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index fe054cbd41e9..bbea1b252529 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -155,6 +155,9 @@
>>>  #include <net/netdev_rx_queue.h>
>>>  #include <net/page_pool/types.h>
>>>  #include <net/page_pool/helpers.h>
>>> +#include <linux/genalloc.h>
>>> +#include <linux/dma-buf.h>
>>> +#include <net/devmem.h>
>>>
>>>  #include "dev.h"
>>>  #include "net-sysfs.h"
>>> diff --git a/net/core/devmem.c b/net/core/devmem.c
>>> new file mode 100644
>>> index 000000000000..779ad990971e
>>> --- /dev/null
>>> +++ b/net/core/devmem.c
>>> @@ -0,0 +1,293 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + *      Devmem TCP
>>> + *
>>> + *      Authors:     Mina Almasry <almasrymina@google.com>
>>> + *                   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
>>> + *                   Kaiyuan Zhang <kaiyuanz@google.com
>>> + */
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/mm.h>
>>> +#include <linux/netdevice.h>
>>> +#include <trace/events/page_pool.h>
>>> +#include <net/netdev_rx_queue.h>
>>> +#include <net/page_pool/types.h>
>>> +#include <net/page_pool/helpers.h>
>>> +#include <linux/genalloc.h>
>>> +#include <linux/dma-buf.h>
>>> +#include <net/devmem.h>
>>> +
>>> +/* Device memory support */
>>> +
>>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>>
>> I still think it is worth adding its own config for devmem or dma-buf
>> for networking, thinking about the embeded system.
>>
> 
> FWIW Willem did weigh on this previously and said he prefers to have
> it unguarded by a CONFIG, but I will submit to whatever the consensus
> here. It shouldn't be a huge deal to add a CONFIG technically
> speaking.

Grepping 'CONFIG_DMA_SHARED_BUFFER' show that the API user of dmabuf
API does not seems to reuse the CONFIG_DMA_SHARED_BUFFER, instead they
seem to define its own config, and select CONFIG_DMA_SHARED_BUFFER
if necessary, it that any reason it is different here?

> 
>>> +static void netdev_dmabuf_free_chunk_owner(struct gen_pool *genpool,
>>> +                                        struct gen_pool_chunk *chunk,
>>> +                                        void *not_used)
>>
>> It seems odd to still keep the netdev_ prefix as it is not really related
>> to netdev, perhaps use 'net_' or something better.
>>
> 
> Yes, thanks for catching. I can change to net_devmem_ maybe or net_dmabuf_*.

FWIW, net_dmabuf_* seems like a better name technically.

> 
>>> +{
>>> +     struct dmabuf_genpool_chunk_owner *owner = chunk->owner;
>>> +
>>> +     kvfree(owner->niovs);
>>> +     kfree(owner);
>>> +}
>>> +
>>> +void __netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding)
>>> +{
>>> +     size_t size, avail;
>>> +
>>> +     gen_pool_for_each_chunk(binding->chunk_pool,
>>> +                             netdev_dmabuf_free_chunk_owner, NULL);
>>> +
>>> +     size = gen_pool_size(binding->chunk_pool);
>>> +     avail = gen_pool_avail(binding->chunk_pool);
>>> +
>>> +     if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu",
>>> +               size, avail))
>>> +             gen_pool_destroy(binding->chunk_pool);
>>> +
>>> +     dma_buf_unmap_attachment(binding->attachment, binding->sgt,
>>> +                              DMA_BIDIRECTIONAL);
>>
>> For now DMA_FROM_DEVICE seems enough as tx is not supported yet.
>>
> 
> Yes, good catch. I suspect we want to reuse this code for TX path. But
> for now, I'll test with DMA_FROM_DEVICE and if I see no issues I'll
> apply this change.
> 
>>> +     dma_buf_detach(binding->dmabuf, binding->attachment);
>>> +     dma_buf_put(binding->dmabuf);
>>> +     xa_destroy(&binding->bound_rxq_list);
>>> +     kfree(binding);
>>> +}
>>> +
>>> +static int netdev_restart_rx_queue(struct net_device *dev, int rxq_idx)
>>> +{
>>> +     void *new_mem;
>>> +     void *old_mem;
>>> +     int err;
>>> +
>>> +     if (!dev || !dev->netdev_ops)
>>> +             return -EINVAL;
>>> +
>>> +     if (!dev->netdev_ops->ndo_queue_stop ||
>>> +         !dev->netdev_ops->ndo_queue_mem_free ||
>>> +         !dev->netdev_ops->ndo_queue_mem_alloc ||
>>> +         !dev->netdev_ops->ndo_queue_start)
>>> +             return -EOPNOTSUPP;
>>> +
>>> +     new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
>>> +     if (!new_mem)
>>> +             return -ENOMEM;
>>> +
>>> +     err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
>>> +     if (err)
>>> +             goto err_free_new_mem;
>>> +
>>> +     err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
>>> +     if (err)
>>> +             goto err_start_queue;
>>> +
>>> +     dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);
>>> +
>>> +     return 0;
>>> +
>>> +err_start_queue:
>>> +     dev->netdev_ops->ndo_queue_start(dev, rxq_idx, old_mem);
>>
>> It might worth mentioning why queue start with old_mem will always
>> success here as the return value seems to be ignored here.
>>
> 
> So the old queue, we stopped it, and if we fail to bring up the new
> queue, then we want to start the old queue back up to get the queue
> back to a workable state.
> 
> I don't see what we can do to recover if restarting the old queue
> fails. Seems like it should be a requirement that the driver tries as
> much as possible to keep the old queue restartable.

Is it possible that we may have the 'old_mem' leaking if the driver
fails to restart the old queue? how does the driver handle the
firmware cmd failure for ndo_queue_start()? it seems a little
tricky to implement it.

> 
> I can improve this by at least logging or warning if restarting the
> old queue fails.

Also the semantics of the above function seems odd that it is not
only restarting rx queue, but also freeing and allocating memory
despite the name only suggests 'restart', I am a litte afraid that
it may conflict with future usecae when user only need the
'restart' part, perhaps rename it to a more appropriate name.

> 
>>> +
>>> +err_free_new_mem:
>>> +     dev->netdev_ops->ndo_queue_mem_free(dev, new_mem);
>>> +
>>> +     return err;
>>> +}
>>> +
>>> +/* Protected by rtnl_lock() */
>>> +static DEFINE_XARRAY_FLAGS(netdev_dmabuf_bindings, XA_FLAGS_ALLOC1);
>>> +
>>> +void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding)
>>> +{
>>> +     struct netdev_rx_queue *rxq;
>>> +     unsigned long xa_idx;
>>> +     unsigned int rxq_idx;
>>> +
>>> +     if (!binding)
>>> +             return;
>>> +
>>> +     if (binding->list.next)
>>> +             list_del(&binding->list);
>>
>> The above does not seems to be a good pattern to delete a entry, is
>> there any reason having a checking before the list_del()? seems like
>> defensive programming?
>>
> 
> I think I needed to apply this condition to handle the case where
> netdev_unbind_dmabuf() is called when binding->list is not initialized
> or is empty.
> 
> netdev_nl_bind_rx_doit() will call unbind to free a partially
> allocated binding in error paths, so, netdev_unbind_dmabuf() may be
> called with a partially initialized binding. This is why we check for
> binding->list is initialized here and check that rxq->binding ==
> binding below. The main point is that netdev_unbind_dmabuf() may be
> asked to unbind a partially bound dmabuf due to error paths.
> 
> Maybe a comment here will test this better. I will double confirm the
> check is needed for the error paths in netdev_nl_bind_rx_doit().
> 
>>> +
>>> +     xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) {
>>> +             if (rxq->binding == binding) {
>>
>> It seems like defensive programming here too?
>>
Mina Almasry March 6, 2024, 10:10 p.m. UTC | #7
On Wed, Mar 6, 2024 at 4:38 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/3/6 5:17, Mina Almasry wrote:
> > On Tue, Mar 5, 2024 at 4:55 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>
> >> On 2024/3/5 10:01, Mina Almasry wrote:
> >>
> >> ...
> >>
> >>>
> >>> The netdev_dmabuf_binding struct is refcounted, and releases its
> >>> resources only when all the refs are released.
> >>>
> >>> Signed-off-by: Willem de Bruijn <willemb@google.com>
> >>> Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
> >>> Signed-off-by: Mina Almasry <almasrymina@google.com>
> >>>
> >>> ---
> >>>
> >>> RFC v6:
> >>> - Validate rx queue index
> >>> - Refactor new functions into devmem.c (Pavel)
> >>
> >> It seems odd that the functions or stucts in a file called devmem.c
> >> are named after 'dmabuf' instead of 'devmem'.
> >>
> >
> > So my intention with this naming that devmem.c contains all the
> > functions for all devmem tcp specific support. Currently the only
> > devmem we support is dmabuf. In the future, other devmem may be
> > supported and it can fit nicely in devmem.c. For example, if we want
> > to extend devmem TCP to support NVMe devices, we need to add support
> > for p2pdma, maybe, and we can add that support under the devmem.c
> > umbrella rather than add new files.
> >
> > But I can rename to dmabuf.c if there is strong objection to the current name.
>
> Grepping 'dmabuf' seems to show that it may be common rename it to
> something as *_dmabuf.c.
>
> >
> >>>
> >>
> >> ...
> >>
> >>> diff --git a/include/net/netmem.h b/include/net/netmem.h
> >>> index d8b810245c1d..72e932a1a948 100644
> >>> --- a/include/net/netmem.h
> >>> +++ b/include/net/netmem.h
> >>> @@ -8,6 +8,16 @@
> >>>  #ifndef _NET_NETMEM_H
> >>>  #define _NET_NETMEM_H
> >>>
> >>> +#include <net/devmem.h>
> >>> +
> >>> +/* net_iov */
> >>> +
> >>> +struct net_iov {
> >>> +     struct dmabuf_genpool_chunk_owner *owner;
> >>> +};
> >>> +
> >>> +/* netmem */
> >>> +
> >>>  /**
> >>>   * typedef netmem_ref - a nonexistent type marking a reference to generic
> >>>   * network memory.
> >>> diff --git a/net/core/Makefile b/net/core/Makefile
> >>> index 821aec06abf1..592f955c1241 100644
> >>> --- a/net/core/Makefile
> >>> +++ b/net/core/Makefile
> >>> @@ -13,7 +13,7 @@ obj-y                    += dev.o dev_addr_lists.o dst.o netevent.o \
> >>>                       neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
> >>>                       sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
> >>>                       fib_notifier.o xdp.o flow_offload.o gro.o \
> >>> -                     netdev-genl.o netdev-genl-gen.o gso.o
> >>> +                     netdev-genl.o netdev-genl-gen.o gso.o devmem.o
> >>>
> >>>  obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
> >>>
> >>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>> index fe054cbd41e9..bbea1b252529 100644
> >>> --- a/net/core/dev.c
> >>> +++ b/net/core/dev.c
> >>> @@ -155,6 +155,9 @@
> >>>  #include <net/netdev_rx_queue.h>
> >>>  #include <net/page_pool/types.h>
> >>>  #include <net/page_pool/helpers.h>
> >>> +#include <linux/genalloc.h>
> >>> +#include <linux/dma-buf.h>
> >>> +#include <net/devmem.h>
> >>>
> >>>  #include "dev.h"
> >>>  #include "net-sysfs.h"
> >>> diff --git a/net/core/devmem.c b/net/core/devmem.c
> >>> new file mode 100644
> >>> index 000000000000..779ad990971e
> >>> --- /dev/null
> >>> +++ b/net/core/devmem.c
> >>> @@ -0,0 +1,293 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>> +/*
> >>> + *      Devmem TCP
> >>> + *
> >>> + *      Authors:     Mina Almasry <almasrymina@google.com>
> >>> + *                   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> >>> + *                   Kaiyuan Zhang <kaiyuanz@google.com
> >>> + */
> >>> +
> >>> +#include <linux/types.h>
> >>> +#include <linux/mm.h>
> >>> +#include <linux/netdevice.h>
> >>> +#include <trace/events/page_pool.h>
> >>> +#include <net/netdev_rx_queue.h>
> >>> +#include <net/page_pool/types.h>
> >>> +#include <net/page_pool/helpers.h>
> >>> +#include <linux/genalloc.h>
> >>> +#include <linux/dma-buf.h>
> >>> +#include <net/devmem.h>
> >>> +
> >>> +/* Device memory support */
> >>> +
> >>> +#ifdef CONFIG_DMA_SHARED_BUFFER
> >>
> >> I still think it is worth adding its own config for devmem or dma-buf
> >> for networking, thinking about the embeded system.
> >>
> >
> > FWIW Willem did weigh on this previously and said he prefers to have
> > it unguarded by a CONFIG, but I will submit to whatever the consensus
> > here. It shouldn't be a huge deal to add a CONFIG technically
> > speaking.
>
> Grepping 'CONFIG_DMA_SHARED_BUFFER' show that the API user of dmabuf
> API does not seems to reuse the CONFIG_DMA_SHARED_BUFFER, instead they
> seem to define its own config, and select CONFIG_DMA_SHARED_BUFFER
> if necessary, it that any reason it is different here?
>
> >
> >>> +static void netdev_dmabuf_free_chunk_owner(struct gen_pool *genpool,
> >>> +                                        struct gen_pool_chunk *chunk,
> >>> +                                        void *not_used)
> >>
> >> It seems odd to still keep the netdev_ prefix as it is not really related
> >> to netdev, perhaps use 'net_' or something better.
> >>
> >
> > Yes, thanks for catching. I can change to net_devmem_ maybe or net_dmabuf_*.
>
> FWIW, net_dmabuf_* seems like a better name technically.
>
> >
> >>> +{
> >>> +     struct dmabuf_genpool_chunk_owner *owner = chunk->owner;
> >>> +
> >>> +     kvfree(owner->niovs);
> >>> +     kfree(owner);
> >>> +}
> >>> +
> >>> +void __netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding)
> >>> +{
> >>> +     size_t size, avail;
> >>> +
> >>> +     gen_pool_for_each_chunk(binding->chunk_pool,
> >>> +                             netdev_dmabuf_free_chunk_owner, NULL);
> >>> +
> >>> +     size = gen_pool_size(binding->chunk_pool);
> >>> +     avail = gen_pool_avail(binding->chunk_pool);
> >>> +
> >>> +     if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu",
> >>> +               size, avail))
> >>> +             gen_pool_destroy(binding->chunk_pool);
> >>> +
> >>> +     dma_buf_unmap_attachment(binding->attachment, binding->sgt,
> >>> +                              DMA_BIDIRECTIONAL);
> >>
> >> For now DMA_FROM_DEVICE seems enough as tx is not supported yet.
> >>
> >
> > Yes, good catch. I suspect we want to reuse this code for TX path. But
> > for now, I'll test with DMA_FROM_DEVICE and if I see no issues I'll
> > apply this change.
> >
> >>> +     dma_buf_detach(binding->dmabuf, binding->attachment);
> >>> +     dma_buf_put(binding->dmabuf);
> >>> +     xa_destroy(&binding->bound_rxq_list);
> >>> +     kfree(binding);
> >>> +}
> >>> +
> >>> +static int netdev_restart_rx_queue(struct net_device *dev, int rxq_idx)
> >>> +{
> >>> +     void *new_mem;
> >>> +     void *old_mem;
> >>> +     int err;
> >>> +
> >>> +     if (!dev || !dev->netdev_ops)
> >>> +             return -EINVAL;
> >>> +
> >>> +     if (!dev->netdev_ops->ndo_queue_stop ||
> >>> +         !dev->netdev_ops->ndo_queue_mem_free ||
> >>> +         !dev->netdev_ops->ndo_queue_mem_alloc ||
> >>> +         !dev->netdev_ops->ndo_queue_start)
> >>> +             return -EOPNOTSUPP;
> >>> +
> >>> +     new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
> >>> +     if (!new_mem)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
> >>> +     if (err)
> >>> +             goto err_free_new_mem;
> >>> +
> >>> +     err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
> >>> +     if (err)
> >>> +             goto err_start_queue;
> >>> +
> >>> +     dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);
> >>> +
> >>> +     return 0;
> >>> +
> >>> +err_start_queue:
> >>> +     dev->netdev_ops->ndo_queue_start(dev, rxq_idx, old_mem);
> >>
> >> It might worth mentioning why queue start with old_mem will always
> >> success here as the return value seems to be ignored here.
> >>
> >
> > So the old queue, we stopped it, and if we fail to bring up the new
> > queue, then we want to start the old queue back up to get the queue
> > back to a workable state.
> >
> > I don't see what we can do to recover if restarting the old queue
> > fails. Seems like it should be a requirement that the driver tries as
> > much as possible to keep the old queue restartable.
>
> Is it possible that we may have the 'old_mem' leaking if the driver
> fails to restart the old queue? how does the driver handle the
> firmware cmd failure for ndo_queue_start()? it seems a little
> tricky to implement it.
>

I'm not sure what we can do to meaningfully recover from failure to
restarting the old queue, except log it so the error is visible. In
theory because we have not modifying any queue configurations
restarting it would be straight forward, but since it's dealing with
hardware then any failures are possible.

> >
> > I can improve this by at least logging or warning if restarting the
> > old queue fails.
>
> Also the semantics of the above function seems odd that it is not
> only restarting rx queue, but also freeing and allocating memory
> despite the name only suggests 'restart', I am a litte afraid that
> it may conflict with future usecae when user only need the
> 'restart' part, perhaps rename it to a more appropriate name.
>

Oh, what we want here is just the 'restart' part. However, Jakub
mandates that if you restart a queue (or a driver), you do it like
this, hence the slightly more complicated implementation.

https://patchwork.kernel.org/project/netdevbpf/patch/20231106024413.2801438-13-almasrymina@google.com/#25590262
https://lore.kernel.org/netdev/20230815171638.4c057dcd@kernel.org/

> >
> >>> +
> >>> +err_free_new_mem:
> >>> +     dev->netdev_ops->ndo_queue_mem_free(dev, new_mem);
> >>> +
> >>> +     return err;
> >>> +}
> >>> +
> >>> +/* Protected by rtnl_lock() */
> >>> +static DEFINE_XARRAY_FLAGS(netdev_dmabuf_bindings, XA_FLAGS_ALLOC1);
> >>> +
> >>> +void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding)
> >>> +{
> >>> +     struct netdev_rx_queue *rxq;
> >>> +     unsigned long xa_idx;
> >>> +     unsigned int rxq_idx;
> >>> +
> >>> +     if (!binding)
> >>> +             return;
> >>> +
> >>> +     if (binding->list.next)
> >>> +             list_del(&binding->list);
> >>
> >> The above does not seems to be a good pattern to delete a entry, is
> >> there any reason having a checking before the list_del()? seems like
> >> defensive programming?
> >>
> >
> > I think I needed to apply this condition to handle the case where
> > netdev_unbind_dmabuf() is called when binding->list is not initialized
> > or is empty.
> >
> > netdev_nl_bind_rx_doit() will call unbind to free a partially
> > allocated binding in error paths, so, netdev_unbind_dmabuf() may be
> > called with a partially initialized binding. This is why we check for
> > binding->list is initialized here and check that rxq->binding ==
> > binding below. The main point is that netdev_unbind_dmabuf() may be
> > asked to unbind a partially bound dmabuf due to error paths.
> >
> > Maybe a comment here will test this better. I will double confirm the
> > check is needed for the error paths in netdev_nl_bind_rx_doit().
> >
> >>> +
> >>> +     xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) {
> >>> +             if (rxq->binding == binding) {
> >>
> >> It seems like defensive programming here too?
> >>
>
Yunsheng Lin March 7, 2024, 12:15 p.m. UTC | #8
On 2024/3/7 6:10, Mina Almasry wrote:

...

>>>>> +static int netdev_restart_rx_queue(struct net_device *dev, int rxq_idx)
>>>>> +{
>>>>> +     void *new_mem;
>>>>> +     void *old_mem;
>>>>> +     int err;
>>>>> +
>>>>> +     if (!dev || !dev->netdev_ops)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     if (!dev->netdev_ops->ndo_queue_stop ||
>>>>> +         !dev->netdev_ops->ndo_queue_mem_free ||
>>>>> +         !dev->netdev_ops->ndo_queue_mem_alloc ||
>>>>> +         !dev->netdev_ops->ndo_queue_start)
>>>>> +             return -EOPNOTSUPP;
>>>>> +
>>>>> +     new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
>>>>> +     if (!new_mem)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
>>>>> +     if (err)
>>>>> +             goto err_free_new_mem;
>>>>> +
>>>>> +     err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
>>>>> +     if (err)
>>>>> +             goto err_start_queue;
>>>>> +
>>>>> +     dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);
>>>>> +
>>>>> +     return 0;
>>>>> +
>>>>> +err_start_queue:
>>>>> +     dev->netdev_ops->ndo_queue_start(dev, rxq_idx, old_mem);
>>>>
>>>> It might worth mentioning why queue start with old_mem will always
>>>> success here as the return value seems to be ignored here.
>>>>
>>>
>>> So the old queue, we stopped it, and if we fail to bring up the new
>>> queue, then we want to start the old queue back up to get the queue
>>> back to a workable state.
>>>
>>> I don't see what we can do to recover if restarting the old queue
>>> fails. Seems like it should be a requirement that the driver tries as
>>> much as possible to keep the old queue restartable.
>>
>> Is it possible that we may have the 'old_mem' leaking if the driver
>> fails to restart the old queue? how does the driver handle the
>> firmware cmd failure for ndo_queue_start()? it seems a little
>> tricky to implement it.
>>
> 
> I'm not sure what we can do to meaningfully recover from failure to
> restarting the old queue, except log it so the error is visible. In
> theory because we have not modifying any queue configurations
> restarting it would be straight forward, but since it's dealing with
> hardware then any failures are possible.

Yes, we may need to have a clear semantics of how should the driver
implement the above interface, for example if the driver should free
the memory when fail to start a queue or the driver should restart
the queue when fail to stop a queue? Otherwise we may have different
driver implementing different behavior.

From the disscusion you mentioned below, does it make senses to
modeling rdma subsystem by using create_queue/modify_queue/destroy_queue
semantics instead?

> 
>>>
>>> I can improve this by at least logging or warning if restarting the
>>> old queue fails.
>>
>> Also the semantics of the above function seems odd that it is not
>> only restarting rx queue, but also freeing and allocating memory
>> despite the name only suggests 'restart', I am a litte afraid that
>> it may conflict with future usecae when user only need the
>> 'restart' part, perhaps rename it to a more appropriate name.
>>
> 
> Oh, what we want here is just the 'restart' part. However, Jakub
> mandates that if you restart a queue (or a driver), you do it like
> this, hence the slightly more complicated implementation.
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20231106024413.2801438-13-almasrymina@google.com/#25590262
> https://lore.kernel.org/netdev/20230815171638.4c057dcd@kernel.org/

Thanks for the link.

I like david's idea of "a more generic design where H/W queues are created
and destroyed - e.g., queues unique to a process which makes the cleanup
so much easier." , but it seems it is a lot of work for networking to
implement that for now.

>
Jakub Kicinski March 8, 2024, 3:58 a.m. UTC | #9
On Mon,  4 Mar 2024 18:01:40 -0800 Mina Almasry wrote:
> +	if (!dev || !dev->netdev_ops)
> +		return -EINVAL;

too defensive

> +	if (!dev->netdev_ops->ndo_queue_stop ||
> +	    !dev->netdev_ops->ndo_queue_mem_free ||
> +	    !dev->netdev_ops->ndo_queue_mem_alloc ||
> +	    !dev->netdev_ops->ndo_queue_start)
> +		return -EOPNOTSUPP;
> +
> +	new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
> +	if (!new_mem)
> +		return -ENOMEM;
> +
> +	err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
> +	if (err)
> +		goto err_free_new_mem;
> +
> +	err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
> +	if (err)
> +		goto err_start_queue;
> +
> +	dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);

nice :)

> +	rxq = __netif_get_rx_queue(dev, rxq_idx);
> +
> +	if (rxq->binding)

nit: a few places have an empty line between call and error check

> +		return -EEXIST;

> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;

this can be a flag on the netlink policy, no?

	flags: [ admin-perm ]

on the op

> +	dmabuf = dma_buf_get(dmabuf_fd);
> +	if (IS_ERR_OR_NULL(dmabuf))
> +		return -EBADFD;


> +	hdr = genlmsg_put(rsp, info->snd_portid, info->snd_seq,

genlmsg_iput()

> +static int netdev_netlink_notify(struct notifier_block *nb, unsigned long state,
> +				 void *_notify)
> +{
> +	struct netlink_notify *notify = _notify;
> +	struct netdev_dmabuf_binding *rbinding;
> +
> +	if (state != NETLINK_URELEASE || notify->protocol != NETLINK_GENERIC)
> +		return NOTIFY_DONE;
> +
> +	rtnl_lock();
> +
> +	list_for_each_entry(rbinding, &netdev_rbinding_list, list) {
> +		if (rbinding->owner_nlportid == notify->portid) {
> +			netdev_unbind_dmabuf(rbinding);
> +			break;
> +		}
> +	}
> +
> +	rtnl_unlock();
> +
> +	return NOTIFY_OK;
> +}

While you were not looking we added three new members to the netlink
family:

 * @sock_priv_size: the size of per-socket private memory
 * @sock_priv_init: the per-socket private memory initializer
 * @sock_priv_destroy: the per-socket private memory destructor

You should be able to associate state with a netlink socket
and have it auto-destroyed if the socket closes.
LMK if that doesn't work for you, I was hoping it would fit nicely.

I just realized now that the code gen doesn't know how to spit
those members out, but I'll send a patch tomorrow, you can hack
it manually until that gets in.
diff mbox series

Patch

diff --git a/include/net/devmem.h b/include/net/devmem.h
new file mode 100644
index 000000000000..85ccbbe84c65
--- /dev/null
+++ b/include/net/devmem.h
@@ -0,0 +1,115 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Device memory TCP support
+ *
+ * Authors:	Mina Almasry <almasrymina@google.com>
+ *		Willem de Bruijn <willemb@google.com>
+ *		Kaiyuan Zhang <kaiyuanz@google.com>
+ *
+ */
+#ifndef _NET_DEVMEM_H
+#define _NET_DEVMEM_H
+
+struct netdev_dmabuf_binding {
+	struct dma_buf *dmabuf;
+	struct dma_buf_attachment *attachment;
+	struct sg_table *sgt;
+	struct net_device *dev;
+	struct gen_pool *chunk_pool;
+
+	/* The user holds a ref (via the netlink API) for as long as they want
+	 * the binding to remain alive. Each page pool using this binding holds
+	 * a ref to keep the binding alive. Each allocated net_iov holds a
+	 * ref.
+	 *
+	 * The binding undos itself and unmaps the underlying dmabuf once all
+	 * those refs are dropped and the binding is no longer desired or in
+	 * use.
+	 */
+	refcount_t ref;
+
+	/* The portid of the user that owns this binding. Used for netlink to
+	 * notify us of the user dropping the bind.
+	 */
+	u32 owner_nlportid;
+
+	/* The list of bindings currently active. Used for netlink to notify us
+	 * of the user dropping the bind.
+	 */
+	struct list_head list;
+
+	/* rxq's this binding is active on. */
+	struct xarray bound_rxq_list;
+
+	/* ID of this binding. Globally unique to all bindings currently
+	 * active.
+	 */
+	u32 id;
+};
+
+/* Owner of the dma-buf chunks inserted into the gen pool. Each scatterlist
+ * entry from the dmabuf is inserted into the genpool as a chunk, and needs
+ * this owner struct to keep track of some metadata necessary to create
+ * allocations from this chunk.
+ */
+struct dmabuf_genpool_chunk_owner {
+	/* Offset into the dma-buf where this chunk starts.  */
+	unsigned long base_virtual;
+
+	/* dma_addr of the start of the chunk.  */
+	dma_addr_t base_dma_addr;
+
+	/* Array of net_iovs for this chunk. */
+	struct net_iov *niovs;
+	size_t num_niovs;
+
+	struct netdev_dmabuf_binding *binding;
+};
+
+#ifdef CONFIG_DMA_SHARED_BUFFER
+void __netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding);
+int netdev_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
+		       struct netdev_dmabuf_binding **out);
+void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding);
+int netdev_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
+				struct netdev_dmabuf_binding *binding);
+#else
+static inline void
+__netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding)
+{
+}
+
+static inline int netdev_bind_dmabuf(struct net_device *dev,
+				     unsigned int dmabuf_fd,
+				     struct netdev_dmabuf_binding **out)
+{
+	return -EOPNOTSUPP;
+}
+static inline void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding)
+{
+}
+
+static inline int
+netdev_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
+			    struct netdev_dmabuf_binding *binding)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
+static inline void
+netdev_dmabuf_binding_get(struct netdev_dmabuf_binding *binding)
+{
+	refcount_inc(&binding->ref);
+}
+
+static inline void
+netdev_dmabuf_binding_put(struct netdev_dmabuf_binding *binding)
+{
+	if (!refcount_dec_and_test(&binding->ref))
+		return;
+
+	__netdev_dmabuf_binding_free(binding);
+}
+
+#endif /* _NET_DEVMEM_H */
diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
index aa1716fb0e53..5dc35628633a 100644
--- a/include/net/netdev_rx_queue.h
+++ b/include/net/netdev_rx_queue.h
@@ -25,6 +25,7 @@  struct netdev_rx_queue {
 	 * Readers and writers must hold RTNL
 	 */
 	struct napi_struct		*napi;
+	struct netdev_dmabuf_binding *binding;
 } ____cacheline_aligned_in_smp;
 
 /*
diff --git a/include/net/netmem.h b/include/net/netmem.h
index d8b810245c1d..72e932a1a948 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h
@@ -8,6 +8,16 @@ 
 #ifndef _NET_NETMEM_H
 #define _NET_NETMEM_H
 
+#include <net/devmem.h>
+
+/* net_iov */
+
+struct net_iov {
+	struct dmabuf_genpool_chunk_owner *owner;
+};
+
+/* netmem */
+
 /**
  * typedef netmem_ref - a nonexistent type marking a reference to generic
  * network memory.
diff --git a/net/core/Makefile b/net/core/Makefile
index 821aec06abf1..592f955c1241 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -13,7 +13,7 @@  obj-y		     += dev.o dev_addr_lists.o dst.o netevent.o \
 			neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
 			sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
 			fib_notifier.o xdp.o flow_offload.o gro.o \
-			netdev-genl.o netdev-genl-gen.o gso.o
+			netdev-genl.o netdev-genl-gen.o gso.o devmem.o
 
 obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
 
diff --git a/net/core/dev.c b/net/core/dev.c
index fe054cbd41e9..bbea1b252529 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -155,6 +155,9 @@ 
 #include <net/netdev_rx_queue.h>
 #include <net/page_pool/types.h>
 #include <net/page_pool/helpers.h>
+#include <linux/genalloc.h>
+#include <linux/dma-buf.h>
+#include <net/devmem.h>
 
 #include "dev.h"
 #include "net-sysfs.h"
diff --git a/net/core/devmem.c b/net/core/devmem.c
new file mode 100644
index 000000000000..779ad990971e
--- /dev/null
+++ b/net/core/devmem.c
@@ -0,0 +1,293 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *      Devmem TCP
+ *
+ *      Authors:	Mina Almasry <almasrymina@google.com>
+ *			Willem de Bruijn <willemdebruijn.kernel@gmail.com>
+ *			Kaiyuan Zhang <kaiyuanz@google.com
+ */
+
+#include <linux/types.h>
+#include <linux/mm.h>
+#include <linux/netdevice.h>
+#include <trace/events/page_pool.h>
+#include <net/netdev_rx_queue.h>
+#include <net/page_pool/types.h>
+#include <net/page_pool/helpers.h>
+#include <linux/genalloc.h>
+#include <linux/dma-buf.h>
+#include <net/devmem.h>
+
+/* Device memory support */
+
+#ifdef CONFIG_DMA_SHARED_BUFFER
+static void netdev_dmabuf_free_chunk_owner(struct gen_pool *genpool,
+					   struct gen_pool_chunk *chunk,
+					   void *not_used)
+{
+	struct dmabuf_genpool_chunk_owner *owner = chunk->owner;
+
+	kvfree(owner->niovs);
+	kfree(owner);
+}
+
+void __netdev_dmabuf_binding_free(struct netdev_dmabuf_binding *binding)
+{
+	size_t size, avail;
+
+	gen_pool_for_each_chunk(binding->chunk_pool,
+				netdev_dmabuf_free_chunk_owner, NULL);
+
+	size = gen_pool_size(binding->chunk_pool);
+	avail = gen_pool_avail(binding->chunk_pool);
+
+	if (!WARN(size != avail, "can't destroy genpool. size=%lu, avail=%lu",
+		  size, avail))
+		gen_pool_destroy(binding->chunk_pool);
+
+	dma_buf_unmap_attachment(binding->attachment, binding->sgt,
+				 DMA_BIDIRECTIONAL);
+	dma_buf_detach(binding->dmabuf, binding->attachment);
+	dma_buf_put(binding->dmabuf);
+	xa_destroy(&binding->bound_rxq_list);
+	kfree(binding);
+}
+
+static int netdev_restart_rx_queue(struct net_device *dev, int rxq_idx)
+{
+	void *new_mem;
+	void *old_mem;
+	int err;
+
+	if (!dev || !dev->netdev_ops)
+		return -EINVAL;
+
+	if (!dev->netdev_ops->ndo_queue_stop ||
+	    !dev->netdev_ops->ndo_queue_mem_free ||
+	    !dev->netdev_ops->ndo_queue_mem_alloc ||
+	    !dev->netdev_ops->ndo_queue_start)
+		return -EOPNOTSUPP;
+
+	new_mem = dev->netdev_ops->ndo_queue_mem_alloc(dev, rxq_idx);
+	if (!new_mem)
+		return -ENOMEM;
+
+	err = dev->netdev_ops->ndo_queue_stop(dev, rxq_idx, &old_mem);
+	if (err)
+		goto err_free_new_mem;
+
+	err = dev->netdev_ops->ndo_queue_start(dev, rxq_idx, new_mem);
+	if (err)
+		goto err_start_queue;
+
+	dev->netdev_ops->ndo_queue_mem_free(dev, old_mem);
+
+	return 0;
+
+err_start_queue:
+	dev->netdev_ops->ndo_queue_start(dev, rxq_idx, old_mem);
+
+err_free_new_mem:
+	dev->netdev_ops->ndo_queue_mem_free(dev, new_mem);
+
+	return err;
+}
+
+/* Protected by rtnl_lock() */
+static DEFINE_XARRAY_FLAGS(netdev_dmabuf_bindings, XA_FLAGS_ALLOC1);
+
+void netdev_unbind_dmabuf(struct netdev_dmabuf_binding *binding)
+{
+	struct netdev_rx_queue *rxq;
+	unsigned long xa_idx;
+	unsigned int rxq_idx;
+
+	if (!binding)
+		return;
+
+	if (binding->list.next)
+		list_del(&binding->list);
+
+	xa_for_each(&binding->bound_rxq_list, xa_idx, rxq) {
+		if (rxq->binding == binding) {
+			/* We hold the rtnl_lock while binding/unbinding
+			 * dma-buf, so we can't race with another thread that
+			 * is also modifying this value. However, the driver
+			 * may read this config while it's creating its
+			 * rx-queues. WRITE_ONCE() here to match the
+			 * READ_ONCE() in the driver.
+			 */
+			WRITE_ONCE(rxq->binding, NULL);
+
+			rxq_idx = get_netdev_rx_queue_index(rxq);
+
+			netdev_restart_rx_queue(binding->dev, rxq_idx);
+		}
+	}
+
+	xa_erase(&netdev_dmabuf_bindings, binding->id);
+
+	netdev_dmabuf_binding_put(binding);
+}
+
+int netdev_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
+				struct netdev_dmabuf_binding *binding)
+{
+	struct netdev_rx_queue *rxq;
+	u32 xa_idx;
+	int err;
+
+	if (rxq_idx >= dev->num_rx_queues)
+		return -ERANGE;
+
+	rxq = __netif_get_rx_queue(dev, rxq_idx);
+
+	if (rxq->binding)
+		return -EEXIST;
+
+	err = xa_alloc(&binding->bound_rxq_list, &xa_idx, rxq, xa_limit_32b,
+		       GFP_KERNEL);
+	if (err)
+		return err;
+
+	/* We hold the rtnl_lock while binding/unbinding dma-buf, so we can't
+	 * race with another thread that is also modifying this value. However,
+	 * the driver may read this config while it's creating its * rx-queues.
+	 * WRITE_ONCE() here to match the READ_ONCE() in the driver.
+	 */
+	WRITE_ONCE(rxq->binding, binding);
+
+	err = netdev_restart_rx_queue(dev, rxq_idx);
+	if (err)
+		goto err_xa_erase;
+
+	return 0;
+
+err_xa_erase:
+	xa_erase(&binding->bound_rxq_list, xa_idx);
+	WRITE_ONCE(rxq->binding, NULL);
+
+	return err;
+}
+
+int netdev_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
+		       struct netdev_dmabuf_binding **out)
+{
+	struct netdev_dmabuf_binding *binding;
+	static u32 id_alloc_next;
+	struct scatterlist *sg;
+	struct dma_buf *dmabuf;
+	unsigned int sg_idx, i;
+	unsigned long virtual;
+	int err;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	dmabuf = dma_buf_get(dmabuf_fd);
+	if (IS_ERR_OR_NULL(dmabuf))
+		return -EBADFD;
+
+	binding = kzalloc_node(sizeof(*binding), GFP_KERNEL,
+			       dev_to_node(&dev->dev));
+	if (!binding) {
+		err = -ENOMEM;
+		goto err_put_dmabuf;
+	}
+	binding->dev = dev;
+
+	err = xa_alloc_cyclic(&netdev_dmabuf_bindings, &binding->id, binding,
+			      xa_limit_32b, &id_alloc_next, GFP_KERNEL);
+	if (err < 0)
+		goto err_free_binding;
+
+	xa_init_flags(&binding->bound_rxq_list, XA_FLAGS_ALLOC);
+
+	refcount_set(&binding->ref, 1);
+
+	binding->dmabuf = dmabuf;
+
+	binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent);
+	if (IS_ERR(binding->attachment)) {
+		err = PTR_ERR(binding->attachment);
+		goto err_free_id;
+	}
+
+	binding->sgt =
+		dma_buf_map_attachment(binding->attachment, DMA_BIDIRECTIONAL);
+	if (IS_ERR(binding->sgt)) {
+		err = PTR_ERR(binding->sgt);
+		goto err_detach;
+	}
+
+	/* For simplicity we expect to make PAGE_SIZE allocations, but the
+	 * binding can be much more flexible than that. We may be able to
+	 * allocate MTU sized chunks here. Leave that for future work...
+	 */
+	binding->chunk_pool =
+		gen_pool_create(PAGE_SHIFT, dev_to_node(&dev->dev));
+	if (!binding->chunk_pool) {
+		err = -ENOMEM;
+		goto err_unmap;
+	}
+
+	virtual = 0;
+	for_each_sgtable_dma_sg(binding->sgt, sg, sg_idx) {
+		dma_addr_t dma_addr = sg_dma_address(sg);
+		struct dmabuf_genpool_chunk_owner *owner;
+		size_t len = sg_dma_len(sg);
+		struct net_iov *niov;
+
+		owner = kzalloc_node(sizeof(*owner), GFP_KERNEL,
+				     dev_to_node(&dev->dev));
+		owner->base_virtual = virtual;
+		owner->base_dma_addr = dma_addr;
+		owner->num_niovs = len / PAGE_SIZE;
+		owner->binding = binding;
+
+		err = gen_pool_add_owner(binding->chunk_pool, dma_addr,
+					 dma_addr, len, dev_to_node(&dev->dev),
+					 owner);
+		if (err) {
+			err = -EINVAL;
+			goto err_free_chunks;
+		}
+
+		owner->niovs = kvmalloc_array(owner->num_niovs,
+					      sizeof(*owner->niovs),
+					      GFP_KERNEL);
+		if (!owner->niovs) {
+			err = -ENOMEM;
+			goto err_free_chunks;
+		}
+
+		for (i = 0; i < owner->num_niovs; i++) {
+			niov = &owner->niovs[i];
+			niov->owner = owner;
+		}
+
+		virtual += len;
+	}
+
+	*out = binding;
+
+	return 0;
+
+err_free_chunks:
+	gen_pool_for_each_chunk(binding->chunk_pool,
+				netdev_dmabuf_free_chunk_owner, NULL);
+	gen_pool_destroy(binding->chunk_pool);
+err_unmap:
+	dma_buf_unmap_attachment(binding->attachment, binding->sgt,
+				 DMA_BIDIRECTIONAL);
+err_detach:
+	dma_buf_detach(dmabuf, binding->attachment);
+err_free_id:
+	xa_erase(&netdev_dmabuf_bindings, binding->id);
+err_free_binding:
+	kfree(binding);
+err_put_dmabuf:
+	dma_buf_put(dmabuf);
+	return err;
+}
+#endif
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 0ed292d87ae0..8f0867ae5eeb 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -9,6 +9,7 @@ 
 #include <net/xdp_sock.h>
 #include <net/netdev_rx_queue.h>
 #include <net/busy_poll.h>
+#include <net/devmem.h>
 
 #include "netdev-genl-gen.h"
 #include "dev.h"
@@ -469,10 +470,93 @@  int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
-/* Stub */
+static LIST_HEAD(netdev_rbinding_list);
+
 int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return 0;
+	struct nlattr *tb[ARRAY_SIZE(netdev_queue_dmabuf_nl_policy)];
+	struct netdev_dmabuf_binding *out_binding;
+	u32 ifindex, dmabuf_fd, rxq_idx;
+	struct net_device *netdev;
+	struct sk_buff *rsp;
+	struct nlattr *attr;
+	int rem, err = 0;
+	void *hdr;
+
+	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_DEV_IFINDEX) ||
+	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_BIND_DMABUF_DMABUF_FD) ||
+	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_BIND_DMABUF_QUEUES))
+		return -EINVAL;
+
+	ifindex = nla_get_u32(info->attrs[NETDEV_A_DEV_IFINDEX]);
+	dmabuf_fd = nla_get_u32(info->attrs[NETDEV_A_BIND_DMABUF_DMABUF_FD]);
+
+	rtnl_lock();
+
+	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
+	if (!netdev) {
+		err = -ENODEV;
+		goto err_unlock;
+	}
+
+	err = netdev_bind_dmabuf(netdev, dmabuf_fd, &out_binding);
+	if (err)
+		goto err_unlock;
+
+	nla_for_each_attr(attr, genlmsg_data(info->genlhdr),
+			  genlmsg_len(info->genlhdr), rem) {
+		if (nla_type(attr) != NETDEV_A_BIND_DMABUF_QUEUES)
+			continue;
+
+		err = nla_parse_nested(
+			tb, ARRAY_SIZE(netdev_queue_dmabuf_nl_policy) - 1, attr,
+			netdev_queue_dmabuf_nl_policy, info->extack);
+
+		if (err < 0)
+			goto err_unbind;
+
+		rxq_idx = nla_get_u32(tb[NETDEV_A_QUEUE_DMABUF_IDX]);
+
+		if (rxq_idx >= netdev->num_rx_queues) {
+			err = -ERANGE;
+			goto err_unbind;
+		}
+
+		err = netdev_bind_dmabuf_to_queue(netdev, rxq_idx, out_binding);
+		if (err)
+			goto err_unbind;
+	}
+
+	out_binding->owner_nlportid = info->snd_portid;
+	list_add(&out_binding->list, &netdev_rbinding_list);
+
+	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!rsp) {
+		err = -ENOMEM;
+		goto err_unbind;
+	}
+
+	hdr = genlmsg_put(rsp, info->snd_portid, info->snd_seq,
+			  &netdev_nl_family, 0, info->genlhdr->cmd);
+	if (!hdr) {
+		err = -EMSGSIZE;
+		goto err_genlmsg_free;
+	}
+
+	nla_put_u32(rsp, NETDEV_A_BIND_DMABUF_DMABUF_ID, out_binding->id);
+	genlmsg_end(rsp, hdr);
+
+	rtnl_unlock();
+
+	return genlmsg_reply(rsp, info);
+
+err_genlmsg_free:
+	nlmsg_free(rsp);
+err_unbind:
+	netdev_unbind_dmabuf(out_binding);
+err_unlock:
+	rtnl_unlock();
+	return err;
 }
 
 static int netdev_genl_netdevice_event(struct notifier_block *nb,
@@ -495,10 +579,37 @@  static int netdev_genl_netdevice_event(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static int netdev_netlink_notify(struct notifier_block *nb, unsigned long state,
+				 void *_notify)
+{
+	struct netlink_notify *notify = _notify;
+	struct netdev_dmabuf_binding *rbinding;
+
+	if (state != NETLINK_URELEASE || notify->protocol != NETLINK_GENERIC)
+		return NOTIFY_DONE;
+
+	rtnl_lock();
+
+	list_for_each_entry(rbinding, &netdev_rbinding_list, list) {
+		if (rbinding->owner_nlportid == notify->portid) {
+			netdev_unbind_dmabuf(rbinding);
+			break;
+		}
+	}
+
+	rtnl_unlock();
+
+	return NOTIFY_OK;
+}
+
 static struct notifier_block netdev_genl_nb = {
 	.notifier_call	= netdev_genl_netdevice_event,
 };
 
+static struct notifier_block netdev_netlink_notifier = {
+	.notifier_call = netdev_netlink_notify,
+};
+
 static int __init netdev_genl_init(void)
 {
 	int err;
@@ -511,8 +622,14 @@  static int __init netdev_genl_init(void)
 	if (err)
 		goto err_unreg_ntf;
 
+	err = netlink_register_notifier(&netdev_netlink_notifier);
+	if (err)
+		goto err_unreg_family;
+
 	return 0;
 
+err_unreg_family:
+	genl_unregister_family(&netdev_nl_family);
 err_unreg_ntf:
 	unregister_netdevice_notifier(&netdev_genl_nb);
 	return err;