On Thu, Dec 26, 2024 at 11:10 AM Stanislav Fomichev
<stfomichev@xxxxxxxxx> 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.
--
Thanks,
Mina
