Re: [ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Han Zhou]

On Fri, Feb 22, 2019 at 8:36 PM Ben Pfaff  wrote:
>
> On Fri, Feb 22, 2019 at 07:31:59PM -0800, Han Zhou wrote:
> > On Fri, Feb 22, 2019 at 5:27 PM Ben Pfaff  wrote:
> > >
> > > On Fri, Feb 22, 2019 at 05:21:51PM -0800, Han Zhou wrote:
> > > > On Fri, Feb 22, 2019 at 3:04 PM Ben Pfaff  wrote:
> > > > >
> > > > > Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can 
> > > > > have
> > > > > more than one item with a given key.  Nothing prevents that, and it 
> > > > > will
> > > > > otherwise work, but it is unusual and it normally makes sense to use 
> > > > > an
> > > > > hindex instead of an hmap for efficiency's sake if it is going to 
> > > > > happen
> > > > > very often.  Does the data structure in question often have 
> > > > > duplicates?
> > > > > Should we change it to an hindex?
> > > >
> > > > Hi Ben, I think you are asking if there can be more than one item with
> > > > a given "hash value". Yes it is possible, but it should not be often
> > > > for the data structure in question here - the json cache. The key is
> > > > uuid of the change set combined with monitor version, and the hash is
> > > > the uuid_hash. So conflicts of uuid_hash should be rare. When there
> > > > are different versions (e.g. monitor v1, v2, v3) of clients referring
> > > > same change sets we do get more items with same hash value, but there
> > > > were at most 2 versions and I am adding 1 more in next patches in the
> > > > series, and I guess it is not likely we add too many versions of
> > > > monitors in the future, and even that is possible, it is unlikely that
> > > > many different versions of clients co-exists in same environment. So I
> > > > don't think it has a real impact to performance.
> > > >
> > > > The reason for using HMAP_FOR_EACH_SAFE_WITH_HASH instead of
> > > > HMAP_FOR_EACH_WITH_HASH is just because the node is going to be
> > > > removed and freed in the loop. It needs to be safe regardless of the
> > > > probability of hash conflicts.
> > >
> > > It's important to be clear about the difference between hashes and keys.
> > > The hash is what's stored in the hmap_node (that is, the uuid_hash()
> > > return value in this case), the key in this case is what uuid_equals()
> > > compares.  I *think* you are saying there can be a small number of
> > > duplicate keys in the hmap in this case.  Is that correct?
> >
> > I thought you were talking about hashes instead of keys (I thought it
> > was a typo), because I assume keys are always unique.
> > Let me clarify a little more. In the json_cache hmap, the key is the
> > combination of  of the struct
> > ovsdb_monitor_json_cache_node. This key uniquely identifies each
> > element in the hmap. The hash function doesn't take version into the
> > hash calculation, so if there are multiple versions of clients exist
> > at the same time, there will be different elements with same hash.
> > However, since the number of versions should be very small (worse case
> > 3, most cases 1), this is not going to cause any performance problem.
> > There do exist probability of hash conflict even if there is only one
> > version, but the chances are very low (ensured by uuid_hash).
> >
> > Now for the function:
> > +/* Free all versions of json cache for a given change_set.*/
> > +static void
> > +ovsdb_monitor_json_cache_destroy(struct ovsdb_monitor *dbmon,
> > + struct ovsdb_monitor_change_set 
> > *change_set)
> >
> > It is different from a general function that destroys an element in a
> > hmap with the unique key. Instead, it destroys *all versions* of the
> > json cache for a given change_set uuid. So it uses only part of the
> > key to do the search, which may have caused some confusion.
>
> Ah.  This is an unusual combination of factors.  Very unusual in fact.
> If it is something we really want, then I think that it should be
> carefully documented in a comment on the data structure.

I agree. This trick introduces confusion without obvious benefit, and
the HMAP_FOR_EACH_SAFE_WITH_HASH is not really necessary. Instead of
playing tricks with the keys, I can simply use the search function to
find the nodes for all versions and destroy them if exists. Please see
below change on top of the current one. I will submit it as v3 if it
looks better.

--8><-><8
diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index e4fc9a4..8aea9c2 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -144,15 +144,6 @@ struct hmap_node *hmap_random_node(const struct hmap *);
  (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
  ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))

-/* Safe when NODE may be freed (not needed when NODE may be removed from the
- * hash map but its members remain accessible and intact). */
-#define HMAP_FOR_EACH_SAFE_WITH_HASH(NODE, NEXT, MEMBER, HASH, HMAP) \
-for 

Re: [ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Ben Pfaff]

On Fri, Feb 22, 2019 at 07:31:59PM -0800, Han Zhou wrote:
> On Fri, Feb 22, 2019 at 5:27 PM Ben Pfaff  wrote:
> >
> > On Fri, Feb 22, 2019 at 05:21:51PM -0800, Han Zhou wrote:
> > > On Fri, Feb 22, 2019 at 3:04 PM Ben Pfaff  wrote:
> > > >
> > > > Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can have
> > > > more than one item with a given key.  Nothing prevents that, and it will
> > > > otherwise work, but it is unusual and it normally makes sense to use an
> > > > hindex instead of an hmap for efficiency's sake if it is going to happen
> > > > very often.  Does the data structure in question often have duplicates?
> > > > Should we change it to an hindex?
> > >
> > > Hi Ben, I think you are asking if there can be more than one item with
> > > a given "hash value". Yes it is possible, but it should not be often
> > > for the data structure in question here - the json cache. The key is
> > > uuid of the change set combined with monitor version, and the hash is
> > > the uuid_hash. So conflicts of uuid_hash should be rare. When there
> > > are different versions (e.g. monitor v1, v2, v3) of clients referring
> > > same change sets we do get more items with same hash value, but there
> > > were at most 2 versions and I am adding 1 more in next patches in the
> > > series, and I guess it is not likely we add too many versions of
> > > monitors in the future, and even that is possible, it is unlikely that
> > > many different versions of clients co-exists in same environment. So I
> > > don't think it has a real impact to performance.
> > >
> > > The reason for using HMAP_FOR_EACH_SAFE_WITH_HASH instead of
> > > HMAP_FOR_EACH_WITH_HASH is just because the node is going to be
> > > removed and freed in the loop. It needs to be safe regardless of the
> > > probability of hash conflicts.
> >
> > It's important to be clear about the difference between hashes and keys.
> > The hash is what's stored in the hmap_node (that is, the uuid_hash()
> > return value in this case), the key in this case is what uuid_equals()
> > compares.  I *think* you are saying there can be a small number of
> > duplicate keys in the hmap in this case.  Is that correct?
> 
> I thought you were talking about hashes instead of keys (I thought it
> was a typo), because I assume keys are always unique.
> Let me clarify a little more. In the json_cache hmap, the key is the
> combination of  of the struct
> ovsdb_monitor_json_cache_node. This key uniquely identifies each
> element in the hmap. The hash function doesn't take version into the
> hash calculation, so if there are multiple versions of clients exist
> at the same time, there will be different elements with same hash.
> However, since the number of versions should be very small (worse case
> 3, most cases 1), this is not going to cause any performance problem.
> There do exist probability of hash conflict even if there is only one
> version, but the chances are very low (ensured by uuid_hash).
> 
> Now for the function:
> +/* Free all versions of json cache for a given change_set.*/
> +static void
> +ovsdb_monitor_json_cache_destroy(struct ovsdb_monitor *dbmon,
> + struct ovsdb_monitor_change_set *change_set)
> 
> It is different from a general function that destroys an element in a
> hmap with the unique key. Instead, it destroys *all versions* of the
> json cache for a given change_set uuid. So it uses only part of the
> key to do the search, which may have caused some confusion.

Ah.  This is an unusual combination of factors.  Very unusual in fact.
If it is something we really want, then I think that it should be
carefully documented in a comment on the data structure.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Han Zhou]

On Fri, Feb 22, 2019 at 5:27 PM Ben Pfaff  wrote:
>
> On Fri, Feb 22, 2019 at 05:21:51PM -0800, Han Zhou wrote:
> > On Fri, Feb 22, 2019 at 3:04 PM Ben Pfaff  wrote:
> > >
> > > Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can have
> > > more than one item with a given key.  Nothing prevents that, and it will
> > > otherwise work, but it is unusual and it normally makes sense to use an
> > > hindex instead of an hmap for efficiency's sake if it is going to happen
> > > very often.  Does the data structure in question often have duplicates?
> > > Should we change it to an hindex?
> >
> > Hi Ben, I think you are asking if there can be more than one item with
> > a given "hash value". Yes it is possible, but it should not be often
> > for the data structure in question here - the json cache. The key is
> > uuid of the change set combined with monitor version, and the hash is
> > the uuid_hash. So conflicts of uuid_hash should be rare. When there
> > are different versions (e.g. monitor v1, v2, v3) of clients referring
> > same change sets we do get more items with same hash value, but there
> > were at most 2 versions and I am adding 1 more in next patches in the
> > series, and I guess it is not likely we add too many versions of
> > monitors in the future, and even that is possible, it is unlikely that
> > many different versions of clients co-exists in same environment. So I
> > don't think it has a real impact to performance.
> >
> > The reason for using HMAP_FOR_EACH_SAFE_WITH_HASH instead of
> > HMAP_FOR_EACH_WITH_HASH is just because the node is going to be
> > removed and freed in the loop. It needs to be safe regardless of the
> > probability of hash conflicts.
>
> It's important to be clear about the difference between hashes and keys.
> The hash is what's stored in the hmap_node (that is, the uuid_hash()
> return value in this case), the key in this case is what uuid_equals()
> compares.  I *think* you are saying there can be a small number of
> duplicate keys in the hmap in this case.  Is that correct?

I thought you were talking about hashes instead of keys (I thought it
was a typo), because I assume keys are always unique.
Let me clarify a little more. In the json_cache hmap, the key is the
combination of  of the struct
ovsdb_monitor_json_cache_node. This key uniquely identifies each
element in the hmap. The hash function doesn't take version into the
hash calculation, so if there are multiple versions of clients exist
at the same time, there will be different elements with same hash.
However, since the number of versions should be very small (worse case
3, most cases 1), this is not going to cause any performance problem.
There do exist probability of hash conflict even if there is only one
version, but the chances are very low (ensured by uuid_hash).

Now for the function:
+/* Free all versions of json cache for a given change_set.*/
+static void
+ovsdb_monitor_json_cache_destroy(struct ovsdb_monitor *dbmon,
+ struct ovsdb_monitor_change_set *change_set)

It is different from a general function that destroys an element in a
hmap with the unique key. Instead, it destroys *all versions* of the
json cache for a given change_set uuid. So it uses only part of the
key to do the search, which may have caused some confusion.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v1] ipf: More cleanup.

[Darrell Ball]

ignore; sent V2 with more cleanup.

On Fri, Feb 22, 2019 at 6:37 PM Darrell Ball  wrote:

> No functional changes here.
>
> Signed-off-by: Darrell Ball 
> ---
>  lib/ipf.c | 18 --
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 97d5b58..50678a6 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -522,9 +522,7 @@ ipf_list_state_transition(struct ipf *ipf, struct
> ipf_list *ipf_list,
>  }
>  break;
>  case IPF_LIST_STATE_FIRST_SEEN:
> -if (ff) {
> -next_state = IPF_LIST_STATE_FIRST_SEEN;
> -} else if (lf) {
> +if (lf) {
>  next_state = IPF_LIST_STATE_FIRST_LAST_SEEN;
>  } else {
>  next_state = IPF_LIST_STATE_FIRST_SEEN;
> @@ -714,16 +712,11 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16
> dl_type, uint16_t zone,
> uint16_t *end_data_byte, bool *ff, bool *lf)
>  {
>  const struct ovs_16aligned_ip6_hdr *l3 = dp_packet_l3(pkt);
> -const char *l4 = dp_packet_l4(pkt);
> -const char *tail = dp_packet_tail(pkt);
> -uint8_t pad = dp_packet_l2_pad_size(pkt);
> -size_t l3_size = tail - (char *)l3 - pad;
> -size_t l4_size = tail - (char *)l4 - pad;
>  size_t l3_hdr_size = sizeof *l3;
>  uint8_t nw_frag = 0;
>  uint8_t nw_proto = l3->ip6_nxt;
>  const void *data = l3 + 1;
> -size_t datasize = l3_size - l3_hdr_size;
> +size_t datasize = dp_packet_l3_size(pkt) - l3_hdr_size;
>  const struct ovs_16aligned_ip6_frag *frag_hdr = NULL;
>
>  parse_ipv6_ext_hdrs(, , _proto, _frag, _hdr);
> @@ -731,7 +724,7 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16
> dl_type, uint16_t zone,
>  ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg;
>  *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) +
>  sizeof (struct ovs_16aligned_ip6_frag);
> -*end_data_byte = *start_data_byte + l4_size - 1;
> +*end_data_byte = *start_data_byte + dp_packet_l4_size(pkt) - 1;
>  *ff = ipf_is_first_v6_frag(ip6f_offlg);
>  *lf = ipf_is_last_v6_frag(ip6f_offlg);
>  memset(key, 0, sizeof *key);
> @@ -1175,12 +1168,9 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
>  }
>
>  const struct ipf_frag *frag_0 = >list->frag_list[0];
> -const char *tail_frag = dp_packet_tail(frag_0->pkt);
> -uint8_t pad_frag = dp_packet_l2_pad_size(frag_0->pkt);
>  void *l4_frag = dp_packet_l4(frag_0->pkt);
>  void *l4_reass = dp_packet_l4(pkt);
> -memcpy(l4_frag, l4_reass,
> -   tail_frag - (char *) l4_frag - pad_frag);
> +memcpy(l4_frag, l4_reass, dp_packet_l4_size(frag_0->pkt));
>
>  if (v6) {
>  struct ovs_16aligned_ip6_hdr *l3_frag
> --
> 1.9.1
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [patch v2] ipf: More cleanup.

[Darrell Ball]

No functional changes here.

Signed-off-by: Darrell Ball 
---

v2: Added another cleanup.

 lib/ipf.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/lib/ipf.c b/lib/ipf.c
index 97d5b58..4cc0f2d 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -522,9 +522,7 @@ ipf_list_state_transition(struct ipf *ipf, struct ipf_list 
*ipf_list,
 }
 break;
 case IPF_LIST_STATE_FIRST_SEEN:
-if (ff) {
-next_state = IPF_LIST_STATE_FIRST_SEEN;
-} else if (lf) {
+if (lf) {
 next_state = IPF_LIST_STATE_FIRST_LAST_SEEN;
 } else {
 next_state = IPF_LIST_STATE_FIRST_SEEN;
@@ -714,16 +712,10 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 
dl_type, uint16_t zone,
uint16_t *end_data_byte, bool *ff, bool *lf)
 {
 const struct ovs_16aligned_ip6_hdr *l3 = dp_packet_l3(pkt);
-const char *l4 = dp_packet_l4(pkt);
-const char *tail = dp_packet_tail(pkt);
-uint8_t pad = dp_packet_l2_pad_size(pkt);
-size_t l3_size = tail - (char *)l3 - pad;
-size_t l4_size = tail - (char *)l4 - pad;
-size_t l3_hdr_size = sizeof *l3;
 uint8_t nw_frag = 0;
 uint8_t nw_proto = l3->ip6_nxt;
 const void *data = l3 + 1;
-size_t datasize = l3_size - l3_hdr_size;
+size_t datasize = dp_packet_l3_size(pkt) - sizeof *l3;
 const struct ovs_16aligned_ip6_frag *frag_hdr = NULL;
 
 parse_ipv6_ext_hdrs(, , _proto, _frag, _hdr);
@@ -731,7 +723,7 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 dl_type, 
uint16_t zone,
 ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg;
 *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) +
 sizeof (struct ovs_16aligned_ip6_frag);
-*end_data_byte = *start_data_byte + l4_size - 1;
+*end_data_byte = *start_data_byte + dp_packet_l4_size(pkt) - 1;
 *ff = ipf_is_first_v6_frag(ip6f_offlg);
 *lf = ipf_is_last_v6_frag(ip6f_offlg);
 memset(key, 0, sizeof *key);
@@ -1175,12 +1167,9 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
 }
 
 const struct ipf_frag *frag_0 = >list->frag_list[0];
-const char *tail_frag = dp_packet_tail(frag_0->pkt);
-uint8_t pad_frag = dp_packet_l2_pad_size(frag_0->pkt);
 void *l4_frag = dp_packet_l4(frag_0->pkt);
 void *l4_reass = dp_packet_l4(pkt);
-memcpy(l4_frag, l4_reass,
-   tail_frag - (char *) l4_frag - pad_frag);
+memcpy(l4_frag, l4_reass, dp_packet_l4_size(frag_0->pkt));
 
 if (v6) {
 struct ovs_16aligned_ip6_hdr *l3_frag
-- 
1.9.1

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

[ovs-dev] [patch v1] ipf: More cleanup.

[Darrell Ball]

No functional changes here.

Signed-off-by: Darrell Ball 
---
 lib/ipf.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/lib/ipf.c b/lib/ipf.c
index 97d5b58..50678a6 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -522,9 +522,7 @@ ipf_list_state_transition(struct ipf *ipf, struct ipf_list 
*ipf_list,
 }
 break;
 case IPF_LIST_STATE_FIRST_SEEN:
-if (ff) {
-next_state = IPF_LIST_STATE_FIRST_SEEN;
-} else if (lf) {
+if (lf) {
 next_state = IPF_LIST_STATE_FIRST_LAST_SEEN;
 } else {
 next_state = IPF_LIST_STATE_FIRST_SEEN;
@@ -714,16 +712,11 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 
dl_type, uint16_t zone,
uint16_t *end_data_byte, bool *ff, bool *lf)
 {
 const struct ovs_16aligned_ip6_hdr *l3 = dp_packet_l3(pkt);
-const char *l4 = dp_packet_l4(pkt);
-const char *tail = dp_packet_tail(pkt);
-uint8_t pad = dp_packet_l2_pad_size(pkt);
-size_t l3_size = tail - (char *)l3 - pad;
-size_t l4_size = tail - (char *)l4 - pad;
 size_t l3_hdr_size = sizeof *l3;
 uint8_t nw_frag = 0;
 uint8_t nw_proto = l3->ip6_nxt;
 const void *data = l3 + 1;
-size_t datasize = l3_size - l3_hdr_size;
+size_t datasize = dp_packet_l3_size(pkt) - l3_hdr_size;
 const struct ovs_16aligned_ip6_frag *frag_hdr = NULL;
 
 parse_ipv6_ext_hdrs(, , _proto, _frag, _hdr);
@@ -731,7 +724,7 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 dl_type, 
uint16_t zone,
 ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg;
 *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) +
 sizeof (struct ovs_16aligned_ip6_frag);
-*end_data_byte = *start_data_byte + l4_size - 1;
+*end_data_byte = *start_data_byte + dp_packet_l4_size(pkt) - 1;
 *ff = ipf_is_first_v6_frag(ip6f_offlg);
 *lf = ipf_is_last_v6_frag(ip6f_offlg);
 memset(key, 0, sizeof *key);
@@ -1175,12 +1168,9 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
 }
 
 const struct ipf_frag *frag_0 = >list->frag_list[0];
-const char *tail_frag = dp_packet_tail(frag_0->pkt);
-uint8_t pad_frag = dp_packet_l2_pad_size(frag_0->pkt);
 void *l4_frag = dp_packet_l4(frag_0->pkt);
 void *l4_reass = dp_packet_l4(pkt);
-memcpy(l4_frag, l4_reass,
-   tail_frag - (char *) l4_frag - pad_frag);
+memcpy(l4_frag, l4_reass, dp_packet_l4_size(frag_0->pkt));
 
 if (v6) {
 struct ovs_16aligned_ip6_hdr *l3_frag
-- 
1.9.1

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

Re: [ovs-dev] [patch v2 2/2] conntrack: Fix L4 csum for V6 extension hdr pkts.

[Ben Pfaff]

On Fri, Feb 22, 2019 at 05:17:42PM -0800, Darrell Ball wrote:
> It is a day one issue that got copied to subsequent code.
> 
> Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> CC: Daniele Di Proietto 
> Signed-off-by: Darrell Ball 
> ---
> 
> Fix will need to be backported as far back as 2.6 and the fix will
> need separate patches for some earlier releases, which I will do.
> 
> v2: Fix compiler warnings.
> Split out from another series; this is version 2 of this
> patch itself.

Thanks for the series.  I applied it to master and as far back as
branch-2.9.  Beyond that, as you said, it will need some help.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Ben Pfaff]

On Fri, Feb 22, 2019 at 05:21:51PM -0800, Han Zhou wrote:
> On Fri, Feb 22, 2019 at 3:04 PM Ben Pfaff  wrote:
> >
> > Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can have
> > more than one item with a given key.  Nothing prevents that, and it will
> > otherwise work, but it is unusual and it normally makes sense to use an
> > hindex instead of an hmap for efficiency's sake if it is going to happen
> > very often.  Does the data structure in question often have duplicates?
> > Should we change it to an hindex?
> 
> Hi Ben, I think you are asking if there can be more than one item with
> a given "hash value". Yes it is possible, but it should not be often
> for the data structure in question here - the json cache. The key is
> uuid of the change set combined with monitor version, and the hash is
> the uuid_hash. So conflicts of uuid_hash should be rare. When there
> are different versions (e.g. monitor v1, v2, v3) of clients referring
> same change sets we do get more items with same hash value, but there
> were at most 2 versions and I am adding 1 more in next patches in the
> series, and I guess it is not likely we add too many versions of
> monitors in the future, and even that is possible, it is unlikely that
> many different versions of clients co-exists in same environment. So I
> don't think it has a real impact to performance.
> 
> The reason for using HMAP_FOR_EACH_SAFE_WITH_HASH instead of
> HMAP_FOR_EACH_WITH_HASH is just because the node is going to be
> removed and freed in the loop. It needs to be safe regardless of the
> probability of hash conflicts.

It's important to be clear about the difference between hashes and keys.
The hash is what's stored in the hmap_node (that is, the uuid_hash()
return value in this case), the key in this case is what uuid_equals()
compares.  I *think* you are saying there can be a small number of
duplicate keys in the hmap in this case.  Is that correct?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Han Zhou]

On Fri, Feb 22, 2019 at 3:04 PM Ben Pfaff  wrote:
>
> Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can have
> more than one item with a given key.  Nothing prevents that, and it will
> otherwise work, but it is unusual and it normally makes sense to use an
> hindex instead of an hmap for efficiency's sake if it is going to happen
> very often.  Does the data structure in question often have duplicates?
> Should we change it to an hindex?

Hi Ben, I think you are asking if there can be more than one item with
a given "hash value". Yes it is possible, but it should not be often
for the data structure in question here - the json cache. The key is
uuid of the change set combined with monitor version, and the hash is
the uuid_hash. So conflicts of uuid_hash should be rare. When there
are different versions (e.g. monitor v1, v2, v3) of clients referring
same change sets we do get more items with same hash value, but there
were at most 2 versions and I am adding 1 more in next patches in the
series, and I guess it is not likely we add too many versions of
monitors in the future, and even that is possible, it is unlikely that
many different versions of clients co-exists in same environment. So I
don't think it has a real impact to performance.

The reason for using HMAP_FOR_EACH_SAFE_WITH_HASH instead of
HMAP_FOR_EACH_WITH_HASH is just because the node is going to be
removed and freed in the loop. It needs to be safe regardless of the
probability of hash conflicts.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [patch v2 2/2] conntrack: Fix L4 csum for V6 extension hdr pkts.

[Darrell Ball]

It is a day one issue that got copied to subsequent code.

Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
CC: Daniele Di Proietto 
Signed-off-by: Darrell Ball 
---

Fix will need to be backported as far back as 2.6 and the fix will
need separate patches for some earlier releases, which I will do.

v2: Fix compiler warnings.
Split out from another series; this is version 2 of this
patch itself.

 lib/conntrack.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 4d76552..4028ba9 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -686,10 +686,9 @@ reverse_nat_packet(struct dp_packet *pkt, const struct 
conn *conn)
  >key.dst.addr.ipv6, true);
 }
 reverse_pat_packet(pkt, conn);
-uint32_t icmp6_csum = packet_csum_pseudoheader6(nh6);
 icmp6->icmp6_base.icmp6_cksum = 0;
-icmp6->icmp6_base.icmp6_cksum = csum_finish(
-csum_continue(icmp6_csum, icmp6, tail - (char *) icmp6 - pad));
+icmp6->icmp6_base.icmp6_cksum = packet_csum_upperlayer6(nh6, icmp6,
+IPPROTO_ICMPV6, tail - (char *) icmp6 - pad);
 }
 pkt->l3_ofs = orig_l3_ofs;
 pkt->l4_ofs = orig_l4_ofs;
@@ -1591,19 +1590,14 @@ static inline bool
 checksum_valid(const struct conn_key *key, const void *data, size_t size,
const void *l3)
 {
-uint32_t csum = 0;
-
 if (key->dl_type == htons(ETH_TYPE_IP)) {
-csum = packet_csum_pseudoheader(l3);
+uint32_t csum = packet_csum_pseudoheader(l3);
+return csum_finish(csum_continue(csum, data, size)) == 0;
 } else if (key->dl_type == htons(ETH_TYPE_IPV6)) {
-csum = packet_csum_pseudoheader6(l3);
+return packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0;
 } else {
 return false;
 }
-
-csum = csum_continue(csum, data, size);
-
-return csum_finish(csum) == 0;
 }
 
 static inline bool
@@ -3261,16 +3255,14 @@ handle_ftp_ctl(struct conntrack *ct, const struct 
conn_lookup_ctx *ctx,
 }
 
 th->tcp_csum = 0;
-uint32_t tcp_csum;
 if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
-tcp_csum = packet_csum_pseudoheader6(nh6);
+th->tcp_csum = packet_csum_upperlayer6(nh6, th, ctx->key.nw_proto,
+   dp_packet_l4_size(pkt));
 } else {
-tcp_csum = packet_csum_pseudoheader(l3_hdr);
+uint32_t tcp_csum = packet_csum_pseudoheader(l3_hdr);
+th->tcp_csum = csum_finish(
+ csum_continue(tcp_csum, th, dp_packet_l4_size(pkt)));
 }
-const char *tail = dp_packet_tail(pkt);
-uint8_t pad = dp_packet_l2_pad_size(pkt);
-th->tcp_csum = csum_finish(
-csum_continue(tcp_csum, th, tail - (char *) th - pad));
 
 if (seq_skew) {
 conn_seq_skew_set(ct, >key, now, seq_skew + ec->seq_skew,
-- 
1.9.1

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

[ovs-dev] [patch v2 1/2] packets: Change return type for 'packet_csum_upperlayer6()'.

[Darrell Ball]

Signed-off-by: Darrell Ball 
---

New patch for series with another patch at v2.

Series will need to be backported to 2.6.

 lib/packets.c | 2 +-
 lib/packets.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index f5a2005..a8fd61f 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -1695,7 +1695,7 @@ packet_csum_pseudoheader6(const struct 
ovs_16aligned_ip6_hdr *ip6)
 /* Calculate the IPv6 upper layer checksum according to RFC2460. We pass the
ip6_nxt and ip6_plen values, so it will also work if extension headers
are present. */
-uint16_t
+ovs_be16
 packet_csum_upperlayer6(const struct ovs_16aligned_ip6_hdr *ip6,
 const void *data, uint8_t l4_protocol,
 uint16_t l4_size)
diff --git a/lib/packets.h b/lib/packets.h
index fcd90b3..e20a70a 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -951,7 +951,7 @@ struct icmp6_error_header {
 BUILD_ASSERT_DECL(ICMP6_ERROR_HEADER_LEN == sizeof(struct icmp6_error_header));
 
 uint32_t packet_csum_pseudoheader6(const struct ovs_16aligned_ip6_hdr *);
-uint16_t packet_csum_upperlayer6(const struct ovs_16aligned_ip6_hdr *,
+ovs_be16 packet_csum_upperlayer6(const struct ovs_16aligned_ip6_hdr *,
  const void *, uint8_t, uint16_t);
 
 /* Neighbor Discovery option field.
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] Prevent kernel warning length of ovs attribute.

[Ben Pfaff]

On Wed, Dec 12, 2018 at 03:06:43PM +, Nathanael Davison wrote:
> Linux kernel commit 6e237d099fac introduced warnings when validating the
> length of some types. This exposed a bug in dpif-netlink.c
> dpif_netlink_vport_to_ofpbuf where ovs was passing the upcall pids as
> variable length array while the kernel was expecting a single u32. This
> resulted in the kernel reporting "netlink: 'ovs-vswitchd': attribute
> type 5 has an invalid length.".
> 
> This patch changes the kernel to expect NLA_UNSPEC, which is for
> arbitrary length data.
> 
> Signed-off-by: Nathanael Davison 
> ---
>  datapath/datapath.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index e3d3c8c..3204bf3 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -2170,7 +2170,7 @@ static const struct nla_policy 
> vport_policy[OVS_VPORT_ATTR_MAX + 1] = {
>   [OVS_VPORT_ATTR_STATS] = { .len = sizeof(struct ovs_vport_stats) },
>   [OVS_VPORT_ATTR_PORT_NO] = { .type = NLA_U32 },
>   [OVS_VPORT_ATTR_TYPE] = { .type = NLA_U32 },
> - [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_U32 },
> + [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_UNSPEC },
>   [OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
>  };

This seems like it fell through the cracks.  Sorry about that.

Nathanael, the usual way to get fixes into the OVS datapath directory is
to first get them into the upstream kernel "net-next".  It doesn't look
like this is there yet, so it's not yet time to submit it to ovs-dev.
Instead, you should start with the Linux netdev mailing list (but you'll
need to modify the patch to apply to net/openvswitch/datapath.c in that
tree instead).

Greg (CCed) might be able to help, if you need it.

Thanks,

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

Re: [ovs-dev] [PATCHv8] Improved Packet Drop Statistics in OVS

[Ben Pfaff]

On Tue, Feb 19, 2019 at 11:38:04AM +, Anju Thomas wrote:
> Currently OVS maintains explicit packet drop/error counters only on port
> level. Packets that are dropped as part of normal OpenFlow processing are
> counted in flow stats of “drop” flows or as table misses in table stats.
> These can only be interpreted by controllers that know the semantics of
> the configured OpenFlow pipeline. Without that knowledge, it is impossible
> for an OVS user to obtain e.g. the total number of packets dropped due to
> OpenFlow rules.
> 
> Furthermore, there are numerous other reasons for which packets can be
> dropped by OVS slow path that are not related to the OpenFlow pipeline.
> The generated datapath flow entries include a drop action to avoid further
> expensive upcalls to the slow path, but subsequent packets dropped by the
> datapath are not accounted anywhere.
> 
> Finally, the datapath itself drops packets in certain error situations.
> Also, these drops are today not accounted for.
> 
> This makes it difficult for OVS users to monitor packet drop in an OVS
> instance and to alert a management system in case of a unexpected increase
> of such drops. Also OVS trouble-shooters face difficulties in analysing
> packet drops.
> 
> With this patch we implement following changes to address the issues
> mentioned above.
> 
> 1. Identify and account all the silent packet drop scenarios
> 
> 2. Display these drops in ovs-appctl coverage/show
> 
> A detailed presentation on this was presented at OvS conference 2017 and
> link for the corresponding presentation is available at:
> 
> https://www.slideshare.net/LF_OpenvSwitch/lfovs17troubleshooting-the-data-plane-in-ovs-82280329
> 
> Co-authored-by: Rohith Basavaraja 
> Co-authored-by: Keshav Gupta 
> Signed-off-by: Anju Thomas 
> Signed-off-by: Rohith Basavaraja 
> Signed-off-by: Keshav Gupta 

Thanks for working on this.

I'm getting a compiler error with this version:
../ofproto/ofproto-dpif-xlate.c:426:13: error: enumeration value 'XLATE_MAX' 
not handled in switch [-Werror,-Wswitch]
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-controller: Provide the option to set the datapath-type of br-int

[Ben Pfaff]

On Tue, Feb 19, 2019 at 06:05:30PM +0530, Numan Siddique wrote:
> On Tue, Feb 19, 2019 at 2:32 AM Mark Michelson  wrote:
> 
> > Looks good to me.
> >
> > Acked-by: Mark Michelson 
> >
> > Out of curiosity, can you think of any other bridge configuration items
> > that might need the same treatment?
> >
> >
> Thanks for the review.  I checked and I don't think we need anything extra
> here.

Thanks, Numan (and Mark).  I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] rstp: add ability to receive VLAN-tagged BPDUs

[Ben Pfaff]

On Mon, Feb 18, 2019 at 12:07:54PM +0100, Matthias May wrote:
> On 15/02/2019 01:28, Ben Pfaff wrote:
> > On Fri, Feb 15, 2019 at 12:27:11AM +0100, Matthias May wrote:
> >> On 14/02/2019 20:17, Ben Pfaff wrote:
> >>> On Thu, Feb 14, 2019 at 10:58:48AM +0100, Matthias May via dev wrote:
>  There are switches which allow to transmit their BPDUs VLAN-tagged.
>  With this change OVS is able to receive VLAN-tagged BPDUs, but still
>  transmits its own BPDUs untagged.
>  This was tested against Westermo RFI-207-F4G-T3G.
> 
>  Signed-off-by: Matthias May 
> >>>
> >>> Thanks for the patch.
> >>>
> >>> To me, it seems really odd to treat packets with and without an
> >>> arbitrary VLAN header the same way.  I could see it if the VLAN header
> >>> had VID 0 or 1 or some other specified value, but it seems unusual to
> >>> ignore it entirely.  Is this standardized or a de facto standard of some
> >>> kind?
> >>>
> >>
> >> I totally agree.
> >> To me a VLAN header has nothing lost on a BPDU of a (R)STP frame, simply
> >> because (R)STP is not per VLAN.
> >>
> >> However the fact is that there are switches which are transmitting
> >> frames on a VLAN.
> >>
> >> With this change we simply ignore the VLAN header if is present. The
> >> meaning of the BPDU doesn't cheange. The provided information still is
> >> not per VLAN and applies to all ports the same.
> >>
> >> This patch does not add the ability to transmit VLAN tagged BPDUs for
> >> the same reasoning above: RSTP/STP is not supposed to be per VLAN.
> >> I was thinking about adding to the patch that one can specify a VLAN via
> >> config and only BPDUs with the configured VLAN are accepted. I guess
> >> this is what you propose: only accept vlan tagged BPDUs on a specified 
> >> VLAN.
> >> Having such a config-parameter would also enable to transmit the BPDUs
> >> VLAN tagged. But I'm still of the opinion that this only suggests that
> >> one could have an (R)STP tree per VLAN.
> > 
> > I see we are basically in agreement, but I'd like more information, if
> > you have it.
> > 
> > Do the switches that transmit RSTP on a VLAN transmit it on a particular
> > VLAN like 0 or 1?  (Maybe they are transmitting them as priority-tagged
> > frames for some reason?)  If so, then it would be possible to accept
> > just that VLAN.
> > 
> 
> Hi Ben
> The switch I tested against has config options to specify with which VLAN the 
> BPDUs should be transmitted.
> From the tests I did, for this switch it doesn't matter if the received BPDUs 
> are tagged or not. The VLAN header is
> simply ignored.
> I have an open question with the vendor regarding this.
> 
> I did some digging on the net how other switches handle this. E.g at [1].
> From what I get, switches which allow tagged BPDUs simply ignore the header.
> 
> I started reading the 802.1d-2004 and 802.1q-2018, but so far haven't found 
> anything which specifies how this situation
> should be handled.

I guess networking doesn't always make total sense.  It's a shame.

I applied the patch to master, thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] rstp: add ability to receive VLAN-tagged BPDUs

[Ben Pfaff]

On Fri, Feb 15, 2019 at 12:16:14AM +0100, Matthias May via dev wrote:
> There are switches which allow to transmit their BPDUs VLAN-tagged.
> With this change OVS is able to receive VLAN-tagged BPDUs, but still
> transmits its own BPDUs untagged.
> This was tested against Westermo RFI-207-F4G-T3G.
> 
> v2: Send patch to different address of mailing list according to suggestion
> of Flavio Leitner.
> 
> Signed-off-by: Matthias May 

I applied this to master.  Thanks!

I fussed with it a bit to make it closer to the preferred style.  Here
is what I committed:

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

From: Matthias May 
Date: Fri, 15 Feb 2019 00:16:14 +0100
Subject: [PATCH] rstp: add ability to receive VLAN-tagged BPDUs

There are switches which allow to transmit their BPDUs VLAN-tagged.
With this change OVS is able to receive VLAN-tagged BPDUs, but still
transmits its own BPDUs untagged.
This was tested against Westermo RFI-207-F4G-T3G.

Signed-off-by: Matthias May 
Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif-xlate.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index acd4817c2549..81b72be37eb5 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1787,7 +1787,11 @@ rstp_process_packet(const struct xport *xport, const 
struct dp_packet *packet)
 dp_packet_set_size(, ntohs(eth->eth_type) + ETH_HEADER_LEN);
 }
 
-if (dp_packet_try_pull(, ETH_HEADER_LEN + LLC_HEADER_LEN)) {
+int len = ETH_HEADER_LEN + LLC_HEADER_LEN;
+if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
+len += VLAN_HEADER_LEN;
+}
+if (dp_packet_try_pull(, len)) {
 rstp_port_received_bpdu(xport->rstp_port, dp_packet_data(),
 dp_packet_size());
 }
-- 
2.20.1

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

Re: [ovs-dev] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Ben Pfaff]

Using HMAP_FOR_EACH_SAFE_WITH_HASH tends to imply that an hmap can have
more than one item with a given key.  Nothing prevents that, and it will
otherwise work, but it is unusual and it normally makes sense to use an
hindex instead of an hmap for efficiency's sake if it is going to happen
very often.  Does the data structure in question often have duplicates?
Should we change it to an hindex?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] Geneve remote_ip as flow for OVN hosts

[Venugopal Iyer]

Thanks, Ben! I'll keep a watch, but if I do miss anything please let me know 
and I'll try to get to it quickly.

thanks,

-venu


From: Ben Pfaff 
Sent: Friday, February 22, 2019 2:42 PM
To: Venugopal Iyer
Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts

This was definitely a bit quicker than I usually prefer for changes that
might have nonobvious side effects, but we are also very close to the
beginning of a release cycle, so I decided to be an optimist for once.

You can watch the OVS CI/CD here:
https://travis-ci.org/openvswitch/ovs

On Fri, Feb 22, 2019 at 10:37:43PM +, Venugopal Iyer wrote:
> Thanks, Ben!
>
> I was thinking there'll be additional test cycles to check for regression 
> before making it to the
> code. From my side, I have tested it for the new feature, compatibility and 
> run the OVN community
> tests (plus testing in our setup for OVN functionality).
>
> How do I monitor for regressions, if any?
>
> Sorry if it is obvious, i haven't pushed any changes to OVS prior to this.
>
> thanks again,
>
> -venu
>
> 
> From: Ben Pfaff 
> Sent: Friday, February 22, 2019 1:47 PM
> To: Venugopal Iyer
> Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
> Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts
>
> I applied this series to master.  Thank you!
>
> On Tue, Feb 12, 2019 at 03:52:48PM +, Venugopal Iyer wrote:
> > HI, Ben:
> >
> > 
> > From: Ben Pfaff 
> > Sent: Monday, February 11, 2019 5:55 PM
> > To: Venugopal Iyer
> > Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
> > Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts
> >
> > On Mon, Feb 11, 2019 at 08:09:59PM +, Venugopal Iyer wrote:
> > > Of course we want users to upgrade the entire system.  We just need to
> > > make sure that it's possible to upgrade one piece at a time in an order
> > > that ensures that the system isn't broken by a partial upgrade.  The
> > > specified order for OVN is to upgrade the HVs first, then the central
> > > node.  (Although apparently some people want to do it in the other
> > > order, which is currently a problem.)
> > >
> > >  Thanks, I have updated the repo to squash all the commits and added 
> > > a high
> > >  level commit message. Please let me know if the message is helpful 
> > > and/or if
> > >  there are some best practices that I should follow. FYI, branch 
> > > mvtep-br
> > >  @ https://github.com/iyervl/nv-ovs
> >
> > Thanks for the revision.
> >
> > It's not clear to me whether you believe that the upgrade compatibility
> > issue is fixed.  Is it?
> >
> >  Sorry, it was not clear. Yes, it is fixed, specifically changes in
> >  ovn/controller/binding.c, L398-400, L413 and L547. Let me know
> >  if you have questions.
> >
> > thanks,
> >
> > -venu
> >
> > Thanks,
> >
> > Ben.
> >
> > ---
> > This email message is for the sole use of the intended recipient(s) and may 
> > contain
> > confidential information.  Any unauthorized review, use, disclosure or 
> > distribution
> > is prohibited.  If you are not the intended recipient, please contact the 
> > sender by
> > reply email and destroy all copies of the original message.
> > ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/7] ovsdb_monitor: Fix style of prototypes.

[Ben Pfaff]

On Fri, Feb 15, 2019 at 12:25:58PM -0800, Han Zhou wrote:
> From: Han Zhou 
> 
> Ommiting the parameter names in prototypes, as suggested by coding
> style: Omit parameter names from function prototypes when the names
> do not give useful information.
> 
> Adjust orders of parameters as suggested by coding style.
> 
> Signed-off-by: Han Zhou 

Thanks a lot, I really appreciate this attention to detail.

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

Re: [ovs-dev] [PATCH v2 1/7] ovsdb-client.c: fix typo

[Ben Pfaff]

On Fri, Feb 15, 2019 at 12:25:57PM -0800, Han Zhou wrote:
> From: Han Zhou 
> 
> Signed-off-by: Han Zhou 

Thanks, applied to master and backported as far as it would go.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] Geneve remote_ip as flow for OVN hosts

[Ben Pfaff]

This was definitely a bit quicker than I usually prefer for changes that
might have nonobvious side effects, but we are also very close to the
beginning of a release cycle, so I decided to be an optimist for once.

You can watch the OVS CI/CD here:
https://travis-ci.org/openvswitch/ovs

On Fri, Feb 22, 2019 at 10:37:43PM +, Venugopal Iyer wrote:
> Thanks, Ben!
> 
> I was thinking there'll be additional test cycles to check for regression 
> before making it to the
> code. From my side, I have tested it for the new feature, compatibility and 
> run the OVN community
> tests (plus testing in our setup for OVN functionality).
> 
> How do I monitor for regressions, if any?
> 
> Sorry if it is obvious, i haven't pushed any changes to OVS prior to this.
> 
> thanks again,
> 
> -venu
> 
> 
> From: Ben Pfaff 
> Sent: Friday, February 22, 2019 1:47 PM
> To: Venugopal Iyer
> Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
> Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts
> 
> I applied this series to master.  Thank you!
> 
> On Tue, Feb 12, 2019 at 03:52:48PM +, Venugopal Iyer wrote:
> > HI, Ben:
> >
> > 
> > From: Ben Pfaff 
> > Sent: Monday, February 11, 2019 5:55 PM
> > To: Venugopal Iyer
> > Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
> > Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts
> >
> > On Mon, Feb 11, 2019 at 08:09:59PM +, Venugopal Iyer wrote:
> > > Of course we want users to upgrade the entire system.  We just need to
> > > make sure that it's possible to upgrade one piece at a time in an order
> > > that ensures that the system isn't broken by a partial upgrade.  The
> > > specified order for OVN is to upgrade the HVs first, then the central
> > > node.  (Although apparently some people want to do it in the other
> > > order, which is currently a problem.)
> > >
> > >  Thanks, I have updated the repo to squash all the commits and added 
> > > a high
> > >  level commit message. Please let me know if the message is helpful 
> > > and/or if
> > >  there are some best practices that I should follow. FYI, branch 
> > > mvtep-br
> > >  @ https://github.com/iyervl/nv-ovs
> >
> > Thanks for the revision.
> >
> > It's not clear to me whether you believe that the upgrade compatibility
> > issue is fixed.  Is it?
> >
> >  Sorry, it was not clear. Yes, it is fixed, specifically changes in
> >  ovn/controller/binding.c, L398-400, L413 and L547. Let me know
> >  if you have questions.
> >
> > thanks,
> >
> > -venu
> >
> > Thanks,
> >
> > Ben.
> >
> > ---
> > This email message is for the sole use of the intended recipient(s) and may 
> > contain
> > confidential information.  Any unauthorized review, use, disclosure or 
> > distribution
> > is prohibited.  If you are not the intended recipient, please contact the 
> > sender by
> > reply email and destroy all copies of the original message.
> > ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] Geneve remote_ip as flow for OVN hosts

[Venugopal Iyer]

Thanks, Ben!

I was thinking there'll be additional test cycles to check for regression 
before making it to the
code. From my side, I have tested it for the new feature, compatibility and run 
the OVN community
tests (plus testing in our setup for OVN functionality).

How do I monitor for regressions, if any?

Sorry if it is obvious, i haven't pushed any changes to OVS prior to this.

thanks again,

-venu


From: Ben Pfaff 
Sent: Friday, February 22, 2019 1:47 PM
To: Venugopal Iyer
Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts

I applied this series to master.  Thank you!

On Tue, Feb 12, 2019 at 03:52:48PM +, Venugopal Iyer wrote:
> HI, Ben:
>
> 
> From: Ben Pfaff 
> Sent: Monday, February 11, 2019 5:55 PM
> To: Venugopal Iyer
> Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
> Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts
>
> On Mon, Feb 11, 2019 at 08:09:59PM +, Venugopal Iyer wrote:
> > Of course we want users to upgrade the entire system.  We just need to
> > make sure that it's possible to upgrade one piece at a time in an order
> > that ensures that the system isn't broken by a partial upgrade.  The
> > specified order for OVN is to upgrade the HVs first, then the central
> > node.  (Although apparently some people want to do it in the other
> > order, which is currently a problem.)
> >
> >  Thanks, I have updated the repo to squash all the commits and added a 
> > high
> >  level commit message. Please let me know if the message is helpful 
> > and/or if
> >  there are some best practices that I should follow. FYI, branch 
> > mvtep-br
> >  @ https://github.com/iyervl/nv-ovs
>
> Thanks for the revision.
>
> It's not clear to me whether you believe that the upgrade compatibility
> issue is fixed.  Is it?
>
>  Sorry, it was not clear. Yes, it is fixed, specifically changes in
>  ovn/controller/binding.c, L398-400, L413 and L547. Let me know
>  if you have questions.
>
> thanks,
>
> -venu
>
> Thanks,
>
> Ben.
>
> ---
> This email message is for the sole use of the intended recipient(s) and may 
> contain
> confidential information.  Any unauthorized review, use, disclosure or 
> distribution
> is prohibited.  If you are not the intended recipient, please contact the 
> sender by
> reply email and destroy all copies of the original message.
> ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v3 6/6] conntrack: Fix L4 csum for V6 extension hdr pkts.

[Darrell Ball]

oops, thanks for pointing that out

I'll fix and resend

Darrell

On Fri, Feb 22, 2019 at 2:18 PM Ben Pfaff  wrote:

> On Wed, Feb 20, 2019 at 08:17:19AM -0800, Darrell Ball wrote:
> > It is a day one issue that got copied to the FTP handling code.
> >
> > Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> > CC: Daniele Di Proietto 
> > Signed-off-by: Darrell Ball 
>
> Thanks for working on this!
>
> There's something goofy with the types here:
>
> ../lib/conntrack.c:1598:14: error: incorrect type in assignment (different
> base types)
> ../lib/conntrack.c:1598:14:expected unsigned int [unsigned] [assigned]
> [usertype] csum
> ../lib/conntrack.c:1598:14:got restricted ovs_be16
> ../lib/conntrack.c:1598:14: error: incorrect type in assignment (different
> base types)
> ../lib/conntrack.c:1598:14:expected unsigned int [unsigned] [assigned]
> [usertype] csum
> ../lib/conntrack.c:1598:14:got restricted ovs_be16
> ../lib/conntrack.c:1598:14: error: incorrect type in assignment (different
> base types)
> ../lib/conntrack.c:1598:14:expected unsigned int [unsigned] [assigned]
> [usertype] csum
> ../lib/conntrack.c:1598:14:got restricted ovs_be16
> ../lib/conntrack.c:3264:22: error: incorrect type in assignment (different
> base types)
> ../lib/conntrack.c:3264:22:expected restricted ovs_be16 [usertype]
> tcp_csum
> ../lib/conntrack.c:3264:22:got unsigned short
> ../lib/conntrack.c:690:39: error: incorrect type in assignment (different
> base types)
> ../lib/conntrack.c:690:39:expected restricted ovs_be16 [usertype]
> icmp6_cksum
> ../lib/conntrack.c:690:39:got unsigned short
>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] netdev-vport: Use the dst_port in tunnel netdev name

[Ben Pfaff]

On Fri, Feb 22, 2019 at 10:10:16AM +0800, Chris Mi wrote:
> If tunnel device dst_port is not the default one, "ovs-dpctl dump-flows"
> will fail. The error message for vxlan is:
> 
> netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed: No 
> such device
> 
> That's because when calling netdev_vport_construct() for netdev
> vxlan_sys_, the default dst_port is used. Actually, the dst_port
> value is in the netdev name. Use it to avoid the error.
> 
> Signed-off-by: Chris Mi 
> Reviewed-by: Roi Dayan 
> ---
> 
> v1
> ==
> 
> Any comment about this patch? We are not sure if it is correct
> to verify the port from the name. If it is correct, is it applicable
> for other tunnels? Thanks!
> 
> v2
> ==
> 
> Apply the same fix to other tunnel types according to Flavio Leitner's
> comment.

Thanks for the patch!

It looks to me that if 'name' and 'dpif_port' are exactly the same, then
this code will read past the null terminator in 'dpif_port':
+if (!strncmp(name, dpif_port, strlen(dpif_port))) {
+p = name + strlen(dpif_port) + 1;
+port = atoi(p);
+}

Thanks,

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

Re: [ovs-dev] [patch v3 6/6] conntrack: Fix L4 csum for V6 extension hdr pkts.

[Ben Pfaff]

On Wed, Feb 20, 2019 at 08:17:19AM -0800, Darrell Ball wrote:
> It is a day one issue that got copied to the FTP handling code.
> 
> Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
> CC: Daniele Di Proietto 
> Signed-off-by: Darrell Ball 

Thanks for working on this!

There's something goofy with the types here:

../lib/conntrack.c:1598:14: error: incorrect type in assignment (different base 
types)
../lib/conntrack.c:1598:14:expected unsigned int [unsigned] [assigned] 
[usertype] csum
../lib/conntrack.c:1598:14:got restricted ovs_be16
../lib/conntrack.c:1598:14: error: incorrect type in assignment (different base 
types)
../lib/conntrack.c:1598:14:expected unsigned int [unsigned] [assigned] 
[usertype] csum
../lib/conntrack.c:1598:14:got restricted ovs_be16
../lib/conntrack.c:1598:14: error: incorrect type in assignment (different base 
types)
../lib/conntrack.c:1598:14:expected unsigned int [unsigned] [assigned] 
[usertype] csum
../lib/conntrack.c:1598:14:got restricted ovs_be16
../lib/conntrack.c:3264:22: error: incorrect type in assignment (different base 
types)
../lib/conntrack.c:3264:22:expected restricted ovs_be16 [usertype] tcp_csum
../lib/conntrack.c:3264:22:got unsigned short
../lib/conntrack.c:690:39: error: incorrect type in assignment (different base 
types)
../lib/conntrack.c:690:39:expected restricted ovs_be16 [usertype] 
icmp6_cksum
../lib/conntrack.c:690:39:got unsigned short

Thanks,

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

Re: [ovs-dev] [patch v3 5/6] ipf: Handle non-zero L2 padding for first fragments.

[Ben Pfaff]

On Wed, Feb 20, 2019 at 08:17:18AM -0800, Darrell Ball wrote:
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Darrell Ball 

Thanks for the fixes!  I applied patches 1 through 5 to master.
I'll send some comments for patch 6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC] OVN: add the possibility to configure a static IPv4 addr/IPv6 prefix and dynamic MAC

[Ben Pfaff]

On Wed, Feb 20, 2019 at 06:28:42PM +0100, Lorenzo Bianconi wrote:
> Add the possibility to configure a static IPv4 address and IPv6 prefix
> and get MAC address dynamically allocated. This can be done using the
> following commands:
> 
> $ovn-nbctl ls-add sw0
> $ovn-nbctl set Logical-Switch sw0 other_config:subnet=192.168.0.0/24
> $ovn-nbctl set Logical-switch sw0 other_config:ipv6_prefix=2001::0
> $ovn-nbctl lsp-add sw0 lsp0 -- lsp-set-addresses lsp0 "dynamic 192.168.0.1 
> 2001::1"
> 
> Signed-off-by: Lorenzo Bianconi 

I think this is OK.  It needs a documentation update and a NEWS item
though.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] faq: Return GRE-IPv6 tunneling support.

[Ben Pfaff]

On Fri, Feb 15, 2019 at 01:37:20PM +0300, Ilya Maximets wrote:
> Accidentially changed while updating conntrack support.
> 
> CC: Darrell Ball 
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Ilya Maximets 

Thanks, Ilya (and Darrell).  Applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-nbctl: Daemon mode should retry when IDL connection lost.

[Ben Pfaff]

On Fri, Feb 15, 2019 at 06:49:52PM -0800, Han Zhou wrote:
> From: Han Zhou 
> 
> When creating IDL, "retry" was set to false. However, in daemon
> mode, reconnecting upon DB server failure should be transparent
> to user. This even impacts HA mode. E.g. in clustered mode, although
> IDL tries to connect to next server, but at the first retry the
> server fail-over may not be completed yet, and it stops retry after
> N (N = number of remotes) times.
> 
> This patch makes sure in daemon mode retry is set to true so that
> the daemon will automatically retry forever.
> 
> Signed-off-by: Han Zhou 

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

Re: [ovs-dev] [ovs-discuss] Geneve remote_ip as flow for OVN hosts

[Ben Pfaff]

I applied this series to master.  Thank you!

On Tue, Feb 12, 2019 at 03:52:48PM +, Venugopal Iyer wrote:
> HI, Ben:
> 
> 
> From: Ben Pfaff 
> Sent: Monday, February 11, 2019 5:55 PM
> To: Venugopal Iyer
> Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org
> Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts
> 
> On Mon, Feb 11, 2019 at 08:09:59PM +, Venugopal Iyer wrote:
> > Of course we want users to upgrade the entire system.  We just need to
> > make sure that it's possible to upgrade one piece at a time in an order
> > that ensures that the system isn't broken by a partial upgrade.  The
> > specified order for OVN is to upgrade the HVs first, then the central
> > node.  (Although apparently some people want to do it in the other
> > order, which is currently a problem.)
> >
> >  Thanks, I have updated the repo to squash all the commits and added a 
> > high
> >  level commit message. Please let me know if the message is helpful 
> > and/or if
> >  there are some best practices that I should follow. FYI, branch 
> > mvtep-br
> >  @ https://github.com/iyervl/nv-ovs
> 
> Thanks for the revision.
> 
> It's not clear to me whether you believe that the upgrade compatibility
> issue is fixed.  Is it?
> 
>  Sorry, it was not clear. Yes, it is fixed, specifically changes in
>  ovn/controller/binding.c, L398-400, L413 and L547. Let me know
>  if you have questions.
> 
> thanks,
> 
> -venu
> 
> Thanks,
> 
> Ben.
> 
> ---
> This email message is for the sole use of the intended recipient(s) and may 
> contain
> confidential information.  Any unauthorized review, use, disclosure or 
> distribution
> is prohibited.  If you are not the intended recipient, please contact the 
> sender by
> reply email and destroy all copies of the original message.
> ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN/OVS code split: POC

[Ben Pfaff]

On Fri, Feb 22, 2019 at 04:15:27PM -0500, Mark Michelson wrote:
> On 2/22/19 4:01 PM, Ben Pfaff wrote:
> > On Mon, Feb 18, 2019 at 09:50:56AM -0500, Mark Michelson wrote:
> > > Hi everyone,
> > > 
> > > I have completed a *rough* POC of an OVN/OVS code split. You can find it 
> > > at
> > > https://github.com/putnopvut/ovn.git
> > > 
> > > Please take a look at the README file since that highlights how the split
> > > was done, as well as some known issues.
> > > 
> > > My attitude towards this repo is that it is a throwaway prototype. It
> > > essentially proves the concept that OVN can include OVS as a git subtree,
> > > while not really attempting to do a production-ready job of
> > > doing so. There is a lot of unfinished work.
> > 
> > It looks like the sort of thing I'd expect to see.
> > 
> > One thing I don't expect to see for a first cut or an nth cut is
> > perfection.  Stuff in corner cases is going to be broken.  That's the
> > nature of big changes.  We'll fix them as we notice them.
> > 
> > The manpages.mk errors are from the rule in the Makefile that tries to
> > rebuild manpages.mk.  That's why touching it helps, since it keeps the
> > Makefile from trying to rebuild it.
> > 
> > What do you think is your next step?
> > 
> 
> Hi Ben,
> 
> Thanks for the reply. My current step is to get buy-in for this approach. If
> people have suggestions for improvement or outright disapproval (with
> logical reasoning of course), then I'll iterate and try to take those
> suggestions into consideration.
> 
> If people are 100% cool with the approach I've taken, then the next step
> will be to come up with a plan to officially split the OVN code out from
> OVS. It'll need a plan for a few reasons:
> 
> 1) We'll need to get an official OVN repo to perform the work in.

That is easy.  I (or any other OVS committer) can create one under the
openvswitch organization on github.  I'm happy to do so whenever you
like, even now.

There's also the "ovn-org".  Justin is the sole owner of that org
currently.  I guess he'll probably add more people.

(It's easy to transfer repos between orgs, so no worries about that.)

> 2) We'll essentially need to have a "freeze" on OVN development during the
> split. Otherwise, changes that occur between the start of the split and the
> completion of the split could get lost. Or it will require some porting of
> the changes between repos. Either way, not a fun task.

Good point.

> 3) We'll want to have testing plans in place. Tests will need to be focused
> on ensuring that OVN works with compatible versions of OVS. Compatibility is
> something that we've avoided trying to make official stances on, but that
> could be a sub-item to perform here as well.

Do you plan to make a proposal here?  

Are you more concerned about OVN or OVS for testing?  It's easy for OVN
to accidentally add a dependency on some later-than-intended version of
OVS or OVSDB (although maybe just testing with the intended version
helps?) but it's also easy for OVS to accidentally make some change that
breaks OVN.

> 4) We'll want to be sure we're all on the same page regarding when the OVN
> split occurs. Will we do it at the same point that OVS 2.12 is being
> released, or will we do it as soon as possible and have OVN start its own
> versioning independent of OVS immediately?

Is there any advantage to waiting, once we're ready?

This might be a good time to increment the OVS major number to 3.0.

> Once all of the above are determined, then I think it's a matter of
> essentially re-doing what I've done already, but in a more official capacity
> (i.e. using an official repo, making more coherent commits, ensuring builds
> work across platforms). Once there is a functioning repo in which work can
> be done, we can work on the finer details (i.e. removing additional
> irrelevant files, reworking documentation). And once all of that is done,
> then I think the administrative details can become important (i.e.
> determining committers, updating websites, mailing lists, etc.)

Oh my, it'll take a long time to get that right.

I wonder whether OVN should adopt a more "modern" workflow, such as
Github pull requests instead of emails.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN/OVS code split: POC

[Mark Michelson]

On 2/22/19 4:01 PM, Ben Pfaff wrote:

On Mon, Feb 18, 2019 at 09:50:56AM -0500, Mark Michelson wrote:

Hi everyone,

I have completed a *rough* POC of an OVN/OVS code split. You can find it at
https://github.com/putnopvut/ovn.git

Please take a look at the README file since that highlights how the split
was done, as well as some known issues.

My attitude towards this repo is that it is a throwaway prototype. It
essentially proves the concept that OVN can include OVS as a git subtree,
while not really attempting to do a production-ready job of
doing so. There is a lot of unfinished work.


It looks like the sort of thing I'd expect to see.

One thing I don't expect to see for a first cut or an nth cut is
perfection.  Stuff in corner cases is going to be broken.  That's the
nature of big changes.  We'll fix them as we notice them.

The manpages.mk errors are from the rule in the Makefile that tries to
rebuild manpages.mk.  That's why touching it helps, since it keeps the
Makefile from trying to rebuild it.

What do you think is your next step?



Hi Ben,

Thanks for the reply. My current step is to get buy-in for this 
approach. If people have suggestions for improvement or outright 
disapproval (with logical reasoning of course), then I'll iterate and 
try to take those suggestions into consideration.


If people are 100% cool with the approach I've taken, then the next step 
will be to come up with a plan to officially split the OVN code out from 
OVS. It'll need a plan for a few reasons:


1) We'll need to get an official OVN repo to perform the work in.

2) We'll essentially need to have a "freeze" on OVN development during 
the split. Otherwise, changes that occur between the start of the split 
and the completion of the split could get lost. Or it will require some 
porting of the changes between repos. Either way, not a fun task.


3) We'll want to have testing plans in place. Tests will need to be 
focused on ensuring that OVN works with compatible versions of OVS. 
Compatibility is something that we've avoided trying to make official 
stances on, but that could be a sub-item to perform here as well.


4) We'll want to be sure we're all on the same page regarding when the 
OVN split occurs. Will we do it at the same point that OVS 2.12 is being 
released, or will we do it as soon as possible and have OVN start its 
own versioning independent of OVS immediately?


Once all of the above are determined, then I think it's a matter of 
essentially re-doing what I've done already, but in a more official 
capacity (i.e. using an official repo, making more coherent commits, 
ensuring builds work across platforms). Once there is a functioning repo 
in which work can be done, we can work on the finer details (i.e. 
removing additional irrelevant files, reworking documentation). And once 
all of that is done, then I think the administrative details can become 
important (i.e. determining committers, updating websites, mailing 
lists, etc.)


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

Re: [ovs-dev] [PATCH] ofctl: break the loop if ovs_pcap_read returns error

[Ben Pfaff]

On Mon, Feb 18, 2019 at 10:56:38AM +0800, Li RongQing wrote:
> otherwise packet is NULL, and dereference it to cause segfault
> 
> Signed-off-by: Li RongQing 

Thanks!  I applied this to master and backported it as far as
branch-2.6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC v4 1/1] datapath: Add a new action check_pkt_len

[Gregory Rose]

Numan,

I intend to test and review this on my local net-next branch but I'm not 
feeling well so it will probably be

early next week before I can do that.  Sorry for the delay...

- Greg

On 2/21/2019 10:42 AM, nusid...@redhat.com wrote:

From: Numan Siddique 

[Please note, this patch is submitted as RFC in ovs-dev ML to
get feedback before submitting to netdev ML. You need net-next tree
to apply this patch]

This patch adds a new action - 'check_pkt_len' which checks the
packet length and executes a set of actions if the packet
length is greater than the specified length or executes
another set of actions if the packet length is lesser or equal to.

This action takes below nlattrs
   * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for

   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER - Nested actions
 to apply if the packet length is greater than the specified 'pkt_len'

   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested
 actions to apply if the packet length is lesser or equal to the
 specified 'pkt_len'.

The main use case for adding this action is to solve the packet
drops because of MTU mismatch in OVN virtual networking solution.
When a VM (which belongs to a logical switch of OVN) sends a packet
destined to go via the gateway router and if the nic which provides
external connectivity, has a lesser MTU, OVS drops the packet
if the packet length is greater than this MTU.

With the help of this action, OVN will check the packet length
and if it is greater than the MTU size, it will generate an
ICMP packet (type 3, code 4) and includes the next hop mtu in it
so that the sender can fragment the packets.

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
Suggested-by: Ben Pfaff 
Signed-off-by: Numan Siddique 
CC: Ben Pfaff 
CC: Greg Rose 
CC: Lorenzo Bianconi 
---

v3 -> v4

  * v4 only has 1 patch - datapath patch which implements the
  * check_pkt_len action
  * Addressed the review comments from Lorenzo, Ben and Greg


  include/uapi/linux/openvswitch.h |  42 
  net/openvswitch/actions.c|  49 +
  net/openvswitch/flow_netlink.c   | 171 +++
  3 files changed, 262 insertions(+)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index dbe0cbe4f1b7..05ab885c718d 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -798,6 +798,44 @@ struct ovs_action_push_eth {
struct ovs_key_ethernet addresses;
  };
  
+/*

+ * enum ovs_check_pkt_len_attr - Attributes for %OVS_ACTION_ATTR_CHECK_PKT_LEN.
+ *
+ * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.
+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*
+ * actions to apply if the packer length is greater than the specified
+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
+ * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested OVS_ACTION_ATTR_*
+ * actions to apply if the packer length is lesser or equal to the specified
+ * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
+ */
+enum ovs_check_pkt_len_attr {
+   OVS_CHECK_PKT_LEN_ATTR_UNSPEC,
+   OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
+   OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
+   OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
+   __OVS_CHECK_PKT_LEN_ATTR_MAX,
+
+#ifdef __KERNEL__
+   OVS_CHECK_PKT_LEN_ATTR_ARG  /* struct check_pkt_len_arg  */
+#endif
+};
+
+#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
+
+#ifdef __KERNEL__
+struct check_pkt_len_arg {
+   u16 pkt_len;/* Same value as OVS_CHECK_PKT_LEN_ATTR_PKT_LEN'. */
+   bool exec_for_greater;  /* When true, actions in IF_GREATE will
+* not change flow keys. False otherwise.
+*/
+   bool exec_for_lesser_equal; /* When true, actions in IF_LESS_EQUAL
+* will not change flow keys. False
+* otherwise.
+*/
+};
+#endif
+
  /**
   * enum ovs_action_attr - Action types.
   *
@@ -842,6 +880,9 @@ struct ovs_action_push_eth {
   * packet, or modify the packet (e.g., change the DSCP field).
   * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
   * actions without affecting the original packet and key.
+ * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set
+ * of actions if greater than the specified packet length, else execute
+ * another set of actions.
   *
   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not 
all
   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -876,6 +917,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_POP_NSH,  /* No argument. */
OVS_ACTION_ATTR_METER,/* u32 meter ID. */
OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*.  */
+  

Re: [ovs-dev] [PATCH v1 1/1] hash: Enable hash_bytes128 optimization for aarch64.

[Ben Pfaff]

On Mon, Feb 18, 2019 at 05:46:01AM +, Yanqin Wei (Arm Technology China) 
wrote:
> "hash_bytes128" has two versions for 64 bits and 32 bits system. This
> should be common optimization for their respective platforms.
> But 64 bits version was only enabled in x86-64. This patch enable it for
> aarch64 platform.
> Micro benchmarking test was run in two kinds of arm platform.  It was
> observed that 50% performance improvement in thunderX2 and 40%
> improvement in TaiShan(Cortex-A72).
> 
> Signed-off-by: Yanqin Wei 

Thanks for working to make OVS better.

This patch is whitespace damaged.  For instance, lines that should begin
with spaces lack them.  I cannot apply it.

You can send the patch another way (for example, with "git send-email")
or use a Github pull request.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN/OVS code split: POC

[Ben Pfaff]

On Mon, Feb 18, 2019 at 09:50:56AM -0500, Mark Michelson wrote:
> Hi everyone,
> 
> I have completed a *rough* POC of an OVN/OVS code split. You can find it at
> https://github.com/putnopvut/ovn.git
> 
> Please take a look at the README file since that highlights how the split
> was done, as well as some known issues.
> 
> My attitude towards this repo is that it is a throwaway prototype. It
> essentially proves the concept that OVN can include OVS as a git subtree,
> while not really attempting to do a production-ready job of
> doing so. There is a lot of unfinished work.

It looks like the sort of thing I'd expect to see.

One thing I don't expect to see for a first cut or an nth cut is
perfection.  Stuff in corner cases is going to be broken.  That's the
nature of big changes.  We'll fix them as we notice them.

The manpages.mk errors are from the rule in the Makefile that tries to
rebuild manpages.mk.  That's why touching it helps, since it keeps the
Makefile from trying to rebuild it.

What do you think is your next step?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch: Escape range operators inside regex.

[Ben Pfaff]

On Mon, Feb 18, 2019 at 12:03:42PM -0500, Aaron Conole wrote:
> Ilya Maximets  writes:
> 
> > ' -(' matches a single character in the range between ' ' (index 32)
> > and '(' (index 40). This leads to the false positive:
> >
> >   WARNING: Line lacks whitespace around operator
> >   #445 FILE: ovsdb/monitor.c:573:
> >   if (--mcs->n_refs == 0) {
> >
> > Need to escape '-' to have a right behaviour.
> > This patch additionally escapes all other '-' chars in the similar
> > regexes and makes them be one per line to ease the review in case of
> > future changes.
> >
> > Basic unit tests added.
> >
> > CC: Joe Stringer 
> > Fixes: 0d7b16daea50 ("checkpatch: Check for infix operator whitespace.")
> > Signed-off-by: Ilya Maximets 
> > ---
> 
> Acked-by: Aaron Conole 
> 
> Thanks for fixing this, Ilya!

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

Re: [ovs-dev] [PATCH] ovs-tcpdump: Fix an undefined variable

[Ben Pfaff]

On Tue, Feb 19, 2019 at 05:34:53PM +0300, Ilya Maximets wrote:
> > On Mon, Feb 04, 2019 at 11:50:22AM -0500, Aaron Conole wrote:
> >> Hyong Youb Kim via dev  writes:
> >> 
> >> > From: Hyong Youb Kim 
> >> >
> >> > Run ovs-tcpdump without --span, and it throws the following
> >> > exception. Define mirror_select_all to avoid the error.
> >> >
> >> > Traceback (most recent call last):
> >> >   File "/usr/local/bin/ovs-tcpdump", line 488, in 
> >> > main()
> >> >   File "/usr/local/bin/ovs-tcpdump", line 454, in main
> >> > mirror_select_all)
> >> > UnboundLocalError: local variable 'mirror_select_all' referenced before 
> >> > assignment
> >> >
> >> > Fixes: 0475db71c650 ("ovs-tcpdump: Add --span to mirror all ports on 
> >> > bridge.")
> >> >
> >> > Signed-off-by: Hyong Youb Kim 
> >> > Acked-by: Ilya Maximets 
> >> > ---
> >> 
> >> Acked-by: Aaron Conole 
> > 
> > Thanks, Hyong (and Aaron).  I applied this to master and backported it
> > as far as it would go.
> 
> Hi Ben.
> Looks like you backported this patch too far. The original patch that
> is fixed here exists only starting from 2.11.
> 
> On branches 2.6 - 2.10 'make flake8-check' fails now:
> 
>   utilities/ovs-tcpdump.in:380:5: F841 local variable 'mirror_select_all'
>is assigned to but never used
>   Makefile:6401: recipe for target 'flake8-check' failed
> 
> 
> We probably should revert this fix from branches 2.6 to 2.10.

Oops.

I reverted it from those branches.  Thank you!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3] netlink: added check to prevent netlink attribute overflow

[Ben Pfaff]

On Tue, Feb 19, 2019 at 10:55:02AM -0800, Toms Atteka wrote:
> If enough large input is passed to odp_actions_from_string it can
> cause netlink attribute to overflow.
> Check for buffer size was added to prevent entering this function
> and returning appropriate error code.
> 
> Basic manual testing was performed.
> 
> Reported-by:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231
> Signed-off-by: Toms Atteka 

Thanks, I applied this to master and backported it as far as needed.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.10 1/2] Set release date for 2.10.2.

[Flavio Leitner]

On Wed, Feb 20, 2019 at 04:47:14PM -0800, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 
> ---

Your patches are okay, but unfortunately while testing I found an
issue which is resolved by the commit below in branch-2.11 and master:

commit 024d93f62ca7a7fef9c8fc5f69a38b60ebf7e0ab
Author: Ben Pfaff 
Date:   Wed Nov 14 16:07:30 2018 -0800

ofp-actions: Make all actions a multiple of OFPACT_ALIGNTO bytes.


Otherwise the ovs-testcontroller segfaults:

1275: bridge - add port after stopping controller FAILED
(bridge.at:99)

#0  0x7faa27ea553f in raise () from /lib64/libc.so.6
#1  0x7faa27e8f895 in abort () from /lib64/libc.so.6
#2  0x0044cfab in ofputil_protocol_to_ofp_version (protocol=(unknown: 
0)) at lib/ofp-protocol.c:123
#3  0x0043152f in ofputil_encode_flow_mod (fm=0x7ffc00eae900, 
protocol=(unknown: 0)) at lib/ofp-flow.c:370
#4  0x00409bf9 in lswitch_handshake (sw=0x1226680) at 
lib/learning-switch.c:219

Notice that protocol is zeroed. It happens when ofpact_init() is called.

branch-2.9 seems to have the bug as well but the test doesn't reproduce the
issue.

fbl

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

[ovs-dev] STPS - Trámites de capacitación

[Aviso Importante]

Cursos esenciales - Webinar Interactivo – Martes 05 de Marzo

Capacitación a tu Medida Guía paso a paso: 
Trámites de capacitación ante la S.T.P.S.

En este webinar te presentamos una guía práctica paso a paso del proceso que 
debemos seguir ante este organismo para 
registrar la capacitación de nuestro personal así como la documentación 
necesaria para cumplir ante una supervisión o auditoría. 

Para mayor información, responder sobre este correo con la palabra STPS + los 
siguientes datos:


NOMBRE:

TELÉFONO:

EMPRESA: 

EMAIL ALTERNO: 

Llamanos al (045) 55 1554 6630
www.Innovalearn.mx 


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

Re: [ovs-dev] [PATCH] OVN: select a random mac_prefix if not provided

[Ben Pfaff]

On Wed, Feb 20, 2019 at 04:53:45PM +0100, Lorenzo Bianconi wrote:
> Select a random IPAM mac_prefix if it has not been provided by the user.
> With this patch the admin can avoid to configure mac_prefix in order to
> avoid L2 address collisions if multiple OVN deployments share the same
> broadcast domain.
> Remove MAC_ADDR_PREFIX definitions/occurrences since now mac_prefix is
> always provided to ovn-northd
> 
> Tested-by: Miguel Duarte de Mora Barroso 
> Signed-off-by: Lorenzo Bianconi 

It's really not a good idea to modify an idl object in-place like this.
That's why they're all 'const'!  The IDL doesn't expect this kind of
thing and if it's not causing trouble now it could easily do so later:
> +} else {
> +eth_addr_random(_prefix);
> +memset(_prefix.ea[3], 0, 3);
> +
> +char *addr_prefix = xasprintf(ETH_ADDR_FMT, 
> ETH_ADDR_ARGS(mac_prefix));
> +smap_add((struct smap *)>options, "mac_prefix", addr_prefix);
> +nbrec_nb_global_set_options(nb, >options);
> +free(addr_prefix);
>  }

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

[ovs-dev] Creating your own VPORT type

[Prathamesh PRABHUDESAI]

Hello Team

I am using ovs-2.8 and I want to create an ipinip6 tunnel. I am using
native tunneling. Do I need to create a VPORT_TYPE for this? and what
need to be done in order to create that? Otherwise its failing because of
UNSPEC_OVS_VPORT_TYPE.

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

Re: [ovs-dev] [PATCH V5 0/2] Do not rewrite fields with the same values as matched

[Ben Pfaff]

On Sun, Feb 17, 2019 at 09:18:46AM +, Eli Britstein wrote:
> This patch set avoids unnecessary rewrite actions to fields with the
> same values as matched on.
> 
> Patch 1 is a pre-step of generating ovs key fields macros
> Patch 2 avoids the unnecessary rewrites and adapts the tests accordingly

Thanks for the revision.

Do you foresee other uses of OVS_KEY_FIELD in the future?  As is,
there's a lot of duplication here from the numerous declarations like

struct ovs_key_field_properties ovs_key_nd_extensions_properties[] = {
#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_nd_extensions, 
name), sizeof(type)},
OVS_KEY_ND_EXTENSIONS_FIELDS
{0, 0}
#undef OVS_KEY_FIELD
};

If this is the only currently foreseen use, it would be better to have
the code generator just generate the declarations directly instead of
forcing these later duplications.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/5] ovn: Add HA chassis group and 'external' port support

[Mark Michelson]

On 2/20/19 11:12 PM, Numan Siddique wrote:


Thanks for the review and comments.

Please see below for few comments.

Thanks
Numan


On Thu, Feb 21, 2019 at 3:29 AM Mark Michelson > wrote:


Hi Numan,

This is quite a large patchset, but I believe I understand it. It
essentially does two things:

1) Deprecates Gateway_Chassis in favor of HA_Chassis_Groups and
HA_Chassis. The benefit of HA_Chassis_Groups is that it allows for
automatic failover of chassis using BFD.


The existing Gateway_Chassis also supports automatic failover using BFD.

(1) actully doesn't add any new extra functionality to support HA for 
gateway router ports.
It only provides a different way to do the same HA. (1) maintains full 
backward compatibility so
that CMS can still use Gateway_Chassis in NB DB and ovn-northd will 
create HA_Chassis_Group

in SB DB instead of creating Gateway_Chassis in SB DB.

2) Adds external logical switch port type that makes use of
HA_Chassis_Groups to allow for services (such as DHCP) to be offered on
ports outside of the OVN environment. This uses the
HA_Chassis_Groups in
order to "bind" the external port to a specific instance of
ovn-controller.

The problem I'm having is that I don't really understand what value (1)
is adding. It seems like it's just changing the syntax behind the
existing Gateway_Chassis. You can already specify multiple
Gateway_Chassis for a logical router port, and you can associate a
priority with a Gateway_Chassis, allowing for controlled failover. And
it seems like you could do the same thing for (2) as well. Am I missing
something?


The present Gateway_Chassis approach is very much tied to the logical 
router ports.
And I felt odd to associate a set of Gateway_Chassis to  'external' 
logical ports.
Right now the association of the logical_router_port to Gateway_Chassis 
in NB DB

is a strong reference.

If we want to use the existing Gateway_Chassis , we need to add a column 
'gateway_chassis' in
Logical_switch_port.  If suppose there are like 10 'external' ports in a 
logical switch, CMS has to create 10 sets
of Gateway_Chassis rows (with each set representing few chassis which 
provides gateway

functionality) and associate each set to the logical_switch_port.

Right now Gateway_Chassis table in the schema doesn't have 'is_Root' set 
to True.
So I think using the present Gateway_Chassis table for the HA of 
'external' ports would be more complicated
for CMS and also semantically it feels odd for a logical port to have 
Gatway_Chassis associated to it.


That's the reason I thought I would add a generic table which could be 
used by both distributed logical router ports

and 'external' ports.

This patch series also simplifies the code to establish BFD tunnels. I 
felt the present

code [1] which does this is quite complicated.

Please let me  know your thoughts and and if you have any more questions.


Thanks for the explanation Numan. Everything you've brought up sounds 
good to me. I especially like the idea of being able to update a 
HA_Chassis_Group instead of having to update lots of individual switch 
or router ports.




[1] - 
https://github.com/openvswitch/ovs/blob/master/ovn/controller/bfd.c#L210

https://github.com/openvswitch/ovs/blob/master/ovn/controller/bfd.c#L91


As far as individual critiques of the code, I would recommend
double-checking the spelling of "chassis" throughout the code.
Sometimes
it's "chassi" and other times it's "chasss". Also, in
ovn/controller/physical.c, I noticed that the "Gateway_Chassis" is
still
referenced in an error message instead of HA_Chassis. Other than
that, I
didn't see anything that was jumped out as wrong.


Thanks I will address that.

Numan


On 2/18/19 2:26 PM, nusid...@redhat.com 
wrote:
 > From: Numan Siddique mailto:nusid...@redhat.com>>
 >
 > This patch series adds a generic HA chassis group support in OVN
 > deprecating the existing Gateway chassis support. The final patch
 > of the series adds the 'external' port support in OVN.
 > The 'external' port patch addresses the review comments from Han Zhou
 > which he provided when 'external' port patch was submitted without
 > the HA support.
 >
 > A generic HA chassis group support is added so that both the
distributed
 > logical router ports (providing gateway functionality) and 'external'
 > ports can use it for HA and also to simplify the existing HA code
 > (which seems to be a bit complicated).
 >
 > To support HA, BFD is configured on tunnel ports. And even though
 > 'external' ports are expected to be used with the logical
 > switches having localnet ports (representing physical networks),
 > BFD is used for now since each chassis uses geneve tunnels with
 > all other chassis in the OVN cluster.
 >
 > 

Re: [ovs-dev] [PATCH v2 1/3] netdev-dpdk: Expose flow creation/destruction calls

[Roni Bar Yanai]

> -Original Message-
> From: Ilya Maximets 
> Sent: Friday, February 22, 2019 1:26 PM
> To: Roni Bar Yanai ; Ophir Munk
> ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Olga Shern ;
> Kevin Traynor ; Asaf Penso ;
> Flavio Leitner 
> Subject: Re: [PATCH v2 1/3] netdev-dpdk: Expose flow creation/destruction
> calls
> 
> On 21.02.2019 19:37, Roni Bar Yanai wrote:
> >
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Thursday, February 21, 2019 5:27 PM
> >> To: Ophir Munk ; ovs-dev@openvswitch.org
> >> Cc: Ian Stokes ; Olga Shern
> ;
> >> Kevin Traynor ; Asaf Penso
> ;
> >> Roni Bar Yanai ; Flavio Leitner
> 
> >> Subject: Re: [PATCH v2 1/3] netdev-dpdk: Expose flow
> creation/destruction
> >> calls
> >>
> >> On 21.02.2019 15:07, Ophir Munk wrote:
> >>> From: Roni Bar Yanai 
> >>>
> >>> Before offloading-code was added to the netdev-dpdk.c file (MARK and
> >>> RSS actions) the only DPDK RTE calls in use were rte_flow_create() and
> >>> rte_flow_destroy(). In preparation for splitting the offloading-code
> >>> from the netdev-dpdk.c file to a separate file, it is required
> >>> to embed these RTE calls into a global netdev-dpdk-* API so that
> >>> they can be called from the new file. An example for this requirement
> >>> can be seen in the handling of dpdk_mutex, which should be
> encapsulated
> >>
> >> You probably meant dev->mutex.
> >>
> >>> inside netdev-dpdk class (netdev-dpdk.c file), and should be unknown
> >>> to the outside callers. This commit embeds the rte_flow_create() call
> >>> inside the netdev_dpdk_flow_create() API and the rte_flow_destroy()
> >>> call inside the netdev_dpdk_rte_flow_destroy() API.
> >>>
> >>> Reviewed-by: Asaf Penso 
> >>> Signed-off-by: Roni Bar Yanai 
> >>> Signed-off-by: Ophir Munk 
> >>> ---
> >>>  lib/netdev-dpdk.c | 51
> >> +++
> >>>  lib/netdev-dpdk.h | 14 ++
> >>>  2 files changed, 53 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >>> index 32a6ffd..163d4ec 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -4203,6 +4203,42 @@ unlock:
> >>>  return err;
> >>>  }
> >>>
> >>> +int
> >>> +netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
> >>> + struct rte_flow *rte_flow,
> >>> + struct rte_flow_error *error)
> >>> +{
> >>> +if (!is_dpdk_class(netdev->netdev_class)) {
> >>> +return -1;
> >>> +}
> >>
> >> Not sure if this check is needed, because we're registering
> >> this offload API only for DPDK devices. However, it's not
> >> correct anyway, because 'is_dpdk_class' will return 'true'
> >> for vhost netdevs which are not rte_ethdev devices, i.e. has
> >> no offloading API.
> >>
> >>> +
> >>> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >>> +int ret;
> >>> +
> >>> +ovs_mutex_lock(>mutex);
> >>> +ret = rte_flow_destroy(dev->port_id, rte_flow, error);
> >>> +ovs_mutex_unlock(>mutex);
> >>> +return ret;
> >>> +}
> >>> +
> >>> +struct rte_flow *
> >>> +netdev_dpdk_rte_flow_create(struct netdev *netdev,
> >>> +const struct rte_flow_attr *attr,
> >>> +const struct rte_flow_item *items,
> >>> +const struct rte_flow_action *actions,
> >>> +struct rte_flow_error *error)
> >>> +{
> >>> +if (!is_dpdk_class(netdev->netdev_class)) {
> >>> +return NULL;
> >>> +}
> >>
> >> Same here.
> >>
> >>> +
> >>> +struct rte_flow *flow;
> >>> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >>
> >> It's better to have an empty line between declarations and code.
> >>
> >>> +ovs_mutex_lock(>mutex);
> >>> +flow = rte_flow_create(dev->port_id, attr, items, actions, error);
> >>> +ovs_mutex_unlock(>mutex);
> >>> +return flow;
> >>> +}
> >>>
> >>>  /* Find rte_flow with @ufid */
> >>>  static struct rte_flow *
> >>> @@ -4554,7 +4590,6 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev *netdev,
> >>>   size_t actions_len OVS_UNUSED,
> >>>   const ovs_u128 *ufid,
> >>>   struct offload_info *info) {
> >>> -struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >>>  const struct rte_flow_attr flow_attr = {
> >>>  .group = 0,
> >>>  .priority = 0,
> >>> @@ -4759,15 +4794,12 @@ end_proto_check:
> >>>  mark.id = info->flow_mark;
> >>>  add_flow_action(, RTE_FLOW_ACTION_TYPE_MARK,
> );
> >>>
> >>> -ovs_mutex_lock(>mutex);
> >>>
> >>>  rss = add_flow_rss_action(, netdev);
> >>
> > Just wondering what will happen if rss action is added with current n_rxq,
> and immediately after
> > there is a reconfiguration. The result will be exactly the same, rss flow 
> > has
> wrong
> > configuration. The lock doesn't solve this issue.
> 
> And I'm 

Re: [ovs-dev] [PATCH v2 1/3] netdev-dpdk: Expose flow creation/destruction calls

[Ilya Maximets]

On 21.02.2019 19:37, Roni Bar Yanai wrote:
> 
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Thursday, February 21, 2019 5:27 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org
>> Cc: Ian Stokes ; Olga Shern ;
>> Kevin Traynor ; Asaf Penso ;
>> Roni Bar Yanai ; Flavio Leitner 
>> Subject: Re: [PATCH v2 1/3] netdev-dpdk: Expose flow creation/destruction
>> calls
>>
>> On 21.02.2019 15:07, Ophir Munk wrote:
>>> From: Roni Bar Yanai 
>>>
>>> Before offloading-code was added to the netdev-dpdk.c file (MARK and
>>> RSS actions) the only DPDK RTE calls in use were rte_flow_create() and
>>> rte_flow_destroy(). In preparation for splitting the offloading-code
>>> from the netdev-dpdk.c file to a separate file, it is required
>>> to embed these RTE calls into a global netdev-dpdk-* API so that
>>> they can be called from the new file. An example for this requirement
>>> can be seen in the handling of dpdk_mutex, which should be encapsulated
>>
>> You probably meant dev->mutex.
>>
>>> inside netdev-dpdk class (netdev-dpdk.c file), and should be unknown
>>> to the outside callers. This commit embeds the rte_flow_create() call
>>> inside the netdev_dpdk_flow_create() API and the rte_flow_destroy()
>>> call inside the netdev_dpdk_rte_flow_destroy() API.
>>>
>>> Reviewed-by: Asaf Penso 
>>> Signed-off-by: Roni Bar Yanai 
>>> Signed-off-by: Ophir Munk 
>>> ---
>>>  lib/netdev-dpdk.c | 51
>> +++
>>>  lib/netdev-dpdk.h | 14 ++
>>>  2 files changed, 53 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 32a6ffd..163d4ec 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -4203,6 +4203,42 @@ unlock:
>>>  return err;
>>>  }
>>>
>>> +int
>>> +netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
>>> + struct rte_flow *rte_flow,
>>> + struct rte_flow_error *error)
>>> +{
>>> +if (!is_dpdk_class(netdev->netdev_class)) {
>>> +return -1;
>>> +}
>>
>> Not sure if this check is needed, because we're registering
>> this offload API only for DPDK devices. However, it's not
>> correct anyway, because 'is_dpdk_class' will return 'true'
>> for vhost netdevs which are not rte_ethdev devices, i.e. has
>> no offloading API.
>>
>>> +
>>> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>> +int ret;
>>> +
>>> +ovs_mutex_lock(>mutex);
>>> +ret = rte_flow_destroy(dev->port_id, rte_flow, error);
>>> +ovs_mutex_unlock(>mutex);
>>> +return ret;
>>> +}
>>> +
>>> +struct rte_flow *
>>> +netdev_dpdk_rte_flow_create(struct netdev *netdev,
>>> +const struct rte_flow_attr *attr,
>>> +const struct rte_flow_item *items,
>>> +const struct rte_flow_action *actions,
>>> +struct rte_flow_error *error)
>>> +{
>>> +if (!is_dpdk_class(netdev->netdev_class)) {
>>> +return NULL;
>>> +}
>>
>> Same here.
>>
>>> +
>>> +struct rte_flow *flow;
>>> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>
>> It's better to have an empty line between declarations and code.
>>
>>> +ovs_mutex_lock(>mutex);
>>> +flow = rte_flow_create(dev->port_id, attr, items, actions, error);
>>> +ovs_mutex_unlock(>mutex);
>>> +return flow;
>>> +}
>>>
>>>  /* Find rte_flow with @ufid */
>>>  static struct rte_flow *
>>> @@ -4554,7 +4590,6 @@ netdev_dpdk_add_rte_flow_offload(struct
>> netdev *netdev,
>>>   size_t actions_len OVS_UNUSED,
>>>   const ovs_u128 *ufid,
>>>   struct offload_info *info) {
>>> -struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>  const struct rte_flow_attr flow_attr = {
>>>  .group = 0,
>>>  .priority = 0,
>>> @@ -4759,15 +4794,12 @@ end_proto_check:
>>>  mark.id = info->flow_mark;
>>>  add_flow_action(, RTE_FLOW_ACTION_TYPE_MARK, );
>>>
>>> -ovs_mutex_lock(>mutex);
>>>
>>>  rss = add_flow_rss_action(, netdev);
>>
> Just wondering what will happen if rss action is added with current n_rxq, 
> and immediately after
> there is a reconfiguration. The result will be exactly the same, rss flow has 
> wrong
> configuration. The lock doesn't solve this issue.

And I'm periodically forgetting about the fact that all offloading
operations are guarded by datapath port_mutex. So, the race below
is not really possible. However, IMHO, having everything under the
datapath port mutex is a bad design decision which is suitable only
for experimental feature. The worst part here is that offloading code
tries to make impression of the thread-safety by using concurrent
hash maps (but it doesn't even protect the insertion). Moving the
code to a separate module is one more step that tries to simulate the
independence. I'm not telling that we don't need that 

Re: [ovs-dev] [PATCH V5 0/2] Do not rewrite fields with the same values as matched

[Eli Britstein]

ping

On 2/17/2019 11:18 AM, Eli Britstein wrote:
> This patch set avoids unnecessary rewrite actions to fields with the
> same values as matched on.
>
> Patch 1 is a pre-step of generating ovs key fields macros
> Patch 2 avoids the unnecessary rewrites and adapts the tests accordingly
>
> Differences from V4:
> - Remove __ prefix
> - Remove unused __OVS_KEY_FIELDS_END
> Tests 1219-1220 failed because I had OVS running with VXLAN and GRE
> configured. Shutting it down and the tests pass.
>
> Thanks,
> Eli
>
> Eli Britstein (2):
>Makefiles: Generate datapath ovs key fields macros
>odp-util: Do not rewrite fields with the same values as matched
>
>   .gitignore  |   1 +
>   build-aux/extract-odp-netlink-xmacros-h |  53 ++
>   include/automake.mk |  11 ++-
>   lib/odp-util.c  | 122 +---
>   tests/mpls-xlate.at |   2 +-
>   tests/ofproto-dpif.at   |  14 +--
>   6 files changed, 180 insertions(+), 23 deletions(-)
>   create mode 100755 build-aux/extract-odp-netlink-xmacros-h
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev