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

[Shashank Ram]

Hi Anand, thanks for looking into the comments. Few comments,


1. Could you please point me to the code in this patch that uses an event to 
wake up the cleaner thread? I looked at OvsIpFragmentEntryDelete() but didn't 
see it there.

2. I don't see spin locks being used instead of RW locks. Like I mentioned in a 
previous review, spin locks are more efficient if all you want is a lock. Also, 
using RW lock without differentiating between Read and Write makes it confusing 
to read the code, and understand why a RW lock was used in the first place.

3. I still feel like this patch is huge to review. You should probably break it 
into smaller patches.


Thanks,
Shashank




From: ovs-dev-boun...@openvswitch.org  on 
behalf of Anand Kumar 
Sent: Friday, January 27, 2017 2:13:26 PM
To: d...@openvswitch.org
Subject: [ovs-dev] [PATCH v3 1/5] datapath-windows: Added a new file to support 
Ipv4 fragments.

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.

v2->v3:
- Use spinlock instead of RW lock
- Trigger cleanup event after reassembling the fragments.
v1->v2: No change

Signed-off-by: Anand Kumar 
---
 datapath-windows/automake.mk   |   2 +
 datapath-windows/ovsext/Debug.h|   3 +-
 datapath-windows/ovsext/IpFragment.c   | 503 +
 datapath-windows/ovsext/IpFragment.h   |  74 +
 datapath-windows/ovsext/Switch.c   |   9 +
 datapath-windows/ovsext/ovsext.vcxproj |   2 +
 6 files changed, 592 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..95fc754
--- /dev/null
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -0,0 +1,503 @@
+/*
+ * 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:
+ *
+ * 
https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=6OuVHk-mnufSWzkKa74UkQ&m=KjW6rnug5W4uvuHLcHDZj3S59ezP3qBmXZsV-V_9qYU&s=AcpoBm0iQKFL_j_fMQGWhP-BbRf205cdg1lcWbA1xQE&e=
+ *
+ * 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
+OvsInitIpFr

Re: [ovs-dev] [patch_v4 5/6] Enable NAT tests for userspace datapath.

[Daniele Di Proietto]

2017-01-24 20:40 GMT-08:00 Darrell Ball :
> Signed-off-by: Darrell Ball 

Thanks for the patch.

The macro is now empty for kernel and userspace.

Would you mind removing it from system-userspace-macros,
system-kmod-macros and from the testsuites?

Also some of the NAT tests fail for me, I guess because they use ALGs:

 57: conntrack - FTP NAT prerecirc   FAILED
(system-traffic.at:2693)
 58: conntrack - FTP NAT prerecirc seqadjFAILED
(system-traffic.at:2704)
 59: conntrack - FTP NAT postrecirc  FAILED
(system-traffic.at:2756)
 60: conntrack - FTP NAT postrecirc seqadj   FAILED
(system-traffic.at:2767)

 62: conntrack - IPv6 FTP with NAT   FAILED
(system-traffic.at:2859)

I think adding CHECK_CONNTRACK_ALG() to them should skip them for the
userspace datapath.


> ---
>  tests/system-userspace-macros.at | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/tests/system-userspace-macros.at 
> b/tests/system-userspace-macros.at
> index 631f71a..6e3d468 100644
> --- a/tests/system-userspace-macros.at
> +++ b/tests/system-userspace-macros.at
> @@ -99,9 +99,6 @@ m4_define([CHECK_CONNTRACK_LOCAL_STACK],
>  # CHECK_CONNTRACK_NAT()
>  #
>  # Perform requirements checks for running conntrack NAT tests. The userspace
> -# doesn't support NATs yet, so skip the tests
> +# datapath supports NAT.
>  #
> -m4_define([CHECK_CONNTRACK_NAT],
> -[
> -AT_SKIP_IF([:])
> -])
> +m4_define([CHECK_CONNTRACK_NAT])
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v4 4/6] Unset CS_NEW for established connections.

[Daniele Di Proietto]

2017-01-24 20:40 GMT-08:00 Darrell Ball :
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 34728a6..aaecb00 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -443,6 +443,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet 
> *pkt,
>  switch (res) {
>  case CT_UPDATE_VALID:
>  *state |= CS_ESTABLISHED;
> +*state &= ~CS_NEW;

Maybe I'm missing something, but can *state be !=0 at this point?

>  if (ctx->reply) {
>  *state |= CS_REPLY_DIR;
>  }
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v4 3/6] Userspace Datapath: Introduce NAT support.

[Daniele Di Proietto]

2017-01-24 20:40 GMT-08:00 Darrell Ball :
> This patch introduces NAT support for the userspace datapath.
> The conntrack module changes are in this patch.
>
> The per packet scope of lookups for NAT and un_NAT is at
> the bucket level rather than global. One hash table is
> introduced to support create/delete handling. The create/delete
> events may be further optimized, if the need becomes clear.
>
> Some NAT options with limited utility (persistent, random) are
> not supported yet, but will be supported in a later patch.
>
> Signed-off-by: Darrell Ball 

Sparse reports some problems:

../lib/conntrack.c:1375:16: warning: constant 0x is so
big it is unsigned long
../lib/conntrack.c:1398:9: warning: constant 0x is so
big it is unsigned long
../lib/conntrack.c:1400:36: warning: constant 0x is so
big it is unsigned long
../lib/conntrack.c:1403:33: warning: constant 0x is so
big it is unsigned long
../lib/conntrack.c:214:30: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:240:30: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1360:52: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1362:52: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1365:52: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1367:52: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1395:44: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1396:44: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1409:25: error: no member 's6_addr32' in struct in6_addr
../lib/conntrack.c:1410:25: error: no member 's6_addr32' in struct in6_addr

There are some minor coding style problems, you can find them with
utilities/checkpatch.py

> ---
>  lib/conntrack-private.h |  16 +-
>  lib/conntrack.c | 740 
> ++--
>  lib/conntrack.h |  44 +++
>  3 files changed, 717 insertions(+), 83 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 493865f..b71af37 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -51,14 +51,23 @@ struct conn_key {
>  uint16_t zone;
>  };
>
> +struct nat_conn_key_node {
> +struct hmap_node node;
> +struct conn_key key;
> +struct conn_key value;
> +};
> +
>  struct conn {
>  struct conn_key key;
>  struct conn_key rev_key;
>  long long expiration;
>  struct ovs_list exp_node;
>  struct hmap_node node;
> -uint32_t mark;
>  ovs_u128 label;
> +/* XXX: consider flattening. */
> +struct nat_action_info_t *nat_info;
> +uint32_t mark;
> +uint8_t conn_type;
>  };
>
>  enum ct_update_res {
> @@ -67,6 +76,11 @@ enum ct_update_res {
>  CT_UPDATE_NEW,
>  };
>
> +enum ct_conn_type {
> +CT_CONN_TYPE_DEFAULT,
> +   CT_CONN_TYPE_UN_NAT,
> +};
> +
>  struct ct_l4_proto {
>  struct conn *(*new_conn)(struct conntrack_bucket *, struct dp_packet 
> *pkt,
>   long long now);
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 0a611a2..34728a6 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -76,6 +76,20 @@ static void set_label(struct dp_packet *, struct conn *,
>const struct ovs_key_ct_labels *mask);
>  static void *clean_thread_main(void *f_);
>
> +static struct nat_conn_key_node *
> +nat_conn_keys_lookup(struct hmap *nat_conn_keys,
> + const struct conn_key *key,
> + uint32_t basis);
> +
> +static void
> +nat_conn_keys_remove(struct hmap *nat_conn_keys,
> +const struct conn_key *key,
> +uint32_t basis);
> +
> +static bool
> +nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
> +  struct conn *nat_conn);
> +
>  static struct ct_l4_proto *l4_protos[] = {
>  [IPPROTO_TCP] = &ct_proto_tcp,
>  [IPPROTO_UDP] = &ct_proto_other,
> @@ -90,9 +104,11 @@ long long ct_timeout_val[] = {
>  };
>
>  /* If the total number of connections goes above this value, no new 
> connections
> - * are accepted */
> + * are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
>  #define DEFAULT_N_CONN_LIMIT 300
>
> +#define DT
> +

I guess this is left here from debugging

>  /* Initializes the connection tracker 'ct'.  The caller is responsible for
>   * calling 'conntrack_destroy()', when the instance is not needed anymore */
>  void
> @@ -101,6 +117,11 @@ conntrack_init(struct conntrack *ct)
>  unsigned i, j;
>  long long now = time_msec();
>
> +ct_rwlock_init(&ct->nat_resources_lock);
> +ct_rwlock_wrlock(&ct->nat_resources_lock);
> +hmap_init(&ct->nat_conn_keys);
> +ct_rwlock_unlock(&ct->nat_resources_lock);
> +
>  for (i = 0; i < CONNTRACK_BUCKETS; i++) {
>  struct conntrack_bucket *ctb = &ct->buckets[i];
>
> @@ -139,13 +160,24 @@ conntr

Re: [ovs-dev] [patch_v4 2/6] Parse NAT netlink for userspace datapath.

[Daniele Di Proietto]

2017-01-24 10:50 GMT-08:00 Darrell Ball :
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack-private.h |  9 --
>  lib/conntrack.c |  3 +-
>  lib/conntrack.h | 31 +-
>  lib/dpif-netdev.c   | 85 
> ++---
>  tests/test-conntrack.c  |  8 +++--
>  5 files changed, 118 insertions(+), 18 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 013f19f..493865f 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -29,15 +29,6 @@
>  #include "packets.h"
>  #include "unaligned.h"
>
> -struct ct_addr {
> -union {
> -ovs_16aligned_be32 ipv4;
> -union ovs_16aligned_in6_addr ipv6;
> -ovs_be32 ipv4_aligned;
> -struct in6_addr ipv6_aligned;
> -};
> -};
> -
>  struct ct_endpoint {
>  struct ct_addr addr;
>  union {
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 9bea3d9..bae42a3 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -273,7 +273,8 @@ conntrack_execute(struct conntrack *ct, struct 
> dp_packet_batch *pkt_batch,
>ovs_be16 dl_type, bool commit, uint16_t zone,
>const uint32_t *setmark,
>const struct ovs_key_ct_labels *setlabel,
> -  const char *helper)
> +  const char *helper,
> + const struct nat_action_info_t 
> *nat_action_info OVS_UNUSED)
>  {
>  struct dp_packet **pkts = pkt_batch->packets;
>  size_t cnt = pkt_batch->count;
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 254f61c..cbdfb91 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -26,6 +26,8 @@
>  #include "openvswitch/thread.h"
>  #include "openvswitch/types.h"
>  #include "ovs-atomic.h"
> +#include "ovs-thread.h"
> +#include "packets.h"
>
>  /* Userspace connection tracker
>   * 
> @@ -61,6 +63,32 @@ struct dp_packet_batch;
>
>  struct conntrack;
>
> +struct ct_addr {
> +union {
> +ovs_16aligned_be32 ipv4;
> +union ovs_16aligned_in6_addr ipv6;
> +ovs_be32 ipv4_aligned;
> +struct in6_addr ipv6_aligned;
> +};
> +};
> +
> +// Both NAT_ACTION_* and NAT_ACTION_*_PORT can be set

We normally don't use // comments

> +enum nat_action_e {
> +   NAT_ACTION = 1 << 0,
> +   NAT_ACTION_SRC = 1 << 1,
> +   NAT_ACTION_SRC_PORT = 1 << 2,
> +   NAT_ACTION_DST = 1 << 3,
> +   NAT_ACTION_DST_PORT = 1 << 4,
> +};

This is indented by tabs, instead of 4 whitespaces.

Is NAT_ACTION really necessary?  I think it should always be set when
nat_action_info is != NULL, so we can probably remove it.

> +
> +struct nat_action_info_t {
> +   struct ct_addr min_addr;
> +   struct ct_addr max_addr;
> +   uint16_t min_port;
> +   uint16_t max_port;

Tabs

> +uint16_t nat_action;
> +};
> +
>  void conntrack_init(struct conntrack *);
>  void conntrack_destroy(struct conntrack *);
>
> @@ -68,7 +96,8 @@ int conntrack_execute(struct conntrack *, struct 
> dp_packet_batch *,
>ovs_be16 dl_type, bool commit,
>uint16_t zone, const uint32_t *setmark,
>const struct ovs_key_ct_labels *setlabel,
> -  const char *helper);
> +  const char *helper,
> +  const struct nat_action_info_t *nat_action_info);
>
>  struct conntrack_dump {
>  struct conntrack *ct;
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 3901129..a71c766 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -97,7 +97,8 @@ static struct shash dp_netdevs 
> OVS_GUARDED_BY(dp_netdev_mutex)
>  static struct vlog_rate_limit upcall_rl = VLOG_RATE_LIMIT_INIT(600, 600);
>
>  #define DP_NETDEV_CS_SUPPORTED_MASK (CS_NEW | CS_ESTABLISHED | CS_RELATED \
> - | CS_INVALID | CS_REPLY_DIR | 
> CS_TRACKED)
> + | CS_INVALID | CS_REPLY_DIR | 
> CS_TRACKED \
> + | CS_SRC_NAT | CS_DST_NAT)
>  #define DP_NETDEV_CS_UNSUPPORTED_MASK 
> (~(uint32_t)DP_NETDEV_CS_SUPPORTED_MASK)
>
>  static struct odp_support dp_netdev_support = {
> @@ -4681,7 +4682,9 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
> *packets_,
>  const char *helper = NULL;
>  const uint32_t *setmark = NULL;
>  const struct ovs_key_ct_labels *setlabel = NULL;
> -
> +struct nat_action_info_t nat_action_info;
> +bool nat = false;
> +memset(&nat_action_info, 0, sizeof nat_action_info);

As discussed offline, can this memset  be moved inside the OVS_CT_ATTR_NAT case?

>  NL_ATTR_FOR_EACH_UNSAFE (b, left, nl_attr_get(a),
>   nl_attr_get_size(a)) {
>  enum ovs_ct_attr sub_type = nl_attr_type(b);
> @@ -4702,15 +4705,89 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
> *pack

Re: [ovs-dev] [patch_v4 1/6] Export packet_set_ipv6_addr()fordpdkdatapath.

[Daniele Di Proietto]

2017-01-24 20:40 GMT-08:00 Darrell Ball :
> Signed-off-by: Darrell Ball 

LGTM, thanks

the commit message is missing a few whitespaces.

> ---
>  lib/packets.c | 2 +-
>  lib/packets.h | 4 
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/packets.c b/lib/packets.c
> index fa70df6..94e7d87 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -986,7 +986,7 @@ packet_update_csum128(struct dp_packet *packet, uint8_t 
> proto,
>  }
>  }
>
> -static void
> +void
>  packet_set_ipv6_addr(struct dp_packet *packet, uint8_t proto,
>   ovs_16aligned_be32 addr[4],
>   const struct in6_addr *new_addr,
> diff --git a/lib/packets.h b/lib/packets.h
> index c4d3799..850f192 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1100,6 +1100,10 @@ void packet_set_ipv4_addr(struct dp_packet *packet, 
> ovs_16aligned_be32 *addr,
>  void packet_set_ipv6(struct dp_packet *, const struct in6_addr *src,
>   const struct in6_addr *dst, uint8_t tc,
>   ovs_be32 fl, uint8_t hlmit);
> +void packet_set_ipv6_addr(struct dp_packet *packet, uint8_t proto,
> +  ovs_16aligned_be32 addr[4],
> +  const struct in6_addr *new_addr,
> +  bool recalculate_csum);
>  void packet_set_tcp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
>  void packet_set_udp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
>  void packet_set_sctp_port(struct dp_packet *, ovs_be16 src, ovs_be16 dst);
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v4 0/6] Userspace Datapath: Introduce NAT support.

[Daniele Di Proietto]

2017-01-24 20:40 GMT-08:00 Darrell Ball :
> This patch series introduces NAT support for the userspace datapath.
>
> The per packet scope of lookups for NAT and un_NAT is at
> the bucket level rather than global. One hash table is
> introduced to support create/delete handling. The create/delete
> events may be further optimized, if the need becomes clear.
>
> The existing NAT tests are enabled for the dpdk datapath,
> with an added enhancement to the V6 NAT test.
>
> Some NAT options with limited utility (persistent, random) are
> not supported yet, but will be supported in a later patch.
>
> One V6 api is exported to facilitate selective editing the V6
> header - packet_set_ipv6_addr().
>
> alg and fragmentation support are not included here but are
> being worked on.
>
> NEWS is not updated in this series yet, until confirmation of
> release.
>
> I realize patch 3 is big. It may be clearer and easier to keep
> as a single patch, so I have done that after some discussion.

Thanks a lot for the series.  All the NAT and OVN system tests
are passing, which is great!

You can include an update to NEWS, it won't be pushed before the
rest of the series :-)

Usually we prefix the commit message with the name of the module
that the commit touches.

More comments in the various commits

I'm sorry I don't have more meaningful comments, yet.  I'll keep looking
at the series

Thanks,

Daniele


>
> v3->v4: Fix rev_key vs key for nat_conn_keys access in a couple
> places; this would have affected cleanup; at same time
> rename some variables and change nat_conn_keys APIs to
> use conn key, rather than conn.
>
> Fix conntrack_flush() CT_CONN_TYPE_DEFAULT flag placement;
> the intention was that it be the same as in sweep_bucket().
>
> Fix nat_ipv6_addrs_delta() max boundary checking logic. I
> also enhanced the conntrack - IPv6 HTTP with NAT test to
> give it more coverage as partial penance.
>
> Rebase
>
> v2->v3: Fix a theoretical resend for closed connection restart.
> Parse out a function to help and also limit
> conn_state_update() to one.
>
> I decided to cap V6 address range delta at 4 billion using
> internal adjustment (user visibility not required).
>
> Some cleanup of deprecated code path.
>
> Parse out some more changes as separate patches.
>
> v1->v2: Updates/fixes that were missed in v1 patches.
>
> Darrell Ball (6):
>   Export packet_set_ipv6_addr()fordpdkdatapath.
>   Parse NAT netlink for userspace datapath.
>   Userspace Datapath: Introduce NAT support.
>   Unset CS_NEW for established connections.
>   Enable NAT tests for userspace datapath.
>   Enhance V6 NAT test.
>
>  lib/conntrack-private.h  |  25 +-
>  lib/conntrack.c  | 742 
> ++-
>  lib/conntrack.h  |  73 +++-
>  lib/dpif-netdev.c|  85 -
>  lib/packets.c|   2 +-
>  lib/packets.h|   4 +
>  tests/system-traffic.at  |   4 +-
>  tests/system-userspace-macros.at |   7 +-
>  tests/test-conntrack.c   |   8 +-
>  9 files changed, 843 insertions(+), 107 deletions(-)
>
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpif-netdev: Conditional EMC insert

[Daniele Di Proietto]

2017-01-26 9:51 GMT-08:00 Ciara Loftus :
> Unconditional insertion of EMC entries results in EMC thrashing at high
> numbers of parallel flows. When this occurs, the performance of the EMC
> often falls below that of the dpcls classifier, rendering the EMC
> practically useless.
>
> Instead of unconditionally inserting entries into the EMC when a miss
> occurs, use a 1% probability of insertion. This ensures that the most
> frequent flows have the highest chance of creating an entry in the EMC,
> and the probability of thrashing the EMC is also greatly reduced.
>
> The probability of insertion is configurable, via the
> other_config:emc-insert-prob option. For example the following command
> increases the insertion probability to 1/10 ie. 10%.
>
> ovs-vsctl set Open_vSwitch . other_config:emc-insert-prob=10
>
> Signed-off-by: Ciara Loftus 
> Signed-off-by: Georg Schmuecking 
> Co-authored-by: Georg Schmuecking 

Thanks for v2

I think the patch doesn't compile without DPDK.  Also there's no way to control
the value without DPDK.

I think we could pass down the value like we do for pmd-cpu-mask, this would
make it work even without DPDK.  I sent a patch that extends what we do for
pmd-cpu-mask:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/328161.html

Can we avoid having to restart the daemon when we want to change this?

I think we should store the probability in 'struct dp_netdev' using an atomic
uint32.  We can read and write to it using atomic relaxed operation which
have no additional cost, like we do for 'enable_megaflows' in
ofproto-dpif-upcall.c

If you want to store it in pmd without atomics, like Jan suggested, I
think we can
use reconfiguration to change it at runtime.

Thanks,

Daniele

> ---
> v2:
> - Enable probability configurability via other_config:emc-insert-prob
>   option.
>
>  Documentation/howto/dpdk.rst | 23 +++
>  NEWS |  2 ++
>  lib/dpdk.c   | 15 +++
>  lib/dpdk.h   |  1 +
>  lib/dpif-netdev.c| 28 ++--
>  vswitchd/vswitch.xml | 17 +
>  6 files changed, 84 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
> index d1e6e89..a37b9d5 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -354,6 +354,29 @@ the `DPDK documentation
>
>  Note: Not all DPDK virtual PMD drivers have been tested and verified to work.
>
> +EMC Insertion Probability
> +-
> +By default 1 in every 100 flows are inserted into the Exact Match Cache 
> (EMC).
> +It is possible to change this insertion probability by setting the
> +``emc-insert-prob`` option::
> +
> +$ ovs-vsctl --no-wait set Open_vSwitch . other_config:emc-insert-prob=N
> +
> +where:
> +
> +``N``
> +  is a positive integer between 0 and 4294967295.
> +
> +If ``N`` is set to 1, an insertion will be performed for every flow. The 
> lower
> +the value of ``emc-insert-prob`` the higher the probability of insertion,
> +except for the value 0 which will result in no insertions being performed and
> +thus essentially disabling the EMC.
> +
> +If ``emc-insert-prob`` is modified, the daemon needs to be restarted in order
> +for the changes to take effect.
> +
> +For more information on the EMC refer to :doc:`/intro/install/dpdk` .
> +
>  .. _dpdk-ovs-in-guest:
>
>  OVS with DPDK Inside VMs
> diff --git a/NEWS b/NEWS
> index 0a9551c..8fb1f53 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -63,6 +63,8 @@ Post-v2.6.0
> device will not be available for use until a valid dpdk-devargs is
> specified.
>   * Virtual DPDK Poll Mode Driver (vdev PMD) support.
> + * New 'other_config:emc-insert-prob' field for userspace netdevs that
> +   allows definition of the EMC insertion probability.
> - Fedora packaging:
>   * A package upgrade does not automatically restart OVS service.
> - ovs-vswitchd/ovs-vsctl:
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 9ae2491..bb9e758 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -38,6 +38,8 @@ VLOG_DEFINE_THIS_MODULE(dpdk);
>
>  static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
>
> +static uint32_t emc_insert_min = UINT32_MAX / 100;
> +
>  static int
>  process_vhost_flags(char *flag, char *default_val, int size,
>  const struct smap *ovs_other_config,
> @@ -272,6 +274,7 @@ dpdk_init__(const struct smap *ovs_other_config)
>  int err = 0;
>  cpu_set_t cpuset;
>  char *sock_dir_subcomponent;
> +int insert_prob;
>
>  if (process_vhost_flags("vhost-sock-dir", xstrdup(ovs_rundir()),
>  NAME_MAX, ovs_other_config,
> @@ -297,6 +300,12 @@ dpdk_init__(const struct smap *ovs_other_config)
>  vhost_sock_dir = sock_dir_subcomponent;
>  }
>
> +/* Configure EMC insertion probability */
> +insert_prob = smap_get_int(ovs_other_config

[ovs-dev] [PATCH] dpif-netdev: Pass Openvswitch other_config smap to dpif.

[Daniele Di Proietto]

Currently we parse the 'other_config' column in Openvswitch table in
bridge.c.  We extract the values (just 'pmd-cpu-mask' for now) and we
pass them down to the datapath, via different layers.

If we want to pass other values to dpif-netdev.c (like we recently
discussed) we would have to touch ofproto.c, ofproto-dpif.c and dpif.c.

This patch sends the entire other_config column to dpif-netdev, so that
dpif-netdev can extract the values it's interested in.

No functional change.

Signed-off-by: Daniele Di Proietto 
---
I don't like that dpif-netdev receives the whole other_config column,
because it contains other values which are completely unrelated, but
unfortunately there's no better place in the database for datapath
specific configuration.
---
 lib/dpif-netdev.c  |  9 +
 lib/dpif-netlink.c |  2 +-
 lib/dpif-provider.h|  8 +++-
 lib/dpif.c | 12 ++--
 lib/dpif.h |  2 +-
 ofproto/ofproto-dpif.c | 19 ---
 ofproto/ofproto-provider.h | 11 ---
 ofproto/ofproto.c  | 13 +
 ofproto/ofproto.h  |  3 ++-
 vswitchd/bridge.c  | 18 +-
 10 files changed, 68 insertions(+), 29 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 719a51823..0be5db514 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2724,12 +2724,13 @@ dpif_netdev_operate(struct dpif *dpif, struct dpif_op 
**ops, size_t n_ops)
 }
 }
 
-/* Changes the number or the affinity of pmd threads.  The changes are actually
- * applied in dpif_netdev_run(). */
+/* Applies datapath configuration from the database. Some of the changes are
+ * actually applied in dpif_netdev_run(). */
 static int
-dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask)
+dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config)
 {
 struct dp_netdev *dp = get_dp_netdev(dpif);
+const char *cmask = smap_get(other_config, "pmd-cpu-mask");
 
 if (!nullable_string_is_equal(dp->pmd_cmask, cmask)) {
 free(dp->pmd_cmask);
@@ -4844,7 +4845,7 @@ const struct dpif_class dpif_netdev_class = {
 dpif_netdev_operate,
 NULL,   /* recv_set */
 NULL,   /* handlers_set */
-dpif_netdev_pmd_set,
+dpif_netdev_set_config,
 dpif_netdev_queue_to_priority,
 NULL,   /* recv */
 NULL,   /* recv_wait */
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index c8b0e37f9..9762a87be 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2387,7 +2387,7 @@ const struct dpif_class dpif_netlink_class = {
 dpif_netlink_operate,
 dpif_netlink_recv_set,
 dpif_netlink_handlers_set,
-NULL,   /* poll_thread_set */
+NULL,   /* set_config */
 dpif_netlink_queue_to_priority,
 dpif_netlink_recv,
 dpif_netlink_recv_wait,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index d3b2bb91d..a0dc1ef35 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -326,11 +326,9 @@ struct dpif_class {
  * */
 int (*handlers_set)(struct dpif *dpif, uint32_t n_handlers);
 
-/* If 'dpif' creates its own I/O polling threads, refreshes poll threads
- * configuration.  'cmask' configures the cpu mask for setting the polling
- * threads' cpu affinity.  The implementation might postpone applying the
- * changes until run() is called. */
-int (*poll_threads_set)(struct dpif *dpif, const char *cmask);
+/* Pass custom configuration options to the datapath.  The implementation
+ * might postpone applying the changes until run() is called. */
+int (*set_config)(struct dpif *dpif, const struct smap *other_config);
 
 /* Translates OpenFlow queue ID 'queue_id' (in host byte order) into a
  * priority value used for setting packet priority. */
diff --git a/lib/dpif.c b/lib/dpif.c
index 374f013ab..57aa3c6c4 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1440,17 +1440,17 @@ dpif_print_packet(struct dpif *dpif, struct dpif_upcall 
*upcall)
 }
 }
 
-/* If 'dpif' creates its own I/O polling threads, refreshes poll threads
- * configuration. */
+/* Pass custom configuration to the datapath implementation.  Some of the
+ * changes can be postponed until dpif_run() is called. */
 int
-dpif_poll_threads_set(struct dpif *dpif, const char *cmask)
+dpif_set_config(struct dpif *dpif, const struct smap *cfg)
 {
 int error = 0;
 
-if (dpif->dpif_class->poll_threads_set) {
-error = dpif->dpif_class->poll_threads_set(dpif, cmask);
+if (dpif->dpif_class->set_config) {
+error = dpif->dpif_class->set_config(dpif, cfg);
 if (error) {
-log_operation(dpif, "poll_threads_set", error);
+log_operation(dpif, "set_config", error);
 }
 }
 
diff --git a/lib/dpif.h b/lib/dpif.h
index aa4fb8b84..d5b4b5827 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -8

Re: [ovs-dev] assertion failing in commit_set_ipv4_action(), flow->nw_proto != base_flow->nw_proto

[Jarno Rajahalme]

> On Jan 27, 2017, at 1:01 PM, Thomas Morin  wrote:
> 
> Jarno,
> 
> 2017-01-27, Jarno Rajahalme:
>> Cleanest solution would be to use the new clone action for the datapath, 
>> which, if important, we could also backport to OVS 2.5.x.
> 
> Yes, clone is indeed much more elegant.
> 
>> 
>> Datapath CLONE is yet to be upstreamed, though, may take a little time.
> 
> Even before net-dev upstreaming, there doesn't seem to even be a clone 
> implementation in ovs/datapath yet, right ?
> 

Right. The process for new features is to first upstream to net-next, then 
backport to ovs/datapath.

  Jarno

> -Thomas
> 
> 
> 
> 
>>> 2017-01-06, Jarno Rajahalme:
 I sent a patch to do this fix on OVS master. IMO we should also make the 
 datapath more flexible so that it would be able to POP back to IP after 
 PUSH actions on an IP packet in the same action list. But that will not 
 make it to OVS 2.7.
 
 I would appreciate if Thomas could apply the and test!
 
   Jarno
 
> On Dec 14, 2016, at 1:52 PM, Jarno Rajahalme  > wrote:
> 
>> 
>> On Dec 13, 2016, at 8:44 PM, Takashi YAMAMOTO > > wrote:
>> 
>> 
>> 
>> On Wed, Dec 14, 2016 at 9:02 AM, Jarno Rajahalme > > wrote:
>> 
>>> On Dec 13, 2016, at 3:32 PM, Takashi YAMAMOTO >> > wrote:
>>> 
>>> 
>>> 
>>> On Tue, Dec 13, 2016 at 6:49 PM, Thomas Morin >> > wrote:
>>> Hi Jarno,
>>> 
>>> 2016-12-10, Jarno Rajahalme:
 On Dec 9, 2016, at 7:04 AM, Thomas Morin >>> > wrote:
> 
> 2016-12-09, Thomas Morin:
>> In the same setup as the one on which the bug was observed, [...]
> 
> I was confused, I in fact tested the patch (branch-2.5) on a 
> /different/ setup as the one on which we hit the bug, using MPLS over 
> a GRE tunnel port, rather than plain MPLS over an eth port.
> Sorry if any confusion arised... I can test on the first setup if 
> relevant.
> 
 
 Maybe the kernel datapath does not support MPLS over a GRE tunnel 
 port. Having ‘dmesg’ output for the test run might reveal why the 
 actions validation fails.
>>> 
>>> The dmesg output was the following: 
>>> 
>>> [171295.258939] openvswitch: netlink: Flow actions may not be safe on 
>>> all matching packets.
>>> 
>>> I've tested the patch on the platform on which the bug was initially 
>>> hit (*not* using MPLS/GRE), and I have the following a few times in the 
>>> logs right after I do an "ovs-appctl fdb/flush":
>>> 
>>> 2016-12-13T09:44:08.449Z|1|dpif(handler68)|WARN|Dropped 3 log 
>>> messages in last 1 seconds (most recently, 1 seconds ago) due to 
>>> excessive rate
>>> 2016-12-13T09:44:08.449Z|2|dpif(handler68)|WARN|system@ovs-system: 
>>> failed to put[create] (Invalid argument) 
>>> ufid:f046c4c4-b97f-436d-bd7c-91ed307275ac 
>>> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(9),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64),eth_type(0x0800),ipv4(src=10.0.1.29,dst=10.0.0.3,proto=6,tos=0/0xfc,ttl=64,frag=no),tcp(src=54253,dst=8080),tcp_flags(0/0),
>>>  
>>> actions:set(ipv4(src=10.0.1.29,dst=10.0.0.3,ttl=63)),set(eth(src=b8:2a:72:de:1b:e3,dst=00:17:cb:79:2c:01)),push_mpls(label=433680,tc=0,ttl=63,bos=1,eth_type=0x8847),7,set(eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64)),pop_mpls(eth_type=0x800),set(ipv4(src=10.0.1.29,dst=10.0.0.3,tos=0/0xfc,ttl=64)),push_vlan(vid=1,pcp=0),3,8,pop_vlan,13
>>> 
>>> And dmesg:
>>> [926833.612372] openvswitch: netlink: Flow actions may not be safe on 
>>> all matching packets.
>>> 
>>> it's complaining about set ipv4 after pop_mpls because it doesn't know 
>>> about the "restored" l3.
>>> while we can improve the kernel, i guess we need to stick with recirc 
>>> for now.
>>>  
>> 
>> This should do it? Does not break any existing tests on branch-2.5, but 
>> I did not create a test for this yet.
>> 
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index fb25f63..6ee2a72 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2899,6 +2899,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
>>  {
>>  bool use_masked = ctx->xbridge->support.masked_set_action;
>>  
>> +/* An MPLS packet can be implicitly popped back to a non-MPLS 
>> packet, if a
>> + * patch port peer or a group bucket pushed MPLS.  Set the 
>> 'was_mpls' flag
>> + * also in that case. */
>> +if (eth_type_mpls(ctx->base_flow.dl_type)
>> +&& !eth_type_mpls(ctx->xin->fl

Re: [ovs-dev] [PATCH 1/3] ovn-trace: Possible null dereference

[Guru Shetty]

On 26 January 2017 at 23:14, Alin Serdean 
wrote:

> Found by inspection.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---
>  ovn/utilities/ovn-trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 307556b..2e2859a 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -1224,7 +1224,7 @@ execute_output(const struct ovntrace_datapath *dp,
> struct flow *uflow,
>  }
>  }
>
> -if (port->tunnel_key != in_key || allow_loopback) {
> +if (port && (port->tunnel_key != in_key || allow_loopback)) {
>
The parentheses above feels odd. Did you want (port &&   port->tunnel_key
!= in_key) ||  allow_loopback) ?


>  struct ovntrace_node *node = ovntrace_node_append(
>  super, OVNTRACE_NODE_PIPELINE,
>  "egress(dp=\"%s\", inport=\"%s\", outport=\"%s\")",
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/3] tests: rstp close input file

[Guru Shetty]

On 26 January 2017 at 23:16, Alin Serdean 
wrote:

> Close the 'input_file' after data is read.
>
> Signed-off-by: Alin Gabriel Serdean 
>
Applied.


> ---
>  tests/test-rstp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/test-rstp.c b/tests/test-rstp.c
> index 2ee8c7e..7bcff83 100644
> --- a/tests/test-rstp.c
> +++ b/tests/test-rstp.c
> @@ -691,6 +691,7 @@ test_rstp_main(int argc, char *argv[])
>  }
>  }
>  free(token);
> +fclose(input_file);
>
>  for (i = 0; i < tc->n_lans; i++) {
>  struct lan *lan = tc->lans[i];
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/3] tests dpdk: ring_client, client_id is unsigned

[Guru Shetty]

On 26 January 2017 at 23:15, Alin Serdean 
wrote:

> 'client_id' is of type unsigned int. Change the printf specifier to match
> it.
>
> Signed-off-by: Alin Gabriel Serdean 
>
Applied.


> ---
>  tests/dpdk/ring_client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/dpdk/ring_client.c b/tests/dpdk/ring_client.c
> index 8a43c64..b153713 100644
> --- a/tests/dpdk/ring_client.c
> +++ b/tests/dpdk/ring_client.c
> @@ -176,7 +176,7 @@ main(int argc, char *argv[])
>
>  RTE_LOG(INFO, APP, "Finished Process Init.\n");
>
> -printf("\nClient process %d handling packets\n", client_id);
> +printf("\nClient process %u handling packets\n", client_id);
>  printf("[Press Ctrl-C to quit ...]\n");
>
>  for (;;) {
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Guru Shetty]

On 27 January 2017 at 14:13, Anand Kumar  wrote:

> 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.
>
> v2->v3:
> - Use spinlock instead of RW lock
> - Trigger cleanup event after reassembling the fragments.
> v1->v2: No change
>
This is not a review. But please do resubmit after a review round with the
above versioning information below the "---". Look at other emails for
examples.


>
> Signed-off-by: Anand Kumar 
> ---
>  datapath-windows/automake.mk   |   2 +
>  datapath-windows/ovsext/Debug.h|   3 +-
>  datapath-windows/ovsext/IpFragment.c   | 503
> +
>  datapath-windows/ovsext/IpFragment.h   |  74 +
>  datapath-windows/ovsext/Switch.c   |   9 +
>  datapath-windows/ovsext/ovsext.vcxproj |   2 +
>  6 files changed, 592 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..95fc754
> --- /dev/null
> +++ b/datapath-windows/ovsext/IpFragment.c
> @@ -0,0 +1,503 @@
> +/*
> + * 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_MEMORY_TAG);
> +if (OvsIpFragTable == NULL) {
> +NdisFreeRWLock(ovsIpFragmentHashLockObj);
> +ovsIpFragmentHashLockObj = NULL;
> +retu

Re: [ovs-dev] [PATCH] python windows: Allow clients to read from server before disconnect

[Guru Shetty]

On 26 January 2017 at 11:44, Alin Serdean 
wrote:

> Wait for clients to read from the pipe before disconnecting the server.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---
> Intended for master and branch-2.7
>
Applied.



> ---
>  python/ovs/stream.py | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/python/ovs/stream.py b/python/ovs/stream.py
> index 58c4925..be69534 100644
> --- a/python/ovs/stream.py
> +++ b/python/ovs/stream.py
> @@ -233,6 +233,9 @@ class Stream(object):
>  self.socket.close()
>  if self.pipe is not None:
>  if self._server:
> +# Flush the pipe to allow the client to read the pipe
> +# before disconnecting.
> +win32pipe.FlushFileBuffers(self.pipe)
>  win32pipe.DisconnectNamedPipe(self.pipe)
>  winutils.close_handle(self.pipe, vlog.warn)
>  winutils.close_handle(self._read.hEvent, vlog.warn)
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Anand Kumar]

Hi Shashank,

Thank you for reviewing the patch.  I have sent out a new version of the patch 
series addressing your comments.
Based on your suggestion, I’m using an event to signal the clean up thread to 
free up the memory.

Regards,
Anand Kumar

From: Shashank Ram 
Date: Wednesday, January 18, 2017 at 9:06 AM
To: Anand Kumar , "d...@openvswitch.org" 

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


Hi Anand, following are my comments:

1. Since you are just using a RW lock without specifically differentiating 
between and read and write protection, you could use a spin lock instead. Spin 
locks in general are recommended if all you want is a lock.

2. Instead of running the fragment cleaner thread every minute, you should also 
make use of an event to signal the thread when a fragment is removed from your 
fragment list. This way, you avoid holding unnecessary memory when it could be 
cleared. Please make use of an event to signal the thread along with a default 
timeout, in case the signal never arrives.



Thanks,

Shashank


From: ovs-dev-boun...@openvswitch.org  on 
behalf of Anand Kumar 
Sent: Thursday, January 12, 2017 1:13:42 PM
To: d...@openvswitch.org
Subject: [ovs-dev] [PATCH v2 1/5] datapath-windows: Added a new file to support 
Ipv4 fragments.

This patch adds functionalities to handle 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.
---
 datapath-windows/automake.mk   |   2 +
 datapath-windows/ovsext/Debug.h|   3 +-
 datapath-windows/ovsext/IpFragment.c   | 506 +
 datapath-windows/ovsext/IpFragment.h   |  74 +
 datapath-windows/ovsext/Switch.c   |   9 +
 datapath-windows/ovsext/ovsext.vcxproj |   2 +
 6 files changed, 595 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..2ce3932
--- /dev/null
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -0,0 +1,506 @@
+/*
+ * 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:
+ *
+ * 
https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=6OuVHk-mnufSWzkKa74UkQ&m=I5c08LjVSUqyr1NmvoFFEPPDfrSIQhDwNr4ybCJddFg&s=MqMW4vcIn0dMvg5iQsDXTsvkSW5hnJ95l3b9ZmAGdwk&e=
+ *
+ * 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 OvsIpFragmentEntryDelet

Re: [ovs-dev] [PATCH] windows: Allow clients to read from server before disconnect

[Guru Shetty]

On 26 January 2017 at 11:12, Alin Serdean 
wrote:

> Wait for clients to read from the pipe before disconnecting the server.
>
> Found while testing.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---
> Intended for master and branch-2.7
>
Applied.


> ---
>  lib/stream-windows.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/stream-windows.c b/lib/stream-windows.c
> index 637920b..1950014 100644
> --- a/lib/stream-windows.c
> +++ b/lib/stream-windows.c
> @@ -183,6 +183,9 @@ windows_close(struct stream *stream)
>  /* Disconnect the named pipe in case it was created from a passive
> stream.
>   */
>  if (s->server) {
> +/* Flush the pipe to allow the client to read the pipe's contents
> + * before disconnecting. */
> +FlushFileBuffers(s->fd);
>  DisconnectNamedPipe(s->fd);
>  }
>  CloseHandle(s->fd);
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Anand Kumar]

Hi Sairam,

Thank you for reviewing the patch series. I have address the comments and sent 
out a new version of the patch.

Regards,
Anand Kumar

On 1/23/17, 3:42 PM, "Sairam Venugopal"  wrote:

Please find the comments inline. I have prefixed them with “sai:”

Thanks,
Sairam




On 1/12/17, 1:13 PM, "ovs-dev-boun...@openvswitch.org on behalf of Anand 
Kumar"  
wrote:

>This patch adds support for Ipv4 fragments in conntrack module.
>Individual fragments are not tracked, the reassembled Ipv4 datagram is 
treated
>as a single ct entry.
>
>Added MRU field in OvsForwardingContext, to keep track of Maximum recieved 
unit
>from all the recieved IPv4 fragments.
>---
> datapath-windows/ovsext/Actions.c   | 15 +++
> datapath-windows/ovsext/Conntrack.c | 31 +--
> datapath-windows/ovsext/Conntrack.h |  7 ++-
> 3 files changed, 42 insertions(+), 11 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
>index b4a286b..f378619 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -125,6 +125,9 @@ typedef struct OvsForwardingContext {
> 
> /* header information */
> OVS_PACKET_HDR_INFO layers;
>+
>+/* Maximum Recieving Unit */
>+UINT16 mru;
> } OvsForwardingContext;
> 
> /*
>@@ -1910,11 +1913,15 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT 
switchContext,
> }
> }
> 
>-status = OvsExecuteConntrackAction(ovsFwdCtx.curNbl, layers,
>-   key, (const PNL_ATTR)a);
>+status = OvsExecuteConntrackAction(switchContext, 
&ovsFwdCtx.curNbl, &(ovsFwdCtx.mru),
>+   ovsFwdCtx.completionList,
>+   
ovsFwdCtx.fwdDetail->SourcePortId,
>+   layers, key, (const 
PNL_ATTR)a);
> if (status != NDIS_STATUS_SUCCESS) {
>-OVS_LOG_ERROR("CT Action failed");
>-dropReason = L"OVS-conntrack action failed";
>+if (status != NDIS_STATUS_PENDING) {
>+OVS_LOG_ERROR("CT Action failed");
>+dropReason = L"OVS-conntrack action failed";
saii: Can you align the  }. Also add a comment saying Pending Packets are 
currently consumed by the Defragmentation process.
>+}
> goto dropit;
> }
> break;
>diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
>index d1be480..219680e 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"
>@@ -312,13 +313,25 @@ OvsCtEntryExpired(POVS_CT_ENTRY entry)
> }
> 
> static __inline NDIS_STATUS
>-OvsDetectCtPacket(OvsFlowKey *key)
>+OvsDetectCtPacket(POVS_SWITCH_CONTEXT switchContext,
>+  PNET_BUFFER_LIST *curNbl,
>+  OvsCompletionList *completionList,
>+  NDIS_SWITCH_PORT_ID sourcePort,
>+  OvsFlowKey *key,
>+  UINT16 *mru,
>+  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(switchContext,
>+  curNbl,
>+  completionList,
>+  sourcePort,
>+  mru,
>+  key->tunKey.tunnelId,
>+  newNbl);
> }
> if (key->ipKey.nwProto == IPPROTO_TCP
> || key->ipKey.nwProto == IPPROTO_UDP
>@@ -688,7 +701,11 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
>  
*---
>  */

Sai: Update the summary to say we consume the original fragment Nbl.

> NDIS_STATUS
>-OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
>+OvsExecuteConntrackAction(POVS_SWITCH_CONTEXT switchContext,
>+  PNET_BUFFER_LIST *curNbl,
>+  UINT16 *mru,
>+  OvsCompletionList *completionL

Re: [ovs-dev] [PATCH] tests windows: change service test

[Guru Shetty]

On 26 January 2017 at 10:32, Alin Serdean 
wrote:

> Skip the test if the service 'ovsdb-server' is already defined.
>
> The arguments of the service are incomplete: in the former state
> it will try to create the pidfile and unixctl in the configuration path.
> This patch adds those arguments.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---
> Intended for master and branch-2.7
>
Applied.

> ---
>  tests/daemon.at | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/daemon.at b/tests/daemon.at
> index 454de37..952d5a7 100644
> --- a/tests/daemon.at
> +++ b/tests/daemon.at
> @@ -168,13 +168,14 @@ AT_SETUP([daemon --service])
>  AT_KEYWORDS([windows-service])
>  AT_SKIP_IF([test "$IS_WIN32" != "yes"])
>  OVS_SKIP_NON_ADMIN_WIN
> +AT_SKIP_IF([sc qc ovsdb-server])
>
>  OVSDB_INIT([db])
>  AT_CAPTURE_FILE([pid])
>  # To create a Windows service, we need the absolute path for the
> executable.
>  abs_path="$(cd $(dirname `which ovsdb-server`); pwd -W; cd $OLDPWD)"
>
> -AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server
> `pwd`/db --log-file=`pwd`/ovsdb-server.log --pidfile
> --remote=punix:`pwd`/socket --service"],
> +AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server
> `pwd`/db --log-file=`pwd`/ovsdb-server.log --pidfile=`pwd`/ovsdb-server.pid
> --unixctl=`pwd`/ovsdb-server.ctl --remote=punix:`pwd`/socket --service"],
>  [0], [[[SC]] CreateService SUCCESS
>  ])
>
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 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.

v2->v3:
- Use spinlock instead of RW lock
- Trigger cleanup event after reassembling the fragments.
v1->v2: No change

Signed-off-by: Anand Kumar 
---
 datapath-windows/automake.mk   |   2 +
 datapath-windows/ovsext/Debug.h|   3 +-
 datapath-windows/ovsext/IpFragment.c   | 503 +
 datapath-windows/ovsext/IpFragment.h   |  74 +
 datapath-windows/ovsext/Switch.c   |   9 +
 datapath-windows/ovsext/ovsext.vcxproj |   2 +
 6 files changed, 592 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..95fc754
--- /dev/null
+++ b/datapath-windows/ovsext/IpFragment.c
@@ -0,0 +1,503 @@
+/*
+ * 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_MEMORY_TAG);
+if (OvsIpFragTable == NULL) {
+NdisFreeRWLock(ovsIpFragmentHashLockObj);
+ovsIpFragmentHashLockObj = NULL;
+return STATUS_INSUFFICIENT_RESOURCES;
+}
+
+for (int i = 0; i < IP_FRAG_HASH_TABLE_SIZE; i++) {
+InitializeListHead(&OvsIpFragTable[i]);
+}
+
+/* Init Cleaner Thread */
+KeInitializeEvent(&ipFragThreadCtx.event, NotificationEvent, FALSE);
+status = PsCreateSystemThread(&threadHandle, SYNCHRONIZE, NULL, NULL,
+  NULL, OvsIpFragmentEntryCleaner,
+  &ipFragThreadCtx);
+
+

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

[Anand Kumar]

This patch retains the MRU value for the reassembled IP datagram in the
OvsForwardingContext when the packet is forwarded to userspace and/or
retrived from userspace.

Also retain the MRU value when there are any deferred actions for the
current NBL.

v2->v3: No change
v1->v2: No change

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Actions.c| 48 +++-
 datapath-windows/ovsext/Actions.h|  3 +++
 datapath-windows/ovsext/DpInternal.h |  2 +-
 datapath-windows/ovsext/PacketIO.c   |  9 ---
 datapath-windows/ovsext/Recirc.c |  6 -
 datapath-windows/ovsext/Recirc.h |  2 ++
 datapath-windows/ovsext/Tunnel.c |  4 +--
 datapath-windows/ovsext/User.c   | 26 +--
 datapath-windows/ovsext/User.h   |  6 +++--
 9 files changed, 72 insertions(+), 34 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index 5f7c50c..14eacb4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -150,6 +150,7 @@ OvsInitForwardingCtx(OvsForwardingContext *ovsFwdCtx,
  UINT32 srcVportNo,
  ULONG sendFlags,
  PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO 
fwdDetail,
+ UINT16 mru,
  OvsCompletionList *completionList,
  OVS_PACKET_HDR_INFO *layers,
  BOOLEAN resetTunnelInfo)
@@ -167,7 +168,7 @@ OvsInitForwardingCtx(OvsForwardingContext *ovsFwdCtx,
 ovsFwdCtx->switchContext = switchContext;
 ovsFwdCtx->completionList = completionList;
 ovsFwdCtx->fwdDetail = fwdDetail;
-
+ovsFwdCtx->mru = mru;
 if (fwdDetail->NumAvailableDestinations > 0) {
 /*
  * XXX: even though MSDN says GetNetBufferListDestinations() returns
@@ -615,6 +616,7 @@ OvsDoFlowLookupOutput(OvsForwardingContext *ovsFwdCtx)
  ovsFwdCtx->srcVportNo,
  ovsFwdCtx->sendFlags,
  &key, &hash, &ovsFwdCtx->layers,
+ ovsFwdCtx->mru,
  flow->actions, flow->actionsLen);
 ovsFwdCtx->curNbl = NULL;
 } else {
@@ -623,9 +625,11 @@ OvsDoFlowLookupOutput(OvsForwardingContext *ovsFwdCtx)
 ovsFwdCtx->switchContext->datapath.misses++;
 InitializeListHead(&missedPackets);
 status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS, vport,
-  &key,ovsFwdCtx->curNbl,
-  FALSE, &ovsFwdCtx->layers,
-  ovsFwdCtx->switchContext, &missedPackets, &num);
+&key,ovsFwdCtx->curNbl,
+FALSE, &ovsFwdCtx->layers,
+ovsFwdCtx->switchContext,
+ovsFwdCtx->mru,
+&missedPackets, &num);
 if (num) {
 OvsQueuePackets(&missedPackets, num);
 }
@@ -722,6 +726,7 @@ OvsTunnelPortTx(OvsForwardingContext *ovsFwdCtx)
 status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
   newNbl, srcVportNo, 0,
   
NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
+  ovsFwdCtx->mru,
   ovsFwdCtx->completionList,
   &ovsFwdCtx->layers, FALSE);
 ovsFwdCtx->curNbl = newNbl;
@@ -822,6 +827,7 @@ OvsTunnelPortRx(OvsForwardingContext *ovsFwdCtx)
 OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
  newNbl, tunnelRxVport->portNo, 0,
  NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
+ ovsFwdCtx->mru,
  ovsFwdCtx->completionList,
  &ovsFwdCtx->layers, FALSE);
 
@@ -921,6 +927,7 @@ OvsOutputForwardingCtx(OvsForwardingContext *ovsFwdCtx)
 status = OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
   newNbl, ovsFwdCtx->srcVportNo, 0,
   
NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
+  ovsFwdCtx->mru,
   ovsFwdCtx->completionList,
   &ovsFwdCtx->layers, FALSE);
 if (status != NDIS_STATUS_SUCCESS) {
@@ -986,7 +993,7 @@ OvsLookupFlowOutput(POVS_SWITCH_CONTEXT switchContext,
 status = OvsInitForwardingCtx(&ovsFwdCtx, switchContext, curNbl,
   internalVport->portNo, 0,
   
NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(curNbl),
-  

[ovs-dev] [PATCH v3 5/5] datapath-windows: Fragment NBL based on MRU size

[Anand Kumar]

This patch adds support for Fragmenting NBL based on the MRU value.
MRU value is updated only for Ipv4 fragments, if it is non zero, then
fragment the NBL and send out the new NBL to the vnic.

v2->v3:
- Updated log message
v1->v2: No change

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Actions.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index 14eacb4..2f029c8 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -34,6 +34,7 @@
 #include "Vport.h"
 #include "Vxlan.h"
 #include "Geneve.h"
+#include "IpFragment.h"
 
 #ifdef OVS_DBG_MOD
 #undef OVS_DBG_MOD
@@ -873,6 +874,7 @@ OvsOutputForwardingCtx(OvsForwardingContext *ovsFwdCtx)
 NDIS_STATUS status = STATUS_SUCCESS;
 POVS_SWITCH_CONTEXT switchContext = ovsFwdCtx->switchContext;
 PCWSTR dropReason;
+PNET_BUFFER_LIST fragNbl = NULL;
 
 /*
  * Handle the case where the some of the destination ports are tunneled
@@ -918,6 +920,30 @@ OvsOutputForwardingCtx(OvsForwardingContext *ovsFwdCtx)
 goto dropit;
 }
 
+if (ovsFwdCtx->mru != 0) {
+/* Fragment nbl based on mru. If it returns NULL then the original
+ * reassembled NBL is sent out to the VIF which will be dropped if
+ * the packet size is more than VIF MTU.
+ */
+fragNbl = OvsFragmentNBL(ovsFwdCtx->switchContext,
+ ovsFwdCtx->curNbl,
+ &(ovsFwdCtx->layers),
+ ovsFwdCtx->mru, 0, TRUE);
+if (fragNbl != NULL) {
+OvsCompleteNBLForwardingCtx(ovsFwdCtx,
+L"Dropped since fragmenting NBL");
+status = OvsInitForwardingCtx(ovsFwdCtx,
+  ovsFwdCtx->switchContext,
+  fragNbl,
+  ovsFwdCtx->srcVportNo,
+  ovsFwdCtx->sendFlags,
+  
NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(fragNbl),
+  ovsFwdCtx->mru,
+  ovsFwdCtx->completionList,
+  &ovsFwdCtx->layers, FALSE);
+}
+}
+
 OvsSendNBLIngress(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl,
   ovsFwdCtx->sendFlags);
 /* End this pipeline by resetting the corresponding context. */
-- 
2.9.3.windows.1

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

[ovs-dev] [PATCH v3 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.

Added MRU field in OvsForwardingContext, to keep track of Maximum recieved unit
from all the recieved IPv4 fragments.

v2->v3:
- Updated log messages and fixed alignment.
v1->v2: No change

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Actions.c   | 16 
 datapath-windows/ovsext/Conntrack.c | 35 ---
 datapath-windows/ovsext/Conntrack.h |  7 ++-
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index b4a286b..5f7c50c 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -125,6 +125,9 @@ typedef struct OvsForwardingContext {
 
 /* header information */
 OVS_PACKET_HDR_INFO layers;
+
+/* Maximum Recieving Unit */
+UINT16 mru;
 } OvsForwardingContext;
 
 /*
@@ -1910,11 +1913,16 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext,
 }
 }
 
-status = OvsExecuteConntrackAction(ovsFwdCtx.curNbl, layers,
-   key, (const PNL_ATTR)a);
+status = OvsExecuteConntrackAction(switchContext, 
&ovsFwdCtx.curNbl, &(ovsFwdCtx.mru),
+   ovsFwdCtx.completionList,
+   
ovsFwdCtx.fwdDetail->SourcePortId,
+   layers, 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;
 }
 break;
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index d1be480..0420f9b 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"
@@ -312,13 +313,25 @@ OvsCtEntryExpired(POVS_CT_ENTRY entry)
 }
 
 static __inline NDIS_STATUS
-OvsDetectCtPacket(OvsFlowKey *key)
+OvsDetectCtPacket(POVS_SWITCH_CONTEXT switchContext,
+  PNET_BUFFER_LIST *curNbl,
+  OvsCompletionList *completionList,
+  NDIS_SWITCH_PORT_ID sourcePort,
+  OvsFlowKey *key,
+  UINT16 *mru,
+  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(switchContext,
+  curNbl,
+  completionList,
+  sourcePort,
+  mru,
+  key->tunKey.tunnelId,
+  newNbl);
 }
 if (key->ipKey.nwProto == IPPROTO_TCP
 || key->ipKey.nwProto == IPPROTO_UDP
@@ -684,11 +697,17 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
 /*
  *---
  * OvsExecuteConntrackAction
- * Executes Conntrack actions XXX - Add more
+ * Executes Conntrack actions
+ * For the Ipv4 fragments, consume the orginal fragment NBL
+ * XXX - Add more
  *---
  */
 NDIS_STATUS
-OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
+OvsExecuteConntrackAction(POVS_SWITCH_CONTEXT switchContext,
+  PNET_BUFFER_LIST *curNbl,
+  UINT16 *mru,
+  OvsCompletionList *completionList,
+  NDIS_SWITCH_PORT_ID sourcePort,
   OVS_PACKET_HDR_INFO *layers,
   OvsFlowKey *key,
   const PNL_ATTR a)
@@ -699,10 +718,11 @@ OvsExecuteConntrackAction(PNET_BUFFER_LIST curNbl,
 MD_MARK *mark = NULL;
 MD_LABELS *labels = NULL;
 PCHAR helper = NULL;
-
+PNET_BUFFER_LIST newNbl = NULL;
 NDIS_STATUS status;
 
-status = OvsDetectCtPacket(key);
+status = OvsDetectCtPacket(switchContext, curNbl, completionList,
+

[ovs-dev] [PATCH v3 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.

v2->v3:
- Updated log message and function summary
v1->v2:
- Fix compile error for release mode.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/BufferMgmt.c | 194 +--
 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, 152 insertions(+), 62 deletions(-)

diff --git a/datapath-windows/ovsext/BufferMgmt.c 
b/datapath-windows/ovsext/BufferMgmt.c
index 6027c35..20ff6f6 100644
--- a/datapath-windows/ovsext/BufferMgmt.c
+++ b/datapath-windows/ovsext/BufferMgmt.c
@@ -1083,6 +1083,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)ðBuf, 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
@@ -1112,15 +1137,16 @@ GetSegmentHeaderInfo(PNET_BUFFER_LIST nbl,
 
 /*
  * --
- * FixSegmentHeader
+ * FixPacketHeader
  *
- *Fix IP length, IP checksum, TCP sequence number and TCP checksum
- *in the segment.
+ *Fix IP length, Offset, IP checksum, TCP sequence number and TCP checksum
+ *in the netbuffer if applicable.
  * --
  */
 static NDIS_STATUS
-FixSegmentHeader(PNET_BUFFER nb, UINT16 segmentSize, UINT32 seqNumber,
- BOOLEAN lastPacket, UINT16 packetCounter)
+FixPacketHeader(PNET_BUFFER nb, UINT16 segmentSize, UINT32 seqNumber,
+BOOLEAN lastPacket, UINT16 packetCounter, UINT16 offset,
+BOOLEAN isFragment)
 {
 EthHdr *dstEth = NULL;
 TCPHdr *dstTCP = NULL;
@@ -1139,41 +1165,55 @@ FixSegmentHeader(PNET_BUFFER nb, UINT16 segmentSize, 
UINT32 seqNumber,
 case ETH_TYPE_IPV4_NBO:
 {
 IPHdr *dstIP = NULL;
-
-ASSERT((INT)MmGetMdlByteCount(mdl) - NET_BUFFER_CURRENT_MDL_OFFSET(nb)
+if (!isFragment) {
+ASSERT((INT)MmGetMdlByteCount(mdl) - 
NET_BUFFER_CURRENT_MDL_OFFSET(nb)
 >= sizeof(EthHdr) + sizeof(IPHdr) + sizeof(TCPHdr));
-dstIP = (IPHdr *)((PCHAR)dstEth + sizeof(*dstEth));
-dstTCP = (TCPHdr *)((PCHAR)dstIP + dstIP->ihl * 4);
-ASSERT((INT)MmGetMdlByteCount(mdl) - NET_BUFFER_CURRENT_MDL_OFFSET(nb)
+dstIP = (IPHdr *)((PCHAR)dstEth + sizeof(*dstEth));
+dstTCP = (TCPHdr *)((PCHAR)dstIP + dstIP->ihl * 4);
+ASSERT((INT)MmGetMdlByteCount(mdl) - 
NET_BUFFER_CURRENT_MDL_OFFSET(nb)
 >= sizeof(EthHdr) + dstIP->ihl * 4 + TCP_HDR_LEN(dstTCP));
 
-/* Fix IP length and checksum */
-ASSERT(dstIP->protocol == IPPROTO_TCP);
-dstIP->tot_len = htons(segmentSize + dstIP->ihl * 4 + 
TCP_HDR_LEN(dstTCP));
-dstIP->id += packetCounter;
+/* Fix IP length and checksum */
+ASSERT(dstIP->protocol == IPPROTO_TCP);
+dstIP->tot_len = htons(segmentSize + dstIP->ihl * 4 + 
TCP_HDR_LEN(dstTCP));
+dstIP->id += packetCounter;
+dstTCP->seq = htonl(seqNumber);
+
+/*
+ * Set the TCP FIN and PSH bit only for the last packet
+ * More information can be found under:
+ * 
https://msdn.microsoft.com/en-us/library/windows/hardware/ff568840%28v=vs.85%29.aspx
+ */
+if (dstTCP->fin) {
+dstTCP->fin = lastPacket;
+}
+if (dstTCP->psh) {
+dstTCP->psh = lastPacket;
+}
+UINT16 csumLength = segmentSize + TCP_HDR_LEN(dstTCP);
+dstTCP->check = IPPseudoChecksum(&dstIP->saddr,
+ &dstIP->daddr,
+ IPPROTO_TCP,
+ csumLength);
+dstTCP->check = CalculateChec

[ovs-dev] [PATCH v3 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.

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 OvsForwardingContext.
  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  |  90 --
 datapath-windows/ovsext/Actions.h  |   3 +
 datapath-windows/ovsext/BufferMgmt.c   | 194 +
 datapath-windows/ovsext/BufferMgmt.h   |  10 +-
 datapath-windows/ovsext/Conntrack.c|  35 ++-
 datapath-windows/ovsext/Conntrack.h|   7 +-
 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   |  74 +
 datapath-windows/ovsext/PacketIO.c |   9 +-
 datapath-windows/ovsext/Recirc.c   |   6 +-
 datapath-windows/ovsext/Recirc.h   |   2 +
 datapath-windows/ovsext/Stt.c  |   2 +-
 datapath-windows/ovsext/Switch.c   |   9 +
 datapath-windows/ovsext/Tunnel.c   |   4 +-
 datapath-windows/ovsext/User.c |  28 +-
 datapath-windows/ovsext/User.h |   6 +-
 datapath-windows/ovsext/Vxlan.c|   2 +-
 datapath-windows/ovsext/ovsext.vcxproj |   2 +
 23 files changed, 888 insertions(+), 109 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

Re: [ovs-dev] [PATCH v3] datapath-windows: Add a wrapper to retreive external vport

[Guru Shetty]

On 24 January 2017 at 12:37, Shashank Ram  wrote:

> This wrapper is to simplify readability.
>
> Signed-off-by: Shashank Ram 
>
Applied.



> ---
>  datapath-windows/ovsext/Actions.c  | 8 
>  datapath-windows/ovsext/PacketIO.c | 6 +++---
>  datapath-windows/ovsext/Vport.c| 6 ++
>  datapath-windows/ovsext/Vport.h| 2 ++
>  4 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/
> Actions.c
> index b4a286b..df1280d 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -290,8 +290,8 @@ OvsDetectTunnelPkt(OvsForwardingContext *ovsFwdCtx,
>   * default port.
>   */
>  BOOLEAN validSrcPort =
> -(ovsFwdCtx->fwdDetail->SourcePortId ==
> - ovsFwdCtx->switchContext->virtualExternalPortId) ||
> +(OvsIsExternalVportByPortId(ovsFwdCtx->switchContext,
> + ovsFwdCtx->fwdDetail->SourcePortId)) ||
>  (ovsFwdCtx->fwdDetail->SourcePortId ==
>   NDIS_SWITCH_DEFAULT_PORT_ID);
>
> @@ -2130,8 +2130,8 @@ OvsDoRecirc(POVS_SWITCH_CONTEXT switchContext,
>  }
>  status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS,
>  vport, key, ovsFwdCtx.curNbl,
> -vport->portId ==
> -switchContext->
> virtualExternalPortId,
> +OvsIsExternalVportByPortId(
> switchContext,
> +vport->portId),
>  &ovsFwdCtx.layers,
>  ovsFwdCtx.switchContext,
>  &missedPackets, &num);
> diff --git a/datapath-windows/ovsext/PacketIO.c b/datapath-windows/ovsext/
> PacketIO.c
> index 4005589..a90b556 100644
> --- a/datapath-windows/ovsext/PacketIO.c
> +++ b/datapath-windows/ovsext/PacketIO.c
> @@ -259,13 +259,13 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT
> switchContext,
>  curNbl,
>  nextNativeForwardedNbl,
>  sendCompleteFlags,
> -sourcePort == switchContext->virtualExternalPortId);
> +OvsIsExternalVportByPortId(switchContext, sourcePort));
>  continue;
>  }
>  #endif /* NDIS_SUPPORT_NDIS640 */
>
>  ctx = OvsInitExternalNBLContext(switchContext, curNbl,
> -  sourcePort == switchContext->virtualExternalPortId);
> +  OvsIsExternalVportByPortId(switchContext, sourcePort));
>  if (ctx == NULL) {
>  RtlInitUnicodeString(&filterReason,
>  L"Cannot allocate external NBL context.");
> @@ -345,7 +345,7 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT switchContext,
>  datapath->misses++;
>  status = OvsCreateAndAddPackets(NULL, 0,
> OVS_PACKET_CMD_MISS,
>   vport, &key, curNbl,
> - sourcePort == switchContext->
> virtualExternalPortId,
> + OvsIsExternalVportByPortId(switchContext,
> sourcePort),
>   &layers, switchContext, &missedPackets,
> &num);
>  if (status == NDIS_STATUS_SUCCESS) {
>  /* Complete the packet since it was copied to user
> diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/
> Vport.c
> index e9e22aa..9142937 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -909,6 +909,12 @@ OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT
> switchContext,
>  }
>  }
>
> +BOOLEAN OvsIsExternalVportByPortId(POVS_SWITCH_CONTEXT switchContext,
> +   NDIS_SWITCH_PORT_ID portId)
> +{
> +return (portId == switchContext->virtualExternalPortId);
> +}
> +
>  POVS_VPORT_ENTRY
>  OvsAllocateVport(VOID)
>  {
> diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/
> Vport.h
> index 4dc4e00..7d88f86 100644
> --- a/datapath-windows/ovsext/Vport.h
> +++ b/datapath-windows/ovsext/Vport.h
> @@ -134,6 +134,8 @@ POVS_VPORT_ENTRY OvsFindVportByHvNameW(POVS_SWITCH_CONTEXT
> switchContext,
>  POVS_VPORT_ENTRY OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT
> switchContext,
>   NDIS_SWITCH_PORT_ID
> portId,
>   NDIS_SWITCH_NIC_INDEX
> index);
> +BOOLEAN OvsIsExternalVportByPortId(POVS_SWITCH_CONTEXT switchContext,
> +   NDIS_SWITCH_PORT_ID portId);
>  POVS_VPORT_ENTRY OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT
> switchContext,
>  UINT16 dstPort,
>  OVS_VPORT_TYPE
> ovsPortType);
> --
> 2.6.2
>
> _

Re: [ovs-dev] [userspace meter v3 3/5] dpif: Meter framework.

[Andy Zhou]

On Thu, Jan 26, 2017 at 6:34 PM, Jarno Rajahalme  wrote:

> It feels weird reviewing my own code from past. What if I make a pass of
> fixing the problems I see and send a new version of the remaining patches
> for review?
>
> Sounds like a plan. I have pushed the first 2 patches in this series, so
you can drop them. I will be happy to review the remaining patches when you
are happy with them.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 4/4] datapath-windows: Add support for OVS_KEY_ATTR_TCP set action

[Guru Shetty]

On 26 January 2017 at 15:43, Alin Serdean 
wrote:

> From: Alin Serdean 
>
> This patch adds support for set action with OVS_KEY_ATTR_TCP attribute
> (change TCP source or destination port).
>
> If the source or destination TCP port was changed, update the TCP checksum.
>
> A sample flow can look like the following:
> set(tcp(src=80,dst=443))
>
> Signed-off-by: Alin Gabriel Serdean 
>

I applied the series to master. Was this the series that you needed for OVN
to work on Windows and needed it in 2.7?


> ---
> v3: check for layers->isTcp as suggested by Sairam Venugopal <
> vsai...@vmware.com>
> v2: set action for tcp attribute changed to function: OvsUpdateTcpPorts
> ---
>  datapath-windows/ovsext/Actions.c | 48 ++
> +
>  1 file changed, 48 insertions(+)
>
> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/
> Actions.c
> index 3dfcc68..bce37f8 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -1430,6 +1430,49 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
>
>  /*
>   *---
> -
> + * OvsUpdateTcpPorts --
> + *  Updates the TCP source or destination port in ovsFwdCtx.curNbl
> inline
> + *  based on the specified key.
> + *---
> -
> + */
> +static __inline NDIS_STATUS
> +OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
> +  const struct ovs_key_tcp *tcpAttr)
> +{
> +PUINT8 bufferStart;
> +OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
> +TCPHdr *tcpHdr = NULL;
> +
> +ASSERT(layers->value != 0);
> +
> +if (!layers->isTcp) {
> +ovsActionStats.noCopiedNbl++;
> +return NDIS_STATUS_FAILURE;
> +}
> +
> +bufferStart = OvsGetHeaderBySize(ovsFwdCtx, layers->l7Offset);
> +if (!bufferStart) {
> +return NDIS_STATUS_RESOURCES;
> +}
> +
> +tcpHdr = (TCPHdr *)(bufferStart + layers->l4Offset);
> +
> +if (tcpHdr->source != tcpAttr->tcp_src) {
> +tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
> + tcpAttr->tcp_src);
> +tcpHdr->source = tcpAttr->tcp_src;
> +}
> +if (tcpHdr->dest != tcpAttr->tcp_dst) {
> +tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
> + tcpAttr->tcp_dst);
> +tcpHdr->dest = tcpAttr->tcp_dst;
> +}
> +
> +return NDIS_STATUS_SUCCESS;
> +}
> +
> +/*
> + *---
> -
>   * OvsUpdateIPv4Header --
>   *  Updates the IPv4 header in ovsFwdCtx.curNbl inline based on the
>   *  specified key.
> @@ -1575,6 +1618,11 @@ OvsExecuteSetAction(OvsForwardingContext
> *ovsFwdCtx,
>  NlAttrGetUnspec(a, sizeof(struct ovs_key_udp)));
>  break;
>
> +case OVS_KEY_ATTR_TCP:
> +status = OvsUpdateTcpPorts(ovsFwdCtx,
> +NlAttrGetUnspec(a, sizeof(struct ovs_key_tcp)));
> +break;
> +
>  default:
>  OVS_LOG_INFO("Unhandled attribute %#x", type);
>  break;
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] assertion failing in commit_set_ipv4_action(), flow->nw_proto != base_flow->nw_proto

[Thomas Morin]

Jarno,

2017-01-27, Jarno Rajahalme:
Cleanest solution would be to use the new clone action for the 
datapath, which, if important, we could also backport to OVS 2.5.x.


Yes, clone is indeed much more elegant.



Datapath CLONE is yet to be upstreamed, though, may take a little time.


Even before net-dev upstreaming, there doesn't seem to even be a clone 
implementation in ovs/datapath yet, right ?


-Thomas





2017-01-06, Jarno Rajahalme:
I sent a patch to do this fix on OVS master. IMO we should also make 
the datapath more flexible so that it would be able to POP back to 
IP after PUSH actions on an IP packet in the same action list. But 
that will not make it to OVS 2.7.


I would appreciate if Thomas could apply the and test!

  Jarno

On Dec 14, 2016, at 1:52 PM, Jarno Rajahalme > wrote:




On Dec 13, 2016, at 8:44 PM, Takashi YAMAMOTO > wrote:




On Wed, Dec 14, 2016 at 9:02 AM, Jarno Rajahalme>wrote:




On Dec 13, 2016, at 3:32 PM, Takashi YAMAMOTO
mailto:yamam...@ovn.org>> wrote:



On Tue, Dec 13, 2016 at 6:49 PM, Thomas
Morinmailto:thomas.mo...@orange.com>>wrote:

Hi Jarno,

2016-12-10, Jarno Rajahalme:

On Dec 9, 2016, at 7:04 AM, Thomas Morin
mailto:thomas.mo...@orange.com>> wrote:


2016-12-09, Thomas Morin:

In the same setup as the one on which the bug was
observed, [...]


I was confused, I in fact tested the patch (branch-2.5)
on a /different/ setup as the one on which we hit the
bug, using MPLS over a GRE tunnel port, rather than
plain MPLS over an eth port.
Sorry if any confusion arised... I can test on the
first setup if relevant.



Maybe the kernel datapath does not support MPLS over a
GRE tunnel port. Having ‘dmesg’ output for the test run
might reveal why the actions validation fails.


The dmesg output was the following:

[171295.258939] openvswitch: netlink: Flow actions may
not be safe on all matching packets.

I've tested the patch on the platform on which the bug
was initially hit (*not* using MPLS/GRE), and I have the
following a few times in the logs right after I do an
"ovs-appctl fdb/flush":

2016-12-13T09:44:08.449Z|1|dpif(handler68)|WARN|Dropped
3 log messages in last 1 seconds (most recently, 1
seconds ago) due to excessive rate
2016-12-13T09:44:08.449Z|2|dpif(handler68)|WARN|system@ovs-system:
failed to put[create] (Invalid argument)
ufid:f046c4c4-b97f-436d-bd7c-91ed307275ac

recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(9),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64),eth_type(0x0800),ipv4(src=10.0.1.29,dst=10.0.0.3,proto=6,tos=0/0xfc,ttl=64,frag=no),tcp(src=54253,dst=8080),tcp_flags(0/0),

actions:set(ipv4(src=10.0.1.29,dst=10.0.0.3,ttl=63)),set(eth(src=b8:2a:72:de:1b:e3,dst=00:17:cb:79:2c:01)),push_mpls(label=433680,tc=0,ttl=63,bos=1,eth_type=0x8847),7,set(eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64)),pop_mpls(eth_type=0x800),set(ipv4(src=10.0.1.29,dst=10.0.0.3,tos=0/0xfc,ttl=64)),push_vlan(vid=1,pcp=0),3,8,pop_vlan,13

And dmesg:
[926833.612372] openvswitch: netlink: Flow actions may
not be safe on all matching packets.


it's complaining about set ipv4 after pop_mpls because it
doesn't know about the "restored" l3.
while we can improve the kernel, i guess we need to stick
with recirc for now.


This should do it? Does not break any existing tests on
branch-2.5, but I did not create a test for this yet.

diff --git a/ofproto/ofproto-dpif-xlate.c
b/ofproto/ofproto-dpif-xlate.c
index fb25f63..6ee2a72 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2899,6 +2899,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
 {
 bool use_masked = ctx->xbridge->support.masked_set_action;
+/* An MPLS packet can be implicitly popped back to a
non-MPLS packet, if a
+ * patch port peer or a group bucket pushed MPLS.  Set
the 'was_mpls' flag
+ * also in that case. */
+if (eth_type_mpls(ctx->base_flow.dl_type)
+&& !eth_type_mpls(ctx->xin->flow.dl_type)
+&& ctx->xbridge->support.odp.recirc) {
+  ctx->was_mpls = true;
+}
+


i guess this check needs to be somewhere around "ctx->was_mpls = 
old_was_mpls" in

affected functions.



Right, as that is where the implicit MPLS POP action happens, when 
the ‘ctx->xin->flow’ is restored.


  Jarno


 ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow,
&ctx->base_flow,
 ctx->odp_actions, ctx->wc,
 use_masked);

Jarno





-Thomas







On Dec 1, 2016, at 5:57 PM, Jarno Rajahalme
mailto:ja

Re: [ovs-dev] assertion failing in commit_set_ipv4_action(), flow->nw_proto != base_flow->nw_proto

[Jarno Rajahalme]

> On Jan 27, 2017, at 7:40 AM, Thomas Morin  wrote:
> 
> Hi Jarno, all,
> 
> Sorry for being slow to react.
> I'd be glad to test a fix, but I'm unclear if there is one ready to be tested.
> 
> Perhasp "xlate: Recirculate also when MPLS POP is implicit." but I'm not sure 
> there has been a follow-up to the discuss between you and Ben?
> Or is the new clone action possibly taking care of this issue (does not seem 
> so, since datapath dependent, but I might have misunderstood)?
> 

Cleanest solution would be to use the new clone action for the datapath, which, 
if important, we could also backport to OVS 2.5.x.

Datapath CLONE is yet to be upstreamed, though, may take a little time.

  Jarno

> -Thomas
> 
> 2017-01-06, Jarno Rajahalme:
>> I sent a patch to do this fix on OVS master. IMO we should also make the 
>> datapath more flexible so that it would be able to POP back to IP after PUSH 
>> actions on an IP packet in the same action list. But that will not make it 
>> to OVS 2.7.
>> 
>> I would appreciate if Thomas could apply the and test!
>> 
>>   Jarno
>> 
>>> On Dec 14, 2016, at 1:52 PM, Jarno Rajahalme >> > wrote:
>>> 
 
 On Dec 13, 2016, at 8:44 PM, Takashi YAMAMOTO >>> > wrote:
 
 
 
 On Wed, Dec 14, 2016 at 9:02 AM, Jarno Rajahalme >>> > wrote:
 
> On Dec 13, 2016, at 3:32 PM, Takashi YAMAMOTO  > wrote:
> 
> 
> 
> On Tue, Dec 13, 2016 at 6:49 PM, Thomas Morin  > wrote:
> Hi Jarno,
> 
> 2016-12-10, Jarno Rajahalme:
>> On Dec 9, 2016, at 7:04 AM, Thomas Morin > > wrote:
>>> 
>>> 2016-12-09, Thomas Morin:
 In the same setup as the one on which the bug was observed, [...]
>>> 
>>> I was confused, I in fact tested the patch (branch-2.5) on a 
>>> /different/ setup as the one on which we hit the bug, using MPLS over a 
>>> GRE tunnel port, rather than plain MPLS over an eth port.
>>> Sorry if any confusion arised... I can test on the first setup if 
>>> relevant.
>>> 
>> 
>> Maybe the kernel datapath does not support MPLS over a GRE tunnel port. 
>> Having ‘dmesg’ output for the test run might reveal why the actions 
>> validation fails.
> 
> The dmesg output was the following: 
> 
> [171295.258939] openvswitch: netlink: Flow actions may not be safe on all 
> matching packets.
> 
> I've tested the patch on the platform on which the bug was initially hit 
> (*not* using MPLS/GRE), and I have the following a few times in the logs 
> right after I do an "ovs-appctl fdb/flush":
> 
> 2016-12-13T09:44:08.449Z|1|dpif(handler68)|WARN|Dropped 3 log 
> messages in last 1 seconds (most recently, 1 seconds ago) due to 
> excessive rate
> 2016-12-13T09:44:08.449Z|2|dpif(handler68)|WARN|system@ovs-system: 
> failed to put[create] (Invalid argument) 
> ufid:f046c4c4-b97f-436d-bd7c-91ed307275ac 
> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(9),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64),eth_type(0x0800),ipv4(src=10.0.1.29,dst=10.0.0.3,proto=6,tos=0/0xfc,ttl=64,frag=no),tcp(src=54253,dst=8080),tcp_flags(0/0),
>  
> actions:set(ipv4(src=10.0.1.29,dst=10.0.0.3,ttl=63)),set(eth(src=b8:2a:72:de:1b:e3,dst=00:17:cb:79:2c:01)),push_mpls(label=433680,tc=0,ttl=63,bos=1,eth_type=0x8847),7,set(eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64)),pop_mpls(eth_type=0x800),set(ipv4(src=10.0.1.29,dst=10.0.0.3,tos=0/0xfc,ttl=64)),push_vlan(vid=1,pcp=0),3,8,pop_vlan,13
> 
> And dmesg:
> [926833.612372] openvswitch: netlink: Flow actions may not be safe on all 
> matching packets.
> 
> it's complaining about set ipv4 after pop_mpls because it doesn't know 
> about the "restored" l3.
> while we can improve the kernel, i guess we need to stick with recirc for 
> now.
>  
 
 This should do it? Does not break any existing tests on branch-2.5, but I 
 did not create a test for this yet.
 
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index fb25f63..6ee2a72 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -2899,6 +2899,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
  {
  bool use_masked = ctx->xbridge->support.masked_set_action;
  
 +/* An MPLS packet can be implicitly popped back to a non-MPLS packet, 
 if a
 + * patch port peer or a group bucket pushed MPLS.  Set the 'was_mpls' 
 flag
 + * also in that case. */
 +if (eth_type_mpls(ctx->base_flow.dl_type)
 +&& !eth_type_mpls(ctx->xin->flow.dl_type)
 +&& ctx->xbridge->support.odp.recirc) {
 +ctx->was_mpls = true;

Re: [ovs-dev] [PATCH] windows: wmi add include

[Guru Shetty]

On 26 January 2017 at 14:15, Alin Serdean 
wrote:

> Add 'util.h' to includes otherwise the definition of the function
> ovs_format_message will be unknown and an abort with the following
> stacktrace will be triggered:
> *1  ovs-vswitchd.exe!_output_l(_iobuf * stream=0x00bb711dcbf0,
> const char * format=0x7ff6016a2c12, localeinfo_struct * plocinfo, char
> * argptr=0x7ff60176b460)
>  2  ovs-vswitchd.exe!_vscprintf_helper(int (_iobuf *, const char *,
> localeinfo_struct *, char *) * outfn=0x7ff60160d4d0, const char *
> format=0x7ff6016a2c10, localeinfo_struct * plocinfo=0x,
> char * ap=0x00bb711dce98)
>  3  ovs-vswitchd.exe!ovs_vsnprintf(char * s=0x03de,
> unsigned __int64 n=140694562417680, const char * format=0x00bb711dce98,
> char * args=0x00bb711dcdf8)
>  4  ovs-vswitchd.exe!ds_put_format_valist(ds * ds=0x00bb711dce98,
> const char * format=0x7ff6016ba1de, char * args_=0x7ff6016a2c10)
>  5  ovs-vswitchd.exe!format_log_message(const vlog_module *
> module=0x02933ce9a560, vlog_level level=VLL_OFF, const char *
> pattern=0x, const char * message=0x, char *
> args_=0x00bb711dce98, ds * s=0x00bb711dcdf8)
>  6  ovs-vswitchd.exe!vlog_valist(const vlog_module *
> module=0x0293000b, vlog_level level=VLL_OFF, const char *
> message=0x80041001, char * args=0x00bb711dcf80)
>  7  ovs-vswitchd.exe!vlog(const vlog_module *
> module=0x80041001, vlog_level level=1022648016, const char *
> message=0x7ff6016a2c10, ...)
>  8  ovs-vswitchd.exe!create_wmi_port(char * name=0x)
>  9  ovs-vswitchd.exe!dpif_netlink_port_add__(dpif_netlink *
> dpif=0x02933cea6840, netdev * netdev=0x, unsigned int *
> port_nop=0x)
>  10 ovs-vswitchd.exe!dpif_netlink_port_add(dpif *
> dpif_=0x00bb711df360, netdev * netdev=0x02933cea05c0, unsigned int
> * port_nop=0x02933cefcdf0)
>  11 ovs-vswitchd.exe!dpif_port_add(dpif * dpif=0x02933cf3ecc0,
> netdev * netdev=0x02933cf0a5b0, unsigned int *
> port_nop=0x02933cf0a5b0)
>  12 ovs-vswitchd.exe!port_add(ofproto * ofproto_=0x02933cefcdf0,
> netdev * netdev=0x)
>  13 ovs-vswitchd.exe!ofproto_port_add(ofproto *
> ofproto=0x, netdev * netdev=0x02933cf3e690, unsigned
> int * ofp_portp=0x00bb711df5c8)
>  14 ovs-vswitchd.exe!iface_do_create(const bridge *
> br=0x02933cf097e0, const ovsrec_interface *
> iface_cfg=0x02933cefcdf0, unsigned int * ofp_portp=0x02933cf3d400,
> netdev * * netdevp=0x02933cee2cb0, char * * errp=0x00bb711df570)
>  15 ovs-vswitchd.exe!iface_create(bridge * br=0x02933cf097e0,
> const ovsrec_interface * iface_cfg=0x, const ovsrec_port *
> port_cfg=0x02933cea35d0)
>  16 ovs-vswitchd.exe!bridge_add_ports__(bridge *
> br=0x, const shash * wanted_ports=0x02933cee2cb0,
> bool with_requested_port=232)
>  17 ovs-vswitchd.exe!bridge_reconfigure(const ovsrec_open_vswitch *
> ovs_cfg=0x02933cee2cb0)
>  18 ovs-vswitchd.exe!bridge_run()
>  19 ovs-vswitchd.exe!main(int argc=1, char * * argv=0x02933ce78f60)
>  20 ovs-vswitchd.exe!__tmainCRTStartup()
>  21 kernel32.dll!BaseThreadInitThunk ()
>  22 ntdll.dll!RtlUserThreadStart ()
>
> Signed-off-by: Alin Gabriel Serdean 
>

Would you mind limiting the width of the commit message to 75 characters?
There are other examples in 'git log' where stack traces have been limited
to 75 characters
(Documentation/internals/contributing/submitting-patches.rst).



> ---
> Intended for master and branch-2.7
> ---
>  lib/wmi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/wmi.c b/lib/wmi.c
> index e38b482..2203df4 100644
> --- a/lib/wmi.c
> +++ b/lib/wmi.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include "openvswitch/vlog.h"
> +#include "util.h"
>
>  VLOG_DEFINE_THIS_MODULE(wmi);
>
> --
> 2.10.2.windows.1
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] windows: wmi add include

[Sairam Venugopal]

Alin,

Can you open a bug in ovs-issues and move the stack trace to that instead? You 
can tag the commit message with the ovs-issue id.

Thanks,
Sairam




On 1/27/17, 12:41 PM, "ovs-dev-boun...@openvswitch.org on behalf of Guru 
Shetty"  wrote:

>On 26 January 2017 at 14:15, Alin Serdean 
>wrote:
>
>> Add 'util.h' to includes otherwise the definition of the function
>> ovs_format_message will be unknown and an abort with the following
>> stacktrace will be triggered:
>> *1  ovs-vswitchd.exe!_output_l(_iobuf * stream=0x00bb711dcbf0,
>> const char * format=0x7ff6016a2c12, localeinfo_struct * plocinfo, char
>> * argptr=0x7ff60176b460)
>>  2  ovs-vswitchd.exe!_vscprintf_helper(int (_iobuf *, const char *,
>> localeinfo_struct *, char *) * outfn=0x7ff60160d4d0, const char *
>> format=0x7ff6016a2c10, localeinfo_struct * plocinfo=0x,
>> char * ap=0x00bb711dce98)
>>  3  ovs-vswitchd.exe!ovs_vsnprintf(char * s=0x03de,
>> unsigned __int64 n=140694562417680, const char * format=0x00bb711dce98,
>> char * args=0x00bb711dcdf8)
>>  4  ovs-vswitchd.exe!ds_put_format_valist(ds * ds=0x00bb711dce98,
>> const char * format=0x7ff6016ba1de, char * args_=0x7ff6016a2c10)
>>  5  ovs-vswitchd.exe!format_log_message(const vlog_module *
>> module=0x02933ce9a560, vlog_level level=VLL_OFF, const char *
>> pattern=0x, const char * message=0x, char *
>> args_=0x00bb711dce98, ds * s=0x00bb711dcdf8)
>>  6  ovs-vswitchd.exe!vlog_valist(const vlog_module *
>> module=0x0293000b, vlog_level level=VLL_OFF, const char *
>> message=0x80041001, char * args=0x00bb711dcf80)
>>  7  ovs-vswitchd.exe!vlog(const vlog_module *
>> module=0x80041001, vlog_level level=1022648016, const char *
>> message=0x7ff6016a2c10, ...)
>>  8  ovs-vswitchd.exe!create_wmi_port(char * name=0x)
>>  9  ovs-vswitchd.exe!dpif_netlink_port_add__(dpif_netlink *
>> dpif=0x02933cea6840, netdev * netdev=0x, unsigned int *
>> port_nop=0x)
>>  10 ovs-vswitchd.exe!dpif_netlink_port_add(dpif *
>> dpif_=0x00bb711df360, netdev * netdev=0x02933cea05c0, unsigned int
>> * port_nop=0x02933cefcdf0)
>>  11 ovs-vswitchd.exe!dpif_port_add(dpif * dpif=0x02933cf3ecc0,
>> netdev * netdev=0x02933cf0a5b0, unsigned int *
>> port_nop=0x02933cf0a5b0)
>>  12 ovs-vswitchd.exe!port_add(ofproto * ofproto_=0x02933cefcdf0,
>> netdev * netdev=0x)
>>  13 ovs-vswitchd.exe!ofproto_port_add(ofproto *
>> ofproto=0x, netdev * netdev=0x02933cf3e690, unsigned
>> int * ofp_portp=0x00bb711df5c8)
>>  14 ovs-vswitchd.exe!iface_do_create(const bridge *
>> br=0x02933cf097e0, const ovsrec_interface *
>> iface_cfg=0x02933cefcdf0, unsigned int * ofp_portp=0x02933cf3d400,
>> netdev * * netdevp=0x02933cee2cb0, char * * errp=0x00bb711df570)
>>  15 ovs-vswitchd.exe!iface_create(bridge * br=0x02933cf097e0,
>> const ovsrec_interface * iface_cfg=0x, const ovsrec_port *
>> port_cfg=0x02933cea35d0)
>>  16 ovs-vswitchd.exe!bridge_add_ports__(bridge *
>> br=0x, const shash * wanted_ports=0x02933cee2cb0,
>> bool with_requested_port=232)
>>  17 ovs-vswitchd.exe!bridge_reconfigure(const ovsrec_open_vswitch *
>> ovs_cfg=0x02933cee2cb0)
>>  18 ovs-vswitchd.exe!bridge_run()
>>  19 ovs-vswitchd.exe!main(int argc=1, char * * argv=0x02933ce78f60)
>>  20 ovs-vswitchd.exe!__tmainCRTStartup()
>>  21 kernel32.dll!BaseThreadInitThunk ()
>>  22 ntdll.dll!RtlUserThreadStart ()
>>
>> Signed-off-by: Alin Gabriel Serdean 
>>
>
>Would you mind limiting the width of the commit message to 75 characters?
>There are other examples in 'git log' where stack traces have been limited
>to 75 characters
>(Documentation/internals/contributing/submitting-patches.rst).
>
>
>
>> ---
>> Intended for master and branch-2.7
>> ---
>>  lib/wmi.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/wmi.c b/lib/wmi.c
>> index e38b482..2203df4 100644
>> --- a/lib/wmi.c
>> +++ b/lib/wmi.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include "openvswitch/vlog.h"
>> +#include "util.h"
>>
>>  VLOG_DEFINE_THIS_MODULE(wmi);
>>
>> --
>> 2.10.2.windows.1
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=GAUaSViQwwj9DpLZv6uQSAZs23bgwqSbHMYDd56F_T8&s=E2ShhyooPYUdqaTjqybwFzT3z-1U3fXJnVC-oK2WUTI&e=
>>  
>>
>___
>dev mailing list
>d...@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58J

Re: [ovs-dev] [PATCH v12 6/6] ovn: specify options:nat-addresses as "router"

[Mickey Spiegel]

On Fri, Jan 27, 2017 at 11:16 AM, Guru Shetty  wrote:

>
>>
>> I should clarify that statement. It is a good thing if the chassis
>> changes, for example if doing simple high availability. The GARP
>> packet will fix L2 learning.
>>
>> As I think about it, if anyone uses logical routers without NAT
>> or load balancers, and the chassis changes, then GARP of the
>> router IP would still be useful. I guess a user could always
>> specify the router IP and MAC in "nat-addresses" to make that
>> happen. I am not sure if anyone is deploying OVN that way.
>>
>> Mickey
>>
>
> I applied the first 5 patches of the series to master. I couldn't apply it
> to 2.7 because of some conflicts. Would you mind reposting it just for 2.7
> with "branch-2.7" in the subject . For e.g: [PATCH branch-2.7]
>

Some previous patches made it into master but not yet
branch-2.7:

https://github.com/openvswitch/ovs/commit/ba8d3816e88f7a702635c09111f58352ecad6506
https://github.com/openvswitch/ovs/commit/41a15b71ed1ef35aa612a1128082219fbfc3f327

Next are a bunch of blp's patches that need to go in
before these 5 patches:

https://github.com/openvswitch/ovs/commit/80b6743d0ab3a39884fe873dd616cb49b6f55fab
https://github.com/openvswitch/ovs/commit/b3bd2c33e83e2039d75e830368a64d596f820aaa
https://github.com/openvswitch/ovs/commit/ebb467ff1c255813d6ba27d91ef6180e9a20fe0a
https://github.com/openvswitch/ovs/commit/8f5de08322673f4e60f44d599fa7ee4de65bc078
https://github.com/openvswitch/ovs/commit/c571f48c36223de360ba0fa4d89104a7da14dbca
https://github.com/openvswitch/ovs/commit/4c99cb181b6937efb3819cffc9765999fd7b7796
https://github.com/openvswitch/ovs/commit/db0e819be065c1474ceef232dcc1260c9a2e7c0e

blp said that he could help with the backport:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327884.html

Mickey

>
>
>>
>>
>>> Mickey
>>>
>>>

 Acked-by: Gurucharan Shetty 

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

Re: [ovs-dev] [PATCH v12 6/6] ovn: specify options:nat-addresses as "router"

[Guru Shetty]

>
>
>
> I should clarify that statement. It is a good thing if the chassis
> changes, for example if doing simple high availability. The GARP
> packet will fix L2 learning.
>
> As I think about it, if anyone uses logical routers without NAT
> or load balancers, and the chassis changes, then GARP of the
> router IP would still be useful. I guess a user could always
> specify the router IP and MAC in "nat-addresses" to make that
> happen. I am not sure if anyone is deploying OVN that way.
>
> Mickey
>

I applied the first 5 patches of the series to master. I couldn't apply it
to 2.7 because of some conflicts. Would you mind reposting it just for 2.7
with "branch-2.7" in the subject . For e.g: [PATCH branch-2.7]


>
>
>> Mickey
>>
>>
>>>
>>> Acked-by: Gurucharan Shetty 
>>>
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v12 6/6] ovn: specify options:nat-addresses as "router"

[Mickey Spiegel]

On Fri, Jan 27, 2017 at 10:29 AM, Mickey Spiegel 
wrote:

> Thanks for the review.
>
> On Fri, Jan 27, 2017 at 10:20 AM, Guru Shetty  wrote:
>
>>
>>
>> On 26 January 2017 at 01:20, Mickey Spiegel 
>> wrote:
>>
>>> Currently in OVN, the "nat-addresses" in the "options" column of a
>>> logical switch port of type "router" must be specified manually.
>>> Typically the user would specify as "nat-addresses" all of the NAT
>>> external IP addresses and load balancer IP addresses that have
>>> already been specified separately on the router.
>>>
>>> This patch allows the logical switch port's "nat-addresses" to be
>>> specified as the string "router".  When ovn-northd sees this string,
>>> it automatically copies the following into the southbound
>>> Port_Binding's "nat-addresses" in the "options" column:
>>> The options:router-port's MAC address.
>>> Each NAT external IP address (of any NAT type) specified on the
>>> logical router of options:router-port.
>>> Each load balancer IP address specified on the logical router of
>>> options:router-port.
>>> This will cause the controller where the gateway router resides to
>>> issue gratuitous ARPs for each NAT external IP address and for each
>>> load balancer IP address specified on the gateway router.
>>>
>>> This patch is written as if it will be included in OVS 2.7.  If it
>>> is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml
>>> will need to be updated from OVS 2.7 to OVS 2.8.
>>>
>>> Signed-off-by: Mickey Spiegel 
>>> ---
>>>  ovn/northd/ovn-northd.c | 114 ++
>>> --
>>>  ovn/ovn-nb.xml  |  42 +++---
>>>  tests/ovn.at|  60 +
>>>  3 files changed, 185 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>>> index e24ff8f..efb8a74 100644
>>> --- a/ovn/northd/ovn-northd.c
>>> +++ b/ovn/northd/ovn-northd.c
>>> @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx,
>>>  }
>>>
>>>  static void
>>> +ip_address_and_port_from_lb_key(const char *key, char **ip_address,
>>> +uint16_t *port);
>>> +
>>> +static void
>>> +get_router_load_balancer_ips(const struct ovn_datapath *od,
>>> + struct sset *all_ips)
>>> +{
>>> +if (!od->nbr) {
>>> +return;
>>> +}
>>> +
>>> +for (int i = 0; i < od->nbr->n_load_balancer; i++) {
>>> +struct nbrec_load_balancer *lb = od->nbr->load_balancer[i];
>>> +struct smap *vips = &lb->vips;
>>> +struct smap_node *node;
>>> +
>>> +SMAP_FOR_EACH (node, vips) {
>>> +/* node->key contains IP:port or just IP. */
>>> +char *ip_address = NULL;
>>> +uint16_t port;
>>> +
>>> +ip_address_and_port_from_lb_key(node->key, &ip_address,
>>> &port);
>>> +if (!ip_address) {
>>> +continue;
>>> +}
>>> +
>>> +if (!sset_contains(all_ips, ip_address)) {
>>> +sset_add(all_ips, ip_address);
>>> +}
>>> +
>>> +free(ip_address);
>>> +}
>>> +}
>>> +}
>>> +
>>> +/* Returns a string consisting of the port's MAC address followed by the
>>> + * external IP addresses of all NAT rules defined on that router and the
>>> + * VIPs of all load balancers defined on that router. */
>>>
>> A comment that the called has to free the returned string is useful.
>>
>
> I can add that.
>
>
>>
>> Also, the load balancer IP address can also be the IP address of the
>> router itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a
>> problem when a GARP is sent for the router IP too.
>>
>
> If the router IP is used as a load balancer IP address or as a SNAT
> address, then sending GARP for the router IP is a good thing.
>

I should clarify that statement. It is a good thing if the chassis
changes, for example if doing simple high availability. The GARP
packet will fix L2 learning.

As I think about it, if anyone uses logical routers without NAT
or load balancers, and the chassis changes, then GARP of the
router IP would still be useful. I guess a user could always
specify the router IP and MAC in "nat-addresses" to make that
happen. I am not sure if anyone is deploying OVN that way.

Mickey


> Mickey
>
>
>>
>> Acked-by: Gurucharan Shetty 
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v12 6/6] ovn: specify options:nat-addresses as "router"

[Mickey Spiegel]

Thanks for the review.

On Fri, Jan 27, 2017 at 10:20 AM, Guru Shetty  wrote:

>
>
> On 26 January 2017 at 01:20, Mickey Spiegel  wrote:
>
>> Currently in OVN, the "nat-addresses" in the "options" column of a
>> logical switch port of type "router" must be specified manually.
>> Typically the user would specify as "nat-addresses" all of the NAT
>> external IP addresses and load balancer IP addresses that have
>> already been specified separately on the router.
>>
>> This patch allows the logical switch port's "nat-addresses" to be
>> specified as the string "router".  When ovn-northd sees this string,
>> it automatically copies the following into the southbound
>> Port_Binding's "nat-addresses" in the "options" column:
>> The options:router-port's MAC address.
>> Each NAT external IP address (of any NAT type) specified on the
>> logical router of options:router-port.
>> Each load balancer IP address specified on the logical router of
>> options:router-port.
>> This will cause the controller where the gateway router resides to
>> issue gratuitous ARPs for each NAT external IP address and for each
>> load balancer IP address specified on the gateway router.
>>
>> This patch is written as if it will be included in OVS 2.7.  If it
>> is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml
>> will need to be updated from OVS 2.7 to OVS 2.8.
>>
>> Signed-off-by: Mickey Spiegel 
>> ---
>>  ovn/northd/ovn-northd.c | 114 ++
>> --
>>  ovn/ovn-nb.xml  |  42 +++---
>>  tests/ovn.at|  60 +
>>  3 files changed, 185 insertions(+), 31 deletions(-)
>>
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index e24ff8f..efb8a74 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx,
>>  }
>>
>>  static void
>> +ip_address_and_port_from_lb_key(const char *key, char **ip_address,
>> +uint16_t *port);
>> +
>> +static void
>> +get_router_load_balancer_ips(const struct ovn_datapath *od,
>> + struct sset *all_ips)
>> +{
>> +if (!od->nbr) {
>> +return;
>> +}
>> +
>> +for (int i = 0; i < od->nbr->n_load_balancer; i++) {
>> +struct nbrec_load_balancer *lb = od->nbr->load_balancer[i];
>> +struct smap *vips = &lb->vips;
>> +struct smap_node *node;
>> +
>> +SMAP_FOR_EACH (node, vips) {
>> +/* node->key contains IP:port or just IP. */
>> +char *ip_address = NULL;
>> +uint16_t port;
>> +
>> +ip_address_and_port_from_lb_key(node->key, &ip_address,
>> &port);
>> +if (!ip_address) {
>> +continue;
>> +}
>> +
>> +if (!sset_contains(all_ips, ip_address)) {
>> +sset_add(all_ips, ip_address);
>> +}
>> +
>> +free(ip_address);
>> +}
>> +}
>> +}
>> +
>> +/* Returns a string consisting of the port's MAC address followed by the
>> + * external IP addresses of all NAT rules defined on that router and the
>> + * VIPs of all load balancers defined on that router. */
>>
> A comment that the called has to free the returned string is useful.
>

I can add that.


>
> Also, the load balancer IP address can also be the IP address of the
> router itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a
> problem when a GARP is sent for the router IP too.
>

If the router IP is used as a load balancer IP address or as a SNAT
address, then sending GARP for the router IP is a good thing.

Mickey


>
> Acked-by: Gurucharan Shetty 
>
>
>
>
>> +static char *
>> +get_nat_addresses(const struct ovn_port *op)
>> +{
>> +struct eth_addr mac;
>> +if (!op->nbrp || !op->od || !op->od->nbr
>> +|| (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer)
>> +|| !eth_addr_from_string(op->nbrp->mac, &mac)) {
>> +return NULL;
>> +}
>> +
>> +struct ds addresses = DS_EMPTY_INITIALIZER;
>> +ds_put_format(&addresses, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
>> +
>> +/* Get NAT IP addresses. */
>> +for (int i = 0; i < op->od->nbr->n_nat; i++) {
>> +const struct nbrec_nat *nat;
>> +nat = op->od->nbr->nat[i];
>> +
>> +ovs_be32 ip, mask;
>> +
>> +char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
>> +if (error || mask != OVS_BE32_MAX) {
>> +free(error);
>> +continue;
>> +}
>> +ds_put_format(&addresses, " %s", nat->external_ip);
>> +}
>> +
>> +/* A set to hold all load-balancer vips. */
>> +struct sset all_ips = SSET_INITIALIZER(&all_ips);
>> +get_router_load_balancer_ips(op->od, &all_ips);
>> +
>> +const char *ip_address;
>> +SSET_FOR_EACH(ip_address, &all_ips) {
>> +ds_put_format(&addresses, " %s", ip_address);
>> +}

Re: [ovs-dev] [PATCH v12 6/6] ovn: specify options:nat-addresses as "router"

[Guru Shetty]

On 26 January 2017 at 01:20, Mickey Spiegel  wrote:

> Currently in OVN, the "nat-addresses" in the "options" column of a
> logical switch port of type "router" must be specified manually.
> Typically the user would specify as "nat-addresses" all of the NAT
> external IP addresses and load balancer IP addresses that have
> already been specified separately on the router.
>
> This patch allows the logical switch port's "nat-addresses" to be
> specified as the string "router".  When ovn-northd sees this string,
> it automatically copies the following into the southbound
> Port_Binding's "nat-addresses" in the "options" column:
> The options:router-port's MAC address.
> Each NAT external IP address (of any NAT type) specified on the
> logical router of options:router-port.
> Each load balancer IP address specified on the logical router of
> options:router-port.
> This will cause the controller where the gateway router resides to
> issue gratuitous ARPs for each NAT external IP address and for each
> load balancer IP address specified on the gateway router.
>
> This patch is written as if it will be included in OVS 2.7.  If it
> is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml
> will need to be updated from OVS 2.7 to OVS 2.8.
>
> Signed-off-by: Mickey Spiegel 
> ---
>  ovn/northd/ovn-northd.c | 114 ++
> --
>  ovn/ovn-nb.xml  |  42 +++---
>  tests/ovn.at|  60 +
>  3 files changed, 185 insertions(+), 31 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index e24ff8f..efb8a74 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx,
>  }
>
>  static void
> +ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> +uint16_t *port);
> +
> +static void
> +get_router_load_balancer_ips(const struct ovn_datapath *od,
> + struct sset *all_ips)
> +{
> +if (!od->nbr) {
> +return;
> +}
> +
> +for (int i = 0; i < od->nbr->n_load_balancer; i++) {
> +struct nbrec_load_balancer *lb = od->nbr->load_balancer[i];
> +struct smap *vips = &lb->vips;
> +struct smap_node *node;
> +
> +SMAP_FOR_EACH (node, vips) {
> +/* node->key contains IP:port or just IP. */
> +char *ip_address = NULL;
> +uint16_t port;
> +
> +ip_address_and_port_from_lb_key(node->key, &ip_address,
> &port);
> +if (!ip_address) {
> +continue;
> +}
> +
> +if (!sset_contains(all_ips, ip_address)) {
> +sset_add(all_ips, ip_address);
> +}
> +
> +free(ip_address);
> +}
> +}
> +}
> +
> +/* Returns a string consisting of the port's MAC address followed by the
> + * external IP addresses of all NAT rules defined on that router and the
> + * VIPs of all load balancers defined on that router. */
>
A comment that the called has to free the returned string is useful.

Also, the load balancer IP address can also be the IP address of the router
itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a problem
when a GARP is sent for the router IP too.

Acked-by: Gurucharan Shetty 




> +static char *
> +get_nat_addresses(const struct ovn_port *op)
> +{
> +struct eth_addr mac;
> +if (!op->nbrp || !op->od || !op->od->nbr
> +|| (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer)
> +|| !eth_addr_from_string(op->nbrp->mac, &mac)) {
> +return NULL;
> +}
> +
> +struct ds addresses = DS_EMPTY_INITIALIZER;
> +ds_put_format(&addresses, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
> +
> +/* Get NAT IP addresses. */
> +for (int i = 0; i < op->od->nbr->n_nat; i++) {
> +const struct nbrec_nat *nat;
> +nat = op->od->nbr->nat[i];
> +
> +ovs_be32 ip, mask;
> +
> +char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
> +if (error || mask != OVS_BE32_MAX) {
> +free(error);
> +continue;
> +}
> +ds_put_format(&addresses, " %s", nat->external_ip);
> +}
> +
> +/* A set to hold all load-balancer vips. */
> +struct sset all_ips = SSET_INITIALIZER(&all_ips);
> +get_router_load_balancer_ips(op->od, &all_ips);
> +
> +const char *ip_address;
> +SSET_FOR_EACH(ip_address, &all_ips) {
> +ds_put_format(&addresses, " %s", ip_address);
> +}
> +sset_destroy(&all_ips);
> +
> +return ds_steal_cstr(&addresses);
> +}
> +
> +static void
>  ovn_port_update_sbrec(const struct ovn_port *op,
>struct hmap *chassis_qdisc_queues)
>  {
> @@ -1524,7 +1604,15 @@ ovn_port_update_sbrec(const struct ovn_port *op,
>
>  const char *nat_addresses = smap_get(&op->nbsp->options,
>   

Re: [ovs-dev] assertion failing in commit_set_ipv4_action(), flow->nw_proto != base_flow->nw_proto

[Thomas Morin]

Hi Jarno, all,

Sorry for being slow to react.
I'd be glad to test a fix, but I'm unclear if there is one ready to be 
tested.


Perhasp "xlate: Recirculate also when MPLS POP is implicit." but I'm not 
sure there has been a follow-up to the discuss between you and Ben?
Or is the new clone action possibly taking care of this issue (does not 
seem so, since datapath dependent, but I might have misunderstood)?


-Thomas

2017-01-06, Jarno Rajahalme:
I sent a patch to do this fix on OVS master. IMO we should also make 
the datapath more flexible so that it would be able to POP back to IP 
after PUSH actions on an IP packet in the same action list. But that 
will not make it to OVS 2.7.


I would appreciate if Thomas could apply the and test!

  Jarno

On Dec 14, 2016, at 1:52 PM, Jarno Rajahalme > wrote:




On Dec 13, 2016, at 8:44 PM, Takashi YAMAMOTO > wrote:




On Wed, Dec 14, 2016 at 9:02 AM, Jarno Rajahalme>wrote:




On Dec 13, 2016, at 3:32 PM, Takashi YAMAMOTO mailto:yamam...@ovn.org>> wrote:



On Tue, Dec 13, 2016 at 6:49 PM, Thomas
Morinmailto:thomas.mo...@orange.com>>wrote:

Hi Jarno,

2016-12-10, Jarno Rajahalme:

On Dec 9, 2016, at 7:04 AM, Thomas Morin
mailto:thomas.mo...@orange.com>>
wrote:


2016-12-09, Thomas Morin:

In the same setup as the one on which the bug was
observed, [...]


I was confused, I in fact tested the patch (branch-2.5)
on a /different/ setup as the one on which we hit the
bug, using MPLS over a GRE tunnel port, rather than plain
MPLS over an eth port.
Sorry if any confusion arised... I can test on the first
setup if relevant.



Maybe the kernel datapath does not support MPLS over a GRE
tunnel port. Having ‘dmesg’ output for the test run might
reveal why the actions validation fails.


The dmesg output was the following:

[171295.258939] openvswitch: netlink: Flow actions may not
be safe on all matching packets.

I've tested the patch on the platform on which the bug was
initially hit (*not* using MPLS/GRE), and I have the
following a few times in the logs right after I do an
"ovs-appctl fdb/flush":

2016-12-13T09:44:08.449Z|1|dpif(handler68)|WARN|Dropped
3 log messages in last 1 seconds (most recently, 1 seconds
ago) due to excessive rate
2016-12-13T09:44:08.449Z|2|dpif(handler68)|WARN|system@ovs-system:
failed to put[create] (Invalid argument)
ufid:f046c4c4-b97f-436d-bd7c-91ed307275ac

recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(9),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64),eth_type(0x0800),ipv4(src=10.0.1.29,dst=10.0.0.3,proto=6,tos=0/0xfc,ttl=64,frag=no),tcp(src=54253,dst=8080),tcp_flags(0/0),

actions:set(ipv4(src=10.0.1.29,dst=10.0.0.3,ttl=63)),set(eth(src=b8:2a:72:de:1b:e3,dst=00:17:cb:79:2c:01)),push_mpls(label=433680,tc=0,ttl=63,bos=1,eth_type=0x8847),7,set(eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64)),pop_mpls(eth_type=0x800),set(ipv4(src=10.0.1.29,dst=10.0.0.3,tos=0/0xfc,ttl=64)),push_vlan(vid=1,pcp=0),3,8,pop_vlan,13

And dmesg:
[926833.612372] openvswitch: netlink: Flow actions may not
be safe on all matching packets.


it's complaining about set ipv4 after pop_mpls because it
doesn't know about the "restored" l3.
while we can improve the kernel, i guess we need to stick with
recirc for now.


This should do it? Does not break any existing tests on
branch-2.5, but I did not create a test for this yet.

diff --git a/ofproto/ofproto-dpif-xlate.c
b/ofproto/ofproto-dpif-xlate.c
index fb25f63..6ee2a72 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2899,6 +2899,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
 {
 bool use_masked = ctx->xbridge->support.masked_set_action;
+/* An MPLS packet can be implicitly popped back to a
non-MPLS packet, if a
+ * patch port peer or a group bucket pushed MPLS.  Set the
'was_mpls' flag
+ * also in that case. */
+if (eth_type_mpls(ctx->base_flow.dl_type)
+&& !eth_type_mpls(ctx->xin->flow.dl_type)
+&& ctx->xbridge->support.odp.recirc) {
+ctx->was_mpls = true;
+}
+


i guess this check needs to be somewhere around "ctx->was_mpls = 
old_was_mpls" in

affected functions.



Right, as that is where the implicit MPLS POP action happens, when 
the ‘ctx->xin->flow’ is restored.


  Jarno


 ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow,
&ctx->base_flow,
 ctx->odp_actions, ctx->wc,
 use_masked);

Jarno





-Thomas







On Dec 1, 2016, at 

[ovs-dev] Supply Chain Management

[Amelia Harper]

Hi,



Would you be interested in an email lead list of Supply Chain Executives? We 
can help you reach out to key Top decision makers like:



Title includes:

*   VP of Supply Chain

*   Director of Supply Chain

*   Supply Chain Manager

*   Purchasing Manager

*   Purchasing Director

*   Procurement Manager

*   Maintenance Managers/ Engineers

*   Procurement Director



The list comes with complete contact information like: Contact name, Email 
address, Title, Company name, Phone number, Mailing address, etc.



I'd be happy to send over few sample records on your request, and set up a time 
to discuss in detail.



If there is someone else in your organization that I need to speak with, I'd be 
grateful if you would forward this email to the appropriate contact and help me 
with the introduction.



Thanks for your time.



Have a great day!



Warm regards,



Amelia Harper| INF Solutions | 302.250.4336

If this email is irrelevant to you, please help us keep our lists clean by 
replying to this email with UNSUBSCRIBE as the subject.

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

[ovs-dev] [PATCH] rhel: Add missing unpackaged file 'ovs-fields.7.gz' in the %files list

[Numan Siddique]

Fixes: commit 96fee5e0a2a0 ("ovs-fields: New manpage to document Open
vSwitch and OpenFlow fields")

Signed-off-by: Numan Siddique 
---
 rhel/openvswitch-fedora.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 3499d63..140c575 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -472,6 +472,7 @@ fi
 %{_mandir}/man1/ovsdb-tool.1*
 %{_mandir}/man5/ovs-vswitchd.conf.db.5*
 %{_mandir}/man5/vtep.5*
+%{_mandir}/man7/ovs-fields.7.*
 %{_mandir}/man8/vtep-ctl.8*
 %{_mandir}/man8/ovs-appctl.8*
 %{_mandir}/man8/ovs-bugtool.8*
-- 
2.9.3

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

[ovs-dev] WITH RESPECT FROM MISS ELENA

[Elena Nana via dev]

WITH RESPECT FROM MISS ELENA

I am Elena Nana. My parents Mr.and Mrs.Jerry Nana were assassinated here in 
IVORY COAST.  Before my father's death he had US$12.5M (TWELVE MILLION FIVE 
HUNDRED THOUSAND UNITED STATES DOLLARS) deposited in a bank here in Abidjan.

I want you to do me a favour to receive these funds to a safe account in your 
country or any safer place as the beneficiary. I want to come over to your 
country for the safety of my life from the hands of this wicked assasins.

I have plans to do investment in your country, like real estate and industrial 
production. This is my reason for writing to you and I am willing to offer you 
20% out of the total funds for assisting me, Please if you are willing to 
assist me, reply me with your direct phone number.

Sincerely yours,

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

Re: [ovs-dev] [PATCH v2] dpif-netdev: Conditional EMC insert

[Jan Scheurich]

Hi Ciara,

On 2017-01-26 18:51, Ciara Loftus wrote:
  
+uint32_t em_flow_insert_min = UINT32_MAX / 100;

+


Have you measured if it is better to store em_flow_insert_min as global 
variable than to include it per PMD in struct dp_netdev_pmd_thread?


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

Re: [ovs-dev] [PATCH] windows: Allow clients to read from server before disconnect

[Sairam Venugopal]

Good catch. We should probably do this for nl_sock_destroy too - 
https://github.com/openvswitch/ovs/blob/75e2077e0c43224bcca92746b28b01a4936fc101/lib/netlink-socket.c#L252

Acked-by: Sairam Venugopal 


Thanks,
Sairam



On 1/26/17, 11:12 AM, "ovs-dev-boun...@openvswitch.org on behalf of Alin 
Serdean"  wrote:

>Wait for clients to read from the pipe before disconnecting the server.
>
>Found while testing.
>
>Signed-off-by: Alin Gabriel Serdean 
>---
>Intended for master and branch-2.7
>---
> lib/stream-windows.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/lib/stream-windows.c b/lib/stream-windows.c
>index 637920b..1950014 100644
>--- a/lib/stream-windows.c
>+++ b/lib/stream-windows.c
>@@ -183,6 +183,9 @@ windows_close(struct stream *stream)
> /* Disconnect the named pipe in case it was created from a passive stream.
>  */
> if (s->server) {
>+/* Flush the pipe to allow the client to read the pipe's contents
>+ * before disconnecting. */
>+FlushFileBuffers(s->fd);
> DisconnectNamedPipe(s->fd);
> }
> CloseHandle(s->fd);
>-- 
>2.10.2.windows.1
>___
>dev mailing list
>d...@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=B91GBdYFjSO8Y5kwBvlStjj3QQ9jkCAtkZoyUCq0AOo&s=62rsfH2_7v8BrFzdR4Q_u2frBjxeHDtqw1_UbqIQvVg&e=
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Windows: Fix wmi.c to use count of wchar_t instead of sizeof

[Alin Serdean]

Thanks for addressing the comment!
Acked-by: Alin Gabriel Serdean 

Alin.
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Sairam Venugopal
> Sent: Friday, January 27, 2017 9:56 AM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v2] Windows: Fix wmi.c to use count of wchar_t
> instead of sizeof
> 
> wcscat_s and wcscpy_s requires number of elements as argument. wchar_t
> uses 2 bytes for storage and using sizeof(internal_port_query) causes access
> violation error on Windows 2012 R2 (64 bit). This patch introduces a #define
> WMI_QUERY_COUNT set to 2048 and uses that instead.
> 
> Signed-off-by: Sairam Venugopal 
> Reported-by: Sairam Venugopal 
> Reported-at: openvswitch/ovs-issues#121
> ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 1/4] datapath-windows: OvsUpdateIPv4Header remove unnecessary addition

[Sairam Venugopal]

Re-Acking - Acked-by: Sairam Venugopal 





On 1/26/17, 3:38 PM, "ovs-dev-boun...@openvswitch.org on behalf of Alin 
Serdean"  wrote:

>From: Alin Serdean 
>
>bufferStart can be used directly to access the data of the net buffer.
>Add the MDL offset to save unnecessary additions.
>
>Signed-off-by: Alin Gabriel Serdean 
>Acked-by: Sairam Venugopal 
>---
>v3: add acked
>v2: no change
>---
> datapath-windows/ovsext/Actions.c | 8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c 
>b/datapath-windows/ovsext/Actions.c
>index b4a286b..760888e 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1390,13 +1390,13 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
> mdlLen -= curMdlOffset;
> ASSERT(mdlLen >= hdrSize);
> }
>-
>-ipHdr = (IPHdr *)(bufferStart + curMdlOffset + layers->l3Offset);
>+bufferStart += curMdlOffset;
>+ipHdr = (IPHdr *)(bufferStart + layers->l3Offset);
> 
> if (layers->isTcp) {
>-tcpHdr = (TCPHdr *)(bufferStart + curMdlOffset + layers->l4Offset);
>+tcpHdr = (TCPHdr *)(bufferStart + layers->l4Offset);
> } else if (layers->isUdp) {
>-udpHdr = (UDPHdr *)(bufferStart + curMdlOffset + layers->l4Offset);
>+udpHdr = (UDPHdr *)(bufferStart + layers->l4Offset);
> }
> 
> /*
>-- 
>2.10.2.windows.1
>___
>dev mailing list
>d...@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 2/4] datapath-windows: Add function to get continuous buffer from context

[Sairam Venugopal]

Re-acking - Acked-by: Sairam Venugopal 





On 1/26/17, 3:45 PM, "ovs-dev-boun...@openvswitch.org on behalf of Alin 
Serdean"  wrote:

>From: Alin Serdean 
>
>This patch extracts the code that tries to get a continuous IPv4 header
>buffer from the function 'OvsUpdateIPv4Header' and moves it to a new
>function 'OvsGetHeaderBySize'.
>
>The new function can be used later when trying to change the UDP/TCP/MPLS
>etc., headers.
>
>Signed-off-by: Alin Gabriel Serdean 
>Acked-by: Sairam Venugopal 
>---
>v3: add acked
>v2: no change
>---
> datapath-windows/ovsext/Actions.c | 100 +++---
> 1 file changed, 62 insertions(+), 38 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c 
>b/datapath-windows/ovsext/Actions.c
>index 760888e..467bfbc 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1306,64 +1306,51 @@ OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
> return NDIS_STATUS_SUCCESS;
> }
> 
>+
> /*
>  *
>- * OvsUpdateIPv4Header --
>- *  Updates the IPv4 header in ovsFwdCtx.curNbl inline based on the
>- *  specified key.
>+ * OvsGetHeaderBySize --
>+ *  Tries to retrieve a continuous buffer from 'ovsFwdCtx->curnbl' of size
>+ *  'size'.
>+ *  If the original buffer is insufficient it will, try to clone the net
>+ *  buffer list and force the size.
>+ *  Returns 'NULL' on failure or a pointer to the first byte of the data
>+ *  in the first net buffer of the net buffer list 'nbl'.
>  *
>  */
>-static __inline NDIS_STATUS
>-OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
>-const struct ovs_key_ipv4 *ipAttr)
>+PUINT8 OvsGetHeaderBySize(OvsForwardingContext *ovsFwdCtx,
>+  UINT32 size)
> {
> PNET_BUFFER curNb;
>+UINT32 mdlLen, packetLen;
> PMDL curMdl;
> ULONG curMdlOffset;
>-PUINT8 bufferStart;
>-UINT32 mdlLen, hdrSize, packetLen;
>-OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>-NDIS_STATUS status;
>-IPHdr *ipHdr;
>-TCPHdr *tcpHdr = NULL;
>-UDPHdr *udpHdr = NULL;
>-
>-ASSERT(layers->value != 0);
>+PUINT8 start;
> 
>-/*
>- * Peek into the MDL to get a handle to the IP header and if required
>- * the TCP/UDP header as well. We check if the required headers are in one
>- * contiguous MDL, and if not, we copy them over to one MDL.
>- */
> curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
> ASSERT(curNb->Next == NULL);
> packetLen = NET_BUFFER_DATA_LENGTH(curNb);
> curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>-NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>-if (!bufferStart) {
>+NdisQueryMdl(curMdl, &start, &mdlLen, LowPagePriority);
>+if (!start) {
> ovsActionStats.noResource++;
>-return NDIS_STATUS_RESOURCES;
>+return NULL;
> }
>+
> curMdlOffset = NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
> mdlLen -= curMdlOffset;
> ASSERT((INT)mdlLen >= 0);
> 
>-if (layers->isTcp || layers->isUdp) {
>-hdrSize = layers->l4Offset +
>-  layers->isTcp ? sizeof (*tcpHdr) : sizeof (*udpHdr);
>-} else {
>-hdrSize = layers->l3Offset + sizeof (*ipHdr);
>-}
>-
> /* Count of number of bytes of valid data there are in the first MDL. */
> mdlLen = MIN(packetLen, mdlLen);
>-if (mdlLen < hdrSize) {
>+if (mdlLen < size) {
> PNET_BUFFER_LIST newNbl;
>+NDIS_STATUS status;
> newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext, 
> ovsFwdCtx->curNbl,
>-   hdrSize, 0, TRUE /*copy NBL info*/);
>+   size, 0, TRUE /*copy NBL info*/);
> if (!newNbl) {
> ovsActionStats.noCopiedNbl++;
>-return NDIS_STATUS_RESOURCES;
>+return NULL;
> }
> OvsCompleteNBLForwardingCtx(ovsFwdCtx,
> L"Complete after partial copy.");
>@@ -1372,25 +1359,62 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
>   newNbl, ovsFwdCtx->srcVportNo, 0,
>   
> NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>   NULL, &ovsFwdCtx->layers, FALSE);
>+
> if (status != NDIS_STATUS_SUCCESS) {
> OvsCompleteNBLForwardingCtx(ovsFwdCtx,
> L"OVS-Dropped due to resources");
>-return NDIS_STATUS_RESOURCES;
>+return NULL;
> }
> 
> curNb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl);
> ASSERT(curNb->Next == NULL);
> curMdl = NET_BUFFER_CURRENT_MDL(curNb);
>-NdisQueryMdl(curMdl, &bufferStart, &mdlLen, LowPagePriority);
>+NdisQueryMdl(curMdl, &start, &mdlLen,

Re: [ovs-dev] [PATCH v3 3/4] datapath-windows: Add support for OVS_KEY_ATTR_UDP set action

[Sairam Venugopal]

Acked-by: Sairam Venugopal 







On 1/26/17, 3:41 PM, "ovs-dev-boun...@openvswitch.org on behalf of Alin 
Serdean"  wrote:

>From: Alin Serdean 
>
>This patch adds support for set action with OVS_KEY_ATTR_UDP attribute
>(change UDP source or destination port).
>
>If the source or destination UDP port was changed, update the UDP checksum.
>
>A sample flow can look like the following:
>set(udp(src=67,dst=68))
>
>Signed-off-by: Alin Gabriel Serdean 
>---
>v3: check if udp checksum is 0
>v2: no change
>---
> datapath-windows/ovsext/Actions.c | 52 ++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c 
>b/datapath-windows/ovsext/Actions.c
>index 467bfbc..3dfcc68 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1306,7 +1306,6 @@ OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
> return NDIS_STATUS_SUCCESS;
> }
> 
>-
> /*
>  *
>  * OvsGetHeaderBySize --
>@@ -1382,6 +1381,52 @@ PUINT8 OvsGetHeaderBySize(OvsForwardingContext 
>*ovsFwdCtx,
> return start + curMdlOffset;
> }
> 
>+/*
>+ *
>+ * OvsUpdateUdpPorts --
>+ *  Updates the UDP source or destination port in ovsFwdCtx.curNbl inline
>+ *  based on the specified key.
>+ *
>+ */
>+static __inline NDIS_STATUS
>+OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
>+  const struct ovs_key_udp *udpAttr)
>+{
>+PUINT8 bufferStart;
>+OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+UDPHdr *udpHdr = NULL;
>+
>+ASSERT(layers->value != 0);
>+
>+if (!layers->isUdp) {
>+ovsActionStats.noCopiedNbl++;
>+return NDIS_STATUS_FAILURE;
>+}
>+
>+bufferStart = OvsGetHeaderBySize(ovsFwdCtx, layers->l7Offset);
>+if (!bufferStart) {
>+return NDIS_STATUS_RESOURCES;
>+}
>+
>+udpHdr = (UDPHdr *)(bufferStart + layers->l4Offset);
>+if (udpHdr->check) {
>+if (udpHdr->source != udpAttr->udp_src) {
>+udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->source,
>+ udpAttr->udp_src);
>+udpHdr->source = udpAttr->udp_src;
>+}
>+if (udpHdr->dest != udpAttr->udp_dst) {
>+udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->dest,
>+ udpAttr->udp_dst);
>+udpHdr->dest = udpAttr->udp_dst;
>+}
>+} else {
>+udpHdr->source = udpAttr->udp_src;
>+udpHdr->dest = udpAttr->udp_dst;
>+}
>+
>+return NDIS_STATUS_SUCCESS;
>+}
> 
> /*
>  *
>@@ -1525,6 +1570,11 @@ OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,
> break;
> }
> 
>+case OVS_KEY_ATTR_UDP:
>+status = OvsUpdateUdpPorts(ovsFwdCtx,
>+NlAttrGetUnspec(a, sizeof(struct ovs_key_udp)));
>+break;
>+
> default:
> OVS_LOG_INFO("Unhandled attribute %#x", type);
> break;
>-- 
>2.10.2.windows.1
>___
>dev mailing list
>d...@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 4/4] datapath-windows: Add support for OVS_KEY_ATTR_TCP set action

[Sairam Venugopal]

Acked-by: Sairam Venugopal 







On 1/26/17, 3:43 PM, "ovs-dev-boun...@openvswitch.org on behalf of Alin 
Serdean"  wrote:

>From: Alin Serdean 
>
>This patch adds support for set action with OVS_KEY_ATTR_TCP attribute
>(change TCP source or destination port).
>
>If the source or destination TCP port was changed, update the TCP checksum.
>
>A sample flow can look like the following:
>set(tcp(src=80,dst=443))
>
>Signed-off-by: Alin Gabriel Serdean 
>---
>v3: check for layers->isTcp as suggested by Sairam Venugopal 
>
>v2: set action for tcp attribute changed to function: OvsUpdateTcpPorts
>---
> datapath-windows/ovsext/Actions.c | 48 +++
> 1 file changed, 48 insertions(+)
>
>diff --git a/datapath-windows/ovsext/Actions.c 
>b/datapath-windows/ovsext/Actions.c
>index 3dfcc68..bce37f8 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1430,6 +1430,49 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
> 
> /*
>  *
>+ * OvsUpdateTcpPorts --
>+ *  Updates the TCP source or destination port in ovsFwdCtx.curNbl inline
>+ *  based on the specified key.
>+ *
>+ */
>+static __inline NDIS_STATUS
>+OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
>+  const struct ovs_key_tcp *tcpAttr)
>+{
>+PUINT8 bufferStart;
>+OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+TCPHdr *tcpHdr = NULL;
>+
>+ASSERT(layers->value != 0);
>+
>+if (!layers->isTcp) {
>+ovsActionStats.noCopiedNbl++;
>+return NDIS_STATUS_FAILURE;
>+}
>+
>+bufferStart = OvsGetHeaderBySize(ovsFwdCtx, layers->l7Offset);
>+if (!bufferStart) {
>+return NDIS_STATUS_RESOURCES;
>+}
>+
>+tcpHdr = (TCPHdr *)(bufferStart + layers->l4Offset);
>+
>+if (tcpHdr->source != tcpAttr->tcp_src) {
>+tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
>+ tcpAttr->tcp_src);
>+tcpHdr->source = tcpAttr->tcp_src;
>+}
>+if (tcpHdr->dest != tcpAttr->tcp_dst) {
>+tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
>+ tcpAttr->tcp_dst);
>+tcpHdr->dest = tcpAttr->tcp_dst;
>+}
>+
>+return NDIS_STATUS_SUCCESS;
>+}
>+
>+/*
>+ *
>  * OvsUpdateIPv4Header --
>  *  Updates the IPv4 header in ovsFwdCtx.curNbl inline based on the
>  *  specified key.
>@@ -1575,6 +1618,11 @@ OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,
> NlAttrGetUnspec(a, sizeof(struct ovs_key_udp)));
> break;
> 
>+case OVS_KEY_ATTR_TCP:
>+status = OvsUpdateTcpPorts(ovsFwdCtx,
>+NlAttrGetUnspec(a, sizeof(struct ovs_key_tcp)));
>+break;
>+
> default:
> OVS_LOG_INFO("Unhandled attribute %#x", type);
> break;
>-- 
>2.10.2.windows.1
>___
>dev mailing list
>d...@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev