I have added my response underneath. Thanks, Sairam Venugopal
On 9/14/15, 2:14 PM, "Alin Serdean" <[email protected]> wrote: >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://urldefense.proofpoint.com/v2/url?u=https-3A__msdn.microsoft.com_en >-2Dus_library_windows_hardware_ff568840-2528v-3Dvs.85-2529.aspx&d=BQIFAg&c >=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dcruz40PROJ40ROzSpxyQSLw6fc >rOWpJgEcEmNR3JEQ&m=ExYraM9o-Pfuh9GGoulFsO4aDPXT9KTIqsfbz0SZeT8&s=n91dZBAap >M63Q5s8alYDHlAyaACeBmYW_d-MeAQqeOY&e= > >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. Sai: You are right. You can ignore this. > >>@@ -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? >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. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
