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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 13 Jun 2024 12:22:54 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 13/06/2024 12:14, Stefano Brivio wrote:
> > On Thu, 13 Jun 2024 10:24:11 +0200
> > Laurent Vivier <lvivier@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