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