Re: [ovs-dev] [PATCH v5 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-09-13 Thread Justin Pettit


> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei  wrote:
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 751535249e21..4b4c4d722645 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> 
> +static void
> +ct_timeout_policy_destroy(struct ct_timeout_policy *ct_tp,
> +  struct id_pool *tp_ids)
> +{
> +id_pool_free_id(tp_ids, ct_tp->tp_id);
> +simap_destroy(_tp->tp);
> +ovsrcu_postpone(free, ct_tp);
> +}

I assume we're delaying the freeing of 'ct_tp' in case there are people that 
still have references to it.  Should id_pool_free_id() and simap_destroy() be 
called from ovsrcu_postpone()?  (I didn't propose a change for this.)

> +static void
> +ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone,
> +   struct simap *timeout_policy)
> +{
> +struct dpif_backer *backer = shash_find_data(_dpif_backers,
> + datapath_type);
> +if (!backer) {
> +return;
> +}
> +
> +struct ct_timeout_policy *ct_tp = 
> ct_timeout_policy_lookup(>ct_tps,
> +   
> timeout_policy);
> +if (!ct_tp) {
> +ct_tp = ct_timeout_policy_alloc(timeout_policy, backer->tp_ids);
> +if (ct_tp) {
> +hmap_insert(>ct_tps, _tp->node, 
> simap_hash(_tp->tp));
> +ct_add_timeout_policy_to_dpif(backer->dpif, ct_tp);
> +} else {
> +VLOG_ERR_RL(, "failed to allocate timeout policy");

The call to ct_timeout_policy_alloc() will already log this error message if 
there was a problem.

> +return;
> +}
> +}
> +
> +struct ct_zone *ct_zone = ct_zone_lookup(>ct_zones, zone);
> +if (ct_zone) {
> +if (ct_zone->ct_tp != ct_tp) {
> +/* Add the new zone timeout pollicy. */
> +struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
> +new_ct_zone->ct_tp = ct_tp;
> +ct_tp->ref_count++;
> +cmap_replace(>ct_zones, _zone->node, 
> _ct_zone->node,
> + hash_int(zone, 0));
> +
> +/* Deletes the old zone timeout policy. */
> +ct_timeout_policy_unref(backer, ct_zone->ct_tp);
> +ct_zone_destroy(ct_zone);

Is there a reason that a new ct_zone needs to be allocated anew?  Couldn't you 
just do something like the following?

struct ct_zone *ct_zone = ct_zone_lookup(>ct_zones, zone_id);
 if (ct_zone) {
 if (ct_zone->ct_tp != ct_tp) {
-/* Add the new zone timeout pollicy. */
-struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
-new_ct_zone->ct_tp = ct_tp;
-ct_tp->ref_count++;
-cmap_replace(>ct_zones, _zone->node, _ct_zone->node,
- hash_int(zone, 0));
-
-/* Deletes the old zone timeout policy. */
+/* Update the zone timeout policy. */
 ct_timeout_policy_unref(backer, ct_zone->ct_tp);
-ct_zone_destroy(ct_zone);
+ct_zone->ct_tp = ct_tp;
+ct_tp->ref_count++;
 }

> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 6cc454371dc7..d53c8e2f80a4 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -58,6 +58,7 @@
> #include "tun-metadata.h"
> #include "versions.h"
> #include "vl-mff-map.h"
> +#include "vswitch-idl.h"

I don't think this #include is necessary with these changes.

> +/* Sets conntrack timeout policy specified by 'timeout_policy' to 'zone'
> + * in datapath type 'dp_type'. */
> +void (*ct_set_zone_timeout_policy)(const char *dp_type, uint16_t zone,
> +   struct simap *timeout_policy);
> +
> +/* Deletes the timeout policy associated with 'zone' in datapath type
> + * 'dp_type'. */
> +void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
> };

I think it's clearer to use 'zone_id' throughout instead of 'zone'.

> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index d921c4ef8d5f..c43f2ac34e67 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -153,9 +153,36 @@ struct aa_mapping {
> char *br_name;
> };
> 
> +/* Internal representation of conntrack zone configuration table in OVSDB. */
> +struct ct_zone {
> +uint16_t zone;

I think 'zone_id' is a clearer name for what this is.  I'd also suggest using 
that through bridge.c.

> +struct simap tp;/* A map from timeout policy attribute to
> + * timeout value. */
> +unsigned int last_used; /* The last idl_seqno that this 'ct_zone' 
> used
> + * in OVSDB. This number is used for garbage
> + * collection. */
> +struct hmap_node node;  /* Node in 'struct datapath' 'ct_zones'
> + * hmap. */
> +};
> +

Re: [ovs-dev] [PATCH] netdev-offload-dpdk : add ipv6 rte flow item support

2019-09-13 Thread Yifeng Sun
Hi Timo,

Please try to format your patch in raw text.

Thanks,
Yifeng

On Wed, Sep 11, 2019 at 7:55 PM Timo_Liu  wrote:
>
>
>
> Nowadays some Nics support hw offloading via dpdk rte_flow lib. Many 
> layer2-layer4 fields can be offloaded to nics, including smac/dmac ipv4 
> sip/dip etc. Also some nics(including intel X710) supports ipv6 header 
> offloading, but when we execute netdev_offload_dpdk_add_flow, there is no 
> IPV6 rte_flow pattern.
>
>
>
>
>
> This patch adds support for IPV6 header hw-offload rte_flow pattern, 
> including ipv6_label, nw_ttl, nw_proto, ipv6 sip and dip, also the 
> corresponding mask filed is added.
>
>
>
>
> Signed-off-by: Liu Chang 
>
>
>
>
>
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>
> index 01e9004..ab3f82b 100644
>
> --- a/lib/netdev-offload-dpdk.c
>
> +++ b/lib/netdev-offload-dpdk.c
>
> @@ -433,7 +433,10 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>
>  struct flow_items {
>
>  struct rte_flow_item_eth  eth;
>
>  struct rte_flow_item_vlan vlan;
>
> -struct rte_flow_item_ipv4 ipv4;
>
> +union {
>
> +struct rte_flow_item_ipv4 ipv4;
>
> +struct rte_flow_item_ipv6 ipv6;
>
> +};
>
>  union {
>
>  struct rte_flow_item_tcp  tcp;
>
>  struct rte_flow_item_udp  udp;
>
> @@ -503,6 +506,31 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>
>  mask.ipv4.hdr.next_proto_id;
>
>  }
>
>
>
>
> +/* IP v6 */
>
> +if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) {
>
> +int i =0;
>
> +
>
> +spec.ipv6.hdr.vtc_flow   = match->flow.ipv6_label;
>
> +spec.ipv6.hdr.proto  = match->flow.nw_proto;
>
> +spec.ipv6.hdr.hop_limits = match->flow.nw_ttl;
>
> +
>
> +mask.ipv6.hdr.vtc_flow   = match->wc.masks.ipv6_label;
>
> +mask.ipv6.hdr.proto  = match->wc.masks.nw_proto;
>
> +mask.ipv6.hdr.hop_limits = match->wc.masks.nw_ttl;
>
> +
>
>
>
> +for (i = 0; i < 16; i++) {
>
> +spec.ipv6.hdr.src_addr[i] = match->flow.ipv6_src.s6_addr[i];
>
> +spec.ipv6.hdr.dst_addr[i] = match->flow.ipv6_dst.s6_addr[i];
>
> +
>
> +mask.ipv6.hdr.src_addr[i] = match->wc.masks.ipv6_src.s6_addr[i];
>
> +mask.ipv6.hdr.dst_addr[i] = match->wc.masks.ipv6_dst.s6_addr[i];
>
> +}
>
> +
>
> +add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV6,
>
> + spec.ipv6, mask.ipv6);
>
> +
>
> +}
>
> +
>
>  if (proto != IPPROTO_ICMP  proto != IPPROTO_UDP  
>
>  proto != IPPROTO_SCTP  proto != IPPROTO_TCP  
>
>  (match->wc.masks.tp_src ||
>
>
>
>
>
>
>
>
> Best Regards
>
> Timo_liu
> ___
> 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: ratelimit the device add log

2019-09-13 Thread Ben Pfaff
On Thu, Sep 12, 2019 at 11:45:41AM -0400, Aaron Conole wrote:
> It's possible that a port added to the system with certain kinds
> of invalid parameters will cause the 'could not add' log to be
> triggered.  When this happens, the vswitch run loop can continually
> re-attempt adding the port.  While the parameters remain invalid
> the vswitch run loop will re-trigger the warning, flooding the
> syslog.
> 
> This patch adds a simple rate limit to the log.
> 
> Signed-off-by: Aaron Conole 
> ---
>  vswitchd/bridge.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index d921c4ef8..49a6f6a37 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1816,8 +1816,12 @@ iface_do_create(const struct bridge *br,
>  *ofp_portp = iface_pick_ofport(iface_cfg);
>  error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
>  if (error) {
> -VLOG_WARN_BUF(errp, "could not add network device %s to ofproto 
> (%s)",
> -  iface_cfg->name, ovs_strerror(error));
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +if (!VLOG_DROP_WARN()) {
> +VLOG_WARN_BUF(errp,
> +  "could not add network device %s to ofproto (%s)",
> +  iface_cfg->name, ovs_strerror(error));
> +}
>  goto error;
>  }

I understand why we want to rate-limit what's going to the log.

We don't want to rate-limit the text being put into errp, though,
because that goes to the database record for that particular interface
to indicate why it's malfunctioning.  It should keep the error message
regardless of how much it recurs.

Can you come up with a way to handle both of these requirements
gracefully?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v5 2/9] ovs-vsctl: Add conntrack zone commands.

2019-09-13 Thread Justin Pettit



> On Sep 13, 2019, at 11:06 AM, William Tu  wrote:
> 
> On Fri, Sep 13, 2019 at 10:48 AM Justin Pettit  wrote:
>> 
>> 
>>> On Sep 13, 2019, at 10:41 AM, William Tu  wrote:
>>> 
 Is there a reason you limited this to 18 arguments and not use INT_MAX?
>>> 
>>> I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
>>> (dp_name, zone_id), so total is 18.
>>> I think using INT_MAX is fine, because at db schema, the value type is set.
>> 
>> That's a fair argument.  However, I'd suggest that we let ovs-vswitchd do 
>> that enforcement.  It's nice to decouple the configuration tool from the 
>> binary as much as possible in case people run different versions.
>> 
>>> I will merge your diff and send next version.
>> 
>> Sounds good.  I'm hopeful that we'll be able to get this version merged with 
>> minimal changes, though.
>> 
>> Thanks,
> 
> Hi Justin,
> 
> I applied your diff and it's working fine.
> I don't have any other change, do you want me to send another
> patch or you want to directly apply?

I'm hoping that I can just apply your patch with the changes directly.

--Justin


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


Re: [ovs-dev] [PATCH] netdev-afxdp: Detect numa node id.

2019-09-13 Thread William Tu
On Fri, Sep 13, 2019 at 11:40 AM Aaron Conole  wrote:
>
> William Tu  writes:
>
> > Hi Aaron,
> >
> > I think this might be a false positive?
>
> Looks like it.

Hi Aaron,

I looked at the checkpatch.py.
Adding a space in front of /sys makes it PASSED, but I couldn't figure it out
a solution to fix, ex:
+numa_node_path = xasprintf(" /sys/class/net/%s/device/numa_node",

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


Re: [ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction

2019-09-13 Thread Daniel Alvarez Sanchez
Acked-by: Daniel Alvarez 


On Fri, Sep 13, 2019 at 11:02 PM Mark Michelson  wrote:
>
> Acked-by: Mark Michelson
>
> It sucks that we lose the efficiency of the conjunctive match altogether
> on port groups because of this error, but I understand this is a huge
> bug and needs fixing.
If I'm not mistaken, from OpenStack standpoint conjunction was *only*
being used when using port groups and ACLs that matched on port ranges
( e.g tcp.dst >= X && tcp.dst <=Y) which was not working. Therefore
we're not losing performance because it was already broken (given that
there was more than one ACL like that).

>
> Perhaps it would be good to start up a discussion on this list about a
> more longterm solution that would allow for conjunctive matches with no
> ambiguity.
Agreed! We already discussed some ideas on IRC but it'd be awesome to
have a thread and brainstorm there.

Thanks a lot everyone!
Daniel
>
> On 9/13/19 4:49 PM, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > If there are multiple ACLs associated with a port group and they
> > match on a range of some field, then ovn-controller doesn't install
> > the flows properly and this results in broken ACL functionality.
> >
> > For example, if there is a port group - pg1 with logical ports - [p1, p2]
> > and if there are below ACLs (only match condition is shown)
> >
> > 1 -  outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
> > 2 -  outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601
> >
> > The first ACL will result in the below OF flows
> >
> > 1.  conj_id=1,tcp
> > 2.  tcp,reg15=0x11: conjunction(1, 1/2)
> > 3.  tcp,reg15=0x12: conjunction(1, 1/2)
> > 5.  tcp,tp_dst=500: conjunction(1, 2/2)
> > 6.  tcp,tp_dst=501: conjunction(1, 2/2)
> >
> > The second ACL will result in the below OF flows
> > 7.  conj_id=2,tcp
> > 8.  tcp,reg15=0x11: conjunction(2, 1/2)
> > 9.  tcp,reg15=0x12: conjunction(2, 1/2)
> > 11. tcp,tp_dst=600: conjunction(2, 2/2)
> > 12. tcp,tp_dst=601: conjunction(2, 3/2)
> >
> > The OF flows (2) and (8) have the exact match but with different action.
> > This results in only one of the flows getting installed. The same goes
> > for the flows (3) and (9). And this completely breaks the ACL functionality
> > for such scenarios.
> >
> > In order to fix this issue, this patch excludes the 'inport' and 'outport' 
> > symbols
> > from conjunction. With this patch we will have the below flows.
> >
> > tcp,reg15=0x11,tp_dst=500
> > tcp,reg15=0x11,tp_dst=501
> > tcp,reg15=0x12,tp_dst=500
> > tcp,reg15=0x12,tp_dst=501
> > tcp,reg15=0x13,tp_dst=500
> > tcp,reg15=0x13,tp_dst=501
> > tcp,reg15=0x11,tp_dst=600
> > tcp,reg15=0x11,tp_dst=601
> > tcp,reg15=0x12,tp_dst=600
> > tcp,reg15=0x12,tp_dst=601
> > tcp,reg15=0x13,tp_dst=600
> > tcp,reg15=0x13,tp_dst=601
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >   lib/expr.c   |  2 +-
> >   tests/ovn.at | 26 ++
> >   2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/expr.c b/lib/expr.c
> > index e4c650f7c..c0871e1e8 100644
> > --- a/lib/expr.c
> > +++ b/lib/expr.c
> > @@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab, const 
> > char *name,
> >   const struct mf_field *field = mf_from_id(id);
> >   struct expr_symbol *symbol;
> >
> > -symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false,
> > +symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, true,
> >   field->writable);
> >   symbol->field = field;
> >   return symbol;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 2a35b4e15..14d9f59b0 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -589,6 +589,24 @@ ip,reg14=0x6
> >   ipv6,reg14=0x5
> >   ipv6,reg14=0x6
> >   ])
> > +AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp 
> > && tcp.dst == {500, 501}'], [0], [dnl
> > +tcp,reg14=0x5,tp_dst=500
> > +tcp,reg14=0x5,tp_dst=501
> > +tcp,reg14=0x6,tp_dst=500
> > +tcp,reg14=0x6,tp_dst=501
> > +])
> > +AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp 
> > && tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
> > +conj_id=1,tcp,reg15=0x5
> > +conj_id=2,tcp,reg15=0x6
> > +tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2)
> > +tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2)
> > +tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2)
> > +tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2)
> > +tcp,reg15=0x6,tp_dst=500: conjunction(2, 0/2)
> > +tcp,reg15=0x6,tp_dst=501: conjunction(2, 0/2)
> > +tcp,reg15=0x6,tp_src=400: conjunction(2, 1/2)
> > +tcp,reg15=0x6,tp_src=401: conjunction(2, 1/2)
> > +])
> >   AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
> >   (no flows)
> >   ])
> > @@ -693,6 +711,14 @@ reg15=0x11
> >   reg15=0x12
> >   reg15=0x13
> >   ])
> > +AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 
> > 10.0.0.5}'], [0], [dnl
> > +ip,reg15=0x11,nw_src=10.0.0.4
> > +ip,reg15=0x11,nw_src=10.0.0.5
> > 

Re: [ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction

2019-09-13 Thread Mark Michelson

Acked-by: Mark Michelson

It sucks that we lose the efficiency of the conjunctive match altogether 
on port groups because of this error, but I understand this is a huge 
bug and needs fixing.


Perhaps it would be good to start up a discussion on this list about a 
more longterm solution that would allow for conjunctive matches with no 
ambiguity.


On 9/13/19 4:49 PM, nusid...@redhat.com wrote:

From: Numan Siddique 

If there are multiple ACLs associated with a port group and they
match on a range of some field, then ovn-controller doesn't install
the flows properly and this results in broken ACL functionality.

For example, if there is a port group - pg1 with logical ports - [p1, p2]
and if there are below ACLs (only match condition is shown)

1 -  outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
2 -  outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601

The first ACL will result in the below OF flows

1.  conj_id=1,tcp
2.  tcp,reg15=0x11: conjunction(1, 1/2)
3.  tcp,reg15=0x12: conjunction(1, 1/2)
5.  tcp,tp_dst=500: conjunction(1, 2/2)
6.  tcp,tp_dst=501: conjunction(1, 2/2)

The second ACL will result in the below OF flows
7.  conj_id=2,tcp
8.  tcp,reg15=0x11: conjunction(2, 1/2)
9.  tcp,reg15=0x12: conjunction(2, 1/2)
11. tcp,tp_dst=600: conjunction(2, 2/2)
12. tcp,tp_dst=601: conjunction(2, 3/2)

The OF flows (2) and (8) have the exact match but with different action.
This results in only one of the flows getting installed. The same goes
for the flows (3) and (9). And this completely breaks the ACL functionality
for such scenarios.

In order to fix this issue, this patch excludes the 'inport' and 'outport' 
symbols
from conjunction. With this patch we will have the below flows.

tcp,reg15=0x11,tp_dst=500
tcp,reg15=0x11,tp_dst=501
tcp,reg15=0x12,tp_dst=500
tcp,reg15=0x12,tp_dst=501
tcp,reg15=0x13,tp_dst=500
tcp,reg15=0x13,tp_dst=501
tcp,reg15=0x11,tp_dst=600
tcp,reg15=0x11,tp_dst=601
tcp,reg15=0x12,tp_dst=600
tcp,reg15=0x12,tp_dst=601
tcp,reg15=0x13,tp_dst=600
tcp,reg15=0x13,tp_dst=601

Signed-off-by: Numan Siddique 
---
  lib/expr.c   |  2 +-
  tests/ovn.at | 26 ++
  2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/lib/expr.c b/lib/expr.c
index e4c650f7c..c0871e1e8 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab, const char 
*name,
  const struct mf_field *field = mf_from_id(id);
  struct expr_symbol *symbol;
  
-symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false,

+symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, true,
  field->writable);
  symbol->field = field;
  return symbol;
diff --git a/tests/ovn.at b/tests/ovn.at
index 2a35b4e15..14d9f59b0 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -589,6 +589,24 @@ ip,reg14=0x6
  ipv6,reg14=0x5
  ipv6,reg14=0x6
  ])
+AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp && 
tcp.dst == {500, 501}'], [0], [dnl
+tcp,reg14=0x5,tp_dst=500
+tcp,reg14=0x5,tp_dst=501
+tcp,reg14=0x6,tp_dst=500
+tcp,reg14=0x6,tp_dst=501
+])
+AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp && tcp.src 
== {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
+conj_id=1,tcp,reg15=0x5
+conj_id=2,tcp,reg15=0x6
+tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2)
+tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2)
+tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2)
+tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2)
+tcp,reg15=0x6,tp_dst=500: conjunction(2, 0/2)
+tcp,reg15=0x6,tp_dst=501: conjunction(2, 0/2)
+tcp,reg15=0x6,tp_src=400: conjunction(2, 1/2)
+tcp,reg15=0x6,tp_src=401: conjunction(2, 1/2)
+])
  AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
  (no flows)
  ])
@@ -693,6 +711,14 @@ reg15=0x11
  reg15=0x12
  reg15=0x13
  ])
+AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 10.0.0.5}'], 
[0], [dnl
+ip,reg15=0x11,nw_src=10.0.0.4
+ip,reg15=0x11,nw_src=10.0.0.5
+ip,reg15=0x12,nw_src=10.0.0.4
+ip,reg15=0x12,nw_src=10.0.0.5
+ip,reg15=0x13,nw_src=10.0.0.4
+ip,reg15=0x13,nw_src=10.0.0.5
+])
  AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
  (no flows)
  ])



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


[ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction

2019-09-13 Thread nusiddiq
From: Numan Siddique 

If there are multiple ACLs associated with a port group and they
match on a range of some field, then ovn-controller doesn't install
the flows properly and this results in broken ACL functionality.

For example, if there is a port group - pg1 with logical ports - [p1, p2]
and if there are below ACLs (only match condition is shown)

1 -  outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
2 -  outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601

The first ACL will result in the below OF flows

1.  conj_id=1,tcp
2.  tcp,reg15=0x11: conjunction(1, 1/2)
3.  tcp,reg15=0x12: conjunction(1, 1/2)
5.  tcp,tp_dst=500: conjunction(1, 2/2)
6.  tcp,tp_dst=501: conjunction(1, 2/2)

The second ACL will result in the below OF flows
7.  conj_id=2,tcp
8.  tcp,reg15=0x11: conjunction(2, 1/2)
9.  tcp,reg15=0x12: conjunction(2, 1/2)
11. tcp,tp_dst=600: conjunction(2, 2/2)
12. tcp,tp_dst=601: conjunction(2, 3/2)

The OF flows (2) and (8) have the exact match but with different action.
This results in only one of the flows getting installed. The same goes
for the flows (3) and (9). And this completely breaks the ACL functionality
for such scenarios.

In order to fix this issue, this patch excludes the 'inport' and 'outport' 
symbols
from conjunction. With this patch we will have the below flows.

tcp,reg15=0x11,tp_dst=500
tcp,reg15=0x11,tp_dst=501
tcp,reg15=0x12,tp_dst=500
tcp,reg15=0x12,tp_dst=501
tcp,reg15=0x13,tp_dst=500
tcp,reg15=0x13,tp_dst=501
tcp,reg15=0x11,tp_dst=600
tcp,reg15=0x11,tp_dst=601
tcp,reg15=0x12,tp_dst=600
tcp,reg15=0x12,tp_dst=601
tcp,reg15=0x13,tp_dst=600
tcp,reg15=0x13,tp_dst=601

Signed-off-by: Numan Siddique 
---
 lib/expr.c   |  2 +-
 tests/ovn.at | 26 ++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/lib/expr.c b/lib/expr.c
index e4c650f7c..c0871e1e8 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab, const char 
*name,
 const struct mf_field *field = mf_from_id(id);
 struct expr_symbol *symbol;
 
-symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false,
+symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, true,
 field->writable);
 symbol->field = field;
 return symbol;
diff --git a/tests/ovn.at b/tests/ovn.at
index 2a35b4e15..14d9f59b0 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -589,6 +589,24 @@ ip,reg14=0x6
 ipv6,reg14=0x5
 ipv6,reg14=0x6
 ])
+AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp && 
tcp.dst == {500, 501}'], [0], [dnl
+tcp,reg14=0x5,tp_dst=500
+tcp,reg14=0x5,tp_dst=501
+tcp,reg14=0x6,tp_dst=500
+tcp,reg14=0x6,tp_dst=501
+])
+AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp && 
tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
+conj_id=1,tcp,reg15=0x5
+conj_id=2,tcp,reg15=0x6
+tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2)
+tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2)
+tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2)
+tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2)
+tcp,reg15=0x6,tp_dst=500: conjunction(2, 0/2)
+tcp,reg15=0x6,tp_dst=501: conjunction(2, 0/2)
+tcp,reg15=0x6,tp_src=400: conjunction(2, 1/2)
+tcp,reg15=0x6,tp_src=401: conjunction(2, 1/2)
+])
 AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
 (no flows)
 ])
@@ -693,6 +711,14 @@ reg15=0x11
 reg15=0x12
 reg15=0x13
 ])
+AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 10.0.0.5}'], 
[0], [dnl
+ip,reg15=0x11,nw_src=10.0.0.4
+ip,reg15=0x11,nw_src=10.0.0.5
+ip,reg15=0x12,nw_src=10.0.0.4
+ip,reg15=0x12,nw_src=10.0.0.5
+ip,reg15=0x13,nw_src=10.0.0.4
+ip,reg15=0x13,nw_src=10.0.0.5
+])
 AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
 (no flows)
 ])
-- 
2.21.0

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


Re: [ovs-dev] [PATCH v3 ovn] northd: add empty_lb controller_event for logical router

2019-09-13 Thread Mark Michelson

Acked-by: Mark Michelson 

On 9/10/19 1:00 PM, Lorenzo Bianconi wrote:

Add empty load balancer controller_event support to logical router
pipeline. Update northd documentation even for logical switch pipeline

Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- improve code readability
Changes since v1:
- rebase on-top of current ovn master branch
---
  northd/ovn-northd.8.xml | 10 
  northd/ovn-northd.c | 24 --
  tests/ovn.at| 56 +++--
  3 files changed, 76 insertions(+), 14 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index b34ef687a..0f4f1c112 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1785,6 +1785,16 @@ icmp6 {
  
  
  

+  
+If controller_event has been enabled for all the configured load
+balancing rules for a Gateway router or Router with gateway port
+in OVN_Northbound database that does not have configured
+backends, a priority-130 flow is added to trigger ovn-controller events
+whenever the chassis receives a packet for that particular VIP.
+If event-elb meter has been previously created, it will be
+associated to the empty_lb logical flow
+  
+

  For all the configured load balancing rules for a Gateway router or
  Router with gateway port in OVN_Northbound database that
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index c24e4d864..f393cebb8 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -6148,9 +6148,17 @@ get_force_snat_ip(struct ovn_datapath *od, const char 
*key_type, ovs_be32 *ip)
  static void
  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
 struct ds *match, struct ds *actions, int priority,
-   const char *lb_force_snat_ip, char *backend_ips,
-   bool is_udp, int addr_family)
+   const char *lb_force_snat_ip, struct smap_node *lb_info,
+   bool is_udp, int addr_family, char *ip_addr,
+   uint16_t l4_port, struct nbrec_load_balancer *lb,
+   struct shash *meter_groups)
  {
+char *backend_ips = lb_info->value;
+
+build_empty_lb_event_flow(od, lflows, lb_info, ip_addr, lb,
+  l4_port, addr_family, S_ROUTER_IN_DNAT,
+  meter_groups);
+
  /* A match and actions for new connections. */
  char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
  if (lb_force_snat_ip) {
@@ -6308,7 +6316,7 @@ copy_ra_to_sb(struct ovn_port *op, const char 
*address_mode)
  
  static void

  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
-struct hmap *lflows)
+struct hmap *lflows, struct shash *meter_groups)
  {
  /* This flow table structure is documented in ovn-northd(8), so please
   * update ovn-northd.8.xml if you change anything. */
@@ -7525,7 +7533,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
  ds_put_format(, "ip && ip6.dst == %s",
  ip_address);
  }
-free(ip_address);
  
  int prio = 110;

  bool is_udp = lb->protocol && !strcmp(lb->protocol, "udp") ?
@@ -7546,8 +7553,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
od->l3redirect_port->json_key);
  }
  add_router_lb_flow(lflows, od, , , prio,
-   lb_force_snat_ip, node->value, is_udp,
-   addr_family);
+   lb_force_snat_ip, node, is_udp,
+   addr_family, ip_address, port, lb,
+   meter_groups);
+
+free(ip_address);
  }
  }
  sset_destroy(_ips);
@@ -8328,7 +8338,7 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
  
  build_lswitch_flows(datapaths, ports, port_groups, , mcgroups,

  igmp_groups, meter_groups);
-build_lrouter_flows(datapaths, ports, );
+build_lrouter_flows(datapaths, ports, , meter_groups);
  
  /* Push changes to the Logical_Flow table to database. */

  const struct sbrec_logical_flow *sbflow, *next_sbflow;
diff --git a/tests/ovn.at b/tests/ovn.at
index de1b3b3ba..2749184eb 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14683,9 +14683,22 @@ ovn_start
  # Create hypervisors hv[12].
  # Add vif1[12] to hv1, vif2[12] to hv2
  # Add all of the vifs to a single logical switch sw0.
+# Create logical router lr0
  
  net_add n1

-ovn-nbctl ls-add sw0
+
+ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
+for i in 0 1; do
+idx=$((i+1))
+ovn-nbctl ls-add sw$i
+ovn-nbctl lrp-add lr0 lrp$i 

[ovs-dev] Boat Owners

2019-09-13 Thread Carla Joseph


Hi,

Greetings of the day!

Would you be interested in acquiring an email list of "Boat Owner List" from 
USA?

We also have data for RV Owners, Cruise Travelers, Fishing Enthusiasts, Scuba 
Divers List, Food Lovers, Car Owners, Apparel Buyers, Time-Share Owners, Real 
Estate Investors, Spa and Resort Visitors and many more..

All the contacts are opt-in verified, complete permission based and can be used 
for unlimited multi-channel marketing.

Please let me know your thoughts towards procuring the Boat Owners List.

Best Regards,
Carla Joseph



We respect your privacy, if you do not wish to receive any further emails from 
our end, please reply with a subject “Leave Out”.

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


Re: [ovs-dev] [PATCH] netdev-afxdp: Detect numa node id.

2019-09-13 Thread Aaron Conole
William Tu  writes:

> Hi Aaron,
>
> I think this might be a false positive?

Looks like it.

> I didn't find any lack whitespace here
>
> On Fri, Sep 13, 2019 at 11:01 AM 0-day Robot  wrote:
>>
>> Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your 
>> patch.
>> Thanks for your contribution.
>>
>> I encountered some error that I wasn't expecting.  See the details below.
>>
>>
>> checkpatch:
>> WARNING: Line lacks whitespace around operator
>> #35 FILE: lib/netdev-afxdp.c:561:
>> numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",

Possibly something with the '/' inside the '"'

If I get some time, I'll test it out.

> Regards,
> William
>
>
>> Lines checked: 67, Warnings: 1, Errors: 0
>>
>>
>> Please check this out.  If you feel there has been an error, please email 
>> acon...@redhat.com
>>
>> Thanks,
>> 0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] netdev-afxdp: Detect numa node id.

2019-09-13 Thread William Tu
Hi Aaron,

I think this might be a false positive?
I didn't find any lack whitespace here

On Fri, Sep 13, 2019 at 11:01 AM 0-day Robot  wrote:
>
> Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> WARNING: Line lacks whitespace around operator
> #35 FILE: lib/netdev-afxdp.c:561:
> numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",
>
Regards,
William


> Lines checked: 67, Warnings: 1, Errors: 0
>
>
> Please check this out.  If you feel there has been an error, please email 
> acon...@redhat.com
>
> Thanks,
> 0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v5 2/9] ovs-vsctl: Add conntrack zone commands.

2019-09-13 Thread William Tu
On Fri, Sep 13, 2019 at 10:48 AM Justin Pettit  wrote:
>
>
> > On Sep 13, 2019, at 10:41 AM, William Tu  wrote:
> >
> >> Is there a reason you limited this to 18 arguments and not use INT_MAX?
> >
> > I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
> > (dp_name, zone_id), so total is 18.
> > I think using INT_MAX is fine, because at db schema, the value type is set.
>
> That's a fair argument.  However, I'd suggest that we let ovs-vswitchd do 
> that enforcement.  It's nice to decouple the configuration tool from the 
> binary as much as possible in case people run different versions.
>
> > I will merge your diff and send next version.
>
> Sounds good.  I'm hopeful that we'll be able to get this version merged with 
> minimal changes, though.
>
> Thanks,

Hi Justin,

I applied your diff and it's working fine.
I don't have any other change, do you want me to send another
patch or you want to directly apply?

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


Re: [ovs-dev] [PATCH] netdev-afxdp: Detect numa node id.

2019-09-13 Thread 0-day Robot
Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
#35 FILE: lib/netdev-afxdp.c:561:
numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",

Lines checked: 67, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

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


Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema

2019-09-13 Thread aginwala
Thanks Yifeng:

Sure I revisited the trigger flow and even if its disconnected with
trigger_success(), ovsdb_jsonrpc_trigger_complete will indeed take care of
destroying t->reply via jsonrpc_msg_destroy() which I missed. So you are
right, we don't need to json_destroy(result) explicitly here. I take it
back!

On Fri, Sep 13, 2019 at 9:46 AM Yifeng Sun  wrote:

> Thanks Aginwala,
>
> Could you please double check if 'json_destroy(result)' is necessary here?
> If result != NULL, then it is passed in trigger_success(), which puts
> result in 't->reply',
> later, jsonrpc_msg_destroy() will free it.
>
> Thanks,
> Yifeng
>
> On Thu, Sep 12, 2019 at 2:00 PM aginwala  wrote:
> >
> > One minor suggestion here:
> > Can you also handle freeing result:
> > diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
> > index 6f4ed96b0..0158957d6 100644
> > --- a/ovsdb/trigger.c
> > +++ b/ovsdb/trigger.c
> > @@ -214,6 +214,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long
> int now)
> >  /* Unsatisfied "wait" condition.  Take no action
> now, retry
> >   * later. */
> >  }
> > +json_destroy(result);
> >  return false;
> >  }
> >
> > Else I can handle that in separate patch. Else, acked by for the series.
> > Acked-by: Aliasgar Ginwala 
> >
> > On Wed, Sep 11, 2019 at 2:19 PM Yifeng Sun 
> wrote:
> >>
> >> Valgrind reported:
> >>
> >> 1925: schema conversion online - standalone
> >>
> >> ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are
> definitely lost in loss record 384 of 420
> >> ==10884==at 0x4C2FB55: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> >> ==10884==by 0x44A592: xcalloc (util.c:121)
> >> ==10884==by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41)
> >> ==10884==by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217)
> >> ==10884==by 0x416C6F: ovsdb_trigger_try (trigger.c:246)
> >> ==10884==by 0x40D4DE: ovsdb_jsonrpc_trigger_create
> (jsonrpc-server.c:1119)
> >> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_got_request
> (jsonrpc-server.c:986)
> >> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_run
> (jsonrpc-server.c:556)
> >> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_run_all
> (jsonrpc-server.c:586)
> >> ==10884==by 0x40D4DE: ovsdb_jsonrpc_server_run
> (jsonrpc-server.c:401)
> >> ==10884==by 0x406A6E: main_loop (ovsdb-server.c:209)
> >> ==10884==by 0x406A6E: main (ovsdb-server.c:460)
> >>
> >> 'new_schema' should also be freed when there is no error.
> >> This patch fixes it.
> >>
> >> Signed-off-by: Yifeng Sun 
> >> ---
> >>  ovsdb/trigger.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
> >> index 6f4ed96b000b..7e62e90ae381 100644
> >> --- a/ovsdb/trigger.c
> >> +++ b/ovsdb/trigger.c
> >> @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long
> long int now)
> >>  if (!error) {
> >>  error = ovsdb_convert(t->db, new_schema, );
> >>  }
> >> +ovsdb_schema_destroy(new_schema);
> >>  if (error) {
> >> -ovsdb_schema_destroy(new_schema);
> >>  trigger_convert_error(t, error);
> >>  return false;
> >>  }
> >> --
> >> 2.7.4
> >>
> >> ___
> >> 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 v5 2/9] ovs-vsctl: Add conntrack zone commands.

2019-09-13 Thread Justin Pettit


> On Sep 13, 2019, at 10:41 AM, William Tu  wrote:
> 
>> Is there a reason you limited this to 18 arguments and not use INT_MAX?
> 
> I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
> (dp_name, zone_id), so total is 18.
> I think using INT_MAX is fine, because at db schema, the value type is set.

That's a fair argument.  However, I'd suggest that we let ovs-vswitchd do that 
enforcement.  It's nice to decouple the configuration tool from the binary as 
much as possible in case people run different versions.

> I will merge your diff and send next version.

Sounds good.  I'm hopeful that we'll be able to get this version merged with 
minimal changes, though.

Thanks,

--Justin


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


Re: [ovs-dev] [PATCH v5 2/9] ovs-vsctl: Add conntrack zone commands.

2019-09-13 Thread William Tu
Hi Justin,

Thanks for your feedbacks.


On Thu, Sep 12, 2019 at 02:14:56PM -0700, Justin Pettit wrote:
> 
> > On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei  wrote:
> > 
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 7c09df79bd29..5b9883ae1c3d 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -353,6 +353,32 @@ list.
> > Prints the name of the bridge that contains \fIiface\fR on standard
> > output.
> > .
> > +.SS "Conntrack Zone Commands"
> > ...
> > +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> > +already exists is an error.  With \fB\-\-may\-exist\fR,
> > +this command does nothing if \fIzone_id\fR is already created\fR.
> 
> I don't think you need that final \fR.
> 
> I also made a few minor language tweaks in the icremental.
> 
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 4948137efe8c..76a708bd5069 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > 
> > +static struct ovsrec_ct_timeout_policy *
> > +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> > +{
> 
> 
> I think it's clearer if we switch "argv" to "tps", since the argument should 
> only be timeout policies.

OK

> 
> > +static void
> > +cmd_list_zone_tp(struct ctl_context *ctx)
> > +{
> > ...
> > +for (int j = 0; j < tp->n_timeouts; j++) {
> > +if (j == tp->n_timeouts - 1) {
> > +ds_put_format(>output, "%s=%"PRIu64"\n",
> > +  tp->key_timeouts[j], tp->value_timeouts[j]);
> > +} else {
> > +ds_put_format(>output, "%s=%"PRIu64" ",
> > +  tp->key_timeouts[j], tp->value_timeouts[j]);
> > +}
> 
> I think this can be done without the duplicated code with a ds_chomp() and 
> ds_put_char().  Let me know if you agree with the incremental patch at the 
> bottom.

OK, looks much better!

> 
> > static void
> > cmd_add_br(struct ctl_context *ctx)
> > {
> > @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax 
> > vsctl_commands[] = {
> > /* Switch commands. */
> > {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL, "", 
> > RW},
> > 
> > +/* Zone and CT Timeout Policy commands. */
> > +{"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
> > + "--may-exist", RW},
> 
> Is there a reason not to make the minimum arguments 3 instead of 2?  It seems 
> like it's required in the code.

OK, using 3 makes sure at least one policies is added.

> 
> Is there a reason you limited this to 18 arguments and not use INT_MAX?

I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
(dp_name, zone_id), so total is 18.
I think using INT_MAX is fine, because at db schema, the value type is set.

I will merge your diff and send next version.

Thanks
William

> 
> Let me know if you agree with the incremental.
> 
> Thanks,
> 
> --Justin
> 
>

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


[ovs-dev] [PATCH 3/3] flake8: also check the ovs-check-dead-ifs script

2019-09-13 Thread Aaron Conole
Signed-off-by: Aaron Conole 
---
 utilities/automake.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/utilities/automake.mk b/utilities/automake.mk
index c379596fd..58f743e04 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -156,6 +156,7 @@ endif
 
 FLAKE8_PYFILES += utilities/ovs-pcap.in \
utilities/checkpatch.py utilities/ovs-dev.py \
+   utilities/ovs-check-dead-ifs.in \
utilities/ovs-tcpdump.in \
utilities/ovs-pipegen.py
 
-- 
2.21.0

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


[ovs-dev] [PATCH 2/3] ovs-check-dead-ifs: unshadow pid variable

2019-09-13 Thread Aaron Conole
The pid variable is being shadowed by the list comprehension in the
os.execvp() call.  This can generate flakes / warnings in some environments
so fix it.

Signed-off-by: Aaron Conole 
---
 utilities/ovs-check-dead-ifs.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/ovs-check-dead-ifs.in b/utilities/ovs-check-dead-ifs.in
index f398a3401..73e4fd9e1 100755
--- a/utilities/ovs-check-dead-ifs.in
+++ b/utilities/ovs-check-dead-ifs.in
@@ -99,4 +99,4 @@ if bad_pids:
 The following processes are listening for packets to arrive on network devices
 that no longer exist. You may want to restart them.""")
 sys.stdout.flush()
-os.execvp("ps", ["ps"] + ["%s" % pid for pid in bad_pids])
+os.execvp("ps", ["ps"] + ["%s" % pspid for pspid in bad_pids])
-- 
2.21.0

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


[ovs-dev] [PATCH 1/3] ovs-check-dead-ifs: python3 print format

2019-09-13 Thread Aaron Conole
The print call changed in python3, so update it.

Signed-off-by: Aaron Conole 
---
 utilities/ovs-check-dead-ifs.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/utilities/ovs-check-dead-ifs.in b/utilities/ovs-check-dead-ifs.in
index ac54f6c9c..f398a3401 100755
--- a/utilities/ovs-check-dead-ifs.in
+++ b/utilities/ovs-check-dead-ifs.in
@@ -37,7 +37,7 @@ for ifname in os.listdir("/sys/class/net"):
 except IOError:
 pass
 except ValueError:
-print "%s: unexpected format\n" % fn
+print("%s: unexpected format\n" % fn)
 
 # Get inodes for all packet sockets whose ifindexes don't exist.
 invalid_inodes = set()
@@ -95,8 +95,8 @@ for pid in os.listdir("/proc"):
 bad_pids.add(pid)
 
 if bad_pids:
-print """
+print("""
 The following processes are listening for packets to arrive on network devices
-that no longer exist. You may want to restart them."""
+that no longer exist. You may want to restart them.""")
 sys.stdout.flush()
 os.execvp("ps", ["ps"] + ["%s" % pid for pid in bad_pids])
-- 
2.21.0

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


[ovs-dev] [PATCH] netdev-afxdp: Detect numa node id.

2019-09-13 Thread William Tu
The patch detects the numa node id from the name of the netdev,
by reading the '/sys/class/net//device/numa_node'.
If not available, ex: virtual device, or any error happens,
return numa id 0.

Signed-off-by: William Tu 
---
 lib/netdev-afxdp.c | 38 +-
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index e5b058d08a09..a94d77468cb7 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -552,11 +552,39 @@ out:
 int
 netdev_afxdp_get_numa_id(const struct netdev *netdev)
 {
-/* FIXME: Get netdev's PCIe device ID, then find
- * its NUMA node id.
- */
-VLOG_INFO("FIXME: Device %s always use numa id 0.",
-  netdev_get_name(netdev));
+const char *numa_node_path;
+long int node_id;
+char buffer[4];
+FILE *stream;
+int n;
+
+numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",
+   netdev_get_name(netdev));
+stream = fopen(numa_node_path, "r");
+if (!stream) {
+/* Virtual device does not have this info. */
+VLOG_WARN_RL(, "Open %s failed: %s, use numa_id 0",
+ numa_node_path, ovs_strerror(errno));
+return 0;
+}
+
+n = fread(buffer, 1, sizeof buffer, stream);
+if (!n) {
+goto error;
+}
+
+node_id = strtol(buffer, NULL, 10);
+if (node_id < 0 || node_id > 2) {
+goto error;
+}
+
+fclose(stream);
+return (int)node_id;
+
+error:
+VLOG_WARN_RL(, "Error detecting numa node of %s, use numa_id 0",
+ numa_node_path);
+fclose(stream);
 return 0;
 }
 
-- 
2.7.4

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


Re: [ovs-dev] NDP for ipv6 in OVS 2.10.0

2019-09-13 Thread Ben Pfaff
On Tue, Sep 10, 2019 at 09:08:59AM -0700, William Tu wrote:
> On Tue, Sep 10, 2019 at 9:06 AM Gregory Rose  wrote:
> >
> >
> > On 9/10/2019 7:02 AM, Thomas Goirand wrote:
> > > Hi Ben and others,
> > >
> > > I just want to know: does OVS 2.10 has support for IPv6 mac learning /
> > > NDP, so that we can avoid the ipv6 broadcast storm in our cloud?
> >
> > OVS will switch/forward IPv6 NDP traffic generated by network stations
> > with IPv6 enabled.
> > Is there some other support you're wondering about?
> 
> I'm checking the lib/mac-learning.c and ofproto/ofproto-dpif-xlate.c
> I think OVS only learns MAC-port mapping from ARP packet, not IPv6 ND.

MAC learning is an Ethernet function that happens on every packet that
passes through the "normal" action.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema

2019-09-13 Thread Yifeng Sun
Thanks Aginwala,

Could you please double check if 'json_destroy(result)' is necessary here?
If result != NULL, then it is passed in trigger_success(), which puts
result in 't->reply',
later, jsonrpc_msg_destroy() will free it.

Thanks,
Yifeng

On Thu, Sep 12, 2019 at 2:00 PM aginwala  wrote:
>
> One minor suggestion here:
> Can you also handle freeing result:
> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
> index 6f4ed96b0..0158957d6 100644
> --- a/ovsdb/trigger.c
> +++ b/ovsdb/trigger.c
> @@ -214,6 +214,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int 
> now)
>  /* Unsatisfied "wait" condition.  Take no action now, 
> retry
>   * later. */
>  }
> +json_destroy(result);
>  return false;
>  }
>
> Else I can handle that in separate patch. Else, acked by for the series.
> Acked-by: Aliasgar Ginwala 
>
> On Wed, Sep 11, 2019 at 2:19 PM Yifeng Sun  wrote:
>>
>> Valgrind reported:
>>
>> 1925: schema conversion online - standalone
>>
>> ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are definitely 
>> lost in loss record 384 of 420
>> ==10884==at 0x4C2FB55: calloc (in 
>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==10884==by 0x44A592: xcalloc (util.c:121)
>> ==10884==by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41)
>> ==10884==by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217)
>> ==10884==by 0x416C6F: ovsdb_trigger_try (trigger.c:246)
>> ==10884==by 0x40D4DE: ovsdb_jsonrpc_trigger_create 
>> (jsonrpc-server.c:1119)
>> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_got_request 
>> (jsonrpc-server.c:986)
>> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556)
>> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_run_all 
>> (jsonrpc-server.c:586)
>> ==10884==by 0x40D4DE: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401)
>> ==10884==by 0x406A6E: main_loop (ovsdb-server.c:209)
>> ==10884==by 0x406A6E: main (ovsdb-server.c:460)
>>
>> 'new_schema' should also be freed when there is no error.
>> This patch fixes it.
>>
>> Signed-off-by: Yifeng Sun 
>> ---
>>  ovsdb/trigger.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
>> index 6f4ed96b000b..7e62e90ae381 100644
>> --- a/ovsdb/trigger.c
>> +++ b/ovsdb/trigger.c
>> @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int 
>> now)
>>  if (!error) {
>>  error = ovsdb_convert(t->db, new_schema, );
>>  }
>> +ovsdb_schema_destroy(new_schema);
>>  if (error) {
>> -ovsdb_schema_destroy(new_schema);
>>  trigger_convert_error(t, error);
>>  return false;
>>  }
>> --
>> 2.7.4
>>
>> ___
>> 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


[ovs-dev] [PATCH ovn] gitignore: Add missing OVN entries.

2019-09-13 Thread Dumitru Ceara
Signed-off-by: Dumitru Ceara 
---
 .gitignore | 8 
 include/ovn/.gitignore | 2 ++
 utilities/.gitignore   | 1 +
 3 files changed, 11 insertions(+)
 create mode 100644 include/ovn/.gitignore

diff --git a/.gitignore b/.gitignore
index 2366fe5..6fee075 100644
--- a/.gitignore
+++ b/.gitignore
@@ -60,6 +60,13 @@
 /manpage-check
 /missing
 /missing-distfiles
+/ovn-architecture.7
+/ovn-nb.5
+/ovn-nb.gv
+/ovn-nb.pic
+/ovn-sb.5
+/ovn-sb.gv
+/ovn-sb.pic
 /package.m4
 /stamp-h1
 /_build-gcc
@@ -80,3 +87,4 @@ testsuite.tmp.orig
 /Documentation/_build
 /.venv
 /cxx-check
+/*.ovsschema.stamp
diff --git a/include/ovn/.gitignore b/include/ovn/.gitignore
new file mode 100644
index 000..ffd087a
--- /dev/null
+++ b/include/ovn/.gitignore
@@ -0,0 +1,2 @@
+/version.h
+
diff --git a/utilities/.gitignore b/utilities/.gitignore
index b319e83..faaa85e 100644
--- a/utilities/.gitignore
+++ b/utilities/.gitignore
@@ -11,3 +11,4 @@
 /ovn-detrace.1
 /ovn-docker-overlay-driver
 /ovn-docker-underlay-driver
+/ovn-lib
-- 
1.8.3.1

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


Re: [ovs-dev] [RFC ovn 3/3] northd: interoduce logical flow for localnet egress shaping

2019-09-13 Thread Dumitru Ceara
On Wed, Sep 11, 2019 at 7:22 PM Lorenzo Bianconi
 wrote:
>
> Add set_queue() action for qos capable localnet port in
> S_SWITCH_OUT_PORT_SEC_L2 stage of logical switch pipeline
>
> Signed-off-by: Lorenzo Bianconi 
> ---
>  northd/ovn-northd.8.xml |  7 ++-
>  northd/ovn-northd.c | 12 +++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index b34ef687a..d28f8965c 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1150,10 +1150,15 @@ output;
>eth.dst are always accepted instead of being subject to 
> the
>port security rules; this is implemented through a priority-100 flow 
> that
>matches on eth.mcast with action output;.
> -  Finally, to ensure that even broadcast and multicast packets are not
> +  Moreover, to ensure that even broadcast and multicast packets are not
>delivered to disabled logical ports, a priority-150 flow for each
>disabled logical outport overrides the priority-100 flow
>with a drop; action.
> +  Finally if egress qos has been enabled on a localnet port, the outgoing
> +  queue id is set through set_queue action. Please remember 
> to
> +  mark the corresponding physical interface with
> +  ovn-egress-iface set to true in  +  table="Interface" db="Open_vSwitch"/>
>  
>
>  Logical Router Datapaths
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 889eeb795..5bae035b3 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5681,10 +5681,20 @@ build_lswitch_flows(struct hmap *datapaths, struct 
> hmap *ports,
>  ds_clear();
>  ds_put_format(, "outport == %s", op->json_key);
>  if (lsp_is_enabled(op->nbsp)) {
> +ds_clear();
> +
> +if (!strcmp(op->nbsp->type, "localnet")) {
> +const char *queue_id = smap_get(>sb->options,
> +"qdisc_queue_id");
> +if (queue_id) {
> +ds_put_format(, "set_queue(%s); ", queue_id);
> +}
> +}

Hi Lorenzo,

Might be nice to refactor this and add a build_qos() function and also
call it in build_lswitch_flows().

Thanks,
Dumitru

> +ds_put_cstr(, "output;");
>  build_port_security_l2("eth.dst", op->ps_addrs, op->n_ps_addrs,
> );
>  ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 50,
> -  ds_cstr(), "output;");
> +  ds_cstr(), ds_cstr());
>  } else {
>  ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 150,
>ds_cstr(), "drop;");
> --
> 2.21.0
>
> ___
> 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] [RFC ovn 1/3] Add egress QoS mapping for non-tunnel interfaces

2019-09-13 Thread Dumitru Ceara
On Wed, Sep 11, 2019 at 7:21 PM Lorenzo Bianconi
 wrote:
>
> Introduce add_egress_interface_mappings routine in order to collect as
> egress interfaces all ovs bridge interfaces marked with ovn-egress-iface
> in the external_ids column of ovs interface table.
> ovn-egress-iface is used to indicate to which localnet ports QoS egress
> shaping has to be applied.
> Refactor add_bridge_mappings routine
>
> Signed-off-by: Lorenzo Bianconi 

Hi Lorenzo,

A few comments below.

Thanks,
Dumitru

> ---
>  controller/binding.c| 60 +
>  controller/binding.h|  4 ++
>  controller/ovn-controller.c |  3 +-
>  controller/patch.c  | 76 +
>  controller/patch.h  |  4 ++
>  5 files changed, 113 insertions(+), 34 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 47d4fea43..39fd79cd0 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -18,6 +18,7 @@
>  #include "ha-chassis.h"
>  #include "lflow.h"
>  #include "lport.h"
> +#include "patch.h"
>
>  #include "lib/bitmap.h"
>  #include "openvswitch/poll-loop.h"
> @@ -520,6 +521,9 @@ consider_local_datapath(struct ovsdb_idl_txn 
> *ovnsb_idl_txn,
>  } else if (!strcmp(binding_rec->type, "localnet")) {
>  /* Add all localnet ports to local_lports so that we allocate ct 
> zones
>   * for them. */
> +if (qos_map && ovs_idl_txn) {
> +get_qos_params(binding_rec, qos_map);
> +}

Can we move this before the comment above? Or update the comment?

>  sset_add(local_lports, binding_rec->logical_port);
>  } else if (!strcmp(binding_rec->type, "external")) {
>  if (ha_chassis_group_contains(binding_rec->ha_chassis_group,
> @@ -632,6 +636,57 @@ consider_localnet_port(const struct sbrec_port_binding 
> *binding_rec,
>  ld->localnet_port = binding_rec;
>  }
>
> +static void
> +add_egress_interface_mappings(const struct ovsrec_bridge_table *bridge_tb,
> +  const struct ovsrec_open_vswitch_table *ovs_tb,
> +  const struct sbrec_port_binding_table *pb_tb,
> +  struct sset *egress_ifaces)
> +{
> +struct shash bridge_mappings = SHASH_INITIALIZER(_mappings);
> +
> +add_ovs_bridge_mappings(ovs_tb, bridge_tb, _mappings);
> +
> +const struct sbrec_port_binding *binding;
> +SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, pb_tb) {
> +const char *network = smap_get(>options, "network_name");
> +if (!network) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_ERR_RL(, "%s port '%s' has no network name.",
> + binding->type, binding->logical_port);
> +continue;
> +}
> +struct ovsrec_bridge *br_ln = shash_find_data(_mappings,
> +  network);
> +if (!br_ln) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_ERR_RL(, "bridge not found for %s port '%s' "
> +"with network name '%s'",
> +binding->type, binding->logical_port, network);
> +continue;
> +}
> +
> +/* Add egress-ifaces from the connected bridge */
> +for (size_t i = 0; i < br_ln->n_ports; i++) {
> +const struct ovsrec_port *port_rec = br_ln->ports[i];
> +
> +for (size_t j = 0; j < port_rec->n_interfaces; j++) {
> +const struct ovsrec_interface *iface_rec;
> +
> +iface_rec = port_rec->interfaces[j];
> +bool is_egress_iface = 
> smap_get_bool(_rec->external_ids,
> + "ovn-egress-iface",
> + false);
> +if (!is_egress_iface) {
> +continue;
> +}
> +sset_add(egress_ifaces, iface_rec->name);
> +}
> +}
> +}
> +
> +shash_destroy(_mappings);
> +}
> +
>  void
>  binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  struct ovsdb_idl_txn *ovs_idl_txn,
> @@ -644,6 +699,8 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  const struct ovsrec_bridge *br_int,
>  const struct sbrec_chassis *chassis_rec,
>  const struct sset *active_tunnels,
> +const struct ovsrec_bridge_table *bridge_table,
> +const struct ovsrec_open_vswitch_table *ovs_table,
>  struct hmap *local_datapaths, struct sset *local_lports,
>  struct sset *local_lport_ids)
>  {
> @@ -656,6 +713,9 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  struct sset egress_ifaces = SSET_INITIALIZER(_ifaces);
>  struct hmap qos_map;
>
> +add_egress_interface_mappings(bridge_table, ovs_table, 
> port_binding_table,
> +

Re: [ovs-dev] [PATCH v6 0/1] Balance-tcp bond mode optimization

2019-09-13 Thread Matteo Croce
On Wed, Sep 11, 2019 at 8:37 PM Matteo Croce  wrote:
>
> On Tue, Sep 10, 2019 at 11:53 AM Vishal Deep Ajmera
>  wrote:
> >
> > v5->v6:
> >  Addressed comments from Ilya Maximets.
> >  https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362001.html
> >  Rebased to OVS master.
> >
> > v4->v5:
> >  Support for stats per hash bucket.
> >  Support for dynamic load balancing.
> >  Rebased to OVS Master.
> >
> > v3->v4:
> >  Addressed Ilya Maximets comments.
> >  https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360452.html
> >
> > v2->v3:
> >  Rebased to OVS master.
> >  Fixed git merge issue.
> >
> > v1->v2:
> >  Updated datapath action to hash + lb-output.
> >  Updated throughput test observations.
> >  Rebased to OVS master.
> >
> > Vishal Deep Ajmera (1):
> >   Avoid dp_hash recirculation for balance-tcp bond selection mode
> >
> >  datapath/linux/compat/include/linux/openvswitch.h |   2 +
> >  lib/dpif-netdev.c | 515 
> > --
> >  lib/dpif-netlink.c|   3 +
> >  lib/dpif-provider.h   |   8 +
> >  lib/dpif.c|  48 ++
> >  lib/dpif.h|   7 +
> >  lib/odp-execute.c |   2 +
> >  lib/odp-util.c|   4 +
> >  ofproto/bond.c|  52 ++-
> >  ofproto/bond.h|   9 +
> >  ofproto/ofproto-dpif-ipfix.c  |   1 +
> >  ofproto/ofproto-dpif-sflow.c  |   1 +
> >  ofproto/ofproto-dpif-xlate.c  |  39 +-
> >  ofproto/ofproto-dpif.c|  32 ++
> >  ofproto/ofproto-dpif.h|  12 +-
> >  tests/lacp.at |   9 +
> >  vswitchd/bridge.c |   4 +
> >  vswitchd/vswitch.xml  |  10 +
> >  18 files changed, 698 insertions(+), 60 deletions(-)
> >
> > --
> > 1.9.1
> >
>
> Hi,
>
> I confirm a decent performance improvement with DPDK and balance-tcp bonding:
>
> lb-output-action=false
>
> rx: 740 Mbps 1446 kpps
>
> lb-output-action=true
>
> rx: 860 Mbps 1680 kpps
>
> I'm running a very simple test with a tweaked version of testpmd which
> generates 256 L4 flows, I guess that with much flows the improvement
> is way higher.
>
> Tested-by: Matteo Croce 
>
> --
> Matteo Croce
> per aspera ad upstream

Hi,

I found an issue with the patch. It's not 100% reproducible, but
sometimes the option gets enabled regardless of the bonding type and
the configuration.
This breaks the unit tests, as the bond/show output is wrong:

# ovs-vsctl add-br br0
[63129.589500] device ovs-system entered promiscuous mode
[63129.591802] device br0 entered promiscuous mode

# ovs-vsctl add-bond br0 bond dummy0 dummy1 -- set Port bond
lacp=active bond-mode=active-backup
[63150.700542] device dummy1 entered promiscuous mode
[63150.700892] device dummy0 entered promiscuous mode

# ovs-vsctl get port bond other_config
{}

# ovs-appctl bond/show
 bond 
bond_mode: active-backup
bond may use recirculation: no, Recirc-ID : -1
bond-hash-basis: 0
lb-output-action: enabled
updelay: 0 ms
downdelay: 0 ms
lacp_status: negotiated
lacp_fallback_ab: false
active slave mac: 00:00:00:00:00:00(none)

slave dummy0: disabled
may_enable: false

slave dummy1: disabled
may_enable: false

Regards,
-- 
Matteo Croce
per aspera ad upstream
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] net: openvswitch: free vport unless register_netdevice() succeeds

2019-09-13 Thread Stefano Brivio
On Sat, 10 Aug 2019 00:34:55 -0700
Pravin Shelar  wrote:

> On Thu, Aug 8, 2019 at 8:55 PM Hillf Danton  wrote:
> >
> >
> > syzbot found the following crash on:
> >
> > HEAD commit:1e78030e Merge tag 'mmc-v5.3-rc1' of git://git.kernel.org/..
> > git tree:   upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=148d3d1a60
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=30cef20daf3e9977
> > dashboard link: https://syzkaller.appspot.com/bug?extid=13210896153522fe1ee5
> > compiler:   gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=136aa8c460
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=109ba79260
> >
> > =
> > BUG: memory leak
> > unreferenced object 0x8881207e4100 (size 128):
> >comm "syz-executor032", pid 7014, jiffies 4294944027 (age 13.830s)
> >hex dump (first 32 bytes):
> >  00 70 16 18 81 88 ff ff 80 af 8c 22 81 88 ff ff  .p."
> >  00 b6 23 17 81 88 ff ff 00 00 00 00 00 00 00 00  ..#.
> >backtrace:
> >  [<0eb78212>] kmemleak_alloc_recursive  
> > include/linux/kmemleak.h:43 [inline]
> >  [<0eb78212>] slab_post_alloc_hook mm/slab.h:522 [inline]
> >  [<0eb78212>] slab_alloc mm/slab.c:3319 [inline]
> >  [<0eb78212>] kmem_cache_alloc_trace+0x145/0x2c0 mm/slab.c:3548
> >  [<006ea6c6>] kmalloc include/linux/slab.h:552 [inline]
> >  [<006ea6c6>] kzalloc include/linux/slab.h:748 [inline]
> >  [<006ea6c6>] ovs_vport_alloc+0x37/0xf0  
> > net/openvswitch/vport.c:130
> >  [] internal_dev_create+0x24/0x1d0  
> > net/openvswitch/vport-internal_dev.c:164
> >  [<56ee7c13>] ovs_vport_add+0x81/0x190  
> > net/openvswitch/vport.c:199
> >  [<5434efc7>] new_vport+0x19/0x80 net/openvswitch/datapath.c:194
> >  [] ovs_dp_cmd_new+0x22f/0x410  
> > net/openvswitch/datapath.c:1614
> >  [] genl_family_rcv_msg+0x2ab/0x5b0  
> > net/netlink/genetlink.c:629
> >  [] genl_rcv_msg+0x54/0x9c net/netlink/genetlink.c:654
> >  [<6694b647>] netlink_rcv_skb+0x61/0x170  
> > net/netlink/af_netlink.c:2477
> >  [<88381f37>] genl_rcv+0x29/0x40 net/netlink/genetlink.c:665
> >  [] netlink_unicast_kernel  
> > net/netlink/af_netlink.c:1302 [inline]
> >  [] netlink_unicast+0x1ec/0x2d0  
> > net/netlink/af_netlink.c:1328
> >  [<67e6b079>] netlink_sendmsg+0x270/0x480  
> > net/netlink/af_netlink.c:1917
> >  [] sock_sendmsg_nosec net/socket.c:637 [inline]
> >  [] sock_sendmsg+0x54/0x70 net/socket.c:657
> >  [<4cb7c11d>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2311
> >  [] __sys_sendmsg+0x80/0xf0 net/socket.c:2356
> >  [] __do_sys_sendmsg net/socket.c:2365 [inline]
> >  [] __se_sys_sendmsg net/socket.c:2363 [inline]
> >  [] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2363
> >
> > BUG: memory leak
> > unreferenced object 0x88811723b600 (size 64):
> >comm "syz-executor032", pid 7014, jiffies 4294944027 (age 13.830s)
> >hex dump (first 32 bytes):
> >  01 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  
> >  00 00 00 00 00 00 00 00 02 00 00 00 05 35 82 c1  .5..
> >backtrace:
> >  [<352f46d8>] kmemleak_alloc_recursive  
> > include/linux/kmemleak.h:43 [inline]
> >  [<352f46d8>] slab_post_alloc_hook mm/slab.h:522 [inline]
> >  [<352f46d8>] slab_alloc mm/slab.c:3319 [inline]
> >  [<352f46d8>] __do_kmalloc mm/slab.c:3653 [inline]
> >  [<352f46d8>] __kmalloc+0x169/0x300 mm/slab.c:3664
> >  [<8e48f3d1>] kmalloc include/linux/slab.h:557 [inline]
> >  [<8e48f3d1>] ovs_vport_set_upcall_portids+0x54/0xd0  
> > net/openvswitch/vport.c:343
> >  [<541e4f4a>] ovs_vport_alloc+0x7f/0xf0  
> > net/openvswitch/vport.c:139
> >  [] internal_dev_create+0x24/0x1d0  
> > net/openvswitch/vport-internal_dev.c:164
> >  [<56ee7c13>] ovs_vport_add+0x81/0x190  
> > net/openvswitch/vport.c:199
> >  [<5434efc7>] new_vport+0x19/0x80 net/openvswitch/datapath.c:194
> >  [] ovs_dp_cmd_new+0x22f/0x410  
> > net/openvswitch/datapath.c:1614
> >  [] genl_family_rcv_msg+0x2ab/0x5b0  
> > net/netlink/genetlink.c:629
> >  [] genl_rcv_msg+0x54/0x9c net/netlink/genetlink.c:654
> >  [<6694b647>] netlink_rcv_skb+0x61/0x170  
> > net/netlink/af_netlink.c:2477
> >  [<88381f37>] genl_rcv+0x29/0x40 net/netlink/genetlink.c:665
> >  [] netlink_unicast_kernel  
>