Hi Alin, Can you look at the current patch instead? I feel it is better to fix the Master first and then make other refactors instead of leaving it at a broken state.
Thanks, Sairam On 3/29/16, 6:53 PM, "Alin Serdean" <aserd...@cloudbasesolutions.com> wrote: >I acked the port part. I will send out a patch that deals with the keylen >soon. I found some more things I don't like. > >Alin. >> -----Mesaj original----- >> De la: Sairam Venugopal [mailto:vsai...@vmware.com] >> Trimis: Wednesday, March 30, 2016 3:53 AM >> Către: Alin Serdean <aserd...@cloudbasesolutions.com>; Nithin Raju >> <nit...@vmware.com>; dev@openvswitch.org >> Subiect: Re: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation >> to use the right parameters >> >> Hi Alin, >> >> I have sent out a newer series of patches with the changes. These are >> necessary to fix the master and test out Connection Tracking patch. We >>can >> circle back and cleanup Flow.c to use KeyLen and align it by 8, after >>ensuring >> that things are working properly. >> >> >> Thanks, >> Sairam >> >> On 3/29/16, 9:28 AM, "Alin Serdean" <aserd...@cloudbasesolutions.com> >> wrote: >> >> >Comments inlined. >> > >> > >> > >> >Thanks, >> > >> >Alin. >> > >> >> -----Mesaj original----- >> > >> >> This comparison determines the value for ŒisRecv¹ parameter. ŒisRecv¹ >> > >> >> determines whether the OOB data should be interpreted as receive data >> >> or >> > >> >> send data. So, the existing code is checking for: >> > >> >> srcPortNo == switchContext->virtualExternalPortId >> > >> >> >> > >> >> Left side data is a UINT32 and right side data is a >>NDIS_SWITCH_PORT_ID. >> > >> >> So, clearly this comparison is not right since we are comparing >> >>different >> > >> >> types. They are not expected to have the same value even if they are >> > >> >> representing the same vport. >> > >> >> >> > >> >[Alin Gabriel Serdean: ] from the header ntddndis.h: >> > >> >typedef UINT32 NDIS_SWITCH_PORT_ID, *PNDIS_SWITCH_PORT_ID; >> > >> >I don't think it is a problem of type but a problem with the number >> >itself. >> > >> >> The proposed fix at least makes sure that we are comparing the right >> >>types. >> > >> >> So, I¹d go with it. Is the comparison right? It seems so. Basically >> >>we want to >> > >> >> determine if the packet came from a VIF or a physical NIC. >> > >> >> ŒfwdDetail¹ is a reliable way of doing it. Only way this could mess >> >>up is if we >> > >> >> mean to explicitly change the ŒsrcPortNo¹ during tunneling. In such >> >>cases, >> > >> >> the 'fwdDetail->SourcePortId¹ will end up different from the >> >>ŒsrcPortNo¹. >> > >> >> This is exactly why we use ŒsrcPortNo¹ for comparison around the code >> >> to >> > >> >> allow flexibility. >> > >> >[Alin Gabriel Serdean: ] fwdDetail is not reliable in our case because >> >we could have cloned the nbl and not update the OOB data and this is >> >not what we want to do in our case. We want to reprocess the packet as >> >it came from the same input port >> >(https://urldefense.proofpoint.com/v2/url?u=https- >> 3A__github.com_openvs >> >wit >> >ch_ovs_blob_master_ofproto_ofproto-2Ddpif-2Drid.h-23L64- >> 2DL69&d=BQIGaQ& >> >c=S >> >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- >> uEs&r=Dcruz40PROJ40ROzSpxyQSLw6f >> >crO >> >WpJgEcEmNR3JEQ&m=SiAZF6ppbO8xujF9FCVi7GvwJdKI2aCc81fTzQUV0ZA >> &s=dzc7xmRZ >> >NyK >> >wLwlgvoBr97WN7PfuX_f4NXQjzkqcLVU&e= ) >> > >> >Indeed, there is a problem because srcportno == vport->portno and we >> >should use in our case vport->portId when doing the comparison. I'll >> >send out a patch to update it. >> > >> >> >> > >> >> >> > >> >> In any case, the problematic case here is tunneling + recirc. We can >> >>deal with >> > >> >> that in a subsequent patch. For now, this is the right fix. >> > >> >> >> > >> >> I¹d also add a XXX comment to investigate that "tunneling + recirc² >> >>case in the >> > >> >> future. >> > >> >> >> > >> >> > &ovsFwdCtx.layers, >> > >> >> > ovsFwdCtx.switchContext, >> >> > diff >> > >> >> >--git a/datapath-windows/ovsext/Flow.c b/datapath- >> > >> >> windows/ovsext/Flow.c >> > >> >> >index 02c41b7..d49697c 100644 >> > >> >> >--- a/datapath-windows/ovsext/Flow.c >> > >> >> >+++ b/datapath-windows/ovsext/Flow.c >> > >> >> >@@ -2133,6 +2133,9 @@ OvsLookupFlow(OVS_DATAPATH *datapath, >> > >> >> > >> > >> >> > if (!hashValid) { >> > >> >> > *hash = OvsJhashBytes(start, size, 0); >> > >> >> >+ if (key->recircId) { >> > >> >> >+ *hash = OvsJhashWords((UINT32*)hash, 1, key->recircId); >> > >> >> >+ } >> > >> >> >> > >> >> Ok, we have two options: >> > >> >> 1. Increment ŒkeyLen¹ and include recircId as part of it. So, while >> >>hashing we >> > >> >> make sure that recircId gets included. >> > >> >> 2. Explicitly add Œkey->recirId¹ during hashing. >> > >> >> >> > >> >> I¹m ok with either way. The ŒkeyLen¹ approach is more efficient for >> >> now, >> > >> >> since it avoids a Œif¹ check and also an additional function call. In >> >>the future, if >> > >> >> we have a lot of meta data such as ŒrecircId¹, then we should >> >> consider >> > >> >> adding a Œkey->metaDataLen¹ and go from there. For now, I¹d add a >> > >> >> comment in ŒOvsFlowKey¹ to set future direction, and go with the >> > >> >> ŒkeyLen¹ approach. >> > >> >[Alin Gabriel Serdean: ] I would go with option one so we maintain the >> >same of hashing we have at the moment. >> > >> >> >> > >> >> Of course, there¹s a bug when ŒkeyLen¹ gets set in >> > >> >> _MapKeyAttrToFlowPut(). >> > >> >> The value gets incremented due to ŒrecircId¹ but gets reset again. >> > >> >> >> > >> >> > } >> > >> >> > >> > >> >> > head = &datapath->flowTable[HASH_BUCKET(*hash)]; >> > >> >> >> > >> >> _______________________________________________ >> > >> >> dev mailing list >> > >> >> dev@openvswitch.org >> > >> >> >> >>https://urldefense.proofpoint.com/v2/url?u=http- >> 3A__openvswitch.org_ma >> >>ilm >> >>an_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw- >> YihVMNtXt-uEs >> >>&r= >> >>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=SiAZF6ppbO8xu >> jF9FCVi7Gvw >> >>JdK >> I2aCc81fTzQUV0ZA&s=rvvo6cM83_V6xVB2DDLyTXvP7e4vuYT5SLhq9HLgLT4& >> e= >> > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
