Re: [Qemu-devel] [RFC Patch v2 04/10] virtio-net rsc: Detailed IPv4 and General TCP data coalescing

[Jason Wang <jasowang@redhat.com>]

On 02/01/2016 04:29 PM, Wei Xu wrote:
>
> On 02/01/2016 02:21 PM, Jason Wang wrote:
>
>>
>> On 02/01/2016 02:13 AM, wexu@redhat.com wrote:
>>> From: Wei Xu <wei@wei-thinkpad.nay.redhat.com>
>>>
>>> Since this feature also needs to support IPv6, and there are
>>> some protocol specific differences difference for IPv4/6 in the header,
>>> so try to make the interface to be general.
>>>
>>> IPv4/6 should set up both the new and old IP/TCP header before invoking
>>> TCP coalescing, and should also tell the real payload.
>>>
>>> The main handler of TCP includes TCP window update, duplicated ACK
>>> check
>>> and the real data coalescing if the new segment passed invalid filter
>>> and is identified as an expected one.
>>>
>>> An expected segment means:
>>> 1. Segment is within current window and the sequence is the expected
>>> one.
>>> 2. ACK of the segment is in the valid window.
>>> 3. If the ACK in the segment is a duplicated one, then it must less
>>> than 2,
>>>     this is to notify upper layer TCP starting retransmission due to
>>> the spec.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> ---
>>>   hw/net/virtio-net.c | 127
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 124 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index cfbac6d..4f77fbe 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -41,6 +41,10 @@
>>>     #define VIRTIO_HEADER   12    /* Virtio net header size */
>>>   #define IP_OFFSET (VIRTIO_HEADER + sizeof(struct eth_header))
>>> +#define TCP_WINDOW      65535
>> The name is confusing, how about TCP_MAX_WINDOW_SIZE ?
>
> Sounds better, will take it in.
>
>>
>>> +
>>> +/* IPv4 max payload, 16 bits in the header */
>>> +#define MAX_IP4_PAYLOAD  (65535 - sizeof(struct ip_header))
>>>     #define MAX_VIRTIO_IP_PAYLOAD  (65535 + IP_OFFSET)
>>>   @@ -1670,13 +1674,130 @@ out:
>>>       return 0;
>>>   }
>>>   +static int32_t virtio_net_rsc_handle_ack(NetRscChain *chain,
>>> NetRscSeg *seg,
>>> +                                 const uint8_t *buf, struct
>>> tcp_header *n_tcp,
>>> +                                 struct tcp_header *o_tcp)
>>> +{
>>> +    uint32_t nack, oack;
>>> +    uint16_t nwin, owin;
>>> +
>>> +    nack = htonl(n_tcp->th_ack);
>>> +    nwin = htons(n_tcp->th_win);
>>> +    oack = htonl(o_tcp->th_ack);
>>> +    owin = htons(o_tcp->th_win);
>>> +
>>> +    if ((nack - oack) >= TCP_WINDOW) {
>>> +        return RSC_FINAL;
>>> +    } else if (nack == oack) {
>>> +        /* duplicated ack or window probe */
>>> +        if (nwin == owin) {
>>> +            /* duplicated ack, add dup ack count due to whql test
>>> up to 1 */
>>> +
>>> +            if (seg->dup_ack_count == 0) {
>>> +                seg->dup_ack_count++;
>>> +                return RSC_COALESCE;
>>> +            } else {
>>> +                /* Spec says should send it directly */
>>> +                return RSC_FINAL;
>>> +            }
>>> +        } else {
>>> +            /* Coalesce window update */
>> Need we flush this immediately consider it was a window update?
>
> The flowchart in the spec says this can be coalesced as normal.
>
> https://msdn.microsoft.com/en-us/library/windows/hardware/jj853325%28v=vs.85%29.aspx

I see.

>
>
>>
>>> +            o_tcp->th_win = n_tcp->th_win;
>>> +            return RSC_COALESCE;
>>> +        }
>>> +    } else {
>> What if nack < oack here?
>
> That should happen, the  modulo-232 arithmetic check at the begin of
> this function will keep the ack is in the current window. 

Ok.

Thanks