[ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to use the right parameters
Alin Serdean
aserdean at cloudbasesolutions.com
Thu Mar 31 18:06:39 UTC 2016
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:vsairam at vmware.com]
> Trimis: Thursday, March 31, 2016 8:47 PM
> Către: Alin Serdean <aserdean at cloudbasesolutions.com>; Nithin Raju
> <nithin at vmware.com>; dev at 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" <aserdean at 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:vsairam at vmware.com]
> >> Trimis: Wednesday, March 30, 2016 3:53 AM
> >> Către: Alin Serdean <aserdean at cloudbasesolutions.com>; Nithin Raju
> >> <nithin at vmware.com>; dev at 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"
> <aserdean at 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 at 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=
> >> >
> >
More information about the dev
mailing list