[ovs-dev] Herzliche Glückwünsche

[Bill Gate Foundation]

-- 
Hallo Charity Gewinner,
 Herzlichen Glückwunsch, wir möchten Sie darüber informieren, dass Sie in der 
aktuellen BILL & MELINDA GATES Business Magnate Charity Spende die Summe
von €850.000,00, gewonnen haben, Ihre E-Mail-Adresse wurde unter den 10
glücklichen Gewinnern ausgewählt . ..

https://www.youtube.com/watch?v=ejgjQP1T-iA

Kontaktieren Sie uns für weitere Informationen, wie Sie Ihr Geld bekommen
können.

HERZLICHE GLÜCKWÜNSCHE!!

Beste Wünsche und Gott segne dich
Sekretär frei andreas beat
Gesetzlicher Vertreter Bill & Melinda Gates
Missionarische Wohltätigkeitsstiftung für das humanitäre Programm.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: Fix wrong push/pop ethernet validation

[Ben Pfaff]

On Fri, Nov 02, 2018 at 08:31:06AM -0700, Gregory Rose wrote:
> On 11/2/2018 4:45 AM, Jaime Caamaño Ruiz wrote:
> >Upstream commit:
> > commit 46ebe2834ba5b541f28ee72e556a3fed42c47570
> > Author: Jaime Caamaño Ruiz 
> > Date:   Wed Oct 31 18:52:03 2018 +0100
> >
> > openvswitch: Fix push/pop ethernet validation
> >
> > When there are both pop and push ethernet header actions among the
> > actions to be applied to a packet, an unexpected EINVAL (Invalid
> > argument) error is obtained. This is due to mac_proto not being reset
> > correctly when those actions are validated.
> >
> > Reported-at:
> > 
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
> > Fixes: 91820da6ae85 ("openvswitch: add Ethernet push and pop actions")
> > Signed-off-by: Jaime Caamaño Ruiz 
> >
> >Reported-at:
> >https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
> >Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")
> >Signed-off-by: Jaime Caamaño Ruiz 

Applied to master, thanks Jaime and Greg!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] checkpatch: Speed up checking when spell checking not enabled.

[Ben Pfaff]

Thanks for the patches, I applied this to master.

On Fri, Nov 02, 2018 at 01:17:25PM -0700, Yifeng Sun wrote:
> Looks good to me, thanks.
> 
> Reviewed-by: Yifeng Sun 
> 
> On Thu, Nov 1, 2018 at 8:07 AM Ben Pfaff  wrote:
> 
> > On my machine it takes almost a second for enchant to read its dictionary.
> > This time is wasted when spell checking is not enabled.  This commit makes
> > checkpatch read the dictionary only when it will be used.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  utilities/checkpatch.py | 117
> > +---
> >  1 file changed, 62 insertions(+), 55 deletions(-)
> >
> > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> > index 54aa4b6346db..8bbda78989c4 100755
> > --- a/utilities/checkpatch.py
> > +++ b/utilities/checkpatch.py
> > @@ -21,59 +21,6 @@ import os
> >  import re
> >  import sys
> >
> > -try:
> > -import enchant
> > -
> > -extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
> > 'netdev',
> > -  'selinux', 'ovs-ctl', 'dpctl', 'ofctl',
> > 'openvswitch',
> > -  'dpdk', 'hugepage', 'hugepages', 'pmd', 'upcall',
> > -  'vhost', 'rx', 'tx', 'vhostuser', 'openflow',
> > 'qsort',
> > -  'rxq', 'txq', 'perf', 'stats', 'struct', 'int',
> > -  'char', 'bool', 'upcalls', 'nicira', 'bitmask',
> > 'ipv4',
> > -  'ipv6', 'tcp', 'tcp4', 'tcpv4', 'udp', 'udp4',
> > 'udpv4',
> > -  'icmp', 'icmp4', 'icmpv6', 'vlan', 'vxlan', 'cksum',
> > -  'csum', 'checksum', 'ofproto', 'numa', 'mempool',
> > -  'mempools', 'mbuf', 'mbufs', 'hmap', 'cmap', 'smap',
> > -  'dhcpv4', 'dhcp', 'dhcpv6', 'opts', 'metadata',
> > -  'geneve', 'mutex', 'netdev', 'netdevs', 'subtable',
> > -  'virtio', 'qos', 'policer', 'datapath', 'tunctl',
> > -  'attr', 'ethernet', 'ether', 'defrag', 'defragment',
> > -  'loopback', 'sflow', 'acl', 'initializer', 'recirc',
> > -  'xlated', 'unclosed', 'netlink', 'msec', 'usec',
> > -  'nsec', 'ms', 'us', 'ns', 'kilobits', 'kbps',
> > -  'kilobytes', 'megabytes', 'mbps', 'gigabytes',
> > 'gbps',
> > -  'megabits', 'gigabits', 'pkts', 'tuple', 'miniflow',
> > -  'megaflow', 'conntrack', 'vlans', 'vxlans', 'arg',
> > -  'tpid', 'xbundle', 'xbundles', 'mbundle',
> > 'mbundles',
> > -  'netflow', 'localnet', 'odp', 'pre', 'dst', 'dest',
> > -  'src', 'ethertype', 'cvlan', 'ips', 'msg', 'msgs',
> > -  'liveness', 'userspace', 'eventmask', 'datapaths',
> > -  'slowpath', 'fastpath', 'multicast', 'unicast',
> > -  'revalidation', 'namespace', 'qdisc', 'uuid',
> > 'ofport',
> > -  'subnet', 'revalidation', 'revalidator',
> > 'revalidate',
> > -  'l2', 'l3', 'l4', 'openssl', 'mtu', 'ifindex',
> > 'enum',
> > -  'enums', 'http', 'https', 'num', 'vconn', 'vconns',
> > -  'conn', 'nat', 'memset', 'memcmp', 'strcmp',
> > -  'strcasecmp', 'tc', 'ufid', 'api', 'ofpbuf',
> > 'ofpbufs',
> > -  'hashmaps', 'hashmap', 'deref', 'dereference', 'hw',
> > -  'prio', 'sendmmsg', 'sendmsg', 'malloc', 'free',
> > 'alloc',
> > -  'pid', 'ppid', 'pgid', 'uid', 'gid', 'sid', 'utime',
> > -  'stime', 'cutime', 'cstime', 'vsize', 'rss',
> > 'rsslim',
> > -  'whcan', 'gtime', 'eip', 'rip', 'cgtime', 'dbg',
> > 'gw',
> > -  'sbrec', 'bfd', 'sizeof', 'pmds', 'nic', 'nics',
> > 'hwol',
> > -  'encap', 'decap', 'tlv', 'tlvs', 'decapsulation',
> > 'fd',
> > -  'cacheline', 'xlate', 'skiplist', 'idl',
> > 'comparator',
> > -  'natting', 'alg', 'pasv', 'epasv', 'wildcard',
> > 'nated',
> > -  'amd64', 'x86_64', 'recirculation']
> > -
> > -spell_check_dict = enchant.Dict("en_US")
> > -for kw in extra_keywords:
> > -spell_check_dict.add(kw)
> > -
> > -no_spellcheck = False
> > -except:
> > -no_spellcheck = True
> > -
> >  RETURN_CHECK_INITIAL_STATE = 0
> >  RETURN_CHECK_STATE_WITH_RETURN = 1
> >  RETURN_CHECK_AWAITING_BRACE = 2
> > @@ -86,6 +33,66 @@ total_line = 0
> >  colors = False
> >  spellcheck_comments = False
> >  quiet = False
> > +spell_check_dict = None
> > +
> > +
> > +def open_spell_check_dict():
> > +import enchant
> > +
> > +try:
> > +extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
> > +  'netdev', 'selinux', 'ovs-ctl', 'dpctl',
> > 'ofctl',
> > +   

Re: [ovs-dev] [PATCH v3] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

[Ben Pfaff]

I guess I had in mind that OVS_RESOLV_CONF would point to an alternate
file to be used instead of /etc/resolve.conf.

On Fri, Nov 02, 2018 at 02:51:19PM -0700, Yifeng Sun wrote:
> Hi Ben,
> 
> Based on the discussion, I am going to create a dns_resolve_timeout__
> specifically for
> sandboxing and testing. In runtime, dns resolving will check the
> environment variable,
> say OVS_RESOLV_CONF, to decide whether or not to use the timeout version of
> actual
> dns resolution. Does this sound good to you?
> 
> Thanks,
> Yifeng
> 
> On Fri, Nov 2, 2018 at 1:21 PM Ben Pfaff  wrote:
> 
> > On Fri, Nov 02, 2018 at 04:05:13PM -0400, Mark Michelson wrote:
> > > On 11/01/2018 02:15 PM, Yifeng Sun wrote:
> > > >Hi Mark,
> > > >
> > > >Thanks a lot for the information, that is very helpful.
> > > >
> > > > From your comments, I understand that we should keep the current
> > > >DNS resolving behavior as is. The thing we need to improve is to
> > > >stop resolving if there is no /etc/resolv.conf configured.
> > > >
> > > >And if /etc/resolv.conf exists and has "127.0.0.1" as the dns server,
> > > >like Numan mentioned, resolver will block. For testing, I feel that a
> > > >timeout dns_resolve makes sense. Can we determine testing
> > > >context in runtime?
> > >
> > > I'm not sure about that. After thinking about this a bit more, it may
> > > actually make more sense to use a sandboxed resolv.conf during our tests.
> > > This way, we have control over what servers are configured and what
> > timeouts
> > > are. I'm not sure exactly how to achieve this, but it seems like a more
> > > ideal option than changing the underlying code for tests.
> >
> > We use OVS_* environment variables for sandboxing other things, we could
> > have an OVS_RESOLV_CONF variable I guess.
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] documentation: man vswitchd.conf.db(5) updated flow-restore-wait

[Ben Pfaff]

On Fri, Nov 02, 2018 at 03:25:29PM -0700, Zak Whittington wrote:
> Commit 7ed73428a changed the behavior of flow-restore-wait to
> also prevent the switch from connecting to controllers in the
> controller table, but failed to update the man page documentation
> generated by vswitchd/vswitch.xml to reflect this.
> 
> This commit adds that documentation.
> 
> Signed-off-by: Zak Whittington 

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

Re: [ovs-dev] [PATCH] datapath: Fix wrong push/pop ethernet validation

[Ben Pfaff]

On Fri, Nov 02, 2018 at 01:57:08PM -0700, Gregory Rose wrote:
> On 11/2/2018 1:46 PM, Ben Pfaff wrote:
> >On Fri, Nov 02, 2018 at 08:31:06AM -0700, Gregory Rose wrote:
> >>On 11/2/2018 4:45 AM, Jaime Caamaño Ruiz wrote:
> >>>Upstream commit:
> >>> commit 46ebe2834ba5b541f28ee72e556a3fed42c47570
> >>> Author: Jaime Caamaño Ruiz 
> >>> Date:   Wed Oct 31 18:52:03 2018 +0100
> >>>
> >>> openvswitch: Fix push/pop ethernet validation
> >>>
> >>> When there are both pop and push ethernet header actions among the
> >>> actions to be applied to a packet, an unexpected EINVAL (Invalid
> >>> argument) error is obtained. This is due to mac_proto not being reset
> >>> correctly when those actions are validated.
> >>>
> >>> Reported-at:
> >>> 
> >>> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
> >>> Fixes: 91820da6ae85 ("openvswitch: add Ethernet push and pop actions")
> >>> Signed-off-by: Jaime Caamaño Ruiz 
> >>>
> >>>Reported-at:
> >>>https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
> >>>Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")
> >>>Signed-off-by: Jaime Caamaño Ruiz 
> >Applied to master, thanks Jaime and Greg!
> 
> Thanks Ben!  It looks like it needs backporting to 2.10 and 2.9 as well.

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

[ovs-dev] Finiquitos y liquidaciones en 50 Casos Prácticos

[Ariel Lara]

¡Cierre de Inscripciones! 

 

Las relaciones laborales pueden y deben ser más prácticas en sus etapas, para 
esto es que existen los departamentos de recursos humanos y otro tipo de 
organismos que se encargan de mediar entre ambas partes cuando por algún motivo 
la relación laboral llega a su fin. 

 

ESTA ES SU OPORTUNIDAD para participar en el WEBINAR INTERACTIVO "Finiquitos y 
liquidaciones en 50 Casos Prácticos" con Innova Learn, en el cual podrás: 

 

- Reglas para el pago de finiquitos y liquidaciones. 

- Determinación de los montos de los finiquitos y liquidaciones. 
- Aspectos fiscales. 

 

Además, te ofrecemos: 

 

Precio especial

 

La cita es el próximo viernes 09 de Noviembre en punto de las 10:00 am por 
nuestra plataforma web. 

 

Para mayor información, marcar a nuestro centro de atención a clientes: 

 

(045) 55 1554 6630 , o responder sobre este correo con la palabra Finiquitos + 
los siguientes datos, NOMBRE, TELÉFONO, EMPRESA. 

 

¡Estamos en contacto!


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

[ovs-dev] [PATCH] documentation: man vswitchd.conf.db(5) updated flow-restore-wait

[Zak Whittington]

Commit 7ed73428a changed the behavior of flow-restore-wait to
also prevent the switch from connecting to controllers in the
controller table, but failed to update the man page documentation
generated by vswitchd/vswitch.xml to reflect this.

This commit adds that documentation.

Signed-off-by: Zak Whittington 
---
 vswitchd/vswitch.xml | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 6d1fc1c..efa1ef8 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -122,6 +122,16 @@
   start receiving packets from the datapath and re-setup the flows.
 
 
+  Additionally, ovs-vswitchd is prevented from connecting
+  to controllers when this value is set to true. This
+  prevents controllers from making changes to the flow table in the
+  middle of flow restoration, which could result in undesirable
+  intermediate states. Once this value has been set to
+  false and the desired flow state has been
+  restored, ovs-vswitchd will be able to reconnect to
+  controllers and process any new flow table modifications.
+
+
   Thus, with this option, the procedure for a hot-upgrade of
   ovs-vswitchd becomes roughly the following:
 
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a cluster may never rejoin.

[Paul Greenberg]

RAFT requires odd number of nodes. With 2 out of 4 down, there never be a 
majority vote.

Best Regards,
Paul Greenberg


From: ovs-dev-boun...@openvswitch.org on behalf of Paul Greenberg 

Sent: Friday, November 2, 2018 5:09 PM
To: Ben Pfaff
Cc: ovs dev
Subject: Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a 
cluster may never rejoin.

Hi Ben,

Let me clarify. Based on my observation, once a server loses touch with the 
rest of the cluster you have to rejoin it. At the same time it is not readily 
apparent that you have to do the clean up (removal) yourself. For example, you 
had a cluster of 3 nodes, then after some time one node is out. You go to that 
node and do a cluster join. My understanding is that a new server id gets 
generated.

Now, if you did not do a cleanup, you end up with 4 node cluster. When 3 out of 
4 are working, there is no issue. It is almost mandatory to do a cleanup after 
join.

Best Regards,
Paul Greenberg


From: Ben Pfaff 
Sent: Friday, November 2, 2018 2:30 PM
To: Paul Greenberg
Cc: Yifeng Sun; ovs dev
Subject: Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a 
cluster may never rejoin.

I'm not sure what you mean. Do you mean that the commands for removing
a server from a cluster aren't documented well enough? Or something
else?

On Thu, Nov 01, 2018 at 07:03:07PM +, Paul Greenberg wrote:
> I would also add that when a server leaves a cluster, it must be removed 
> using “ovs-appctl -t db.sock.ctl cluster/kick ”
>
> Best Regards,
> Paul Greenberg
>
> 
> From: ovs-dev-boun...@openvswitch.org on behalf of Yifeng Sun 
> 
> Sent: Thursday, November 1, 2018 2:25 PM
> To: Ben Pfaff
> Cc: ovs dev
> Subject: Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a 
> cluster may never rejoin.
>
> Looks good to me, thanks.
> Reviewed-by: Yifeng Sun 
>
> On Mon, Oct 29, 2018 at 4:37 PM Ben Pfaff  wrote:
>
> > This wasn't clear from the documentation.
> >
> > Reported-by; Paul Greenberg 
> > Signed-off-by: Ben Pfaff 
> > ---
> > Documentation/ref/ovsdb.7.rst | 3 +++
> > ovsdb/ovsdb-server.1.in | 3 +++
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> > index 39d85b6e5774..f891309fc05d 100644
> > --- a/Documentation/ref/ovsdb.7.rst
> > +++ b/Documentation/ref/ovsdb.7.rst
> > @@ -255,6 +255,9 @@ cluster: transactions not yet replicated to the server
> > will be lost, and
> > transactions not yet applied to the cluster may be committed. Afterward,
> > any
> > servers in its former cluster will regard the server to have failed.
> >
> > +Once a server leaves a cluster, it may never rejoin it. Instead, create
> > a new
> > +server and join it to the cluster.
> > +
> > The servers in a cluster synchronize data over a cluster management
> > protocol
> > that is specific to Open vSwitch; it is not the same as the OVSDB protocol
> > specified in RFC 7047. For this purpose, a server in a cluster is tied
> > to a
> > diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
> > index 2d80dafffb7d..9f78e87f655c 100644
> > --- a/ovsdb/ovsdb-server.1.in
> > +++ b/ovsdb/ovsdb-server.1.in
> > @@ -339,6 +339,9 @@ executed.
> > .IP
> > Use \fBovsdb\-client wait\fR (see \fBovsdb\-client\fR(1)) to wait
> > until the server has left the cluster.
> > +.IP
> > +Once a server leaves a cluster, it may never rejoin it. Instead,
> > +create a new server and join it to the cluster.
> > .
> > .IP "\fBcluster/kick \fIdb server\fR"
> > Start graceful removal of \fIserver\fR from \fIdb\fR's cluster, like
> > --
> > 2.16.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

[Yifeng Sun]

Hi Ben,

Based on the discussion, I am going to create a dns_resolve_timeout__
specifically for
sandboxing and testing. In runtime, dns resolving will check the
environment variable,
say OVS_RESOLV_CONF, to decide whether or not to use the timeout version of
actual
dns resolution. Does this sound good to you?

Thanks,
Yifeng

On Fri, Nov 2, 2018 at 1:21 PM Ben Pfaff  wrote:

> On Fri, Nov 02, 2018 at 04:05:13PM -0400, Mark Michelson wrote:
> > On 11/01/2018 02:15 PM, Yifeng Sun wrote:
> > >Hi Mark,
> > >
> > >Thanks a lot for the information, that is very helpful.
> > >
> > > From your comments, I understand that we should keep the current
> > >DNS resolving behavior as is. The thing we need to improve is to
> > >stop resolving if there is no /etc/resolv.conf configured.
> > >
> > >And if /etc/resolv.conf exists and has "127.0.0.1" as the dns server,
> > >like Numan mentioned, resolver will block. For testing, I feel that a
> > >timeout dns_resolve makes sense. Can we determine testing
> > >context in runtime?
> >
> > I'm not sure about that. After thinking about this a bit more, it may
> > actually make more sense to use a sandboxed resolv.conf during our tests.
> > This way, we have control over what servers are configured and what
> timeouts
> > are. I'm not sure exactly how to achieve this, but it seems like a more
> > ideal option than changing the underlying code for tests.
>
> We use OVS_* environment variables for sandboxing other things, we could
> have an OVS_RESOLV_CONF variable I guess.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] checkpatch: Speed up checking when spell checking not enabled.

[Yifeng Sun]

Looks good to me, thanks.

Reviewed-by: Yifeng Sun 

On Thu, Nov 1, 2018 at 8:07 AM Ben Pfaff  wrote:

> On my machine it takes almost a second for enchant to read its dictionary.
> This time is wasted when spell checking is not enabled.  This commit makes
> checkpatch read the dictionary only when it will be used.
>
> Signed-off-by: Ben Pfaff 
> ---
>  utilities/checkpatch.py | 117
> +---
>  1 file changed, 62 insertions(+), 55 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 54aa4b6346db..8bbda78989c4 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -21,59 +21,6 @@ import os
>  import re
>  import sys
>
> -try:
> -import enchant
> -
> -extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
> 'netdev',
> -  'selinux', 'ovs-ctl', 'dpctl', 'ofctl',
> 'openvswitch',
> -  'dpdk', 'hugepage', 'hugepages', 'pmd', 'upcall',
> -  'vhost', 'rx', 'tx', 'vhostuser', 'openflow',
> 'qsort',
> -  'rxq', 'txq', 'perf', 'stats', 'struct', 'int',
> -  'char', 'bool', 'upcalls', 'nicira', 'bitmask',
> 'ipv4',
> -  'ipv6', 'tcp', 'tcp4', 'tcpv4', 'udp', 'udp4',
> 'udpv4',
> -  'icmp', 'icmp4', 'icmpv6', 'vlan', 'vxlan', 'cksum',
> -  'csum', 'checksum', 'ofproto', 'numa', 'mempool',
> -  'mempools', 'mbuf', 'mbufs', 'hmap', 'cmap', 'smap',
> -  'dhcpv4', 'dhcp', 'dhcpv6', 'opts', 'metadata',
> -  'geneve', 'mutex', 'netdev', 'netdevs', 'subtable',
> -  'virtio', 'qos', 'policer', 'datapath', 'tunctl',
> -  'attr', 'ethernet', 'ether', 'defrag', 'defragment',
> -  'loopback', 'sflow', 'acl', 'initializer', 'recirc',
> -  'xlated', 'unclosed', 'netlink', 'msec', 'usec',
> -  'nsec', 'ms', 'us', 'ns', 'kilobits', 'kbps',
> -  'kilobytes', 'megabytes', 'mbps', 'gigabytes',
> 'gbps',
> -  'megabits', 'gigabits', 'pkts', 'tuple', 'miniflow',
> -  'megaflow', 'conntrack', 'vlans', 'vxlans', 'arg',
> -  'tpid', 'xbundle', 'xbundles', 'mbundle',
> 'mbundles',
> -  'netflow', 'localnet', 'odp', 'pre', 'dst', 'dest',
> -  'src', 'ethertype', 'cvlan', 'ips', 'msg', 'msgs',
> -  'liveness', 'userspace', 'eventmask', 'datapaths',
> -  'slowpath', 'fastpath', 'multicast', 'unicast',
> -  'revalidation', 'namespace', 'qdisc', 'uuid',
> 'ofport',
> -  'subnet', 'revalidation', 'revalidator',
> 'revalidate',
> -  'l2', 'l3', 'l4', 'openssl', 'mtu', 'ifindex',
> 'enum',
> -  'enums', 'http', 'https', 'num', 'vconn', 'vconns',
> -  'conn', 'nat', 'memset', 'memcmp', 'strcmp',
> -  'strcasecmp', 'tc', 'ufid', 'api', 'ofpbuf',
> 'ofpbufs',
> -  'hashmaps', 'hashmap', 'deref', 'dereference', 'hw',
> -  'prio', 'sendmmsg', 'sendmsg', 'malloc', 'free',
> 'alloc',
> -  'pid', 'ppid', 'pgid', 'uid', 'gid', 'sid', 'utime',
> -  'stime', 'cutime', 'cstime', 'vsize', 'rss',
> 'rsslim',
> -  'whcan', 'gtime', 'eip', 'rip', 'cgtime', 'dbg',
> 'gw',
> -  'sbrec', 'bfd', 'sizeof', 'pmds', 'nic', 'nics',
> 'hwol',
> -  'encap', 'decap', 'tlv', 'tlvs', 'decapsulation',
> 'fd',
> -  'cacheline', 'xlate', 'skiplist', 'idl',
> 'comparator',
> -  'natting', 'alg', 'pasv', 'epasv', 'wildcard',
> 'nated',
> -  'amd64', 'x86_64', 'recirculation']
> -
> -spell_check_dict = enchant.Dict("en_US")
> -for kw in extra_keywords:
> -spell_check_dict.add(kw)
> -
> -no_spellcheck = False
> -except:
> -no_spellcheck = True
> -
>  RETURN_CHECK_INITIAL_STATE = 0
>  RETURN_CHECK_STATE_WITH_RETURN = 1
>  RETURN_CHECK_AWAITING_BRACE = 2
> @@ -86,6 +33,66 @@ total_line = 0
>  colors = False
>  spellcheck_comments = False
>  quiet = False
> +spell_check_dict = None
> +
> +
> +def open_spell_check_dict():
> +import enchant
> +
> +try:
> +extra_keywords = ['ovs', 'vswitch', 'vswitchd', 'ovs-vswitchd',
> +  'netdev', 'selinux', 'ovs-ctl', 'dpctl',
> 'ofctl',
> +  'openvswitch', 'dpdk', 'hugepage', 'hugepages',
> +  'pmd', 'upcall', 'vhost', 'rx', 'tx',
> 'vhostuser',
> +  'openflow', 'qsort', 'rxq', 'txq', 'perf',
> 'stats',
> +  'struct', 'int', 'char', 'bool', 'upcalls',
> 'nicira',
> +  

[ovs-dev] OVN/OVS Split amended proposal

[Mark Michelson]

A few weeks ago, I sent a detailed document[1] with a tasklist of what 
would need to be done to separate OVN from OVS. To summarize, the work 
was divided into four phases:


1) Physical separation.
2) Compile time compatibilty.
3) Runtime compatibility.
4) Separation of OVN and OVS packages.

Initially, the idea was to perform all four of these phases by the time 
that OVS 2.11 was released. However, there are a few problems.


In addition to this major project, there is also another major project 
involving rewrites of OVN components to use DDLog. Trying to separate 
the OVN code out could step on the toes of the efforts to convert to 
DDLog. The conversion to DDLog is going to require a lot of testing and 
verification, and the separation of the code will require even more 
testing and verification on top of it. While we like to think we'd do it 
100% correctly, there's a decent chance that we are going to introduce 
regressions.


Red Hat developers had planned to head the development effort to get the 
separation completed. Realistically, we have too many other tasks on our 
plate to guarantee the successful completion of the full separation by 
the 2.11 release.


In addition to the technical aspects of the separation, administrative 
aspects still would need to be implemented. This involves deciding the 
pace of versioning of OVN, creation of web sites and new mailing lists, 
etc. It may be possible for us to complete all this in time for the 2.11 
release, but I believe we'd feel more comfortable if we had a longer 
lead time for this.


With this in mind, we'd like to propose an amended plan.

The short version: rather than completing all four phases by the 2.11 
release, instead we will implement phases 3 and 4 for 2.11, and then 
complete phases 1 and 2 for 2.12.


What this means is that we will change the packaging so OVN is no longer 
a sub-package of OVS. Rather, OVN will be an independent package, 
allowing it to be updated without having to also update OVS.  By 
attacking the problem this way, we scratch an itch somewhat by allowing 
for projects to move forward with newer OVN builds while remaining on 
their current version of OVS. What we won't get is the ability for OVN 
to be developed and versioned at a separate pace.


In order to facilitate this, OVS will need to be outfitted with ways for 
OVN to probe for the existence of features at runtime. The most obvious 
use of this would be ovn-controller querying ovs-vswitchd about the 
existence of certain OpenFlow match fields and actions.


We believe this is a more realistically achievable goal, is less likely 
to conflict with other development projects, and is less likely to 
result in bugs being introduced. Since this plan directly implements 
phases that are necessary for the full separation, it acts as a stepping 
stone rather than a band-aid or stop-gap.


What are your thoughts on this?

Mark Michelson

[1] 
https://mail.openvswitch.org/pipermail/ovs-dev/2018-September/352414.html

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

Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a cluster may never rejoin.

[Paul Greenberg]

Hi Ben,

Let me clarify. Based on my observation, once a server loses touch with the 
rest of the cluster you have to rejoin it. At the same time it is not readily 
apparent that you have to do the clean up (removal) yourself. For example, you 
had a cluster of 3 nodes, then after some time one node is out. You go to that 
node and do a cluster join. My understanding is that a new server id gets 
generated.

Now, if you did not do a cleanup, you end up with 4 node cluster. When 3 out of 
4 are working, there is no issue. It is almost mandatory to do a cleanup after 
join.

Best Regards,
Paul Greenberg


From: Ben Pfaff 
Sent: Friday, November 2, 2018 2:30 PM
To: Paul Greenberg
Cc: Yifeng Sun; ovs dev
Subject: Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a 
cluster may never rejoin.

I'm not sure what you mean. Do you mean that the commands for removing
a server from a cluster aren't documented well enough? Or something
else?

On Thu, Nov 01, 2018 at 07:03:07PM +, Paul Greenberg wrote:
> I would also add that when a server leaves a cluster, it must be removed 
> using “ovs-appctl -t db.sock.ctl cluster/kick ”
>
> Best Regards,
> Paul Greenberg
>
> 
> From: ovs-dev-boun...@openvswitch.org on behalf of Yifeng Sun 
> 
> Sent: Thursday, November 1, 2018 2:25 PM
> To: Ben Pfaff
> Cc: ovs dev
> Subject: Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a 
> cluster may never rejoin.
>
> Looks good to me, thanks.
> Reviewed-by: Yifeng Sun 
>
> On Mon, Oct 29, 2018 at 4:37 PM Ben Pfaff  wrote:
>
> > This wasn't clear from the documentation.
> >
> > Reported-by; Paul Greenberg 
> > Signed-off-by: Ben Pfaff 
> > ---
> > Documentation/ref/ovsdb.7.rst | 3 +++
> > ovsdb/ovsdb-server.1.in | 3 +++
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> > index 39d85b6e5774..f891309fc05d 100644
> > --- a/Documentation/ref/ovsdb.7.rst
> > +++ b/Documentation/ref/ovsdb.7.rst
> > @@ -255,6 +255,9 @@ cluster: transactions not yet replicated to the server
> > will be lost, and
> > transactions not yet applied to the cluster may be committed. Afterward,
> > any
> > servers in its former cluster will regard the server to have failed.
> >
> > +Once a server leaves a cluster, it may never rejoin it. Instead, create
> > a new
> > +server and join it to the cluster.
> > +
> > The servers in a cluster synchronize data over a cluster management
> > protocol
> > that is specific to Open vSwitch; it is not the same as the OVSDB protocol
> > specified in RFC 7047. For this purpose, a server in a cluster is tied
> > to a
> > diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
> > index 2d80dafffb7d..9f78e87f655c 100644
> > --- a/ovsdb/ovsdb-server.1.in
> > +++ b/ovsdb/ovsdb-server.1.in
> > @@ -339,6 +339,9 @@ executed.
> > .IP
> > Use \fBovsdb\-client wait\fR (see \fBovsdb\-client\fR(1)) to wait
> > until the server has left the cluster.
> > +.IP
> > +Once a server leaves a cluster, it may never rejoin it. Instead,
> > +create a new server and join it to the cluster.
> > .
> > .IP "\fBcluster/kick \fIdb server\fR"
> > Start graceful removal of \fIserver\fR from \fIdb\fR's cluster, like
> > --
> > 2.16.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVS DPDK: dpdk_merge pull request for branch-2.6

[Ben Pfaff]

On Fri, Nov 02, 2018 at 05:34:56PM +, Stokes, Ian wrote:
> Hi Ben,
> 
> The following changes since commit ff7067e076bc0219aba7ab180e8442644d0c1054:
> 
>   python-c-ext: Fix memory leak in Parser_finish (2018-10-31 10:35:45 -0700)
> 
> are available in the git repository at:
> 
>   https://github.com/istokes/ovs dpdk_merge_2_6
> 
> for you to fetch changes up to b1e1200021cad749143827c037ae2742db37273c:
> 
>   netdev-dpdk: Fix netdev_dpdk_get_features(). (2018-11-02 15:34:09 +)

Thanks.  I merged the branches for 2.6 to 2.10.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

[Ben Pfaff]

On Fri, Nov 02, 2018 at 04:05:13PM -0400, Mark Michelson wrote:
> On 11/01/2018 02:15 PM, Yifeng Sun wrote:
> >Hi Mark,
> >
> >Thanks a lot for the information, that is very helpful.
> >
> > From your comments, I understand that we should keep the current
> >DNS resolving behavior as is. The thing we need to improve is to
> >stop resolving if there is no /etc/resolv.conf configured.
> >
> >And if /etc/resolv.conf exists and has "127.0.0.1" as the dns server,
> >like Numan mentioned, resolver will block. For testing, I feel that a
> >timeout dns_resolve makes sense. Can we determine testing
> >context in runtime?
> 
> I'm not sure about that. After thinking about this a bit more, it may
> actually make more sense to use a sandboxed resolv.conf during our tests.
> This way, we have control over what servers are configured and what timeouts
> are. I'm not sure exactly how to achieve this, but it seems like a more
> ideal option than changing the underlying code for tests.

We use OVS_* environment variables for sandboxing other things, we could
have an OVS_RESOLV_CONF variable I guess.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] checkpatch: Add explicit test for mailing list as author.

[Yifeng Sun]

Looks good to me, thanks.

Reviewed-by: Yifeng Sun 

On Thu, Nov 1, 2018 at 8:06 AM Ben Pfaff  wrote:

> Somehow some such patches snuck through.  checkpatch caught them (and the
> committer missed that) but this makes it even more explicit.
>
> Signed-off-by: Ben Pfaff 
> ---
>  tests/checkpatch.at | 8 
>  utilities/checkpatch.py | 3 +++
>  2 files changed, 11 insertions(+)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index 8af3a8c0371e..bd7422494e33 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -51,6 +51,14 @@ try_checkpatch \
>  Commit: A" \
> "ERROR: Author A needs to sign off."
>
> +# Single author but somehow the mailing list is the author.
> +try_checkpatch \
> +   "Author: Foo Bar via dev 
> +Commit: A
> +
> +Signed-off-by: A" \
> +   "ERROR: Author should not be mailing list."
> +
>  # Sign-off for single author and different committer.
>  try_checkpatch \
> "Author: A
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 5f5dd8318e32..54aa4b6346db 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -731,6 +731,9 @@ def ovs_checkpatch_parse(text, filename, author=None,
> committer=None):
>  if not author:
>  print_error("Patch lacks author.")
>  continue
> +if " via " in author or "@openvswitch.org" in author:
> +print_error("Author should not be mailing list.")
> +continue
>  if author in co_authors:
>  print_error("Author should not be also be
> co-author.")
>  continue
> --
> 2.16.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

[Mark Michelson]

On 11/01/2018 02:15 PM, Yifeng Sun wrote:

Hi Mark,

Thanks a lot for the information, that is very helpful.

 From your comments, I understand that we should keep the current
DNS resolving behavior as is. The thing we need to improve is to
stop resolving if there is no /etc/resolv.conf configured.

And if /etc/resolv.conf exists and has "127.0.0.1" as the dns server,
like Numan mentioned, resolver will block. For testing, I feel that a
timeout dns_resolve makes sense. Can we determine testing
context in runtime?


I'm not sure about that. After thinking about this a bit more, it may 
actually make more sense to use a sandboxed resolv.conf during our 
tests. This way, we have control over what servers are configured and 
what timeouts are. I'm not sure exactly how to achieve this, but it 
seems like a more ideal option than changing the underlying code for tests.




Best,
Yifeng


On Thu, Nov 1, 2018 at 6:20 AM Mark Michelson > wrote:


On 10/31/2018 06:24 PM, Yifeng Sun wrote:
 > Hi Ben,
 >
 > The dns resolving depends on libunbound's ub_resolve, which, from
 > Numan's experience as well as my reading on its documentation,
 > doesn't support timeout. I agree there is a bug and we should fix it.
 >
 > Thanks,
 > Yifeng
 >

I don't think you're going to find many resolvers that support timeouts
being passed to them directly. Most of the time, the system settings
are
going to be honored. On Linux distributions, this means using the
resolv.conf timeout and attempts values. By default, these values are
set to 5 and 2 respectively. This means that the resolution will wait 5
seconds before it determines it has timed out, and will attempt the
query 2 times before it decides that the query has failed.

Working this way is great when it comes to user-friendliness. System
admins are accustomed to using resolv.conf to control resolver
behavior,
so the DNS library isn't doing anything unexpected.

However, this *sucks* when it comes to trying to test your application.
Those defaults I specified before are not guaranteed to be the same
across different Linux distributions, not to mention other platforms.
Trying to predict what the timeout for your DNS query is going to be is
going to be a pain.

If you want to implement an upper bound on a timeout, your best bet is
to use an asynchronous query and start your own timer. When your timer
expires, then cancel the query. However, I would only recommend doing
this in a test environment. Like I said before, administrators won't
like it if we're messing with their configured DNS timeouts.

I think you're onto the right idea here by modifying the behavior when
there are no servers configured. This way, you're not relying on a
timeout in your test for something that really should fail immediately.

 > On Wed, Oct 31, 2018 at 1:59 PM Ben Pfaff mailto:b...@ovn.org>> wrote:
 >
 >> On Thu, Oct 25, 2018 at 03:27:41PM +0530, nusid...@redhat.com
 wrote:
 >>> From: Numan Siddique mailto:nusid...@redhat.com>>
 >>>
 >>> When 'make check' is called by the mock rpm build (which disables
 >> networking),
 >>> the test "ovn-nbctl: LBs - daemon" fails when it runs the command
 >>> "ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80
,192.168.10.20:80 ".
 >> ovn-nbctl
 >>> extracts the vip by calling the socket util function
 >> 'inet_parse_active()',
 >>> and this function blocks when libunbound function ub_resolve()
is called
 >>> further down. ub_resolve() is a blocking function without
timeout and
 >> all the
 >>> ovs/ovn utilities use this function.
 >>>
 >>> As reported by Timothy Redaelli, the issue can also be
reproduced by
 >> running
 >>> the below commands
 >>>
 >>> $ sudo unshare -mn -- sh -c 'ip addr add dev lo 127.0.0.1 && \
 >>>    mount --bind /dev/null /etc/resolv.conf && runuser $SUDO_USER'
 >>> $ make sandbox SANDBOXFLAGS="--ovn"
 >>> $ ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a \
 >>> 192.168.10.10:80 ,192.168.10.20:80

 >>>
 >>> To address this issue, this patch adds a new function -
 >> inet_parse_ip_addr_and_port()
 >>> which expects IP:[port] address in the 'target_' argument and
disables
 >> resolving
 >>> the host. This new function is now used in ovn-northd,
ovn-nbctl and
 >> ovn-trace.
 >>> It is fine to use this function as load balancer VIP cannot be a
 >> hostname.
 >>>
 >>> Reported-by: Timothy Redaelli mailto:tredae...@redhat.com>>
 >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1641672
 >>> Tested-by: Timothy Redaelli mailto:tredae...@redhat.com>>
 >>> 

Re: [ovs-dev] OVS DPDK: dpdk_merge pull request for master

[Ben Pfaff]

On Fri, Nov 02, 2018 at 05:33:39PM +, Stokes, Ian wrote:
> Hi Ben,
> 
> The following changes since commit af26093ab197c309dc0cfa83c5a2db34706f6021:
> 
>   connmgr: Improve interface for setting controllers. (2018-10-31 16:04:36 
> -0700)
> 
> are available in the git repository at:
> 
>   https://github.com/istokes/ovs dpdk_merge
> 
> for you to fetch changes up to 3aaa62015158581dea8de162555abab353f90e8b:
> 
>   dp-packet: Fix allocated size on DPDK init. (2018-11-02 16:29:14 +)

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

Re: [ovs-dev] OVN/OVS Split amended proposal

[Ben Pfaff]

On Fri, Nov 02, 2018 at 03:13:35PM -0400, Mark Michelson wrote:
> With this in mind, we'd like to propose an amended plan.

[...]

> In order to facilitate this, OVS will need to be outfitted with ways for OVN
> to probe for the existence of features at runtime. The most obvious use of
> this would be ovn-controller querying ovs-vswitchd about the existence of
> certain OpenFlow match fields and actions.
> 
> We believe this is a more realistically achievable goal, is less likely to
> conflict with other development projects, and is less likely to result in
> bugs being introduced. Since this plan directly implements phases that are
> necessary for the full separation, it acts as a stepping stone rather than a
> band-aid or stop-gap.
> 
> What are your thoughts on this?

Seems reasonable to me.

My only real comment is that I don't think new OVS features are needed
to query for match fields and actions.  I think that we can do that with
what's already available.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofp-actions: Let parse_UNROLL_XLATE return error message instead of aborting program

[Ben Pfaff]

On Thu, Nov 01, 2018 at 03:05:31PM -0700, Yifeng Sun wrote:
> Currently, if unroll_xlate is passed to ovs-ofctl as one of actions,
> let say 'ovs-ofctl add-flow br0 in_port=1,actions=unroll_xlate',
> ovs-ofctl will crash. This patch fixes it by returning an error
> message.
> 
> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11184
> Signed-off-by: Yifeng Sun 

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

Re: [ovs-dev] [PATCH] datapath: Fix wrong push/pop ethernet validation

[Gregory Rose]

On 11/2/2018 1:46 PM, Ben Pfaff wrote:

On Fri, Nov 02, 2018 at 08:31:06AM -0700, Gregory Rose wrote:

On 11/2/2018 4:45 AM, Jaime Caamaño Ruiz wrote:

Upstream commit:
 commit 46ebe2834ba5b541f28ee72e556a3fed42c47570
 Author: Jaime Caamaño Ruiz 
 Date:   Wed Oct 31 18:52:03 2018 +0100

 openvswitch: Fix push/pop ethernet validation

 When there are both pop and push ethernet header actions among the
 actions to be applied to a packet, an unexpected EINVAL (Invalid
 argument) error is obtained. This is due to mac_proto not being reset
 correctly when those actions are validated.

 Reported-at:
 https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
 Fixes: 91820da6ae85 ("openvswitch: add Ethernet push and pop actions")
 Signed-off-by: Jaime Caamaño Ruiz 

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")
Signed-off-by: Jaime Caamaño Ruiz 

Applied to master, thanks Jaime and Greg!


Thanks Ben!  It looks like it needs backporting to 2.10 and 2.9 as well.

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

Re: [ovs-dev] [PATCH] ovn-controller: function ofctrl_inject_pkt() with wrong parameter

[Ben Pfaff]

On Fri, Nov 02, 2018 at 08:34:16AM +0800, xurong00037997 wrote:
> Signed-off-by: Xu Rong 

Thanks for finding the bug and fixing it!  I applied this to master and
branch-2.10.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Don't purge ukeys while in a quiescent state.

[Ben Pfaff]

On Fri, Nov 02, 2018 at 11:24:30AM +0300, Ilya Maximets wrote:
> On 01.11.2018 21:29, Ben Pfaff wrote:
> > On Thu, Nov 01, 2018 at 04:58:40PM +0300, Ilya Maximets wrote:
> >> revalidator_purge() iterates and modifies umap->cmap. This should
> >> not happen in quiescent state, because cmap implementation based
> >> on rcu protected variables. Let's move the thread to active state
> >> before purging to avoid possible wrong memory accesses.
> >>
> >> CC: Joe Stringer 
> >> Fixes: 9fce0584a643 ("revalidator: Use 'cmap' for storing ukeys.")
> >> Signed-off-by: Ilya Maximets 
> > 
> > Thank you for catching the problem, and for the fix.  This is quite
> > subtle.
> > 
> > Looking at the code here, and the history, it's not at all clear to me
> > why there needs to be such a wide window of RCU quiescing.  Usually, RCU
> > quiescing is to avoid delaying the RCU freeing thread across an
> > operation that can block for a considerable amount of time.  Across
> > udpif_stop_threads() and udpif_start_threads(), I only see a few
> > operations that can do that, in particular the xpthread_join() calls.
> > dpif_disable_upcall() could also arguably be slow for dpif-netdev since
> > it's capturing a rwlock for writing.
> > 
> > So, I'd be inclined to fix this by narrowing the quiescent window,
> > something like the following.  Do you see flaws in my analysis?
> 
> I agree with you. One thing is that pthread_create() also could
> consume valuable amount of time on some systems. Up to few hundreds
> of milliseconds. This could be a problem in case of big number of
> revalidators. So, I'd like to have following incremental:
> 
> ---
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 57869cb32..e44339338 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -565,6 +565,8 @@ udpif_start_threads(struct udpif *udpif, size_t 
> n_handlers_,
>  size_t n_revalidators_)
>  {
>  if (udpif && n_handlers_ && n_revalidators_) {
> +ovsrcu_quiesce_start();
> +
>  udpif->n_handlers = n_handlers_;
>  udpif->n_revalidators = n_revalidators_;
>  
> @@ -595,6 +597,7 @@ udpif_start_threads(struct udpif *udpif, size_t 
> n_handlers_,
>  revalidator->thread = ovs_thread_create(
>  "revalidator", udpif_revalidator, revalidator);
>  }
> +ovsrcu_quiesce_end();
>  }
>  }
>  
> ---
> 
> What do you think?
> 
> Will you format a patch?

Thanks, I applied all your comments and sent v2:
https://patchwork.ozlabs.org/patch/992499/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] oss-fuzz: Free error string in ofctl_parse_flow

[Ben Pfaff]

No problem.  I fixed that up and applied this to master.

On Thu, Nov 01, 2018 at 04:11:57PM -0700, Yifeng Sun wrote:
> Sorry, there is a typo in the message,
> the leaked free string => the leaked error string
> Ben, do you mind fixing it? Thanks.
> 
> Best,
> Yifeng
> 
> On Thu, Nov 1, 2018 at 11:40 AM Yifeng Sun  wrote:
> 
> > This patch frees the leaked free string to stop oss-fuzz from
> > complaining.
> >
> > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11161
> > Signed-off-by
> > :
> > Yifeng Sun 
> > ---
> >  tests/oss-fuzz/ofctl_parse_target.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tests/oss-fuzz/ofctl_parse_target.c
> > b/tests/oss-fuzz/ofctl_parse_target.c
> > index 13d0899dbbae..8a906400a5cc 100644
> > --- a/tests/oss-fuzz/ofctl_parse_target.c
> > +++ b/tests/oss-fuzz/ofctl_parse_target.c
> > @@ -58,6 +58,7 @@ ofctl_parse_flow(const char *input, int command)
> > command, _protocols);
> >  if (error) {
> >  printf("Error encountered: %s\n", error);
> > +free(error);
> >  } else {
> >  ofctl_parse_flows__(, 1, usable_protocols);
> >  }
> > --
> > 2.7.4
> >
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] oss-fuzz: Use unsigned for left shift in ofctl_parse_flows__

[Ben Pfaff]

On Thu, Nov 01, 2018 at 11:51:21AM -0700, Yifeng Sun wrote:
> Left shift int (1 here) can result in a negative value. This is an undefined
> behavior according to ISO C99 (6.5.7). 
> 
> The error message reported by oss-fuzz is:
> runtime error: left shift of 1 by 31 places cannot be represented in type 
> 'int'
> 
> This patch fixes it by changing signed int to unsigned int.
> 
> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11166
> Signed-off-by: Yifeng Sun 

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

Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a cluster may never rejoin.

[Ben Pfaff]

I'm not sure what you mean.  Do you mean that the commands for removing
a server from a cluster aren't documented well enough?  Or something
else?

On Thu, Nov 01, 2018 at 07:03:07PM +, Paul Greenberg wrote:
> I would also add that when a server leaves a cluster, it must be removed 
> using “ovs-appctl -t db.sock.ctl cluster/kick ”
> 
> Best Regards,
> Paul Greenberg
> 
> 
> From: ovs-dev-boun...@openvswitch.org on behalf of Yifeng Sun 
> 
> Sent: Thursday, November 1, 2018 2:25 PM
> To: Ben Pfaff
> Cc: ovs dev
> Subject: Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a 
> cluster may never rejoin.
> 
> Looks good to me, thanks.
> Reviewed-by: Yifeng Sun 
> 
> On Mon, Oct 29, 2018 at 4:37 PM Ben Pfaff  wrote:
> 
> > This wasn't clear from the documentation.
> >
> > Reported-by; Paul Greenberg 
> > Signed-off-by: Ben Pfaff 
> > ---
> > Documentation/ref/ovsdb.7.rst | 3 +++
> > ovsdb/ovsdb-server.1.in | 3 +++
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> > index 39d85b6e5774..f891309fc05d 100644
> > --- a/Documentation/ref/ovsdb.7.rst
> > +++ b/Documentation/ref/ovsdb.7.rst
> > @@ -255,6 +255,9 @@ cluster: transactions not yet replicated to the server
> > will be lost, and
> > transactions not yet applied to the cluster may be committed. Afterward,
> > any
> > servers in its former cluster will regard the server to have failed.
> >
> > +Once a server leaves a cluster, it may never rejoin it. Instead, create
> > a new
> > +server and join it to the cluster.
> > +
> > The servers in a cluster synchronize data over a cluster management
> > protocol
> > that is specific to Open vSwitch; it is not the same as the OVSDB protocol
> > specified in RFC 7047. For this purpose, a server in a cluster is tied
> > to a
> > diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
> > index 2d80dafffb7d..9f78e87f655c 100644
> > --- a/ovsdb/ovsdb-server.1.in
> > +++ b/ovsdb/ovsdb-server.1.in
> > @@ -339,6 +339,9 @@ executed.
> > .IP
> > Use \fBovsdb\-client wait\fR (see \fBovsdb\-client\fR(1)) to wait
> > until the server has left the cluster.
> > +.IP
> > +Once a server leaves a cluster, it may never rejoin it. Instead,
> > +create a new server and join it to the cluster.
> > .
> > .IP "\fBcluster/kick \fIdb server\fR"
> > Start graceful removal of \fIserver\fR from \fIdb\fR's cluster, like
> > --
> > 2.16.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb: Clarify that a server that leaves a cluster may never rejoin.

[Ben Pfaff]

Thanks, applied.

On Thu, Nov 01, 2018 at 11:25:10AM -0700, Yifeng Sun wrote:
> Looks good to me, thanks.
> Reviewed-by: Yifeng Sun 
> 
> On Mon, Oct 29, 2018 at 4:37 PM Ben Pfaff  wrote:
> 
> > This wasn't clear from the documentation.
> >
> > Reported-by; Paul Greenberg 
> > Signed-off-by: Ben Pfaff 
> > ---
> >  Documentation/ref/ovsdb.7.rst | 3 +++
> >  ovsdb/ovsdb-server.1.in   | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> > index 39d85b6e5774..f891309fc05d 100644
> > --- a/Documentation/ref/ovsdb.7.rst
> > +++ b/Documentation/ref/ovsdb.7.rst
> > @@ -255,6 +255,9 @@ cluster: transactions not yet replicated to the server
> > will be lost, and
> >  transactions not yet applied to the cluster may be committed.  Afterward,
> > any
> >  servers in its former cluster will regard the server to have failed.
> >
> > +Once a server leaves a cluster, it may never rejoin it.  Instead, create
> > a new
> > +server and join it to the cluster.
> > +
> >  The servers in a cluster synchronize data over a cluster management
> > protocol
> >  that is specific to Open vSwitch; it is not the same as the OVSDB protocol
> >  specified in RFC 7047.  For this purpose, a server in a cluster is tied
> > to a
> > diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
> > index 2d80dafffb7d..9f78e87f655c 100644
> > --- a/ovsdb/ovsdb-server.1.in
> > +++ b/ovsdb/ovsdb-server.1.in
> > @@ -339,6 +339,9 @@ executed.
> >  .IP
> >  Use \fBovsdb\-client wait\fR (see \fBovsdb\-client\fR(1)) to wait
> >  until the server has left the cluster.
> > +.IP
> > +Once a server leaves a cluster, it may never rejoin it.  Instead,
> > +create a new server and join it to the cluster.
> >  .
> >  .IP "\fBcluster/kick \fIdb server\fR"
> >  Start graceful removal of \fIserver\fR from \fIdb\fR's cluster, like
> > --
> > 2.16.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] ofproto-dpif-upcall: Don't purge ukeys while in a quiescent state.

[Ben Pfaff]

revalidator_purge() iterates and modifies umap->cmap. This should
not happen in quiescent state, because cmap implementation based
on rcu protected variables. Let's narrow the quiescent period
to avoid possible wrong memory accesses.

CC: Joe Stringer 
Fixes: 9fce0584a643 ("revalidator: Use 'cmap' for storing ukeys.")
Reported-by: Ilya Maximets 
Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif-upcall.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index e36cfa0eecca..dc3082477106 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -504,34 +504,32 @@ udpif_destroy(struct udpif *udpif)
 free(udpif);
 }
 
-/* Stops the handler and revalidator threads, must be enclosed in
- * ovsrcu quiescent state unless when destroying udpif. */
+/* Stops the handler and revalidator threads. */
 static void
 udpif_stop_threads(struct udpif *udpif)
 {
 if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) {
 size_t i;
 
+/* Tell the threads to exit. */
 latch_set(>exit_latch);
 
+/* Wait for the threads to exit.  Quiesce because this can take a long
+ * time.. */
+ovsrcu_quiesce_start();
 for (i = 0; i < udpif->n_handlers; i++) {
-struct handler *handler = >handlers[i];
-
-xpthread_join(handler->thread, NULL);
+xpthread_join(udpif->handlers[i].thread, NULL);
 }
-
 for (i = 0; i < udpif->n_revalidators; i++) {
 xpthread_join(udpif->revalidators[i].thread, NULL);
 }
-
 dpif_disable_upcall(udpif->dpif);
+ovsrcu_quiesce_end();
 
+/* Delete ukeys, and delete all flows from the datapath to prevent
+ * double-counting stats. */
 for (i = 0; i < udpif->n_revalidators; i++) {
-struct revalidator *revalidator = >revalidators[i];
-
-/* Delete ukeys, and delete all flows from the datapath to prevent
- * double-counting stats. */
-revalidator_purge(revalidator);
+revalidator_purge(>revalidators[i]);
 }
 
 latch_poll(>exit_latch);
@@ -549,13 +547,16 @@ udpif_stop_threads(struct udpif *udpif)
 }
 }
 
-/* Starts the handler and revalidator threads, must be enclosed in
- * ovsrcu quiescent state. */
+/* Starts the handler and revalidator threads. */
 static void
 udpif_start_threads(struct udpif *udpif, size_t n_handlers_,
 size_t n_revalidators_)
 {
 if (udpif && n_handlers_ && n_revalidators_) {
+/* Creating a thread can take a significant amount of time on some
+ * systems, even hundred of milliseconds, so quiesce around it. */
+ovsrcu_quiesce_start();
+
 udpif->n_handlers = n_handlers_;
 udpif->n_revalidators = n_revalidators_;
 
@@ -586,6 +587,7 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers_,
 revalidator->thread = ovs_thread_create(
 "revalidator", udpif_revalidator, revalidator);
 }
+ovsrcu_quiesce_end();
 }
 }
 
@@ -623,7 +625,6 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
 ovs_assert(udpif);
 ovs_assert(n_handlers_ && n_revalidators_);
 
-ovsrcu_quiesce_start();
 if (udpif->n_handlers != n_handlers_
 || udpif->n_revalidators != n_revalidators_) {
 udpif_stop_threads(udpif);
@@ -641,7 +642,6 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
 
 udpif_start_threads(udpif, n_handlers_, n_revalidators_);
 }
-ovsrcu_quiesce_end();
 }
 
 /* Waits for all ongoing upcall translations to complete.  This ensures that
@@ -657,10 +657,8 @@ udpif_synchronize(struct udpif *udpif)
 size_t n_handlers_ = udpif->n_handlers;
 size_t n_revalidators_ = udpif->n_revalidators;
 
-ovsrcu_quiesce_start();
 udpif_stop_threads(udpif);
 udpif_start_threads(udpif, n_handlers_, n_revalidators_);
-ovsrcu_quiesce_end();
 }
 
 /* Notifies 'udpif' that something changed which may render previous
@@ -700,13 +698,9 @@ udpif_flush(struct udpif *udpif)
 size_t n_handlers_ = udpif->n_handlers;
 size_t n_revalidators_ = udpif->n_revalidators;
 
-ovsrcu_quiesce_start();
-
 udpif_stop_threads(udpif);
 dpif_flow_flush(udpif->dpif);
 udpif_start_threads(udpif, n_handlers_, n_revalidators_);
-
-ovsrcu_quiesce_end();
 }
 
 /* Removes all flows from all datapaths. */
-- 
2.16.1

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

[ovs-dev] OVS DPDK: dpdk_merge pull request for branch-2.6

[Stokes, Ian]

Hi Ben,

The following changes since commit ff7067e076bc0219aba7ab180e8442644d0c1054:

  python-c-ext: Fix memory leak in Parser_finish (2018-10-31 10:35:45 -0700)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge_2_6

for you to fetch changes up to b1e1200021cad749143827c037ae2742db37273c:

  netdev-dpdk: Fix netdev_dpdk_get_features(). (2018-11-02 15:34:09 +)


Ian Stokes (1):
  netdev-dpdk: Fix netdev_dpdk_get_features().

 lib/netdev-dpdk.c | 64 
+++-
 1 file changed, 39 insertions(+), 25 deletions(-)

Thanks
Ian

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

Re: [ovs-dev] [PATCH v12 2/3] dp-packet: Init specific mbuf fields.

[Ilya Maximets]

On 02.11.2018 19:45, Stokes, Ian wrote:
>> On 02/11/2018 10:35, Ilya Maximets wrote:
>>> On 02.11.2018 12:06, Tiago Lam wrote:
 From: Mark Kavanagh 

 dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
 possible the the resultant mbuf portion of the dp_packet contains
 random data. For some mbuf fields, specifically those related to
 multi-segment mbufs and/or offload features, random values may cause
 unexpected behaviour, should the dp_packet's contents be later copied
 to a DPDK mbuf. It is critical therefore, that these fields should be
 initialized to 0.

 This patch ensures that the following mbuf fields are initialized to
 appropriate values on creation of a new dp_packet:
- ol_flags=0
- nb_segs=1
- tx_offload=0
- packet_type=0
- next=NULL
>>>
>>> I don't understand why we need this, at least without multi-segs.
>>> malloced packets never reaches dpdk library functions. We're always
>>> re-allocating them. The only exception is the call to egress policer,
>>> but we can clear some fields right before it inside dpdk_do_tx_copy().
>>
>> These won't be objective conclusions, it will have to do with your
>> preference vs mine. In my opinion, independent of multi-seg mbufs, it is
>> best to initialize these field members to proper values, instead of
>> carrying on with random / garbage data. Why do it in a specific place and
>> then potentially forget about it in some other place later on?
>>
>> Tiago.
>>
> 
> I think it would still make sense to apply this patch. As Tiago has said, I 
> don’t see why we should handle clearing fields in a separate part for the 
> code for the policer use case if they can be initialized here along with the 
> fields that are already initialized.

In this case I do not understand why only these few fields initialized?
Why not other fields like 'vlan_tci' or 'packet_type' ? They are not used
in OVS and we're passing mbufs with uninitialized fields to the rte_meter
library.
And yes, there are fields that we just can not initialize, like 'pool'.

>>>
>>> We're only using ol_flags, but this field already cleared in current
>> code.
>>>
>>> In general, I'd like to drop these 'mbuf' related references in
>>> generic code at all like it done in my recent patch-set:
>>> https://patchwork.ozlabs.org/patch/990181/
 Best regards, Ilya Maximets.
> 
> I'd like to start applying some of patches in the multiseg series so that we 
> can reduce the overall size of the series and so that the multiseg work (and 
> the follow up TSO work) would have some soakage before the 2.11 release.
> 
> I understand your preference for reworking the mbuf related code but it would 
> push the multiseg work out again, and may have fallout in terms of the mseg 
> series being re-reviewed.
> 
> What I propose here is that the mbuf related rework you suggest be rebased 
> after the multi seg series is applied.

IMHO, multiseg work is not ready still. dp-packet APIs still returns NULL 
pointers
and callers just ignores that fact, we're still passing mbufs with the
number of segments that could be not supported by the driver.
Profit of multisegs without TSO is very arguable, and TSO patch set contains
more questions than answers. At the same time code becomes highly unreadable
and hard to maintain.
This is not that I can call an improvement.

> 
> Thanks
> Ian
> 
>>>

 Adapted from an idea by Michael Qiu :
 https://patchwork.ozlabs.org/patch/777570/

 Co-authored-by: Tiago Lam 

 Signed-off-by: Mark Kavanagh 
 Signed-off-by: Tiago Lam 
 Acked-by: Eelco Chaudron 
 Acked-by: Flavio Leitner 
 ---
  lib/dp-packet.h | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 5b4c9c7..fe34d04
 100644
 --- a/lib/dp-packet.h
 +++ b/lib/dp-packet.h
 @@ -498,14 +498,14 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet
>> *p)
  p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;  }

 -/* This initialization is needed for packets that do not come
 - * from DPDK interfaces, when vswitchd is built with --with-dpdk.
 - * The DPDK rte library will still otherwise manage the mbuf.
 - * We only need to initialize the mbuf ol_flags. */
 +/* This initialization is needed for packets that do not come from
 +DPDK
 + * interfaces, when vswitchd is built with --with-dpdk. */
  static inline void
  dp_packet_mbuf_init(struct dp_packet *p)  {
 -p->mbuf.ol_flags = 0;
 +p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
 +p->mbuf.nb_segs = 1;
 +p->mbuf.next = NULL;
  }

  static inline bool

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

[ovs-dev] OVS DPDK: dpdk_merge pull request for branch-2.7

[Stokes, Ian]

Hi Ben,

The following changes since commit 5272e0c71efe377317de562ce9aa1f025130be38:

  python-c-ext: Fix memory leak in Parser_finish (2018-10-31 10:35:39 -0700)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge_2_7

for you to fetch changes up to 5373a16f3458655327ac94b20a9fa8f84cc18d17:

  netdev-dpdk: Fix netdev_dpdk_get_features(). (2018-11-02 15:32:02 +)


Ben Pfaff (1):
  ovn-northd: Fix memory leak in free_chassis_queueid().

Ian Stokes (1):
  netdev-dpdk: Fix netdev_dpdk_get_features().

 lib/netdev-dpdk.c   | 64 
+++-
 ovn/northd/ovn-northd.c |  1 +
 2 files changed, 40 insertions(+), 25 deletions(-)

Thanks
Ian


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

[ovs-dev] OVS DPDK: dpdk_merge pull request for branch-2.8

[Stokes, Ian]

Hi Ben,

The following changes since commit b9d89442eabbb71267c60469eb69450583de31a3:

  ovn-northd: Fix memory leak in free_chassis_queueid(). (2018-10-31 10:55:19 
-0700)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge_2_8

for you to fetch changes up to 4f9af89ac08b0386187b379a44a82981925d6a89:

  netdev-dpdk: Fix netdev_dpdk_get_features(). (2018-11-02 15:30:01 +)


Ian Stokes (1):
  netdev-dpdk: Fix netdev_dpdk_get_features().

 lib/netdev-dpdk.c | 64 
+++-
 1 file changed, 39 insertions(+), 25 deletions(-)

Thanks
Ian

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

[ovs-dev] OVS DPDK: dpdk_merge pull request for branch-2.9

[Stokes, Ian]

Hi Ben,

The following changes since commit 9d469b1c2d2d473ca733f661e8c9004489bf6c0e:

  ovn: Fix IPv6 DAD failure for container ports (2018-10-31 13:59:24 -0700)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge_2_9

for you to fetch changes up to 09a6e5937920ab406a48fb913db6b299ce9231cd:

  netdev-dpdk: Fix netdev_dpdk_get_features(). (2018-11-02 15:24:21 +)


Ian Stokes (2):
  Docs: Remove zero-copy QEMU limitation.
  netdev-dpdk: Fix netdev_dpdk_get_features().

 Documentation/topics/dpdk/vhost-user.rst |  6 --
 lib/netdev-dpdk.c| 64 
+++-
 2 files changed, 39 insertions(+), 31 deletions(-)

Thanks
Ian

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

[ovs-dev] OVS DPDK: dpdk_merge pull request for master

[Stokes, Ian]

Hi Ben,

The following changes since commit af26093ab197c309dc0cfa83c5a2db34706f6021:

  connmgr: Improve interface for setting controllers. (2018-10-31 16:04:36 
-0700)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge

for you to fetch changes up to 3aaa62015158581dea8de162555abab353f90e8b:

  dp-packet: Fix allocated size on DPDK init. (2018-11-02 16:29:14 +)


Ian Stokes (4):
  Docs: Remove zero-copy QEMU limitation.
  Docs: Remove HWOL DPDK limitation.
  netdev-dpdk: Fix netdev_dpdk_get_features().
  netdev-dpdk: Add link speed to get_status().

Ilya Maximets (6):
  netdev-dpdk: Drop offload API for vhost ports.
  netdev-dpdk: Secure flow offload API.
  dpif-netdev: Fix cmap node use after free on flow disassociation.
  netdev-dpdk: Print port name in offload API messages.
  netdev-dpdk: Dump flow patterns only if debug enabled.
  dpif-netdev: End the quiescent state for flow offloading thread.

Mark Kavanagh (2):
  netdev-dpdk: fix mbuf sizing
  dp-packet: Init specific mbuf fields.

Tiago Lam (1):
  dp-packet: Fix allocated size on DPDK init.

 Documentation/intro/install/dpdk.rst |   1 -
 Documentation/topics/dpdk/memory.rst |  28 +-
 Documentation/topics/dpdk/vhost-user.rst |   6 
 lib/dp-packet.c  |  11 ++-
 lib/dp-packet.h  |  12 
 lib/dpif-netdev.c|   3 +-
 lib/netdev-dpdk.c| 299 
+++-
 lib/netdev-dpdk.h|   4 ---
 8 files changed, 227 insertions(+), 137 deletions(-)

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

[ovs-dev] OVS DPDK: dpdk_merge pull request for branch-2.10

[Stokes, Ian]

Hi Ben,

The following changes since commit 855949977a0b494e556d87e795712db676d2e66c:

  ovn-northd: Fix memory leak in free_chassis_queueid(). (2018-10-31 10:55:05 
-0700)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge_2_10

for you to fetch changes up to ea9e1d8a27da8a0383c1b580ab0b0a47b6a613e8:

  netdev-dpdk: Fix netdev_dpdk_get_features(). (2018-11-02 15:07:06 +)


Ian Stokes (3):
  Docs: Remove zero-copy QEMU limitation.
  Docs: Remove HWOL DPDK limitation.
  netdev-dpdk: Fix netdev_dpdk_get_features().

Ilya Maximets (5):
  netdev-dpdk: Secure flow offload API.
  dpif-netdev: Fix cmap node use after free on flow disassociation.
  netdev-dpdk: Print port name in offload API messages.
  netdev-dpdk: Dump flow patterns only if debug enabled.
  dpif-netdev: End the quiescent state for flow offloading thread.

 Documentation/intro/install/dpdk.rst |   1 -
 Documentation/topics/dpdk/vhost-user.rst |   6 --
 lib/dpif-netdev.c|   3 ++-
 lib/netdev-dpdk.c| 198 
+---
 4 files changed, 125 insertions(+), 83 deletions(-)

Thanks
Ian

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

Re: [ovs-dev] [PATCH v12 2/3] dp-packet: Init specific mbuf fields.

[Lam, Tiago]

On 02/11/2018 10:35, Ilya Maximets wrote:
> On 02.11.2018 12:06, Tiago Lam wrote:
>> From: Mark Kavanagh 
>>
>> dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
>> possible the the resultant mbuf portion of the dp_packet contains
>> random data. For some mbuf fields, specifically those related to
>> multi-segment mbufs and/or offload features, random values may cause
>> unexpected behaviour, should the dp_packet's contents be later copied
>> to a DPDK mbuf. It is critical therefore, that these fields should be
>> initialized to 0.
>>
>> This patch ensures that the following mbuf fields are initialized to
>> appropriate values on creation of a new dp_packet:
>>- ol_flags=0
>>- nb_segs=1
>>- tx_offload=0
>>- packet_type=0
>>- next=NULL
> 
> I don't understand why we need this, at least without multi-segs.
> malloced packets never reaches dpdk library functions. We're always
> re-allocating them. The only exception is the call to egress policer,
> but we can clear some fields right before it inside dpdk_do_tx_copy().

These won't be objective conclusions, it will have to do with your
preference vs mine. In my opinion, independent of multi-seg mbufs, it is
best to initialize these field members to proper values, instead of
carrying on with random / garbage data. Why do it in a specific place
and then potentially forget about it in some other place later on?

Tiago.

> 
> We're only using ol_flags, but this field already cleared in current code.
> 
> In general, I'd like to drop these 'mbuf' related references in generic
> code at all like it done in my recent patch-set:
>   https://patchwork.ozlabs.org/patch/990181/
> > Best regards, Ilya Maximets.
> 
>>
>> Adapted from an idea by Michael Qiu :
>> https://patchwork.ozlabs.org/patch/777570/
>>
>> Co-authored-by: Tiago Lam 
>>
>> Signed-off-by: Mark Kavanagh 
>> Signed-off-by: Tiago Lam 
>> Acked-by: Eelco Chaudron 
>> Acked-by: Flavio Leitner 
>> ---
>>  lib/dp-packet.h | 10 +-
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index 5b4c9c7..fe34d04 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -498,14 +498,14 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet *p)
>>  p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
>>  }
>>  
>> -/* This initialization is needed for packets that do not come
>> - * from DPDK interfaces, when vswitchd is built with --with-dpdk.
>> - * The DPDK rte library will still otherwise manage the mbuf.
>> - * We only need to initialize the mbuf ol_flags. */
>> +/* This initialization is needed for packets that do not come from DPDK
>> + * interfaces, when vswitchd is built with --with-dpdk. */
>>  static inline void
>>  dp_packet_mbuf_init(struct dp_packet *p)
>>  {
>> -p->mbuf.ol_flags = 0;
>> +p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
>> +p->mbuf.nb_segs = 1;
>> +p->mbuf.next = NULL;
>>  }
>>  
>>  static inline bool
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-northd: Always set nat_addresses and options in Port_Binding.

[Ben Pfaff]

In some cases the code didn't set these columns.

Found by inspection.

Signed-off-by: Ben Pfaff 
---
 ovn/northd/ovn-northd.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index e42ceea99264..665b0d820a05 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2170,6 +2170,8 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 
 struct smap ids = SMAP_INITIALIZER();
 sbrec_port_binding_set_external_ids(op->sb, );
+
+sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
 } else {
 if (strcmp(op->nbsp->type, "router")) {
 uint32_t queue_id = smap_get_int(
@@ -2202,6 +2204,8 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 , "Unknown port type '%s' set on logical switch '%s'.",
 op->nbsp->type, op->nbsp->name);
 }
+
+sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
 } else {
 const char *chassis = NULL;
 if (op->peer && op->peer->od && op->peer->od->nbr) {
@@ -2229,6 +2233,8 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 }
 sbrec_port_binding_set_options(op->sb, );
 smap_destroy();
+} else {
+sbrec_port_binding_set_options(op->sb, NULL);
 }
 
 const char *nat_addresses = smap_get(>nbsp->options,
-- 
2.16.1

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

Re: [ovs-dev] [PATCH v12 2/3] dp-packet: Init specific mbuf fields.

[Stokes, Ian]

> On 02/11/2018 10:35, Ilya Maximets wrote:
> > On 02.11.2018 12:06, Tiago Lam wrote:
> >> From: Mark Kavanagh 
> >>
> >> dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
> >> possible the the resultant mbuf portion of the dp_packet contains
> >> random data. For some mbuf fields, specifically those related to
> >> multi-segment mbufs and/or offload features, random values may cause
> >> unexpected behaviour, should the dp_packet's contents be later copied
> >> to a DPDK mbuf. It is critical therefore, that these fields should be
> >> initialized to 0.
> >>
> >> This patch ensures that the following mbuf fields are initialized to
> >> appropriate values on creation of a new dp_packet:
> >>- ol_flags=0
> >>- nb_segs=1
> >>- tx_offload=0
> >>- packet_type=0
> >>- next=NULL
> >
> > I don't understand why we need this, at least without multi-segs.
> > malloced packets never reaches dpdk library functions. We're always
> > re-allocating them. The only exception is the call to egress policer,
> > but we can clear some fields right before it inside dpdk_do_tx_copy().
> 
> These won't be objective conclusions, it will have to do with your
> preference vs mine. In my opinion, independent of multi-seg mbufs, it is
> best to initialize these field members to proper values, instead of
> carrying on with random / garbage data. Why do it in a specific place and
> then potentially forget about it in some other place later on?
> 
> Tiago.
> 

I think it would still make sense to apply this patch. As Tiago has said, I 
don’t see why we should handle clearing fields in a separate part for the code 
for the policer use case if they can be initialized here along with the fields 
that are already initialized.

> >
> > We're only using ol_flags, but this field already cleared in current
> code.
> >
> > In general, I'd like to drop these 'mbuf' related references in
> > generic code at all like it done in my recent patch-set:
> > https://patchwork.ozlabs.org/patch/990181/
> > > Best regards, Ilya Maximets.

I'd like to start applying some of patches in the multiseg series so that we 
can reduce the overall size of the series and so that the multiseg work (and 
the follow up TSO work) would have some soakage before the 2.11 release.

I understand your preference for reworking the mbuf related code but it would 
push the multiseg work out again, and may have fallout in terms of the mseg 
series being re-reviewed.

What I propose here is that the mbuf related rework you suggest be rebased 
after the multi seg series is applied.

Thanks
Ian

> >
> >>
> >> Adapted from an idea by Michael Qiu :
> >> https://patchwork.ozlabs.org/patch/777570/
> >>
> >> Co-authored-by: Tiago Lam 
> >>
> >> Signed-off-by: Mark Kavanagh 
> >> Signed-off-by: Tiago Lam 
> >> Acked-by: Eelco Chaudron 
> >> Acked-by: Flavio Leitner 
> >> ---
> >>  lib/dp-packet.h | 10 +-
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 5b4c9c7..fe34d04
> >> 100644
> >> --- a/lib/dp-packet.h
> >> +++ b/lib/dp-packet.h
> >> @@ -498,14 +498,14 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet
> *p)
> >>  p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;  }
> >>
> >> -/* This initialization is needed for packets that do not come
> >> - * from DPDK interfaces, when vswitchd is built with --with-dpdk.
> >> - * The DPDK rte library will still otherwise manage the mbuf.
> >> - * We only need to initialize the mbuf ol_flags. */
> >> +/* This initialization is needed for packets that do not come from
> >> +DPDK
> >> + * interfaces, when vswitchd is built with --with-dpdk. */
> >>  static inline void
> >>  dp_packet_mbuf_init(struct dp_packet *p)  {
> >> -p->mbuf.ol_flags = 0;
> >> +p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
> >> +p->mbuf.nb_segs = 1;
> >> +p->mbuf.next = NULL;
> >>  }
> >>
> >>  static inline bool
> >>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] openvswitch: fix linking without CONFIG_NF_CONNTRACK_LABELS

[Arnd Bergmann]

When CONFIG_CC_OPTIMIZE_FOR_DEBUGGING is enabled, the compiler
fails to optimize out a dead code path, which leads to a link failure:

net/openvswitch/conntrack.o: In function `ovs_ct_set_labels':
conntrack.c:(.text+0x2e60): undefined reference to `nf_connlabels_replace'

In this configuration, we can take a shortcut, and completely
remove the contrack label code. This may also help the regular
optimization.

Signed-off-by: Arnd Bergmann 
---
 net/openvswitch/conntrack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 6bec37ab4472..a4660c48ff01 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1203,7 +1203,8 @@ static int ovs_ct_commit(struct net *net, struct 
sw_flow_key *key,
 >labels.mask);
if (err)
return err;
-   } else if (labels_nonzero(>labels.mask)) {
+   } else if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
+  labels_nonzero(>labels.mask)) {
err = ovs_ct_set_labels(ct, key, >labels.value,
>labels.mask);
if (err)
-- 
2.18.0

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

Re: [ovs-dev] [PATCH] datapath: Fix wrong push/pop ethernet validation

[Gregory Rose]

On 11/2/2018 4:45 AM, Jaime Caamaño Ruiz wrote:

Upstream commit:
 commit 46ebe2834ba5b541f28ee72e556a3fed42c47570
 Author: Jaime Caamaño Ruiz 
 Date:   Wed Oct 31 18:52:03 2018 +0100

 openvswitch: Fix push/pop ethernet validation

 When there are both pop and push ethernet header actions among the
 actions to be applied to a packet, an unexpected EINVAL (Invalid
 argument) error is obtained. This is due to mac_proto not being reset
 correctly when those actions are validated.

 Reported-at:
 https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
 Fixes: 91820da6ae85 ("openvswitch: add Ethernet push and pop actions")
 Signed-off-by: Jaime Caamaño Ruiz 

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")
Signed-off-by: Jaime Caamaño Ruiz 
---
  datapath/flow_netlink.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index c3f1baa05..ee0c18422 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -2998,7 +2998,7 @@ static int __ovs_nla_copy_actions(struct net *net, const 
struct nlattr *attr,
 * is already present */
if (mac_proto != MAC_PROTO_NONE)
return -EINVAL;
-   mac_proto = MAC_PROTO_NONE;
+   mac_proto = MAC_PROTO_ETHERNET;
break;
  
  		case OVS_ACTION_ATTR_POP_ETH:

@@ -3006,7 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const 
struct nlattr *attr,
return -EINVAL;
if (vlan_tci & htons(VLAN_TAG_PRESENT))
return -EINVAL;
-   mac_proto = MAC_PROTO_ETHERNET;
+   mac_proto = MAC_PROTO_NONE;
break;
  
  		case OVS_ACTION_ATTR_PUSH_NSH:


Thanks for all your work on this Jaime!

Tested-by: Greg Rose 
Reviewed-by: Greg Rose 

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

Re: [ovs-dev] [PATCH v2 1/2] netdev-dpdk: Fix netdev_dpdk_get_features().

[Ilya Maximets]

On 02.11.2018 1:31, Ian Stokes wrote:
> This commit fixes netdev_dpdk_get_features() by initializing a bitmap
> that represents current features to zero and accounting for non defined
> link speed values in the OpenFlow spec.
> 
> The current approach for retrieving netdev dpdk features uses a
> pointer allocated in the stack without being initialized. As such there
> is no guarantee that the bitmap will be accurate. Fix this by declaring
> and initializing local variable 'feature' to be used when building the
> bitmap, with its value then assigned to the pointer. Also account for
> link speeds not defined in the OpenFlow spec by defaulting to
> NETDEV_F_OTHER for undefined link speeds.
> 
> Fixes: 8a9562d21a40 ("dpif-netdev: Add DPDK netdev.")
> Co-authored-by: Flavio Leitner 
> Signed-off-by: Flavio Leitner 
> Signed-off-by: Ian Stokes 
> ---
> 
> v1 -> v2
> * Moved link speed comment to apply to all of link feature statement.
> * Remove reference to enum ofp_port_features in link feature comment.
> * Moved else if to same line as bracket from previous if statement.
> * Added Co-author and Sign-Off for Flavio.
> ---
>  lib/netdev-dpdk.c | 64 
> +--
>  1 file changed, 39 insertions(+), 25 deletions(-)
> 

Minor thing is that you could add a period to the end of a comment
to match with the coding-style.

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

[ovs-dev] [PATCH] datapath: Fix wrong push/pop ethernet validation

[Jaime Caamaño Ruiz]

Upstream commit:
commit 46ebe2834ba5b541f28ee72e556a3fed42c47570
Author: Jaime Caamaño Ruiz 
Date:   Wed Oct 31 18:52:03 2018 +0100

openvswitch: Fix push/pop ethernet validation

When there are both pop and push ethernet header actions among the
actions to be applied to a packet, an unexpected EINVAL (Invalid
argument) error is obtained. This is due to mac_proto not being reset
correctly when those actions are validated.

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
Fixes: 91820da6ae85 ("openvswitch: add Ethernet push and pop actions")
Signed-off-by: Jaime Caamaño Ruiz 

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")
Signed-off-by: Jaime Caamaño Ruiz 
---
 datapath/flow_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index c3f1baa05..ee0c18422 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -2998,7 +2998,7 @@ static int __ovs_nla_copy_actions(struct net *net, const 
struct nlattr *attr,
 * is already present */
if (mac_proto != MAC_PROTO_NONE)
return -EINVAL;
-   mac_proto = MAC_PROTO_NONE;
+   mac_proto = MAC_PROTO_ETHERNET;
break;
 
case OVS_ACTION_ATTR_POP_ETH:
@@ -3006,7 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const 
struct nlattr *attr,
return -EINVAL;
if (vlan_tci & htons(VLAN_TAG_PRESENT))
return -EINVAL;
-   mac_proto = MAC_PROTO_ETHERNET;
+   mac_proto = MAC_PROTO_NONE;
break;
 
case OVS_ACTION_ATTR_PUSH_NSH:
-- 
2.16.4

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

Re: [ovs-dev] [PATCH v4] netdev-dpdk: Add link speed to get_status().

[Stokes, Ian]

> On 02.11.2018 13:45, Ian Stokes wrote:
> > Report the link speed of the device in netdev_dpdk_get_status()
> > function.
> >
> > Link speed is already reported as part of the netdev_get_features()
> > function. However only link speeds defined in the OpenFlow specs are
> > supported so speeds such as 25 Gbps etc. are not shown. The link speed
> > for the device is available in Mbps in rte_eth_link.
> > This commit converts the link speed for a given dpdk device to an easy
> > to read string and reports it in get_status().
> >
> > Suggested-by: Flavio Leitner 
> > Signed-off-by: Ian Stokes 
> > ---
> 
> This version looks OK to me. One possible simplification is not to copy
> the full rte_eth_link structure.
> Like:
> uint32_t speed;
> <...>
> speed = dev->link.link_speed;
> <...>
> smap_add(args, "link_speed", netdev_dpdk_link_speed_to_str__(speed));
> 

Ya that would make sense, we don’t use anything like duplex from the link so 
the uint32 will be quicker again. I can make this change on commit.

> or even:
> 
> const char *speed;
> <...>
> speed = netdev_dpdk_link_speed_to_str__(dev->link.link_speed);
> <...>
> smap_add(args, "link_speed", speed);
> 
> Anyway, any version (current or any of above) looks good to me.
> 
> Acked-by: Ilya Maximets 

Thanks Ilya. Do you have any more comments on patch 1 of the series. I've 
submitted a v2 with the changes you requested. I'd like to get both in today's 
pull request if you get a chance to look at it.

https://mail.openvswitch.org/pipermail/ovs-dev/2018-November/353526.html

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

Re: [ovs-dev] [PATCH v3] netdev-dpdk: Add link speed to get_status().

[Ilya Maximets]

On 02.11.2018 12:54, Ian Stokes wrote:
> Report the link speed of the device in netdev_dpdk_get_status()
> function.
> 
> Link speed is already reported as part of the netdev_get_features()
> function. However only link speeds defined in the OpenFlow specs are
> supported so speeds such as 25 Gbps etc. are not shown. The link
> speed for the device is available in Mbps in rte_eth_link.
> This commit converts the link speed for a given dpdk device to an
> easy to read string and reports it in get_status().
> 
> Suggested-by: Flavio Leitner 
> Signed-off-by: Ian Stokes 
> ---
> v2 -> v3
> * Remove local variable link_speed and instead call
>   netdev_dpdk_link_speed_to_str__ within smap_add_format.
> 
> v1 -> v2
> * Replace snprintf with a return call of for each string.
> ---
>  lib/netdev-dpdk.c | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 59f4ccbfe..47b7a1ccd 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -3021,11 +3021,36 @@ netdev_dpdk_vhost_user_get_status(const struct netdev 
> *netdev,
>  return 0;
>  }
>  
> +/*
> + * Convert a given uint32_t link speed defined in DPDK to a string
> + * equivalent.
> + */
> +static const char *
> +netdev_dpdk_link_speed_to_str__(uint32_t link_speed)
> +{
> +switch (link_speed) {
> +case ETH_SPEED_NUM_10M:return "10Mbps";
> +case ETH_SPEED_NUM_100M:   return "100Mbps";
> +case ETH_SPEED_NUM_1G: return "1Gbps";
> +case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
> +case ETH_SPEED_NUM_5G: return "5Gbps";
> +case ETH_SPEED_NUM_10G:return "10Gbps";
> +case ETH_SPEED_NUM_20G:return "20Gbps";
> +case ETH_SPEED_NUM_25G:return "25Gbps";
> +case ETH_SPEED_NUM_40G:return "40Gbps";
> +case ETH_SPEED_NUM_50G:return "50Gbps";
> +case ETH_SPEED_NUM_56G:return "56Gbps";
> +case ETH_SPEED_NUM_100G:   return "100Gbps";
> +default:   return "Not Defined";
> +}
> +}
> +
>  static int
>  netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args)
>  {
>  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>  struct rte_eth_dev_info dev_info;
> +struct rte_eth_link link;
>  
>  if (!rte_eth_dev_is_valid_port(dev->port_id)) {
>  return ENODEV;
> @@ -3033,6 +3058,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
> struct smap *args)
>  
>  ovs_mutex_lock(>mutex);
>  rte_eth_dev_info_get(dev->port_id, _info);
> +link = dev->link;
>  ovs_mutex_unlock(>mutex);
>  
>  smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
> @@ -3064,6 +3090,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
> struct smap *args)
>  dev_info.pci_dev->id.device_id);
>  }
>  
> +/* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps.
> + * In that case the speed will not be reported as part of the usual
> + * call to get_features(). Get the link speed of the device and add it
> + * to the device status in an easy to read string format.
> + */
> +smap_add_format(args, "link_speed", "%s",
> +netdev_dpdk_link_speed_to_str__(link.link_speed));

But why not a simple smap_add() as I suggested for a v2 ?
There is no reason for additional xvasprintf of the constant string.

> +
>  return 0;
>  }
>  
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v12 2/3] dp-packet: Init specific mbuf fields.

[Ilya Maximets]

On 02.11.2018 12:06, Tiago Lam wrote:
> From: Mark Kavanagh 
> 
> dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
> possible the the resultant mbuf portion of the dp_packet contains
> random data. For some mbuf fields, specifically those related to
> multi-segment mbufs and/or offload features, random values may cause
> unexpected behaviour, should the dp_packet's contents be later copied
> to a DPDK mbuf. It is critical therefore, that these fields should be
> initialized to 0.
> 
> This patch ensures that the following mbuf fields are initialized to
> appropriate values on creation of a new dp_packet:
>- ol_flags=0
>- nb_segs=1
>- tx_offload=0
>- packet_type=0
>- next=NULL

I don't understand why we need this, at least without multi-segs.
malloced packets never reaches dpdk library functions. We're always
re-allocating them. The only exception is the call to egress policer,
but we can clear some fields right before it inside dpdk_do_tx_copy().

We're only using ol_flags, but this field already cleared in current code.

In general, I'd like to drop these 'mbuf' related references in generic
code at all like it done in my recent patch-set:
https://patchwork.ozlabs.org/patch/990181/

Best regards, Ilya Maximets.

> 
> Adapted from an idea by Michael Qiu :
> https://patchwork.ozlabs.org/patch/777570/
> 
> Co-authored-by: Tiago Lam 
> 
> Signed-off-by: Mark Kavanagh 
> Signed-off-by: Tiago Lam 
> Acked-by: Eelco Chaudron 
> Acked-by: Flavio Leitner 
> ---
>  lib/dp-packet.h | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 5b4c9c7..fe34d04 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -498,14 +498,14 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet *p)
>  p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
>  }
>  
> -/* This initialization is needed for packets that do not come
> - * from DPDK interfaces, when vswitchd is built with --with-dpdk.
> - * The DPDK rte library will still otherwise manage the mbuf.
> - * We only need to initialize the mbuf ol_flags. */
> +/* This initialization is needed for packets that do not come from DPDK
> + * interfaces, when vswitchd is built with --with-dpdk. */
>  static inline void
>  dp_packet_mbuf_init(struct dp_packet *p)
>  {
> -p->mbuf.ol_flags = 0;
> +p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
> +p->mbuf.nb_segs = 1;
> +p->mbuf.next = NULL;
>  }
>  
>  static inline bool
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

[nusiddiq]

From: Numan Siddique 

When 'make check' is called by the mock rpm build (which disables networking),
the test "ovn-nbctl: LBs - daemon" fails when it runs the command
"ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80". ovn-nbctl
extracts the vip by calling the socket util function 'inet_parse_active()',
and this function blocks when libunbound function ub_resolve() is called
further down. ub_resolve() is a blocking function without timeout and all the
ovs/ovn utilities use this function.

As reported by Timothy Redaelli, the issue can also be reproduced by running
the below commands

$ sudo unshare -mn -- sh -c 'ip addr add dev lo 127.0.0.1 && \
  mount --bind /dev/null /etc/resolv.conf && runuser $SUDO_USER'
$ make sandbox SANDBOXFLAGS="--ovn"
$ ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a \
  192.168.10.10:80,192.168.10.20:80

To address this issue, this patch adds a new bool argument 'resolve_host' to
the function inet_parse_active() to resolve the host only if it is 'true'.

ovn-nbctl/ovn-northd will pass 'false' when it calls this function to parse
the load balancer values.

Reported-by: Timothy Redaelli 
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1641672
Signed-off-by: Numan Siddique 
---

v3 -> v4
--
   * Instead of adding a new function, added new arguement
 'resolve_dns' to the existing function inet_parse_active() as
 suggested by Ben

v2 -> v3
--
   * Renamed the function inet_parse_active_address_and_port()
 to inet_parse_ip_addr_and_port()

v1 -> v2
---
  * Addressed review comments from Mark
 - Updated the documentation of the inet_parse_active()
 - Used the new function inet_parse_active_address_and_port()
   in ovn-trace

 lib/socket-util.c| 7 ---
 lib/socket-util.h| 2 +-
 lib/stream.c | 2 +-
 ofproto/ofproto-dpif-sflow.c | 2 +-
 ovn/northd/ovn-northd.c  | 2 +-
 ovn/utilities/ovn-nbctl.c| 6 +++---
 ovn/utilities/ovn-trace.c| 2 +-
 ovsdb/raft-private.c | 2 +-
 8 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/lib/socket-util.c b/lib/socket-util.c
index df9b01a9e..8d18e043d 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -518,12 +518,13 @@ exit:
  * is optional and defaults to 'default_port' (use 0 to make the kernel choose
  * an available port, although this isn't usually appropriate for active
  * connections).  If 'default_port' is negative, then  is required.
+ * It resolves the host if 'resolve_host' is true.
  *
  * On success, returns true and stores the parsed remote address into '*ss'.
  * On failure, logs an error, stores zeros into '*ss', and returns false. */
 bool
 inet_parse_active(const char *target_, int default_port,
-  struct sockaddr_storage *ss)
+  struct sockaddr_storage *ss, bool resolve_host)
 {
 char *target = xstrdup(target_);
 char *port, *host;
@@ -538,7 +539,7 @@ inet_parse_active(const char *target_, int default_port,
 ok = false;
 } else {
 ok = parse_sockaddr_components(ss, host, port, default_port,
-   target_, true);
+   target_, resolve_host);
 }
 if (!ok) {
 memset(ss, 0, sizeof *ss);
@@ -575,7 +576,7 @@ inet_open_active(int style, const char *target, int 
default_port,
 int error;
 
 /* Parse. */
-if (!inet_parse_active(target, default_port, )) {
+if (!inet_parse_active(target, default_port, , true)) {
 error = EAFNOSUPPORT;
 goto exit;
 }
diff --git a/lib/socket-util.h b/lib/socket-util.h
index 6d386304d..a65433d90 100644
--- a/lib/socket-util.h
+++ b/lib/socket-util.h
@@ -49,7 +49,7 @@ ovs_be32 guess_netmask(ovs_be32 ip);
 void inet_parse_host_port_tokens(char *s, char **hostp, char **portp);
 void inet_parse_port_host_tokens(char *s, char **portp, char **hostp);
 bool inet_parse_active(const char *target, int default_port,
-   struct sockaddr_storage *ssp);
+   struct sockaddr_storage *ssp, bool resolve_host);
 int inet_open_active(int style, const char *target, int default_port,
  struct sockaddr_storage *ssp, int *fdp, uint8_t dscp);
 
diff --git a/lib/stream.c b/lib/stream.c
index 4e15fe0c8..c4dabda39 100644
--- a/lib/stream.c
+++ b/lib/stream.c
@@ -751,7 +751,7 @@ stream_parse_target_with_default_port(const char *target, 
int default_port,
   struct sockaddr_storage *ss)
 {
 return ((!strncmp(target, "tcp:", 4) || !strncmp(target, "ssl:", 4))
-&& inet_parse_active(target + 4, default_port, ss));
+&& inet_parse_active(target + 4, default_port, ss, true));
 }
 
 /* Attempts to guess the content type of a stream whose first few bytes were
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 62a09b5d1..7da31753c 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ 

Re: [ovs-dev] [PATCH 0/3] Few more fixes/enhancements for rte_flow offloading.

[Stokes, Ian]

> On 02.11.2018 14:01, Stokes, Ian wrote:
> >> Ilya Maximets (3):
> >>   dpif-netdev: Fix cmap node use after free on flow disassociation.
> >>   netdev-dpdk: Print port name in offload API messages.
> >>   netdev-dpdk: Dump flow patterns only if debug enabled.
> >>
> >
> > I take it the intention is to backport this series to 2.10 also Ilya?
> Patches 2 and 3 seem quite useful even though they are not fixes.
> 
> If you wish. I agree that these patches will be useful for 2.10.
> 

OK, will backport these as well.

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

Re: [ovs-dev] [PATCH v4] netdev-dpdk: Add link speed to get_status().

[Ilya Maximets]

On 02.11.2018 13:45, Ian Stokes wrote:
> Report the link speed of the device in netdev_dpdk_get_status()
> function.
> 
> Link speed is already reported as part of the netdev_get_features()
> function. However only link speeds defined in the OpenFlow specs are
> supported so speeds such as 25 Gbps etc. are not shown. The link
> speed for the device is available in Mbps in rte_eth_link.
> This commit converts the link speed for a given dpdk device to an
> easy to read string and reports it in get_status().
> 
> Suggested-by: Flavio Leitner 
> Signed-off-by: Ian Stokes 
> ---

This version looks OK to me. One possible simplification is not
to copy the full rte_eth_link structure.
Like:
uint32_t speed;
<...>
speed = dev->link.link_speed;
<...>
smap_add(args, "link_speed", netdev_dpdk_link_speed_to_str__(speed));

or even:

const char *speed;
<...>
speed = netdev_dpdk_link_speed_to_str__(dev->link.link_speed);
<...>
smap_add(args, "link_speed", speed);

Anyway, any version (current or any of above) looks good to me.

Acked-by: Ilya Maximets 

> v3 -> v4
> * Replace smap_add_format with smap_add.
> 
> v2 -> v3
> * Remove local variable link_speed and instead call
> * Remove local variable link_speed and instead call
>   netdev_dpdk_link_speed_to_str__ within smap_add_format.
> 
> v1 -> v2
> * Replace snprintf with a return call of for each string.
> ---
>  lib/netdev-dpdk.c | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 59f4ccbfe..755f7a895 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -3021,11 +3021,36 @@ netdev_dpdk_vhost_user_get_status(const struct netdev 
> *netdev,
>  return 0;
>  }
>  
> +/*
> + * Convert a given uint32_t link speed defined in DPDK to a string
> + * equivalent.
> + */
> +static const char *
> +netdev_dpdk_link_speed_to_str__(uint32_t link_speed)
> +{
> +switch (link_speed) {
> +case ETH_SPEED_NUM_10M:return "10Mbps";
> +case ETH_SPEED_NUM_100M:   return "100Mbps";
> +case ETH_SPEED_NUM_1G: return "1Gbps";
> +case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
> +case ETH_SPEED_NUM_5G: return "5Gbps";
> +case ETH_SPEED_NUM_10G:return "10Gbps";
> +case ETH_SPEED_NUM_20G:return "20Gbps";
> +case ETH_SPEED_NUM_25G:return "25Gbps";
> +case ETH_SPEED_NUM_40G:return "40Gbps";
> +case ETH_SPEED_NUM_50G:return "50Gbps";
> +case ETH_SPEED_NUM_56G:return "56Gbps";
> +case ETH_SPEED_NUM_100G:   return "100Gbps";
> +default:   return "Not Defined";
> +}
> +}
> +
>  static int
>  netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args)
>  {
>  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>  struct rte_eth_dev_info dev_info;
> +struct rte_eth_link link;
>  
>  if (!rte_eth_dev_is_valid_port(dev->port_id)) {
>  return ENODEV;
> @@ -3033,6 +3058,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
> struct smap *args)
>  
>  ovs_mutex_lock(>mutex);
>  rte_eth_dev_info_get(dev->port_id, _info);
> +link = dev->link;
>  ovs_mutex_unlock(>mutex);
>  
>  smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
> @@ -3064,6 +3090,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
> struct smap *args)
>  dev_info.pci_dev->id.device_id);
>  }
>  
> +/* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps.
> + * In that case the speed will not be reported as part of the usual
> + * call to get_features(). Get the link speed of the device and add it
> + * to the device status in an easy to read string format.
> + */
> +smap_add(args, "link_speed",
> + netdev_dpdk_link_speed_to_str__(link.link_speed));
> +
>  return 0;
>  }
>  
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/3] Few more fixes/enhancements for rte_flow offloading.

[Ilya Maximets]

On 02.11.2018 14:01, Stokes, Ian wrote:
>> Ilya Maximets (3):
>>   dpif-netdev: Fix cmap node use after free on flow disassociation.
>>   netdev-dpdk: Print port name in offload API messages.
>>   netdev-dpdk: Dump flow patterns only if debug enabled.
>>
> 
> I take it the intention is to backport this series to 2.10 also Ilya? Patches 
> 2 and 3 seem quite useful even though they are not fixes.

If you wish. I agree that these patches will be useful for 2.10.

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

Re: [ovs-dev] [PATCH 0/3] Few more fixes/enhancements for rte_flow offloading.

[Stokes, Ian]

> Ilya Maximets (3):
>   dpif-netdev: Fix cmap node use after free on flow disassociation.
>   netdev-dpdk: Print port name in offload API messages.
>   netdev-dpdk: Dump flow patterns only if debug enabled.
> 

I take it the intention is to backport this series to 2.10 also Ilya? Patches 2 
and 3 seem quite useful even though they are not fixes.

Ian

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

Re: [ovs-dev] [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports.

[Stokes, Ian]

> On 31.10.2018 20:01, Ophir Munk wrote:
> > Hi,
> > It is a good idea to remove the macro from the h file.
> > Please find more comments inline
> >
> >> -Original Message-
> >> From: Lam, Tiago [mailto:tiago@intel.com]
> >> Sent: Wednesday, October 31, 2018 2:41 PM
> >> To: Stokes, Ian ; Ilya Maximets
> >> ; ovs-dev@openvswitch.org; Ophir Munk
> >> 
> >> Cc: Finn Christensen ; Yuanhan Liu
> >> ; Shahaf Shuler ;
> >> Chandran, Sugesh 
> >> Subject: Re: [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports.
> >>
> >> On 31/10/2018 11:40, Stokes, Ian wrote:
>  On 31.10.2018 13:42, Stokes, Ian wrote:
> >> vhost ports are not DPDK eth ports and has no rte_flow API.
> >> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time
> >> wasting and errors in log.
> >>
> >> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file,
> >> because there is no need to expose it in header.
> >
> > Adding Ophir and Tiago as they are both looking at the HWOL work
> > at the
>  moment.
> >
> >>
> >> CC: Finn Christensen 
> >> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with
> >> rte
> >> flow")
> >> Signed-off-by: Ilya Maximets 
> >> ---
> >>  lib/netdev-dpdk.c | 10 +++---  lib/netdev-dpdk.h |  4 
> >>  2 files changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >> f91aa27cd..7fe5eb087 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev
> >> *netdev,
> >> const
> >> ovs_u128 *ufid,
> >>  ufid, rte_flow);  }
> >>
> >> +#define DPDK_FLOW_OFFLOAD_API   \
> >> +.flow_put = netdev_dpdk_flow_put,   \
> >> +.flow_del = netdev_dpdk_flow_del
> >> +
> >
> > Regarding the macro name - I suggest using DPDK_FLOW_API.
> > The offload promise is to scale with the HW abilities.
> > If the HW cannot support the flow there would be no offload (just normal
> OVS operation).
> > So better omitting "OFFLOAD" from the macro name.
> 
> IMHO, it's better not to rename. At first, all the corresponding macro
> definitions for other netdevs has OFFLOAD in their names. At second, this
> API is intended for offloading and has no sense without it.
> Most of the users of this API has 'offload' in function and data structure
> names.

+1, let's keep them uniform across the netdevs.

Ian
 
> 
> >
> >>  #define NETDEV_DPDK_CLASS_COMMON\
> >>  .is_pmd = true, \
> >>  .alloc = netdev_dpdk_alloc, \
> >> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev
> >> *netdev,
> >> const
> >> ovs_u128 *ufid,
> >>  .rxq_alloc = netdev_dpdk_rxq_alloc, \
> >>  .rxq_construct = netdev_dpdk_rxq_construct, \
> >>  .rxq_destruct = netdev_dpdk_rxq_destruct,   \
> >> -.rxq_dealloc = netdev_dpdk_rxq_dealloc, \
> >> -DPDK_FLOW_OFFLOAD_API
> >> +.rxq_dealloc = netdev_dpdk_rxq_dealloc
> >>
> >>  #define NETDEV_DPDK_CLASS_BASE  \
> >>  NETDEV_DPDK_CLASS_COMMON,   \
> >> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev
> >> *netdev,
> >> const
> >> ovs_u128 *ufid,
> >>  .get_features = netdev_dpdk_get_features,   \
> >>  .get_status = netdev_dpdk_get_status,   \
> >>  .reconfigure = netdev_dpdk_reconfigure, \
> >> -.rxq_recv = netdev_dpdk_rxq_recv
> >> +.rxq_recv = netdev_dpdk_rxq_recv,   \
> >> +DPDK_FLOW_OFFLOAD_API
> >>
> >
> > Here is the only place where the new macro is used and it only has 2
> callback assignments.
> > What do you say about directly assigning the 2 callbacks and giving up
> the macro?
> > Something like:
> > -.rxq_recv = netdev_dpdk_rxq_recv
> > +.rxq_recv = netdev_dpdk_rxq_recv,   \
> > -DPDK_FLOW_OFFLOAD_API
> > +.flow_put = netdev_dpdk_flow_put,   \
> > +.flow_del = netdev_dpdk_flow_del
> 
> Not sure. Having a macro here groups the offload API together and makes it
> visible for a reader. And, probably, we'll implement more offload APIs in
> the future and there will be more functions.
> 

I'd agree, I believe in the original patch to simplify the netdev classes 
proposed something similar to using callbacks but ultimately it was decided 
that the API would be expanded in the future so should be kept.

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

Re: [ovs-dev] [PATCH] dpif-netdev: End the quiescent state for flow offloading thread.

[Stokes, Ian]

> On 01.11.2018 3:30, Flavio Leitner wrote:
> > On Wed, Oct 31, 2018 at 06:44:09PM +0300, Ilya Maximets wrote:
> >> Flow offloading thread uses concurrent hash maps which are based on
> >> rcu protected variables. It must use them while in active state.
> >> Working in a quiescent state could cause segmentation faults because
> >> of possible cmap internal structure changes.
> >>
> >> Fixes: 02bb2824e51d ("dpif-netdev: do hw flow offload in a thread")
> >> Signed-off-by: Ilya Maximets 
> >> ---
> >>  lib/dpif-netdev.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> >> 5839f2375..8136b0389 100644
> >> --- a/lib/dpif-netdev.c
> >> +++ b/lib/dpif-netdev.c
> >> @@ -2396,6 +2396,7 @@ dp_netdev_flow_offload_main(void *data
> OVS_UNUSED)
> >>  ovsrcu_quiesce_start();
> >>  ovs_mutex_cond_wait(_flow_offload.cond,
> >>  _flow_offload.mutex);
> >> +ovsrcu_quiesce_end();
> >>  }
> >>  list = ovs_list_pop_front(_flow_offload.list);
> >>  offload = CONTAINER_OF(list, struct dp_flow_offload_item,
> >> node);
> >
> > Indeed, this is very similar to system_stats_thread_func().
> > Must have been an interesting journey to find the root cause if not by
> > code inspection.
> 
> Yeah. Bugs like this are really tricky. We had one with quiescent state
> while iterating and deleting pmd threads a few years ago. Was fixed by
> Daniele.
> Fortunately, current one was found by inspection.
> 

Good catch, I'll add this to the pull request today and backport to 2.10.

Thanks
Ian
> >
> > Acked-by: Flavio Leitner 
> >
> > fbl
> >
> >
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] netdev-dpdk: Add link speed to get_status().

[Stokes, Ian]

> On 02.11.2018 12:54, Ian Stokes wrote:



> > +/* Not all link speeds are defined in the OpenFlow specs e.g. 25
> Gbps.
> > + * In that case the speed will not be reported as part of the usual
> > + * call to get_features(). Get the link speed of the device and add
> it
> > + * to the device status in an easy to read string format.
> > + */
> > +smap_add_format(args, "link_speed", "%s",
> > +
> > + netdev_dpdk_link_speed_to_str__(link.link_speed));
> 
> But why not a simple smap_add() as I suggested for a v2 ?
> There is no reason for additional xvasprintf of the constant string.
> 

Apologies Ilya, I missed the removal of the smap_add_format in the first pass,

I've sent a new revision with the correct change

https://mail.openvswitch.org/pipermail/ovs-dev/2018-November/353548.html

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

[ovs-dev] [PATCH v4] netdev-dpdk: Add link speed to get_status().

[Ian Stokes]

Report the link speed of the device in netdev_dpdk_get_status()
function.

Link speed is already reported as part of the netdev_get_features()
function. However only link speeds defined in the OpenFlow specs are
supported so speeds such as 25 Gbps etc. are not shown. The link
speed for the device is available in Mbps in rte_eth_link.
This commit converts the link speed for a given dpdk device to an
easy to read string and reports it in get_status().

Suggested-by: Flavio Leitner 
Signed-off-by: Ian Stokes 
---
v3 -> v4
* Replace smap_add_format with smap_add.

v2 -> v3
* Remove local variable link_speed and instead call
* Remove local variable link_speed and instead call
  netdev_dpdk_link_speed_to_str__ within smap_add_format.

v1 -> v2
* Replace snprintf with a return call of for each string.
---
 lib/netdev-dpdk.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 59f4ccbfe..755f7a895 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -3021,11 +3021,36 @@ netdev_dpdk_vhost_user_get_status(const struct netdev 
*netdev,
 return 0;
 }
 
+/*
+ * Convert a given uint32_t link speed defined in DPDK to a string
+ * equivalent.
+ */
+static const char *
+netdev_dpdk_link_speed_to_str__(uint32_t link_speed)
+{
+switch (link_speed) {
+case ETH_SPEED_NUM_10M:return "10Mbps";
+case ETH_SPEED_NUM_100M:   return "100Mbps";
+case ETH_SPEED_NUM_1G: return "1Gbps";
+case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
+case ETH_SPEED_NUM_5G: return "5Gbps";
+case ETH_SPEED_NUM_10G:return "10Gbps";
+case ETH_SPEED_NUM_20G:return "20Gbps";
+case ETH_SPEED_NUM_25G:return "25Gbps";
+case ETH_SPEED_NUM_40G:return "40Gbps";
+case ETH_SPEED_NUM_50G:return "50Gbps";
+case ETH_SPEED_NUM_56G:return "56Gbps";
+case ETH_SPEED_NUM_100G:   return "100Gbps";
+default:   return "Not Defined";
+}
+}
+
 static int
 netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 struct rte_eth_dev_info dev_info;
+struct rte_eth_link link;
 
 if (!rte_eth_dev_is_valid_port(dev->port_id)) {
 return ENODEV;
@@ -3033,6 +3058,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 
 ovs_mutex_lock(>mutex);
 rte_eth_dev_info_get(dev->port_id, _info);
+link = dev->link;
 ovs_mutex_unlock(>mutex);
 
 smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
@@ -3064,6 +3090,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 dev_info.pci_dev->id.device_id);
 }
 
+/* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps.
+ * In that case the speed will not be reported as part of the usual
+ * call to get_features(). Get the link speed of the device and add it
+ * to the device status in an easy to read string format.
+ */
+smap_add(args, "link_speed",
+ netdev_dpdk_link_speed_to_str__(link.link_speed));
+
 return 0;
 }
 
-- 
2.13.6

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

Re: [ovs-dev] [PATCH v2 2/2] netdev-dpdk: Add link speed to get_status().

[Ilya Maximets]

On 02.11.2018 12:23, Stokes, Ian wrote:
>> On 02.11.2018 1:31, Ian Stokes wrote:
>>> Report the link speed of the device in netdev_dpdk_get_status()
>>> function.
>>>
>>> Link speed is already reported as part of the netdev_get_features()
>>> function. However only link speeds defined in the OpenFlow specs are
>>> supported so speeds such as 25 Gbps etc. are not shown. The link speed
>>> for the device is available in Mbps in rte_eth_link.
>>> This commit converts the link speed for a given dpdk device to an easy
>>> to read string and reports it in get_status().
>>>
>>> Suggested-by: Flavio Leitner 
>>> Signed-off-by: Ian Stokes 
>>> ---
>>> v1 -> v2
>>> * Replace snprintf with a return call of for each string.
>>> ---
>>>  lib/netdev-dpdk.c | 35 +++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>> 59f4ccbfe..5e6a72bc9 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -3021,11 +3021,37 @@ netdev_dpdk_vhost_user_get_status(const struct
>> netdev *netdev,
>>>  return 0;
>>>  }
>>>
>>> +/*
>>> + * Convert a given uint32_t link speed defined in DPDK to a string
>>> + * equivalent.
>>> + */
>>> +static const char *
>>> +netdev_dpdk_link_speed_to_str__(uint32_t link_speed) {
>>> +switch (link_speed) {
>>> +case ETH_SPEED_NUM_10M:return "10Mbps";
>>> +case ETH_SPEED_NUM_100M:   return "100Mbps";
>>> +case ETH_SPEED_NUM_1G: return "1Gbps";
>>> +case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
>>> +case ETH_SPEED_NUM_5G: return "5Gbps";
>>> +case ETH_SPEED_NUM_10G:return "10Gbps";
>>> +case ETH_SPEED_NUM_20G:return "20Gbps";
>>> +case ETH_SPEED_NUM_25G:return "25Gbps";
>>> +case ETH_SPEED_NUM_40G:return "40Gbps";
>>> +case ETH_SPEED_NUM_50G:return "50Gbps";
>>> +case ETH_SPEED_NUM_56G:return "56Gbps";
>>> +case ETH_SPEED_NUM_100G:   return "100Gbps";
>>> +default:   return "Not Defined";
>>> +}
>>> +}
>>> +
>>>  static int
>>>  netdev_dpdk_get_status(const struct netdev *netdev, struct smap
>>> *args)  {
>>>  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>  struct rte_eth_dev_info dev_info;
>>> +struct rte_eth_link link;
>>> +const char *link_speed_str;
>>>
>>>  if (!rte_eth_dev_is_valid_port(dev->port_id)) {
>>>  return ENODEV;
>>> @@ -3033,6 +3059,7 @@ netdev_dpdk_get_status(const struct netdev
>>> *netdev, struct smap *args)
>>>
>>>  ovs_mutex_lock(>mutex);
>>>  rte_eth_dev_info_get(dev->port_id, _info);
>>> +link = dev->link;
>>>  ovs_mutex_unlock(>mutex);
>>>
>>>  smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
>>> @@ -3064,6 +3091,14 @@ netdev_dpdk_get_status(const struct netdev
>> *netdev, struct smap *args)
>>>  dev_info.pci_dev->id.device_id);
>>>  }
>>>
>>> +/* Not all link speeds are defined in the OpenFlow specs e.g. 25
>> Gbps.
>>> + * In that case the speed will not be reported as part of the usual
>>> + * call to get_features(). Get the link speed of the device and add
>> it
>>> + * to the device status in an easy to read string format.
>>> + */
>>> +link_speed_str = netdev_dpdk_link_speed_to_str__(link.link_speed);
>>> +smap_add_format(args, "link_speed", "%s", link_speed_str);
>>
>> How about this:
>>
>> smap_add(args, "link_speed",
>>  netdev_dpdk_link_speed_to_str__(link.link_speed));
>>
>> ?
> 
> Yes, I'm happy to use above.
> 
> On a side note, I was going to use this approach but wasn't sure as regards 
> the preference of adding a parameter to a function call via a direct return 
> from a another function.

IMHO, it's OK for the constant string.

> 
> I've come across examples in other reviews wherein this approach is not 
> preferred, in particular when dealing with if statements. Maybe it's a coding 
> style preference, there doesn’t seem to be any definition within the OVS 
> style guide.
> 
> Ian
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3] netdev-dpdk: Add link speed to get_status().

[Ian Stokes]

Report the link speed of the device in netdev_dpdk_get_status()
function.

Link speed is already reported as part of the netdev_get_features()
function. However only link speeds defined in the OpenFlow specs are
supported so speeds such as 25 Gbps etc. are not shown. The link
speed for the device is available in Mbps in rte_eth_link.
This commit converts the link speed for a given dpdk device to an
easy to read string and reports it in get_status().

Suggested-by: Flavio Leitner 
Signed-off-by: Ian Stokes 
---
v2 -> v3
* Remove local variable link_speed and instead call
  netdev_dpdk_link_speed_to_str__ within smap_add_format.

v1 -> v2
* Replace snprintf with a return call of for each string.
---
 lib/netdev-dpdk.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 59f4ccbfe..47b7a1ccd 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -3021,11 +3021,36 @@ netdev_dpdk_vhost_user_get_status(const struct netdev 
*netdev,
 return 0;
 }
 
+/*
+ * Convert a given uint32_t link speed defined in DPDK to a string
+ * equivalent.
+ */
+static const char *
+netdev_dpdk_link_speed_to_str__(uint32_t link_speed)
+{
+switch (link_speed) {
+case ETH_SPEED_NUM_10M:return "10Mbps";
+case ETH_SPEED_NUM_100M:   return "100Mbps";
+case ETH_SPEED_NUM_1G: return "1Gbps";
+case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
+case ETH_SPEED_NUM_5G: return "5Gbps";
+case ETH_SPEED_NUM_10G:return "10Gbps";
+case ETH_SPEED_NUM_20G:return "20Gbps";
+case ETH_SPEED_NUM_25G:return "25Gbps";
+case ETH_SPEED_NUM_40G:return "40Gbps";
+case ETH_SPEED_NUM_50G:return "50Gbps";
+case ETH_SPEED_NUM_56G:return "56Gbps";
+case ETH_SPEED_NUM_100G:   return "100Gbps";
+default:   return "Not Defined";
+}
+}
+
 static int
 netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 struct rte_eth_dev_info dev_info;
+struct rte_eth_link link;
 
 if (!rte_eth_dev_is_valid_port(dev->port_id)) {
 return ENODEV;
@@ -3033,6 +3058,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 
 ovs_mutex_lock(>mutex);
 rte_eth_dev_info_get(dev->port_id, _info);
+link = dev->link;
 ovs_mutex_unlock(>mutex);
 
 smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
@@ -3064,6 +3090,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 dev_info.pci_dev->id.device_id);
 }
 
+/* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps.
+ * In that case the speed will not be reported as part of the usual
+ * call to get_features(). Get the link speed of the device and add it
+ * to the device status in an easy to read string format.
+ */
+smap_add_format(args, "link_speed", "%s",
+netdev_dpdk_link_speed_to_str__(link.link_speed));
+
 return 0;
 }
 
-- 
2.13.6

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

Re: [ovs-dev] [PATCH v2 2/2] netdev-dpdk: Add link speed to get_status().

[Stokes, Ian]

> On 02.11.2018 1:31, Ian Stokes wrote:
> > Report the link speed of the device in netdev_dpdk_get_status()
> > function.
> >
> > Link speed is already reported as part of the netdev_get_features()
> > function. However only link speeds defined in the OpenFlow specs are
> > supported so speeds such as 25 Gbps etc. are not shown. The link speed
> > for the device is available in Mbps in rte_eth_link.
> > This commit converts the link speed for a given dpdk device to an easy
> > to read string and reports it in get_status().
> >
> > Suggested-by: Flavio Leitner 
> > Signed-off-by: Ian Stokes 
> > ---
> > v1 -> v2
> > * Replace snprintf with a return call of for each string.
> > ---
> >  lib/netdev-dpdk.c | 35 +++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 59f4ccbfe..5e6a72bc9 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -3021,11 +3021,37 @@ netdev_dpdk_vhost_user_get_status(const struct
> netdev *netdev,
> >  return 0;
> >  }
> >
> > +/*
> > + * Convert a given uint32_t link speed defined in DPDK to a string
> > + * equivalent.
> > + */
> > +static const char *
> > +netdev_dpdk_link_speed_to_str__(uint32_t link_speed) {
> > +switch (link_speed) {
> > +case ETH_SPEED_NUM_10M:return "10Mbps";
> > +case ETH_SPEED_NUM_100M:   return "100Mbps";
> > +case ETH_SPEED_NUM_1G: return "1Gbps";
> > +case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
> > +case ETH_SPEED_NUM_5G: return "5Gbps";
> > +case ETH_SPEED_NUM_10G:return "10Gbps";
> > +case ETH_SPEED_NUM_20G:return "20Gbps";
> > +case ETH_SPEED_NUM_25G:return "25Gbps";
> > +case ETH_SPEED_NUM_40G:return "40Gbps";
> > +case ETH_SPEED_NUM_50G:return "50Gbps";
> > +case ETH_SPEED_NUM_56G:return "56Gbps";
> > +case ETH_SPEED_NUM_100G:   return "100Gbps";
> > +default:   return "Not Defined";
> > +}
> > +}
> > +
> >  static int
> >  netdev_dpdk_get_status(const struct netdev *netdev, struct smap
> > *args)  {
> >  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >  struct rte_eth_dev_info dev_info;
> > +struct rte_eth_link link;
> > +const char *link_speed_str;
> >
> >  if (!rte_eth_dev_is_valid_port(dev->port_id)) {
> >  return ENODEV;
> > @@ -3033,6 +3059,7 @@ netdev_dpdk_get_status(const struct netdev
> > *netdev, struct smap *args)
> >
> >  ovs_mutex_lock(>mutex);
> >  rte_eth_dev_info_get(dev->port_id, _info);
> > +link = dev->link;
> >  ovs_mutex_unlock(>mutex);
> >
> >  smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
> > @@ -3064,6 +3091,14 @@ netdev_dpdk_get_status(const struct netdev
> *netdev, struct smap *args)
> >  dev_info.pci_dev->id.device_id);
> >  }
> >
> > +/* Not all link speeds are defined in the OpenFlow specs e.g. 25
> Gbps.
> > + * In that case the speed will not be reported as part of the usual
> > + * call to get_features(). Get the link speed of the device and add
> it
> > + * to the device status in an easy to read string format.
> > + */
> > +link_speed_str = netdev_dpdk_link_speed_to_str__(link.link_speed);
> > +smap_add_format(args, "link_speed", "%s", link_speed_str);
> 
> How about this:
> 
> smap_add(args, "link_speed",
>  netdev_dpdk_link_speed_to_str__(link.link_speed));
> 
> ?

Yes, I'm happy to use above.

On a side note, I was going to use this approach but wasn't sure as regards the 
preference of adding a parameter to a function call via a direct return from a 
another function.

I've come across examples in other reviews wherein this approach is not 
preferred, in particular when dealing with if statements. Maybe it's a coding 
style preference, there doesn’t seem to be any definition within the OVS style 
guide.

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

Re: [ovs-dev] [PATCH v11 02/14] dp-packet: Init specific mbuf fields.

[Lam, Tiago]

On 01/11/2018 19:30, Stokes, Ian wrote:
>> Hi Tiago, Ian,
>>
>> On Tue, Oct 16, 2018 at 02:28:56PM +, Stokes, Ian wrote:
 From: Mark Kavanagh 

 dp_packets are created using xmalloc(); in the case of OvS-DPDK,
 it's possible the the resultant mbuf portion of the dp_packet
 contains random data. For some mbuf fields, specifically those
 related to multi-segment mbufs and/or offload features, random
 values may cause unexpected behaviour, should the dp_packet's contents
>> be later copied to a DPDK mbuf.
 It is critical therefore, that these fields should be initialized to
>> 0.

 This patch ensures that the following mbuf fields are initialized to
 appropriate values on creation of a new dp_packet:
- ol_flags=0
- nb_segs=1
- tx_offload=0
- packet_type=0
- next=NULL

 Adapted from an idea by Michael Qiu :
 https://patchwork.ozlabs.org/patch/777570/

 Co-authored-by: Tiago Lam 

 Signed-off-by: Mark Kavanagh 
 Signed-off-by: Tiago Lam 
 Acked-by: Eelco Chaudron 
 Acked-by: Flavio Leitner 
>>>
>>> Thanks Tiago,
>>>
>>> This will need a rebase. One minor comment below.
 ---
  lib/dp-packet.h | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

 diff --git a/lib/dp-packet.h b/lib/dp-packet.h index
 ba91e58..b948fe1
 100644
 --- a/lib/dp-packet.h
 +++ b/lib/dp-packet.h
 @@ -625,14 +625,15 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet
 *p
 OVS_UNUSED)  }

  /* This initialization is needed for packets that do not come
 - * from DPDK interfaces, when vswitchd is built with --with-dpdk.
 - * The DPDK rte library will still otherwise manage the mbuf.
 - * We only need to initialize the mbuf ol_flags. */
 + * from DPDK interfaces, when vswitchd is built with --with-dpdk.
 + */
  static inline void
  dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)  {  #ifdef
 DPDK_NETDEV
 -p->mbuf.ol_flags = 0;
 +struct rte_mbuf *mbuf = &(p->mbuf);
 +mbuf->ol_flags = mbuf->tx_offload = mbuf->packet_type = 0;
 +mbuf->nb_segs = 1;
>>>
>>> Is it just for ease of access that you cast a pointer to mbuf?
>>>
>>> Just looking at the other rte specific functions implemented in the
>>> file, they access mbuf via p->mbuf rather than the cast. I would like
>>> to keep implementation uniform across functions if possible.
>>
>> This seems to be needed but not related to the multi seg, so I wonder if
>> this patch could be posted as a standalone fix.  The goal is to apply it
>> sooner and reduce the patchset size.
>>
>> What do you think?
>>
> 
> +1, I think patches 1-3 could be applied independent of the series itself.
> 
> There's been a few comments on the latest revision, Tiago if you get a chance 
> to send another revision of these separate to the series we could look to 
> merge as part of the pull request this week.
> 

Sounds good. Just sent a new series with the first three patches only -
it addresses the latest comments in v11 of the multi-segment mbuf
series. I've also kept the ACK's, let me know if that's not OK.

Thanks,
Tiago.

> Thanks
> Ian
>> Thanks,
>> fbl
>>
>>
>>>
>>> Thanks
>>> Ian
>>>
 +mbuf->next = NULL;
  #endif
  }

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

[ovs-dev] [PATCH v12 3/3] dp-packet: Fix allocated size on DPDK init.

[Tiago Lam]

When enabled with DPDK OvS deals with two types of packets, the ones
coming from the mempool and the ones locally created by OvS - which are
copied to mempool mbufs before output. In the latter, the space is
allocated from the system, while in the former the mbufs are allocated
from a mempool, which takes care of initialising them appropriately.

In the current implementation, during mempool's initialisation of mbufs,
dp_packet_set_allocated() is called from dp_packet_init_dpdk() without
considering that the allocated space, in the case of multi-segment
mbufs, might be greater than a single mbuf.  Furthermore, given that
dp_packet_init_dpdk() is on the code path that's called upon mempool's
initialisation, a call to dp_packet_set_allocated() is redundant, since
mempool takes care of initialising it.

To fix this, dp_packet_set_allocated() is no longer called after
initialisation of a mempool, only in dp_packet_init__(), which is still
called by OvS when initialising locally created packets.

Signed-off-by: Tiago Lam 
Acked-by: Eelco Chaudron 
Acked-by: Flavio Leitner 
---
 lib/dp-packet.c   | 11 +++
 lib/dp-packet.h   |  2 +-
 lib/netdev-dpdk.c |  2 +-
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 443c225..93b0e9c 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -92,16 +92,11 @@ dp_packet_use_const(struct dp_packet *b, const void *data, 
size_t size)
 dp_packet_set_size(b, size);
 }
 
-/* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes.
- * DPDK allocated dp_packet and *data is allocated from one continous memory
- * region as part of memory pool, so in memory data start right after
- * dp_packet.  Therefore, there is a special method to free this type of
- * buffer.  Here, non-transient ovs dp-packet fields are initialized for
- * packets that are part of a DPDK memory pool. */
+/* Initializes 'b' as a DPDK dp-packet, which must have been allocated from a
+ * DPDK memory pool. */
 void
-dp_packet_init_dpdk(struct dp_packet *b, size_t allocated)
+dp_packet_init_dpdk(struct dp_packet *b)
 {
-dp_packet_set_allocated(b, allocated);
 b->source = DPBUF_DPDK;
 }
 
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index fe34d04..7b85dd9 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -114,7 +114,7 @@ void dp_packet_use(struct dp_packet *, void *, size_t);
 void dp_packet_use_stub(struct dp_packet *, void *, size_t);
 void dp_packet_use_const(struct dp_packet *, const void *, size_t);
 
-void dp_packet_init_dpdk(struct dp_packet *, size_t allocated);
+void dp_packet_init_dpdk(struct dp_packet *);
 
 void dp_packet_init(struct dp_packet *, size_t);
 void dp_packet_uninit(struct dp_packet *);
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index a72438a..ff88331 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -550,7 +550,7 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp OVS_UNUSED,
 {
 struct rte_mbuf *pkt = _p;
 
-dp_packet_init_dpdk((struct dp_packet *) pkt, pkt->buf_len);
+dp_packet_init_dpdk((struct dp_packet *) pkt);
 }
 
 static int
-- 
2.7.4

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

[ovs-dev] [PATCH v12 2/3] dp-packet: Init specific mbuf fields.

[Tiago Lam]

From: Mark Kavanagh 

dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
possible the the resultant mbuf portion of the dp_packet contains
random data. For some mbuf fields, specifically those related to
multi-segment mbufs and/or offload features, random values may cause
unexpected behaviour, should the dp_packet's contents be later copied
to a DPDK mbuf. It is critical therefore, that these fields should be
initialized to 0.

This patch ensures that the following mbuf fields are initialized to
appropriate values on creation of a new dp_packet:
   - ol_flags=0
   - nb_segs=1
   - tx_offload=0
   - packet_type=0
   - next=NULL

Adapted from an idea by Michael Qiu :
https://patchwork.ozlabs.org/patch/777570/

Co-authored-by: Tiago Lam 

Signed-off-by: Mark Kavanagh 
Signed-off-by: Tiago Lam 
Acked-by: Eelco Chaudron 
Acked-by: Flavio Leitner 
---
 lib/dp-packet.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 5b4c9c7..fe34d04 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -498,14 +498,14 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet *p)
 p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
 }
 
-/* This initialization is needed for packets that do not come
- * from DPDK interfaces, when vswitchd is built with --with-dpdk.
- * The DPDK rte library will still otherwise manage the mbuf.
- * We only need to initialize the mbuf ol_flags. */
+/* This initialization is needed for packets that do not come from DPDK
+ * interfaces, when vswitchd is built with --with-dpdk. */
 static inline void
 dp_packet_mbuf_init(struct dp_packet *p)
 {
-p->mbuf.ol_flags = 0;
+p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
+p->mbuf.nb_segs = 1;
+p->mbuf.next = NULL;
 }
 
 static inline bool
-- 
2.7.4

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

[ovs-dev] [PATCH v12 1/3] netdev-dpdk: fix mbuf sizing

[Tiago Lam]

From: Mark Kavanagh 

There are numerous factors that must be considered when calculating
the size of an mbuf:
- the data portion of the mbuf must be sized in accordance With Rx
  buffer alignment (typically 1024B). So, for example, in order to
  successfully receive and capture a 1500B packet, mbufs with a
  data portion of size 2048B must be used.
- in OvS, the elements that comprise an mbuf are:
  * the dp packet, which includes a struct rte mbuf (704B)
  * RTE_PKTMBUF_HEADROOM (128B)
  * packet data (aligned to 1k, as previously described)
  * RTE_PKTMBUF_TAILROOM (typically 0)

Some PMDs require that the total mbuf size (i.e. the total sum of all
of the above-listed components' lengths) is cache-aligned. To satisfy
this requirement, it may be necessary to round up the total mbuf size
with respect to cacheline size. In doing so, it's possible that the
dp_packet's data portion is inadvertently increased in size, such that
it no longer adheres to Rx buffer alignment. Consequently, the
following property of the mbuf no longer holds true:

mbuf.data_len == mbuf.buf_len - mbuf.data_off

This creates a problem in the case of multi-segment mbufs, where that
assumption is assumed to be true for all but the final segment in an
mbuf chain. Resolve this issue by adjusting the size of the mbuf's
private data portion, as opposed to the packet data portion when
aligning mbuf size to cachelines.

Co-authored-by: Tiago Lam 

Fixes: 4be4d22 ("netdev-dpdk: clean up mbuf initialization")
Fixes: 31b88c9 ("netdev-dpdk: round up mbuf_size to cache_line_size")
CC: Santosh Shukla 
Signed-off-by: Mark Kavanagh 
Signed-off-by: Tiago Lam 
Acked-by: Santosh Shukla 
Acked-by: Eelco Chaudron 
---
 Documentation/topics/dpdk/memory.rst | 28 +-
 lib/netdev-dpdk.c| 55 
 2 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/Documentation/topics/dpdk/memory.rst 
b/Documentation/topics/dpdk/memory.rst
index e5fb166..c9b739f 100644
--- a/Documentation/topics/dpdk/memory.rst
+++ b/Documentation/topics/dpdk/memory.rst
@@ -107,8 +107,8 @@ Example 1
 
  MTU = 1500 Bytes
  Number of mbufs = 262144
- Mbuf size = 3008 Bytes
- Memory required = 262144 * 3008 = 788 MB
+ Mbuf size = 2752 Bytes
+ Memory required = 262144 * 2752 = 721 MB
 
 Example 2
 +
@@ -116,8 +116,8 @@ Example 2
 
  MTU = 1800 Bytes
  Number of mbufs = 262144
- Mbuf size = 3008 Bytes
- Memory required = 262144 * 3008 = 788 MB
+ Mbuf size = 2752 Bytes
+ Memory required = 262144 * 2752 = 721 MB
 
 .. note::
 
@@ -130,8 +130,8 @@ Example 3
 
  MTU = 6000 Bytes
  Number of mbufs = 262144
- Mbuf size = 8128 Bytes
- Memory required = 262144 * 8128 = 2130 MB
+ Mbuf size = 8000 Bytes
+ Memory required = 262144 * 8000 = 2097 MB
 
 Example 4
 +
@@ -139,8 +139,8 @@ Example 4
 
  MTU = 9000 Bytes
  Number of mbufs = 262144
- Mbuf size = 10176 Bytes
- Memory required = 262144 * 10176 = 2667 MB
+ Mbuf size = 10048 Bytes
+ Memory required = 262144 * 10048 = 2634 MB
 
 Per Port Memory Calculations
 
@@ -194,8 +194,8 @@ Example 1: (1 rxq, 1 PMD, 1500 MTU)
 
  MTU = 1500
  Number of mbufs = (1 * 2048) + (2 * 2048) + (1 * 32) + (16384) = 22560
- Mbuf size = 3008 Bytes
- Memory required = 22560 * 3008 = 67 MB
+ Mbuf size = 2752 Bytes
+ Memory required = 22560 * 2752 = 62 MB
 
 Example 2: (1 rxq, 2 PMD, 6000 MTU)
 +++
@@ -203,8 +203,8 @@ Example 2: (1 rxq, 2 PMD, 6000 MTU)
 
  MTU = 6000
  Number of mbufs = (1 * 2048) + (3 * 2048) + (1 * 32) + (16384) = 24608
- Mbuf size = 8128 Bytes
- Memory required = 24608 * 8128 = 200 MB
+ Mbuf size = 8000 Bytes
+ Memory required = 24608 * 8000 = 196 MB
 
 Example 3: (2 rxq, 2 PMD, 9000 MTU)
 +++
@@ -212,5 +212,5 @@ Example 3: (2 rxq, 2 PMD, 9000 MTU)
 
  MTU = 9000
  Number of mbufs = (2 * 2048) + (3 * 2048) + (1 * 32) + (16384) = 26656
- Mbuf size = 10176 Bytes
- Memory required = 26656 * 10176 = 271 MB
+ Mbuf size = 10048 Bytes
+ Memory required = 26656 * 10048 = 267 MB
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index f91aa27..a72438a 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -88,10 +88,6 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 
20);
 #define MTU_TO_MAX_FRAME_LEN(mtu)   ((mtu) + ETHER_HDR_MAX_LEN)
 #define FRAME_LEN_TO_MTU(frame_len) ((frame_len)\
  - ETHER_HDR_LEN - ETHER_CRC_LEN)
-#define MBUF_SIZE(mtu)  ROUND_UP((MTU_TO_MAX_FRAME_LEN(mtu) \
- + sizeof(struct dp_packet) \
- + RTE_PKTMBUF_HEADROOM),   \
- RTE_CACHE_LINE_SIZE)
 #define NETDEV_DPDK_MBUF_ALIGN  1024
 #define NETDEV_DPDK_MAX_PKT_LEN 9728
 
@@ -637,7 +633,11 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool 
per_port_mp)
 char 

[ovs-dev] [PATCH v12 0/3] Misc fixes in dp_packet and netdev-dpdk.

[Tiago Lam]

This series is a split from the multi-seg mbuf series, "v11 - Support
multi-segment mbufs"; Hence why it starts at v12.

v12: - Rebase on master af26093 ("connmgr: Improve interface for setting
   controllers.");
 - Address Ian's comments:
   - Updated memory model calculations in dpdk/memory.rst docs to reflect
 the mbufs sizes;
   - Avoid casting in dp_packet_mbuf_init() to comply with the style in the
 same file;
   - Reword comment dp_packet_init_dpdk() which weren't making much sense
 after the code changes.
 - Address Ilya's comments:
   - Use MTU_TO_FRAME_LEN macro in dpdk_mp_create() instead of
 recalculating the mbuf size from the original dev->mtu;
   - Remove RTE_PKTMBUF_HEADROOM when calculating from 'pkt_size' in
 dpdk_mp_create(). 

Mark Kavanagh (2):
  netdev-dpdk: fix mbuf sizing
  dp-packet: Init specific mbuf fields.

Tiago Lam (1):
  dp-packet: Fix allocated size on DPDK init.

 Documentation/topics/dpdk/memory.rst | 28 +-
 lib/dp-packet.c  | 11 ++-
 lib/dp-packet.h  | 12 
 lib/netdev-dpdk.c| 57 
 4 files changed, 61 insertions(+), 47 deletions(-)

-- 
2.7.4

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

Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-upcall: Don't flush datapath flows in quiescent state.

[Ilya Maximets]

On 01.11.2018 21:30, Ben Pfaff wrote:
> On Thu, Nov 01, 2018 at 04:58:41PM +0300, Ilya Maximets wrote:
>> Datapath implementation of 'flow_flush()' could use cmaps.
>> dpif-netdev actively uses them to store flow tables and the polling
>> threads. Flushing flows while in a quiescent state may lead to
>> wrong memory accesses because implementaion of concurrent hash maps
>> relies on the rcu protected pointers which could float away.
>>
>> CC: Alex Wang 
>> Fixes: 1f8675481e8c ("ofproto-dpif-upcall: Fix ovs-vswitchd crash.")
>> Signed-off-by: Ilya Maximets 
> 
> I think this patch becomes unnecessary if my logic for the first one is
> correct.

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

Re: [ovs-dev] [PATCH v2 2/2] netdev-dpdk: Add link speed to get_status().

[Ilya Maximets]

On 02.11.2018 1:31, Ian Stokes wrote:
> Report the link speed of the device in netdev_dpdk_get_status()
> function.
> 
> Link speed is already reported as part of the netdev_get_features()
> function. However only link speeds defined in the OpenFlow specs are
> supported so speeds such as 25 Gbps etc. are not shown. The link
> speed for the device is available in Mbps in rte_eth_link.
> This commit converts the link speed for a given dpdk device to an
> easy to read string and reports it in get_status().
> 
> Suggested-by: Flavio Leitner 
> Signed-off-by: Ian Stokes 
> ---
> v1 -> v2
> * Replace snprintf with a return call of for each string.
> ---
>  lib/netdev-dpdk.c | 35 +++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 59f4ccbfe..5e6a72bc9 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -3021,11 +3021,37 @@ netdev_dpdk_vhost_user_get_status(const struct netdev 
> *netdev,
>  return 0;
>  }
>  
> +/*
> + * Convert a given uint32_t link speed defined in DPDK to a string
> + * equivalent.
> + */
> +static const char *
> +netdev_dpdk_link_speed_to_str__(uint32_t link_speed)
> +{
> +switch (link_speed) {
> +case ETH_SPEED_NUM_10M:return "10Mbps";
> +case ETH_SPEED_NUM_100M:   return "100Mbps";
> +case ETH_SPEED_NUM_1G: return "1Gbps";
> +case ETH_SPEED_NUM_2_5G:   return "2.5Gbps";
> +case ETH_SPEED_NUM_5G: return "5Gbps";
> +case ETH_SPEED_NUM_10G:return "10Gbps";
> +case ETH_SPEED_NUM_20G:return "20Gbps";
> +case ETH_SPEED_NUM_25G:return "25Gbps";
> +case ETH_SPEED_NUM_40G:return "40Gbps";
> +case ETH_SPEED_NUM_50G:return "50Gbps";
> +case ETH_SPEED_NUM_56G:return "56Gbps";
> +case ETH_SPEED_NUM_100G:   return "100Gbps";
> +default:   return "Not Defined";
> +}
> +}
> +
>  static int
>  netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args)
>  {
>  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>  struct rte_eth_dev_info dev_info;
> +struct rte_eth_link link;
> +const char *link_speed_str;
>  
>  if (!rte_eth_dev_is_valid_port(dev->port_id)) {
>  return ENODEV;
> @@ -3033,6 +3059,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
> struct smap *args)
>  
>  ovs_mutex_lock(>mutex);
>  rte_eth_dev_info_get(dev->port_id, _info);
> +link = dev->link;
>  ovs_mutex_unlock(>mutex);
>  
>  smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
> @@ -3064,6 +3091,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
> struct smap *args)
>  dev_info.pci_dev->id.device_id);
>  }
>  
> +/* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps.
> + * In that case the speed will not be reported as part of the usual
> + * call to get_features(). Get the link speed of the device and add it
> + * to the device status in an easy to read string format.
> + */
> +link_speed_str = netdev_dpdk_link_speed_to_str__(link.link_speed);
> +smap_add_format(args, "link_speed", "%s", link_speed_str);

How about this:

smap_add(args, "link_speed",
 netdev_dpdk_link_speed_to_str__(link.link_speed));

?

> +
>  return 0;
>  }
>  
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Don't purge ukeys while in a quiescent state.

[Ilya Maximets]

On 01.11.2018 21:29, Ben Pfaff wrote:
> On Thu, Nov 01, 2018 at 04:58:40PM +0300, Ilya Maximets wrote:
>> revalidator_purge() iterates and modifies umap->cmap. This should
>> not happen in quiescent state, because cmap implementation based
>> on rcu protected variables. Let's move the thread to active state
>> before purging to avoid possible wrong memory accesses.
>>
>> CC: Joe Stringer 
>> Fixes: 9fce0584a643 ("revalidator: Use 'cmap' for storing ukeys.")
>> Signed-off-by: Ilya Maximets 
> 
> Thank you for catching the problem, and for the fix.  This is quite
> subtle.
> 
> Looking at the code here, and the history, it's not at all clear to me
> why there needs to be such a wide window of RCU quiescing.  Usually, RCU
> quiescing is to avoid delaying the RCU freeing thread across an
> operation that can block for a considerable amount of time.  Across
> udpif_stop_threads() and udpif_start_threads(), I only see a few
> operations that can do that, in particular the xpthread_join() calls.
> dpif_disable_upcall() could also arguably be slow for dpif-netdev since
> it's capturing a rwlock for writing.
> 
> So, I'd be inclined to fix this by narrowing the quiescent window,
> something like the following.  Do you see flaws in my analysis?

I agree with you. One thing is that pthread_create() also could
consume valuable amount of time on some systems. Up to few hundreds
of milliseconds. This could be a problem in case of big number of
revalidators. So, I'd like to have following incremental:

---
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 57869cb32..e44339338 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -565,6 +565,8 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers_,
 size_t n_revalidators_)
 {
 if (udpif && n_handlers_ && n_revalidators_) {
+ovsrcu_quiesce_start();
+
 udpif->n_handlers = n_handlers_;
 udpif->n_revalidators = n_revalidators_;
 
@@ -595,6 +597,7 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers_,
 revalidator->thread = ovs_thread_create(
 "revalidator", udpif_revalidator, revalidator);
 }
+ovsrcu_quiesce_end();
 }
 }
 
---

What do you think?

Will you format a patch?

Few comments inline.

Best regards, Ilya Maximets.

> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index e36cfa0eecca..22beaa569719 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -504,34 +504,34 @@ udpif_destroy(struct udpif *udpif)
>  free(udpif);
>  }
>  
> -/* Stops the handler and revalidator threads, must be enclosed in
> - * ovsrcu quiescent state unless when destroying udpif. */
> +/* Stops the handler and revalidator threads. */
>  static void
>  udpif_stop_threads(struct udpif *udpif)
>  {
>  if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) {
>  size_t i;
>  
> +/* Tell the threads to exit. */
>  latch_set(>exit_latch);
>  
> +/* Wait for the threads to exit.  This can take a long time so 
> quiesce
> + * so as not to block RCU freeing. */

Something wrong with above comment. Could you fix or re-word?

> +ovsrcu_quiesce_start();
>  for (i = 0; i < udpif->n_handlers; i++) {
>  struct handler *handler = >handlers[i];
>  
>  xpthread_join(handler->thread, NULL);

As you're dropping the local variable for purging loop below, maybe
it'll be good to drop the local variable here too. This will make all
three loops look uniform.

>  }
> -
>  for (i = 0; i < udpif->n_revalidators; i++) {
>  xpthread_join(udpif->revalidators[i].thread, NULL);
>  }
> -
>  dpif_disable_upcall(udpif->dpif);
> +ovsrcu_quiesce_end();
>  
> +/* Delete ukeys, and delete all flows from the datapath to prevent
> + * double-counting stats. */
>  for (i = 0; i < udpif->n_revalidators; i++) {
> -struct revalidator *revalidator = >revalidators[i];
> -
> -/* Delete ukeys, and delete all flows from the datapath to 
> prevent
> - * double-counting stats. */
> -revalidator_purge(revalidator);
> +revalidator_purge(>revalidators[i]);
>  }
>  
>  latch_poll(>exit_latch);
> @@ -549,8 +549,7 @@ udpif_stop_threads(struct udpif *udpif)
>  }
>  }
>  
> -/* Starts the handler and revalidator threads, must be enclosed in
> - * ovsrcu quiescent state. */
> +/* Starts the handler and revalidator threads. */
>  static void
>  udpif_start_threads(struct udpif *udpif, size_t n_handlers_,
>  size_t n_revalidators_)
> @@ -623,7 +622,6 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
>  ovs_assert(udpif);
>  ovs_assert(n_handlers_ && n_revalidators_);
>  
> -ovsrcu_quiesce_start();
>  if (udpif->n_handlers