Re: [ovs-dev] [PATCH v2 1/2] tunneling: Avoid recirculation on datapath by computing the recirculate actions at translate time.

2017-01-24 Thread Jarno Rajahalme 
The userspace CLONE action is now merged to OVS master, so you can rebase the 
patches to use that.

  Jarno

> On Jan 24, 2017, at 3:49 AM, Zoltán Balogh  wrote:
> 
> Hi,
> 
> All right, we will rebase the code once the patch is merged.
> 
> Could you have a look at the code and review it, please?
> So we could receive feedback in time. 
> 
> Furthermore it would be nice to get some help regarding the failing test 
> cases.
> 
> 768: tunnel_push_pop - packet_outFAILED 
> (tunnel-push-pop.at:203)
> 2261: ovn -- 3 HVs, 1 LS, 3 lports/HV FAILED (ovn.at:1295)
> 2266: ovn -- 3 HVs, 3 LS, 3 lports/LS, 1 LR   FAILED (ovn.at:2468)
> 2268: ovn -- 2 HVs, 2 LS, 1 lport/LS, 2 peer LRs  FAILED (ovn.at:2979)
> 2298: ovn -- 1 LR with distributed router gateway port FAILED (ovn.at:6480)
> 
> The patches are available here:
> https://patchwork.ozlabs.org/patch/717683/
> https://patchwork.ozlabs.org/patch/717681/
> 
> 
> Best regards,
> Zoltan
> 
> 
> 
>> On Jan 22, 2017, at 5:50 AM, Jan Scheurich  wrote:
>> =20
>> How does the dpif-netdev CLONE action introduced here relate to the simil=
> ar action already introduced in the context of the OpenFLow CLONE action (s=
> ee for example https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/=
> 327808.html).
>> =20
> 
> OpenFlow level clone action was added a bit earlier, the email you refer to=
> is a dpif-netdev level clone action.
> 
>> I assume these do comparable things and the native tunnel implementation =
> should re-use the new CLONE action.
>> =20
> 
> This is true. There were other needs for the datapath level clone action, a=
> nd once that is merged this patch series should rebase to that.
> 
>  Jarno
> 
>> /Jan
>> =20
>> On 2017-01-20 14:40, Zolt=E1n Balogh wrote:
>>> From: Sugesh Chandran 
>>> =20
>>> Openvswitch datapath recirculates packets for tunneling, i.e.
>>> the incoming packets are encapsulated at first pass. Further actions are
>>> applied on encapsulated packets on the second pass after recirculating.
>>> The proposed patch compute and append the post tunnel actions at the tim=
> e of
>>> translation itself instead of recirculating at datapath. These actions a=
> re solely
>>> depends on tunnel attributes so there is no need of datapath recirculati=
> on.
>>> By avoiding the recirculation at datapath, the patch offers upto 30%
>>> performance improvement for VxLAN tunneling in our testing.
>>> The action execution logic is also extended with new CLONE action to def=
> ine
>>> the packet cloning when the actions are combined. The lenght in the CLON=
> E
>>> action specifies the size of nested action set.
>>> =20
>>> Signed-off-by: Sugesh Chandran 
>>> Signed-off-by: Zolt=E1n Balogh 
>>> Co-authored-by: Zolt=E1n Balogh 
>>> ---
>>> datapath/linux/compat/include/linux/openvswitch.h |   1 +
>>> lib/dpif-netdev.c |  20 +-
>>> lib/dpif.c|   1 +
>>> lib/odp-execute.c |  57 -
>>> lib/odp-util.c|  96 +++-
>>> lib/odp-util.h|   5 +
>>> ofproto/ofproto-dpif-sflow.c  |   1 +
>>> ofproto/ofproto-dpif-xlate.c  | 267 +++=
> ---
>>> 8 files changed, 288 insertions(+), 160 deletions(-)
>>> =20
>>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapat=
> h/linux/compat/include/linux/openvswitch.h
>>> index 12260d8..91849d6 100644
>>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>>> @@ -818,6 +818,7 @@ enum ovs_action_attr {
>>> #ifndef __KERNEL__
>>> =09OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>>> =09OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>>> +=09OVS_ACTION_ATTR_CLONE,
>>> #endif
>>> =09__OVS_ACTION_ATTR_MAX,=09  /* Nothing past this will be accepted
>>> =09=09=09=09   * from userspace. */
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 3901129..0b85fe4 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -4547,24 +4547,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch =
> *packets_,
>>>   case OVS_ACTION_ATTR_TUNNEL_PUSH:
>>> if (*depth < MAX_RECIRC_DEPTH) {
>>> -struct dp_packet_batch tnl_pkt;
>>> -struct dp_packet_batch *orig_packets_ =3D packets_;
>>> -int err;
>>> -
>>> -if (!may_steal) {
>>> -dp_packet_batch_clone(_pkt, packets_);
>>> -packets_ =3D _pkt;
>>> -dp_packet_batch_reset_cutlen(orig_packets_);
>>> -}
>>> -
>>> dp_packet_batch_apply_cutlen(packets_);
>>> -
>>> -err =3D push_tnl_action(pmd, a,

Re: [ovs-dev] [PATCH v2 1/2] tunneling: Avoid recirculation on datapath by computing the recirculate actions at translate time.

2017-01-23 Thread Jarno Rajahalme 

> On Jan 22, 2017, at 5:50 AM, Jan Scheurich  wrote:
> 
> How does the dpif-netdev CLONE action introduced here relate to the similar 
> action already introduced in the context of the OpenFLow CLONE action (see 
> for example 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327808.html).
> 

OpenFlow level clone action was added a bit earlier, the email you refer to is 
a dpif-netdev level clone action.

> I assume these do comparable things and the native tunnel implementation 
> should re-use the new CLONE action.
> 

This is true. There were other needs for the datapath level clone action, and 
once that is merged this patch series should rebase to that.

  Jarno

> /Jan
> 
> On 2017-01-20 14:40, Zoltán Balogh wrote:
>> From: Sugesh Chandran 
>> 
>> Openvswitch datapath recirculates packets for tunneling, i.e.
>> the incoming packets are encapsulated at first pass. Further actions are
>> applied on encapsulated packets on the second pass after recirculating.
>> The proposed patch compute and append the post tunnel actions at the time of
>> translation itself instead of recirculating at datapath. These actions are 
>> solely
>> depends on tunnel attributes so there is no need of datapath recirculation.
>> By avoiding the recirculation at datapath, the patch offers upto 30%
>> performance improvement for VxLAN tunneling in our testing.
>> The action execution logic is also extended with new CLONE action to define
>> the packet cloning when the actions are combined. The lenght in the CLONE
>> action specifies the size of nested action set.
>> 
>> Signed-off-by: Sugesh Chandran 
>> Signed-off-by: Zoltán Balogh 
>> Co-authored-by: Zoltán Balogh 
>> ---
>>  datapath/linux/compat/include/linux/openvswitch.h |   1 +
>>  lib/dpif-netdev.c |  20 +-
>>  lib/dpif.c|   1 +
>>  lib/odp-execute.c |  57 -
>>  lib/odp-util.c|  96 +++-
>>  lib/odp-util.h|   5 +
>>  ofproto/ofproto-dpif-sflow.c  |   1 +
>>  ofproto/ofproto-dpif-xlate.c  | 267 
>> +++---
>>  8 files changed, 288 insertions(+), 160 deletions(-)
>> 
>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
>> b/datapath/linux/compat/include/linux/openvswitch.h
>> index 12260d8..91849d6 100644
>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>> @@ -818,6 +818,7 @@ enum ovs_action_attr {
>>  #ifndef __KERNEL__
>>  OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>>  OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>> +OVS_ACTION_ATTR_CLONE,
>>  #endif
>>  __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>> * from userspace. */
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 3901129..0b85fe4 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -4547,24 +4547,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
>> *packets_,
>>case OVS_ACTION_ATTR_TUNNEL_PUSH:
>>  if (*depth < MAX_RECIRC_DEPTH) {
>> -struct dp_packet_batch tnl_pkt;
>> -struct dp_packet_batch *orig_packets_ = packets_;
>> -int err;
>> -
>> -if (!may_steal) {
>> -dp_packet_batch_clone(_pkt, packets_);
>> -packets_ = _pkt;
>> -dp_packet_batch_reset_cutlen(orig_packets_);
>> -}
>> -
>>  dp_packet_batch_apply_cutlen(packets_);
>> -
>> -err = push_tnl_action(pmd, a, packets_);
>> -if (!err) {
>> -(*depth)++;
>> -dp_netdev_recirculate(pmd, packets_);
>> -(*depth)--;
>> -}
>> +push_tnl_action(pmd, a, packets_);
>>  return;
>>  }
>>  break;
>> @@ -4714,6 +4698,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
>> *packets_,
>>  break;
>>  }
>>  +case OVS_ACTION_ATTR_CLONE:
>> +break;
>>  case OVS_ACTION_ATTR_PUSH_VLAN:
>>  case OVS_ACTION_ATTR_POP_VLAN:
>>  case OVS_ACTION_ATTR_PUSH_MPLS:
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 50c3382..b698da2 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1183,6 +1183,7 @@ dpif_execute_helper_cb(void *aux_, struct 
>> dp_packet_batch *packets_,
>>  case OVS_ACTION_ATTR_SAMPLE:
>>  case OVS_ACTION_ATTR_TRUNC:
>>  case OVS_ACTION_ATTR_UNSPEC:
>> +case OVS_ACTION_ATTR_CLONE:
>>  case __OVS_ACTION_ATTR_MAX:
>>  OVS_NOT_REACHED();
>>  }
>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>> index 73e1016..77bca94 100644
>> --- a/lib/odp-execute.c
>> +++

[ovs-dev] [PATCH v2 1/2] tunneling: Avoid recirculation on datapath by computing the recirculate actions at translate time.

2017-01-20 Thread Zoltán Balogh 
From: Sugesh Chandran 

Openvswitch datapath recirculates packets for tunneling, i.e.
the incoming packets are encapsulated at first pass. Further actions are
applied on encapsulated packets on the second pass after recirculating.
The proposed patch compute and append the post tunnel actions at the time of
translation itself instead of recirculating at datapath. These actions are 
solely
depends on tunnel attributes so there is no need of datapath recirculation.
By avoiding the recirculation at datapath, the patch offers upto 30%
performance improvement for VxLAN tunneling in our testing.
The action execution logic is also extended with new CLONE action to define
the packet cloning when the actions are combined. The lenght in the CLONE
action specifies the size of nested action set.

Signed-off-by: Sugesh Chandran 
Signed-off-by: Zoltán Balogh 
Co-authored-by: Zoltán Balogh 
---
 datapath/linux/compat/include/linux/openvswitch.h |   1 +
 lib/dpif-netdev.c |  20 +-
 lib/dpif.c|   1 +
 lib/odp-execute.c |  57 -
 lib/odp-util.c|  96 +++-
 lib/odp-util.h|   5 +
 ofproto/ofproto-dpif-sflow.c  |   1 +
 ofproto/ofproto-dpif-xlate.c  | 267 +++---
 8 files changed, 288 insertions(+), 160 deletions(-)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
b/datapath/linux/compat/include/linux/openvswitch.h
index 12260d8..91849d6 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -818,6 +818,7 @@ enum ovs_action_attr {
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
+   OVS_ACTION_ATTR_CLONE,
 #endif
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
   * from userspace. */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 3901129..0b85fe4 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4547,24 +4547,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
*packets_,
 
 case OVS_ACTION_ATTR_TUNNEL_PUSH:
 if (*depth < MAX_RECIRC_DEPTH) {
-struct dp_packet_batch tnl_pkt;
-struct dp_packet_batch *orig_packets_ = packets_;
-int err;
-
-if (!may_steal) {
-dp_packet_batch_clone(_pkt, packets_);
-packets_ = _pkt;
-dp_packet_batch_reset_cutlen(orig_packets_);
-}
-
 dp_packet_batch_apply_cutlen(packets_);
-
-err = push_tnl_action(pmd, a, packets_);
-if (!err) {
-(*depth)++;
-dp_netdev_recirculate(pmd, packets_);
-(*depth)--;
-}
+push_tnl_action(pmd, a, packets_);
 return;
 }
 break;
@@ -4714,6 +4698,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
*packets_,
 break;
 }
 
+case OVS_ACTION_ATTR_CLONE:
+break;
 case OVS_ACTION_ATTR_PUSH_VLAN:
 case OVS_ACTION_ATTR_POP_VLAN:
 case OVS_ACTION_ATTR_PUSH_MPLS:
diff --git a/lib/dpif.c b/lib/dpif.c
index 50c3382..b698da2 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1183,6 +1183,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_SAMPLE:
 case OVS_ACTION_ATTR_TRUNC:
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_CLONE:
 case __OVS_ACTION_ATTR_MAX:
 OVS_NOT_REACHED();
 }
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 73e1016..77bca94 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -541,6 +541,7 @@ requires_datapath_assistance(const struct nlattr *a)
 case OVS_ACTION_ATTR_USERSPACE:
 case OVS_ACTION_ATTR_RECIRC:
 case OVS_ACTION_ATTR_CT:
+case OVS_ACTION_ATTR_CLONE:
 return true;
 
 case OVS_ACTION_ATTR_SET:
@@ -562,6 +563,29 @@ requires_datapath_assistance(const struct nlattr *a)
 return false;
 }
 
+static inline size_t
+find_combine_action_end(const struct nlattr **actions_, size_t *actions_len,
+const struct nlattr **comb_start,
+bool *comb_last_action)
+{
+const struct nlattr *a;
+const struct nlattr *actions = *actions_;
+unsigned int left;
+bool last_action;
+size_t comb_size = nl_attr_get_u32(actions); /* Size of combined actions */
+*actions_len -= NLA_ALIGN(actions->nla_len);
+actions = nl_attr_next(actions);
+
+*comb_start = actions;
+a = (void *) ((uint8_t *) actions + NLA_ALIGN(comb_size));
+left = (*actions_len) - comb_size;
+last_action = (left <=