Re: [ovs-dev] [PATCH 1/2] ovn-ctl: Allow passing ssl certs when starting OVN DBs in ssl mode.

[aginwala aginwala]

Thanks for the review Han. Please find the comments inline below:
On Thu, Oct 4, 2018 at 9:58 AM Han Zhou  wrote:

> Thanks Ali, please see my comments below
>
> On Fri, Sep 21, 2018 at 5:34 PM  wrote:
> >
> >  For OVN DBs to work with SSL in HA, we need to have capability to
> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs in
> active
> >  passive mode, in order for the standby DBs to sync from master node, it
> >  cannot sync because the required ssl certs are not passed when standby
> DBs
> >  are initialized. Hence, we need to have this option.
> >
> > e.g. start nb db with ssl certs as below:
> > /usr/share/openvswitch/scripts/ovn-ctl
> --ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
> > --db-nb-create-insecure-remote=no start_nb_ovsdb
> >
> > Certs can be generated based on ovs ssl docs:
> > http://docs.openvswitch.org/en/latest/howto/ssl/
> >
> > Signed-off-by: aginwala 
> > ---
> >  ovn/utilities/ovn-ctl | 50
> +++---
> >  1 file changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> > index 3ff0df6..4f45f3d 100755
> > --- a/ovn/utilities/ovn-ctl
> > +++ b/ovn/utilities/ovn-ctl
> > @@ -116,6 +116,9 @@ start_ovsdb__() {
> >  local addr
> >  local active_conf_file
> >  local use_remote_in_db
> > +local ovn_db_ssl_key
> > +local ovn_db_ssl_cert
> > +local ovn_db_ssl_cacert
> >  eval pid=\$DB_${DB}_PID
> >  eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
> >  eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
> > @@ -137,6 +140,9 @@ start_ovsdb__() {
> >  eval addr=\$DB_${DB}_ADDR
> >  eval active_conf_file=\$ovn${db}_active_conf_file
> >  eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
> > +eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
> > +eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
> > +eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
> >
> >  # Check and eventually start ovsdb-server for DB
> >  if pidfile_is_running $pid; then
> > @@ -182,17 +188,32 @@ $cluster_remote_port
> >
> >  if test X"$use_remote_in_db" != Xno; then
> >  set "$@" --remote=db:$schema_name,$table_name,connections
> > +if test X"$create_insecure_remote" = Xno; then
> > +set "$@" --remote=pssl:$port:$addr
> > +elif test X"$create_insecure_remote" = Xyes; then
> > +set "$@" --remote=ptcp:$port:$addr
> > +fi
> Why moving the logic here? This if block only says if the connection
> settings in DB table should be used. Whether insecure mode is allowed was
> supposed to be independent with this condition. Could you explain the
> reason behind the change?
> >> I moved it because remote=db is needed if ovsdb is running as a
> standalone node or an active ovsdb server node. For standby nodes in case
> of active_passive mode, remote=db will not be there because it uses
> --sync-from. Hope its clear.
>


> >  fi
> > -set "$@" --private-key=db:$schema_name,SSL,private_key
> > -set "$@" --certificate=db:$schema_name,SSL,certificate
> > -set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> > -set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> > -set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
>
> So it will not use the settings in DB any more? It seems this change
> removed support for "--use-remote-in-db=yes", which is the default behavior
> that should be kept. The DB settings should not be used only if
> "--use-remote-in-db=no"
>
>>  As discussed, this is similar support that we have for ovn-controller
with ssl. If the key is passed from cli, it will use the key else fall back
to default setters for ssl.

> >
> > -if test X"$create_insecure_remote" = Xyes; then
> > -set "$@" --remote=ptcp:$port:$addr
> > +if test X"$ovn_db_ssl_key" != X; then
> > +set "$@" --private-key=$ovn_db_ssl_key
> > +else
> > +set "$@" --private-key=db:$schema_name,SSL,private_key
> > +fi
> > +if test X"$ovn_db_ssl_cert" != X; then
> > +set "$@" --certificate=$ovn_db_ssl_cert
> > +else
> > +set "$@" --certificate=db:$schema_name,SSL,certificate
> > +fi
> > +if test X"$ovn_db_ssl_cacert" != X; then
> > +set "$@" --ca-cert=$ovn_db_ssl_cacert
> > +else
> > +set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> >  fi
> >
> > +set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> > +set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
> > +
> >  if test $mode = active_passive; then
> >  set "$@" --sync-from=`cat $active_conf_file`
> >  fi
> > @@ -481,6 +502,15 @@ set_defaults () {
> >  OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
> >  DB_NB_USE_REMOTE_IN_DB="yes"
> >  DB_SB_USE_REMOTE_IN_DB="yes"
> > +
> > +OVN_NB_DB_SSL_KEY=""

Re: [ovs-dev] [PATCH 2/2] ovndb-servers.ocf: Add ssl support for managing OVN DB resources with pacemaker using LB VIP.

[aginwala aginwala]

Thanks for the review Han. Please find the comments inline below:

On Thu, Oct 4, 2018 at 10:16 AM Han Zhou  wrote:

> Thanks Ali, please see my comm
>
> On Fri, Sep 21, 2018 at 5:38 PM  wrote:
> >
> >  When starting OVN DBs in HA using pacemaker with ssl, we need to pass
> ssl
> >  certs for starting standby DBs. Hence, we need this change.
> >
> > Signed-off-by: aginwala 
> > ---
> >  ovn/utilities/ovndb-servers.ocf | 74
> -
> >  1 file changed, 73 insertions(+), 1 deletion(-)
> >
> > diff --git a/ovn/utilities/ovndb-servers.ocf
> b/ovn/utilities/ovndb-servers.ocf
> > index 52141c7..80f81ae 100755
> > --- a/ovn/utilities/ovndb-servers.ocf
> > +++ b/ovn/utilities/ovndb-servers.ocf
> > @@ -10,6 +10,12 @@
> >  : ${MANAGE_NORTHD_DEFAULT="no"}
> >  : ${INACTIVE_PROBE_DEFAULT="5000"}
> >  : ${LISTEN_ON_MASTER_IP_ONLY_DEFAULT="yes"}
> > +: ${NB_SSL_KEY_DEFAULT="/etc/openvswitch/ovnnb-privkey.pem"}
> > +: ${NB_SSL_CERT_DEFAULT="/etc/openvswitch/ovnnb-cert.pem"}
> > +: ${NB_SSL_CACERT_DEFAULT="/etc/openvswitch/cacert.pem"}
> > +: ${SB_SSL_KEY_DEFAULT="/etc/openvswitch/ovnsb-privkey.pem"}
> > +: ${SB_SSL_CERT_DEFAULT="/etc/openvswitch/ovnsb-cert.pem"}
> > +: ${SB_SSL_CACERT_DEFAULT="/etc/openvswitch/cacert.pem"}
> >
> >  CRM_MASTER="${HA_SBIN_DIR}/crm_master -l reboot"
> >  CRM_ATTR_REPL_INFO="${HA_SBIN_DIR}/crm_attribute --type crm_config
> --name OVN_REPL_INFO -s ovn_ovsdb_master_server"
> > @@ -21,6 +27,13 @@
> SB_MASTER_PORT=${OCF_RESKEY_sb_master_port:-${SB_MASTER_PORT_DEFAULT}}
> >
>  SB_MASTER_PROTO=${OCF_RESKEY_sb_master_protocol:-${SB_MASTER_PROTO_DEFAULT}}
> >  MANAGE_NORTHD=${OCF_RESKEY_manage_northd:-${MANAGE_NORTHD_DEFAULT}}
> >
>  
> INACTIVE_PROBE=${OCF_RESKEY_inactive_probe_interval:-${INACTIVE_PROBE_DEFAULT}}
> > +NB_PRIVKEY=${OCF_RESKEY_ovn_nb_db_privkey:-${NB_SSL_KEY_DEFAULT}}
> > +NB_CERT=${OCF_RESKEY_ovn_nb_db_cert:-${NB_SSL_CERT_DEFAULT}}
> > +NB_CACERT=${OCF_RESKEY_ovn_nb_db_cacert:-${NB_SSL_CACERT_DEFAULT}}
> > +SB_PRIVKEY=${OCF_RESKEY_ovn_sb_db_privkey:-${SB_SSL_KEY_DEFAULT}}
> > +SB_CERT=${OCF_RESKEY_ovn_sb_db_cert:-${SB_SSL_CERT_DEFAULT}}
> > +SB_CACERT=${OCF_RESKEY_ovn_sb_db_cacert:-${SB_SSL_CACERT_DEFAULT}}
> > +
> >
> >  # In order for pacemaker to work with LB, we can set
> LISTEN_ON_MASTER_IP_ONLY
> >  # to false and pass LB vip IP while creating pcs resource.
> > @@ -132,6 +145,54 @@ ovsdb_server_metadata() {
> >
> >
> >
> > +  
> > +  
> > +  OVN NB DB private key absolute path for ssl setup.
> > +  
> > +  OVN NB DB private key file
> > +  
> > +  
> > +
> > +  
> > +  
> > +  OVN NB DB certificate absolute path for ssl setup.
> > +  
> > +  OVN NB DB cert file
> > +  
> > +  
> > +
> > +  
> > +  
> > +  OVN NB DB CA certificate absolute path for ssl setup.
> > +  
> > +  OVN NB DB cacert file
> > +  
> > +  
> > +
> > +  
> > +  
> > +  OVN SB DB private key absolute path for ssl setup.
> > +  
> > +  OVN SB DB private key file
> > +  
> > +  
> > +
> > +  
> > +  
> > +  OVN SB DB certificate absolute path for ssl setup.
> > +  
> > +  OVN SB DB cert file
> > +  
> > +  
> > +
> > +  
> > +  
> > +  OVN SB DB CA certificate absolute path for ssl setup.
> > +  
> > +  OVN SB DB cacert file
> > +  
> > +  
> > +
> >
> >
> >
> > @@ -326,6 +387,18 @@ ovsdb_server_start() {
> > set $@ --db-sb-addr=${MASTER_IP} --db-sb-port=${SB_MASTER_PORT}
> >  fi
> >
> > +if [ "x${NB_MASTER_PROTO}" = xssl ]; then
> > +set $@ --db-nb-create-insecure-remote=no
> "no" is the default value, so this line is not needed.
>
>> Sure. This makes sense. Will check out the default behavior and update
it the revised patch!

>
> > +set $@ --ovn-nb-db-ssl-key=${NB_PRIVKEY}
> > +set $@ --ovn-nb-db-ssl-cert=${NB_CERT}
> > +set $@ --ovn-nb-db-ssl-ca-cert=${NB_CACERT}
> This should be needed only for standby which sets
> --db-sb-use-remote-in-db=no.
>
> As discussed, for each of the modes either ssl or tcp, all the nodes
should have this option set.

>
> > +fi
> > +if [ "x${SB_MASTER_PROTO}" = xssl ]; then
> > +set $@ --db-sb-create-insecure-remote=no
> > +set $@ --ovn-sb-db-ssl-key=${SB_PRIVKEY}
> > +set $@ --ovn-sb-db-ssl-cert=${SB_CERT}
> > +set $@ --ovn-sb-db-ssl-ca-cert=${SB_CACERT}
> > +fi
> >  if [ "x${present_master}" = x ]; then
> >  # No master detected, or the previous master is not among the
> >  # set starting.
> > @@ -343,7 +416,6 @@ ovsdb_server_start() {
> >  set $@ --db-nb-sync-from-addr=${INVALID_IP_ADDRESS}
> --db-sb-sync-from-addr=${INVALID_IP_ADDRESS}
> >
> >  elif [ ${present_master} != ${host_name} ]; then
> > -# TODO: for using LB vip, need to test for ssl.
> >  if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xyes ]; then
> >  if [ "x${NB_MASTER_PROTO}" = xtcp ]; then
> >  set $@ --db-nb-create-insecure-remote=yes
> > --
> > 1.9.1
> >
> > 

Re: [ovs-dev] [PATCH] Python: Make Row's __getattr__ less error prone

[Ben Pfaff]

On Fri, Oct 05, 2018 at 04:31:07PM +0100, lucasago...@gmail.com wrote:
> From: Lucas Alvares Gomes 
> 
> Calling getattr() on a Row object after invoking delkey() with a value
> that does not exist in the object will cause getattr() to fail with a
> KeyError error. For example:
> 
> Oct 05 14:59:28 neutron-server[28435]:   File
> "/usr/local/lib/python2.7/dist-packages/ovsdbapp/backend/ovs_idl/connection.py",
> line 122, in run
> Oct 05 14:59:28 neutron-server[28435]:
> txn.results.put(txn.do_commit())
> Oct 05 14:59:28 neutron-server[28435]:   File
> "/usr/local/lib/python2.7/dist-packages/ovsdbapp/backend/ovs_idl/transaction.py",
> line 86, in do_commit
> Oct 05 14:59:28 neutron-server[28435]: command.run_idl(txn)
> Oct 05 14:59:28 neutron-server[28435]:   File
> "/usr/local/lib/python2.7/dist-packages/ovsdbapp/backend/ovs_idl/command.py",
> line 299, in run_idl
> Oct 05 14:59:28 neutron-server[28435]: if
> isinstance(getattr(record, self.column), dict):
> Oct 05 14:59:28 neutron-server[28435]:   File
> "/usr/local/lib/python2.7/dist-packages/ovs/db/idl.py", line 843, in
> __getattr__
> Oct 05 14:59:28 neutron-server[28435]: del dmap[key]
> Oct 05 14:59:28 neutron-server[28435]: KeyError: 'bogusvalue'
> 
> This patch is replacing the "del dmap[key]" instruction with a
> "dmap.pop(key, None)" instruction instead because a pop() (with a
> default value) will not raise an exception in case the key does not
> exist in the object in the first place, it will just ignore it.
> 
> Signed-Off-By: Lucas Alvares Gomes 

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

Re: [ovs-dev] [PATCH v2 0/2] Add nanosecond resolution PCAP support

[Ben Pfaff]

On Fri, Oct 05, 2018 at 12:52:38PM -0400, Mark Michelson wrote:
> PCAP files can support nanosecond resolution instead of microsecond
> resolution in timestamps. It indicates this is in effect in the header
> of the file in the magic number. Without these patches we'll think that
> the pcap file is invalid if it is using nanosecond resolution.
> 
> Mark Michelson (2):
>   ovs-pcap: Support nanosecond resolution pcap files.
>   pcap-file: Add nanosecond resolution pcap support.

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

Re: [ovs-dev] [PATCH v2] dpif-netlink: don't allocate per thread netlink sockets

[Matteo Croce]

On Fri, Oct 5, 2018 at 11:27 PM Guru Shetty  wrote:
> I get a segfault with this patch with the following backtrace. I have not 
> investigated.
>
> Program received signal SIGSEGV, Segmentation fault.
> nl_sock_pid (sock=0x0) at lib/netlink-socket.c:1424
> 1424 return sock->pid;
> (gdb) where
> #0  nl_sock_pid (sock=0x0) at lib/netlink-socket.c:1424
> #1  0x0052fb79 in dpif_netlink_port_add__ (dpif=dpif@entry=0x89de90, 
> name=name@entry=0x7fffdf20 "genev_sys_6081", 
> type=type@entry=OVS_VPORT_TYPE_GENEVE,
> options=0x7fffdee0, port_nop=0x7fffdf9c) at lib/dpif-netlink.c:731
> #2  0x0052fdc7 in dpif_netlink_port_add_compat 
> (port_nop=0x7fffdf9c, netdev=, dpif=0x89de90) at 
> lib/dpif-netlink.c:836
> #3  dpif_netlink_port_add (dpif_=0x89de90, netdev=, 
> port_nop=0x7fffdf9c) at lib/dpif-netlink.c:882
> #4  0x004778be in dpif_port_add (dpif=0x89de90, 
> netdev=netdev@entry=0x911830, port_nop=port_nop@entry=0x7fffdffc) at 
> lib/dpif.c:579
> #5  0x00426efe in port_add (ofproto_=0x8a97f0, netdev=0x911830) at 
> ofproto/ofproto-dpif.c:3689
> #6  0x0041d951 in ofproto_port_add (ofproto=0x8a97f0, 
> netdev=0x911830, ofp_portp=ofp_portp@entry=0x7fffe0d8) at 
> ofproto/ofproto.c:2008
> #7  0x0040c709 in iface_do_create (errp=0x7fffe0e8, 
> netdevp=0x7fffe0e0, ofp_portp=0x7fffe0d8, iface_cfg=0x8a0620, 
> br=0x8a8bb0) at vswitchd/bridge.c:1803
> #8  iface_create (port_cfg=0x8a4e60, iface_cfg=0x8a0620, br=0x8a8bb0) at 
> vswitchd/bridge.c:1841
> #9  bridge_add_ports__ (br=br@entry=0x8a8bb0, 
> wanted_ports=wanted_ports@entry=0x8a8c90, 
> with_requested_port=with_requested_port@entry=false) at vswitchd/bridge.c:935
> #10 0x0040e02f in bridge_add_ports (wanted_ports=0x8a8c90, 
> br=0x8a8bb0) at vswitchd/bridge.c:951
> #11 bridge_reconfigure (ovs_cfg=ovs_cfg@entry=0x8a7fc0) at 
> vswitchd/bridge.c:665
> #12 0x004114b9 in bridge_run () at vswitchd/bridge.c:3023
> #13 0x004080c5 in main (argc=2, argv=0x7fffe5e8) at 
> vswitchd/ovs-vswitchd.c:125
>
>

Hi Guru,

I received a similar report on IRC and I have already investigated it.
Can you try if the following patch fixes it?

Fixes: 69c51582f ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Matteo Croce 
---
 lib/dpif-netlink.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 21315033cc..310bc947db 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -712,7 +712,7 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif,
const char *name,
 struct dpif_netlink_vport request, reply;
 struct ofpbuf *buf;
 struct nl_sock *socksp = NULL;
-uint32_t upcall_pids;
+uint32_t upcall_pids = 0;
 int error = 0;

 if (dpif->handlers) {
@@ -728,7 +728,8 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif,
const char *name,
 request.name = name;

 request.port_no = *port_nop;
-upcall_pids = nl_sock_pid(socksp);
+if (socksp)
+upcall_pids = nl_sock_pid(socksp);
 request.n_upcall_pids = 1;
 request.upcall_pids = _pids;


--
Matteo Croce
per aspera ad upstream
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/2] windows, ovn-nbctl: Add service_start call inside the server loop

[Ben Pfaff]

On Fri, Oct 05, 2018 at 05:59:41PM +0300, Alin Gabriel Serdean wrote:
> 
> 
> > On 3 Oct 2018, at 01:37, Ben Pfaff  wrote:
> > 
> > On Tue, Oct 02, 2018 at 06:01:26PM +0300, Alin Gabriel Serdean wrote:
> >> Currently all ovn-nbctl (daemon) tests are failing due to the missing
> >> call to `service_start` which is required on Windows.
> >> 
> >> Windows lacks fork so we need to pass all arguments, so we can spawn a new
> >> process and interpret it properly when calling `service_start`.
> >> 
> >> Signed-off-by: Alin Gabriel Serdean 
> >> Acked-by: Ben Pfaff 
> >> ---
> >> v2: Remove OVS_UNUSED, add acks
> > 
> > I think that this can be made non-Windows specific by rewriting it as
> > follows.
> > 
> > diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> > index eabd30308ac2..7e42b53a9079 100644
> > --- a/ovn/utilities/ovn-nbctl.c
> > +++ b/ovn/utilities/ovn-nbctl.c
> > 
> 
> Thanks for the suggestion and patch Ben. It looks cleaner and it passes the 
> unit tests.
> 
> I sent a new version here:
> https://patchwork.ozlabs.org/patch/979499/ 
> 
> 
> Do you mind adding a sign off?

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

Re: [ovs-dev] [PATCH v2] dpif-netlink: don't allocate per thread netlink sockets

[Guru Shetty]

Reproduction looks to be easy. I just need to use the kernel module from
OVS repo and run 'ovs-vswitchd --pidfile'. If I do a 'ovs-vsctl add-br
br0', ovs-vswitchd will segfault.  I do see the the following message in
ovs-vswitchd log:

2018-10-05T12:39:19Z|4|netlink_socket|INFO|netlink: could not enable
listening to all nsid (Protocol not available)

This is a Ubuntu machine:

root@kube-master:~/git/ovs# uname -a
Linux kube-master 3.13.0-53-generic #89-Ubuntu SMP Wed May 20 10:34:39 UTC
2015 x86_64 x86_64 x86_64 GNU/Linux

root@kube-master:~/git/ovs# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 14.04.2 LTS
Release: 14.04
Codename: trusty

On Fri, 5 Oct 2018 at 16:27, Guru Shetty  wrote:

>
>
> On Tue, 25 Sep 2018 at 01:51, Matteo Croce  wrote:
>
>> When using the kernel datapath, OVS allocates a pool of sockets to handle
>> netlink events. The number of sockets is: ports * n-handler-threads, where
>> n-handler-threads is user configurable and defaults to 3/4*number of
>> cores.
>>
>> This because vswitchd starts n-handler-threads threads, each one with a
>> netlink socket for every port of the switch. Every thread then, starts
>> listening on events on its set of sockets with epoll().
>>
>> On setup with lot of CPUs and ports, the number of sockets easily hits
>> the process file descriptor limit, and ovs-vswitchd will exit with
>> -EMFILE.
>>
>> Change the number of allocated sockets to just one per port by moving
>> the socket array from a per handler structure to a per datapath one,
>> and let all the handlers share the same sockets by using EPOLLEXCLUSIVE
>> epoll flag which avoids duplicate events, on systems that support it.
>>
>> The patch was tested on a 56 core machine running Linux 4.18 and latest
>> Open vSwitch. A bridge was created with 2000+ ports, some of them being
>> veth interfaces with the peer outside the bridge. The latency of the
>> upcall
>> is measured by setting a single 'action=controller,local' OpenFlow rule to
>> force all the packets going to the slow path and then to the local port.
>> A tool[1] injects some packets to the veth outside the bridge, and
>> measures
>> the delay until the packet is captured on the local port. The rx timestamp
>> is get from the socket ancillary data in the attribute SO_TIMESTAMPNS, to
>> avoid having the scheduler delay in the measured time.
>>
>> The first test measures the average latency for an upcall generated from
>> a single port. To measure it 100k packets, one every msec, are sent to a
>> single port and the latencies are measured.
>>
>> The second test is meant to check latency fairness among ports, namely if
>> latency is equal between ports or if some ports have lower priority.
>> The previous test is repeated for every port, the average of the average
>> latencies and the standard deviation between averages is measured.
>>
>> The third test serves to measure responsiveness under load. Heavy traffic
>> is sent through all ports, latency and packet loss is measured
>> on a single idle port.
>>
>> The fourth test is all about fairness. Heavy traffic is injected in all
>> ports but one, latency and packet loss is measured on the single idle
>> port.
>>
>> This is the test setup:
>>
>>   # nproc
>>   56
>>   # ovs-vsctl show |grep -c Port
>>   2223
>>   # ovs-ofctl dump-flows ovs_upc_br
>>cookie=0x0, duration=4.827s, table=0, n_packets=0, n_bytes=0,
>> actions=CONTROLLER:65535,LOCAL
>>   # uname -a
>>   Linux fc28 4.18.7-200.fc28.x86_64 #1 SMP Mon Sep 10 15:44:45 UTC 2018
>> x86_64 x86_64 x86_64 GNU/Linux
>>
>> And these are the results of the tests:
>>
>>   Stock OVS
>>  Patched
>>   netlink sockets
>>   in use by vswitchd
>>   lsof -p $(pidof ovs-vswitchd) \
>>   |grep -c GENERIC91187
>> 2227
>>
>>   Test 1
>>   one port latency
>>   min/avg/max/mdev (us)   2.7/6.6/238.7/1.8
>>  1.6/6.8/160.6/1.7
>>
>>   Test 2
>>   all port
>>   avg latency/mdev (us)   6.51/0.97
>>  6.86/0.17
>>
>>   Test 3
>>   single port latency
>>   under load
>>   avg/mdev (us) 7.5/5.9
>>  3.8/4.8
>>   packet loss  95 %62
>> %
>>
>>   Test 4
>>   idle port latency
>>   under load
>>   min/avg/max/mdev (us)   0.8/1.5/210.5/0.9
>>  1.0/2.1/344.5/1.2
>>   packet loss  94 % 4
>> %
>>
>> CPU and RAM usage seems not to be affected, the resource usage of vswitchd
>> idle with 2000+ ports is unchanged:
>>
>>   # ps u $(pidof ovs-vswitchd)
>>   USER   PID %CPU %MEMVSZ   RSS TTY  STAT START   TIME COMMAND
>>   openvsw+  5430 54.3  0.3 4263964 510968 pts/1  RLl+ 16:20   0:50
>> ovs-vswitchd
>>
>> Additionally, to check if vswitchd is thread safe with this patch, the
>> following test was run for circa 48 hours: on a 56 core machine, a
>> bridge with kernel datapath is filled with 2200 

Re: [ovs-dev] [PATCH v2] dpif-netlink: don't allocate per thread netlink sockets

[Guru Shetty]

On Tue, 25 Sep 2018 at 01:51, Matteo Croce  wrote:

> When using the kernel datapath, OVS allocates a pool of sockets to handle
> netlink events. The number of sockets is: ports * n-handler-threads, where
> n-handler-threads is user configurable and defaults to 3/4*number of cores.
>
> This because vswitchd starts n-handler-threads threads, each one with a
> netlink socket for every port of the switch. Every thread then, starts
> listening on events on its set of sockets with epoll().
>
> On setup with lot of CPUs and ports, the number of sockets easily hits
> the process file descriptor limit, and ovs-vswitchd will exit with -EMFILE.
>
> Change the number of allocated sockets to just one per port by moving
> the socket array from a per handler structure to a per datapath one,
> and let all the handlers share the same sockets by using EPOLLEXCLUSIVE
> epoll flag which avoids duplicate events, on systems that support it.
>
> The patch was tested on a 56 core machine running Linux 4.18 and latest
> Open vSwitch. A bridge was created with 2000+ ports, some of them being
> veth interfaces with the peer outside the bridge. The latency of the upcall
> is measured by setting a single 'action=controller,local' OpenFlow rule to
> force all the packets going to the slow path and then to the local port.
> A tool[1] injects some packets to the veth outside the bridge, and measures
> the delay until the packet is captured on the local port. The rx timestamp
> is get from the socket ancillary data in the attribute SO_TIMESTAMPNS, to
> avoid having the scheduler delay in the measured time.
>
> The first test measures the average latency for an upcall generated from
> a single port. To measure it 100k packets, one every msec, are sent to a
> single port and the latencies are measured.
>
> The second test is meant to check latency fairness among ports, namely if
> latency is equal between ports or if some ports have lower priority.
> The previous test is repeated for every port, the average of the average
> latencies and the standard deviation between averages is measured.
>
> The third test serves to measure responsiveness under load. Heavy traffic
> is sent through all ports, latency and packet loss is measured
> on a single idle port.
>
> The fourth test is all about fairness. Heavy traffic is injected in all
> ports but one, latency and packet loss is measured on the single idle port.
>
> This is the test setup:
>
>   # nproc
>   56
>   # ovs-vsctl show |grep -c Port
>   2223
>   # ovs-ofctl dump-flows ovs_upc_br
>cookie=0x0, duration=4.827s, table=0, n_packets=0, n_bytes=0,
> actions=CONTROLLER:65535,LOCAL
>   # uname -a
>   Linux fc28 4.18.7-200.fc28.x86_64 #1 SMP Mon Sep 10 15:44:45 UTC 2018
> x86_64 x86_64 x86_64 GNU/Linux
>
> And these are the results of the tests:
>
>   Stock OVS Patched
>   netlink sockets
>   in use by vswitchd
>   lsof -p $(pidof ovs-vswitchd) \
>   |grep -c GENERIC911872227
>
>   Test 1
>   one port latency
>   min/avg/max/mdev (us)   2.7/6.6/238.7/1.8   1.6/6.8/160.6/1.7
>
>   Test 2
>   all port
>   avg latency/mdev (us)   6.51/0.97   6.86/0.17
>
>   Test 3
>   single port latency
>   under load
>   avg/mdev (us) 7.5/5.9 3.8/4.8
>   packet loss  95 %62 %
>
>   Test 4
>   idle port latency
>   under load
>   min/avg/max/mdev (us)   0.8/1.5/210.5/0.9   1.0/2.1/344.5/1.2
>   packet loss  94 % 4 %
>
> CPU and RAM usage seems not to be affected, the resource usage of vswitchd
> idle with 2000+ ports is unchanged:
>
>   # ps u $(pidof ovs-vswitchd)
>   USER   PID %CPU %MEMVSZ   RSS TTY  STAT START   TIME COMMAND
>   openvsw+  5430 54.3  0.3 4263964 510968 pts/1  RLl+ 16:20   0:50
> ovs-vswitchd
>
> Additionally, to check if vswitchd is thread safe with this patch, the
> following test was run for circa 48 hours: on a 56 core machine, a
> bridge with kernel datapath is filled with 2200 dummy interfaces and 22
> veth, then 22 traffic generators are run in parallel piping traffic into
> the veths peers outside the bridge.
> To generate as many upcalls as possible, all packets were forced to the
> slowpath with an openflow rule like 'action=controller,local' and packet
> size was set to 64 byte. Also, to avoid overflowing the FDB early and
> slowing down the upcall processing, generated mac addresses were restricted
> to a small interval. vswitchd ran without problems for 48+ hours,
> obviously with all the handler threads with almost 99% CPU usage.
>
> [1] https://github.com/teknoraver/network-tools/blob/master/weed.c
>
> Signed-off-by: Matteo Croce 
>

I get a segfault with this patch with the following backtrace. I have not
investigated.

Program received signal SIGSEGV, 

Re: [ovs-dev] [PATCH] odp-util: Fix a use-afer-free bug

[Yifeng Sun]

This patch should also fix the bug reported at
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10802

On Fri, Oct 5, 2018 at 2:50 PM Yifeng Sun  wrote:

> After ofpbug_put, actions may have been reallocated and
> key will point to invalid memory address.
>
> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10796
> Signed-off-by
> :
> Yifeng Sun 
> ---
>  lib/odp-util.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 890c71b7f336..7705bb30ae21 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2242,13 +2242,14 @@ parse_odp_action(const char *s, const struct simap
> *port_names,
>  key->nla_len += size;
>  ofpbuf_put(actions, mask + 1, size);
>
> -/* Add new padding as needed */
> -ofpbuf_put_zeros(actions, NLA_ALIGN(key->nla_len) -
> -  key->nla_len);
> -
>  /* 'actions' may have been reallocated by ofpbuf_put(). */
>  nested = ofpbuf_at_assert(actions, start_ofs, sizeof
> *nested);
>  nested->nla_type = OVS_ACTION_ATTR_SET_MASKED;
> +
> +key = nested + 1;
> +/* Add new padding as needed */
> +ofpbuf_put_zeros(actions, NLA_ALIGN(key->nla_len) -
> +  key->nla_len);
>  }
>  }
>  ofpbuf_uninit();
> --
> 2.7.4
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/2] ovn-nbctl: Don't parse table-formatting options in nbctl_client

[Mark Michelson]

When ovn-nbctl is running in daemon mode, nbctl_client attempts to parse
table formatting options. The problem is that this then removes the table
formatting options from the array of options passed to the server loop. The
server loop resets the table formatting options to the defaults and then
attempts again to parse table formatting options. Unfortunately, they aren't
present any longer. The result is that tables are always formatted with
the default style.

This patch solves the issue by not parsing the table formatting options
in nbctl_client. Instead, the table formatting options are passed to the
server loop and parsed there instead.

Signed-off-by: Mark Michelson 
---
 ovn/utilities/ovn-nbctl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index d65a9ba08..bff6d1380 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -5443,7 +5443,6 @@ nbctl_client(const char *socket_name,
 break;
 
 VLOG_OPTION_HANDLERS
-TABLE_OPTION_HANDLERS(_style)
 
 case OPT_LOCAL:
 default:
-- 
2.14.4

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

[ovs-dev] [PATCH 1/2] table: Create method for resetting table formatting.

[Mark Michelson]

Table formatting has a local static integer that is intended to insert
line breaks between tables. This works exactly as intended, as long as
each call to table_format() is done as a single unit within the run of a
process.

When ovn-nbctl is run in daemon mode, it is a long-running process that
makes multiple calls to table_format() throughout its lifetime. After
the first call, this results in an unexpected newline prepended to table
output on each subsequent ovn-nbctl invocation.

The solution is to introduce a function to reset table formatting. This
way, the first time after resetting table formatting, no newline is
prepended.

Signed-off-by: Mark Michelson 
---
 lib/table.c   | 23 +--
 lib/table.h   |  1 +
 ovn/utilities/ovn-nbctl.c |  1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/lib/table.c b/lib/table.c
index ab72668c7..48d18b651 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -236,15 +236,18 @@ table_print_timestamp__(const struct table *table, struct 
ds *s)
 }
 }
 
+static bool first_table = true;
+
 static void
 table_print_table__(const struct table *table, const struct table_style *style,
 struct ds *s)
 {
-static int n = 0;
 int *widths;
 size_t x, y;
 
-if (n++ > 0) {
+if (first_table) {
+first_table = false;
+} else {
 ds_put_char(s, '\n');
 }
 
@@ -318,10 +321,11 @@ static void
 table_print_list__(const struct table *table, const struct table_style *style,
struct ds *s)
 {
-static int n = 0;
 size_t x, y;
 
-if (n++ > 0) {
+if (first_table) {
+first_table = false;
+} else {
 ds_put_char(s, '\n');
 }
 
@@ -469,10 +473,11 @@ static void
 table_print_csv__(const struct table *table, const struct table_style *style,
   struct ds *s)
 {
-static int n = 0;
 size_t x, y;
 
-if (n++ > 0) {
+if (first_table) {
+first_table = false;
+} else {
 ds_put_char(s, '\n');
 }
 
@@ -614,6 +619,12 @@ table_format(const struct table *table, const struct 
table_style *style,
 }
 }
 
+void
+table_format_reset(void)
+{
+first_table = true;
+}
+
 /* Outputs 'table' on stdout in the specified 'style'. */
 void
 table_print(const struct table *table, const struct table_style *style)
diff --git a/lib/table.h b/lib/table.h
index 76e65bb70..33263e2a2 100644
--- a/lib/table.h
+++ b/lib/table.h
@@ -133,6 +133,7 @@ void table_parse_cell_format(struct table_style *, const 
char *format);
 void table_print(const struct table *, const struct table_style *);
 void table_format(const struct table *, const struct table_style *,
   struct ds *);
+void table_format_reset(void);
 void table_usage(void);
 
 #endif /* table.h */
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index eabd30308..d65a9ba08 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -5311,6 +5311,7 @@ server_cmd_run(struct unixctl_conn *conn, int argc, const 
char **argv_,
 }
 
 struct ds output = DS_EMPTY_INITIALIZER;
+table_format_reset();
 for (struct ctl_command *c = commands; c < [n_commands]; c++) {
 if (c->table) {
 table_format(c->table, _style, );
-- 
2.14.4

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

[ovs-dev] [PATCH 0/2] Fix ovn-nbctl daemon table printing issues.

[Mark Michelson]

ovn-nbctl when run in daemon mode has two issues:
1) An extra newline is prepended to table output
2) Table formatting issues are ignored.

This patch series fixes both issues.

Mark Michelson (2):
  table: Create method for resetting table formatting.
  ovn-nbctl: Don't parse table-formatting options in nbctl_client

 lib/table.c   | 23 +--
 lib/table.h   |  1 +
 ovn/utilities/ovn-nbctl.c |  2 +-
 3 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.14.4

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

[ovs-dev] [PATCH v3] extend-table: Fix a bug that iterates wrong table

[Yifeng Sun]

This seems to be a copy and paste bug that iterates and frees
the wrong table. This commit fixes that.

Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10730
Co-authored-by: Ben Pfaff 
Signed-off-by: Yifeng Sun 
Signed-off-by: Ben Pfaff 
---
v1->v2: Fix email subject by adding [ovs-dev]
v2->v3: Remove duplicated code by Ben.

 ovn/lib/extend-table.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/ovn/lib/extend-table.c b/ovn/lib/extend-table.c
index 511d1a84b..56c784f79 100644
--- a/ovn/lib/extend-table.c
+++ b/ovn/lib/extend-table.c
@@ -33,26 +33,25 @@ ovn_extend_table_init(struct ovn_extend_table *table)
 hmap_init(>existing);
 }
 
+static void
+ovn_extend_table_info_destroy(struct hmap *target)
+{
+struct ovn_extend_table_info *e, *next;
+HMAP_FOR_EACH_SAFE (e, next, hmap_node, target) {
+hmap_remove(target, >hmap_node);
+free(e->name);
+free(e);
+}
+hmap_destroy(target);
+}
+
 void
 ovn_extend_table_destroy(struct ovn_extend_table *table)
 {
 bitmap_free(table->table_ids);
 
-struct ovn_extend_table_info *desired, *d_next;
-HMAP_FOR_EACH_SAFE (desired, d_next, hmap_node, >existing) {
-hmap_remove(>existing, >hmap_node);
-free(desired->name);
-free(desired);
-}
-hmap_destroy(>desired);
-
-struct ovn_extend_table_info *existing, *e_next;
-HMAP_FOR_EACH_SAFE (existing, e_next, hmap_node, >existing) {
-hmap_remove(>existing, >hmap_node);
-free(existing->name);
-free(existing);
-}
-hmap_destroy(>existing);
+ovn_extend_table_info_destroy(>desired);
+ovn_extend_table_info_destroy(>existing);
 }
 
 /* Finds and returns a group_info in 'existing' whose key is identical
-- 
2.14.1

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

[ovs-dev] [PATCH] odp-util: Fix a use-afer-free bug

[Yifeng Sun]

After ofpbug_put, actions may have been reallocated and
key will point to invalid memory address.

Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10796
Signed-off-by: Yifeng Sun 
---
 lib/odp-util.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 890c71b7f336..7705bb30ae21 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2242,13 +2242,14 @@ parse_odp_action(const char *s, const struct simap 
*port_names,
 key->nla_len += size;
 ofpbuf_put(actions, mask + 1, size);
 
-/* Add new padding as needed */
-ofpbuf_put_zeros(actions, NLA_ALIGN(key->nla_len) -
-  key->nla_len);
-
 /* 'actions' may have been reallocated by ofpbuf_put(). */
 nested = ofpbuf_at_assert(actions, start_ofs, sizeof *nested);
 nested->nla_type = OVS_ACTION_ATTR_SET_MASKED;
+
+key = nested + 1;
+/* Add new padding as needed */
+ofpbuf_put_zeros(actions, NLA_ALIGN(key->nla_len) -
+  key->nla_len);
 }
 }
 ofpbuf_uninit();
-- 
2.7.4

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

Re: [ovs-dev] ossfuzz: Speed up flow extract fuzzing by refactoring

[Aaron Conole]

Bhargava Shastry  writes:

> Hello,
>
> I can take a look at these on Monday. In any case, they look easily fixable.
>
> P.S. I wonder why the build bot did not complain on the exact same
> code that has now been refactored out of flow_extract_target.c. As far
> as I can remember, that code also used asserts.

Depends on how the code arrived in the repository.  If it came by way of
a github pull request, the bot won't 'see' it.  Also, if it was sent
before the bot existed (June), then it also would have missed the
complaints.  Perhaps the code in question pre-dates the ovs_assert check
being added to checkpatch, also.

> On October 5, 2018 2:56:39 PM GMT+02:00, 0-day Robot  wrote:
>>Bleep bloop.  Greetings Bhargava Shastry, I am a robot and I have tried
>>out your patch.
>>Thanks for your contribution.
>>
>>I encountered some error that I wasn't expecting.  See the details
>>below.
>>
>>
>>checkpatch:
>>WARNING: Line lacks whitespace around operator
>>#585 FILE: tests/oss-fuzz/miniflow_target.c:95:
>>flow_u32[*idxp + i] = random_value();
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#722 FILE: tests/oss-fuzz/miniflow_target.c:232:
>>assert(miniflow_get_vid(miniflow, i) ==
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#726 FILE: tests/oss-fuzz/miniflow_target.c:236:
>>assert(miniflow_get(miniflow, i) == flow_u64[i]);
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#730 FILE: tests/oss-fuzz/miniflow_target.c:240:
>>assert(miniflow_equal(miniflow, miniflow));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#734 FILE: tests/oss-fuzz/miniflow_target.c:244:
>>assert(flow_equal(flow, ));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#737 FILE: tests/oss-fuzz/miniflow_target.c:247:
>>assert(miniflow_equal(miniflow, miniflow2));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#738 FILE: tests/oss-fuzz/miniflow_target.c:248:
>> assert(miniflow_hash__(miniflow, 0) == miniflow_hash__(miniflow2, 0));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#740 FILE: tests/oss-fuzz/miniflow_target.c:250:
>>assert(flow_equal(flow, ));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#748 FILE: tests/oss-fuzz/miniflow_target.c:258:
>>assert(minimask_is_catchall(minimask)
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#750 FILE: tests/oss-fuzz/miniflow_target.c:260:
>>assert(miniflow_equal_in_minimask(miniflow, miniflow2, minimask));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#751 FILE: tests/oss-fuzz/miniflow_target.c:261:
>>   assert(miniflow_equal_flow_in_minimask(miniflow, , minimask));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#752 FILE: tests/oss-fuzz/miniflow_target.c:262:
>>assert(miniflow_hash_in_minimask(miniflow, minimask, 0x12345678) ==
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#754 FILE: tests/oss-fuzz/miniflow_target.c:264:
>>assert(minimask_hash(minimask, 0) ==
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#760 FILE: tests/oss-fuzz/miniflow_target.c:270:
>>  assert(!miniflow_equal_flow_in_minimask(miniflow, , minimask));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#762 FILE: tests/oss-fuzz/miniflow_target.c:272:
>>assert(!miniflow_equal_in_minimask(miniflow, miniflow3, minimask));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#778 FILE: tests/oss-fuzz/miniflow_target.c:288:
>>assert(minimask_is_catchall(minicatchall));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#785 FILE: tests/oss-fuzz/miniflow_target.c:295:
>>assert(!minimask_has_extra(minimask, minimask));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#786 FILE: tests/oss-fuzz/miniflow_target.c:296:
>>assert(minimask_has_extra(minicatchall, minimask)
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#793 FILE: tests/oss-fuzz/miniflow_target.c:303:
>>assert(minimask_has_extra(minimask2, minimask));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#794 FILE: tests/oss-fuzz/miniflow_target.c:304:
>>assert(!minimask_has_extra(minimask, minimask2));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#810 FILE: tests/oss-fuzz/miniflow_target.c:320:
>>assert(minimask_is_catchall(minicatchall));
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#824 FILE: tests/oss-fuzz/miniflow_target.c:334:
>>assert(minimask_is_catchall());
>>
>>ERROR: Use ovs_assert() in place of assert()
>>#833 FILE: tests/oss-fuzz/miniflow_target.c:343:
>>assert(flow_wildcards_equal(, ));
>>
>>Lines checked: 861, Warnings: 1, Errors: 22
>>
>>
>>Please check this out.  If you feel there has been an error, please
>>email acon...@bytheb.org
>>
>>Thanks,
>>0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Add basic PG commands.

[Mark Michelson]

On 10/04/2018 06:08 PM, Han Zhou wrote:



On Wed, Oct 3, 2018 at 2:14 PM Mark Michelson > wrote:

 >
 > Signed-off-by: Mark Michelson >

 > ---
 >  ovn/utilities/ovn-nbctl.8.xml | 22 +
 >  ovn/utilities/ovn-nbctl.c     | 72 
+++
 >  tests/ovn.at                   | 37 
++

 >  3 files changed, 131 insertions(+)
 >
 > diff --git a/ovn/utilities/ovn-nbctl.8.xml 
b/ovn/utilities/ovn-nbctl.8.xml

 > index 5e18fa7c0..14cc02f53 100644
 > --- a/ovn/utilities/ovn-nbctl.8.xml
 > +++ b/ovn/utilities/ovn-nbctl.8.xml
 > @@ -881,6 +881,28 @@
 >        
 >      
 >
 > +    Port Group commands
 > +
 > +    
 > +      pg-add group [port]...
 > +      
 > +        Creates a new port group in the Port_Group 
table named
 > +        group with optional ports added to 
the group.

 > +      
 > +
 > +      pg-set-ports group 
port...

 > +      
 > +        Sets ports on the port group named 
group. It

 > +        is an error if group does not exist.
 > +      
 > +
 > +      pg-del group
 > +      
 > +        Deletes port group group. It is an error if
 > +        group does not exist.
 > +      
 > +    
 > +
 >      Database Commands
 >      These commands query and modify the contents of 
ovsdb tables.
 >      They are a slight abstraction of the ovsdb 
interface and

 > diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
 > index eabd30308..b586db502 100644
 > --- a/ovn/utilities/ovn-nbctl.c
 > +++ b/ovn/utilities/ovn-nbctl.c
 > @@ -4655,6 +4655,73 @@ cmd_set_ssl(struct ctl_context *ctx)
 >      nbrec_nb_global_set_ssl(nb_global, ssl);
 >  }
 >
 > +static char *
 > +set_ports_on_pg(struct ctl_context *ctx, const struct 
nbrec_port_group *pg,

 > +                char **new_ports, size_t num_new_ports)
 > +{
 > +    struct nbrec_logical_switch_port **lports;
 > +    lports = xmalloc(sizeof *lports * num_new_ports);
 > +
 > +    size_t i;
 > +    char *error = NULL;
 > +    for (i = 0; i < num_new_ports; i++) {
 > +        const struct nbrec_logical_switch_port *lsp;
 > +        error = lsp_by_name_or_uuid(ctx, new_ports[i], true, );
 > +        if (error) {
 > +            goto out;
 > +        }
 > +        lports[i] = (struct nbrec_logical_switch_port *) lsp;
 > +    }
 > +
 > +    nbrec_port_group_set_ports(pg, lports, num_new_ports);
 > +
 > +out:
 > +    free(lports);
 > +    return error;
 > +}
 > +
 > +static void
 > +cmd_pg_add(struct ctl_context *ctx)
 > +{
 > +    const struct nbrec_port_group *pg;
 > +
 > +    pg = nbrec_port_group_insert(ctx->txn);
 > +    nbrec_port_group_set_name(pg, ctx->argv[1]);
 > +    if (ctx->argc > 2) {
 > +        ctx->error = set_ports_on_pg(ctx, pg, ctx->argv + 2, 
ctx->argc - 2);

 > +    }
 > +}
 > +
 > +static void
 > +cmd_pg_set_ports(struct ctl_context *ctx)
 > +{
 > +    const struct nbrec_port_group *pg;
 > +
 > +    char *error;
 > +    error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, );
 > +    if (error) {
 > +        ctx->error = error;
 > +        return;
 > +    }
 > +
 > +    ctx->error = set_ports_on_pg(ctx, pg, ctx->argv + 2, ctx->argc - 2);
 > +}
 > +
 > +static void
 > +cmd_pg_del(struct ctl_context *ctx)
 > +{
 > +    const struct nbrec_port_group *pg;
 > +
 > +    char *error;
 > +    error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, );
 > +    if (error) {
 > +        ctx->error = error;
 > +        return;
 > +    }
 > +
 > +    nbrec_port_group_delete(pg);
 > +}
 > +
 >  static const struct ctl_table_class tables[NBREC_N_TABLES] = {
 >      [NBREC_TABLE_DHCP_OPTIONS].row_ids
 >      = {{_logical_switch_port_col_name, NULL,
 > @@ -5123,6 +5190,11 @@ static const struct ctl_command_syntax 
nbctl_commands[] = {

 >          "PRIVATE-KEY CERTIFICATE CA-CERT [SSL-PROTOS [SSL-CIPHERS]]",
 >          pre_cmd_set_ssl, cmd_set_ssl, NULL, "--bootstrap", RW},
 >
 > +    /* Port Group Commands */
 > +    {"pg-add", 1, INT_MAX, "", NULL, cmd_pg_add, NULL, "", RW },
 > +    {"pg-set-ports", 2, INT_MAX, "", NULL, cmd_pg_set_ports, NULL, 
"", RW },

 > +    {"pg-del", 1, 1, "", NULL, cmd_pg_del, NULL, "", RW },
 > +
 >      {NULL, 0, 0, NULL, NULL, NULL, NULL, "", RO},
 >  };
 >
 > diff --git a/tests/ovn.at  b/tests/ovn.at 
 > index 769e09f81..81dff3da2 100644
 > --- a/tests/ovn.at 
 > +++ b/tests/ovn.at 
 > @@ -11253,3 +11253,40 @@ AT_CHECK([ovn-nbctl lsp-set-addresses sw2-p1 
"00:00:00:00:00:04 192.168.0.3"])
 >  AT_CHECK([ovn-nbctl lsp-set-addresses sw2-p1 "00:00:00:00:00:04 
aef0::1"])

 >
 >  AT_CLEANUP
 > +
 > +AT_SETUP([ovn -- ovn-nbctl port group commands])
 > +ovn_start
 > +
 > +ovn-nbctl pg-add pg1
 > +AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],
 > +[pg1
 > +])
 > +
 > +ovn-nbctl pg-del pg1
 > +AT_CHECK([ovn-nbctl list port_group], [0], [])
 > +
 > +ovn-nbctl ls-add sw1
 > +ovn-nbctl lsp-add sw1 sw1-p1
 > 

Re: [ovs-dev] [PATCH v2] ovn: Fix IPv6 DAD failure for container ports

[Guru Shetty]

On Fri, 5 Oct 2018 at 07:35,  wrote:

> From: Numan Siddique 
>
> When a container port is created inside a VM, the below kernel message
> is seen and IPv6 doesn't work on that interface.
>
> [  138.000753] IPv6: vlan4: IPv6 duplicate address  detected!
>
> When a container port sends a ethernet broadcast packet, OVN delivers the
> same
> packet back to the child port (and hence the DAD check fails).
>
> This is because
>  - 'MLF_ALLOW_LOOPBACK_BIT' is set in REG10 in table 0 for the packets
> received
>from any child port.
>  - for ethernet broadcast packets, Table 33 (OFTABLE_LOCAL_OUTPUT) clones
> the
>packet for every local port 'P' which belongs to the same datapath i.e
>'P'->REG15, resubmit(,34)
>  - If REG14 and REG15 are same, Table 34 (OFTABLE_CHECK_LOOPBACK) drops
> the packet
>if 'MLF_ALLOW_LOOPBACK_BIT' is not set.
>  - But in the case of container ports, this bit will be set and hence
> doesn't gets
>dropped and eventually gets delivered to the source container port.
>  - The VM's kernel thinks its a DAD packet. The latest kernels (4.19)
> implements
>the RFC -7527 (enhanced DAD), but it is still a problem for older
> kernels.
>
> This patch fixes the issue by using a new register bit
> (MLF_NESTED_CONTAINER_BIT)
> instead of 'MLF_ALLOW_LOOPBACK_BIT' and sets it in REG10 for the packets
> received
> from child ports so that Table 34 drops the packet for the source port.
>
> Signed-off-by: Numan Siddique 
>

Thank you for the detailed comments in v2. Super helpful. One minor nit
below.
Acked-by: Gurucharan Shetty 


> ---
>
> v1 -> v2
> ---
>   * Addressed the review comments from Guru - Updated the commit message
> and added few comments in the code.
>
>  ovn/controller/ofctrl.c   | 29 +++--
>  ovn/controller/ofctrl.h   |  5 +++
>  ovn/controller/physical.c | 65 ++-
>  ovn/lib/logical-fields.h  |  4 +++
>  tests/ovn.at  | 11 +++
>  5 files changed, 97 insertions(+), 17 deletions(-)
>
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index f5b53195d..218612787 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -630,9 +630,11 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype
> type)
>   *
>   * The caller should initialize its own hmap to hold the flows. */
>  void
> -ofctrl_add_flow(struct hmap *desired_flows,
> -uint8_t table_id, uint16_t priority, uint64_t cookie,
> -const struct match *match, const struct ofpbuf *actions)
> +ofctrl_check_and_add_flow(struct hmap *desired_flows,
> +  uint8_t table_id, uint16_t priority, uint64_t
> cookie,
> +  const struct match *match,
> +  const struct ofpbuf *actions,
> +  bool log_duplicate_flow)
>  {
>  struct ovn_flow *f = xmalloc(sizeof *f);
>  f->table_id = table_id;
> @@ -644,13 +646,14 @@ ofctrl_add_flow(struct hmap *desired_flows,
>  f->cookie = cookie;
>
>  if (ovn_flow_lookup(desired_flows, f)) {
> -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> -if (!VLOG_DROP_DBG()) {
> -char *s = ovn_flow_to_string(f);
> -VLOG_DBG("dropping duplicate flow: %s", s);
> -free(s);
> +if (log_duplicate_flow) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> +if (!VLOG_DROP_DBG()) {
> +char *s = ovn_flow_to_string(f);
> +VLOG_DBG("dropping duplicate flow: %s", s);
> +free(s);
> +}
>  }
> -
>  ovn_flow_destroy(f);
>  return;
>  }
> @@ -658,6 +661,14 @@ ofctrl_add_flow(struct hmap *desired_flows,
>  hmap_insert(desired_flows, >hmap_node, f->hmap_node.hash);
>  }
>
> +void
> +ofctrl_add_flow(struct hmap *desired_flows,
> +uint8_t table_id, uint16_t priority, uint64_t cookie,
> +const struct match *match, const struct ofpbuf *actions)
> +{
> +ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
> +  match, actions, true);
> +}
>
>  /* ovn_flow. */
>
> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> index e3fc95b05..ae0cfa513 100644
> --- a/ovn/controller/ofctrl.h
> +++ b/ovn/controller/ofctrl.h
> @@ -55,4 +55,9 @@ void ofctrl_add_flow(struct hmap *desired_flows, uint8_t
> table_id,
>   uint16_t priority, uint64_t cookie,
>   const struct match *, const struct ofpbuf *ofpacts);
>
> +void ofctrl_check_and_add_flow(struct hmap *desired_flows, uint8_t
> table_id,
> +   uint16_t priority, uint64_t cookie,
> +   const struct match *,
> +   const struct ofpbuf *ofpacts,
> +   bool log_duplicate_flow);
>  #endif /* ovn/ofctrl.h */
> diff 

Re: [ovs-dev] [PATCH v5 11/20] ovn-controller: incremental processing for multicast group changes

[Han Zhou]

On Mon, Aug 13, 2018 at 10:48 AM Han Zhou  wrote:
>
> This patch handles SB Multicast_Group table changes incrementally.
> The Multicast_Group table changes can be triggered by creating/deleting
> a lport of a lswitch. It can be also triggered indirectly by an
> updating of a port-binding which is referenced by the multicast
> group.
>
> This patch together with previous incremental processing engine
> related changes supports incremental processing for lflow changes
> and port-binding changes of lports on other HVs, which are the most
> common scenarios in a cloud where workloads come up and down.
>
> In ovn-scale-test env [1], the total execution time of creating and
> binding 10k ports on 1k HVs with 40 lswitches and 8 lrouters
> (5 lswitches/lrouter), decreased from 3h40m to 1h50m because of the
> less CPU on HVs. The CPU time of ovn-controller for additional 500
> lports creating and binding (on top of already existed 10k lports)
> decreased 90% comparing with master.
>
> Latency for end-to-end operations of one extra port on top of the
> 10k lports, start from port-creation until all flows installation
> on all related HVs is also improved significantly:
>
> before: 20.6s in total
> - lsp-add: 0.4s
> - wait-until port up=true: 4.8s
> - --wait=hv sync: 15.4s
>
> after: 7.3s in total
> - lsp-add: 0.4s
> - wait-until port up=true: 4.0s
> - --wait=hv sync: 2.9s
>
> [1] https://github.com/openvswitch/ovn-scale-test
>
> Signed-off-by: Han Zhou 
> ---
>  lib/ovsdb-idl.c |  4 +++-
>  ovn/controller/ovn-controller.c | 52
-
>  ovn/controller/physical.c   | 23 ++
>  ovn/controller/physical.h   |  7 ++
>  4 files changed, 84 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 468b3bf..784ae4e 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -2308,6 +2308,9 @@ add_tracked_change_for_references(struct
ovsdb_idl_row *row)
>  ovsdb_idl_track_is_set(row->table)) {
>  ovs_list_push_back(>table->track_list,
> >track_node);
> +row->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
> += row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
> += row->table->db->change_seqno + 1;
>

I just noticed that this part of the patch should have been split and
combined with a previous patch in this series.

[PATCH v5 04/20] ovsdb-idl.c: Track changes for table references.

Now that the previous patch has been merged, I sent a patch to add this
change:
https://mail.openvswitch.org/pipermail/ovs-dev/2018-October/352752.html

Without this new patch, the previous one doesn't break anything, but the
feature is incomplete.

>  const struct ovsdb_idl_arc *arc;
>  LIST_FOR_EACH (arc, dst_node, >dst_arcs) {
> @@ -2316,7 +2319,6 @@ add_tracked_change_for_references(struct
ovsdb_idl_row *row)
>  }
>  }
>
> -
>  /* Returns true if a column with mode OVSDB_IDL_MODE_RW changed, false
>   * otherwise.
>   *
> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index 9fd2cba..4399d6e 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -1180,6 +1180,56 @@ flow_output_sb_port_binding_handler(struct
engine_node *node)
>  return true;
>  }
>
> +static bool
> +flow_output_sb_multicast_group_handler(struct engine_node *node)
> +{
> +struct ed_type_runtime_data *data =
> +(struct ed_type_runtime_data *)engine_get_input(
> +"runtime_data", node)->data;
> +struct hmap *local_datapaths = >local_datapaths;
> +struct simap *ct_zones = >ct_zones;
> +
> +struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve =
> +(struct ed_type_mff_ovn_geneve *)engine_get_input(
> +"mff_ovn_geneve", node)->data;
> +enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
> +
> +struct ovsrec_open_vswitch_table *ovs_table =
> +(struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> +engine_get_input("OVS_open_vswitch", node));
> +struct ovsrec_bridge_table *bridge_table =
> +(struct ovsrec_bridge_table *)EN_OVSDB_GET(
> +engine_get_input("OVS_bridge", node));
> +const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
ovs_table);
> +const char *chassis_id = get_chassis_id(ovs_table);
> +
> +struct ovsdb_idl_index *sbrec_chassis_by_name =
> +engine_ovsdb_node_get_index(
> +engine_get_input("SB_chassis", node),
> +"name");
> +const struct sbrec_chassis *chassis = NULL;
> +if (chassis_id) {
> +chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
chassis_id);
> +}
> +ovs_assert(br_int && chassis);
> +
> +struct ed_type_flow_output *fo =
> +(struct ed_type_flow_output *)node->data;
> +struct ovn_desired_flow_table *flow_table = >flow_table;
> +
> +struct sbrec_multicast_group_table 

[ovs-dev] Zyxel Users list

[Michelle Stern]

Hello there,

I would like to know if you are interested in acquiring Zyxel Users list.

Information fields: Names, Title, Email, Phone, Company Name, Company URL, 
Company physical address, SIC Code, Industry, Company Size (Revenue and 
Employee), LinkedIn profile link and kind of technology using/solution in place.

If this is not relevant to you please get back to me with your Target Market, 
we cater to all types of target market available.

Let me know your thoughts on this and I will get back with more information for 
your review.

Await your response!
Regards,
Michelle Stern
Demand Generation Manager

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

[ovs-dev] [PATCH] ovsdb-idl.c: Increase seqno for change-tracking of table references.

[Han Zhou]

From: Han Zhou 

This is an enhancement for commit:
102781c ovsdb-idl: Track changes for table references.

The seqno change is needed so that the change-tracking helper function
..._is_new() can work properly.

Signed-off-by: Han Zhou 
---
 lib/ovsdb-idl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 6a19b07..9f44cef 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2359,6 +2359,9 @@ add_tracked_change_for_references(struct ovsdb_idl_row 
*row)
 ovsdb_idl_track_is_set(row->table)) {
 ovs_list_push_back(>table->track_list,
>track_node);
+row->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
+= row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
+= row->table->db->change_seqno + 1;
 
 const struct ovsdb_idl_arc *arc;
 LIST_FOR_EACH (arc, dst_node, >dst_arcs) {
-- 
2.1.0

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

[ovs-dev] [PATCH 2/2] ovn: Support a new Logical_Switch_Port.type - 'external'

[nusiddiq]

From: Numan Siddique 

In the case of OpenStack + OVN, when the VMs are booted on
hypervisors supporting SR-IOV nics, there are no OVS ports
for these VMs. When these VMs sends DHCPv4, DHPCv6 or IPv6
Router Solicitation requests, the local ovn-controller
cannot reply to these packets. OpenStack Neutron dhcp agent
service needs to be run to serve these requests.

With the new logical port type - 'external', OVN itself can
handle these requests avoiding the need to deploy any
external services like neutron dhcp agent.

To make use of this feature, CMS has to
 - create a logical port for such VMs
 - set the type to 'external'
 - set requested-chassis="" in the options
   column.
 - create a localnet port for the logical switch
 - configure the ovn-bridge-mappings option in the OVS db.

When the ovn-controller running in that 'chassis', detects
the Port_Binding row, it adds the necessary DHCPv4/v6 OF
flows. Since the packet enters the logical switch pipeline
via the localnet port, the inport register (reg14) is set
to the tunnel key of localnet port in the match conditions.

In case the chassis goes down for some reason, it is the
responsibility of CMS to change the 'requested-chassis'
option to some other active chassis, so that it can serve
these requests.

When the VM with the external port, sends an ARP request for
the router ips, only the chassis which has claimed the port,
will reply to the ARP requests. Rest of the chassis on
receiving these packets drop them in the ingress switch
datapath stage - S_SWITCH_IN_EXTERNAL_PORT which is just
before S_SWITCH_IN_L2_LKUP.

This would guarantee that only the chassis which has claimed
the external ports will run the router datapath pipeline.

Signed-off-by: Numan Siddique 
---
 ovn/controller/binding.c|  15 +-
 ovn/controller/lflow.c  |  41 ++-
 ovn/controller/lflow.h  |   2 +
 ovn/controller/lport.c  |  26 ++
 ovn/controller/lport.h  |   5 +
 ovn/controller/ovn-controller.c |   6 +
 ovn/lib/ovn-util.c  |   1 +
 ovn/northd/ovn-northd.8.xml |  52 +++-
 ovn/northd/ovn-northd.c | 123 ++--
 ovn/ovn-architecture.7.xml  |  66 
 ovn/ovn-nb.xml  |  33 ++
 tests/ovn.at| 530 +++-
 12 files changed, 868 insertions(+), 32 deletions(-)

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 021ecddcf..ee396c93d 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -471,13 +471,26 @@ consider_local_datapath(struct ovsdb_idl_txn 
*ovnsb_idl_txn,
  * for them. */
 sset_add(local_lports, binding_rec->logical_port);
 our_chassis = false;
+} else if (!strcmp(binding_rec->type, "external")) {
+const char *chassis_id = smap_get(_rec->options,
+  "requested-chassis");
+our_chassis = chassis_id && (
+!strcmp(chassis_id, chassis_rec->name) ||
+!strcmp(chassis_id, chassis_rec->hostname));
+if (our_chassis) {
+add_local_datapath(sbrec_datapath_binding_by_key,
+   sbrec_port_binding_by_datapath,
+   sbrec_port_binding_by_name,
+   binding_rec->datapath, true, local_datapaths);
+}
 }
 
 if (our_chassis
 || !strcmp(binding_rec->type, "patch")
 || !strcmp(binding_rec->type, "localport")
 || !strcmp(binding_rec->type, "vtep")
-|| !strcmp(binding_rec->type, "localnet")) {
+|| !strcmp(binding_rec->type, "localnet")
+|| !strcmp(binding_rec->type, "external")) {
 update_local_lport_ids(local_lport_ids, binding_rec);
 }
 
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 8db81927e..98e8ed3b9 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -52,7 +52,10 @@ lflow_init(void)
 struct lookup_port_aux {
 struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
 struct ovsdb_idl_index *sbrec_port_binding_by_name;
+struct ovsdb_idl_index *sbrec_port_binding_by_type;
+struct ovsdb_idl_index *sbrec_datapath_binding_by_key;
 const struct sbrec_datapath_binding *dp;
+const struct sbrec_chassis *chassis;
 };
 
 struct condition_aux {
@@ -66,6 +69,8 @@ static void consider_logical_flow(
 struct ovsdb_idl_index *sbrec_chassis_by_name,
 struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
 struct ovsdb_idl_index *sbrec_port_binding_by_name,
+struct ovsdb_idl_index *sbrec_port_binding_by_type,
+struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
 const struct sbrec_logical_flow *,
 const struct hmap *local_datapaths,
 const struct sbrec_chassis *,
@@ -89,8 +94,24 @@ lookup_port_cb(const void *aux_, const char *port_name, 
unsigned int *portp)
 const struct sbrec_port_binding *pb
 = lport_lookup_by_name(aux->sbrec_port_binding_by_name, 

[ovs-dev] [PATCH 1/2] ovn: Avoid tunneling for VLAN packets redirected to a gateway chassis

[nusiddiq]

From: Numan Siddique 

An OVN deployment can have multiple logical switches each with a
localnet port connected to a distributed logical router with one
logical router port providing external connectivity (provider network)
and others used as tenant networks with VLAN tagging.

As reported in [1], external traffic from these VLAN tenant networks
are tunnelled to the gateway chassis (chassis hosting a distributed
gateway port which applies NAT rules). As part of the discussion in
[1], there were few possible solutions proposed by Russell [2]. This
patch implements the first option in [2].

With this patch, a new option 'reside-on-redirect-chassis' in 'options'
column of Logical_Router_Port table is added. If the value of this
option is set to 'true' and if the logical router also have a
distributed gateway port, then routing for this logical router port
is centralized in the chassis hosting the distributed gateway port.

If a logical switch 'sw0' is connected to a router 'lr0' with the
router port - 'lr0-sw0' with the address - "00:00:00:00:af:12 192.168.1.1"
, and it has a distributed logical port - 'lr0-public', then the
below logical flow is added in the logical switch pipeline
of 'sw0' if the 'reside-on-redirect-chassis' option is set on 'lr-sw0' -

table=16(ls_in_l2_lkup), priority=50,
match=(eth.dst == 00:00:00:00:af:12 && is_chassis_resident("cr-lr0-public")),
action=(outport = "sw0-lr0"; output;)

With the above flow, the packet doesn't enter the router pipeline in
the source chassis. Instead the packet is sent out via the localnet
port of 'sw0'. The gateway chassis upon receiving this packet, runs
the logical router pipeline applying NAT rules and sends the traffic
out via the localnet port of the provider network. The gateway
chassis will also reply to the ARP requests for the router port IPs.

With this approach, we avoid redirecting the external traffic to the
gateway chassis via the tunnel port. There are a couple of drawbacks
with this approach:

  - East - West routing is no more distributed for the VLAN tenant
networks if 'reside-on-redirect-chassis' option is defined

  - 'dnat_and_snat' NAT rules with 'logical_mac' and 'logical_port'
columns defined will not work for the VLAN tenant networks.

This approach is taken for now as it is simple. If there is a requirement
to support distributed routing for these VLAN tenant networks, we
can explore other possible solutions.

[1] -  https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046543.html
[2] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046557.html

Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046543.html
Reported-by: venkata anil 
Co-authored-by: venkata anil 
Signed-off-by: Numan Siddique 
Signed-off-by: venkata anil 
---
 ovn/northd/ovn-northd.8.xml |  30 
 ovn/northd/ovn-northd.c |  71 +++---
 ovn/ovn-architecture.7.xml  | 160 +
 ovn/ovn-nb.xml  |  43 ++
 tests/ovn.at| 273 
 5 files changed, 561 insertions(+), 16 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 7352c6764..f52699bd3 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -874,6 +874,25 @@ output;
 resident.
   
 
+
+
+  For the Ethernet address on a logical switch port of type
+  router, when that logical switch port's
+   column is set to router and
+  the connected logical router port specifies a
+  reside-on-redirect-chassis and the logical router
+  to which the connected logical router port belongs to has a
+  redirect-chassis distributed gateway logical router
+  port:
+
+
+
+  
+The flow for the connected logical router port's Ethernet
+address is only programmed on the redirect-chassis.
+  
+
   
 
   
@@ -1179,6 +1198,17 @@ output;
   upstream MAC learning to point to the
   redirect-chassis.
 
+
+
+  For the logical router port with the option
+  reside-on-redirect-chassis set (which is centralized),
+  the above flows are only programmed on the gateway port instance on
+  the redirect-chassis (if the logical router has a
+  distributed gateway port). This behavior avoids generation
+  of multiple ARP responses from different chassis, and allows
+  upstream MAC learning to point to the
+  redirect-chassis.
+
   
 
   
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 31ea5f410..3998a898c 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -4426,13 +4426,32 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 ds_put_format(, "eth.dst == "ETH_ADDR_FMT,
   

[ovs-dev] [PATCH 0/2] Addressing VLAN tenant network issues and add

[nusiddiq]

From: Numan Siddique 

Patch 1 addresses the VLAN tenant network issue.

An OVN deployment can have multiple logical switches each with a
localnet port connected to a distributed logical router with one
logical router port providing external connectivity (provider network)
and others used as tenant networks with VLAN tagging.

As reported in [1], external traffic from these VLAN tenant networks
are tunnelled to the gateway chassis (chassis hosting a distributed
gateway port which applies NAT rules). As part of the discussion in
[1], there were few possible solutions proposed by Russell [2]. This
patch implements the first option in [2].

Patch 2 adds a new logical port type - 'external' to support using
of OVN native services like DHCP for VMs which are not bound to any
ovn-controller chassis. The main use case is for SR-IOV VMs.
Patch 2 also addresses the comments from Ben about adding documenation
in ovn-architecture.

I had submitted these patches separately earlier. Now I have made it
as a series and reset the patch version to 1.

Numan Siddique (2):
  ovn: Avoid tunneling for VLAN packets redirected to a gateway chassis
  ovn: Support a new Logical_Switch_Port.type - 'external'

 ovn/controller/binding.c|  15 +-
 ovn/controller/lflow.c  |  41 +-
 ovn/controller/lflow.h  |   2 +
 ovn/controller/lport.c  |  26 ++
 ovn/controller/lport.h  |   5 +
 ovn/controller/ovn-controller.c |   6 +
 ovn/lib/ovn-util.c  |   1 +
 ovn/northd/ovn-northd.8.xml |  82 +++-
 ovn/northd/ovn-northd.c | 194 ++--
 ovn/ovn-architecture.7.xml  | 226 +
 ovn/ovn-nb.xml  |  76 +++
 tests/ovn.at| 803 +++-
 12 files changed, 1429 insertions(+), 48 deletions(-)

-- 
2.17.1

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

[ovs-dev] [PATCH v3] OVN: add buffering support for ip packets

[Lorenzo Bianconi]

Add buffering support for IPv4/IPv6 packets that will be processed
by arp{}/nd_ns{} action when L2 address is not discovered yet since
otherwise the packet will be substituted with an ARP/Neighbor
Solicitation frame and this will result in the lost of the first
packet of the connection.
Moreover fix following automatic tests broken by ip-buffering support
since now original ip packets are transmitted by OVN logical
router:
- ovn -- 3 HVs, 3 LS, 3 lports/LS, 1 LR
- ovn -- /32 router IP address

Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- use in6_addr instead of ovs_be128 as hash key
- fix spelling error
- merge patch 2/3 and 3/3 in 1/3

Changes since v1:
- add automatic test support for IPv4 and IPv6
- fix automatic tests broken by ip-buffering support
- reduce queue depth to 3 packets
- use binary form of the IP address as hash key
- in buffered_send_packets() routine signature use eth_addr instead of
  unsigned char pointer
- use unsigned int for {head/tail} queue index
---
 ovn/controller/pinctrl.c | 256 ++-
 tests/ovn.at | 157 +++-
 2 files changed, 374 insertions(+), 39 deletions(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 8ae4c9e52..69a811902 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -61,6 +61,9 @@ static struct rconn *swconn;
  * rconn_get_connection_seqno(rconn), 'swconn' has reconnected. */
 static unsigned int conn_seq_no;
 
+static void init_buffered_packets_map(void);
+static void destroy_buffered_packets_map(void);
+
 static void pinctrl_handle_put_mac_binding(const struct flow *md,
const struct flow *headers,
bool is_arp);
@@ -96,6 +99,7 @@ static void pinctrl_handle_put_nd_ra_opts(
 struct ofputil_packet_in *pin, struct ofpbuf *userdata,
 struct ofpbuf *continuation);
 static void pinctrl_handle_nd_ns(const struct flow *ip_flow,
+ struct dp_packet *pkt_in,
  const struct match *md,
  struct ofpbuf *userdata);
 static void init_ipv6_ras(void);
@@ -108,6 +112,7 @@ static void send_ipv6_ras(
 ;
 
 COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
+COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map);
 
 void
 pinctrl_init(void)
@@ -117,6 +122,7 @@ pinctrl_init(void)
 init_put_mac_bindings();
 init_send_garps();
 init_ipv6_ras();
+init_buffered_packets_map();
 }
 
 static ovs_be32
@@ -190,9 +196,180 @@ set_actions_and_enqueue_msg(const struct dp_packet 
*packet,
 ofpbuf_uninit();
 }
 
+struct buffer_info {
+struct ofpbuf ofpacts;
+struct dp_packet *p;
+};
+
+#define BUFFER_QUEUE_DEPTH 4
+struct buffered_packets {
+struct hmap_node hmap_node;
+
+/* key */
+struct in6_addr ip;
+
+long long int timestamp;
+
+struct buffer_info data[BUFFER_QUEUE_DEPTH];
+uint32_t head, tail;
+};
+
+static struct hmap buffered_packets_map;
+
+static void
+init_buffered_packets_map(void)
+{
+hmap_init(_packets_map);
+}
+
+static void
+destroy_buffered_packets(struct buffered_packets *bp)
+{
+struct buffer_info *bi;
+
+while (bp->head != bp->tail) {
+bi = >data[bp->head];
+dp_packet_uninit(bi->p);
+ofpbuf_uninit(>ofpacts);
+
+bp->head = (bp->head + 1) % BUFFER_QUEUE_DEPTH;
+}
+hmap_remove(_packets_map, >hmap_node);
+free(bp);
+}
+
+static void
+destroy_buffered_packets_map(void)
+{
+struct buffered_packets *bp;
+HMAP_FOR_EACH_POP (bp, hmap_node, _packets_map) {
+destroy_buffered_packets(bp);
+}
+hmap_destroy(_packets_map);
+}
+
+static void
+buffered_push_packet(struct buffered_packets *bp,
+ struct dp_packet *packet,
+ const struct match *md)
+{
+uint32_t next = (bp->tail + 1) % BUFFER_QUEUE_DEPTH;
+struct buffer_info *bi = >data[bp->tail];
+
+ofpbuf_init(>ofpacts, 4096);
+
+reload_metadata(>ofpacts, md);
+struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(>ofpacts);
+resubmit->in_port = OFPP_CONTROLLER;
+resubmit->table_id = OFTABLE_REMOTE_OUTPUT;
+
+bi->p = packet;
+
+if (next == bp->head) {
+bi = >data[bp->head];
+dp_packet_uninit(bi->p);
+ofpbuf_uninit(>ofpacts);
+bp->head = (bp->head + 1) % BUFFER_QUEUE_DEPTH;
+}
+bp->tail = next;
+}
+
+static void
+buffered_send_packets(struct buffered_packets *bp, struct eth_addr *addr)
+{
+enum ofp_version version = rconn_get_version(swconn);
+enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
+
+while (bp->head != bp->tail) {
+struct buffer_info *bi = >data[bp->head];
+struct eth_header *eth = dp_packet_data(bi->p);
+
+eth->eth_dst = *addr;
+struct ofputil_packet_out po = {
+.packet = dp_packet_data(bi->p),
+

[ovs-dev] [PATCH v2 2/2] pcap-file: Add nanosecond resolution pcap support.

[Mark Michelson]

PCAP header magic numbers are different for microsecond and nanosecond
resolution timestamps. This patch adds support for understanding the
difference and reporting the time correctly with ovs_pcap_read().

When writing pcap files, OVS will always use microsecond resolution, so
no new calculations were added to those functions.

Signed-off-by: Mark Michelson 
---
 lib/netdev-dummy.c |  12 +++---
 lib/pcap-file.c| 102 ++---
 lib/pcap-file.h|  14 ---
 tests/test-conntrack.c |   4 +-
 tests/test-flows.c |  11 ++
 utilities/ovs-ofctl.c  |  20 +-
 6 files changed, 102 insertions(+), 61 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 2cf05634e..6d0b2e2d8 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -118,7 +118,7 @@ struct netdev_dummy {
 
 struct dummy_packet_conn conn OVS_GUARDED;
 
-FILE *tx_pcap, *rxq_pcap OVS_GUARDED;
+struct pcap_file *tx_pcap, *rxq_pcap OVS_GUARDED;
 
 struct in_addr address, netmask;
 struct in6_addr ipv6, ipv6_mask;
@@ -719,10 +719,10 @@ netdev_dummy_destruct(struct netdev *netdev_)
 
 ovs_mutex_lock(>mutex);
 if (netdev->rxq_pcap) {
-fclose(netdev->rxq_pcap);
+ovs_pcap_close(netdev->rxq_pcap);
 }
 if (netdev->tx_pcap && netdev->tx_pcap != netdev->rxq_pcap) {
-fclose(netdev->tx_pcap);
+ovs_pcap_close(netdev->tx_pcap);
 }
 dummy_packet_conn_close(>conn);
 netdev->conn.type = NONE;
@@ -859,10 +859,10 @@ netdev_dummy_set_config(struct netdev *netdev_, const 
struct smap *args,
 dummy_packet_conn_set_config(>conn, args);
 
 if (netdev->rxq_pcap) {
-fclose(netdev->rxq_pcap);
+ovs_pcap_close(netdev->rxq_pcap);
 }
 if (netdev->tx_pcap && netdev->tx_pcap != netdev->rxq_pcap) {
-fclose(netdev->tx_pcap);
+ovs_pcap_close(netdev->tx_pcap);
 }
 netdev->rxq_pcap = netdev->tx_pcap = NULL;
 pcap = smap_get(args, "pcap");
@@ -1144,7 +1144,6 @@ netdev_dummy_send(struct netdev *netdev, int qid 
OVS_UNUSED,
 
 dp_packet_use_const(, buffer, size);
 ovs_pcap_write(dev->tx_pcap, );
-fflush(dev->tx_pcap);
 }
 
 ovs_mutex_unlock(>mutex);
@@ -1529,7 +1528,6 @@ netdev_dummy_queue_packet(struct netdev_dummy *dummy, 
struct dp_packet *packet,
 
 if (dummy->rxq_pcap) {
 ovs_pcap_write(dummy->rxq_pcap, packet);
-fflush(dummy->rxq_pcap);
 }
 prev = NULL;
 LIST_FOR_EACH (rx, node, >rxes) {
diff --git a/lib/pcap-file.c b/lib/pcap-file.c
index ea5cfa3b2..81a094c2a 100644
--- a/lib/pcap-file.c
+++ b/lib/pcap-file.c
@@ -34,6 +34,16 @@
 
 VLOG_DEFINE_THIS_MODULE(pcap);
 
+enum ts_resolution {
+PCAP_USEC,
+PCAP_NSEC,
+};
+
+struct pcap_file {
+FILE *file;
+enum ts_resolution resolution;
+};
+
 struct pcap_hdr {
 uint32_t magic_number;   /* magic number */
 uint16_t version_major;  /* major version number */
@@ -47,25 +57,27 @@ BUILD_ASSERT_DECL(sizeof(struct pcap_hdr) == 24);
 
 struct pcaprec_hdr {
 uint32_t ts_sec; /* timestamp seconds */
-uint32_t ts_usec;/* timestamp microseconds */
+uint32_t ts_subsec;  /* timestamp subseconds */
 uint32_t incl_len;   /* number of octets of packet saved in file */
 uint32_t orig_len;   /* actual length of packet */
 };
 BUILD_ASSERT_DECL(sizeof(struct pcaprec_hdr) == 16);
 
-FILE *
+struct pcap_file *
 ovs_pcap_open(const char *file_name, const char *mode)
 {
 struct stat s;
-FILE *file;
+struct pcap_file *p_file;
 int error;
 
 ovs_assert(!strcmp(mode, "rb") ||
!strcmp(mode, "wb") ||
!strcmp(mode, "ab"));
 
-file = fopen(file_name, mode);
-if (file == NULL) {
+p_file = xmalloc(sizeof *p_file);
+p_file->file = fopen(file_name, mode);
+p_file->resolution = PCAP_USEC;
+if (p_file->file == NULL) {
 VLOG_WARN("%s: failed to open pcap file for %s (%s)", file_name,
   (mode[0] == 'r' ? "reading"
: mode[0] == 'w' ? "writing"
@@ -76,49 +88,64 @@ ovs_pcap_open(const char *file_name, const char *mode)
 
 switch (mode[0]) {
 case 'r':
-error = ovs_pcap_read_header(file);
+error = ovs_pcap_read_header(p_file);
 if (error) {
 errno = error;
-fclose(file);
+ovs_pcap_close(p_file);
 return NULL;
 }
 break;
 
 case 'w':
-ovs_pcap_write_header(file);
+ovs_pcap_write_header(p_file);
 break;
 
 case 'a':
-if (!fstat(fileno(file), ) && !s.st_size) {
-ovs_pcap_write_header(file);
+if (!fstat(fileno(p_file->file), ) && !s.st_size) {
+ovs_pcap_write_header(p_file);
 }
 break;
 
 default:
 OVS_NOT_REACHED();
 }
-return file;
+
+return p_file;
+}
+
+struct pcap_file *

[ovs-dev] [PATCH v2 1/2] ovs-pcap: Support nanosecond resolution pcap files.

[Mark Michelson]

pcap files with nanosecond resolution use a different magic number in
the pcap header than those with microsecond resolution.

Signed-off-by: Mark Michelson 
---
 utilities/ovs-pcap.in | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/utilities/ovs-pcap.in b/utilities/ovs-pcap.in
index fc1532110..7bebc0795 100755
--- a/utilities/ovs-pcap.in
+++ b/utilities/ovs-pcap.in
@@ -34,13 +34,14 @@ class PcapReader(object):
 raise PcapException("end of file reading pcap header")
 magic, version, thiszone, sigfigs, snaplen, network = \
 struct.unpack(">6I", header)
-if magic == 0xa1b2c3d4:
+if magic == 0xa1b2c3d4 or magic == 0xa1b23c4d:
 self.header_format = ">4I"
-elif magic == 0xd4c3b2a1:
+elif magic == 0xd4c3b2a1 or magic == 0x4d3cb2a1:
 self.header_format = "<4I"
 else:
 raise PcapException("bad magic %u reading pcap file "
-"(expected 0xa1b2c3d4 or 0xd4c3b2a1)" % magic)
+"(expected 0xa1b2c3d4, 0xa1b23c4d, 0x4d3cb2a1 or "
+"0xd4c3b2a1)" % magic)
 
 def read(self):
 header = self.file.read(16)
-- 
2.14.4

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

[ovs-dev] [PATCH v2 0/2] Add nanosecond resolution PCAP support

[Mark Michelson]

PCAP files can support nanosecond resolution instead of microsecond
resolution in timestamps. It indicates this is in effect in the header
of the file in the magic number. Without these patches we'll think that
the pcap file is invalid if it is using nanosecond resolution.

Mark Michelson (2):
  ovs-pcap: Support nanosecond resolution pcap files.
  pcap-file: Add nanosecond resolution pcap support.

 lib/netdev-dummy.c |  12 +++---
 lib/pcap-file.c| 102 ++---
 lib/pcap-file.h|  14 ---
 tests/test-conntrack.c |   4 +-
 tests/test-flows.c |  11 ++
 utilities/ovs-ofctl.c  |  20 +-
 utilities/ovs-pcap.in  |   7 ++--
 7 files changed, 106 insertions(+), 64 deletions(-)

-- 
2.14.4

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

Re: [ovs-dev] ossfuzz: Speed up flow extract fuzzing by refactoring

[Bhargava Shastry]

Hello,

I can take a look at these on Monday. In any case, they look easily fixable.

P.S. I wonder why the build bot did not complain on the exact same code that 
has now been refactored out of flow_extract_target.c. As far as I can remember, 
that code also used asserts.

On October 5, 2018 2:56:39 PM GMT+02:00, 0-day Robot  wrote:
>Bleep bloop.  Greetings Bhargava Shastry, I am a robot and I have tried
>out your patch.
>Thanks for your contribution.
>
>I encountered some error that I wasn't expecting.  See the details
>below.
>
>
>checkpatch:
>WARNING: Line lacks whitespace around operator
>#585 FILE: tests/oss-fuzz/miniflow_target.c:95:
>flow_u32[*idxp + i] = random_value();
>
>ERROR: Use ovs_assert() in place of assert()
>#722 FILE: tests/oss-fuzz/miniflow_target.c:232:
>assert(miniflow_get_vid(miniflow, i) ==
>
>ERROR: Use ovs_assert() in place of assert()
>#726 FILE: tests/oss-fuzz/miniflow_target.c:236:
>assert(miniflow_get(miniflow, i) == flow_u64[i]);
>
>ERROR: Use ovs_assert() in place of assert()
>#730 FILE: tests/oss-fuzz/miniflow_target.c:240:
>assert(miniflow_equal(miniflow, miniflow));
>
>ERROR: Use ovs_assert() in place of assert()
>#734 FILE: tests/oss-fuzz/miniflow_target.c:244:
>assert(flow_equal(flow, ));
>
>ERROR: Use ovs_assert() in place of assert()
>#737 FILE: tests/oss-fuzz/miniflow_target.c:247:
>assert(miniflow_equal(miniflow, miniflow2));
>
>ERROR: Use ovs_assert() in place of assert()
>#738 FILE: tests/oss-fuzz/miniflow_target.c:248:
> assert(miniflow_hash__(miniflow, 0) == miniflow_hash__(miniflow2, 0));
>
>ERROR: Use ovs_assert() in place of assert()
>#740 FILE: tests/oss-fuzz/miniflow_target.c:250:
>assert(flow_equal(flow, ));
>
>ERROR: Use ovs_assert() in place of assert()
>#748 FILE: tests/oss-fuzz/miniflow_target.c:258:
>assert(minimask_is_catchall(minimask)
>
>ERROR: Use ovs_assert() in place of assert()
>#750 FILE: tests/oss-fuzz/miniflow_target.c:260:
>assert(miniflow_equal_in_minimask(miniflow, miniflow2, minimask));
>
>ERROR: Use ovs_assert() in place of assert()
>#751 FILE: tests/oss-fuzz/miniflow_target.c:261:
>   assert(miniflow_equal_flow_in_minimask(miniflow, , minimask));
>
>ERROR: Use ovs_assert() in place of assert()
>#752 FILE: tests/oss-fuzz/miniflow_target.c:262:
>assert(miniflow_hash_in_minimask(miniflow, minimask, 0x12345678) ==
>
>ERROR: Use ovs_assert() in place of assert()
>#754 FILE: tests/oss-fuzz/miniflow_target.c:264:
>assert(minimask_hash(minimask, 0) ==
>
>ERROR: Use ovs_assert() in place of assert()
>#760 FILE: tests/oss-fuzz/miniflow_target.c:270:
>  assert(!miniflow_equal_flow_in_minimask(miniflow, , minimask));
>
>ERROR: Use ovs_assert() in place of assert()
>#762 FILE: tests/oss-fuzz/miniflow_target.c:272:
>assert(!miniflow_equal_in_minimask(miniflow, miniflow3, minimask));
>
>ERROR: Use ovs_assert() in place of assert()
>#778 FILE: tests/oss-fuzz/miniflow_target.c:288:
>assert(minimask_is_catchall(minicatchall));
>
>ERROR: Use ovs_assert() in place of assert()
>#785 FILE: tests/oss-fuzz/miniflow_target.c:295:
>assert(!minimask_has_extra(minimask, minimask));
>
>ERROR: Use ovs_assert() in place of assert()
>#786 FILE: tests/oss-fuzz/miniflow_target.c:296:
>assert(minimask_has_extra(minicatchall, minimask)
>
>ERROR: Use ovs_assert() in place of assert()
>#793 FILE: tests/oss-fuzz/miniflow_target.c:303:
>assert(minimask_has_extra(minimask2, minimask));
>
>ERROR: Use ovs_assert() in place of assert()
>#794 FILE: tests/oss-fuzz/miniflow_target.c:304:
>assert(!minimask_has_extra(minimask, minimask2));
>
>ERROR: Use ovs_assert() in place of assert()
>#810 FILE: tests/oss-fuzz/miniflow_target.c:320:
>assert(minimask_is_catchall(minicatchall));
>
>ERROR: Use ovs_assert() in place of assert()
>#824 FILE: tests/oss-fuzz/miniflow_target.c:334:
>assert(minimask_is_catchall());
>
>ERROR: Use ovs_assert() in place of assert()
>#833 FILE: tests/oss-fuzz/miniflow_target.c:343:
>assert(flow_wildcards_equal(, ));
>
>Lines checked: 861, Warnings: 1, Errors: 22
>
>
>Please check this out.  If you feel there has been an error, please
>email acon...@bytheb.org
>
>Thanks,
>0-day Robot

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v10 05/14] dp-packet: Fix data_len handling multi-seg mbufs.

[Flavio Leitner]

On Fri, Oct 05, 2018 at 03:50:46PM +0100, Lam, Tiago wrote:
> On 03/10/2018 19:26, Flavio Leitner wrote:
> > On Fri, Sep 28, 2018 at 05:15:06PM +0100, Tiago Lam wrote:
> >> When a dp_packet is from a DPDK source, and it contains multi-segment
> >> mbufs, the data_len is not equal to the packet size, pkt_len. Instead,
> >> the data_len of each mbuf in the chain should be considered while
> >> distributing the new (provided) size.
> >>
> >> To account for the above dp_packet_set_size() has been changed so that,
> >> in the multi-segment mbufs case, only the data_len on the last mbuf of
> >> the chain and the total size of the packet, pkt_len, are changed. The
> >> data_len on the intermediate mbufs preceeding the last mbuf is not
> >> changed by dp_packet_set_size(). Furthermore, in some cases
> >> dp_packet_set_size() may be used to set a smaller size than the current
> >> packet size, thus effectively trimming the end of the packet. In the
> >> multi-segment mbufs case this may lead to lingering mbufs that may need
> >> freeing.
> >>
> >> __dp_packet_set_data() now also updates an mbufs' data_len after setting
> >> the data offset. This is so that both fields are always in sync for each
> >> mbuf in a chain.
> > 
> > I think we need an assert(source) to make sure dp_packet_shift() will
> > never be called for a multi-seg.
> 
> I guess you made this comment before seeing patch 07/14 where a
> dp_packet_shift() implementation is introduced for DPDK packets? But
> nonetheless, it has been mentioned before by Darrell that it might not
> be worth implementing. Given that the only users for now are in
> lib/pcap-file.c and lib/ovs-ofctl.c, where packets are always of
> DPBUF_MALLOC type, I guess it could make sense to drop the
> implementation. It would certainly help reduce the footprint of this
> patchset for now. If a use case comes for that later on we can always
> introduce it then.

Sure, my point is that it doesn't expect anything other than
DPBUF_MALLOC type, so adding an assert() would be helpful to
catch bugs, that's all.

Thanks
fbl

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

Re: [ovs-dev] [PATCH v2 0/3] add buffering support for IP packets

[Lorenzo Bianconi]

> On Tue, Oct 02, 2018 at 03:59:11PM +0200, Lorenzo Bianconi wrote:
> > Add buffering support for IP traffic that will be processed
> > by neighboring subsystem (arp{} or nd_ns{} actions) since this
> > will result in the lost of the first packet of the connection
> 
> Thanks for the revision.
> 
> The third patch seems like it should be folded into one of the others
> (possibly the first?) because it's not good to knowingly add a patch
> that breaks tests.  Instead, tests should be updated in the same patch
> that would otherwise break them.

ack, agree. Will do in v3

> 
> I'm not sure there's a reason to separate IPv4 and IPv6, either.  So I
> would be inclined to squash these into a single patch.
> 

ack. Will do in v3

> I noticed a spelling error in the name of pinctrl_handle_bufferd_packets
> ("bufferd").
> 
> The customary type for ipv6 addresses is struct in6_addr.  I don't see
> much benefit to using ovs_be128.  They are the same size but ovs_be128
> doesn't have the same connotation.

ack

> 
> I don't see much benefit to storing and hashing a string version of the
> ipv6 address.  It's only needed in one place and we can generate it on
> the fly there.
> 
> I'm appending an incremental that could be applied to the squashed
> series.  Please let me know what you think of it.  I have tested that it
> passes the unit tests.

Thanks, it works properly :)
I am sending v3 addressing your comments and adding your changes.

Regards,
Lorenzo

> 
> Thanks a lot!
> 
> --8<--cut here-->8--
> 
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 325f924477a3..69a811902fc0 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -206,7 +206,7 @@ struct buffered_packets {
>  struct hmap_node hmap_node;
>  
>  /* key */
> -ovs_be128 ip;
> +struct in6_addr ip;
>  
>  long long int timestamp;
>  
> @@ -317,13 +317,13 @@ buffered_packets_map_gc(void)
>  }
>  
>  static struct buffered_packets *
> -pinctrl_find_buffered_packets(const ovs_be128 *ip, uint32_t hash)
> +pinctrl_find_buffered_packets(const struct in6_addr *ip, uint32_t hash)
>  {
>  struct buffered_packets *qp;
>  
>  HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash,
>   _packets_map) {
> -if (!memcmp(>ip, ip, sizeof(ovs_be128))) {
> +if (IN6_ARE_ADDR_EQUAL(>ip, ip)) {
>  return qp;
>  }
>  }
> @@ -331,9 +331,9 @@ pinctrl_find_buffered_packets(const ovs_be128 *ip, 
> uint32_t hash)
>  }
>  
>  static int
> -pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
> -   struct dp_packet *pkt_in,
> -   const struct match *md, bool is_arp)
> +pinctrl_handle_buffered_packets(const struct flow *ip_flow,
> +struct dp_packet *pkt_in,
> +const struct match *md, bool is_arp)
>  {
>  struct buffered_packets *bp;
>  struct dp_packet *clone;
> @@ -345,9 +345,8 @@ pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
>  addr = ip_flow->ipv6_dst;
>  }
>  
> -ovs_be128 ip = get_32aligned_be128((const ovs_32aligned_be128 *));
> -uint32_t hash = hash_bytes(, sizeof(ovs_be128), 0);
> -bp = pinctrl_find_buffered_packets(, hash);
> +uint32_t hash = hash_bytes(, sizeof addr, 0);
> +bp = pinctrl_find_buffered_packets(, hash);
>  if (!bp) {
>  if (hmap_count(_packets_map) >= 1000) {
>  COVERAGE_INC(pinctrl_drop_buffered_packets_map);
> @@ -357,7 +356,7 @@ pinctrl_handle_bufferd_packets(const struct flow *ip_flow,
>  bp = xmalloc(sizeof *bp);
>  hmap_insert(_packets_map, >hmap_node, hash);
>  bp->head = bp->tail = 0;
> -bp->ip = ip;
> +bp->ip = addr;
>  }
>  bp->timestamp = time_msec();
>  /* clone the packet to send it later with correct L2 address */
> @@ -381,7 +380,7 @@ pinctrl_handle_arp(const struct flow *ip_flow, struct 
> dp_packet *pkt_in,
>  return;
>  }
>  
> -pinctrl_handle_bufferd_packets(ip_flow, pkt_in, md, true);
> +pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, true);
>  
>  /* Compose an ARP packet. */
>  uint64_t packet_stub[128 / 8];
> @@ -1815,7 +1814,7 @@ struct put_mac_binding {
>  /* Key. */
>  uint32_t dp_key;
>  uint32_t port_key;
> -char ip_s[INET6_ADDRSTRLEN + 1];
> +struct in6_addr ip_key;
>  
>  /* Value. */
>  struct eth_addr mac;
> @@ -1839,13 +1838,13 @@ destroy_put_mac_bindings(void)
>  
>  static struct put_mac_binding *
>  pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t port_key,
> - const char *ip_s, uint32_t hash)
> + const struct in6_addr *ip_key, uint32_t hash)
>  {
>  struct put_mac_binding *pa;
>  HMAP_FOR_EACH_WITH_HASH (pa, hmap_node, hash, _mac_bindings) {
>  if (pa->dp_key 

[ovs-dev] [PATCH] ofp-packet: Fix NXT_RESUME with geneve tunnel metadata

[Yi-Hung Wei]

The patch address vswitchd crash when it receives NXT_RESUME with geneve
tunnel metadata.  The crash is due to segmentation fault with the
following stack trace, and it is observed only in kernel datapath.
A test is added to prevent regression.

Thread 1 "ovs-vswitchd" received signal SIGSEGV, Segmentation fault.
0  0x7fcffd0c5412 in tun_metadata_to_geneve__ 
(flow=flow@entry=0x7ffcb7106680, b=b@entry=0x7ffcb70eb5a8, 
crit_opt=crit_opt@entry=0x7ffcb70eb287)
   at lib/tun-metadata.c:676
1  0x7fcffd0c6858 in tun_metadata_to_geneve_nlattr_flow (b=0x7ffcb70eb5a8, 
flow=0x7ffcb7106638) at lib/tun-metadata.c:706
2  tun_metadata_to_geneve_nlattr (tun=tun@entry=0x7ffcb7106638, 
flow=flow@entry=0x7ffcb7106638, key=key@entry=0x0, b=b@entry=0x7ffcb70eb5a8)
   at lib/tun-metadata.c:810
3  0x7fcffd048464 in tun_key_to_attr (a=a@entry=0x7ffcb70eb5a8, 
tun_key=tun_key@entry=0x7ffcb7106638, 
tun_flow_key=tun_flow_key@entry=0x7ffcb7106638,
   key_buf=key_buf@entry=0x0, tnl_type=, tnl_type@entry=0x0) at 
lib/odp-util.c:2886
4  0x7fcffd0551cf in odp_key_from_dp_packet (buf=buf@entry=0x7ffcb70eb5a8, 
packet=0x7ffcb7106590) at lib/odp-util.c:5909
5  0x7fcffd0d7870 in dpif_netlink_encode_execute (buf=0x7ffcb70eb5a8, 
d_exec=0x7ffcb7106428, dp_ifindex=) at lib/dpif-netlink.c:1873
6  dpif_netlink_operate__ (dpif=dpif@entry=0xe65e00, 
ops=ops@entry=0x7ffcb7106418, n_ops=n_ops@entry=1) at lib/dpif-netlink.c:1959
7  0x7fcffd0d842e in dpif_netlink_operate_chunks (n_ops=1, 
ops=0x7ffcb7106418, dpif=) at lib/dpif-netlink.c:2258
8  dpif_netlink_operate (dpif_=0xe65e00, ops=, n_ops=) at lib/dpif-netlink.c:2294
9  0x7fcffd014680 in dpif_operate (dpif=, ops=, ops@entry=0x7ffcb7106418, n_ops=n_ops@entry=1) at lib/dpif.c:1359
10 0x7fcffd014c58 in dpif_execute (dpif=, 
execute=execute@entry=0x7ffcb71064e0) at lib/dpif.c:1324
11 0x7fcffd40d3e6 in nxt_resume (ofproto_=0xe6af50, pin=0x7ffcb7107150) at 
ofproto/ofproto-dpif.c:4885
12 0x7fcffd3f88c3 in handle_nxt_resume (ofconn=ofconn@entry=0xf8c8f0, 
oh=oh@entry=0xf7ebd0) at ofproto/ofproto.c:3612
13 0x7fcffd404a3b in handle_openflow__ (msg=0xeac460, ofconn=0xf8c8f0) at 
ofproto/ofproto.c:8137
14 handle_openflow (ofconn=0xf8c8f0, ofp_msg=0xeac460) at ofproto/ofproto.c:8258
15 0x7fcffd3f4653 in ofconn_run (handle_openflow=0x7fcffd4046f0 
, ofconn=0xf8c8f0) at ofproto/connmgr.c:1432
16 connmgr_run (mgr=0xe422f0, 
handle_openflow=handle_openflow@entry=0x7fcffd4046f0 ) at 
ofproto/connmgr.c:363
17 0x7fcffd3fdc76 in ofproto_run (p=0xe6af50) at ofproto/ofproto.c:1821
18 0x0040ca94 in bridge_run__ () at vswitchd/bridge.c:2939
19 0x00411d44 in bridge_run () at vswitchd/bridge.c:2997
20 0x004094fd in main (argc=12, argv=0x7ffcb71085b8) at 
vswitchd/ovs-vswitchd.c:119

VMWare-BZ: #2210216
Signed-off-by: Yi-Hung Wei 
---
Please backport it to 2.10.

---
 lib/ofp-packet.c|  2 ++
 tests/system-traffic.at | 40 
 2 files changed, 42 insertions(+)

diff --git a/lib/ofp-packet.c b/lib/ofp-packet.c
index b74c29b612a9..aa3417c9b051 100644
--- a/lib/ofp-packet.c
+++ b/lib/ofp-packet.c
@@ -160,6 +160,7 @@ decode_nx_packet_in2(const struct ofp_header *oh, bool 
loose,
 error = oxm_decode_match(payload.msg, ofpbuf_msgsize(),
  loose, tun_table, vl_mff_map,
  >flow_metadata);
+pin->flow_metadata.flow.tunnel.metadata.tab = tun_table;
 break;
 
 case NXPINT_USERDATA:
@@ -244,6 +245,7 @@ ofputil_decode_packet_in(const struct ofp_header *oh, bool 
loose,
   : NULL);
 enum ofperr error = oxm_pull_match_loose(, false, tun_table,
  >flow_metadata);
+pin->flow_metadata.flow.tunnel.metadata.tab = tun_table;
 if (error) {
 return error;
 }
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 277227bdd4cf..4c524317a644 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -532,6 +532,46 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 
10.1.1.100 | FORMAT_PI
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([datapath - flow resume with geneve tun_metadata])
+OVS_CHECK_GENEVE()
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-underlay])
+
+ADD_NAMESPACES(at_ns0)
+
+dnl Set up underlay link from host into the namespace using veth pair.
+ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
+AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
+AT_CHECK([ip link set dev br-underlay up])
+
+dnl Set up tunnel endpoints on OVS outside the namespace and with a native
+dnl linux device inside the namespace.
+ADD_OVS_TUNNEL([geneve], [br0], [at_gnv0], [172.31.1.1], [10.1.1.100/24])
+ADD_NATIVE_TUNNEL([geneve], [ns_gnv0], [at_ns0], [172.31.1.100], [10.1.1.1/24],
+  [vni 0])
+
+dnl Set up flows
+AT_DATA([flows.txt], [dnl

[ovs-dev] [PATCH] Python: Make Row's __getattr__ less error prone

[lucasagomes]

From: Lucas Alvares Gomes 

Calling getattr() on a Row object after invoking delkey() with a value
that does not exist in the object will cause getattr() to fail with a
KeyError error. For example:

Oct 05 14:59:28 neutron-server[28435]:   File
"/usr/local/lib/python2.7/dist-packages/ovsdbapp/backend/ovs_idl/connection.py",
line 122, in run
Oct 05 14:59:28 neutron-server[28435]:
txn.results.put(txn.do_commit())
Oct 05 14:59:28 neutron-server[28435]:   File
"/usr/local/lib/python2.7/dist-packages/ovsdbapp/backend/ovs_idl/transaction.py",
line 86, in do_commit
Oct 05 14:59:28 neutron-server[28435]: command.run_idl(txn)
Oct 05 14:59:28 neutron-server[28435]:   File
"/usr/local/lib/python2.7/dist-packages/ovsdbapp/backend/ovs_idl/command.py",
line 299, in run_idl
Oct 05 14:59:28 neutron-server[28435]: if
isinstance(getattr(record, self.column), dict):
Oct 05 14:59:28 neutron-server[28435]:   File
"/usr/local/lib/python2.7/dist-packages/ovs/db/idl.py", line 843, in
__getattr__
Oct 05 14:59:28 neutron-server[28435]: del dmap[key]
Oct 05 14:59:28 neutron-server[28435]: KeyError: 'bogusvalue'

This patch is replacing the "del dmap[key]" instruction with a
"dmap.pop(key, None)" instruction instead because a pop() (with a
default value) will not raise an exception in case the key does not
exist in the object in the first place, it will just ignore it.

Signed-Off-By: Lucas Alvares Gomes 
---
 python/ovs/db/idl.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 03110a76f..250e89756 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -855,7 +855,7 @@ class Row(object):
 if removes is not None:
 for key in removes:
 if key not in (inserts or {}):
-del dmap[key]
+dmap.pop(key, None)
 datum = data.Datum.from_python(column.type, dmap,
_row_to_uuid)
 else:
-- 
2.19.0

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

Re: [ovs-dev] [PATCH v2 2/2] windows, ovn-nbctl: Add service_start call inside the server loop

[Alin Gabriel Serdean]

> On 3 Oct 2018, at 01:37, Ben Pfaff  wrote:
> 
> On Tue, Oct 02, 2018 at 06:01:26PM +0300, Alin Gabriel Serdean wrote:
>> Currently all ovn-nbctl (daemon) tests are failing due to the missing
>> call to `service_start` which is required on Windows.
>> 
>> Windows lacks fork so we need to pass all arguments, so we can spawn a new
>> process and interpret it properly when calling `service_start`.
>> 
>> Signed-off-by: Alin Gabriel Serdean 
>> Acked-by: Ben Pfaff 
>> ---
>> v2: Remove OVS_UNUSED, add acks
> 
> I think that this can be made non-Windows specific by rewriting it as
> follows.
> 
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index eabd30308ac2..7e42b53a9079 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> 

Thanks for the suggestion and patch Ben. It looks cleaner and it passes the 
unit tests.

I sent a new version here:
https://patchwork.ozlabs.org/patch/979499/ 


Do you mind adding a sign off?

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

[ovs-dev] [PATCH v3 2/2] windows: Add set_detach function to daemon-windows.c

[Alin Gabriel Serdean]

The daemon-windows file is missing a `set_detach` routine, so add it.

This will be useful in the long run.

Signed-off-by: Alin Gabriel Serdean 
Acked-by: Ben Pfaff 
---
v3: no change.
v2: Fix typo in title, add Ack
---
 lib/daemon-windows.c | 10 +-
 lib/daemon.h |  3 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c
index 70c5f5d56..7e5f264f5 100644
--- a/lib/daemon-windows.c
+++ b/lib/daemon-windows.c
@@ -82,6 +82,14 @@ daemon_usage(void)
"unexpected failure. \n");
 }
 
+/* Sets up a following call to service_start() to detach from the foreground
+ * session, running this process in the background.  */
+void
+set_detach(void)
+{
+detach = true;
+}
+
 /* Registers the call-back and configures the actions in case of a failure
  * with the Windows services manager. */
 void
@@ -357,7 +365,7 @@ detach_process(int argc, char *argv[])
 
 /* We are only interested in the '--detach' and '--pipe-handle'. */
 for (i = 0; i < argc; i ++) {
-if (!strcmp(argv[i], "--detach")) {
+if (!detach && !strcmp(argv[i], "--detach")) {
 detach = true;
 } else if (!strncmp(argv[i], "--pipe-handle", 13)) {
 /* If running as a child, return. */
diff --git a/lib/daemon.h b/lib/daemon.h
index f33e9df8d..094157496 100644
--- a/lib/daemon.h
+++ b/lib/daemon.h
@@ -121,6 +121,7 @@ pid_t read_pidfile(const char *name);
 
 #define DAEMON_OPTION_HANDLERS  \
 case OPT_DETACH:\
+set_detach();   \
 break;  \
 \
 case OPT_NO_SELF_CONFINEMENT:   \
@@ -139,6 +140,7 @@ pid_t read_pidfile(const char *name);
 break;  \
 \
 case OPT_SERVICE:   \
+set_detach();   \
 break;  \
 \
 case OPT_SERVICE_MONITOR:   \
@@ -159,6 +161,7 @@ pid_t read_pidfile(const char *name);
 
 void control_handler(DWORD request);
 void set_pipe_handle(const char *pipe_handle);
+void set_detach(void);
 #endif /* _WIN32 */
 
 bool get_detach(void);
-- 
2.16.1.windows.1

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

[ovs-dev] [PATCH v3 1/2] windows, ovn-nbctl: Add service_start call inside the server loop

[Alin Gabriel Serdean]

Currently all ovn-nbctl (daemon) tests are failing due to the missing
call to `service_start` which is required on Windows.

Windows lacks fork so we need to pass all arguments, so we can spawn a new
process and interpret it properly when calling `service_start`.

Signed-off-by: Alin Gabriel Serdean 
Signed-off-by: Ben Pfaff 
Co-authored-by: Ben Pfaff 
---
v3: Switch to Ben's patch
v2: Remove OVS_UNUSED, add acks
---
---
 ovn/utilities/ovn-nbctl.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index eabd30308..7e42b53a9 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -117,7 +117,7 @@ static char * OVS_WARN_UNUSED_RESULT main_loop(const char 
*args,
size_t n_commands,
struct ovsdb_idl *idl,
const struct timer *);
-static void server_loop(struct ovsdb_idl *idl);
+static void server_loop(struct ovsdb_idl *idl, int argc, char *argv[]);
 
 int
 main(int argc, char *argv[])
@@ -173,26 +173,24 @@ main(int argc, char *argv[])
 shash_init(_options);
 apply_options_direct(parsed_options, n_parsed_options, _options);
 free(parsed_options);
-argc -= optind;
-argv += optind;
 
 /* Initialize IDL. */
 idl = the_idl = ovsdb_idl_create(db, _idl_class, true, false);
 ovsdb_idl_set_leader_only(idl, leader_only);
 
 if (get_detach()) {
-if (argc != 0) {
+if (argc != optind) {
 ctl_fatal("non-option arguments not supported with --detach "
   "(use --help for help)");
 }
-server_loop(idl);
+server_loop(idl, argc, argv);
 } else {
 struct ctl_command *commands;
 size_t n_commands;
 char *error;
 
-error = ctl_parse_commands(argc, argv, _options, ,
-   _commands);
+error = ctl_parse_commands(argc - optind, argv + optind,
+   _options, , _commands);
 if (error) {
 ctl_fatal("%s", error);
 }
@@ -5349,11 +5347,12 @@ server_cmd_init(struct ovsdb_idl *idl, bool *exiting)
 }
 
 static void
-server_loop(struct ovsdb_idl *idl)
+server_loop(struct ovsdb_idl *idl, int argc, char *argv[])
 {
 struct unixctl_server *server = NULL;
 bool exiting = false;
 
+service_start(, );
 daemonize_start(false);
 int error = unixctl_server_create(unixctl_path, );
 if (error) {
-- 
2.16.1.windows.1

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

Re: [ovs-dev] [PATCH v10 11/14] netdev-dpdk: support multi-segment jumbo frames

[Lam, Tiago]

On 03/10/2018 19:27, Flavio Leitner wrote:
> On Fri, Sep 28, 2018 at 05:15:12PM +0100, Tiago Lam wrote:
>> From: Mark Kavanagh 
>>
>> Currently, jumbo frame support for OvS-DPDK is implemented by
>> increasing the size of mbufs within a mempool, such that each mbuf
>> within the pool is large enough to contain an entire jumbo frame of
>> a user-defined size. Typically, for each user-defined MTU,
>> 'requested_mtu', a new mempool is created, containing mbufs of size
>> ~requested_mtu.
>>
>> With the multi-segment approach, a port uses a single mempool,
>> (containing standard/default-sized mbufs of ~2k bytes), irrespective
>> of the user-requested MTU value. To accommodate jumbo frames, mbufs
>> are chained together, where each mbuf in the chain stores a portion of
>> the jumbo frame. Each mbuf in the chain is termed a segment, hence the
>> name.
>>
>> == Enabling multi-segment mbufs ==
>> Multi-segment and single-segment mbufs are mutually exclusive, and the
>> user must decide on which approach to adopt on init. The introduction
>> of a new OVSDB field, 'dpdk-multi-seg-mbufs', facilitates this. This
>> is a global boolean value, which determines how jumbo frames are
>> represented across all DPDK ports. In the absence of a user-supplied
>> value, 'dpdk-multi-seg-mbufs' defaults to false, i.e. multi-segment
>> mbufs must be explicitly enabled / single-segment mbufs remain the
>> default.
>>
>> Setting the field is identical to setting existing DPDK-specific OVSDB
>> fields:
>>
>> ovs-vsctl set Open_vSwitch . other_config:dpdk-init=true
>> ovs-vsctl set Open_vSwitch . other_config:dpdk-lcore-mask=0x10
>> ovs-vsctl set Open_vSwitch . other_config:dpdk-socket-mem=4096,0
>> ==> ovs-vsctl set Open_vSwitch . other_config:dpdk-multi-seg-mbufs=true
>>
>> Co-authored-by: Tiago Lam 
>>
>> Signed-off-by: Mark Kavanagh 
>> Signed-off-by: Tiago Lam 
>> Acked-by: Eelco Chaudron 
>> ---
>>  Documentation/topics/dpdk/jumbo-frames.rst | 73 
>> ++
>>  Documentation/topics/dpdk/memory.rst   | 36 +++
>>  NEWS   |  1 +
>>  lib/dpdk.c |  8 
>>  lib/netdev-dpdk.c  | 60 
>>  lib/netdev-dpdk.h  |  1 +
>>  vswitchd/vswitch.xml   | 22 +
>>  7 files changed, 193 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/topics/dpdk/jumbo-frames.rst 
>> b/Documentation/topics/dpdk/jumbo-frames.rst
>> index 00360b4..5888f1e 100644
>> --- a/Documentation/topics/dpdk/jumbo-frames.rst
>> +++ b/Documentation/topics/dpdk/jumbo-frames.rst
>> @@ -71,3 +71,76 @@ Jumbo frame support has been validated against 9728B 
>> frames, which is the
>>  largest frame size supported by Fortville NIC using the DPDK i40e driver, 
>> but
>>  larger frames and other DPDK NIC drivers may be supported. These cases are
>>  common for use cases involving East-West traffic only.
>> +
>> +---
>> +Multi-segment mbufs
>> +---
>> +
>> +Instead of increasing the size of mbufs within a mempool, such that each 
>> mbuf
>> +within the pool is large enough to contain an entire jumbo frame of a
>> +user-defined size, mbufs can be chained together instead. In this approach 
>> each
>> +mbuf in the chain stores a portion of the jumbo frame, by default ~2K bytes,
>> +irrespective of the user-requested MTU value. Since each mbuf in the chain 
>> is
>> +termed a segment, this approach is named "multi-segment mbufs".
>> +
>> +This approach may bring more flexibility in use cases where the maximum 
>> packet
>> +length may be hard to guess. For example, in cases where packets originate 
>> from
>> +sources marked for oflload (such as TSO), each packet may be larger than the
> typo   ^^

Fixed, thanks.

> 
>> +MTU, and as such, when forwarding it to a DPDK port a single mbuf may not be
>> +enough to hold all of the packet's data.
>> +
>> +Multi-segment and single-segment mbufs are mutually exclusive, and the user
>> +must decide on which approach to adopt on initialisation. If multi-segment
>> +mbufs is to be enabled, it can be done so with the following command::
>> +
>> +$ ovs-vsctl set Open_vSwitch . other_config:dpdk-multi-seg-mbufs=true
>> +
>> +Single-segment mbufs still remain the default when using OvS-DPDK, and the
>> +above option `dpdk-multi-seg-mbufs` must be explicitly set to `true` if
>> +multi-segment mbufs are to be used.
>> +
>> +~
>> +Performance notes
>> +~
>> +
>> +When using multi-segment mbufs some PMDs may not support vectorized Tx
>> +functions, due to its non-contiguous nature. As a result this can hit
>> +performance for smaller packet sizes. For example, on a setup sending 64B
>> +packets at line rate, a decrease of ~20% has been observed. The performance
>> +impact stops being noticeable for larger packet sizes, although the exact 
>> size

Re: [ovs-dev] [PATCH v10 09/14] dp-packet: Add support for data "linearization".

[Lam, Tiago]

On 03/10/2018 19:26, Flavio Leitner wrote:
> 
> Hi Tiago,
> 
> There is a lot to digest in this patch, so I only have a general
> comment about it for now.
> 
> See below.
> 
> On Fri, Sep 28, 2018 at 05:15:10PM +0100, Tiago Lam wrote:
>> Previous commits have added support to the dp_packet API to handle
>> multi-segmented packets, where data is not stored contiguously in
>> memory. However, in some cases, it is inevitable and data must be
>> provided contiguously. Examples of such cases are when performing csums
>> over the entire packet data, or when write()'ing to a file descriptor
>> (for a tap interface, for example). For such cases, the dp_packet API
>> has been extended to provide a way to transform a multi-segmented
>> DPBUF_DPDK packet into a DPBUF_MALLOC system packet (at the expense of a
>> copy of memory). If the packet's data is already stored in memory
>> contigously then there's no need to convert the packet.
>>
>> Thus, the main use cases that were assuming that a dp_packet's data is
>> always held contiguously in memory were changed to make use of the new
>> "linear functions" in the dp_packet API when there's a need to traverse
>> the entire's packet data. Per the example above, when the packet's data
>> needs to be write() to the tap's file descriptor, or when the conntrack
>> module needs to verify a packet's checksum, the data is now linearized.
>>
>> Additionally, the layer functions, such as dp_packet_l3() and variants,
>> have been modified to check if there's enough data in the packet before
>> returning a pointer to the data (and callers have been modified
>> accordingly). This requirement is needed to guarantee that a caller
>> doesn't read beyond the available memory.
>>
>> Signed-off-by: Tiago Lam 
>> ---
>>  lib/bfd.c |   3 +-
>>  lib/cfm.c |   5 +-
>>  lib/conntrack-icmp.c  |   4 +-
>>  lib/conntrack-private.h   |   4 +-
>>  lib/conntrack-tcp.c   |   6 +-
>>  lib/conntrack.c   | 109 +
>>  lib/dp-packet.c   |  18 
>>  lib/dp-packet.h   | 217 
>> +++---
>>  lib/dpif-netdev.c |   5 +
>>  lib/dpif-netlink.c|   5 +
>>  lib/dpif.c|   9 ++
>>  lib/flow.c|  29 +++---
>>  lib/lacp.c|   3 +-
>>  lib/mcast-snooping.c  |   8 +-
>>  lib/netdev-bsd.c  |   5 +
>>  lib/netdev-dummy.c|  13 ++-
>>  lib/netdev-linux.c|  13 ++-
>>  lib/netdev-native-tnl.c   |  39 +---
>>  lib/odp-execute.c |  28 --
>>  lib/ofp-print.c   |  10 +-
>>  lib/ovs-lldp.c|   3 +-
>>  lib/packets.c |  81 +---
>>  lib/pcap-file.c   |   2 +-
>>  ofproto/ofproto-dpif-upcall.c |  20 +++-
>>  ofproto/ofproto-dpif-xlate.c  |  42 ++--
>>  ovn/controller/pinctrl.c  |  29 +++---
>>  tests/test-conntrack.c|   2 +-
>>  tests/test-rstp.c |   8 +-
>>  tests/test-stp.c  |   8 +-
>>  29 files changed, 483 insertions(+), 245 deletions(-)
>>
>> diff --git a/lib/bfd.c b/lib/bfd.c
>> index 5308262..d50d2da 100644
>> --- a/lib/bfd.c
>> +++ b/lib/bfd.c
>> @@ -722,7 +722,8 @@ bfd_process_packet(struct bfd *bfd, const struct flow 
>> *flow,
>>  if (!msg) {
>>  VLOG_INFO_RL(, "%s: Received too-short BFD control message (only 
>> "
>>   "%"PRIdPTR" bytes long, at least %d required).",
>> - bfd->name, (uint8_t *) dp_packet_tail(p) - l7,
>> + bfd->name, dp_packet_size(p) -
>> + (l7 - (uint8_t *) dp_packet_data(p)),
>>   BFD_PACKET_LEN);
>>  goto out;
>>  }
>> diff --git a/lib/cfm.c b/lib/cfm.c
>> index 71d2c02..83baf2a 100644
>> --- a/lib/cfm.c
>> +++ b/lib/cfm.c
>> @@ -584,7 +584,7 @@ cfm_compose_ccm(struct cfm *cfm, struct dp_packet 
>> *packet,
>>  
>>  atomic_read_relaxed(>extended, );
>>  
>> -ccm = dp_packet_l3(packet);
>> +ccm = dp_packet_l3(packet, sizeof(*ccm));
>>  ccm->mdlevel_version = 0;
>>  ccm->opcode = CCM_OPCODE;
>>  ccm->tlv_offset = 70;
>> @@ -759,8 +759,7 @@ cfm_process_heartbeat(struct cfm *cfm, const struct 
>> dp_packet *p)
>>  atomic_read_relaxed(>extended, );
>>  
>>  eth = dp_packet_eth(p);
>> -ccm = dp_packet_at(p, (uint8_t *)dp_packet_l3(p) - (uint8_t 
>> *)dp_packet_data(p),
>> -CCM_ACCEPT_LEN);
>> +ccm = dp_packet_l3(p, CCM_ACCEPT_LEN);
>>  
>>  if (!ccm) {
>>  VLOG_INFO_RL(, "%s: Received an unparseable 802.1ag CCM 
>> heartbeat.",
>> diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c
>> index 40fd1d8..0575d0e 100644
>> --- a/lib/conntrack-icmp.c
>> +++ b/lib/conntrack-icmp.c
>> @@ -63,7 +63,7 @@ icmp_conn_update(struct conn *conn_, struct 
>> conntrack_bucket *ctb,
>>  static bool
>>  icmp4_valid_new(struct 

Re: [ovs-dev] [PATCH v10 10/14] netdev-dpdk: copy large packet to multi-seg. mbufs

[Lam, Tiago]

On 03/10/2018 19:27, Flavio Leitner wrote:
> On Fri, Sep 28, 2018 at 05:15:11PM +0100, Tiago Lam wrote:
>> From: Mark Kavanagh 
>>
>> Currently, packets are only copied to a single segment in the function
>> dpdk_do_tx_copy(). This could be an issue in the case of jumbo frames,
>> particularly when multi-segment mbufs are involved.
>>
>> This patch calculates the number of segments needed by a packet and
>> copies the data to each segment.
>>
>> A new function, dpdk_buf_alloc(), has also been introduced as a wrapper
>> around the nonpmd_mp_mutex to serialise allocations from a non-pmd
>> context.
>>
>> Co-authored-by: Michael Qiu 
>> Co-authored-by: Tiago Lam 
>>
>> Signed-off-by: Mark Kavanagh 
>> Signed-off-by: Michael Qiu 
>> Signed-off-by: Tiago Lam 
>> Acked-by: Eelco Chaudron 
>> ---
>>  lib/netdev-dpdk.c | 91 
>> +--
>>  1 file changed, 82 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 8484239..e58e7ac 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -552,6 +552,27 @@ dpdk_rte_mzalloc(size_t sz)
>>  return rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE);
>>  }
>>  
>> +static struct rte_mbuf *
>> +dpdk_buf_alloc(struct rte_mempool *mp)
>> +{
>> +struct rte_mbuf *mbuf = NULL;
>> +
>> +/* If non-pmd we need to lock on nonpmd_mp_mutex mutex */
>> +if (!dpdk_thread_is_pmd()) {
>> +ovs_mutex_lock(_mp_mutex);
>> +
>> +mbuf = rte_pktmbuf_alloc(mp);
>> +
>> +ovs_mutex_unlock(_mp_mutex);
>> +
>> +return mbuf;
>> +}
>> +
>> +mbuf = rte_pktmbuf_alloc(mp);
>> +
>> +return mbuf;
>> +}
> 
> What about:
> 
> if (dpdk_thread_is_pmd()) {
> mbuf = rte_pktmbuf_alloc(mp);
> } else {
> ovs_mutex_lock(_mp_mutex);
> 
> mbuf = rte_pktmbuf_alloc(mp);
> 
> ovs_mutex_unlock(_mp_mutex);
> }
> 
> return mbuf;

It does read better. I'll use that, thanks.

Tiago.

> 
> 
> 
> 
> 
>> +
>>  void
>>  free_dpdk_buf(struct dp_packet *packet)
>>  {
>> @@ -2316,6 +2337,56 @@ out:
>>  }
>>  }
>>  
>> +static int
>> +dpdk_copy_dp_packet_to_mbuf(struct dp_packet *packet, struct rte_mbuf 
>> **head,
>> +struct rte_mempool *mp)
>> +{
>> +struct rte_mbuf *mbuf, *fmbuf;
>> +uint16_t max_data_len;
>> +uint32_t nb_segs = 0;
>> +uint32_t size = 0;
>> +
>> +/* We will need the whole data for copying below */
>> +if (!dp_packet_is_linear(packet)) {
>> +dp_packet_linearize(packet);
>> +}
>> +
>> +/* Allocate first mbuf to know the size of data available */
>> +fmbuf = mbuf = *head = dpdk_buf_alloc(mp);
>> +if (OVS_UNLIKELY(!mbuf)) {
>> +return ENOMEM;
>> +}
>> +
>> +size = dp_packet_size(packet);
>> +
>> +/* All new allocated mbuf's max data len is the same */
>> +max_data_len = mbuf->buf_len - mbuf->data_off;
>> +
>> +/* Calculate # of output mbufs. */
>> +nb_segs = size / max_data_len;
>> +if (size % max_data_len) {
>> +nb_segs = nb_segs + 1;
>> +}
>> +
>> +/* Allocate additional mbufs, less the one alredy allocated above */
>> +for (int i = 1; i < nb_segs; i++) {
>> +mbuf->next = dpdk_buf_alloc(mp);
>> +if (!mbuf->next) {
>> +free_dpdk_buf(CONTAINER_OF(fmbuf, struct dp_packet, mbuf));
>> +fmbuf = NULL;
>> +return ENOMEM;
>> +}
>> +mbuf = mbuf->next;
>> +}
>> +
>> +fmbuf->nb_segs = nb_segs;
>> +fmbuf->pkt_len = size;
>> +
>> +dp_packet_mbuf_write(fmbuf, 0, size, dp_packet_data(packet));
>> +
>> +return 0;
>> +}
>> +
>>  /* Tx function. Transmit packets indefinitely */
>>  static void
>>  dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch 
>> *batch)
>> @@ -2332,6 +2403,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
>> dp_packet_batch *batch)
>>  struct rte_mbuf *pkts[PKT_ARRAY_SIZE];
>>  uint32_t cnt = batch_cnt;
>>  uint32_t dropped = 0;
>> +uint32_t i;
>>  
>>  if (dev->type != DPDK_DEV_VHOST) {
>>  /* Check if QoS has been configured for this netdev. */
>> @@ -2342,28 +2414,29 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, 
>> struct dp_packet_batch *batch)
>>  
>>  uint32_t txcnt = 0;
>>  
>> -for (uint32_t i = 0; i < cnt; i++) {
>> +for (i = 0; i < cnt; i++) {
>>  struct dp_packet *packet = batch->packets[i];
>>  uint32_t size = dp_packet_size(packet);
>> +int err = 0;
>>  
>>  if (OVS_UNLIKELY(size > dev->max_packet_len)) {
>>  VLOG_WARN_RL(, "Too big size %u max_packet_len %d",
>>   size, dev->max_packet_len);
>> -
>>  dropped++;
>>  continue;
>>  }
>>  
>> -pkts[txcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
>> -if (OVS_UNLIKELY(!pkts[txcnt])) {
>> +err = dpdk_copy_dp_packet_to_mbuf(packet, 

Re: [ovs-dev] [PATCH v10 08/14] dp-packet: copy data from multi-seg. DPDK mbuf

[Lam, Tiago]

On 03/10/2018 19:26, Flavio Leitner wrote:
> On Fri, Sep 28, 2018 at 05:15:09PM +0100, Tiago Lam wrote:
>> From: Michael Qiu 
>>
>> When doing packet clone, if packet source is from DPDK driver,
>> multi-segment must be considered, and copy the segment's data one by
>> one.
>>
>> Also, lots of DPDK mbuf's info is missed during a copy, like packet
>> type, ol_flags, etc.  That information is very important for DPDK to do
>> packets processing.
>>
>> Co-authored-by: Mark Kavanagh 
>> Co-authored-by: Tiago Lam 
>>
>> Signed-off-by: Michael Qiu 
>> Signed-off-by: Mark Kavanagh 
>> Signed-off-by: Tiago Lam 
>> Acked-by: Eelco Chaudron 
>> ---
>>  lib/dp-packet.c   | 69 
>> ++-
>>  lib/dp-packet.h   |  3 +++
>>  lib/netdev-dpdk.c |  1 +
>>  3 files changed, 62 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 167bf43..806640b 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -48,6 +48,22 @@ dp_packet_use__(struct dp_packet *b, void *base, size_t 
>> allocated,
>>  dp_packet_set_size(b, 0);
>>  }
>>  
>> +#ifdef DPDK_NETDEV
>> +void
>> +dp_packet_copy_mbuf_flags(struct dp_packet *dst, const struct dp_packet 
>> *src)
>> +{
>> +ovs_assert(dst != NULL && src != NULL);
>> +struct rte_mbuf *buf_dst = &(dst->mbuf);
>> +struct rte_mbuf buf_src = src->mbuf;
>> +
>> +buf_dst->ol_flags = buf_src.ol_flags;
>> +buf_dst->packet_type = buf_src.packet_type;
>> +buf_dst->tx_offload = buf_src.tx_offload;
> 
> Nit: use dereferencing on both sides.
> 
> Also, since this is a simple function that only copies few fields, maybe it 
> could
> be in the dp-packet.h, so that netdev-dpdk.c can inline it.
> 
>> +}
>> +#else
>> +#define dp_packet_copy_mbuf_flags(arg1, arg2)
> 
> Nit2: since this is very specific to mbuf, we don't need to expose it
> outside of DPDK.
> 
>> +#endif
>> +
>>  /* Initializes 'b' as an empty dp_packet that contains the 'allocated' 
>> bytes of
>>   * memory starting at 'base'.  'base' should be the first byte of a region
>>   * obtained from malloc().  It will be freed (with free()) if 'b' is 
>> resized or
>> @@ -158,6 +174,44 @@ dp_packet_clone(const struct dp_packet *buffer)
>>  return dp_packet_clone_with_headroom(buffer, 0);
>>  }
>>  
>> +#ifdef DPDK_NETDEV
>> +struct dp_packet *
>> +dp_packet_clone_with_headroom(const struct dp_packet *b, size_t headroom) {
>> +struct dp_packet *new_buffer;
>> +uint32_t pkt_len = dp_packet_size(b);
>> +
>> +/* copy multi-seg data */
>> +if (b->source == DPBUF_DPDK && !rte_pktmbuf_is_contiguous(>mbuf)) {
>> +void *dst = NULL;
>> +struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, >mbuf);
>> +
>> +new_buffer = dp_packet_new_with_headroom(pkt_len, headroom);
>> +dst = dp_packet_data(new_buffer);
>> +dp_packet_set_size(new_buffer, pkt_len);
>> +
>> +if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) {
> 
> Here we leak new_buffer which is xmalloc'ed.

Fixed. Thanks!

> 
>> +return NULL;
>> +}
>> +} else {
>> +new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(b),
>> +dp_packet_size(b),
>> +headroom);
>> +}
>> +
>> +/* Copy the following fields into the returned buffer: l2_pad_size,
>> + * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>> +memcpy(_buffer->l2_pad_size, >l2_pad_size,
>> +   sizeof(struct dp_packet) -
>> +   offsetof(struct dp_packet, l2_pad_size));
>> +
>> +dp_packet_copy_mbuf_flags(new_buffer, b);
>> +if (dp_packet_rss_valid(new_buffer)) {
>> +new_buffer->mbuf.hash.rss = b->mbuf.hash.rss;
>> +}
>> +
>> +return new_buffer;
>> +}
>> +#else
>>  /* Creates and returns a new dp_packet whose data are copied from 'buffer'.
>>   * The returned dp_packet will additionally have 'headroom' bytes of
>>   * headroom. */
>> @@ -165,32 +219,25 @@ struct dp_packet *
>>  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t 
>> headroom)
>>  {
>>  struct dp_packet *new_buffer;
>> +uint32_t pkt_len = dp_packet_size(buffer);
>>  
>>  new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
>> - dp_packet_size(buffer),
>> - headroom);
>> + pkt_len, headroom);
>> +
>>  /* Copy the following fields into the returned buffer: l2_pad_size,
>>   * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>>  memcpy(_buffer->l2_pad_size, >l2_pad_size,
>>  sizeof(struct dp_packet) -
>>  offsetof(struct dp_packet, l2_pad_size));
>>  
>> -#ifdef DPDK_NETDEV
>> -new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>> -#else
>>  

Re: [ovs-dev] [PATCH v10 05/14] dp-packet: Fix data_len handling multi-seg mbufs.

[Lam, Tiago]

On 03/10/2018 19:26, Flavio Leitner wrote:
> On Fri, Sep 28, 2018 at 05:15:06PM +0100, Tiago Lam wrote:
>> When a dp_packet is from a DPDK source, and it contains multi-segment
>> mbufs, the data_len is not equal to the packet size, pkt_len. Instead,
>> the data_len of each mbuf in the chain should be considered while
>> distributing the new (provided) size.
>>
>> To account for the above dp_packet_set_size() has been changed so that,
>> in the multi-segment mbufs case, only the data_len on the last mbuf of
>> the chain and the total size of the packet, pkt_len, are changed. The
>> data_len on the intermediate mbufs preceeding the last mbuf is not
>> changed by dp_packet_set_size(). Furthermore, in some cases
>> dp_packet_set_size() may be used to set a smaller size than the current
>> packet size, thus effectively trimming the end of the packet. In the
>> multi-segment mbufs case this may lead to lingering mbufs that may need
>> freeing.
>>
>> __dp_packet_set_data() now also updates an mbufs' data_len after setting
>> the data offset. This is so that both fields are always in sync for each
>> mbuf in a chain.
> 
> I think we need an assert(source) to make sure dp_packet_shift() will
> never be called for a multi-seg.

I guess you made this comment before seeing patch 07/14 where a
dp_packet_shift() implementation is introduced for DPDK packets? But
nonetheless, it has been mentioned before by Darrell that it might not
be worth implementing. Given that the only users for now are in
lib/pcap-file.c and lib/ovs-ofctl.c, where packets are always of
DPBUF_MALLOC type, I guess it could make sense to drop the
implementation. It would certainly help reduce the footprint of this
patchset for now. If a use case comes for that later on we can always
introduce it then.

>  
>> Co-authored-by: Michael Qiu 
>> Co-authored-by: Mark Kavanagh 
>> Co-authored-by: Przemyslaw Lal 
>> Co-authored-by: Marcin Ksiadz 
>> Co-authored-by: Yuanhan Liu 
>>
>> Signed-off-by: Michael Qiu 
>> Signed-off-by: Mark Kavanagh 
>> Signed-off-by: Przemyslaw Lal 
>> Signed-off-by: Marcin Ksiadz 
>> Signed-off-by: Yuanhan Liu 
>> Signed-off-by: Tiago Lam 
>> Acked-by: Eelco Chaudron 
>> ---
>>  lib/dp-packet.h | 84 
>> -
>>  1 file changed, 72 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index 6376039..4545041 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -426,20 +426,60 @@ dp_packet_size(const struct dp_packet *b)
>>  return b->mbuf.pkt_len;
>>  }
>>  
>> +/* Sets the size of the packet 'b' to 'v'. For non-DPDK packets this only 
>> means
>> + * setting b->size_, but if used in a DPDK packet it means adjusting the 
>> first
>> + * mbuf pkt_len and last mbuf data_len, to reflect the real size, which can
>> + * lead to free'ing tail mbufs that are no longer used.
>> + *
>> + * This function should be used for setting the size only, and if there's an
>> + * assumption that the tail end of 'b' will be trimmed. For adjustng the 
>> head
>> + * 'end' of 'b', dp_packet_pull() should be used instead. */
>>  static inline void
>>  dp_packet_set_size(struct dp_packet *b, uint32_t v)
>>  {
>> -/* netdev-dpdk does not currently support segmentation; consequently, 
>> for
>> - * all intents and purposes, 'data_len' (16 bit) and 'pkt_len' (32 bit) 
>> may
>> - * be used interchangably.
>> - *
>> - * On the datapath, it is expected that the size of packets
>> - * (and thus 'v') will always be <= UINT16_MAX; this means that there 
>> is no
>> - * loss of accuracy in assigning 'v' to 'data_len'.
>> - */
>> -b->mbuf.data_len = (uint16_t)v;  /* Current seg length. */
>> -b->mbuf.pkt_len = v; /* Total length of all segments linked 
>> to
>> -  * this segment. */
>> +if (b->source == DPBUF_DPDK) {
>> +struct rte_mbuf *mbuf = >mbuf;
>> +uint16_t new_len = v;
>> +uint16_t data_len;
>> +uint16_t nb_segs = 0;
>> +uint16_t pkt_len = 0;
>> +
>> +/* Trim 'v' length bytes from the end of the chained buffers, 
>> freeing
>> +   any buffers that may be left floating */
>> +while (mbuf) {
>> +data_len = MIN(new_len, mbuf->data_len);
>> +mbuf->data_len = data_len;
>> +
>> +if (new_len - data_len <= 0) {
>> +/* Free the rest of chained mbufs */
>> +free_dpdk_buf(CONTAINER_OF(mbuf->next, struct dp_packet,
>> +   mbuf));
>> +mbuf->next = NULL;
>> +} else if (!mbuf->next) {
>> +/* Don't assign more than what we have available */
>> +mbuf->data_len = MIN(new_len,
>> + mbuf->buf_len - mbuf->data_off);
>> +}
>> +
>> +new_len -= data_len;
>> +nb_segs += 1;
>> +

Re: [ovs-dev] [PATCH v10 01/14] netdev-dpdk: fix mbuf sizing

[Lam, Tiago]

On 03/10/2018 19:25, Flavio Leitner wrote:
> 
> Hi Tiago,
> 
> Thanks for working on this. This is my first pass trying to wrap my head
> around the whole thing, so I may have missed something.
> 
> See below.
> 
> On Fri, Sep 28, 2018 at 05:15:02PM +0100, Tiago Lam wrote:
>> 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.
> 
> Do you have a pointer to the DPDK documentation about this issue?
> Because this looks like an internal DPDK requirement which shouldn't
> be exposed to OVS. Maybe we can take it now, but yeah, doesn't make
> sense to have every DPDK application out there worrying about mbuf
> internal constrains/alignments.

Unfortunately, no (if someone does, please link me to it). This seems to
happen mostly ad-hoc for each PMD. I can see that this alignment fix has
been initially added for the vNIC thunderx PMD (commit 31b88c9), looking
at the history, which seems to fail initialization unless the mbuf_size
is a multiple of cache_line. This patch is just rationalizing this odd
logic for setting the buf_size and priv_size.

DPDK already checks if the passed priv_size is aligned to
RTE_MBUF_PRIV_ALIGN when creating the pool, but doesn't seem to do any
check about the whole mbuf size. The PMD above then just fails if not
aligned. I would agree that this has room for improvement.

> 
> 
>> 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 
>> Acked-by: Santosh Shukla 
>> Acked-by: Eelco Chaudron 
>> ---
>>  lib/netdev-dpdk.c | 56 
>> +--
>>  1 file changed, 38 insertions(+), 18 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index f91aa27..1e19622 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 mp_name[RTE_MEMPOOL_NAMESIZE];
>>  const char *netdev_name = netdev_get_name(>up);
>>  int socket_id = dev->requested_socket_id;
>> -uint32_t n_mbufs;
>> +uint32_t n_mbufs = 0;
>> +uint32_t mbuf_size = 0;
>> +uint32_t aligned_mbuf_size = 0;
>> +uint32_t mbuf_priv_data_len = 0;
>> +uint32_t pkt_size = 0;
>>  uint32_t hash = hash_string(netdev_name, 0);
>>  struct dpdk_mp *dmp = NULL;
>>  int ret;
>> @@ -650,6 +650,9 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool 
>> per_port_mp)
>>  dmp->mtu = mtu;
>>  dmp->refcount = 1;
>>  
>> +/* Get the size of each mbuf, based on the MTU */
>> +mbuf_size = dpdk_buf_size(dev->requested_mtu);
>> +
> 
> Here mbuf_size accounts for RTE_PKTMBUF_HEADROOM.
> 
>>  n_mbufs = dpdk_calculate_mbufs(dev, mtu, per_port_mp);
>>  
>>  do {
>> @@ -661,8 +664,8 @@ 

[ovs-dev] [PATCH v2] ovn: Fix IPv6 DAD failure for container ports

[nusiddiq]

From: Numan Siddique 

When a container port is created inside a VM, the below kernel message
is seen and IPv6 doesn't work on that interface.

[  138.000753] IPv6: vlan4: IPv6 duplicate address  detected!

When a container port sends a ethernet broadcast packet, OVN delivers the same
packet back to the child port (and hence the DAD check fails).

This is because
 - 'MLF_ALLOW_LOOPBACK_BIT' is set in REG10 in table 0 for the packets received
   from any child port.
 - for ethernet broadcast packets, Table 33 (OFTABLE_LOCAL_OUTPUT) clones the
   packet for every local port 'P' which belongs to the same datapath i.e
   'P'->REG15, resubmit(,34)
 - If REG14 and REG15 are same, Table 34 (OFTABLE_CHECK_LOOPBACK) drops the 
packet
   if 'MLF_ALLOW_LOOPBACK_BIT' is not set.
 - But in the case of container ports, this bit will be set and hence doesn't 
gets
   dropped and eventually gets delivered to the source container port.
 - The VM's kernel thinks its a DAD packet. The latest kernels (4.19) implements
   the RFC -7527 (enhanced DAD), but it is still a problem for older kernels.

This patch fixes the issue by using a new register bit 
(MLF_NESTED_CONTAINER_BIT)
instead of 'MLF_ALLOW_LOOPBACK_BIT' and sets it in REG10 for the packets 
received
from child ports so that Table 34 drops the packet for the source port.

Signed-off-by: Numan Siddique 
---

v1 -> v2 
---
  * Addressed the review comments from Guru - Updated the commit message
and added few comments in the code.

 ovn/controller/ofctrl.c   | 29 +++--
 ovn/controller/ofctrl.h   |  5 +++
 ovn/controller/physical.c | 65 ++-
 ovn/lib/logical-fields.h  |  4 +++
 tests/ovn.at  | 11 +++
 5 files changed, 97 insertions(+), 17 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index f5b53195d..218612787 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -630,9 +630,11 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
  *
  * The caller should initialize its own hmap to hold the flows. */
 void
-ofctrl_add_flow(struct hmap *desired_flows,
-uint8_t table_id, uint16_t priority, uint64_t cookie,
-const struct match *match, const struct ofpbuf *actions)
+ofctrl_check_and_add_flow(struct hmap *desired_flows,
+  uint8_t table_id, uint16_t priority, uint64_t cookie,
+  const struct match *match,
+  const struct ofpbuf *actions,
+  bool log_duplicate_flow)
 {
 struct ovn_flow *f = xmalloc(sizeof *f);
 f->table_id = table_id;
@@ -644,13 +646,14 @@ ofctrl_add_flow(struct hmap *desired_flows,
 f->cookie = cookie;
 
 if (ovn_flow_lookup(desired_flows, f)) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
-if (!VLOG_DROP_DBG()) {
-char *s = ovn_flow_to_string(f);
-VLOG_DBG("dropping duplicate flow: %s", s);
-free(s);
+if (log_duplicate_flow) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
+if (!VLOG_DROP_DBG()) {
+char *s = ovn_flow_to_string(f);
+VLOG_DBG("dropping duplicate flow: %s", s);
+free(s);
+}
 }
-
 ovn_flow_destroy(f);
 return;
 }
@@ -658,6 +661,14 @@ ofctrl_add_flow(struct hmap *desired_flows,
 hmap_insert(desired_flows, >hmap_node, f->hmap_node.hash);
 }
 
+void
+ofctrl_add_flow(struct hmap *desired_flows,
+uint8_t table_id, uint16_t priority, uint64_t cookie,
+const struct match *match, const struct ofpbuf *actions)
+{
+ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
+  match, actions, true);
+}
 
 /* ovn_flow. */
 
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index e3fc95b05..ae0cfa513 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -55,4 +55,9 @@ void ofctrl_add_flow(struct hmap *desired_flows, uint8_t 
table_id,
  uint16_t priority, uint64_t cookie,
  const struct match *, const struct ofpbuf *ofpacts);
 
+void ofctrl_check_and_add_flow(struct hmap *desired_flows, uint8_t table_id,
+   uint16_t priority, uint64_t cookie,
+   const struct match *,
+   const struct ofpbuf *ofpacts,
+   bool log_duplicate_flow);
 #endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index c38d7b09f..ab3b02ab1 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -189,7 +189,8 @@ get_zone_ids(const struct sbrec_port_binding *binding,
 
 static void
 put_local_common_flows(uint32_t dp_key, uint32_t port_key,
-   bool nested_container, const struct 

Re: [ovs-dev] ossfuzz: Speed up flow extract fuzzing by refactoring

[0-day Robot]

Bleep bloop.  Greetings Bhargava Shastry, I am a robot and I have tried out 
your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
#585 FILE: tests/oss-fuzz/miniflow_target.c:95:
flow_u32[*idxp + i] = random_value();

ERROR: Use ovs_assert() in place of assert()
#722 FILE: tests/oss-fuzz/miniflow_target.c:232:
assert(miniflow_get_vid(miniflow, i) ==

ERROR: Use ovs_assert() in place of assert()
#726 FILE: tests/oss-fuzz/miniflow_target.c:236:
assert(miniflow_get(miniflow, i) == flow_u64[i]);

ERROR: Use ovs_assert() in place of assert()
#730 FILE: tests/oss-fuzz/miniflow_target.c:240:
assert(miniflow_equal(miniflow, miniflow));

ERROR: Use ovs_assert() in place of assert()
#734 FILE: tests/oss-fuzz/miniflow_target.c:244:
assert(flow_equal(flow, ));

ERROR: Use ovs_assert() in place of assert()
#737 FILE: tests/oss-fuzz/miniflow_target.c:247:
assert(miniflow_equal(miniflow, miniflow2));

ERROR: Use ovs_assert() in place of assert()
#738 FILE: tests/oss-fuzz/miniflow_target.c:248:
assert(miniflow_hash__(miniflow, 0) == miniflow_hash__(miniflow2, 0));

ERROR: Use ovs_assert() in place of assert()
#740 FILE: tests/oss-fuzz/miniflow_target.c:250:
assert(flow_equal(flow, ));

ERROR: Use ovs_assert() in place of assert()
#748 FILE: tests/oss-fuzz/miniflow_target.c:258:
assert(minimask_is_catchall(minimask)

ERROR: Use ovs_assert() in place of assert()
#750 FILE: tests/oss-fuzz/miniflow_target.c:260:
assert(miniflow_equal_in_minimask(miniflow, miniflow2, minimask));

ERROR: Use ovs_assert() in place of assert()
#751 FILE: tests/oss-fuzz/miniflow_target.c:261:
assert(miniflow_equal_flow_in_minimask(miniflow, , minimask));

ERROR: Use ovs_assert() in place of assert()
#752 FILE: tests/oss-fuzz/miniflow_target.c:262:
assert(miniflow_hash_in_minimask(miniflow, minimask, 0x12345678) ==

ERROR: Use ovs_assert() in place of assert()
#754 FILE: tests/oss-fuzz/miniflow_target.c:264:
assert(minimask_hash(minimask, 0) ==

ERROR: Use ovs_assert() in place of assert()
#760 FILE: tests/oss-fuzz/miniflow_target.c:270:
assert(!miniflow_equal_flow_in_minimask(miniflow, , minimask));

ERROR: Use ovs_assert() in place of assert()
#762 FILE: tests/oss-fuzz/miniflow_target.c:272:
assert(!miniflow_equal_in_minimask(miniflow, miniflow3, minimask));

ERROR: Use ovs_assert() in place of assert()
#778 FILE: tests/oss-fuzz/miniflow_target.c:288:
assert(minimask_is_catchall(minicatchall));

ERROR: Use ovs_assert() in place of assert()
#785 FILE: tests/oss-fuzz/miniflow_target.c:295:
assert(!minimask_has_extra(minimask, minimask));

ERROR: Use ovs_assert() in place of assert()
#786 FILE: tests/oss-fuzz/miniflow_target.c:296:
assert(minimask_has_extra(minicatchall, minimask)

ERROR: Use ovs_assert() in place of assert()
#793 FILE: tests/oss-fuzz/miniflow_target.c:303:
assert(minimask_has_extra(minimask2, minimask));

ERROR: Use ovs_assert() in place of assert()
#794 FILE: tests/oss-fuzz/miniflow_target.c:304:
assert(!minimask_has_extra(minimask, minimask2));

ERROR: Use ovs_assert() in place of assert()
#810 FILE: tests/oss-fuzz/miniflow_target.c:320:
assert(minimask_is_catchall(minicatchall));

ERROR: Use ovs_assert() in place of assert()
#824 FILE: tests/oss-fuzz/miniflow_target.c:334:
assert(minimask_is_catchall());

ERROR: Use ovs_assert() in place of assert()
#833 FILE: tests/oss-fuzz/miniflow_target.c:343:
assert(flow_wildcards_equal(, ));

Lines checked: 861, Warnings: 1, Errors: 22


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ossfuzz: Speed up flow extract fuzzing by refactoring

[bshastry]

From: Bhargava Shastry 

Refactor miniflow tests out of flow_extract_target.c into a
 new target called miniflow_target.c

The biggest motivation for this massive (7-10x) increase in fuzzing
speed. Prior to the refactoring, we were doing roughly 900 executions
per second on flow_extract_target. Now, we are doing roughly 6000
executions per second on the flow_extract_target and roughly 9000
executions per second on the new miniflow_target.

Moving forward, creating micro fuzz targets that are really fast is a
better strategy. Since all these micro targets can be scheduled in
parallel by oss-fuzz, the test throughput increases by a non-trivial
amount.

Signed-off-by: Bhargava Shastry 
---
 tests/oss-fuzz/automake.mk|  12 +-
 tests/oss-fuzz/config/miniflow_target.options |   3 +
 tests/oss-fuzz/flow_extract_target.c  | 367 ++
 tests/oss-fuzz/miniflow_target.c  | 366 +
 4 files changed, 410 insertions(+), 338 deletions(-)
 create mode 100644 tests/oss-fuzz/config/miniflow_target.options
 create mode 100644 tests/oss-fuzz/miniflow_target.c

diff --git a/tests/oss-fuzz/automake.mk b/tests/oss-fuzz/automake.mk
index 4fbdb4c2b..2c506be3d 100644
--- a/tests/oss-fuzz/automake.mk
+++ b/tests/oss-fuzz/automake.mk
@@ -3,7 +3,8 @@ OSS_FUZZ_TARGETS = \
tests/oss-fuzz/json_parser_target \
tests/oss-fuzz/ofp_print_target \
tests/oss-fuzz/expr_parse_target \
-   tests/oss-fuzz/odp_target
+   tests/oss-fuzz/odp_target \
+   tests/oss-fuzz/miniflow_target
 EXTRA_PROGRAMS += $(OSS_FUZZ_TARGETS)
 oss-fuzz-targets: $(OSS_FUZZ_TARGETS)
 
@@ -38,12 +39,19 @@ tests_oss_fuzz_odp_target_SOURCES = \
 tests_oss_fuzz_odp_target_LDADD = lib/libopenvswitch.la
 tests_oss_fuzz_odp_target_LDFLAGS = $(LIB_FUZZING_ENGINE) -lc++
 
+tests_oss_fuzz_miniflow_target_SOURCES = \
+tests/oss-fuzz/miniflow_target.c \
+tests/oss-fuzz/fuzzer.h
+tests_oss_fuzz_miniflow_target_LDADD = lib/libopenvswitch.la
+tests_oss_fuzz_miniflow_target_LDFLAGS = $(LIB_FUZZING_ENGINE) -lc++
+
 EXTRA_DIST += \
tests/oss-fuzz/config/flow_extract_target.options \
tests/oss-fuzz/config/json_parser_target.options \
tests/oss-fuzz/config/ofp_print_target.options \
tests/oss-fuzz/config/expr_parse_target.options \
tests/oss-fuzz/config/odp_target.options \
+   tests/oss-fuzz/config/miniflow_target.options \
tests/oss-fuzz/config/ovs.dict \
tests/oss-fuzz/config/expr.dict \
-   tests/oss-fuzz/config/odp.dict
+   tests/oss-fuzz/config/odp.dict
\ No newline at end of file
diff --git a/tests/oss-fuzz/config/miniflow_target.options 
b/tests/oss-fuzz/config/miniflow_target.options
new file mode 100644
index 0..4849f282d
--- /dev/null
+++ b/tests/oss-fuzz/config/miniflow_target.options
@@ -0,0 +1,3 @@
+[libfuzzer]
+dict = ovs.dict
+close_fd_mask = 3
\ No newline at end of file
diff --git a/tests/oss-fuzz/flow_extract_target.c 
b/tests/oss-fuzz/flow_extract_target.c
index 3ad1a9628..9e82675e3 100644
--- a/tests/oss-fuzz/flow_extract_target.c
+++ b/tests/oss-fuzz/flow_extract_target.c
@@ -10,338 +10,36 @@
 #include "classifier-private.h"
 
 static void
-shuffle_u32s(uint32_t *p, size_t n)
+test_flow_hash(const struct flow *flow)
 {
-for (; n > 1; n--, p++) {
-uint32_t *q = [random_range(n)];
-uint32_t tmp = *p;
-*p = *q;
-*q = tmp;
-}
-}
-
-/* Returns a copy of 'src'.  The caller must eventually free the returned
- * miniflow with free(). */
-static struct miniflow *
-miniflow_clone__(const struct miniflow *src)
-{
-struct miniflow *dst;
-size_t data_size;
-
-data_size = miniflow_alloc(, 1, src);
-miniflow_clone(dst, src, data_size / sizeof(uint64_t));
-return dst;
-}
-
-/* Returns a hash value for 'flow', given 'basis'. */
-static inline uint32_t
-miniflow_hash__(const struct miniflow *flow, uint32_t basis)
-{
-const uint64_t *p = miniflow_get_values(flow);
-size_t n_values = miniflow_n_values(flow);
-struct flowmap hash_map = FLOWMAP_EMPTY_INITIALIZER;
-uint32_t hash = basis;
-size_t idx;
-
-FLOWMAP_FOR_EACH_INDEX (idx, flow->map) {
-uint64_t value = *p++;
-
-if (value) {
-hash = hash_add64(hash, value);
-flowmap_set(_map, idx, 1);
-}
-}
-map_t map;
-FLOWMAP_FOR_EACH_MAP (map, hash_map) {
-hash = hash_add64(hash, map);
-}
-
-return hash_finish(hash, n_values);
-}
-
-static uint32_t
-random_value(void)
-{
-static const uint32_t values_[] =
-{ 0x, 0x, 0x, 0x8000,
-  0x0001, 0xface, 0x00d00d1e, 0xdeadbeef };
-
-return values_[random_range(ARRAY_SIZE(values_))];
-}
-
-static bool
-choose(unsigned int n, unsigned int *idxp)
-{
-if (*idxp < n) {
-return true;
-} else {
-*idxp -= n;
-return false;
-}
-}
-
-#define FLOW_U32S (FLOW_U64S * 2)
-

Re: [ovs-dev] [PATCH] ovn: Fix IPv6 DAD failure for child ports

[Numan Siddique]

On Fri, Oct 5, 2018 at 2:32 AM Guru Shetty  wrote:

>
>
> On Thu, 4 Oct 2018 at 13:29, Numan Siddique  wrote:
>
>>
>> Thanks for the review Guru.
>>
>> Please see below for the comments.
>>
>>
>>
>> On Fri, Oct 5, 2018 at 12:09 AM Guru Shetty  wrote:
>>
>>>
>>>
>>> On Mon, 17 Sep 2018 at 02:24,  wrote:
>>>
 From: Numan Siddique 

 When a child vlan interface is created inside a VM, the below kernel
 message
 is seen and IPv6 doesn't work on that interface.

>>>
>>> On which interface doesn't IPv6 work? On the Vm's interface or on the
>>> container's interface?
>>>
>>>
>>
>> It is seen on the container's interface.
>>
>>>
 [  138.000753] IPv6: vlan4: IPv6 duplicate address  detected!

 When a child port sends a broadcast packet, OVN delivers the same
 packet back to the child port (and hence the DAD check fails).

>>>
>>> Is this limited to IP broadcast only? Or does this happen with mac
>>> broadcast too? (I am surprised why I hadn't seen this behavior with ARP
>>> packets of containers).
>>>
>>
>> It happens with mac broadcast. You can actually reproduce the issue by
>> applying the code in tests/ovn.at without this patch.
>>
>> Looks like I have confused you with the patch.  Let me try to give an
>> example. Suppose we have switch - sw0 - with ports p1, p2 and p3 and all of
>> them reside in the same chassis.
>> When p1 sends a broadcast/multicast packet, the below logical flow is hit
>>
>> table=17(ls_in_l2_lkup  ), priority=100  , match=(eth.mcast),
>> action=(outport = "_MC_flood"; output;)
>>
>> This translates to the OF flow
>> - metadata=0x1,dl_dst=01:00:00:00:00:00/01:00:00:00:00:00
>> actions=load:0x->NXM_NX_REG15[],resubmit(,32)
>>
>> In the table 33 if reg15 has 0x, it will resubmit the flow with reg15
>> set to each logical port
>>
>> i.e reg15=0x,metadata=0x1
>> actions=load:0x1->NXM_NX_REG15[],resubmit(,34),load:0x2>NXM_NX_REG15[],resubmit(,34),load:0x3->NXM_NX_REG15[],resubmit(,34),load:0x->NXM_NX_REG15[]
>>
>> In the normal case, the packet will not be delivered back to p1 and
>> instead will be dropped in table 34 (OFTABLE_CHECK_LOOPBACK)
>> since reg10[0] is not set.
>>
>> table=34, n_packets=0, n_bytes=0,
>> priority=100,reg10=0/0x1,reg14=0x2,reg15=0x2,metadata=0x1 actions=drop
>>
>> When the register reg10[0] is set, it means the packet needs to be loop
>> backed to the source port. So table 64 has flows to clear the in_port
>> i,.e
>> table=64,  priority=100,reg10=0x1/0x1,reg15=0x1,metadata=0x1
>> actions=push:NXM_OF_IN_PORT[],load:0->NXM_OF_IN_PORT[],resubmit(,65),pop:NXM_OF_IN_PORT[]
>>
>> Now if we take the example of child ports, let's say "p2" is the child
>> port of "p1" and in_port of p1 is 1
>> In table 0, we will see 2 flows for in_port=1
>>
>> match=(in_port=1) -
>> actions=(load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[] and submit to
>> table 8)
>> match=(in_port=1, dl_vlan=4) actions(load:0x1->NXM_NX_REG10[0],
>> strip_vlan, load:0x1->OXM_OF_METADATA[],load:0x2->NXM_NX_REG14[] , and
>> submit to table 8)
>>
>> Suppose if p2 sends a braodcast packet, reg10[0] is set in table 0.
>>
>> In this case, since reg10[0] is set, the broadcast packet is not dropped
>> in table 34 for port "p2" and it gets delivered back.
>> So this patch instead of using reg10[0], it uses a new register bit
>> -MLF_NESTED_CONTAINER_BIT
>>
>
> Thanks for the detailed response. I understand till the above point. So
> not using NXM_NX_REG10[0] (or MLF_ALLOW_LOOPBACK_BIT) actually lets the
> packet drop in table 34.  Can you please add that information to the commit
> message. I think it will be helpful in the future.
>
> I then don't understand, what we are doing with the new code
> in OFTABLE_SAVE_INPORT.  I would expect that the new code is needed to save
> inport if the destination is a nested container. Not quite understand why
> we have a match for parent port. I will wait for the comment in the code to
> understand it.
>

The code to push, clear the inport and pop the inport is already present
for the child ports -
https://github.com/openvswitch/ovs/blob/master/ovn/controller/physical.c#L265
And the code to clear the inport for the parent port or any port when
reg10[0] is set is here -
https://github.com/openvswitch/ovs/blob/master/ovn/controller/physical.c#L256

Now with this patch since we are using reg10[5] to indicate the packet is
from child ports, we need to clear the in_port if the packet from the child
port needs to be sent to the parent port.
And that's why have a new match for the parent ports.

I changed the signature of the function 'put_local_common_flows' because I
wanted to add the flow to check reg10[5] only if a port is a parent port.
Otherwise we will add the flow to check reg10[5] for all
the ports in the switch.

I will add proper documentation and make it clear.

Thanks
Numan


>
>
>>
>> We also need to clear the in_port in table 64 if MLF_NESTED_CONTAINER_BIT
>> is set.
>>
>> I will address the review 

Re: [ovs-dev] [PATCH v2] dpif-netlink: don't allocate per thread netlink sockets

[Markos Chandras]

On 05/10/2018 09:55, Matteo Croce wrote:
> On Fri, Oct 5, 2018 at 8:32 AM Markos Chandras  wrote:
>>
>> On 25/09/2018 22:14, Ben Pfaff wrote:
>>>
>>> Applied to master thanks!
>>>
>>> I sent a patch to remove support for multiple queues in the
>>> infrastructure layer:
>>> https://patchwork.ozlabs.org/patch/974755/
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>> Hello Ben and Matteo,
>>
>> This patch applies cleanly to branch-2.10, branch-2.9, and branch-2.8.
>> Would it make sense to backport it to these as well?
>>
>> --
>> markos
>>
>> SUSE LINUX GmbH | GF: Felix Imendörffer, Jane Smithard, Graham Norton
>> HRB 21284 (AG Nürnberg) Maxfeldstr. 5, D-90409, Nürnberg
> 
> Hi Markos,
> 
> the file descriptor limit issue was first reported on OVS 2.8, so it's worth 
> it.
> If you backport it, please consider backporting the following commit from Ben,
> which removes some code which become unused after my patch:
> 
> commit 769b50349f28c5f9e4bff102bc61dadcb9b99c37
> Author: Ben Pfaff 
> Date:   Tue Sep 25 15:14:13 2018 -0700
> 
> dpif: Remove support for multiple queues per port.
> 
> Regards,
> 

Thank you Matteo,

Ben, please let me know if you can backport these up to branch-2.8
otherwise I can send patches for each branch myself.

-- 
markos

SUSE LINUX GmbH | GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg) Maxfeldstr. 5, D-90409, Nürnberg
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpif-netlink: don't allocate per thread netlink sockets

[Matteo Croce]

On Fri, Oct 5, 2018 at 8:32 AM Markos Chandras  wrote:
>
> On 25/09/2018 22:14, Ben Pfaff wrote:
> >
> > Applied to master thanks!
> >
> > I sent a patch to remove support for multiple queues in the
> > infrastructure layer:
> > https://patchwork.ozlabs.org/patch/974755/
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> Hello Ben and Matteo,
>
> This patch applies cleanly to branch-2.10, branch-2.9, and branch-2.8.
> Would it make sense to backport it to these as well?
>
> --
> markos
>
> SUSE LINUX GmbH | GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg) Maxfeldstr. 5, D-90409, Nürnberg

Hi Markos,

the file descriptor limit issue was first reported on OVS 2.8, so it's worth it.
If you backport it, please consider backporting the following commit from Ben,
which removes some code which become unused after my patch:

commit 769b50349f28c5f9e4bff102bc61dadcb9b99c37
Author: Ben Pfaff 
Date:   Tue Sep 25 15:14:13 2018 -0700

dpif: Remove support for multiple queues per port.

Regards,
-- 
Matteo Croce
per aspera ad upstream
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpif-netlink: don't allocate per thread netlink sockets

[Markos Chandras]

On 25/09/2018 22:14, Ben Pfaff wrote:
> 
> Applied to master thanks!
> 
> I sent a patch to remove support for multiple queues in the
> infrastructure layer:
> https://patchwork.ozlabs.org/patch/974755/
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 

Hello Ben and Matteo,

This patch applies cleanly to branch-2.10, branch-2.9, and branch-2.8.
Would it make sense to backport it to these as well?

-- 
markos

SUSE LINUX GmbH | GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg) Maxfeldstr. 5, D-90409, Nürnberg
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev