Re: [PATCH v6 1/8] tcp: extract buffer management from tcp_send_flag()

On 13/06/2024 12:49, Stefano Brivio wrote:
...
  On Thu, 13 Jun 2024 12:22:54 +0200
 Laurent Vivier <lvivier(a)redhat.com> wrote:
 
  On 13/06/2024 12:14, Stefano Brivio wrote:
  On Thu, 13 Jun 2024 10:24:11 +0200
 Laurent Vivier <lvivier(a)redhat.com> wrote:
    
  Hi,

 I think the problem can be because tcp_l2_buf_fill_headers() has been moved out of
 tcp_prepare_flags() and so moved after:

            if (th->fin || th->syn)
                    conn->seq_to_tap++;

 and con->seq_to_tap is also a parameter of tcp_l2_buf_fill_headers(). So it is
increased
 before and not after.

 Could you try:

 diff --git a/tcp.c b/tcp.c
 index 6800209d4122..647f42291fcf 100644
 --- a/tcp.c
 +++ b/tcp.c
 @@ -1607,6 +1607,7 @@ static int tcp_prepare_flags(struct ctx *c, struct tcp_tap_conn
*conn,
            if (!tcp_update_seqack_wnd(c, conn, flags, &tinfo) && !flags)
                    return 0;

 +       *optlen = 0;
            if (flags & SYN) {
                    int mss;

 @@ -1643,7 +1644,6 @@ static int tcp_prepare_flags(struct ctx *c, struct tcp_tap_conn
*conn,
                    *data++ = conn->ws_to_tap;
            } else if (!(flags & RST)) {
                    flags |= ACK;
 -               *optlen = 0;
            }

            th->doff = (sizeof(*th) + *optlen) / 4;
 @@ -1663,10 +1663,6 @@ static int tcp_prepare_flags(struct ctx *c, struct tcp_tap_conn
*conn,
            if (th->fin)
                    conn_flag(c, conn, ACK_FROM_TAP_DUE);

 -       /* RFC 793, 3.1: "[...] and the first data octet is ISN+1." */
 -       if (th->fin || th->syn)
 -               conn->seq_to_tap++;
 -
            return 1;
     }

 @@ -1702,6 +1698,10 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn
*conn,
 int flags)
                                            conn->seq_to_tap);
            iov[TCP_IOV_PAYLOAD].iov_len = l4len;

 +       /* RFC 793, 3.1: "[...] and the first data octet is ISN+1." */
 +       if (th->fin || th->syn)
 +               conn->seq_to_tap++;
 +
            if (flags & DUP_ACK) {
                    struct iovec *dup_iov;
                    int i; 
 Ah, yes, good catch, I missed this one! It works. Note that it needs to
 be:

 	if ((flags & FIN) || (flags & SYN))
 		...

 because we don't have a TCP header there, yet.
     
 I'm wondering if I can do:

 +       seq = conn->seq_to_tap;
           ret = tcp_prepare_flags(c, conn, flags, &payload->th,
                                   payload->opts, &optlen);
           if (ret <= 0)
                   return ret;

           l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL,
 -                                       conn->seq_to_tap);
 +                                       seq);

 to keep  the flags in tcp_prepare_flags()? 
 
 Maybe, yes. I don't really have a strong preference. On the other hand
 tcp_send_flag() has to deal with the DUP_ACK case on its own anyway.
 
 But sure, this version would be a bit cleaner, and if vhost-user code
 will then want to call tcp_prepare_flags() without tcp_send_flag(),
 it avoids code duplication, I guess?
  
Yes.
I prepare a new version of the series with the changes.

Thanks,
Laurent