[ovs-dev] [PATCH v3] datapath-windows: Add GRE checksum

[Nithin Raju]

>>
>>-----Mesaj original-----
>> De la: Nithin Raju [mailto:nithin at vmware.com]
>> Trimis: Wednesday, June 8, 2016 10:10 PM
>> Către: Alin Serdean <aserdean at cloudbasesolutions.com>;
>> dev at openvswitch.org
>> Subiect: Re: [ovs-dev] [PATCH v3] datapath-windows: Add GRE checksum
>> 
>> 
>> >+    /* Get a contignuous buffer for the maxmimum length of a GRE
>> >+ header
>> >*/
>> >+    bufferStart = NdisGetDataBuffer(curNb, OVS_MAX_GRE_LGTH, NULL, 1,
>> >+ 0);
>> 
>> Sorry we have to go back and forth on this. Like I mentioned in the
>>previous
>> email, the idea is to get a contiguous chunk of memory so we can walk
>>all the
>> header until the GRE header. It is a good idea to use
>> NdisGetDataBuffer() instead of copying the NBL. But, we won¹t avoid the
>> copied NBL anyway since decap has to happen on the copied NBL.
>[Alin Gabriel Serdean: ] Agreed. I only wanted a check to make sure the
>whole ETH, IP, GRE was contiguous.
>> 
>> In any case, NdisGetDataBuffer() has some pitfalls:
>> "If the requested data in the buffer is contiguous, this return value
>>is a
>> pointer to a location that NDIS provides. If the data is not
>>contiguous, NDIS
>> uses the Storage parameter as follows:
>> 
>> * If the Storage parameter is non-NULL, NDIS copies the data to the
>>buffer at
>> Storage. This return value is the pointer passed to the Storage
>>parameter.
>> * If the Storage parameter is NULL, this return value is NULL.²
>> 
>> So, if the first MDL does not fit the headers until the GRE headers, we
>>need
>> to pass explicit memory to NdisGetDataBuffer() in argument #3 in order
>>for
>> NDIS to copy it over to a contiguous chunk of memory.
>[Alin Gabriel Serdean: ] We have to take into consideration that NDIS 6.3
>supports NVGRE and in NDIS 6.4 GRE
>(https://urldefense.proofpoint.com/v2/url?u=https-3A__support.microsoft.co
>m_en-2Dus_kb_3022776&d=CwIFAA&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uE
>s&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=Xih_0c1MPDafKSkW1nO5-mIu
>tm1WK_ZmMcu7likx3jM&s=OR7CpNr_D_lG7SqFfSsi2qb63XGh1WaXVKtbLqjfT8s&e=  ).

[Nithin]: I am not sure how NDIS supporting GRE and NVGRE would affect
help with putting the ETH + IP + GRE header in a contiguous buffer. The
MDLs are typically formed by the mini port driver, and NDIS will either
run a parser on top of it or use an offload if the NIC supports it.

>Packets can be split in multiple Mdl's, if the miniport supports it, as
>early as at the beginning of the upper-layer-protocol
>(https://urldefense.proofpoint.com/v2/url?u=https-3A__msdn.microsoft.com_e
>n-2Dus_library_windows_hardware_ff571065-28v-3Dvs.85-29.aspx&d=CwIFAA&c=Sq
>cl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxd
>DuTLBYD3JqV80&m=Xih_0c1MPDafKSkW1nO5-mIutm1WK_ZmMcu7likx3jM&s=sZElhQvcdQPd
>zDWCX8jmxd33zxPVzkGsudCT0Z8pdr4&e= ) and if the split header is not
>greater than the maximum header size which is configurable. In the case
>of VXLAN / STT we can be in the case above, because of the UDP/TCP like
>packet

[Nithin]: Yes, in fact even for NICs that support header-split (mostly
Intel), the payload of the packet WILL NOT be in the first MDL. In case of
GRE, the split would happen at the IP header itself. So, GRE header would
not be in the contigous buffer.

>, but for GRE things differ since there are cards which support
>offloading:
>https://urldefense.proofpoint.com/v2/url?u=https-3A__msdn.microsoft.com_en
>-2Dus_library_windows_hardware_dn605832-28v-3Dvs.85-29.aspx&d=CwIFAA&c=Sqc
>l0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdD
>uTLBYD3JqV80&m=Xih_0c1MPDafKSkW1nO5-mIutm1WK_ZmMcu7likx3jM&s=A4LA4vS20hf4l
>keq-wjCc6N9kbBMqswRnC-SSulTF6s&e=
>"The protocol driver will guarantee that all the prepended encapsulation
>headers (ETH, IP, GRE) will be physically contiguous and will be in the
>first MDL of the packet."


>I do not think we will be in a situation in which a GRE header will not
>be continuous.
>If you consider we should still have a fragmented header problem, I will
>allocate a buffer and execute another NdisGetDataBuffer call if the first
>one failed.

[Nithin]: The documention you point to is to the send direction. In case
of receive, where does it say that the protocol/filter driver can expect
the miniport driver to pack the ETH + IP + GRE header in one contiguous
buffer?

In any case, if you are particular that the code is correct, I am ok with
the patch. I was trying to give suggestions to make the code solid.

Acked-by: Nithin Raju <nithin at vmware.com>

-- Nithin