>
>>@@ -504,9 +517,14 @@ CalculateChecksumNB(const PNET_BUFFER nb,
>>+        /*
>>+         * To this point we do not have VXLAN offloading.
>>+         * Apply defined checksums
>>+         */
>
>Sai: Correct me if am wrong, this computes checksum for the inner 
>packet if there is no segmentation.
>Alin: You are correct.
>Sai: In case the inner packet is segmented via LSO, shouldn't there be 
>additional checksum calculations for each inner-segment?
>Alin: it is computed for each inner segment OvsTcpSegmentNBL-> 
>FixSegmentHeader which computes tcp checksums for each NB created by 
>NdisAllocateFragmentNetBufferList.
>          Looking again over the code I think we could change it to 
>compute IP checksum only once and not for each segment. I will change 
>this in v2.

Sai: OvsTcpSegmentNBL gets called prior to this code. Are you going to reorder 
the code to achieve this?

Alin: I will change FixSegmentHeader to use a predefined value for ip checksums 
instead of computing it for every net buffer.
Maybe I misunderstood your commentary, could you please detail your thoughts.

>Sai: If this is just for non-lso packet, then you can do the following 
>prior to creating *newNbl to avoid having to compute bufferStart.
>Alin: I do not want to compute the checksum if the copy failed
>
>>+        /* Compute IP checksum only if the NIC does not support
>>offloading */
>
>Sai: I don't think this check is appropriate.
>csumInfo is still pointing at the inner packet. The ipHdr is for outer 
>packet.
>This just checks if the underlying VM had offloaded the 
>IpHeaderChecksum calculation.
>
>>+        if (!csumInfo.Transmit.IpHeaderChecksum) {
>>+            ipHdr->check = 0;
>>+            ipHdr->check = IPChecksum((UINT8 *)ipHdr, sizeof *ipHdr, 0);
>>+        }
>> 
>>         /* UDP header */
>>         udpHdr = (UDPHdr *)((PCHAR)ipHdr + sizeof *ipHdr);
>>--
>
>Alin: I am going on the following assumption:
>If you have LSO or IP/TCP/UDP checksums enabled in your VM it means 
>that the NIC on which the switch is created has them enabled.
>The check above means that the VM did not have IP checksum enabled, 
>which means that the physical one did not had it enabled also, telling 
>us to compute it.
>If the VM had it enabled, means the physical one had it thus meaning we 
>should not compute it.
>It is a rather hard assumption but I will leave it for more discussion 
>if we should leave it or not.

Sai: I think you can skip this check and instead offload the task to NDIS.
I did something similar in my patch for enabling checksum in STT.
I was able to get pings to work even when the offloads were disabled on the 
Hyper-V Host, but enabled in the underlying VM.

Alin: I will look over your patch tonight. It isn't a question if it works or 
not. The idea is that I am not fond of lying to the user of the VM that we 
support capability which doesn't exist. It is a virtual 
http://www.hackerthreads.org/Topic-10367 :).

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to