Re: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to use the right parameters
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
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
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
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
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
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
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
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
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
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