Re: [PATCH RFC net-next v1 5/5] net: devmem: Implement TX path
From: Mina Almasry <hidden>
Date: 2025-01-27 22:52:51
Also in:
kvm, linux-doc, linux-kselftest, lkml, virtualization
On Thu, Dec 26, 2024 at 11:10 AM Stanislav Fomichev [off-list ref] wrote:
On 12/20, Stanislav Fomichev wrote:quoted
On 12/21, Mina Almasry wrote:quoted
void netdev_nl_sock_priv_init(struct list_head *priv)diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 815245d5c36b..eb6b41a32524 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c@@ -1882,8 +1882,10 @@ EXPORT_SYMBOL_GPL(msg_zerocopy_ubuf_ops); int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb, struct msghdr *msg, int len, - struct ubuf_info *uarg) + struct ubuf_info *uarg, + struct net_devmem_dmabuf_binding *binding) { + struct iov_iter *from = binding ? &binding->tx_iter : &msg->msg_iter;For tx, I feel like this needs a copy of binding->tx_iter: struct iov_iter tx_iter = binding->tx_iter; struct iov_iter *from = binding ? &tx_iter : &msg->msg_iter; Or something similar (rewind?). The tx_iter is advanced in zerocopy_fill_skb_from_devmem but never reset back it seems (or I'm missing something). In you case, if you call sendmsg twice with the same offset, the second one will copy from 2*offset.Can confirm that it's broken. We should probably have a mode in ncdevmem to call sendmsg with the fixed sized chunks, something like this:
Thanks for catching. Yes, I've been able to repro and I believe I fixed it locally and will include a fix with the next iteration. I also agree using a binding->tx_iter here is not necessary, and it makes the code a bit confusing as there is an iteration in msg and another one in binding and we have to be careful which to advance/revert etc. I've prototyped implementation without binding->tx_iter with help from your series on github and seems to work fine in my tests.
quoted hunk ↗ jump to hunk
@@ -912,7 +916,11 @@ static int do_client(struct memory_buffer *mem) line_size, off); iov.iov_base = NULL; - iov.iov_len = line_size; + iov.iov_len = line_size <= 4096 ?: 4096; msg.msg_iov = &iov; msg.msg_iovlen = 1;@@ -933,6 +941,8 @@ static int do_client(struct memory_buffer *mem) ret = sendmsg(socket_fd, &msg, MSG_ZEROCOPY); if (ret < 0) error(1, errno, "Failed sendmsg"); + if (ret == 0) + break; fprintf(stderr, "sendmsg_ret=%d\n", ret);I can put it on my todo to extend the selftests..
FWIW I've been able to repro this and extended the tests to catch this; those changes should come with the next iteration. -- Thanks, Mina