On Thu, Dec 26, 2024 at 11:10 AM Stanislav Fomichev stfomichev@gmail.com wrote:
On 12/20, Stanislav Fomichev wrote:
On 12/21, Mina Almasry wrote:
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.
@@ -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.