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

[Laurent Vivier <lvivier@redhat.com>]

On 13/06/2024 12:49, Stefano Brivio wrote:
> 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?
> 

Yes.
I prepare a new version of the series with the changes.

Thanks,
Laurent