Re: [ovs-dev] [PATCH] dpif-netdev: Fix unsafe accesses to pmd polling lists.
On 11.02.2019 22:15, Aaron Conole wrote:
> Ilya Maximets writes:
>
>> All accesses to 'pmd->poll_list' should be guarded by
>> 'pmd->port_mutex'. Additionally fixed inappropriate usage
>> of 'HMAP_FOR_EACH_SAFE' (hmap doesn't change in a loop)
>> and dropped not needed local variable 'proc_cycles'.
>>
>> CC: Nitin Katiyar
>> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
>> Signed-off-by: Ilya Maximets
>> ---
>> lib/dpif-netdev.c | 14 +-
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 4d5bc576a..914b2bb8b 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -3776,9 +3776,12 @@ set_pmd_auto_lb(struct dp_netdev *dp)
>> continue;
>> }
>>
>> +ovs_mutex_lock(&pmd->port_mutex);
>> if (hmap_count(&pmd->poll_list) > 1) {
>> multi_rxq = true;
>> }
>> +ovs_mutex_unlock(&pmd->port_mutex);
>
> What does this mutex provide here? I ask because as soon as we unlock,
> the result is no longer assured, and we've used it to inform the
> multi_rxq value.
>
> Are you afraid that the read is non-atomic so you'll end up with the
> multi_rxq condition true, even when it should not be? That can happen
> anyway - as soon as we unlock the count can change.
>
> Maybe there's a better change to support that, where hmap_count does an
> atomic read, or there's a version that can guarantee a full load with
> no intervening writes. But I don't see
>
> Maybe the pmd object lifetime goes away... but that wouldn't make sense,
> because then the port_mutex itself would be invalid.
>
> I'm confused what it does to add a lock here for either the safety, or
> the logic. I probably missed something. Or maybe it's just trying to
> be safe in case the hmap implementation would change in the future? I
> think if that's the case, there might be an alternate to implement?
The mutex here is mostly for a code consistency. We're marking the 'poll_list'
as OVS_GUARDED. This means that we should guard it. Unfortunately, unlike С++,
in C we can not use OVS_GUARDED_BY(port_mutex) inside the structure and
clang could not track what metex should protect the variable. I agree that we're
not really need to protect this. Furthermore, the only writer for 'poll_list'
is the main thread and 'set_pmd_auto_lb' is also called only in a main thread.
So we don't need to protect any read accesses.
However, for the safety, these details should not be taken into account to not
provide code snippets that could be copied to the other (less safe) places and
produce issues there. Especially because clang can't protect us from this kind
of mistakes. Code architecture also could be changed in the future.
As a developer I'd like to always assume that any access to hmap is not thread
safe regardless of its internal implementation. Just like I'm assuming that
cmap is safe for readers.
This is affordable since we're not on a hot path.
>
>> +
>> if (cnt && multi_rxq) {
>> enable_alb = true;
>> break;
>> @@ -5095,7 +5098,7 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
>> uint64_t improvement = 0;
>> uint32_t num_pmds;
>> uint32_t *pmd_corelist;
>> -struct rxq_poll *poll, *poll_next;
>> +struct rxq_poll *poll;
>> bool ret;
>>
>> num_pmds = cmap_count(&dp->poll_threads);
>> @@ -5121,13 +5124,14 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
>> /* Estimate the cycles to cover all intervals. */
>> total_cycles *= PMD_RXQ_INTERVAL_MAX;
>>
>> -HMAP_FOR_EACH_SAFE (poll, poll_next, node, &pmd->poll_list) {
>> -uint64_t proc_cycles = 0;
>> +ovs_mutex_lock(&pmd->port_mutex);
>> +HMAP_FOR_EACH (poll, node, &pmd->poll_list) {
>> for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
>> -proc_cycles += dp_netdev_rxq_get_intrvl_cycles(poll->rxq,
>> i);
>> +total_proc += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
>> }
>> -total_proc += proc_cycles;
>> }
>> +ovs_mutex_unlock(&pmd->port_mutex);
>> +
>
> This lock, otoh, makes sense to me.
>
>> if (total_proc) {
>> curr_pmd_usage[num_pmds] = (total_proc * 100) / total_cycles;
>> }
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v6 1/1] acinclude: Also use LIBS from dpkg pkg-config
DPDK 18.11 builds using the more modern meson build system no more provide the -ldpdk linker script. Instead it is expected to use pkgconfig for linker options as well. This change will set DPDK_LIB from pkg-config (if pkg-config was available) and since that already carries the whole-archive flags around the PMDs skips the further wrapping in more whole-archive if that is already part of DPDK_LIB. To work reliable in all environments this needs pkg-config 0.29.1. We want to be able to use PKG_CHECK_MODULES_STATIC which is not yet available in 0.24. Therefore update pkg.m4 to pkg-config 0.29.1. This should be backport-safe as these macro files are all versioned. autoconf is smart enough to check the version if you have it locally, and if the system's is higher, it will use that one instead. Acked-by: Luca Boccassi Acked-by: Aaron Conole Signed-off-by: Christian Ehrhardt Signed-off-by: Christian Ehrhardt --- acinclude.m4 | 22 -- m4/pkg.m4| 217 +-- 2 files changed, 155 insertions(+), 84 deletions(-) diff --git a/acinclude.m4 b/acinclude.m4 index c51af246a..1fb6dc61f 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -223,9 +223,13 @@ AC_DEFUN([OVS_CHECK_DPDK], [ case "$with_dpdk" in yes) DPDK_AUTO_DISCOVER="true" -PKG_CHECK_MODULES([DPDK], [libdpdk], - [DPDK_INCLUDE="$DPDK_CFLAGS"], - [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"]) +PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk], [ +DPDK_INCLUDE="$DPDK_CFLAGS" +DPDK_LIB="$DPDK_LIBS" + ], [ +DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk" +DPDK_LIB="-ldpdk" + ]) ;; *) DPDK_AUTO_DISCOVER="false" @@ -238,11 +242,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [ DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk" fi DPDK_LIB_DIR="$with_dpdk/lib" +DPDK_LIB="-ldpdk" ;; esac -DPDK_LIB="-ldpdk" - ovs_save_CFLAGS="$CFLAGS" ovs_save_LDFLAGS="$LDFLAGS" CFLAGS="$CFLAGS $DPDK_INCLUDE" @@ -342,7 +345,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [ # # These options are specified inside a single -Wl directive to prevent # autotools from reordering them. -DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive +# +# OTOH newer versions of dpdk pkg-config (generated with Meson) +# will already have flagged just the right set of libs with +# --whole-archive - in those cases do not wrap it once more. +case "$DPDK_LIB" in + *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;; + *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive +esac AC_SUBST([DPDK_vswitchd_LDFLAGS]) AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.]) fi diff --git a/m4/pkg.m4 b/m4/pkg.m4 index c5b26b52e..82bea96ee 100644 --- a/m4/pkg.m4 +++ b/m4/pkg.m4 @@ -1,29 +1,60 @@ -# pkg.m4 - Macros to locate and utilise pkg-config.-*- Autoconf -*- -# serial 1 (pkg-config-0.24) -# -# Copyright © 2004 Scott James Remnant . -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -# General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. -# -# As a special exception to the GNU General Public License, if you -# distribute this file as part of a program that contains a -# configuration script generated by Autoconf, you may include it under -# the same distribution terms that you use for the rest of that program. - -# PKG_PROG_PKG_CONFIG([MIN-VERSION]) -# -- +dnl pkg.m4 - Macros to locate and utilise pkg-config. -*- Autoconf -*- +dnl serial 11 (pkg-config-0.29.1) +dnl +dnl Copyright © 2004 Scott James Remnant . +dnl Copyright © 2012-2015 Dan Nicholson +dnl +dnl This program is free software; you can redistribute it and/or modify +dnl it under the terms of the GNU General Public License as published by +dnl the Free Software Foundation; either version 2 of the License, or +dnl (at your option) any later version. +dnl +dnl This program is distributed in the hope that it will be useful, but +dnl WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl General Pub
[ovs-dev] [PATCH v6 0/1] fix build with DPDK 18.11 without -ldpdk
DPDK 18.11 can be built using the more modern meson build system. In that case it no more provides the -ldpdk linker script. Instead it is expected to use pkgconfig for linker options as well. FYI here a build log on Ubuntu 19.04 with the three patches applied: https://launchpadlibrarian.net/409021278/buildlog_ubuntu-disco-amd64.openvswitch_2.11.0~git20190121.4e4f80ec2-0ubuntu1~ppa5_BUILDING.txt.gz Updates from v5: - rebased again - removed remaining whitespace damage in configure.ac Updates from v4: - drop explicit m4_include([m4/pkg.m4]) as aclocal should be sufficient - add Acked-by's I got so far to the commit message Updates from v3: - change separation of multiple actions in PKG_CHECK_MODULES_STATIC to newlines to avoid trailing commas in some build environments Updates from v2: - patches were formerly structured for reviewability, but to guarantee travis requirements to pass the build individually and not as a series they are now squashed to be acceptable Updates from v1: - add updated pkg.m4 to support PKG_CHECK_MODULES_STATIC - include local pkg.m4 in configure.ac Christian Ehrhardt (1): acinclude: Also use LIBS from dpkg pkg-config acinclude.m4 | 22 -- m4/pkg.m4| 217 +-- 2 files changed, 155 insertions(+), 84 deletions(-) -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 1/1] acinclude: Also use LIBS from dpkg pkg-config
On Tue, Feb 12, 2019 at 2:52 AM Ben Pfaff wrote: > > On Mon, Feb 11, 2019 at 08:36:47PM +0100, Christian Ehrhardt wrote: > > DPDK 18.11 builds using the more modern meson build system no more > > provide the -ldpdk linker script. Instead it is expected to use > > pkgconfig for linker options as well. > > > > This change will set DPDK_LIB from pkg-config (if pkg-config was > > available) and since that already carries the whole-archive flags > > around the PMDs skips the further wrapping in more whole-archive > > if that is already part of DPDK_LIB. > > > > To work reliable in all environments this needs pkg-config 0.29.1. > > We want to be able to use PKG_CHECK_MODULES_STATIC which > > is not yet available in 0.24. Therefore update pkg.m4 > > to pkg-config 0.29.1. > > > > This should be backport-safe as these macro files are all versioned. > > autoconf is smart enough to check the version if you have it locally, > > and if the system's is higher, it will use that one instead. > > > > Acked-by: Luca Boccassi > > Acked-by: Aaron Conole > > Signed-off-by: Christian Ehrhardt > > Like the robot, I wasn't able to apply this. I guess it needs a rebase. I rebased minutes before I sent it, but by accident to "another" master branch I from another remote :-) But anyway, we are already at V5 so a V6 isn't far away > Thank you! -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Updated Quotation
Good Morning. Kindly send us an updated quotation. If the attached still valid, please let us know. Thanks. Cordialement, Best Regards ___ Marie GERBAUD BERNARD Assistante Export Tel : +33 1 64 86 27 86 Fax : +33 1 69 10 65 49 E-mail : marie.bern...@htds.fr Parc d'Activit�s du Moulin de Massy - 3, rue du Saule Trapu - BP 246 - 91 882 Massy Cedex 13 France --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v4 2/2] conntrack: Exclude l2 padding in 'conn_key_extract()'.
On Mon, Feb 04, 2019 at 04:23:07PM -0800, Darrell Ball wrote:
> 'conn_key_extract()' in userspace conntrack is including L2
> (Ethernet) pad bytes for both L3 and L4 sizes. One problem is
> any packet with non-zero L2 padding can incorrectly fail L4
> checksum validation.
>
> This patch fixes conn_key_extract() by ignoring L2 pad bytes.
>
> Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
> CC: Daniele Di Proietto
> Co-authored-by: Vishal Deep Ajmera
> Co-authored-by: Venkatesan Pradeep
> Co-authored-by: Nitin Katiyar
> Signed-off-by: Vishal Deep Ajmera
> Signed-off-by: Venkatesan Pradeep
> Signed-off-by: Nitin Katiyar
> Signed-off-by: Darrell Ball
Thanks.
I applied this series to master. Please let me know what branches I
should backport it to.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 4/5] ovn: Support OVS action 'check_pkt_larger' in OVN
On Thu, Jan 10, 2019 at 11:31:25PM +0530, nusid...@redhat.com wrote: > From: Numan Siddique > > Previous commit added a new OVS action 'check_pkt_larger'. This > patch supports that action in OVN. The syntax to use this would be > > reg0[0] = check_pkt_larger(LEN) > > Upcoming commit will make use of this action in ovn-northd and > will generate an ICMP4 packet if the packet length is greater than > the specified length. > > Signed-off-by: Numan Siddique Thanks In pinctrl_handle_icmp(), this attempts to add two integers that are in network byte order, without first converting them to host byte order: nh->ip_tot_len += htons(orig_ip_datagram_len); I think that the code that includes the original IP header in pinctrl_handle_icmp() should handle the case where the original IP header is longer than 20 bytes (and it should also handle the case where it is short or invalid or followed by fewer than 8 bytes of payload). I guess that ovn-trace should allow the user to specify what value to return. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 3/5] ovn: Add a new OVN field icmp4.frag_mtu
On Thu, Jan 10, 2019 at 11:30:58PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique
>
> In order to support OVN specific fields (which are not yet
> supported in OpenvSwitch to set or modify values) a generic
> OVN field support is added in this patch. These OVN fields are
> expected to be used as nested OVN actions inside OVN actions
> like icmp4, icmp6 etc. This patch adds only one field for now
> - icmp4.frag_mtu. It should be fairly straightforward to
> add similar fields in the near future.
>
> This field is expected to be used as an inner field with in
> the 'icmp4' action.
>
> Eg.
> "icmp4 {"eth.dst <-> eth.src; "
> "icmp4.type = 3; /* Destination Unreachable */ "
> "icmp4.code = 4; /* Fragmentation Needed */ "
> icmp4.frag_mtu = 1442;
> ...
> "next; };",
>
> pinctrl module of ovn-controller will set the specified value
> in the the low-order 16 bits of the ICMP4 header field that is
> labelled "unused" in the ICMP specification as defined in the RFC 1191.
>
> Upcoming patch will use it to send an icmp4 packet if the
> source IPv4 packet destined to go via external gateway needs to
> be fragmented.
>
> Signed-off-by: Numan Siddique
Thanks!
This ntohs in encode_OVNFIELD_LOAD should be htons:
oah->len = ntohs(sizeof(ovs_be16));
In encode_nested_actions(), it's unsafe to use the pointer
n_ovnfields_acts after calling ovnacts_encode(), because the buffer
might have been reallocated.
I don't understand why icmp4.frag_mtu is so heavily special-cased that
it only works inside nested actions.
Thanks,
Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 2/5] Add a new OVS action check_pkt_larger
On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusid...@redhat.com wrote: > From: Numan Siddique > > This patch adds a new action 'check_pkt_larger' which checks if the > packet is larger than the given size and stores the result in the > destination register. Thanks. This patch renumbers OVS_ACTION_ATTR_CLONE and moves it into the "userspace only" group of datapath actions. It should not do that. It should also not remove OVS_CLONE_ATTR_EXEC. ofpact_remaining_len() should not return bool. (This mistake suggests that the case where it should return nonzero was not tested.) I don't understand the check for check_pkt_larger->dst.field in xlate_check_pkt_larger(). The action would be invalid if this was missing. Such an ofpact should not be translated. The following comment appears incomplete: /* If datapath doesn't support check_pkt_len action, then set the * SLOW_ACTION flag so that .. */ odp-util.c has code for formatting the new action, but not for parsing. We need both. The format might be a little too cute. It might be wise to use something more like check_pkt_len(size=123, gt(...), le(...)). decode_OFPAT_RAW_CHECK_PKT_LARGER() should check that the field specified and the bits in it are valid and usable and return an error if they are not. As is, the OpenFlow action format only allows NXM headers. All new actions should also support OXM 64-bit experimenter headers. Look around at examples of mf_vl_mff_nx_pull_header() and nx_put_mff_header() for more information. The OpenFlow syntax looks really weird. I don't understand it at all. I doubt that the action translation handles the case where freezing has to happen properly. I don't think we have any other actions where there are two opportunities to freeze translation. This might require novel work. In check_check_pkt_len(), the inner ct_clear action kind of mixes up two different checks. I don't think that check_pkt_len should rely on ct_clear. I recommend putting some other action inside, if one is needed. There isn't any documentation, but some is needed. Tests are needed. A NEWS item is appropriate. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] monitor.c: Fix crash when monitor condition adds new columns.
Bleep bloop. Greetings Han Zhou, 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 is 110 characters long (recommended limit is 79)
ERROR: C99 style comment
#171 FILE: ovsdb/monitor.c:463:
//VLOG_WARN("dbmon %p, cond_add_column: %s, n_columns %"PRIuSIZE,
dbmon, columns[i]->name, n_columns);
WARNING: Line is 117 characters long (recommended limit is 79)
ERROR: C99 style comment
#187 FILE: ovsdb/monitor.c:538:
//VLOG_WARN("untrack change: %"PRIu64", found changes %p, n_refs %d",
transaction, changes, changes->n_refs);
WARNING: Line is 123 characters long (recommended limit is 79)
ERROR: C99 style comment
#189 FILE: ovsdb/monitor.c:540:
//VLOG_WARN("untrack change: %"PRIu64", destroy changes %p, n_refs
%d", transaction, changes, changes->n_refs);
WARNING: Line is 119 characters long (recommended limit is 79)
ERROR: C99 style comment
#197 FILE: ovsdb/monitor.c:558:
//VLOG_WARN("track new change: %"PRIu64", found changes %p, n_refs %d",
transaction, changes, changes->n_refs);
ERROR: C99 style comment
#200 FILE: ovsdb/monitor.c:561:
//VLOG_WARN("track new change: %"PRIu64, transaction);
ERROR: C99 style comment
#342 FILE: ovsdb/monitor.c:1617:
//VLOG_WARN("monitor commit: m_transactions: %"PRIu64, m->n_transactions);
Lines checked: 425, Warnings: 4, Errors: 6
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] [RFC v3 1/5] datapath: Add a new action check_pkt_len
Greg, I recommend that you take a look at this one. (More commentary below.) On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusid...@redhat.com wrote: > From: Numan Siddique > > [Please note, this patch is submitted as RFC in ovs-dev ML to > get feedback before submitting to netdev ML] > > This patch adds a new action which checks the packet length and > executes a set of actions if the packet length is greater than > the specified length or executes another set of actions if the > packet length is lesser or equal to. > > This action takes below nlattrs > * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for > > * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions > to apply if the packet length is greater than the specified 'pkt_len' > > * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested > actions to apply if the packet length is lesser or equal to the > specified 'pkt_len'. > > The main use case for adding this action is to solve the packet > drops because of MTU mismatch in OVN virtual networking solution. > When a VM (which belongs to a logical switch of OVN) sends a packet > destined to go via the gateway router and if the nic which provides > external connectivity, has a lesser MTU, OVS drops the packet > if the packet length is greater than this MTU. > > With the help of this action, OVN will check the packet length > and if it is greater than the MTU size, it will generate an > ICMP packet (type 3, code 4) and includes the next hop mtu in it > so that the sender can fragment the packets. I'm not used to seeing series where different patches apply to different repos! It took me a while to figure out why "git am" didn't want to work at all (but then I actually looked at the patch). I didn't have a net-next tree handy so my comments are based on reading the patch without applying or building it. There is a special issue with this action, which is a lot like a special issue with the "sample". That is that the action has flow control that might change the flow in different ways, and this can make an important difference for actions that follow this one. If one fork of the action pops off a header, and the other one does not, then that makes validating actions that come after it complicated. I do not see anything here that takes that into account. I recommend reading the validation code for the sample action for inspiration. In validate_and_copy_check_pkt_len(), it might be better to use nla_policy and nla_parse_nested(). Or maybe not. I did not look closely. I don't think that execute_check_pkt_len() should need to check for netlink format errors. The validation code should take care of that. It might also be able to assume a particular order for the attributes, depending on how you implement validation. I'm still not thrilled by the naming. I don't have any wonderful names though. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] monitor.c: Fix crash when monitor condition adds new columns.
From: Han Zhou
The OVSDB conditional monitor implementation allows many clients
to share same copy of monitored data if the clients are sharing
same tables and columns being monitored, while they can have
different monitor conditions. In monitor conditions they can
have different columns which can be different from the columns
being monitored. So the struct ovsdb_monitor_table maintains the
union of the all the columns being used in any conditions.
The problem of the current implementation is that for each change
set generated, it doesn't maintain any metadata for the number of
columns for the data that has already populated in it. Instead, it
always rely on the n_columns field of the struct ovsdb_monitor_table
to manipulate the data. However, the n_columns in struct
ovsdb_monitor_table can increase (e.g. when a client changes its
condition which involves more columns). So it can result in that
the existing rows in a change set with N columns being later processed
as if it had more than N columns, typically, when the row is freed.
This causes the ovsdb-server crashing (see an example of the
backtrace).
The patch fixes the problem by maintaining n_columns for each
change set, and added a test case which fails without the fix.
(gdb) bt
at lib/ovsdb-data.c:1031
out>, mt=) at ovsdb/monitor.c:320
mt=0x1e7b940) at ovsdb/monitor.c:333
out>, transaction=) at ovsdb/monitor.c:527
initial=, cond_updated=cond_updated@entry=false,
unflushed_=unflushed_@entry=0x20dae70,
condition=, version=) at ovsdb/monitor.c:1156
(m=m@entry=0x20dae40, initial=initial@entry=false) at
ovsdb/jsonrpc-server.c:1655
at ovsdb/jsonrpc-server.c:1729
ovsdb/jsonrpc-server.c:551
ovsdb/jsonrpc-server.c:586
ovsdb/jsonrpc-server.c:401
exiting=0x7ffdb947f76f, run_process=0x0, remotes=0x7ffdb947f7c0,
unixctl=0x1e7a560, all_dbs=0x7ffdb947f800,
jsonrpc=, config=0x7ffdb947f820) at ovsdb/ovsdb-server.c:209
Signed-off-by: Han Zhou
---
v1->v2: remove debug logs which were left in v1 by mistake.
ovsdb/monitor.c| 82 +++---
tests/ovsdb-monitor.at | 68 +
2 files changed, 119 insertions(+), 31 deletions(-)
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index dd06e26..29cf93e 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -120,6 +120,12 @@ struct ovsdb_monitor_changes {
struct hmap rows;
int n_refs;
uint64_t transaction;
+
+/* Save the mt->n_columns that is used when creating the changes.
+ * It can be different from the current mt->n_columns because
+ * mt->n_columns can be increased when there are condition changes
+ * from any of the clients sharing the dbmon. */
+size_t n_columns;
};
/* A particular table being monitored. */
@@ -156,7 +162,8 @@ typedef struct json *
const struct ovsdb_monitor_session_condition * condition,
enum ovsdb_monitor_row_type row_type,
const void *,
- bool initial, unsigned long int *changed);
+ bool initial, unsigned long int *changed,
+ size_t n_columns);
static void ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon);
static struct ovsdb_monitor_changes * ovsdb_monitor_table_add_changes(
@@ -255,14 +262,15 @@ ovsdb_monitor_changes_row_find(const struct
ovsdb_monitor_changes *changes,
return NULL;
}
-/* Allocates an array of 'mt->n_columns' ovsdb_datums and initializes them as
+/* Allocates an array of 'n_columns' ovsdb_datums and initializes them as
* copies of the data in 'row' drawn from the columns represented by
* mt->columns[]. Returns the array.
*
* If 'row' is NULL, returns NULL. */
static struct ovsdb_datum *
clone_monitor_row_data(const struct ovsdb_monitor_table *mt,
- const struct ovsdb_row *row)
+ const struct ovsdb_row *row,
+ size_t n_columns)
{
struct ovsdb_datum *data;
size_t i;
@@ -271,8 +279,8 @@ clone_monitor_row_data(const struct ovsdb_monitor_table *mt,
return NULL;
}
-data = xmalloc(mt->n_columns * sizeof *data);
-for (i = 0; i < mt->n_columns; i++) {
+data = xmalloc(n_columns * sizeof *data);
+for (i = 0; i < n_columns; i++) {
const struct ovsdb_column *c = mt->columns[i].column;
const struct ovsdb_datum *src = &row->fields[c->index];
struct ovsdb_datum *dst = &data[i];
@@ -283,16 +291,17 @@ clone_monitor_row_data(const struct ovsdb_monitor_table
*mt,
return data;
}
-/* Replaces the mt->n_columns ovsdb_datums in row[] by copies of the data from
+/* Replaces the n_columns ovsdb_datums in row[] by copies of the data from
* in 'row' drawn from the columns represented by mt->columns[]. */
static void
update_monitor_row_data(const struct ovsdb_monitor_table *mt,
const struct ovsdb_row *row,
-struct ovsdb_datum *data)
+struct ovsdb_datum *data,
+size_t n_columns)
{
Re: [ovs-dev] [PATCH][v2] conntrack: Remove unnecessary check in process_ftp_ctl_v4
On Mon, Feb 11, 2019 at 10:52:54AM +0800, Li RongQing wrote: > It has been assured that both first and second int from ftp > command are not bigger than 255, so their combination(first > int << 8 +second int) must not bigger than 65535 > > Co-authored-by: Wang Li > Signed-off-by: Wang Li > Signed-off-by: Li RongQing > Cc: Darrell Ball Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-discuss] Geneve remote_ip as flow for OVN hosts
On Mon, Feb 11, 2019 at 08:09:59PM +, Venugopal Iyer wrote: > Of course we want users to upgrade the entire system. We just need to > make sure that it's possible to upgrade one piece at a time in an order > that ensures that the system isn't broken by a partial upgrade. The > specified order for OVN is to upgrade the HVs first, then the central > node. (Although apparently some people want to do it in the other > order, which is currently a problem.) > > Thanks, I have updated the repo to squash all the commits and added a > high > level commit message. Please let me know if the message is helpful > and/or if > there are some best practices that I should follow. FYI, branch mvtep-br > @ https://github.com/iyervl/nv-ovs Thanks for the revision. It's not clear to me whether you believe that the upgrade compatibility issue is fixed. Is it? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] monitor.c: Fix crash when monitor condition adds new columns.
From: Han Zhou
The OVSDB conditional monitor implementation allows many clients
to share same copy of monitored data if the clients are sharing
same tables and columns being monitored, while they can have
different monitor conditions. In monitor conditions they can
have different columns which can be different from the columns
being monitored. So the struct ovsdb_monitor_table maintains the
union of the all the columns being used in any conditions.
The problem of the current implementation is that for each change
set generated, it doesn't maintain any metadata for the number of
columns for the data that has already populated in it. Instead, it
always rely on the n_columns field of the struct ovsdb_monitor_table
to manipulate the data. However, the n_columns in struct
ovsdb_monitor_table can increase (e.g. when a client changes its
condition which involves more columns). So it can result in that
the existing rows in a change set with N columns being later processed
as if it had more than N columns, typically, when the row is freed.
This causes the ovsdb-server crashing (see an example of the
backtrace).
The patch fixes the problem by maintaining n_columns for each
change set, and added a test case which fails without the fix.
(gdb) bt
at lib/ovsdb-data.c:1031
out>, mt=) at ovsdb/monitor.c:320
mt=0x1e7b940) at ovsdb/monitor.c:333
out>, transaction=) at ovsdb/monitor.c:527
initial=, cond_updated=cond_updated@entry=false,
unflushed_=unflushed_@entry=0x20dae70,
condition=, version=) at ovsdb/monitor.c:1156
(m=m@entry=0x20dae40, initial=initial@entry=false) at
ovsdb/jsonrpc-server.c:1655
at ovsdb/jsonrpc-server.c:1729
ovsdb/jsonrpc-server.c:551
ovsdb/jsonrpc-server.c:586
ovsdb/jsonrpc-server.c:401
exiting=0x7ffdb947f76f, run_process=0x0, remotes=0x7ffdb947f7c0,
unixctl=0x1e7a560, all_dbs=0x7ffdb947f800,
jsonrpc=, config=0x7ffdb947f820) at ovsdb/ovsdb-server.c:209
Signed-off-by: Han Zhou
---
ovsdb/monitor.c| 88 --
tests/ovsdb-monitor.at | 68 ++
2 files changed, 125 insertions(+), 31 deletions(-)
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index dd06e26..8909ae1 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -120,6 +120,12 @@ struct ovsdb_monitor_changes {
struct hmap rows;
int n_refs;
uint64_t transaction;
+
+/* Save the mt->n_columns that is used when creating the changes.
+ * It can be different from the current mt->n_columns because
+ * mt->n_columns can be increased when there are condition changes
+ * from any of the clients sharing the dbmon. */
+size_t n_columns;
};
/* A particular table being monitored. */
@@ -156,7 +162,8 @@ typedef struct json *
const struct ovsdb_monitor_session_condition * condition,
enum ovsdb_monitor_row_type row_type,
const void *,
- bool initial, unsigned long int *changed);
+ bool initial, unsigned long int *changed,
+ size_t n_columns);
static void ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon);
static struct ovsdb_monitor_changes * ovsdb_monitor_table_add_changes(
@@ -255,14 +262,15 @@ ovsdb_monitor_changes_row_find(const struct
ovsdb_monitor_changes *changes,
return NULL;
}
-/* Allocates an array of 'mt->n_columns' ovsdb_datums and initializes them as
+/* Allocates an array of 'n_columns' ovsdb_datums and initializes them as
* copies of the data in 'row' drawn from the columns represented by
* mt->columns[]. Returns the array.
*
* If 'row' is NULL, returns NULL. */
static struct ovsdb_datum *
clone_monitor_row_data(const struct ovsdb_monitor_table *mt,
- const struct ovsdb_row *row)
+ const struct ovsdb_row *row,
+ size_t n_columns)
{
struct ovsdb_datum *data;
size_t i;
@@ -271,8 +279,8 @@ clone_monitor_row_data(const struct ovsdb_monitor_table *mt,
return NULL;
}
-data = xmalloc(mt->n_columns * sizeof *data);
-for (i = 0; i < mt->n_columns; i++) {
+data = xmalloc(n_columns * sizeof *data);
+for (i = 0; i < n_columns; i++) {
const struct ovsdb_column *c = mt->columns[i].column;
const struct ovsdb_datum *src = &row->fields[c->index];
struct ovsdb_datum *dst = &data[i];
@@ -283,16 +291,17 @@ clone_monitor_row_data(const struct ovsdb_monitor_table
*mt,
return data;
}
-/* Replaces the mt->n_columns ovsdb_datums in row[] by copies of the data from
+/* Replaces the n_columns ovsdb_datums in row[] by copies of the data from
* in 'row' drawn from the columns represented by mt->columns[]. */
static void
update_monitor_row_data(const struct ovsdb_monitor_table *mt,
const struct ovsdb_row *row,
-struct ovsdb_datum *data)
+struct ovsdb_datum *data,
+size_t n_columns)
{
size_t i;
-for (i = 0; i < mt->n_columns; i++) {
+
Re: [ovs-dev] [PATCH 0/2] ovs-ctl: Permit to specify additional options
On Mon, Feb 11, 2019 at 07:55:51PM +0100, Timothy Redaelli wrote: > Currently using ovs-ctl is not possible to specify additional options > for ovs-vswitchd and ovsdb-server (for example to specify a different > loglevel during daemon startup). > > This series adds --ovs-vswitchd-options and --ovsdb-server-options > options to ovs-ctl in order to specify the additional options. > > Due to word splitting it may not be possible to specify an option that > includes whitespaces. > > The series also adds an example to > rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template. > > Timothy Redaelli (2): > ovs-ctl: Permit to specify additional options > rhel: Add an example to specify custom options > > rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 4 > utilities/ovs-ctl.in | 4 > 2 files changed, 8 insertions(+) Thank you for the patches. Please document the new options in utilities/ovs-ctl.8. I'd be open to folding these together, unless you prefer to keep them apart. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 1/1] acinclude: Also use LIBS from dpkg pkg-config
On Mon, Feb 11, 2019 at 08:36:47PM +0100, Christian Ehrhardt wrote: > DPDK 18.11 builds using the more modern meson build system no more > provide the -ldpdk linker script. Instead it is expected to use > pkgconfig for linker options as well. > > This change will set DPDK_LIB from pkg-config (if pkg-config was > available) and since that already carries the whole-archive flags > around the PMDs skips the further wrapping in more whole-archive > if that is already part of DPDK_LIB. > > To work reliable in all environments this needs pkg-config 0.29.1. > We want to be able to use PKG_CHECK_MODULES_STATIC which > is not yet available in 0.24. Therefore update pkg.m4 > to pkg-config 0.29.1. > > This should be backport-safe as these macro files are all versioned. > autoconf is smart enough to check the version if you have it locally, > and if the system's is higher, it will use that one instead. > > Acked-by: Luca Boccassi > Acked-by: Aaron Conole > Signed-off-by: Christian Ehrhardt Like the robot, I wasn't able to apply this. I guess it needs a rebase. Thank you! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netlink: added check to prevent netlink attribute overflow
On Mon, Feb 11, 2019 at 04:23:57PM -0800, Toms Atteka wrote:
> If enough large input is passed to odp_actions_from_string it can
> cause netlink attribute to overflow.
> ovs_assert was added just before the problematic code so it could
> be debugged faster in similar cases if they would arise. Check
> for buffer size was added to prevent entering this function and
> returning appropriate error code.
>
> Basic manual testing was performed.
>
> Reported-by:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231
> Signed-off-by: Toms Atteka
Thank you for the fix!
An nlattr's size is a uint16_t. I am more comfortable using UINT16_MAX
as the maximum value for this type than I am USHRT_MAX, since the latter
can in theory be bigger than 16 bits.
I am not sure why the assertion in nl_msg_end_nested() is sure to be
correct. It seems risky to me. Can you explain?
Thanks,
Ben.
> ---
> lib/netlink.c | 1 +
> lib/odp-util.c | 4
> 2 files changed, 5 insertions(+)
>
> diff --git a/lib/netlink.c b/lib/netlink.c
> index de3ebcd..c91c868 100644
> --- a/lib/netlink.c
> +++ b/lib/netlink.c
> @@ -498,6 +498,7 @@ void
> nl_msg_end_nested(struct ofpbuf *msg, size_t offset)
> {
> struct nlattr *attr = ofpbuf_at_assert(msg, offset, sizeof *attr);
> +ovs_assert(msg->size - offset <= USHRT_MAX);
> attr->nla_len = msg->size - offset;
> }
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index e893f46..9f637ca 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap
> *port_names,
> n += retval;
> }
>
> +if (actions->size > USHRT_MAX) {
> +return -EFBIG;
> +}
> +
> return n;
> }
>
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v12 8/8] Userspace datapath: Add fragmentation handling.
On Sun, Feb 10, 2019 at 06:56:30PM -0800, Darrell Ball wrote:
> Fragmentation handling is added for supporting conntrack.
> Both v4 and v6 are supported.
>
> After discussion with several people, I decided to not store
> configuration state in the database to be more consistent with
> the kernel in future, similarity with other conntrack configuration
> which will not be in the database as well and overall simplicity.
> Accordingly, fragmentation handling is enabled by default.
>
> This patch enables fragmentation tests for the userspace datapath.
>
> Signed-off-by: Darrell Ball
Thanks.
It's not obvious to me why ipf_reassemble_v6_frags() accounts for pad
size but ipf_reassemble_v4_frags() does not. (Is there an inherent
difference for padding between v4 and v6? If so, I've forgotten about
it.)
I had some other comments, but I decided that it would be easier to
present them as an incremental patch. Please let me know what you think
of folding in the following changes. They are mainly stylistic one way
or another, in the sense that I don't remember any of them actually
fixing bugs.
Thanks,
Ben.
--8<--cut here-->8--
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 46b5dd570d53..6f2e788c6703 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2015, 2016, 2017 Nicira, Inc.
+ * Copyright (c) 2015, 2016, 2017, 2019 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -341,7 +341,7 @@ conntrack_init(struct conntrack *ct)
atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);
latch_init(&ct->clean_thread_exit);
ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main, ct);
-ipf_init(&ct->ipf);
+ct->ipf = ipf_init();
}
/* Destroys the connection tracker 'ct' and frees all the allocated memory. */
diff --git a/lib/ipf.c b/lib/ipf.c
index bdfd85579165..8dc63ea99529 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2018 Nicira, Inc.
+ * Copyright (c) 2018, 2019 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -84,10 +84,8 @@ enum ipf_counter_type {
};
union ipf_addr {
-ovs_16aligned_be32 ipv4;
-union ovs_16aligned_in6_addr ipv6;
-ovs_be32 ipv4_aligned;
-struct in6_addr ipv6_aligned;
+ovs_be32 ipv4;
+struct in6_addr ipv6;
};
/* Represents a single fragment; part of a list of fragments. */
@@ -101,6 +99,8 @@ struct ipf_frag {
/* The key for a collection of fragments potentially making up an unfragmented
* packet. */
struct ipf_list_key {
+/* ipf_list_key_hash() requires 'src_addr' and 'dst_addr' to be the first
+ * two members. */
union ipf_addr src_addr;
union ipf_addr dst_addr;
uint32_t recirc_id;
@@ -112,8 +112,9 @@ struct ipf_list_key {
/* A collection of fragments potentially making up an unfragmented packet. */
struct ipf_list {
-struct hmap_node node;
-struct ovs_list list_node;
+struct hmap_node node; /* In struct ipf's 'frag_lists'. */
+struct ovs_list list_node; /* In struct ipf's 'frag_exp_list' or
+* 'frag_complete_list'. */
struct ipf_frag *frag_list;/* List of fragments for this list. */
struct ipf_list_key key; /* The key for the fragemnt list. */
struct dp_packet *reass_execute_ctx; /* Reassembled packet. */
@@ -127,15 +128,11 @@ struct ipf_list {
/* Represents a reassambled packet which typically is passed through
* conntrack. */
struct reassembled_pkt {
-struct ovs_list rp_list_node;
+struct ovs_list rp_list_node; /* In struct ipf's 'reassembled_pkt_list'. */
struct dp_packet *pkt;
struct ipf_list *list;
};
-struct OVS_LOCKABLE ipf_lock {
-struct ovs_mutex lock;
-};
-
struct ipf {
/* The clean thread is used to clean up fragments in the 'ipf'
* module if packet batches are not longer be sent through its user. */
@@ -144,11 +141,12 @@ struct ipf {
int max_v4_frag_list_size;
-/* Adaptive mutex protecting the following frag_list hmap andlists. */
-struct ipf_lock ipf_lock;
+struct ovs_mutex ipf_lock; /* Protects all of the following. */
+/* These contain "struct ipf_list"s. */
struct hmap frag_lists OVS_GUARDED;
struct ovs_list frag_exp_list OVS_GUARDED;
struct ovs_list frag_complete_list OVS_GUARDED;
+/* Contains "struct reassembled_pkt"s. */
struct ovs_list reassembled_pkt_list OVS_GUARDED;
/* Used to allow disabling fragmentation reassembly. */
@@ -171,19 +169,16 @@ struct ipf {
atomic_uint64_t n6frag_cnt[IPF_NFRAGS_NUM_CNTS];
};
-#define IPF_PTR(POINTER) \
-CONST_CAST(struct ipf *, POINTER)
-
static void
-ipf_print_reass_packet(char *es, void *pkt)
+ipf_print_reass_pac
Re: [ovs-dev] dpif-netdev:fix reload pmd's dead lock
Ok, I got it, thanks for your explanation. > -Original Message- > From: Ilya Maximets [mailto:i.maxim...@samsung.com] > Sent: Monday, February 11, 2019 9:55 PM > To: Lilijun (Jerry, Cloud Networking) ; Stokes, Ian > ; d...@openvswitch.org > Subject: Re: [ovs-dev] dpif-netdev:fix reload pmd's dead lock > > On 11.02.2019 6:22, Lilijun (Jerry, Cloud Networking) wrote: > > Yes, Thanks for your help again. > > > > Let's forget the patch :). But in my opinion, the function > reload_affected_pmds() doesn't need be locked within dp->port_mutex. Is > that true? > > That's true that 'reload_affected_pmds()' itself doesn't need to be locked. > However, you can't just unlock the mutex and lock it again, because upper > layer functions that calls 'reconfigure_datapath()' expects that mutex will be > held all the way down. > For example, 'dp_netdev_free()' iterates over the 'dp->ports' hmap and it > doesn't expect that 'do_del_port()' could unlock the mutex. If hmap will be > changed (some ports added), the iterator could crash. > Another example is 'dpif_dummy_change_port_number()' that calls the > reconfiguration twice in a row expecting no changes in 'dp->ports' between. > > Best regards, Ilya Maximets. > > > > >> -Original Message- > >> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > >> boun...@openvswitch.org] On Behalf Of Stokes, Ian > >> Sent: Thursday, February 07, 2019 5:25 PM > >> To: Ilya Maximets ; d...@openvswitch.org > >> Subject: Re: [ovs-dev] dpif-netdev:fix reload pmd's dead lock > >> > >>> On 31.01.2019 11:48, Lilijun wrote: > This patch fix the dead lock when using dpdk userspace datapath. > The problem is described as follows: > 1) when add or delete port, the main thread will call > reconfigure_datapath() in the function dpif_netdev_run() > 2) Here the dp->port_mutex is locked. In dp_netdev_reload_pmd__(), > it will notify each pmd to reload. > 3) If pmd is doing packet upcall in fast_path_processing() and try > to get > dp->port_mutex in > do_xlate_actions()-->tnl_route_lookup_flow()-- > dpif_netdev_port_query_by_name(). > Here pmd get lock failed because the main thread has got the lock > in step 2. > 4) So the main thread was stuck for waiting pmd to reload done. Now > we got a dead lock. > > Here reload_affected_pmds() may not need to lock dp->port_mutex. > So > we release the lock temporarily when calling reload_affected_pmds(). > > Signed-off-by: Lilijun > >>> > >>> Replying just to keep answers on a list/patchwork consistent. > >>> The deadlock caused by some local changes done by the user. > >>> Not possible in upstream master. See discussion for the previous > >>> version of this patch that is missing in patchwork for some reason: > >>> > >>> > >>> https://mail.openvswitch.org/pipermail/ovs-dev/2019- > >> January/355756.htm > >>> l > >>> > >>> Best regards, Ilya Maximets. > >>> > >> > >> Thanks for clarifying Ilya, there was discussion of this at the > >> community meeting yesterday but it seems a non-issue now. > >> > >> Thanks > >> Ian > >> ___ > >> dev mailing list > >> d...@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] netlink: added check to prevent netlink attribute overflow
If enough large input is passed to odp_actions_from_string it can
cause netlink attribute to overflow.
ovs_assert was added just before the problematic code so it could
be debugged faster in similar cases if they would arise. Check
for buffer size was added to prevent entering this function and
returning appropriate error code.
Basic manual testing was performed.
Reported-by:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231
Signed-off-by: Toms Atteka
---
lib/netlink.c | 1 +
lib/odp-util.c | 4
2 files changed, 5 insertions(+)
diff --git a/lib/netlink.c b/lib/netlink.c
index de3ebcd..c91c868 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -498,6 +498,7 @@ void
nl_msg_end_nested(struct ofpbuf *msg, size_t offset)
{
struct nlattr *attr = ofpbuf_at_assert(msg, offset, sizeof *attr);
+ovs_assert(msg->size - offset <= USHRT_MAX);
attr->nla_len = msg->size - offset;
}
diff --git a/lib/odp-util.c b/lib/odp-util.c
index e893f46..9f637ca 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap
*port_names,
n += retval;
}
+if (actions->size > USHRT_MAX) {
+return -EFBIG;
+}
+
return n;
}
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v12 7/8] dp-packet: Add 'do_not_steal' packet batch flag.
sorry, one line of this patch was placed in the wrong function.
The following incremental relocates it to the proper function.
dball@ubuntu:~/openvswitch/ovs$ git diff
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index c4ecd2d..dae197e 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -750,6 +750,7 @@ dp_packet_batch_init(struct dp_packet_batch *batch)
{
batch->count = 0;
batch->trunc = false;
+batch->do_not_steal = false;
}
static inline void
@@ -796,7 +797,6 @@ dp_packet_batch_init_packet(struct dp_packet_batch
*batch, s
{
dp_packet_batch_init(batch);
batch->count = 1;
-batch->do_not_steal = false;
batch->packets[0] = p;
}
On Sun, Feb 10, 2019 at 6:56 PM Darrell Ball wrote:
> This is needed in a subsequent patch and may otherwise be useful.
>
> Signed-off-by: Darrell Ball
> ---
> lib/dp-packet.h | 2 ++
> lib/dpif-netdev.c | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 034b81b..c4ecd2d 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -741,6 +741,7 @@ enum { NETDEV_MAX_BURST = 32 }; /* Maximum number
> packets in a batch. */
> struct dp_packet_batch {
> size_t count;
> bool trunc; /* true if the batch needs truncate. */
> +bool do_not_steal; /* Indicate that the packets should not be stolen.
> */
> struct dp_packet *packets[NETDEV_MAX_BURST];
> };
>
> @@ -795,6 +796,7 @@ dp_packet_batch_init_packet(struct dp_packet_batch
> *batch, struct dp_packet *p)
> {
> dp_packet_batch_init(batch);
> batch->count = 1;
> +batch->do_not_steal = false;
> batch->packets[0] = p;
> }
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 0f57e3f..47e6c80 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3716,6 +3716,7 @@ dpif_netdev_execute(struct dpif *dpif, struct
> dpif_execute *execute)
> }
>
> dp_packet_batch_init_packet(&pp, execute->packet);
> +pp.do_not_steal = true;
> dp_netdev_execute_actions(pmd, &pp, false, execute->flow,
>execute->actions, execute->actions_len);
> dp_netdev_pmd_flush_output_packets(pmd, true);
> --
> 1.9.1
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] acinclude: Also use LIBS from dpkg pkg-config
Bleep bloop. Greetings Christian Ehrhardt, 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. git-am: Failed to merge in the changes. Patch failed at 0001 acinclude: Also use LIBS from dpkg pkg-config The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". 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] [ovs-discuss] Geneve remote_ip as flow for OVN hosts
From: Ben Pfaff Sent: Thursday, February 7, 2019 6:53 PM To: Venugopal Iyer Cc: Guru Shetty; Leonid Grossman; d...@openvswitch.org Subject: Re: [ovs-discuss] [ovs-dev] Geneve remote_ip as flow for OVN hosts On Thu, Feb 07, 2019 at 08:10:48PM +, Venugopal Iyer wrote: > > Also, as I mentioned the changes will mean that the ovn-controller will > > need the ovn-central > > to be updated to the changed version as well (i.e. if someone just installs > > ovs and ovn-host > > s/he can't expect it to be backward compatible with the older version of > > ovn-sb). Is that > > acceptable? > > That's not what we usually want. The OVN upgrade process expects the > HVs to be upgraded before the central nodes. If that breaks things, > especially in the case where the deployment is only using a single > interface per HV, it's a problem. > > What would it take to retain compatibility? > > That is possible; I have committed the changes to the repo[1]. I have > tested > ovn-host upgrade with these changes against an OVN central based on 2.9 > (without m-vtep changes). > Initially, I was planning to bind the port to an encap even if > "encap-ip" external_ids > is not configured; so when the updated ovn-host comes up, it'll try to > bind the > port to an encap and the transaction failure caused the port bindings to > fail. Implicit > binding is not needed, probably not preferred either. So, an updated > ovn-host > should work with a prev version of SB, however, the OVN central will be > updated too, right? Else, if one configures "encap-ip" subsequently on > an updated > ovn-host to work with m-vtep, we'll hit the same issue. > [1]https://github.com/iyervl/nv-ovs Of course we want users to upgrade the entire system. We just need to make sure that it's possible to upgrade one piece at a time in an order that ensures that the system isn't broken by a partial upgrade. The specified order for OVN is to upgrade the HVs first, then the central node. (Although apparently some people want to do it in the other order, which is currently a problem.) Thanks, I have updated the repo to squash all the commits and added a high level commit message. Please let me know if the message is helpful and/or if there are some best practices that I should follow. FYI, branch mvtep-br @ https://github.com/iyervl/nv-ovs thanks! -venu --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 1/1] acinclude: Also use LIBS from dpkg pkg-config
DPDK 18.11 builds using the more modern meson build system no more provide the -ldpdk linker script. Instead it is expected to use pkgconfig for linker options as well. This change will set DPDK_LIB from pkg-config (if pkg-config was available) and since that already carries the whole-archive flags around the PMDs skips the further wrapping in more whole-archive if that is already part of DPDK_LIB. To work reliable in all environments this needs pkg-config 0.29.1. We want to be able to use PKG_CHECK_MODULES_STATIC which is not yet available in 0.24. Therefore update pkg.m4 to pkg-config 0.29.1. This should be backport-safe as these macro files are all versioned. autoconf is smart enough to check the version if you have it locally, and if the system's is higher, it will use that one instead. Acked-by: Luca Boccassi Acked-by: Aaron Conole Signed-off-by: Christian Ehrhardt --- acinclude.m4 | 21 +++-- configure.ac | 1 + m4/pkg.m4| 217 +-- 3 files changed, 156 insertions(+), 83 deletions(-) diff --git a/acinclude.m4 b/acinclude.m4 index 8b43d2bfe..f000cbb3e 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -223,9 +223,13 @@ AC_DEFUN([OVS_CHECK_DPDK], [ case "$with_dpdk" in yes) DPDK_AUTO_DISCOVER="true" -PKG_CHECK_MODULES([DPDK], [libdpdk], - [DPDK_INCLUDE="$DPDK_CFLAGS"], - [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"]) +PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk], [ +DPDK_INCLUDE="$DPDK_CFLAGS" +DPDK_LIB="$DPDK_LIBS" + ], [ +DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk" +DPDK_LIB="-ldpdk" + ]) ;; *) DPDK_AUTO_DISCOVER="false" @@ -238,10 +242,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [ DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk" fi DPDK_LIB_DIR="$with_dpdk/lib" +DPDK_LIB="-ldpdk" ;; esac -DPDK_LIB="-ldpdk" DPDK_EXTRA_LIB="" ovs_save_CFLAGS="$CFLAGS" @@ -346,7 +350,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [ # # These options are specified inside a single -Wl directive to prevent # autotools from reordering them. -DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive +# +# OTOH newer versions of dpdk pkg-config (generated with Meson) +# will already have flagged just the right set of libs with +# --whole-archive - in those cases do not wrap it once more. +case "$DPDK_LIB" in + *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;; + *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive +esac AC_SUBST([DPDK_vswitchd_LDFLAGS]) AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.]) fi diff --git a/configure.ac b/configure.ac index 8a05870a4..6ed8415b3 100644 --- a/configure.ac +++ b/configure.ac @@ -28,6 +28,7 @@ AC_PROG_CPP AC_PROG_MKDIR_P AC_PROG_FGREP AC_PROG_EGREP + PKG_PROG_PKG_CONFIG AM_MISSING_PROG([AUTOM4TE], [autom4te]) diff --git a/m4/pkg.m4 b/m4/pkg.m4 index c5b26b52e..82bea96ee 100644 --- a/m4/pkg.m4 +++ b/m4/pkg.m4 @@ -1,29 +1,60 @@ -# pkg.m4 - Macros to locate and utilise pkg-config.-*- Autoconf -*- -# serial 1 (pkg-config-0.24) -# -# Copyright © 2004 Scott James Remnant . -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 2 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -# General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. -# -# As a special exception to the GNU General Public License, if you -# distribute this file as part of a program that contains a -# configuration script generated by Autoconf, you may include it under -# the same distribution terms that you use for the rest of that program. - -# PKG_PROG_PKG_CONFIG([MIN-VERSION]) -# -- +dnl pkg.m4 - Macros to locate and utilise pkg-config. -*- Autoconf -*- +dnl serial 11 (pkg-config-0.29.1) +dnl +dnl Copyright © 2004 Scott James Remnant . +dnl Copyright © 2012-2015 Dan Nicholson +dnl +dnl This program is free software; you can redistribute it and/or modify +dnl it under the terms of the GNU General Public License as published by +dnl the Free Software Foundation; either version 2 of the License, or +dnl (at your option) any later version. +dnl +dnl This program is distrib
[ovs-dev] [PATCH v5 0/1] fix build with DPDK 18.11 without -ldpdk
DPDK 18.11 can be built using the more modern meson build system. In that case it no more provides the -ldpdk linker script. Instead it is expected to use pkgconfig for linker options as well. FYI here a build log on Ubuntu 19.04 with the three patches applied: https://launchpadlibrarian.net/409021278/buildlog_ubuntu-disco-amd64.openvswitch_2.11.0~git20190121.4e4f80ec2-0ubuntu1~ppa5_BUILDING.txt.gz Updates from v4: - drop explicit m4_include([m4/pkg.m4]) as aclocal should be sufficient - add Acked-by's I got so far to the commit message Updates from v3: - change separation of multiple actions in PKG_CHECK_MODULES_STATIC to newlines to avoid trailing commas in some build environments Updates from v2: - patches were formerly structured for reviewability, but to guarantee travis requirements to pass the build individually and not as a series they are now squashed to be acceptable Updates from v1: - add updated pkg.m4 to support PKG_CHECK_MODULES_STATIC - include local pkg.m4 in configure.ac Christian Ehrhardt (1): acinclude: Also use LIBS from dpkg pkg-config acinclude.m4 | 21 +++-- configure.ac | 1 + m4/pkg.m4| 217 +-- 3 files changed, 156 insertions(+), 83 deletions(-) -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix unsafe accesses to pmd polling lists.
Ilya Maximets writes:
> All accesses to 'pmd->poll_list' should be guarded by
> 'pmd->port_mutex'. Additionally fixed inappropriate usage
> of 'HMAP_FOR_EACH_SAFE' (hmap doesn't change in a loop)
> and dropped not needed local variable 'proc_cycles'.
>
> CC: Nitin Katiyar
> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
> Signed-off-by: Ilya Maximets
> ---
> lib/dpif-netdev.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4d5bc576a..914b2bb8b 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3776,9 +3776,12 @@ set_pmd_auto_lb(struct dp_netdev *dp)
> continue;
> }
>
> +ovs_mutex_lock(&pmd->port_mutex);
> if (hmap_count(&pmd->poll_list) > 1) {
> multi_rxq = true;
> }
> +ovs_mutex_unlock(&pmd->port_mutex);
What does this mutex provide here? I ask because as soon as we unlock,
the result is no longer assured, and we've used it to inform the
multi_rxq value.
Are you afraid that the read is non-atomic so you'll end up with the
multi_rxq condition true, even when it should not be? That can happen
anyway - as soon as we unlock the count can change.
Maybe there's a better change to support that, where hmap_count does an
atomic read, or there's a version that can guarantee a full load with
no intervening writes. But I don't see
Maybe the pmd object lifetime goes away... but that wouldn't make sense,
because then the port_mutex itself would be invalid.
I'm confused what it does to add a lock here for either the safety, or
the logic. I probably missed something. Or maybe it's just trying to
be safe in case the hmap implementation would change in the future? I
think if that's the case, there might be an alternate to implement?
> +
> if (cnt && multi_rxq) {
> enable_alb = true;
> break;
> @@ -5095,7 +5098,7 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
> uint64_t improvement = 0;
> uint32_t num_pmds;
> uint32_t *pmd_corelist;
> -struct rxq_poll *poll, *poll_next;
> +struct rxq_poll *poll;
> bool ret;
>
> num_pmds = cmap_count(&dp->poll_threads);
> @@ -5121,13 +5124,14 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
> /* Estimate the cycles to cover all intervals. */
> total_cycles *= PMD_RXQ_INTERVAL_MAX;
>
> -HMAP_FOR_EACH_SAFE (poll, poll_next, node, &pmd->poll_list) {
> -uint64_t proc_cycles = 0;
> +ovs_mutex_lock(&pmd->port_mutex);
> +HMAP_FOR_EACH (poll, node, &pmd->poll_list) {
> for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
> -proc_cycles += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
> +total_proc += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
> }
> -total_proc += proc_cycles;
> }
> +ovs_mutex_unlock(&pmd->port_mutex);
> +
This lock, otoh, makes sense to me.
> if (total_proc) {
> curr_pmd_usage[num_pmds] = (total_proc * 100) / total_cycles;
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] rhel: Add an example to specify custom options
Add an example to specify custom options of ovs-vswitchd and ovsdb-server. In the example, the log level for file and console destinations is set to dbg. Signed-off-by: Timothy Redaelli --- rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 4 1 file changed, 4 insertions(+) diff --git a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template index 936445402..c467d02db 100644 --- a/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template +++ b/rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template @@ -21,6 +21,10 @@ # --ovs-vswitchd-wrapper=valgrind # --ovsdb-server-wrapper=valgrind # +# Specify additional options, for example to start with debug logs: +# --ovs-vswitchd-options='-vconsole:dbg -vfile:dbg' +# --ovsdb-server-options='-vconsole:dbg -vfile:dbg' +# OPTIONS="" # Uncomment and set the OVS User/Group value -- 2.20.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] ovs-ctl: Permit to specify additional options
Currently using ovs-ctl is not possible to specify additional options
for ovs-vswitchd and ovsdb-server (for example to specify a different
loglevel during daemon startup).
This patch adds --ovs-vswitchd-options and --ovsdb-server-options
options to ovs-ctl in order to specify the additional options.
Due to word splitting it may not be possible to specify an option that
includes whitespaces.
Reported-at: https://bugzilla.redhat.com/1664794
Reported-by: Matt Flusche
Signed-off-by: Timothy Redaelli
---
utilities/ovs-ctl.in | 4
1 file changed, 4 insertions(+)
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index 9c2a092ea..8c5cd7032 100644
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -144,6 +144,7 @@ do_start_ovsdb () {
set "$@" --certificate=db:Open_vSwitch,SSL,certificate
set "$@" --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert
[ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER"
+[ "$OVSDB_SERVER_OPTIONS" != "" ] && set "$@" $OVSDB_SERVER_OPTIONS
start_daemon "$OVSDB_SERVER_PRIORITY" "$OVSDB_SERVER_WRAPPER" "$@" \
|| return 1
@@ -213,6 +214,7 @@ do_start_forwarding () {
set "$@" --no-self-confinement
fi
[ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER"
+[ "$OVS_VSWITCHD_OPTIONS" != "" ] &&set "$@" $OVS_VSWITCHD_OPTIONS
start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@" ||
return 1
@@ -326,6 +328,8 @@ set_defaults () {
OVS_VSWITCHD_PRIORITY=-10
OVSDB_SERVER_WRAPPER=
OVS_VSWITCHD_WRAPPER=
+OVSDB_SERVER_OPTIONS=
+OVS_VSWITCHD_OPTIONS=
DB_FILE=$dbdir/conf.db
DB_SOCK=$rundir/db.sock
--
2.20.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/2] ovs-ctl: Permit to specify additional options
Currently using ovs-ctl is not possible to specify additional options for ovs-vswitchd and ovsdb-server (for example to specify a different loglevel during daemon startup). This series adds --ovs-vswitchd-options and --ovsdb-server-options options to ovs-ctl in order to specify the additional options. Due to word splitting it may not be possible to specify an option that includes whitespaces. The series also adds an example to rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template. Timothy Redaelli (2): ovs-ctl: Permit to specify additional options rhel: Add an example to specify custom options rhel/usr_share_openvswitch_scripts_systemd_sysconfig.template | 4 utilities/ovs-ctl.in | 4 2 files changed, 8 insertions(+) -- 2.20.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix unsafe accesses to pmd polling lists.
On 02/11/2019 05:34 PM, Ilya Maximets wrote:
> All accesses to 'pmd->poll_list' should be guarded by
> 'pmd->port_mutex'. Additionally fixed inappropriate usage
> of 'HMAP_FOR_EACH_SAFE' (hmap doesn't change in a loop)
> and dropped not needed local variable 'proc_cycles'.
>
> CC: Nitin Katiyar
> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
> Signed-off-by: Ilya Maximets
LGTM. Thanks Ilya,
Acked-by: Kevin Traynor
> ---
> lib/dpif-netdev.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4d5bc576a..914b2bb8b 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3776,9 +3776,12 @@ set_pmd_auto_lb(struct dp_netdev *dp)
> continue;
> }
>
> +ovs_mutex_lock(&pmd->port_mutex);
> if (hmap_count(&pmd->poll_list) > 1) {
> multi_rxq = true;
> }
> +ovs_mutex_unlock(&pmd->port_mutex);
> +
> if (cnt && multi_rxq) {
> enable_alb = true;
> break;
> @@ -5095,7 +5098,7 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
> uint64_t improvement = 0;
> uint32_t num_pmds;
> uint32_t *pmd_corelist;
> -struct rxq_poll *poll, *poll_next;
> +struct rxq_poll *poll;
> bool ret;
>
> num_pmds = cmap_count(&dp->poll_threads);
> @@ -5121,13 +5124,14 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
> /* Estimate the cycles to cover all intervals. */
> total_cycles *= PMD_RXQ_INTERVAL_MAX;
>
> -HMAP_FOR_EACH_SAFE (poll, poll_next, node, &pmd->poll_list) {
> -uint64_t proc_cycles = 0;
> +ovs_mutex_lock(&pmd->port_mutex);
> +HMAP_FOR_EACH (poll, node, &pmd->poll_list) {
> for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
> -proc_cycles += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
> +total_proc += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
> }
> -total_proc += proc_cycles;
> }
> +ovs_mutex_unlock(&pmd->port_mutex);
> +
> if (total_proc) {
> curr_pmd_usage[num_pmds] = (total_proc * 100) / total_cycles;
> }
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Add thread safety annotation to sorted_poll_list.
On 02/11/2019 05:35 PM, Ilya Maximets wrote:
> 'sorted_poll_list()' uses the 'pmd->poll_list' that should be
> guarded by 'pmd->port_mutex'.
>
> Signed-off-by: Ilya Maximets
> ---
LGTM
Acked-by: Kevin Traynor
> lib/dpif-netdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 914b2bb8b..34f2d2b07 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1109,6 +1109,7 @@ compare_poll_list(const void *a_, const void *b_)
> static void
> sorted_poll_list(struct dp_netdev_pmd_thread *pmd, struct rxq_poll **list,
> size_t *n)
> +OVS_REQUIRES(pmd->port_mutex)
> {
> struct rxq_poll *ret, *poll;
> size_t i;
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Inversez votre Diabète Grâce au Démarrage Assisté de votre Pancréas
Salut, Est-ce que votre diab�te vous interdit vos plats pr�f�r�s ? Voici une nouvelle nettement mieux que bonne Vous devez prendre connaissance de ceci : � Je croyais ne plus jamais pouvoir manger ce que j 19aime le plus. . . . . .puis j 19ai d�couvert le secret pour inverser mon diab�te de type 2 � Souffrez-vous d 19un diab�te de type 2 ? Si votre r�ponse est (malheureusement) oui, vous savez certainement pourquoi : Votre pancr�as ne produit pas le bon niveau d 19 insuline. Heureusement, une r�cente �tude de l 19Universit� de Newcastle en Angleterre a r�v�l�. . . Un moyen simple et naturel pour obliger votre pancr�as � faire son travail. Aujourd 19hui plus de 39 264 personnes ont utilis� ce moyen simple pour assister leur pancr�as et inverser leur diab�te. . . Et aussi vite qu 19en 11 jours ! >>>D�couvrez le secret pour : comment inverser VOTRE diab�te de type 2 C 19est gr�ce � un chercheur isol� qui devait trouver le moyen d 19inverser d�finitivement son diab�te de type 2 ou �tre confront� � l 19amputation compl�te de sa jambe droite. . . Qui a utilis� les recherches de l 19Universit� de Newcastle, en Angleterre, pour �viter cette amputation et litt�ralement sauver sa vie. . . Ceci gr�ce � une m�thode destructrice de diab�te, en 3 �tapes. Des personnes dans le monde entier (� ce jour 39264) inversent leur diab�te type 2 et mangent ce qui leur plait et surtout ce qu 19ils pr�f�rent. Je parle par exemple de croissants ou tartines � la confiture. . . De cr�mes glac�e ou de flan. . . De pizzas ou de spaghettis. . . Tous ces mets d�licieux qui vous font tant plaisir et qui vous sont interdits � cause de votre diab�te. Mais ceux qui aujourd 19hui ont d�couvert la VERITE au sujet du diab�te mangent tout ce qu 19ils veulent 26et inversent leur diab�te de type 2. Cliquez sur le lien ci-dessous pour d�couvrir le secret qui vous permettra d 19inverser votre diab�te afin que vous puissiez � nouveau profiter de tous les aliments que vous aimez tant. >>>D�couvrez la VERITE au sujet de vos aliments pr�f�r�s et comment inverser votre diab�te D�truisez d�finitivement votre diab�te en 3 simples �tapes Les grands labos pharmaceutiques sont furieux au sujet de la fuite de ce rem�de pour se d�barrasser du diab�te type 2. V�rifiez ces 3 �tapes en cliquant ICI Soyez en bonne sant� ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC] OVS robot discussion - check API integration
11/02/2019 18:03, Ben Pfaff: > On Mon, Feb 11, 2019 at 10:24:01AM -0500, Aaron Conole wrote: > > I'm following up on a discussion where we briefly talked about adding > > a 'check' to OVS patch series during the OVS Conference. 'Check' is > > what the patchwork project provides to show that certain tests have been > > performed. It is made up of a name, a status, and a URL. Some of the > > projects in the ozlabs patchwork instance are using the check feature > > already, so there's some examples to reference. > > > > My idea is to push some data into travis / cirrus that can be used to > > insert a check against a patch series. This would allow committers to > > look at the status page on patchwork and see at a glance the links to > > the travis, and if a robot email was sent. > > The approach seems reasonable. > > You mention that there are existing examples. How do they do it? In DPDK project, we are sending the results to a specific mailing-list with this script: http://git.dpdk.org/tools/dpdk-ci/tree/tools/send-patch-report.sh It is possible to filter some reports by using mail filtering. Then it is parsed on the server by the following script which updates patchwork data using server-specific credentials: http://git.dpdk.org/tools/dpdk-ci/tree/tools/update-pw.sh Note that we improved patchwork to highlight CI results with colors: https://patches.dpdk.org ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netdev: Add thread safety annotation to sorted_poll_list.
'sorted_poll_list()' uses the 'pmd->poll_list' that should be
guarded by 'pmd->port_mutex'.
Signed-off-by: Ilya Maximets
---
lib/dpif-netdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 914b2bb8b..34f2d2b07 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1109,6 +1109,7 @@ compare_poll_list(const void *a_, const void *b_)
static void
sorted_poll_list(struct dp_netdev_pmd_thread *pmd, struct rxq_poll **list,
size_t *n)
+OVS_REQUIRES(pmd->port_mutex)
{
struct rxq_poll *ret, *poll;
size_t i;
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netdev: Fix unsafe accesses to pmd polling lists.
All accesses to 'pmd->poll_list' should be guarded by
'pmd->port_mutex'. Additionally fixed inappropriate usage
of 'HMAP_FOR_EACH_SAFE' (hmap doesn't change in a loop)
and dropped not needed local variable 'proc_cycles'.
CC: Nitin Katiyar
Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
Signed-off-by: Ilya Maximets
---
lib/dpif-netdev.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 4d5bc576a..914b2bb8b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3776,9 +3776,12 @@ set_pmd_auto_lb(struct dp_netdev *dp)
continue;
}
+ovs_mutex_lock(&pmd->port_mutex);
if (hmap_count(&pmd->poll_list) > 1) {
multi_rxq = true;
}
+ovs_mutex_unlock(&pmd->port_mutex);
+
if (cnt && multi_rxq) {
enable_alb = true;
break;
@@ -5095,7 +5098,7 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
uint64_t improvement = 0;
uint32_t num_pmds;
uint32_t *pmd_corelist;
-struct rxq_poll *poll, *poll_next;
+struct rxq_poll *poll;
bool ret;
num_pmds = cmap_count(&dp->poll_threads);
@@ -5121,13 +5124,14 @@ pmd_rebalance_dry_run(struct dp_netdev *dp)
/* Estimate the cycles to cover all intervals. */
total_cycles *= PMD_RXQ_INTERVAL_MAX;
-HMAP_FOR_EACH_SAFE (poll, poll_next, node, &pmd->poll_list) {
-uint64_t proc_cycles = 0;
+ovs_mutex_lock(&pmd->port_mutex);
+HMAP_FOR_EACH (poll, node, &pmd->poll_list) {
for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
-proc_cycles += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
+total_proc += dp_netdev_rxq_get_intrvl_cycles(poll->rxq, i);
}
-total_proc += proc_cycles;
}
+ovs_mutex_unlock(&pmd->port_mutex);
+
if (total_proc) {
curr_pmd_usage[num_pmds] = (total_proc * 100) / total_cycles;
}
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/1] acinclude: Also use LIBS from dpkg pkg-config
On Mon, Feb 11, 2019 at 11:37:25AM +0100, Christian Ehrhardt wrote: > On Mon, Feb 11, 2019 at 11:06 AM Luca Boccassi wrote: > > > > On Mon, 2019-02-11 at 07:59 +0100, Christian Ehrhardt wrote: > > > On Sun, Feb 10, 2019 at 5:53 PM Ben Pfaff wrote: > > > > > > > > On Thu, Feb 07, 2019 at 12:46:40PM +0100, Christian Ehrhardt wrote: > > > > > DPDK 18.11 builds using the more modern meson build system no > > > > > more > > > > > provide the -ldpdk linker script. Instead it is expected to use > > > > > pkgconfig for linker options as well. > > > > > > > > > > This change will set DPDK_LIB from pkg-config (if pkg-config was > > > > > available) and since that already carries the whole-archive flags > > > > > around the PMDs skips the further wrapping in more whole-archive > > > > > if that is already part of DPDK_LIB. > > > > > > > > > > To work reliable in all environments this needs pkg-config > > > > > 0.29.1. > > > > > We want to be able to use PKG_CHECK_MODULES_STATIC which > > > > > is not yet available in 0.24. Therefore update pkg.m4 > > > > > to pkg-config 0.29.1. > > > > > > > > > > This should be backport-safe as these macro files are all > > > > > versioned. > > > > > autoconf is smart enough to check the version if you have it > > > > > locally, > > > > > and if the system's is higher, it will use that one instead. > > > > > > > > > > Finally make configure.ac use the locally provided pkg.m4 before > > > > > calling the PKG_PROG_PKG_CONFIG macro. > > > > > > > > > > Signed-off-by: Christian Ehrhardt > > > > om> > > > > > > > > The explicit m4_include([m4/pkg.m4]) should not be > > > > necessary. aclocal > > > > should pick it up automatically. > > > > > > Thanks Ben for the hint. > > > I agree that it might pick it up automatically, but I have learned > > > not > > > rely on too much with autotools :-/ > > > @Luca - to add the explicit include was your suggestion from Friday > > > the 25th, do you see a potential issue if we don't? > > > > Yes it should work without - but only one way to be sure :-) > > yeah - "should (tm)" :-) > > @Ben Pfaff - it surely works with the include and I see now drawback :-) > If you want me I can send a V5 which drops the explicit include, but > without being called to do so I'd lean towards keeping it explicit to > be sure. > Let me know what you prefer. We rely on aclocal for other .m4 files so I see no reason not to rely on it here too. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 1/3] dpdk: Unify passing of DPDK EAL arguments.
On 08.02.2019 13:36, Kevin Traynor wrote: > Hi Ilya, > > Thanks for raising this and explaining in detail so it can be discussed. Thanks for sharing your thoughts. > Comments below, > > On 02/01/2019 01:11 PM, Ilya Maximets wrote: >> This patch deprecates the usage of current configuration knobs for >> DPDK EAL arguments: >> >> - dpdk-alloc-mem >> - dpdk-socket-mem >> - dpdk-socket-limit >> - dpdk-lcore-mask >> - dpdk-hugepage-dir >> - dpdk-extra >> >> All above configuration options replaced with single 'dpdk-options' >> which has same format as old 'dpdk-extra', i.e. just a string with >> all the DPDK EAL arguments. >> >> All the documentation about old configuration knobs removed. Users >> could still use an old interface, but the deprecation warning will be >> printed. In case 'dpdk-options' provided, values of old options will >> be ignored. New users will start using new 'dpdk-options' as this is >> the only documented way providing EAL arguments. >> >> Rationale: >> >> From release to release DPDK becomes more complex. New EAL arguments >> appears, old arguments becomes deprecated. Some changes their meaning >> and behaviour. It's not easy task to manage all this and current >> code in OVS that tries to manage DPDK options is not flexible/extendable >> enough even to track all the dependencies between options in DPDK 18.11. >> For example, '--socket-mem' changed its meaning, '--legacy-mem' and >> '--socket-limit' appeared. But '--legacy-mem' and '--socket-limit' >> could not be used at the same time and, also, we want to provide >> good default value for '--socket-limit' to keep the consistent >> behaviour of OVS memory usage. And this will be worse in the future. >> > > I think it is an exception that shows it is quite stable. There's > probably been something like 10 DPDK releases (didn't check exactly) > since these params were introduced and this seems to be first time that > there was an issue with an argument. > > It was also a very rare change in DPDK where the memory mgmt was > re-designed and re-written from the ground up. That meant an extra flag > was needed to keep the same behavior. Perhaps if the existing flag was > not reused, there wouldn't have been an issue at all (or at least it > would have been much clearer). I want to clarify that the 'socket-mem' related issue is not fully solved in current OVS. It's still possible for user to specify the 'dpdk-socket-limit' knob and use '--legacy-mem' in 'dpdk-extra'. OVS is not able to track this in current parsing code without introducing nasty workarounds. > >> Another point is that even now we have mutually exclusive database >> configs in OVS and we have to handle them. i.e we're providing >> 'dpdk-alloc-mem' and 'dpdk-socket-mem' at the same time. Additionally, >> user allowed to configure same options in 'dpdk-extra'. So, we have >> similar/equal options in a three places in ovsdb and we need to >> choose which one to use. It's pretty easy for user to get into >> inconsistent database configuration, because users could update >> one field without removing others, and OVS has to resolve all the >> conflicts somehow constructing the EAL args. But we do not know in >> practice, if the resulted arguments are the arguments that user wanted >> to see or just forget to remove the higher priority knob. >> > > That's true about dpdk-alloc-mem and dpdk-socket-mem. I'm not sure if > the docs give some direction on that. IIRC, it is documented and checked > about precedence when using dpdk-extra's. > >> Next point is that we have 'dpdk-lcore-mask' (-c) which is actually not >> so user-friendly as '-l', but we have no option for it. Will we create >> additional 'dpdk-lcore-list' ? I guess, no, because it's not really >> important thing. But users will stuck with this not so user-friendly >> masks. >> > > I'm not sure which is more user-friendly but I agree adding > dpdk-lcore-list is not needed, and probably could add confusion > (especially as dpdk-lcore-mask doesn't need '0x' prefix). IMHO, numbers are more visual than masks. But yes, this will add more confusion. > >> Another thing is that OVS is not really need to check the consistency >> of the EAL arguments because DPDK could check it instead. DPDK will >> always check the args and it will do it better. There is no real >> need to duplicate this functionality. >> >> Keeping all the options in a single string 'dpdk-options' allows: >> >> * To have only one place for all the configs, i.e. it will be harder >> for user to forget to remove something from the single string while >> updating it. >> * Not to check the consistency between different configs, DPDK could >> check the cmdline itself. >> * Not to document every DPDK EAL argument in OVS. We can just >> describe few of them and point to DPDK docs for more information. >> * OVS still able to provide some minimal default arguments. >> Thanks to DPDK 18.11 we only need to specify an lcore.
Re: [ovs-dev] [RFC] OVS robot discussion - check API integration
On Mon, Feb 11, 2019 at 10:24:01AM -0500, Aaron Conole wrote: > I'm following up on a discussion where we briefly talked about adding > a 'check' to OVS patch series during the OVS Conference. 'Check' is > what the patchwork project provides to show that certain tests have been > performed. It is made up of a name, a status, and a URL. Some of the > projects in the ozlabs patchwork instance are using the check feature > already, so there's some examples to reference. > > My idea is to push some data into travis / cirrus that can be used to > insert a check against a patch series. This would allow committers to > look at the status page on patchwork and see at a glance the links to > the travis, and if a robot email was sent. The approach seems reasonable. You mention that there are existing examples. How do they do it? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn: change load balancer references to weak in NB schema
Thanks Daniel (and Lucas). I applied this to master.
On Mon, Feb 11, 2019 at 04:13:39PM +, Lucas Alvares Gomes wrote:
> Thanks Daniel,I agree it makes things easier for the integration with
> OpenStack.
> On Mon, Feb 11, 2019 at 4:07 PM Daniel Alvarez wrote:
> >
> > When a load balancer is added to multiple logical switches
> > and routers it has be to be removed from all of them before
> > being able to delete due to the current 'strong' reference
> > in the NB schema.
> >
> > By changing it to 'weak', users can simply remove the load
> > balancer without having to remove all the references manually.
> > In particular, this will make things easier for networking-ovn,
> > the OpenStack integration project as it'll save some
> > calculations upon load balancer deletion.
> >
> > The update path has been successfully from the previous version
> > of the schema.
> >
> > Signed-off-by: Daniel Alvarez
> > ---
> > ovn/ovn-nb.ovsschema | 8
> > tests/ovn-nbctl.at | 10 +-
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> > index f3683df14..10a59649a 100644
> > --- a/ovn/ovn-nb.ovsschema
> > +++ b/ovn/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Northbound",
> > -"version": "5.14.0",
> > -"cksum": "3600467067 20513",
> > +"version": "5.14.1",
> > +"cksum": "3758097843 20509",
> > "tables": {
> > "NB_Global": {
> > "columns": {
> > @@ -46,7 +46,7 @@
> >"max": "unlimited"}},
> > "load_balancer": {"type": {"key": {"type": "uuid",
> >"refTable":
> > "Load_Balancer",
> > - "refType": "strong"},
> > + "refType": "weak"},
> > "min": 0,
> > "max": "unlimited"}},
> > "dns_records": {"type": {"key": {"type": "uuid",
> > @@ -250,7 +250,7 @@
> > "max": "unlimited"}},
> > "load_balancer": {"type": {"key": {"type": "uuid",
> >"refTable":
> > "Load_Balancer",
> > - "refType": "strong"},
> > + "refType": "weak"},
> > "min": 0,
> > "max": "unlimited"}},
> > "options": {
> > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> > index f55277cee..7a5903c3a 100644
> > --- a/tests/ovn-nbctl.at
> > +++ b/tests/ovn-nbctl.at
> > @@ -752,7 +752,15 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
> > AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
> > AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
> > AT_CHECK([ovn-nbctl lr-lb-del lr0])
> > -AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
> > +AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
> > +
> > +dnl Remove load balancers after adding them to a logical router/switch.
> > +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
> > +AT_CHECK([ovn-nbctl ls-lb-add ls0 lb1])
> > +AT_CHECK([ovn-nbctl lb-del lb0])
> > +AT_CHECK([ovn-nbctl lb-del lb1])
> > +AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
> > +AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])])
> >
> > dnl -
> >
> > --
> > 2.17.2 (Apple Git-113)
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Acked-By: Lucas Alvares Gomes
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-lib.in: Cleanup old socket and pidfiles in stop_daemon
On Mon, Feb 11, 2019 at 05:22:53PM +0100, Timothy Redaelli wrote: > Currently if a client crashes (signal 11) the unix socket (.ctl) and the > pidfile may not be deleted when you use ovs-ctl stop or restart. > > Moreover since ovs-appctl is used on a closed socket some warnings are > printed. > > This commit deletes the pidfile and the unix socket then returns without > running ovs-appctl if the pidfile point to a not-existing pid. > > Reported-at: https://bugzilla.redhat.com/1667845 > Reported-by: Candido Campos > Signed-off-by: Timothy Redaelli Thanks. I applied this to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/8] travis: Fix equal distcheck runs.
On Fri, Feb 08, 2019 at 07:48:52PM +0300, Ilya Maximets wrote: > All the distcheck runs in Travis are equal. Only compiler changes. > This patch set targeted to actually run testsuite with desired > build options. See patch #4 for details. Thank you! I applied this series to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Normalize exit code for Windows
On Mon, Feb 11, 2019 at 05:57:27PM +0200, Alin Gabriel Serdean wrote: > Using python `sys.exit(-1)` on Windows produces mixed results. > Let's take the following results from different shells: > CMD > >python -c "import sys; sys.exit(-1)" & echo %errorlevel% > 1 > MSYS > $ python -c "import sys; sys.exit(-1)" && echo $? > 0 > WSL > $ python -c "import sys; sys.exit(-1)"; echo $? > 255 > > this results in the following tests to fail: > checkpatch > > 10: checkpatch - sign-offs FAILED (checkpatch.at:32) > 11: checkpatch - parenthesized constructs FAILED (checkpatch.at:32) > 12: checkpatch - parenthesized constructs - for FAILED (checkpatch.at:32) > 13: checkpatch - comments FAILED (checkpatch.at:32) > > because of: > ./checkpatch.at:32: exit code was 0, expected 255 > > This patch introduces a positive constant for the default exit code. > > Signed-off-by: Alin Gabriel Serdean > Acked-by: Ben Pfaff > Acked-by: Aaron Conole I'm pretty sure that this is just for exit on failure, so probably EXIT_FAILURE (or similar) would be a better name. I'm not sure why we're using -1 (or 255). Most OVS utilities, and most Unix-like software in general, use exit status 1 on failure. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovs-lib.in: Cleanup old socket and pidfiles in stop_daemon
Currently if a client crashes (signal 11) the unix socket (.ctl) and the
pidfile may not be deleted when you use ovs-ctl stop or restart.
Moreover since ovs-appctl is used on a closed socket some warnings are
printed.
This commit deletes the pidfile and the unix socket then returns without
running ovs-appctl if the pidfile point to a not-existing pid.
Reported-at: https://bugzilla.redhat.com/1667845
Reported-by: Candido Campos
Signed-off-by: Timothy Redaelli
---
utilities/ovs-lib.in | 4
1 file changed, 4 insertions(+)
diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
index 9a0af2e82..fa840ec63 100644
--- a/utilities/ovs-lib.in
+++ b/utilities/ovs-lib.in
@@ -235,6 +235,10 @@ start_daemon () {
stop_daemon () {
if test -e "$rundir/$1.pid"; then
if pid=`cat "$rundir/$1.pid"`; then
+if pid_exists "$pid" >/dev/null 2>&1; then :; else
+rm -f $rundir/$1.$pid.ctl $rundir/$1.$pid
+return 0
+fi
graceful="EXIT .1 .25 .65 1"
actions="TERM .1 .25 .65 1 1 1 1 \
--
2.20.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn: change load balancer references to weak in NB schema
Thanks Daniel,I agree it makes things easier for the integration with OpenStack.
On Mon, Feb 11, 2019 at 4:07 PM Daniel Alvarez wrote:
>
> When a load balancer is added to multiple logical switches
> and routers it has be to be removed from all of them before
> being able to delete due to the current 'strong' reference
> in the NB schema.
>
> By changing it to 'weak', users can simply remove the load
> balancer without having to remove all the references manually.
> In particular, this will make things easier for networking-ovn,
> the OpenStack integration project as it'll save some
> calculations upon load balancer deletion.
>
> The update path has been successfully from the previous version
> of the schema.
>
> Signed-off-by: Daniel Alvarez
> ---
> ovn/ovn-nb.ovsschema | 8
> tests/ovn-nbctl.at | 10 +-
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index f3683df14..10a59649a 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> -"version": "5.14.0",
> -"cksum": "3600467067 20513",
> +"version": "5.14.1",
> +"cksum": "3758097843 20509",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -46,7 +46,7 @@
>"max": "unlimited"}},
> "load_balancer": {"type": {"key": {"type": "uuid",
>"refTable":
> "Load_Balancer",
> - "refType": "strong"},
> + "refType": "weak"},
> "min": 0,
> "max": "unlimited"}},
> "dns_records": {"type": {"key": {"type": "uuid",
> @@ -250,7 +250,7 @@
> "max": "unlimited"}},
> "load_balancer": {"type": {"key": {"type": "uuid",
>"refTable":
> "Load_Balancer",
> - "refType": "strong"},
> + "refType": "weak"},
> "min": 0,
> "max": "unlimited"}},
> "options": {
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index f55277cee..7a5903c3a 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -752,7 +752,15 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
> AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
> AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
> AT_CHECK([ovn-nbctl lr-lb-del lr0])
> -AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
> +AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
> +
> +dnl Remove load balancers after adding them to a logical router/switch.
> +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
> +AT_CHECK([ovn-nbctl ls-lb-add ls0 lb1])
> +AT_CHECK([ovn-nbctl lb-del lb0])
> +AT_CHECK([ovn-nbctl lb-del lb1])
> +AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
> +AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])])
>
> dnl -
>
> --
> 2.17.2 (Apple Git-113)
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Acked-By: Lucas Alvares Gomes
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovn: change load balancer references to weak in NB schema
When a load balancer is added to multiple logical switches
and routers it has be to be removed from all of them before
being able to delete due to the current 'strong' reference
in the NB schema.
By changing it to 'weak', users can simply remove the load
balancer without having to remove all the references manually.
In particular, this will make things easier for networking-ovn,
the OpenStack integration project as it'll save some
calculations upon load balancer deletion.
The update path has been successfully from the previous version
of the schema.
Signed-off-by: Daniel Alvarez
---
ovn/ovn-nb.ovsschema | 8
tests/ovn-nbctl.at | 10 +-
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index f3683df14..10a59649a 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Northbound",
-"version": "5.14.0",
-"cksum": "3600467067 20513",
+"version": "5.14.1",
+"cksum": "3758097843 20509",
"tables": {
"NB_Global": {
"columns": {
@@ -46,7 +46,7 @@
"max": "unlimited"}},
"load_balancer": {"type": {"key": {"type": "uuid",
"refTable": "Load_Balancer",
- "refType": "strong"},
+ "refType": "weak"},
"min": 0,
"max": "unlimited"}},
"dns_records": {"type": {"key": {"type": "uuid",
@@ -250,7 +250,7 @@
"max": "unlimited"}},
"load_balancer": {"type": {"key": {"type": "uuid",
"refTable": "Load_Balancer",
- "refType": "strong"},
+ "refType": "weak"},
"min": 0,
"max": "unlimited"}},
"options": {
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index f55277cee..7a5903c3a 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -752,7 +752,15 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
AT_CHECK([ovn-nbctl lr-lb-del lr0])
-AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
+
+dnl Remove load balancers after adding them to a logical router/switch.
+AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
+AT_CHECK([ovn-nbctl ls-lb-add ls0 lb1])
+AT_CHECK([ovn-nbctl lb-del lb0])
+AT_CHECK([ovn-nbctl lb-del lb1])
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
+AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])])
dnl -
--
2.17.2 (Apple Git-113)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] checkpatch: Normalize exit code for Windows
> De la: ovs-dev-boun...@openvswitch.org boun...@openvswitch.org> În numele Aaron Conole
> Trimis: Tuesday, February 5, 2019 10:39 PM
> Către: Alin Gabriel Serdean
> Cc: d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] checkpatch: Normalize exit code for Windows
>
> Alin Gabriel Serdean writes:
>
> > Using python `sys.exit(-1)` on Windows produces mixed results.
> > Let's take the following results from different shells:
> > CMD
> >>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
> > 1
> > MSYS
> > $ python -c "import sys; sys.exit(-1)" && echo $?
> > 0
> > WSL
> > $ python -c "import sys; sys.exit(-1)"; echo $?
> > 255
> >
> > this results in the following tests to fail:
> > checkpatch
> >
> > 10: checkpatch - sign-offs FAILED
(checkpatch.at:32)
> > 11: checkpatch - parenthesized constructs FAILED
(checkpatch.at:32)
> > 12: checkpatch - parenthesized constructs - for FAILED
> (checkpatch.at:32)
> > 13: checkpatch - comments FAILED
(checkpatch.at:32)
> >
> > because of:
> > ./checkpatch.at:32: exit code was 0, expected 255
> >
> > This patch introduces a positive constant for the default exit code.
> >
> > Signed-off-by: Alin Gabriel Serdean
> > ---
>
> Acked-by: Aaron Conole
>
> With one little fixup:
>
> 1. either we should eliminate the result argument to
>ovs_checkpatch_print_result
>
> 2. or the test for the result < 0 needs to change to if result:
>
> Otherwise, when an error is introduced, the exit code will be propagated,
> but the printout will be confusing:
>
> 03:30:50 aconole {(0a8c1ec04...)} ~/git/ovs$ git commit -m "dpdk: This
is a
> test
> Just testing
> "
> ERROR: C99 style comment
> #10 FILE: lib/dpdk.c:17:
> // BREAK!!
>
> Lines checked: 15, no obvious problems found
>
> :-)
>
[Alin Serdean] Thanks for the feedback. I sent an incremental:
https://patchwork.ozlabs.org/patch/1039912/ . Do you mind taking another
look?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Detectar Fraudes - Control Interno
Cursos escenciales - Webinar Interactivo – Miércoles 20 de Febrero Guía de Auditoría y Control Interno para Detectar Fraudes Nuestro webinar funciona como una guía para que el participante revise los conceptos clave del Control Interno y la Auditoría para la prevención de escenarios que conlleven malas prácticas sobre estructuras organizacionales de cualquier tamaño y reconozca la documentación necesaria así como los procedimientos y procesos para implementar las mejores prácticas corporativas. Perfeccionar, actualizar y mejorar el proceso en que la Auditoría Interna se lleva a cabo para detección oportuna de fraudes. OBJETIVOS ESPECÍFICOS: • El participante repasará los conceptos básicos de Auditoría y Control Interno. • El participante revisará los requerimientos éticos del Auditor Interno y los principales tipos de Auditoría Interna. • El participante estudiará los procesos y procedimientos de Auditoría Interna. • El participante revisará el concepto de fraude y su responsabilidad ante el descubrimiento del mismo. Para mayor información, responder sobre este correo con la palabra Auditoría + los siguientes datos: NOMBRE: TELÉFONO: EMPRESA: Llamanos al (045) 55 1554 6630 www.Innovalearn.mx ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] checkpatch: Normalize exit code for Windows
Using python `sys.exit(-1)` on Windows produces mixed results.
Let's take the following results from different shells:
CMD
>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
1
MSYS
$ python -c "import sys; sys.exit(-1)" && echo $?
0
WSL
$ python -c "import sys; sys.exit(-1)"; echo $?
255
this results in the following tests to fail:
checkpatch
10: checkpatch - sign-offs FAILED (checkpatch.at:32)
11: checkpatch - parenthesized constructs FAILED (checkpatch.at:32)
12: checkpatch - parenthesized constructs - for FAILED (checkpatch.at:32)
13: checkpatch - comments FAILED (checkpatch.at:32)
because of:
./checkpatch.at:32: exit code was 0, expected 255
This patch introduces a positive constant for the default exit code.
Signed-off-by: Alin Gabriel Serdean
Acked-by: Ben Pfaff
Acked-by: Aaron Conole
---
v2: factor out result from `ovs_checkpatch_print_result as suggested by
---
utilities/checkpatch.py | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 8eab31f60..9b5822324 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -24,6 +24,7 @@ import sys
RETURN_CHECK_INITIAL_STATE = 0
RETURN_CHECK_STATE_WITH_RETURN = 1
RETURN_CHECK_AWAITING_BRACE = 2
+EXIT_CODE = 255
__errors = 0
__warnings = 0
empty_return_check_state = 0
@@ -837,7 +838,7 @@ def ovs_checkpatch_parse(text, filename, author=None,
committer=None):
run_file_checks(text)
if __errors or __warnings:
-return -1
+return EXIT_CODE
return 0
@@ -863,10 +864,10 @@ Check options:
% sys.argv[0])
-def ovs_checkpatch_print_result(result):
+def ovs_checkpatch_print_result():
global quiet, __warnings, __errors, total_line
-if result < 0:
+if __errors or __warnings:
print("Lines checked: %d, Warnings: %d, Errors: %d\n" %
(total_line, __warnings, __errors))
elif not quiet:
@@ -886,7 +887,7 @@ def ovs_checkpatch_file(filename):
result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
mail.get('Author', mail['From']),
mail['Commit'])
-ovs_checkpatch_print_result(result)
+ovs_checkpatch_print_result()
return result
@@ -920,7 +921,7 @@ if __name__ == '__main__':
"quiet"])
except:
print("Unknown option encountered. Please rerun with -h for help.")
-sys.exit(-1)
+sys.exit(EXIT_CODE)
for o, a in optlist:
if o in ("-h", "--help"):
@@ -946,7 +947,7 @@ if __name__ == '__main__':
quiet = True
else:
print("Unknown option '%s'" % o)
-sys.exit(-1)
+sys.exit(EXIT_CODE)
if sys.stdout.isatty():
colors = True
@@ -972,17 +973,17 @@ Subject: %s
if not quiet:
print('== Checking %s ("%s") ==' % (revision[0:12], name))
result = ovs_checkpatch_parse(patch, revision)
-ovs_checkpatch_print_result(result)
+ovs_checkpatch_print_result()
if result:
-status = -1
+status = EXIT_CODE
sys.exit(status)
if not args:
if sys.stdin.isatty():
usage()
-sys.exit(-1)
+sys.exit(EXIT_CODE)
result = ovs_checkpatch_parse(sys.stdin.read(), '-')
-ovs_checkpatch_print_result(result)
+ovs_checkpatch_print_result()
sys.exit(result)
status = 0
@@ -991,5 +992,5 @@ Subject: %s
print('== Checking "%s" ==' % filename)
result = ovs_checkpatch_file(filename)
if result:
-status = -1
+status = EXIT_CODE
sys.exit(status)
--
2.16.1.windows.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Have needs to retouch your photos?
Do you have needs for retouching your photos? Or do deep etching and masking for your photos, We are the image service provider who can do this for you. Please send photos to start testing, then you cam judge the quality of our service. Thanks, Cindy Wesdl Bruchsdal ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 1/3] dpdk: Unify passing of DPDK EAL arguments.
On 07.02.2019 19:56, Flavio Leitner wrote: > On Wed, Feb 06, 2019 at 11:33:04AM +0300, Ilya Maximets wrote: >> On 05.02.2019 23:19, Flavio Leitner wrote: >>> On Tue, Feb 05, 2019 at 06:43:08PM +0300, Ilya Maximets wrote: Hi Flavio. Thanks for taking a look. On 05.02.2019 15:38, Flavio Leitner wrote: > > Hi Ilya, > > Thanks for the patch and I think we knew about that pain when we > exposed the very first parameter. I still remember Aaron struggling > to find the best path forward. Time flies and here we are. > > The problem is that this change is not friendly from the community > perspective to its users. That is like an exposed API which we should > avoid break as much as possible. > > For instance, there are users (OpenStack) with support life cycle of > 5+ years that cannot keep up with this kind of change but they expect > to be able to update to newer OVS. Sure, there are users that wants stable API that will never change. But this is really impossible in practice. I'm working with OpenStack too and will have to fixup deployment tools with these changes. BTW, from the whole OpenStack only few deployment projects (like TripleO) will need to make changes and these changes are not very big. >>> >>> That's only part of the work. There will be work on QA, documentation >>> and even migration path from one to another. And we can't change the >>> past for existing deployments. >> >> Sure. But incompatible API changes are almost unavoidable for a young >> projects that wants to be better. > > My point here is that the associated impact and cost of this change is > far from trivial. I agree. > >>> I don't think we should remove the docs if the parameters are there as >>> a first step. I mean, assume an existing deployment, there is a parameter >>> which might be in use but there is no documentation available. That >>> doesn't sound like a good user experience to me. >> >> Maybe we could save a man pages while removing the guides. There is no much >> information in Documentation/intro/install/dpdk.rst anyway. > > Agreed. > >>> On another hand, you could introduce the new interface and update the >>> docs to recommend using the new one because the old one will be removed >>> in the future. Warning messages come next, and then finally its removal. >> >> I'd prefer to have warning messages to be there right from the start to >> push users to migrate to the new interfaces as early as possible. > > If it's a WARN level message then I can tell you it will break > environments over here. If that is a INFO level message, then it might > be okay. Though I still think there could be a period of time where > both could co-exist before we announce/warn about the deprecation of > the current interface. Yeah, most testing tools wants to have no warning at all. INFO level messages could be fine for the period when we're still supporting old knobs. I'm not sure about co-existence without any messages at all. I think that we need something that will push users to migrate to the new interface. > >> How about this: >> >> First stage (apply now, release in 2.12): >> - Introduce new interface 'dpdk-options'. >> - Rewrite installation guide with new interface fully removing the old one. >> - Add new interface to man pages (vswitch.xml) and mark all the old options >> as deprecated by adding something like: "Deprecated. 'dpdk-options' >> should >> be used instead. Will be ignored in the future." >> - Add a runtime deprecation warning if old interface is in use. > > ^^^ this is bad as an initial step as explained above. Sure. Could be changed to INFO. > >> - Ignore values of old knobs if 'dpdk-options' provided. > > Sounds like a good transition barrier. > > >> Second stage (release in 2.13 or 2.14, could wait longer if required): >> - Remove old interfaces wile keeping the warnings. (i.e. values always >> ignored) >> - Remove old knobs from the man pages. >> >> Third stage (optional): >> - Remove warnings. >> >> So the main difference from the current patches is delaying removal of the >> man >> pages to the second stage. > [...] We're keeping few sane defaults like providing default lcore and setting the socket-limit if needed. And we'll try to do that in the future. The thing this patch tries to eliminate is the dependency tracking between different EAL arguments, i.e. duplicating the work with rte_eal_init() and having too many configuration knobs with similar meanings what does not work at the same time. Right now there are no critical arguments that user must provide. So, IMHO, having special configs for them is really unnecessary. >>> >>> Do you think the defaults work for the majority of DPDK users? >> >> My personal experience is opposite. Most of my deployments and deployments >> that >> I had to work with had massive 'dpdk-
[ovs-dev] [RFC] OVS robot discussion - check API integration
I'm following up on a discussion where we briefly talked about adding
a 'check' to OVS patch series during the OVS Conference. 'Check' is
what the patchwork project provides to show that certain tests have been
performed. It is made up of a name, a status, and a URL. Some of the
projects in the ozlabs patchwork instance are using the check feature
already, so there's some examples to reference.
My idea is to push some data into travis / cirrus that can be used to
insert a check against a patch series. This would allow committers to
look at the status page on patchwork and see at a glance the links to
the travis, and if a robot email was sent.
The naivest way I can think of getting these results published is by
modifying ${OS}-build.sh like:
---
diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index 0a2091061..38359c86f 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -8,6 +8,47 @@ SPARSE_FLAGS=""
EXTRA_OPTS=""
TARGET="x86_64-native-linuxapp-gcc"
+function start_patchwork_check()
+{
+if [ ! -e ".git/config" ]; then
+return;
+fi
+
+if ! git branch | grep \\* | grep series_ >/dev/null; then
+return;
+fi
+
+curl -s $PW_API_URL/$(SERIES_ID) ...submit check start ...
+}
+
+function bad_check_result()
+{
+if [ ! -e ".git/config" ]; then
+return;
+fi
+
+if ! git branch | grep \\* | grep series_ >/dev/null; then
+return;
+fi
+
+curl -s $PW_API_URL/$(SERIES_ID) ...set check to bad...
+}
+
+function check_success()
+{
+if [ ! -e ".git/config" ]; then
+return;
+fi
+
+if ! git branch | grep \\* | grep series_ >/dev/null; then
+return;
+fi
+
+curl -s $PW_API_URL/$(SERIES_ID) ...set check to success...
+}
+
+trap '[ "$?" -eq 0 ] || bad_check_result' INT ERR TERM
+
function install_kernel()
{
if [[ "$1" =~ ^4.* ]]; then
---
I think this could work. We can use the travis "encrypted info" data to
push the credentials but that probably needs to be encrypted for the
robot key. It does put some kind of pollution in the travis / cirrus
builds (because 'series_*' appears nowhere in the official repository).
Maybe that isn't acceptable.
I am planning to follow the guidebook at
https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings
and store the credentials variables for just the OVS robot repo.
Maybe there's another opinion. Probably there's a better way of
sending the data around, sharing credentials, or passing requests so
that other projects can also integrate. Thoughts, opinions, comments?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] dpif-netdev:fix reload pmd's dead lock
On 11.02.2019 6:22, Lilijun (Jerry, Cloud Networking) wrote: > Yes, Thanks for your help again. > > Let's forget the patch :). But in my opinion, the function > reload_affected_pmds() doesn't need be locked within dp->port_mutex. Is > that true? That's true that 'reload_affected_pmds()' itself doesn't need to be locked. However, you can't just unlock the mutex and lock it again, because upper layer functions that calls 'reconfigure_datapath()' expects that mutex will be held all the way down. For example, 'dp_netdev_free()' iterates over the 'dp->ports' hmap and it doesn't expect that 'do_del_port()' could unlock the mutex. If hmap will be changed (some ports added), the iterator could crash. Another example is 'dpif_dummy_change_port_number()' that calls the reconfiguration twice in a row expecting no changes in 'dp->ports' between. Best regards, Ilya Maximets. > >> -Original Message- >> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- >> boun...@openvswitch.org] On Behalf Of Stokes, Ian >> Sent: Thursday, February 07, 2019 5:25 PM >> To: Ilya Maximets ; d...@openvswitch.org >> Subject: Re: [ovs-dev] dpif-netdev:fix reload pmd's dead lock >> >>> On 31.01.2019 11:48, Lilijun wrote: This patch fix the dead lock when using dpdk userspace datapath. The problem is described as follows: 1) when add or delete port, the main thread will call reconfigure_datapath() in the function dpif_netdev_run() 2) Here the dp->port_mutex is locked. In dp_netdev_reload_pmd__(), it will notify each pmd to reload. 3) If pmd is doing packet upcall in fast_path_processing() and try to get dp->port_mutex in do_xlate_actions()-->tnl_route_lookup_flow()-- dpif_netdev_port_query_by_name(). Here pmd get lock failed because the main thread has got the lock in step 2. 4) So the main thread was stuck for waiting pmd to reload done. Now we got a dead lock. Here reload_affected_pmds() may not need to lock dp->port_mutex. So we release the lock temporarily when calling reload_affected_pmds(). Signed-off-by: Lilijun >>> >>> Replying just to keep answers on a list/patchwork consistent. >>> The deadlock caused by some local changes done by the user. >>> Not possible in upstream master. See discussion for the previous >>> version of this patch that is missing in patchwork for some reason: >>> >>> >>> https://mail.openvswitch.org/pipermail/ovs-dev/2019- >> January/355756.htm >>> l >>> >>> Best regards, Ilya Maximets. >>> >> >> Thanks for clarifying Ilya, there was discussion of this at the community >> meeting yesterday but it seems a non-issue now. >> >> Thanks >> Ian >> ___ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On Mon, Feb 11, 2019 at 11:25:28AM +, Roi Dayan wrote: > > > On 11/02/2019 12:13, Simon Horman wrote: > > On Mon, Feb 11, 2019 at 09:57:10AM +, Roi Dayan wrote: > >> > >> > >> On 11/02/2019 11:11, Simon Horman wrote: > >>> On Sun, Feb 10, 2019 at 08:04:39AM +, Roi Dayan wrote: > > > On 04/02/2019 15:20, Roi Dayan wrote: > > Hi Simon, > > > > I did some checks and think the correct fix is to offload exact > > match. > > if key is partial we can ignore the mask and offload exact match and > > it will be correct as we do more strict matching. > > > > it is also documented that the kernel datapath is doing the same > > (from datapath.rst) > > > > "The kernel can ignore the mask attribute, installing an exact > > match flow" > > > > So I think the first patch V0 is actually correct as we > > check the tunnel key flag exists and offload exact match if > > there was any mask or offload without a key if the mask is 0 > > or no key. > > > > in netdev-tc-offloads.c > > > > +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > > + tnl_mask->tun_id : 0; > > > I think this is fine so long as tun_id is all-ones. Is that always > the case? > Should the code check that it is the case? Am I missing the point? > > >>> it looks like tun_id mask is always set to all-ones. > >>> but even if it won't be in the future, we shouldn't really care here. > >>> tc adds exact match on the tun_id and ignores the tun_id mask. > >>> this is considered ok as the matching is more strict. > >>> if new match is needed with different tun_id then ovs will try to add > >>> another rule for it. > >>> so with tc we could have multiple rules vs 1 rule that support mask. > >> > >> Thanks for looking into this. That sounds find to me but I wonder if > >> we should make > >> this behaviour explicit. > >> > >> /* > >> * Comment describing why the mask is 0 or all-ones > >> */ > >> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > >> UINT32_MAX : 0; > >> > > I think its nicer like this and symetric currently. here it's generic > > and "use" mask. > > in tc.c when we fill the netlink msg we ignore the mas and also when > > parsing tc dump, > > tun mask is set to here (tc.c) and not netdev-tc-offloads.c > > > > Hi Simon, > > any update? i remind i suggested v0 is the correct one. > >>> > >>> Thanks and sorry for the ongoing delays. > >>> > >>> I have added this (v2) to a travisci instance and plan to apply the patch > >>> to master if that passes. > >> > >> Hi Simon, > >> > >> I agreed with you v2 has an issue and we should use v0. > >> v2 will not pass id to tc if there is an id with partial mask which is > >> incorrect. > >> v0 will add rule with exact match. > >> can we go with v0 ? > > > > Sure, can I confirm that you mean this version from 17 Jan: > > > > "[PATCH] lib/tc: Support optional tunnel id" > > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1026771%2F&data=02%7C01%7Croid%40mellanox.com%7Ca5fa4a3f7136417620d508d69009845d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854767892746889&sdata=dU0B%2BPf3H6a4fHCTQfXFkuyuuFcZoYZFihfcGb9dIMY%3D&reserved=0 > > > > yes Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V4 3/3] acinclude: Omit unnecessary define
Commit fc3b425fa02f ("acinclude: Include libmnl when needed") added
unnecessary include of DPDK_MNL. Omit it.
Fixes: fc3b425fa02f ("acinclude: Include libmnl when needed")
Signed-off-by: Eli Britstein
---
acinclude.m4 | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 1dcae6e85..7973a0bd0 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -293,8 +293,7 @@ AC_DEFUN([OVS_CHECK_DPDK], [
#endif
], [])
], [],
- [AC_SEARCH_LIBS([mnl_attr_put],[mnl],[],[AC_MSG_ERROR([unable to find
libmnl, install the dependency package])])
- AC_DEFINE([DPDK_MNL], [1], [MLX5 PMD detected in DPDK.])])
+ [AC_SEARCH_LIBS([mnl_attr_put],[mnl],[],[AC_MSG_ERROR([unable to find
libmnl, install the dependency package])])])
AC_COMPILE_IFELSE([
AC_LANG_PROGRAM(
--
2.14.5
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V4 2/3] acinclude: Include libverbs and libmlx4 when needed
DPDK 18.11 uses libverbs and libmlx4 when MLX4 PMD is enabled. This commit makes OVS to link to libverbs and libmlx4 when MLX4 PMD is enabled on DPDK. Signed-off-by: Eli Britstein Reviewed-by: Asaf Penso --- acinclude.m4 | 14 ++ 1 file changed, 14 insertions(+) diff --git a/acinclude.m4 b/acinclude.m4 index f5a5948fc..1dcae6e85 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -307,12 +307,26 @@ AC_DEFUN([OVS_CHECK_DPDK], [ ], [], [AC_SEARCH_LIBS([mlx5dv_create_wq],[mlx5],[],[AC_MSG_ERROR([unable to find libmlx5, install the dependency package])])]) +AC_COMPILE_IFELSE([ + AC_LANG_PROGRAM( +[ + #include +#if defined(RTE_LIBRTE_MLX4_PMD) && !defined(RTE_LIBRTE_MLX4_DLOPEN_DEPS) +#error +#endif +], []) + ], [], + [AC_SEARCH_LIBS([mlx4dv_init_obj],[mlx4],[],[AC_MSG_ERROR([unable to find libmlx4, install the dependency package])])]) + AC_COMPILE_IFELSE([ AC_LANG_PROGRAM( [ #include #if defined(RTE_LIBRTE_MLX5_PMD) && !defined(RTE_LIBRTE_MLX5_DLOPEN_DEPS) #error +#endif +#if defined(RTE_LIBRTE_MLX4_PMD) && !defined(RTE_LIBRTE_MLX4_DLOPEN_DEPS) +#error #endif ], []) ], [], -- 2.14.5 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V4 1/3] acinclude: Include libverbs and libmlx5 when needed
DPDK 18.11 uses libverbs and libmlx5 when MLX5 PMD is enabled.
This commit makes OVS to link to libverbs and libmlx5 when MLX5 PMD is
enabled on DPDK.
Signed-off-by: Eli Britstein
Reviewed-by: Shahaf Shuler
Reviewed-by: Asaf Penso
---
acinclude.m4 | 22 ++
1 file changed, 22 insertions(+)
diff --git a/acinclude.m4 b/acinclude.m4
index c51af246a..f5a5948fc 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -296,6 +296,28 @@ AC_DEFUN([OVS_CHECK_DPDK], [
[AC_SEARCH_LIBS([mnl_attr_put],[mnl],[],[AC_MSG_ERROR([unable to find
libmnl, install the dependency package])])
AC_DEFINE([DPDK_MNL], [1], [MLX5 PMD detected in DPDK.])])
+AC_COMPILE_IFELSE([
+ AC_LANG_PROGRAM(
+[
+ #include
+#if defined(RTE_LIBRTE_MLX5_PMD) && !defined(RTE_LIBRTE_MLX5_DLOPEN_DEPS)
+#error
+#endif
+], [])
+ ], [],
+ [AC_SEARCH_LIBS([mlx5dv_create_wq],[mlx5],[],[AC_MSG_ERROR([unable to
find libmlx5, install the dependency package])])])
+
+AC_COMPILE_IFELSE([
+ AC_LANG_PROGRAM(
+[
+ #include
+#if defined(RTE_LIBRTE_MLX5_PMD) && !defined(RTE_LIBRTE_MLX5_DLOPEN_DEPS)
+#error
+#endif
+], [])
+ ], [],
+ [AC_SEARCH_LIBS([verbs_init_cq],[ibverbs],[],[AC_MSG_ERROR([unable to
find libibverbs, install the dependency package])])])
+
# On some systems we have to add -ldl to link with dpdk
#
# This code, at first, tries to link without -ldl (""),
--
2.14.5
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V4 0/3] Include libverbs and libmlx4/5 when needed
This patch set automatically links with the necessary libraries for MLX4/5 DPDK PMDs. Patch 1 automatically links with the necessary libraries for MLX5 PMD Patch 2 automatically links with the necessary libraries for MLX4 PMD Patch 3 removes unnecessary define previously done for automatically link libmnl Eli Britstein (3): acinclude: Include libverbs and libmlx5 when needed acinclude: Include libverbs and libmlx4 when needed acinclude: Omit unnecessary define acinclude.m4 | 39 +-- 1 file changed, 37 insertions(+), 2 deletions(-) -- 2.14.5 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On 11/02/2019 12:13, Simon Horman wrote: > On Mon, Feb 11, 2019 at 09:57:10AM +, Roi Dayan wrote: >> >> >> On 11/02/2019 11:11, Simon Horman wrote: >>> On Sun, Feb 10, 2019 at 08:04:39AM +, Roi Dayan wrote: On 04/02/2019 15:20, Roi Dayan wrote: > Hi Simon, > > I did some checks and think the correct fix is to offload exact match. > if key is partial we can ignore the mask and offload exact match and > it will be correct as we do more strict matching. > > it is also documented that the kernel datapath is doing the same > (from datapath.rst) > > "The kernel can ignore the mask attribute, installing an exact > match flow" > > So I think the first patch V0 is actually correct as we > check the tunnel key flag exists and offload exact match if > there was any mask or offload without a key if the mask is 0 > or no key. > > in netdev-tc-offloads.c > > +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > + tnl_mask->tun_id : 0; > I think this is fine so long as tun_id is all-ones. Is that always the case? Should the code check that it is the case? Am I missing the point? >>> it looks like tun_id mask is always set to all-ones. >>> but even if it won't be in the future, we shouldn't really care here. >>> tc adds exact match on the tun_id and ignores the tun_id mask. >>> this is considered ok as the matching is more strict. >>> if new match is needed with different tun_id then ovs will try to add >>> another rule for it. >>> so with tc we could have multiple rules vs 1 rule that support mask. >> >> Thanks for looking into this. That sounds find to me but I wonder if we >> should make >> this behaviour explicit. >> >> /* >> * Comment describing why the mask is 0 or all-ones >> */ >> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >> UINT32_MAX : 0; >> > I think its nicer like this and symetric currently. here it's generic > and "use" mask. > in tc.c when we fill the netlink msg we ignore the mas and also when > parsing tc dump, > tun mask is set to here (tc.c) and not netdev-tc-offloads.c > Hi Simon, any update? i remind i suggested v0 is the correct one. >>> >>> Thanks and sorry for the ongoing delays. >>> >>> I have added this (v2) to a travisci instance and plan to apply the patch >>> to master if that passes. >> >> Hi Simon, >> >> I agreed with you v2 has an issue and we should use v0. >> v2 will not pass id to tc if there is an id with partial mask which is >> incorrect. >> v0 will add rule with exact match. >> can we go with v0 ? > > Sure, can I confirm that you mean this version from 17 Jan: > > "[PATCH] lib/tc: Support optional tunnel id" > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1026771%2F&data=02%7C01%7Croid%40mellanox.com%7Ca5fa4a3f7136417620d508d69009845d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854767892746889&sdata=dU0B%2BPf3H6a4fHCTQfXFkuyuuFcZoYZFihfcGb9dIMY%3D&reserved=0 > yes ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Fwd: Hello
Good day how are you and your family doing can you hold a deal Добрый день как дела у вас и вашей семьи можете ли вы заключить сделку? С уважением, ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/1] acinclude: Also use LIBS from dpkg pkg-config
On Mon, Feb 11, 2019 at 11:06 AM Luca Boccassi wrote: > > On Mon, 2019-02-11 at 07:59 +0100, Christian Ehrhardt wrote: > > On Sun, Feb 10, 2019 at 5:53 PM Ben Pfaff wrote: > > > > > > On Thu, Feb 07, 2019 at 12:46:40PM +0100, Christian Ehrhardt wrote: > > > > DPDK 18.11 builds using the more modern meson build system no > > > > more > > > > provide the -ldpdk linker script. Instead it is expected to use > > > > pkgconfig for linker options as well. > > > > > > > > This change will set DPDK_LIB from pkg-config (if pkg-config was > > > > available) and since that already carries the whole-archive flags > > > > around the PMDs skips the further wrapping in more whole-archive > > > > if that is already part of DPDK_LIB. > > > > > > > > To work reliable in all environments this needs pkg-config > > > > 0.29.1. > > > > We want to be able to use PKG_CHECK_MODULES_STATIC which > > > > is not yet available in 0.24. Therefore update pkg.m4 > > > > to pkg-config 0.29.1. > > > > > > > > This should be backport-safe as these macro files are all > > > > versioned. > > > > autoconf is smart enough to check the version if you have it > > > > locally, > > > > and if the system's is higher, it will use that one instead. > > > > > > > > Finally make configure.ac use the locally provided pkg.m4 before > > > > calling the PKG_PROG_PKG_CONFIG macro. > > > > > > > > Signed-off-by: Christian Ehrhardt > > > om> > > > > > > The explicit m4_include([m4/pkg.m4]) should not be > > > necessary. aclocal > > > should pick it up automatically. > > > > Thanks Ben for the hint. > > I agree that it might pick it up automatically, but I have learned > > not > > rely on too much with autotools :-/ > > @Luca - to add the explicit include was your suggestion from Friday > > the 25th, do you see a potential issue if we don't? > > Yes it should work without - but only one way to be sure :-) yeah - "should (tm)" :-) @Ben Pfaff - it surely works with the include and I see now drawback :-) If you want me I can send a V5 which drops the explicit include, but without being called to do so I'd lean towards keeping it explicit to be sure. Let me know what you prefer. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On Mon, Feb 11, 2019 at 09:57:10AM +, Roi Dayan wrote: > > > On 11/02/2019 11:11, Simon Horman wrote: > > On Sun, Feb 10, 2019 at 08:04:39AM +, Roi Dayan wrote: > >> > >> > >> On 04/02/2019 15:20, Roi Dayan wrote: > >>> Hi Simon, > >>> > >>> I did some checks and think the correct fix is to offload exact match. > >>> if key is partial we can ignore the mask and offload exact match and > >>> it will be correct as we do more strict matching. > >>> > >>> it is also documented that the kernel datapath is doing the same > >>> (from datapath.rst) > >>> > >>> "The kernel can ignore the mask attribute, installing an exact > >>> match flow" > >>> > >>> So I think the first patch V0 is actually correct as we > >>> check the tunnel key flag exists and offload exact match if > >>> there was any mask or offload without a key if the mask is 0 > >>> or no key. > >>> > >>> in netdev-tc-offloads.c > >>> > >>> +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > >>> + tnl_mask->tun_id : 0; > >>> > >> I think this is fine so long as tun_id is all-ones. Is that always the > >> case? > >> Should the code check that it is the case? Am I missing the point? > >> > > it looks like tun_id mask is always set to all-ones. > > but even if it won't be in the future, we shouldn't really care here. > > tc adds exact match on the tun_id and ignores the tun_id mask. > > this is considered ok as the matching is more strict. > > if new match is needed with different tun_id then ovs will try to add > > another rule for it. > > so with tc we could have multiple rules vs 1 rule that support mask. > > Thanks for looking into this. That sounds find to me but I wonder if we > should make > this behaviour explicit. > > /* > * Comment describing why the mask is 0 or all-ones > */ > flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > UINT32_MAX : 0; > > >>> I think its nicer like this and symetric currently. here it's generic > >>> and "use" mask. > >>> in tc.c when we fill the netlink msg we ignore the mas and also when > >>> parsing tc dump, > >>> tun mask is set to here (tc.c) and not netdev-tc-offloads.c > >>> > >> > >> Hi Simon, > >> > >> any update? i remind i suggested v0 is the correct one. > > > > Thanks and sorry for the ongoing delays. > > > > I have added this (v2) to a travisci instance and plan to apply the patch > > to master if that passes. > > Hi Simon, > > I agreed with you v2 has an issue and we should use v0. > v2 will not pass id to tc if there is an id with partial mask which is > incorrect. > v0 will add rule with exact match. > can we go with v0 ? Sure, can I confirm that you mean this version from 17 Jan: "[PATCH] lib/tc: Support optional tunnel id" https://patchwork.ozlabs.org/patch/1026771/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/1] acinclude: Also use LIBS from dpkg pkg-config
On Mon, 2019-02-11 at 07:59 +0100, Christian Ehrhardt wrote: > On Sun, Feb 10, 2019 at 5:53 PM Ben Pfaff wrote: > > > > On Thu, Feb 07, 2019 at 12:46:40PM +0100, Christian Ehrhardt wrote: > > > DPDK 18.11 builds using the more modern meson build system no > > > more > > > provide the -ldpdk linker script. Instead it is expected to use > > > pkgconfig for linker options as well. > > > > > > This change will set DPDK_LIB from pkg-config (if pkg-config was > > > available) and since that already carries the whole-archive flags > > > around the PMDs skips the further wrapping in more whole-archive > > > if that is already part of DPDK_LIB. > > > > > > To work reliable in all environments this needs pkg-config > > > 0.29.1. > > > We want to be able to use PKG_CHECK_MODULES_STATIC which > > > is not yet available in 0.24. Therefore update pkg.m4 > > > to pkg-config 0.29.1. > > > > > > This should be backport-safe as these macro files are all > > > versioned. > > > autoconf is smart enough to check the version if you have it > > > locally, > > > and if the system's is higher, it will use that one instead. > > > > > > Finally make configure.ac use the locally provided pkg.m4 before > > > calling the PKG_PROG_PKG_CONFIG macro. > > > > > > Signed-off-by: Christian Ehrhardt > > om> > > > > The explicit m4_include([m4/pkg.m4]) should not be > > necessary. aclocal > > should pick it up automatically. > > Thanks Ben for the hint. > I agree that it might pick it up automatically, but I have learned > not > rely on too much with autotools :-/ > @Luca - to add the explicit include was your suggestion from Friday > the 25th, do you see a potential issue if we don't? Yes it should work without - but only one way to be sure :-) -- Kind regards, Luca Boccassi___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On 11/02/2019 11:11, Simon Horman wrote: > On Sun, Feb 10, 2019 at 08:04:39AM +, Roi Dayan wrote: >> >> >> On 04/02/2019 15:20, Roi Dayan wrote: >>> Hi Simon, >>> >>> I did some checks and think the correct fix is to offload exact match. >>> if key is partial we can ignore the mask and offload exact match and >>> it will be correct as we do more strict matching. >>> >>> it is also documented that the kernel datapath is doing the same >>> (from datapath.rst) >>> >>> "The kernel can ignore the mask attribute, installing an exact >>> match flow" >>> >>> So I think the first patch V0 is actually correct as we >>> check the tunnel key flag exists and offload exact match if >>> there was any mask or offload without a key if the mask is 0 >>> or no key. >>> >>> in netdev-tc-offloads.c >>> >>> +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >>> + tnl_mask->tun_id : 0; >>> >> I think this is fine so long as tun_id is all-ones. Is that always the >> case? >> Should the code check that it is the case? Am I missing the point? >> > it looks like tun_id mask is always set to all-ones. > but even if it won't be in the future, we shouldn't really care here. > tc adds exact match on the tun_id and ignores the tun_id mask. > this is considered ok as the matching is more strict. > if new match is needed with different tun_id then ovs will try to add > another rule for it. > so with tc we could have multiple rules vs 1 rule that support mask. Thanks for looking into this. That sounds find to me but I wonder if we should make this behaviour explicit. /* * Comment describing why the mask is 0 or all-ones */ flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; >>> I think its nicer like this and symetric currently. here it's generic >>> and "use" mask. >>> in tc.c when we fill the netlink msg we ignore the mas and also when >>> parsing tc dump, >>> tun mask is set to here (tc.c) and not netdev-tc-offloads.c >>> >> >> Hi Simon, >> >> any update? i remind i suggested v0 is the correct one. > > Thanks and sorry for the ongoing delays. > > I have added this (v2) to a travisci instance and plan to apply the patch > to master if that passes. Hi Simon, I agreed with you v2 has an issue and we should use v0. v2 will not pass id to tc if there is an id with partial mask which is incorrect. v0 will add rule with exact match. can we go with v0 ? Thanks, Roi > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fhorms2%2Fovs%2Fbuilds%2F491523655&data=02%7C01%7Croid%40mellanox.com%7C8eae4ff8d37947fcd5c208d69000eb6e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854730964289416&sdata=iWAaOzcJf%2B5L4ahKeHb2Hcfxp0LpNOQfto%2BMaohkskk%3D&reserved=0 > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id
On Sun, Feb 10, 2019 at 08:04:39AM +, Roi Dayan wrote: > > > On 04/02/2019 15:20, Roi Dayan wrote: > > Hi Simon, > > > > I did some checks and think the correct fix is to offload exact match. > > if key is partial we can ignore the mask and offload exact match and > > it will be correct as we do more strict matching. > > > > it is also documented that the kernel datapath is doing the same > > (from datapath.rst) > > > > "The kernel can ignore the mask attribute, installing an exact > > match flow" > > > > So I think the first patch V0 is actually correct as we > > check the tunnel key flag exists and offload exact match if > > there was any mask or offload without a key if the mask is 0 > > or no key. > > > > in netdev-tc-offloads.c > > > > +flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > > + tnl_mask->tun_id : 0; > > > I think this is fine so long as tun_id is all-ones. Is that always the > case? > Should the code check that it is the case? Am I missing the point? > > >>> it looks like tun_id mask is always set to all-ones. > >>> but even if it won't be in the future, we shouldn't really care here. > >>> tc adds exact match on the tun_id and ignores the tun_id mask. > >>> this is considered ok as the matching is more strict. > >>> if new match is needed with different tun_id then ovs will try to add > >>> another rule for it. > >>> so with tc we could have multiple rules vs 1 rule that support mask. > >> > >> Thanks for looking into this. That sounds find to me but I wonder if we > >> should make > >> this behaviour explicit. > >> > >> /* > >> * Comment describing why the mask is 0 or all-ones > >> */ > >> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX > >> : 0; > >> > > I think its nicer like this and symetric currently. here it's generic > > and "use" mask. > > in tc.c when we fill the netlink msg we ignore the mas and also when > > parsing tc dump, > > tun mask is set to here (tc.c) and not netdev-tc-offloads.c > > > > Hi Simon, > > any update? i remind i suggested v0 is the correct one. Thanks and sorry for the ongoing delays. I have added this (v2) to a travisci instance and plan to apply the patch to master if that passes. https://travis-ci.org/horms2/ovs/builds/491523655 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 答复: [PATCH] netdev-dpdk: shrink critical region under tx_q[qid].tx_lock
> -邮件原件-
> 发件人: Ian Stokes [mailto:ian.sto...@intel.com]
> 发送时间: 2019年2月6日 23:34
> 收件人: Li,Rongqing ; d...@openvswitch.org
> 主题: Re: [ovs-dev] [PATCH] netdev-dpdk: shrink critical region under
> tx_q[qid].tx_lock
>
> On 1/31/2019 2:47 AM, Li RongQing wrote:
> > netdev_dpdk_filter_packet_len() does not need to be protected by
> > tx_q[].tx_lock, and tx_q[].tx_lock can not protect it too, same to
> > netdev_dpdk_qos_run
> >
> > so move them out of this lock to improve the scalability
> >
>
> Thanks for the Patch Li, can you give more details by what you mean in terms
> of
> scalability? The changes are small beow so I'm curious to as to the usecase
> you
> have where your seeing an improvement?
>
means to increase parallel
I did not have performance data, but it is strange from code review, critical
region
Have some codes which is not need to protect by the same lock
thanks
-RongQing
> > Signed-off-by: Li RongQing
> > ---
> > lib/netdev-dpdk.c | 33 ++---
> > 1 file changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9e8..bf4918e2c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2333,15 +2333,15 @@ __netdev_dpdk_vhost_send(struct netdev
> *netdev, int qid,
> > goto out;
> > }
> >
> > -rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > -
> > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
> > /* Check has QoS has been configured for the netdev */
> > cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);
> > dropped = total_pkts - cnt;
> >
> > +rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +
> > +int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> > do {
> > -int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> > unsigned int tx_pkts;
> >
> > tx_pkts = rte_vhost_enqueue_burst(vid, vhost_qid, cur_pkts,
> > cnt); @@ -2462,15 +2462,20 @@ netdev_dpdk_send__(struct netdev_dpdk
> *dev, int qid,
> > return;
> > }
> >
> > -if (OVS_UNLIKELY(concurrent_txq)) {
> > -qid = qid % dev->up.n_txq;
> > -rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > -}
> > -
> > if (OVS_UNLIKELY(batch->packets[0]->source != DPBUF_DPDK)) {
> > struct netdev *netdev = &dev->up;
> >
>
> The change below introduces code duplication in both the if and else
> statements specifically for the unlikely case. I'm slow to introduce this
> change
> as it seems the key benefit is in the case where concurrent txqs are used
> which
> to date has not been the common use case for the wider community. I take it
> here this case is the beneficiary?
>
> Ian
>
> > +if (OVS_UNLIKELY(concurrent_txq)) {
> > +qid = qid % dev->up.n_txq;
> > +rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +}
> > +
> > dpdk_do_tx_copy(netdev, qid, batch);
> > +
> > +if (OVS_UNLIKELY(concurrent_txq)) {
> > +rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > +}
> > +
> > dp_packet_delete_batch(batch, true);
> > } else {
> > int tx_cnt, dropped;
> > @@ -2481,8 +2486,17 @@ netdev_dpdk_send__(struct netdev_dpdk *dev,
> int qid,
> > tx_cnt = netdev_dpdk_qos_run(dev, pkts, tx_cnt, true);
> > dropped = batch_cnt - tx_cnt;
> >
> > +if (OVS_UNLIKELY(concurrent_txq)) {
> > +qid = qid % dev->up.n_txq;
> > +rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +}
> > +
> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, tx_cnt);
> >
> > +if (OVS_UNLIKELY(concurrent_txq)) {
> > +rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > +}
> > +
> > if (OVS_UNLIKELY(dropped)) {
> > rte_spinlock_lock(&dev->stats_lock);
> > dev->stats.tx_dropped += dropped;
> > @@ -2490,9 +2504,6 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int
> qid,
> > }
> > }
> >
> > -if (OVS_UNLIKELY(concurrent_txq)) {
> > -rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > -}
> > }
> >
> > static int
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
