Re: [PATCH v8 7/8] vhost-user: add vhost-user

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 23 Oct 2024 17:27:49 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 22/10/2024 14:59, Laurent Vivier wrote:
> > On 17/10/2024 02:10, Stefano Brivio wrote:  
> >> On Wed, 16 Oct 2024 11:41:34 +1100
> >> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>  
> >>> On Tue, Oct 15, 2024 at 09:54:38PM +0200, Stefano Brivio wrote:  
> >>>> [Still partial review]  
> >>> [snip]  
> >>>>> +    if (peek_offset_cap)
> >>>>> +        already_sent = 0;
> >>>>> +
> >>>>> +    iov_vu[0].iov_base = tcp_buf_discard;
> >>>>> +    iov_vu[0].iov_len = already_sent;  
> >>>>
> >>>> I think I had a similar comment to a previous revision. Now, I haven't
> >>>> tested this (yet) on a kernel with support for SO_PEEK_OFF on TCP, but
> >>>> I think this should eventually follow the same logic as the (updated)
> >>>> tcp_buf_data_from_sock(): we should use tcp_buf_discard only if
> >>>> (!peek_offset_cap).
> >>>>
> >>>> It's fine to always initialise VIRTQUEUE_MAX_SIZE iov_vu items,
> >>>> starting from 1, for simplicity. But I'm not sure if it's safe to pass a
> >>>> zero iov_len if (peek_offset_cap).  
> >>>  
> >>>> I'll test that (unless you already did) -- if it works, we can fix this
> >>>> up later as well.  
> >>>
> >>> I believe I tested it at some point, and I think we're already using
> >>> it somewhere.  
> >>
> >> I tested it again just to be sure on a recent net.git kernel: sometimes
> >> the first test in passt_vu_in_ns/tcp, "TCP/IPv4: host to guest: big
> >> transfer" hangs on my setup, sometimes it's the "TCP/IPv4: ns to guest
> >> (using loopback address): big transfer" test instead.
> >>
> >> I can reproduce at least one of the two issues consistently (tests
> >> stopped 5 times out of 5).
> >>
> >> The socat client completes the transfer, the server is still waiting
> >> for something. I haven't taken captures yet or tried to re-send from
> >> the client.
> >>
> >> It all works (consistently) with an older kernel without support for
> >> SO_PEEK_OFF on TCP, but also on this kernel if I force peek_offset_cap
> >> to false in tcp_init().
> >>  
> > 
> > I have a fix for that but there is an error I don't understand:
> > when I run twice the test, the second time I have:
> > 
> > guest:
> > # socat -u TCP4-LISTEN:10001 OPEN:test_big.bin,create,trunc
> > # socat -u TCP4-LISTEN:10001 OPEN:test_big.bin,create,trunc
> > 2024/10/22 08:51:58 socat[1485] E bind(5, {AF=2 0.0.0.0:10001}, 16): Address already in use
> > 
> > host:
> > $ socat -u OPEN:test/big.bin TCP4:127.0.0.1:10001
> > 
> > If I wait a little it can work again several times and fails again.
> > 
> > Any idea?
> > 
> > The patch is:
> > diff --git a/tcp_vu.c b/tcp_vu.c
> > index 78884c673215..83e40fb07a03 100644
> > --- a/tcp_vu.c
> > +++ b/tcp_vu.c
> > @@ -379,6 +379,10 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn 
> > *conn)
> >                             conn->seq_ack_from_tap, conn->seq_to_tap);
> >                  conn->seq_to_tap = conn->seq_ack_from_tap;
> >                  already_sent = 0;
> > +               if (tcp_set_peek_offset(conn->sock, 0)) {
> > +                       tcp_rst(c, conn);
> > +                       return -1;
> > +               }
> >          }
> > 
> >          if (!wnd_scaled || already_sent >= wnd_scaled) {
> > @@ -389,14 +393,13 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn 
> > *conn)
> > 
> >          /* Set up buffer descriptors we'll fill completely and partially. */
> > 
> > -       fillsize = wnd_scaled;
> > +       fillsize = wnd_scaled - already_sent;
> > 
> >          if (peek_offset_cap)
> >                  already_sent = 0;
> > 
> >          iov_vu[0].iov_base = tcp_buf_discard;
> >          iov_vu[0].iov_len = already_sent;
> > -       fillsize -= already_sent;
> > 
> >          /* collect the buffers from vhost-user and fill them with the
> >           * data from the socket
> > 
> >   
> 
> For the moment, I can see a behavior change of recvmsg() with the new kernel.
> 
> without peek_offset_cap, if no new data is available, it returns "already_sent", so it 
> enters in (found with tcp_vu.c but code samples from tcp_buf.c):
> 
> 	==> recvmsg() returns already_sent, so len > 0  
> 
>          ...
>          sendlen -= already_sent; ==> here sendlen becomes 0
> 
>          if (sendlen <= 0) {
>                  conn_flag(c, conn, STALLED);
>                  return 0;
>          }
> 
> With peek_offset, it returns -1, so it enters in:

This is expected, I think (and unfortunately not documented).

> 
>          if (len < 0)
>                  goto err;
> ...
> err:
>          if (errno != EAGAIN && errno != EWOULDBLOCK) {

But errno here should be EAGAIN, so yes, it looks buggy to me in the
sense that:

>                  ret = -errno;
>                  tcp_rst(c, conn);
>          }

we return 0 here without setting the STALLED flag. While it should
be fixed, that flag is some kind of optimisation, so this doesn't
really explain the issue that I mentioned in
20241022201914.072f7c7d@elisabeth:

  https://archives.passt.top/passt-dev/20241022201914.072f7c7d@elisabeth/

As a quick fix, you should probably do this in tcp_vu_data_from_sock():

	if (peek_offset_cap)	/* add this condition */
		len -= already_sent;

	if (len <= 0 || (peek_offset_cap && len == -1 && errno == EAGAIN))
		/* change this condition */
		...

...or you mean that due to this behaviour you don't call vu_queue_rewind()
and that causes troubles?

-- 
Stefano