[ovs-dev] vport-gtp: add metadata_dst support result in "netlink: Unexpected mask"

[Jiannan Ouyang]

Hi,

I’m experimenting with attaching metadata_dst to skb in the gtp recv path. The 
goal is to let ovs to meter the gtp traffic. However, I encountered the 
following error in ovs_flow_cmd_new
:
[ 411.462443] openvswitch: netlink: Unexpected mask (mask=110058, 
allowed=41980cc)
Seems the OVS_KEY_ATTR_ETHERNET bit is not allowed.

Why does the error happen?

My setup is as following,

libgtpnl:
modprobe gtp
./gtp-link add gtp0
./gtp-tunnel add gtp0 v1 999 123 1.1.1.1 192.168.60.143

ovs dev branch:
ovs-vsctl add-br br0
ifconfig br0 1.1.1.122/24 up
ovs-vsctl add-port br0 gtp0


Linux 4.9 drivers/net/gtp.c:
@@ -270,12 +274,33 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, 
struct sk_buff *skb,
goto out_rcu;
}
rcu_read_unlock();
+*/
+if (ip_tunnel_collect_metadata() || true) { // always collect metadata
+__be16 flags;
+   struct metadata_dst *tun_dst;
+
+flags = TUNNEL_KEY;
+
+tun_dst = udp_tun_rx_dst(skb, gtp->sock1u->sk->sk_family, 
flags,
+ ntohl(gtp1->tid), 0);
+if (!tun_dst)
+goto drop;
+
+   netdev_info(gtp->dev, "Attaching tun_dst to skb...");
+   skb_dst_set(skb, (struct dst_entry *)tun_dst);
+} else {
+   netdev_err(gtp->dev, "Not collecting metadata");
+}


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

Re: [ovs-dev] [PATCH] bridge: Prohibit "default" bridge name.

[Ben Pfaff]

On Thu, Apr 27, 2017 at 02:47:03PM -0700, William Tu wrote:
> On Thu, Apr 27, 2017 at 11:17 AM, Ben Pfaff  wrote:
> > On Thu, Apr 27, 2017 at 10:17:20AM -0700, William Tu wrote:
> >> On Thu, Apr 27, 2017 at 9:57 AM, Ben Pfaff  wrote:
> >> > On Thu, Apr 27, 2017 at 04:02:10AM -0700, William Tu wrote:
> >> >> Before the patch, when users create bridge named "default", although
> >> >> ovs-vsctl fails but vswitchd in the background will keep retrying it,
> >> >> causing the systemd-udev to reach 100% cpu utilization.  The reason is
> >> >> due to frequent calls into kernel's register_netdevice function,
> >> >> which will invoke several kernel elements who has registered on the
> >> >> netdevice notifier chain.  One of the notifier, the inetdev_event 
> >> >> rejects
> >> >> this devname and register_netdevice fails.  The patch prohibits creating
> >> >> "default" bridge name.
> >> >>
> >> >> VMWare-BZ: #1842388
> >> >> Signed-off-by: William Tu 
> >> >
> >> > It all seems very arbitrary. Do we understand why this is an invalid
> >> > device name?
> >>
> >> Yes, when kernel is configured with CONFIG_SYSCTL, creating a new
> >> netdev creates a dir in /proc/sys/net/ipv4/conf/
> >>
> >> The  "default" and "all" is pre-existed when SYSCTL
> >> starts (which means we should also prohibit "all") for default
> >> property of the system's netdev. So sysctl prevents creating dev->name
> >> is "default" or "all". A call stack is below if interested:
> >> sysctl_dev_name_is_allowed
> >>devinet_sysctl_register
> >>  inetdev_event
> >>notifier_call_chain
> >>  raw_notifier_call_chain
> >>call_netdevice_notifiers_info
> >>  register_netdevice
> >
> > Do we get the same behavior (100% CPU) if creating a bridge fails for
> > other reasons?  For example, it might fail because a network device
> > already exists with the given name, or because the name is too long for
> > a network device name.  If we do get 100% CPU from such a failure, then
> > we should fix the root of the problem instead of blacklisting particular
> > names.
> 
> There are two scenarios:
> 1) if the bridge name exists, ex: eth0
> then "ovs-vsctl add-br eth0" fails due to EEXIST
> 2) if the bridge name does not exists, but is "default" or "all"
> then "ovs-vsctl add-br default" fails due to EINVAL
> 
> From OVS's point of view it's the same, ovs-vsctl fails creating the
> bridge, and keeps retry. From the whole system's point of view, (2)
> has impact on other Linux subsystems, due to kernel netdev notifier
> chain mechanism informing other subsystems when trying to add a new
> device, while (1) fails pretty early in register_netdevice() and has
> no impact.

OK, if I understand properly here, the problem is that asking the Linux
kernel datapath to add a bridge named "default" does two things.  First,
it fails.  Second, it triggers a notifier that causes OVS to try again.
The second part is what makes this different from other failures and
what makes OVS use 100% CPU in this case but not other cases.  Is that
right?

If I'm right, then how about something like the following instead?  It
pushes this down to Linux-specific code, which seems better because this
is a Linux-specific issue, and it thoroughly documents the problem.  It
compiles but I have not tested it.

Thanks,

Ben.

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 9ff1333f8e85..79e827303d07 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, 
Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -773,10 +773,28 @@ netdev_linux_alloc(void)
 return >up;
 }
 
-static void
-netdev_linux_common_construct(struct netdev_linux *netdev)
-{
+static int
+netdev_linux_common_construct(struct netdev *netdev_)
+{
+/* Prevent any attempt to create (or open) a network device named "default"
+ * or "all".  These device names are effectively reserved on Linux because
+ * /proc/sys/net/ipv4/conf/ always contains directories by these names.  By
+ * itself this wouldn't call for any special treatment, but in practice if
+ * a program tries to create devices with these names, it causes the kernel
+ * to fire a "new device" notification event even though creation failed,
+ * and in turn that causes OVS to wake up and try to create them again,
+ * which ends up as a 100% CPU loop. */
+struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+const char *name = netdev_->name;
+if (!strcmp(name, "default") || !strcmp(name, "all")) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(, "%s: Linux forbids network device with this name",
+ 

Re: [ovs-dev] [PATCH] bridge: Prohibit "default" bridge name.

[Greg Rose]

On Thu, 2017-04-27 at 14:47 -0700, William Tu wrote:
> On Thu, Apr 27, 2017 at 11:17 AM, Ben Pfaff  wrote:
> > On Thu, Apr 27, 2017 at 10:17:20AM -0700, William Tu wrote:
> >> On Thu, Apr 27, 2017 at 9:57 AM, Ben Pfaff  wrote:
> >> > On Thu, Apr 27, 2017 at 04:02:10AM -0700, William Tu wrote:
> >> >> Before the patch, when users create bridge named "default", although
> >> >> ovs-vsctl fails but vswitchd in the background will keep retrying it,
> >> >> causing the systemd-udev to reach 100% cpu utilization.  The reason is
> >> >> due to frequent calls into kernel's register_netdevice function,
> >> >> which will invoke several kernel elements who has registered on the
> >> >> netdevice notifier chain.  One of the notifier, the inetdev_event 
> >> >> rejects
> >> >> this devname and register_netdevice fails.  The patch prohibits creating
> >> >> "default" bridge name.
> >> >>
> >> >> VMWare-BZ: #1842388
> >> >> Signed-off-by: William Tu 
> >> >
> >> > It all seems very arbitrary. Do we understand why this is an invalid
> >> > device name?
> >>
> >> Yes, when kernel is configured with CONFIG_SYSCTL, creating a new
> >> netdev creates a dir in /proc/sys/net/ipv4/conf/
> >>
> >> The  "default" and "all" is pre-existed when SYSCTL
> >> starts (which means we should also prohibit "all") for default
> >> property of the system's netdev. So sysctl prevents creating dev->name
> >> is "default" or "all". A call stack is below if interested:
> >> sysctl_dev_name_is_allowed
> >>devinet_sysctl_register
> >>  inetdev_event
> >>notifier_call_chain
> >>  raw_notifier_call_chain
> >>call_netdevice_notifiers_info
> >>  register_netdevice
> >
> > Do we get the same behavior (100% CPU) if creating a bridge fails for
> > other reasons?  For example, it might fail because a network device
> > already exists with the given name, or because the name is too long for
> > a network device name.  If we do get 100% CPU from such a failure, then
> > we should fix the root of the problem instead of blacklisting particular
> > names.
> 
> There are two scenarios:
> 1) if the bridge name exists, ex: eth0
> then "ovs-vsctl add-br eth0" fails due to EEXIST
> 2) if the bridge name does not exists, but is "default" or "all"
> then "ovs-vsctl add-br default" fails due to EINVAL
> 
> From OVS's point of view it's the same, ovs-vsctl fails creating the
> bridge, and keeps retry. From the whole system's point of view, (2)
> has impact on other Linux subsystems, due to kernel netdev notifier
> chain mechanism informing other subsystems when trying to add a new
> device, while (1) fails pretty early in register_netdevice() and has
> no impact.
> 
> Or instead of blacklisting, maybe add a max retry count?

Can you see if using a retry count still ensures this bug is fixed?

VMWare-BZ: #1842388

If so that's probably a better approach. Like Ben I'm a little queasy
about saying we can't have a 'default' bridge under any circumstance.

Thanks,

- Greg

> 
> Regards,
> William
> ___
> 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 3/4] compat: Fix build error in kernels 4.10+

[Guoshuai Li]

Good. Better than me, learning.


This is an alternative solution patch for the issue reported by
Raymond Burkholder and the patch submitted by Guoshuai Li.  It uses
the acinclude.m4 configuration file to check for the net parameter
that was added  to the ipv4 and ipv6 frags init functions in the 4.10
Linux kernel to check whether DEFRAG_ENABLE_TAKES_NET should be
set and then checks for that at compile time.

V2 - Incorporate suggestion from Joe Stringer to just return values
  of the init functions.

Reported-by: Raymond Burkholder 
CC: Guoshuai Li 
Signed-off-by: Greg Rose 
---
  datapath/linux/compat/ip_fragment.c| 12 
  datapath/linux/compat/nf_conntrack_reasm.c | 12 
  2 files changed, 24 insertions(+)

diff --git a/datapath/linux/compat/ip_fragment.c 
b/datapath/linux/compat/ip_fragment.c
index b0f5d0e..47b51b5 100644
--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -729,18 +729,30 @@ int rpl_ip_defrag(struct net *net, struct sk_buff *skb, 
u32 user)
return -ENOMEM;
  }
  
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET

+static int __net_init ipv4_frags_init_net(struct net *net)
+{
+   return nf_defrag_ipv4_enable(net);
+}
+#endif
+
  static void __net_exit ipv4_frags_exit_net(struct net *net)
  {
inet_frags_exit_net(>ipv4.frags, _frags);
  }
  
  static struct pernet_operations ip4_frags_ops = {

+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+   .init = ipv4_frags_init_net,
+#endif
.exit = ipv4_frags_exit_net,
  };
  
  int __init rpl_ipfrag_init(void)

  {
+#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
nf_defrag_ipv4_enable();
+#endif
register_pernet_subsys(_frags_ops);
ip4_frags.hashfn = ip4_hashfn;
ip4_frags.constructor = ip4_frag_init;
diff --git a/datapath/linux/compat/nf_conntrack_reasm.c 
b/datapath/linux/compat/nf_conntrack_reasm.c
index 0bc4d9e..0da9463 100644
--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -558,12 +558,22 @@ out_unlock:
return ret;
  }
  
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET

+static int nf_ct_net_init(struct net *net)
+{
+   return nf_defrag_ipv6_enable(net);
+}
+#endif
+
  static void nf_ct_net_exit(struct net *net)
  {
inet_frags_exit_net(>nf_frag.frags, _frags);
  }
  
  static struct pernet_operations nf_ct_net_ops = {

+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+   .init = nf_ct_net_init,
+#endif
.exit = nf_ct_net_exit,
  };
  
@@ -571,7 +581,9 @@ int rpl_nf_ct_frag6_init(void)

  {
int ret = 0;
  
+#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET

nf_defrag_ipv6_enable();
+#endif
nf_frags.hashfn = nf_hashfn;
nf_frags.constructor = ip6_frag_init;
nf_frags.destructor = NULL;


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

Re: [ovs-dev] [PATCH branch-2.6] travis: Fix path of DPDK directory

[Joe Stringer]

On 27 April 2017 at 03:18, Timothy Redaelli  wrote:
> Call configure with --with-dpdk=./dpdk-stable-$DPDK_VER/build
> instead of --with-dpdk=./dpdk-$DPDK_VER/build
>
> Fixes: c007c58af492 ("docs: Use DPDK 16.07.2 stable release")
> CC: Ian Stokes 
> Tested-at: https://travis-ci.org/drizzt/ovs/builds/226330348
> Signed-off-by: Timothy Redaelli 
> ---

Thanks, applied to branch-2.6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: ICMP related to original direction test.

[Jarno Rajahalme]

> On Apr 27, 2017, at 4:55 PM, Joe Stringer  wrote:
> 
> On 10 March 2017 at 16:10, Jarno Rajahalme  wrote:
>> Normally ICMPP responses are in the reply direction of a conntrack
> 
> 's/ICMPP/ICMP/'
> 
>> entry.  This test exercises an ICMP response to the original direction
>> of the conntrack entry.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
> 
> Somehow this slipped past my radar. Might want to roll in the
> incremental I posted below, but otherwise LGTM.
> 
> Acked-by: Joe Stringer 
> 

Thanks for the review!

Applied to master,

  Jarno

> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 62eb7bda7e31..f66ed10501a5 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1392,7 +1392,7 @@ priority=1,action=drop
> table=1,ip,action=ct(zone=34673,table=2)
> table=2,in_port=2,udp,action=ct(commit,zone=34673),1
> table=2,in_port=1,udp,action=ct(commit,zone=34673),2
> -table=2,in_port=2,icmp,action=1
> +table=2,in_port=2,ct_state=+rel,icmp,action=1
> ])
> 
> AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> @@ -1421,7 +1421,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip
> | sort | grep -v drop], [0], [d
> table=1, n_packets=4, n_bytes=224, ip actions=ct(table=2,zone=34673)
> table=2, n_packets=1, n_bytes=42, udp,in_port=1
> actions=ct(commit,zone=34673),output:2
> table=2, n_packets=1, n_bytes=42, udp,in_port=2
> actions=ct(commit,zone=34673),output:1
> - table=2, n_packets=2, n_bytes=140, icmp,in_port=2 actions=output:1
> + table=2, n_packets=2, n_bytes=140, ct_state=+rel,icmp,in_port=2
> actions=output:1
> NXST_FLOW reply:
> ])

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

[ovs-dev] [PATCH v5] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Jarno Rajahalme]

Specify the event mask with CT commit including bits for CT features
exposed at the OVS interface (mark and label changes in addition to
basic creation and destruction of conntrack entries).

Without this any listener of conntrack update events will typically
(depending on system configuration) receive events for each L4 (e.g.,
TCP) state machine change, which can multiply the number of events
received per connection.

By including the new, related, and destroy events any listener of new
conntrack events gets notified of new related and non-related
connections, and any listener of destroy events will get notified of
deleted (typically timed out) conntrack entries.

By including the flags for mark and labels, any listener of conntrack
update events gets notified whenever the connmark or conntrack labels
are changed from the values reported within the new events.

VMware-BZ: #1837218
Signed-off-by: Jarno Rajahalme 
Acked-by: Joe Stringer 
---
v5: Use addresses in the loopback range and zero port numbers in the probe
to avoid hitting any real conntrack entries.

build-aux/extract-odp-netlink-h |  2 ++
 ofproto/ofproto-dpif-xlate.c|  6 
 ofproto/ofproto-dpif.c  | 63 +
 ofproto/ofproto-dpif.h  |  5 +++-
 4 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/build-aux/extract-odp-netlink-h b/build-aux/extract-odp-netlink-h
index 907a70a..7fb6ce8 100755
--- a/build-aux/extract-odp-netlink-h
+++ b/build-aux/extract-odp-netlink-h
@@ -19,6 +19,8 @@ $i\
 #ifdef _WIN32\
 #include "OvsDpInterfaceExt.h"\
 #include "OvsDpInterfaceCtExt.h"\
+#else\
+#include "linux/netfilter/nf_conntrack_common.h"\
 #endif\
 
 # Use OVS's own struct eth_addr instead of a 6-byte char array.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d8c6a7c..ab5eef8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5351,6 +5351,12 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct 
ofpact_conntrack *ofc)
 if (ofc->flags & NX_CT_F_COMMIT) {
 nl_msg_put_flag(ctx->odp_actions, ofc->flags & NX_CT_F_FORCE ?
 OVS_CT_ATTR_FORCE_COMMIT : OVS_CT_ATTR_COMMIT);
+if (ctx->xbridge->support.ct_eventmask) {
+nl_msg_put_u32(ctx->odp_actions, OVS_CT_ATTR_EVENTMASK,
+   1 << IPCT_NEW | 1 << IPCT_RELATED |
+   1 << IPCT_DESTROY | 1 << IPCT_MARK |
+   1 << IPCT_LABEL);
+}
 }
 nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
 put_ct_mark(>xin->flow, ctx->odp_actions, ctx->wc);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c73c273..23896f0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1241,6 +1241,68 @@ check_clone(struct dpif_backer *backer)
 return !error;
 }
 
+/* Tests whether 'backer''s datapath supports the OVS_CT_ATTR_EVENTMASK
+ * attribute in OVS_ACTION_ATTR_CT. */
+static bool
+check_ct_eventmask(struct dpif_backer *backer)
+{
+struct dpif_execute execute;
+struct dp_packet packet;
+struct ofpbuf actions;
+struct flow flow = {
+.dl_type = CONSTANT_HTONS(ETH_TYPE_IP),
+.nw_proto = IPPROTO_UDP,
+.nw_ttl = 64,
+/* Use the broadcast address on the loopback address range 127/8 to
+ * avoid hitting any real conntrack entries.  We leave the UDP ports to
+ * zeroes for the same purpose. */
+.nw_src = CONSTANT_HTONL(0x7fff),
+.nw_dst = CONSTANT_HTONL(0x7fff),
+};
+size_t ct_start;
+int error;
+
+/* Compose CT action with eventmask attribute and check if datapath can
+ * decode the message.  */
+ofpbuf_init(, 64);
+ct_start = nl_msg_start_nested(, OVS_ACTION_ATTR_CT);
+/* Eventmask has no effect without the commit flag, but currently the
+ * datapath will accept an eventmask even without commit.  This is useful
+ * as we do not want to persist the probe connection in the conntrack
+ * table. */
+nl_msg_put_u32(, OVS_CT_ATTR_EVENTMASK, ~0);
+nl_msg_end_nested(, ct_start);
+
+/* Compose a dummy UDP packet. */
+dp_packet_init(, 0);
+flow_compose(, );
+
+/* Execute the actions.  On older datapaths this fails with EINVAL, on
+ * newer datapaths it succeeds. */
+execute.actions = actions.data;
+execute.actions_len = actions.size;
+execute.packet = 
+execute.flow = 
+execute.needs_help = false;
+execute.probe = true;
+execute.mtu = 0;
+
+error = dpif_execute(backer->dpif, );
+
+dp_packet_uninit();
+ofpbuf_uninit();
+
+if (error) {
+VLOG_INFO("%s: Datapath does not support eventmask in conntrack 
action",
+  dpif_name(backer->dpif));
+} else {
+VLOG_INFO("%s: Datapath supports eventmask in conntrack action",
+  dpif_name(backer->dpif));
+}
+
+

Re: [ovs-dev] [PATCH] tests: ICMP related to original direction test.

[Joe Stringer]

On 10 March 2017 at 16:10, Jarno Rajahalme  wrote:
> Normally ICMPP responses are in the reply direction of a conntrack

's/ICMPP/ICMP/'

> entry.  This test exercises an ICMP response to the original direction
> of the conntrack entry.
>
> Signed-off-by: Jarno Rajahalme 
> ---

Somehow this slipped past my radar. Might want to roll in the
incremental I posted below, but otherwise LGTM.

Acked-by: Joe Stringer 

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 62eb7bda7e31..f66ed10501a5 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1392,7 +1392,7 @@ priority=1,action=drop
table=1,ip,action=ct(zone=34673,table=2)
table=2,in_port=2,udp,action=ct(commit,zone=34673),1
table=2,in_port=1,udp,action=ct(commit,zone=34673),2
-table=2,in_port=2,icmp,action=1
+table=2,in_port=2,ct_state=+rel,icmp,action=1
])

AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
@@ -1421,7 +1421,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip
| sort | grep -v drop], [0], [d
 table=1, n_packets=4, n_bytes=224, ip actions=ct(table=2,zone=34673)
 table=2, n_packets=1, n_bytes=42, udp,in_port=1
actions=ct(commit,zone=34673),output:2
 table=2, n_packets=1, n_bytes=42, udp,in_port=2
actions=ct(commit,zone=34673),output:1
- table=2, n_packets=2, n_bytes=140, icmp,in_port=2 actions=output:1
+ table=2, n_packets=2, n_bytes=140, ct_state=+rel,icmp,in_port=2
actions=output:1
NXST_FLOW reply:
])
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Jarno Rajahalme]

> On Apr 27, 2017, at 4:15 PM, Joe Stringer  wrote:
> 
> On 27 April 2017 at 14:48, Jarno Rajahalme  > wrote:
>> Specify the event mask with CT commit including bits for CT features
>> exposed at the OVS interface (mark and label changes in addition to
>> basic creation and destruction of conntrack entries).
>> 
>> Without this any listener of conntrack update events will typically
>> (depending on system configuration) receive events for each L4 (e.g.,
>> TCP) state machine change, which can multiply the number of events
>> received per connection.
>> 
>> By including the new, related, and destroy events any listener of new
>> conntrack events gets notified of new related and non-related
>> connections, and any listener of destroy events will get notified of
>> deleted (typically timed out) conntrack entries.
>> 
>> By including the flags for mark and labels, any listener of conntrack
>> update events gets notified whenever the connmark or conntrack labels
>> are changed from the values reported within the new events.
>> 
>> VMware-BZ: #1837218
>> Signed-off-by: Jarno Rajahalme 
>> Acked-by: Joe Stringer 
>> ---
> 
> Did you try building with sparse? I'm seeing:
> 
> ofproto/ofproto-dpif.c:1256:19: error: incorrect type in initializer
> (different base types)
> ofproto/ofproto-dpif.c:1256:19: expected restricted ovs_be32 [usertype] nw_src
> ofproto/ofproto-dpif.c:1256:19: got int
> ofproto/ofproto-dpif.c:1257:19: error: incorrect type in initializer
> (different base types)
> ofproto/ofproto-dpif.c:1257:19: expected restricted ovs_be32 [usertype] nw_dst
> ofproto/ofproto-dpif.c:1257:19: got int
> ofproto/ofproto-dpif.c:1258:19: error: incorrect type in initializer
> (different base types)
> ofproto/ofproto-dpif.c:1258:19: expected restricted ovs_be16 [usertype] tp_src
> ofproto/ofproto-dpif.c:1258:19: got int
> ofproto/ofproto-dpif.c:1259:19: error: incorrect type in initializer
> (different base types)
> ofproto/ofproto-dpif.c:1259:19: expected restricted ovs_be16 [usertype] tp_dst
> ofproto/ofproto-dpif.c:1259:19: got int

Thanks for reporting this! I’ve been unlucky getting sparse to not give me a 
ton of meaningless errors…

Posted a v4 fixing this,

  Jarno

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

[ovs-dev] [PATCH v4] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Jarno Rajahalme]

Specify the event mask with CT commit including bits for CT features
exposed at the OVS interface (mark and label changes in addition to
basic creation and destruction of conntrack entries).

Without this any listener of conntrack update events will typically
(depending on system configuration) receive events for each L4 (e.g.,
TCP) state machine change, which can multiply the number of events
received per connection.

By including the new, related, and destroy events any listener of new
conntrack events gets notified of new related and non-related
connections, and any listener of destroy events will get notified of
deleted (typically timed out) conntrack entries.

By including the flags for mark and labels, any listener of conntrack
update events gets notified whenever the connmark or conntrack labels
are changed from the values reported within the new events.

VMware-BZ: #1837218
Signed-off-by: Jarno Rajahalme 
Acked-by: Joe Stringer 
---
v4: Added the missing byte swap operations.

build-aux/extract-odp-netlink-h |  2 ++
 ofproto/ofproto-dpif-xlate.c|  6 
 ofproto/ofproto-dpif.c  | 62 +
 ofproto/ofproto-dpif.h  |  5 +++-
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/build-aux/extract-odp-netlink-h b/build-aux/extract-odp-netlink-h
index 907a70a..7fb6ce8 100755
--- a/build-aux/extract-odp-netlink-h
+++ b/build-aux/extract-odp-netlink-h
@@ -19,6 +19,8 @@ $i\
 #ifdef _WIN32\
 #include "OvsDpInterfaceExt.h"\
 #include "OvsDpInterfaceCtExt.h"\
+#else\
+#include "linux/netfilter/nf_conntrack_common.h"\
 #endif\
 
 # Use OVS's own struct eth_addr instead of a 6-byte char array.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d8c6a7c..ab5eef8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5351,6 +5351,12 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct 
ofpact_conntrack *ofc)
 if (ofc->flags & NX_CT_F_COMMIT) {
 nl_msg_put_flag(ctx->odp_actions, ofc->flags & NX_CT_F_FORCE ?
 OVS_CT_ATTR_FORCE_COMMIT : OVS_CT_ATTR_COMMIT);
+if (ctx->xbridge->support.ct_eventmask) {
+nl_msg_put_u32(ctx->odp_actions, OVS_CT_ATTR_EVENTMASK,
+   1 << IPCT_NEW | 1 << IPCT_RELATED |
+   1 << IPCT_DESTROY | 1 << IPCT_MARK |
+   1 << IPCT_LABEL);
+}
 }
 nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
 put_ct_mark(>xin->flow, ctx->odp_actions, ctx->wc);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c73c273..23a8575 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1241,6 +1241,67 @@ check_clone(struct dpif_backer *backer)
 return !error;
 }
 
+/* Tests whether 'backer''s datapath supports the OVS_CT_ATTR_EVENTMASK
+ * attribute in OVS_ACTION_ATTR_CT. */
+static bool
+check_ct_eventmask(struct dpif_backer *backer)
+{
+struct dpif_execute execute;
+struct dp_packet packet;
+struct ofpbuf actions;
+struct flow flow = {
+.dl_type = CONSTANT_HTONS(ETH_TYPE_IP),
+.nw_proto = IPPROTO_UDP,
+.nw_ttl = 64,
+.nw_src = CONSTANT_HTONL(0x0a010101),
+.nw_dst = CONSTANT_HTONL(0x0a010102),
+.tp_src = CONSTANT_HTONS(42387),
+.tp_dst = CONSTANT_HTONS(13264)
+};
+size_t ct_start;
+int error;
+
+/* Compose CT action with eventmask attribute and check if datapath can
+ * decode the message.  */
+ofpbuf_init(, 64);
+ct_start = nl_msg_start_nested(, OVS_ACTION_ATTR_CT);
+/* Eventmask has no effect without the commit flag, but currently the
+ * datapath will accept an eventmask even without commit.  This is useful
+ * as we do not want to persist the probe connection in the conntrack
+ * table. */
+nl_msg_put_u32(, OVS_CT_ATTR_EVENTMASK, ~0);
+nl_msg_end_nested(, ct_start);
+
+/* Compose a dummy UDP packet. */
+dp_packet_init(, 0);
+flow_compose(, );
+
+/* Execute the actions.  On older datapaths this fails with EINVAL, on
+ * newer datapaths it succeeds. */
+execute.actions = actions.data;
+execute.actions_len = actions.size;
+execute.packet = 
+execute.flow = 
+execute.needs_help = false;
+execute.probe = true;
+execute.mtu = 0;
+
+error = dpif_execute(backer->dpif, );
+
+dp_packet_uninit();
+ofpbuf_uninit();
+
+if (error) {
+VLOG_INFO("%s: Datapath does not support eventmask in conntrack 
action",
+  dpif_name(backer->dpif));
+} else {
+VLOG_INFO("%s: Datapath supports eventmask in conntrack action",
+  dpif_name(backer->dpif));
+}
+
+return !error;
+}
+
 #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE)\
 static bool \
 check_##NAME(struct 

Re: [ovs-dev] [PATCH v3] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Joe Stringer]

On 27 April 2017 at 14:48, Jarno Rajahalme  wrote:
> Specify the event mask with CT commit including bits for CT features
> exposed at the OVS interface (mark and label changes in addition to
> basic creation and destruction of conntrack entries).
>
> Without this any listener of conntrack update events will typically
> (depending on system configuration) receive events for each L4 (e.g.,
> TCP) state machine change, which can multiply the number of events
> received per connection.
>
> By including the new, related, and destroy events any listener of new
> conntrack events gets notified of new related and non-related
> connections, and any listener of destroy events will get notified of
> deleted (typically timed out) conntrack entries.
>
> By including the flags for mark and labels, any listener of conntrack
> update events gets notified whenever the connmark or conntrack labels
> are changed from the values reported within the new events.
>
> VMware-BZ: #1837218
> Signed-off-by: Jarno Rajahalme 
> Acked-by: Joe Stringer 
> ---

Did you try building with sparse? I'm seeing:

ofproto/ofproto-dpif.c:1256:19: error: incorrect type in initializer
(different base types)
ofproto/ofproto-dpif.c:1256:19: expected restricted ovs_be32 [usertype] nw_src
ofproto/ofproto-dpif.c:1256:19: got int
ofproto/ofproto-dpif.c:1257:19: error: incorrect type in initializer
(different base types)
ofproto/ofproto-dpif.c:1257:19: expected restricted ovs_be32 [usertype] nw_dst
ofproto/ofproto-dpif.c:1257:19: got int
ofproto/ofproto-dpif.c:1258:19: error: incorrect type in initializer
(different base types)
ofproto/ofproto-dpif.c:1258:19: expected restricted ovs_be16 [usertype] tp_src
ofproto/ofproto-dpif.c:1258:19: got int
ofproto/ofproto-dpif.c:1259:19: error: incorrect type in initializer
(different base types)
ofproto/ofproto-dpif.c:1259:19: expected restricted ovs_be16 [usertype] tp_dst
ofproto/ofproto-dpif.c:1259:19: got int
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH V2 4/4] travis: Update kernels to kernel.org latest

[Greg Rose]

Signed-off-by: Greg Rose 
---
 .travis.yml | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 6f2b065..3a73873 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -30,13 +30,13 @@ env:
   - BUILD_ENV="-m32" OPTS="--disable-ssl"
   - KERNEL=3.16.39 DPDK=1
   - KERNEL=3.16.39 DPDK=1 OPTS="--enable-shared"
-  - KERNEL=4.9
-  - KERNEL=4.8.14
-  - KERNEL=4.4.38
-  - KERNEL=4.1.36
-  - KERNEL=3.18.45
-  - KERNEL=3.12.68
-  - KERNEL=3.10.104
+  - KERNEL=4.10.12
+  - KERNEL=4.9.24
+  - KERNEL=4.4.63
+  - KERNEL=4.1.39
+  - KERNEL=3.18.50
+  - KERNEL=3.12.73
+  - KERNEL=3.10.105
   - TESTSUITE=1 LIBS=-ljemalloc
 
 matrix:
-- 
1.8.3.1

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

[ovs-dev] [PATCH V2 3/4] compat: Fix build error in kernels 4.10+

[Greg Rose]

This is an alternative solution patch for the issue reported by
Raymond Burkholder and the patch submitted by Guoshuai Li.  It uses
the acinclude.m4 configuration file to check for the net parameter
that was added  to the ipv4 and ipv6 frags init functions in the 4.10
Linux kernel to check whether DEFRAG_ENABLE_TAKES_NET should be
set and then checks for that at compile time.

V2 - Incorporate suggestion from Joe Stringer to just return values
 of the init functions.

Reported-by: Raymond Burkholder 
CC: Guoshuai Li 
Signed-off-by: Greg Rose 
---
 datapath/linux/compat/ip_fragment.c| 12 
 datapath/linux/compat/nf_conntrack_reasm.c | 12 
 2 files changed, 24 insertions(+)

diff --git a/datapath/linux/compat/ip_fragment.c 
b/datapath/linux/compat/ip_fragment.c
index b0f5d0e..47b51b5 100644
--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -729,18 +729,30 @@ int rpl_ip_defrag(struct net *net, struct sk_buff *skb, 
u32 user)
return -ENOMEM;
 }
 
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+static int __net_init ipv4_frags_init_net(struct net *net)
+{
+   return nf_defrag_ipv4_enable(net);
+}
+#endif
+
 static void __net_exit ipv4_frags_exit_net(struct net *net)
 {
inet_frags_exit_net(>ipv4.frags, _frags);
 }
 
 static struct pernet_operations ip4_frags_ops = {
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+   .init = ipv4_frags_init_net,
+#endif
.exit = ipv4_frags_exit_net,
 };
 
 int __init rpl_ipfrag_init(void)
 {
+#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
nf_defrag_ipv4_enable();
+#endif
register_pernet_subsys(_frags_ops);
ip4_frags.hashfn = ip4_hashfn;
ip4_frags.constructor = ip4_frag_init;
diff --git a/datapath/linux/compat/nf_conntrack_reasm.c 
b/datapath/linux/compat/nf_conntrack_reasm.c
index 0bc4d9e..0da9463 100644
--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -558,12 +558,22 @@ out_unlock:
return ret;
 }
 
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+static int nf_ct_net_init(struct net *net)
+{
+   return nf_defrag_ipv6_enable(net);
+}
+#endif
+
 static void nf_ct_net_exit(struct net *net)
 {
inet_frags_exit_net(>nf_frag.frags, _frags);
 }
 
 static struct pernet_operations nf_ct_net_ops = {
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+   .init = nf_ct_net_init,
+#endif
.exit = nf_ct_net_exit,
 };
 
@@ -571,7 +581,9 @@ int rpl_nf_ct_frag6_init(void)
 {
int ret = 0;
 
+#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
nf_defrag_ipv6_enable();
+#endif
nf_frags.hashfn = nf_hashfn;
nf_frags.constructor = ip6_frag_init;
nf_frags.destructor = NULL;
-- 
1.8.3.1

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

[ovs-dev] [PATCH V2 2/4] acinclude.m4: Add check for struct net parameter

[Greg Rose]

The net parameter was added to nf_defrag_ipv6_enable in linux 4.10

Signed-off-by: Greg Rose 
---
 acinclude.m4 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 0d6647e..8653a30 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -703,7 +703,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
 [udp_add_offload], [net],
 [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])
-
+  OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/ipv6/nf_defrag_ipv6.h],
+[nf_defrag_ipv6_enable], [net],
+[OVS_DEFINE([HAVE_DEFRAG_ENABLE_TAKES_NET])])
 
   if cmp -s datapath/linux/kcompat.h.new \
 datapath/linux/kcompat.h >/dev/null 2>&1; then
-- 
1.8.3.1

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

[ovs-dev] [PATCH 0/4] Fixup 4.10 kernel compatability layer

[Greg Rose]

* Fixes a small bug in the acinclude.m4 file
* Adds check in acinclude.m4 for the frag init struct *net requirement
* Fixes up the ip_fragment.c and nf_conntrack_reasm.c compat files
* Updates the .travis.yml build verification script

V2 - Incorporate suggestion from Joe Stringer to just return values
 of the init functions. Compile tested locally on 4.9.24 and 4.10.12
 Linux kernels to ensure the inicremental change is valid but
 not verified again with Travis.

Verified here:

https://travis-ci.org/gvrose8192/ovs-experimental

Greg Rose (4):
  acinclude.m4: Fix parenthetical balance
  acinclude.m4: Add check for struct net parameter
  compat: Fix build error in kernels 4.10+
  travis: Update kernels to kernel.org latest

 .travis.yml| 14 +++---
 acinclude.m4   | 10 ++
 datapath/linux/compat/ip_fragment.c| 14 ++
 datapath/linux/compat/nf_conntrack_reasm.c | 14 ++
 4 files changed, 41 insertions(+), 11 deletions(-)

-- 
1.8.3.1

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

Re: [ovs-dev] [action upcall meters 5/5] ofproto: Meter slowpath action when action upcall meters are configured

[Jarno Rajahalme]

See comments below.

  Jarno

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> If a slow path action is a controller action, meter it when the
> controller meter is configured.  For other kinds of slow path actions,
> meter it when the slowpath meter is configured.
> 
> Note, this patch only considers the meters configuration of the
> packet's input bridge, which may not be the same bridge that the
> action is generated.
> 
> Signed-off-by: Andy Zhou 
> ---
> ofproto/ofproto-dpif-upcall.c | 34 +++---
> ofproto/ofproto-dpif-xlate.c  | 12 ++--
> tests/ofproto-dpif.at | 31 +++
> 3 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 3b28f9a22939..37f345b235b1 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1018,7 +1018,8 @@ classify_upcall(enum dpif_upcall_type type, const 
> struct nlattr *userdata)
> static void
> compose_slow_path(struct udpif *udpif, struct xlate_out *xout,
>   const struct flow *flow, odp_port_t odp_in_port,
> -  struct ofpbuf *buf)
> +  struct ofpbuf *buf, uint32_t slowpath_meter_id,
> +  uint32_t controller_meter_id)
> {
> union user_action_cookie cookie;
> odp_port_t port;
> @@ -1032,8 +1033,28 @@ compose_slow_path(struct udpif *udpif, struct 
> xlate_out *xout,
> ? ODPP_NONE
> : odp_in_port;
> pid = dpif_port_get_pid(udpif->dpif, port, flow_hash_5tuple(flow, 0));
> +
> +size_t offset;
> +size_t ac_offset;
> +uint32_t meter_id = xout->slow & SLOW_CONTROLLER ? controller_meter_id
> + : slowpath_meter_id;
> +
> +if (meter_id != UINT32_MAX) {
> +/* If slowpath meter is configured, generate clone(meter, userspace)
> + * action.   */
> +offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_SAMPLE);
> +nl_msg_put_u32(buf, OVS_SAMPLE_ATTR_PROBABILITY, UINT32_MAX);
> +ac_offset = nl_msg_start_nested(buf, OVS_SAMPLE_ATTR_ACTIONS);
> +nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER, meter_id);
> +}
> +
> odp_put_userspace_action(pid, , sizeof cookie.slow_path,
>  ODPP_NONE, false, buf);
> +
> +if (meter_id != UINT32_MAX) {
> +nl_msg_end_nested(buf, ac_offset);
> +nl_msg_end_nested(buf, offset);
> +}
> }
> 
> /* If there is no error, the upcall must be destroyed with upcall_uninit()
> @@ -1136,10 +1157,12 @@ upcall_xlate(struct udpif *udpif, struct upcall 
> *upcall,
> ofpbuf_use_const(>put_actions,
>  odp_actions->data, odp_actions->size);
> } else {
> +uint32_t smid= upcall->ofproto->up.slowpath_meter_id;

white space error.

> +uint32_t cmid = upcall->ofproto->up.controller_meter_id;
> /* upcall->put_actions already initialized by upcall_receive(). */
> compose_slow_path(udpif, >xout, upcall->flow,
>   upcall->flow->in_port.odp_port,
> -  >put_actions);
> +  >put_actions, smid, cmid);
> }
> 
> /* This function is also called for slow-pathed flows.  As we are only
> @@ -1956,9 +1979,14 @@ revalidate_ukey__(struct udpif *udpif, const struct 
> udpif_key *ukey,
> }
> 
> if (xoutp->slow) {
> +struct ofproto_dpif *ofproto;
> +ofproto = xlate_lookup_ofproto(udpif->backer, , NULL);
> +uint32_t smid= ofproto->up.slowpath_meter_id;
> +uint32_t cmid= ofproto->up.controller_meter_id;
> +
> ofpbuf_clear(odp_actions);
> compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
> -  odp_actions);
> +  odp_actions, smid, cmid);
> }
> 
> if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, _mask, )
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 52e0d3f1b0bb..416012ab6930 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2849,8 +2849,13 @@ compose_sample_action(struct xlate_ctx *ctx,
> return 0;
> }
> 
> +/* If the slow path meter is configured by the controller,
> + * Insert a meter action before the user space action.   */
> +struct ofproto *ofproto = >xin->ofproto->up;
> +uint32_t meter_id = ofproto->slowpath_meter_id;
> +
> /* No need to generate sample action for 100% sampling rate. */
> -bool is_sample = probability < UINT32_MAX;
> +bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX;

This seems to fix the problem in the previous patch.

> size_t sample_offset, actions_offset;
> if (is_sample) {
> sample_offset = nl_msg_start_nested(ctx->odp_actions,
> @@ -2861,11 +2866,6 @@ compose_sample_action(struct xlate_ctx *ctx,
> 

Re: [ovs-dev] [action upcall meters 4/5] ofproto: Meter sample action when configured.

[Jarno Rajahalme]

This breaks the test "ofproto-dpif - Bridge IPFIX sanity check” (currently test 
#1128), which appears to be the tests case that is being modified.

More comments below.

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> When slowpath meter is configured, add meter action when translate
> sample action.
> 
> Signed-off-by: Andy Zhou 
> ---
> ofproto/ofproto-dpif-xlate.c |  9 +
> tests/ofproto-dpif.at| 14 ++
> 2 files changed, 23 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a24aef9a43a1..52e0d3f1b0bb 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2861,6 +2861,15 @@ compose_sample_action(struct xlate_ctx *ctx,
>  OVS_SAMPLE_ATTR_ACTIONS);
> }
> 
> +/* If the slow path meter is configured by the controller,
> + * Insert a meter action before the user space action.   */
> +struct ofproto *ofproto = >xin->ofproto->up;
> +uint32_t meter_id = ofproto->slowpath_meter_id;
> +
> +if (meter_id != UINT32_MAX) {
> +nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
> +}
> +
> odp_port_t odp_port = ofp_port_to_odp_port(
> ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
> uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 0c2ea384b422..3c3037b16548 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6491,6 +6491,20 @@ flow-dump from non-dpdk interfaces:
> packets:2, bytes:68, used:0.001s, 
> actions:userspace(pid=0,ipfix(output_port=4294967295))
> ])
> 
> +AT_CHECK([ovs-appctl revalidator/purge])
> +dnl
> +dnl Add a slowpath meter. The userspace action should be metered.
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps burst 
> stats bands=type=drop rate=3 burst_size=1'])
> +
> +dnl Send some packets that should be sampled and metered.
> +for i in `seq 1 3`; do
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)'])
> +done
> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 
> 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
> +flow-dump from non-dpdk interfaces:
> +packets:2, bytes:68, used:0.001s, 
> actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295
> +])
> +

This is the test failure:

-packets:2, bytes:68, used:0.001s, 
actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295
+packets:2, bytes:68, used:0.001s, 
actions:meter(0),userspace(pid=0,ipfix(output_port=4294967295))

Applied to current master the sample envelope is not being inserted when 
probability is 100%. However, when using a meter the sample envelope is needed 
in all cases, as if the meter drops the packet, we still need to continue 
execution if there are further actions after the sample action.


> dnl Remove the IPFIX configuration.
> AT_CHECK([ovs-vsctl clear bridge br0 ipfix])
> AT_CHECK([ovs-appctl revalidator/purge])
> -- 
> 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] [action upcall meters 3/5] ofproto: Support action upcall meters

[Jarno Rajahalme]

With small nits below:

Acked-by: Jarno Rajahalme 

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> Allow action upcall meters, i.e. slowpath and controller meters,
> to be added and displayed.
> 
> Keep track of datapath meter ID of those action upcall meters in
> ofproto to aid action translation. Later patches will make use of them.
> 
> Signed-off-by: Andy Zhou 
> ---
> lib/ofp-print.c| 33 ++---
> ofproto/ofproto-provider.h |  4 
> ofproto/ofproto.c  | 52 ++
> 3 files changed, 82 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index a8cdfcbf20b1..140af05950b7 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1333,11 +1333,36 @@ ofp_print_meter_band(struct ds *s, uint16_t flags,
> }
> 
> static void
> +ofp_print_meter_id(struct ds *s, uint32_t meter_id, char seperator)
> +{
> +if (meter_id <= OFPM13_MAX) {
> +ds_put_format(s, "meter%c%"PRIu32, seperator, meter_id);
> +} else {
> +const char *name;
> +switch (meter_id) {
> +case OFPM13_SLOWPATH:
> +name = "slowpath";
> +break;
> +case OFPM13_CONTROLLER:
> +name = "controller";
> +break;
> +case OFPM13_ALL:
> +name = "ALL”;

We require lower case “all” when parsing, so better print that way also.

> +break;
> +default:
> +name = "unknown";
> +}
> +ds_put_format(s, "meter%c%s", seperator, name);
> +}
> +}
> +
> +static void
> ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms)
> {
> uint16_t i;
> 
> -ds_put_format(s, "meter:%"PRIu32" ", ms->meter_id);
> +ofp_print_meter_id(s, ms->meter_id, ':');
> +ds_put_char(s, ' ');
> ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count);
> ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count);
> ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count);
> @@ -1358,7 +1383,8 @@ ofp_print_meter_config(struct ds *s, const struct 
> ofputil_meter_config *mc)
> {
> uint16_t i;
> 
> -ds_put_format(s, "meter=%"PRIu32" ", mc->meter_id);
> +ofp_print_meter_id(s, mc->meter_id, '=');
> +ds_put_char(s, ' ');
> 
> ofp_print_meter_flags(s, mc->flags);
> 
> @@ -1412,8 +1438,9 @@ ofp_print_meter_stats_request(struct ds *s, const 
> struct ofp_header *oh)
> uint32_t meter_id;
> 
> ofputil_decode_meter_request(oh, _id);
> +ds_put_char(s, ' ');
> 
> -ds_put_format(s, " meter=%"PRIu32, meter_id);
> +ofp_print_meter_id(s, meter_id, '=');
> }
> 
> static const char *
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 000326d7f79d..688a9e5d32eb 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -112,6 +112,10 @@ struct ofproto {
> /* Meter table.  */
> struct ofputil_meter_features meter_features;
> struct hmap meters; /* uint32_t indexed 'struct meter *'.  */
> +uint32_t slowpath_meter_id; /* Datapath slowpath meter.  UINT32_MAX
> +   if not defined.  */
> +uint32_t controller_meter_id;   /* Datapath controller meter. UINT32_MAX
> +   if not defined.  */
> 
> /* OpenFlow connections. */
> struct connmgr *connmgr;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 8c4c7e67f213..abbb849a384b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -568,6 +568,8 @@ ofproto_create(const char *datapath_name, const char 
> *datapath_type,
> memset(>meter_features, 0, sizeof ofproto->meter_features);
> }
> hmap_init(>meters);
> +ofproto->slowpath_meter_id = UINT32_MAX;
> +ofproto->controller_meter_id = UINT32_MAX;
> 
> /* Set the initial tables version. */
> ofproto_bump_tables_version(ofproto);
> @@ -6232,9 +6234,33 @@ ofproto_get_meter(const struct ofproto *ofproto, 
> uint32_t meter_id)
> return NULL;
> }
> 
> +static uint32_t *
> +ofproto_upcall_meter_ptr(struct ofproto *ofproto, uint32_t meter_id)
> +{
> +switch(meter_id) {
> +case OFPM13_SLOWPATH:
> +return >slowpath_meter_id;
> +break;
> +case OFPM13_CONTROLLER:
> +return >controller_meter_id;
> +break;
> +case OFPM13_ALL:
> +OVS_NOT_REACHED();
> +default:
> +return NULL;
> +}
> +}
> +
> static void
> ofproto_add_meter(struct ofproto *ofproto, struct meter *meter)
> {
> +uint32_t *upcall_meter_ptr = ofproto_upcall_meter_ptr(ofproto, 
> meter->id);
> +
> +/* Cache datapath meter IDs of special meters. */
> +if (upcall_meter_ptr) {
> +*upcall_meter_ptr = meter->provider_meter_id.uint32;
> +}
> +
> hmap_insert(>meters, >node, hash_int(meter->id, 0));
> }
> 
> @@ -6248,7 +6274,7 @@ static bool
> 

Re: [ovs-dev] [action upcall meters 2/5] ofproto-dpif: Use backer to manage datapath meter allocation

[Jarno Rajahalme]

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> Add 'meter_ids', an id-pool object to manage datapath meter id.
> 
> Currently, only userspace datapath supports meter, and it implements
> the provider_meter_id management. Moving this function to 'backer'
> allows other datapath implementation to share the same logic.
> 
> Signed-off-by: Andy Zhou 
> ---
> lib/dpif-netdev.c  | 24 
> ofproto/ofproto-dpif.c | 44 ++--
> ofproto/ofproto-dpif.h |  4 
> 3 files changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a14a2ebb5b2a..d5417162b7af 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -260,7 +260,6 @@ struct dp_netdev {
> /* Meters. */
> struct ovs_mutex meter_locks[N_METER_LOCKS];
> struct dp_meter *meters[MAX_METERS]; /* Meter bands. */
> -uint32_t meter_free; /* Next free meter. */
> 
> /* Protects access to ofproto-dpif-upcall interface during revalidator
>  * thread synchronization. */
> @@ -3896,9 +3895,6 @@ dpif_netdev_meter_set(struct dpif *dpif, 
> ofproto_meter_id *meter_id,
> struct dp_meter *meter;
> int i;
> 
> -if (mid == UINT32_MAX) {
> -mid = dp->meter_free;
> -}
> if (mid >= MAX_METERS) {
> return EFBIG; /* Meter_id out of range. */
> }
> @@ -3958,21 +3954,6 @@ dpif_netdev_meter_set(struct dpif *dpif, 
> ofproto_meter_id *meter_id,
> dp->meters[mid] = meter;
> meter_unlock(dp, mid);
> 
> -meter_id->uint32 = mid; /* Store on success. */
> -
> -/* Find next free meter */
> -if (dp->meter_free == mid) { /* Now taken. */
> -do {
> -if (++mid >= MAX_METERS) { /* Wrap around */
> -mid = 0;
> -}
> -if (mid == dp->meter_free) { /* Full circle */
> -mid = MAX_METERS;
> -break;
> -}
> -} while (dp->meters[mid]);
> -dp->meter_free = mid; /* Next free meter or MAX_METERS */
> -}
> return 0;
> }
> return ENOMEM;
> @@ -4027,11 +4008,6 @@ dpif_netdev_meter_del(struct dpif *dpif,
> meter_lock(dp, meter_id);
> dp_delete_meter(dp, meter_id);
> meter_unlock(dp, meter_id);
> -
> -/* Keep free meter index as low as possible */
> -if (meter_id < dp->meter_free) {
> -dp->meter_free = meter_id;
> -}
> }
> return error;
> }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6a5ffb94fa94..a026d4913731 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -662,6 +662,7 @@ close_dpif_backer(struct dpif_backer *backer)
> free(backer->type);
> free(backer->dp_version_string);
> dpif_close(backer->dpif);
> +id_pool_destroy(backer->meter_ids);
> free(backer);
> }
> 
> @@ -787,6 +788,15 @@ open_dpif_backer(const char *type, struct dpif_backer 
> **backerp)
> = check_variable_length_userdata(backer);
> backer->dp_version_string = dpif_get_dp_version(backer->dpif);
> 
> +/* Manage Datpath meter IDs if supported. */

->”Datapath"

