[prev in list] [next in list] [prev in thread] [next in thread]
List: openvswitch-dev
Subject: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to use the right parameters
From: vsairam () vmware ! com (Sairam Venugopal)
Date: 2016-03-31 18:56:35
Message-ID: D322BF1F.713B%vsairam () vmware ! com
[Download RAW message or body]
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" <aserdean at 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: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=
>> >> >
>> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic