Re: [ovs-dev] [PATCH] ovn pacemaker: Provide the option to configure inactivity probe value

[Andy Zhou]

Hi, Numan,

I am curious why default 5 seconds inactivity time does not work? Do
you have more details?

Does the glitch usually happen around the HA switch over?  If this
happens during normal operation,
Then this is not HA specific issue, but an indication of some
connectivity issues.


On Thu, Oct 12, 2017 at 11:08 AM, Andy Zhou <az...@ovn.org> wrote:
> Sure, I will take a look.
>
> On Thu, Oct 12, 2017 at 10:49 AM, Ben Pfaff <b...@ovn.org> wrote:
>> Hi Andy.  In the IRC meeting today, Numan suggested that you might be an
>> appropriate reviewer for this patch, so if you agree and you have a
>> chance to look at this then it would be appreciated.
>>
>> Thanks,
>>
>> Ben.
>>
>> On Wed, Oct 11, 2017 at 02:22:33PM +0530, nusid...@redhat.com wrote:
>>> From: Numan Siddique <nusid...@redhat.com>
>>>
>>> In the case of OVN HA deployments with openstack, it has been noticed
>>> that the 5 seconds inactivity probe interval is not enough and ovsdb-servers
>>> time out.
>>> This patch
>>>- providdes an option to configure this value.
>>>    - creates a connection row in NB/SB dbs and sets the target and
>>>  inactivity_probe values when the node is promoted to master.
>>>
>>> CC: Andy Zhou <az...@ovn.org>
>>> Signed-off-by: Numan Siddique <nusid...@redhat.com>
>>> ---
>>>  ovn/utilities/ovndb-servers.ocf | 27 +++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/ovn/utilities/ovndb-servers.ocf 
>>> b/ovn/utilities/ovndb-servers.ocf
>>> index fe1207c22..92620af6a 100755
>>> --- a/ovn/utilities/ovndb-servers.ocf
>>> +++ b/ovn/utilities/ovndb-servers.ocf
>>> @@ -8,6 +8,8 @@
>>>  : ${SB_MASTER_PORT_DEFAULT="6642"}
>>>  : ${SB_MASTER_PROTO_DEFAULT="tcp"}
>>>  : ${MANAGE_NORTHD_DEFAULT="no"}
>>> +: ${INACTIVE_PROBE_DEFAULT="6"}
>>> +
>>>  CRM_MASTER="${HA_SBIN_DIR}/crm_master -l reboot"
>>>  CRM_ATTR_REPL_INFO="${HA_SBIN_DIR}/crm_attribute --type crm_config --name 
>>> OVN_REPL_INFO -s ovn_ovsdb_master_server"
>>>  OVN_CTL=${OCF_RESKEY_ovn_ctl:-${OVN_CTL_DEFAULT}}
>>> @@ -17,6 +19,7 @@ 
>>> NB_MASTER_PROTO=${OCF_RESKEY_nb_master_protocol:-${NB_MASTER_PROTO_DEFAULT}}
>>>  SB_MASTER_PORT=${OCF_RESKEY_sb_master_port:-${SB_MASTER_PORT_DEFAULT}}
>>>  
>>> SB_MASTER_PROTO=${OCF_RESKEY_sb_master_protocol:-${SB_MASTER_PROTO_DEFAULT}}
>>>  MANAGE_NORTHD=${OCF_RESKEY_manage_northd:-${MANAGE_NORTHD_DEFAULT}}
>>> +INACTIVE_PROBE=${OCF_RESKEY_inactive_probe_interval:-${INACTIVE_PROBE_DEFAULT}}
>>>
>>>  # Invalid IP address is an address that can never exist in the network, as
>>>  # mentioned in rfc-5737. The ovsdb servers connects to this IP address till
>>> @@ -101,6 +104,14 @@ ovsdb_server_metadata() {
>>>
>>>
>>>
>>> +  
>>> +  
>>> +  Inactive probe interval to set for ovsdb-server.
>>> +  
>>> +  Set inactive probe interval
>>> +  
>>> +  
>>> +
>>>
>>>
>>>
>>> @@ -138,6 +149,22 @@ ovsdb_server_notify() {
>>>  ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
>>>  fi
>>>
>>> +conn=`ovn-nbctl get NB_global . connections`
>>> +if [ "$conn" == "[]" ]
>>> +then
>>> +ovn-nbctl -- --id=@conn_uuid create Connection \
>>> +target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${MASTER_IP}" \
>>> +inactivity_probe=$INACTIVE_PROBE -- set NB_Global . connections=@conn_uuid
>>> +fi
>>> +
>>> +conn=`ovn-sbctl get SB_global . connections`
>>> +if [ "$conn" == "[]" ]
>>> +then
>>> +ovn-sbctl -- --id=@conn_uuid create Connection \
>>> +target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${MASTER_IP}" \
>>> +inactivity_probe=$INACTIVE_PROBE -- set SB_Global . connections=@conn_uuid
>>> +fi
>>> +
>>>  else
>>>  if [ "$MANAGE_NORTHD" = "yes" ]; then
>>>  # Stop ovn-northd service. Set --ovn-manage-ovsdb=no so that
>>> --
>>> 2.13.5
>>>
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] vswitch.xml: Better document patch ports.

[Andy Zhou]

On Thu, Oct 12, 2017 at 9:09 AM, Ben Pfaff <b...@ovn.org> wrote:
> Reported-by: Hui Xiang <xiangh...@gmail.com>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

Looks good.

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn pacemaker: Provide the option to configure inactivity probe value

[Andy Zhou]

Sure, I will take a look.

On Thu, Oct 12, 2017 at 10:49 AM, Ben Pfaff <b...@ovn.org> wrote:
> Hi Andy.  In the IRC meeting today, Numan suggested that you might be an
> appropriate reviewer for this patch, so if you agree and you have a
> chance to look at this then it would be appreciated.
>
> Thanks,
>
> Ben.
>
> On Wed, Oct 11, 2017 at 02:22:33PM +0530, nusid...@redhat.com wrote:
>> From: Numan Siddique <nusid...@redhat.com>
>>
>> In the case of OVN HA deployments with openstack, it has been noticed
>> that the 5 seconds inactivity probe interval is not enough and ovsdb-servers
>> time out.
>> This patch
>>- providdes an option to configure this value.
>>- creates a connection row in NB/SB dbs and sets the target and
>>  inactivity_probe values when the node is promoted to master.
>>
>> CC: Andy Zhou <az...@ovn.org>
>> Signed-off-by: Numan Siddique <nusid...@redhat.com>
>> ---
>>  ovn/utilities/ovndb-servers.ocf | 27 +++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/ovn/utilities/ovndb-servers.ocf 
>> b/ovn/utilities/ovndb-servers.ocf
>> index fe1207c22..92620af6a 100755
>> --- a/ovn/utilities/ovndb-servers.ocf
>> +++ b/ovn/utilities/ovndb-servers.ocf
>> @@ -8,6 +8,8 @@
>>  : ${SB_MASTER_PORT_DEFAULT="6642"}
>>  : ${SB_MASTER_PROTO_DEFAULT="tcp"}
>>  : ${MANAGE_NORTHD_DEFAULT="no"}
>> +: ${INACTIVE_PROBE_DEFAULT="6"}
>> +
>>  CRM_MASTER="${HA_SBIN_DIR}/crm_master -l reboot"
>>  CRM_ATTR_REPL_INFO="${HA_SBIN_DIR}/crm_attribute --type crm_config --name 
>> OVN_REPL_INFO -s ovn_ovsdb_master_server"
>>  OVN_CTL=${OCF_RESKEY_ovn_ctl:-${OVN_CTL_DEFAULT}}
>> @@ -17,6 +19,7 @@ 
>> NB_MASTER_PROTO=${OCF_RESKEY_nb_master_protocol:-${NB_MASTER_PROTO_DEFAULT}}
>>  SB_MASTER_PORT=${OCF_RESKEY_sb_master_port:-${SB_MASTER_PORT_DEFAULT}}
>>  SB_MASTER_PROTO=${OCF_RESKEY_sb_master_protocol:-${SB_MASTER_PROTO_DEFAULT}}
>>  MANAGE_NORTHD=${OCF_RESKEY_manage_northd:-${MANAGE_NORTHD_DEFAULT}}
>> +INACTIVE_PROBE=${OCF_RESKEY_inactive_probe_interval:-${INACTIVE_PROBE_DEFAULT}}
>>
>>  # Invalid IP address is an address that can never exist in the network, as
>>  # mentioned in rfc-5737. The ovsdb servers connects to this IP address till
>> @@ -101,6 +104,14 @@ ovsdb_server_metadata() {
>>
>>
>>
>> +  
>> +  
>> +  Inactive probe interval to set for ovsdb-server.
>> +  
>> +  Set inactive probe interval
>> +  
>> +  
>> +
>>
>>
>>
>> @@ -138,6 +149,22 @@ ovsdb_server_notify() {
>>  ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
>>  fi
>>
>> +conn=`ovn-nbctl get NB_global . connections`
>> +if [ "$conn" == "[]" ]
>> +then
>> +ovn-nbctl -- --id=@conn_uuid create Connection \
>> +target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${MASTER_IP}" \
>> +inactivity_probe=$INACTIVE_PROBE -- set NB_Global . connections=@conn_uuid
>> +fi
>> +
>> +conn=`ovn-sbctl get SB_global . connections`
>> +if [ "$conn" == "[]" ]
>> +then
>> +ovn-sbctl -- --id=@conn_uuid create Connection \
>> +target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${MASTER_IP}" \
>> +inactivity_probe=$INACTIVE_PROBE -- set SB_Global . connections=@conn_uuid
>> +fi
>> +
>>  else
>>  if [ "$MANAGE_NORTHD" = "yes" ]; then
>>  # Stop ovn-northd service. Set --ovn-manage-ovsdb=no so that
>> --
>> 2.13.5
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove assertion for truncated

[Andy Zhou]

On Tue, Oct 10, 2017 at 12:28 AM, Iwase Yusuke  wrote:
> Hi Andy,
>
> Thank you for your quick response.
>
> Your patch looks good to me.
> I couldn't judge whether we can "truncate" completely or not, that sounds
> great!
>
> Thanks,
> Iwase
>
Thanks. I have pushed the patch to master. Also added your name to the
AUTHORS.rst file.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove assertion for truncated

[Andy Zhou]

On Mon, Oct 9, 2017 at 8:18 PM, Iwase Yusuke  wrote:
> Hi Ben and Andy,
>
> Thank you very much!
> I'm looking forward to review comments.
>
>
Hi, Iwase,

I miss interpreted max_len in the spec, and thought (wrongly) that
only the output controller action should have a non-zero value.
Apparently, this
is too restrictive as you have pointed out.

In terms of the fix. I think we can remove the 'truncate" variable
completely. If the following incremental looks O.K. to you, I'd like
to fold it in before pushing your patch to master.

Thanks for reporting the bug!


diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d320d570b304..a492c2215851 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4829,31 +4829,26 @@ xlate_output_action(struct xlate_ctx *ctx,
 bool is_last_action)
 {
 ofp_port_t prev_nf_output_iface = ctx->nf_output_iface;
-bool truncate = max_len != 0;

 ctx->nf_output_iface = NF_OUT_DROP;

 switch (port) {
 case OFPP_IN_PORT:
 compose_output_action(ctx, ctx->xin->flow.in_port.ofp_port, NULL,
-  is_last_action, truncate);
+  is_last_action, false);
 break;
 case OFPP_TABLE:
-ovs_assert(!truncate);
 xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
0, may_packet_in, true, false, false,
do_xlate_actions);
 break;
 case OFPP_NORMAL:
-ovs_assert(!truncate);
 xlate_normal(ctx);
 break;
 case OFPP_FLOOD:
-ovs_assert(!truncate);
 flood_packets(ctx, false, is_last_action);
 break;
 case OFPP_ALL:
-ovs_assert(!truncate);
 flood_packets(ctx, true, is_last_action);
 break;
 case OFPP_CONTROLLER:
@@ -4869,7 +4864,7 @@ xlate_output_action(struct xlate_ctx *ctx,
 case OFPP_LOCAL:
 default:
 if (port != ctx->xin->flow.in_port.ofp_port) {
-compose_output_action(ctx, port, NULL, is_last_action, truncate);
+compose_output_action(ctx, port, NULL, is_last_action, false);
 } else {
 xlate_report(ctx, OFT_WARN, "skipping output to input port");
 }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netdev: Use portable error code for zero rate meter band

[Andy Zhou]

On Thu, Sep 28, 2017 at 7:31 PM, Joe Stringer <j...@ovn.org> wrote:
> On 28 September 2017 at 12:39, Andy Zhou <az...@ovn.org> wrote:
>> 'EBADRQC' is only defined on the Linux platform. Without this fix,
>> The travis MacOS build fails. Switching to using EDOM which is more
>> portable.
>>
>> Fixes: 2029ce9ac3a601 (dpif-netdev: Fix a zero-rate bug for meter)
>> CC: Ali Volkan ATLI <volkan.a...@argela.com.tr>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>
> LGTM.
>
> Acked-by: Joe Stringer <j...@ovn.org>

Thanks for the review!  Pushed to master and branch 2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] dpif-netdev: Use portable error code for zero rate meter band

[Andy Zhou]

'EBADRQC' is only defined on the Linux platform. Without this fix,
The travis MacOS build fails. Switching to using EDOM which is more
portable.

Fixes: 2029ce9ac3a601 (dpif-netdev: Fix a zero-rate bug for meter)
CC: Ali Volkan ATLI <volkan.a...@argela.com.tr>
Signed-off-by: Andy Zhou <az...@ovn.org>
---
 lib/dpif-netdev.c  | 2 +-
 ofproto/ofproto-dpif.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 0fce94e0e409..d5eb8305c8a2 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4285,7 +4285,7 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id 
*meter_id,
 /* Validate rates */
 for (i = 0; i < config->n_bands; i++) {
 if (config->bands[i].rate == 0) {
-return EBADRQC; /* rate must be non-zero */
+return EDOM; /* rate must be non-zero */
 }
 }
 
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d99dc9d88788..43d670a15c3f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5695,7 +5695,7 @@ meter_set(struct ofproto *ofproto_, ofproto_meter_id 
*meter_id,
 return OFPERR_OFPMMFC_OUT_OF_BANDS;
 case ENODEV: /* Unsupported band type */
 return OFPERR_OFPMMFC_BAD_BAND;
-case EBADRQC: /* Rate must be non-zero */
+case EDOM: /* Rate must be non-zero */
 return OFPERR_OFPMMFC_BAD_RATE;
 default:
 return OFPERR_OFPMMFC_UNKNOWN;
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netdev: Fix a zero-rate bug for meter

[Andy Zhou]

On Wed, Sep 27, 2017 at 9:23 AM, Ali Volkan Atli
 wrote:
> From 23bc166eecd6e4db7b55720cdd780012df62a0cc Mon Sep 17 00:00:00 2001
> From: Ali Volkan ATLI 
> Date: Wed, 27 Sep 2017 18:33:57 +0300
> Subject: [PATCH] dpif-netdev: Fix a zero-rate bug for meter
>
> Open vSwitch daemon crashes (by receiving signal SIGFPE,
> Arithmetic exception) when a controller tries to send
> a meter-mod message with zero rate.
>
> Signed-off-by: Ali Volkan ATLI 

Thanks for the fix! Pushed to master with some minor style
adjustments.  Also back ported to branch 2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [merge native tunneling and patch port 1/7] ofproto-dpif: Unfreeze within clone

[Andy Zhou]

On Thu, Sep 21, 2017 at 9:44 AM, Greg Rose <gvrose8...@gmail.com> wrote:
> On 09/12/2017 12:49 PM, Andy Zhou wrote:
>>
>> When translating actions within open flow clone, actions generated
>> by finish_freezeing() should also be enclosed within the datapath
>> clone netlink encoding.
>>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>> ---
>>   ofproto/ofproto-dpif-xlate.c | 9 +
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 9e1f837cb23e..e5ad832d7c47 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -5353,6 +5353,9 @@ compose_clone(struct xlate_ctx *ctx, const struct
>> ofpact_nest *oc)
>>   if (reversible_actions(oc->actions, oc_actions_len)) {
>>   old_flow = ctx->xin->flow;
>>   do_xlate_actions(oc->actions, oc_actions_len, ctx);
>> +if (ctx->freezing) {
>> +finish_freezing(ctx);
>> +}
>>   goto xlate_done;
>>   }
>>   @@ -5372,6 +5375,9 @@ compose_clone(struct xlate_ctx *ctx, const struct
>> ofpact_nest *oc)
>>   offset = nl_msg_start_nested(ctx->odp_actions,
>> OVS_ACTION_ATTR_CLONE);
>>   ac_offset = ctx->odp_actions->size;
>>   do_xlate_actions(oc->actions, oc_actions_len, ctx);
>> +if (ctx->freezing) {
>> +finish_freezing(ctx);
>> +}
>>   nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
>>   goto dp_clone_done;
>>   }
>> @@ -5382,6 +5388,9 @@ compose_clone(struct xlate_ctx *ctx, const struct
>> ofpact_nest *oc)
>>   ac_offset = nl_msg_start_nested(ctx->odp_actions,
>>   OVS_SAMPLE_ATTR_ACTIONS);
>>   do_xlate_actions(oc->actions, oc_actions_len, ctx);
>> +if (ctx->freezing) {
>> +finish_freezing(ctx);
>> +}
>>   if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
>>   nl_msg_cancel_nested(ctx->odp_actions, offset);
>>   } else {
>>
>
> Tested-by: Greg Rose <gvrose8...@gmail.com>
> Reviewed-by: Greg Rose <gvrose8...@gmail.com>

Thanks for the review Greg!  I applied your suggestion on patch 6 and pushed
the series to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] datapath: Optimize updating for OvS flow_stats.

[Andy Zhou]

On Sun, Sep 10, 2017 at 6:00 PM, Tonghao Zhang  wrote:
> Upstream commit:
> commit c57c054eb5b1ccf230c49f736f7a018fcbc3e952
> Author: Tonghao Zhang 
> Date:   Mon Jul 17 23:28:05 2017 -0700
>
> openvswitch: Optimize updating for OvS flow_stats.
>
> In the ovs_flow_stats_update(), we only use the node
> var to alloc flow_stats struct. But this is not a
> common case, it is unnecessary to call the numa_node_id()
> everytime. This patch is not a bugfix, but there maybe
> a small increase.
>
> Signed-off-by: Tonghao Zhang 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Tonghao Zhang 
Applied both patches to master. Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/15] acinclude: Check for SKB_GSO_UDP

[Andy Zhou]

On Mon, Sep 11, 2017 at 10:55 AM, Greg Rose  wrote:
> Removed in kernel 4.13
>
> Signed-off-by: Greg Rose 

Pushed the series to master with some minor edits.
Patch 2 and 3 of the series are dropped in favor of Tonghao's patch as
per mailing list discussion.

Thanks for the back ports.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 14/15] Documentation: Update NEWS and FAQ.

[Andy Zhou]

On Fri, Sep 22, 2017 at 8:02 AM, Greg Rose <gvrose8...@gmail.com> wrote:
> On 09/20/2017 03:18 PM, Andy Zhou wrote:
>>
>> On Mon, Sep 11, 2017 at 10:56 AM, Greg Rose <gvrose8...@gmail.com> wrote:
>>>
>>> Document Open vSwitch Linux kernel support for the 4.13 kernel
>>> release.
>>>
>>> Signed-off-by: Greg Rose <gvrose8...@gmail.com>
>>> ---
>>>   Documentation/faq/releases.rst | 2 +-
>>>   NEWS   | 2 ++
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/faq/releases.rst
>>> b/Documentation/faq/releases.rst
>>> index c785529..064a496 100644
>>> --- a/Documentation/faq/releases.rst
>>> +++ b/Documentation/faq/releases.rst
>>> @@ -65,7 +65,7 @@ Q: What Linux kernel versions does each Open vSwitch
>>> release work with?
>>>   2.5.x2.6.32 to 4.3
>>>   2.6.x3.10 to 4.7
>>>   2.7.x3.10 to 4.9
>>> -2.8.x3.10 to 4.12
>>> +2.8.x3.10 to 4.13
>>>    ==
>>
>>
>> Should actually be:
>>
>> 2.8.x  3.10 to 4.12
>> 2.9.x  3.10 to 4.13
>>
>
> OK, I'll fix that up and resend V2.

You don't have to. I will fix this up before pushing the rest of the series.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 14/15] Documentation: Update NEWS and FAQ.

[Andy Zhou]

On Mon, Sep 11, 2017 at 10:56 AM, Greg Rose  wrote:
> Document Open vSwitch Linux kernel support for the 4.13 kernel
> release.
>
> Signed-off-by: Greg Rose 
> ---
>  Documentation/faq/releases.rst | 2 +-
>  NEWS   | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
> index c785529..064a496 100644
> --- a/Documentation/faq/releases.rst
> +++ b/Documentation/faq/releases.rst
> @@ -65,7 +65,7 @@ Q: What Linux kernel versions does each Open vSwitch 
> release work with?
>  2.5.x2.6.32 to 4.3
>  2.6.x3.10 to 4.7
>  2.7.x3.10 to 4.9
> -2.8.x3.10 to 4.12
> +2.8.x3.10 to 4.13
>   ==

Should actually be:

2.8.x  3.10 to 4.12
2.9.x  3.10 to 4.13


>
>  Open vSwitch userspace should also work with the Linux kernel module 
> built
> diff --git a/NEWS b/NEWS
> index 6a5d2bf..a3c1a02 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -3,6 +3,8 @@ Post-v2.8.0
> - OVN:
>   * The "requested-chassis" option for a logical switch port now accepts a
> chassis "hostname" in addition to a chassis "name".
> +   - Linux kernel 4.13
> + * Add support for compiling OVS with the latest Linux 4.13 kernel
>
>  v2.8.0 - xx xxx 
>  -
> --
> 1.8.3.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [merge native tunneling and patch port 1/7] ofproto-dpif: Unfreeze within clone

[Andy Zhou]

On Tue, Sep 19, 2017 at 1:56 PM, Greg Rose <gvrose8...@gmail.com> wrote:
> On 09/19/2017 01:26 PM, Andy Zhou wrote:
>>
>> On Tue, Sep 19, 2017 at 9:55 AM, Greg Rose <gvrose8...@gmail.com> wrote:
>>>
>>> On 09/12/2017 12:49 PM, Andy Zhou wrote:
>>>>
>>>>
>>>> When translating actions within open flow clone, actions generated
>>>> by finish_freezeing() should also be enclosed within the datapath
>>>> clone netlink encoding.
>>>>
>>>> Signed-off-by: Andy Zhou <az...@ovn.org>
>>>
>>>
>>>
>>> Andy,
>>>
>>> I am reviewing and testing your patches.  I have applied them to my
>>> private
>>> github repository
>>> on a branch named test-813027-35.
>>>
>>> https://github.com/gvrose8192/ovs-experimental/tree/test-813027-35
>>>
>>> However, the Travis 'TESTSUITE=1 KERNEL=3.16.46 build fails:
>>>
>>> https://travis-ci.org/gvrose8192/ovs-experimental/jobs/277364409
>>>
>>> Have you noticed this as well?
>>
>>
>> No. It passed my local test, and passed travis test from my private
>> branch (just rebased this morning)
>>
>> https://github.com/azhou-nicira/ovs-review/tree/patch_port
>>
>> https://travis-ci.org/azhou-nicira/ovs-review/builds/277412765
>>
>> (The --disable-ssl build is slow for some reason, same as master).
>>
>> May be this is caused by travis running slow for some reason?
>>
>> Did your local test pass?
>>
>
> Yes, I just tried on a VM running Centos 7.3 with the 4.9 kernel and it
> passed there.
>
> /shrug?
>
> OK, I'll continue with review then.
>
> Thanks!
>
> - Greg

FWIW. the --disable-ssl build finally passed. The total build/test
time is 4hr 42min.
Travis CI is definitely slow today.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [merge native tunneling and patch port 1/7] ofproto-dpif: Unfreeze within clone

[Andy Zhou]

On Tue, Sep 19, 2017 at 9:55 AM, Greg Rose <gvrose8...@gmail.com> wrote:
> On 09/12/2017 12:49 PM, Andy Zhou wrote:
>>
>> When translating actions within open flow clone, actions generated
>> by finish_freezeing() should also be enclosed within the datapath
>> clone netlink encoding.
>>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>
>
> Andy,
>
> I am reviewing and testing your patches.  I have applied them to my private
> github repository
> on a branch named test-813027-35.
>
> https://github.com/gvrose8192/ovs-experimental/tree/test-813027-35
>
> However, the Travis 'TESTSUITE=1 KERNEL=3.16.46 build fails:
>
> https://travis-ci.org/gvrose8192/ovs-experimental/jobs/277364409
>
> Have you noticed this as well?

No. It passed my local test, and passed travis test from my private
branch (just rebased this morning)

https://github.com/azhou-nicira/ovs-review/tree/patch_port

https://travis-ci.org/azhou-nicira/ovs-review/builds/277412765

(The --disable-ssl build is slow for some reason, same as master).

May be this is caused by travis running slow for some reason?

Did your local test pass?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [v2] bridge: Fix controller status update to passive connections

[Andy Zhou]

On Wed, Sep 13, 2017 at 8:30 PM, Joe Stringer <j...@ovn.org> wrote:
> On 13 September 2017 at 13:05, Andy Zhou <az...@ovn.org> wrote:
>> The bug can cause ovs-vswitchd to crash (due to assert) when it is
>> set up with a passive controller connection. Since only active
>> connections are kept, the passive connection status update should be
>> ignored and not trigger asserts.
>>
>> Fixes: 85c55772a453 ("bridge: Fix controller status update")
>> Reported-by: Josh Bailey <j...@faucet.nz>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>
> Acked-by: Joe Stringer <j...@ovn.org>

Thanks for the review. Pushed to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [v2] bridge: Fix controller status update to passive connections

[Andy Zhou]

The bug can cause ovs-vswitchd to crash (due to assert) when it is
set up with a passive controller connection. Since only active
connections are kept, the passive connection status update should be
ignored and not trigger asserts.

Fixes: 85c55772a453 ("bridge: Fix controller status update")
Reported-by: Josh Bailey <j...@faucet.nz>
Signed-off-by: Andy Zhou <az...@ovn.org>

---
v1->v2:  Set defaults when cinfo is NULL.
---
 AUTHORS.rst   |  1 +
 vswitchd/bridge.c | 16 +++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/AUTHORS.rst b/AUTHORS.rst
index 4b9e78f07f87..5d8b723f62e6 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -390,6 +390,7 @@ Ben Basler  bbas...@nicira.com
 Bhargava Shastrybshas...@sec.t-labs.tu-berlin.de
 Bob Ballbob.b...@citrix.com
 Brad Hall   b...@nicira.com
+Brailey Joshj...@faucet.nz
 Brandon Heller  brand...@stanford.edu
 Brendan Kelley  bkel...@nicira.com
 Brent Salisbury brent.salisb...@gmail.com
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a8cbae78cb23..eec9689e6dbd 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2720,11 +2720,17 @@ refresh_controller_status(void)
 struct ofproto_controller_info *cinfo =
 shash_find_data(, cfg->target);
 
-ovs_assert(cinfo);
-ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
-const char *role = ofp12_controller_role_to_str(cinfo->role);
-ovsrec_controller_set_role(cfg, role);
-ovsrec_controller_set_status(cfg, >pairs);
+/* cinfo is NULL when 'cfg->target' is a passive connection.  */
+if (cinfo) {
+ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
+const char *role = ofp12_controller_role_to_str(cinfo->role);
+ovsrec_controller_set_role(cfg, role);
+ovsrec_controller_set_status(cfg, >pairs);
+} else {
+ovsrec_controller_set_is_connected(cfg, false);
+ovsrec_controller_set_role(cfg, NULL);
+ovsrec_controller_set_status(cfg, NULL);
+}
 }
 
 ofproto_free_ofproto_controller_info();
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [merge native tunneling and patch port 7/7] ofproto-dpif-xlate: Refactor native tunnel handling logic

[Andy Zhou]

Merge native tunnel handling with patch port handling
as much as possible.

Current native tunnel handling logic inspects the generated actions
to determine if truncate has been applied to the packet. (since if
it is then recirculation should be used).  This logic can be
simplified by passing the 'truncate' boolean argument into
compose_output_action().

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 243 +--
 1 file changed, 95 insertions(+), 148 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 94aa071a37cd..d320d570b304 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -544,7 +544,7 @@ struct xlate_bond_recirc {
 
 static void compose_output_action(struct xlate_ctx *, ofp_port_t ofp_port,
   const struct xlate_bond_recirc *xr,
-  bool is_last_action);
+  bool is_last_action, bool truncate);
 
 static struct xbridge *xbridge_lookup(struct xlate_cfg *,
   const struct ofproto_dpif *);
@@ -2227,7 +2227,7 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle 
*out_xbundle,
 xvlan_put(>xin->flow, _xvlan);
 
 compose_output_action(ctx, xport->ofp_port, use_recirc ?  : NULL,
-  false);
+  false, false);
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }
 
@@ -3241,130 +3241,10 @@ propagate_tunnel_data_to_flow(struct xlate_ctx *ctx, 
struct eth_addr dmac,
 is_tnl_ipv6, nw_proto);
 }
 
-/* Validate if the transalated combined actions are OK to proceed.
- * If actions consist of TRUNC action, it is not allowed to do the
- * tunnel_push combine as it cannot update stats correctly.
- */
-static bool
-is_tunnel_actions_clone_ready(struct xlate_ctx *ctx)
-{
-struct nlattr *tnl_actions;
-const struct nlattr *a;
-unsigned int left;
-size_t actions_len;
-struct ofpbuf *actions = ctx->odp_actions;
-
-if (!actions) {
-/* No actions, no harm in doing combine. */
-return true;
-}
-
-/* Cannot perform tunnel push on slow path action CONTROLLER_OUTPUT. */
-if (ctx->xout->slow & SLOW_CONTROLLER) {
-return false;
-}
-actions_len = actions->size;
-
-tnl_actions =(struct nlattr *)(actions->data);
-NL_ATTR_FOR_EACH_UNSAFE (a, left, tnl_actions, actions_len) {
-int type = nl_attr_type(a);
-if (type == OVS_ACTION_ATTR_TRUNC) {
-VLOG_DBG("Cannot do tunnel action-combine on trunc action");
-return false;
-break;
-}
-}
-return true;
-}
-
-static bool
-validate_and_combine_post_tnl_actions(struct xlate_ctx *ctx,
-  const struct xport *xport,
-  struct xport *out_dev,
-  struct ovs_action_push_tnl tnl_push_data)
-{
-const struct dpif_flow_stats *backup_resubmit_stats;
-struct xlate_cache *backup_xcache;
-bool nested_act_flag = false;
-struct flow_wildcards tmp_flow_wc;
-struct flow_wildcards *backup_flow_wc_ptr;
-bool backup_side_effects;
-const struct dp_packet *backup_pkt;
-
-memset(_flow_wc, 0 , sizeof tmp_flow_wc);
-backup_flow_wc_ptr = ctx->wc;
-ctx->wc = _flow_wc;
-ctx->xin->wc = NULL;
-backup_resubmit_stats = ctx->xin->resubmit_stats;
-backup_xcache = ctx->xin->xcache;
-backup_side_effects = ctx->xin->allow_side_effects;
-backup_pkt = ctx->xin->packet;
-
-size_t push_action_size = 0;
-size_t clone_ofs = nl_msg_start_nested(ctx->odp_actions,
-   OVS_ACTION_ATTR_CLONE);
-odp_put_tnl_push_action(ctx->odp_actions, _push_data);
-push_action_size = ctx->odp_actions->size;
-
-ctx->xin->resubmit_stats =  NULL;
-ctx->xin->xcache = xlate_cache_new(); /* Use new temporary cache. */
-ctx->xin->allow_side_effects = false;
-ctx->xin->packet = NULL;
-
-/* Push the cache entry for the tunnel first. */
-struct xc_entry *entry;
-entry = xlate_cache_add_entry(ctx->xin->xcache, XC_TUNNEL_HEADER);
-entry->tunnel_hdr.hdr_size = tnl_push_data.header_len;
-entry->tunnel_hdr.operation = ADD;
-
-patch_port_output(ctx, xport, out_dev);
-nested_act_flag = is_tunnel_actions_clone_ready(ctx);
-
-if (nested_act_flag) {
- /* Similar to the stats update in revalidation, the x_cache entries
-  * are populated by the previous translation are used to update the
-  * stats correctly.
-  */
-if (backup_resubmit_stats) {
-struct dpif_flow_stats tmp_resubmit_stats;
-memcpy(_resubmit_stats, 

[ovs-dev] [merge native tunneling and patch port 6/7] ofproto-dpif-xlate: Rename apply_nested_clone_actions()

[Andy Zhou]

To patch_port_output(). The original function name does not make
much sense.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 81853c29afbf..94aa071a37cd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -432,8 +432,8 @@ static void xlate_action_set(struct xlate_ctx *ctx);
 static void xlate_commit_actions(struct xlate_ctx *ctx);
 
 static void
-apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport *in_dev,
-   struct xport *out_dev);
+patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
+  struct xport *out_dev);
 
 static void
 ctx_trigger_freeze(struct xlate_ctx *ctx)
@@ -3317,7 +3317,7 @@ validate_and_combine_post_tnl_actions(struct xlate_ctx 
*ctx,
 entry->tunnel_hdr.hdr_size = tnl_push_data.header_len;
 entry->tunnel_hdr.operation = ADD;
 
-apply_nested_clone_actions(ctx, xport, out_dev);
+patch_port_output(ctx, xport, out_dev);
 nested_act_flag = is_tunnel_actions_clone_ready(ctx);
 
 if (nested_act_flag) {
@@ -3509,21 +3509,22 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, 
const struct flow *flow, co
 xport_in->xbundle->protected && xport_out->xbundle->protected);
 }
 
-/* Function to combine actions from following device/port with the current
- * device actions in openflow pipeline. Mainly used for the translation of
- * patch/tunnel port output actions. It pushes the openflow state into a stack
- * first, clear out to execute the packet through the device and finally pop
- * the openflow state back from the stack. This is equivalent to cloning
- * a packet in translation for the duration of execution.
+/* Function handles when a packet is sent from one bridge to another bridge.
  *
- * On output to a patch port, the output action will be replaced with set of
- * nested actions on the peer patch port.
- * Similarly on output to a tunnel port, the post nested actions on
- * tunnel are chained up with the tunnel-push action.
+ * The bridges are internally connected, either with patch ports or with
+ * tunnel ports.
+ *
+ * The output action to another bridge causes translation to continue within
+ * the next bridge. This process can be recursive; the next bridge can
+ * output yet to another bridge.
+ *
+ * The translated actions from the second bridge onwards are enclosed within
+ * the clone action, so that any modification to the packet will not be visible
+ * to the remaining actions of the originating bridge.
  */
 static void
-apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport *in_dev,
-  struct xport *out_dev)
+patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
+  struct xport *out_dev)
 {
 struct flow *flow = >xin->flow;
 struct flow old_flow = ctx->xin->flow;
@@ -3760,7 +3761,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t 
ofp_port,
 }
 
 if (xport->peer) {
-   apply_nested_clone_actions(ctx, xport, xport->peer);
+   patch_port_output(ctx, xport, xport->peer);
return;
 }
 
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [merge native tunneling and patch port 5/7] ofproto-dpif-xlate: Refactor xlate_table_actions()

[Andy Zhou]

Allow xlate_table_actions() to generate actions with or without
enclosed in clone().

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9e39a4ff7f49..81853c29afbf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -505,14 +505,20 @@ struct xlate_cfg {
 static OVSRCU_TYPE(struct xlate_cfg *) xcfgp = OVSRCU_INITIALIZER(NULL);
 static struct xlate_cfg *new_xcfg = NULL;
 
+typedef void xlate_actions_handler(const struct ofpact *, size_t ofpacts_len,
+   struct xlate_ctx *, bool);
+
 static bool may_receive(const struct xport *, struct xlate_ctx *);
 static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
  struct xlate_ctx *, bool);
+static void clone_xlate_actions(const struct ofpact *, size_t ofpacts_len,
+struct xlate_ctx *, bool);
 static void xlate_normal(struct xlate_ctx *);
 static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
uint8_t table_id, bool may_packet_in,
bool honor_table_miss, bool with_ct_orig,
-   bool is_last_action);
+   bool is_last_action, xlate_actions_handler *);
+
 static bool input_vid_is_valid(const struct xlate_ctx *,
uint16_t vid, struct xbundle *);
 static void xvlan_copy(struct xvlan *dst, const struct xvlan *src);
@@ -3560,7 +3566,7 @@ apply_nested_clone_actions(struct xlate_ctx *ctx, const 
struct xport *in_dev,
 if (xport_stp_forward_state(out_dev) &&
 xport_rstp_forward_state(out_dev)) {
 xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
-   false, true);
+   false, true, clone_xlate_actions);
 if (!ctx->freezing) {
 xlate_action_set(ctx);
 }
@@ -3575,7 +3581,7 @@ apply_nested_clone_actions(struct xlate_ctx *ctx, const 
struct xport *in_dev,
 mirror_mask_t old_mirrors2 = ctx->mirrors;
 
 xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
-   false, true);
+   false, true, clone_xlate_actions);
 ctx->mirrors = old_mirrors2;
 ctx->base_flow = old_base_flow;
 ctx->odp_actions->size = old_size;
@@ -3894,7 +3900,8 @@ compose_output_action(struct xlate_ctx *ctx, ofp_port_t 
ofp_port,
 
 static void
 xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule,
-  bool deepens, bool is_last_action)
+  bool deepens, bool is_last_action,
+  xlate_actions_handler *actions_xlator)
 {
 struct rule_dpif *old_rule = ctx->rule;
 ovs_be64 old_cookie = ctx->rule_cookie;
@@ -3910,8 +3917,8 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif 
*rule,
 ctx->rule = rule;
 ctx->rule_cookie = rule->up.flow_cookie;
 actions = rule_get_actions(>up);
-do_xlate_actions(actions->ofpacts, actions->ofpacts_len, ctx,
- is_last_action);
+actions_xlator(actions->ofpacts, actions->ofpacts_len, ctx,
+   is_last_action);
 ctx->rule_cookie = old_cookie;
 ctx->rule = old_rule;
 ctx->depth -= deepens;
@@ -3986,7 +3993,8 @@ tuple_swap(struct flow *flow, struct flow_wildcards *wc)
 static void
 xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
bool may_packet_in, bool honor_table_miss,
-   bool with_ct_orig, bool is_last_action)
+   bool with_ct_orig, bool is_last_action,
+   xlate_actions_handler *xlator)
 {
 /* Check if we need to recirculate before matching in a table. */
 if (ctx->was_mpls) {
@@ -4038,7 +4046,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t 
in_port, uint8_t table_id,
 struct ovs_list *old_trace = ctx->xin->trace;
 xlate_report_table(ctx, rule, table_id);
 xlate_recursively(ctx, rule, table_id <= old_table_id,
-  is_last_action);
+  is_last_action, xlator);
 ctx->xin->trace = old_trace;
 }
 
@@ -4341,7 +4349,7 @@ xlate_ofpact_resubmit(struct xlate_ctx *ctx,
 
 xlate_table_action(ctx, in_port, table_id, may_packet_in,
honor_table_miss, resubmit->with_ct_orig,
-   is_last_action);
+   is_last_action, do_xlate_actions);
 }
 
 static void
@@ -4888,7 +4896,8 @@ xlate_output_action(struct xlate_ctx *ctx,

[ovs-dev] [merge native tunneling and patch port 4/7] ofproto-dpif-xlate: Keep track of the last action

[Andy Zhou]

When Openflow clone is last action of an action list, it does not
make sense for xlated actions to generate datapath clone action.

This patch attemps to Keep track of last clone action, but not
necessarily for every case.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 include/openvswitch/ofp-actions.h |   7 ++
 ofproto/ofproto-dpif-xlate.c  | 214 +++---
 2 files changed, 140 insertions(+), 81 deletions(-)

diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index ad8e1bec06ba..03c1d11dc9a2 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -216,6 +216,13 @@ ofpact_end(const struct ofpact *ofpacts, size_t 
ofpacts_len)
 return ALIGNED_CAST(struct ofpact *, (uint8_t *) ofpacts + ofpacts_len);
 }
 
+static inline bool
+ofpact_last(const struct ofpact *a, const struct ofpact *ofpacts,
+size_t ofpact_len)
+{
+return ofpact_next(a) == ofpact_end(ofpacts, ofpact_len);
+}
+
 static inline const struct ofpact *
 ofpact_find_type_flattened(const struct ofpact *a, enum ofpact_type type,
const struct ofpact * const end)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 223313d4ecba..9e39a4ff7f49 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -507,11 +507,12 @@ static struct xlate_cfg *new_xcfg = NULL;
 
 static bool may_receive(const struct xport *, struct xlate_ctx *);
 static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
- struct xlate_ctx *);
+ struct xlate_ctx *, bool);
 static void xlate_normal(struct xlate_ctx *);
 static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
uint8_t table_id, bool may_packet_in,
-   bool honor_table_miss, bool with_ct_orig);
+   bool honor_table_miss, bool with_ct_orig,
+   bool is_last_action);
 static bool input_vid_is_valid(const struct xlate_ctx *,
uint16_t vid, struct xbundle *);
 static void xvlan_copy(struct xvlan *dst, const struct xvlan *src);
@@ -536,7 +537,8 @@ struct xlate_bond_recirc {
 };
 
 static void compose_output_action(struct xlate_ctx *, ofp_port_t ofp_port,
-  const struct xlate_bond_recirc *xr);
+  const struct xlate_bond_recirc *xr,
+  bool is_last_action);
 
 static struct xbridge *xbridge_lookup(struct xlate_cfg *,
   const struct ofproto_dpif *);
@@ -2218,7 +2220,8 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle 
*out_xbundle,
 memcpy(_vlans, >xin->flow.vlans, sizeof(old_vlans));
 xvlan_put(>xin->flow, _xvlan);
 
-compose_output_action(ctx, xport->ofp_port, use_recirc ?  : NULL);
+compose_output_action(ctx, xport->ofp_port, use_recirc ?  : NULL,
+  false);
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }
 
@@ -3557,7 +3560,7 @@ apply_nested_clone_actions(struct xlate_ctx *ctx, const 
struct xport *in_dev,
 if (xport_stp_forward_state(out_dev) &&
 xport_rstp_forward_state(out_dev)) {
 xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
-   false);
+   false, true);
 if (!ctx->freezing) {
 xlate_action_set(ctx);
 }
@@ -3572,7 +3575,7 @@ apply_nested_clone_actions(struct xlate_ctx *ctx, const 
struct xport *in_dev,
 mirror_mask_t old_mirrors2 = ctx->mirrors;
 
 xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
-   false);
+   false, true);
 ctx->mirrors = old_mirrors2;
 ctx->base_flow = old_base_flow;
 ctx->odp_actions->size = old_size;
@@ -3716,7 +3719,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, ofp_port_t 
ofp_port,
 
 static void
 compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
-const struct xlate_bond_recirc *xr, bool check_stp)
+const struct xlate_bond_recirc *xr, bool check_stp,
+bool is_last_action OVS_UNUSED)
 {
 const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
 struct flow_wildcards *wc = ctx->wc;
@@ -3882,13 +3886,15 @@ compose_output_action__(struct xlate_ctx *ctx, 
ofp_port_t ofp_port,
 
 static void
 compose_output_action(struct xlate_ctx *ctx, ofp_port_t ofp_port,
-  const struct xlate_bond_recirc *xr)
+  const struct xlate_bond_recirc *xr,
+  bool is_last_action)
 {
-compose_output

[ovs-dev] [merge native tunneling and patch port 2/7] ofproto-dpif-xlate: Remove uncessary assignment

[Andy Zhou]

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e5ad832d7c47..d5b47666e974 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5373,7 +5373,6 @@ compose_clone(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 if (ctx->xbridge->support.clone) { /* Use clone action */
 /* Use clone action as datapath clone. */
 offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
-ac_offset = ctx->odp_actions->size;
 do_xlate_actions(oc->actions, oc_actions_len, ctx);
 if (ctx->freezing) {
 finish_freezing(ctx);
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [merge native tunneling and patch port 1/7] ofproto-dpif: Unfreeze within clone

[Andy Zhou]

When translating actions within open flow clone, actions generated
by finish_freezeing() should also be enclosed within the datapath
clone netlink encoding.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9e1f837cb23e..e5ad832d7c47 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5353,6 +5353,9 @@ compose_clone(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 if (reversible_actions(oc->actions, oc_actions_len)) {
 old_flow = ctx->xin->flow;
 do_xlate_actions(oc->actions, oc_actions_len, ctx);
+if (ctx->freezing) {
+finish_freezing(ctx);
+}
 goto xlate_done;
 }
 
@@ -5372,6 +5375,9 @@ compose_clone(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
 ac_offset = ctx->odp_actions->size;
 do_xlate_actions(oc->actions, oc_actions_len, ctx);
+if (ctx->freezing) {
+finish_freezing(ctx);
+}
 nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
 goto dp_clone_done;
 }
@@ -5382,6 +5388,9 @@ compose_clone(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 ac_offset = nl_msg_start_nested(ctx->odp_actions,
 OVS_SAMPLE_ATTR_ACTIONS);
 do_xlate_actions(oc->actions, oc_actions_len, ctx);
+if (ctx->freezing) {
+finish_freezing(ctx);
+}
 if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
 nl_msg_cancel_nested(ctx->odp_actions, offset);
 } else {
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.8] Prepare for 2.8.1.

[Andy Zhou]

On Fri, Sep 1, 2017 at 11:50 AM, Justin Pettit <jpet...@ovn.org> wrote:
> Signed-off-by: Justin Pettit <jpet...@ovn.org>
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] bridge: Fix controller status update to passive connections

[Andy Zhou]

On Wed, Sep 6, 2017 at 11:59 PM, Joe Stringer <j...@ovn.org> wrote:
> On 6 September 2017 at 15:24, Andy Zhou <az...@ovn.org> wrote:
>> The bug can cause ovs-vswitchd to crash (due to assert) when it is
>> set up with a passive controller connection. Since only active
>> connections are kept, the passive connection status update should be
>> ignored and not trigger asserts.
>>
>
> Fixes: 85c55772a453 ("bridge: Fix controller status update")

Thanks. Will add to the commit message.
>
>> Reported-by: Josh Bailey <j...@faucet.nz>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>> ---
>
> This bug was reported on v2.8.0, so will need backport to branch-2.8.

Will keep this in mind when pushing.
>
>>  AUTHORS.rst   |  1 +
>>  vswitchd/bridge.c | 12 +++-
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/AUTHORS.rst b/AUTHORS.rst
>> index cb0cd91b21ba..e304609b2b27 100644
>> --- a/AUTHORS.rst
>> +++ b/AUTHORS.rst
>> @@ -389,6 +389,7 @@ Ben Basler  bbas...@nicira.com
>>  Bhargava Shastrybshas...@sec.t-labs.tu-berlin.de
>>  Bob Ballbob.b...@citrix.com
>>  Brad Hall   b...@nicira.com
>> +Brailey Joshj...@faucet.nz
>>  Brandon Heller  brand...@stanford.edu
>>  Brendan Kelley  bkel...@nicira.com
>>  Brent Salisbury brent.salisb...@gmail.com
>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>> index a8cbae78cb23..00f86182c820 100644
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -2720,11 +2720,13 @@ refresh_controller_status(void)
>>  struct ofproto_controller_info *cinfo =
>>  shash_find_data(, cfg->target);
>>
>> -ovs_assert(cinfo);
>> -ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
>> -const char *role = ofp12_controller_role_to_str(cinfo->role);
>> -ovsrec_controller_set_role(cfg, role);
>> -ovsrec_controller_set_status(cfg, >pairs);
>> +/* cinfo is NULL when 'cfg->target' is a passive connection.  */
>> +if (cinfo) {
>> +ovsrec_controller_set_is_connected(cfg, 
>> cinfo->is_connected);
>> +const char *role = 
>> ofp12_controller_role_to_str(cinfo->role);
>> +ovsrec_controller_set_role(cfg, role);
>> +ovsrec_controller_set_status(cfg, >pairs);
>> +}
>
> Prior to commit 85c55772a453f, the following lines were in the alternative 
> case:
> ovsrec_controller_set_is_connected(cfg, false);
> ovsrec_controller_set_role(cfg, NULL);
> ovsrec_controller_set_status(cfg, NULL);
>
> Should we update the records in the else case here like was previously done?

I don't think it is necessary. Those are the default values and we are
not changing them.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] bridge: Fix controller status update to passive connections

[Andy Zhou]

The bug can cause ovs-vswitchd to crash (due to assert) when it is
set up with a passive controller connection. Since only active
connections are kept, the passive connection status update should be
ignored and not trigger asserts.

Reported-by: Josh Bailey <j...@faucet.nz>
Signed-off-by: Andy Zhou <az...@ovn.org>
---
 AUTHORS.rst   |  1 +
 vswitchd/bridge.c | 12 +++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/AUTHORS.rst b/AUTHORS.rst
index cb0cd91b21ba..e304609b2b27 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -389,6 +389,7 @@ Ben Basler  bbas...@nicira.com
 Bhargava Shastrybshas...@sec.t-labs.tu-berlin.de
 Bob Ballbob.b...@citrix.com
 Brad Hall   b...@nicira.com
+Brailey Joshj...@faucet.nz
 Brandon Heller  brand...@stanford.edu
 Brendan Kelley  bkel...@nicira.com
 Brent Salisbury brent.salisb...@gmail.com
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a8cbae78cb23..00f86182c820 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2720,11 +2720,13 @@ refresh_controller_status(void)
 struct ofproto_controller_info *cinfo =
 shash_find_data(, cfg->target);
 
-ovs_assert(cinfo);
-ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
-const char *role = ofp12_controller_role_to_str(cinfo->role);
-ovsrec_controller_set_role(cfg, role);
-ovsrec_controller_set_status(cfg, >pairs);
+/* cinfo is NULL when 'cfg->target' is a passive connection.  */
+if (cinfo) {
+ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
+const char *role = ofp12_controller_role_to_str(cinfo->role);
+ovsrec_controller_set_role(cfg, role);
+ovsrec_controller_set_status(cfg, >pairs);
+}
 }
 
 ofproto_free_ofproto_controller_info();
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of conditional monitor_cond requests.

[Andy Zhou]

On Wed, Aug 30, 2017 at 9:33 AM, Ben Pfaff <b...@ovn.org> wrote:
> The current implementation of ovsdb-server caches only non-conditional
> monitors, that is, monitors for every table row, not those that monitor
> only rows that match some condition.  To figure out which monitors are
> conditional, the code track the number of tables that have conditions that
> are uniformly true (cond->n_true_cnd) and compares that against the number
> of tables in the condition (shash_count(>tables)).  If they are the
> same, then every table has (effectively) no condition, and so
> cond->conditional is set to false.
>
> However, the implementation was buggy.  The function that adds a new
> table condition, ovsdb_monitor_table_condition_create(), only updated
> cond->conditional if the table condition being added was true.  This is
> wrong; only adding a non-true condition can actually change
> cond->conditional.  This commit fixes the problem by always recalculating
> cond->conditional.
>
> The most visible side effect of cond->conditional being true when it
> should be false, as caused by this bug, was that conditional monitors were
> being mixed with unconditional monitors for the purpose of caching.  This
> meant that, if a client requested a conditional monitor that was the
> same as an unconditional one, except for the condition, then the client
> would receive the cached data previously sent for the unconditional one.
> This commit fixes the problem.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>

I think the first paragraph of the commit message can be useful as comments
in the code as well.

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] monitor: Simplify calculation of cond->conditional.

[Andy Zhou]

On Wed, Aug 30, 2017 at 9:33 AM, Ben Pfaff <b...@ovn.org> wrote:
> This removes n_true_cnd from struct ovsdb_monitor_session_condition.
> It was an "optimization" that is not part of any inner loop, but
> make the code harder to reason about.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>

Thanks for the simplification.
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH/RFC] bond: add enable-recirc configuration for bond.

[Andy Zhou]

On Wed, Aug 16, 2017 at 7:59 AM, Simon Horman
<simon.hor...@netronome.com> wrote:
> Hi Andy,
>
> On Tue, Aug 15, 2017 at 01:31:36PM -0700, Andy Zhou wrote:
>> On Tue, Aug 15, 2017 at 9:13 AM, Simon Horman
>> <simon.hor...@netronome.com> wrote:
>> > From: Pieter Jansen van Vuuren <pieter.jansenvanvuu...@netronome.com>
>> >
>> > Adds a config parameter that determines if a bond will use recirculation
>> > in the kernel datapath when implementing a bond in balance-tcp mode.
>>
>> Using recirc for bonding is a lower level decision. I am not sure
>> OVSDB is right level
>> for exposing this detail.
>
> I do see that this configuration is arguably more detailed than existing
> bond parameters set via OVSDB, but it is still a bond parameter so perhaps
> it isn't entirely out of place here.
>
>> > The default for enable-recirc is "true", resulting in the traditional
>> > implementation of a bond in balance-tcp mode. Setting enable-recirc to
>> > false results in datapath rules that do not rely on the recirculation
>> > action.
>> >
>> > example usage:
>> > ovs-vsctl set port bond0 other_config:enable-recirc=false
>> >
>> > Advantages:
>> > - Allows TC offloading of OVS bonds on hardware which
>> >   does not support recirculation
>>
>> Not sure it is an advantage. Bonding with recirc allows the first flow to
>> be a mega flow.  without megaflow, all bond traffic has to be micro
>> flows, which was how OVS implements bonding. The recirc was introduced to
>> solve the flow explosion problem
>>
>> Without supporting recirc, I'd image TC offloading would suffer the same
>> issue. May be it is correct not to offload the bond flows, at least not
>> before recirc is supported in TC.
>>
>> Another way to look at this is that when TC ran out of flow space, and
>> bonding is supported by the kernel module. This configuration would
>> prevent kernel module from using recirc.
>
> I believe that your points regarding flow explosion and fallback to
> the kernel datapath are well made. However, I also believe there is
> hardware where using recirculation is not practical. For that
> reason I think it is worth exploring this proposal further.

I am not sure offloading microflows to hardware is a good ideal because of
flow explosion.
>
>> > - Appears to result in lower latency (in systems using few flows).
>>
>> Not sure this is true in general. When using recirc with proper
>> megaflow, the latency should
>> improve for new microflows since upcalls can be omitted.
>
> That may be true in terms of flow setup cost but I believe that
> the per-packet cost is going to be higher when recirculation is used.
>
>> > Quick ping test results in:
>> >
>> > other_config:enable-recirc=false
>> > rtt min/avg/max/mdev = 0.039/0.193/7.612/1.059 ms
>> >
>> > other_config:enable-recirc=true
>> > rtt min/avg/max/mdev = 0.038/0.321/14.091/1.967 ms
>> >
>>
>> When setting up the flow, 'recirc" requires 2 miss  upcalls, thus set
>> up latency is larger.
>> However, once flows are set up, the latency difference should be minimal.
>>
>> I'd expect the max latency to be different with or without recirc.
>> Notice the minimal latency difference is small.
>> I'd expect the average latency to converge to minimal for sufficiently
>> large number of ping packets.
>
> Latency may not be the best way to measure this but having
> to classify each packet twice, as I believe is the case when
> recirculation is used, must cost something.

I agree that recirculation is not free. The trade off is that it
avoids flow explosion.
>
>> > More comprehensive testing is in progress.
>> >
>> > Signed-off-by: Pieter Jansen van Vuuren 
>> > <pieter.jansenvanvuu...@netronome.com>
>> > Signed-off-by: Simon Horman <simon.hor...@netronome.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dp-packet: Introduce dp_packet_batch_add_unsafe().

[Andy Zhou]

On Fri, Aug 11, 2017 at 7:41 AM, Ilya Maximets  wrote:
>
> Could you suggest another dp_packet_batch_XXX() name (which can be exposed
> to the end user) for the function that doesn't check the boundaries instead
> of 'add_unsafe', if you think it is not accurate?

How about using an existing API, dp_packet_batch_refill()
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dp-packet: Introduce dp_packet_batch_add_unsafe().

[Andy Zhou]

> Maybe I don't fully understand what you're trying to say, but I want to use
> unsafe function in dpif-netdev for per-flow packet batching (see the patch)
> and it should not be internal for that case.
> (It's safe to use unsafe function there because per-flow batches are
> guaranteed to be less than original rx batch.)

Let me try again.  As you ave pointed out, it is not really unsafe to
use the function
at those calling sites, I think dp_packet_batch_add__() is a suitable
name. In case
you prefer a different function name, that's fine too, I just don't
think unsafe is accurate.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3 2/2] dpif: Clean up netdev_ports map on dpif_close().

[Andy Zhou]

On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer <j...@ovn.org> wrote:
> Commit 32b77c316d9982("dpif: Save added ports in a port map.")
> introduced tracking of all dpif ports by taking a reference on each
> available netdev when the dpif is opened, but it failed to clear out and
> release references to these netdevs when the dpif is closed.
>
> One of the problems introduced by this was that upon clean exit of
> ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device
> was not deleted. This which could cause problems in subsequent start up.
> Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed
> this particular problem by not adding such devices to the netdev_ports
> map, but the referencing/unreferencing upon dpif_open()/dpif_close() is
> still not balanced.
>
> Balance the referencing of netdevs by clearing these during dpif_close().
>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Signed-off-by: Joe Stringer <j...@ovn.org>

A minor comment below.
Acked-by: Andy Zhou <az...@ovn.org>
> ---
> v3: Rather than flushing ports from this dpif, iterate ports and remove
> them.
> v2: Update commit message.
> Rebase.
> v1: Initial posting.
> ---
>  lib/dpif.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index e71f6a3d1475..eccd8607f17e 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -435,8 +435,17 @@ dpif_close(struct dpif *dpif)
>  {
>  if (dpif) {
>  struct registered_dpif_class *rc;
> +struct dpif_port_dump port_dump;
> +struct dpif_port dpif_port;
>
>  rc = shash_find_data(_classes, dpif->dpif_class->type);
> +
> +DPIF_PORT_FOR_EACH (_port, _dump, dpif) {
> +if (dpif_is_internal_port(dpif_port.type)) {
> +continue;
> +}
> +netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
Would it be easier to read with:
if (!dpif_is_internal_port(dpif_port.type)) {
 netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
   }

> +}
>  dpif_uninit(dpif, true);
>  dp_class_unref(rc);
>  }
> --
> 2.13.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3 1/2] netdev: Free ifidx mapping in netdev_ports_remove().

[Andy Zhou]

On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer <j...@ovn.org> wrote:
> Previously, netdev_ports_insert() would allocate and insert an
> ifindex->odp_port mapping, but netdev_ports_remove() would never remove
> the mapping or free the mapping structure. This patch fixes these up.
>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Reported-by: Andy Zhou <az...@ovn.org>
> Signed-off-by: Joe Stringer <j...@ovn.org>

A few minor comments inline. Otherwise,

Acked-by: Andy Zhou <az...@ovn.org>

> ---
> v3: First post.
> ---
>  lib/netdev.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 7e9896b82928..a8f5348264c8 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -2266,9 +2266,20 @@ netdev_ports_remove(odp_port_t port_no, const struct 
> dpif_class *dpif_class)
>  data = netdev_ports_lookup(port_no, dpif_class);
>
>  if (data) {
> +int ifindex = netdev_get_ifindex(data->netdev);

I think it will be safer to check the return value of ifindex.

> +struct ifindex_to_port_data *ifidx;
> +
> +HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, _to_port) {
> +if (ifidx->port == port_no) {
> +break;
> +}
> +}

Here we expect ifidx to be a valid pointer. I think the code can be
more readable and safer to
add an assertion.
> +
>  dpif_port_destroy(>dpif_port);
>  netdev_close(data->netdev); /* unref and possibly close */
> +hmap_remove(_to_port, >node);
>  hmap_remove(_to_netdev, >node);
> +free(ifidx);
>  free(data);
>  ret = 0;
>  }
> --
> 2.13.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 2/2] dpif: Fix clean up of dpif_ports on dpif_close().

[Andy Zhou]

On Tue, Aug 8, 2017 at 11:23 AM, Joe Stringer  wrote:
> Commit 32b77c316d9982("dpif: Save added ports in a port map.")
> introduced tracking of all dpif ports by taking a reference on each
> available netdev when the dpif is opened, but it failed to clear out and
> release references to these netdevs when the dpif is closed.
>
> One of the problems introduced by this was that upon clean exit of
> ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device
> was not deleted. This which could cause problems in subsequent start up.
> Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed
> this particular problem by not adding such devices to the netdev_ports
> map, but the referencing/unreferencing upon dpif_open()/dpif_close() is
> still not balanced.
>
> Balance the referencing of netdevs by introducing netdev_ports_flush()
> and clearing these during dpif_close().
>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Signed-off-by: Joe Stringer 

I have a slightly different take on the bug fix. I am not super
familiar with this code,
so it could be wrong.

Tracing the calling API, dpif_open -> do_open -> netdev_ports_insert().  So it
would make sense for dpif_close to call netdev_ports_remove() to balance the
reference issue mentioned in the comment. If true, then
netdev_ports_flush() (previous
patch) is not necessary.

Currently, netdev_ports_remove() does not clean up ifindex_to_port
map. Not clear
it is correct.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dp-packet: Introduce dp_packet_batch_add_unsafe().

[Andy Zhou]

On Mon, Aug 7, 2017 at 11:01 PM, Ilya Maximets <i.maxim...@samsung.com> wrote:
> On 07.08.2017 23:24, Andy Zhou wrote:
>> On Mon, Aug 7, 2017 at 8:50 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
>>> Almost all batch usecases covered by the new API introduced
>>> in commit 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
>>> except unsafe batch addition. It used in dpif-netdev for fast
>>> per-flow batches filling. Introduction of this new function will
>>> allow to avoid most direct accesses to the batch structure.
>>>
>>> Function defined as 'inline' in .h file. Should not impact on
>>> performance.
>>>
>>> Additionally unsafe version now used in dp_packet_batch_clone()
>>> to speed up it a little, and also fixed few cases missed while
>>> API enchancing.
>>>
>>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>>
>> Those are worthwhile optimizations.  I have a minor suggestion for you
>> to consider:
>>
>> Instead of adding a new API dp_packet_batch_add_unsafe(), how about
>> just redefine
>> current APIs.
>>
>> dp_packet_batch_add() currently does not do much, it simply call
>> dp_packet_batch_add__(),
>> We can just turn this API into the safe version, same as
>> dp_packet_batch_add__().
>>
>> dp_packet_batch_add__() can be changed into the unsafe version. Use it
>> within the reset
>> of the patch where boundary checks can be avoided.
>
> dp_packet_batch_add__() should be renamed in this case to *add_unsafe()
> or something else anyway because "__" suffix means "internal use only".
> We should not expose function with such name for global using.

Because the add__() version omits boundary checks, it should not be used
by the end user. Rather it should only be used for implementing
dp_packet_batch_xxx APIs.
So, in this case, it is internal use only.  I don't think add_unsafe()
adds any additional value.
>
> Also, dp_packet_batch_add__() used inside dp_packet_batch_refill(). We
> will need to modify refill logic somehow in this case. --> more API
> changes.

We can make add__() API takes one additional argument, "idx",  Would this help?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-kmod-macros: Load TFTP module.

[Andy Zhou]

On Mon, Aug 7, 2017 at 2:58 PM, Joe Stringer <j...@ovn.org> wrote:
> Just like the FTP module needs to be loaded to ensure that the FTP tests
> work, the TFTP module needs to be loaded to ensure that the TFTP tests
> work. This patch does so.
>
> Fixes: 200a9af97d1c ("System tests: Add 4 new ftp and tftp tests.")
> Signed-off-by: Joe Stringer <j...@ovn.org>
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-traffic: Fix TFTP NAT skip check.

[Andy Zhou]

On Mon, Aug 7, 2017 at 1:05 PM, Joe Stringer <j...@ovn.org> wrote:
> This test checked whether FTP support was available rather than TFTP.
> It should check for TFTP, fix it.
>
> Fixes: 200a9af97d1c ("System tests: Add 4 new ftp and tftp tests.")
> Signed-off-by: Joe Stringer <j...@ovn.org>
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif-ipfix: Use common OVS functions for getting current time.

[Andy Zhou]

On Mon, Jun 5, 2017 at 6:04 PM, Ben Pfaff <b...@ovn.org> wrote:
> OVS has common infrastructure functions for getting the current time, but
> this code was not using them.  It is not clear why, so this commit changes
> it to use them.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dp-packet: Introduce dp_packet_batch_add_unsafe().

[Andy Zhou]

On Mon, Aug 7, 2017 at 8:50 AM, Ilya Maximets  wrote:
> Almost all batch usecases covered by the new API introduced
> in commit 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
> except unsafe batch addition. It used in dpif-netdev for fast
> per-flow batches filling. Introduction of this new function will
> allow to avoid most direct accesses to the batch structure.
>
> Function defined as 'inline' in .h file. Should not impact on
> performance.
>
> Additionally unsafe version now used in dp_packet_batch_clone()
> to speed up it a little, and also fixed few cases missed while
> API enchancing.
>
> Signed-off-by: Ilya Maximets 

Those are worthwhile optimizations.  I have a minor suggestion for you
to consider:

Instead of adding a new API dp_packet_batch_add_unsafe(), how about
just redefine
current APIs.

dp_packet_batch_add() currently does not do much, it simply call
dp_packet_batch_add__(),
We can just turn this API into the safe version, same as
dp_packet_batch_add__().

dp_packet_batch_add__() can be changed into the unsafe version. Use it
within the reset
of the patch where boundary checks can be avoided.

Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] debian.rst: Clarify that "dpkg" needs manual help with dependencies.

[Andy Zhou]

On Thu, Jul 6, 2017 at 4:47 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Mon, May 29, 2017 at 11:40:51AM -0700, Ben Pfaff wrote:
>> Reported-by: Mircea Ulinic <p...@mirceaulinic.net>
>> Signed-off-by: Ben Pfaff <b...@ovn.org>

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: Document clarification for some bad wording in RFC 7047.

[Andy Zhou]

On Thu, Jul 27, 2017 at 4:20 PM, Ben Pfaff <b...@ovn.org> wrote:
> Reported-by: Harish Kanakaraju <hkanakar...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@ovn.org>

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Add OFPACT_ENCAP, OFPACT_DECAP to reversible_actions().

[Andy Zhou]

On Thu, Aug 3, 2017 at 1:20 PM, Ben Pfaff <b...@ovn.org> wrote:
> Fixes a broken build when building with --enable-Werror.
>
> I guess that encap and decap are often reversible, but it's safe to
> consider them irreversible.
>
> CC: Andy Zhou <az...@ovn.org>
> Fixes: eee693934aac ("xlate: Emit datapath clone only when necessary.")
> Signed-off-by: Ben Pfaff <b...@ovn.org>

Sorry for breaking the build. The change make sense.
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovs-ofctl: Avoid unnecessary flow replacement in "replace-flows" command.

[Andy Zhou]

On Wed, Aug 2, 2017 at 2:13 PM, Ben Pfaff <b...@ovn.org> wrote:
> This bug fix still needs a review.  Also, Kevin, if you can verify that
> it fixes the behavior you see, that would also be helpful.
>
> On Thu, Jul 06, 2017 at 04:40:30PM -0700, Ben Pfaff wrote:
>> The ovs-ofctl "diff-flows" and "replace-flows" command compare the flows
>> in two flow tables.  Until now, the "replace-flows" command has considered
>> certain almost meaningless differences related to the version of OpenFlow
>> used to add a flow as significant, which caused it to replace a flow by an
>> identical-in-practice version, e.g. in the following, the "replace-flows"
>> command prints a FLOW_MOD that adds the flow that was already added
>> previously:
>>
>> $ cat > flows
>> actions=resubmit(,1)
>> $ ovs-vsctl add-br br0
>> $ ovs-ofctl del-flows br0
>> $ ovs-ofctl add-flows br0 flows
>> $ ovs-ofctl -vvconn replace-flows br0 flows 2>&1 | grep FLOW_MOD
>>
>> Re-adding an existing flow has some effects, for example, it resets the
>> flow's duration, so it's better to avoid it.
>>
>> This commit fixes the problem using the same trick previously used for a
>> similar problem with the "diff-flows" command, which was fixed in commit
>> 98f7f427bf8b ("ovs-ofctl: Avoid printing false differences on "ovs-ofctl
>> diff-flows".").
>>
>> Reported-by: Kevin Lin <ke...@quilt.io>
>> Signed-off-by: Ben Pfaff <b...@ovn.org>
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] bridge: Avoid read of uninitialized data configuring Auto-Attach.

[Andy Zhou]

On Wed, Aug 2, 2017 at 2:12 PM, Ben Pfaff <b...@ovn.org> wrote:
> This still needs a (trivial) review.
>
> On Thu, Jul 06, 2017 at 02:33:42PM -0700, Ben Pfaff wrote:
>> Reported-by: "qintao (F)" <qint...@huawei.com>
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2017-April/044309.html
>> Signed-off-by: Ben Pfaff <b...@ovn.org>

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [clone optmization v2 0/7] Clone optimization

[Andy Zhou]

On Thu, Aug 3, 2017 at 11:27 AM, Ben Pfaff <b...@ovn.org> wrote:
> On Fri, Jul 21, 2017 at 03:46:26PM -0700, Andy Zhou wrote:
>> I respined the patches based on review feedback at:
>>
>> http://mail.openvswitch.org/pipermail/ovs-dev/2017-June/333502.html
>>
>> Patch 1-5: Implements a generic command that can defeature and restore
>>the datapath support level.  They are not directly related
>>to the main objective this series, but helps in writing tests
>>for later patches.
>>
>> Patch 6-7: The main patches, especially patch 7.
>>
>> Compare to the original patch series, This patch series did not
>> go into using clone for patch port translation. The main reason
>> is that patch port translation is being worked on as part of
>> the native tunneling work. I plan to work on it after this series
>> has been reviewed, and after the dust settles with the native
>> tunneling patch series.
>
> For the series:
> Acked-by: Ben Pfaff <b...@ovn.org>

Ben, thanks for the review. Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] Eliminate most shadowing for local variable names.

[Andy Zhou]

On Wed, Aug 2, 2017 at 8:56 AM, Ben Pfaff <b...@ovn.org> wrote:
> Shadowing is when a variable with a given name in an inner scope hides a
> different variable with the same name in a surrounding scope.  This is
> generally undesirable because it can confuse programmers.  This commit
> eliminates most of it.
>
> Found with -Wshadow=local in GCC 7.  The repo is not really ready to enable
> this option by default because of a few cases that are harder to fix, and
> harmless, such as nested use of CMAP_FOR_EACH.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif-xlate: Eliminate duplicate read of xcfgp.

[Andy Zhou]

On Wed, Aug 2, 2017 at 8:56 AM, Ben Pfaff <b...@ovn.org> wrote:
> This inner 'xcfg' shadowed the outer one and could have read a different
> value if 'xcfgp' was changing, so this is possibly a bug fix.
>
> Found by -Wshadow=local in GCC 7.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] bond: Fix broken rebalancing after link state changes.

[Andy Zhou]

On Thu, Jul 20, 2017 at 10:21 AM, Ilya Maximets  wrote:
> There are 3 constraints for moving hashes from one slave to another:
>
> 1. The load difference is larger than ~3% of one slave's load.
> 2. The load difference between slaves exceeds 10 bytes.
> 3. Moving of the hash makes the load difference lower by > 10%.
>
> In current implementation if one of the slaves goes DOWN state, all
> the hashes assigned to it will be moved to other slaves. After that,
> if slave will go UP it will wait for rebalancing to get some hashes.
> But in case where we have more than 10 equally loaded hashes it
> will never fit constraint #3, because each hash will handle less than
> 10% of the load. Situation become worse when number of flows grows
> higher and it's almost impossible to migrate any hash when all the
> 256 hash entries are used which is very likely when we have few
> hundreds/thousands of flows.
>
> As a result, if one of the slaves goes down and up while traffic
> flows, it will never be used again for packet transmission.
> Situation will not be fixed even if we'll stop traffic completely
> and start it again because first two constraints will block
> rebalancing on the earlier stages while we have low amount of traffic.
>
> Moving of one hash if destination has no hashes as it was before
> commit c460a6a7bc75 ("ofproto/bond: simplify rebalancing logic")
> will not help because having one hash isn't enough to make load
> difference less than 10% of total load and this slave will
> handle only that one hash forever.
>
> To fix this lets try to move few hashes simultaniously to fit
> constraint #3.

Thanks for working on this.

Current interface tries to only rebalance one hash bucket at a time.
There are two reasons I can think of:

In general, It is good idea to keep flow binding stable. Moving flow
introduces side effects, such as packet re-ordering, that impacts
application performances significantly.

The second reason is the bandwidth each flow consumes tends to
be more dynamic in practice.  It is usually pointless to use a snapshot
of bandwidth consumption to come up a "perfect" bond port binding.
For example, TCP flows rate adapt to available bandwidth.  It is
probably better to make smaller changes in each rebalancing step.

Is it still possible to address the issue mentioned while maintaining
current interface? Can we find some way to break the tie in case current
algorithm fail to nominate any bucket to move?

Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/6] Fix build breakage due to uninitalized data

[Andy Zhou]

On Mon, Jul 31, 2017 at 1:02 PM, Ben Pfaff <b...@ovn.org> wrote:
> One of the builds on travis (using jemalloc), as well as all of the builds
> on my own local system, get failures in a couple of tests due to
> uninitialized data seen by the odp_mask_attr_is_wildcard() function.  This
> series fixes the problem.
>
> Ben Pfaff (6):
>   odp-util: Fix misleading parameter names.
>   odp-util: More carefully validate attribute length in
> odp_flow_format().
>   odp-util: Rewrite odp_mask_attr_is_exact().
>   odp-util: Drop special case for OVS_KEY_ATTR_TUNNEL for exact mask
> checks.
>   util: New function is_all_byte().
>   odp-util: Make checks for exact or wildcard masks more precise.

Thanks for fixing the travis build and for improve the readability of
those functions.

For the whole series,
Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] flow: Refactor flow_compose() API.

[Andy Zhou]

>
> I think, it's better to use 'dp_packet_delete(packet)' instead of two
> lines above. 'delete' is the better pair for 'new'. 'uninit' is mostly
> for packets allocated statically and initialized by 'dp_packet_init()'.
>
> Additionally this will help to avoid issues if 'dp_packet_new' will
> be able to allocate DPDK memory someday.

You are right. I folded the change in. Thanks for the suggestion.
>
> With this change:
> Acked-by: Ilya Maximets 
>
Thanks for the review. Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-userspace-macros: Fix ethtool with new kernels.

[Andy Zhou]

On Wed, Jul 26, 2017 at 12:49 PM, Joe Stringer <j...@ovn.org> wrote:
> The latest net-next kernels have removed the UFO feature, which results
> in older ethtool reporting the following error:
>
> Cannot get device udp-fragmentation-offload settings: Operation not
> supported
>
> Currently, we rely on no errors being reported, and if there is an error
> then a failure is reported. However, in this case we can safely ignore
> the stderr output. We still check the return code so if something is
> truly fatal, a failure will still be reported; otherwise, we will not
> fail the test due to the above.
>
> Signed-off-by: Joe Stringer <j...@ovn.org>

LGTM.

Acked-by: Andy Zhou <az...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/3] vswitch.xml: Fix L2 balancing mentioning for balance-tcp bond.

[Andy Zhou]

On Tue, Jul 25, 2017 at 11:44 AM, Andy Zhou <az...@ovn.org> wrote:
> Thanks. I have pushed the series to master.
>
> While applying the patches. I noticed a minor issue and sent a follow
> up patch at:
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336231.html
>
> Will consider back porting after this patch is reviewed.

I have back ported the documentation change to branch 2.5. 2.6 and 2.7.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] bond: Adjust bond hash masks

[Andy Zhou]

On Wed, Jul 26, 2017 at 12:06 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
> On 25.07.2017 21:39, Andy Zhou wrote:
>> Commit 42781e77035d (bond: Unify hash functions in hash action and entry
>> lookup.) changed the BM_TCP's hash function, but did not update
>> hash mask fields accordingly.  Found by inspection.
>>
>> CC: Ilya Maximets <i.maxim...@samsung.com>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>> ---
>
> Good catch. I forget about this.
> One minor comment below, but it's not important.
>
> Acked-by: Ilya Maximets <i.maxim...@samsung.com>

Thanks for the review. Pushed.

>> +flow_mask_hash_fields(flow, wc,
>> +  NX_HASH_FIELDS_SYMMETRIC_L3L4_UDP);
>
> Above function call, actually, fits in 79 characters limit and can be
> written in one line.
>
You are right. Thanks. I merged those two lines before pushing.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/3] flow: Add flow_compose_size().

[Andy Zhou]

On Tue, Jul 25, 2017 at 6:02 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
> This allows to compose packets with different real lenghts from
> odp flows i.e. memory will be allocated for requested packet
> size and all required headers like ip->tot_len filled correctly.
>
> Will be used in netdev-dummy to properly handle '--len' option.
>
> Suggested-by: Andy Zhou <az...@ovn.org>
> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
> ---

Thanks for the bug fixes. I have pushed this series to master.

I think the readability can be further improved. I have a follow up
patch at:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336237.html
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] flow: Refactor flow_compose() API.

[Andy Zhou]

Currently, flow_compose_size() is only supposed to be called after
flow_compose(). I find this API to be unintuitive.

Change flow_compose() API to take the 'size' argument, and
returns 'true' if the packet can be created, 'false' otherwise.

This change also improves error detection and reporting when
'size' is unreasonably small.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 lib/flow.c   | 53 ++--
 lib/flow.h   |  3 +--
 lib/netdev-dummy.c   |  7 --
 ofproto/ofproto-dpif-trace.c |  2 +-
 ofproto/ofproto-dpif.c   |  2 +-
 ovn/controller/ofctrl.c  |  2 +-
 tests/test-ovn.c |  2 +-
 7 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/lib/flow.c b/lib/flow.c
index 8da9f3235d0a..39aed51837a6 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -2735,17 +2735,17 @@ flow_compose_l4_csum(struct dp_packet *p, const struct 
flow *flow,
 }
 }
 
-/* Tries to increase the size of packet composed by 'flow_compose' up to
- * 'size' bytes.  Fixes all the required packet headers like ip/udp lengths
- * and l3/l4 checksums. */
-void
-flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size)
+/* Increase the size of packet composed by 'flow_compose_minimal'
+ * up to 'size' bytes.  Fixes all the required packet headers like
+ * ip/udp lengths and l3/l4 checksums.
+ *
+ * 'size' needs to be larger then the current packet size.  */
+static void
+packet_expand(struct dp_packet *p, const struct flow *flow, size_t size)
 {
 size_t extra_size;
 
-if (size <= dp_packet_size(p)) {
-return;
-}
+ovs_assert(size > dp_packet_size(p));
 
 extra_size = size - dp_packet_size(p);
 dp_packet_put_zeros(p, extra_size);
@@ -2754,7 +2754,6 @@ flow_compose_size(struct dp_packet *p, const struct flow 
*flow, size_t size)
 struct eth_header *eth = dp_packet_eth(p);
 
 eth->eth_type = htons(dp_packet_size(p));
-
 } else if (dl_type_is_ip_any(flow->dl_type)) {
 uint32_t pseudo_hdr_csum;
 size_t l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p);
@@ -2789,9 +2788,12 @@ flow_compose_size(struct dp_packet *p, const struct flow 
*flow, size_t size)
  * 'flow'.
  *
  * (This is useful only for testing, obviously, and the packet isn't really
- * valid.  Lots of fields are just zeroed.) */
-void
-flow_compose(struct dp_packet *p, const struct flow *flow)
+ * valid.  Lots of fields are just zeroed.)
+ *
+ * The created packet will have minimal packet length, just large
+ * enough to hold the packet header fields.  */
+static void
+flow_compose_minimal(struct dp_packet *p, const struct flow *flow)
 {
 uint32_t pseudo_hdr_csum;
 size_t l4_len;
@@ -2896,6 +2898,33 @@ flow_compose(struct dp_packet *p, const struct flow 
*flow)
 }
 }
 }
+
+/* Puts into 'p' a Ethernet frame of size 'size' that flow_extract() would
+ * parse as having the given 'flow'.
+ *
+ * When 'size' is zero, 'p' is a minimal size packet that only big enough
+ * to contains all packet headers.
+ *
+ * When 'size' is larger than the minimal packet size, the packet will
+ * be expended to 'size' with the payload set to zero.
+ *
+ * Return 'true' if the packet is successfully created. 'false' otherwise.
+ * Note, when 'size' is set to zero, this function always returns true.  */
+bool
+flow_compose(struct dp_packet *p, const struct flow *flow, size_t size)
+{
+flow_compose_minimal(p, flow);
+
+if (size && size < dp_packet_size(p)) {
+return false;
+}
+
+if (size > dp_packet_size(p)) {
+packet_expand(p, flow, size);
+}
+
+return true;
+}
 
 /* Compressed flow. */
 
diff --git a/lib/flow.h b/lib/flow.h
index 1bbbe410c6d2..0c6069c66c74 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -124,8 +124,7 @@ void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc);
 void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack);
 void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse);
 
-void flow_compose(struct dp_packet *, const struct flow *);
-void flow_compose_size(struct dp_packet *, const struct flow *, size_t size);
+bool flow_compose(struct dp_packet *, const struct flow *, size_t);
 
 bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto,
  uint8_t *nw_frag);
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 83e30b37cbc3..6e110b1c7376 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1478,8 +1478,11 @@ eth_from_flow(const char *s, size_t packet_size)
 }
 
 packet = dp_packet_new(0);
-flow_compose(packet, );
-flow_compose_size(packet, , packet_size);
+if (!flow_compose(packet, , packet_size)) {
+dp_packet_uninit(packet);
+free(packet);
+packet = NULL;
+};
 
 ofpbuf_uninit(_key);
 return packet;
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c

Re: [ovs-dev] [PATCH v2 1/3] vswitch.xml: Fix L2 balancing mentioning for balance-tcp bond.

[Andy Zhou]

Thanks. I have pushed the series to master.

While applying the patches. I noticed a minor issue and sent a follow
up patch at:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336231.html

Will consider back porting after this patch is reviewed.

On Tue, Jul 25, 2017 at 3:46 AM, Ilya Maximets  wrote:
> L2 fields are not used in userspace hash action since
> commit 4f150744921f ("dpif-netdev: Use miniflow as a flow key.").
> In kernel datapath RSS (which is not include L2 by default for
> most of the NICs) was used from the beginning. This means that
> if recirculation is in use, L2 fields are not used for flow
> balancing.
>
> Fix the documentation accordingly.
>
> Signed-off-by: Ilya Maximets 
> ---
>
> I think, this should be applied to some stable branches too.
>
>  vswitchd/vswitch.xml | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 883ecd8..074535b 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1569,9 +1569,8 @@
>
>  balance-tcp
>  
> -  Balances flows among slaves based on L2, L3, and L4 protocol
> -  information such as destination MAC address, IP address, and TCP
> -  port.
> +  Balances flows among slaves based on L3 and L4 protocol information
> +  such as IP addresses and TCP/UDP ports.
>  
>
>
> --
> 2.7.4
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] bond: Adjust bond hash masks

[Andy Zhou]

Commit 42781e77035d (bond: Unify hash functions in hash action and entry
lookup.) changed the BM_TCP's hash function, but did not update
hash mask fields accordingly.  Found by inspection.

CC: Ilya Maximets <i.maxim...@samsung.com>
Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/bond.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ofproto/bond.c b/ofproto/bond.c
index e09136efbd98..7d8d6560c690 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1798,11 +1798,12 @@ choose_output_slave(const struct bond *bond, const 
struct flow *flow,
 return NULL;
 }
 if (wc) {
-flow_mask_hash_fields(flow, wc, NX_HASH_FIELDS_SYMMETRIC_L4);
+flow_mask_hash_fields(flow, wc,
+  NX_HASH_FIELDS_SYMMETRIC_L3L4_UDP);
 }
 /* Fall Through. */
 case BM_SLB:
-if (wc) {
+if (wc && balance == BM_SLB) {
 flow_mask_hash_fields(flow, wc, NX_HASH_FIELDS_ETH_SRC);
 }
 e = lookup_bond_entry(bond, flow, vlan);
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action and entry lookup.

[Andy Zhou]

On Sat, Jul 22, 2017 at 2:02 PM, Darrell Ball <db...@vmware.com> wrote:
>
>
> -Original Message-
> From: <ovs-dev-boun...@openvswitch.org> on behalf of Andy Zhou <az...@ovn.org>
> Date: Friday, July 21, 2017 at 2:17 PM
> To: "<d...@openvswitch.org>" <d...@openvswitch.org>
> Subject: [ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action 
>   and entry lookup.
>
> Add dev mailing list. It got dropped by accident.
>
>
> -- Forwarded message --
> From: Andy Zhou <az...@ovn.org>
> Date: Fri, Jul 21, 2017 at 2:14 PM
> Subject: Re: [PATCH] bond: Unify hash functions in hash action and entry 
> lookup.
> To: Ilya Maximets <i.maxim...@samsung.com>
>
>
> As it turns out, we can go even further:
>
> Notice that lookup_bond_entry() is only called with the code path of 
> BM_SLB.
> and bond_hash() is only called by lookup_bond_entry().
>
> I think we can just absorb the logic of lookup_bond_entry() into
>     choose_output_slave()
> and remove bond_hash() all together.  What do you think?
>
>
> On Fri, Jul 21, 2017 at 1:06 PM, Andy Zhou <az...@ovn.org> wrote:
> > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maxim...@samsung.com> 
> wrote:
> >> 'lookup_bond_entry' currently uses 'flow_hash_symmetric_l4' while
> >> OVS_ACTION_ATTR_HASH uses 'flow_hash_5tuple'. This may lead to
> >> inconsistency in slave choosing for the new flows.  In general,
> >> there is no point to unify hash functions, because it's not
> >> required for correct work, but it's logically wrong to use
> >> different hash functions there.
> >>
> >> Unfortunately we're not able to use RSS hash here, because we have
> >> no packet at this point, but we may reduce inconsistency by using
> >> 'flow_hash_5tuple' instead of 'flow_hash_symmetric_l4' because
> >> symmetric quality is not needed.
> >>
> >> 'flow_hash_symmetric_l4' was used previously just because there
> >> was no other implemented hash function at the moment. Now we
> >> have 5tuple hash and may replace the old function.
>
> [Darrell]
>
> What other load balance option is available to do load balancing of L2 
> packets (non-IP)
> ‘at the same time’ as IPv4/6 packets for bonds ?
> Unless there is another, I am not sure giving up the load balancing of L2 
> packets is desirable.
> There would be a loss of feature functionality with this patch.

I agree with Llya's comment on this. When recirc is in use. this
hashing value only affect
the first packet. I would not consider this as loss of feature.
>
> A bond at a gateway (one of the most common use cases) could handle many CFM
> sessions, for example and dropping L2 fields from the hash sends all L2 
> packets to a
> single interface of a bond (single point of failure).
> The algorithm flow_hash_symmetric_l4 includes L2 fields (macs and vlans)
> in addition to IPv4/6 and L4 fields, which means it can load balance L2 
> packets (eg CFM)
> in addition to IPv4/6 packets.

CFM is usually used for detect tunnel connectivity issues, thus it is
usually sent within a
tunneled packet. The most popular tunnels are UDP based, we should get
a fair distruction
with a 5 tuple hash.
>
> We have documented that L2 load balancing is included in balance-tcp, which 
> at the very
> least would need to change, assuming we thought such a change had more 
> advantages than disadvantages.
>
> http://openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.pdf
>
> “The following modes require the upstream switch to support 802.3ad with 
> successful LACP negotiation. If
> LACP negotiation fails and other-config:lacp-fallback-ab is true, then 
> active−backup mode is used:
>
>balance−tcp
> Balances flows among slaves based on L2, L3, and L4 
> protocol information such as destination
> MAC address, IP address, and TCP port.”
>
I agree that documentation needs update.

> What is the overall time cost savings in the scope of the whole code pipeline 
> for flow creation, not
> just the hash function itself (as mentioned in the commit message) ?
> This is not a per packet cost; it is per flow cost. Since this patch removes 
> feature content in lieu of
> some performance gain, I think it might be good to have some pipeline 
> performance measurements to
> make a decision whether it is worth it.
>
>
> >>
> >> 'flow_hash_5tuple' is preferable solution because it in 2 - 8 times
> >> (de

Re: [ovs-dev] [PATCH 1/3] flow: Add packet_size option to flow_compose.

[Andy Zhou]

On Mon, Jul 24, 2017 at 6:33 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
> On 22.07.2017 01:38, Andy Zhou wrote:
>> On Wed, Jul 19, 2017 at 7:51 AM, Ilya Maximets <i.maxim...@samsung.com> 
>> wrote:
>>> This allows to compose packets with different real lenghts from
>>> odp flows i.e. memory will be allocated for requested packet
>>> size and all required headers like ip->tot_len filled correctly.
>>>
>>> Will be used in netdev-dummy to properly handle '--len' option.
>>>
>>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>>
>> Thank you for working on this.  Although those functions are mainly
>> for testing, it is still good that we improve them.
>>
>> I am wondering about a slightly different approach. Instead of adding
>> 'packet_size' to the flow_compose() interface, would it make
>> sense to come up with a new function whose task is to
>> expand a packet to the final length, (similar to flow_compose_l4_csum())
>>
>> We would first create the necessary headers for all layers based on
>> flow, without fill in the actual size related field or compute checksums.
>>
>> Then the fix size function will take over, fill in data, and
>> update various headers.
>>
>> Then checksums can be computed and filled in.
>>
>> I think the logics will be easier to follow with this approach. What
>> do you think?
>
>
> I thought about this. I just tried to avoid double packet parsing,
> but such approach could be interesting.

This approach looks fine to me.
>
> Below is the possible implementation.
> If you think that it's better than the modification of flow_compose(),
> I can send v2 with below implementation:

I don't think that eth_from_flow() adds much value. Would it
be less clear if we just use flow_compose_xxx() APIs ?

Please go ahead with V2. Looking forward to it.

>
> --8<--->8--
> diff --git a/lib/flow.c b/lib/flow.c
> index e1597fa..ce99c06 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -2706,40 +2706,87 @@ flow_compose_l4_csum(struct dp_packet *p, const 
> struct flow *flow,
>  if (flow->nw_proto == IPPROTO_TCP) {
>  struct tcp_header *tcp = dp_packet_l4(p);
>
> -/* Checksum has already been zeroed by put_zeros call in
> - * flow_compose_l4(). */
> +tcp->tcp_csum = 0;
>  tcp->tcp_csum = csum_finish(csum_continue(pseudo_hdr_csum,
>tcp, l4_len));
>  } else if (flow->nw_proto == IPPROTO_UDP) {
>  struct udp_header *udp = dp_packet_l4(p);
>
> -/* Checksum has already been zeroed by put_zeros call in
> - * flow_compose_l4(). */
> +udp->udp_csum = 0;
>  udp->udp_csum = csum_finish(csum_continue(pseudo_hdr_csum,
>udp, l4_len));
>  } else if (flow->nw_proto == IPPROTO_ICMP) {
>  struct icmp_header *icmp = dp_packet_l4(p);
>
> -/* Checksum has already been zeroed by put_zeros call in
> - * flow_compose_l4(). */
> +icmp->icmp_csum = 0;
>  icmp->icmp_csum = csum(icmp, l4_len);
>  } else if (flow->nw_proto == IPPROTO_IGMP) {
>  struct igmp_header *igmp = dp_packet_l4(p);
>
> -/* Checksum has already been zeroed by put_zeros call in
> - * flow_compose_l4(). */
> +igmp->igmp_csum = 0;
>  igmp->igmp_csum = csum(igmp, l4_len);
>  } else if (flow->nw_proto == IPPROTO_ICMPV6) {
>  struct icmp6_hdr *icmp = dp_packet_l4(p);
>
> -/* Checksum has already been zeroed by put_zeros call in
> - * flow_compose_l4(). */
> +icmp->icmp6_cksum = 0;
>  icmp->icmp6_cksum = (OVS_FORCE uint16_t)
>  csum_finish(csum_continue(pseudo_hdr_csum, icmp, l4_len));
>  }
>  }
>  }
>
> +/* Tries to increase the size of packet composed by 'flow_compose' up to
> + * 'size' bytes.  Fixes all the required packet headers like ip/udp lengths
> + * and l3/l4 checksums. */
> +void
> +flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size)
> +{
> +size_t extra_size, l4_len;
> +uint32_t pseudo_hdr_csum;
> +
> +if (size <= dp_packet_size(p)) {
> +return;
> +}
> +
> +extra_size = size - dp_packet_size(p);
> +dp_packet_put_zeros(p, extra_size);
> +
> +l4_len = (char *) d

[ovs-dev] [clone optmization v2 7/7] xlate: Emit datapath clone only when necessary.

[Andy Zhou]

Currently the open flow 'clone' action is always translated into
datapath clone. While this is valid translation, the datapath
'clone' action is more expensive and has more restrictions than
not using them.

This patch optimizing the open flow 'clone' translation. Whenever
the open flow actions within the 'clone' is reversible, i.e.
any datapath actions that modifies a packet can be reversed
by using another datapath action. Reversible actions can be
translated without emitting datapath clone.

This patch combines xlate_clone() and compose_clone() into
a single compose_clone() API, since the layering boundary is not
obvious.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 169 ---
 tests/ofproto-dpif.at|  39 +-
 2 files changed, 164 insertions(+), 44 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9b7a6a4f7620..66df4693dd3e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5245,49 +5245,83 @@ xlate_sample_action(struct xlate_ctx *ctx,
   tunnel_out_port, false);
 }
 
-/* For datapath that does not support 'clone' action, but have a suitable
- *  sample action implementation for clone. The upstream Linux kernel
- *  version 4.11 or greater, and kernel module from OVS version 2.8 or
- *  greater version have suitable sample action implementations.  */
-static void
-compose_clone(struct xlate_ctx *ctx, struct flow *old_base_flow,
-  const struct ofpact_nest *oc)
+/* Determine if an datapath action translated from the openflow action
+ * can be reversed by another datapath action.
+ *
+ * Openflow actions that do not emit datapath actions are trivially
+ * reversible. Reversiblity of other actions depends on nature of
+ * action and their translation.  */
+static bool
+reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len)
 {
-size_t offset, ac_offset;
+const struct ofpact *a;
 
-if (ctx->xbridge->support.clone) {
-/* Use clone action as datapath clone. */
-offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
-ctx->base_flow = *old_base_flow;
-} else if (ctx->xbridge->support.sample_nesting > 3) {
-/* Use sample action as datapath clone. */
-offset = nl_msg_start_nested(ctx->odp_actions,
- OVS_ACTION_ATTR_SAMPLE);
-ac_offset = nl_msg_start_nested(ctx->odp_actions,
-OVS_SAMPLE_ATTR_ACTIONS);
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
-nl_msg_cancel_nested(ctx->odp_actions, offset);
-} else {
-nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
-   UINT32_MAX); /* 100% probability. */
-nl_msg_end_nested(ctx->odp_actions, offset);
+OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+switch (a->type) {
+case OFPACT_BUNDLE:
+case OFPACT_CLEAR_ACTIONS:
+case OFPACT_CLONE:
+case OFPACT_CONJUNCTION:
+case OFPACT_CONTROLLER:
+case OFPACT_CT_CLEAR:
+case OFPACT_DEBUG_RECIRC:
+case OFPACT_DEC_MPLS_TTL:
+case OFPACT_DEC_TTL:
+case OFPACT_ENQUEUE:
+case OFPACT_EXIT:
+case OFPACT_FIN_TIMEOUT:
+case OFPACT_GOTO_TABLE:
+case OFPACT_GROUP:
+case OFPACT_LEARN:
+case OFPACT_MULTIPATH:
+case OFPACT_NOTE:
+case OFPACT_OUTPUT:
+case OFPACT_OUTPUT_REG:
+case OFPACT_POP_MPLS:
+case OFPACT_POP_QUEUE:
+case OFPACT_PUSH_MPLS:
+case OFPACT_PUSH_VLAN:
+case OFPACT_REG_MOVE:
+case OFPACT_RESUBMIT:
+case OFPACT_SAMPLE:
+case OFPACT_SET_ETH_DST:
+case OFPACT_SET_ETH_SRC:
+case OFPACT_SET_FIELD:
+case OFPACT_SET_IP_DSCP:
+case OFPACT_SET_IP_ECN:
+case OFPACT_SET_IP_TTL:
+case OFPACT_SET_IPV4_DST:
+case OFPACT_SET_IPV4_SRC:
+case OFPACT_SET_L4_DST_PORT:
+case OFPACT_SET_L4_SRC_PORT:
+case OFPACT_SET_MPLS_LABEL:
+case OFPACT_SET_MPLS_TC:
+case OFPACT_SET_MPLS_TTL:
+case OFPACT_SET_QUEUE:
+case OFPACT_SET_TUNNEL:
+case OFPACT_SET_VLAN_PCP:
+case OFPACT_SET_VLAN_VID:
+case OFPACT_STACK_POP:
+case OFPACT_STACK_PUSH:
+case OFPACT_STRIP_VLAN:
+case OFPACT_UNROLL_XLATE:
+case OFPACT_WRITE_ACTIONS:
+case OFPACT_WRITE_METADATA:
+break;
+
+case OFPACT_CT:
+case OFPACT_METER:
+case OFPACT_NA

[ovs-dev] [clone optmization v2 6/7] xlate: Refactor compose_clone() API

[Andy Zhou]

Create a new function that hides the details of netlink encoding
for the translated clone action.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 76 +++-
 tests/ofproto-dpif.at|  7 
 2 files changed, 39 insertions(+), 44 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a03c27e5b860..9b7a6a4f7620 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5245,35 +5245,39 @@ xlate_sample_action(struct xlate_ctx *ctx,
   tunnel_out_port, false);
 }
 
-/* Use datapath 'clone' or sample to enclose the translation of 'oc'.   */
+/* For datapath that does not support 'clone' action, but have a suitable
+ *  sample action implementation for clone. The upstream Linux kernel
+ *  version 4.11 or greater, and kernel module from OVS version 2.8 or
+ *  greater version have suitable sample action implementations.  */
 static void
-compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
+compose_clone(struct xlate_ctx *ctx, struct flow *old_base_flow,
+  const struct ofpact_nest *oc)
 {
-size_t clone_offset = nl_msg_start_nested(ctx->odp_actions,
-  OVS_ACTION_ATTR_CLONE);
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-nl_msg_end_non_empty_nested(ctx->odp_actions, clone_offset);
-}
-
-/* Use datapath 'sample' action to translate clone.  */
-static void
-compose_clone_action_using_sample(struct xlate_ctx *ctx,
-  const struct ofpact_nest *oc)
-{
-size_t offset = nl_msg_start_nested(ctx->odp_actions,
-OVS_ACTION_ATTR_SAMPLE);
-
-size_t ac_offset = nl_msg_start_nested(ctx->odp_actions,
-   OVS_SAMPLE_ATTR_ACTIONS);
-
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+size_t offset, ac_offset;
 
-if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
-nl_msg_cancel_nested(ctx->odp_actions, offset);
-} else {
-nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
-   UINT32_MAX); /* 100% probability. */
-nl_msg_end_nested(ctx->odp_actions, offset);
+if (ctx->xbridge->support.clone) {
+/* Use clone action as datapath clone. */
+offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
+do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
+ctx->base_flow = *old_base_flow;
+} else if (ctx->xbridge->support.sample_nesting > 3) {
+/* Use sample action as datapath clone. */
+offset = nl_msg_start_nested(ctx->odp_actions,
+ OVS_ACTION_ATTR_SAMPLE);
+ac_offset = nl_msg_start_nested(ctx->odp_actions,
+OVS_SAMPLE_ATTR_ACTIONS);
+do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
+nl_msg_cancel_nested(ctx->odp_actions, offset);
+} else {
+nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
+   UINT32_MAX); /* 100% probability. */
+nl_msg_end_nested(ctx->odp_actions, offset);
+}
+ctx->base_flow = *old_base_flow;
+} else {  /* Use actions. */
+do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
 }
 }
 
@@ -5294,27 +5298,11 @@ xlate_clone(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub);
 ofpbuf_put(>action_set, old_action_set.data, old_action_set.size);
 
-/* Datapath clone action will make sure the pre clone packets
- * are used for actions after clone. Save and restore
- * ctx->base_flow to reflect this for the openflow pipeline. */
-if (ctx->xbridge->support.clone) {
-struct flow old_base_flow = ctx->base_flow;
-compose_clone_action(ctx, oc);
-ctx->base_flow = old_base_flow;
-} else if (ctx->xbridge->support.sample_nesting > 3) {
-/* Avoid generate sample action if datapath
- * only allow small number of nesting. Deeper nesting
- * can cause the datapath to reject the generated flow.  */
-struct flow old_base_flow = ctx->base_flow;
-compose_clone_action_using_sample(ctx, oc);
-ctx->base_flow = old_base_flow;
-} else {
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-}
+struct flow old_base = ctx->base_flow;
+compose_clone(ctx, _base, oc);
 
 ofpbuf_uninit(

[ovs-dev] [clone optmization v2 5/7] ofproto-dpif: Remove ofprto/tnl-push-pop command.

[Andy Zhou]

Use dpif/set-dp-features command instead.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif.c  | 34 --
 tests/ofproto-macros.at |  5 ++---
 2 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 59212dd3cf8e..6ea6837b4360 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5478,37 +5478,6 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn 
*conn,
 }
 
 static void
-ofproto_revalidate_all_backers(void)
-{
-const struct shash_node **backers;
-int i;
-
-backers = shash_sort(_dpif_backers);
-for (i = 0; i < shash_count(_dpif_backers); i++) {
-struct dpif_backer *backer = backers[i]->data;
-backer->need_revalidate = REV_RECONFIGURE;
-}
-free(backers);
-}
-
-static void
-disable_tnl_push_pop(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED,
- const char *argv[], void *aux OVS_UNUSED)
-{
-if (!strcasecmp(argv[1], "off")) {
-ofproto_use_tnl_push_pop = false;
-unixctl_command_reply(conn, "Tunnel push-pop off");
-ofproto_revalidate_all_backers();
-} else if (!strcasecmp(argv[1], "on")) {
-ofproto_use_tnl_push_pop = true;
-unixctl_command_reply(conn, "Tunnel push-pop on");
-ofproto_revalidate_all_backers();
-} else {
-unixctl_command_reply_error(conn, "Invalid argument");
-}
-}
-
-static void
 ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
   int argc, const char *argv[],
   void *aux OVS_UNUSED)
@@ -5582,9 +5551,6 @@ ofproto_unixctl_init(void)
  ofproto_unixctl_dpif_dump_flows, NULL);
 unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
  ofproto_unixctl_dpif_set_dp_features, NULL);
-
-unixctl_command_register("ofproto/tnl-push-pop", "[on]|[off]", 1, 1,
- disable_tnl_push_pop, NULL);
 }
 
 static odp_port_t
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 23b032158f50..bbb1c3bee2a3 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -457,9 +457,8 @@ m4_define([OVS_VSWITCHD_STOP],
OVS_APP_EXIT_AND_WAIT([ovsdb-server])])
 
 m4_define([OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP],
-  [AT_CHECK([ovs-appctl ofproto/tnl-push-pop off], [0], [dnl
-Tunnel push-pop off
-])])
+  [AT_CHECK([ovs-appctl dpif/set-dp-features br0 tnl_push_pop false], [0])
+])
 
 # WAIT_FOR_DUMMY_PORTS(NETDEV_DUMMY_PORT[, NETDEV_DUMMY_PORT...])
 #
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [clone optmization v2 4/7] ofproto-dpif: Remove dpif/disable-truncate command.

[Andy Zhou]

Use 'dpif/set-dp-features' command instead.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif.c  | 21 -
 tests/system-traffic.at |  8 ++--
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 134fa6d82c09..59212dd3cf8e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5509,24 +5509,6 @@ disable_tnl_push_pop(struct unixctl_conn *conn 
OVS_UNUSED, int argc OVS_UNUSED,
 }
 
 static void
-disable_datapath_truncate(struct unixctl_conn *conn OVS_UNUSED,
-  int argc OVS_UNUSED,
-  const char *argv[] OVS_UNUSED,
-  void *aux OVS_UNUSED)
-{
-const struct shash_node **backers;
-int i;
-
-backers = shash_sort(_dpif_backers);
-for (i = 0; i < shash_count(_dpif_backers); i++) {
-struct dpif_backer *backer = backers[i]->data;
-backer->rt_support.trunc = false;
-}
-free(backers);
-unixctl_command_reply(conn, "Datapath truncate action diabled");
-}
-
-static void
 ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
   int argc, const char *argv[],
   void *aux OVS_UNUSED)
@@ -5603,9 +5585,6 @@ ofproto_unixctl_init(void)
 
 unixctl_command_register("ofproto/tnl-push-pop", "[on]|[off]", 1, 1,
  disable_tnl_push_pop, NULL);
-
-unixctl_command_register("dpif/disable-truncate", "", 0, 0,
- disable_datapath_truncate, NULL);
 }
 
 static odp_port_t
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index b2393f5f4241..15e144cb44d2 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -569,9 +569,7 @@ n_bytes=542
 
 dnl SLOW_ACTION: disable kernel datapath truncate support
 dnl Repeat the test above, but exercise the SLOW_ACTION code path
-AT_CHECK([ovs-appctl dpif/disable-truncate], [0],
-[Datapath truncate action diabled
-])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 trunc false], [0])
 
 dnl SLOW_ACTION test1: check datapatch actions
 AT_CHECK([ovs-ofctl del-flows br0])
@@ -692,9 +690,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | 
ofctl_strip], [0], [dnl
 
 dnl SLOW_ACTION: disable datapath truncate support
 dnl Repeat the test above, but exercise the SLOW_ACTION code path
-AT_CHECK([ovs-appctl dpif/disable-truncate], [0],
-[Datapath truncate action diabled
-])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 trunc false], [0])
 
 dnl SLOW_ACTION test1: check datapatch actions
 AT_CHECK([ovs-ofctl del-flows br0])
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [clone optmization v2 2/7] ofproto-dpif: Add boottime support field.

[Andy Zhou]

When changing support fields, it may be unsafe to set support level
beyond what datapath can support.

This patch introduce the notion of boot time support and
runtime support fields. Boot time support are set only
once during ofproto start up phase, and not changed during
runtime. The runtime support fields are the same as boot time
support fields at the startup time, but can be changed via
the 'ovs-appctl' command.  However, each change will
be checked against the corresponding boot time support field. Only
feature reduction from the boot time support is allowed.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/bond.c|   2 +-
 ofproto/ofproto-dpif-upcall.c |   6 +-
 ofproto/ofproto-dpif-xlate.c  |  12 
 ofproto/ofproto-dpif-xlate.h  |   2 +
 ofproto/ofproto-dpif.c| 135 +++---
 ofproto/ofproto-dpif.h|   9 ++-
 6 files changed, 113 insertions(+), 53 deletions(-)

diff --git a/ofproto/bond.c b/ofproto/bond.c
index cb25a1df7369..65cbf7ed6200 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1163,7 +1163,7 @@ bond_rebalance(struct bond *bond)
 }
 bond->next_rebalance = time_msec() + bond->rebalance_interval;
 
-use_recirc = bond->ofproto->backer->support.odp.recirc &&
+use_recirc = bond->ofproto->backer->rt_support.odp.recirc &&
  bond_may_recirc(bond);
 
 if (use_recirc) {
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b2f9d91d2d9c..c5623d1d37ac 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -548,7 +548,7 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers,
 "handler", udpif_upcall_handler, handler);
 }
 
-enable_ufid = udpif->backer->support.ufid;
+enable_ufid = udpif->backer->rt_support.ufid;
 atomic_init(>enable_ufid, enable_ufid);
 dpif_enable_upcall(udpif->dpif);
 
@@ -707,7 +707,7 @@ udpif_use_ufid(struct udpif *udpif)
 bool enable;
 
 atomic_read_relaxed(_ufid, );
-return enable && udpif->backer->support.ufid;
+return enable && udpif->backer->rt_support.ufid;
 }
 
 
@@ -1543,7 +1543,7 @@ ukey_create_from_upcall(struct upcall *upcall, struct 
flow_wildcards *wc)
 .mask = wc ? >masks : NULL,
 };
 
-odp_parms.support = upcall->ofproto->backer->support.odp;
+odp_parms.support = upcall->ofproto->backer->rt_support.odp;
 if (upcall->key_len) {
 ofpbuf_use_const(, upcall->key, upcall->key_len);
 } else {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7f7adb280eaf..44074b37320c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7002,3 +7002,15 @@ xlate_disable_dp_clone(const struct ofproto_dpif 
*ofproto)
 xbridge->support.clone = false;
 }
 }
+
+void
+xlate_set_support(const struct ofproto_dpif *ofproto,
+const struct dpif_backer_support *support)
+{
+struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, );
+struct xbridge *xbridge = xbridge_lookup(xcfg, ofproto);
+
+if (xbridge) {
+xbridge->support = *support;
+}
+}
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 3de7dec8765d..916a15c67b5b 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -234,6 +234,8 @@ void xlate_mac_learning_update(const struct ofproto_dpif 
*ofproto,
int vlan, bool is_grat_arp);
 
 void xlate_disable_dp_clone(const struct ofproto_dpif *);
+void xlate_set_support(const struct ofproto_dpif *,
+   const struct dpif_backer_support *);
 
 void xlate_txn_start(void);
 void xlate_txn_commit(void);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 28d1eb5c648d..ddf0d643bf7b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -446,7 +446,7 @@ type_run(const char *type)
   ofproto->netflow,
   ofproto->up.forward_bpdu,
   connmgr_has_in_band(ofproto->up.connmgr),
-  >backer->support);
+  >backer->rt_support);
 
 HMAP_FOR_EACH (bundle, hmap_node, >bundles) {
 xlate_bundle_set(ofproto, bundle, bundle->name,
@@ -786,7 +786,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
**backerp)
 /* This check fails if performed before udpif threads have been set,
  * as the kernel module checks that the 'pid' in userspace action
  * is non-zero. */
-backer->support.variable_length_userdata
+backer->rt_support.variable_length_userdata
 = check_variable_length_userdata(backer);
 backer->dp_version_string = dpif_get_dp_versio

[ovs-dev] [clone optmization v2 3/7] ofproto-dpif: Remove dpif/disable-dp-clone command.

[Andy Zhou]

Use 'dpif/set-dp-features' command instead.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 11 ---
 ofproto/ofproto-dpif-xlate.h |  1 -
 ofproto/ofproto-dpif.c   | 24 
 tests/ofproto-dpif.at|  4 +---
 4 files changed, 1 insertion(+), 39 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 44074b37320c..a03c27e5b860 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6993,17 +6993,6 @@ xlate_mac_learning_update(const struct ofproto_dpif 
*ofproto,
 }
 
 void
-xlate_disable_dp_clone(const struct ofproto_dpif *ofproto)
-{
-struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, );
-struct xbridge *xbridge = xbridge_lookup(xcfg, ofproto);
-
-if (xbridge) {
-xbridge->support.clone = false;
-}
-}
-
-void
 xlate_set_support(const struct ofproto_dpif *ofproto,
 const struct dpif_backer_support *support)
 {
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 916a15c67b5b..a299d109f368 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -233,7 +233,6 @@ void xlate_mac_learning_update(const struct ofproto_dpif 
*ofproto,
ofp_port_t in_port, struct eth_addr dl_src,
int vlan, bool is_grat_arp);
 
-void xlate_disable_dp_clone(const struct ofproto_dpif *);
 void xlate_set_support(const struct ofproto_dpif *,
const struct dpif_backer_support *);
 
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ddf0d643bf7b..134fa6d82c09 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5527,27 +5527,6 @@ disable_datapath_truncate(struct unixctl_conn *conn 
OVS_UNUSED,
 }
 
 static void
-disable_datapath_clone(struct unixctl_conn *conn OVS_UNUSED,
-   int argc, const char *argv[],
-   void *aux OVS_UNUSED)
-{
-struct ds ds = DS_EMPTY_INITIALIZER;
-const char *br = argv[argc -1];
-struct ofproto_dpif *ofproto;
-
-ofproto = ofproto_dpif_lookup(br);
-if (!ofproto) {
-unixctl_command_reply_error(conn, "no such bridge");
-return;
-}
-xlate_disable_dp_clone(ofproto);
-udpif_flush(ofproto->backer->udpif);
-ds_put_format(, "Datapath clone action disabled for bridge %s", br);
-unixctl_command_reply(conn, ds_cstr());
-ds_destroy();
-}
-
-static void
 ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
   int argc, const char *argv[],
   void *aux OVS_UNUSED)
@@ -5627,9 +5606,6 @@ ofproto_unixctl_init(void)
 
 unixctl_command_register("dpif/disable-truncate", "", 0, 0,
  disable_datapath_truncate, NULL);
-
-unixctl_command_register("dpif/disable-dp-clone", "bridge", 1, 1,
- disable_datapath_clone, NULL);
 }
 
 static odp_port_t
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 5e1347702aaf..9a08c135b9a5 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6784,9 +6784,7 @@ Datapath actions: 
clone(set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2),clone(set(e
 ])
 
 dnl Test flow xlate openflow clone action without using datapath clone action.
-AT_CHECK([ovs-appctl dpif/disable-dp-clone br0], [0],
-[Datapath clone action disabled for bridge br0
-])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
 
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
 [0], [stdout])
 
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [clone optmization v2 1/7] ofproto-dpif: A new command for changing dpif support fields

[Andy Zhou]

dpif support fields contain various datapath capabilities detected
by ofproto at start up time. Usually those fields are read-only,
not intended to be changed at runtime.

However in practice, when writing tests or running experiments, it
becomes necessary to set those fields to emulate different
datapath feature combinations.

Currently there are several separate commands that can be used
defeature individual support fields. This patch generalize those
implementations, provides a single command to change any support
fields. Later patches will remove those individual defeature
commands.

The new command also allow the support fields to be
changed multiple times. Currently defeature commands does not
allow the support level to be restored.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif.c | 134 +
 1 file changed, 134 insertions(+)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 50f440fd8964..28d1eb5c648d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5143,6 +5143,20 @@ show_dp_feature_size_t(struct ds *ds, const char 
*feature, size_t s)
 ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s);
 }
 
+enum dpif_support_field_type {
+DPIF_SUPPORT_FIELD_bool,
+DPIF_SUPPORT_FIELD_size_t,
+};
+
+struct dpif_support_field {
+void *ptr;
+const char *title;
+enum dpif_support_field_type type;
+};
+
+#define DPIF_SUPPORT_FIELD_INTIALIZER(PTR, TITLE, TYPE) \
+(struct dpif_support_field) {PTR, TITLE, TYPE}
+
 static void
 dpif_show_support(const struct dpif_backer_support *support, struct ds *ds)
 {
@@ -5158,6 +5172,103 @@ dpif_show_support(const struct dpif_backer_support 
*support, struct ds *ds)
 }
 
 static void
+display_support_field(const char *name,
+  const struct dpif_support_field *field,
+  struct ds *ds)
+{
+switch (field->type) {
+case DPIF_SUPPORT_FIELD_bool: {
+bool b = *(bool *)field->ptr;
+ds_put_format(ds, "%s (%s) : %s\n", name,
+  field->title, b ? "true" : "false");
+break;
+}
+case DPIF_SUPPORT_FIELD_size_t:
+ds_put_format(ds, "%s (%s) : %"PRIuSIZE"\n", name,
+  field->title, *(size_t *)field->ptr);
+break;
+default:
+OVS_NOT_REACHED();
+}
+}
+
+static void
+dpif_set_support(struct dpif_backer_support *support,
+ const char *name, const char *value, struct ds *ds)
+{
+struct shash all_fields = SHASH_INITIALIZER(_fields);
+struct dpif_support_field *field;
+struct shash_node *node;
+
+#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \
+{\
+  struct dpif_support_field *f = xmalloc(sizeof *f);\
+  *f = DPIF_SUPPORT_FIELD_INTIALIZER(>NAME,\
+TITLE,  \
+DPIF_SUPPORT_FIELD_##TYPE); \
+  shash_add_once(_fields, #NAME, f);\
+}
+DPIF_SUPPORT_FIELDS;
+#undef DPIF_SUPPORT_FIELD
+
+#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \
+{\
+struct dpif_support_field *f = xmalloc(sizeof *f);\
+*f = DPIF_SUPPORT_FIELD_INTIALIZER(>odp.NAME,\
+  TITLE,  \
+  DPIF_SUPPORT_FIELD_##TYPE); \
+  shash_add_once(_fields, #NAME, f);  \
+}
+ODP_SUPPORT_FIELDS;
+#undef ODP_SUPPORT_FIELD
+
+if (!name) {
+struct shash_node *node;
+
+SHASH_FOR_EACH (node, _fields) {
+display_support_field(node->name, node->data, ds);
+}
+goto done;
+}
+
+node = shash_find(_fields, name);
+if (!node) {
+ds_put_cstr(ds, "Unexpected support field");
+goto done;
+}
+field = node->data;
+
+if (!value) {
+display_support_field(node->name, field, ds);
+goto done;
+}
+
+if (field->type == DPIF_SUPPORT_FIELD_bool) {
+if (strcasecmp(value, "true") == 0) {
+*(bool *)field->ptr = true;
+} else if (strcasecmp(value, "false") == 0) {
+*(bool *)field->ptr = false;
+} else {
+ds_put_cstr(ds, "Boolean value expected");
+}
+} else if (field->type == DPIF_SUPPORT_FIELD_size_t) {
+int v;
+if (str_to_int(value, 10, )) {
+if (v >= 0) {
+*(size_t *)field->ptr = v;
+} else {
+ds_put_format(ds, "Negative number not expected");
+}
+} else {
+ds_put_cstr(ds, "Integer number expected");
+}
+}
+
+done:
+shash_destroy_free_data(_fields);
+}
+
+sta

[ovs-dev] [clone optmization v2 0/7] Clone optimization

[Andy Zhou]

I respined the patches based on review feedback at:

http://mail.openvswitch.org/pipermail/ovs-dev/2017-June/333502.html

Patch 1-5: Implements a generic command that can defeature and restore
   the datapath support level.  They are not directly related
   to the main objective this series, but helps in writing tests
   for later patches.

Patch 6-7: The main patches, especially patch 7.

Compare to the original patch series, This patch series did not
go into using clone for patch port translation. The main reason
is that patch port translation is being worked on as part of
the native tunneling work. I plan to work on it after this series
has been reviewed, and after the dust settles with the native
tunneling patch series.

---
  V1->V2:  Fixed system test failures notice by Joe.

Andy Zhou (7):
  ofproto-dpif: A new command for changing dpif support fields
  ofproto-dpif: Add boottime support field.
  ofproto-dpif: Remove dpif/disable-dp-clone command.
  ofproto-dpif: Remove dpif/disable-truncate command.
  ofproto-dpif: Remove ofprto/tnl-push-pop command.
  xlate: Refactor compose_clone() API
  xlate: Emit datapath clone only when necessary.

 ofproto/bond.c|   2 +-
 ofproto/ofproto-dpif-upcall.c |   6 +-
 ofproto/ofproto-dpif-xlate.c  | 184 +++
 ofproto/ofproto-dpif-xlate.h  |   3 +-
 ofproto/ofproto-dpif.c| 286 --
 ofproto/ofproto-dpif.h|   9 +-
 tests/ofproto-dpif.at |  48 ++-
 tests/ofproto-macros.at   |   5 +-
 tests/system-traffic.at   |   8 +-
 9 files changed, 381 insertions(+), 170 deletions(-)

-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] flow: Add packet_size option to flow_compose.

[Andy Zhou]

On Wed, Jul 19, 2017 at 7:51 AM, Ilya Maximets  wrote:
> This allows to compose packets with different real lenghts from
> odp flows i.e. memory will be allocated for requested packet
> size and all required headers like ip->tot_len filled correctly.
>
> Will be used in netdev-dummy to properly handle '--len' option.
>
> Signed-off-by: Ilya Maximets 

Thank you for working on this.  Although those functions are mainly
for testing, it is still good that we improve them.

I am wondering about a slightly different approach. Instead of adding
'packet_size' to the flow_compose() interface, would it make
sense to come up with a new function whose task is to
expand a packet to the final length, (similar to flow_compose_l4_csum())

We would first create the necessary headers for all layers based on
flow, without fill in the actual size related field or compute checksums.

Then the fix size function will take over, fill in data, and
update various headers.

Then checksums can be computed and filled in.

I think the logics will be easier to follow with this approach. What
do you think?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action and entry lookup.

[Andy Zhou]

Add dev mailing list. It got dropped by accident.


-- Forwarded message --
From: Andy Zhou <az...@ovn.org>
Date: Fri, Jul 21, 2017 at 2:14 PM
Subject: Re: [PATCH] bond: Unify hash functions in hash action and entry lookup.
To: Ilya Maximets <i.maxim...@samsung.com>


As it turns out, we can go even further:

Notice that lookup_bond_entry() is only called with the code path of BM_SLB.
and bond_hash() is only called by lookup_bond_entry().

I think we can just absorb the logic of lookup_bond_entry() into
choose_output_slave()
and remove bond_hash() all together.  What do you think?


On Fri, Jul 21, 2017 at 1:06 PM, Andy Zhou <az...@ovn.org> wrote:
> On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
>> 'lookup_bond_entry' currently uses 'flow_hash_symmetric_l4' while
>> OVS_ACTION_ATTR_HASH uses 'flow_hash_5tuple'. This may lead to
>> inconsistency in slave choosing for the new flows.  In general,
>> there is no point to unify hash functions, because it's not
>> required for correct work, but it's logically wrong to use
>> different hash functions there.
>>
>> Unfortunately we're not able to use RSS hash here, because we have
>> no packet at this point, but we may reduce inconsistency by using
>> 'flow_hash_5tuple' instead of 'flow_hash_symmetric_l4' because
>> symmetric quality is not needed.
>>
>> 'flow_hash_symmetric_l4' was used previously just because there
>> was no other implemented hash function at the moment. Now we
>> have 5tuple hash and may replace the old function.
>>
>> 'flow_hash_5tuple' is preferable solution because it in 2 - 8 times
>> (depending on the flow) faster than symmetric function.
>> So, this change will also speed up handling of the new flows and
>> statistics accounting.
>>
>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>> ---
>>  ofproto/bond.c | 6 ++
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>> index cb25a1d..72b373c 100644
>> --- a/ofproto/bond.c
>> +++ b/ofproto/bond.c
>> @@ -1746,12 +1746,10 @@ static unsigned int
>>  bond_hash_tcp(const struct flow *flow, uint16_t vlan, uint32_t basis)
>>  {
>>  struct flow hash_flow = *flow;
>> +
>>  hash_flow.vlans[0].tci = htons(vlan);
>>
>> -/* The symmetric quality of this hash function is not required, but
>> - * flow_hash_symmetric_l4 already exists, and is sufficient for our
>> - * purposes, so we use it out of convenience. */
>> -return flow_hash_symmetric_l4(_flow, basis);
>> +return flow_hash_5tuple(_flow, basis);
>>  }
>>
>>  static unsigned int
>> --
>> 2.7.4
>>
>
> llya, thanks for the patch. I agree with the patch comments,  but I think
> it can further by remove the bond_hash_tcp() function.
> This should further speed up hashing by avoid copying flow.
>
> What do you think? Would you please consider and evaluate this approach?
>
> While at it, we can probably get rid of bond_hash_src() also. It
> can be a separate patch or fold into this one -- up to you.
>
> Thanks!
>
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 21370b5f9a47..eb965b04cd3a 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -177,8 +177,6 @@ static void bond_choose_active_slave(struct bond *)
>  OVS_REQ_WRLOCK(rwlock);
>  static unsigned int bond_hash_src(const struct eth_addr mac,
>uint16_t vlan, uint32_t basis);
> -static unsigned int bond_hash_tcp(const struct flow *, uint16_t vlan,
> -  uint32_t basis);
>  static struct bond_entry *lookup_bond_entry(const struct bond *,
>  const struct flow *,
>  uint16_t vlan)
> @@ -1742,24 +1740,12 @@ bond_hash_src(const struct eth_addr mac,
> uint16_t vlan, uint32_t basis)
>  }
>
>  static unsigned int
> -bond_hash_tcp(const struct flow *flow, uint16_t vlan, uint32_t basis)
> -{
> -struct flow hash_flow = *flow;
> -hash_flow.vlans[0].tci = htons(vlan);
> -
> -/* The symmetric quality of this hash function is not required, but
> - * flow_hash_symmetric_l4 already exists, and is sufficient for our
> - * purposes, so we use it out of convenience. */
> -return flow_hash_symmetric_l4(_flow, basis);
> -}
> -
> -static unsigned int
>  bond_hash(const struct bond *bond, const struct flow *flow, uint16_t vlan)
>  {
>  ovs_assert(bond->balance == BM_TCP || bond->balance == BM_SLB);
>
>  return (bond->balance == BM_TCP
> -? bond_hash_tcp(flow, vlan, bond->basis)
> +? flow_hash_5tuple(flow, bond->basis)
>  : bond_hash_src(flow->dl_src, vlan, bond->basis));
>  }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] odp-execute: Reuse rss hash in OVS_ACTION_ATTR_HASH.

[Andy Zhou]

> Acked-by: Darrell Ball 

Thanks llya for the patch and Darrell for the review!
Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [clone optmization 6/7] xlate: Refactor compose_clone() API

[Andy Zhou]

Create a new function that hides the details of netlink encoding
for the translated clone action.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 76 +++-
 tests/ofproto-dpif.at|  7 
 2 files changed, 39 insertions(+), 44 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a03c27e5b860..9b7a6a4f7620 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5245,35 +5245,39 @@ xlate_sample_action(struct xlate_ctx *ctx,
   tunnel_out_port, false);
 }
 
-/* Use datapath 'clone' or sample to enclose the translation of 'oc'.   */
+/* For datapath that does not support 'clone' action, but have a suitable
+ *  sample action implementation for clone. The upstream Linux kernel
+ *  version 4.11 or greater, and kernel module from OVS version 2.8 or
+ *  greater version have suitable sample action implementations.  */
 static void
-compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
+compose_clone(struct xlate_ctx *ctx, struct flow *old_base_flow,
+  const struct ofpact_nest *oc)
 {
-size_t clone_offset = nl_msg_start_nested(ctx->odp_actions,
-  OVS_ACTION_ATTR_CLONE);
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-nl_msg_end_non_empty_nested(ctx->odp_actions, clone_offset);
-}
-
-/* Use datapath 'sample' action to translate clone.  */
-static void
-compose_clone_action_using_sample(struct xlate_ctx *ctx,
-  const struct ofpact_nest *oc)
-{
-size_t offset = nl_msg_start_nested(ctx->odp_actions,
-OVS_ACTION_ATTR_SAMPLE);
-
-size_t ac_offset = nl_msg_start_nested(ctx->odp_actions,
-   OVS_SAMPLE_ATTR_ACTIONS);
-
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+size_t offset, ac_offset;
 
-if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
-nl_msg_cancel_nested(ctx->odp_actions, offset);
-} else {
-nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
-   UINT32_MAX); /* 100% probability. */
-nl_msg_end_nested(ctx->odp_actions, offset);
+if (ctx->xbridge->support.clone) {
+/* Use clone action as datapath clone. */
+offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
+do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
+ctx->base_flow = *old_base_flow;
+} else if (ctx->xbridge->support.sample_nesting > 3) {
+/* Use sample action as datapath clone. */
+offset = nl_msg_start_nested(ctx->odp_actions,
+ OVS_ACTION_ATTR_SAMPLE);
+ac_offset = nl_msg_start_nested(ctx->odp_actions,
+OVS_SAMPLE_ATTR_ACTIONS);
+do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
+nl_msg_cancel_nested(ctx->odp_actions, offset);
+} else {
+nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
+   UINT32_MAX); /* 100% probability. */
+nl_msg_end_nested(ctx->odp_actions, offset);
+}
+ctx->base_flow = *old_base_flow;
+} else {  /* Use actions. */
+do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
 }
 }
 
@@ -5294,27 +5298,11 @@ xlate_clone(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub);
 ofpbuf_put(>action_set, old_action_set.data, old_action_set.size);
 
-/* Datapath clone action will make sure the pre clone packets
- * are used for actions after clone. Save and restore
- * ctx->base_flow to reflect this for the openflow pipeline. */
-if (ctx->xbridge->support.clone) {
-struct flow old_base_flow = ctx->base_flow;
-compose_clone_action(ctx, oc);
-ctx->base_flow = old_base_flow;
-} else if (ctx->xbridge->support.sample_nesting > 3) {
-/* Avoid generate sample action if datapath
- * only allow small number of nesting. Deeper nesting
- * can cause the datapath to reject the generated flow.  */
-struct flow old_base_flow = ctx->base_flow;
-compose_clone_action_using_sample(ctx, oc);
-ctx->base_flow = old_base_flow;
-} else {
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-}
+struct flow old_base = ctx->base_flow;
+compose_clone(ctx, _base, oc);
 
 ofpbuf_uninit(

[ovs-dev] [clone optmization 5/7] ofproto-dpif: Remove ofprto/tnl-push-pop command.

[Andy Zhou]

Use dpif/set-dp-features command instead.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif.c  | 34 --
 tests/ofproto-macros.at |  5 ++---
 2 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c4f97edc7ce1..b52a7ea4ca9f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5477,37 +5477,6 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn 
*conn,
 }
 
 static void
-ofproto_revalidate_all_backers(void)
-{
-const struct shash_node **backers;
-int i;
-
-backers = shash_sort(_dpif_backers);
-for (i = 0; i < shash_count(_dpif_backers); i++) {
-struct dpif_backer *backer = backers[i]->data;
-backer->need_revalidate = REV_RECONFIGURE;
-}
-free(backers);
-}
-
-static void
-disable_tnl_push_pop(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED,
- const char *argv[], void *aux OVS_UNUSED)
-{
-if (!strcasecmp(argv[1], "off")) {
-ofproto_use_tnl_push_pop = false;
-unixctl_command_reply(conn, "Tunnel push-pop off");
-ofproto_revalidate_all_backers();
-} else if (!strcasecmp(argv[1], "on")) {
-ofproto_use_tnl_push_pop = true;
-unixctl_command_reply(conn, "Tunnel push-pop on");
-ofproto_revalidate_all_backers();
-} else {
-unixctl_command_reply_error(conn, "Invalid argument");
-}
-}
-
-static void
 ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
   int argc, const char *argv[],
   void *aux OVS_UNUSED)
@@ -5581,9 +5550,6 @@ ofproto_unixctl_init(void)
  ofproto_unixctl_dpif_dump_flows, NULL);
 unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
  ofproto_unixctl_dpif_set_dp_features, NULL);
-
-unixctl_command_register("ofproto/tnl-push-pop", "[on]|[off]", 1, 1,
- disable_tnl_push_pop, NULL);
 }
 
 static odp_port_t
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 23b032158f50..bbb1c3bee2a3 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -457,9 +457,8 @@ m4_define([OVS_VSWITCHD_STOP],
OVS_APP_EXIT_AND_WAIT([ovsdb-server])])
 
 m4_define([OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP],
-  [AT_CHECK([ovs-appctl ofproto/tnl-push-pop off], [0], [dnl
-Tunnel push-pop off
-])])
+  [AT_CHECK([ovs-appctl dpif/set-dp-features br0 tnl_push_pop false], [0])
+])
 
 # WAIT_FOR_DUMMY_PORTS(NETDEV_DUMMY_PORT[, NETDEV_DUMMY_PORT...])
 #
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [clone optmization 4/7] ofproto-dpif: Remove dpif/disable-truncate command.

[Andy Zhou]

Use 'dpif/set-dp-features' command instead.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif.c  | 21 -
 tests/system-traffic.at |  8 ++--
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ff67f18710b7..c4f97edc7ce1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5508,24 +5508,6 @@ disable_tnl_push_pop(struct unixctl_conn *conn 
OVS_UNUSED, int argc OVS_UNUSED,
 }
 
 static void
-disable_datapath_truncate(struct unixctl_conn *conn OVS_UNUSED,
-  int argc OVS_UNUSED,
-  const char *argv[] OVS_UNUSED,
-  void *aux OVS_UNUSED)
-{
-const struct shash_node **backers;
-int i;
-
-backers = shash_sort(_dpif_backers);
-for (i = 0; i < shash_count(_dpif_backers); i++) {
-struct dpif_backer *backer = backers[i]->data;
-backer->rt_support.trunc = false;
-}
-free(backers);
-unixctl_command_reply(conn, "Datapath truncate action diabled");
-}
-
-static void
 ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
   int argc, const char *argv[],
   void *aux OVS_UNUSED)
@@ -5602,9 +5584,6 @@ ofproto_unixctl_init(void)
 
 unixctl_command_register("ofproto/tnl-push-pop", "[on]|[off]", 1, 1,
  disable_tnl_push_pop, NULL);
-
-unixctl_command_register("dpif/disable-truncate", "", 0, 0,
- disable_datapath_truncate, NULL);
 }
 
 static odp_port_t
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index b2393f5f4241..a47b9b78bef6 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -569,9 +569,7 @@ n_bytes=542
 
 dnl SLOW_ACTION: disable kernel datapath truncate support
 dnl Repeat the test above, but exercise the SLOW_ACTION code path
-AT_CHECK([ovs-appctl dpif/disable-truncate], [0],
-[Datapath truncate action diabled
-])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 truncate false], [0])
 
 dnl SLOW_ACTION test1: check datapatch actions
 AT_CHECK([ovs-ofctl del-flows br0])
@@ -692,9 +690,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | 
ofctl_strip], [0], [dnl
 
 dnl SLOW_ACTION: disable datapath truncate support
 dnl Repeat the test above, but exercise the SLOW_ACTION code path
-AT_CHECK([ovs-appctl dpif/disable-truncate], [0],
-[Datapath truncate action diabled
-])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 truncate false], [0])
 
 dnl SLOW_ACTION test1: check datapatch actions
 AT_CHECK([ovs-ofctl del-flows br0])
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [clone optmization 2/7] ofproto-dpif: Add boottime support field.

[Andy Zhou]

When changing support fields, it may be unsafe to set support level
beyond what datapath can support.

This patch introduce the notion of boot time support and
runtime support fields. Boot time support are set only
once during ofproto start up phase, and not changed during
runtime. The runtime support fields are the same as boot time
support fields at the startup time, but can be changed via
the 'ovs-appctl' command.  However, each change will
be checked against the corresponding boot time support field. Only
feature reduction from the boot time support is allowed.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/bond.c|   2 +-
 ofproto/ofproto-dpif-upcall.c |   6 +-
 ofproto/ofproto-dpif-xlate.c  |  12 
 ofproto/ofproto-dpif-xlate.h  |   2 +
 ofproto/ofproto-dpif.c| 135 +++---
 ofproto/ofproto-dpif.h|   9 ++-
 6 files changed, 113 insertions(+), 53 deletions(-)

diff --git a/ofproto/bond.c b/ofproto/bond.c
index cb25a1df7369..65cbf7ed6200 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1163,7 +1163,7 @@ bond_rebalance(struct bond *bond)
 }
 bond->next_rebalance = time_msec() + bond->rebalance_interval;
 
-use_recirc = bond->ofproto->backer->support.odp.recirc &&
+use_recirc = bond->ofproto->backer->rt_support.odp.recirc &&
  bond_may_recirc(bond);
 
 if (use_recirc) {
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b2f9d91d2d9c..c5623d1d37ac 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -548,7 +548,7 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers,
 "handler", udpif_upcall_handler, handler);
 }
 
-enable_ufid = udpif->backer->support.ufid;
+enable_ufid = udpif->backer->rt_support.ufid;
 atomic_init(>enable_ufid, enable_ufid);
 dpif_enable_upcall(udpif->dpif);
 
@@ -707,7 +707,7 @@ udpif_use_ufid(struct udpif *udpif)
 bool enable;
 
 atomic_read_relaxed(_ufid, );
-return enable && udpif->backer->support.ufid;
+return enable && udpif->backer->rt_support.ufid;
 }
 
 
@@ -1543,7 +1543,7 @@ ukey_create_from_upcall(struct upcall *upcall, struct 
flow_wildcards *wc)
 .mask = wc ? >masks : NULL,
 };
 
-odp_parms.support = upcall->ofproto->backer->support.odp;
+odp_parms.support = upcall->ofproto->backer->rt_support.odp;
 if (upcall->key_len) {
 ofpbuf_use_const(, upcall->key, upcall->key_len);
 } else {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7f7adb280eaf..44074b37320c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7002,3 +7002,15 @@ xlate_disable_dp_clone(const struct ofproto_dpif 
*ofproto)
 xbridge->support.clone = false;
 }
 }
+
+void
+xlate_set_support(const struct ofproto_dpif *ofproto,
+const struct dpif_backer_support *support)
+{
+struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, );
+struct xbridge *xbridge = xbridge_lookup(xcfg, ofproto);
+
+if (xbridge) {
+xbridge->support = *support;
+}
+}
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 3de7dec8765d..916a15c67b5b 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -234,6 +234,8 @@ void xlate_mac_learning_update(const struct ofproto_dpif 
*ofproto,
int vlan, bool is_grat_arp);
 
 void xlate_disable_dp_clone(const struct ofproto_dpif *);
+void xlate_set_support(const struct ofproto_dpif *,
+   const struct dpif_backer_support *);
 
 void xlate_txn_start(void);
 void xlate_txn_commit(void);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 264e41c21dd3..3836b21b7aa4 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -446,7 +446,7 @@ type_run(const char *type)
   ofproto->netflow,
   ofproto->up.forward_bpdu,
   connmgr_has_in_band(ofproto->up.connmgr),
-  >backer->support);
+  >backer->rt_support);
 
 HMAP_FOR_EACH (bundle, hmap_node, >bundles) {
 xlate_bundle_set(ofproto, bundle, bundle->name,
@@ -786,7 +786,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
**backerp)
 /* This check fails if performed before udpif threads have been set,
  * as the kernel module checks that the 'pid' in userspace action
  * is non-zero. */
-backer->support.variable_length_userdata
+backer->rt_support.variable_length_userdata
 = check_variable_length_userdata(backer);
 backer->dp_version_string = dpif_get_dp_versio

[ovs-dev] [clone optmization 1/7] ofproto-dpif: A new command for changing dpif support fields

[Andy Zhou]

dpif support fields contain various datapath capabilities detected
by ofproto at start up time. Usually those fields are read-only,
not intended to be changed at runtime.

However in practice, when writing tests or running experiments, it
becomes necessary to set those fields to emulate different
datapath feature combinations.

Currently there are several separate commands that can be used
defeature individual support fields. This patch generalize those
implementations, provides a single command to change any support
fields. Later patches will remove those individual defeature
commands.

The new command also allow the support fields to be
changed multiple times. Currently defeature commands does not
allow the support level to be restored.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif.c | 133 +
 1 file changed, 133 insertions(+)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 50f440fd8964..264e41c21dd3 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5143,6 +5143,20 @@ show_dp_feature_size_t(struct ds *ds, const char 
*feature, size_t s)
 ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s);
 }
 
+enum dpif_support_field_type {
+DPIF_SUPPORT_FIELD_bool,
+DPIF_SUPPORT_FIELD_size_t,
+};
+
+struct dpif_support_field {
+void *ptr;
+const char *title;
+enum dpif_support_field_type type;
+};
+
+#define DPIF_SUPPORT_FIELD_INTIALIZER(PTR, TITLE, TYPE) \
+(struct dpif_support_field) {PTR, TITLE, TYPE}
+
 static void
 dpif_show_support(const struct dpif_backer_support *support, struct ds *ds)
 {
@@ -5158,6 +5172,102 @@ dpif_show_support(const struct dpif_backer_support 
*support, struct ds *ds)
 }
 
 static void
+display_support_field(const char *name,
+  const struct dpif_support_field *field,
+  struct ds *ds)
+{
+switch (field->type) {
+case DPIF_SUPPORT_FIELD_bool: {
+bool b = *(bool *)field->ptr;
+ds_put_format(ds, "%s (%s) : %s\n", name,
+  field->title, b ? "true" : "false");
+break;
+}
+case DPIF_SUPPORT_FIELD_size_t:
+ds_put_format(ds, "%s (%s) : %"PRIuSIZE"\n", name,
+  field->title, *(size_t *)field->ptr);
+break;
+default:
+OVS_NOT_REACHED();
+}
+}
+
+static void
+dpif_set_support(struct dpif_backer_support *support,
+ const char *name, const char *value, struct ds *ds)
+{
+struct shash all_fields = SHASH_INITIALIZER(_fields);
+struct dpif_support_field *field;
+struct shash_node *node;
+
+#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \
+{\
+  struct dpif_support_field *f = xmalloc(sizeof *f);\
+  *f = DPIF_SUPPORT_FIELD_INTIALIZER(>NAME,\
+TITLE,  \
+DPIF_SUPPORT_FIELD_##TYPE); \
+  shash_add_once(_fields, #NAME, f);\
+}
+DPIF_SUPPORT_FIELDS;
+#undef DPIF_SUPPORT_FIELD
+
+#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \
+{\
+struct dpif_support_field *f = xmalloc(sizeof *f);\
+*f = DPIF_SUPPORT_FIELD_INTIALIZER(>odp.NAME,\
+  TITLE,  \
+  DPIF_SUPPORT_FIELD_##TYPE); \
+  shash_add_once(_fields, #NAME, f);  \
+}
+ODP_SUPPORT_FIELDS;
+#undef ODP_SUPPORT_FIELD
+
+if (!name) {
+struct shash_node *node;
+
+SHASH_FOR_EACH (node, _fields) {
+display_support_field(node->name, node->data, ds);
+}
+goto done;
+}
+
+node = shash_find(_fields, name);
+if (!node) {
+ds_put_cstr(ds, "Unexpected support field");
+}
+field = node->data;
+
+if (!value) {
+display_support_field(node->name, field, ds);
+goto done;
+}
+
+if (field->type == DPIF_SUPPORT_FIELD_bool) {
+if (strcasecmp(value, "true") == 0) {
+*(bool *)field->ptr = true;
+} else if (strcasecmp(value, "false") == 0) {
+*(bool *)field->ptr = false;
+} else {
+ds_put_cstr(ds, "Boolean value expected");
+}
+} else if (field->type == DPIF_SUPPORT_FIELD_size_t) {
+int v;
+if (str_to_int(value, 10, )) {
+if (v >= 0) {
+*(size_t *)field->ptr = v;
+} else {
+ds_put_format(ds, "Negative number not expected");
+}
+} else {
+ds_put_cstr(ds, "Integer number expected");
+}
+}
+
+done:
+shash_destroy_free_data(_fields);
+}
+
+static void
 dpif_show_

[ovs-dev] [clone optmization 0/7] Clone optimization

[Andy Zhou]

I respined the patches based on review feedback at:

http://mail.openvswitch.org/pipermail/ovs-dev/2017-June/333502.html

Patch 1-5: Implements a generic command that can defeature and restore
   the datapath support level.  They are not directly related
   to the main objective this series, but helps in writing tests
   for later patches.

Patch 6-7: The main patches, especially patch 7.

Compare to the original patch series, This patch series did not
go into using clone for patch port translation. The main reason
is that patch port translation is being worked on as part of
the native tunneling work. I plan to work on it after this series
has been reviewed, and after the dust settles with the native
tunneling patch series.

Andy Zhou (7):
  ofproto-dpif: A new command for changing dpif support fields
  ofproto-dpif: Add boottime support field.
  ofproto-dpif: Remove dpif/disable-dp-clone command.
  ofproto-dpif: Remove dpif/disable-truncate command.
  ofproto-dpif: Remove ofprto/tnl-push-pop command.
  xlate: Refactor compose_clone() API
  xlate: Emit datapath clone only when necessary.

 ofproto/bond.c|   2 +-
 ofproto/ofproto-dpif-upcall.c |   6 +-
 ofproto/ofproto-dpif-xlate.c  | 184 +++
 ofproto/ofproto-dpif-xlate.h  |   3 +-
 ofproto/ofproto-dpif.c| 285 --
 ofproto/ofproto-dpif.h|   9 +-
 tests/ofproto-dpif.at |  48 ++-
 tests/ofproto-macros.at   |   5 +-
 tests/system-traffic.at   |   8 +-
 9 files changed, 380 insertions(+), 170 deletions(-)

-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVSDB Replication: Clarifications required

[Andy Zhou]

On Sat, Jul 15, 2017 at 10:58 PM, Arunkumar Rg <arunkumar...@gmail.com> wrote:
> Hi Andy Zhou,
>
> Thanks for looking into this!
>
> Please find my replies inline:
>
> On Tue, Jul 11, 2017 at 1:30 AM, Andy Zhou <az...@ovn.org> wrote:
>>
>> On Tue, Jul 4, 2017 at 10:52 PM, Arunkumar Rg <arunkumar...@gmail.com>
>> wrote:
>> > Hi,
>> >
>> > Got few clarifications on OVSDB replication. Please let me know your
>> > inputs
>> > on it.
>> >
>> > 1. From the ovsdb-server code(main_loop()), it seems that the standby
>> > ovsdb-server becomes 'Active' if the JSONRPC session with the
>> > active-ovsdb-server is not alive.
>> > Instead of this behavior, is there a way(probably some CLI option)
>> > wherein
>> > we can instruct the ovsdb-server to not become active even if the
>> > session
>> > to 'active' is not alive??
>>
>> It is technically possible.  What is a good use case for this?
>
> Arun: The use case I'm looking at is - ovsdb-server runs in a VxLAN HW-VTEP.
> Now if we want to provide HW-VTEP redundancy(something like MC-LAG), then
> we'll use
> ovsdb-server replication in these redundant HW-VTEPs i.e ovsdb-server
> running in one of the
> HW-VTEPs will be acting as 'Active' and other HW-VTEP as 'standby'. With
> this, the controller
> will see a single HW-VTEP instead of multiple HW-VTEPs. Now, in this case,
> if the HW-VTEPs
> already has an inherent mechanism to call which HW-VTEP is active and which
> is standby, then I want
> to use that mechansim to dictate ovsdb-server to become active/standby
> accordingly.

In this use case, would you want to switch the back-up HW-VTEP to
backup the new 'active'
server?   Whenever an backup server become active due to the current active
server failure, you can always force it into backup server mode again.
The limitation
is there will be a brief moment that multiple active server will be
running. I am not
sure this is a concern in practice. If this is a concern, what you
proposed is a reasonable
solution. May be you post a patch for this feature.
>
>> >
>> > 2. I understand in the standby-ovsdb-server then no transaction(write
>> > operation) can be done on it's DB. At the same time, I see an option in
>> > appctl to exclude few tables from syncing, whether we can do
>> > transaction(write operation) on those tables in the
>> > standby-ovsdb-server??
>>
>> This may be a good extension to current implementation.
>
> Arun: This would be helpful for the above use case I described. I'm new to
> this ovs-dev.
> If you are aware, please share me on how to put this request to ovs
> community.

Sending email to the 'dev' mailing list is the usual approach I am aware of.

>
>>
>> >
>> > 3. Can the below runtime management commands of ovsdb-server can be done
>> > via API calls(something like IDL APIs)??
>> > ovsdb-server/connect-remote-ovsdb-server
>> > ovsdb-server/disconnect-remote-ovsdb-server
>> > ovsdb-server/set-sync-exclude-tables {db:table,...}
>> > ovsdb-server/get-sync-excluded-tables
>>
>> Current design and implementation are for integration with Linux HA
>> framework. Those
>> extensions are technically possible. Would implementing them improve
>> Linux HA integration
>> better? or for supporting other HA framework?
>
> Arun: Currently I'm trying a prototype of running a ovsdb-server on a
> HW-VTEP.
> For this I have written a small ovsdb-client program using the
> IDL(vtep-idl).
> Now I'm planning to do some prototype for HW-VTEP redundancy(as described in
> my
> above replies) and for this it seems to me using ovsdb-server replication to
> be useful.
> So if IDL has APIs for ovsdb-server replication related operations as well,
> then I can
> use those APIs straight away instead of calling the command lines through
> system calls.

ovs-appctl talks to the ovsdb-server process over a unix domain
socket. If your client
program can open a socket, it should also be able to talk to
ovsdb-server without
using cli.
>
> Thanks,
> Arun.
>>
>> >
>> > Thanks,
>> > Arun.
>> > ___
>> > dev mailing list
>> > d...@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dp-packet: Remove misleading comment for refill init function.

[Andy Zhou]

On Fri, Jul 14, 2017 at 1:11 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
> Function 'dp_packet_batch_refill_init' doesn't return anything.
> Looks like this comment came from one of the intermediate versions
> of the API enhancement patch. Additionally comment style changed
> to be consistent with other comments in the same file.
>
> CC: Andy Zhou <az...@ovn.org>
> Fixes: 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>

Thanks. Applied.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] odp-execute: Reuse rss hash in OVS_ACTION_ATTR_HASH.

[Andy Zhou]

On Thu, Jul 13, 2017 at 8:07 AM, Ilya Maximets  wrote:
> If RSS hash exists in a packet it can be reused instead of
> 5 tuple hash re-calculation in OVS_ACTION_ATTR_HASH. This
> leads to increasing the performance of sending packets to
> the OVS bonding in userspace datapath up to 10-15%.
>
> Additionally fixed unit test 'select group with dp_hash
> selection method' to not depend on dp_hash value.
>
> Signed-off-by: Ilya Maximets 

Thanks for address my concern. I am glad to hear the change did not
impact the performance.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/6] test-conntrack: Fix dead store reported by clang.

[Andy Zhou]

On Tue, Jul 11, 2017 at 9:16 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Fri, Jun 23, 2017 at 09:49:19AM +, Kavanagh, Mark B wrote:
>> >From: ovs-dev-boun...@openvswitch.org 
>> >[mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of
>> >Bhanuprakash Bodireddy
>> >Sent: Monday, June 19, 2017 7:54 PM
>> >To: d...@openvswitch.org
>> >Subject: [ovs-dev] [PATCH 5/6] test-conntrack: Fix dead store reported by 
>> >clang.
>> >
>> >Clang reports that value store to 'batch_size' is never read.
>> >
>> >Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com>
>>
>> Hi Bhanu,
>>
>> LGTM - I also compiled this with gcc, clang, and sparse without issue. 
>> Checkpatch reports no obvious problems either.
>>
>> Acked-by: Mark Kavanagh <mark.b.kavan...@intel.com>
>
> Thanks for noticing the problem.
>
> Even with this change, batch_size is a write-only variable.  Nothing
> ever uses it.
>
> I think that the right fix is something more like this.  I have not
> tested it.
>
> Andy, it looks like you made the previous change here, does this make
> sense?

I think this version is closer to the original intent of the test
interface than what I have changed.
Here is my Ack for this change.  I have a minor comment below.

Acked-by: Andy Zhou <az...@ovn.org>

>
> Thanks,
>
> Ben
>
> --8<--cut here-->8--
>
> From: Ben Pfaff <b...@ovn.org>
> Date: Tue, 11 Jul 2017 21:15:21 -0700
> Subject: [PATCH] test-conntrack: Restore packet batching to pcap test.
>
> The test accepted but then ignored the batch count argument.
>
> Reported-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com>
> CC: Andy Zhou <az...@ovn.org>
> Fixes: 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
>  tests/test-conntrack.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
> index f79a9fcc61e3..6a77db8df5ae 100644
> --- a/tests/test-conntrack.c
> +++ b/tests/test-conntrack.c
> @@ -211,17 +211,22 @@ test_pcap(struct ovs_cmdl_context *ctx)
>
>  conntrack_init();
>  total_count = 0;
> -while (!err) {
> +for (;;) {
>  struct dp_packet *packet;
>  struct dp_packet_batch pkt_batch_;
>  struct dp_packet_batch *batch = _batch_;
>
>  dp_packet_batch_init(batch);
> -err = ovs_pcap_read(pcap, , NULL);
> -if (err) {
> +for (int i = 0; i < batch_size; i++) {
> +err = ovs_pcap_read(pcap, , NULL);
> +if (err) {
> +break;
Would it make sense to use 'continue' here instead of 'break'?
> +}
> +dp_packet_batch_add(batch, packet);
> +}
> +if (!batch->count) {
>  break;
>  }
> -dp_packet_batch_add(batch, packet);
>  pcap_batch_execute_conntrack(, batch);
>
>  DP_PACKET_BATCH_FOR_EACH (packet, batch) {
> --
> 2.10.2
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-execute: Reuse rss hash in OVS_ACTION_ATTR_HASH.

[Andy Zhou]

On Tue, Jul 11, 2017 at 7:30 AM, Ilya Maximets  wrote:
> If RSS hash exists in a packet it can be reused instead of
> 5 tuple hash re-calculation in OVS_ACTION_ATTR_HASH. This
> leads to increasing the performance of sending packets to
> the OVS bonding in userspace datapath up to 10-15%.
>
> Signed-off-by: Ilya Maximets 
> ---
>  lib/odp-execute.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index d656334..471a364 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -646,8 +646,17 @@ odp_execute_actions(void *dp, struct dp_packet_batch 
> *batch, bool steal,
>  uint32_t hash;
>
>  DP_PACKET_BATCH_FOR_EACH (packet, batch) {
> -flow_extract(packet, );
> -hash = flow_hash_5tuple(, hash_act->hash_basis);
> +/* RSS hash can be used here instead of 5tuple for
> + * performance reasons. */
> +if (dp_packet_rss_valid(packet)) {
> +hash = dp_packet_get_rss_hash(packet);

> +if (hash_act->hash_basis) {
> +hash = hash_finish(hash, hash_act->hash_basis);
> +}

This is not a full review. I have some comments on the 3 lines of code above.

Would it make more sense to always include 'hash_basis'?  Also it
seems hash_int() would be more appropriate than hash_finish().

I suppose the performance gain may not be as significant. On the other
hand, I am not sure if we should count on the performance
gain by assuming hash_basis is always zero.

> +} else {
> +flow_extract(packet, );
> +hash = flow_hash_5tuple(, hash_act->hash_basis);
> +}
>  packet->md.dp_hash = hash;
>  }
>  } else {
> --
> 2.7.4
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVSDB Replication: Clarifications required

[Andy Zhou]

On Tue, Jul 4, 2017 at 10:52 PM, Arunkumar Rg  wrote:
> Hi,
>
> Got few clarifications on OVSDB replication. Please let me know your inputs
> on it.
>
> 1. From the ovsdb-server code(main_loop()), it seems that the standby
> ovsdb-server becomes 'Active' if the JSONRPC session with the
> active-ovsdb-server is not alive.
> Instead of this behavior, is there a way(probably some CLI option) wherein
> we can instruct the ovsdb-server to not become active even if the session
> to 'active' is not alive??

It is technically possible.  What is a good use case for this?
>
> 2. I understand in the standby-ovsdb-server then no transaction(write
> operation) can be done on it's DB. At the same time, I see an option in
> appctl to exclude few tables from syncing, whether we can do
> transaction(write operation) on those tables in the standby-ovsdb-server??

This may be a good extension to current implementation.
>
> 3. Can the below runtime management commands of ovsdb-server can be done
> via API calls(something like IDL APIs)??
> ovsdb-server/connect-remote-ovsdb-server
> ovsdb-server/disconnect-remote-ovsdb-server
> ovsdb-server/set-sync-exclude-tables {db:table,...}
> ovsdb-server/get-sync-excluded-tables

Current design and implementation are for integration with Linux HA
framework. Those
extensions are technically possible. Would implementing them improve
Linux HA integration
better? or for supporting other HA framework?
>
> Thanks,
> Arun.
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dp-packet: Copy trunc flag on batch clone.

[Andy Zhou]

On Fri, Jun 30, 2017 at 4:00 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
> Without this applying of the cutlen action will not work
> on copied batch. Cutlen works for linux and dummy netdevs
> only because they tries to apply it per-packet inside
> send function.
>
> Cutlen action doesn't work for dpdk ports in case batch clone
> occured because invoked by the 'dp_packet_batch_apply_cutlen()'.
>
> CC: Andy Zhou <az...@ovn.org>
> Fixes: 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>

Acked-by: Andy Zhou <az...@ovn.org>

Thanks for the fix! Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] xlate: Use datapath clone action for patch port translation

[Andy Zhou]

On Tue, Jun 6, 2017 at 5:01 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Fri, May 26, 2017 at 08:47:45PM -0700, Andy Zhou wrote:
>> When available, use datapath 'clone' for patch port translation.
>> Clone provides a stronger guarantee that packet will be restored
>> after going through a patch port, Even in case the packet is
>> NAT'd by the bridge behind the patch port.
>>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>
> Thanks for working on this.  It is good to improve the correctness of
> the datapath implementation of OpenFlow actions, and this is the weakest
> point in correctness that I currently know about.
>
> This approach seems correct, but expensive in the common case where the
> packet does not need to be restored, since "clone" and "sample" are
> expensive datapath actions: I expect that they are more expensive than a
> few "set field" actions, and certainly more expensive than doing
> nothing.  I think that there are only a few datapath actions that make
> changes that later datapath actions can't restore.  Can the code here
> check whether any of those actions are actually used, and avoid using
> "clone" or "sample" in the common case?

Make sense. I will work on implementing this and repost.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] xlate: Add datapath clone generation API

[Andy Zhou]

On Tue, Jun 6, 2017 at 4:55 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Fri, May 26, 2017 at 08:47:44PM -0700, Andy Zhou wrote:
>> There are three methods of translating openflow layer clone action.
>> Using datapath clone action, datapath sample action or using actions
>> to negating the any changes within a clone.  Xlate logic selects
>> a specific method depends on datapath features detected at the boot time.
>>
>> Currently only xlate_clone() implements the selection logic since it
>> is the solo user, but patches will add more users. Introduce
>>  new APIs that hide the details of generating datapath clone action.
>>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>
> This adds a nice layer of abstraction.  Thanks!

Thanks for the careful review and comments.
>
> I don't think it's necessary to use malloc and free to allocate these
> structures, if the caller provides an instance of struct
> compose_clone_info.  That seems like a better choice, in my opinion.
>
Good point. Malloc is not essential here. Will drop.

> I think that this implementation fails when it chooses
> COMPOSE_CLONE_USING_ACTIONS, because in that case it does not save and
> restore the base flow.
>
> A possible improvement to the implementation would be to keep track of
> the nesting level and fall back from COMPOSE_CLONE_USING_SAMPLE to
> COMPOSE_CLONE_USING_ACTIONS when it exceeds the datapath's capability.
>
If datapath is actually required, then the translation won't be
correct. Should we just abort the translation
and log an error?

> The code might be a little more straightforward without breaking
> compose_clone_method() into a separate function, because then there is
> no need to have a separate switch statement to again discover what
> method to use.  But if you prefer this implementation, I understand that
> too.
It is likely I will drop the compose_clone_method() when the
enhancements required to address
your comment for the next patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/2] xlate: Use datapath clone action for patch port translation

[Andy Zhou]

When available, use datapath 'clone' for patch port translation.
Clone provides a stronger guarantee that packet will be restored
after going through a patch port, Even in case the packet is
NAT'd by the bridge behind the patch port.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 12 +++-
 tests/mpls-xlate.at  |  2 +-
 tests/ofproto-dpif.at| 10 +-
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index de419402b9de..63040c002a00 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -577,6 +577,10 @@ static void xlate_xbundle_copy(struct xbridge *, struct 
xbundle *);
 static void xlate_xport_copy(struct xbridge *, struct xbundle *,
  struct xport *);
 static void xlate_xcfg_free(struct xlate_cfg *);
+
+struct compose_clone_info;
+static struct compose_clone_info *compose_clone_open(struct xlate_ctx *);
+static void compose_clone_close(struct xlate_ctx *, struct compose_clone_info 
*);
 
 /* Tracing helpers. */
 
@@ -3395,9 +3399,15 @@ compose_output_action__(struct xlate_ctx *ctx, 
ofp_port_t ofp_port,
 = ofproto_dpif_get_tables_version(ctx->xbridge->ofproto);
 
 if (!process_special(ctx, peer) && may_receive(peer, ctx)) {
-if (xport_stp_forward_state(peer) && 
xport_rstp_forward_state(peer)) {
+if (xport_stp_forward_state(peer)
+&& xport_rstp_forward_state(peer)) {
+struct compose_clone_info *info;
+
+info = compose_clone_open(ctx);
 xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
false);
+compose_clone_close(ctx, info);
+
 if (!ctx->freezing) {
 xlate_action_set(ctx);
 }
diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
index 9bbf22a99a3a..ebf1b7df582e 100644
--- a/tests/mpls-xlate.at
+++ b/tests/mpls-xlate.at
@@ -173,7 +173,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 
in_port=1,ip,actions=dec_ttl,push
 dnl MPLS push+pop
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'],
 [0], [stdout])
 AT_CHECK([tail -1 stdout], [0],
-  [Datapath actions: 
set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(tos=0/0xfc,ttl=64)),1,1
+  [Datapath actions: 
clone(set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3),1,1
 ])
 
 OVS_VSWITCHD_STOP
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1f6cd8422e04..c26bc99974aa 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -7371,15 +7371,15 @@ dummy@ovs-dummy: hit:13 missed:2
 ])
 
 AT_CHECK([strip_ufid < ovs-vswitchd.log | filter_flow_install | strip_used], 
[0], [dnl
-recirc_id(0),in_port(100),eth_type(0x0800),ipv4(frag=no), actions:101,3,2
-recirc_id(0),in_port(101),eth_type(0x0800),ipv4(frag=no), actions:100,2,3
+recirc_id(0),in_port(100),eth_type(0x0800),ipv4(frag=no), 
actions:clone(101,3),2
+recirc_id(0),in_port(101),eth_type(0x0800),ipv4(frag=no), 
actions:clone(100,2),3
 ])
 
 AT_CHECK([grep -e 'in_port(100).*packets:9' ovs-vswitchd.log | strip_ufid | 
filter_flow_dump], [0], [dnl
-skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(100),eth(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no),icmp(type=8/0,code=0/0),
 packets:9, bytes:378, used:0.0s, actions:101,3,2
+skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(100),eth(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no),icmp(type=8/0,code=0/0),
 packets:9, bytes:378, used:0.0s, actions:clone(101,3),2
 ])
 AT_CHECK([grep -e 'in_port(101).*packets:4' ovs-vswitchd.log | strip_ufid | 
filter_flow_dump], [0], [dnl
-skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(101),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no),icmp(type=8/0,code=0/0),
 packets:4, bytes:168, used:0.0s, actions:100,2,3
+skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(101),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:0

[ovs-dev] [PATCH 1/2] xlate: Add datapath clone generation API

[Andy Zhou]

There are three methods of translating openflow layer clone action.
Using datapath clone action, datapath sample action or using actions
to negating the any changes within a clone.  Xlate logic selects
a specific method depends on datapath features detected at the boot time.

Currently only xlate_clone() implements the selection logic since it
is the solo user, but patches will add more users. Introduce
 new APIs that hide the details of generating datapath clone action.

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 179 +--
 1 file changed, 140 insertions(+), 39 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f71a9db0a6b3..de419402b9de 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4956,36 +4956,151 @@ xlate_sample_action(struct xlate_ctx *ctx,
   tunnel_out_port, false);
 }
 
-/* Use datapath 'clone' or sample to enclose the translation of 'oc'.   */
-static void
-compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
+enum clone_method {
+/* For datapath that does not support clone, nor have a suitable
+ * sample action that can be used for clone. The xlate logic generates
+ * actions that restore the packets post clone.
+ *
+ * The draw back is not all packet changes inside a clone can be restored.
+ * For example, NAT'd packet can not be restored easily. Metered packet
+ * that dropped inside a clone can not be restored.
+ *
+ * This method is only used as the last resort for supporting older
+ * datapaths, and is used mainly for backward compatibility.   */
+COMPOSE_CLONE_USING_ACTIONS,
+
+/* For datapth that supports the 'clone' action.  Only OVS
+ * userspace datapath implements this action.  */
+COMPOSE_CLONE_USING_CLONE,
+
+/* For datapath that does not support 'clone' action, but have a suitable
+ * sample action implementation for clone. The upstream Linux kernel
+ * version 4.11 or greater, and kernel module from OVS version 2.8 or
+ * greater version have suitable sample action implementations.  */
+COMPOSE_CLONE_USING_SAMPLE
+};
+
+struct compose_clone_info {
+ enum clone_method method;
+ struct flow old_base_flow;
+
+ union {
+struct {
+size_t offset;
+} clone;
+struct {
+size_t offset;
+size_t ac_offset;
+} sample;
+};
+};
+
+static enum clone_method
+compose_clone_method(struct xlate_ctx *ctx)
 {
-size_t clone_offset = nl_msg_start_nested(ctx->odp_actions,
-  OVS_ACTION_ATTR_CLONE);
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-nl_msg_end_non_empty_nested(ctx->odp_actions, clone_offset);
+if (ctx->xbridge->support.clone) {
+return COMPOSE_CLONE_USING_CLONE;
+}
+
+if (ctx->xbridge->support.sample_nesting > 3) {
+return COMPOSE_CLONE_USING_SAMPLE;
+}
+
+return COMPOSE_CLONE_USING_ACTIONS;
 }
 
-/* Use datapath 'sample' action to translate clone.  */
-static void
-compose_clone_action_using_sample(struct xlate_ctx *ctx,
-  const struct ofpact_nest *oc)
+/* Generic datapath clone generation API
+ *
+ * This API generates the appropriate datapath nested clone
+ * netlink messages that surrounds inner actions. The inner
+ * action are generated with any logic that 
+ * below implements.
+ *
+ * The specific datapath clone action (clone, sample or none) is
+ * selected based on the datapath features detected at boot time.
+ *
+ * Usage
+ * =
+ *
+ * compose_clone_info *info;
+ * info = compose_clone_open(ctx);
+ *  ...
+ *  
+ *  ...
+ * compose_clone_close(ctx, info);
+ *
+ * These calls can be nested.
+ *
+ * 'info' returned from compose_clone_open() can be NULL, or it
+ * is created within this function. 'info' should be treated as
+ * an opaque structure, and should be passed later into
+ * compose_clone_close(), which will properly close the nested datapath
+ * clone action and dispose memory of 'info'.  */
+static struct compose_clone_info *
+compose_clone_open(struct xlate_ctx *ctx)
 {
-size_t offset = nl_msg_start_nested(ctx->odp_actions,
-OVS_ACTION_ATTR_SAMPLE);
+enum clone_method method = compose_clone_method(ctx);
 
-size_t ac_offset = nl_msg_start_nested(ctx->odp_actions,
-   OVS_SAMPLE_ATTR_ACTIONS);
+if (method == COMPOSE_CLONE_USING_ACTIONS) {
+return NULL;
+}
 
-do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+struct compose_clone_info *info = xmalloc(sizeof *info);
 
-if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
-nl_msg_cancel_nested(ctx->odp_actions, offset);
-} el

Re: [ovs-dev] [PATCH 2/2] faq: Expand on answer about OVS meter action support

[Andy Zhou]

On Fri, May 26, 2017 at 11:41 AM, Ben Pfaff <b...@ovn.org> wrote:
> On Fri, May 26, 2017 at 11:36:53AM -0700, Andy Zhou wrote:
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>
> For both:
> Acked-by: Ben Pfaff <b...@ovn.org>
>
> Thank you!

Thanks Ben. Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/2] faq: Expand on answer about OVS meter action support

[Andy Zhou]

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 Documentation/faq/qos.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/faq/qos.rst b/Documentation/faq/qos.rst
index 65a14eb9f3b2..ad3d95bf4fc0 100644
--- a/Documentation/faq/qos.rst
+++ b/Documentation/faq/qos.rst
@@ -166,4 +166,6 @@ Q: Does Open vSwitch support OpenFlow meters?
 
 A: Since version 2.0, Open vSwitch has OpenFlow protocol support for
 OpenFlow meters.  There is no implementation of meters in the Open vSwitch
-software switch (neither the kernel-based nor userspace switches).
+software switch (neither the kernel-based nor userspace switches)
+prior to version 2.8. Userspace switch meter implementation has been
+added to the master branch and is planned to be part of 2.8 release.
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/2] news: Add userspace meter action support.

[Andy Zhou]

Signed-off-by: Andy Zhou <az...@ovn.org>
---
 NEWS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NEWS b/NEWS
index 25eb477a8910..1c3035145724 100644
--- a/NEWS
+++ b/NEWS
@@ -94,6 +94,7 @@ v2.7.0 - 21 Feb 2017
  * The "sample" action now supports "ingress" and "egress" options.
  * The "ct" action now supports the TFTP ALG where support is available.
  * New actions "clone" and "ct_clear".
+ * The "meter" action is now supported in the userspace datapath.
- ovs-ofctl:
  * 'bundle' command now supports packet-out messages.
  * New syntax for 'ovs-ofctl packet-out' command, which uses the
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-ctl: Add commands to manage OVN DB ovsdb-servers individually

[Andy Zhou]

On Fri, May 26, 2017 at 4:48 AM, Numan Siddique <nusid...@redhat.com> wrote:
>
>
> On Fri, May 26, 2017 at 3:30 AM, Andy Zhou <az...@ovn.org> wrote:
>>
>> On Thu, May 25, 2017 at 1:55 AM,  <nusid...@redhat.com> wrote:
>> > From: Numan Siddique <nusid...@redhat.com>
>> >
>> > This patch adds the following functions
>> >   - start_nb_ovsdb, stop_nb_ovsdb, restart_nb_ovsdb to start, stop and
>> > restart the OVN NB DB ovsdb-server independently.
>> >   - start_sb_ovsdb, stop_sb_ovsdb, restart_sb_ovsdb to start, stop and
>> > restart the OVN SB DB ovsdb-server independently.
>> >
>> > These commands can be used to run ovsdb-server for each DB in a separate
>> > container.
>> >
>> > Signed-off-by: Numan Siddique <nusid...@redhat.com>
>>
>> LGTM.
>> Acked-by: Andy Zhou <az...@ovn.org>
>>
>> What's the intended use cases for running ovsdb-servers in separate
>> containers?
>> Where would northd run?
>
>
>
> The Openstack kolla project [1], it generates a separate container image for
> vswitchd and ovsdb-server.
> The goal is to support kolla images for OVN. I have submitted the patch here
> [2].
> Based on the present approach taken in kolla for openvswitch, this patch
> generates a separate container image for nb ovsdb-server, sb ovsdb-server,
> ovn-northd and ovn-controller. the nb ovsdb-server, sb ovsdb-server and
> ovn-northd is expected to be run in the same host (as separate containers).
>
> [1] - https://github.com/openstack/kolla/tree/master/docker/openvswitch
> [2] - https://review.openstack.org/#/c/459179/
>
Thanks, this is useful background information. I will push the patch
to master soon.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-ctl: Add commands to manage OVN DB ovsdb-servers individually

[Andy Zhou]

On Thu, May 25, 2017 at 1:55 AM,  <nusid...@redhat.com> wrote:
> From: Numan Siddique <nusid...@redhat.com>
>
> This patch adds the following functions
>   - start_nb_ovsdb, stop_nb_ovsdb, restart_nb_ovsdb to start, stop and
> restart the OVN NB DB ovsdb-server independently.
>   - start_sb_ovsdb, stop_sb_ovsdb, restart_sb_ovsdb to start, stop and
> restart the OVN SB DB ovsdb-server independently.
>
> These commands can be used to run ovsdb-server for each DB in a separate
> container.
>
> Signed-off-by: Numan Siddique <nusid...@redhat.com>

LGTM.
Acked-by: Andy Zhou <az...@ovn.org>

What's the intended use cases for running ovsdb-servers in separate containers?
Where would northd run?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] tests: fix hanging test on windows

[Andy Zhou]

On Tue, May 23, 2017 at 7:05 AM, Alin Serdean
<aserd...@cloudbasesolutions.com> wrote:
> From: Alin Serdean <aserd...@cloudbasesolutions.com>
>
> 'multiple bridges share a controller' hangs on windows because it is
> lacking the exit information (it will hang when the test has finished)
>
> Introduce a pidfile to 'ovs-testcontroller' and end it on exit based on
> the pidfile.
>
> Signed-off-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com>

Thanks Alin for the fix and Joe for the review. It looks good to me.

Acked-by: Andy Zhou <az...@ovn.org>

I will push it in a few minutes.

> ---
> v2: move 'on_exit' after 'ovs-testcontroller' creation (Joe Stringer)
> ---
>  tests/bridge.at | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/bridge.at b/tests/bridge.at
> index cc7619d..f6c2327 100644
> --- a/tests/bridge.at
> +++ b/tests/bridge.at
> @@ -48,7 +48,8 @@ OVS_VSWITCHD_START(
>  set bridge br1 datapath-type=dummy other-config:datapath-id=1234 ])
>
>  dnl Start ovs-testcontroller
> -AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore])
> +AT_CHECK([ovs-testcontroller --detach punix:controller --pidfile], [0], 
> [ignore])
> +on_exit 'kill `cat ovs-testcontroller.pid`'
>  OVS_WAIT_UNTIL([test -e controller])
>
>  dnl Add the controller to both bridges, 5 seconds apart.
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ovn pacemaker: Fix return code errors in start/stop action

