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

[Sairam Venugopal]

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_

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

[Alin Serdean]

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.
> >> >
> >> >[

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

[Sairam Venugopal]

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=BQIGaQ&
>> >c=S
>> >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-
>> uEs=Dcruz40PROJ40ROzSpxyQSLw6f
>> >crO
>> >WpJgEcEmNR3JEQ=SiAZF6ppbO8xujF9FCVi7GvwJdKI2aCc81fTzQUV0ZA
>> =dzc7xmRZ
>> >NyK
>> >wLwlgvoBr97WN7PfuX_f4NXQjzkqcLVU= )
>> >
>> >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

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

[Alin Serdean]

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=BQIGaQ&
> >c=S
> >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-
> uEs=Dcruz40PROJ40ROzSpxyQSLw6f
> >crO
> >WpJgEcEmNR3JEQ=SiAZF6ppbO8xujF9FCVi7GvwJdKI2aCc81fTzQUV0ZA
> =dzc7xmRZ
> >NyK
> >wLwlgvoBr97WN7PfuX_f4NXQjzkqcLVU= )
> >
> >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.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

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

[Sairam Venugopal]

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" 
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_openvswit
>ch_ovs_blob_master_ofproto_ofproto-2Ddpif-2Drid.h-23L64-2DL69=BQIGaQ=S
>qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=Dcruz40PROJ40ROzSpxyQSLw6fcrO
>WpJgEcEmNR3JEQ=SiAZF6ppbO8xujF9FCVi7GvwJdKI2aCc81fTzQUV0ZA=dzc7xmRZNyK
>wLwlgvoBr97WN7PfuX_f4NXQjzkqcLVU= )
>
>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.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 = >flowTable[HASH_BUCKET(*hash)];
>
>> 
>
>> ___
>
>> dev mailing list
>
>> dev@openvswitch.org
>
>> 
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>an_listinfo_dev=BQIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=
>>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=SiAZF6ppbO8xujF9FCVi7GvwJdK
>>I2aCc81fTzQUV0ZA=rvvo6cM83_V6xVB2DDLyTXvP7e4vuYT5SLhq9HLgLT4=
>

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Alin Serdean]

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://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-rid.h#L64-L69)
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.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 = >flowTable[HASH_BUCKET(*hash)];
> 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Nithin Raju]

Thanks for sending out the fix.

I¹m ok with one of the fixes. The other one needs to be reworked to use
one of the approaches consistently. Pls. see inlined comments.

-- Nithin

-Original Message-
From: dev <dev-boun...@openvswitch.org> on behalf of Sairam Venugopal
<vsai...@vmware.com>
Date: Friday, March 25, 2016 at 11:06 AM
To: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to
use the right parameters

>Update OvsLookupFlow() to include flowKey->RecircId when computing hash.
>Use the right source port Id for checking if a packet is received or
>transmitted.
>
>Signed-off-by: Sairam Venugopal <vsai...@vmware.com>
>---
> datapath-windows/ovsext/Actions.c | 2 +-
> datapath-windows/ovsext/Flow.c| 3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index a91454d..7742096 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -2015,7 +2015,7 @@ OvsDoRecirc(POVS_SWITCH_CONTEXT switchContext,
> }
> status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS,
> vport, key, ovsFwdCtx.curNbl,
>-srcPortNo ==
>+ 
>ovsFwdCtx.fwdDetail->SourcePortId ==
>  
>switchContext->virtualExternalPortId,

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.

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.


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.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.

Of course, there¹s a bug when ŒkeyLen¹ gets set in _MapKeyAttrToFlowPut().
The value gets incremented due to ŒrecircId¹ but gets reset again.

> }
> 
> head = >flowTable[HASH_BUCKET(*hash)];

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Sairam Venugopal]

Hi Alin,

I don’t think portNo translates properly to portId in this case. If you
test out a scenario with a Flow Miss, then you will encounter Assert
failures in User.c since the IsReceive flags are set incorrectly. An easy
way to test this is to always reset flow to NULL and test this.

Also, the RecircId isn’t being included in the current patch since it gets
overwritten - 
https://github.com/openvswitch/ovs/blob/106dc9059176bf6b2b48016cd228e362366
1bc94/datapath-windows/ovsext/Flow.c#L1417

Moving the KeyLen computation after this line causes assertion failures at
this line - 
https://github.com/openvswitch/ovs/blob/106dc9059176bf6b2b48016cd228e362366
1bc94/datapath-windows/ovsext/Flow.c#L2038


I just tried testing the latest patch with MPLS and Conntrack and couldn’t
get the Recirc-Id to show up in ovs-dpctl dump-flows. If you are able to
get MPLS and Recirculation to work correctly with the current Master,
please let me know.

Thanks,
Sairam

On 3/28/16, 3:30 AM, "Alin Serdean" <aserd...@cloudbasesolutions.com>
wrote:

>Comments inlined.
>
>
>
>Thanks,
>
>Alin.
>
>
>
>> -Mesaj original-
>
>> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Sairam
>
>> Venugopal
>
>> Trimis: Friday, March 25, 2016 8:07 PM
>
>> Către: dev@openvswitch.org
>
>> Subiect: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to
>
>> use the right parameters
>
>> 
>
>> Update OvsLookupFlow() to include flowKey->RecircId when computing
>
>> hash.
>
>[Alin Gabriel Serdean: ]
>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitc
>h_ovs_blob_106dc9059176bf6b2b48016cd228e3623661bc94_datapath-2Dwindows_ovs
>ext_Flow.c-23L1385=BQIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&
>r=Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=P863NH9lY71NGZ9SvpvM-k8Ls6
>yoLwr4TCRYnKXrx-8=-j4huueRgVnR2YhTitknmflC-Llp_TepYx10G43HFAM=
>
>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitc
>h_ovs_blob_106dc9059176bf6b2b48016cd228e3623661bc94_datapath-2Dwindows_ovs
>ext_Flow.c-23L2125-2DL2126=BQIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMN
>tXt-uEs=Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=P863NH9lY71NGZ9Svp
>vM-k8Ls6yoLwr4TCRYnKXrx-8=3kagGUlhoBG-YKOYW8H3rbxUixmmr6u1I10h3wEckPg=
> 
>
>(it is included)
>
>> Use the right source port Id for checking if a packet is received or
>
>> transmitted.
>
>[Alin Gabriel Serdean: ]
>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitc
>h_ovs_blob_106dc9059176bf6b2b48016cd228e3623661bc94_datapath-2Dwindows_ovs
>ext_Actions.c-23L1932-2DL1933=BQIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-Yih
>VMNtXt-uEs=Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=P863NH9lY71NGZ9
>SvpvM-k8Ls6yoLwr4TCRYnKXrx-8=VQdYf5lm8nClznv7Ogdbs9he7KBtSpno3BEUrxkNPDI
>= 
>
>(it is the right source port id)
>
>> 
>
>> Signed-off-by: Sairam Venugopal <vsai...@vmware.com>
>
>> ---
>
>>  datapath-windows/ovsext/Actions.c | 2 +-
>
>>  datapath-windows/ovsext/Flow.c| 3 +++
>
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>
>> 
>
>> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
>
>> windows/ovsext/Actions.c
>
>> index a91454d..7742096 100644
>
>> --- a/datapath-windows/ovsext/Actions.c
>
>> +++ b/datapath-windows/ovsext/Actions.c
>
>> @@ -2015,7 +2015,7 @@ OvsDoRecirc(POVS_SWITCH_CONTEXT
>
>> switchContext,
>
>>  }
>
>>  status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS,
>
>>  vport, key, ovsFwdCtx.curNbl,
>
>> -srcPortNo ==
>
>> +
>
>> + ovsFwdCtx.fwdDetail->SourcePortId ==
>
>> 
>>switchContext->virtualExternalPortId,
>
>>  ,
>
>>  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);
>
>> +}
>
>>  }
>
>> 
>
>>  head = >flowTable[HASH_BUCKET(*hash)];
>
>> --
>
>> 2.5.0.windows.1
>
>> 
>
>> ___
>
>> dev mailing list
>
>> dev@openvswitch.org
>
>> 
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>an_listinfo_dev=BQIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=
>>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=P863NH9lY71NGZ9SvpvM-k8Ls6y
>>oLwr4TCRYnKXrx-8=kpm2_L6E-KoTPyH_6iXNxsooJzgke-7Lo_TftSp2d2g=
>

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Alin Serdean]

Comments inlined.

Thanks,
Alin.

> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Sairam
> Venugopal
> Trimis: Friday, March 25, 2016 8:07 PM
> Către: dev@openvswitch.org
> Subiect: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to
> use the right parameters
> 
> Update OvsLookupFlow() to include flowKey->RecircId when computing
> hash.
[Alin Gabriel Serdean: ] 
https://github.com/openvswitch/ovs/blob/106dc9059176bf6b2b48016cd228e3623661bc94/datapath-windows/ovsext/Flow.c#L1385
https://github.com/openvswitch/ovs/blob/106dc9059176bf6b2b48016cd228e3623661bc94/datapath-windows/ovsext/Flow.c#L2125-L2126
(it is included)
> Use the right source port Id for checking if a packet is received or
> transmitted.
[Alin Gabriel Serdean: ] 
https://github.com/openvswitch/ovs/blob/106dc9059176bf6b2b48016cd228e3623661bc94/datapath-windows/ovsext/Actions.c#L1932-L1933
(it is the right source port id)
> 
> Signed-off-by: Sairam Venugopal <vsai...@vmware.com>
> ---
>  datapath-windows/ovsext/Actions.c | 2 +-
>  datapath-windows/ovsext/Flow.c| 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
> windows/ovsext/Actions.c
> index a91454d..7742096 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -2015,7 +2015,7 @@ OvsDoRecirc(POVS_SWITCH_CONTEXT
> switchContext,
>  }
>  status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS,
>  vport, key, ovsFwdCtx.curNbl,
> -srcPortNo ==
> +
> + ovsFwdCtx.fwdDetail->SourcePortId ==
>  switchContext->virtualExternalPortId,
>  ,
>  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);
> +}
>  }
> 
>  head = >flowTable[HASH_BUCKET(*hash)];
> --
> 2.5.0.windows.1
> 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Sairam Venugopal]

Update OvsLookupFlow() to include flowKey->RecircId when computing hash.
Use the right source port Id for checking if a packet is received or 
transmitted.

Signed-off-by: Sairam Venugopal 
---
 datapath-windows/ovsext/Actions.c | 2 +-
 datapath-windows/ovsext/Flow.c| 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index a91454d..7742096 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -2015,7 +2015,7 @@ OvsDoRecirc(POVS_SWITCH_CONTEXT switchContext,
 }
 status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS,
 vport, key, ovsFwdCtx.curNbl,
-srcPortNo ==
+ovsFwdCtx.fwdDetail->SourcePortId ==
 switchContext->virtualExternalPortId,
 ,
 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);
+}
 }
 
 head = >flowTable[HASH_BUCKET(*hash)];
-- 
2.5.0.windows.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev