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 [off-list ref] wrote:

quoted

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 :-)