Re: [ovs-dev] [PATCH] ofproto-dpif: Send QinQ related sFlow counters

[Ben Pfaff]

On Thu, Apr 20, 2017 at 10:50:17AM -0700, Neil McKee wrote:
> I don't think OVS is likely to encounter non-standard ethernet types
> in that way,  or hardware that strips vlan tags from the packet before
> software even sees it.  So extended_vlantunnel can perhaps be regarded
> as an anachronism that dates back to wild-west days?

OVS only supports 0x8100 and 0x88a8 Ethertype types for VLANs (so far),
so I guess that you're right.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/10] userspace: Add OXM field MFF_PACKET_TYPE

[Ben Pfaff]

On Fri, Apr 21, 2017 at 07:05:41PM -0700, Ben Pfaff wrote:
> On Tue, Apr 18, 2017 at 11:19:44AM +, Zoltán Balogh wrote:
> > From: Jan Scheurich 
> > 
> > Allow packet type namespace OFPHTN_ETHERTYPE as alternative pre-requisite
> > for matching L3 protocols (MPLS, IP, IPv6, ARP etc).
> > 
> > Scan packet_type in odp_flow string.
> > 
> > Added dummy entry for MFF_PACKET_TYPE to lib/meta-flow.xml
> > 
> > Added packet_type to the matching properties in tests/ofproto.at. Should be
> > removed later, when packet_type_aware bridge attribute will be introduced.
> > 
> > Signed-off-by: Jan Scheurich 
> 
> Zoltán, thanks a lot for working on this feature.  But I'm seeing a lot
> of patch rejects even on the first patch, so I'd appreciate very much a
> rebase.

Oh, it's because there's another series dependency, never mind.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Gao Zhenyu]

Hi Jarno,

Thanks for the suggestion,  I will try to get a performance number and
submit it into Linux first. :)

Thanks
Zhenyu Gao

2017-04-22 4:24 GMT+08:00 Jarno Rajahalme :

> As a policy, Linux kernel datapath changes other than backports need to go
> to upstream Linux first, new features to net-next tree, and bug fixes to
> net tree. See the documentation file ‘backporting-patches.rst’ in directory
> 'Documentation/internals/contributing/‘ of the OVS tree for more detailed
> description of the process.
>
> Also, the commit message should contain a clear motivation for the change.
> Changes that enhance readability may adversely affect datapath performance,
> so having a report on performance testing would be helpful in determining
> whether to apply the change.
>
> Regards,
>
>   Jarno
>
> > On Apr 21, 2017, at 12:21 AM, Zhenyu Gao 
> wrote:
> >
> > 1. Consume switch/case to judge type of frame instead of using if/else.
> > 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> >
> > Signed-off-by: Zhenyu Gao 
> > ---
> > datapath/flow.c | 230 --
> --
> > 1 file changed, 117 insertions(+), 113 deletions(-)
> >
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 2bc1ad0..0b35de6 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> > sizeof(struct icmphdr));
> > }
> >
> > +/**
> > +  * Parse ipv4 header from an Ethernet frame.
> > +  * Return ipv4 header length if successful, otherwise a negative errno
> value.
> > +  */
> > +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > + int err;
> > + struct iphdr *nh;
> > + __be16 offset;
> > +
> > + err = check_iphdr(skb);
> > + if (unlikely(err))
> > + return err;
> > +
> > + nh = ip_hdr(skb);
> > + key->ipv4.addr.src = nh->saddr;
> > + key->ipv4.addr.dst = nh->daddr;
> > +
> > + key->ip.proto = nh->protocol;
> > + key->ip.tos = nh->tos;
> > + key->ip.ttl = nh->ttl;
> > +
> > + offset = nh->frag_off & htons(IP_OFFSET);
> > + if (offset) {
> > + key->ip.frag = OVS_FRAG_TYPE_LATER;
> > + } else {
> > + if (nh->frag_off & htons(IP_MF) ||
> > + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> > + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > + } else {
> > + key->ip.frag = OVS_FRAG_TYPE_NONE;
> > + }
> > + }
> > + return ip_hdrlen(skb);
> > +}
> > +
> > +/**
> > +  * Parse ipv6 header from an Ethernet frame.
> > +  * Return ipv6 header length if successful, otherwise a negative errno
> value.
> > +  */
> > static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> > {
> >   unsigned int nh_ofs = skb_network_offset(skb);
> > @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb,
> struct sw_flow_key *key)
> >   else
> >   key->ip.frag = OVS_FRAG_TYPE_FIRST;
> >   } else {
> > - key->ip.frag = OVS_FRAG_TYPE_NONE;
> > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> > + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > + else
> > + key->ip.frag = OVS_FRAG_TYPE_NONE;
> >   }
> >
> >   /* Delayed handling of error in ipv6_skip_exthdr() as it
> > @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >   key->eth.type = skb->protocol;
> >
> >   /* Network layer. */
> > - if (key->eth.type == htons(ETH_P_IP)) {
> > - struct iphdr *nh;
> > - __be16 offset;
> > + switch(key->eth.type) {
> > + case htons(ETH_P_IP):
> > + case htons(ETH_P_IPV6): {
> > + int nh_len;
> > + if (key->eth.type == htons(ETH_P_IP)) {
> > + nh_len = parse_ipv4hdr(skb, key);
> > + } else {
> > + nh_len = parse_ipv6hdr(skb, key);
> > + }
> >
> > - error = check_iphdr(skb);
> > - if (unlikely(error)) {
> > - memset(>ip, 0, sizeof(key->ip));
> > - memset(>ipv4, 0, sizeof(key->ipv4));
> > - if (error == -EINVAL) {
> > + if (unlikely(nh_len < 0)) {
> > + switch (nh_len) {
> > + case -EINVAL:
> > + memset(>ip, 0, sizeof(key->ip));
> > + if (key->eth.type == htons(ETH_P_IP)) {
> > + memset(>ipv4.addr, 0,
> sizeof(key->ipv4.addr));
> > + } else {
> > + memset(>ipv6.addr, 0,
> sizeof(key->ipv6.addr));
> > + }
> > + 

Re: [ovs-dev] [PATCH 0/10] userspace: Packet type-aware pipeline

[Ben Pfaff]

On Tue, Apr 18, 2017 at 11:19:36AM +, Zoltán Balogh wrote:
> 
> This patch set is the 2nd part of an initiative presented by Jan Scheurich: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/330488.html
> 
> It takes the patch set referred by the link above as a bases.

Oh, I didn't realize that when I tried to apply this one.  I'll look at
that one first then.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/10] userspace: Add OXM field MFF_PACKET_TYPE

[Ben Pfaff]

On Tue, Apr 18, 2017 at 11:19:44AM +, Zoltán Balogh wrote:
> From: Jan Scheurich 
> 
> Allow packet type namespace OFPHTN_ETHERTYPE as alternative pre-requisite
> for matching L3 protocols (MPLS, IP, IPv6, ARP etc).
> 
> Scan packet_type in odp_flow string.
> 
> Added dummy entry for MFF_PACKET_TYPE to lib/meta-flow.xml
> 
> Added packet_type to the matching properties in tests/ofproto.at. Should be
> removed later, when packet_type_aware bridge attribute will be introduced.
> 
> Signed-off-by: Jan Scheurich 

Zoltán, thanks a lot for working on this feature.  But I'm seeing a lot
of patch rejects even on the first patch, so I'd appreciate very much a
rebase.

Thanks,

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

Re: [ovs-dev] [PATCH v5] ovn-controller: add quiet mode

[Han Zhou]

On Thu, Apr 20, 2017 at 11:03 PM, Han Zhou  wrote:
>
> As discussed in [1], what the incremental processing code
> actually accomplished was that the ovn-controller would
> be "quiet" and not burn CPU when things weren't changing.
> This patch set recreates this state by calculating whether
> changes have occured that would require a full calculation
> to be performed.  It does this by persisting a copy of
> the localvif_to_ofport and tunnel information in the
> controller module, rather than in the physical.c module
> as was the case with previous commits.
>
> Performance improved extremely in below test scenario:
>
> - 1 lswitch with 10 lports bound locally
> - Each lport has an ingress ACL, referencing the same address-set
> - The address-set has 10,000 IPv4 addresses
>
> For each IP address in the address-set, there will be 3
> OpenFlow rules generated for each ACL. So the total number
> of rules is 300k+.
>
> Without the patch, it takes 50+ minutes to install all the
> rules to ovs-vswitchd.
>
> With the patch, it takes 20 seconds to install all the rules
> to ovs-vswitchd.
>
> The reason is that the large number of rules are sent to
> ovs-vswitchd gradually in many iterations of ovn-controller
> main loop. Without the patch, cpu cycles are wasted in
> lflow_run to re-processing the large address set in every
> main loop iteration. With the patch, lflow_run is not
> executed in most iterations because there is no change of
> input.
>
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/078272.html
>
> Signed-off-by: Ryan Moats 
> Signed-off-by: Han Zhou 
> ---
>
> Notes:
> v4->v5:
> - Based on the old patch from Ryan Moats and rebased to current
master.
> - Fixed a problem: when there are large number of flows being
installed
>   to OVS, changes made at the same time to SB DB (e.g. lflows) or
>   physical mappings won't get processed until there are new changes to
>   trigger processing, which may not happen at all. The root cause is
>   ofctrl_put can abort temporarily if there are in-flight messages,
>   which causes last round of flow updates got lost.

I just realized that the ofctrl_can_put logic alone is able to achieve the
performance improvement, so I split it to 2 patches in a series for the
first (simpler) one to be reviewed/merged faster probably. The quiet mode
change depends on the first one to behave correctly.

1) https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331286.html
2) https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331287.html

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

Re: [ovs-dev] [PATCH 1/3] netdev-dpdk: Fix double attaching of virtual devices.

[Ben Pfaff]

On Mon, Apr 03, 2017 at 06:04:16PM +0300, Ilya Maximets wrote:
> 'devargs' for virtual devices contains not only name but
> also a list of arguments like this:
> 
>   'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap'
>   or
>   'eth_af_packet0,iface=eth0'
> 
> We must cut off the arguments from this string before calling
> 'rte_eth_dev_get_port_by_name()' to avoid double attaching of
> the same device.
> 
> CC: Ciara Loftus 
> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)")
> Signed-off-by: Ilya Maximets 
> ---
>  lib/netdev-dpdk.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index ddc651b..c8d7108 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1114,9 +1114,16 @@ static int
>  netdev_dpdk_process_devargs(const char *devargs, char **errp)
>  {
>  uint8_t new_port_id = UINT8_MAX;
> +char *ind, *name = xstrdup(devargs);
> +
> +/* Get the name from the comma separated list of arguments. */
> +ind = index(name, ',');
> +if (ind != NULL) {
> +*ind = '\0';
> +}
>  
>  if (!rte_eth_dev_count()
> -|| rte_eth_dev_get_port_by_name(devargs, _port_id)
> +|| rte_eth_dev_get_port_by_name(name, _port_id)
>  || !rte_eth_dev_is_valid_port(new_port_id)) {
>  /* Device not found in DPDK, attempt to attach it */
>  if (!rte_eth_dev_attach(devargs, _port_id)) {
> @@ -1129,6 +1136,7 @@ netdev_dpdk_process_devargs(const char *devargs, char 
> **errp)
>  }
>  }
>  
> +free(name);
>  return new_port_id;
>  }

Maybe this is a simpler version?

--8<--cut here-->8--

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ddc651bf98a6..736b7908ee0e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1113,10 +1113,11 @@ netdev_dpdk_lookup_by_port_id(int port_id)
 static int
 netdev_dpdk_process_devargs(const char *devargs, char **errp)
 {
+/* Get the name up to the first comma. */
+char *name = xmemdup0(devargs, strcspn(devargs, ","));
 uint8_t new_port_id = UINT8_MAX;
-
 if (!rte_eth_dev_count()
-|| rte_eth_dev_get_port_by_name(devargs, _port_id)
+|| rte_eth_dev_get_port_by_name(name, _port_id)
 || !rte_eth_dev_is_valid_port(new_port_id)) {
 /* Device not found in DPDK, attempt to attach it */
 if (!rte_eth_dev_attach(devargs, _port_id)) {
@@ -1129,6 +1130,7 @@ netdev_dpdk_process_devargs(const char *devargs, char 
**errp)
 }
 }
 
+free(name);
 return new_port_id;
 }
 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC] [PATCH 6/6] port_group: ofproto-dpif-xlate: implement port_group

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:41:00AM +0200, Matthias May wrote:
> Signed-off-by: Matthias May 

Seems reasonable enough, thanks.

I think that you should add a NEWS item to describe the new feature.

Thanks,

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

Re: [ovs-dev] [RFC] [PATCH 5/6] port_group: ofproto-dpif: add port_group definition and init

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:40:59AM +0200, Matthias May wrote:
> Signed-off-by: Matthias May 

I'm getting lots of errors with this, I guess that the next patch needs
to be folded in.

(I'm concluding that there should only a single patch for this feature.)

Thanks,

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

[ovs-dev] [PATCH 2/2] ovn-controller: add quiet mode

[Han Zhou]

As discussed in [1], what the incremental processing code
actually accomplished was that the ovn-controller would
be "quiet" and not burn CPU when things weren't changing.
This patch set recreates this state by calculating whether
changes have occured that would require a full calculation
to be performed.  It does this by persisting a copy of
the localvif_to_ofport and tunnel information in the
controller module, rather than in the physical.c module
as was the case with previous commits.

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/078272.html

Signed-off-by: Ryan Moats 
Co-authored-by: Han Zhou 
Signed-off-by: Han Zhou 
---

Notes:
- Based on the old patch from Ryan Moats and rebased to current master.
- Fixed a problem: when there are large number of flows being installed
  to OVS, changes made at the same time to SB DB (e.g. lflows) or
  physical mappings won't get processed until there are new changes to
  trigger processing, which may not happen at all. The root cause is
  ofctrl_put can abort temporarily if there are in-flight messages,
  which causes last round of flow updates got lost. So force_full_process
  is added in that case, and checking logic changed accordingly in the
  main loop so that old physcal_change is preserved for next iteration
  in such case. This is also the reason why this patch is put into a
  patch serie because it depends on the ofctrl_can_put() change.

 ovn/controller/ofctrl.c |   3 ++
 ovn/controller/ovn-controller.c |  43 ++---
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c  |   2 +
 ovn/controller/physical.c   | 102 +---
 ovn/controller/physical.h   |   5 ++
 6 files changed, 111 insertions(+), 45 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 417fdc9..5c24771 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -383,6 +383,8 @@ run_S_CLEAR_FLOWS(void)
 queue_msg(encode_group_mod());
 ofputil_uninit_group_mod();
 
+force_full_process();
+
 /* Clear existing groups, to match the state of the switch. */
 if (groups) {
 ovn_group_table_clear(groups, true);
@@ -848,6 +850,7 @@ ofctrl_put(struct hmap *flow_table, struct shash 
*pending_ct_zones,
 if (!ofctrl_can_put()) {
 ovn_flow_table_clear(flow_table);
 ovn_group_table_clear(groups, false);
+force_full_process();
 return;
 }
 
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 480d06d..0a75f65 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -484,6 +484,23 @@ get_nb_cfg(struct ovsdb_idl *idl)
 return sb ? sb->nb_cfg : 0;
 }
 
+/* Contains mapping of localvifs to OF ports for detecting if the physical
+ * configuration of the switch has changed. */
+static struct simap localvif_to_ofport =
+SIMAP_INITIALIZER(_to_ofport);
+
+/* Contains the list of known tunnels. */
+static struct hmap tunnels = HMAP_INITIALIZER();
+
+/* The last sequence number seen from the southbound IDL. */
+static unsigned int seqno = 0;
+
+void
+force_full_process(void)
+{
+seqno = 0;
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -576,6 +593,7 @@ main(int argc, char *argv[])
 
 /* Main loop. */
 exiting = false;
+bool physical_change = true;
 while (!exiting) {
 /* Check OVN SB database. */
 char *new_ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
@@ -613,10 +631,8 @@ main(int argc, char *argv[])
 
 struct ldatapath_index ldatapaths;
 struct lport_index lports;
-struct mcgroup_index mcgroups;
 ldatapath_index_init(, ctx.ovnsb_idl);
 lport_index_init(, ctx.ovnsb_idl);
-mcgroup_index_init(, ctx.ovnsb_idl);
 
 const struct sbrec_chassis *chassis = NULL;
 if (chassis_id) {
@@ -627,21 +643,34 @@ main(int argc, char *argv[])
 }
 
 if (br_int && chassis) {
+enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
+ _ct_zones);
+if (detect_and_save_physical_changes(
+_to_ofport, , mff_ovn_geneve, br_int,
+chassis)) {
+physical_change = true;
+}
+unsigned int cur_seqno = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl);
+
 struct shash addr_sets = SHASH_INITIALIZER(_sets);
 addr_sets_init(, _sets);
 
 patch_run(, br_int, chassis, _datapaths);
 
-enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
- _ct_zones);
-
 pinctrl_run(, , br_int, chassis, _datapaths);
 update_ct_zones(_lports, _datapaths, _zones,
 ct_zone_bitmap, _ct_zones);
 if 

Re: [ovs-dev] [RFC] [PATCH 4/6] port_group: ofproto: add port_group definition

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:40:58AM +0200, Matthias May wrote:
> Signed-off-by: Matthias May 

"sparse" now additionally says:

../vswitchd/bridge.c:973:26: warning: incorrect type in assignment 
(different base types)
../vswitchd/bridge.c:973:26:expected restricted ofp_port_t [assigned] 
[usertype] port_group
../vswitchd/bridge.c:973:26:got unsigned int [unsigned] [usertype] 


which means that you should use u16_to_ofp() to convert the
configuration data into an ofp_port_t.

Thanks,

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

Re: [ovs-dev] [RFC] [PATCH 3/6] port_group: bridge: set port_group in port config

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:40:57AM +0200, Matthias May wrote:
> Signed-off-by: Matthias May 
> ---
>  vswitchd/bridge.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 867a26d8d..316e4742e 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -966,6 +966,17 @@ port_configure(struct port *port)
>  s.slaves[s.n_slaves++] = iface->ofp_port;
>  }
>  
> +/* Get port_group. */
> +if (cfg->port_group) {
> +if ((*cfg->port_group > 0 && *cfg->port_group < OFPP_MAX)
> +|| *cfg->port_group == OFPP_LOCAL) {
> +s.port_group = (uint32_t) *cfg->port_group;
> +}
> +} else {
> +/* On a port with multiple interfaces we default to the first. */
> +s.port_group = s.slaves[0];
> +}
> +

This fails to build because:

../vswitchd/bridge.c:973:15: error: no member named 'port_group' in 'struct 
ofproto_bundle_settings'
../vswitchd/bridge.c:977:11: error: no member named 'port_group' in 'struct 
ofproto_bundle_settings'

Probably that indicates that this patch should be combined with a later
patch that adds the port_group member.

"sparse" says:

../vswitchd/bridge.c:971:57: warning: restricted ofp_port_t degrades to 
integer
../vswitchd/bridge.c:972:36: warning: restricted ofp_port_t degrades to 
integer

which probably indicates that you should be using use ofp_to_u16() to
get integer values for OFPP_MAX and OFPP_LOCAL.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC] [PATCH 2/6] port_group: add documentation entry

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:40:56AM +0200, Matthias May wrote:
> Signed-off-by: Matthias May 

Thanks for adding the documentation.  I'd like to encourage you to add
some more background that explains in what circumstances this feature is
useful, perhaps based on the description in part 0.

This documentation says that "Valid values are 1-65279, 65534." but the
constraint on the column would prevent the value 65534 from being used.

(Also, it's not necessary to describe such a constraint by hand in the
documentation because the documentation generator will automatically
describe it.)

Thanks,

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

Re: [ovs-dev] [RFC] [PATCH 1/6] port_group: add db definitions

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:40:55AM +0200, Matthias May wrote:
> Signed-off-by: Matthias May 
> ---
>  vswitchd/vswitch.ovsschema | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 19b49daf1..7ebcd0d6a 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
>  {"name": "Open_vSwitch",
> - "version": "7.15.0",
> - "cksum": "544856471 23228",
> + "version": "7.16.0",
> + "cksum": "2597117342 23427",
>   "tables": {
> "Open_vSwitch": {
>   "columns": {
> @@ -160,6 +160,11 @@
> "enum": ["set", ["trunk", "access", "native-tagged",
>  "native-untagged", "dot1q-tunnel"]]},
>   "min": 0, "max": 1}},
> +   "port_group": {
> + "type": {"key": {"type": "integer",
> +  "minInteger": 1,
> +  "maxInteger": 65279},
> +  "min": 0, "max": 1}},

By itself, this fails to build with the following error because the
documentation is not part of the patch.  Our normal practice is to
document a feature in the same patch as the feature:

../ovsdb/ovsdb-doc: table Port has undocumented column port_group

I am not sure whether this feature will be widely enough used that a new
column is warranted.  If only a few users or a few ports are likely to
use a feature, then we are usually inclined to add a new key-value pair
to other_config rather than to add a new column.  Please consider which
is the better solution in this case.

Thanks,

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

Re: [ovs-dev] [RFC] [PATCH 0/6] Add port_group to prevent loopback between interfaces

[Ben Pfaff]

On Mon, Apr 03, 2017 at 11:40:54AM +0200, Matthias May wrote:
> When using openvswitch in combination with a hardware switch attached via dsa
> it may be desirable to prevent frames from being looped back to interfaces
> which reside on the same switch and are already processed by the
> switching fabric in hardware.
> This patch series achieves this by introducing a new parameter port_group to
> be used by the normal action.
> When the port_group is not set explicitly, it defaults to the ofp number.
> Ports which are in the same group will not forward a frame between each other.

Thank you for proposing (and implementing) a new feature!  It's always
great to see new people and companies coming into the Open vSwitch
development community.  I'll take a more detailed look at each patch,
but I have a few general questions here too.

What's dsa?

How is this feature related to LACP?

I guess that the answers to these questions should go in the
documentation as well as in the thread here.

It looks like these patches depend on each other, so that if only some
of them are applied, in some cases the system does not build.  The OVS
philosophy is that each patch should be self-contained, so that after
each one is applied (in order) the system builds, works, is
self-consistent, and is completely documented.  I am not sure yet
because I have not finished review, but it seems likely that this new
feature should be a single patch.

Thanks again!

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

Re: [ovs-dev] [PATCH 1/6] rstp/stp: Unref the rstp/stp when bridges destroyed.

[Ben Pfaff]

I applied this one to master and branch-2.7 back to branch-2.4.

I'm not planning to review the rest of the series, at least not for now.

On Thu, Apr 20, 2017 at 05:26:33PM -0700, Jarno Rajahalme wrote:
> Looks correct to me.
> 
> Acked-by: Jarno Rajahalme 
> 
> > On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao  
> > wrote:
> > 
> > When bridges destroyed, which stp enabled, you can
> > still get stp info via the command 'ovs-appctl stp/show'.
> > And the rstp is also in the same case. We should unref
> > them. The rstp/stp ports have been unregistered via
> > 'ofproto_port_unregister' function when ports destroyed.
> > We will unref rstp/stp struct in the 'destruct' of
> > ofproto-dpif provider.
> > 
> > Signed-off-by: nickcooper-zhangtonghao 
> > ---
> > ofproto/ofproto-dpif.c | 2 ++
> > 1 file changed, 2 insertions(+)
> > 
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 523adad..4beacda 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1494,6 +1494,8 @@ destruct(struct ofproto *ofproto_)
> > hmap_destroy(>bundles);
> > mac_learning_unref(ofproto->ml);
> > mcast_snooping_unref(ofproto->ms);
> > +stp_unref(ofproto->stp);
> > +rstp_unref(ofproto->rstp);
> > 
> > sset_destroy(>ports);
> > sset_destroy(>ghost_ports);
> > -- 
> > 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-sbctl: fix lflow-list when uuid has leading 0s.

[Ben Pfaff]

On Fri, Mar 31, 2017 at 04:46:22PM -0700, Han Zhou wrote:
> When uuid starts with 0s, lflow-list will fail if leading 0s are
> not included in command argument. This leads to unexpected results
> considering that leading 0s are usually not shown up in cookies
> of OpenFlow outputs of tools such as ovs-ofctl dump-flows
> and ovs-appctl ofproto/trace. E.g.
> 
> lflow uuid: 0c16ceb4-0409-484b-8297-a6e7f264ac2d
> $ ovn-nbctl lflow-list 0c16ceb4 # works fine
> $ ovn-nbctl lflow-list c16ceb4 # doesn't work
> 
> This patch fixes the problem.
> 
> Signed-off-by: Han Zhou 

Wow, that's subtle.  Good catch!

I applied this to master and branch-2.7.  I couldn't resist changing
strip_leading_zero() to just

static const char *
strip_leading_zero(const char *s)
{
return s + strspn(s, "0");
}

though.

(By the way, this is going to result in "ovn-nbctl lflow-list 0"
listing all the flows, but I guess that's not a big deal.)

Thanks,

Ben.
___
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.

[Ben Pfaff]

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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

Thanks for keeping this going!  I applied it to master.

On Thu, Apr 13, 2017 at 08:25:07AM +, Chandran, Sugesh wrote:
> Can anyone have a look on this patch??
> 
> Regards
> _Sugesh
> 
> 
> > -Original Message-
> > From: Chandran, Sugesh
> > Sent: Tuesday, April 11, 2017 11:14 AM
> > To: d...@openvswitch.org; b...@ovn.org
> > Cc: Chandran, Sugesh ; Zoltán Balogh
> > 
> > Subject: [PATCH v5] tunneling: Avoid recirculation on datapath by computing
> > the recirculate actions at translate time.
> > 
> > Openvswitch datapath recirculates packets for tunneling, i.e.
> > the incoming packets are encapsulated at first pass. Further actions are
> > applied on encapsulated packets on the second pass after recirculating.
> > The proposed patch compute and append the post tunnel actions at the time
> > of translation itself instead of recirculating at datapath. These actions 
> > are
> > solely depends on tunnel attributes so there is no need of datapath
> > recirculation.
> > By avoiding the recirculation at datapath, the patch offers upto 30%
> > performance improvement for VxLAN tunneling in our testing.
> > The action execution logic is using the new CLONE action to define the 
> > packet
> > cloning when the actions are combined. The lenght in the CLONE action
> > specifies the size of nested action set.
> > 
> > It also fixing the test suites failures that are introduced by nested CLONE
> > action in tunneling.
> > 
> > v5
> > - Fix the OVN test case failure by commenting the test validation as its not
> >   relevant with the new tunnel CLONE action.
> > - Code changes for applying CLONE action on a batch than individual packets
> >   are already pushed to the master. V5 patch is now only doing CLONE at
> > tunnel
> >   push.
> > v4
> > - Rename the function to compute post tunnel nested function.
> > - Use the clone action syntax itself for the flow display.
> > - Use nl_msg functions for handling the nested attribute.
> > - Modify the CLONE action to process packets in batch than individually.
> > v3
> > - Rebase with newely clone action and use it for tunneling.
> > v2
> > - Use only single CLONE action with length to mark the tunnel combine action
> > set.
> > - Update the datapath trace display functions to handle CLONE.
> > - Fixed test cases to work with CLONE action.
> > 
> > Signed-off-by: Sugesh Chandran 
> > Signed-off-by: Zoltán Balogh 
> > Co-authored-by: Zoltán Balogh 
> > ---
> >  lib/dpif-netdev.c |  18 +--
> >  ofproto/ofproto-dpif-xlate.c  | 280 ++---
> > -
> >  tests/ofproto-dpif.at |  11 +-
> >  tests/ovn.at  |   6 +-
> >  tests/tunnel-push-pop-ipv6.at |  10 +-
> >  tests/tunnel-push-pop.at  |  12 +-
> >  6 files changed, 164 insertions(+), 173 deletions(-)
> > 
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index a14a2eb..41d0836
> > 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4995,24 +4995,8 @@ dp_execute_cb(void *aux_, struct
> > dp_packet_batch *packets_,
> > 
> >  case OVS_ACTION_ATTR_TUNNEL_PUSH:
> >  if (*depth < MAX_RECIRC_DEPTH) {
> > -struct dp_packet_batch tnl_pkt;
> > -struct dp_packet_batch *orig_packets_ = packets_;
> > -int err;
> > -
> > -if (!may_steal) {
> > -dp_packet_batch_clone(_pkt, packets_);
> > -packets_ = _pkt;
> > -dp_packet_batch_reset_cutlen(orig_packets_);
> > -}
> > -
> >  dp_packet_batch_apply_cutlen(packets_);
> > -
> > -err = push_tnl_action(pmd, a, packets_);
> > -if (!err) {
> > -(*depth)++;
> > -dp_netdev_recirculate(pmd, packets_);
> > -(*depth)--;
> > -}
> > +push_tnl_action(pmd, a, packets_);
> >  return;
> >  }
> >  break;
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c 
> > index
> > a24aef9..b416f46 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -423,6 +423,10 @@ static void xlate_action_set(struct xlate_ctx *ctx);
> > static void xlate_commit_actions(struct xlate_ctx *ctx);
> > 
> >  static void
> > +apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport
> > *in_dev,
> > +  struct xport *out_dev);
> > +
> > +static void
> >  ctx_trigger_freeze(struct xlate_ctx *ctx)  {
> >  ctx->exit = true;
> > @@ -3204,7 +3208,17 @@ build_tunnel_send(struct xlate_ctx *ctx, const
> > struct xport *xport,
> >  }
> >  tnl_push_data.tnl_port = odp_to_u32(tunnel_odp_port);
> >  tnl_push_data.out_port = odp_to_u32(out_dev->odp_port);
> > +
> > +size_t push_action_size = 0;
> > +size_t clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> > 

Re: [ovs-dev] [PATCH] ofproto: Limit log rate when controller disconnected.

[Ben Pfaff]

On Fri, Mar 24, 2017 at 05:16:32AM -0700, nickcooper-zhangtonghao wrote:
> There are a lot of logs when OvS bridges can’t connect
> to controllers. The controller may stop or something
> causes a disruption in network traffic.
> 
> Signed-off-by: nickcooper-zhangtonghao 

This shouldn't be necessary if rconn_logging_connection_attempts__() is
doing its job.  Are you seeing more output than there should be?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] flow: Further refinements to flow_pop_vlan().

[Ben Pfaff]

On Mon, Apr 17, 2017 at 11:23:43AM -0400, Eric Garver wrote:
> On Fri, Apr 14, 2017 at 09:25:41PM -0700, Ben Pfaff wrote:
> > This may help to suppress warnings from know-it-all compilers, and it helps
> > to make the code clearer too.
> > 
> > Reported-by: Darrell Ball 
> > Signed-off-by: Ben Pfaff 
> > ---
> > Sorry, I didn't see v2 of the memset patch until I'd already applied v1.
> > Here's my version of this patch.
> > 
> >  lib/flow.c | 17 +
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/flow.c b/lib/flow.c
> > index 2b1ec4fed7ef..9d2ff93e89e4 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -2205,16 +2205,17 @@ void
> >  flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
> >  {
> >  int n = flow_count_vlan_headers(flow);
> > -if (n == 0) {
> > -return;
> > +if (n > 1) {
> > +if (wc) {
> > +memset(>masks.vlans[1], 0xff,
> > +   sizeof(union flow_vlan_hdr) * (n - 1));
> > +}
> > +memmove(>vlans[0], >vlans[1],
> > +sizeof(union flow_vlan_hdr) * (n - 1));
> >  }
> > -if (wc) {
> > -memset(>masks.vlans[1], 0xff,
> > -   sizeof(union flow_vlan_hdr) * (n - 1));
> > +if (n > 0) {
> > +memset(>vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> >  }
> > -memmove(>vlans[0], >vlans[1],
> > -sizeof(union flow_vlan_hdr) * (n - 1));
> > -memset(>vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> >  }
> >  
> >  void
> > -- 
> > 2.10.2
> 
> Thanks Ben and Darrell!
> 
> Acked-by: Eric Garver 

Thanks!  I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/2] ovn-detrace: A tool decoding ofproto/trace output for ovn debugging.

[Ben Pfaff]

On Thu, Mar 23, 2017 at 11:43:26PM -0700, Han Zhou wrote:
> A python script to decode ofproto/trace output to add ovn lflow
> information inline. It expands lflow further to ACLs when relevant.
> 
> $ ovs-appctl ofproto/trace ... | ovn-decode
> 
> Signed-off-by: Han Zhou 

Thanks!  I applied this.  (I didn't test it though--I hope that you did.)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/2] ovn-northd: Add hint in lflow to link back to acl

[Ben Pfaff]

On Mon, Apr 17, 2017 at 08:04:06AM -0700, Ben Pfaff wrote:
> On Sat, Apr 15, 2017 at 12:12:36AM -0700, Han Zhou wrote:
> > On Fri, Apr 14, 2017 at 9:48 PM, Ben Pfaff  wrote:
> > >
> > > On Thu, Mar 23, 2017 at 11:43:25PM -0700, Han Zhou wrote:
> > > > It will be helpful for trouble-shooting if we can link a logical flow
> > > > back to the ACL that generated it. This patch is to add a stage-hint as
> > > > an external-id in lflow. The hint contains stage specific information.
> > > > Now only lflows in ACL stages have hint, which is the ACL uuid, though
> > > > the same mechanism can be used to add hint for other stages later.
> > > >
> > > > Signed-off-by: Han Zhou 
> > >
> > > Did you consider adding the hint to the match or the action as a
> > > comment?  I guess that there are advantages and disadvantages to each
> > > approach, and I'd like to hear that they've been considered.
> > 
> > I prefer the external-id approach because it is more *structured* and
> > easier to parse. Since we already save "where" information in external-ids,
> > I think it is natural to add stage-hint there, too.
> 
> OK.  This is a reasonable position.

I applied this to master.  Thanks again!

I added a bit of documentation for state-hint in ovn-sb.xml.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5 3/3] ovn-northd: Add logical flows to support native DNS

[Guru Shetty]

On 17 April 2017 at 08:13,  wrote:

> From: Numan Siddique 
>
> OVN implements native DNS resolution which can be used to resolve the
> internal DNS names belonging to a logical datapath.
>
> To support this, a new table 'DNS' is added in the NB DB. A new column
> 'dns_records' is added in 'Logical_Switch' table which references to the
> 'DNS' table.
>
> Following flows are added for each logical switch if configured with
> DNS records in the 'dns_records' column
>  - A logical flow in DNS_LOOKUP stage which uses the action 'dns_lookup'
>to transform the DNS query to DNS reply packet and advances
>to the next stage - DNS_RESPONSE.
>
>  - A logical flow in DNS_RESPONSE stage which implements the DNS responder
>by sending the DNS reply from previous stage back to the inport.
>
> Signed-off-by: Numan Siddique 
>

Acked-by: Gurucharan Shetty 

A few comments below.


> ---
>  ovn/northd/ovn-northd.8.xml |  85 +-
>  ovn/northd/ovn-northd.c | 183 -
>  ovn/ovn-nb.ovsschema|  20 ++-
>  ovn/ovn-nb.xml  |  27 +++-
>  ovn/utilities/ovn-nbctl.c   |   3 +
>  tests/ovn.at| 377 ++
> ++
>  6 files changed, 682 insertions(+), 13 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index ab8fd88..b7e2325 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -724,7 +724,71 @@ output;
>
>  
>
> -Ingress Table 13 Destination Lookup
> +Ingress Table 13 DNS Lookup
> +
> +
> +  This table looks up and resolves the DNS names of the logical ports
> +  if configured with the host names.
>

I think the above should say something like:
This table looks up and resolves the DNS names to the corresponding
configured IP address.



> +
> +
> +
> +  
> +
> +  A priority-100 logical flow for each logical switch datapath
> +  if it is configured with DNS records, which matches the IPv4
> and IPv6
> +  packets with udp.dst = 53 and applies the action
> +  dns_lookup and advances the packet to the next
> table.
> +
> +
> +
> +reg0[4] = dns_lookup(); next;
> +
> +
> +
> +  For valid DNS packets, this transforms the packet into a DNS
> +  reply if the DNS name can be resolved, and stores 1 into
> reg0[4].
> +  For failed DNS resolution or other kinds of packets, it just
> stores
> +  0 into reg0[4]. Either way, it continues to the next table.
> +
> +  
> +
> +
> +Ingress Table 14 DNS Responses
> +
> +
> +  This table implements DNS responder for the DNS replies generated by
> +  the previous table.
> +
> +
> +
> +  
> +
> +  A priority-100 logical flow for each logical switch datapath
> +  if it is configured with DNS records, which matches the IPv4
> and IPv6
> +  packets with udp.dst = 53  reg0[4] == 1
> +  and responds back to the inport after applying
> these
> +  actions.  If reg0[4] is set to 1, it means that the
> +  action dns_lookup was successful.
> +
> +
> +
> +eth.dst - eth.src;
> +ip4.src - ip4.dst;
> +udp.dst = udp.src;
> +udp.src = 53;
> +outport = P;
> +flags.loopback = 1;
> +output;
> +
> +
> +
> +  (This terminates ingress packet processing; the packet does not
> go
> +   to the next ingress table.)
> +
> +  
> +
> +
> +Ingress Table 15 Destination Lookup
>
>  
>This table implements switching behavior.  It contains these logical
> @@ -834,11 +898,22 @@ output;
>  
>
>  
> -  Also a priority 34000 logical flow is added for each logical port
> which
> -  has DHCPv4 options defined to allow the DHCPv4 reply packet and
> which has
> -  DHCPv6 options defined to allow the DHCPv6 reply packet from the
> -  Ingress Table 12: DHCP responses.
> +  Also the following flows are added.
>  
> +
> +  
> +A priority 34000 logical flow is added for each logical port which
> +has DHCPv4 options defined to allow the DHCPv4 reply packet and
> which has
> +DHCPv6 options defined to allow the DHCPv6 reply packet from the
> +Ingress Table 12: DHCP responses.
> +  
> +
> +  
> +A priority 34000 logical flow is added for each logical switch
> datapath
> +if it is configured with DNS records, which allows the DNS reply
> packet
> +from the Ingress Table 14:DNS responses.
>
Usually we mention the match and actions that are added, right?


> +  
> +
>
>  Egress Table 7: Egress Port Security - IP
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 027e5a1..226d05c 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> 

Re: [ovs-dev] [PATCH] doc: fix doc build when not using ovs theme

[Ben Pfaff]

On Tue, Apr 18, 2017 at 11:16:42AM +0100, Stephen Finucane wrote:
> On Mon, 2017-04-17 at 10:21 -0700, Ben Pfaff wrote:
> > On Mon, Apr 17, 2017 at 11:47:30AM -0500, Matthew Thode wrote:
> > > On 04/17/2017 11:42 AM, Ben Pfaff wrote:
> > > > On Mon, Apr 17, 2017 at 11:32:13AM -0500, Matthew Thode wrote:
> > > > > On 04/17/2017 11:20 AM, Ben Pfaff wrote:
> > > > > > On Mon, Apr 17, 2017 at 10:36:26AM -0500, Matthew Thode via
> > > > > > dev wrote:
> > > > > > > Fixes the following warning.
> > > > > > > 
> > > > > > > WARNING: 'default' html theme has been renamed to
> > > > > > > 'classic'. Please change your
> > > > > > > html_theme setting either to the new 'alabaster' default
> > > > > > > theme, or to 'classic'
> > > > > > > to keep using the old default.
> > > > > > > 
> > > > > > > As reported by https://bugs.gentoo.org/show_bug.cgi?id=6145
> > > > > > > 20
> > > > > > > 
> > > > > > > Signed-off-by: Matthew Thode 
> > > > > > 
> > > > > > Thanks.  Do you know whether this is going to break the docs
> > > > > > build for
> > > > > > people with older sphinx?  That is, was "classic" introduced
> > > > > > in sphinx
> > > > > > sometime after 1.1 (since that's the current minimum version
> > > > > > for OVS)?
> > > > > > 
> > > > > 
> > > > > I'm not sure, the oldest version we have is 1.11.0, and the
> > > > > oldest
> > > > > stable version we support is 2.5.0.  This is the first I've
> > > > > seen this
> > > > > bug reported though.
> > > > 
> > > > I guess that you are talking about OVS versions, but I'm asking
> > > > about
> > > > Sphinx versions.  Does that make any difference?  I don't know
> > > > Sphinx
> > > > well at all.
> > > > 
> > > 
> > > I was talking about OVS versions.  This code change only changes
> > > anything if sphinx is not installed.
> > > 
> > > try:
> > > import ovs_sphinx_theme
> > > use_ovs_theme = True
> > > except ImportError:
> > > print("Cannot find 'ovs_sphinx' package. Falling back to
> > > default
> > > theme.")
> > > use_ovs_theme = False
> > > 
> > > then
> > > 
> > > if use_ovs_theme:
> > > html_theme = 'ovs'
> > > else:
> > > html_theme = 'default'
> > 
> > This code only runs at all if sphinx is installed, since it's
> > sphinx-build that runs it.  The conditional is whether the OVS sphinx
> > theme is installed.
> > 
> > My question is, what version of Sphinx (not OVS, not the OVS sphinx
> > theme) introduced a theme named "classic"?  If it is newer than the
> > oldest version of Sphinx that OVS requires, then this patch will
> > break
> > things and we will need to make a choice:
> > 
> > 1. Refine the patch to use "default" if "classic" is not
> >    available.
> > 
> > 2. Live with the warning.
> > 
> > 3. Increase OVS's minimum required Sphinx version.
> 
> It would appear this was a feature introduced in Sphinx 1.3.0 [1][2],
> but which was reverted in Sphinx 1.3.2 [3][4]. We should handle this
> but if we simply rename 'default' to 'classic' then we will break
> support for Sphinx 1.1 and 1.2 users.
> 
> My suggestion would be to simply remove the 'else' clause. This will
> cause Sphinx to use it's own default (alabaster in recent releases,
> default/classic before that) when the 'ovs_sphinx_theme' package is not
> available. I didn't do this before because the 'alabaster' theme is a
> little too sparse for my liking but if it fixes issues for some folks
> then I'm sure I can live with it :) I'll submit a patch shortly that
> will do just this.

This is great.  Thank you.

> PS: If you're using a package then you might want to talk to the
> maintainers of said package: 1.3.2 was released over 2 years ago and
> should really be in use by now.

I personally seem to be using Sphinx 1.4.9, but I'm trying to preserve
compatibility with the versions that we seem to claim to support.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] doc: Don't override default theme

[Ben Pfaff]

On Tue, Apr 18, 2017 at 09:54:24AM -0500, Matthew Thode via dev wrote:
> On 04/18/2017 05:26 AM, Stephen Finucane wrote:
> > Sphinx 1.3 renamed the 'default' theme to 'classic' and configured the
> > 'alabaster' theme as the new default. To prevent breaking existing
> > builds, the 'default' name was reserved as an alias for 'classic' [1].
> > However, initially this raised a warning [1] with a message to use
> > 'classic' instead. This warning was removed in 1.3.2 [2], but it will
> > result in errors (due to the use of the '-W' flag) for Sphinx 1.3.0 and
> > 1.3.1 users.
> > 
> > Mitigate the issue by not setting a theme if the 'ovs_sphinx_theme'
> > package is absent. This will result in Sphinx using its default theme,
> > be that 'classic' (Sphinx < 1.3) or 'alabaster'.
> > 
> > [1] https://github.com/sphinx-doc/sphinx/commit/68021b0bd
> > [2] https://github.com/sphinx-doc/sphinx/commit/034c4e942
> > 
> > Signed-off-by: Stephen Finucane 
> > Cc: Matthew Thode 
> > ---
> > We might want to backport this if there are any releases with the Sphinx
> > docs present.
> > ---
> >  Documentation/conf.py | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/Documentation/conf.py b/Documentation/conf.py
> > index 49514ec..97f402e 100644
> > --- a/Documentation/conf.py
> > +++ b/Documentation/conf.py
> > @@ -145,8 +145,6 @@ linkcheck_anchors = False
> >  #
> >  if use_ovs_theme:
> >  html_theme = 'ovs'
> > -else:
> > -html_theme = 'default'
> >  
> >  # Theme options are theme-specific and customize the look and feel of a 
> > theme
> >  # further.  For a list of options available for each theme, see the
> > 
> Thanks for this, looks good and keeps default behaviour by default
> instead of hard coding a default.

This is a nice way to fix the problem.  Thanks to both of you.

I applied this to master and branch-2.7.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] doc: Remove cruft from conf.py

[Ben Pfaff]

On Tue, Apr 18, 2017 at 11:30:00AM +0100, Stephen Finucane wrote:
> This file has enough going on as-is without keeping all this commented
> out noise around.
> 
> Signed-off-by: Stephen Finucane 

Thanks!  I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 6/6] doc: Remove latex output configuration

[Ben Pfaff]

On Tue, Apr 18, 2017 at 11:59:03AM +0100, Stephen Finucane wrote:
> On Thu, 2017-04-13 at 21:43 -0700, Ben Pfaff wrote:
> > From: Stephen Finucane 
> > 
> > We don't care about building LaTeX documentation, so there's no need
> > to
> > keep this build cruft around.
> > 
> > Signed-off-by: Stephen Finucane 
> > Signed-off-by: Ben Pfaff 
> 
> Unchanged, but fwiw...
> 
> Acked-by: Stephen Finucane 

Thanks a lot for taking a look at these commits.  I applied all of them
to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] openvswitch-2.7.0 installation problem

[Ben Pfaff]

On Wed, Apr 19, 2017 at 11:00:43AM +0100, rihab isims wrote:
> I am following the installation guide from this link
> http://docs.openvswitch.org/en/latest/intro/install/general/
> however i am blocked at "make" command execution whith the following error:
> pthread -lrt -lm -Wl,-rpath -Wl,/root/openvswitch-2.7.0/ovn/lib/.libs
> ovn/lib/.libs/libovn.so: undefined reference to
> `ovsdb_idl_condition_add_clause'
> ovn/lib/.libs/libovn.so: undefined reference to `ovsdb_idl_get_row_for_uuid'

Can you provide the whole build log?  This doesn't make sense.

Thanks,

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

Re: [ovs-dev] [PATCH] ofproto: Report only un-deleted groups in group stats replies.

[Ben Pfaff]

On Thu, Apr 20, 2017 at 11:19:07AM +0200, Timothy M. Redaelli wrote:
> On 04/19/2017 08:29 PM, Ben Pfaff wrote:
> > Deleted groups hang around in the group table until the next grace period,
> > so it's important for the group stats code to pretend that they're gone
> > until they really get deleted.
> 
> [...]
> Thank you for the patch, I tested it in all the scenarios and it works.
> 
> Tested-by: Timothy Redaelli 

Thanks, I applied this to master, branch-2.7, and branch-2.6.  (Earlier
versions didn't have this problem.)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v5 1/3] datapath-windows: Add support for NAT in conntrack

[Yin Lin]

From: Anand Kumar 

Add support for parsing netlink attributes related to NAT
in conntrack.

Co-Authored-by: Anand Kumar 
Co-Authored-by: Darrell Ball 
Signed-off-by: Yin Lin 
---
 datapath-windows/ovsext/Conntrack.c | 73 -
 datapath-windows/ovsext/Conntrack.h | 17 +
 datapath-windows/ovsext/Flow.c  |  4 +-
 3 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 8658910..212e0f0 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -637,7 +637,8 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
   UINT16 zone,
   MD_MARK *mark,
   MD_LABELS *labels,
-  PCHAR helper)
+  PCHAR helper,
+  PNAT_ACTION_INFO natInfo)
 {
 NDIS_STATUS status = NDIS_STATUS_SUCCESS;
 POVS_CT_ENTRY entry = NULL;
@@ -646,6 +647,9 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
 UINT64 currentTime;
 NdisGetCurrentSystemTime((LARGE_INTEGER *) );
 
+/* XXX: Not referenced for now */
+UNREFERENCED_PARAMETER(natInfo);
+
 /* Retrieve the Conntrack Key related fields from packet */
 OvsCtSetupLookupCtx(key, zone, , curNbl, layers->l4Offset);
 
@@ -721,11 +725,14 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 MD_MARK *mark = NULL;
 MD_LABELS *labels = NULL;
 PCHAR helper = NULL;
+NAT_ACTION_INFO natActionInfo;
 PNET_BUFFER_LIST curNbl = fwdCtx->curNbl;
 OVS_PACKET_HDR_INFO *layers = >layers;
 
+NAT_ACTION_INFO natActionInfo;
 NDIS_STATUS status;
 
+memset(, 0, sizeof natActionInfo);
 status = OvsDetectCtPacket(key);
 if (status != NDIS_STATUS_SUCCESS) {
 return status;
@@ -748,6 +755,68 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 if (ctAttr) {
 labels = NlAttrGet(ctAttr);
 }
+natActionInfo.natAction = NAT_ACTION_NONE;
+ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_NAT);
+if (ctAttr) {
+/* Pares Nested NAT attributes. */
+PNL_ATTR natAttr;
+unsigned int left;
+BOOLEAN hasMinIp = FALSE;
+BOOLEAN hasMinPort = FALSE;
+BOOLEAN hasMaxIp = FALSE;
+BOOLEAN hasMaxPort = FALSE;
+NL_NESTED_FOR_EACH_UNSAFE (natAttr, left, ctAttr) {
+enum ovs_nat_attr sub_type_nest = NlAttrType(natAttr);
+switch(sub_type_nest) {
+case OVS_NAT_ATTR_SRC:
+case OVS_NAT_ATTR_DST:
+natActionInfo.natAction |=
+((sub_type_nest == OVS_NAT_ATTR_SRC)
+? NAT_ACTION_SRC : NAT_ACTION_DST);
+break;
+case OVS_NAT_ATTR_IP_MIN:
+memcpy(,
+   NlAttrData(natAttr), natAttr->nlaLen - NLA_HDRLEN);
+hasMinIp = TRUE;
+break;
+case OVS_NAT_ATTR_IP_MAX:
+memcpy(,
+   NlAttrData(natAttr), natAttr->nlaLen - NLA_HDRLEN);
+hasMaxIp = TRUE;
+break;
+case OVS_NAT_ATTR_PROTO_MIN:
+natActionInfo.minPort = NlAttrGetU16(natAttr);
+hasMinPort = TRUE;
+break;
+case OVS_NAT_ATTR_PROTO_MAX:
+natActionInfo.maxPort = NlAttrGetU16(natAttr);
+hasMaxPort = TRUE;
+break;
+case OVS_NAT_ATTR_PERSISTENT:
+case OVS_NAT_ATTR_PROTO_HASH:
+case OVS_NAT_ATTR_PROTO_RANDOM:
+break;
+}
+}
+if (natActionInfo.natAction == NAT_ACTION_NONE) {
+natActionInfo.natAction = NAT_ACTION_REVERSE;
+}
+if (hasMinIp && !hasMaxIp) {
+memcpy(,
+   ,
+   sizeof(natActionInfo.maxAddr));
+}
+if (hasMinPort && !hasMaxPort) {
+natActionInfo.maxPort = natActionInfo.minPort;
+}
+if (hasMinPort || hasMaxPort) {
+if (natActionInfo.natAction & NAT_ACTION_SRC) {
+natActionInfo.natAction |= NAT_ACTION_SRC_PORT;
+} else if (natActionInfo.natAction & NAT_ACTION_DST) {
+natActionInfo.natAction |= NAT_ACTION_DST_PORT;
+}
+}
+}
 ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_HELPER);
 if (ctAttr) {
 helper = NlAttrGetString(ctAttr);
@@ -767,7 +836,7 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 }
 
 status = OvsCtExecute_(curNbl, key, layers, commit, force,
-   zone, mark, labels, helper);
+   zone, mark, labels, helper, );
 return status;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack.h 
b/datapath-windows/ovsext/Conntrack.h
index 87d7eeb..1ad289f 100644
--- a/datapath-windows/ovsext/Conntrack.h

Re: [ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Jarno Rajahalme]

As a policy, Linux kernel datapath changes other than backports need to go to 
upstream Linux first, new features to net-next tree, and bug fixes to net tree. 
See the documentation file ‘backporting-patches.rst’ in directory 
'Documentation/internals/contributing/‘ of the OVS tree for more detailed 
description of the process.

Also, the commit message should contain a clear motivation for the change. 
Changes that enhance readability may adversely affect datapath performance, so 
having a report on performance testing would be helpful in determining whether 
to apply the change.

Regards,

  Jarno

> On Apr 21, 2017, at 12:21 AM, Zhenyu Gao  wrote:
> 
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> 
> Signed-off-by: Zhenyu Gao 
> ---
> datapath/flow.c | 230 
> 1 file changed, 117 insertions(+), 113 deletions(-)
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> sizeof(struct icmphdr));
> }
> 
> +/**
> +  * Parse ipv4 header from an Ethernet frame.
> +  * Return ipv4 header length if successful, otherwise a negative errno 
> value.
> +  */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + int err;
> + struct iphdr *nh;
> + __be16 offset;
> +
> + err = check_iphdr(skb);
> + if (unlikely(err))
> + return err;
> +
> + nh = ip_hdr(skb);
> + key->ipv4.addr.src = nh->saddr;
> + key->ipv4.addr.dst = nh->daddr;
> +
> + key->ip.proto = nh->protocol;
> + key->ip.tos = nh->tos;
> + key->ip.ttl = nh->ttl;
> +
> + offset = nh->frag_off & htons(IP_OFFSET);
> + if (offset) {
> + key->ip.frag = OVS_FRAG_TYPE_LATER;
> + } else {
> + if (nh->frag_off & htons(IP_MF) ||
> + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + } else {
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
> + }
> + }
> + return ip_hdrlen(skb);
> +}
> +
> +/**
> +  * Parse ipv6 header from an Ethernet frame.
> +  * Return ipv6 header length if successful, otherwise a negative errno 
> value.
> +  */
> static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> {
>   unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   else
>   key->ip.frag = OVS_FRAG_TYPE_FIRST;
>   } else {
> - key->ip.frag = OVS_FRAG_TYPE_NONE;
> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + else
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
>   }
> 
>   /* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   key->eth.type = skb->protocol;
> 
>   /* Network layer. */
> - if (key->eth.type == htons(ETH_P_IP)) {
> - struct iphdr *nh;
> - __be16 offset;
> + switch(key->eth.type) {
> + case htons(ETH_P_IP):
> + case htons(ETH_P_IPV6): {
> + int nh_len;
> + if (key->eth.type == htons(ETH_P_IP)) {
> + nh_len = parse_ipv4hdr(skb, key);
> + } else {
> + nh_len = parse_ipv6hdr(skb, key);
> + }
> 
> - error = check_iphdr(skb);
> - if (unlikely(error)) {
> - memset(>ip, 0, sizeof(key->ip));
> - memset(>ipv4, 0, sizeof(key->ipv4));
> - if (error == -EINVAL) {
> + if (unlikely(nh_len < 0)) {
> + switch (nh_len) {
> + case -EINVAL:
> + memset(>ip, 0, sizeof(key->ip));
> + if (key->eth.type == htons(ETH_P_IP)) {
> + memset(>ipv4.addr, 0, 
> sizeof(key->ipv4.addr));
> + } else {
> + memset(>ipv6.addr, 0, 
> sizeof(key->ipv6.addr));
> + }
> + /* fall-through */
> + case -EPROTO:
>   skb->transport_header = skb->network_header;
>   error = 0;
> + break;
> + default:
> + error = nh_len;
>   }
>   return error;
>   }
> 
> - nh = ip_hdr(skb);

Re: [ovs-dev] [RFC v2] OVN localport type support

[Russell Bryant]

On Fri, Apr 21, 2017 at 10:11 AM, Daniel Alvarez  wrote:
> This patch introduces a new type of OVN ports called "localport".
> These ports will be present in every hypervisor and may have the
> same IP/MAC addresses. They are not bound to any chassis and traffic
> to these ports will never go through a tunnel.
>
> Its main use case is the OpenStack metadata API support which relies
> on a local agent running on every hypervisor and serving metadata to
> VM's locally. This service is described in detail at [0].
>
> TODO: tests

will also need to document the new port type in ovn-nb.xml and ovn-sb.xml.

ovn-controller.xml should be updated documenting the new external-id
set on rows in the open_vswitch db.

>
> [0] https://review.openstack.org/#/c/452811/
>
> Signed-off-by: Daniel Alvarez 
> ---
>  ovn/controller/binding.c| 87 
> +
>  ovn/controller/physical.c   | 53 +++
>  ovn/northd/ovn-northd.8.xml |  4 +--
>  ovn/northd/ovn-northd.c |  6 ++--
>  ovn/ovn-architecture.7.xml  | 20 ---
>  5 files changed, 147 insertions(+), 23 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 95e9deb..f96127c 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -352,6 +352,43 @@ setup_qos(const char *egress_iface, struct hmap 
> *queue_map)
>  hmap_destroy(_queues);
>  netdev_close(netdev_phy);
>  }
> +static bool
> +set_ovsport_external_ids(struct controller_ctx *ctx,
> + const struct ovsrec_bridge *bridge,
> + const struct ovsrec_interface *iface_rec,
> + const char *key,
> + const char *value)
> +{
> +if (!ctx->ovs_idl_txn) {
> +return false;
> +}
> +
> +/* Find the port with this interface and add the key/value pair to its
> + * external_ids. Assume 1-1 relationship between port and interface. */
> +int i;
> +for (i = 0; i < bridge->n_ports; i++) {
> +const struct ovsrec_port *port_rec = bridge->ports[i];
> +int j;
> +
> +if (!strcmp(port_rec->name, bridge->name)) {
> +continue;
> +}
> +
> +for (j = 0; j < port_rec->n_interfaces; j++) {
> +if (port_rec->interfaces[j] == iface_rec) {
> +struct smap new_ids;
> +smap_clone(_ids, _rec->external_ids);
> +smap_replace(_ids, key, value);
> +ovsrec_port_verify_external_ids(port_rec);
> +ovsrec_port_set_external_ids(port_rec, _ids);
> +smap_destroy(_ids);
> +return true;
> +}
> +}
> +}
> +return false;
> +}
> +
>
>  static void
>  consider_local_datapath(struct controller_ctx *ctx,
> @@ -359,6 +396,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>  const struct lport_index *lports,
>  const struct sbrec_chassis *chassis_rec,
>  const struct sbrec_port_binding *binding_rec,
> +const struct ovsrec_bridge *bridge,
>  struct hmap *qos_map,
>  struct hmap *local_datapaths,
>  struct shash *lport_to_iface,
> @@ -368,19 +406,40 @@ consider_local_datapath(struct controller_ctx *ctx,
>  = shash_find_data(lport_to_iface, binding_rec->logical_port);
>
>  bool our_chassis = false;
> -if (iface_rec
> -|| (binding_rec->parent_port && binding_rec->parent_port[0] &&
> -sset_contains(local_lports, binding_rec->parent_port))) {
> -if (binding_rec->parent_port && binding_rec->parent_port[0]) {
> -/* Add child logical port to the set of all local ports. */
> -sset_add(local_lports, binding_rec->logical_port);
> -}
> -add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> -   false, local_datapaths);
> -if (iface_rec && qos_map && ctx->ovs_idl_txn) {
> -get_qos_params(binding_rec, qos_map);
> +
> +if (iface_rec && !strcmp(binding_rec->type, "localport")) {
> +/* Make sure localport external_id is present, otherwise set it
> + * for both interface and port. This should only happen the first
> + * time. We set the value on both the interface and port because it's
> + * most convenient to check for the value here on the interface. 
> Later,
> + * we need to read this value in physical.c, and expect to read it 
> from
> + * the port there.*/
> +if (!smap_get(_rec->external_ids, "ovn-localport-port")) {
> +if (set_ovsport_external_ids(ctx, bridge, iface_rec,
> + "ovn-localport-port",
> + binding_rec->logical_port)) {
> +   

Re: [ovs-dev] [PATCH] bridge: Log interface deletion

[Andy Zhou]

On Thu, Apr 20, 2017 at 6:18 PM, Jarno Rajahalme  wrote:
> Acked-by: Jarno Rajahalme 

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

[ovs-dev] [PATCH v7 0/5] datapath-windows: Add support for Ipv4 fragments

[Anand Kumar]

Add support for maintaining and tracking IPv4 fragments.
This patch add a new file IpFragment.c and IpFragment.h which 
includes Ipv4 fragment related API's.
---
v6->v7: Rebase and address comments
v5->v6: Rebase
v4->v5:
  - Modified Patch 3 to retain MRU in _OVS_BUFFER_CONTEXT instead of
using it in ovsForwardingContext with minor changes in rest of the
patches.
v3->v4:
  - Rebase
  - Acquire read lock for read operations.
v2->v3:
  - using spinlock instead of RW lock.
  - updated log messages, summary, fixed alignment issues.
v1->v2:
  - Patch 4 updated to make it compile for release mode.
---
Anand Kumar (5):
  datapath-windows: Added a new file to support Ipv4 fragments.
  datapath-windows: Added Ipv4 fragments support in Conntrack
  datapath-windows: Retain MRU value in the _OVS_BUFFER_CONTEXT.
  datapath-windows: Updated OvsTcpSegmentNBL to handle IP fragments.
  datapath-windows: Fragment NBL based on MRU size

 datapath-windows/automake.mk   |   2 +
 datapath-windows/ovsext/Actions.c  |  72 -
 datapath-windows/ovsext/BufferMgmt.c   | 172 +--
 datapath-windows/ovsext/BufferMgmt.h   |  13 +-
 datapath-windows/ovsext/Conntrack.c|  23 +-
 datapath-windows/ovsext/Debug.h|   3 +-
 datapath-windows/ovsext/DpInternal.h   |   2 +-
 datapath-windows/ovsext/Geneve.c   |   2 +-
 datapath-windows/ovsext/Gre.c  |   2 +-
 datapath-windows/ovsext/IpFragment.c   | 503 +
 datapath-windows/ovsext/IpFragment.h   |  73 +
 datapath-windows/ovsext/Stt.c  |   2 +-
 datapath-windows/ovsext/Switch.c   |   9 +
 datapath-windows/ovsext/User.c |  22 +-
 datapath-windows/ovsext/Util.h |   1 +
 datapath-windows/ovsext/Vxlan.c|   2 +-
 datapath-windows/ovsext/ovsext.vcxproj |   2 +
 17 files changed, 864 insertions(+), 41 deletions(-)
 create mode 100644 datapath-windows/ovsext/IpFragment.c
 create mode 100644 datapath-windows/ovsext/IpFragment.h

-- 
2.9.3.windows.1

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

[ovs-dev] [PATCH v7 4/5] datapath-windows: Updated OvsTcpSegmentNBL to handle IP fragments.

[Anand Kumar]

With this patch, OvsTcpSegmentNBL not only supports fragmenting NBL
to TCP segments but also Ipv4 fragments.

To reflect the new changes, renamed function name from OvsTcpSegmentNBL
to OvsFragmentNBL and created a wrapper for OvsTcpSegmentNBL.

Signed-off-by: Anand Kumar 
---
v6->v7: As per Alins suggesstion, leaving FixSegmentHeader as it is 
and introdude new function FixFragmentHeader to make it look 
simple and clean.
v5->v6: No Change
v4->v5: Changed a variable mss to fragmentSize.
v3->v4: No Change
v2->v3:
- Updated log message and function summary
v1->v2:
- Fix compile error for release mode
---
 datapath-windows/ovsext/BufferMgmt.c | 171 ++-
 datapath-windows/ovsext/BufferMgmt.h |  10 +-
 datapath-windows/ovsext/Geneve.c |   2 +-
 datapath-windows/ovsext/Gre.c|   2 +-
 datapath-windows/ovsext/Stt.c|   2 +-
 datapath-windows/ovsext/User.c   |   2 +-
 datapath-windows/ovsext/Vxlan.c  |   2 +-
 7 files changed, 164 insertions(+), 27 deletions(-)

diff --git a/datapath-windows/ovsext/BufferMgmt.c 
b/datapath-windows/ovsext/BufferMgmt.c
index d99052d..5048ada 100644
--- a/datapath-windows/ovsext/BufferMgmt.c
+++ b/datapath-windows/ovsext/BufferMgmt.c
@@ -1084,6 +1084,31 @@ nblcopy_error:
 return NULL;
 }
 
+NDIS_STATUS
+GetIpHeaderInfo(PNET_BUFFER_LIST curNbl,
+UINT32 *hdrSize)
+{
+CHAR *ethBuf[sizeof(EthHdr)];
+EthHdr *eth;
+IPHdr *ipHdr;
+PNET_BUFFER curNb;
+
+curNb = NET_BUFFER_LIST_FIRST_NB(curNbl);
+ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL);
+
+eth = (EthHdr *)NdisGetDataBuffer(curNb, ETH_HEADER_LENGTH,
+  (PVOID), 1, 0);
+if (eth == NULL) {
+return NDIS_STATUS_INVALID_PACKET;
+}
+ipHdr = (IPHdr *)((PCHAR)eth + ETH_HEADER_LENGTH);
+if (ipHdr == NULL) {
+return NDIS_STATUS_INVALID_PACKET;
+}
+*hdrSize = (UINT32)(ETH_HEADER_LENGTH + (ipHdr->ihl * 4));
+return NDIS_STATUS_SUCCESS;
+}
+
 /*
  * --
  * GetSegmentHeaderInfo
@@ -1110,6 +1135,62 @@ GetSegmentHeaderInfo(PNET_BUFFER_LIST nbl,
 return NDIS_STATUS_SUCCESS;
 }
 
+/*
+ * --
+ * FixFragmentHeader
+ *
+ *Fix IP length, Offset, IP checksum.
+ *XXX - Support IpV6 Fragments
+ * --
+ */
+static NDIS_STATUS
+FixFragmentHeader(PNET_BUFFER nb, UINT16 fragmentSize,
+  BOOLEAN lastPacket, UINT16 offset)
+{
+EthHdr *dstEth = NULL;
+PMDL mdl = NULL;
+PUINT8 bufferStart = NULL;
+
+mdl = NET_BUFFER_FIRST_MDL(nb);
+
+bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(mdl, LowPagePriority);
+if (!bufferStart) {
+return NDIS_STATUS_RESOURCES;
+}
+dstEth = (EthHdr *)(bufferStart + NET_BUFFER_CURRENT_MDL_OFFSET(nb));
+
+switch (dstEth->Type) {
+case ETH_TYPE_IPV4_NBO:
+{
+IPHdr *dstIP = NULL;
+ASSERT((INT)MmGetMdlByteCount(mdl) - NET_BUFFER_CURRENT_MDL_OFFSET(nb)
+>= sizeof(EthHdr) + sizeof(IPHdr));
+
+dstIP = (IPHdr *)((PCHAR)dstEth + sizeof(*dstEth));
+ASSERT((INT)MmGetMdlByteCount(mdl) - NET_BUFFER_CURRENT_MDL_OFFSET(nb)
+>= sizeof(EthHdr) + dstIP->ihl * 4);
+dstIP->tot_len = htons(fragmentSize + dstIP->ihl * 4);
+if (lastPacket) {
+dstIP->frag_off = htons(offset & IP_OFFSET);
+} else {
+dstIP->frag_off = htons((offset & IP_OFFSET) | IP_MF);
+}
+
+dstIP->check = 0;
+dstIP->check = IPChecksum((UINT8 *)dstIP, dstIP->ihl * 4, 0);
+break;
+}
+case ETH_TYPE_IPV6_NBO:
+{
+return NDIS_STATUS_NOT_SUPPORTED;
+}
+default:
+OVS_LOG_ERROR("Invalid eth type: %d\n", dstEth->Type);
+ASSERT(! "Invalid eth type");
+}
+
+return STATUS_SUCCESS;
+}
 
 /*
  * --
@@ -1217,12 +1298,29 @@ FixSegmentHeader(PNET_BUFFER nb, UINT16 segmentSize, 
UINT32 seqNumber,
 
 return STATUS_SUCCESS;
 }
+ /*
+  * --
+ * OvsTcpSegmentNBL --
+ *Wrapper function to Fragment a given NBL based on MSS
+ * --
+ */
+PNET_BUFFER_LIST
+OvsTcpSegmentNBL(PVOID ovsContext,
+ PNET_BUFFER_LIST nbl,
+ POVS_PACKET_HDR_INFO hdrInfo,
+ UINT32 mss,
+ UINT32 headRoom,
+ BOOLEAN isIpFragment)
+{
+return OvsFragmentNBL(ovsContext, nbl, hdrInfo, mss, headRoom, 
isIpFragment);
+}
+
 
 /*
  * --
- * OvsTcpSegmentNBL --
+ * 

[ovs-dev] [PATCH v7 1/5] datapath-windows: Added a new file to support Ipv4 fragments.

[Anand Kumar]

This patch adds functionalities to support IPv4 fragments, which will be
used by Conntrack module.

Added a new structure to hold the Ipv4 fragments and a hash table to
hold Ipv4 datagram entries. Also added a clean up thread that runs
every minute to delete the expired IPv4 datagram entries.

The individual fragments are ignored by the conntrack. Once all the
fragments are recieved, a new NBL is created out of the reassembled
fragments and conntrack executes actions on the new NBL.

Created new APIs OvsProcessIpv4Fragment() to process individual fragments,
OvsIpv4Reassemble() to reassemble Ipv4 fragments.

Signed-off-by: Anand Kumar 
---
v6->v7: Added new memory pool tag and align function description 
v5->v6: No Change
v4->v5:
  - Modified Patch 3 to retain MRU in _OVS_BUFFER_CONTEXT instead of
using it in ovsForwardingContext with minor changes in rest of the
patches.
v3->v4:
  - Rebase
  - Acquire read lock for read operations.
v2->v3:
  - using spinlock instead of RW lock.
  - updated log messages, summary, fixed alignment issues.
v1->v2:
  - Patch 4 updated to make it compile for release mode.
---
 datapath-windows/automake.mk   |   2 +
 datapath-windows/ovsext/Debug.h|   3 +-
 datapath-windows/ovsext/IpFragment.c   | 501 +
 datapath-windows/ovsext/IpFragment.h   |  73 +
 datapath-windows/ovsext/Switch.c   |   9 +
 datapath-windows/ovsext/Util.h |   1 +
 datapath-windows/ovsext/ovsext.vcxproj |   2 +
 7 files changed, 590 insertions(+), 1 deletion(-)
 create mode 100644 datapath-windows/ovsext/IpFragment.c
 create mode 100644 datapath-windows/ovsext/IpFragment.h

diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk
index 53983ae..4f7b55a 100644
--- a/datapath-windows/automake.mk
+++ b/datapath-windows/automake.mk
@@ -32,6 +32,8 @@ EXTRA_DIST += \
datapath-windows/ovsext/Flow.h \
datapath-windows/ovsext/Gre.h \
datapath-windows/ovsext/Gre.c \
+   datapath-windows/ovsext/IpFragment.c \
+   datapath-windows/ovsext/IpFragment.h \
datapath-windows/ovsext/IpHelper.c \
datapath-windows/ovsext/IpHelper.h \
datapath-windows/ovsext/Jhash.c \
diff --git a/datapath-windows/ovsext/Debug.h b/datapath-windows/ovsext/Debug.h
index cae6ac9..6de1812 100644
--- a/datapath-windows/ovsext/Debug.h
+++ b/datapath-windows/ovsext/Debug.h
@@ -42,8 +42,9 @@
 #define OVS_DBG_STT  BIT32(22)
 #define OVS_DBG_CONTRK   BIT32(23)
 #define OVS_DBG_GENEVE   BIT32(24)
+#define OVS_DBG_IPFRAG   BIT32(25)
 
-#define OVS_DBG_LAST 24  /* Set this to the last defined module number. */
+#define OVS_DBG_LAST 25  /* Set this to the last defined module number. */
 /* Please add above OVS_DBG_LAST. */
 
 #define OVS_DBG_ERRORDPFLTR_ERROR_LEVEL
diff --git a/datapath-windows/ovsext/IpFragment.c 
b/datapath-windows/ovsext/IpFragment.c
new file mode 100644
index 000..a459610
--- /dev/null
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -0,0 +1,501 @@
+/*
+ * Copyright (c) 2017 VMware, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "Conntrack.h"
+#include "Debug.h"
+#include "IpFragment.h"
+#include "Jhash.h"
+#include "Offload.h"
+#include "PacketParser.h"
+
+#ifdef OVS_DBG_MOD
+#undef OVS_DBG_MOD
+#endif
+#define OVS_DBG_MOD OVS_DBG_IPFRAG
+
+/* Function declarations */
+static VOID OvsIpFragmentEntryCleaner(PVOID data);
+static VOID OvsIpFragmentEntryDelete(POVS_IPFRAG_ENTRY entry);
+
+/* Global and static variables */
+static OVS_IPFRAG_THREAD_CTX ipFragThreadCtx;
+static PNDIS_RW_LOCK_EX ovsIpFragmentHashLockObj;
+static UINT64 ipTotalEntries;
+static PLIST_ENTRY OvsIpFragTable;
+
+NDIS_STATUS
+OvsInitIpFragment(POVS_SWITCH_CONTEXT context)
+{
+
+NDIS_STATUS status;
+HANDLE threadHandle = NULL;
+
+/* Init the sync-lock */
+ovsIpFragmentHashLockObj = NdisAllocateRWLock(context->NdisFilterHandle);
+if (ovsIpFragmentHashLockObj == NULL) {
+return STATUS_INSUFFICIENT_RESOURCES;
+}
+
+/* Init the Hash Buffer */
+OvsIpFragTable = OvsAllocateMemoryWithTag(sizeof(LIST_ENTRY)
+  * IP_FRAG_HASH_TABLE_SIZE,
+  OVS_IPFRAG_POOL_TAG);
+if (OvsIpFragTable == NULL) {
+NdisFreeRWLock(ovsIpFragmentHashLockObj);
+ovsIpFragmentHashLockObj = NULL;
+return 

[ovs-dev] [PATCH v7 3/5] datapath-windows: Retain MRU value in the _OVS_BUFFER_CONTEXT.

[Anand Kumar]

This patch introduces a new field MRU(Maximum Recieved Unit) in the
_OVS_BUFFER_CONTEXT and it is used only for Ip Fragments to retain MRU for
the reassembled IP datagram when the packet is forwarded to userspace.

Signed-off-by: Anand Kumar 
---
v6->v7: Increase value size in OVS_BUFFER_CONTEXT
v5->v6: No Change
v4->v5:
- Refactored the patch as MRU field is removed form 
ovsForwardingContext.
- Added MRU field in _OVS_BUFFER_CONTEXT.
- Updated commit message.
v3->v4: No Change
v2->v3: No change
v1->v2: No change
---
 datapath-windows/ovsext/BufferMgmt.c |  1 +
 datapath-windows/ovsext/BufferMgmt.h |  3 ++-
 datapath-windows/ovsext/DpInternal.h |  2 +-
 datapath-windows/ovsext/IpFragment.c |  2 ++
 datapath-windows/ovsext/User.c   | 20 +++-
 5 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/datapath-windows/ovsext/BufferMgmt.c 
b/datapath-windows/ovsext/BufferMgmt.c
index 6027c35..d99052d 100644
--- a/datapath-windows/ovsext/BufferMgmt.c
+++ b/datapath-windows/ovsext/BufferMgmt.c
@@ -266,6 +266,7 @@ OvsInitNBLContext(POVS_BUFFER_CONTEXT ctx,
 ctx->flags = flags;
 ctx->srcPortNo = srcPortNo;
 ctx->origDataLength = origDataLength;
+ctx->mru = 0;
 }
 
 
diff --git a/datapath-windows/ovsext/BufferMgmt.h 
b/datapath-windows/ovsext/BufferMgmt.h
index 11a05b2..294d40a 100644
--- a/datapath-windows/ovsext/BufferMgmt.h
+++ b/datapath-windows/ovsext/BufferMgmt.h
@@ -58,9 +58,10 @@ typedef union _OVS_BUFFER_CONTEXT {
 UINT32 origDataLength;
 UINT32 dataOffsetDelta;
 };
+UINT16 mru;
 };
 
-UINT64 value[MEM_ALIGN_SIZE(16) >> 3];
+UINT64 value[MEM_ALIGN_SIZE(32) >> 3];
 } OVS_BUFFER_CONTEXT, *POVS_BUFFER_CONTEXT;
 
 typedef struct _OVS_NBL_POOL {
diff --git a/datapath-windows/ovsext/DpInternal.h 
b/datapath-windows/ovsext/DpInternal.h
index f62fc55..9d1a783 100644
--- a/datapath-windows/ovsext/DpInternal.h
+++ b/datapath-windows/ovsext/DpInternal.h
@@ -298,7 +298,7 @@ typedef struct _OVS_PACKET_INFO {
 typedef struct OvsPacketExecute {
uint32_t dpNo;
uint32_t inPort;
-
+   uint16 mru;
uint32_t packetLen;
uint32_t actionsLen;
PNL_MSG_HDR nlMsgHdr;
diff --git a/datapath-windows/ovsext/IpFragment.c 
b/datapath-windows/ovsext/IpFragment.c
index a459610..b13fbbf 100644
--- a/datapath-windows/ovsext/IpFragment.c
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -208,6 +208,8 @@ OvsIpv4Reassemble(POVS_SWITCH_CONTEXT switchContext,
 OvsCompleteNBL(switchContext, *curNbl, TRUE);
 }
 /* Store mru in the ovs buffer context. */
+ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(*newNbl);
+ctx->mru = entry->mru;
 *curNbl = *newNbl;
 return status;
 }
diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
index c7ac284..3154640 100644
--- a/datapath-windows/ovsext/User.c
+++ b/datapath-windows/ovsext/User.c
@@ -283,7 +283,8 @@ OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT 
usrParamsCtx,
 [OVS_PACKET_ATTR_ACTIONS] = {.type = NL_A_UNSPEC, .optional = FALSE},
 [OVS_PACKET_ATTR_USERDATA] = {.type = NL_A_UNSPEC, .optional = TRUE},
 [OVS_PACKET_ATTR_EGRESS_TUN_KEY] = {.type = NL_A_UNSPEC,
-.optional = TRUE}
+.optional = TRUE},
+[OVS_PACKET_ATTR_MRU] = { .type = NL_A_U16, .optional = TRUE }
 };
 
 RtlZeroMemory(, sizeof(OvsPacketExecute));
@@ -381,6 +382,10 @@ _MapNlAttrToOvsPktExec(PNL_MSG_HDR nlMsgHdr, PNL_ATTR 
*nlAttrs,
 ASSERT(keyAttrs[OVS_KEY_ATTR_IN_PORT]);
 execute->inPort = NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_IN_PORT]);
 execute->keyAttrs = keyAttrs;
+
+if (nlAttrs[OVS_PACKET_ATTR_MRU]) {
+execute->mru = NlAttrGetU16(nlAttrs[OVS_PACKET_ATTR_MRU]);
+}
 }
 
 NTSTATUS
@@ -397,6 +402,7 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
 POVS_VPORT_ENTRYvport = NULL;
 PNL_ATTR tunnelAttrs[__OVS_TUNNEL_KEY_ATTR_MAX];
 OvsFlowKey tempTunKey = {0};
+POVS_BUFFER_CONTEXT ctx;
 
 if (execute->packetLen == 0) {
 status = STATUS_INVALID_PARAMETER;
@@ -459,6 +465,9 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
 ndisStatus = OvsExtractFlow(pNbl, execute->inPort, , ,
  tempTunKey.tunKey.dst == 0 ? NULL : );
 
+ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(pNbl);
+ctx->mru = execute->mru;
+
 if (ndisStatus == NDIS_STATUS_SUCCESS) {
 NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, , 0);
 ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl,
@@ -988,6 +997,7 @@ OvsCreateQueueNlPacket(PVOID userData,
 UINT32 nlMsgSize;
 NL_BUFFER nlBuf;
 PNL_MSG_HDR nlMsg;
+POVS_BUFFER_CONTEXT ctx;
 
 if (vport == NULL){
 /* No vport is not fatal. */
@@ -1071,6 +1081,14 @@ OvsCreateQueueNlPacket(PVOID userData,
 goto fail;
 }
 

[ovs-dev] [PATCH v7 2/5] datapath-windows: Added Ipv4 fragments support in Conntrack

[Anand Kumar]

This patch adds support for tracking Ipv4 fragments in conntrack module.
Individual fragments are not tracked and are consumed by the
fragmentation/reassembly. Only the reassembled Ipv4 datagram is tracked and
treated as a single ct entry.

Signed-off-by: Anand Kumar 
---
v6->v7: Made changes to use FowardingCtx and initialize forwarding ctx 
for the reassembled packet
v5->v6: No Change
v4->v5:
- Removed MRU argument from function declarations as MRU is now retained
in _OVS_BUFFER_CONTEXT.
v3->v4: No Change
v2->v3:
- Updated log messages and fixed alignment.
v1->v2: No change
---
 datapath-windows/ovsext/Actions.c   | 21 +++--
 datapath-windows/ovsext/Conntrack.c | 23 ---
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index 3bd00a7..b5c13c7 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1975,12 +1975,29 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext,
 }
 }
 
+PNET_BUFFER_LIST oldNbl = ovsFwdCtx.curNbl;
 status = OvsExecuteConntrackAction(, key,
(const PNL_ATTR)a);
 if (status != NDIS_STATUS_SUCCESS) {
-OVS_LOG_ERROR("CT Action failed");
-dropReason = L"OVS-conntrack action failed";
+/* Pending NBLs are consumed by Defragmentation. */
+if (status != NDIS_STATUS_PENDING) {
+OVS_LOG_ERROR("CT Action failed");
+dropReason = L"OVS-conntrack action failed";
+}
 goto dropit;
+} else if (oldNbl != ovsFwdCtx.curNbl) {
+   /*
+* OvsIpv4Reassemble consumes the original NBL and creates a
+* new one and assigns it to the curNbl of ovsFwdCtx.
+*/
+   OvsInitForwardingCtx(,
+ovsFwdCtx.switchContext,
+ovsFwdCtx.curNbl,
+ovsFwdCtx.srcVportNo,
+ovsFwdCtx.sendFlags,
+
NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(ovsFwdCtx.curNbl),
+ovsFwdCtx.completionList,
+, FALSE);
 }
 break;
 }
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 8658910..dce0c1b 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -15,6 +15,7 @@
  */
 
 #include "Conntrack.h"
+#include "IpFragment.h"
 #include "Jhash.h"
 #include "PacketParser.h"
 #include "Event.h"
@@ -317,13 +318,20 @@ OvsCtEntryExpired(POVS_CT_ENTRY entry)
 }
 
 static __inline NDIS_STATUS
-OvsDetectCtPacket(OvsFlowKey *key)
+OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
+  OvsFlowKey *key,
+  PNET_BUFFER_LIST *newNbl)
 {
 /* Currently we support only Unfragmented TCP packets */
 switch (ntohs(key->l2.dlType)) {
 case ETH_TYPE_IPV4:
 if (key->ipKey.nwFrag != OVS_FRAG_TYPE_NONE) {
-return NDIS_STATUS_NOT_SUPPORTED;
+return OvsProcessIpv4Fragment(fwdCtx->switchContext,
+  >curNbl,
+  fwdCtx->completionList,
+  fwdCtx->fwdDetail->SourcePortId,
+  key->tunKey.tunnelId,
+  newNbl);
 }
 if (key->ipKey.nwProto == IPPROTO_TCP
 || key->ipKey.nwProto == IPPROTO_UDP
@@ -707,6 +715,7 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
  *---
  * OvsExecuteConntrackAction
  * Executes Conntrack actions XXX - Add more
+ * For the Ipv4 fragments, consume the orginal fragment NBL
  *---
  */
 NDIS_STATUS
@@ -723,10 +732,10 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 PCHAR helper = NULL;
 PNET_BUFFER_LIST curNbl = fwdCtx->curNbl;
 OVS_PACKET_HDR_INFO *layers = >layers;
-
+PNET_BUFFER_LIST newNbl = NULL;
 NDIS_STATUS status;
 
-status = OvsDetectCtPacket(key);
+status = OvsDetectCtPacket(fwdCtx, key, );
 if (status != NDIS_STATUS_SUCCESS) {
 return status;
 }
@@ -765,9 +774,9 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 /* Force implicitly means commit */
 commit = TRUE;
 }
-
-status = OvsCtExecute_(curNbl, key, layers, commit, force,
-   zone, mark, labels, helper);
+/* If newNbl is not allocated, use the current 

Re: [ovs-dev] [PATCH 2/2] ovn-util: Allow /32 IP addresses for router ports.

[Guru Shetty]

On 21 April 2017 at 10:42, Ben Pfaff  wrote:

> On Fri, Apr 21, 2017 at 10:10:26AM -0700, Guru Shetty wrote:
> > On 14 April 2017 at 20:38, Ben Pfaff  wrote:
> >
> > > On Thu, Mar 09, 2017 at 11:46:38PM -0800, Gurucharan Shetty wrote:
> > > > On Google cloud, a VM gets a /32 IP address. When OVN
> > > > is deployed on such VMs, the OVN gateway router's IP
> > > > address becomes a /32 IP address. This commit allows
> > > > such a configuration.
> > > >
> > > > Signed-off-by: Gurucharan Shetty 
> > >
> > > A patch that adds a feature, but only deletes code, and adds a test?
> > > SIGN ME UP!
> > >
> > > Acked-by: Ben Pfaff 
> > >
> >
> > Thank you for the review. There is a first patch in the series too. I am
> > not sure whether you looked at it.
>
> I did.  I guess I forgot to send my ack.  Done now.
>
Thank you. I applied the series to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] ovn-util: Allow /32 IP addresses for router ports.

[Ben Pfaff]

On Fri, Apr 21, 2017 at 10:10:26AM -0700, Guru Shetty wrote:
> On 14 April 2017 at 20:38, Ben Pfaff  wrote:
> 
> > On Thu, Mar 09, 2017 at 11:46:38PM -0800, Gurucharan Shetty wrote:
> > > On Google cloud, a VM gets a /32 IP address. When OVN
> > > is deployed on such VMs, the OVN gateway router's IP
> > > address becomes a /32 IP address. This commit allows
> > > such a configuration.
> > >
> > > Signed-off-by: Gurucharan Shetty 
> >
> > A patch that adds a feature, but only deletes code, and adds a test?
> > SIGN ME UP!
> >
> > Acked-by: Ben Pfaff 
> >
> 
> Thank you for the review. There is a first patch in the series too. I am
> not sure whether you looked at it.

I did.  I guess I forgot to send my ack.  Done now.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ovn-northd: Allow static routes with nexthop in different subnet.

[Ben Pfaff]

On Thu, Mar 09, 2017 at 11:46:37PM -0800, Gurucharan Shetty wrote:
> There are cases where the default gateway of a interface is in
> a different subnet than its IP address. Linux allows such
> configuration. For e.g, one could set the IP address of
> a Linux interface as 172.16.1.2/32 and then give it a default
> gateway of 172.16.1.1.  This can be done for e.g. by running the
> following commands.
> 
> ifconfig eth0 172.16.1.2 netmask 255.255.255.255 broadcast 172.16.1.2
> route add 172.16.1.1 dev eth0
> route add default gw 172.16.1.1
> 
> The above configuration is what google cloud uses for its VMs.
> 
> In OVN static routes, we currently have the ability to specify the
> router port via which the packet needs to be pushed out to reach a
> next hop.  But when support for IPv6 was added, we only allowed
> nexthops to be in the same subnet as one of the router's IP addresses.
> 
> This commit relaxes that restriction. When a outport is specified in
> static routes and when a nexthop is in a different subnet than any
> of the router IP addresses, we will assume that it is reachable from
> the first IP address of the router.  Since this is a corner case,
> we just go with the first IP address.  If it turns out that there
> are more cases, we can let users choose the IP address via which
> the destination is reachable.
> 
> Signed-off-by: Gurucharan Shetty 
> ---
> Patch2 of the series includes a unit test that also covers this
> case.

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

Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness

[Ben Pfaff]

Thanks for the revision.

I still saw the failures, so I spent some time looking at why.  It
turned out to be related to the two different database transactions used
to configure BFD.  When I combined the two transactions into one,
e.g. changed

AT_CHECK([ovs-vsctl add-br br1 -- \
  set bridge br1 datapath-type=dummy \
  other-config:hwaddr=aa:55:aa:56:00:00 -- \
  add-port br1 p1 -- set Interface p1 type=patch \
  options:peer=p0 ofport_request=2 -- \
  add-port br0 p0 -- set Interface p0 type=patch \
  options:peer=p1 ofport_request=1])

AT_CHECK([ovs-vsctl \
  set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 -- 
\
  set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])

into

AT_CHECK([ovs-vsctl add-br br1 -- \
  set bridge br1 datapath-type=dummy \
  other-config:hwaddr=aa:55:aa:56:00:00 -- \
  add-port br1 p1 -- set Interface p1 type=patch \
  options:peer=p0 ofport_request=2 -- \
  add-port br0 p0 -- set Interface p0 type=patch \
  options:peer=p1 ofport_request=1 -- \
  set Interface p0 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100 -- 
\
  set Interface p1 bfd:enable=true bfd:min_tx=100 bfd:min_rx=100])

I no longer saw the failures.

I made that change in each of the BFD tests.  I also got rid of the use
of the GNU grep --no-group-separator option, which BSD doesn't appear to
support.

With those changes, I applied this to master.  Thanks again!

On Thu, Apr 20, 2017 at 03:41:57PM +, László Sürü wrote:
> Thanks for the review and comments.
> All unit tests were passing for me before with 'make check', I haven't seen 
> failing unit tests.
> It might have been a temporary misbehavior.
> 
> I've applied the proposed changes and successfully rerun the tests 
> also with 'testsuite' this time based on your attached log (see attachment).
> Also, I've adapted the tests to the new OF 1.6 test as well.
> 
> Please find the modified patched below in this mail.
> 
> Thanks and regards
> Laszlo
> 
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org] 
> Sent: Saturday, April 15, 2017 6:44 AM
> To: László Sürü 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as 
> link aliveness
> 
> Thanks a lot for the revised patch!  It will be good to get this done 
> properly.
> 
> I'm appending some changes that I suggest folding in to better match the 
> usual OVS coding style.
> 
> However, when I run the testsuite, I get the failures below.  Do you see 
> these too?  Can you take a look at them?  I'm also attaching the full 
> testsuite.log in case it helps.
> 
> bfd
> 
>  22: bfd - liveness propagation - OF1.3  FAILED (bfd.at:847)
>  23: bfd - liveness propagation - OF1.4  FAILED (bfd.at:920)
>  24: bfd - liveness propagation - OF1.5  FAILED (bfd.at:993)
> 
> ofproto
> 
> 890: ofproto - mod-port (OpenFlow 1.6)   FAILED (ofproto.at:1308)
> 
> --8<--cut here-->8--
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 218e8eb..c82640d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1894,6 +1894,16 @@ port_modified(struct ofport *port_)
>  bfd_set_netdev(port->bfd, netdev);
>  }
>  
> +/* Set liveness, unless the link is administratively or
> + * operationally down or link monitoring false */
> +if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) &&
> +port->may_enable) {
> +port->up.pp.state |= OFPUTIL_PS_LIVE;
> +} else {
> +port->up.pp.state &= ~OFPUTIL_PS_LIVE;
> +}
> +
>  ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm,
>   port->lldp, >up.pp.hw_addr);
>  
> @@ -3457,6 +3467,19 @@ port_run(struct ofport_dpif *ofport)
>  if (ofport->rstp_port) {
>  rstp_port_set_mac_operational(ofport->rstp_port, enable);
>  }
> +
> +/* Propagate liveness, unless the link is administratively or
> + * operationally down. */
> +if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
> +!(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
> +enum ofputil_port_state of_state = ofport->up.pp.state;
> +if (enable) {
> +of_state |= OFPUTIL_PS_LIVE;
> +} else {
> +of_state &= ~OFPUTIL_PS_LIVE;
> +}
> +ofproto_port_set_state(>up, of_state);
> +}
>  }
>  
>  ofport->may_enable = enable;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7440d5b..d41e35a 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2473,9 +2473,6 @@ 

Re: [ovs-dev] [PATCH 2/2] ovn-util: Allow /32 IP addresses for router ports.

[Guru Shetty]

On 14 April 2017 at 20:38, Ben Pfaff  wrote:

> On Thu, Mar 09, 2017 at 11:46:38PM -0800, Gurucharan Shetty wrote:
> > On Google cloud, a VM gets a /32 IP address. When OVN
> > is deployed on such VMs, the OVN gateway router's IP
> > address becomes a /32 IP address. This commit allows
> > such a configuration.
> >
> > Signed-off-by: Gurucharan Shetty 
>
> A patch that adds a feature, but only deletes code, and adds a test?
> SIGN ME UP!
>
> Acked-by: Ben Pfaff 
>

Thank you for the review. There is a first patch in the series too. I am
not sure whether you looked at it.


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

Re: [ovs-dev] [PATCH] datapath-windows: Add software checksums for nbl which contain multiple nb

[Guru Shetty]

On 20 April 2017 at 19:43, Alin Serdean 
wrote:

> Until now we only needed to compute software checksums on net buffer lists
> containing a single net buffer.
>
> This patch allows the software checksums to be applied on a net buffer list
> with multiple net buffers. The hard assumption for this, is the net
> buffers are
> part of the same connection. The position of the offsets is pointed by the
> layers parameter.
>
> This will be useful for introducing support ip fragments in conntrack.
>
> Signed-off-by: Alin Gabriel Serdean 
>
Applied, thanks!


> ---
>  datapath-windows/ovsext/Offload.c | 105 --
> 
>  1 file changed, 54 insertions(+), 51 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Offload.c b/datapath-windows/ovsext/
> Offload.c
> index f3ab0e1..65d3b67 100644
> --- a/datapath-windows/ovsext/Offload.c
> +++ b/datapath-windows/ovsext/Offload.c
> @@ -647,7 +647,9 @@ OvsCalculateUDPChecksum(PNET_BUFFER_LIST curNbl,
>   * OvsApplySWChecksumOnNB --
>   *
>   * This function calculates and sets the required software offloads given
> by
> - * csumInfo for a given NBL(nbl) with a single NB.
> + * csumInfo for a given NBL(nbl). If the given net buffer list 'nbl'
> + * has multiple net buffers, we assume that they are part of the same
> + * connection with the same offsets defined in 'layers'.
>   *
>   */
>  NDIS_STATUS
> @@ -661,60 +663,61 @@ OvsApplySWChecksumOnNB(POVS_PACKET_HDR_INFO layers,
>  UINT32 packetLength = 0;
>  ASSERT(nbl != NULL);
>
> -curNb = NET_BUFFER_LIST_FIRST_NB(nbl);
> -ASSERT(curNb->Next == NULL);
> -packetLength = NET_BUFFER_DATA_LENGTH(curNb);
> -curMdl = NET_BUFFER_CURRENT_MDL(curNb);
> -bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
> -   LowPagePriority);
> -if (!bufferStart) {
> -return NDIS_STATUS_RESOURCES;
> -}
> +for (curNb = NET_BUFFER_LIST_FIRST_NB(nbl); curNb != NULL;
> + curNb = curNb->Next) {
> +packetLength = NET_BUFFER_DATA_LENGTH(curNb);
> +curMdl = NET_BUFFER_CURRENT_MDL(curNb);
> +bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
> +
>  LowPagePriority);
> +if (!bufferStart) {
> +return NDIS_STATUS_RESOURCES;
> +}
>
> -bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
> +bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
>
> -if (layers->isIPv4) {
> -IPHdr *ip = (IPHdr *)(bufferStart + layers->l3Offset);
> +if (layers->isIPv4) {
> +IPHdr *ip = (IPHdr *)(bufferStart + layers->l3Offset);
>
> -if (csumInfo->Transmit.IpHeaderChecksum) {
> -ip->check = 0;
> -ip->check = IPChecksum((UINT8 *)ip, 4 * ip->ihl, 0);
> -}
> +if (csumInfo->Transmit.IpHeaderChecksum) {
> +ip->check = 0;
> +ip->check = IPChecksum((UINT8 *)ip, 4 * ip->ihl, 0);
> +}
>
> -if (layers->isTcp && csumInfo->Transmit.TcpChecksum) {
> -UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
> -TCPHdr *tcp = (TCPHdr *)(bufferStart + layers->l4Offset);
> -tcp->check = IPPseudoChecksum(>saddr, >daddr,
> -  IPPROTO_TCP, csumLength);
> -tcp->check = CalculateChecksumNB(curNb, csumLength,
> - (UINT32)(layers->l4Offset));
> -} else if (layers->isUdp && csumInfo->Transmit.UdpChecksum) {
> -UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
> -UDPHdr *udp = (UDPHdr *)((PCHAR)ip + sizeof *ip);
> -udp->check = IPPseudoChecksum(>saddr, >daddr,
> -  IPPROTO_UDP, csumLength);
> -udp->check = CalculateChecksumNB(curNb, csumLength,
> - (UINT32)(layers->l4Offset));
> -}
> -} else if (layers->isIPv6) {
> -IPv6Hdr *ip = (IPv6Hdr *)(bufferStart + layers->l3Offset);
> -
> -if (layers->isTcp && csumInfo->Transmit.TcpChecksum) {
> -UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
> -TCPHdr *tcp = (TCPHdr *)(bufferStart + layers->l4Offset);
> -tcp->check = IPv6PseudoChecksum((UINT32 *) >saddr,
> -(UINT32 *) >daddr,
> -IPPROTO_TCP, csumLength);
> -tcp->check = CalculateChecksumNB(curNb, csumLength,
> - (UINT32)(layers->l4Offset));
> -} else if (layers->isUdp && csumInfo->Transmit.UdpChecksum) {
> -UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
> -UDPHdr *udp = (UDPHdr *)((PCHAR)ip + sizeof *ip);
> -udp->check = 

Re: [ovs-dev] [PATCH v2] datapath-windows: Pass fwdCtx to conntrack

[Guru Shetty]

On 20 April 2017 at 15:26, Yin Lin  wrote:

> There dependencies in Contrack module such as NAT and fragmentation on
> OvsForwardingContext. This patch will make OvsForwardingContext public
> in order to implement these functionalities.
>
> Signed-off-by: Yin Lin 
> Acked-by: Alin Serdean 
>
Applied, thanks.


> ---
>  datapath-windows/ovsext/Actions.c   | 61 ++
> ---
>  datapath-windows/ovsext/Actions.h   | 58 ++
> +
>  datapath-windows/ovsext/Conntrack.c |  5 +--
>  datapath-windows/ovsext/Conntrack.h |  4 +--
>  4 files changed, 65 insertions(+), 63 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/
> Actions.c
> index 46f84bc..3bd00a7 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -71,63 +71,6 @@ typedef struct _OVS_ACTION_STATS {
>  OVS_ACTION_STATS ovsActionStats;
>
>  /*
> - * There a lot of data that needs to be maintained while executing the
> pipeline
> - * as dictated by the actions of a flow, across different functions at
> different
> - * levels. Such data is put together in a 'context' structure. Care
> should be
> - * exercised while adding new members to the structure - only add ones
> that get
> - * used across multiple stages in the pipeline/get used in multiple
> functions.
> - */
> -typedef struct OvsForwardingContext {
> -POVS_SWITCH_CONTEXT switchContext;
> -/* The NBL currently used in the pipeline. */
> -PNET_BUFFER_LIST curNbl;
> -/* NDIS forwarding detail for 'curNbl'. */
> -PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO fwdDetail;
> -/* Array of destination ports for 'curNbl'. */
> -PNDIS_SWITCH_FORWARDING_DESTINATION_ARRAY destinationPorts;
> -/* send flags while sending 'curNbl' into NDIS. */
> -ULONG sendFlags;
> -/* Total number of output ports, used + unused, in 'curNbl'. */
> -UINT32 destPortsSizeIn;
> -/* Total number of used output ports in 'curNbl'. */
> -UINT32 destPortsSizeOut;
> -/*
> - * If 'curNbl' is not owned by OVS, they need to be tracked, if they
> need to
> - * be freed/completed.
> - */
> -OvsCompletionList *completionList;
> -/*
> - * vport number of 'curNbl' when it is passed from the PIF bridge to
> the INT
> - * bridge. ie. during tunneling on the Rx side.
> - */
> -UINT32 srcVportNo;
> -
> -/*
> - * Tunnel key:
> - * - specified in actions during tunneling Tx
> - * - extracted from an NBL during tunneling Rx
> - */
> -OvsIPv4TunnelKey tunKey;
> -
> -/*
> - * Tunneling - Tx:
> - * To store the output port, when it is a tunneled port. We don't
> foresee
> - * multiple tunneled ports as outport for any given NBL.
> - */
> -POVS_VPORT_ENTRY tunnelTxNic;
> -
> -/*
> - * Tunneling - Rx:
> - * Points to the Internal port on the PIF Bridge, if the packet needs
> to be
> - * de-tunneled.
> - */
> -POVS_VPORT_ENTRY tunnelRxNic;
> -
> -/* header information */
> -OVS_PACKET_HDR_INFO layers;
> -} OvsForwardingContext;
> -
> -/*
>   * 
> --
>   * OvsInitForwardingCtx --
>   * Function to init/re-init the 'ovsFwdCtx' context as the actions
> pipeline
> @@ -2032,8 +1975,8 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT
> switchContext,
>  }
>  }
>
> -status = OvsExecuteConntrackAction(ovsFwdCtx.curNbl, layers,
> -   key, (const PNL_ATTR)a);
> +status = OvsExecuteConntrackAction(, key,
> +   (const PNL_ATTR)a);
>  if (status != NDIS_STATUS_SUCCESS) {
>  OVS_LOG_ERROR("CT Action failed");
>  dropReason = L"OVS-conntrack action failed";
> diff --git a/datapath-windows/ovsext/Actions.h b/datapath-windows/ovsext/
> Actions.h
> index c56c260..1ce6c20 100644
> --- a/datapath-windows/ovsext/Actions.h
> +++ b/datapath-windows/ovsext/Actions.h
> @@ -20,6 +20,64 @@
>  #include "Switch.h"
>  #include "PacketIO.h"
>
> +
> +/*
> + * There a lot of data that needs to be maintained while executing the
> pipeline
> + * as dictated by the actions of a flow, across different functions at
> different
> + * levels. Such data is put together in a 'context' structure. Care
> should be
> + * exercised while adding new members to the structure - only add ones
> that get
> + * used across multiple stages in the pipeline/get used in multiple
> functions.
> + */
> +typedef struct OvsForwardingContext {
> +POVS_SWITCH_CONTEXT switchContext;
> +/* The NBL currently used in the pipeline. */
> +PNET_BUFFER_LIST curNbl;
> +/* NDIS forwarding detail for 'curNbl'. */
> +PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO fwdDetail;
> +/* 

Re: [ovs-dev] [PATCH] datapath-windows: Add software checksums for nbl which contain multiple nb

[Anand Kumar]

Acked-by: Anand Kumar 

On 4/20/17, 7:43 PM, "ovs-dev-boun...@openvswitch.org on behalf of Alin 
Serdean"  wrote:

Until now we only needed to compute software checksums on net buffer lists
containing a single net buffer.

This patch allows the software checksums to be applied on a net buffer list
with multiple net buffers. The hard assumption for this, is the net buffers 
are
part of the same connection. The position of the offsets is pointed by the
layers parameter.

This will be useful for introducing support ip fragments in conntrack.

Signed-off-by: Alin Gabriel Serdean 
---
 datapath-windows/ovsext/Offload.c | 105 
--
 1 file changed, 54 insertions(+), 51 deletions(-)

diff --git a/datapath-windows/ovsext/Offload.c 
b/datapath-windows/ovsext/Offload.c
index f3ab0e1..65d3b67 100644
--- a/datapath-windows/ovsext/Offload.c
+++ b/datapath-windows/ovsext/Offload.c
@@ -647,7 +647,9 @@ OvsCalculateUDPChecksum(PNET_BUFFER_LIST curNbl,
  * OvsApplySWChecksumOnNB --
  *
  * This function calculates and sets the required software offloads given 
by
- * csumInfo for a given NBL(nbl) with a single NB.
+ * csumInfo for a given NBL(nbl). If the given net buffer list 'nbl'
+ * has multiple net buffers, we assume that they are part of the same
+ * connection with the same offsets defined in 'layers'.
  *
  */
 NDIS_STATUS
@@ -661,60 +663,61 @@ OvsApplySWChecksumOnNB(POVS_PACKET_HDR_INFO layers,
 UINT32 packetLength = 0;
 ASSERT(nbl != NULL);
 
-curNb = NET_BUFFER_LIST_FIRST_NB(nbl);
-ASSERT(curNb->Next == NULL);
-packetLength = NET_BUFFER_DATA_LENGTH(curNb);
-curMdl = NET_BUFFER_CURRENT_MDL(curNb);
-bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
-   LowPagePriority);
-if (!bufferStart) {
-return NDIS_STATUS_RESOURCES;
-}
+for (curNb = NET_BUFFER_LIST_FIRST_NB(nbl); curNb != NULL;
+ curNb = curNb->Next) {
+packetLength = NET_BUFFER_DATA_LENGTH(curNb);
+curMdl = NET_BUFFER_CURRENT_MDL(curNb);
+bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
+   
LowPagePriority);
+if (!bufferStart) {
+return NDIS_STATUS_RESOURCES;
+}
 
-bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
 
-if (layers->isIPv4) {
-IPHdr *ip = (IPHdr *)(bufferStart + layers->l3Offset);
+if (layers->isIPv4) {
+IPHdr *ip = (IPHdr *)(bufferStart + layers->l3Offset);
 
-if (csumInfo->Transmit.IpHeaderChecksum) {
-ip->check = 0;
-ip->check = IPChecksum((UINT8 *)ip, 4 * ip->ihl, 0);
-}
+if (csumInfo->Transmit.IpHeaderChecksum) {
+ip->check = 0;
+ip->check = IPChecksum((UINT8 *)ip, 4 * ip->ihl, 0);
+}
 
-if (layers->isTcp && csumInfo->Transmit.TcpChecksum) {
-UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
-TCPHdr *tcp = (TCPHdr *)(bufferStart + layers->l4Offset);
-tcp->check = IPPseudoChecksum(>saddr, >daddr,
-  IPPROTO_TCP, csumLength);
-tcp->check = CalculateChecksumNB(curNb, csumLength,
- (UINT32)(layers->l4Offset));
-} else if (layers->isUdp && csumInfo->Transmit.UdpChecksum) {
-UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
-UDPHdr *udp = (UDPHdr *)((PCHAR)ip + sizeof *ip);
-udp->check = IPPseudoChecksum(>saddr, >daddr,
-  IPPROTO_UDP, csumLength);
-udp->check = CalculateChecksumNB(curNb, csumLength,
- (UINT32)(layers->l4Offset));
-}
-} else if (layers->isIPv6) {
-IPv6Hdr *ip = (IPv6Hdr *)(bufferStart + layers->l3Offset);
-
-if (layers->isTcp && csumInfo->Transmit.TcpChecksum) {
-UINT16 csumLength = (UINT16)(packetLength - layers->l4Offset);
-TCPHdr *tcp = (TCPHdr *)(bufferStart + layers->l4Offset);
-tcp->check = IPv6PseudoChecksum((UINT32 *) >saddr,
-(UINT32 *) >daddr,
-IPPROTO_TCP, csumLength);
-tcp->check = 

Re: [ovs-dev] [branch-2.7 0/4] Backport of variable length metaflow field fixes.

[Ben Pfaff]

On Mon, Apr 17, 2017 at 12:58:02PM -0700, Joe Stringer wrote:
> On 14 April 2017 at 20:48, Ben Pfaff  wrote:
> > On Wed, Mar 15, 2017 at 04:01:37PM -0700, Joe Stringer wrote:
> >> Commit 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs."), 
> >> on
> >> branch-2.7 as 9554b03d6ab7, attempted to address incorrect encode and 
> >> decode of
> >> variable length metaflow fields where the OXM/NXM encoding of the variable
> >> length fields would incorrectly serialize the length. The patch addresses 
> >> this
> >> by introducing a new per-bridge structure that adds additional metaflow 
> >> fields
> >> for the variable-length fields on demand when the TLVs are configured by a
> >> controller.
> >>
> >> Unfortunately, in the original patch there was nothing ensuring that flows
> >> referring to variable length fields would retain valid field references 
> >> when
> >> controllers reconfigure the TLVs. In practice, this could lead to a crash 
> >> of
> >> ovs-vswitchd by configuring a TLV field, adding a flow which refers to it,
> >> removing the TLV field, then running some traffic that hit the configured 
> >> flow.
> >>
> >> This series looks to remedy the situation by reference counting the 
> >> variable
> >> length fields and preventing a controller from reconfiguring TLV fields 
> >> when
> >> there are active flows whose match or actions refer to the field.
> >>
> >> This series was applied to master, but given the size of the change and the
> >> minor changes necessary to apply to branch-2.7, I would feel more 
> >> confident in
> >> backporting it if there was an extra round of review to ensure that 
> >> nothing was
> >> missed when this series was first applied to master.
> >
> > Thanks a lot for backporting this.  Backporting is sometimes difficult
> > work and rarely rewarding, so I really appreciate seeing it done.
> >
> > Who should review this?  Jarno, I see you made a comment; do you plan to
> > review it?
> 
> Hi Ben,
> 
> I think for the library ABI side changes, I would appreciate feedback
> from you on how in particular we expect this to be handled (as per my
> update to the thread below).
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329954.html
> 
> I spoke to Jarno offline and he mentioned he would be OK reviewing the
> code itself. The previous piece of feedback about including another
> patch will be addressed by another series which I intend to repost
> shortly.

OK.  I sent some thoughts in the direction of your longer more detailed
message on the subject.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.7 0/4] Backport of variable length metaflow field fixes.

[Ben Pfaff]

On Thu, Apr 20, 2017 at 06:58:18PM -0700, Joe Stringer wrote:
> On 17 March 2017 at 14:30, Joe Stringer  wrote:
> > On 15 March 2017 at 16:01, Joe Stringer  wrote:
> >> Commit 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs."), 
> >> on
> >> branch-2.7 as 9554b03d6ab7, attempted to address incorrect encode and 
> >> decode of
> >> variable length metaflow fields where the OXM/NXM encoding of the variable
> >> length fields would incorrectly serialize the length. The patch addresses 
> >> this
> >> by introducing a new per-bridge structure that adds additional metaflow 
> >> fields
> >> for the variable-length fields on demand when the TLVs are configured by a
> >> controller.
> >>
> >> Unfortunately, in the original patch there was nothing ensuring that flows
> >> referring to variable length fields would retain valid field references 
> >> when
> >> controllers reconfigure the TLVs. In practice, this could lead to a crash 
> >> of
> >> ovs-vswitchd by configuring a TLV field, adding a flow which refers to it,
> >> removing the TLV field, then running some traffic that hit the configured 
> >> flow.
> >>
> >> This series looks to remedy the situation by reference counting the 
> >> variable
> >> length fields and preventing a controller from reconfiguring TLV fields 
> >> when
> >> there are active flows whose match or actions refer to the field.
> >>
> >> This series was applied to master, but given the size of the change and the
> >> minor changes necessary to apply to branch-2.7, I would feel more 
> >> confident in
> >> backporting it if there was an extra round of review to ensure that 
> >> nothing was
> >> missed when this series was first applied to master.
> >
> > One further concern I have with this series is that while it allows us
> > to fix bugs in OVS 2.7, it would change some files in
> > include/openvswitch/, which I believe indirectly implies that it could
> > break the libopenvswitch ABI, which we try not to do within a release
> > series:
> >
> > http://docs.openvswitch.org/en/latest/internals/contributing/libopenvswitch-abi/
> 
> Reporting back, using abipkgdiff from
> libabigail[https://sourceware.org/libabigail], I was able to identify
> the following ABI breakages from v2.7.0 to branch-2.7 with these
> patches applied, details below.
> 
> A bunch of these are libraries only exported through headers in lib/,
> which I believe is not considered 'stable' ABI.
> 
> However, there are several that are exported in include/openvswitch:
> 
> ofpacts_pull_openflow_instructions
> ofperr_encode_hello
> ofputil_decode_flow_stats_request
> ofputil_decode_packet_in
> ofputil_decode_packet_in_private
> ofputil_encode_bundle_msgs
> ofputil_pull_ofp11_match
> 
> Now, the shared library is currently something like
> 'libopenvswitch-2.so.7.0.0'  (or, when we prepare 2.7.1,
> 'libopenvswitch-2.so.7.0.1' unless other changes are made). The
> libtool ABI numbering does not appear to have any influence on the
> naming of the shared library. It is (1,0,0) for current, revision and
> age. I'm suspecting that the right answer to this is to bump current
> and age as per 
> [https://lists.gnu.org/archive/html/libtool/2009-08/msg00034.html].
> When I do this, the ABI appears completely different according to
> abipkgdiff due to the different 'current' number.
> 
> I'm not sure if we are supposed to make the ABI versioning number
> consistent with our shared library naming, or if it is reasonable for
> each OVS release series (eg, 2.7.x) to have independent libtool
> versioning numbers.

Thank you for looking into this.

It sounds like for 2.7.1 we should change the library name from
libopenvswitch-2 to libopenvswitch-2.7.1.  For the long term, it sounds
like maybe we need to include an extra component of the version in the
library name, so that we'd end up with libopenvswitch-X.Y.so.1.0.Z by
default.

What are your thoughts?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVS installation issue with NSH patch

[Ben Pfaff]

On Fri, Apr 21, 2017 at 05:40:57AM +, Kiruthiga R wrote:
> Hi Team,
> 
> I am trying to install OVS 2.5 with the NSH patch available in 
> https://github.com/yyang13/ovs_nsh_patches
> I am facing error while doing "make". Attached the screenshot of error 
> message.

I don't see any attachment.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/5] ovn-trace: Implement ct_next and ct_clear actions.

[Ben Pfaff]

Thank you for all the reviews!  I'm glad to see new people helping out
with reviews.  I applied this series to master.

(No worries about the mistake with strtok_r().  The comment helped to
reassure me that you were reading the code; sometimes I'm a little
worried about that when reviews don't have much to criticize.)

On Fri, Apr 21, 2017 at 10:51:41AM +0200, Miguel Angel Ajo Pelayo wrote:
> Sorry, ignore my previous email.
> 
> "delim" parameter of strtok_r will take any of the characters as delimiter,
> so either "," or " " will work when we pass ", ".
> 
> Apparently my C has got a bit rusty ;)
> 
> 
> The whole patch series looks good to me.
> 
> Acked-By: Miguel Angel Ajo 
> 
> On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo <
> majop...@redhat.com> wrote:
> 
> > Just a very nit opinion/comment. nice work.
> >
> > On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff  wrote:
> >
> >> Signed-off-by: Ben Pfaff 
> >> ---
> >>  NEWS  |  1 +
> >>  ovn/utilities/ovn-trace.8.xml | 58 +
> >>  ovn/utilities/ovn-trace.c | 99 ++
> >> -
> >>  3 files changed, 156 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/NEWS b/NEWS
> >> index eb825ac72161..f0ada38b4019 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -19,6 +19,7 @@ Post-v2.7.0
> >>   * Gratuitous ARP for NAT addresses on a distributed logical router.
> >>   * Allow ovn-controller SSL configuration to be obtained from
> >> vswitchd
> >> database.
> >> + * ovn-trace now has basic support for tracing distributed firewalls.
> >> - Add the command 'ovs-appctl stp/show' (see ovs-vswitchd(8)).
> >> - OpenFlow:
> >>   * Increased support for OpenFlow 1.6 (draft).
> >> diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xm
> >> l
> >> index 78914240d88c..8bb329bfbd71 100644
> >> --- a/ovn/utilities/ovn-trace.8.xml
> >> +++ b/ovn/utilities/ovn-trace.8.xml
> >> @@ -307,6 +307,64 @@
> >>  
> >>
> >>  
> >> +
> >> +--ct=flags
> >> +
> >> +  
> >> +This option sets the ct_state flags that a
> >> +ct_next logical action will report.  The
> >> flags
> >> +must be a comma- or space-separated list of the following
> >> connection
> >> +tracking flags:
> >> +  
> >> +
> >> +  
> >> +
> >> +  trk: Include to indicate connection tracking has
> >> taken
> >> +  place.  (This bit is set automatically even if not listed in
> >> +  flags.
> >> +
> >> +new: Include to indicate a new flow.
> >> +est: Include to indicate an established
> >> flow.
> >> +rel: Include to indicate a related flow.
> >> +rpl: Include to indicate a reply flow.
> >> +inv: Include to indicate a connection entry in a
> >> +bad state.
> >> +dnat: Include to indicate a packet whose
> >> +destination IP address has been changed.
> >> +snat: Include to indicate a packet whose source
> >> IP
> >> +address has been changed.
> >> +  
> >> +
> >> +  
> >> +The ct_next action is used to implement the OVN
> >> +distributed firewall.  For testing, useful flag combinations
> >> include:
> >> +  
> >> +
> >> +  
> >> +trk,new: A packet in a flow in either direction
> >> +through a firewall that has not yet been committed (with
> >> +ct_commit).
> >> +trk,est: A packet in an established flow going
> >> out
> >> +through a firewall.
> >> +trk,rpl: A packet coming in through a firewall
> >> in
> >> +reply to an established flow.
> >> +trk,inv: An invalid packet in either
> >> direction.
> >> +  
> >> +
> >> +  
> >> +A packet might pass through the connection tracker twice in one
> >> trip
> >> +through OVN: once following egress from a VM as it passes outward
> >> +through a firewall, and once preceding ingress to a second VM as
> >> it
> >> +passes inward through a firewall.  Use multiple --ct
> >> +options to specify the flags for multiple ct_next
> >> actions.
> >> +  
> >> +
> >> +  
> >> +When --ct is unspecified, or when there are fewer
> >> +--ct options than ct_next actions, the
> >> +flags default to trk,est.
> >> +  
> >> +
> >>
> >>
> >>Daemon Options
> >> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> >> index 66844b11ac1d..e9463f02a70f 100644
> >> --- a/ovn/utilities/ovn-trace.c
> >> +++ b/ovn/utilities/ovn-trace.c
> >> @@ -69,6 +69,11 @@ static bool minimal;
> >>  static const char *ovs;
> >>  static struct vconn *vconn;
> >>
> >> +/* --ct: Connection tracking state to use for ct_next() actions. */
> >> +static uint32_t *ct_states;
> >> +static size_t n_ct_states;
> >> +static size_t ct_state_idx;
> >> +
> >>  

Re: [ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Greg Rose]

On Fri, 2017-04-21 at 00:21 -0700, Zhenyu Gao wrote:
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> 

I think this patch addresses too many issues and should be broken up
into at least two different patches with appropriate subjects and commit
comments for each.

Thanks,

- Greg

> Signed-off-by: Zhenyu Gao 
> ---
>  datapath/flow.c | 230 
> 
>  1 file changed, 117 insertions(+), 113 deletions(-)
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> sizeof(struct icmphdr));
>  }
>  
> +/**
> +  * Parse ipv4 header from an Ethernet frame.
> +  * Return ipv4 header length if successful, otherwise a negative errno 
> value.
> +  */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + int err;
> + struct iphdr *nh;
> + __be16 offset;
> +
> + err = check_iphdr(skb);
> + if (unlikely(err))
> + return err;
> +
> + nh = ip_hdr(skb);
> + key->ipv4.addr.src = nh->saddr;
> + key->ipv4.addr.dst = nh->daddr;
> +
> + key->ip.proto = nh->protocol;
> + key->ip.tos = nh->tos;
> + key->ip.ttl = nh->ttl;
> +
> + offset = nh->frag_off & htons(IP_OFFSET);
> + if (offset) {
> + key->ip.frag = OVS_FRAG_TYPE_LATER;
> + } else {
> + if (nh->frag_off & htons(IP_MF) ||
> + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + } else {
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
> + }
> + }
> + return ip_hdrlen(skb);
> +}
> +
> +/**
> +  * Parse ipv6 header from an Ethernet frame.
> +  * Return ipv6 header length if successful, otherwise a negative errno 
> value.
> +  */
>  static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>  {
>   unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   else
>   key->ip.frag = OVS_FRAG_TYPE_FIRST;
>   } else {
> - key->ip.frag = OVS_FRAG_TYPE_NONE;
> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + else
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
>   }
>  
>   /* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   key->eth.type = skb->protocol;
>  
>   /* Network layer. */
> - if (key->eth.type == htons(ETH_P_IP)) {
> - struct iphdr *nh;
> - __be16 offset;
> + switch(key->eth.type) {
> + case htons(ETH_P_IP):
> + case htons(ETH_P_IPV6): {
> + int nh_len;
> + if (key->eth.type == htons(ETH_P_IP)) {
> + nh_len = parse_ipv4hdr(skb, key);
> + } else {
> + nh_len = parse_ipv6hdr(skb, key);
> + }
>  
> - error = check_iphdr(skb);
> - if (unlikely(error)) {
> - memset(>ip, 0, sizeof(key->ip));
> - memset(>ipv4, 0, sizeof(key->ipv4));
> - if (error == -EINVAL) {
> + if (unlikely(nh_len < 0)) {
> + switch (nh_len) {
> + case -EINVAL:
> + memset(>ip, 0, sizeof(key->ip));
> + if (key->eth.type == htons(ETH_P_IP)) {
> + memset(>ipv4.addr, 0, 
> sizeof(key->ipv4.addr));
> + } else {
> + memset(>ipv6.addr, 0, 
> sizeof(key->ipv6.addr));
> + }
> + /* fall-through */
> + case -EPROTO:
>   skb->transport_header = skb->network_header;
>   error = 0;
> + break;
> + default:
> + error = nh_len;
>   }
>   return error;
>   }
>  
> - nh = ip_hdr(skb);
> - key->ipv4.addr.src = nh->saddr;
> - key->ipv4.addr.dst = nh->daddr;
> -
> - key->ip.proto = nh->protocol;
> - key->ip.tos = nh->tos;
> - key->ip.ttl = nh->ttl;
> -
> - offset = nh->frag_off & htons(IP_OFFSET);
> - if (offset) {
> - key->ip.frag = OVS_FRAG_TYPE_LATER;
> + if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
> 

[ovs-dev] VMware contact list

[alicia . vaughn]

Hi,



Would you be interested in acquiring VMware contact list for marketing or
email campaign?



We can segment Industry List & Technology Users List by C-level, VP-level,
Director-Level and Manager Level as per your requirements.



We also have alternative Technology such as: Hyper-V, XenServer, Oracle VM,
Red Hat Virtualization, Proxmox VE, vCloud Director, KVM, Xen Project,
Virtuozzo, OpenVZ, Nutanix Acropolis, LXC, UCS Virtual Machine Manager,
Virtkick, Pivot3 vSTAC OS, DataSphere,  and many more.



Kindly review and advice or forward this email to the marketing head in
your company who are currently working on some requirements.



Await your response.



Regards

Alicia Vaughn

Data Specialist



To opt-out replay in
subject line.


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

Re: [ovs-dev] [PATCH] ofproto-dpif: Send QinQ related sFlow counters

[Rzasik, LukaszX]

Hi Neil,

Thanks for the clarification. I now understand that the extended_vlantunnel 
structure should not be used with QinQ scenario.
Most probably I will start the discussion at sflow.org as you suggested.

I will keep you informed about that.

-Original Message-
From: Neil McKee [mailto:neil.mc...@inmon.com] 
Sent: Thursday, April 20, 2017 7:50 PM
To: Eric Garver ; Rzasik, LukaszX ; 
Thomas F Herbert ; d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] ofproto-dpif: Send QinQ related sFlow counters

The purpose of the extended_vlantunnel structure was to help describe the 
packet at the point of sampling, not to describe encap/decap forwarding actions 
that were subsequently applied.  We will need to define new structures on 
sFlow.org to describe QinQ encap/decap actions,  similar to 
http://sflow.org/sflow_tunnels.txt.  Please start a thread at sFlow.org to get 
that rolling.  Once we have those new sFlow structures defined your patch will 
probably look mostly the same.

So what was extended_vlantunnel really for?  To clarify, let's take a specific 
example: the test packet in your patch.  It shows a standard QinQ header using 
Ethernet types 0x88A8 and 0x8100.  If you put that into Wireshark it will have 
no trouble decoding it correctly.  There is nothing we can add to make it 
clearer.  So the sFlow collector should decode it and extract the tags (I know 
of several that will).
No need for any further annotations.  However, if those ethernet types were 
completely arbitrary and non-standard (so that Wireshark would be
defeated) then you would doctor the sampled packet header, remove the two 
802.1Q headers and add 8 to the "stripped" field.  Only then would you report 
the tags in an extended_vlantunnel annotation.

I don't think OVS is likely to encounter non-standard ethernet types in that 
way,  or hardware that strips vlan tags from the packet before software even 
sees it.  So extended_vlantunnel can perhaps be regarded as an anachronism that 
dates back to wild-west days?



On Thu, Apr 20, 2017 at 5:39 AM Rzasik, LukaszX  
wrote:
>
> Hi Neil,
>
> Thanks for the comments. I'll try to explain what is my purpose and why I 
> implemented it that way.
>
> I'm aiming at implementing QinQ related sFlow counters and it seemed that 
> extended_vlantunnel is the right place.
> The only other place that I can find where VLAN tags can be reported is 
> extended_switch but there is place only for a single src_vlan and dst_vlan.
>
> From the description of extended_vlantunnel it fits QinQ scenario ideally. I 
> tried to keep my implementation as close as possible with the description. It 
> does not report all VLAN tags. It reports only stripped VLAN tags when there 
> are multiple tags in the packet.
>
> Also TODO that you left in the code made me confident that it is the right 
> approach:
> /* TODO: 802.1AD(QinQ) is not supported by OVS (yet), so do not
>   * construct a VLAN-stack. The sFlow user-action cookie already
>   * captures the egress VLAN ID so there is nothing more to do here.
>   */
>
> After your comment I looked for some more information about using 
> extended_vlantunnel for reporting non-standard 802.1Q headers but I cannot 
> find any information about that in the sFlow spec.
>
> As you mentioned, the spec states only these three conditions:
>  1. The packet has nested vlan tags, AND
>  2. The reporting device is VLAN aware, AND
>  3. One or more VLAN tags have been stripped, either
> because they represent proprietary encapsulations, or
> because switch hardware automatically strips the outer VLAN
> encapsulation.
>
> And I think they are satisfied in the implementation.
>
> Could you explain in more details why you think this is not the right place 
> to report stripped VLAN tags in QinQ scenario?
>
> The implementation does not consider 0x9100 ethernet type as I could not find 
> it anywhere used in OVS code.
> I could extend the implementation to take it into account as the alternative 
> for 0x88a8.
>
> Thanks!
>
> -Original Message-
> From: Neil McKee [mailto:neil.mc...@inmon.com]
> Sent: Tuesday, April 18, 2017 7:17 PM
> To: Eric Garver ; Rzasik, LukaszX 
> ; d...@openvswitch.org; Thomas F Herbert 
> 
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif: Send QinQ related sFlow 
> counters
>
> This patch seems like it may be reporting a VLAN stack that can already be 
> decoded from the (ingress-sampled) packet header?
>
> The TODO I left in the code may have been misleading...  this structure is 
> intended for the case where the sampled packet-header includes non-standard 
> 802.1Q headers -- which then have to be stripped out of the sampled header to 
> make it decodable,  or where the 802.1Q headers have already been stripped 
> out of the packet before you sampled it 

[ovs-dev] [PATCH] tests/pmd.at: Fix race in "PMD - change numa node" test

[Timothy Redaelli]

Sometimes the test fails since dpif-netdev may process the 2 packets
in the "wrong" order.

This commit avoids the problem by printing (monitor) and verifying
any single packet instead of checking the 2 packets at the same time.

CC: Daniele Di Proietto 
Fixes: a12e2a88d672 ("test: Add more pmd tests.")
Signed-off-by: Timothy Redaelli 
---
 tests/pmd.at | 28 ++--
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/tests/pmd.at b/tests/pmd.at
index 5686bedc..2816d45c 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -338,14 +338,22 @@ AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl 
--detach --no-chdir --pidfile
 
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 --qid 0 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
 
-AT_CHECK([ovs-appctl netdev-dummy/receive p2 --qid 1 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
-
-OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 4])
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
 OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
 
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
 NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=42 in_port=1 (via action) 
data_len=42 (unbuffered)
 
icmp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0
 icmp_csum:f7ff
+])
+
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir 
--pidfile 2> ofctl_monitor.log])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p2 --qid 1 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
 NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=42 in_port=2 (via action) 
data_len=42 (unbuffered)
 
icmp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0
 icmp_csum:f7ff
 ])
@@ -363,14 +371,22 @@ AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl 
--detach --no-chdir --pidfile
 
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 --qid 1 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
 
-AT_CHECK([ovs-appctl netdev-dummy/receive p2 --qid 0 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
-
-OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 4])
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
 OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
 
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
 NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=42 in_port=1 (via action) 
data_len=42 (unbuffered)
 
icmp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0
 icmp_csum:f7ff
+])
+
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir 
--pidfile 2> ofctl_monitor.log])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p2 --qid 0 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
 NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=42 in_port=2 (via action) 
data_len=42 (unbuffered)
 
icmp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0
 icmp_csum:f7ff
 ])
-- 
2.12.0

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

Re: [ovs-dev] [PATCH 6/6] rstp: Add the 'ovs-appctl rstp/show' command.

[nickcooper-zhangtonghao]

> On Apr 21, 2017, at 9:08 AM, Jarno Rajahalme  wrote:
> 
> I’d like to see one of the existing RSTP test cases modified to use this new 
> feature.
> 
> One more comment below,
> 
>  Jarno
> 
> 
>> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao  
>> wrote:
>> 
>> The rstp/show command will help users and developers to
>> get more details about rstp. This patch works together with
>> the previous patches.
>> 
>> Signed-off-by: nickcooper-zhangtonghao 
>> ---
>> NEWS   |   4 +-
>> lib/rstp.c | 113 
>> +++--
>> lib/rstp.h |   2 +-
>> vswitchd/ovs-vswitchd.8.in |  11 -
>> 4 files changed, 123 insertions(+), 7 deletions(-)
>> 
>> diff --git a/NEWS b/NEWS
>> index 00c9106..a28b8da 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -15,7 +15,9 @@ Post-v2.7.0
>> "dot1q-tunnel" port VLAN mode.
>>   - OVN:
>> * Make the DHCPv4 router setting optional.
>> -   - Add the command 'ovs-appctl stp/show' (see ovs-vswitchd(8)).
>> +   - STP/RSTP
>> + * Add the command 'ovs-appctl stp/show' and 'ovs-appctl rstp/show'
>> +   (see ovs-vswitchd(8)).
>> 
>> v2.7.0 - 21 Feb 2017
>> -
>> diff --git a/lib/rstp.c b/lib/rstp.c
>> index b942f6e..7a4f1ea 100644
>> --- a/lib/rstp.c
>> +++ b/lib/rstp.c
>> @@ -120,6 +120,10 @@ static void rstp_port_set_mcheck__(struct rstp_port *, 
>> bool mcheck)
>>OVS_REQUIRES(rstp_mutex);
>> static void reinitialize_port__(struct rstp_port *p)
>>OVS_REQUIRES(rstp_mutex);
>> +static void rstp_unixctl_tcn(struct unixctl_conn *, int argc,
>> + const char *argv[], void *aux);
>> +static void rstp_unixctl_show(struct unixctl_conn *, int argc,
>> +  const char *argv[], void *aux);
>> 
>> const char *
>> rstp_state_name(enum rstp_state state)
>> @@ -208,9 +212,6 @@ rstp_port_get_number(const struct rstp_port *p)
>>return number;
>> }
>> 
>> -static void rstp_unixctl_tcn(struct unixctl_conn *, int argc,
>> - const char *argv[], void *aux);
>> -
>> /* Decrements the State Machines' timers. */
>> void
>> rstp_tick_timers(struct rstp *rstp)
>> @@ -246,6 +247,8 @@ rstp_init(void)
>> 
>>unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, 
>> rstp_unixctl_tcn,
>> NULL);
>> +unixctl_command_register("rstp/show", "[bridge]", 0, 1,
>> + rstp_unixctl_show, NULL);
>>ovsthread_once_done();
>>}
>> }
>> @@ -1398,7 +1401,7 @@ rstp_get_designated_root(const struct rstp *rstp)
>> * there is no such port.
>> */
>> struct rstp_port *
>> -rstp_get_root_port(struct rstp *rstp)
>> +rstp_get_root_port(const struct rstp *rstp)
>>OVS_EXCLUDED(rstp_mutex)
>> {
>>struct rstp_port *p;
>> @@ -1545,3 +1548,105 @@ rstp_unixctl_tcn(struct unixctl_conn *conn, int argc,
>> out:
>>ovs_mutex_unlock(_mutex);
>> }
>> +
>> +static void
>> +rstp_bridge_id_details(struct ds *ds, const rstp_identifier bridge_id,
>> +   const uint16_t hello_time, const uint16_t max_age,
>> +   const uint16_t forward_delay)
>> +OVS_REQUIRES(rstp_mutex)
>> +{
>> +uint16_t priority = bridge_id >> 48;
>> +ds_put_format(ds, "\tstp-priority\t%"PRIu16"\n", priority);
>> +
>> +struct eth_addr mac;
>> +const uint64_t mac_bits = (UINT64_C(1) << 48) - 1;
>> +eth_addr_from_uint64(bridge_id & mac_bits, );
>> +ds_put_format(ds, "\tstp-system-id\t"ETH_ADDR_FMT"\n", 
>> ETH_ADDR_ARGS(mac));
>> +ds_put_format(ds, "\tstp-hello-time\t%"PRIu16"s\n", hello_time);
>> +ds_put_format(ds, "\tstp-max-age\t%"PRIu16"s\n", max_age);
>> +ds_put_format(ds, "\tstp-fwd-delay\t%"PRIu16"s\n", forward_delay);
>> +}
>> +
>> +static void
>> +rstp_print_details(struct ds *ds, const struct rstp *rstp)
>> +OVS_REQUIRES(rstp_mutex)
>> +{
>> +ds_put_format(ds, " %s \n", rstp->name);
>> +ds_put_cstr(ds, "Root ID:\n");
>> +
>> +bool is_root = rstp_is_root_bridge(rstp);
>> +struct rstp_port *p = rstp_get_root_port(rstp);
>> +
>> +rstp_identifier bridge_id =
>> +is_root ? rstp->bridge_identifier : rstp_get_root_id(rstp);
>> +uint16_t hello_time =
>> +is_root ? rstp->bridge_hello_time : p->designated_times.hello_time;
>> +uint16_t max_age =
>> +is_root ? rstp->bridge_max_age : p->designated_times.max_age;
>> +uint16_t forward_delay =
>> +is_root ? rstp->bridge_forward_delay : 
>> p->designated_times.forward_delay;
>> +
>> +rstp_bridge_id_details(ds, bridge_id, hello_time, max_age, 
>> forward_delay);
>> +if (is_root) {
>> +ds_put_cstr(ds, "\tThis bridge is the root\n");
>> +} else {
>> +ds_put_format(ds, "\troot-port\t%s\n", p->port_name);
>> +ds_put_format(ds, "\troot-path-cost\t%u\n",
>> +  rstp_get_root_path_cost(rstp));

Re: [ovs-dev] [PATCH 4/6] rstp: Init a recursive mutex for rstp.

[nickcooper-zhangtonghao]

> On Apr 21, 2017, at 8:59 AM, Jarno Rajahalme  > wrote:
> 
>> 
>> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao > > wrote:
>> 
>> This patch will be used for next patch.
> 
> I don’t see exactly what in the following patch(es) need this. Could you 
> elaborate?

The next patch 6/6 will call some functions which use the mutex and the 
‘rstp/show’ use it agin. 
we should init it as a recursive mutex. 

> 
>> 
>> Signed-off-by: nickcooper-zhangtonghao > >
>> ---
>> lib/rstp.c | 15 ---
>> lib/rstp.h |  6 --
>> 2 files changed, 12 insertions(+), 9 deletions(-)
>> 
>> diff --git a/lib/rstp.c b/lib/rstp.c
>> index 907a907..6f1c1e3 100644
>> --- a/lib/rstp.c
>> +++ b/lib/rstp.c
>> @@ -50,7 +50,7 @@
>> 
>> VLOG_DEFINE_THIS_MODULE(rstp);
>> 
>> -struct ovs_mutex rstp_mutex = OVS_MUTEX_INITIALIZER;
>> +static struct ovs_mutex rstp_mutex;
>> 
>> static struct ovs_list all_rstps__ = OVS_LIST_INITIALIZER(_rstps__);
>> static struct ovs_list *const all_rstps OVS_GUARDED_BY(rstp_mutex) = 
>> _rstps__;
>> @@ -239,8 +239,15 @@ void
>> rstp_init(void)
>>OVS_EXCLUDED(rstp_mutex)
>> {
>> -unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, rstp_unixctl_tcn,
>> - NULL);
>> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> +
>> +if (ovsthread_once_start()) {
>> +ovs_mutex_init_recursive(_mutex);
>> +
>> +unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, 
>> rstp_unixctl_tcn,
>> + NULL);
>> +ovsthread_once_done();
>> +}
>> }
>> 
>> /* Creates and returns a new RSTP instance that initially has no ports. */
>> @@ -255,6 +262,8 @@ rstp_create(const char *name, rstp_identifier 
>> bridge_address,
>> 
>>VLOG_DBG("Creating RSTP instance");
>> 
>> +rstp_init();
>> +
> 
> rstp_init() is already called earlier from the bridge_init(), so I see little 
> point calling it from here. Not having multiple call sites would also remove 
> the need for most of the changes above.

Yes, but some rstp testes, which run with ovstest, will not run rstp_init in 
the bridge_init.

> 
>>rstp = xzalloc(sizeof *rstp);
>>rstp->name = xstrdup(name);
>> 
>> diff --git a/lib/rstp.h b/lib/rstp.h
>> index 4942d59..78e07fb 100644
>> --- a/lib/rstp.h
>> +++ b/lib/rstp.h
>> @@ -36,12 +36,6 @@
>> #include "compiler.h"
>> #include "util.h"
>> 
>> -/* Thread Safety: Callers passing in RSTP and RSTP port object
>> - * pointers must hold a reference to the passed object to ensure that
>> - * the object does not become stale while it is being accessed. */
>> -
>> -extern struct ovs_mutex rstp_mutex;
>> -
> 
> This change, if needed, should be in a separate patch with it’s own commit 
> message.
> 
>  Jarno

Yes, If other patches will be ok, I will put it to a separate patch.




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

Re: [ovs-dev] [PATCH 4/6] rstp: Init a recursive mutex for rstp.

[nickcooper-zhangtonghao]

> On Apr 21, 2017, at 8:59 AM, Jarno Rajahalme  > wrote:
> 
>> 
>> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao > > wrote:
>> 
>> This patch will be used for next patch.
> 
> I don’t see exactly what in the following patch(es) need this. Could you 
> elaborate?

The next patch 6/6 will call some functions which use the mutex and the 
‘rstp/show’ use it agin. 
we should init it as a recursive mutex. 

> 
>> 
>> Signed-off-by: nickcooper-zhangtonghao > >
>> ---
>> lib/rstp.c | 15 ---
>> lib/rstp.h |  6 --
>> 2 files changed, 12 insertions(+), 9 deletions(-)
>> 
>> diff --git a/lib/rstp.c b/lib/rstp.c
>> index 907a907..6f1c1e3 100644
>> --- a/lib/rstp.c
>> +++ b/lib/rstp.c
>> @@ -50,7 +50,7 @@
>> 
>> VLOG_DEFINE_THIS_MODULE(rstp);
>> 
>> -struct ovs_mutex rstp_mutex = OVS_MUTEX_INITIALIZER;
>> +static struct ovs_mutex rstp_mutex;
>> 
>> static struct ovs_list all_rstps__ = OVS_LIST_INITIALIZER(_rstps__);
>> static struct ovs_list *const all_rstps OVS_GUARDED_BY(rstp_mutex) = 
>> _rstps__;
>> @@ -239,8 +239,15 @@ void
>> rstp_init(void)
>>OVS_EXCLUDED(rstp_mutex)
>> {
>> -unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, rstp_unixctl_tcn,
>> - NULL);
>> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> +
>> +if (ovsthread_once_start()) {
>> +ovs_mutex_init_recursive(_mutex);
>> +
>> +unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, 
>> rstp_unixctl_tcn,
>> + NULL);
>> +ovsthread_once_done();
>> +}
>> }
>> 
>> /* Creates and returns a new RSTP instance that initially has no ports. */
>> @@ -255,6 +262,8 @@ rstp_create(const char *name, rstp_identifier 
>> bridge_address,
>> 
>>VLOG_DBG("Creating RSTP instance");
>> 
>> +rstp_init();
>> +
> 
> rstp_init() is already called earlier from the bridge_init(), so I see little 
> point calling it from here. Not having multiple call sites would also remove 
> the need for most of the changes above.

Yes, but some rstp testes, which run with ovstest, will not run rstp_init in 
the bridge_init.

> 
>>rstp = xzalloc(sizeof *rstp);
>>rstp->name = xstrdup(name);
>> 
>> diff --git a/lib/rstp.h b/lib/rstp.h
>> index 4942d59..78e07fb 100644
>> --- a/lib/rstp.h
>> +++ b/lib/rstp.h
>> @@ -36,12 +36,6 @@
>> #include "compiler.h"
>> #include "util.h"
>> 
>> -/* Thread Safety: Callers passing in RSTP and RSTP port object
>> - * pointers must hold a reference to the passed object to ensure that
>> - * the object does not become stale while it is being accessed. */
>> -
>> -extern struct ovs_mutex rstp_mutex;
>> -
> 
> This change, if needed, should be in a separate patch with it’s own commit 
> message.
> 
>  Jarno

Yes, If other patches will be ok, I will put it to a separate patch.




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

Re: [ovs-dev] [PATCH 2/6] stp: Use OpenFlow port number for stp ports.

[nickcooper-zhangtonghao]

> On Apr 21, 2017, at 8:34 AM, Jarno Rajahalme  wrote:
> 
>> 
>> On Mar 31, 2017, at 8:11 PM, nickcooper-zhangtonghao > > wrote:
>> 
>> When a bridge stp enabled, we assign sequentially element
>> of stp_port array (in stp struct) to bridge ports. That is
>> ok when no ports are added to bridge. When adding a port
>> to bridge which stp enabled, the ovs-vswitchd will assign
>> stp_port sequentially again. Then the stp_port belonging
>> to one port may belong to other one.
> 
> Could you elaborate the problem with this STP port renumbering?

When we add a port to bridge which stp is enabled, the stp ports are renumbered.

For example:
p0 is mapped to one array element of struct stp_port ports[STP_MAX_PORTS] (e.g. 
ports[0])
p1 is mapped to one array element of struct stp_port ports[STP_MAX_PORTS] (e.g. 
ports[1])

When we add a port p2 to bridge, the p2 may be mapped to ports[0], p0 is mapped 
to ports[1], and  p1 is mapped to ports[2].
But we hope that p2 may be mapped to ports[2], p0 is mapped to ports[0], and  
p1 is mapped to ports[1].
If not, the stp ports of bridge will converge. As a general rule, the state of 
new port should be changed
(e.g goto forwarding state or blocking.) and other stp ports remain the same.


Test shell:

#!/bin/bash
ovs-vsctl add-br br0
ovs-vsctl add-br br1

ovs-appctl vlog/set ofproto_dpif:dbg

ovs-vsctl add-port br0 p1 -- \
set interface p1 type=dummy options:pstream=punix:/tmp/p1.sock 
ofport_request=1

ovs-vsctl add-port br0 p2 -- \
set interface p2 type=dummy options:pstream=punix:/tmp/p2.sock 
ofport_request=2

ovs-vsctl add-port br1 p6 -- \
set interface p6 type=dummy options:stream=unix:/tmp/p1.sock 
ofport_request=6

ovs-vsctl add-port br1 p7 -- \
set interface p7 type=dummy options:stream=unix:/tmp/p2.sock 
ofport_request=7

ovs-ofctl add-flow br0 action=normal
ovs-ofctl add-flow br1 action=normal

ovs-appctl netdev-dummy/set-admin-state up

ovs-vsctl set port br0 other_config:stp-enable=false -- \
set bridge br0 stp_enable=true other-config:hwaddr=aa:66:aa:66:00:00

ovs-vsctl set port br1 other_config:stp-enable=false -- \
set bridge br1 stp_enable=true other-config:hwaddr=aa:66:aa:66:00:01


# ovs-appctl stp/show
<---cut--->
Bridge ID:
stp-priority32768
stp-system-id   aa:66:aa:66:00:01
stp-hello-time  2s
stp-max-age 20s
stp-fwd-delay   15s

Interface  Role   State  Cost  Pri.Nbr
-- -- -- - ---
p6 alternate  blocking   19128.1
p7 root   forwarding 19128.2


# ovs-vsctl add-port br1 p8
# ovs-appctl stp/show
<---cut--->
Bridge ID:
stp-priority32768
stp-system-id   aa:66:aa:66:00:01
stp-hello-time  2s
stp-max-age 20s
stp-fwd-delay   15s

Interface  Role   State  Cost  Pri.Nbr
-- -- -- - ---
p8 alternate  blocking   19128.1
p6 root   forwarding 19128.2
p7 designated listening  19128.3


After adding the p8 port, p8 uses the p6 state, p6 uses the p7 state, and p7 
uses the new state.
The rstp works well without elaboration.

>> This patch uses the
>> OpenFlow port numbers instead of sequence numbers to avoid
>> it.
>> 
> 
> Using OpenFlow port numbers (32-bit) as STP port numbers (8-bit) seems wrong 
> to me. Besides, you can always use the other-config stp-port-num in ovsdb to 
> specify which STP port number you want, if you do not want them to be 
> numbered automatically.

Yes, the max number of stp ports on bridge is STP_MAX_PORTS(255), and this 
should not be problem. Assigning the openflow port num to stp port is simple. 
But we can use other way to assign stp port number. If you have any idea, 
please let me know.









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

[ovs-dev] Let's work together for the earth to reduce the load.

[Vivian]

Good day,
  Inquiries regarding our new product, the Solar Light,
 have been coming in from all parts of the world. 
Reports from users confirm what we knew before it was put on
 the market - that it is the best solar product available.
 Enclosed is our brochure.   
Yours faithfully!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/5] ovn-trace: Implement ct_next and ct_clear actions.

[Miguel Angel Ajo Pelayo]

Sorry, ignore my previous email.

"delim" parameter of strtok_r will take any of the characters as delimiter,
so either "," or " " will work when we pass ", ".

Apparently my C has got a bit rusty ;)


The whole patch series looks good to me.

Acked-By: Miguel Angel Ajo 

On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo <
majop...@redhat.com> wrote:

> Just a very nit opinion/comment. nice work.
>
> On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff  wrote:
>
>> Signed-off-by: Ben Pfaff 
>> ---
>>  NEWS  |  1 +
>>  ovn/utilities/ovn-trace.8.xml | 58 +
>>  ovn/utilities/ovn-trace.c | 99 ++
>> -
>>  3 files changed, 156 insertions(+), 2 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index eb825ac72161..f0ada38b4019 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -19,6 +19,7 @@ Post-v2.7.0
>>   * Gratuitous ARP for NAT addresses on a distributed logical router.
>>   * Allow ovn-controller SSL configuration to be obtained from
>> vswitchd
>> database.
>> + * ovn-trace now has basic support for tracing distributed firewalls.
>> - Add the command 'ovs-appctl stp/show' (see ovs-vswitchd(8)).
>> - OpenFlow:
>>   * Increased support for OpenFlow 1.6 (draft).
>> diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xm
>> l
>> index 78914240d88c..8bb329bfbd71 100644
>> --- a/ovn/utilities/ovn-trace.8.xml
>> +++ b/ovn/utilities/ovn-trace.8.xml
>> @@ -307,6 +307,64 @@
>>  
>>
>>  
>> +
>> +--ct=flags
>> +
>> +  
>> +This option sets the ct_state flags that a
>> +ct_next logical action will report.  The
>> flags
>> +must be a comma- or space-separated list of the following
>> connection
>> +tracking flags:
>> +  
>> +
>> +  
>> +
>> +  trk: Include to indicate connection tracking has
>> taken
>> +  place.  (This bit is set automatically even if not listed in
>> +  flags.
>> +
>> +new: Include to indicate a new flow.
>> +est: Include to indicate an established
>> flow.
>> +rel: Include to indicate a related flow.
>> +rpl: Include to indicate a reply flow.
>> +inv: Include to indicate a connection entry in a
>> +bad state.
>> +dnat: Include to indicate a packet whose
>> +destination IP address has been changed.
>> +snat: Include to indicate a packet whose source
>> IP
>> +address has been changed.
>> +  
>> +
>> +  
>> +The ct_next action is used to implement the OVN
>> +distributed firewall.  For testing, useful flag combinations
>> include:
>> +  
>> +
>> +  
>> +trk,new: A packet in a flow in either direction
>> +through a firewall that has not yet been committed (with
>> +ct_commit).
>> +trk,est: A packet in an established flow going
>> out
>> +through a firewall.
>> +trk,rpl: A packet coming in through a firewall
>> in
>> +reply to an established flow.
>> +trk,inv: An invalid packet in either
>> direction.
>> +  
>> +
>> +  
>> +A packet might pass through the connection tracker twice in one
>> trip
>> +through OVN: once following egress from a VM as it passes outward
>> +through a firewall, and once preceding ingress to a second VM as
>> it
>> +passes inward through a firewall.  Use multiple --ct
>> +options to specify the flags for multiple ct_next
>> actions.
>> +  
>> +
>> +  
>> +When --ct is unspecified, or when there are fewer
>> +--ct options than ct_next actions, the
>> +flags default to trk,est.
>> +  
>> +
>>
>>
>>Daemon Options
>> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
>> index 66844b11ac1d..e9463f02a70f 100644
>> --- a/ovn/utilities/ovn-trace.c
>> +++ b/ovn/utilities/ovn-trace.c
>> @@ -69,6 +69,11 @@ static bool minimal;
>>  static const char *ovs;
>>  static struct vconn *vconn;
>>
>> +/* --ct: Connection tracking state to use for ct_next() actions. */
>> +static uint32_t *ct_states;
>> +static size_t n_ct_states;
>> +static size_t ct_state_idx;
>> +
>>  OVS_NO_RETURN static void usage(void);
>>  static void parse_options(int argc, char *argv[]);
>>  static char *trace(const char *datapath, const char *flow);
>> @@ -156,6 +161,42 @@ default_ovs(void)
>>  }
>>
>>  static void
>> +parse_ct_option(const char *state_s_)
>> +{
>> +uint32_t state = CS_TRACKED;
>> +
>> +char *state_s = xstrdup(state_s_);
>> +char *save_ptr = NULL;
>> +for (char *cs = strtok_r(state_s, ", ", _ptr); cs;
>>
>
> why do we enforce the use of spaces? ", ", would it
> make sense to accept just "," as delimiter and then
> strip/ignore spaces for usability reasons?
>
>
>> + cs = strtok_r(NULL, ", ", _ptr)) {
>> +

[ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Zhenyu Gao]

1. Consume switch/case to judge type of frame instead of using if/else.
2. Add parse_ipv4hdr for ipv4 frame header parsing.

Signed-off-by: Zhenyu Gao 
---
 datapath/flow.c | 230 
 1 file changed, 117 insertions(+), 113 deletions(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index 2bc1ad0..0b35de6 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
  sizeof(struct icmphdr));
 }
 
+/**
+  * Parse ipv4 header from an Ethernet frame.
+  * Return ipv4 header length if successful, otherwise a negative errno value.
+  */
+static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
+{
+   int err;
+   struct iphdr *nh;
+   __be16 offset;
+
+   err = check_iphdr(skb);
+   if (unlikely(err))
+   return err;
+
+   nh = ip_hdr(skb);
+   key->ipv4.addr.src = nh->saddr;
+   key->ipv4.addr.dst = nh->daddr;
+
+   key->ip.proto = nh->protocol;
+   key->ip.tos = nh->tos;
+   key->ip.ttl = nh->ttl;
+
+   offset = nh->frag_off & htons(IP_OFFSET);
+   if (offset) {
+   key->ip.frag = OVS_FRAG_TYPE_LATER;
+   } else {
+   if (nh->frag_off & htons(IP_MF) ||
+   skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
+   key->ip.frag = OVS_FRAG_TYPE_FIRST;
+   } else {
+   key->ip.frag = OVS_FRAG_TYPE_NONE;
+   }
+   }
+   return ip_hdrlen(skb);
+}
+
+/**
+  * Parse ipv6 header from an Ethernet frame.
+  * Return ipv6 header length if successful, otherwise a negative errno value.
+  */
 static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 {
unsigned int nh_ofs = skb_network_offset(skb);
@@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct 
sw_flow_key *key)
else
key->ip.frag = OVS_FRAG_TYPE_FIRST;
} else {
-   key->ip.frag = OVS_FRAG_TYPE_NONE;
+   if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+   key->ip.frag = OVS_FRAG_TYPE_FIRST;
+   else
+   key->ip.frag = OVS_FRAG_TYPE_NONE;
}
 
/* Delayed handling of error in ipv6_skip_exthdr() as it
@@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct 
sw_flow_key *key)
key->eth.type = skb->protocol;
 
/* Network layer. */
-   if (key->eth.type == htons(ETH_P_IP)) {
-   struct iphdr *nh;
-   __be16 offset;
+   switch(key->eth.type) {
+   case htons(ETH_P_IP):
+   case htons(ETH_P_IPV6): {
+   int nh_len;
+   if (key->eth.type == htons(ETH_P_IP)) {
+   nh_len = parse_ipv4hdr(skb, key);
+   } else {
+   nh_len = parse_ipv6hdr(skb, key);
+   }
 
-   error = check_iphdr(skb);
-   if (unlikely(error)) {
-   memset(>ip, 0, sizeof(key->ip));
-   memset(>ipv4, 0, sizeof(key->ipv4));
-   if (error == -EINVAL) {
+   if (unlikely(nh_len < 0)) {
+   switch (nh_len) {
+   case -EINVAL:
+   memset(>ip, 0, sizeof(key->ip));
+   if (key->eth.type == htons(ETH_P_IP)) {
+   memset(>ipv4.addr, 0, 
sizeof(key->ipv4.addr));
+   } else {
+   memset(>ipv6.addr, 0, 
sizeof(key->ipv6.addr));
+   }
+   /* fall-through */
+   case -EPROTO:
skb->transport_header = skb->network_header;
error = 0;
+   break;
+   default:
+   error = nh_len;
}
return error;
}
 
-   nh = ip_hdr(skb);
-   key->ipv4.addr.src = nh->saddr;
-   key->ipv4.addr.dst = nh->daddr;
-
-   key->ip.proto = nh->protocol;
-   key->ip.tos = nh->tos;
-   key->ip.ttl = nh->ttl;
-
-   offset = nh->frag_off & htons(IP_OFFSET);
-   if (offset) {
-   key->ip.frag = OVS_FRAG_TYPE_LATER;
+   if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
return 0;
}
-   if (nh->frag_off & htons(IP_MF) ||
-   skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
-   key->ip.frag = OVS_FRAG_TYPE_FIRST;
-   else
-   key->ip.frag = OVS_FRAG_TYPE_NONE;
 
/* 

Re: [ovs-dev] [PATCH v5] [RFC] ovn-controller: add quiet mode

[Han Zhou]

On Thu, Apr 20, 2017 at 3:14 PM, Han Zhou  wrote:
>
>
>
>
> >
> > [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html
>
> This link is old, should be:
https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/078272.html
>
>
> > Notes:
> > v4->v5: Based on the old patch from Ryan Moats and rebased to
current
> > master. Current version is RFC only because a problem is found in
> > testing. Sometimes latest SB DB change is not seen by
ovn-controller:
> > cur_seqno in ovn-controller stays behind the real seqno seen by
> > ovsdb-tool,
>
> Please ignore this ovsdb-tool part, which is misleading. ovsdb-tool would
never see seqno of an idl :o)
> The problem seems to be related to ctx.ovs_idl_txn. I am still debugging.
>
The root cause of the problem is found and fixed, so I submitted formal v5:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331226.html

The root cause is mentioned in the notes.

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

[ovs-dev] [PATCH v5] ovn-controller: add quiet mode

[Han Zhou]

As discussed in [1], what the incremental processing code
actually accomplished was that the ovn-controller would
be "quiet" and not burn CPU when things weren't changing.
This patch set recreates this state by calculating whether
changes have occured that would require a full calculation
to be performed.  It does this by persisting a copy of
the localvif_to_ofport and tunnel information in the
controller module, rather than in the physical.c module
as was the case with previous commits.

Performance improved extremely in below test scenario:

- 1 lswitch with 10 lports bound locally
- Each lport has an ingress ACL, referencing the same address-set
- The address-set has 10,000 IPv4 addresses

For each IP address in the address-set, there will be 3
OpenFlow rules generated for each ACL. So the total number
of rules is 300k+.

Without the patch, it takes 50+ minutes to install all the
rules to ovs-vswitchd.

With the patch, it takes 20 seconds to install all the rules
to ovs-vswitchd.

The reason is that the large number of rules are sent to
ovs-vswitchd gradually in many iterations of ovn-controller
main loop. Without the patch, cpu cycles are wasted in
lflow_run to re-processing the large address set in every
main loop iteration. With the patch, lflow_run is not
executed in most iterations because there is no change of
input.

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/078272.html

Signed-off-by: Ryan Moats 
Signed-off-by: Han Zhou 
---

Notes:
v4->v5:
- Based on the old patch from Ryan Moats and rebased to current master.
- Fixed a problem: when there are large number of flows being installed
  to OVS, changes made at the same time to SB DB (e.g. lflows) or
  physical mappings won't get processed until there are new changes to
  trigger processing, which may not happen at all. The root cause is
  ofctrl_put can abort temporarily if there are in-flight messages,
  which causes last round of flow updates got lost.

 ovn/controller/ofctrl.c |  24 +++---
 ovn/controller/ofctrl.h |   1 +
 ovn/controller/ovn-controller.c |  63 ++---
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c  |   2 +
 ovn/controller/physical.c   | 102 +---
 ovn/controller/physical.h   |   5 ++
 7 files changed, 137 insertions(+), 61 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 10c8105..5c24771 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -383,6 +383,8 @@ run_S_CLEAR_FLOWS(void)
 queue_msg(encode_group_mod());
 ofputil_uninit_group_mod();
 
+force_full_process();
+
 /* Clear existing groups, to match the state of the switch. */
 if (groups) {
 ovn_group_table_clear(groups, true);
@@ -813,6 +815,20 @@ add_ct_flush_zone(uint16_t zone_id, struct ovs_list *msgs)
 ovs_list_push_back(msgs, >list_node);
 }
 
+/* The flow table can be updated if the connection to the switch is up and
+ * in the correct state and not backlogged with existing flow_mods.  (Our
+ * criteria for being backlogged appear very conservative, but the socket
+ * between ovn-controller and OVS provides some buffering.) */
+bool
+ofctrl_can_put(void)
+{
+if (state != S_UPDATE_FLOWS
+|| rconn_packet_counter_n_packets(tx_counter)) {
+return false;
+}
+return true;
+}
+
 /* Replaces the flow table on the switch, if possible, by the flows added
  * with ofctrl_add_flow().
  *
@@ -831,14 +847,10 @@ void
 ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
int64_t nb_cfg)
 {
-/* The flow table can be updated if the connection to the switch is up and
- * in the correct state and not backlogged with existing flow_mods.  (Our
- * criteria for being backlogged appear very conservative, but the socket
- * between ovn-controller and OVS provides some buffering.) */
-if (state != S_UPDATE_FLOWS
-|| rconn_packet_counter_n_packets(tx_counter)) {
+if (!ofctrl_can_put()) {
 ovn_flow_table_clear(flow_table);
 ovn_group_table_clear(groups, false);
+force_full_process();
 return;
 }
 
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index bf5ba01..d83f6ae 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -34,6 +34,7 @@ struct shash;
 void ofctrl_init(struct group_table *group_table);
 enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int,
 struct shash *pending_ct_zones);
+bool ofctrl_can_put(void);
 void ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
 int64_t nb_cfg);
 void ofctrl_wait(void);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index e00f57a..0a75f65 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@