Message-ID: <ZvYLfLKpdZj58YKo@mini-arch>
Date: Thu, 26 Sep 2024 18:33:48 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [RFC PATCH net-next v1 1/4] net: devmem: Implement TX path

On 09/26, Mina Almasry wrote:
> On Fri, Sep 13, 2024 at 8:09 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
> >
> > Preliminary implementation of the TX path. The API is as follows:
> >
> > 1. bind-tx netlink call to attach dmabuf for TX; queue is not
> >    required, only netdev for dmabuf attachment
> > 2. a set of iovs where iov_base is the offset in the dmabuf and iov_len
> >    is the size of the chunk to send; multiple iovs are supported
> > 3. SCM_DEVMEM_DMABUF cmsg with the dmabuf id from bind-tx
> > 4. MSG_SOCK_DEVMEM sendmsg flag to mirror receive path
> >
> > In sendmsg, lookup binding by id and refcnt it for every frag in the
> > skb. None of the drivers are implemented, but skb_frag_dma_unmap
> > should return proper DMA address. Extra care (TODO) must be taken in the
> > drivers to not dma_unmap those mappings on completions.
> >
> > The changes in the kernel/dma/mapping.c are only required to make
> > devmem work with virtual networking devices (and they expose 1:1
> > identity mapping) and to enable 'loopback' mode. Loopback mode
> > lets us test TCP and UAPI paths without having real HW. Not sure
> > whether it should be a part of a real upstream submission, but it
> > was useful during the development.
> >
> > TODO:
> > - skb_page_unref and __skb_frag_ref seem out of place; unref paths
> >   in general need more care
> > - potentially something better than tx_iter/tx_vec with its
> >   O(len/PAGE_SIZE) lookups
> > - move xa_alloc_cyclic to the end
> > - potentially better separate bind-rx and bind-tx;
> >   direction == DMA_TO_DEVICE feels hacky
> > - rename skb_add_rx_frag_netmem to skb_add_frag_netmem
> >
> 
> Thank you very much for this, and sorry for the late reply. I think I
> got busy with some post RX merge follow ups and then other stuff.
> Coming back to look at this now.

Sure and thanks for pushing it further!

> This looks like a great start. Agreed with many of the todos above,
> and in addition some things I wanna look deeper into (but not
> necessarily set on changing yet):

[..]
 
> Loopback: I do plan to drop that. My understanding is that it's a bit
> complicated to make work. In addition to the mapping.c changes, the TX
> zerocopy code falls back to copying for loopback for reasons I don't
> have my head wrapped around. devmem can't be copied. You get around
> that with a change in skb_copy_ubufs but I'm not sure we can assume
> success there. In any case I don't have a use case for loop back and
> it can be mode to work properly later.

Up to you. It was useful during working on on the syscall side, but
I understand that it's an unnecessary complication.

> control path locking: You added net_devmem_dmabuf_lock, but AFAICT
> dma-buf allocation should be already mutexed by rtnl_lock. Maybe I
> missed something. I'll take a deeper look.

Yeah, agreed, I think it was a leftover from my some other non-rcu
attempt.

> fast path locking: you use rcu, which is a good way to do it. I had
> something else in mind, where we associate the binding with a socket
> and keep it alive for the duration of the socket and (I think) no need
> to lock anymore. Not sure which is better. Associating the binding
> with a socket does require uapi. But it may be good to keep the
> binding alive while the socket is using it anyway, rather than the
> sendmsg returning -EINVAL if the binding has been freed underneath it.
> I'll take a deeper look.

That should work as well as long as we can bind multiple sockets.
Maybe that's even better because it avoids xa_load on every sendmsg,
so go for it.

> get_page/put_page: I was thinking we need to implement
> get_netmem/put_netmem equivalents as the tx path uses
> get_page/put_page and page_pool refcounting is not used there. You
> seem to instead ref/unref the binding. That may be fine, but we may
> need get_page/put_page equivalents for netmem eventually and may be
> worth getting them done now. I need to rack my brain a bit more.

That should work as well, I haven't spent a ton of time polishing that
part. Note that in my tests, I saw half throughput with this API+udmabuf
vs regular MSG_ZEROCOPY. So it's either my sloppy locking, xa_load
or iovec parsing. Or something else, idk :-)

Powered by blists - more mailing lists