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

[Mina Almasry]

On Wed, Mar 6, 2024 at 4:38 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2024/3/6 5:17, Mina Almasry wrote:
> > On Tue, Mar 5, 2024 at 4:55 AM Yunsheng Lin <linyunsheng@huaweicom> 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@xxxxxxxxxx>
> >>> Signed-off-by: Kaiyuan Zhang <kaiyuanz@xxxxxxxxxx>
> >>> Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> >>>
> >>> ---
> >>>
> >>> 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@xxxxxxxxxx>
> >>> + *                   Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> >>> + *                   Kaiyuan Zhang <kaiyuanz@xxxxxxxxxx
> >>> + */
> >>> +
> >>> +#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@xxxxxxxxxx/#25590262
https://lore.kernel.org/netdev/20230815171638.4c057dcd@xxxxxxxxxx/

> >
> >>> +
> >>> +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?
> >>
>


-- 
Thanks,
Mina