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

[Ankur Sharma]

Hi Numan,

Excellent find. I missed the POP while iterating the hmap.
Submitted a V2, added relevant tags as well.

Please take a look.

Regards,
Ankur

From: Numan Siddique 
Sent: Thursday, May 16, 2019 11:31 AM
To: Ankur Sharma 
Cc: Han Zhou ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100% usage issue 
with put_mac_bindings



On Thu, May 16, 2019 at 12:26 AM Numan Siddique 
mailto:nusid...@redhat.com>> wrote:


On Thu, May 16, 2019 at 12:20 AM Ankur Sharma 
mailto:ankur.sha...@nutanix.com>> wrote:
Hi Numan,

Just confirming, anything else pending from my side?
or given that patch has been Acked, hence someone will apply it to master?

I don't think you have anything pending unless the committer has any comments.

Thanks for the fix :)

Numan


Thanks

Regards,
Ankur

From: Numan Siddique mailto:nusid...@redhat.com>>
Sent: Wednesday, May 15, 2019 11:43 AM
To: Ankur Sharma mailto:ankur.sha...@nutanix.com>>
Cc: Han Zhou mailto:zhou...@gmail.com>>; 
ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100% usage issue 
with put_mac_bindings

Gentle ping on this patch. This fix is a bit critical to fix the ovn-controller 
CPU usage.

Thanks
Numan

On Sat, May 11, 2019 at 2:34 PM Numan Siddique 
mailto:nusid...@redhat.com>> wrote:


On Sat, May 11, 2019 at 2:09 PM Numan Siddique 
mailto:nusid...@redhat.com>> wrote:


On Sat, May 11, 2019 at 5:38 AM Ankur Sharma 
mailto:ankur.sha...@nutanix.com>> 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 mailto:zhou...@gmail.com>>
Sent: Friday, May 10, 2019 4:47 PM
To: Ankur Sharma mailto:ankur.sha...@nutanix.com>>
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 
mailto:ankur.sha...@nutanix.com>>>
 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.

Hi Ankur,

I was very curious why this 100% CPU issue came now and why we didn't see 
earlier.
I looked into  the actual commit which added the pinctrl thread [1]
The function run_put_mac_bindings() didn't have any issues earlier as the code 
was like below

***
...
/* 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();
 }


and the function buffer_put_mac_bindings() popped the put_mac_bindings hmap.

The recent commit [2] has introduced this issue because it deleted the function 
buffer_put_mac_bindings().

I think its better to correct the commit message  and also add "Fixes" tag.

[1] - 
https://github.com/openvswitch/ovs/commit/3594ffab6b4b423aa635a313f6b304180d7dbaf7#diff-426c527fd2f3cf3c57def4b2747a48c3
 
[github.com]
[2] - 
https://github.com/openvswitch/ovs/commit/1c24b2f490ba002bbfeb23006965188a7c5b9747#diff-426c527fd2f3cf3c57def4b2747a48c3
 

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

[Ankur Sharma]

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. 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();
}
"

And buffer_put_mac_binding would pop the bindings from
put_mac_bindings, thereby emptying it.

c.  However, 1c24b2f490ba002bbfeb23006965188a7c5b9747
changed the buffer dequeueing logic and in the process
removed buffer_put_mac_binding, as a result put_mac_bindings
would never get empty.

FIX:
a. Added call to flush_put_mac_bindings back in
   run_put_mac_bindings.

b. Additionally, updated the documentation in pinctrl.c to
   reflect the new buffer dequeueing logic added by
   1c24b2f490ba002bbfeb23006965188a7c5b9747.

Signed-off-by: Ankur Sharma 
Reported-by: Ankur Sharma 
CC: Lorenzo Bianconi 
Fixes: 1c24b2f490ba ("OVN: fix pinctrl ip buffering for gw router port")
---
 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();
 }
 
 static void
-- 
1.8.3.1

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

Re: [ovs-dev] [PATCH] ovn-controller: Fix parsing of OVN tunnel IDs

[venugopal iyer via dev]

 Hi, Dumitru:

On Thursday, May 16, 2019, 2:13:41 AM CDT, Dumitru Ceara 
 wrote:  
 
 On Wed, May 15, 2019 at 10:30 PM venugopal iyer  wrote:
>
> Thanks for fixing this! a comment below:
>
> On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara  
> wrote:
>
>
> Add tunnel-id creation and parsing functions in encaps.h.
>
> Reported-at: https://bugzilla.redhat.com/1708131
> Reported-by: Haidong Li 
> Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> Signed-off-by: Dumitru Ceara 
> ---
> ovn/controller/bfd.c            | 31 ---
> ovn/controller/encaps.c        | 87 -
> ovn/controller/encaps.h        |  6 +++
> ovn/controller/ovn-controller.h |  7 
> ovn/controller/physical.c      | 51 +---
> ovn/controller/pinctrl.c        |  4 +-
> 6 files changed, 130 insertions(+), 56 deletions(-)
>
> diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c
> index d016e27..b6aef04 100644
> --- a/ovn/controller/bfd.c
> +++ b/ovn/controller/bfd.c
> @@ -15,6 +15,7 @@
>
> #include 
> #include "bfd.h"
> +#include "encaps.h"
> #include "lport.h"
> #include "ovn-controller.h"
>
> @@ -72,14 +73,16 @@ bfd_calculate_active_tunnels(const struct ovsrec_bridge 
> *br_int,
>                        const char *id = smap_get(_rec->external_ids,
>                                                  "ovn-chassis-id");
>                        if (id) {
> -                            char *chassis_name;
> -                            char *save_ptr = NULL;
> -                            char *tokstr = xstrdup(id);
> -                            chassis_name = strtok_r(tokstr, 
> OVN_MVTEP_CHASSISID_DELIM, _ptr);
> -                            if (chassis_name && 
> !sset_contains(active_tunnels, chassis_name)) {
> -                                sset_add(active_tunnels, chassis_name);
> +                            char *chassis_name = NULL;
> +
> +                            if (encaps_tunnel_id_parse(id, _name,
> +                                                      NULL)) {
> +                                if (!sset_contains(active_tunnels,
> +                                                  chassis_name)) {
> +                                    sset_add(active_tunnels, chassis_name);
> +                                }
> +                                free(chassis_name);
>                            }
> -                            free(tokstr);
>                        }
>                    }
>                }
> @@ -193,17 +196,17 @@ bfd_run(const struct ovsrec_interface_table 
> *interface_table,
>        const char *tunnel_id = smap_get(_int->ports[k]->external_ids,
>                                          "ovn-chassis-id");
>        if (tunnel_id) {
> -            char *chassis_name;
> -            char *save_ptr = NULL;
> -            char *tokstr = xstrdup(tunnel_id);
> +            char *chassis_name = NULL;
>            char *port_name = br_int->ports[k]->name;
>
>            sset_add(, port_name);
> -            chassis_name = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, 
> _ptr);
> -            if (chassis_name && sset_contains(_chassis, chassis_name)) {
> -                sset_add(_ifaces, port_name);
> +
> +            if (encaps_tunnel_id_parse(tunnel_id, _name, NULL)) {
> +                if (sset_contains(_chassis, chassis_name)) {
> +                    sset_add(_ifaces, port_name);
> +                }
> +                free(chassis_name);
>            }
> -            free(tokstr);
>        }
>    }
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index dcf7810..d467540 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -26,6 +26,13 @@
>
> VLOG_DEFINE_THIS_MODULE(encaps);
>
> +/*
> + * Given there could be multiple tunnels with different IPs to the same
> + * chassis we annotate the ovn-chassis-id with
> + * OVN_MVTEP_CHASSISID_DELIM.
> + */
> +#define    OVN_MVTEP_CHASSISID_DELIM '@'
> +
> void
> encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> {
> @@ -78,6 +85,83 @@ tunnel_create_name(struct tunnel_ctx *tc, const char 
> *chassis_id)
>    return NULL;
> }
>
> +/*
> + * Returns a tunnel-id of the form 'chassis_id'-delimiter-'encap_ip'.
> + */
> +char *
> +encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
> +{
> +    return xasprintf("%s%c%s", chassis_id, OVN_MVTEP_CHASSISID_DELIM,
> +                    encap_ip);
> +}
> +
> +/*
> + * Parses a 'tunnel_id' of the form .
> + * If the 'chassis_id' argument is not NULL the function will allocate memory
> + * and store the chassis-id part of the tunnel-id at '*chassis_id'.
> + * If the 'encap_ip' argument is not NULL the function will allocate memory
> + * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
> + */
> +bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> +                            char **encap_ip)
> +{
> +    const char *match = strchr(tunnel_id, 

[ovs-dev] Bien-être au travail

[Augustin]

Challenge du personnel,
dynamique de groupe : 
par le sport !



 


Avec la technologie connectée 

OuiMoveUp,

transformez l'activité en points ! 




 
 
 

 
Message commercial destiné aux entreprises et collectivités
Si vous ne souhaitez plus recevoir nos messages, merci de répondre à ce mail en 
mettant
NE PLUS dans l'objet du mail.
.

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

Re: [ovs-dev] [PATCH v2] ovn: Add a new logical switch port type - 'virtual'

[Han Zhou]

On Thu, May 16, 2019 at 11:40 AM Numan Siddique  wrote:

>
>
> On Thu, May 16, 2019 at 10:20 PM Han Zhou  wrote:
>
>>
>>
>> On Wed, May 15, 2019 at 11:59 PM Numan Siddique 
>> wrote:
>>
>>>
>>>
>>> On Thu, May 16, 2019 at 1:12 AM Han Zhou  wrote:
>>>


 On Wed, May 15, 2019 at 11:55 AM Numan Siddique 
 wrote:

>
>
> On Thu, May 16, 2019 at 12:10 AM Han Zhou  wrote:
>
>>
>>
>> On Wed, May 15, 2019 at 11:36 AM  wrote:
>> >
>> > From: Numan Siddique 
>> >
>> > This new type is added for the following reasons:
>> >
>> >   - When a load balancer is created in an OpenStack deployment with
>> Octavia
>> > service, it creates a logical port 'VIP' for the virtual ip.
>> >
>> >   - This logical port is not bound to any VIF.
>> >
>> >   - Octavia service creates a service VM (with another logical port
>> 'P' which
>> > belongs to the same logical switch)
>> >
>> >   - The virtual ip 'VIP' is configured on this service VM.
>> >
>> >   - This service VM provides the load balancing for the VIP with
>> the configured
>> > backend IPs.
>> >
>> >   - Octavia service can be configured to create few service VMs
>> with active-standby mode
>> > with the active VM configured with the VIP.  The VIP can move
>> between
>> > these service nodes.
>> >
>> > Presently there are few problems:
>> >
>> >   - When a floating ip (externally reachable IP) is associated to
>> the VIP and if
>> > the compute nodes have external connectivity then the external
>> traffic cannot
>> > reach the VIP using the floating ip as the VIP logical port
>> would be down.
>> > dnat_and_snat entry in NAT table for this vip will have
>> 'external_mac' and
>> > 'logical_port' configured.
>> >
>> >   - The only way to make it work is to clear the 'external_mac'
>> entry so that
>> > the gateway chassis does the DNAT for the VIP.
>> >
>> > To solve these problems, this patch proposes a new logical port
>> type - virtual.
>> > CMS when creating the logical port for the VIP, should
>> >
>> >  - set the type as 'virtual'
>> >
>> >  - configure the VIP in the newly added column
>> Logical_Switch_Port.virtual_ip
>> >
>> >  - And set the virtual parents in the new added column
>> Logical_Switch_Port.virtual_parents.
>> >These virtual parents are the one which can be configured wit
>> the VIP.
>> >
>> > If suppose the virtual_ip is configured to 10.0.0.10 on a virtual
>> logical port 'sw0-vip'
>> > and the virtual_parents are set to - [sw0-p1, sw0-p2] then below
>> logical flows are added in the
>> > lsp_in_arp_rsp logical switch pipeline
>> >
>> >  - table=11(ls_in_arp_rsp), priority=100,
>> >match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa ==
>> 10.0.0.10 && arp.tpa == 10.0.0.10) ||
>> >  (arp.op == 2 && arp.spa ==
>> 10.0.0.10))),
>> >action=(bind_vport("sw0-vip", inport); next;)
>> > - table=11(ls_in_arp_rsp), priority=100,
>> >match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa ==
>> 10.0.0.10 && arp.tpa == 10.0.0.10) ||
>> >  (arp.op == 2 && arp.spa ==
>> 10.0.0.10))),
>> >action=(bind_vport("sw0-vip", inport); next;)
>> >
>> > The action bind_vport will claim the logical port - sw0-vip on the
>> chassis where this action
>> > is executed. Since the port - sw0-vip is claimed by a chassis, the
>> dnat_and_snat rule for
>> > the VIP will be handled by the compute node.
>> >
>> > Signed-off-by: Numan Siddique 
>>
>> Hi Numan, this looks interesting. I haven't reviewed code yet, but
>> just some questions to better understand the feature.
>>
>> Firstly, can Octavia be implemented by using the distributed LB
>> feature of OVN, instead of using dedicated node? What's the major gap for
>> using the OVN LB?
>>
>>
> Yes. Its possible to use the native OVN LB feature. There's already a
> provider driver in Octavia for
> OVN (
> https://github.com/openstack/networking-ovn/blob/master/networking_ovn/octavia/ovn_driver.py
> ).
> When creating the LB providing the option --provider-driver=ovn will
> create OVN LB.
>
> However OVN LB is limited to L4 and there are no health checks.
> Octavia amphora driver supports lots of
> features like L7, health check etc. I think we should definitely look
> into adding the health monitor feature for OVN LB.
> But I think supporting L7 LBs is out of question for OVN LB.
> For complex load balancer needs, I think its better to rely on
> external load balancers like the Octavia amphora driver.
> Octavia amphora driver creates a service VM 

Re: [ovs-dev] [PATCH v2] ovn: Add a new logical switch port type - 'virtual'

[Numan Siddique]

On Thu, May 16, 2019 at 10:20 PM Han Zhou  wrote:

>
>
> On Wed, May 15, 2019 at 11:59 PM Numan Siddique 
> wrote:
>
>>
>>
>> On Thu, May 16, 2019 at 1:12 AM Han Zhou  wrote:
>>
>>>
>>>
>>> On Wed, May 15, 2019 at 11:55 AM Numan Siddique 
>>> wrote:
>>>


 On Thu, May 16, 2019 at 12:10 AM Han Zhou  wrote:

>
>
> On Wed, May 15, 2019 at 11:36 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > This new type is added for the following reasons:
> >
> >   - When a load balancer is created in an OpenStack deployment with
> Octavia
> > service, it creates a logical port 'VIP' for the virtual ip.
> >
> >   - This logical port is not bound to any VIF.
> >
> >   - Octavia service creates a service VM (with another logical port
> 'P' which
> > belongs to the same logical switch)
> >
> >   - The virtual ip 'VIP' is configured on this service VM.
> >
> >   - This service VM provides the load balancing for the VIP with the
> configured
> > backend IPs.
> >
> >   - Octavia service can be configured to create few service VMs with
> active-standby mode
> > with the active VM configured with the VIP.  The VIP can move
> between
> > these service nodes.
> >
> > Presently there are few problems:
> >
> >   - When a floating ip (externally reachable IP) is associated to
> the VIP and if
> > the compute nodes have external connectivity then the external
> traffic cannot
> > reach the VIP using the floating ip as the VIP logical port
> would be down.
> > dnat_and_snat entry in NAT table for this vip will have
> 'external_mac' and
> > 'logical_port' configured.
> >
> >   - The only way to make it work is to clear the 'external_mac'
> entry so that
> > the gateway chassis does the DNAT for the VIP.
> >
> > To solve these problems, this patch proposes a new logical port type
> - virtual.
> > CMS when creating the logical port for the VIP, should
> >
> >  - set the type as 'virtual'
> >
> >  - configure the VIP in the newly added column
> Logical_Switch_Port.virtual_ip
> >
> >  - And set the virtual parents in the new added column
> Logical_Switch_Port.virtual_parents.
> >These virtual parents are the one which can be configured wit the
> VIP.
> >
> > If suppose the virtual_ip is configured to 10.0.0.10 on a virtual
> logical port 'sw0-vip'
> > and the virtual_parents are set to - [sw0-p1, sw0-p2] then below
> logical flows are added in the
> > lsp_in_arp_rsp logical switch pipeline
> >
> >  - table=11(ls_in_arp_rsp), priority=100,
> >match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa ==
> 10.0.0.10 && arp.tpa == 10.0.0.10) ||
> >  (arp.op == 2 && arp.spa ==
> 10.0.0.10))),
> >action=(bind_vport("sw0-vip", inport); next;)
> > - table=11(ls_in_arp_rsp), priority=100,
> >match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa ==
> 10.0.0.10 && arp.tpa == 10.0.0.10) ||
> >  (arp.op == 2 && arp.spa ==
> 10.0.0.10))),
> >action=(bind_vport("sw0-vip", inport); next;)
> >
> > The action bind_vport will claim the logical port - sw0-vip on the
> chassis where this action
> > is executed. Since the port - sw0-vip is claimed by a chassis, the
> dnat_and_snat rule for
> > the VIP will be handled by the compute node.
> >
> > Signed-off-by: Numan Siddique 
>
> Hi Numan, this looks interesting. I haven't reviewed code yet, but
> just some questions to better understand the feature.
>
> Firstly, can Octavia be implemented by using the distributed LB
> feature of OVN, instead of using dedicated node? What's the major gap for
> using the OVN LB?
>
>
 Yes. Its possible to use the native OVN LB feature. There's already a
 provider driver in Octavia for
 OVN (
 https://github.com/openstack/networking-ovn/blob/master/networking_ovn/octavia/ovn_driver.py
 ).
 When creating the LB providing the option --provider-driver=ovn will
 create OVN LB.

 However OVN LB is limited to L4 and there are no health checks. Octavia
 amphora driver supports lots of
 features like L7, health check etc. I think we should definitely look
 into adding the health monitor feature for OVN LB.
 But I think supporting L7 LBs is out of question for OVN LB.
 For complex load balancer needs, I think its better to rely on external
 load balancers like the Octavia amphora driver.
 Octavia amphora driver creates a service VM and runs an haproxy
 instance inside it to provider the load balancing.

 Thanks for the detailed explain. Yes, this makes sense!
>>>



> Secondly, how is 

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

[Numan Siddique]

On Thu, May 16, 2019 at 12:26 AM Numan Siddique  wrote:

>
>
> On Thu, May 16, 2019 at 12:20 AM Ankur Sharma 
> wrote:
>
>> Hi Numan,
>>
>> Just confirming, anything else pending from my side?
>> or given that patch has been Acked, hence someone will apply it to master?
>>
>
> I don't think you have anything pending unless the committer has any
> comments.
>
> Thanks for the fix :)
>
> Numan
>
>
>> Thanks
>>
>> Regards,
>> Ankur
>>
>>
>>
>> *From:* Numan Siddique 
>> *Sent:* Wednesday, May 15, 2019 11:43 AM
>> *To:* Ankur Sharma 
>> *Cc:* Han Zhou ; ovs-dev@openvswitch.org
>> *Subject:* Re: [ovs-dev] [PATCH v1] OVN: Fix the ovn-controller 100%
>> usage issue with put_mac_bindings
>>
>>
>>
>> Gentle ping on this patch. This fix is a bit critical to fix the
>> ovn-controller CPU usage.
>>
>>
>>
>> Thanks
>>
>> Numan
>>
>>
>>
>> On Sat, May 11, 2019 at 2:34 PM Numan Siddique 
>> wrote:
>>
>>
>>
>>
>>
>> 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.
>>
>>
Hi Ankur,

I was very curious why this 100% CPU issue came now and why we didn't see
earlier.
I looked into  the actual commit which added the pinctrl thread [1]
The function run_put_mac_bindings() didn't have any issues earlier as the
code was like below

***
...
/* 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();
 }


and the function buffer_put_mac_bindings() popped the put_mac_bindings hmap.

The recent commit [2] has introduced this issue because it deleted the
function buffer_put_mac_bindings().

I think its better to correct the commit message  and also add "Fixes" tag.

[1] -
https://github.com/openvswitch/ovs/commit/3594ffab6b4b423aa635a313f6b304180d7dbaf7#diff-426c527fd2f3cf3c57def4b2747a48c3
[2] -
https://github.com/openvswitch/ovs/commit/1c24b2f490ba002bbfeb23006965188a7c5b9747#diff-426c527fd2f3cf3c57def4b2747a48c3


Thanks
Numan

>
>> > 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
>> 

Re: [ovs-dev] [PATCH 2/2] doc: Add CirrusCI status badge to README.

[Aaron Conole]

Ilya Maximets  writes:

> Badge for CirrusCI just like for other CI systems.
>
> Signed-off-by: Ilya Maximets 
> ---
>  README.rst | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/README.rst b/README.rst
> index 54d06d04b..e06ddf267 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -10,6 +10,8 @@ Open vSwitch
>  :target: https://travis-ci.org/openvswitch/ovs
>  .. image:: 
> https://ci.appveyor.com/api/projects/status/github/openvswitch/ovs?branch=master=true=true
>  :target: https://ci.appveyor.com/project/blp/ovs/history
> +.. image:: https://api.cirrus-ci.com/github/openvswitch/ovs.svg
> +:target: https://cirrus-ci.com/github/openvswitch/ovs
>  
>  What is Open vSwitch?
>  -

Makes sense to me:

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

Re: [ovs-dev] [PATCH v2] ovn: Add a new logical switch port type - 'virtual'

[Han Zhou]

On Wed, May 15, 2019 at 11:59 PM Numan Siddique  wrote:

>
>
> On Thu, May 16, 2019 at 1:12 AM Han Zhou  wrote:
>
>>
>>
>> On Wed, May 15, 2019 at 11:55 AM Numan Siddique 
>> wrote:
>>
>>>
>>>
>>> On Thu, May 16, 2019 at 12:10 AM Han Zhou  wrote:
>>>


 On Wed, May 15, 2019 at 11:36 AM  wrote:
 >
 > From: Numan Siddique 
 >
 > This new type is added for the following reasons:
 >
 >   - When a load balancer is created in an OpenStack deployment with
 Octavia
 > service, it creates a logical port 'VIP' for the virtual ip.
 >
 >   - This logical port is not bound to any VIF.
 >
 >   - Octavia service creates a service VM (with another logical port
 'P' which
 > belongs to the same logical switch)
 >
 >   - The virtual ip 'VIP' is configured on this service VM.
 >
 >   - This service VM provides the load balancing for the VIP with the
 configured
 > backend IPs.
 >
 >   - Octavia service can be configured to create few service VMs with
 active-standby mode
 > with the active VM configured with the VIP.  The VIP can move
 between
 > these service nodes.
 >
 > Presently there are few problems:
 >
 >   - When a floating ip (externally reachable IP) is associated to the
 VIP and if
 > the compute nodes have external connectivity then the external
 traffic cannot
 > reach the VIP using the floating ip as the VIP logical port would
 be down.
 > dnat_and_snat entry in NAT table for this vip will have
 'external_mac' and
 > 'logical_port' configured.
 >
 >   - The only way to make it work is to clear the 'external_mac' entry
 so that
 > the gateway chassis does the DNAT for the VIP.
 >
 > To solve these problems, this patch proposes a new logical port type
 - virtual.
 > CMS when creating the logical port for the VIP, should
 >
 >  - set the type as 'virtual'
 >
 >  - configure the VIP in the newly added column
 Logical_Switch_Port.virtual_ip
 >
 >  - And set the virtual parents in the new added column
 Logical_Switch_Port.virtual_parents.
 >These virtual parents are the one which can be configured wit the
 VIP.
 >
 > If suppose the virtual_ip is configured to 10.0.0.10 on a virtual
 logical port 'sw0-vip'
 > and the virtual_parents are set to - [sw0-p1, sw0-p2] then below
 logical flows are added in the
 > lsp_in_arp_rsp logical switch pipeline
 >
 >  - table=11(ls_in_arp_rsp), priority=100,
 >match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10
 && arp.tpa == 10.0.0.10) ||
 >  (arp.op == 2 && arp.spa ==
 10.0.0.10))),
 >action=(bind_vport("sw0-vip", inport); next;)
 > - table=11(ls_in_arp_rsp), priority=100,
 >match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa == 10.0.0.10
 && arp.tpa == 10.0.0.10) ||
 >  (arp.op == 2 && arp.spa ==
 10.0.0.10))),
 >action=(bind_vport("sw0-vip", inport); next;)
 >
 > The action bind_vport will claim the logical port - sw0-vip on the
 chassis where this action
 > is executed. Since the port - sw0-vip is claimed by a chassis, the
 dnat_and_snat rule for
 > the VIP will be handled by the compute node.
 >
 > Signed-off-by: Numan Siddique 

 Hi Numan, this looks interesting. I haven't reviewed code yet, but just
 some questions to better understand the feature.

 Firstly, can Octavia be implemented by using the distributed LB feature
 of OVN, instead of using dedicated node? What's the major gap for using the
 OVN LB?


>>> Yes. Its possible to use the native OVN LB feature. There's already a
>>> provider driver in Octavia for
>>> OVN (
>>> https://github.com/openstack/networking-ovn/blob/master/networking_ovn/octavia/ovn_driver.py
>>> ).
>>> When creating the LB providing the option --provider-driver=ovn will
>>> create OVN LB.
>>>
>>> However OVN LB is limited to L4 and there are no health checks. Octavia
>>> amphora driver supports lots of
>>> features like L7, health check etc. I think we should definitely look
>>> into adding the health monitor feature for OVN LB.
>>> But I think supporting L7 LBs is out of question for OVN LB.
>>> For complex load balancer needs, I think its better to rely on external
>>> load balancers like the Octavia amphora driver.
>>> Octavia amphora driver creates a service VM and runs an haproxy instance
>>> inside it to provider the load balancing.
>>>
>>> Thanks for the detailed explain. Yes, this makes sense!
>>
>>>
>>>
>>>
 Secondly, how is associating the floating-ip with the VIP configured
 currently?

>>>
>>> networking-ovn creates a dnat_and_snat entry when floating ip is
>>> associated for the VIP port. Right now it doesn't 

[ovs-dev] [RFC 2/3] OVN: introduce send_event() action

[Lorenzo Bianconi]

Add send_event() ovn action in order to allow ovs-vswitchd to report
CMS related events.
This commit introduces a new event, empty_lb_backends. This event is
raised if a received packet is destined for a load balancer VIP that has
no configured backend destinations. For this event, the event info
includes the load balancer VIP, the load balancer UUID, and the
transport protocol.
The use case for this particular event is for the CMS to supply backend
resources to handle this traffic. For example, in Openshift, this event
can be used to spin up new containers to handle the incoming traffic.

Signed-off-by: Mark Michelson 
Co-authored-by: Mark Michelson 
Signed-off-by: Lorenzo Bianconi 
---
 include/ovn/actions.h |  17 +++-
 ovn/controller/lflow.c|   8 ++
 ovn/controller/pinctrl.c  | 109 
 ovn/lib/actions.c | 169 ++
 ovn/lib/ovn-l7.h  |  46 +++
 ovn/utilities/ovn-trace.c |   3 +
 tests/ovn.at  |  10 +++
 tests/test-ovn.c  |  11 ++-
 8 files changed, 370 insertions(+), 3 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index e07ad9aa3..0d5920023 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -83,7 +83,8 @@ struct ovn_extend_table;
 OVNACT(ND_NS, ovnact_nest)\
 OVNACT(SET_METER, ovnact_set_meter)   \
 OVNACT(OVNFIELD_LOAD, ovnact_load)\
-OVNACT(CHECK_PKT_LARGER,  ovnact_check_pkt_larger)
+OVNACT(CHECK_PKT_LARGER,  ovnact_check_pkt_larger) \
+OVNACT(SEND_EVENT,ovnact_controller_event)
 
 /* enum ovnact_type, with a member OVNACT_ for each action. */
 enum OVS_PACKED_ENUM ovnact_type {
@@ -318,6 +319,14 @@ struct ovnact_check_pkt_larger {
 struct expr_field dst;  /* 1-bit destination field. */
 };
 
+/* OVNACT_EVENT. */
+struct ovnact_controller_event {
+struct ovnact ovnact;
+int event_type;   /* controller event type */
+struct ovnact_gen_option *options;
+size_t n_options;
+};
+
 /* Internal use by the helpers below. */
 void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
 void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
@@ -486,6 +495,9 @@ enum action_opcode {
  * The actions, in OpenFlow 1.3 format, follow the action_header.
  */
 ACTION_OPCODE_ICMP4_ERROR,
+
+/* "send_event (event_type)" */
+ACTION_OPCODE_EVENT,
 };
 
 /* Header. */
@@ -515,6 +527,9 @@ struct ovnact_parse_params {
 /* hmap of 'struct gen_opts_map' to support 'put_nd_ra_opts' action */
 const struct hmap *nd_ra_opts;
 
+/* Array of hmap of 'struct gen_opts_map' to support 'send_event' action */
+const struct controller_event_options *controller_event_opts;
+
 /* Each OVN flow exists in a logical table within a logical pipeline.
  * These parameters express this context for a set of OVN actions being
  * parsed:
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 661407bcc..8d7f51204 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -70,6 +70,7 @@ static void consider_logical_flow(
 struct hmap *dhcp_opts,
 struct hmap *dhcpv6_opts,
 struct hmap *nd_ra_opts,
+struct controller_event_options *controller_event_opts,
 const struct shash *addr_sets,
 const struct shash *port_groups,
 const struct sset *active_tunnels,
@@ -173,11 +174,15 @@ add_logical_flows(
 struct hmap nd_ra_opts = HMAP_INITIALIZER(_ra_opts);
 nd_ra_opts_init(_ra_opts);
 
+struct controller_event_options controller_event_opts;
+controller_event_opts_init(_event_opts);
+
 SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, logical_flow_table) {
 consider_logical_flow(sbrec_multicast_group_by_name_datapath,
   sbrec_port_binding_by_name,
   lflow, local_datapaths,
   chassis, _opts, _opts, _ra_opts,
+  _event_opts,
   addr_sets, port_groups, active_tunnels,
   local_lport_ids, _id_ofs,
   flow_table, group_table, meter_table);
@@ -186,6 +191,7 @@ add_logical_flows(
 dhcp_opts_destroy(_opts);
 dhcp_opts_destroy(_opts);
 nd_ra_opts_destroy(_ra_opts);
+controller_event_opts_destroy(_event_opts);
 }
 
 static void
@@ -198,6 +204,7 @@ consider_logical_flow(
 struct hmap *dhcp_opts,
 struct hmap *dhcpv6_opts,
 struct hmap *nd_ra_opts,
+struct controller_event_options *controller_event_opts,
 const struct shash *addr_sets,
 const struct shash *port_groups,
 const struct sset *active_tunnels,
@@ -237,6 +244,7 @@ consider_logical_flow(
 .dhcp_opts = dhcp_opts,
 .dhcpv6_opts = dhcpv6_opts,
 .nd_ra_opts = nd_ra_opts,
+.controller_event_opts = controller_event_opts,
 
 .pipeline = ingress ? 

[ovs-dev] [RFC 3/3] OVN: use send_event action to report 'empty_lb_rule' events

[Lorenzo Bianconi]

Add northd logical flows in order to reports that the controller
received an IP packet for LB rule witn no backends.
This configuration is used by OpenShift to spin up a idle POD

Signed-off-by: Mark Michelson 
Co-authored-by: Mark Michelson 
Signed-off-by: Lorenzo Bianconi 
---
 ovn/northd/ovn-northd.c | 32 
 ovn/ovn-nb.xml  |  5 
 tests/ovn.at| 66 +
 3 files changed, 103 insertions(+)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index de0c06d4b..16b38db0a 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -70,6 +70,8 @@ static const char *unixctl_path;
 static struct hmap macam = HMAP_INITIALIZER();
 static struct eth_addr mac_prefix;
 
+static bool controller_event_en;
+
 #define MAX_OVN_TAGS 4096
 
 /* Pipeline stages. */
@@ -3571,6 +3573,33 @@ build_pre_lb(struct ovn_datapath *od, struct hmap 
*lflows)
 sset_add(_ips, ip_address);
 }
 
+if (controller_event_en && !strlen(node->value)) {
+struct ds match = DS_EMPTY_INITIALIZER;
+char *action;
+
+if (addr_family == AF_INET) {
+ds_put_format(, "ip && ip4.dst == %s && %s",
+  ip_address, lb->protocol);
+} else {
+ds_put_format(, "ip && ip6.dst == %s && %s",
+  ip_address, lb->protocol);
+}
+if (port) {
+ds_put_format(, " && %s.dst == %u", lb->protocol,
+  port);
+}
+action = xasprintf("send_event(event = %d, vip = \"%s\", "
+   "protocol = \"%s\", "
+   "load_balancer = \"" UUID_FMT "\");",
+   OVN_EVENT_EMPTY_LB_BACKENDS, node->key,
+   lb->protocol, UUID_ARGS(>header_.uuid));
+ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 120,
+  ds_cstr(), action);
+ds_destroy();
+free(action);
+continue;
+}
+
 free(ip_address);
 
 /* Ignore L4 port information in the key because fragmented packets
@@ -8044,6 +8073,9 @@ ovnnb_db_run(struct northd_context *ctx,
 smap_destroy();
 }
 
+controller_event_en = smap_get_bool(>options,
+"controller_event", false);
+
 cleanup_macam();
 }
 
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index cbaa9495f..c9d4acbe4 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -107,6 +107,11 @@
 Configure a given OUI to be used as prefix when L2 address is
 dynamically assigned, e.g. 00:11:22
   
+
+  
+Value used to enable/disable ovn-controller event reporting to the CMS.
+Please see the  table in SBDB.
+  
 
 
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 1c54dd920..2df2833e9 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14031,3 +14031,69 @@ ovn-hv4-0
 
 OVN_CLEANUP([hv1], [hv2], [hv3])
 AT_CLEANUP
+
+AT_SETUP([ovn -- controller event])
+ovn_start
+
+# Create hypervisors hv[12].
+# Add vif1[12] to hv1, vif2[12] to hv2
+# Add all of the vifs to a single logical switch sw0.
+
+net_add n1
+ovn-nbctl ls-add sw0
+for i in 1 2; do
+sim_add hv$i
+as hv$i
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.$i
+
+for j in 1 2; do
+ovn-nbctl lsp-add sw0 sw0-p$i$j -- \
+lsp-set-addresses sw0-p$i$j "00:00:00:00:00:$i$j 
192.168.1.$i$j"
+
+ovs-vsctl -- add-port br-int vif$i$j -- \
+set interface vif$i$j \
+external-ids:iface-id=sw0-p$i$j \
+options:tx_pcap=hv$i/vif$i$j-tx.pcap \
+options:rxq_pcap=hv$i/vif$i$j-rx.pcap \
+ofport-request=$i$j
+done
+done
+
+ovn-nbctl --wait=hv set NB_Global . options:controller_event=true
+ovn-nbctl lb-add lb0 192.168.1.100:80 ""
+ovn-nbctl ls-lb-add sw0 lb0
+uuid_lb=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
+
+OVN_POPULATE_ARP
+ovn-nbctl --timeout=3 --wait=hv sync
+ovn-sbctl lflow-list
+as hv1 ovs-ofctl dump-flows br-int
+
+packet="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && 
eth.dst==00:00:00:00:00:21 &&
+   ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
+   tcp && tcp.src==1 && tcp.dst==80"
+as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+ovn-sbctl list controller_event
+uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
+AT_CHECK([ovn-sbctl get controller_event $uuid event_type], [0], [dnl
+empty_lb_backends
+])
+AT_CHECK([ovn-sbctl get controller_event $uuid event_info:vip], [0], [dnl
+"192.168.1.100:80"
+])
+AT_CHECK([ovn-sbctl get controller_event $uuid event_info:protocol], 

[ovs-dev] [RFC 1/3] OVN: introduce Controller_Event table

[Lorenzo Bianconi]

Add Controller_Event table to OVN SBDB in order to
report CMS related event.
Introduce event_table hashmap array and controller_event related
structures to ovn-controller in order to track pending events
forwarded by ovs-vswitchd. Moreover integrate event_table hashmap
array with event_table ovn-sbdb table

Signed-off-by: Mark Michelson 
Co-authored-by: Mark Michelson 
Signed-off-by: Lorenzo Bianconi 
---
 include/ovn/logical-fields.h|  17 +
 ovn/controller/ovn-controller.c |   2 +
 ovn/controller/pinctrl.c| 130 
 ovn/controller/pinctrl.h|   2 +
 ovn/ovn-sb.ovsschema|  16 +++-
 ovn/ovn-sb.xml  |  33 
 6 files changed, 197 insertions(+), 3 deletions(-)

diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
index 164b338b5..431ad03d0 100644
--- a/include/ovn/logical-fields.h
+++ b/include/ovn/logical-fields.h
@@ -20,6 +20,23 @@
 
 struct shash;
 
+enum ovn_controller_event {
+OVN_EVENT_EMPTY_LB_BACKENDS = 0,
+OVN_EVENT_MAX,
+};
+
+static inline char *
+event_to_string(enum ovn_controller_event event)
+{
+switch (event) {
+case OVN_EVENT_EMPTY_LB_BACKENDS:
+return "empty_lb_backends";
+case OVN_EVENT_MAX:
+default:
+return "";
+}
+}
+
 /* Logical fields.
  *
  * These values are documented in ovn-architecture(7), please update the
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 695dc..d6494590b 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -765,6 +765,8 @@ main(int argc, char *argv[])
 sbrec_port_binding_by_name,
 sbrec_mac_binding_by_lport_ip,
 sbrec_dns_table_get(ovnsb_idl_loop.idl),
+sbrec_controller_event_table_get(
+ovnsb_idl_loop.idl),
 br_int, chassis,
 _datapaths, _tunnels);
 update_ct_zones(_lports, _datapaths, _zones,
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 8ae1f9bd6..ca191d961 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -223,6 +223,132 @@ static bool may_inject_pkts(void);
 
 COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
 COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map);
+COVERAGE_DEFINE(pinctrl_drop_controller_event);
+
+struct empty_lb_backends_event {
+struct hmap_node hmap_node;
+char *vip;
+char *protocol;
+char *load_balancer;
+};
+
+static struct hmap event_table[OVN_EVENT_MAX];
+
+static void init_event_table(void)
+{
+for (size_t i = 0; i < OVN_EVENT_MAX; i++) {
+hmap_init(_table[i]);
+}
+}
+
+static void
+empty_lb_backends_event_flush(void)
+{
+struct empty_lb_backends_event *ce;
+HMAP_FOR_EACH_POP (ce, hmap_node,
+   _table[OVN_EVENT_EMPTY_LB_BACKENDS]) {
+free(ce->vip);
+free(ce->protocol);
+free(ce->load_balancer);
+free(ce);
+}
+}
+
+static void event_table_flush(void)
+{
+empty_lb_backends_event_flush();
+}
+
+static void event_table_destroy(void)
+{
+event_table_flush();
+for (size_t i = 0; i < OVN_EVENT_MAX; i++) {
+hmap_destroy(_table[i]);
+}
+}
+
+static struct empty_lb_backends_event *
+pinctrl_find_empty_lb_backends_event(char *vip, char *protocol,
+ char *load_balancer, uint32_t hash)
+{
+struct empty_lb_backends_event *ce;
+HMAP_FOR_EACH_WITH_HASH (ce, hmap_node, hash,
+ _table[OVN_EVENT_EMPTY_LB_BACKENDS]) {
+if (!strcmp(ce->vip, vip) &&
+!strcmp(ce->protocol, protocol) &&
+!strcmp(ce->load_balancer, load_balancer)) {
+return ce;
+}
+}
+return NULL;
+}
+
+static const struct sbrec_controller_event *
+empty_lb_backends_lookup(struct empty_lb_backends_event *event,
+ const struct sbrec_controller_event_table *ce_table)
+{
+const struct sbrec_controller_event *sbrec_event;
+const char *event_type = event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS);
+SBREC_CONTROLLER_EVENT_TABLE_FOR_EACH (sbrec_event, ce_table) {
+if (strcmp(sbrec_event->event_type, event_type)) {
+continue;
+}
+
+const char *vip = smap_get(_event->event_info, "vip");
+const char *protocol = smap_get(_event->event_info, "protocol");
+const char *load_balancer = smap_get(_event->event_info,
+ "load_balancer");
+
+if (!strcmp(event->vip, vip) &&
+!strcmp(event->protocol, protocol) &&
+!strcmp(event->load_balancer, load_balancer)) {
+return sbrec_event;
+}
+}
+
+return NULL;
+}
+
+static void
+controller_event_run(struct ovsdb_idl_txn 

[ovs-dev] [RFC 0/3] OVN: add Controller Events

[Lorenzo Bianconi]

There are situations where arrival of certain types of traffic into OVS
does not warrant a "typical" action, such as output to a specific port
or dropping. Rather, the decision about what to do needs to be left to a
CMS.

The series here introduces a new table, Controller_Event, for this
purpose. Traffic into OVS can raise a controller() event that results in
a Controller_Event being written to the southbound database. The
intention is for a CMS to see the events and take some sort of action.
The "handled" column of the event is initially set to "false" by OVN.
When the CMS has seen the event and taken appropriate action, then the
CMS sets this field to "true'. When ovn-controller sees that the event
has been handled, it deletes the event from the database.

Controller events are only added to the southbound database if they have
been enabled in the nb_global table via the options:controller_event
setting.

This series introduces a new event, empty_lb_backends. This event is
raised if a received packet is destined for a load balancer VIP that has
no configured backend destinations. For this event, the event info
includes the load balancer VIP, the load balancer UUID, and the
transport protocol.

The use case for this particular event is for the CMS to supply backend
resources to handle this traffic. For example, in Openshift, this event
can be used to spin up new containers to handle the incoming traffic.

Lorenzo Bianconi (3):
  OVN: introduce Controller_Event table
  OVN: introduce send_event() action
  OVN: use send_event action to report 'empty_lb_rule' events

 include/ovn/actions.h   |  17 ++-
 include/ovn/logical-fields.h|  17 +++
 ovn/controller/lflow.c  |   8 ++
 ovn/controller/ovn-controller.c |   2 +
 ovn/controller/pinctrl.c| 239 
 ovn/controller/pinctrl.h|   2 +
 ovn/lib/actions.c   | 169 ++
 ovn/lib/ovn-l7.h|  46 ++
 ovn/northd/ovn-northd.c |  32 +
 ovn/ovn-nb.xml  |   5 +
 ovn/ovn-sb.ovsschema|  16 ++-
 ovn/ovn-sb.xml  |  33 +
 ovn/utilities/ovn-trace.c   |   3 +
 tests/ovn.at|  76 ++
 tests/test-ovn.c|  11 +-
 15 files changed, 670 insertions(+), 6 deletions(-)

-- 
2.20.1

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

Re: [ovs-dev] [PATCH 1/2] doc: Fix cropped what-is-ovs page.

[Stephen Finucane]

On Thu, 2019-05-16 at 16:40 +0300, Ilya Maximets wrote:
> Despite of comments in both files no-one ever adjusted start/end-line
> in 'what-is-ovs' document. As a result, current document contains
> truncated "tools" section.
> 
> Let's replace start/end-line with start-after/end-before which requires
> less attention. Additionally, 'make docs-check' will fail if specified
> lines will not be found, i.e it'll be harder to mess up the docs again.
> 
> "Fixes" tag points to commit that broke the lines first.
> 
> Fixes: 602e24ee189b ("doc: Remove experimental warning for DPDK.")
> Signed-off-by: Ilya Maximets 

Good call.

Acked-by: Stephen Finucane 

> ---
>  Documentation/intro/what-is-ovs.rst | 8 
>  README.rst  | 5 +++--
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/intro/what-is-ovs.rst 
> b/Documentation/intro/what-is-ovs.rst
> index bf7071f7a..002e1b063 100644
> --- a/Documentation/intro/what-is-ovs.rst
> +++ b/Documentation/intro/what-is-ovs.rst
> @@ -33,9 +33,9 @@ What Is Open vSwitch?
>  Overview
>  
>  
> -.. NOTE(stephenfin): The below line numbers may need to be updated if the
> -   README is modified
> +.. NOTE(stephenfin): The below start-after/end-before may need to be updated
> +   if the README is modified.
>  
>  .. include:: ../../README.rst
> -   :start-line: 13
> -   :end-line: 71
> +   :start-after: What is Open vSwitch?
> +   :end-before: What other documentation is available?
> diff --git a/README.rst b/README.rst
> index 4c9d9eddd..54d06d04b 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -1,5 +1,6 @@
> -.. NOTE(stephenfin): If making changes to this file, ensure that the line
> -   numbers found in 'Documentation/intro/what-is-ovs' are kept up-to-date.
> +.. NOTE(stephenfin): If making changes to this file, ensure that the
> +   start-after/end-before lines found in 'Documentation/intro/what-is-ovs'
> +   are kept up-to-date.
>  
>  
>  Open vSwitch

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

[ovs-dev] [PATCH 2/2] doc: Add CirrusCI status badge to README.

[Ilya Maximets]

Badge for CirrusCI just like for other CI systems.

Signed-off-by: Ilya Maximets 
---
 README.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/README.rst b/README.rst
index 54d06d04b..e06ddf267 100644
--- a/README.rst
+++ b/README.rst
@@ -10,6 +10,8 @@ Open vSwitch
 :target: https://travis-ci.org/openvswitch/ovs
 .. image:: 
https://ci.appveyor.com/api/projects/status/github/openvswitch/ovs?branch=master=true=true
 :target: https://ci.appveyor.com/project/blp/ovs/history
+.. image:: https://api.cirrus-ci.com/github/openvswitch/ovs.svg
+:target: https://cirrus-ci.com/github/openvswitch/ovs
 
 What is Open vSwitch?
 -
-- 
2.17.1

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

[ovs-dev] [PATCH 1/2] doc: Fix cropped what-is-ovs page.

[Ilya Maximets]

Despite of comments in both files no-one ever adjusted start/end-line
in 'what-is-ovs' document. As a result, current document contains
truncated "tools" section.

Let's replace start/end-line with start-after/end-before which requires
less attention. Additionally, 'make docs-check' will fail if specified
lines will not be found, i.e it'll be harder to mess up the docs again.

"Fixes" tag points to commit that broke the lines first.

Fixes: 602e24ee189b ("doc: Remove experimental warning for DPDK.")
Signed-off-by: Ilya Maximets 
---
 Documentation/intro/what-is-ovs.rst | 8 
 README.rst  | 5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/intro/what-is-ovs.rst 
b/Documentation/intro/what-is-ovs.rst
index bf7071f7a..002e1b063 100644
--- a/Documentation/intro/what-is-ovs.rst
+++ b/Documentation/intro/what-is-ovs.rst
@@ -33,9 +33,9 @@ What Is Open vSwitch?
 Overview
 
 
-.. NOTE(stephenfin): The below line numbers may need to be updated if the
-   README is modified
+.. NOTE(stephenfin): The below start-after/end-before may need to be updated
+   if the README is modified.
 
 .. include:: ../../README.rst
-   :start-line: 13
-   :end-line: 71
+   :start-after: What is Open vSwitch?
+   :end-before: What other documentation is available?
diff --git a/README.rst b/README.rst
index 4c9d9eddd..54d06d04b 100644
--- a/README.rst
+++ b/README.rst
@@ -1,5 +1,6 @@
-.. NOTE(stephenfin): If making changes to this file, ensure that the line
-   numbers found in 'Documentation/intro/what-is-ovs' are kept up-to-date.
+.. NOTE(stephenfin): If making changes to this file, ensure that the
+   start-after/end-before lines found in 'Documentation/intro/what-is-ovs'
+   are kept up-to-date.
 
 
 Open vSwitch
-- 
2.17.1

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

[ovs-dev] [PATCH 0/2] doc: what-is-ovs fix + CirrusCI badge.

[Ilya Maximets]

Ilya Maximets (2):
  doc: Fix cropped what-is-ovs page.
  doc: Add CirrusCI status badge to README.

 Documentation/intro/what-is-ovs.rst | 8 
 README.rst  | 7 +--
 2 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.17.1

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

[ovs-dev] [PATCH] OVN: run local logical flows first in S_ROUTER_OUT_SNAT table

[Lorenzo Bianconi]

Run local logical flows first if the gw router port is scheduled
on the local chassis in order to properly manage snat traffic

Tested-by: Eran Kuris 
Signed-off-by: Lorenzo Bianconi 
---
 ovn/northd/ovn-northd.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index de0c06d4b..4348eb281 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -6566,6 +6566,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
   count_1bits(ntohl(mask)) + 1,
   ds_cstr(), ds_cstr());
 } else {
+uint16_t priority = count_1bits(ntohl(mask)) + 1;
 /* Distributed router. */
 ds_clear();
 ds_put_format(, "ip && ip4.src == %s"
@@ -6575,6 +6576,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
 if (!distributed && od->l3redirect_port) {
 /* Flows for NAT rules that are centralized are only
  * programmed on the "redirect-chassis". */
+priority += 128;
 ds_put_format(, " && is_chassis_resident(%s)",
   od->l3redirect_port->json_key);
 }
@@ -6589,8 +6591,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
  * nat->logical_ip with the longest mask gets a higher
  * priority. */
 ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
-  count_1bits(ntohl(mask)) + 1,
-  ds_cstr(), ds_cstr());
+  priority, ds_cstr(),
+  ds_cstr());
 }
 }
 
-- 
2.20.1

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

Re: [ovs-dev] [PATCH v4 4/4] netdev-offload: Rename offload providers.

[Aaron Conole]

Ilya Maximets  writes:

> On 15.05.2019 23:32, Aaron Conole wrote:
>> Aaron Conole  writes:
>> 
>>> Ilya Maximets  writes:
>>>
 On 15.05.2019 19:13, Ilya Maximets wrote:
> Hi Aaron.
>
> Robot complains about lines not touched by this patch.
> This is strange.

 I suspect that it's because of file renaming.
>>>
>>> Yes, I think you're correct.
>>>
 What git version you're using? Or maybe some special options?
>>>
>>> I don't think it matters the version (I just tried locally with my git
>>> 2.19.1).  I think when the diff is being generated via git for
>>> processing (the robot doesn't patch file directly, it uses checkpatch.py
>>> -1), it includes all of old file as '-' and all of the new file as '+',
>>> so the robot will see it.
>>>
>>> I get a similar checkpatch complaint just doing a git mv on the file and
>>> trying to commit, because I have checkpatch setup as a commit hook.
>> 
>> I'll re-work the robot to do the git-am and then run against the
>> original patch file.  That will suppress these kinds of issues.
>
> This will probably help.
> However, I don't see any issues while using "checkpatch.py -1".
>
> On my Ubuntu:
>
> $ ./utilities/checkpatch.py -1
> == Checking 6921c382c9fd ("netdev-offload: Rename offload providers.") ==
> Lines checked: 263, no obvious problems found
>
> $ git --version
> git version 2.17.1
>
> And on my FreeBSD VM:
>
> # ./utilities/checkpatch.py -1
> == Checking 6a7b77036f3c ("netdev-offload: Rename offload providers.")
> ==
> Lines checked: 263, no obvious problems found
>   
>
> # git --version
> git version 2.20.1
>
>
> Also, this patch is exactly same in all previous versions since v2.
> And there was no complains for them.

I'll take a look at the older versions, too.

AFAIK nothing changed.

>> 
> Best regards, Ilya Maximets.
>
> On 15.05.2019 19:05, 0-day Robot wrote:
>> Bleep bloop.  Greetings Ilya Maximets, I am a robot and I have tried out 
>> your patch.
>> Thanks for your contribution.
>>
>> I encountered some error that I wasn't expecting.  See the details below.
>>
>>
>> checkpatch:
>> ERROR: Improper whitespace around control block
>> #1065 FILE: lib/netdev-offload-tc.c:173:
>> HMAP_FOR_EACH_WITH_HASH(data, ufid_node, ufid_hash, _tc) {
>>
>> ERROR: Improper whitespace around control block
>> #1135 FILE: lib/netdev-offload-tc.c:243:
>> HMAP_FOR_EACH_WITH_HASH(data, ufid_node, ufid_hash, _tc) {
>>
>> ERROR: Improper whitespace around control block
>> #1165 FILE: lib/netdev-offload-tc.c:273:
>> HMAP_FOR_EACH_WITH_HASH(data, tc_node, tc_hash,  _tc) {
>>
>> ERROR: Improper whitespace around control block
>> #1209 FILE: lib/netdev-offload-tc.c:317:
>> HMAP_FOR_EACH_WITH_HASH(data, node, hash, ) {
>>
>> WARNING: Line is 80 characters long (recommended limit is 79)
>> #1471 FILE: lib/netdev-offload-tc.c:579:
>> match_set_nw_src_masked(match, key->ipv4.ipv4_src, 
>> mask->ipv4.ipv4_src);
>>
>> WARNING: Line is 80 characters long (recommended limit is 79)
>> #1472 FILE: lib/netdev-offload-tc.c:580:
>> match_set_nw_dst_masked(match, key->ipv4.ipv4_dst, 
>> mask->ipv4.ipv4_dst);
>>
>> WARNING: Line is 82 characters long (recommended limit is 79)
>> #1545 FILE: lib/netdev-offload-tc.c:653:
>> size_t set_offset = nl_msg_start_nested(buf, 
>> OVS_ACTION_ATTR_SET);
>>
>> WARNING: Line is 83 characters long (recommended limit is 79)
>> #1550 FILE: lib/netdev-offload-tc.c:658:
>> nl_msg_put_be64(buf, OVS_TUNNEL_KEY_ATTR_ID, 
>> action->encap.id);
>>
>> WARNING: Line is 81 characters long (recommended limit is 79)
>> #1600 FILE: lib/netdev-offload-tc.c:708:
>> nl_msg_put_u32(buf, OVS_ACTION_ATTR_OUTPUT, 
>> odp_to_u32(outport));
>>
>> WARNING: Line is 84 characters long (recommended limit is 79)
>> #1618 FILE: lib/netdev-offload-tc.c:726:
>>|| (flower->offloaded_state == 
>> TC_OFFLOADED_STATE_UNDEFINED);
>>
>> ERROR: Improper whitespace around control block
>> #1756 FILE: lib/netdev-offload-tc.c:864:
>> NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) {
>>
>> WARNING: Line is 85 characters long (recommended limit is 79)
>> #2026 FILE: lib/netdev-offload-tc.c:1134:
>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? 
>> tnl_mask->tun_id : 0;
>>
>> ERROR: Improper whitespace around control block
>> #2189 FILE: lib/netdev-offload-tc.c:1297:
>> NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
>>
>> WARNING: Line is 81 characters long (recommended limit is 79)
>> #2191 FILE: 

[ovs-dev] Array Networks B2B Contact List

[lilly . martin]

Hi,



Would you be interested in *Targeted Array Networks B2B Contact List*? We
are happy to provide the database across globe, specifically North America,
EMEA, APAC and Latin America.



*We also have other users like:* Cloud flare, VMware, Incapsula, Fastly,
Zscaler, Distil Networks and many more.



Please review and let me know your thoughts, I will get back to you with
counts pricing and more information in my next email.



Await your response.



Regards

*Lilly Martin*

Marketing Manager



To opt-out replay in subject line.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] cirrus: Disable coredumps on FreeBSD.

[Ilya Maximets]

On 15.05.2019 21:15, Ben Pfaff wrote:
> On Wed, May 15, 2019 at 11:14:15AM +0300, Ilya Maximets wrote:
>> On 15.05.2019 8:51, Ben Pfaff wrote:
>>> On Tue, May 14, 2019 at 07:23:59PM +0300, Ilya Maximets wrote:
 Some tests uses 'kill -SEGV' to simulate segfault of a child process.
 This causes test failures on CirrusCI because process hangs in DL state
 for more than 10 seconds:
>>>
>>> That seems weird.  Do coredumps take a long time on FreeBSD?
>>
>> On my local FreeBSD 12.0 VM wait succeeds after 1 second just like
>> on my desktop with Linux.
>> However, on CirrusCI with FreeBSD 11.2 it takes 4+ seconds and
>> with FreeBSD 12.0 it takes 8+ seconds for successful runs and
>> fails frequently.
>>
>> I don't know why it takes so long. Maybe community cluster in
>> CirrusCI is overloaded or there is come configuration difference
>> (my coredump settings are equal to settings on CirrusCI).
> 
> OK.
> 
> Maybe some of that could go in the commit message.
> 
> Acked-by: Ben Pfaff 

Thanks Ben and Aaron! I added above information to commit message
and pushed to master and branch-2.11.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Toxicologist Contact Info

[Jody Kohn]

Hi,

 

Would you be interested in Toxicologists Contact’s list?

 

Such as:  Audiologists, Nurses, Diagnostic Radiologists, Oncology Nurses,
Urologist, Internist's, Dental Laboratories, Dermatologist’s,
Rheumatologist’, ENT Specialists, Family Practitioners, Pulmonologists,
Orthodontists, General Practitioners, Otolaryngologist, Cardiologists,
Dentists, Internal Medicine Specialists, Oral Surgeon’s, Psychiatrists,
Orthopedists, Gynecologists, Orthopedic Surgeons, Oncologists,
Hematologists, Ophthalmologists, Pathologists, Radiation Oncologists,
Radiologists, Chiropractors, Neurologists, Nephrologists , Obstetrics,
Pediatricians, ENT Specialists  EMR/EHR and lot more.

 

We can also help you reach out to Hospitals & Related Institutions:

 

ü  Medical Practitioners & Specialty Care Providers

ü  Long-term Care Facilities

ü  Health Insurance Carriers & Brokers

ü  Medical Equipment Manufacturers & Distributors

ü  Medical Laboratory/Diagnostic Facilities

ü  Pharmaceutical & Biotech Companies

ü  Healthcare & Medical Research Groups

 

Kindly let me know if we can schedule a call to discuss on this opportunity
and I will look forward to hearing from you on this.

 

Warm Regards,

Jody Kohn - Information Specialist

 

IF YOU WOULD RATHER NOT HEAR FROM US, PLEASE RESPOND MENTIONING “LEAVE OUT”
IN THE SUBJECT LINE. 
OR 
PASS ON THE MESSAGE TO THE RIGHT PERSON IN YOUR COMPANY

 

 

 

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

Re: [ovs-dev] [RFC V2] netdev-rte-offloads: HW offload virtio-forwarder

[Roni Bar Yanai]

>-Original Message-
>From: Ilya Maximets 
>Sent: Wednesday, May 15, 2019 4:37 PM
>To: Roni Bar Yanai ; ovs-dev@openvswitch.org; Ian
>Stokes ; Kevin Traynor 
>Cc: Eyal Lavee ; Oz Shlomo ; Eli
>Britstein ; Rony Efraim ; Asaf
>Penso 
>Subject: Re: [RFC V2] netdev-rte-offloads: HW offload virtio-forwarder
>
>On 15.05.2019 16:01, Roni Bar Yanai wrote:
>> Hi Ilya,
>>
>> Thanks for the comment.
>> I think the suggested arch is very good and has many advantages, and in fact 
>> I
>had something very similar as my internally first approach.
>> However, I had one problem: it doesn't solves the kernel case. It make sense
>doing forwarding using dpdk also when OVS is kernel (port representor and rule
>offloads are done with kernel OVS). It makes sense because we can have one
>solution and because DPDK has better performance.
>
>I'm not sure if it makes practical sense to run separate userpace
>datapath just to pass packets between vhost and VF. This actually
>matches with some of your own disadvantages of separate DPDK apps.
>Separate userspace datapath will need its own complex start,
>configuration and maintenance. Also it will consume additional cpu
>cores which will not be shared with kernel packet processing.
>I think that just move everything to userspace in this case would
>be much more simple for user than maintaining such configuration.
>

Maybe It doesn't make sense for OVS-DPDK but for OVS users it does.
When you run offload with OVS-kernel, and for some vendors this is the current
status, and virtio is a requirement, you now have millions of packets that
should be forwarded. Basically you have two options:
1. use external application (we discussed that).
2. create user space data plane and configure forwarding (OVS), but then
 you have performance issues as OVS is not optimized for this. And for kernel
 data plane much worse off course.
Regarding burning a core. In case of HW offload you will do it either way, and
there is no benefit for adding FW functionality for kernel data path, mainly 
because 
of kernel performance limitations.  
I agree that in such case moving to user space is a solution for some, but keep 
in
mind that some doesn't have such support for DPDK and for others they have
their own OVS based data path with their adjustments, so it will be a hard
transition. 
While arch is good for the two DPDK use cases, it leaves the kernel one out. 
Any thoughts how we can add this use case as well and still keep the suggested 
arch?

>> Do you think of a way we can create also support just FW case when there is 
>> no
>port representors?
>>
>> Thanks,
>> Roni
>>
>>
>>> -Original Message-
>>> From: Ilya Maximets 
>>> Sent: Wednesday, May 15, 2019 2:25 PM
>>> To: Roni Bar Yanai ; ovs-dev@openvswitch.org; Ian
>>> Stokes ; Kevin Traynor 
>>> Cc: Eyal Lavee ; Oz Shlomo ; Eli
>>> Britstein ; Rony Efraim ; Asaf
>>> Penso 
>>> Subject: Re: [RFC V2] netdev-rte-offloads: HW offload virtio-forwarder
>>>
>>> On 06.05.2019 13:43, Roni Bar Yanai wrote:
 Background
 ==
 OVS HW offload solution is consisted of forwarding and control. HW
>implements
 embedded switch that connects SRIOV VF's and forwards packets according
>to
>>> the
 dynamically configured HW rules (packets can be altered by HW rules).
>Packets
 that have no forwarding rule, called exception packets, are sent to the 
 control
 path (OVS SW). OVS SW will handle the exception packet (just like in SW 
 only
 mode),  namely calling up-call if no DP flow exists. OVS SW will use port
 representor for representing the VF. see:

     (_https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fd
>oc.
>>>
>dpdk.org%2Fguides%2Fprog_guide%2Fswitch_representation.html)._data
>>>
>=02%7C01%7Croniba%40mellanox.com%7Ce6056cf632f343dd734308d6d927f1e0%
>>>
>7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636935162916810125
>>>
>sdata=OT%2BXn0yz2pllFVRqFZDQCmXYymDuZfRamK72EX4Z2ZU%3Dreserv
>>> ed=0

 Packets sent from VF will get to the port representor and packets
 sent to the port representor will get to the VF. Once OVS SW generates a
 data plane flow, a new HW rules will be configured in the embedded switch.
 following packet on the on the same flow will be directed by HW only. Will
 arrive directly from VF (also uplink) to VF without getting to SW.

 For some HW architecture only the shortly presented SRIOV hw offload
 architecture is supported. SRIOV architecture requires that the guest will
 install a driver which is specific for the underlying HW. Specific HW 
 driver
 interduces two main problems for virtualization:

 1. It breaks virtualization in some sense (VM aware of the HW).
 2. less natural support for live migration.

 Using virtio interface solves both problems (on the expense of some loss in
 functionality and performance). However, for some HW offload, working
>directly
 with vitrio cannot be 

Re: [ovs-dev] [PATCHv8] netdev-afxdp: add new netdev type for AF_XDP.

[Ilya Maximets]

On 16.05.2019 2:27, William Tu wrote:
> Hi Ilya,
> 
> Thanks for your feedback.
> 
> On Mon, May 13, 2019 at 10:48 AM Ilya Maximets  wrote:
>>
>> On 10.05.2019 2:54, William Tu wrote:
>>> The patch introduces experimental AF_XDP support for OVS netdev.
>>> AF_XDP, Address Family of the eXpress Data Path, is a new Linux socket type
>>> built upon the eBPF and XDP technology.  It is aims to have comparable
>>> performance to DPDK but cooperate better with existing kernel's networking
>>> stack.  An AF_XDP socket receives and sends packets from an eBPF/XDP program
>>> attached to the netdev, by-passing a couple of Linux kernel's subsystems
>>> As a result, AF_XDP socket shows much better performance than AF_PACKET
>>> For more details about AF_XDP, please see linux kernel's
>>> Documentation/networking/af_xdp.rst. Note that by default, this is not
>>> compiled in.
>>>
>>> Signed-off-by: William Tu 
>>>
>>> ---
> 
> snip
> 
>>> +netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
>>> +char **errp OVS_UNUSED)
>>> +{
>>> +struct netdev_linux *dev = netdev_linux_cast(netdev);
>>> +const char *xdpmode;
>>> +int new_n_rxq;
>>> +
>>> +ovs_mutex_lock(>mutex);
>>> +
>>> +new_n_rxq = MAX(smap_get_int(args, "n_rxq", NR_QUEUE), 1);
>>> +if (new_n_rxq > MAX_XSKQ) {
>>> +ovs_mutex_unlock(>mutex);
>>> +return EINVAL;
>>> +}
>>> +
>>> +if (new_n_rxq != netdev->n_rxq) {
>>> +dev->requested_n_rxq = new_n_rxq;
>>> +netdev_request_reconfigure(netdev);
>>> +}
>>> +
>>> +xdpmode = smap_get(args, "xdpmode");
>>> +if (xdpmode && strncmp(xdpmode, "drv", 3) == 0) {
>>> +dev->requested_xdpmode = XDP_ZEROCOPY;
>>> +if (dev->xdpmode != dev->requested_xdpmode) {
>>> +netdev_request_reconfigure(netdev);
>>> +}
>>> +} else {
>>> +dev->requested_xdpmode = XDP_COPY;
>>> +if (dev->xdpmode != dev->requested_xdpmode) {
>>> +netdev_request_reconfigure(netdev);
>>> +}
>>> +}
>>
>> Above code will request reconfiguration infinitely until it reconfiguration
>> finished. This could cause multiple reconfigurations in a row for the same
>> configuration change. Better version could look like this:
>>
>> new_n_rxq = MAX(smap_get_int(args, "n_rxq", NR_QUEUE), 1);
>> if (new_n_rxq > MAX_XSKQ) {
>> ovs_mutex_unlock(>mutex);
>> VLOG_ERR("%s: Too big 'n_rxq' (%d > %d).",
>>  netdev_get_name(netdev), new_n_rxq, MAX_XSKQ);
>> return EINVAL;
>> }
>>
>> str_xdpmode = smap_get_def(args, "xdpmode", "skb");
>> if (!strcasecmp(str_xdpmode, "drv")) {
>> xdpmode = XDP_ZEROCOPY;
>> } else if (!strcasecmp(str_xdpmode, "skb")) {
>> xdpmode = XDP_COPY;
>> } else {
>> VLOG_ERR("%s: Incorrect xdpmode (%s).",
>>  netdev_get_name(netdev), str_xdpmode);
>> ovs_mutex_unlock(>mutex);
>> return EINVAL;
>> }
>>
>> if (dev->requested_n_rxq != new_n_rxq
>> || dev->requested_xdpmode != xdpmode) {
>> dev->requested_n_rxq = new_n_rxq;
>> dev->requested_xdpmode = xdpmode
>> netdev_request_reconfigure(netdev);
>> }
>>
>> The main difference is checking "new" with "requested", not the "new" with
>> "current". This allows us to request reconfiguration only once for each
>> change. I also made few cosmetic changes which you may find useful, however
>> it's up to you.
> 
> Thanks, will fix it in next version.
> 
>>
>>> +for (i = 0; i < rcvd; i++) {
>>> +uint64_t addr = xsk_ring_cons__rx_desc(>rx, idx_rx)->addr;
>>> +uint32_t len = xsk_ring_cons__rx_desc(>rx, idx_rx)->len;
>>> +char *pkt = xsk_umem__get_data(xsk->umem->buffer, addr);
>>> +uint64_t index;
>>> +
>>> +struct dp_packet_afxdp *xpacket;
>>> +struct dp_packet *packet;
>>> +
>>> +index = addr >> FRAME_SHIFT;
>>> +xpacket = UMEM2XPKT(xsk->umem->xpool.array, index);
>>> +
>>> +packet = >packet;
>>> +xpacket->mpool = >umem->mpool;
>>> +
>>> +/* Initialize the struct dp_packet */
>>> +dp_packet_use_afxdp(packet, pkt, FRAME_SIZE - FRAME_HEADROOM);
>>> +dp_packet_set_size(packet, len);
>>> +
>>> +/* Add packet into batch, increase batch->count */
>>> +dp_packet_batch_add(batch, packet);
>>> +
>>> +idx_rx++;
>>> +}
>>> +
>>> +/* We've consume rcvd packets in RX, now re-fill the
>>> + * same number back to FILL queue.
>>> + */
>>> +ret = umem_elem_pop_n(>umem->mpool, rcvd, (void **)elems);
>>> +if (OVS_UNLIKELY(ret)) {> +return -ENOMEM;
>>> +}
>>
>> Can this be done before actually receiving packets? i.e. don't receive
>> anything if cant refill.
> 
> I'm not sure I understand your point.
> Do you suggest moving this umem_elem_pop_n in the beginning?


Yes. I meant this.
It's just an optimization. We don't need to 

Re: [ovs-dev] [PATCH] ovn-controller: Fix parsing of OVN tunnel IDs

[Dumitru Ceara]

On Wed, May 15, 2019 at 10:30 PM venugopal iyer  wrote:
>
> Thanks for fixing this! a comment below:
>
> On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara  
> wrote:
>
>
> Add tunnel-id creation and parsing functions in encaps.h.
>
> Reported-at: https://bugzilla.redhat.com/1708131
> Reported-by: Haidong Li 
> Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> Signed-off-by: Dumitru Ceara 
> ---
> ovn/controller/bfd.c| 31 ---
> ovn/controller/encaps.c| 87 -
> ovn/controller/encaps.h|  6 +++
> ovn/controller/ovn-controller.h |  7 
> ovn/controller/physical.c  | 51 +---
> ovn/controller/pinctrl.c|  4 +-
> 6 files changed, 130 insertions(+), 56 deletions(-)
>
> diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c
> index d016e27..b6aef04 100644
> --- a/ovn/controller/bfd.c
> +++ b/ovn/controller/bfd.c
> @@ -15,6 +15,7 @@
>
> #include 
> #include "bfd.h"
> +#include "encaps.h"
> #include "lport.h"
> #include "ovn-controller.h"
>
> @@ -72,14 +73,16 @@ bfd_calculate_active_tunnels(const struct ovsrec_bridge 
> *br_int,
> const char *id = smap_get(_rec->external_ids,
>   "ovn-chassis-id");
> if (id) {
> -char *chassis_name;
> -char *save_ptr = NULL;
> -char *tokstr = xstrdup(id);
> -chassis_name = strtok_r(tokstr, 
> OVN_MVTEP_CHASSISID_DELIM, _ptr);
> -if (chassis_name && 
> !sset_contains(active_tunnels, chassis_name)) {
> -sset_add(active_tunnels, chassis_name);
> +char *chassis_name = NULL;
> +
> +if (encaps_tunnel_id_parse(id, _name,
> +  NULL)) {
> +if (!sset_contains(active_tunnels,
> +  chassis_name)) {
> +sset_add(active_tunnels, chassis_name);
> +}
> +free(chassis_name);
> }
> -free(tokstr);
> }
> }
> }
> @@ -193,17 +196,17 @@ bfd_run(const struct ovsrec_interface_table 
> *interface_table,
> const char *tunnel_id = smap_get(_int->ports[k]->external_ids,
>   "ovn-chassis-id");
> if (tunnel_id) {
> -char *chassis_name;
> -char *save_ptr = NULL;
> -char *tokstr = xstrdup(tunnel_id);
> +char *chassis_name = NULL;
> char *port_name = br_int->ports[k]->name;
>
> sset_add(, port_name);
> -chassis_name = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, 
> _ptr);
> -if (chassis_name && sset_contains(_chassis, chassis_name)) {
> -sset_add(_ifaces, port_name);
> +
> +if (encaps_tunnel_id_parse(tunnel_id, _name, NULL)) {
> +if (sset_contains(_chassis, chassis_name)) {
> +sset_add(_ifaces, port_name);
> +}
> +free(chassis_name);
> }
> -free(tokstr);
> }
> }
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index dcf7810..d467540 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -26,6 +26,13 @@
>
> VLOG_DEFINE_THIS_MODULE(encaps);
>
> +/*
> + * Given there could be multiple tunnels with different IPs to the same
> + * chassis we annotate the ovn-chassis-id with
> + * OVN_MVTEP_CHASSISID_DELIM.
> + */
> +#defineOVN_MVTEP_CHASSISID_DELIM '@'
> +
> void
> encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> {
> @@ -78,6 +85,83 @@ tunnel_create_name(struct tunnel_ctx *tc, const char 
> *chassis_id)
> return NULL;
> }
>
> +/*
> + * Returns a tunnel-id of the form 'chassis_id'-delimiter-'encap_ip'.
> + */
> +char *
> +encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
> +{
> +return xasprintf("%s%c%s", chassis_id, OVN_MVTEP_CHASSISID_DELIM,
> +encap_ip);
> +}
> +
> +/*
> + * Parses a 'tunnel_id' of the form .
> + * If the 'chassis_id' argument is not NULL the function will allocate memory
> + * and store the chassis-id part of the tunnel-id at '*chassis_id'.
> + * If the 'encap_ip' argument is not NULL the function will allocate memory
> + * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
> + */
> +bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> +char **encap_ip)
> +{
> +const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> +
> +if (!match) {
> +return false;
> +}

Re: [ovs-dev] [PATCH v2] ovn: Add a new logical switch port type - 'virtual'

[Numan Siddique]

On Thu, May 16, 2019 at 1:12 AM Han Zhou  wrote:

>
>
> On Wed, May 15, 2019 at 11:55 AM Numan Siddique 
> wrote:
>
>>
>>
>> On Thu, May 16, 2019 at 12:10 AM Han Zhou  wrote:
>>
>>>
>>>
>>> On Wed, May 15, 2019 at 11:36 AM  wrote:
>>> >
>>> > From: Numan Siddique 
>>> >
>>> > This new type is added for the following reasons:
>>> >
>>> >   - When a load balancer is created in an OpenStack deployment with
>>> Octavia
>>> > service, it creates a logical port 'VIP' for the virtual ip.
>>> >
>>> >   - This logical port is not bound to any VIF.
>>> >
>>> >   - Octavia service creates a service VM (with another logical port
>>> 'P' which
>>> > belongs to the same logical switch)
>>> >
>>> >   - The virtual ip 'VIP' is configured on this service VM.
>>> >
>>> >   - This service VM provides the load balancing for the VIP with the
>>> configured
>>> > backend IPs.
>>> >
>>> >   - Octavia service can be configured to create few service VMs with
>>> active-standby mode
>>> > with the active VM configured with the VIP.  The VIP can move
>>> between
>>> > these service nodes.
>>> >
>>> > Presently there are few problems:
>>> >
>>> >   - When a floating ip (externally reachable IP) is associated to the
>>> VIP and if
>>> > the compute nodes have external connectivity then the external
>>> traffic cannot
>>> > reach the VIP using the floating ip as the VIP logical port would
>>> be down.
>>> > dnat_and_snat entry in NAT table for this vip will have
>>> 'external_mac' and
>>> > 'logical_port' configured.
>>> >
>>> >   - The only way to make it work is to clear the 'external_mac' entry
>>> so that
>>> > the gateway chassis does the DNAT for the VIP.
>>> >
>>> > To solve these problems, this patch proposes a new logical port type -
>>> virtual.
>>> > CMS when creating the logical port for the VIP, should
>>> >
>>> >  - set the type as 'virtual'
>>> >
>>> >  - configure the VIP in the newly added column
>>> Logical_Switch_Port.virtual_ip
>>> >
>>> >  - And set the virtual parents in the new added column
>>> Logical_Switch_Port.virtual_parents.
>>> >These virtual parents are the one which can be configured wit the
>>> VIP.
>>> >
>>> > If suppose the virtual_ip is configured to 10.0.0.10 on a virtual
>>> logical port 'sw0-vip'
>>> > and the virtual_parents are set to - [sw0-p1, sw0-p2] then below
>>> logical flows are added in the
>>> > lsp_in_arp_rsp logical switch pipeline
>>> >
>>> >  - table=11(ls_in_arp_rsp), priority=100,
>>> >match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10
>>> && arp.tpa == 10.0.0.10) ||
>>> >  (arp.op == 2 && arp.spa ==
>>> 10.0.0.10))),
>>> >action=(bind_vport("sw0-vip", inport); next;)
>>> > - table=11(ls_in_arp_rsp), priority=100,
>>> >match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa == 10.0.0.10
>>> && arp.tpa == 10.0.0.10) ||
>>> >  (arp.op == 2 && arp.spa ==
>>> 10.0.0.10))),
>>> >action=(bind_vport("sw0-vip", inport); next;)
>>> >
>>> > The action bind_vport will claim the logical port - sw0-vip on the
>>> chassis where this action
>>> > is executed. Since the port - sw0-vip is claimed by a chassis, the
>>> dnat_and_snat rule for
>>> > the VIP will be handled by the compute node.
>>> >
>>> > Signed-off-by: Numan Siddique 
>>>
>>> Hi Numan, this looks interesting. I haven't reviewed code yet, but just
>>> some questions to better understand the feature.
>>>
>>> Firstly, can Octavia be implemented by using the distributed LB feature
>>> of OVN, instead of using dedicated node? What's the major gap for using the
>>> OVN LB?
>>>
>>>
>> Yes. Its possible to use the native OVN LB feature. There's already a
>> provider driver in Octavia for
>> OVN (
>> https://github.com/openstack/networking-ovn/blob/master/networking_ovn/octavia/ovn_driver.py
>> ).
>> When creating the LB providing the option --provider-driver=ovn will
>> create OVN LB.
>>
>> However OVN LB is limited to L4 and there are no health checks. Octavia
>> amphora driver supports lots of
>> features like L7, health check etc. I think we should definitely look
>> into adding the health monitor feature for OVN LB.
>> But I think supporting L7 LBs is out of question for OVN LB.
>> For complex load balancer needs, I think its better to rely on external
>> load balancers like the Octavia amphora driver.
>> Octavia amphora driver creates a service VM and runs an haproxy instance
>> inside it to provider the load balancing.
>>
>> Thanks for the detailed explain. Yes, this makes sense!
>
>>
>>
>>
>>> Secondly, how is associating the floating-ip with the VIP configured
>>> currently?
>>>
>>
>> networking-ovn creates a dnat_and_snat entry when floating ip is
>> associated for the VIP port. Right now it doesn't set
>> the external_mac and logical_port columns for DVR deployments. And this
>> has been a draw back.
>>
>
> Ok, thanks. Maybe I should check more details on neutron networking-ovn.
>
>>
>>> 

Re: [ovs-dev] [PATCHv8] netdev-afxdp: add new netdev type for AF_XDP.

[Ilya Maximets]

On 16.05.2019 2:20, William Tu wrote:
>>> +   OVS AF_XDP netdev is using the userspace datapath, the same datapath
>>> +   as used by OVS-DPDK.  So it requires --disable-system for ovs-vswitchd
>>> +   and datapath_type=netdev when adding a new bridge.
>>
>> I don't think that '--disable-system' is needed. It doesn't affect anything.
>>
> 
> Thanks I will remove it.
> 
>> 
>>
>>> +int
>>> +netdev_linux_afxdp_batch_send(struct xsk_socket_info *xsk,
>>> +  struct dp_packet_batch *batch)
>>> +{
>>
>> One important issue here. netdev_linux_send() is thread-safe, because
>> all the syscalls and memory allocations there are thread-safe.
>> However, all the xsk_ring_* APIs are not thread safe and if two
>> threads will try to send packets to the same tx queue they might
>> destroy the rings. So, it's necessary to start using 'concurrent_txq'
>> flag with per-queue locks.
>> Note that 'concurrent_txq' == 'false' only if 'n_txq' > 'n_pmd_threads'.
>>
> 
> Thanks!
> I have one question. For example if I have n_txq=4 and n_pmd_threds=2,
> then concurrent_txq = false.
> 
> Assume pmd1 processing rx queue0 on port1 and pmd2 processes rx queue0 on 
> port2.
> What if both pmd1 and pmd2 try to send AF_XDP packet tx queue0 on port2?
> Then both pmd threads are calling the send function on port2 queue0
> concurrently.
> Does that mean I have to unconditionally add per-queue lock?

No. You don't need that. dpif-netdev manages Tx queues in a way that
two threads will never use same Tx queue of the same netdev without
'concurrent_txq' set to 'true'.

In your case above you have 'n_txq' > 'n_pmd_threads' and dpif-netdev
will use static Tx queue ids, i.e. pmd1 will always use Tx queue #0
and pmd2 will always use Tx queue #1, main thread will always use
Tx queue #2.

If you'll have 'n_txq' <= 'n_pmd_threads', dpif-netdev will use dynamic
Tx queue ids with some kind of XPS mechanism. i.e. threads will allocate
Tx queue id before sending. XPS will try, but it doesn't guarantee that
other thread will not use same queue, so 'concurrent_txq' will be always
"true" for this port.

You may use 'netdev_dpdk_send__()' as a reference.

> 
> Regards,
> William
> 
>>> +struct umem_elem *elems_pop[BATCH_SIZE];
>>> +struct umem_elem *elems_push[BATCH_SIZE];
>>> +uint32_t tx_done, idx_cq = 0;
>>> +struct dp_packet *packet;
>>> +uint32_t idx = 0;
>>> +int j, ret, retry_count = 0;
>>> +const int max_retry = 4;
>>> +
>>> +ret = umem_elem_pop_n(>umem->mpool, batch->count, (void 
>>> **)elems_pop);
>>> +if (OVS_UNLIKELY(ret)) {
>>> +return EAGAIN;
>>> +}
>>> +
>>> +/* Make sure we have enough TX descs */
>>> +ret = xsk_ring_prod__reserve(>tx, batch->count, );
>>> +if (OVS_UNLIKELY(ret == 0)) {
>>> +umem_elem_push_n(>umem->mpool, batch->count, (void 
>>> **)elems_pop);
>>> +return EAGAIN;
>>> +}
>>> +
>>> +DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
>>> +struct umem_elem *elem;
>>> +uint64_t index;
>>> +
>>> +elem = elems_pop[i];
>>> +/* Copy the packet to the umem we just pop from umem pool.
>>> + * We can avoid this copy if the packet and the pop umem
>>> + * are located in the same umem.
>>> + */
>>> +memcpy(elem, dp_packet_data(packet), dp_packet_size(packet));
>>> +
>>> +index = (uint64_t)((char *)elem - (char *)xsk->umem->buffer);
>>> +xsk_ring_prod__tx_desc(>tx, idx + i)->addr = index;
>>> +xsk_ring_prod__tx_desc(>tx, idx + i)->len
>>> += dp_packet_size(packet);
>>> +}
>>> +xsk_ring_prod__submit(>tx, batch->count);
>>> +xsk->outstanding_tx += batch->count;
>>> +
>>> +ret = kick_tx(xsk);
>>> +if (OVS_UNLIKELY(ret)) {
>>> +umem_elem_push_n(>umem->mpool, batch->count, (void 
>>> **)elems_pop);
>>> +VLOG_WARN_RL(, "error sending AF_XDP packet: %s",
>>> + ovs_strerror(ret));
>>> +return ret;
>>> +}
>>> +
>>> +retry:
>>> +/* Process CQ */
>>> +tx_done = xsk_ring_cons__peek(>umem->cq, batch->count, _cq);
>>> +if (tx_done > 0) {
>>> +xsk->outstanding_tx -= tx_done;
>>> +xsk->tx_npkts += tx_done;
>>> +}
>>> +
>>> +/* Recycle back to umem pool */
>>> +for (j = 0; j < tx_done; j++) {
>>> +struct umem_elem *elem;
>>> +uint64_t addr;
>>> +
>>> +addr = *xsk_ring_cons__comp_addr(>umem->cq, idx_cq++);
>>> +
>>> +elem = ALIGNED_CAST(struct umem_elem *,
>>> +(char *)xsk->umem->buffer + addr);
>>> +elems_push[j] = elem;
>>> +}
>>> +
>>> +ret = umem_elem_push_n(>umem->mpool, tx_done, (void 
>>> **)elems_push);
>>> +ovs_assert(ret == 0);
>>> +
>>> +xsk_ring_cons__release(>umem->cq, tx_done);
>>> +
>>> +if (xsk->outstanding_tx > PROD_NUM_DESCS - (PROD_NUM_DESCS >> 2)) {
>>> +/* If there are still a lot not transmitted, try harder. */
>>> +if 

Re: [ovs-dev] [PATCH v4 4/4] netdev-offload: Rename offload providers.

[Ilya Maximets]

On 15.05.2019 23:32, Aaron Conole wrote:
> Aaron Conole  writes:
> 
>> Ilya Maximets  writes:
>>
>>> On 15.05.2019 19:13, Ilya Maximets wrote:
 Hi Aaron.

 Robot complains about lines not touched by this patch.
 This is strange.
>>>
>>> I suspect that it's because of file renaming.
>>
>> Yes, I think you're correct.
>>
>>> What git version you're using? Or maybe some special options?
>>
>> I don't think it matters the version (I just tried locally with my git
>> 2.19.1).  I think when the diff is being generated via git for
>> processing (the robot doesn't patch file directly, it uses checkpatch.py
>> -1), it includes all of old file as '-' and all of the new file as '+',
>> so the robot will see it.
>>
>> I get a similar checkpatch complaint just doing a git mv on the file and
>> trying to commit, because I have checkpatch setup as a commit hook.
> 
> I'll re-work the robot to do the git-am and then run against the
> original patch file.  That will suppress these kinds of issues.

This will probably help.
However, I don't see any issues while using "checkpatch.py -1".

On my Ubuntu:

$ ./utilities/checkpatch.py -1
== Checking 6921c382c9fd ("netdev-offload: Rename offload providers.") ==
Lines checked: 263, no obvious problems found

$ git --version
git version 2.17.1

And on my FreeBSD VM:

# ./utilities/checkpatch.py -1  

== Checking 6a7b77036f3c ("netdev-offload: Rename offload providers.") ==   
 
Lines checked: 263, no obvious problems found   
 

 
# git --version 

git version 2.20.1


Also, this patch is exactly same in all previous versions since v2.
And there was no complains for them.

> 
 Best regards, Ilya Maximets.

 On 15.05.2019 19:05, 0-day Robot wrote:
> Bleep bloop.  Greetings Ilya Maximets, I am a robot and I have tried out 
> your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> ERROR: Improper whitespace around control block
> #1065 FILE: lib/netdev-offload-tc.c:173:
> HMAP_FOR_EACH_WITH_HASH(data, ufid_node, ufid_hash, _tc) {
>
> ERROR: Improper whitespace around control block
> #1135 FILE: lib/netdev-offload-tc.c:243:
> HMAP_FOR_EACH_WITH_HASH(data, ufid_node, ufid_hash, _tc) {
>
> ERROR: Improper whitespace around control block
> #1165 FILE: lib/netdev-offload-tc.c:273:
> HMAP_FOR_EACH_WITH_HASH(data, tc_node, tc_hash,  _tc) {
>
> ERROR: Improper whitespace around control block
> #1209 FILE: lib/netdev-offload-tc.c:317:
> HMAP_FOR_EACH_WITH_HASH(data, node, hash, ) {
>
> WARNING: Line is 80 characters long (recommended limit is 79)
> #1471 FILE: lib/netdev-offload-tc.c:579:
> match_set_nw_src_masked(match, key->ipv4.ipv4_src, 
> mask->ipv4.ipv4_src);
>
> WARNING: Line is 80 characters long (recommended limit is 79)
> #1472 FILE: lib/netdev-offload-tc.c:580:
> match_set_nw_dst_masked(match, key->ipv4.ipv4_dst, 
> mask->ipv4.ipv4_dst);
>
> WARNING: Line is 82 characters long (recommended limit is 79)
> #1545 FILE: lib/netdev-offload-tc.c:653:
> size_t set_offset = nl_msg_start_nested(buf, 
> OVS_ACTION_ATTR_SET);
>
> WARNING: Line is 83 characters long (recommended limit is 79)
> #1550 FILE: lib/netdev-offload-tc.c:658:
> nl_msg_put_be64(buf, OVS_TUNNEL_KEY_ATTR_ID, 
> action->encap.id);
>
> WARNING: Line is 81 characters long (recommended limit is 79)
> #1600 FILE: lib/netdev-offload-tc.c:708:
> nl_msg_put_u32(buf, OVS_ACTION_ATTR_OUTPUT, 
> odp_to_u32(outport));
>
> WARNING: Line is 84 characters long (recommended limit is 79)
> #1618 FILE: lib/netdev-offload-tc.c:726:
>|| (flower->offloaded_state == 
> TC_OFFLOADED_STATE_UNDEFINED);
>
> ERROR: Improper whitespace around control block
> #1756 FILE: lib/netdev-offload-tc.c:864:
> NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) {
>
> WARNING: Line is 85 characters long (recommended limit is 79)
> #2026 FILE: lib/netdev-offload-tc.c:1134:
> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? 
> tnl_mask->tun_id : 0;
>
> ERROR: Improper whitespace around control block
> #2189 FILE: lib/netdev-offload-tc.c:1297:
> NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
>
> WARNING: