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

Sairam Venugopal vsairam at vmware.com
Thu Mar 31 17:46:48 UTC 2016


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