[ovs-dev] [PATCH V2 1/2] ofproto-dpif-xlate: Change priority tags from boolean to enum

2019-05-11 Thread Eli Britstein 
Priority tags is a port configuration to determine how the port treats
priority tags, e.g. zero VLAN ID. Change the type from boolean to enum
as a pre-step towards introducing additional modes. The new options are
"omit", equivalent to previously "false", and "include-non-zero",
equivalent to previously "true". "true" is still supported for backwards
compatibility.

Signed-off-by: Eli Britstein 
Reviewed-by: Roi Dayan 
---
 ofproto/ofproto-dpif-xlate.c |  9 +
 ofproto/ofproto-dpif-xlate.h |  2 +-
 ofproto/ofproto-dpif.c   |  3 ++-
 ofproto/ofproto.h| 10 +-
 tests/ofproto-dpif.at|  6 +++---
 vswitchd/bridge.c|  8 ++--
 vswitchd/vswitch.xml |  4 ++--
 7 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5cee37f7b..4c1cdfa58 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -149,7 +149,8 @@ struct xbundle {
 * NULL if all VLANs are trunked. */
 unsigned long *cvlans; /* Bitmap of allowed customer vlans,
 * NULL if all VLANs are allowed */
-bool use_priority_tags;/* Use 802.1p tag for frames in VLAN 0? */
+enum port_priority_tags_mode use_priority_tags;
+   /* Use 802.1p tag for frames in VLAN 0? */
 bool floodable;/* No port has OFPUTIL_PC_NO_FLOOD set? */
 bool protected;/* Protected port mode */
 };
@@ -604,7 +605,7 @@ static void xlate_xbundle_set(struct xbundle *xbundle,
   enum port_vlan_mode vlan_mode,
   uint16_t qinq_ethtype, int vlan,
   unsigned long *trunks, unsigned long *cvlans,
-  bool use_priority_tags,
+  enum port_priority_tags_mode use_priority_tags,
   const struct bond *bond, const struct lacp *lacp,
   bool floodable, bool protected);
 static void xlate_xport_set(struct xport *xport, odp_port_t odp_port,
@@ -999,7 +1000,7 @@ static void
 xlate_xbundle_set(struct xbundle *xbundle,
   enum port_vlan_mode vlan_mode, uint16_t qinq_ethtype,
   int vlan, unsigned long *trunks, unsigned long *cvlans,
-  bool use_priority_tags,
+  enum port_priority_tags_mode use_priority_tags,
   const struct bond *bond, const struct lacp *lacp,
   bool floodable, bool protected)
 {
@@ -1316,7 +1317,7 @@ xlate_bundle_set(struct ofproto_dpif *ofproto, struct 
ofbundle *ofbundle,
  const char *name, enum port_vlan_mode vlan_mode,
  uint16_t qinq_ethtype, int vlan,
  unsigned long *trunks, unsigned long *cvlans,
- bool use_priority_tags,
+ enum port_priority_tags_mode use_priority_tags,
  const struct bond *bond, const struct lacp *lacp,
  bool floodable, bool protected)
 {
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 5a51d7b30..c947bd801 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -181,7 +181,7 @@ void xlate_bundle_set(struct ofproto_dpif *, struct 
ofbundle *,
   const char *name, enum port_vlan_mode,
   uint16_t qinq_ethtype, int vlan,
   unsigned long *trunks, unsigned long *cvlans,
-  bool use_priority_tags,
+  enum port_priority_tags_mode use_priority_tags,
   const struct bond *, const struct lacp *,
   bool floodable, bool protected);
 void xlate_bundle_remove(struct ofbundle *);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index db461ac07..3db97ca37 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -101,7 +101,8 @@ struct ofbundle {
 unsigned long *cvlans;
 struct lacp *lacp;  /* LACP if LACP is enabled, otherwise NULL. */
 struct bond *bond;  /* Nonnull iff more than one port. */
-bool use_priority_tags; /* Use 802.1p tag for frames in VLAN 0? */
+enum port_priority_tags_mode use_priority_tags;
+/* Use 802.1p tag for frames in VLAN 0? */
 
 bool protected; /* Protected port mode */
 
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 510ace103..caf5e50b5 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -416,6 +416,13 @@ enum port_vlan_mode {
 PORT_VLAN_DOT1Q_TUNNEL
 };
 
+/* The behaviour of the port regarding priority tags */
+enum port_priority_tags_mode {
+PORT_PRIORITY_TAGS_OMIT = 0,
+PORT_PRIORITY_TAGS_INCLUDE_NON_ZERO,
+};
+
+/* The behaviour of the port regarding priority tags */
 /* Configuration of bundles. */

[ovs-dev] [PATCH V2 2/2] ofproto-dpif-xlate: Add include mode to priority tags

2019-05-11 Thread Eli Britstein 
Configure "include-non-zero" priority tags to retain the 802.1Q header
when the VLAN ID is zero, except both the VLAN ID and priority are zero.
Add a "include" configuration option to retain the 802.1Q header in such
frames as well.

Signed-off-by: Eli Britstein 
Reviewed-by: Roi Dayan 
---
 NEWS |  4 
 ofproto/ofproto-dpif-xlate.c | 11 +++
 ofproto/ofproto.h|  1 +
 vswitchd/bridge.c|  2 ++
 vswitchd/vswitch.xml |  7 ---
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index 293531db0..7c5929fa6 100644
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,10 @@ Post-v2.11.0
  * New "ovs-appctl dpctl/ipf-get-status" command for userspace datapath
conntrack fragmentation support.
  * New action "check_pkt_len".
+ * Changed options for "other-config:priority-tags" port configuration:
+   "false"/"true" changed to "omit"/"include-non-zero", and added a new
+   option "include" to always retain the 802.1Q header, for frames with
+   both the VLAN ID and priority are zero as well.
- OVSDB:
  * OVSDB clients can now resynchronize with clustered servers much more
quickly after a brief disconnection, saving bandwidth and CPU time.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4c1cdfa58..28897bf67 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -549,7 +549,8 @@ static void xvlan_copy(struct xvlan *dst, const struct 
xvlan *src);
 static void xvlan_pop(struct xvlan *src);
 static void xvlan_push_uninit(struct xvlan *src);
 static void xvlan_extract(const struct flow *, struct xvlan *);
-static void xvlan_put(struct flow *, const struct xvlan *);
+static void xvlan_put(struct flow *, const struct xvlan *,
+  enum port_priority_tags_mode);
 static void xvlan_input_translate(const struct xbundle *,
   const struct xvlan *in,
   struct xvlan *xvlan);
@@ -2270,13 +2271,15 @@ xvlan_extract(const struct flow *flow, struct xvlan 
*xvlan)
 
 /* Put VLAN information (headers) to flow */
 static void
-xvlan_put(struct flow *flow, const struct xvlan *xvlan)
+xvlan_put(struct flow *flow, const struct xvlan *xvlan,
+  enum port_priority_tags_mode use_priority_tags)
 {
 ovs_be16 tci;
 int i;
 for (i = 0; i < FLOW_MAX_VLAN_HEADERS; i++) {
 tci = htons(xvlan->v[i].vid | (xvlan->v[i].pcp & VLAN_PCP_MASK));
-if (tci) {
+if (tci || ((use_priority_tags == PORT_PRIORITY_TAGS_INCLUDE) &&
+xvlan->v[i].tpid)) {
 tci |= htons(VLAN_CFI);
 flow->vlans[i].tpid = xvlan->v[i].tpid ?
   htons(xvlan->v[i].tpid) :
@@ -2450,7 +2453,7 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle 
*out_xbundle,
 }
 
 memcpy(_vlans, >xin->flow.vlans, sizeof(old_vlans));
-xvlan_put(>xin->flow, _xvlan);
+xvlan_put(>xin->flow, _xvlan, out_xbundle->use_priority_tags);
 
 compose_output_action(ctx, xport->ofp_port, use_recirc ?  : NULL,
   false, false);
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index caf5e50b5..2f83ee077 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -420,6 +420,7 @@ enum port_vlan_mode {
 enum port_priority_tags_mode {
 PORT_PRIORITY_TAGS_OMIT = 0,
 PORT_PRIORITY_TAGS_INCLUDE_NON_ZERO,
+PORT_PRIORITY_TAGS_INCLUDE,
 };
 
 /* The behaviour of the port regarding priority tags */
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 06c8bf4ca..930b66896 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1026,6 +1026,8 @@ port_configure(struct port *port)
 const char *pt = smap_get_def(>other_config, "priority-tags", "");
 if (!strcmp(pt, "include-non-zero") || !strcmp(pt, "true")) {
 s.use_priority_tags = PORT_PRIORITY_TAGS_INCLUDE_NON_ZERO;
+} else if (!strcmp(pt, "include")) {
+s.use_priority_tags = PORT_PRIORITY_TAGS_INCLUDE;
 } else {
 s.use_priority_tags = PORT_PRIORITY_TAGS_OMIT;
 }
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index d5f2b0186..37785a385 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -1860,7 +1860,7 @@
   
 
   
+  type='{"type": "string", "enum": ["set", ["omit", 
"include-non-zero", "include"]]}'>
 
   An 802.1Q header contains two important pieces of information: a VLAN
   ID and a priority.  A frame with a zero VLAN ID, called a
@@ -1877,8 +1877,9 @@
 
 
 
-  Regardless of this setting, Open vSwitch omits the 802.1Q header on
-  output if both the VLAN ID and priority would be zero.
+  For include-non-zero Open vSwitch omits the 802.1Q 
header on output
+  if both the VLAN ID and priority would be zero. Set to 
include
+  to retain the 802.1Q header in such frames as well.

[ovs-dev] [PATCH V2 0/2] Add include mode to priority tags port option

2019-05-11 Thread Eli Britstein 
Setting priority-tags to "true" Open vSwitch still omits the
802.1Q header on output if both the VLAN ID and priority would be zero.
Add an option to retain the 8021Q header in such frames as well.

Patch #1: change boolean to enum as a pre-step to adding addition option
Patch #2: add "include" mode for priority-tags configuration

V2 changes:
- Added NEWS item.
- A bit changed documentation.

Eli Britstein (2):
  ofproto-dpif-xlate: Change priority tags from boolean to enum
  ofproto-dpif-xlate: Add include mode to priority tags

 NEWS |  4 
 ofproto/ofproto-dpif-xlate.c | 20 
 ofproto/ofproto-dpif-xlate.h |  2 +-
 ofproto/ofproto-dpif.c   |  3 ++-
 ofproto/ofproto.h| 11 ++-
 tests/ofproto-dpif.at|  6 +++---
 vswitchd/bridge.c| 10 --
 vswitchd/vswitch.xml |  9 +
 8 files changed, 45 insertions(+), 20 deletions(-)

-- 
2.17.2

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

[ovs-dev] Joint Venture !!!!!!

2019-05-11 Thread Mr. Vladimir Oleg 
Dear sir,

I  represent a high net-worth investor who is seeking the attention of 
investors, project owners and general business facilitators away from  his 
native country. 

Can you help achieve this? kindly respond back for further details.

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

Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100% usage issue with put_mac_bindings

2019-05-11 Thread Numan Siddique 
On Sat, May 11, 2019 at 2:09 PM Numan Siddique  wrote:

>
>
> On Sat, May 11, 2019 at 5:38 AM Ankur Sharma 
> wrote:
>
>> Hi Han,
>>
>> Thanks for review.
>>
>> Yup, looks like it, although I did not try the scenario myself, but yes
>> entry are not getting removed once added,
>> hence, new mac bindings will not be added after some time (as existing
>> count reaches 1000).
>>
>> Regards,
>> Ankur
>>
>> From: Han Zhou 
>> Sent: Friday, May 10, 2019 4:47 PM
>> To: Ankur Sharma 
>> Cc: ovs-dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100% usage
>> issue with put_mac_bindings
>>
>>
>>
>> On Fri, May 10, 2019 at 2:46 PM Ankur Sharma > > wrote:
>> >
>> > ISSUE:
>> > a. As soon as entries get added to put_mac_bindings in pinctrl.c,
>> >ovn-controller CPU consumption reached 100%.
>> >
>> > b. This happens because in wait_put_mac_bindings,
>> >if !hmap_is_empty(_mac_bindings) succeeds and
>> >calls poll_immediat_wake().
>> >
>> >ovn-controller.log:
>> >"2019-05-10T19:43:28.035Z|00035|poll_loop|INFO|wakeup due to
>> > 0-ms timeout at ovn/controller/pinctrl.c:2520 (99% CPU usage)"
>> >
>> > ROOT_CAUSE:
>> > a. Earlier it used to work fine, because in run_put_mac_bindings
>> >all the bindings in put_mac_bindings would be flushed.
>> >
>> > b. Looks like, as a part of adding a new thread in pinctrl, this
>> >line got replaced with calling buffer_put_mac_bindings.
>> >
>> > "
>> > .
>> > .
>> > .
>> >/* Move the mac bindings from 'put_mac_bindings' hmap to
>> >  * 'buffered_mac_bindings' and notify the pinctrl_handler.
>> >  * pinctrl_handler will reinject the buffered packets. */
>> > if (!hmap_is_empty(_mac_bindings)) {
>> > buffer_put_mac_bindings();
>> > notify_pinctrl_handler();
>> > }
>> > "
>> >
>> > c. Because of which put_mac_bindings is never emptied and
>> wait_put_mac_bindings
>> >ends up doing poll_immediate_wake.
>> >
>> > FIX:
>> > a. Added call to flush_put_mac_bindings back in
>> >run_put_mac_bindings.
>> >
>> > b. Additioanlly, logic mentioned in ROOT_CAUSE.b has been changed
>> >in 1c24b2f490ba002bbfeb23006965188a7c5b9747, hence updated
>> >the documentation in pinctrl.c accordingly.
>> >
>> > Signed-off-by: Ankur Sharma > ankur.sha...@nutanix.com>>
>> > ---
>> >  ovn/controller/pinctrl.c | 9 ++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>> > index 8ae1f9b..b7bb4c9 100644
>> > --- a/ovn/controller/pinctrl.c
>> > +++ b/ovn/controller/pinctrl.c
>> > @@ -91,11 +91,13 @@ VLOG_DEFINE_THIS_MODULE(pinctrl);
>> >   *
>> >   *   - arp/nd_ns  - These actions generate an ARP/IPv6 Neighbor
>> solicit
>> >   *  requests. The original packets are buffered and
>> > - *  injected back when put_arp/put_nd actions are
>> called.
>> > + *  injected back when put_arp/put_nd resolves
>> > + *  corresponding ARP/IPv6 Neighbor solicit
>> requests.
>> >   *  When pinctrl_run(), writes the mac bindings
>> from the
>> >   *  'put_mac_bindings' hmap to the MAC_Binding
>> table in
>> > - *  SB DB, it moves these mac bindings to another
>> hmap -
>> > - *  'buffered_mac_bindings'.
>> > + *  SB DB, run_buffered_binding will add the
>> buffered
>> > + *  packets to buffered_mac_bindings and notify
>> > + *  pinctrl_handler.
>> >   *
>> >   *  The pinctrl_handler thread calls the function -
>> >   *  send_mac_binding_buffered_pkts(), which uses
>> > @@ -2468,6 +2470,7 @@ run_put_mac_bindings(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>> >  sbrec_mac_binding_by_lport_ip,
>> >  pmb);
>> >  }
>> > +"flush_put_mac_bindings();
>
>
> Thanks for the patch and finding the issue. This is a big mistake from me
> when I added the pinctrl_thread.
>
> Instead of calling  "flush_put_mac_bindings()" in the function
> "run_put_mac_bindings()"
> I would suggest to use - HMAP_FOR_EACH_POP in run_put_mac_bindings() and
> then free
> pmb after the call to run_put_mac_binding().
>
> If you see flush_put_mac_bindings() also does the same thing. We can avoid
> running the for loop twice.
>

You can ignore my suggestion. Calling flush_put_mac_bindings() seems much
cleaner.

Acked-by: Numan Siddique 

Thanks
Numan


> Thanks
> Numan
>
> >  }
>> >
>> >  static void
>> > --
>> > 1.8.3.1
>> >
>> > ___
>> > dev mailing list
>> > d...@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev [
>> mail.openvswitch.org]<
>>

Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100% usage issue with put_mac_bindings

2019-05-11 Thread Numan Siddique 
On Sat, May 11, 2019 at 5:38 AM Ankur Sharma 
wrote:

> Hi Han,
>
> Thanks for review.
>
> Yup, looks like it, although I did not try the scenario myself, but yes
> entry are not getting removed once added,
> hence, new mac bindings will not be added after some time (as existing
> count reaches 1000).
>
> Regards,
> Ankur
>
> From: Han Zhou 
> Sent: Friday, May 10, 2019 4:47 PM
> To: Ankur Sharma 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100% usage
> issue with put_mac_bindings
>
>
>
> On Fri, May 10, 2019 at 2:46 PM Ankur Sharma  > wrote:
> >
> > ISSUE:
> > a. As soon as entries get added to put_mac_bindings in pinctrl.c,
> >ovn-controller CPU consumption reached 100%.
> >
> > b. This happens because in wait_put_mac_bindings,
> >if !hmap_is_empty(_mac_bindings) succeeds and
> >calls poll_immediat_wake().
> >
> >ovn-controller.log:
> >"2019-05-10T19:43:28.035Z|00035|poll_loop|INFO|wakeup due to
> > 0-ms timeout at ovn/controller/pinctrl.c:2520 (99% CPU usage)"
> >
> > ROOT_CAUSE:
> > a. Earlier it used to work fine, because in run_put_mac_bindings
> >all the bindings in put_mac_bindings would be flushed.
> >
> > b. Looks like, as a part of adding a new thread in pinctrl, this
> >line got replaced with calling buffer_put_mac_bindings.
> >
> > "
> > .
> > .
> > .
> >/* Move the mac bindings from 'put_mac_bindings' hmap to
> >  * 'buffered_mac_bindings' and notify the pinctrl_handler.
> >  * pinctrl_handler will reinject the buffered packets. */
> > if (!hmap_is_empty(_mac_bindings)) {
> > buffer_put_mac_bindings();
> > notify_pinctrl_handler();
> > }
> > "
> >
> > c. Because of which put_mac_bindings is never emptied and
> wait_put_mac_bindings
> >ends up doing poll_immediate_wake.
> >
> > FIX:
> > a. Added call to flush_put_mac_bindings back in
> >run_put_mac_bindings.
> >
> > b. Additioanlly, logic mentioned in ROOT_CAUSE.b has been changed
> >in 1c24b2f490ba002bbfeb23006965188a7c5b9747, hence updated
> >the documentation in pinctrl.c accordingly.
> >
> > Signed-off-by: Ankur Sharma  ankur.sha...@nutanix.com>>
> > ---
> >  ovn/controller/pinctrl.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 8ae1f9b..b7bb4c9 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -91,11 +91,13 @@ VLOG_DEFINE_THIS_MODULE(pinctrl);
> >   *
> >   *   - arp/nd_ns  - These actions generate an ARP/IPv6 Neighbor
> solicit
> >   *  requests. The original packets are buffered and
> > - *  injected back when put_arp/put_nd actions are
> called.
> > + *  injected back when put_arp/put_nd resolves
> > + *  corresponding ARP/IPv6 Neighbor solicit
> requests.
> >   *  When pinctrl_run(), writes the mac bindings
> from the
> >   *  'put_mac_bindings' hmap to the MAC_Binding
> table in
> > - *  SB DB, it moves these mac bindings to another
> hmap -
> > - *  'buffered_mac_bindings'.
> > + *  SB DB, run_buffered_binding will add the
> buffered
> > + *  packets to buffered_mac_bindings and notify
> > + *  pinctrl_handler.
> >   *
> >   *  The pinctrl_handler thread calls the function -
> >   *  send_mac_binding_buffered_pkts(), which uses
> > @@ -2468,6 +2470,7 @@ run_put_mac_bindings(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> >  sbrec_mac_binding_by_lport_ip,
> >  pmb);
> >  }
> > +"flush_put_mac_bindings();


Thanks for the patch and finding the issue. This is a big mistake from me
when I added the pinctrl_thread.

Instead of calling  "flush_put_mac_bindings()" in the function
"run_put_mac_bindings()"
I would suggest to use - HMAP_FOR_EACH_POP in run_put_mac_bindings() and
then free
pmb after the call to run_put_mac_binding().

If you see flush_put_mac_bindings() also does the same thing. We can avoid
running the for loop twice.

Thanks
Numan

>  }
> >
> >  static void
> > --
> > 1.8.3.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev [
> mail.openvswitch.org]<
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwMFaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=P-XiYg1pCA7c5_H6MKaFGMBLWLPE7GAfuUnfELvcIGY=6QBH6hjY9c7Vc-Bh4_AzkwWJBBGuOH3Y2v7sXIIU-kc=
> >
>
> Thanks for the fix. Does it mean the the mac-binding will not work after
> some time since it is never flushed, and the check
>