[ovs-dev] [PATCH] dpif-netdev: Fix a race condition in deletion of offloaded flows
In dp_netdev_pmd_remove_flow() we schedule the deletion of an
offloaded flow, if a mark has been assigned to the flow. But if
this occurs in the window in which the offload thread completes
offloading the flow and assigns a mark to the flow, then we miss
deleting the flow. This problem has been observed while adding
and deleting flows in a loop. To fix this, always enqueue flow
deletion regardless of the flow->mark being set.
Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
Signed-off-by: Sriharsha Basavapatna
---
lib/dpif-netdev.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index e28e0b554..22c5f19a8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread
*pmd,
dp_netdev_simple_match_remove(pmd, flow);
cmap_remove(&pmd->flow_table, node, dp_netdev_flow_hash(&flow->ufid));
ccmap_dec(&pmd->n_flows, odp_to_u32(in_port));
-if (flow->mark != INVALID_FLOW_MARK) {
-queue_netdev_flow_del(pmd, flow);
-}
+queue_netdev_flow_del(pmd, flow);
flow->dead = true;
dp_netdev_flow_unref(flow);
--
2.30.0.349.g30b29f044a
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Copy external_ids from Logical_Router_Port to SB database
References: <20220127062350.177574-1-selvara...@nutanix.com> Bleep bloop. Greetings Selvaraj Palaniyappan, 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: error: sha1 information is lacking or useless (northd/northd.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 Copy external_ids from Logical_Router_Port to SB database When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] Copy external_ids from Logical_Router_Port to SB database
This patch makes ovn-northd copy all string-string pairs in external_ids column of the Logical_Router_Port table in Northbound database to the equivalent column of the Port_Binding table in Southbound database. --- northd/northd.c | 1 + ovn-nb.xml | 6 ++ ovn-sb.xml | 3 ++- tests/ovn-northd.at | 14 ++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/northd/northd.c b/northd/northd.c index fc7a64f99..090922ae2 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3240,6 +3240,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, ds_destroy(&s); struct smap ids = SMAP_INITIALIZER(&ids); +smap_clone(&ids, &op->nbrp->external_ids); sbrec_port_binding_set_external_ids(op->sb, &ids); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); diff --git a/ovn-nb.xml b/ovn-nb.xml index 6a6972856..293d25b32 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2895,6 +2895,12 @@ See External IDs at the beginning of this document. + + The ovn-northd program copies all these pairs into the + column of the + table in + database. + diff --git a/ovn-sb.xml b/ovn-sb.xml index 9ddacdf09..f7c41ccdc 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -3354,7 +3354,8 @@ tcp.flags = RST; The ovn-northd program populates this column with all entries into the column of the - table of the + and + tables of the database. diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 84e52e701..f9c5259f1 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -144,6 +144,20 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([check external id propagation to SBDB]) +ovn_start + +ovn-nbctl lr-add ro +ovn-nbctl lrp-add ro lrp0 00:00:00:00:00:01 192.168.1.1/24 +ovn-nbctl --wait=sb set logical_router_port lrp0 external_ids=test=123 +AT_CHECK([ovn-sbctl --columns=external_ids --bare find Port_Binding logical_port=lrp0], +[0], [test=123 +]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([check IPv6 RA config propagation to SBDB]) ovn_start -- 2.22.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] acinclude: Detect avx512 vpopcntdq compiler support.
Ubuntu Xenial 16.04 is using GCC 5.4 and it does not support
target "-mavx512vpopcntdq" and cuases error
lib/dpif-netdev-lookup-avx512-gather.c:356:47:
error: attribute(target("avx512vpopcntdq")) is unknown
GCC 7+ supports vpopcntdq:
https://gcc.gnu.org/gcc-7/changes.html
The patch detects vpopcntdq and disables AVX512 when not found.
Reported-by: Greg Rose
Signed-off-by: William Tu
---
acinclude.m4 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 5c971e98ce91..0c360fd1ef73 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -77,7 +77,7 @@ dnl Checks if compiler and binutils supports AVX512.
AC_DEFUN([OVS_CHECK_AVX512], [
OVS_CHECK_BINUTILS_AVX512
OVS_CHECK_CC_OPTION(
-[-mavx512f], [ovs_have_cc_mavx512f=yes], [ovs_have_cc_mavx512f=no])
+[-mavx512f -mavx512vpopcntdq], [ovs_have_cc_mavx512f=yes],
[ovs_have_cc_mavx512f=no])
AM_CONDITIONAL([HAVE_AVX512F], [test $ovs_have_cc_mavx512f = yes])
if test "$ovs_have_cc_mavx512f" = yes; then
AC_DEFINE([HAVE_AVX512F], [1],
--
2.30.1 (Apple Git-130)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH RFC 4/5] conntrack: Split single cmap to multiple buckets.
Hi Paolo,
On Mon, Nov 29, 2021, at 19:06, Paolo Valerio wrote:
> The purpose of this commit is to split the current way of storing the
> conn nodes. Before this patch the nodes were stored into a single cmap
> using ct->lock to avoid concurrent write access.
> With this commit a single connection can be stored into one or two (at
> most) CONNTRACK_BUCKETS available based on the outcome of the function
> hash_scale() on the key.
I think this approach overall works.
Removing the expiration list altogether might be worth it, but it would be nice
to have data to back it up. It simplifies expiration update and removes
another shared data structure, which is nice. However to accept the change
we must ensure that the ct sweep is within acceptable bounds.
How does the two solution compares regarding:
* expiration latency (delta between expected expiration time vs. actual).
* Memory consumption: depending on the structure, it's one additional node
per conn.
For a list, that represents 2x64 bits.
Not that much, but that's a benefit of removing the exp list.
* CPU utilization / average cycles per conn: how heavy is it on the CPU to
iterate a CMAP vs. a linked-list?
CMAPs are heavy in coherency traffic due to constant atomic reads, vs. a
list
that is mostly pointer dereference.
Maybe a micro-benchmark in tests/test-conntrack.c for the aging process could
help
measure those metrics isolated from the rest of OVS.
That seems quite a bit of additional work, maybe you would be able to provide
relevant
comparison without writing this additional benchmark.
On the other hand, maybe it only consists in inserting N conns, artificially
advancing
the internal OVS clock by the timeout and measuring the behavior. I'm not sure.
I measured the time to insert 1M connections with your patchset, my ct-scale
series and
the current baseline:
n-thread\version baseline ct-scale ct-bucket
1831 ms737 ms715 ms
2 1177 ms882 ms867 ms
3 1524 ms 1000 ms 1012 ms
4 2595 ms 1128 ms 1139 ms
So overall at least on insertion in conntrack_execute, it seem your solution is
pretty much on par with the series I proposed, and both are improvements
compared to current baseline.
Scaling is not yet linear though, and the mutex locking still appears in
profiling with
multiple threads.
> Every bucket has its local lock that needs to be acquired every time a
> node has to be removed/inserted from/to the cmap.
> This means that, in case the hash of the CT_DIR_FWD key differs from
> the one of the CT_DIR_REV, we can end up having the reference of the
> two key nodes in different buckets, and consequently acquiring two locks
> (one per bucket).
> This approach may be handy in different ways, depending on the way the
> stale connection removal gets designed. The attempt of this patch is
> to remove the expiration lists, removing the stale entries mostly in
> two ways:
>
> - during the key lookup
> - when the sweeper task wakes up
>
> the first case is not very strict, as we remove only expired entries
> with the same hash. To increase its effectiveness, we should probably
> increase the number of buckets and replace the cmaps with other data
> structures like rcu lists.
Did you try several number of buckets?
I see 2^10 being used now, it seems a bit large to me. Does it provide tangible
benefits? Did you measure contention vs. 512, or 256 buckets?
Replacing a CMAP by N rculists seems to be re-implementing a sharded CMAP with
one lock per internal bucket in a sense. However the bucket array would be
static
and would now resize with the CMAP.
It would result in a poorly implemented hash-map with integrated
locks and static number of buckets. It should either be possible to find a
solution
with existing structure, or a new structure should be instead cleanly designed.
> The sweeper task instead takes charge of the remaining stale entries
> removal. The heuristics used in the sweeper task are mostly an
> example, but could be modified to match any possible uncovered use
> case.
Which metric did you use to choose the new heuristics for the cleanup?
Otherwise I have a few remarks or suggestions below.
>
> Signed-off-by: Paolo Valerio
> ---
> The cover letter includes further details.
> ---
> lib/conntrack-private.h | 34 +++
> lib/conntrack-tp.c | 42
> lib/conntrack.c | 461
> +++
> tests/system-traffic.at |5 -
> 4 files changed, 331 insertions(+), 211 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index ea5ba3d9e..a89ff96fa 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -95,6 +95,7 @@ struct alg_exp_node {
>
> struct conn_key_node {
> struct conn_key key;
> +uint32_t key_hash;
Caching the key hash is necessary, for quick lookup and deletion
in the cmap.
Unfortunately, as yo
Re: [ovs-dev] [PATCH] daemon-unix: Close log file in monitor process while waiting on child.
On Wed, Jan 26, 2022 at 8:56 AM Ben Pfaff wrote:
>
> Until now, the monitor process held its log file open while it waited for
> the child to exit, and then it reopened it after the child exited. Holding
> the log file open this way prevented log rotation from freeing disk space.
> This commit avoids the problem by closing the log file before waiting, then
> reopening it afterward.
>
> Signed-off-by: Ben Pfaff
> Reported-by: Antonin Bas
> VMware-BZ: #2743409
> ---
Hi Ben,
Thanks, ASAN reports a memory leak,
2022-01-27T02:17:41.3146557Z 1788: ovsdb-server/add-db with --monitor
FAILED (ovs-macros.at:217)
2022-01-27T02:17:41.3467168Z 1789: ovsdb-server/add-db and remove-db
with --monitor FAILED (ovs-macros.at:217)
==29645==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 99 byte(s) in 1 object(s) allocated from:
#0 0x49633d in malloc
(/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/ovsdb/ovsdb-server+0x49633d)
#1 0x56df34 in xmalloc__
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/util.c:137:15
#2 0x56e098 in xmemdup0
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/util.c:193:15
#3 0x576800 in vlog_get_log_file
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/vlog.c:352:16
#4 0x57a80e in monitor_daemon
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/daemon-unix.c:363:30
#5 0x57a05a in daemonize_start
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/daemon-unix.c:481:13
#6 0x4c5e66 in main
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ovsdb/ovsdb-server.c:356:5
#7 0x7f0effa20bf6 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
SUMMARY: AddressSanitizer: 99 byte(s) leaked in 1 allocation(s).
I think we should "free(log_file)" after vlog_set_log_file(log_file)?
William
> include/openvswitch/vlog.h | 2 +
> lib/daemon-unix.c | 4 +-
> lib/vlog.c | 93 ++
> 3 files changed, 70 insertions(+), 29 deletions(-)
>
> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
> index 886fce5e0..e53ce6d81 100644
> --- a/include/openvswitch/vlog.h
> +++ b/include/openvswitch/vlog.h
> @@ -131,7 +131,9 @@ void vlog_set_verbosity(const char *arg);
>
> /* Configuring log destinations. */
> void vlog_set_pattern(enum vlog_destination, const char *pattern);
> +char *vlog_get_log_file(void);
> int vlog_set_log_file(const char *file_name);
> +void vlog_close_log_file(void);
> int vlog_reopen_log_file(void);
> #ifndef _WIN32
> void vlog_change_owner_unix(uid_t, gid_t);
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index 34d45b82a..10806bf81 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -360,12 +360,14 @@ monitor_daemon(pid_t daemon_pid)
> (unsigned long int) daemon_pid, status_msg);
>
> if (child_ready) {
> +char *log_file = vlog_get_log_file();
> +vlog_close_log_file();
> int error;
> do {
> retval = waitpid(daemon_pid, &status, 0);
> error = retval == -1 ? errno : 0;
> } while (error == EINTR);
> -vlog_reopen_log_file();
> +vlog_set_log_file(log_file);
> if (error) {
> VLOG_FATAL("waitpid failed (%s)", ovs_strerror(error));
> }
> diff --git a/lib/vlog.c b/lib/vlog.c
> index 533f93755..0a615bb66 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -343,13 +343,25 @@ vlog_set_pattern(enum vlog_destination destination,
> const char *pattern)
> }
> }
>
> -/* Sets the name of the log file used by VLF_FILE to 'file_name', or to the
> - * default file name if 'file_name' is null. Returns 0 if successful,
> - * otherwise a positive errno value. */
> -int
> -vlog_set_log_file(const char *file_name)
> +/* Returns a copy of the name of the log file used by VLF_FILE, or NULL if
> none
> + * is configured. The caller must eventually free the returned string. */
> +char *
> +vlog_get_log_file(void)
> +{
> +ovs_mutex_lock(&log_file_mutex);
> +char *fn = nullable_xstrdup(log_file_name);
> +ovs_mutex_unlock(&log_file_mutex);
> +
> +return fn;
> +}
> +
> +/* Sets the name of the log file used by VLF_FILE to 'new_log_file_name', or
> + * closes the current log file if 'new_log_file_name' is NULL. Takes
> ownership
> + * of 'new_log_file_name'. Returns 0 if successful, otherwise a positive
> errno
> + * value. */
> +static int
> +vlog_set_log_file__(char *new_log_file_name)
> {
> -char *new_log_file_name;
> struct vlog_module *mp;
> struct stat old_stat;
> struct stat new_stat;
> @@ -358,25 +370,29 @@ vlog_set_log_file(const char *file_name)
> bool log_close;
>
> /* Open new log file. */
> -new_log_file_name = (file_name
> - ? xstrdup(file_name)
> -
Re: [ovs-dev] [PATCH] conntrack: Check TCP state while testing established connections pick up.
On Mon, Dec 27, 2021, at 14:26, Paolo Valerio wrote: > When testing if an established connection is picked up, it could be > useful to verify that the protocol state matches the expectation, that > is, it moves to ESTABLISHED, as there's a chance that code modifications > may break the TCP conn_update() in a way that it returns CT_UPDATE_VALID > without moving to the correct state leading to a false positive. > > Signed-off-by: Paolo Valerio > --- > tests/ofproto-dpif.at |6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 1660b0856..07cb1e4eb 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -10875,6 +10875,12 @@ dnl AT_CHECK([ovs-appctl netdev-dummy/receive > p2 '5054000a50540009080045 > AT_CHECK([ovs-appctl netdev-dummy/receive p1 > '505400095054000a08004528fc264000400628a50a0101020a01010100020001396bb35a8cadbdb4501a629b']) > AT_CHECK([ovs-appctl netdev-dummy/receive p1 > '505400095054000a08004529fc274000400628a30a0101020a01010100020001396bb35a8cadbdb45018000a58920a']) > AT_CHECK([ovs-appctl netdev-dummy/receive p2 > '5054000a5054000908004528f2c84000400632030a0101010a010102000100028cadbdb4396bb35b501a629a']) > + > +# dnl Check that the protocol state moved to established after the > pickup > +AT_CHECK([ovs-appctl dpctl/dump-conntrack], [0], [dnl > +tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),protoinfo=(state=ESTABLISHED) > +]) > + > AT_CHECK([ovs-appctl netdev-dummy/receive p1 > '505400095054000a08004500022afc284000400626a10a0101020a01010100020001396bb35b8cadbdb45018000a941f > > dnl > > > > dnl > > > > dnl > In the context of this test, and considering the current coverage, I think this makes sense. I think however that if we want to edit the TCP state machine, we ought to implement unit-tests specifically for that, to explore the full state machine and verify that each possible combination of input results in the expected state. So if that happens, this current patch would ossify the test suite in a secondary place. I don't think it's a big issue, it just means that if such more advanced testing was done, there is potential for additional diff noise. This is not blocking as far as I'm concerned. So in my opinion, Acked-by: Gaetan Rivet -- Gaetan Rivet ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Avoid sporadic failures during IPv6 HTTP/FTP tests
On Tue, Jan 25, 2022, at 17:10, Mike Pattrick wrote: > On Mon, Dec 27, 2021 at 8:27 AM Paolo Valerio wrote: >> >> Some sporadic false positive may be visible for the following tests: >> >> - conntrack - IPv6 HTTP >> - conntrack - FTP over IPv6 >> >> The failures show up randomly. >> The reason appears to be source address used when performing the >> request using wget: >> -tcp,orig=(src=fc00::1,dst=fc00::2,sport=,dport=),reply=(src=fc00::2,dst=fc00::1,sport=,dport=),protoinfo=(state=) >> +tcp,orig=(src=fe80::f0eb:f8ff:fef0:138f,dst=fc00::2,sport=,dport=),reply=(src=fc00::2,dst=fe80::f0eb:f8ff:fef0:138f,sport=,dport=),protoinfo=(state=) >> >> It seems that the problem can be addressed in multiple ways, but using >> "nodad" seems to be safe enough to fix the issue that now, after >> hundreds of attempts, is no longer present. >> >> Signed-off-by: Paolo Valerio >> --- > > I wasn't able to reproduce the sporadic failures after 100 iterations > of "conntrack - FTP over IPv6", but I still think this fix makes > sense, and shouldn't break anything. > > Acked-by: Mike Pattrick > Hi Paolo, I'm sorry, I don't understand the root cause of the failure. Seeing the src IP switched from fc00:: to fe80::, and how the nodad flag solves it, I am guessing that assigning fc00::1 on the veth failed due to this address already being in use in the LAN? How does it happen if all involved links are veths? Could it be that either other tests are running in parallel with veths having the same IP? Or maybe something else unrelated to OVS? Nodad might make the link creation work, but I'm worried that it could make the result unpredictable, i.e. making it less likely, and less easy to understand and debug in the rare remaining times it triggers. Best, -- Gaetan Rivet ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] Revert "odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP."
On 1/6/22 19:07, Aaron Conole wrote:
> This reverts commit c645550bb249 ("odp-util: Always report
> ODP_FIT_TOO_LITTLE for IGMP.")
>
> Always forcing a slow path action can result in some over-broad
> flows which swallow all traffic and force them to userspace, as reported
> in the thread at
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html
> and at
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html
>
> Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically.
> Additionally, remove the userspace wc mask to prevent revalidator from
> cycling flows.
>
> Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.")
> Signed-off-by: Aaron Conole
> Acked-by: Eelco Chaudron
> ---
> v1->v2: Add unit test for coverage of mcast traffic when not doing snooping
> v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the
> matching issue.
> v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split
> IGMP unit test into two different tests. Use a multicast eth addr for
> the FLOOD under NORMAL action case.
>
> lib/odp-util.c | 5 ---
> ofproto/ofproto-dpif-xlate.c | 1 -
> tests/mcast-snooping.at | 68 ++
> tests/ofproto-macros.at | 15 +++
> tests/system-traffic.at | 80
> 5 files changed, 163 insertions(+), 6 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index fbdfc7ad83..0f21ffe639 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr
> *attrs[OVS_KEY_ATTR_MAX + 1],
> }
> }
> }
> -} else if (src_flow->nw_proto == IPPROTO_IGMP
> - && src_flow->dl_type == htons(ETH_TYPE_IP)) {
> -/* OVS userspace parses the IGMP type, code, and group, but its
> - * datapaths do not, so there is always missing information. */
> -return ODP_FIT_TOO_LITTLE;
> }
> if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) {
> if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) {
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index cafcd014ad..29d26cf9ac 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx)
> */
> ctx->xout->slow |= SLOW_ACTION;
>
> -memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
> if (mcast_snooping_is_membership(flow->tp_src) ||
> mcast_snooping_is_query(flow->tp_src)) {
> if (ctx->xin->allow_side_effects && ctx->xin->packet) {
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index 757cf7186e..e24f293a0c 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -216,3 +216,70 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
> ])
>
> AT_CLEANUP
> +
> +
> +AT_SETUP([mcast - igmp flood for non-snoop enabled])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +ovs-vsctl set bridge br0 \
> +datapath_type=dummy], [0])
> +
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +ovs-appctl time/stop
> +
> +dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP
> +dnl in reverse direction
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> +
> '0101000c29a0aa55aa550001080046c000284102d3494565eb4ae01694042200f90200010400e0fb'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \
> +
> 'aa55aa5500010101000c29a00800451c0001400164dc0a0101010a0101020800f7ff'])
> +
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl
> + strip_stats | strip_used | strip_recirc | dnl
> + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
> + [0], [dnl
> +recirc_id(),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no),
> packets:0, bytes:0, used:never, actions:100,2
> +recirc_id(),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no),
> packets:0, bytes:0, used:never, actions:1
> +])
> +
> +ovs-appctl time/warp 10
> +
> +dnl Next we should clear the flows and install a complex case
> +AT_CHECK([ovs-ofctl del-flows br0])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, arp actions=NORMAL
> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000)
> +table=0, in_port=2 actions=output:1
> +table=1, ip,ct_state=+trk+inv actions=drop
> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2
> +table=1, in_port=1,ip,ct_state=+trk+new
> actions=controller(userdata=00.de.ad.be.ef.ca.fe.01)
> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +ovs-appctl time/warp 10
>
[ovs-dev] [PATCH] daemon-unix: Close log file in monitor process while waiting on child.
Until now, the monitor process held its log file open while it waited for
the child to exit, and then it reopened it after the child exited. Holding
the log file open this way prevented log rotation from freeing disk space.
This commit avoids the problem by closing the log file before waiting, then
reopening it afterward.
Signed-off-by: Ben Pfaff
Reported-by: Antonin Bas
VMware-BZ: #2743409
---
include/openvswitch/vlog.h | 2 +
lib/daemon-unix.c | 4 +-
lib/vlog.c | 93 ++
3 files changed, 70 insertions(+), 29 deletions(-)
diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
index 886fce5e0..e53ce6d81 100644
--- a/include/openvswitch/vlog.h
+++ b/include/openvswitch/vlog.h
@@ -131,7 +131,9 @@ void vlog_set_verbosity(const char *arg);
/* Configuring log destinations. */
void vlog_set_pattern(enum vlog_destination, const char *pattern);
+char *vlog_get_log_file(void);
int vlog_set_log_file(const char *file_name);
+void vlog_close_log_file(void);
int vlog_reopen_log_file(void);
#ifndef _WIN32
void vlog_change_owner_unix(uid_t, gid_t);
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index 34d45b82a..10806bf81 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -360,12 +360,14 @@ monitor_daemon(pid_t daemon_pid)
(unsigned long int) daemon_pid, status_msg);
if (child_ready) {
+char *log_file = vlog_get_log_file();
+vlog_close_log_file();
int error;
do {
retval = waitpid(daemon_pid, &status, 0);
error = retval == -1 ? errno : 0;
} while (error == EINTR);
-vlog_reopen_log_file();
+vlog_set_log_file(log_file);
if (error) {
VLOG_FATAL("waitpid failed (%s)", ovs_strerror(error));
}
diff --git a/lib/vlog.c b/lib/vlog.c
index 533f93755..0a615bb66 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -343,13 +343,25 @@ vlog_set_pattern(enum vlog_destination destination, const
char *pattern)
}
}
-/* Sets the name of the log file used by VLF_FILE to 'file_name', or to the
- * default file name if 'file_name' is null. Returns 0 if successful,
- * otherwise a positive errno value. */
-int
-vlog_set_log_file(const char *file_name)
+/* Returns a copy of the name of the log file used by VLF_FILE, or NULL if none
+ * is configured. The caller must eventually free the returned string. */
+char *
+vlog_get_log_file(void)
+{
+ovs_mutex_lock(&log_file_mutex);
+char *fn = nullable_xstrdup(log_file_name);
+ovs_mutex_unlock(&log_file_mutex);
+
+return fn;
+}
+
+/* Sets the name of the log file used by VLF_FILE to 'new_log_file_name', or
+ * closes the current log file if 'new_log_file_name' is NULL. Takes ownership
+ * of 'new_log_file_name'. Returns 0 if successful, otherwise a positive errno
+ * value. */
+static int
+vlog_set_log_file__(char *new_log_file_name)
{
-char *new_log_file_name;
struct vlog_module *mp;
struct stat old_stat;
struct stat new_stat;
@@ -358,25 +370,29 @@ vlog_set_log_file(const char *file_name)
bool log_close;
/* Open new log file. */
-new_log_file_name = (file_name
- ? xstrdup(file_name)
- : xasprintf("%s/%s.log", ovs_logdir(), program_name));
-new_log_fd = open(new_log_file_name, O_WRONLY | O_CREAT | O_APPEND, 0660);
-if (new_log_fd < 0) {
-VLOG_WARN("failed to open %s for logging: %s",
- new_log_file_name, ovs_strerror(errno));
-free(new_log_file_name);
-return errno;
+if (new_log_file_name) {
+new_log_fd = open(new_log_file_name, O_WRONLY | O_CREAT | O_APPEND,
+ 0660);
+if (new_log_fd < 0) {
+VLOG_WARN("failed to open %s for logging: %s",
+ new_log_file_name, ovs_strerror(errno));
+free(new_log_file_name);
+return errno;
+}
+} else {
+new_log_fd = -1;
}
/* If the new log file is the same one we already have open, bail out. */
ovs_mutex_lock(&log_file_mutex);
-same_file = (log_fd >= 0
- && new_log_fd >= 0
- && !fstat(log_fd, &old_stat)
- && !fstat(new_log_fd, &new_stat)
- && old_stat.st_dev == new_stat.st_dev
- && old_stat.st_ino == new_stat.st_ino);
+same_file = ((log_fd < 0
+ && new_log_fd < 0) ||
+ (log_fd >= 0
+ && new_log_fd >= 0
+ && !fstat(log_fd, &old_stat)
+ && !fstat(new_log_fd, &new_stat)
+ && old_stat.st_dev == new_stat.st_dev
+ && old_stat.st_ino == new_stat.st_ino));
ovs_mutex_unlock(&log_file_mutex);
if (same_file) {
close(new_log_fd);
@@ -392,19 +408,18 @@ vlog_set_log_file(const c
Re: [ovs-dev] [PATCH ovn] northd: Remove potential duplicates in SB Load_Balancer table.
I went ahead and pushed this to main and branch-21.12 due to the
simplicity of the patch.
On 1/26/22 09:33, Mark Michelson wrote:
Thanks Dumitru,
Acked-by: Mark Michelson
On 1/26/22 09:24, Dumitru Ceara wrote:
The Southbound Load_Balancer table (like the Northbound one) doesn't
define an index (e.g., by name). This essentially means that there can
be duplicate records in the database. Moreover, the OVSDB RAFT
implementation ensures "at-least-once" consistency [0] making it
possible for such duplicates to "spontaneously" appear.
ovn-northd now cleans up such duplicates.
[0]
https://github.com/openvswitch/ovs/blob/master/Documentation/ref/ovsdb.7.rst#understanding-cluster-consistency
Fixes: 42e694f03c18 ("Add new table Load_Balancer in Southbound
database.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2046274
Signed-off-by: Dumitru Ceara
---
northd/northd.c | 16 +---
tests/ovn-northd.at | 17 +
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index c0ecf2346..fc7a64f99 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3803,6 +3803,7 @@ build_ovn_lbs(struct northd_input *input_data,
}
/* Delete any stale SB load balancer rows. */
+ struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs);
const struct sbrec_load_balancer *sbrec_lb, *next;
SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next,
input_data->sbrec_load_balancer_table) {
@@ -3813,13 +3814,22 @@ build_ovn_lbs(struct northd_input *input_data,
continue;
}
+ /* Delete any SB load balancer entries that refer to NB load
balancers
+ * that don't exist anymore or are not applied to switches
anymore.
+ *
+ * There is also a special case in which duplicate LBs might
be created
+ * in the SB, e.g., due to the fact that OVSDB only ensures
+ * "at-least-once" consistency for clustered database tables
that
+ * are not indexed in any way.
+ */
lb = ovn_northd_lb_find(lbs, &lb_uuid);
- if (lb && lb->n_nb_ls) {
- lb->slb = sbrec_lb;
- } else {
+ if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
sbrec_load_balancer_delete(sbrec_lb);
+ } else {
+ lb->slb = sbrec_lb;
}
}
+ hmapx_destroy(&existing_lbs);
/* Create SB Load balancer records if not present and sync
* the SB load balancer columns. */
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 652903761..84e52e701 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5336,6 +5336,23 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
's/table=./table=?/' | sort], [0], [
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([Load Balancer SB duplicates])
+ovn_start
+
+check ovn-nbctl ls-add ls -- lb-add lb1 10.0.0.1:80 10.0.0.2:80 --
ls-lb-add ls lb1
+check ovn-nbctl --wait=sb sync
+
+dps=$(fetch_column Load_Balancer datapaths)
+nlb=$(fetch_column nb:Load_Balancer _uuid)
+AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 datapaths="$dps"
external_ids="lb_id=$nlb"], [0], [ignore])
+
+check ovn-nbctl --wait=sb sync
+check_row_count Load_Balancer 1
+
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn -- Add tags to logical flows])
ovn_start
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Remove potential duplicates in SB Load_Balancer table.
Thanks Dumitru,
Acked-by: Mark Michelson
On 1/26/22 09:24, Dumitru Ceara wrote:
The Southbound Load_Balancer table (like the Northbound one) doesn't
define an index (e.g., by name). This essentially means that there can
be duplicate records in the database. Moreover, the OVSDB RAFT
implementation ensures "at-least-once" consistency [0] making it
possible for such duplicates to "spontaneously" appear.
ovn-northd now cleans up such duplicates.
[0]
https://github.com/openvswitch/ovs/blob/master/Documentation/ref/ovsdb.7.rst#understanding-cluster-consistency
Fixes: 42e694f03c18 ("Add new table Load_Balancer in Southbound database.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2046274
Signed-off-by: Dumitru Ceara
---
northd/northd.c | 16 +---
tests/ovn-northd.at | 17 +
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index c0ecf2346..fc7a64f99 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3803,6 +3803,7 @@ build_ovn_lbs(struct northd_input *input_data,
}
/* Delete any stale SB load balancer rows. */
+struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs);
const struct sbrec_load_balancer *sbrec_lb, *next;
SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next,
input_data->sbrec_load_balancer_table) {
@@ -3813,13 +3814,22 @@ build_ovn_lbs(struct northd_input *input_data,
continue;
}
+/* Delete any SB load balancer entries that refer to NB load balancers
+ * that don't exist anymore or are not applied to switches anymore.
+ *
+ * There is also a special case in which duplicate LBs might be created
+ * in the SB, e.g., due to the fact that OVSDB only ensures
+ * "at-least-once" consistency for clustered database tables that
+ * are not indexed in any way.
+ */
lb = ovn_northd_lb_find(lbs, &lb_uuid);
-if (lb && lb->n_nb_ls) {
-lb->slb = sbrec_lb;
-} else {
+if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
sbrec_load_balancer_delete(sbrec_lb);
+} else {
+lb->slb = sbrec_lb;
}
}
+hmapx_destroy(&existing_lbs);
/* Create SB Load balancer records if not present and sync
* the SB load balancer columns. */
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 652903761..84e52e701 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5336,6 +5336,23 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
's/table=./table=?/' | sort], [0], [
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([Load Balancer SB duplicates])
+ovn_start
+
+check ovn-nbctl ls-add ls -- lb-add lb1 10.0.0.1:80 10.0.0.2:80 -- ls-lb-add
ls lb1
+check ovn-nbctl --wait=sb sync
+
+dps=$(fetch_column Load_Balancer datapaths)
+nlb=$(fetch_column nb:Load_Balancer _uuid)
+AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 datapaths="$dps"
external_ids="lb_id=$nlb"], [0], [ignore])
+
+check ovn-nbctl --wait=sb sync
+check_row_count Load_Balancer 1
+
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn -- Add tags to logical flows])
ovn_start
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: Remove potential duplicates in SB Load_Balancer table.
The Southbound Load_Balancer table (like the Northbound one) doesn't
define an index (e.g., by name). This essentially means that there can
be duplicate records in the database. Moreover, the OVSDB RAFT
implementation ensures "at-least-once" consistency [0] making it
possible for such duplicates to "spontaneously" appear.
ovn-northd now cleans up such duplicates.
[0]
https://github.com/openvswitch/ovs/blob/master/Documentation/ref/ovsdb.7.rst#understanding-cluster-consistency
Fixes: 42e694f03c18 ("Add new table Load_Balancer in Southbound database.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2046274
Signed-off-by: Dumitru Ceara
---
northd/northd.c | 16 +---
tests/ovn-northd.at | 17 +
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index c0ecf2346..fc7a64f99 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3803,6 +3803,7 @@ build_ovn_lbs(struct northd_input *input_data,
}
/* Delete any stale SB load balancer rows. */
+struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs);
const struct sbrec_load_balancer *sbrec_lb, *next;
SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next,
input_data->sbrec_load_balancer_table) {
@@ -3813,13 +3814,22 @@ build_ovn_lbs(struct northd_input *input_data,
continue;
}
+/* Delete any SB load balancer entries that refer to NB load balancers
+ * that don't exist anymore or are not applied to switches anymore.
+ *
+ * There is also a special case in which duplicate LBs might be created
+ * in the SB, e.g., due to the fact that OVSDB only ensures
+ * "at-least-once" consistency for clustered database tables that
+ * are not indexed in any way.
+ */
lb = ovn_northd_lb_find(lbs, &lb_uuid);
-if (lb && lb->n_nb_ls) {
-lb->slb = sbrec_lb;
-} else {
+if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
sbrec_load_balancer_delete(sbrec_lb);
+} else {
+lb->slb = sbrec_lb;
}
}
+hmapx_destroy(&existing_lbs);
/* Create SB Load balancer records if not present and sync
* the SB load balancer columns. */
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 652903761..84e52e701 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5336,6 +5336,23 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
's/table=./table=?/' | sort], [0], [
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([Load Balancer SB duplicates])
+ovn_start
+
+check ovn-nbctl ls-add ls -- lb-add lb1 10.0.0.1:80 10.0.0.2:80 -- ls-lb-add
ls lb1
+check ovn-nbctl --wait=sb sync
+
+dps=$(fetch_column Load_Balancer datapaths)
+nlb=$(fetch_column nb:Load_Balancer _uuid)
+AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 datapaths="$dps"
external_ids="lb_id=$nlb"], [0], [ignore])
+
+check ovn-nbctl --wait=sb sync
+check_row_count Load_Balancer 1
+
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn -- Add tags to logical flows])
ovn_start
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] ovs-sandbox: Convert initial databases if needed.
The --nbdb-source and --sbdb-source options allow users to point the
sandbox daemons to already existing database files. It's useful if
these are first converted to use the currently supported schema.
Signed-off-by: Dumitru Ceara
---
tutorial/ovs-sandbox | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tutorial/ovs-sandbox b/tutorial/ovs-sandbox
index d679cd39b..d81e00496 100755
--- a/tutorial/ovs-sandbox
+++ b/tutorial/ovs-sandbox
@@ -542,7 +542,11 @@ ovn_start_db() {
case $model in
standalone)
case $source_type in
-database) run cp "$source" ${db}1.db ;;
+database)
+run cp "$source" ${db}1.db
+run ovsdb-tool convert ${db}1.db \
+$srcdir/ovn-${db}.ovsschema
+;;
schema) run ovsdb-tool create ${db}1.db "$source" ;;
esac
ovn_start_ovsdb_server 1
@@ -551,7 +555,11 @@ ovn_start_db() {
backup)
for i in 1 2; do
case $source_type in
-database) run cp "$source" $db$i.db ;;
+database)
+run cp "$source" $db$i.db
+run ovsdb-tool convert $db$i.db \
+$srcdir/ovn-${db}.ovsschema
+;;
schema) run ovsdb-tool create $db$i.db "$source" ;;
esac
done
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 14/18] python: introduce unit tests
On 1/26/22 14:33, Adrian Moreno wrote: > > > On 1/26/22 13:43, Ilya Maximets wrote: >> On 1/26/22 13:34, Adrian Moreno wrote: >>> Hi, >>> >> diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 >> index 772825a71..192147ff5 100644 >> --- a/m4/openvswitch.m4 >> +++ b/m4/openvswitch.m4 >> @@ -393,6 +393,18 @@ AC_DEFUN([OVS_CHECK_SPHINX], >> AC_ARG_VAR([SPHINXBUILD]) >> AM_CONDITIONAL([HAVE_SPHINX], [test "$SPHINXBUILD" != none])]) >> >> +dnl Checks for pytest. >> +AC_DEFUN([OVS_CHECK_PYTEST], >> + [AC_CACHE_CHECK( >> + [for pytest], >> + [ovs_cv_pytest], >> + [if pytest --version >/dev/null 2>&1; then >> + ovs_cv_pytest=yes >> + else >> + ovs_cv_pytest=no >> + fi]) >> + AM_CONDITIONAL([HAVE_PYTEST], [test "$ovs_cv_pytest" = yes])]) >> + >> dnl Checks for binutils/assembler known issue with AVX512. >> dnl Due to backports, we probe assembling a reproducer instead of >> checking >> dnl binutils version string. More details, including ASM dumps and >> debug here: >>> >>> >>> Just FYI, In the new version, I'm changing this macro to check for all test >>> dependencies. >>> >> >> Is pytest a build dependency or a test dependency? >> If it's only for testing, the check should, probably, >> be part of the tests/atlocal.in, which is executed >> every time before starting the testsuite. >> > > Hi Ilya, that's a good question. It's for testing but since I was > mimicking the flake8 tests, I have included them in the ALL_LOCAL > target. But I think it'd better if they are run as part of "make check", > right? Yeah, flake8 is just a style check, while these are actual unit tests, so should belong to a testsuite, I think. > >> Best regards, Ilya Maximets. >> > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 14/18] python: introduce unit tests
On 1/26/22 13:43, Ilya Maximets wrote: On 1/26/22 13:34, Adrian Moreno wrote: Hi, diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 index 772825a71..192147ff5 100644 --- a/m4/openvswitch.m4 +++ b/m4/openvswitch.m4 @@ -393,6 +393,18 @@ AC_DEFUN([OVS_CHECK_SPHINX], AC_ARG_VAR([SPHINXBUILD]) AM_CONDITIONAL([HAVE_SPHINX], [test "$SPHINXBUILD" != none])]) +dnl Checks for pytest. +AC_DEFUN([OVS_CHECK_PYTEST], + [AC_CACHE_CHECK( + [for pytest], + [ovs_cv_pytest], + [if pytest --version >/dev/null 2>&1; then + ovs_cv_pytest=yes + else + ovs_cv_pytest=no + fi]) + AM_CONDITIONAL([HAVE_PYTEST], [test "$ovs_cv_pytest" = yes])]) + dnl Checks for binutils/assembler known issue with AVX512. dnl Due to backports, we probe assembling a reproducer instead of checking dnl binutils version string. More details, including ASM dumps and debug here: Just FYI, In the new version, I'm changing this macro to check for all test dependencies. Is pytest a build dependency or a test dependency? If it's only for testing, the check should, probably, be part of the tests/atlocal.in, which is executed every time before starting the testsuite. Hi Ilya, that's a good question. It's for testing but since I was mimicking the flake8 tests, I have included them in the ALL_LOCAL target. But I think it'd better if they are run as part of "make check", right? Best regards, Ilya Maximets. -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 14/18] python: introduce unit tests
On 1/26/22 13:34, Adrian Moreno wrote: > Hi, > diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 index 772825a71..192147ff5 100644 --- a/m4/openvswitch.m4 +++ b/m4/openvswitch.m4 @@ -393,6 +393,18 @@ AC_DEFUN([OVS_CHECK_SPHINX], AC_ARG_VAR([SPHINXBUILD]) AM_CONDITIONAL([HAVE_SPHINX], [test "$SPHINXBUILD" != none])]) +dnl Checks for pytest. +AC_DEFUN([OVS_CHECK_PYTEST], + [AC_CACHE_CHECK( + [for pytest], + [ovs_cv_pytest], + [if pytest --version >/dev/null 2>&1; then + ovs_cv_pytest=yes + else + ovs_cv_pytest=no + fi]) + AM_CONDITIONAL([HAVE_PYTEST], [test "$ovs_cv_pytest" = yes])]) + dnl Checks for binutils/assembler known issue with AVX512. dnl Due to backports, we probe assembling a reproducer instead of checking dnl binutils version string. More details, including ASM dumps and debug here: > > > Just FYI, In the new version, I'm changing this macro to check for all test > dependencies. > Is pytest a build dependency or a test dependency? If it's only for testing, the check should, probably, be part of the tests/atlocal.in, which is executed every time before starting the testsuite. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 14/18] python: introduce unit tests
Hi, diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 index 772825a71..192147ff5 100644 --- a/m4/openvswitch.m4 +++ b/m4/openvswitch.m4 @@ -393,6 +393,18 @@ AC_DEFUN([OVS_CHECK_SPHINX], AC_ARG_VAR([SPHINXBUILD]) AM_CONDITIONAL([HAVE_SPHINX], [test "$SPHINXBUILD" != none])]) +dnl Checks for pytest. +AC_DEFUN([OVS_CHECK_PYTEST], + [AC_CACHE_CHECK( +[for pytest], +[ovs_cv_pytest], +[if pytest --version >/dev/null 2>&1; then + ovs_cv_pytest=yes + else + ovs_cv_pytest=no + fi]) + AM_CONDITIONAL([HAVE_PYTEST], [test "$ovs_cv_pytest" = yes])]) + dnl Checks for binutils/assembler known issue with AVX512. dnl Due to backports, we probe assembling a reproducer instead of checking dnl binutils version string. More details, including ASM dumps and debug here: Just FYI, In the new version, I'm changing this macro to check for all test dependencies. -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 00/27] dpif-netdev: Parallel offload processing
On Wed, Jan 26, 2022 at 3:26 AM Ilya Maximets wrote:
>
> On 1/23/22 17:28, Sriharsha Basavapatna wrote:
> > Hi Ilya,
> >
> > On Wed, Jan 19, 2022 at 7:24 AM Ilya Maximets wrote:
> >
> >>
> >> I also spotted one bug where the flow stays offloaded if it gets added
> >> and removed shortly after that. But it's an old existing race condition,
> >> not introduced by this patch set. We basically have to enqueue the
> >> flow deletion regardless of the flow->mark being set.
> >> I'll send a patch for that issue, probably, in the next couple of days.
> >> Or if you want to work on that, that's fine for me too.
> >
> > We are seeing a similar issue with OVS-2.16. While adding and deleting
> > flows in a loop, after a few iterations, rte_flow_destroy() is not
> > seen by the PMD for some of the offloaded flows. And those flows stay
> > offloaded in HW. I tried the following change in 2.16 like you
> > suggested above and it resolved the issue.
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index d6bee2a5a..8cca57f1f 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2758,9 +2758,7 @@ dp_netdev_pmd_remove_flow(struct
> > dp_netdev_pmd_thread *pmd,
> > ovs_assert(cls != NULL);
> > dpcls_remove(cls, &flow->cr);
> > cmap_remove(&pmd->flow_table, node, dp_netdev_flow_hash(&flow->ufid));
> > -if (flow->mark != INVALID_FLOW_MARK) {
> > -queue_netdev_flow_del(pmd, flow);
> > -}
> > +queue_netdev_flow_del(pmd, flow);
> > flow->dead = true;
> >
> > dp_netdev_flow_unref(flow);
>
> Yeah, I was thinking about something similar. Would you mind sending an
> official patch?
>
> Best regards, Ilya Maximets.
Sure, I will send it.
Thanks,
-Harsha
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