> +struct ofputil_meter_features features;
> +dpif_meter_get_features(backer->dpif, );
> +if (features.max_meters) {
> +backer->meter_ids = id_pool_create(0, features.max_meters);
> +} else {
> +backer->meter_ids = NULL;
> +}
> +
> return error;
> }
> 
> @@ -5385,6 +5395,17 @@ meter_set(struct ofproto *ofproto_, ofproto_meter_id 
> *meter_id,
> {
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> 
> +/* Provider ID unknown. Use backer to allocate a new DP meter */
> +if (meter_id->uint32 == UINT32_MAX) {
> +if (!ofproto->backer->meter_ids) {
> +return EFBIG; /* Datapath does not support meter.  */
> +}
> +
> +if(!id_pool_alloc_id(ofproto->backer->meter_ids, _id->uint32)) 
> {
> +return ENOMEM; /* Can't allocate a DP meter. */
> +}
> +}
> +
> switch (dpif_meter_set(ofproto->backer->dpif, meter_id, config)) {
> case 0:
> return 0;
> @@ -5414,12 +5435,31 @@ meter_get(const struct ofproto *ofproto_, 
> ofproto_meter_id meter_id,
> return OFPERR_OFPMMFC_UNKNOWN_METER;
> }
> 
> +struct free_meter_id_args {
> +struct ofproto_dpif *ofproto;
> +ofproto_meter_id meter_id;
> +};
> +
> +static void
> +free_meter_id(struct free_meter_id_args *args)
> +{
> +struct ofproto_dpif *ofproto = args->ofproto;
> +
> +dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
> +id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
> +free(args);
> +}
> +
> static void
> meter_del(struct ofproto *ofproto_, ofproto_meter_id meter_id)
> {
> -struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> +struct 

Re: [ovs-dev] [action upcall meters 1/5] ofproto: Store meters using hmap

[Jarno Rajahalme]

This incremental needed to satisfy GCC 4.9.2, due to ‘meter’ potentially being 
used uninitialized:

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 2e80db8..eb060d0 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6564,7 +6564,7 @@ handle_meter_request(struct ofconn *ofconn, const struct 
ofp_header *request,
 struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
 struct ovs_list replies;
 uint32_t meter_id;
-struct meter *meter;
+struct meter *meter = NULL;
 
 ofputil_decode_meter_request(request, _id);
 

Otherwise:

Acked-by: Jarno Rajahalme 

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> Currently, meters are stored in a fixed pointer array. It is not
> very efficient since the controller, at least in theory, can
> pick any meter id (up to the limits to uint32_t), not necessarily
> within the lower end of a region, or in close range to each other.
> In particular, OFPM_SLOWPATH and OFPM_CONTROLLER meters are specified
> at the high region.
> 
> Switching to using hmap. Ofproto layer does not restrict
> the number of meters that controller can add, nor does it care
> about the value of meter_id. Datapth limits the number of meters
> ofproto layer can support at run time.
> 
> Signed-off-by: Andy Zhou 
> ---
> ofproto/ofproto-provider.h |   7 +-
> ofproto/ofproto.c  | 242 +++--
> 2 files changed, 146 insertions(+), 103 deletions(-)
> 
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index b7b12cdfd5f4..000326d7f79d 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -109,12 +109,9 @@ struct ofproto {
> /* List of expirable flows, in all flow tables. */
> struct ovs_list expirable OVS_GUARDED_BY(ofproto_mutex);
> 
> -/* Meter table.
> - * OpenFlow meters start at 1.  To avoid confusion we leave the first
> - * pointer in the array un-used, and index directly with the OpenFlow
> - * meter_id. */
> +/* Meter table.  */
> struct ofputil_meter_features meter_features;
> -struct meter **meters; /* 'meter_features.max_meter' + 1 pointers. */
> +struct hmap meters; /* uint32_t indexed 'struct meter *'.  */
> 
> /* OpenFlow connections. */
> struct connmgr *connmgr;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7440d5b52092..8c4c7e67f213 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -281,7 +281,8 @@ static uint64_t pick_fallback_dpid(void);
> static void ofproto_destroy__(struct ofproto *);
> static void update_mtu(struct ofproto *, struct ofport *);
> static void update_mtu_ofproto(struct ofproto *);
> -static void meter_delete(struct ofproto *, uint32_t first, uint32_t last);
> +static void meter_delete(struct ofproto *, uint32_t);
> +static void meter_delete_all(struct ofproto *);
> static void meter_insert_rule(struct rule *);
> 
> /* unixctl. */
> @@ -566,8 +567,7 @@ ofproto_create(const char *datapath_name, const char 
> *datapath_type,
> } else {
> memset(>meter_features, 0, sizeof ofproto->meter_features);
> }
> -ofproto->meters = xzalloc((ofproto->meter_features.max_meters + 1)
> -  * sizeof(struct meter *));
> +hmap_init(>meters);
> 
> /* Set the initial tables version. */
> ofproto_bump_tables_version(ofproto);
> @@ -1635,12 +1635,8 @@ ofproto_destroy(struct ofproto *p, bool del)
> return;
> }
> 
> -if (p->meters) {
> -meter_delete(p, 1, p->meter_features.max_meters);
> -p->meter_features.max_meters = 0;
> -free(p->meters);
> -p->meters = NULL;
> -}
> +meter_delete_all(p);
> +hmap_destroy(>meters);
> 
> ofproto_flush__(p);
> HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, >ports) {
> @@ -6211,14 +6207,37 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, 
> const struct ofp_header *oh)
>  * 'provider_meter_id' is for the provider's private use.
>  */
> struct meter {
> +struct hmap_node node;  /* In ofproto->meters. */
> long long int created;  /* Time created. */
> struct ovs_list rules;  /* List of "struct rule_dpif"s. */
> +uint32_t id;/* OpenFlow meter_id. */
> ofproto_meter_id provider_meter_id;
> uint16_t flags; /* Meter flags. */
> uint16_t n_bands;   /* Number of meter bands. */
> struct ofputil_meter_band *bands;
> };
> 
> +static struct meter *
> +ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id)
> +{
> +struct meter *meter;
> +uint32_t hash = hash_int(meter_id, 0);
> +
> +HMAP_FOR_EACH_WITH_HASH (meter, node, hash, >meters) {
> +if (meter->id == meter_id) {
> +return meter;
> +}
> +}
> +
> +return NULL;
> +}
> +
> +static void
> +ofproto_add_meter(struct ofproto *ofproto, struct meter *meter)
> +{
> +hmap_insert(>meters, 

[ovs-dev] [PATCH v3] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Jarno Rajahalme]

Specify the event mask with CT commit including bits for CT features
exposed at the OVS interface (mark and label changes in addition to
basic creation and destruction of conntrack entries).

Without this any listener of conntrack update events will typically
(depending on system configuration) receive events for each L4 (e.g.,
TCP) state machine change, which can multiply the number of events
received per connection.

By including the new, related, and destroy events any listener of new
conntrack events gets notified of new related and non-related
connections, and any listener of destroy events will get notified of
deleted (typically timed out) conntrack entries.

By including the flags for mark and labels, any listener of conntrack
update events gets notified whenever the connmark or conntrack labels
are changed from the values reported within the new events.

VMware-BZ: #1837218
Signed-off-by: Jarno Rajahalme 
Acked-by: Joe Stringer 
---
v3: Added feature probing to not use the new attribute on datapaths
the do not support it.

 build-aux/extract-odp-netlink-h |  2 ++
 ofproto/ofproto-dpif-xlate.c|  6 
 ofproto/ofproto-dpif.c  | 62 +
 ofproto/ofproto-dpif.h  |  5 +++-
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/build-aux/extract-odp-netlink-h b/build-aux/extract-odp-netlink-h
index 907a70a..7fb6ce8 100755
--- a/build-aux/extract-odp-netlink-h
+++ b/build-aux/extract-odp-netlink-h
@@ -19,6 +19,8 @@ $i\
 #ifdef _WIN32\
 #include "OvsDpInterfaceExt.h"\
 #include "OvsDpInterfaceCtExt.h"\
+#else\
+#include "linux/netfilter/nf_conntrack_common.h"\
 #endif\
 
 # Use OVS's own struct eth_addr instead of a 6-byte char array.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d8c6a7c..ab5eef8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5351,6 +5351,12 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct 
ofpact_conntrack *ofc)
 if (ofc->flags & NX_CT_F_COMMIT) {
 nl_msg_put_flag(ctx->odp_actions, ofc->flags & NX_CT_F_FORCE ?
 OVS_CT_ATTR_FORCE_COMMIT : OVS_CT_ATTR_COMMIT);
+if (ctx->xbridge->support.ct_eventmask) {
+nl_msg_put_u32(ctx->odp_actions, OVS_CT_ATTR_EVENTMASK,
+   1 << IPCT_NEW | 1 << IPCT_RELATED |
+   1 << IPCT_DESTROY | 1 << IPCT_MARK |
+   1 << IPCT_LABEL);
+}
 }
 nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
 put_ct_mark(>xin->flow, ctx->odp_actions, ctx->wc);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c73c273..b052b04 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1241,6 +1241,67 @@ check_clone(struct dpif_backer *backer)
 return !error;
 }
 
+/* Tests whether 'backer''s datapath supports the OVS_CT_ATTR_EVENTMASK
+ * attribute in OVS_ACTION_ATTR_CT. */
+static bool
+check_ct_eventmask(struct dpif_backer *backer)
+{
+struct dpif_execute execute;
+struct dp_packet packet;
+struct ofpbuf actions;
+struct flow flow = {
+.dl_type = htons(ETH_TYPE_IP),
+.nw_ttl = 64,
+.nw_proto = IPPROTO_UDP,
+.nw_src = 0x0a010101,
+.nw_dst = 0x0a010102,
+.tp_src = 42387,
+.tp_dst = 13264,
+};
+size_t ct_start;
+int error;
+
+/* Compose CT action with eventmask attribute and check if datapath can
+ * decode the message.  */
+ofpbuf_init(, 64);
+ct_start = nl_msg_start_nested(, OVS_ACTION_ATTR_CT);
+/* Eventmask has no effect without the commit flag, but currently the
+ * datapath will accept an eventmask even without commit.  This is useful
+ * as we do not want to persist the probe connection in the conntrack
+ * table. */
+nl_msg_put_u32(, OVS_CT_ATTR_EVENTMASK, ~0);
+nl_msg_end_nested(, ct_start);
+
+/* Compose a dummy UDP packet. */
+dp_packet_init(, 0);
+flow_compose(, );
+
+/* Execute the actions.  On older datapaths this fails with EINVAL, on
+ * newer datapaths it succeeds. */
+execute.actions = actions.data;
+execute.actions_len = actions.size;
+execute.packet = 
+execute.flow = 
+execute.needs_help = false;
+execute.probe = true;
+execute.mtu = 0;
+
+error = dpif_execute(backer->dpif, );
+
+dp_packet_uninit();
+ofpbuf_uninit();
+
+if (error) {
+VLOG_INFO("%s: Datapath does not support eventmask in conntrack 
action",
+  dpif_name(backer->dpif));
+} else {
+VLOG_INFO("%s: Datapath supports eventmask in conntrack action",
+  dpif_name(backer->dpif));
+}
+
+return !error;
+}
+
 #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE)\
 static bool \
 check_##NAME(struct dpif_backer *backer)  

Re: [ovs-dev] [PATCH] bridge: Prohibit "default" bridge name.

[William Tu]

On Thu, Apr 27, 2017 at 11:17 AM, Ben Pfaff  wrote:
> On Thu, Apr 27, 2017 at 10:17:20AM -0700, William Tu wrote:
>> On Thu, Apr 27, 2017 at 9:57 AM, Ben Pfaff  wrote:
>> > On Thu, Apr 27, 2017 at 04:02:10AM -0700, William Tu wrote:
>> >> Before the patch, when users create bridge named "default", although
>> >> ovs-vsctl fails but vswitchd in the background will keep retrying it,
>> >> causing the systemd-udev to reach 100% cpu utilization.  The reason is
>> >> due to frequent calls into kernel's register_netdevice function,
>> >> which will invoke several kernel elements who has registered on the
>> >> netdevice notifier chain.  One of the notifier, the inetdev_event rejects
>> >> this devname and register_netdevice fails.  The patch prohibits creating
>> >> "default" bridge name.
>> >>
>> >> VMWare-BZ: #1842388
>> >> Signed-off-by: William Tu 
>> >
>> > It all seems very arbitrary. Do we understand why this is an invalid
>> > device name?
>>
>> Yes, when kernel is configured with CONFIG_SYSCTL, creating a new
>> netdev creates a dir in /proc/sys/net/ipv4/conf/
>>
>> The  "default" and "all" is pre-existed when SYSCTL
>> starts (which means we should also prohibit "all") for default
>> property of the system's netdev. So sysctl prevents creating dev->name
>> is "default" or "all". A call stack is below if interested:
>> sysctl_dev_name_is_allowed
>>devinet_sysctl_register
>>  inetdev_event
>>notifier_call_chain
>>  raw_notifier_call_chain
>>call_netdevice_notifiers_info
>>  register_netdevice
>
> Do we get the same behavior (100% CPU) if creating a bridge fails for
> other reasons?  For example, it might fail because a network device
> already exists with the given name, or because the name is too long for
> a network device name.  If we do get 100% CPU from such a failure, then
> we should fix the root of the problem instead of blacklisting particular
> names.

There are two scenarios:
1) if the bridge name exists, ex: eth0
then "ovs-vsctl add-br eth0" fails due to EEXIST
2) if the bridge name does not exists, but is "default" or "all"
then "ovs-vsctl add-br default" fails due to EINVAL

>From OVS's point of view it's the same, ovs-vsctl fails creating the
bridge, and keeps retry. From the whole system's point of view, (2)
has impact on other Linux subsystems, due to kernel netdev notifier
chain mechanism informing other subsystems when trying to add a new
device, while (1) fails pretty early in register_netdevice() and has
no impact.

Or instead of blacklisting, maybe add a max retry count?

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

Re: [ovs-dev] [PATCH v3 0/7] create tunnel devices using rtnetlink interface

[Joe Stringer]

On 13 April 2017 at 13:47, Eric Garver  wrote:
> This series adds support for the creation of tunnels using the rtnetlink
> interface. This will open the possibility for new features and flags on those
> vports without the need to change vport compatibility code.
>
> Support for STT and LISP have not been added because these are not upstream 
> yet,
> so we don't know how the interface will be like upstream. And there are no
> features in the current drivers right now we could make use of.
>
> Note: This work originally started by Thadeu Lima de Souza Cascardo.

While reviewing, I noticed a bit of duplicate code and minor style
differences that I thought improved readability. I assembled a short
series below, which I could send on top once this series is applied,
or you could feel free to fold the changes into your next revision:

https://github.com/joestringer/openvswitch/tree/dev/rtnl_refactor

I didn't look into how the *_create_kind() functions could share more
code, but if there is a way then I think that may be beneficial.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 3/7] dpif-netlink: code to create/destroy tunnel ports via rtnetlink

[Joe Stringer]

On 13 April 2017 at 13:47, Eric Garver  wrote:
> In order to be able to add those tunnels, we need to add code to create
> the tunnels and add them as NETDEV vports. And when there is no support
> to create them, we need to fallback to compatibility code and add them
> as tunnel vports.
>
> When removing those tunnels, we need to remove the interfaces as well,
> and detecting the right type might be important, at least to distinguish
> the tunnel vports that we should remove and the interfaces that we
> shouldn't.
>
> Co-authored-by: Thadeu Lima de Souza Cascardo 
> Signed-off-by: Thadeu Lima de Souza Cascardo 
> Signed-off-by: Eric Garver 
> ---

Hi Eric (and Thadeu!), thanks for the patches. A couple of minor
comments on this patch:

As a matter of style, please use the imperative in commit messages and
keep them ideally below 50 characters. For instance, rather than
"dpif-netlink: code to create/destroy tunnel ports via rtnetlink", try
"dpif-netlink: Support rtnetlink port creation.". For a refresher, see
the documentation here:

http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

This fixup also makes the error messages more clear in the logs when
something fails on the rtnetlink interface.

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 5244d7e24179..4d3d4ffe81bc 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -967,8 +967,8 @@ dpif_netlink_rtnl_port_create_and_add(struct
dpif_netlink *dpif,
error = dpif_netlink_rtnl_port_create(netdev);
if (error) {
if (error != EOPNOTSUPP) {
-VLOG_INFO_RL(, "Failed to create %s with rtnetlink. error = %d",
- netdev_get_name(netdev), error);
+VLOG_INFO_RL(, "Failed to create %s with rtnetlink: %s",
+ netdev_get_name(netdev), ovs_strerror(error));
}
return error;
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 0/7] create tunnel devices using rtnetlink interface

[Joe Stringer]

On 13 April 2017 at 13:47, Eric Garver  wrote:
> This series adds support for the creation of tunnels using the rtnetlink
> interface. This will open the possibility for new features and flags on those
> vports without the need to change vport compatibility code.
>
> Support for STT and LISP have not been added because these are not upstream 
> yet,
> so we don't know how the interface will be like upstream. And there are no
> features in the current drivers right now we could make use of.
>
> Note: This work originally started by Thadeu Lima de Souza Cascardo.
>
> Testing:
>   - kernel 4.9.22, in-tree datapath
> - rtnetlink successfully creates devices
>   - kernel 4.2.8, in-tree datapath
> - rtnetlink is tried, but fails due to no COLLECT_METADATA support
> - genetlink successfully creates devices
>   - kernel 4.2.8, out-of-tree datapath
> - rtnetlink is not tried
> - genetlink successfully creates devices
>
> v3:
>   - commonzie code to get port data to verify port
>   - eliminate dpif_netlink_rtnl_vxlan_destroy() and alike
>   - minor changes for coding style guidelines
>   - add ACKs from previous reviews

Sorry about the delay. I almost pushed it the other day, but had a
concern around how this series handles shutdown/restart of OVS.

I have a couple of minor pieces of feedback on some patches, please
roll those in.

In my setup I removed the vport-* modules so there was no way to fall
back to compat code, then ran depmod to fix up the dependencies.

When I run the GRE tests, the first gre test succeeds then the second fails:

$ make check-kernel TESTSUITEFLAGS="-k gre"
 10: datapath - ping over gre tunnel ok
14: datapath - truncate and output to gre tunnelFAILED
(system-traffic.at:507)

$ ip link | grep gre_sys
62: gre_sys@NONE:  mtu 1472 qdisc
pfifo_fast state UNKNOWN mode DEFAULT group default qlen 1000

$ sudo ip li del dev gre_sys

$ make check-kernel TESTSUITEFLAGS="14"
 14: datapath - truncate and output to gre tunnelok

$ make check-kernel TESTSUITEFLAGS="14"
  14: datapath - truncate and output to gre tunnelFAILED
(system-traffic.at:507)

This isn't failing randomly; first time it works, but if the device
exists, it fails. This is similar to ovs restart or crash with
--monitor.

Looking at the logs:
> 2017-04-27T21:24:19.270Z|00041|dpif_netlink|INFO|Failed to create at_gre0 
> with rtnetlink: Invalid argument
> 2017-04-27T21:24:19.271Z|00042|dpif|WARN|system@ovs-system: failed to add 
> at_gre0 as port: Address family not supported by protocol
> 2017-04-27T21:24:19.271Z|00043|bridge|WARN|could not add network device 
> at_gre0 to ofproto (Address family not supported by protocol)
> 2017-04-27T21:24:19.272Z|00044|dpif_netlink|INFO|Failed to create at_gre0 
> with rtnetlink: Invalid argument

Seems like the kernel is incorrectly returing -EINVAL rather than
-EEXIST. So even if we were to make the probe/tunnel creation logic a
bit smarter to understand if the port already exists (eg, because OVS
was restarted), it won't handle correctly.

Could you look further into this? Ideally the kernel could tell us
when we're trying to create a device that already exists, and return
EEXIST.. then it would be up to our validation logic to ensure that
all the correct options are configured on that device. But maybe the
workaround for all existing kernels is to try to delete the tunnel
device the first time, then try to recreate it.

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

Re: [ovs-dev] [PATCH] build: Don't run tests in rpm makefile targets.

[Aaron Conole]

Lance Richardson  writes:

>> From: "Russell Bryant" 
>> To: "Ben Pfaff" 
>> Cc: "ovs dev" , "Flavio Leitner"
>> , "Lance Richardson" ,
>> "Aaron Conole" 
>> Sent: Thursday, 27 April, 2017 3:04:43 PM
>> Subject: Re: [ovs-dev] [PATCH] build: Don't run tests in rpm makefile 
>> targets.
>> 
>> On Fri, Apr 21, 2017 at 7:01 PM, Ben Pfaff  wrote:
>> > On Fri, Mar 31, 2017 at 11:27:23AM -0400, Russell Bryant wrote:
>> >> The RPM build makefile targets are helpful during development and testing,
>> >> but I personally almost never want the tests to run when I use them.
>> >> Leave tests on by default in the spec file for when the package is built
>> >> by
>> >> distro build systems, but disable it by default in the Makefile targets
>> >> and
>> >> update the documentation accordingly.
>> >>
>> >> Signed-off-by: Russell Bryant 
>> >
>> > I'm OK with this.
>> 
>> Flavio, Lance, Aaron, have any opinions on this?  It's not that big of
>> a deal.  I guess I was feeling both slightly annoyed and lazy that
>> day.  :-)
>> 
>
> Seems fine to me.
>
> Acked-by: Lance Richardson 

+1

Acked-by: Aaron Conole 

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

Re: [ovs-dev] [PATCH v2 0/6] Add OVS DPDK keep-alive functionality

[Aaron Conole]

Hi Bhanu,

Bhanuprakash Bodireddy  writes:

> This patch is aimed at achieving Fastpath Service Assurance in
> OVS-DPDK deployments. This commit adds support for monitoring the packet
> processing cores(pmd thread cores) by dispatching heartbeats at regular
> intervals. Incase of heartbeat miss the failure shall be detected &
> reported to higher level fault management systems/frameworks.
>
> The implementation uses POSIX shared memory object for storing the
> events that will be read by monitoring framework. keep-alive feature
> can be enabled through below OVSDB settings.

I've been thinking about this design, and I'm concerned - shared memory
is inflexible, and allows multiple actors to mess with the information.
Is there a reason to use shared memory?  I am not sure of what
advantage this form of reporting has vs. simply using a message
passing interface.  With messages there is clear abstraction, and the
projects / processes are truly separated.  Shared memory is leads to a
situation of inextricably coupling two (or more) processes.

As an example, if the constant changes, or a new statistic is desired to
be tracked, the consumer which wants to use this data needs to be
recompiled, and needs to have the *exact* correct version.  If the pad
bits from the compiler change, if anything from the ovs side causes
alignment to be shifted, if OvS wants to redefine the struct, if OvS
uses any data from there as the rhs... the list of scenarios where this
interface can fail goes on - and the failures are quite catastrophic.

I think maybe a design doc of this interface would be good to read
through, as it will explain why this design was chosen.  It also might
allow for better feedback, putting a more generic solution (for example,
any new threads that OvS spawns we might want to monitor, as well - and
that would be good to report).  Do you agree?

> keepalive=true
>- Keepalive feature is disabled by default
>
> keepalive-interval="50"
>- Timer interval in milliseconds for monitoring the packet
>  processing cores.
>
> keepalive-shm-name="/dpdk_keepalive_shm_name"
>- Shared memory block name where the events shall be updated.
>
> When KA is enabled, 'ovs-keepalive' thread shall be spawned that wakes
> up at regular intervals to update the timestamp and status of pmd cores
> in shared memory region.
>
> An external monitoring framework like collectd(with dpdk plugin support)
> can read the status updates from shared memory. On a missing heartbeat,
> the collectd shall relay the status to ceilometer service running in the
> controller. Below is the high level overview of deployment model.
>
> Compute Node   Controller
>
>  Collectd  <-> Ceilometer
>
>  OVS DPDK
>
>+-+
>| VM  |
>+--+--+
>   \---+---/
>   |
>+--+---+   ++--+ +--+---+
>| OVS  |-> |dpdkevents plugin  | --> |   collectd   |
>+--+---+   ++--+ +--+---+
>
>  +--+-+ +---++
>  | Ceilometer | <-- | collectd ceilometer plugin |  <
>  +--+-+ +---++
>
> v1->v2:
> - Sort the patches in the order leaving no dead code behind.
> - Remove xusleep() implementation and instead used usleep().
> - Replace '_WIN32' with '__linux__' in get_process_status().
> - Add comments for different Keepalive states.
> - Remove semaphore and all the logic associated with it.
> - Fix the documentation as suggested.
> - Fix and added few appropriate comments to KA helper functions.
> - Add latency stats details in the commit log for future reference.
>
> Bhanuprakash Bodireddy (6):
>   dpdk: Add helper functions for DPDK keepalive.
>   process: Retrieve process status.
>   dpif-netdev: Register packet processing cores for keepalive.
>   netdev-dpdk: Add support for keepalive functionality.
>   vswitch.xml: Add keepalive support.
>   Documentation: Update DPDK doc with Keepalive feature.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/4] acinclude.m4: Add check for struct net parameter

[Greg Rose]

On Thu, 2017-04-27 at 14:05 -0700, Andy Zhou wrote:
> On Thu, Apr 27, 2017 at 1:08 PM, Greg Rose  wrote:
> > The net parameter was added to nf_defrag_ipv6_enable in linux 4.10
> >
> > Signed-off-by: Greg Rose 
> 
> Thanks for working on this!
> 
> It seems odd that we only detect the signatures of a ipv6 function,
> then apply it for ipv4l.

The common attribute between them is that they're both frag init
functions and would need to go together by my understanding.

> In practice, they are both introduced in 4.10, and will probably be
> back ported together.

I think so.

> So it may be O.K.  On the other hand, the cost of adding another
> #define seems small.

I'll go with consensus opinion... if others agree then I'll re-spin the
patches to do so.

Thanks for the review!

- Greg

> 
> > ---
> >  acinclude.m4 | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index 0d6647e..8653a30 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -703,7 +703,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
> >  [udp_add_offload], [net],
> >  [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])
> > -
> > +  
> > OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/ipv6/nf_defrag_ipv6.h],
> > +[nf_defrag_ipv6_enable], [net],
> > +[OVS_DEFINE([HAVE_DEFRAG_ENABLE_TAKES_NET])])
> >
> >if cmp -s datapath/linux/kcompat.h.new \
> >  datapath/linux/kcompat.h >/dev/null 2>&1; then
> > --
> > 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] [PATCH 2/4] acinclude.m4: Add check for struct net parameter

[Andy Zhou]

On Thu, Apr 27, 2017 at 1:08 PM, Greg Rose  wrote:
> The net parameter was added to nf_defrag_ipv6_enable in linux 4.10
>
> Signed-off-by: Greg Rose 

Thanks for working on this!

It seems odd that we only detect the signatures of a ipv6 function,
then apply it for ipv4l.
In practice, they are both introduced in 4.10, and will probably be
back ported together.
So it may be O.K.  On the other hand, the cost of adding another
#define seems small.

> ---
>  acinclude.m4 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 0d6647e..8653a30 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -703,7 +703,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
>  [udp_add_offload], [net],
>  [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])
> -
> +  OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/ipv6/nf_defrag_ipv6.h],
> +[nf_defrag_ipv6_enable], [net],
> +[OVS_DEFINE([HAVE_DEFRAG_ENABLE_TAKES_NET])])
>
>if cmp -s datapath/linux/kcompat.h.new \
>  datapath/linux/kcompat.h >/dev/null 2>&1; then
> --
> 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] [PATCH 3/4] compat: Fix build error in kernels 4.10+

[Greg Rose]

On Thu, 2017-04-27 at 13:53 -0700, Jarno Rajahalme wrote:
> > On Apr 27, 2017, at 1:08 PM, Greg Rose  wrote:
> > 
> > This is an alternative solution patch for the issue reported by
> > Raymond Burkholder and the patch submitted by Guoshuai Li.  It uses
> > the acinclude.m4 configuration file to check for the net parameter
> > that was added  to the ipv4 and ipv6 frags init functions in the 4.10
> > Linux kernel to check whether DEFRAG_ENABLE_TAKES_NET should be
> > set and then checks for that at compile time.
> > 
> > Reported-by: Raymond Burkholder 
> > CC: Guoshuai Li 
> > Signed-off-by: Greg Rose 
> > ---
> > datapath/linux/compat/ip_fragment.c| 14 ++
> > datapath/linux/compat/nf_conntrack_reasm.c | 14 ++
> > 2 files changed, 28 insertions(+)
> > 
> > diff --git a/datapath/linux/compat/ip_fragment.c 
> > b/datapath/linux/compat/ip_fragment.c
> > index b0f5d0e..fccd992 100644
> > --- a/datapath/linux/compat/ip_fragment.c
> > +++ b/datapath/linux/compat/ip_fragment.c
> > @@ -729,18 +729,32 @@ int rpl_ip_defrag(struct net *net, struct sk_buff 
> > *skb, u32 user)
> > return -ENOMEM;
> > }
> > 
> > +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> > +static int __net_init ipv4_frags_init_net(struct net *net)
> > +{
> > +   nf_defrag_ipv4_enable(net);
> > +
> > +   return 0;
> > +}
> > +#endif
> > +
> 
> Did you consider Joe’s proposal to pass the error return to the caller? If it 
> makes sense, then maybe we could use nf_ functions directly and not define 
> the _init_net() functions at all (as the stubs prototype is the same as the 
> _enable function prototype, except for the “__net_init” attribute)?

I must have missed that.  Let me go back to the list and see if I can
find it.

Thanks for the review!

- Greg

> 
> > static void __net_exit ipv4_frags_exit_net(struct net *net)
> > {
> > inet_frags_exit_net(>ipv4.frags, _frags);
> > }
> > 
> > static struct pernet_operations ip4_frags_ops = {
> > +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> > +   .init = ipv4_frags_init_net,
> > +#endif
> > .exit = ipv4_frags_exit_net,
> > };
> > 
> > int __init rpl_ipfrag_init(void)
> > {
> > +#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
> > nf_defrag_ipv4_enable();
> > +#endif
> > register_pernet_subsys(_frags_ops);
> > ip4_frags.hashfn = ip4_hashfn;
> > ip4_frags.constructor = ip4_frag_init;
> > diff --git a/datapath/linux/compat/nf_conntrack_reasm.c 
> > b/datapath/linux/compat/nf_conntrack_reasm.c
> > index 0bc4d9e..701faf5 100644
> > --- a/datapath/linux/compat/nf_conntrack_reasm.c
> > +++ b/datapath/linux/compat/nf_conntrack_reasm.c
> > @@ -558,12 +558,24 @@ out_unlock:
> > return ret;
> > }
> > 
> > +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> > +static int nf_ct_net_init(struct net *net)
> > +{
> > +   nf_defrag_ipv6_enable(net);
> > +
> > +   return 0;
> > +}
> > +#endif
> > +
> > static void nf_ct_net_exit(struct net *net)
> > {
> > inet_frags_exit_net(>nf_frag.frags, _frags);
> > }
> > 
> > static struct pernet_operations nf_ct_net_ops = {
> > +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> > +   .init = nf_ct_net_init,
> > +#endif
> > .exit = nf_ct_net_exit,
> > };
> > 
> > @@ -571,7 +583,9 @@ int rpl_nf_ct_frag6_init(void)
> > {
> > int ret = 0;
> > 
> > +#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
> > nf_defrag_ipv6_enable();
> > +#endif
> > nf_frags.hashfn = nf_hashfn;
> > nf_frags.constructor = ip6_frag_init;
> > nf_frags.destructor = NULL;
> > -- 
> > 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] [PATCH] datapath-windows: Add missing IPCT_LABEL.

[Jarno Rajahalme]

Pushed to master,

  Jarno

> On Apr 27, 2017, at 11:07 AM, Sairam Venugopal  wrote:
> 
> Thanks for adding this in.
> 
> Acked-by: Sairam Venugopal 
> 
> 
> 
> 
> 
> On 4/19/17, 7:01 PM, "ovs-dev-boun...@openvswitch.org on behalf of Jarno 
> Rajahalme"  wrote:
> 
>> Add the missing enum definition for IPCT_LABEL.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> datapath-windows/include/OvsDpInterfaceCtExt.h | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h 
>> b/datapath-windows/include/OvsDpInterfaceCtExt.h
>> index 2795edc..3b94778 100644
>> --- a/datapath-windows/include/OvsDpInterfaceCtExt.h
>> +++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
>> @@ -132,6 +132,7 @@ enum ip_conntrack_events {
>>IPCT_MARK,
>>IPCT_NATSEQADJ,
>>IPCT_SECMARK,
>> +IPCT_LABEL,
>> };
>> 
>> enum ip_conntrack_expect_events {
>> -- 
>> 2.1.4
>> 
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo=R8iw4Jr-HMBdatGO514ei63gCELrhnMK2QS9dEZUYCA=IjblaY4BjRXCwAXsMU3BHL3ShZjfcICvbBGlg1afGfY=
>>  

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

Re: [ovs-dev] [PATCH 3/4] compat: Fix build error in kernels 4.10+

[Jarno Rajahalme]

> On Apr 27, 2017, at 1:08 PM, Greg Rose  wrote:
> 
> This is an alternative solution patch for the issue reported by
> Raymond Burkholder and the patch submitted by Guoshuai Li.  It uses
> the acinclude.m4 configuration file to check for the net parameter
> that was added  to the ipv4 and ipv6 frags init functions in the 4.10
> Linux kernel to check whether DEFRAG_ENABLE_TAKES_NET should be
> set and then checks for that at compile time.
> 
> Reported-by: Raymond Burkholder 
> CC: Guoshuai Li 
> Signed-off-by: Greg Rose 
> ---
> datapath/linux/compat/ip_fragment.c| 14 ++
> datapath/linux/compat/nf_conntrack_reasm.c | 14 ++
> 2 files changed, 28 insertions(+)
> 
> diff --git a/datapath/linux/compat/ip_fragment.c 
> b/datapath/linux/compat/ip_fragment.c
> index b0f5d0e..fccd992 100644
> --- a/datapath/linux/compat/ip_fragment.c
> +++ b/datapath/linux/compat/ip_fragment.c
> @@ -729,18 +729,32 @@ int rpl_ip_defrag(struct net *net, struct sk_buff *skb, 
> u32 user)
>   return -ENOMEM;
> }
> 
> +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> +static int __net_init ipv4_frags_init_net(struct net *net)
> +{
> + nf_defrag_ipv4_enable(net);
> +
> + return 0;
> +}
> +#endif
> +

Did you consider Joe’s proposal to pass the error return to the caller? If it 
makes sense, then maybe we could use nf_ functions directly and not define the 
_init_net() functions at all (as the stubs prototype is the same as the _enable 
function prototype, except for the “__net_init” attribute)?

> static void __net_exit ipv4_frags_exit_net(struct net *net)
> {
>   inet_frags_exit_net(>ipv4.frags, _frags);
> }
> 
> static struct pernet_operations ip4_frags_ops = {
> +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> + .init = ipv4_frags_init_net,
> +#endif
>   .exit = ipv4_frags_exit_net,
> };
> 
> int __init rpl_ipfrag_init(void)
> {
> +#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
>   nf_defrag_ipv4_enable();
> +#endif
>   register_pernet_subsys(_frags_ops);
>   ip4_frags.hashfn = ip4_hashfn;
>   ip4_frags.constructor = ip4_frag_init;
> diff --git a/datapath/linux/compat/nf_conntrack_reasm.c 
> b/datapath/linux/compat/nf_conntrack_reasm.c
> index 0bc4d9e..701faf5 100644
> --- a/datapath/linux/compat/nf_conntrack_reasm.c
> +++ b/datapath/linux/compat/nf_conntrack_reasm.c
> @@ -558,12 +558,24 @@ out_unlock:
>   return ret;
> }
> 
> +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> +static int nf_ct_net_init(struct net *net)
> +{
> + nf_defrag_ipv6_enable(net);
> +
> + return 0;
> +}
> +#endif
> +
> static void nf_ct_net_exit(struct net *net)
> {
>   inet_frags_exit_net(>nf_frag.frags, _frags);
> }
> 
> static struct pernet_operations nf_ct_net_ops = {
> +#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
> + .init = nf_ct_net_init,
> +#endif
>   .exit = nf_ct_net_exit,
> };
> 
> @@ -571,7 +583,9 @@ int rpl_nf_ct_frag6_init(void)
> {
>   int ret = 0;
> 
> +#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
>   nf_defrag_ipv6_enable();
> +#endif
>   nf_frags.hashfn = nf_hashfn;
>   nf_frags.constructor = ip6_frag_init;
>   nf_frags.destructor = NULL;
> -- 
> 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

[ovs-dev] [PATCH 4/4] travis: Update kernels to kernel.org latest

[Greg Rose]

Signed-off-by: Greg Rose 
---
 .travis.yml | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 6f2b065..3a73873 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -30,13 +30,13 @@ env:
   - BUILD_ENV="-m32" OPTS="--disable-ssl"
   - KERNEL=3.16.39 DPDK=1
   - KERNEL=3.16.39 DPDK=1 OPTS="--enable-shared"
-  - KERNEL=4.9
-  - KERNEL=4.8.14
-  - KERNEL=4.4.38
-  - KERNEL=4.1.36
-  - KERNEL=3.18.45
-  - KERNEL=3.12.68
-  - KERNEL=3.10.104
+  - KERNEL=4.10.12
+  - KERNEL=4.9.24
+  - KERNEL=4.4.63
+  - KERNEL=4.1.39
+  - KERNEL=3.18.50
+  - KERNEL=3.12.73
+  - KERNEL=3.10.105
   - TESTSUITE=1 LIBS=-ljemalloc
 
 matrix:
-- 
1.8.3.1

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

[ovs-dev] [PATCH 3/4] compat: Fix build error in kernels 4.10+

[Greg Rose]

This is an alternative solution patch for the issue reported by
Raymond Burkholder and the patch submitted by Guoshuai Li.  It uses
the acinclude.m4 configuration file to check for the net parameter
that was added  to the ipv4 and ipv6 frags init functions in the 4.10
Linux kernel to check whether DEFRAG_ENABLE_TAKES_NET should be
set and then checks for that at compile time.

Reported-by: Raymond Burkholder 
CC: Guoshuai Li 
Signed-off-by: Greg Rose 
---
 datapath/linux/compat/ip_fragment.c| 14 ++
 datapath/linux/compat/nf_conntrack_reasm.c | 14 ++
 2 files changed, 28 insertions(+)

diff --git a/datapath/linux/compat/ip_fragment.c 
b/datapath/linux/compat/ip_fragment.c
index b0f5d0e..fccd992 100644
--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -729,18 +729,32 @@ int rpl_ip_defrag(struct net *net, struct sk_buff *skb, 
u32 user)
return -ENOMEM;
 }
 
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+static int __net_init ipv4_frags_init_net(struct net *net)
+{
+   nf_defrag_ipv4_enable(net);
+
+   return 0;
+}
+#endif
+
 static void __net_exit ipv4_frags_exit_net(struct net *net)
 {
inet_frags_exit_net(>ipv4.frags, _frags);
 }
 
 static struct pernet_operations ip4_frags_ops = {
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+   .init = ipv4_frags_init_net,
+#endif
.exit = ipv4_frags_exit_net,
 };
 
 int __init rpl_ipfrag_init(void)
 {
+#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
nf_defrag_ipv4_enable();
+#endif
register_pernet_subsys(_frags_ops);
ip4_frags.hashfn = ip4_hashfn;
ip4_frags.constructor = ip4_frag_init;
diff --git a/datapath/linux/compat/nf_conntrack_reasm.c 
b/datapath/linux/compat/nf_conntrack_reasm.c
index 0bc4d9e..701faf5 100644
--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -558,12 +558,24 @@ out_unlock:
return ret;
 }
 
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+static int nf_ct_net_init(struct net *net)
+{
+   nf_defrag_ipv6_enable(net);
+
+   return 0;
+}
+#endif
+
 static void nf_ct_net_exit(struct net *net)
 {
inet_frags_exit_net(>nf_frag.frags, _frags);
 }
 
 static struct pernet_operations nf_ct_net_ops = {
+#ifdef HAVE_DEFRAG_ENABLE_TAKES_NET
+   .init = nf_ct_net_init,
+#endif
.exit = nf_ct_net_exit,
 };
 
@@ -571,7 +583,9 @@ int rpl_nf_ct_frag6_init(void)
 {
int ret = 0;
 
+#ifndef HAVE_DEFRAG_ENABLE_TAKES_NET
nf_defrag_ipv6_enable();
+#endif
nf_frags.hashfn = nf_hashfn;
nf_frags.constructor = ip6_frag_init;
nf_frags.destructor = NULL;
-- 
1.8.3.1

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

[ovs-dev] [PATCH 1/4] acinclude.m4: Fix parenthetical balance

[Greg Rose]

Parenthetical imbalance was causing some checks to not run

Signed-off-by: Greg Rose 
---
 acinclude.m4 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 9f8e30d..0d6647e 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -681,9 +681,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
 
   OVS_GREP_IFELSE([$KSRC/include/net/vxlan.h], [struct vxlan_metadata],
   [OVS_DEFINE([HAVE_VXLAN_METADATA])])
-  OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [udp_flow_src_port],
-  [OVS_GREP_IFELSE([$KSRC/include/net/udp.h], 
[inet_get_local_port_range(net],
-   [OVS_DEFINE([HAVE_UDP_FLOW_SRC_PORT])])])
+  OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [udp_flow_src_port])
+  OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [inet_get_local_port_range(net)],
+   [OVS_DEFINE([HAVE_UDP_FLOW_SRC_PORT])])
   OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [udp_v4_check])
   OVS_GREP_IFELSE([$KSRC/include/net/udp_tunnel.h], [udp_tunnel_gro_complete])
   OVS_GREP_IFELSE([$KSRC/include/net/udp_tunnel.h], 
[sk_buff.*udp_tunnel_handle_offloads],
-- 
1.8.3.1

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

[ovs-dev] [PATCH 0/4] Fixup 4.10 kernel compatability layer

[Greg Rose]

* Fixes a small bug in the acinclude.m4 file
* Adds check in acinclude.m4 for the frag init struct *net requirement
* Fixes up the ip_fragment.c and nf_conntrack_reasm.c compat files
* Updates the .travis.yml build verification script

Verified here:

https://travis-ci.org/gvrose8192/ovs-experimental

Greg Rose (4):
  acinclude.m4: Fix parenthetical balance
  acinclude.m4: Add check for struct net parameter
  compat: Fix build error in kernels 4.10+
  travis: Update kernels to kernel.org latest

 .travis.yml| 14 +++---
 acinclude.m4   | 10 ++
 datapath/linux/compat/ip_fragment.c| 14 ++
 datapath/linux/compat/nf_conntrack_reasm.c | 14 ++
 4 files changed, 41 insertions(+), 11 deletions(-)

-- 
1.8.3.1

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

[ovs-dev] [PATCH] db-ctl-base: Allow record UUIDs to be abbreviated.

[Ben Pfaff]

This makes it easier to type ovs-vsctl, ovn-sbctl, ovn-nbctl, and vtep-ctl
commands without cut-and-paste.

Signed-off-by: Ben Pfaff 
---
 NEWS  |  4 
 lib/db-ctl-base.c | 28 
 ovn/utilities/ovn-nbctl.8.xml | 15 +--
 ovn/utilities/ovn-sbctl.8.in  | 26 ++
 ovn/utilities/ovn-sbctl.c |  3 +++
 utilities/ovs-vsctl.8.in  | 10 ++
 vtep/vtep-ctl.8.in| 10 ++
 7 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/NEWS b/NEWS
index ea97d84a2dea..7a7e7fcd8a3a 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,8 @@ Post-v2.7.0
still can be configured via extra arguments for DPDK EAL.
- New support for multiple VLANs (802.1ad or "QinQ"), including a new
  "dot1q-tunnel" port VLAN mode.
+   - In ovn-vsctl and vtep-ctl, record UUIDs in commands may now be
+ abbreviated to 4 hex digits.
- OVN:
  * IPAM for IPv4 can now exclude user-defined addresses from assignment.
  * IPAM can now assign IPv6 addresses.
@@ -20,6 +22,8 @@ Post-v2.7.0
  * Allow ovn-controller SSL configuration to be obtained from vswitchd
database.
  * ovn-trace now has basic support for tracing distributed firewalls.
+ * In ovn-nbctl and ovn-sbctl, record UUIDs in commands may now be
+   abbreviated to 4 hex digits.
- Add the command 'ovs-appctl stp/show' (see ovs-vswitchd(8)).
- OpenFlow:
  * Increased support for OpenFlow 1.6 (draft).
diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index afc70eb06e36..ad98454c903d 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -300,6 +300,14 @@ get_row_by_id(struct ctl_context *ctx,
 return final;
 }
 
+static bool
+is_partial_uuid_match(const struct uuid *uuid, const char *match)
+{
+char uuid_s[UUID_LEN + 1];
+snprintf(uuid_s, sizeof uuid_s, UUID_FMT, UUID_ARGS(uuid));
+return !strncmp(uuid_s, match, strlen(match));
+}
+
 static const struct ovsdb_idl_row *
 get_row(struct ctl_context *ctx,
 const struct ovsdb_idl_table_class *table, const char *record_id,
@@ -330,6 +338,26 @@ get_row(struct ctl_context *ctx,
 }
 }
 }
+if (!row
+&& record_id[uuid_is_partial_string(record_id)] == '\0'
+&& strlen(record_id) >= 4) {
+for (const struct ovsdb_idl_row *r = ovsdb_idl_first_row(ctx->idl,
+ table);
+ r != NULL;
+ r = ovsdb_idl_next_row(r)) {
+if (is_partial_uuid_match(>uuid, record_id)) {
+if (!row) {
+row = r;
+} else {
+ctl_fatal("%s contains 2 or more rows whose UUIDs begin "
+  "with %s: at least "UUID_FMT" and "UUID_FMT,
+  table->name, record_id,
+  UUID_ARGS(>uuid),
+  UUID_ARGS(>uuid));
+}
+}
+}
+}
 if (must_exist && !row) {
 ctl_fatal("no row \"%s\" in table %s", record_id, table->name);
 }
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 70afc1080f62..adea29a4e84f 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -669,12 +669,13 @@
 Database Commands
 These commands query and modify the contents of ovsdb 
tables.
 They are a slight abstraction of the ovsdb interface and
-as suchthey operate at a lower level than other ovn-nbctl 
commands.
+as such they operate at a lower level than other ovn-nbctl 
commands.
 Identifying Tables, Records, and Columns
 Each of these commands has a table parameter to identify a 
table
 within the database.  Many of them also take a record parameter
 that identifies a particular record within a table.  The record
-parameter may be the UUID for a record, and many tables offer
+parameter may be the UUID for a record, which may be abbreviated to its
+first 4 (or more) hex digits, as long as that is unique.  Many tables offer
 additional ways to identify records.  Some commands also take
 column parameters that identify a particular field within the
 records in a table.
@@ -737,6 +738,16 @@
 
 
 
+
+  Record names must be specified in full and with correct capitalization,
+  except that UUIDs may be abbreviated to their first 4 (or more) hex
+  digits, as long as that is unique within the table.  Names of tables and
+  columns are not case-sensitive, and - and _ are
+  treated interchangeably.  Unique abbreviations of table and column names
+  are acceptable, e.g. d or dhcp is sufficient
+  to identify the DHCP_Options table.
+
+
 http://www.w3.org/2003/XInclude"/>
 
 Synchronization Commands
diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in
index 

Re: [ovs-dev] [PATCH] build: Don't run tests in rpm makefile targets.

[Flavio Leitner]

On Thu, Apr 27, 2017 at 03:04:43PM -0400, Russell Bryant wrote:
> On Fri, Apr 21, 2017 at 7:01 PM, Ben Pfaff  wrote:
> > On Fri, Mar 31, 2017 at 11:27:23AM -0400, Russell Bryant wrote:
> >> The RPM build makefile targets are helpful during development and testing,
> >> but I personally almost never want the tests to run when I use them.
> >> Leave tests on by default in the spec file for when the package is built by
> >> distro build systems, but disable it by default in the Makefile targets and
> >> update the documentation accordingly.
> >>
> >> Signed-off-by: Russell Bryant 
> >
> > I'm OK with this.
> 
> Flavio, Lance, Aaron, have any opinions on this?  It's not that big of
> a deal.  I guess I was feeling both slightly annoyed and lazy that
> day.  :-)

I am also OK.

-- 
Flavio



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

Re: [ovs-dev] [PATCH] build: Don't run tests in rpm makefile targets.

[Lance Richardson]

> From: "Russell Bryant" 
> To: "Ben Pfaff" 
> Cc: "ovs dev" , "Flavio Leitner" , 
> "Lance Richardson" ,
> "Aaron Conole" 
> Sent: Thursday, 27 April, 2017 3:04:43 PM
> Subject: Re: [ovs-dev] [PATCH] build: Don't run tests in rpm makefile targets.
> 
> On Fri, Apr 21, 2017 at 7:01 PM, Ben Pfaff  wrote:
> > On Fri, Mar 31, 2017 at 11:27:23AM -0400, Russell Bryant wrote:
> >> The RPM build makefile targets are helpful during development and testing,
> >> but I personally almost never want the tests to run when I use them.
> >> Leave tests on by default in the spec file for when the package is built
> >> by
> >> distro build systems, but disable it by default in the Makefile targets
> >> and
> >> update the documentation accordingly.
> >>
> >> Signed-off-by: Russell Bryant 
> >
> > I'm OK with this.
> 
> Flavio, Lance, Aaron, have any opinions on this?  It's not that big of
> a deal.  I guess I was feeling both slightly annoyed and lazy that
> day.  :-)
> 

Seems fine to me.

Acked-by: Lance Richardson 

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

Re: [ovs-dev] [PATCH] build: Don't run tests in rpm makefile targets.

[Russell Bryant]

On Fri, Apr 21, 2017 at 7:01 PM, Ben Pfaff  wrote:
> On Fri, Mar 31, 2017 at 11:27:23AM -0400, Russell Bryant wrote:
>> The RPM build makefile targets are helpful during development and testing,
>> but I personally almost never want the tests to run when I use them.
>> Leave tests on by default in the spec file for when the package is built by
>> distro build systems, but disable it by default in the Makefile targets and
>> update the documentation accordingly.
>>
>> Signed-off-by: Russell Bryant 
>
> I'm OK with this.

Flavio, Lance, Aaron, have any opinions on this?  It's not that big of
a deal.  I guess I was feeling both slightly annoyed and lazy that
day.  :-)

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

Re: [ovs-dev] [PATCH] datapath-windows: Add missing IPCT_LABEL.

[Sairam Venugopal]

Thanks for adding this in.

Acked-by: Sairam Venugopal 





On 4/19/17, 7:01 PM, "ovs-dev-boun...@openvswitch.org on behalf of Jarno 
Rajahalme"  wrote:

>Add the missing enum definition for IPCT_LABEL.
>
>Signed-off-by: Jarno Rajahalme 
>---
> datapath-windows/include/OvsDpInterfaceCtExt.h | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h 
>b/datapath-windows/include/OvsDpInterfaceCtExt.h
>index 2795edc..3b94778 100644
>--- a/datapath-windows/include/OvsDpInterfaceCtExt.h
>+++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
>@@ -132,6 +132,7 @@ enum ip_conntrack_events {
> IPCT_MARK,
> IPCT_NATSEQADJ,
> IPCT_SECMARK,
>+IPCT_LABEL,
> };
> 
> enum ip_conntrack_expect_events {
>-- 
>2.1.4
>
>___
>dev mailing list
>d...@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo=R8iw4Jr-HMBdatGO514ei63gCELrhnMK2QS9dEZUYCA=IjblaY4BjRXCwAXsMU3BHL3ShZjfcICvbBGlg1afGfY=
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/2] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Jarno Rajahalme]

> On Apr 27, 2017, at 10:18 AM, Joe Stringer  wrote:
> 
> On 24 April 2017 at 19:09, Jarno Rajahalme  > wrote:
>> Specify the event mask with CT commit including bits for CT features
>> exposed at the OVS interface (mark and label changes in addition to
>> basic creation and destruction of conntrack entries).
>> 
>> Without this any listener of conntrack update events will typically
>> (depending on system configuration) receive events for each L4 (e.g.,
>> TCP) state machine change, which can multiply the number of events
>> received per connection.
>> 
>> By including the new, related, and destroy events any listener of new
>> conntrack events gets notified of new related and non-related
>> connections, and any listener of destroy events will get notified of
>> deleted (typically timed out) conntrack entries.
>> 
>> By including the flags for mark and labels, any listener of conntrack
>> update events gets notified whenever the connmark or conntrack labels
>> are chnaged from the values reported within the new events.
> 
> s/chnaged/changed/
> 
>> 
>> VMware-BZ: #1837218
>> Signed-off-by: Jarno Rajahalme >
>> ---
> 
> 
> Acked-by: Joe Stringer >

Thanks for the review!

I need a review on the related windows-datapath change (“datapath-windows: Add 
missing IPCT_LABEL.”) before I can push this!

  Jarno

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

Re: [ovs-dev] [PATCH] datapath: Delete conntrack entry clashing with an expectation.

[Jarno Rajahalme]

> On Apr 27, 2017, at 10:36 AM, Joe Stringer  wrote:
> 
> On 26 April 2017 at 13:13, Jarno Rajahalme  wrote:
>> Upstream commit:
>> 
>>commit cf5d70918877c6a6655dc1e92e2ebb661ce904fd
>>Author: Jarno Rajahalme 
>>Date:   Fri Apr 14 14:26:38 2017 -0700
>> 
>>openvswitch: Delete conntrack entry clashing with an expectation.
>> 
>>Conntrack helpers do not check for a potentially clashing conntrack
>>entry when creating a new expectation.  Also, nf_conntrack_in() will
>>check expectations (via init_conntrack()) only if a conntrack entry
>>can not be found.  The expectation for a packet which also matches an
>>existing conntrack entry will not be removed by conntrack, and is
>>currently handled inconsistently by OVS, as OVS expects the
>>expectation to be removed when the connection tracking entry matching
>>that expectation is confirmed.
>> 
>>It should be noted that normally an IP stack would not allow reuse of
>>a 5-tuple of an old (possibly lingering) connection for a new data
>>connection, so this is somewhat unlikely corner case.  However, it is
>>possible that a misbehaving source could cause conntrack entries be
>>created that could then interfere with new related connections.
>> 
>>Fix this in the OVS module by deleting the clashing conntrack entry
>>after an expectation has been matched.  This causes the following
>>nf_conntrack_in() call also find the expectation and remove it when
>>creating the new conntrack entry, as well as the forthcoming reply
>>direction packets to match the new related connection instead of the
>>old clashing conntrack entry.
>> 
>>Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
>>Reported-by: Yang Song 
>>Signed-off-by: Jarno Rajahalme 
>>Acked-by: Joe Stringer 
>>Signed-off-by: Pablo Neira Ayuso 
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
> 
> Acked-by: Joe Stringer 

Thanks for the review, pushed to master,

  Jarno


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

Re: [ovs-dev] [PATCH v2 2/2] datapath: nf_connlabels_replace() backport.

[Jarno Rajahalme]

> On Apr 27, 2017, at 10:31 AM, Joe Stringer  wrote:
> 
> On 24 April 2017 at 19:09, Jarno Rajahalme  wrote:
>> Linux 4.7 changed nf_connlabels_replace() to trigger conntrack event
>> for a label change only when the labels actually changed.  Without
>> this change an update event is triggered even if the labels already
>> have the values they are being set to.
>> 
>> There is no way we can detect this functional change from Linux
>> headers, so provide replacements that work the same for older Linux
>> releases regardless if a distribution provides backports or not.
>> 
>> VMware-BZ: #1837218
>> Signed-off-by: Jarno Rajahalme 
>> ---
> 
> I assume that by "Linux 4.7", you're actually referring to the following 
> commit?
> 5a8145f7b222 ("netfilter: labels: don't emit ct event if labels were
> not changed")
> 
> It might be helpful for future reference (including review) to refer
> to this commit in the code comment below.
> 

Right, thanks for suggesting this. I added this to both the commit message and 
the code comment.

> Otherwise LGTM.
> 
> Acked-by: Joe Stringer 

Pushed to master,

  Jarno

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

Re: [ovs-dev] [PATCH 2/2] datapath: Add eventmask support to CT action.

[Jarno Rajahalme]

> On Apr 27, 2017, at 10:15 AM, Joe Stringer  wrote:
> 
> On 24 April 2017 at 13:50, Jarno Rajahalme  wrote:
>> Upstream commit:
>> 
>>commit 120645513f55a4ac5543120d9e79925d30a0156f
>>Author: Jarno Rajahalme 
>>Date:   Fri Apr 21 16:48:06 2017 -0700
>> 
>>openvswitch: Add eventmask support to CT action.
>> 
>>Add a new optional conntrack action attribute OVS_CT_ATTR_EVENTMASK,
>>which can be used in conjunction with the commit flag
>>(OVS_CT_ATTR_COMMIT) to set the mask of bits specifying which
>>conntrack events (IPCT_*) should be delivered via the Netfilter
>>netlink multicast groups.  Default behavior depends on the system
>>configuration, but typically a lot of events are delivered.  This can be
>>very chatty for the NFNLGRP_CONNTRACK_UPDATE group, even if only some
>>types of events are of interest.
>> 
>>Netfilter core init_conntrack() adds the event cache extension, so we
>>only need to set the ctmask value.  However, if the system is
>>configured without support for events, the setting will be skipped due
>>to extension not being found.
>> 
>>Signed-off-by: Jarno Rajahalme 
>>Reviewed-by: Greg Rose 
>>Acked-by: Joe Stringer 
>>Signed-off-by: David S. Miller 
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Joe Stringer 

Thanks for the review, series pushed to master.

  Jarno

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

Re: [ovs-dev] [PATCH] datapath: Delete conntrack entry clashing with an expectation.

[Joe Stringer]

On 26 April 2017 at 13:13, Jarno Rajahalme  wrote:
> Upstream commit:
>
> commit cf5d70918877c6a6655dc1e92e2ebb661ce904fd
> Author: Jarno Rajahalme 
> Date:   Fri Apr 14 14:26:38 2017 -0700
>
> openvswitch: Delete conntrack entry clashing with an expectation.
>
> Conntrack helpers do not check for a potentially clashing conntrack
> entry when creating a new expectation.  Also, nf_conntrack_in() will
> check expectations (via init_conntrack()) only if a conntrack entry
> can not be found.  The expectation for a packet which also matches an
> existing conntrack entry will not be removed by conntrack, and is
> currently handled inconsistently by OVS, as OVS expects the
> expectation to be removed when the connection tracking entry matching
> that expectation is confirmed.
>
> It should be noted that normally an IP stack would not allow reuse of
> a 5-tuple of an old (possibly lingering) connection for a new data
> connection, so this is somewhat unlikely corner case.  However, it is
> possible that a misbehaving source could cause conntrack entries be
> created that could then interfere with new related connections.
>
> Fix this in the OVS module by deleting the clashing conntrack entry
> after an expectation has been matched.  This causes the following
> nf_conntrack_in() call also find the expectation and remove it when
> creating the new conntrack entry, as well as the forthcoming reply
> direction packets to match the new related connection instead of the
> old clashing conntrack entry.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Yang Song 
> Signed-off-by: Jarno Rajahalme 
> Acked-by: Joe Stringer 
> Signed-off-by: Pablo Neira Ayuso 
>
> Signed-off-by: Jarno Rajahalme 
> ---

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

Re: [ovs-dev] [PATCH v2 2/2] datapath: nf_connlabels_replace() backport.

[Joe Stringer]

On 24 April 2017 at 19:09, Jarno Rajahalme  wrote:
> Linux 4.7 changed nf_connlabels_replace() to trigger conntrack event
> for a label change only when the labels actually changed.  Without
> this change an update event is triggered even if the labels already
> have the values they are being set to.
>
> There is no way we can detect this functional change from Linux
> headers, so provide replacements that work the same for older Linux
> releases regardless if a distribution provides backports or not.
>
> VMware-BZ: #1837218
> Signed-off-by: Jarno Rajahalme 
> ---

I assume that by "Linux 4.7", you're actually referring to the following commit?
5a8145f7b222 ("netfilter: labels: don't emit ct event if labels were
not changed")

It might be helpful for future reference (including review) to refer
to this commit in the code comment below.

Otherwise LGTM.

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

Re: [ovs-dev] [PATCH v2 1/2] xlate: Use OVS_CT_ATTR_EVENTMASK.

[Joe Stringer]

On 24 April 2017 at 19:09, Jarno Rajahalme  wrote:
> Specify the event mask with CT commit including bits for CT features
> exposed at the OVS interface (mark and label changes in addition to
> basic creation and destruction of conntrack entries).
>
> Without this any listener of conntrack update events will typically
> (depending on system configuration) receive events for each L4 (e.g.,
> TCP) state machine change, which can multiply the number of events
> received per connection.
>
> By including the new, related, and destroy events any listener of new
> conntrack events gets notified of new related and non-related
> connections, and any listener of destroy events will get notified of
> deleted (typically timed out) conntrack entries.
>
> By including the flags for mark and labels, any listener of conntrack
> update events gets notified whenever the connmark or conntrack labels
> are chnaged from the values reported within the new events.

s/chnaged/changed/

>
> VMware-BZ: #1837218
> Signed-off-by: Jarno Rajahalme 
> ---


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

Re: [ovs-dev] [PATCH 2/2] datapath: Add eventmask support to CT action.

[Joe Stringer]

On 24 April 2017 at 13:50, Jarno Rajahalme  wrote:
> Upstream commit:
>
> commit 120645513f55a4ac5543120d9e79925d30a0156f
> Author: Jarno Rajahalme 
> Date:   Fri Apr 21 16:48:06 2017 -0700
>
> openvswitch: Add eventmask support to CT action.
>
> Add a new optional conntrack action attribute OVS_CT_ATTR_EVENTMASK,
> which can be used in conjunction with the commit flag
> (OVS_CT_ATTR_COMMIT) to set the mask of bits specifying which
> conntrack events (IPCT_*) should be delivered via the Netfilter
> netlink multicast groups.  Default behavior depends on the system
> configuration, but typically a lot of events are delivered.  This can be
> very chatty for the NFNLGRP_CONNTRACK_UPDATE group, even if only some
> types of events are of interest.
>
> Netfilter core init_conntrack() adds the event cache extension, so we
> only need to set the ctmask value.  However, if the system is
> configured without support for events, the setting will be skipped due
> to extension not being found.
>
> Signed-off-by: Jarno Rajahalme 
> Reviewed-by: Greg Rose 
> Acked-by: Joe Stringer 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Jarno Rajahalme 

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

Re: [ovs-dev] [PATCH 1/2] datapath: Typo fix.

[Joe Stringer]

On 24 April 2017 at 13:50, Jarno Rajahalme  wrote:
> Upstream commit:
>
> commit abd0a4f2b41812e9ba334945e256909e3d28da57
> Author: Jarno Rajahalme 
> Date:   Fri Apr 21 16:48:05 2017 -0700
>
> openvswitch: Typo fix.
>
> Fix typo in a comment.
>
> Signed-off-by: Jarno Rajahalme 
> Acked-by: Greg Rose 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Jarno Rajahalme 

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

Re: [ovs-dev] [PATCH 4/5] dpif-netdev: Fix comments for dp_netdev_pmd_thread struct.

[Ben Pfaff]

On Tue, Apr 18, 2017 at 07:13:26PM +, Bodireddy, Bhanuprakash wrote:
> >On Sun, Mar 12, 2017 at 05:33:27PM +, Bhanuprakash Bodireddy wrote:
> >> The sorted subtable ranking patch introduced a classifier instance per
> >> ingress port with its subtables ranked on the frequency of hits. The
> >> pmd thread can have more classifier instances now and solely depends
> >> on the number of ingress ports currently handled by the pmd thread.
> >>
> >> Signed-off-by: Bhanuprakash Bodireddy
> >> 
> >
> >Thank you for improving comments!
> >
> >I'm not the right person to review this patch, but from a process 
> >perspective I
> >find myself wondering whether it corrects a comment that was already wrong
> >before the beginning of the series (in which case it is fine as-is) or 
> >whether it
> >only needs correction following the series (in which case it should be folded
> >into whichever patch made it incorrect).
> >
> >Thanks again!
> 
> Hi Ben,
> Please note that the comments were right initially but after the "subtable 
> ranking" feature got introduced the comments needed correction.  The subtable 
> ranking patch series got merged a while ago  but this particular comment 
> wasn't fixed then. I happened to find this during code inspection.  What's 
> the best way to handle this now?

Ah.

The ideal process would be to send this as an independent patch not part
of a series, and to add a Fixes: tag that cites the commit that made the
comment incorrect.  That should make it clear to everyone what's going
on.

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

Re: [ovs-dev] [PATCH 4/5] dpif-netdev: Fix comments for dp_netdev_pmd_thread struct.

[Darrell Ball]

On 3/12/17, 10:33 AM, "ovs-dev-boun...@openvswitch.org on behalf of 
Bhanuprakash Bodireddy"  wrote:

The sorted subtable ranking patch introduced a classifier instance per
ingress port with its subtables ranked on the frequency of hits. The pmd
thread can have more classifier instances now and solely depends on the
number of ingress ports currently handled by the pmd thread.

Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/dpif-netdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 006cea6..628690a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -507,9 +507,9 @@ struct tx_port {
  * I/O of all non-pmd threads.  There will be no actual thread created
  * for the instance.
  *
- * Each struct has its own flow table and classifier.  Packets received
- * from managed ports are looked up in the corresponding pmd thread's
- * flow table, and are executed with the found actions.
+ * Each struct has its own flow table.  Packets received from managed
+ * ports are looked up in the corresponding pmd thread's flow table, and
+ * are executed with the found actions.
  * */


How about something like this ?

+ * Each struct has its own flow cache and classifier per managed inport.  
Packets
+ *received from managed ports are looked up in the corresponding pmd
+ * thread's flow cache and corresponding classifier, if the flow cache 
misses.
+ * Packets are executed with the found actions in either case.




 struct dp_netdev_pmd_thread {
 struct dp_netdev *dp;
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=BzWvTpxh4aosMWU3tYyArvcIZSrYJOZDD5DbkkfruuM=kiwt5ivNb7cbSl6ewXwPqbc9KUO75L8zJO3HO0vyhnE=
 


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

Re: [ovs-dev] [PATCH] bridge: Prohibit "default" bridge name.

[Ben Pfaff]

On Thu, Apr 27, 2017 at 04:02:10AM -0700, William Tu wrote:
> Before the patch, when users create bridge named "default", although
> ovs-vsctl fails but vswitchd in the background will keep retrying it,
> causing the systemd-udev to reach 100% cpu utilization.  The reason is
> due to frequent calls into kernel's register_netdevice function,
> which will invoke several kernel elements who has registered on the
> netdevice notifier chain.  One of the notifier, the inetdev_event rejects
> this devname and register_netdevice fails.  The patch prohibits creating
> "default" bridge name.
> 
> VMWare-BZ: #1842388
> Signed-off-by: William Tu 

It all seems very arbitrary. Do we understand why this is an invalid
device name?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-nbctl: Drop gratuitous indentation for "show" output.

[Ben Pfaff]

"ovn-nbctl show" indented every line of output by at least 4 spaces, which
needlessly wastes horizontal space.  This drops 4 spaces of indent from
each line of output.

Signed-off-by: Ben Pfaff 
---
 ovn/utilities/ovn-nbctl.c | 26 +-
 tests/ovn-nbctl.at| 16 
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index e5999a654131..476b03e57c72 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -569,18 +569,18 @@ lb_by_name_or_uuid(struct ctl_context *ctx, const char 
*id, bool must_exist)
 static void
 print_lr(const struct nbrec_logical_router *lr, struct ds *s)
 {
-ds_put_format(s, "router "UUID_FMT" (%s)\n",
+ds_put_format(s, "router "UUID_FMT" (%s)\n",
   UUID_ARGS(>header_.uuid), lr->name);
 
 for (size_t i = 0; i < lr->n_ports; i++) {
 const struct nbrec_logical_router_port *lrp = lr->ports[i];
-ds_put_format(s, "port %s\n", lrp->name);
+ds_put_format(s, "port %s\n", lrp->name);
 if (lrp->mac) {
-ds_put_cstr(s, "mac: ");
+ds_put_cstr(s, "mac: ");
 ds_put_format(s, "\"%s\"\n", lrp->mac);
 }
 if (lrp->n_networks) {
-ds_put_cstr(s, "networks: [");
+ds_put_cstr(s, "networks: [");
 for (size_t j = 0; j < lrp->n_networks; j++) {
 ds_put_format(s, "%s\"%s\"",
 j == 0 ? "" : ", ",
@@ -592,13 +592,13 @@ print_lr(const struct nbrec_logical_router *lr, struct ds 
*s)
 
 for (size_t i = 0; i < lr->n_nat; i++) {
 const struct nbrec_nat *nat = lr->nat[i];
-ds_put_format(s, "nat "UUID_FMT"\n",
+ds_put_format(s, "nat "UUID_FMT"\n",
   UUID_ARGS(>header_.uuid));
-ds_put_cstr(s, "external ip: ");
+ds_put_cstr(s, "external ip: ");
 ds_put_format(s, "\"%s\"\n", nat->external_ip);
-ds_put_cstr(s, "logical ip: ");
+ds_put_cstr(s, "logical ip: ");
 ds_put_format(s, "\"%s\"\n", nat->logical_ip);
-ds_put_cstr(s, "type: ");
+ds_put_cstr(s, "type: ");
 ds_put_format(s, "\"%s\"\n", nat->type);
 }
 }
@@ -606,21 +606,21 @@ print_lr(const struct nbrec_logical_router *lr, struct ds 
*s)
 static void
 print_ls(const struct nbrec_logical_switch *ls, struct ds *s)
 {
-ds_put_format(s, "switch "UUID_FMT" (%s)\n",
+ds_put_format(s, "switch "UUID_FMT" (%s)\n",
   UUID_ARGS(>header_.uuid), ls->name);
 
 for (size_t i = 0; i < ls->n_ports; i++) {
 const struct nbrec_logical_switch_port *lsp = ls->ports[i];
 
-ds_put_format(s, "port %s\n", lsp->name);
+ds_put_format(s, "port %s\n", lsp->name);
 if (lsp->parent_name) {
-ds_put_format(s, "parent: %s\n", lsp->parent_name);
+ds_put_format(s, "parent: %s\n", lsp->parent_name);
 }
 if (lsp->n_tag) {
-ds_put_format(s, "tag: %"PRIu64"\n", lsp->tag[0]);
+ds_put_format(s, "tag: %"PRIu64"\n", lsp->tag[0]);
 }
 if (lsp->n_addresses) {
-ds_put_cstr(s, "addresses: [");
+ds_put_cstr(s, "addresses: [");
 for (size_t j = 0; j < lsp->n_addresses; j++) {
 ds_put_format(s, "%s\"%s\"",
 j == 0 ? "" : ", ",
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 9f0a2779bec1..975d70287983 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -56,14 +56,14 @@ AT_CHECK([ovn-nbctl ls-list | ${PERL} $srcdir/uuidfilt.pl], 
[0], [dnl
 AT_CHECK([ovn-nbctl show ls0])
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl show ls0 | ${PERL} $srcdir/uuidfilt.pl], [0],
-  [switch <0> (ls0)
+  [switch <0> (ls0)
 ])
 AT_CHECK([ovn-nbctl ls-add ls0], [1], [],
   [ovn-nbctl: ls0: a switch with this name already exists
 ])
 AT_CHECK([ovn-nbctl --may-exist ls-add ls0])
 AT_CHECK([ovn-nbctl show ls0 | ${PERL} $srcdir/uuidfilt.pl], [0],
-  [switch <0> (ls0)
+  [switch <0> (ls0)
 ])
 AT_CHECK([ovn-nbctl --add-duplicate ls-add ls0])
 AT_CHECK([ovn-nbctl --may-exist --add-duplicate ls-add ls0], [1], [],
@@ -645,14 +645,14 @@ AT_CHECK([ovn-nbctl lr-list | ${PERL} 
$srcdir/uuidfilt.pl], [0], [dnl
 AT_CHECK([ovn-nbctl show lr0])
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl show lr0 | ${PERL} $srcdir/uuidfilt.pl], [0],
-  [router <0> (lr0)
+  [router <0> (lr0)
 ])
 AT_CHECK([ovn-nbctl lr-add lr0], [1], [],
   [ovn-nbctl: lr0: a router with this name already exists
 ])
 AT_CHECK([ovn-nbctl --may-exist lr-add lr0])
 AT_CHECK([ovn-nbctl show lr0 | ${PERL} $srcdir/uuidfilt.pl], [0],
-  [router <0> (lr0)
+  [router <0> (lr0)
 ])
 

Re: [ovs-dev] [PATCH 2/2] revalidator: Improve logging for transition_ukey().

[Ben Pfaff]

On Thu, Apr 27, 2017 at 09:24:04AM -0700, Joe Stringer wrote:
> On 26 April 2017 at 18:35, Jarno Rajahalme  wrote:
> >
> >> On Apr 26, 2017, at 6:03 PM, Joe Stringer  wrote:
> >>
> >> There are a few cases where more introspection into ukey transitions
> >> would be relevant for logging or assertion. Track the SOURCE_LOCATOR and
> >> thread id when states are transitioned and use these for logging.
> >>
> >> Suggested-by: Jarno Rajahalme 
> >> Signed-off-by: Joe Stringer 
> >> ---
> >> ofproto/ofproto-dpif-upcall.c | 17 -
> >> 1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> >> index ccf15a3c80b3..8aff613161d9 100644
> >> --- a/ofproto/ofproto-dpif-upcall.c
> >> +++ b/ofproto/ofproto-dpif-upcall.c
> >> @@ -281,6 +281,10 @@ struct udpif_key {
> >> uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. 
> >> */
> >> enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
> >>
> >> +/* 'state' debug information. */
> >> +unsigned int state_thread OVS_GUARDED;/* Thread that transitions. 
> >> */
> >> +const char *state_where OVS_GUARDED;  /* transition_ukey() 
> >> locator. */
> >> +
> >> /* Datapath flow actions as nlattrs.  Protected by RCU.  Read with
> >>  * ukey_get_actions(), and write with ukey_set_actions(). */
> >> OVSRCU_TYPE(struct ofpbuf *) actions;
> >> @@ -1484,6 +1488,8 @@ ukey_create__(const struct nlattr *key, size_t 
> >> key_len,
> >> ukey->dump_seq = dump_seq;
> >> ukey->reval_seq = reval_seq;
> >> ukey->state = UKEY_CREATED;
> >> +ukey->state_thread = ovsthread_id_self();
> >> +ukey->state_where = OVS_SOURCE_LOCATOR;
> >> ukey->created = time_msec();
> >> memset(>stats, 0, sizeof ukey->stats);
> >> ukey->stats.used = used;
> >> @@ -1674,7 +1680,11 @@ static void
> >> transition_ukey(struct udpif_key *ukey, enum ukey_state dst)
> >> OVS_REQUIRES(ukey->mutex)
> >> {
> >> -ovs_assert(dst >= ukey->state);
> >> +if (dst >= ukey->state) {
> >> +VLOG_ABORT("Invalid ukey transition %d->%d (last transitioned 
> >> from "
> >> +   "thread %u at %s)", ukey->state, dst, 
> >> ukey->state_thread,
> >> +   ukey->state_where);
> >> +}
> >> if (ukey->state == dst && dst == UKEY_OPERATIONAL) {
> >> return;
> >> }
> >> @@ -1709,6 +1719,8 @@ transition_ukey(struct udpif_key *ukey, enum 
> >> ukey_state dst)
> >>  ds_cstr(), ukey->state, dst);
> >> ds_destroy();
> >> }
> >> +ukey->state_thread = ovsthread_id_self();
> >> +ukey->state_where = OVS_SOURCE_LOCATOR;
> >
> > You’ll want to evaluate OVS_SOURCE_LOCATOR at the caller of the 
> > transition_ukey() instead. Top do that you’ll need to add a “const char 
> > *where” argument, and then make a macro that uses OVS_SOURCE_LOCATOR as 
> > that additional argument, and make the callers use the new macro instead of 
> > calling the function directly.
> >
> > As is it will always report the same line (right here).
> 
> Thanks for the reviews. I plan to roll in the following incremental
> before applying to master and branch-2.7:

Oops.  I feel sad that I missed that!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] revalidator: Improve logging for transition_ukey().

[Joe Stringer]

On 26 April 2017 at 18:35, Jarno Rajahalme  wrote:
>
>> On Apr 26, 2017, at 6:03 PM, Joe Stringer  wrote:
>>
>> There are a few cases where more introspection into ukey transitions
>> would be relevant for logging or assertion. Track the SOURCE_LOCATOR and
>> thread id when states are transitioned and use these for logging.
>>
>> Suggested-by: Jarno Rajahalme 
>> Signed-off-by: Joe Stringer 
>> ---
>> ofproto/ofproto-dpif-upcall.c | 17 -
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index ccf15a3c80b3..8aff613161d9 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -281,6 +281,10 @@ struct udpif_key {
>> uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
>> enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
>>
>> +/* 'state' debug information. */
>> +unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
>> +const char *state_where OVS_GUARDED;  /* transition_ukey() locator. 
>> */
>> +
>> /* Datapath flow actions as nlattrs.  Protected by RCU.  Read with
>>  * ukey_get_actions(), and write with ukey_set_actions(). */
>> OVSRCU_TYPE(struct ofpbuf *) actions;
>> @@ -1484,6 +1488,8 @@ ukey_create__(const struct nlattr *key, size_t key_len,
>> ukey->dump_seq = dump_seq;
>> ukey->reval_seq = reval_seq;
>> ukey->state = UKEY_CREATED;
>> +ukey->state_thread = ovsthread_id_self();
>> +ukey->state_where = OVS_SOURCE_LOCATOR;
>> ukey->created = time_msec();
>> memset(>stats, 0, sizeof ukey->stats);
>> ukey->stats.used = used;
>> @@ -1674,7 +1680,11 @@ static void
>> transition_ukey(struct udpif_key *ukey, enum ukey_state dst)
>> OVS_REQUIRES(ukey->mutex)
>> {
>> -ovs_assert(dst >= ukey->state);
>> +if (dst >= ukey->state) {
>> +VLOG_ABORT("Invalid ukey transition %d->%d (last transitioned from "
>> +   "thread %u at %s)", ukey->state, dst, ukey->state_thread,
>> +   ukey->state_where);
>> +}
>> if (ukey->state == dst && dst == UKEY_OPERATIONAL) {
>> return;
>> }
>> @@ -1709,6 +1719,8 @@ transition_ukey(struct udpif_key *ukey, enum 
>> ukey_state dst)
>>  ds_cstr(), ukey->state, dst);
>> ds_destroy();
>> }
>> +ukey->state_thread = ovsthread_id_self();
>> +ukey->state_where = OVS_SOURCE_LOCATOR;
>
> You’ll want to evaluate OVS_SOURCE_LOCATOR at the caller of the 
> transition_ukey() instead. Top do that you’ll need to add a “const char 
> *where” argument, and then make a macro that uses OVS_SOURCE_LOCATOR as that 
> additional argument, and make the callers use the new macro instead of 
> calling the function directly.
>
> As is it will always report the same line (right here).

Thanks for the reviews. I plan to roll in the following incremental
before applying to master and branch-2.7:

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 8aff613161d9..25aa8f2a92bc 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -354,8 +354,11 @@ static void ukey_get_actions(struct udpif_key *,
const struct nlattr **actions,
 static bool ukey_install__(struct udpif *, struct udpif_key *ukey)
 OVS_TRY_LOCK(true, ukey->mutex);
 static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
-static void transition_ukey(struct udpif_key *ukey, enum ukey_state dst)
+static void transition_ukey_at(struct udpif_key *ukey, enum ukey_state dst,
+   const char *where)
 OVS_REQUIRES(ukey->mutex);
+#define transition_ukey(UKEY, DST) \
+transition_ukey_at(UKEY, DST, OVS_SOURCE_LOCATOR)
 static struct udpif_key *ukey_lookup(struct udpif *udpif,
  const ovs_u128 *ufid,
  const unsigned pmd_id);
@@ -1677,7 +1680,8 @@ ukey_install__(struct udpif *udpif, struct
udpif_key *new_ukey)
 }

 static void
-transition_ukey(struct udpif_key *ukey, enum ukey_state dst)
+transition_ukey_at(struct udpif_key *ukey, enum ukey_state dst,
+   const char *where)
 OVS_REQUIRES(ukey->mutex)
 {
 if (dst >= ukey->state) {
@@ -1720,7 +1724,7 @@ transition_ukey(struct udpif_key *ukey, enum
ukey_state dst)
 ds_destroy();
 }
 ukey->state_thread = ovsthread_id_self();
-ukey->state_where = OVS_SOURCE_LOCATOR;
+ukey->state_where = where;
 }

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

[ovs-dev] You have just received a payment of $250.02

[Stripe Support]

Congratulations! You have just received a payment of $250.02. You can view the 
full details of this payment in your dashboard

  https://dashboard.stripe. com/review

We'll be here to help you with any step along the way. You can find answers to 
most questions and get in touch with us at

Yours,

The Stripe team

 Charge Summary 

Charge ID: ch_16sLfsdOCjFfeh1W2ttdPefxxSS
Charge description: Payment  order #K0TJ-63228?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] tests/ofproto.at: Workaround some races

[Timothy Redaelli]

Port commit a6d1a2997db4:
ofproto.at: Workaround a race

While a barrier serializes requests from the same connection,
it doesn't wait for requests from other connections to the switch.
Replace the barrier with infamous "sleep 1" to workaround the problem.

to the following tests:
"ofproto - asynchronous message control (OpenFlow 1.0)",
"ofproto - asynchronous message control (OpenFlow 1.3)",
"ofproto - asynchronous message control (OpenFlow 1.4)" and
"ofproto - asynchronous message control (OpenFlow 1.5)"

Sometimes one of these tests fails because the OFPT_BARRIER_REPLY is
printed before the other message we expect to have.

Suggested-by: Lance Richardson 
Signed-off-by: Timothy Redaelli 
---
 tests/ofproto.at | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tests/ofproto.at b/tests/ofproto.at
index 4431c15..69916a4 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -3212,8 +3212,7 @@ 
udp,vlan_tci=0x,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
 fi
 AT_FAIL_IF([test X"$1" != X])
 
-ovs-appctl -t ovs-ofctl ofctl/barrier
-echo >>expout "OFPT_BARRIER_REPLY:"
+sleep 1
 
 AT_CHECK(
   [[sed '
@@ -3435,8 +3434,7 @@ 
udp,vlan_tci=0x,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
 
 AT_FAIL_IF([test X"$1" != X])
 
-ovs-appctl -t ovs-ofctl ofctl/barrier
-echo >>expout "OFPT_BARRIER_REPLY (OF1.3):"
+sleep 1
 
 AT_CHECK(
   [[sed '
@@ -3645,8 +3643,7 @@ table_desc:-
 
 AT_FAIL_IF([test X"$1" != X])
 
-ovs-appctl -t ovs-ofctl ofctl/barrier
-echo >>expout "OFPT_BARRIER_REPLY (OF1.4):"
+sleep 1
 
 AT_CHECK(
   [[sed '
@@ -3734,8 +3731,7 @@ OFPT_PORT_STATUS (OF1.5): MOD: 2(test): 
addr:aa:55:aa:55:00:0x
 
 AT_FAIL_IF([test X"$1" != X])
 
-ovs-appctl -t ovs-ofctl ofctl/barrier
-echo >>expout "OFPT_BARRIER_REPLY (OF1.5):"
+sleep 1
 
 AT_CHECK(
   [[sed '
-- 
2.9.3

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

Re: [ovs-dev] [PATCH] bridge: Prohibit "default" bridge name.

[Greg Rose]

On Thu, 2017-04-27 at 04:02 -0700, William Tu wrote:
> Before the patch, when users create bridge named "default", although
> ovs-vsctl fails but vswitchd in the background will keep retrying it,
> causing the systemd-udev to reach 100% cpu utilization.  The reason is
> due to frequent calls into kernel's register_netdevice function,
> which will invoke several kernel elements who has registered on the
> netdevice notifier chain.  One of the notifier, the inetdev_event rejects
> this devname and register_netdevice fails.  The patch prohibits creating
> "default" bridge name.
> 
> VMWare-BZ: #1842388
> Signed-off-by: William Tu 
> ---
>  vswitchd/bridge.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index ebb6249416fa..e8a22f82e1d6 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1710,7 +1710,8 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>  const struct ovsrec_bridge *br_cfg = cfg->bridges[i];
>  
> -if (strchr(br_cfg->name, '/') || strchr(br_cfg->name, '\\')) {
> +if (strchr(br_cfg->name, '/') || strchr(br_cfg->name, '\\')
> +|| !strcmp(br_cfg->name, "default")) {
>  /* Prevent remote ovsdb-server users from accessing arbitrary
>   * directories, e.g. consider a bridge named "../../../etc/".
>   *

Acked-by: Greg Rose 

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

Re: [ovs-dev] [PATCH] ovn: Bump ovn-nb schema version.

[Russell Bryant]

On Mon, Apr 24, 2017 at 2:04 PM, Ben Pfaff  wrote:
> On Wed, Apr 19, 2017 at 12:41:37PM -0400, Russell Bryant wrote:
>> Commit b89d25e5694b made the "router" DHCPv4 option optional instead of
>> mandatory.  This did not actually change the schema, but there's no good
>> way for a client of the northbound database to know if this change is
>> present without bumping the schema version.  This is needed for a client to
>> work with versions before and after this change.
>>
>> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1670666
>> Fixes: b89d25e5694b ("ovn: Modify the DHCPv4 router option to optional")
>> Signed-off-by: Russell Bryant 
>
> Acked-by: Ben Pfaff 
>
> However, keep in mind that this version distinction is only useful if
> ovn-northd and the OVSDB schema are upgraded almost simultaneously.

Good point, thanks.

ovn-ctl is set up to update the schema if needed before starting
ovn-northd, including during a restart, so it should generally be the
case.

I applied this to master.

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

[ovs-dev] [PATCH] bridge: Prohibit "default" bridge name.

[William Tu]

Before the patch, when users create bridge named "default", although
ovs-vsctl fails but vswitchd in the background will keep retrying it,
causing the systemd-udev to reach 100% cpu utilization.  The reason is
due to frequent calls into kernel's register_netdevice function,
which will invoke several kernel elements who has registered on the
netdevice notifier chain.  One of the notifier, the inetdev_event rejects
this devname and register_netdevice fails.  The patch prohibits creating
"default" bridge name.

VMWare-BZ: #1842388
Signed-off-by: William Tu 
---
 vswitchd/bridge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index ebb6249416fa..e8a22f82e1d6 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1710,7 +1710,8 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 const struct ovsrec_bridge *br_cfg = cfg->bridges[i];
 
-if (strchr(br_cfg->name, '/') || strchr(br_cfg->name, '\\')) {
+if (strchr(br_cfg->name, '/') || strchr(br_cfg->name, '\\')
+|| !strcmp(br_cfg->name, "default")) {
 /* Prevent remote ovsdb-server users from accessing arbitrary
  * directories, e.g. consider a bridge named "../../../etc/".
  *
-- 
2.7.4

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

[ovs-dev] [PATCH branch-2.6] travis: Fix path of DPDK directory

[Timothy Redaelli]

Call configure with --with-dpdk=./dpdk-stable-$DPDK_VER/build
instead of --with-dpdk=./dpdk-$DPDK_VER/build

Fixes: c007c58af492 ("docs: Use DPDK 16.07.2 stable release")
CC: Ian Stokes 
Tested-at: https://travis-ci.org/drizzt/ovs/builds/226330348
Signed-off-by: Timothy Redaelli 
---
 .travis/linux-build.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index 81c209a..f15f706 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -87,7 +87,7 @@ if [ "$DPDK" ]; then
 # Disregard cast alignment errors until DPDK is fixed
 CFLAGS="$CFLAGS -Wno-cast-align"
 fi
-EXTRA_OPTS="$EXTRA_OPTS --with-dpdk=./dpdk-$DPDK_VER/build"
+EXTRA_OPTS="$EXTRA_OPTS --with-dpdk=./dpdk-stable-$DPDK_VER/build"
 elif [ "$CC" != "clang" ]; then
 # DPDK headers currently trigger sparse errors
 SPARSE_FLAGS="$SPARSE_FLAGS -Wsparse-error"
-- 
2.9.3

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

Re: [ovs-dev] [PATCH v2 6/6] Documentation: Update DPDK doc with Keepalive feature.

[Bodireddy, Bhanuprakash]

>>
>>    Compute NodeControllerCompute Node
>>
>> Collectd  <--> Ceilometer <>   Collectd
>>
>> OvS DPDK   OvS DPDK
>>
>>    +-+
>>    | VM  |
>>    +--+--+
>>   \---+---/
>>   |
>>    +--+---+   ++--+ +--+---+
>>    | OVS  |-> |dpdkevents plugin  | --> |   collectd   |
>>    +--+---+   ++--+ +--+---+
>>    |
>>  +--+-+ +---++ |
>>  | Ceilometer | <-- | collectd ceilometer plugin |  <---
>>  +--+-+ +---++
>
>You see all of this, right here ^^^ ? That's excellent. Put *that* in a doc. I
>would suggest 'Documentation/topics/dpdk/keepalive.rst'. You could include
>a reference to the below doc using something like the
>following:
>
>For information on how to use the keepalive feature, refer to
>:ref:`the HOWTO `.

I have plans to write a separate document with detail explanation on how this 
feature should be used with OpenStack. That also includes how this is 
integrated with ceilometer. But I will do the document as a separate patch once 
this feature gets accepted. 

>
>The only changes I'd make is to indent the diagram by four spaces and
>precede it with by '::' (to format as a literal block), and change the OVSDB
>settings overview piece to use definitions lists, which look like
>this:
>
>``keepalive=true``
>
>  Enable the keepalive feature. Defaults to false (disabled).
>
>This could be done as a separate patch unless you need to respin this series.

I will take this suggestion and do this if I have to send v3.

>
>> Performance impact:
>> No noticeable performance or latency impact is observed with KA
>> feature enabled. The tests were run with 100ms KA interval and latency
>> is (Min:134,710ns, Avg:173,005ns, Max:1,504,670ns) for Phy2Phy
>> loopback test case with 100 unique streams.
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> 
>> ---
>>  Documentation/howto/dpdk.rst | 93
>> 
>>  1 file changed, 93 insertions(+)
>>
>> diff --git a/Documentation/howto/dpdk.rst
>> b/Documentation/howto/dpdk.rst index dc63f7d..e482166 100644
>> --- a/Documentation/howto/dpdk.rst
>> +++ b/Documentation/howto/dpdk.rst
>> @@ -400,6 +400,99 @@ If ``N`` is set to 1, an insertion will be
>> performed for every flow. If set to
>>
>>  For more information on the EMC refer to :doc:`/intro/install/dpdk` .
>>
>> +.. _dpdk_keepalive:
>> +
>> +KeepAlive
>> +-
>> +
>> +OvS KeepAlive(KA) feature is disabled by default. To enable KA
>> feature::
>
>s/OvS/OVS/
>s/KeepAlive(KA)/KeepAlive (KA)/
>
Ok.

>> +
>> +$ ovs-vsctl --no-wait set Open_vSwitch .
>> other_config:keepalive=true
>> +
>> +The default timer interval for monitoring packet processing cores is
>> 100ms.
>> +To set a different timer value, run::
>> +
>> +$ ovs-vsctl --no-wait set Open_vSwitch . \
>> +other_config:keepalive-interval="50"
>> +
>> +The events comprise of core states and the last seen timestamps. The
>> events
>> +are written in to shared memory region
>> ``/dev/shm/dpdk_keepalive_shm_name``.
>> +To write in to a different shared memory region, run::
>> +
>> +$ ovs-vsctl --no-wait set Open_vSwitch . \
>> +other_config:keepalive-shm-name="/"
>> +
>
>nit: I assume the '/' before '' was a typo. Drop that, if
>so.
It's not a typo. It is expected.

>
>> +The events in the shared memory block can be read by external
>> monitoring
>> +framework (or) applications. `collectd `__
>> has built-in
>> +support for DPDK and provides a `dpdkevents` plugin that can be
>> enabled to
>> +relay the datapath core status to OpenStack service `Ceilometer
>> +`__.
>> +
>> +To install and configure `collectd`, run::
>> +
>> +# Clone collectd from Git repository
>> +$ git clone https://github.com/collectd/collectd.git
>> +
>> +# configure and install collectd
>> +$ cd collectd
>> +$ ./build.sh
>> +$ ./configure --enable-syslog --enable-logfile --with-
>> libdpdk=/usr
>> +$ make
>> +$ make install
>> +
>
>I should have called this out first time, but I'm not sure we want to duplicate
>collectd's installation procedure as these things can change.
>We might be better of linking to installation docs. _However_, we do this
>already for DPDK so there is precedent. If you think we should keep them
>(and you likely do), I'd be happy to simply include a link to the installation 
>docs
>like so:

While I agree to this, collectd documentation is vast and users may be lost 
reading the installation.
I would mention the basic steps here and can point a link to the documentation 
as suggested so that they can refer to the documentation for any advanced 
debugging.

>
>

Re: [ovs-dev] [PATCH v2 6/6] Documentation: Update DPDK doc with Keepalive feature.

[Stephen Finucane]

On Wed, 2017-04-26 at 23:59 +0100, Bhanuprakash Bodireddy wrote:
> OvS DPDK Keepalive feature is aimed at achieving Fastpath Service
> Assurance in OVS-DPDK deployments. It adds support for monitoring the
> packet processing cores(PMD thread cores) by dispatching heartbeats
> at regular intervals. Incase of heartbeat misses the failure shall be
> detected & reported to higher level fault management
> systems/frameworks.
> 
> The implementation uses POSIX shared memory object for storing the
> events that will be periodically read by monitoring framework. keep-
> alive feature can be enabled through below OVSDB settings.
> 
> keepalive=true
>    - Keepalive feature is disabled by default.
> 
> keepalive-interval="50"
>    - Timer interval in milliseconds for monitoring the packet
>  processing cores.
> 
> keepalive-shm-name="/dpdk_keepalive_shm_name"
>    - Shared memory block name where the events shall be updated.
> 
> When KA is enabled, 'ovs-keepalive' thread shall be spawned that
> wakes up at regular intervals to update the timestamp and status of
> pmd cores in shared memory region.
> 
> An external monitoring framework like collectd(with dpdk plugin
> support) can read the core status updates from shared memory. When
> the core state is updated, the collectd shall relay the status to
> ceilometer service running in the controller. Below is the high level
> overview of deployment model.
> 
>    Compute NodeControllerCompute Node
> 
> Collectd  <--> Ceilometer <>   Collectd
> 
> OvS DPDK   OvS DPDK
> 
>    +-+
>    | VM  |
>    +--+--+
>   \---+---/
>   |
>    +--+---+   ++--+ +--+---+
>    | OVS  |-> |dpdkevents plugin  | --> |   collectd   |
>    +--+---+   ++--+ +--+---+
>    |
>  +--+-+ +---++ |
>  | Ceilometer | <-- | collectd ceilometer plugin |  <---
>  +--+-+ +---++

You see all of this, right here ^^^ ? That's excellent. Put *that* in a
doc. I would suggest 'Documentation/topics/dpdk/keepalive.rst'. You
could include a reference to the below doc using something like the
following:

For information on how to use the keepalive feature, refer to
:ref:`the HOWTO `.

The only changes I'd make is to indent the diagram by four spaces and
precede it with by '::' (to format as a literal block), and change the
OVSDB settings overview piece to use definitions lists, which look like
this:

``keepalive=true``

  Enable the keepalive feature. Defaults to false (disabled).

This could be done as a separate patch unless you need to respin this
series.

> Performance impact:
> No noticeable performance or latency impact is observed with
> KA feature enabled. The tests were run with 100ms KA interval
> and latency is (Min:134,710ns, Avg:173,005ns, Max:1,504,670ns)
> for Phy2Phy loopback test case with 100 unique streams.
> 
> Signed-off-by: Bhanuprakash Bodireddy
> 
> ---
>  Documentation/howto/dpdk.rst | 93
> 
>  1 file changed, 93 insertions(+)
> 
> diff --git a/Documentation/howto/dpdk.rst
> b/Documentation/howto/dpdk.rst
> index dc63f7d..e482166 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -400,6 +400,99 @@ If ``N`` is set to 1, an insertion will be
> performed for every flow. If set to
>  
>  For more information on the EMC refer to :doc:`/intro/install/dpdk`
> .
>  
> +.. _dpdk_keepalive:
> +
> +KeepAlive
> +-
> +
> +OvS KeepAlive(KA) feature is disabled by default. To enable KA
> feature::

s/OvS/OVS/
s/KeepAlive(KA)/KeepAlive (KA)/

> +
> +$ ovs-vsctl --no-wait set Open_vSwitch .
> other_config:keepalive=true
> +
> +The default timer interval for monitoring packet processing cores is
> 100ms.
> +To set a different timer value, run::
> +
> +$ ovs-vsctl --no-wait set Open_vSwitch . \
> +other_config:keepalive-interval="50"
> +
> +The events comprise of core states and the last seen timestamps. The
> events
> +are written in to shared memory region
> ``/dev/shm/dpdk_keepalive_shm_name``.
> +To write in to a different shared memory region, run::
> +
> +$ ovs-vsctl --no-wait set Open_vSwitch . \
> +other_config:keepalive-shm-name="/"
> +

nit: I assume the '/' before '' was a typo. Drop
that, if so.

> +The events in the shared memory block can be read by external
> monitoring
> +framework (or) applications. `collectd `__
> has built-in
> +support for DPDK and provides a `dpdkevents` plugin that can be
> enabled to
> +relay the datapath core status to OpenStack service `Ceilometer
> +`__.
> +
> +To install and configure `collectd`, run::
> +