I already sent a patch that should fix master but does not solve fully resolve our problem with the key length for future use.
I will drop that patch and acked the one you have instead and resend the full solution for it tomorrow. Regarding the conntrack patch itself I have some comments and I will send them tomorrow/Monday. So the two patches in first. Alin. > -----Mesaj original----- > De la: Sairam Venugopal [mailto:vsai...@vmware.com] > Trimis: Thursday, March 31, 2016 8:47 PM > 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, > > 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