[ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to use the right parameters

Alin Serdean aserdean at cloudbasesolutions.com
Wed Mar 30 01:53:38 UTC 2016


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