Hi Alin, Looks like our email spam filters are aggressive about letting through ovs-dev patches. I just saw your patch on Patchwork. I tried to make the changes with KeyLen and realized it became complicated as we added in more fields - ct_state, ct_zone, ct_mark, ct_label, priority etc., Since not all fields may be present, you had to keep track of the offsets and increment/decrement them accordingly. Also, in case of recirc/ct_state where the fields are set in datapath, you will need to increment the keylen and offset in Actions.c as well. I will look forward to your newer patch and see if that simplifies this. I will incorporate your comments and resend the patches. I will spin the first two patches separately and keep Conntrack as a separate patch to keep it simple.
Thanks, Sairam On 3/31/16, 11:06 AM, "Alin Serdean" <aserd...@cloudbasesolutions.com> wrote: >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
