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

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?

-- 
Stefano