Thanks for the review Sairam. I trimmed the message a bit.
See my answers inlined. > status = OvsAllocateNBLContext(context, newNbl); diff --git >a/datapath-windows/ovsext/Checksum.c >b/datapath-windows/ovsext/Checksum.c >index 510a094..5d9b035 100644 >--- a/datapath-windows/ovsext/Checksum.c >+++ b/datapath-windows/ovsext/Checksum.c Sai: Are there any tests to validate these changes in Checksum.c? If not, can someone else ack it. I forgot to add a comment in the code which details on what should happen in tcp segmentation. I will add it in v2. But basically it is the following: https://msdn.microsoft.com/en-us/library/windows/hardware/ff568840%28v=vs.85%29.aspx I used a tool to test the changes which is based on winrm. >- csum = CalculateOnesComplement(src, csLen, csum, TRUE); Sai: You can reuse the swapEnd below - !swapEnd >+ csum = CalculateOnesComplement(src, csLen, csum, !(1 & >csumDataLen)); > fold64(csum); > > csumDataLen -= csLen; csumDataLen changes in the inner loop, I cannot reuse swapend. >@@ -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: 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. Alin _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