[Andy Zhou]

On Mon, May 22, 2017 at 9:12 PM, Numan Siddique <nusid...@redhat.com> wrote:
>
>
> On Tue, May 23, 2017 at 5:21 AM, Andy Zhou <az...@ovn.org> wrote:
>>
>> On Sun, May 21, 2017 at 6:35 PM,  <nusid...@redhat.com> wrote:
>> > From: Numan Siddique <nusid...@redhat.com>
>> >
>> > start action returns OCF_RUNNING_MASTER in certain scenarios.
>> > But as per the OCF guidelines, status code OCF_RUNNING_MASTER shoud
>> > be returned only in monitor action [1].
>> >
>> > Whenever the start action returns OCF_RUNNING_MASTER, it is observed
>> > in the testing that, pacemaker stops the ovsdb-server ocf resource
>> > in that node. This patch fixes this issue by returning OCF_SUCESS in
>> > such cases.
>> >
>> > stop action returns OCF_RUNNING_MASTER if the ovsdb-servers are
>> > running as master. But as per the OCF guidelines [2], stop action
>> > should only return OCF_SUCCESS. If any other code is returned,
>> > pacemaker cluster would block that resource in that node.
>> >
>> > This patch fixes this issue by stopping the ovsdb-servers when they
>> > are running as masters (which is the expected case) and returns
>> > OCF_SUCCESS.
>> >
>> > [1] -
>> > http://www.linux-ha.org/doc/dev-guides/_literal_ocf_running_master_literal_8.html
>> > [2] -
>> > http://www.linux-ha.org/doc/dev-guides/_literal_stop_literal_action.html
>> >
>> > CC: Andy Zhou <az...@ovn.org>
>> > Signed-off-by: Numan Siddique <nusid...@redhat.com>
>>
>> Thanks for the fixes!  Both patches look reasonable to me. I pushed
>> them to master.
>
>
> Thanks Andy. Can these patches be back ported to  branch 2.7 ? It would be
> great since the tripleo patches for OVN needs these fixes
>
> Numan
Done. Thanks for the reminder.
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ovn pacemaker: Fix return code errors in start/stop action

[Andy Zhou]

On Sun, May 21, 2017 at 6:35 PM,  <nusid...@redhat.com> wrote:
> From: Numan Siddique <nusid...@redhat.com>
>
> start action returns OCF_RUNNING_MASTER in certain scenarios.
> But as per the OCF guidelines, status code OCF_RUNNING_MASTER shoud
> be returned only in monitor action [1].
>
> Whenever the start action returns OCF_RUNNING_MASTER, it is observed
> in the testing that, pacemaker stops the ovsdb-server ocf resource
> in that node. This patch fixes this issue by returning OCF_SUCESS in
> such cases.
>
> stop action returns OCF_RUNNING_MASTER if the ovsdb-servers are
> running as master. But as per the OCF guidelines [2], stop action
> should only return OCF_SUCCESS. If any other code is returned,
> pacemaker cluster would block that resource in that node.
>
> This patch fixes this issue by stopping the ovsdb-servers when they
> are running as masters (which is the expected case) and returns
> OCF_SUCCESS.
>
> [1] - 
> http://www.linux-ha.org/doc/dev-guides/_literal_ocf_running_master_literal_8.html
> [2] - http://www.linux-ha.org/doc/dev-guides/_literal_stop_literal_action.html
>
> CC: Andy Zhou <az...@ovn.org>
> Signed-off-by: Numan Siddique <nusid...@redhat.com>

Thanks for the fixes!  Both patches look reasonable to me. I pushed
them to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Fwd: [PATCH] bfd: Fix signs in ovs-appctl bfd/show Detect Time, Next Tx Time, Last TX Time

[Andy Zhou]

On Thu, May 18, 2017 at 6:39 AM, Gabor Szucs  wrote:
> Hi,
> This is a fix for ovs-appctl bfd/show printout that looks partly incorrect.
>
> ovs-appctl bfd/show command printout
> shows negative time lag from now for upcoming events:
> Detect Time: now -2632ms
> Next TX Time: now -800ms
> and positive time lag from now for past event:
> Last TX Time: now +150ms
>
> The fix negates the signs.
>
> Signed-off-by: Gábor Szűcs 
> Co-authored-by: Csaba Ihllye 
> Signed-off-by: Csaba Ihllye 

Thanks for sending the patch. Pushed to master.

The patch itself seems to be corrupted, may be it was sent with git send-email?
Since it is simple enough I just fixed it up locally before pushing.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: Fix timing dependency bridge - multiple bridges share a controller

[Andy Zhou]

On Wed, May 17, 2017 at 1:43 PM, Joe Stringer <j...@ovn.org> wrote:
> On 17 May 2017 at 13:16, Andy Zhou <az...@ovn.org> wrote:
>> On Wed, May 17, 2017 at 12:33 PM, Joe Stringer <j...@ovn.org> wrote:
>>> On 17 May 2017 at 11:37, Andy Zhou <az...@ovn.org> wrote:
>>>> Without the fix, this test currently consistently fail when running
>>>> on Travis CI. Connecting to the controller can take more time than
>>>> running locally. Because the exact connecting time is variable, the
>>>> exact output should not be used for correctness checking.
>>>>
>>>> Fixes: 85c55772a453(bridge: Fix controller status update)
>>>> Signed-off-by: Andy Zhou <az...@ovn.org>
>>>> ---
>>>
>>> Thanks, this seems to improve the situation.
>>>
>>>>  tests/bridge.at | 23 +++
>>>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tests/bridge.at b/tests/bridge.at
>>>> index 58b27d445062..cc7619d3f035 100644
>>>> --- a/tests/bridge.at
>>>> +++ b/tests/bridge.at
>>>> @@ -49,23 +49,30 @@ OVS_VSWITCHD_START(
>>>>
>>>>  dnl Start ovs-testcontroller
>>>>  AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore])
>>>> +OVS_WAIT_UNTIL([test -e controller])
>>>>
>>>>  dnl Add the controller to both bridges, 5 seconds apart.
>>>>  AT_CHECK([ovs-vsctl set-controller br0 unix:controller])
>>>> +AT_CHECK([ovs-vsctl set-fail-mode br0 secure])
>>>>  AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
>>>>  AT_CHECK([ovs-vsctl set-controller br1 unix:controller])
>>>> +AT_CHECK([ovs-vsctl set-fail-mode br1 secure])
>>>>
>>>> -dnl Wait for the controller connection to come up
>>>> -for i in `seq 0 7`
>>>> +dnl Wait for the controller connectionsi to be up
>>>> +for i in `seq 0 19`
>>>>  do
>>>> -AT_CHECK([ovs-appctl time/warp 10], [0], [ignore])
>>>> +if ovs-vsctl --column=is_connected list controller |grep "false"; then
>>>> +:
>>>> +else
>>>> +break
>>>> +fi
>>>> +ovs-appctl time/warp 1100
>>>>  done
>>>
>>> Can we substitute this for something like OVS_WAIT_WHILE([ovs-vsctl
>>> --column=is_connected list controller | grep "false"]) ?
>>
>> I have tested using OVS_WAIT_WHILE() and found some times 10 seconds
>> is not enough. The patch uses ovs-appctl time/warp instead wall clock, so it
>> should have less delay than using sleep.
>
> I see, just noticed this myself on Travis.
>
> My main concern is to get this fixed up on master, we can argue about
> how the test is layed out later.

May be we can consider having another kind of wait what only advances
ovs internal clock? At any rate, I agree that fix the master is a good
thing to do.

>
> Acked-by: Joe Stringer <j...@ovn.org>
Thanks for the review, Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] tests: Fix timing dependency bridge - multiple bridges share a controller

[Andy Zhou]

Without the fix, this test currently consistently fail when running
on Travis CI. Connecting to the controller can take more time than
running locally. Because the exact connecting time is variable, the
exact output should not be used for correctness checking.

Fixes: 85c55772a453(bridge: Fix controller status update)
Signed-off-by: Andy Zhou <az...@ovn.org>
---
 tests/bridge.at | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/tests/bridge.at b/tests/bridge.at
index 58b27d445062..cc7619d3f035 100644
--- a/tests/bridge.at
+++ b/tests/bridge.at
@@ -49,23 +49,30 @@ OVS_VSWITCHD_START(
 
 dnl Start ovs-testcontroller
 AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore])
+OVS_WAIT_UNTIL([test -e controller])
 
 dnl Add the controller to both bridges, 5 seconds apart.
 AT_CHECK([ovs-vsctl set-controller br0 unix:controller])
+AT_CHECK([ovs-vsctl set-fail-mode br0 secure])
 AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
 AT_CHECK([ovs-vsctl set-controller br1 unix:controller])
+AT_CHECK([ovs-vsctl set-fail-mode br1 secure])
 
-dnl Wait for the controller connection to come up
-for i in `seq 0 7`
+dnl Wait for the controller connectionsi to be up
+for i in `seq 0 19`
 do
-AT_CHECK([ovs-appctl time/warp 10], [0], [ignore])
+if ovs-vsctl --column=is_connected list controller |grep "false"; then
+:
+else
+break
+fi
+ovs-appctl time/warp 1100
 done
 
-dnl Make sure the connection status are different
-AT_CHECK([ovs-vsctl --columns=status list controller | sort], [0], [dnl
-
-status  : {sec_since_connect="0", state=ACTIVE}
-status  : {sec_since_connect="5", state=ACTIVE}
+dnl Make sure the connection status have two records and they are different.
+dnl (The exact output contains timing information that are machine dependent.)
+AT_CHECK([ovs-vsctl --column=status list controller | dnl
+  grep "status" | sort -u |wc -l], [0], [2
 ])
 
 OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] bridge: Fix controller status update

[Andy Zhou]

On Fri, May 12, 2017 at 12:07 PM, Andy Zhou <az...@ovn.org> wrote:
> On Thu, May 11, 2017 at 8:35 PM, Greg Rose <gvrose8...@gmail.com> wrote:
>> On Thu, 2017-05-11 at 16:16 -0700, Andy Zhou wrote:
>>> When multiple bridges connects to the same controller, the controller
>>> status should be maintained separately for each bridge. Current
>>> logic pushes status updates only based on the connection string,
>>> which is the same across multiple bridges when connecting to the
>>> same controller.
>>>
>>> Report-at: 
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2017-May/044412.html
>>> Reported-by: Tulio Ribeiro <tribe...@lasige.di.fc.ul.pt>
>>> Signed-off-by: Andy Zhou <az...@ovn.org>
>>> ---
>>>  AUTHORS.rst   |  1 +
>>>  tests/bridge.at   | 33 +
>>>  vswitchd/bridge.c | 33 +++--
>>>  3 files changed, 49 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/AUTHORS.rst b/AUTHORS.rst
>>> index 59639756b09f..147ce48d15ca 100644
>>> --- a/AUTHORS.rst
>>> +++ b/AUTHORS.rst
>>> @@ -530,6 +530,7 @@ Teemu Koponen   kopo...@nicira.com
>>>  Thomas Morinthomas.mo...@orange.com
>>>  Timothy Chentc...@nicira.com
>>>  Torbjorn Tornkvist  kruska...@gmail.com
>>> +Tulio Ribeiro   tribe...@lasige.di.fc.ul.pt
>>>  Tytus Kurek tytus.ku...@pega.com
>>>  Valentin Budvalen...@hackaserver.com
>>>  Vasiliy Tolstov v.tols...@selfip.ru
>>> diff --git a/tests/bridge.at b/tests/bridge.at
>>> index 3dbabe511780..58b27d445062 100644
>>> --- a/tests/bridge.at
>>> +++ b/tests/bridge.at
>>> @@ -38,3 +38,36 @@ dummy@ovs-dummy: hit:0 missed:0
>>>  OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>>>  OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>>>  AT_CLEANUP
>>> +
>>> +dnl When multiple bridges are connected to the same controller, make
>>> +dnl sure their status are tracked independently.
>>> +AT_SETUP([bridge - multiple bridges share a controller])
>>> +OVS_VSWITCHD_START(
>>> +   [add-br br1 -- \
>>> +set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
>>> +set bridge br1 datapath-type=dummy other-config:datapath-id=1234 ])
>>> +
>>> +dnl Start ovs-testcontroller
>>> +AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore])
>>> +
>>> +dnl Add the controller to both bridges, 5 seconds apart.
>>> +AT_CHECK([ovs-vsctl set-controller br0 unix:controller])
>>> +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
>>> +AT_CHECK([ovs-vsctl set-controller br1 unix:controller])
>>> +
>>> +dnl Wait for the controller connection to come up
>>> +for i in `seq 0 7`
>>> +do
>>> +AT_CHECK([ovs-appctl time/warp 10], [0], [ignore])
>>> +done
>>> +
>>> +dnl Make sure the connection status are different
>>> +AT_CHECK([ovs-vsctl --columns=status list controller | sort], [0], [dnl
>>> +
>>> +status  : {sec_since_connect="0", state=ACTIVE}
>>> +status  : {sec_since_connect="5", state=ACTIVE}
>>> +])
>>> +
>>> +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>>> +AT_CLEANUP
>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>>> index 31203d1ec232..972146e8da6d 100644
>>> --- a/vswitchd/bridge.c
>>> +++ b/vswitchd/bridge.c
>>> @@ -2704,34 +2704,31 @@ static void
>>>  refresh_controller_status(void)
>>>  {
>>>  struct bridge *br;
>>> -struct shash info;
>>> -const struct ovsrec_controller *cfg;
>>> -
>>> -shash_init();
>>>
>>>  /* Accumulate status for controllers on all bridges. */
>>>  HMAP_FOR_EACH (br, node, _bridges) {
>>> +struct shash info = SHASH_INITIALIZER();
>>>  ofproto_get_ofproto_controller_info(br->ofproto, );
>>> -}
>>>
>>> -/* Update each controller in the database with current status. */
>>> -OVSREC_CONTROLLER_FOR_EACH(cfg, idl) {
>>> -struct ofproto_controller_info *cinfo =
>>> -shash_find_data(, cfg->target);
>>> +/* Update each controller of the bridge in the database with
>>

Re: [ovs-dev] [PATCH] bridge: Fix controller status update

[Andy Zhou]

On Thu, May 11, 2017 at 8:35 PM, Greg Rose <gvrose8...@gmail.com> wrote:
> On Thu, 2017-05-11 at 16:16 -0700, Andy Zhou wrote:
>> When multiple bridges connects to the same controller, the controller
>> status should be maintained separately for each bridge. Current
>> logic pushes status updates only based on the connection string,
>> which is the same across multiple bridges when connecting to the
>> same controller.
>>
>> Report-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2017-May/044412.html
>> Reported-by: Tulio Ribeiro <tribe...@lasige.di.fc.ul.pt>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>> ---
>>  AUTHORS.rst   |  1 +
>>  tests/bridge.at   | 33 +
>>  vswitchd/bridge.c | 33 +++--
>>  3 files changed, 49 insertions(+), 18 deletions(-)
>>
>> diff --git a/AUTHORS.rst b/AUTHORS.rst
>> index 59639756b09f..147ce48d15ca 100644
>> --- a/AUTHORS.rst
>> +++ b/AUTHORS.rst
>> @@ -530,6 +530,7 @@ Teemu Koponen   kopo...@nicira.com
>>  Thomas Morinthomas.mo...@orange.com
>>  Timothy Chentc...@nicira.com
>>  Torbjorn Tornkvist  kruska...@gmail.com
>> +Tulio Ribeiro   tribe...@lasige.di.fc.ul.pt
>>  Tytus Kurek tytus.ku...@pega.com
>>  Valentin Budvalen...@hackaserver.com
>>  Vasiliy Tolstov v.tols...@selfip.ru
>> diff --git a/tests/bridge.at b/tests/bridge.at
>> index 3dbabe511780..58b27d445062 100644
>> --- a/tests/bridge.at
>> +++ b/tests/bridge.at
>> @@ -38,3 +38,36 @@ dummy@ovs-dummy: hit:0 missed:0
>>  OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>>  OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>>  AT_CLEANUP
>> +
>> +dnl When multiple bridges are connected to the same controller, make
>> +dnl sure their status are tracked independently.
>> +AT_SETUP([bridge - multiple bridges share a controller])
>> +OVS_VSWITCHD_START(
>> +   [add-br br1 -- \
>> +set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
>> +set bridge br1 datapath-type=dummy other-config:datapath-id=1234 ])
>> +
>> +dnl Start ovs-testcontroller
>> +AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore])
>> +
>> +dnl Add the controller to both bridges, 5 seconds apart.
>> +AT_CHECK([ovs-vsctl set-controller br0 unix:controller])
>> +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
>> +AT_CHECK([ovs-vsctl set-controller br1 unix:controller])
>> +
>> +dnl Wait for the controller connection to come up
>> +for i in `seq 0 7`
>> +do
>> +AT_CHECK([ovs-appctl time/warp 10], [0], [ignore])
>> +done
>> +
>> +dnl Make sure the connection status are different
>> +AT_CHECK([ovs-vsctl --columns=status list controller | sort], [0], [dnl
>> +
>> +status  : {sec_since_connect="0", state=ACTIVE}
>> +status  : {sec_since_connect="5", state=ACTIVE}
>> +])
>> +
>> +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>> +AT_CLEANUP
>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>> index 31203d1ec232..972146e8da6d 100644
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -2704,34 +2704,31 @@ static void
>>  refresh_controller_status(void)
>>  {
>>  struct bridge *br;
>> -struct shash info;
>> -const struct ovsrec_controller *cfg;
>> -
>> -shash_init();
>>
>>  /* Accumulate status for controllers on all bridges. */
>>  HMAP_FOR_EACH (br, node, _bridges) {
>> +struct shash info = SHASH_INITIALIZER();
>>  ofproto_get_ofproto_controller_info(br->ofproto, );
>> -}
>>
>> -/* Update each controller in the database with current status. */
>> -OVSREC_CONTROLLER_FOR_EACH(cfg, idl) {
>> -struct ofproto_controller_info *cinfo =
>> -shash_find_data(, cfg->target);
>> +/* Update each controller of the bridge in the database with
>> + * current status. */
>> +struct ovsrec_controller **controllers;
>> +size_t n_controllers = bridge_get_controllers(br, );
>> +size_t i;
>> +for (i = 0; i < n_controllers; i++) {
>> +struct ovsrec_controller *cfg = controllers[i];
>> +struct ofproto_controller_info *cinfo =
>> +shash_find_data(, cfg->target);
>>