Re: [ovs-dev] [PATCH] python: Remove duplicate UnixctlClient implementation.
On 10/26/23 14:10, jm...@redhat.com wrote:
> From: Jakob Meng
>
> The unixctl implementation in Python has been split into three parts in
> the past. During this process the UnixctlClient was duplicated, in
> python/ovs/unixctl/client.py and python/ovs/unixctl/server.py. This
> patch removes the duplicate from the latter.
>
> Fixes: 53cf9963ccc6 ("python: Break unixctl implementation into re...")
Please, don't abbreviate tags, i.e. the Fixes tag should contain the full
commit name regardless of the length.
Otherwise, the change seem fine.
> Signed-off-by: Jakob Meng
> ---
> python/ovs/unixctl/server.py | 44
> 1 file changed, 44 deletions(-)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.
On 10/27/23 23:51, Ilya Maximets wrote:
> On 10/26/23 13:44, Jakob Meng wrote:
>> On 25.10.23 11:37, jm...@redhat.com wrote:
>>> From: Jakob Meng
>>>
>>> For monitoring systems such as Prometheus it would be beneficial if
>>> OVS and OVS-DPDK would expose statistics in a machine-readable format.
BTW, there is no such separate thing as OVS-DPDK, it's just OVS.
>>>
>>> This patch introduces support for different output formats to ovs-xxx
>>> tools. They gain a global option '-f,--format' which allows users to
>>> request JSON instead of plain-text for humans. An example call
>>> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>>>
>>> For that it is necessary to change the JSON-RPC API lib/unixctl.*
>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>> sockets. It now allows to transport the requested output format
>>> besides the command and its args. This change has been implemented in
>>> a backward compatible way. One can use an updated client
>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>> Of course, JSON output only works when both sides have been updated.
>>>
>>> Previously, the command was copied to the 'method' parameter in
>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>> any other means to transport parameters via JSON-RPC left, the meaning
>>> of 'method' and 'params' had to be changed: 'method' will now be
>>> 'execute/v1' when an output format other than 'text' is requested. In
>>> that case, the first parameter of the JSON array 'params' will now be
>>> the designated command, the second one the output format and the rest
>>> will be command args.
>>
>> Ilya brought up the question why I changed the meaning of 'method' and
>> 'params' instead of adding the output format as an addition argument to the
>> command arguments in 'params'. The server side would then interpret and
>> filter out this argument before passing the remaining arguments to the
>> command callbacks.
>>
>> I decided against this approach because the code would get more involved, in
>> particular we would have to implement option/argument parsing inside
>> process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a
>> safe way would be difficult in general because their global state.)
>> The current implementation is based purely on JSON objects/arrays which are
>> nicely supported by OVS with functions from lib/json.*.
>
> I'm not sure I got this point, but see below.
>
>>
>> Ilya also voiced concerns about the limited extensibility of the proposed
>> API. To fix this, this patch series could be tweaked in the follow way:
>>
>> (1.) Instead of passing the output format as second entry in the JSON array
>> 'params', turn the second entry into a JSON object (shash).
It has to be an array, not an object. Parameters are positional.
Unless you want to change API for every existing command and give
each argument a unique name.
And that will not help with supporting older servers, because they
expect an array.
>> The output format would be one entry in this JSON object, e.g. called
>> 'format'.
>>
>> The server (process_command() from lib/unixctl.c) will parse the content of
>> this JSON object into known options. Clients would only add non-default
>> options to this JSON object to keep compatibility with older servers.
>> Options which are supported by the server but not transferred via this JSON
>> object would be initialized with default values to keep compatibility with
>> older clients. Unknown entries would cause the server to return an error.
>
> This kind of implements what I had in mind, but in a slightly different
> manner. How about something like this:
>
> * ovs-appctl has a global parameter --format passed to it, not part of
>the requested command paramaters. (I think this is implemented in the
>code, below, but I didn't read very carefully.)
>
> * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
>i.e. separate element in the array to every command it executes, if
>provided.
>
> * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
>that argument, removes it from the list of parameters before passing
>to the actual handler.
>
> * Before calling a handler, server stores these special parameters
>into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
>really matter).
>
> * Handler always has the conn pointer. Handler can ask in which format
>the user prefers the output and generate one. E.g.:
>
> if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
> unixctl_command_reply_json(conn, ...);
> } else {
> unixctl_command_reply(conn, ...);
> }
>
>Notice that nothing bad happens if command is not aware of the formats,
>because unixctl_command_reply() is a JSON reply, but with a simple
>JSON string instead of any fancy s
Re: [ovs-dev] [PATCH v3] editorconfig: Remove [*] section and trim_trailing_whitespace.
Hi Jakob,
Just a comment about the use of 'whitespace' in the commit message.
There is an effort to try and use more inclusive language, so it might
be good to use empty-space or blank-space in the commit message. I know
that it is an editor config property, so not much to do about
trim_trailing_whitespaces in the message.
Additionally, for things like "contain trailing whitespaces" maybe we
can use "contain trailing blank characters?"
Thanks!
jm...@redhat.com writes:
> From: Jakob Meng
>
> Wildcard sections [*] and [**] are unsafe because properties cannot be
> applied safely to any filetype in general. For example, IDEs like
> Visual Studio Code and KDevelop store configuration files in subfolders
> like .vscode or .kdev4. Properties from wildcard sections also apply to
> those files which is not safe in general.
> Another example are patches created with 'git format-patch' which can
> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
> in the title, trailing whitespaces should not be removed.
>
> Property trim_trailing_whitespace should not be defined at all because
> it is interpreted differently by editors. Some wipe whitespaces from
> the whole file, others remove them from edited lines only and a few
> change their behavior between releases [0]. Limiting the property to a
> subset of files like *.c/*.h will not mitigate the issue:
>
> Multiple definitions of a whitespace exist. Unicode considers a form
> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
> follows this definition, causing the Kate editor identify a form feed
> as a trailing whitespace and removing it from sources [3]. This breaks
> patches when editors remove form feeds and thus causing broken patches
> which cannot be applied cleanly.
>
> Removing trim_trailing_whitespace will be a minor inconvienence, in
> particular because utilities/checkpatch.py and thus 0-day Robot will
> prevent trailing whitespaces for our definition of a whitespace.
Luckily, developers can install a hook that will run checkpatch on a
commit. Something like the below installed in .git/hooks/pre-commit
should run when trying to commit and catch trailing spaces errors.
--8<--
#!/bin/sh
if git rev-parse --verify HEAD 2>/dev/null
then
git diff-index -p --cached HEAD
else
:
fi | utilities/checkpatch.py -s -S
-->8--
> [0]
> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
> [1] https://en.wikipedia.org/wiki/Whitespace_character
> [2]
> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
> [3]
> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>
> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>
> Signed-off-by: Jakob Meng
> ---
> .editorconfig | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/.editorconfig b/.editorconfig
> index 685c72750..41ba51bf3 100644
> --- a/.editorconfig
> +++ b/.editorconfig
> @@ -2,15 +2,18 @@
>
> root = true
>
> -[*]
> -end_of_line = lf
> -insert_final_newline = true
> -trim_trailing_whitespace = true
> -charset = utf-8
> +# No wildcard sections [*] and [**] because properties cannot be
> +# applied safely to any filetype in general.
> +
> +# Property trim_trailing_whitespace should not be defined at all
> +# because it is interpreted differently by editors.
>
> [*.{c,h}]
> +charset = utf-8
> +end_of_line = lf
> indent_style = space
> indent_size = 4
> +insert_final_newline = true
> max_line_length = 79
>
> [include/linux/**.h]
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.
On 10/26/23 13:44, Jakob Meng wrote:
> On 25.10.23 11:37, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> For monitoring systems such as Prometheus it would be beneficial if
>> OVS and OVS-DPDK would expose statistics in a machine-readable format.
>>
>> This patch introduces support for different output formats to ovs-xxx
>> tools. They gain a global option '-f,--format' which allows users to
>> request JSON instead of plain-text for humans. An example call
>> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>>
>> For that it is necessary to change the JSON-RPC API lib/unixctl.*
>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>> sockets. It now allows to transport the requested output format
>> besides the command and its args. This change has been implemented in
>> a backward compatible way. One can use an updated client
>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>> Of course, JSON output only works when both sides have been updated.
>>
>> Previously, the command was copied to the 'method' parameter in
>> JSON-RPC while the args were copied to the 'params' parameter. Without
>> any other means to transport parameters via JSON-RPC left, the meaning
>> of 'method' and 'params' had to be changed: 'method' will now be
>> 'execute/v1' when an output format other than 'text' is requested. In
>> that case, the first parameter of the JSON array 'params' will now be
>> the designated command, the second one the output format and the rest
>> will be command args.
>
> Ilya brought up the question why I changed the meaning of 'method' and
> 'params' instead of adding the output format as an addition argument to the
> command arguments in 'params'. The server side would then interpret and
> filter out this argument before passing the remaining arguments to the
> command callbacks.
>
> I decided against this approach because the code would get more involved, in
> particular we would have to implement option/argument parsing inside
> process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a
> safe way would be difficult in general because their global state.)
> The current implementation is based purely on JSON objects/arrays which are
> nicely supported by OVS with functions from lib/json.*.
I'm not sure I got this point, but see below.
>
> Ilya also voiced concerns about the limited extensibility of the proposed
> API. To fix this, this patch series could be tweaked in the follow way:
>
> (1.) Instead of passing the output format as second entry in the JSON array
> 'params', turn the second entry into a JSON object (shash). The output format
> would be one entry in this JSON object, e.g. called 'format'.
>
> The server (process_command() from lib/unixctl.c) will parse the content of
> this JSON object into known options. Clients would only add non-default
> options to this JSON object to keep compatibility with older servers. Options
> which are supported by the server but not transferred via this JSON object
> would be initialized with default values to keep compatibility with older
> clients. Unknown entries would cause the server to return an error.
This kind of implements what I had in mind, but in a slightly different
manner. How about something like this:
* ovs-appctl has a global parameter --format passed to it, not part of
the requested command paramaters. (I think this is implemented in the
code, below, but I didn't read very carefully.)
* ovs-appctl adds this paramater as an argument to a 'params' JSON array.
i.e. separate element in the array to every command it executes, if
provided.
* Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
that argument, removes it from the list of parameters before passing
to the actual handler.
* Before calling a handler, server stores these special parameters
into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
really matter).
* Handler always has the conn pointer. Handler can ask in which format
the user prefers the output and generate one. E.g.:
if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
unixctl_command_reply_json(conn, ...);
} else {
unixctl_command_reply(conn, ...);
}
Notice that nothing bad happens if command is not aware of the formats,
because unixctl_command_reply() is a JSON reply, but with a simple
JSON string instead of any fancy structure.
* Result is sent back to the ovs-appctl.
* If the --format=json was originally requested, just dump the result
as JSON to the output in the unixctl_client_transact(). If format
wasn't requested, parse it and print out the same way as it is done
right now, if the result is not a simple JSON string - error.
This approach requires no changes in the code that doesn't want to provide
JSON output and no changes to the command registering API.
All commands
[ovs-dev] Kernel panic during Incus (ex-LXD) OVN tests
Hello, I'm currently working on re-enabling our daily OVN tests in Incus (the LXD fork). Unfortunately I'm not having much luck getting our testsuite to go all the way through as it's triggering kernel panics. Here is the stack trace I'm getting: ``` [ 484.818312] BUG: TASK stack guard page was hit at 75451289 (stack is d168f1fe..c11d6033) [ 484.818331] stack guard page: [#1] PREEMPT SMP PTI [ 484.818338] CPU: 0 PID: 19480 Comm: revalidator49 Not tainted 6.5.9-zabbly+ #ubuntu22.04 [ 484.818343] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009)/LXD, BIOS unknown 2/2/2022 [ 484.818345] RIP: 0010:reserve_sfa_size+0x5/0x110 [openvswitch] [ 484.818412] Code: cc 0f 0b eb d1 48 c7 c0 f4 ff ff ff 5d c3 cc cc cc cc 0f 1f 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 <55> 48 89 e5 41 57 41 89 d7 41 56 44 8d 76 03 41 55 41 83 e6 fc 41 [ 484.818414] RSP: 0018:a8a840df8000 EFLAGS: 00010246 [ 484.818417] RAX: 9411b473 RBX: RCX: [ 484.818419] RDX: 0001 RSI: 0004 RDI: a8a840dfb278 [ 484.818420] RBP: a8a840df8030 R08: 0001 R09: dd86 [ 484.818421] R10: a8a840df80ff R11: a8a840dfb280 R12: a8a84113247c [ 484.818422] R13: 0004 R14: R15: 0019 [ 484.818424] FS: 7f25032f5640() GS:94123720() knlGS: [ 484.818426] CS: 0010 DS: ES: CR0: 80050033 [ 484.818428] CR2: a8a840df7ff8 CR3: 00011315a001 CR4: 00170ef0 [ 484.818433] Call Trace: [ 484.818445] <#DF> [ 484.818450] ? show_regs+0x6a/0x80 [ 484.818467] ? die+0x38/0xa0 [ 484.818470] ? handle_stack_overflow+0x4e/0x60 [ 484.818478] ? exc_double_fault+0x106/0x190 [ 484.818502] ? asm_exc_double_fault+0x1f/0x30 [ 484.818518] ? reserve_sfa_size+0x5/0x110 [openvswitch] [ 484.818530] [ 484.818531] [ 484.818531] ? __add_action+0x2a/0x80 [openvswitch] [ 484.818542] validate_set.constprop.0+0x23e/0x410 [openvswitch] [ 484.818555] __ovs_nla_copy_actions+0x40f/0xfb0 [openvswitch] [ 484.818566] ? reserve_sfa_size+0x32/0x110 [openvswitch] [ 484.818577] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818593] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818604] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818615] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818630] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818642] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818671] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818683] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818694] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818705] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818717] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818728] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818739] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818750] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818761] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818772] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818783] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818794] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818806] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818817] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818828] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818839] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818850] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818861] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818872] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818883] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818894] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818905] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818916] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818927] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818939] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818950] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818961] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818972] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818983] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.818994] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819005] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819016] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819027] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819038] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819049] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819060] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [ 484.819071] __ovs_nla_copy_actions+0xbda/0xfb0 [openvswitch] [
Re: [ovs-dev] [PATCH ovn v3] ci: Remove '--recheck' in CI.
On Fri, Jul 14, 2023 at 2:12 AM Ales Musil wrote:
>
> On Thu, Jul 13, 2023 at 12:52 PM Dumitru Ceara wrote:
>
> > If we want to catch new failures faster we have a better chance if CI
> > doesn't auto-retry (once).
> >
> > There are some tests that are still "unstable" and fail every now and
> > then. In order to reduce the number of false negatives keep the
> > --recheck for them. To achieve that we use a new macro, TAG_UNSTABLE,
> > to tag all these tests. The list of "unstable" tests is compiled based
> > on the following discussion:
> > https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405465.html
> >
> > In order to avoid new GitHub actions jobs, we re-purpose the last job of
> > each target type to also run the unstable tests. These jobs were
> > already running less tests than others so the additional run time should
> > not be an issue.
> >
> > Signed-off-by: Dumitru Ceara
> > ---
> > V3:
> > - Addressed Ales' comment:
> > - pass RECHECK to the container running the tests in ci.sh
> > V2:
> > - Addressed Ales' comments:
> > - always run stable and unstable tests before declaring pass/fail
> > Changes in v1 (since RFC):
> > - kept recheck for unstable tests
> > - introduced TAG_UNSTABLE
> > - changed test.yml to run unstable tests in the last batch of every
> > test target type.
> > ---
> > .ci/ci.sh | 2 +-
> > .ci/linux-build.sh | 76 ++
> > .github/workflows/test.yml | 15
> > tests/ovn-ic.at| 1 +
> > tests/ovn-ipsec.at | 1 +
> > tests/ovn-macros.at| 5 +++
> > tests/ovn-northd.at| 1 +
> > tests/ovn-performance.at | 1 +
> > tests/ovn.at | 13 +++
> > 9 files changed, 92 insertions(+), 23 deletions(-)
> >
> > diff --git a/.ci/ci.sh b/.ci/ci.sh
> > index 10f11939c5..3a7ee97696 100755
> > --- a/.ci/ci.sh
> > +++ b/.ci/ci.sh
> > @@ -101,7 +101,7 @@ function run_tests() {
> > && \
> > ARCH=$ARCH CC=$CC LIBS=$LIBS OPTS=$OPTS TESTSUITE=$TESTSUITE \
> > TEST_RANGE=$TEST_RANGE SANITIZERS=$SANITIZERS DPDK=$DPDK \
> > -./.ci/linux-build.sh
> > +RECHECK=$RECHECK UNSTABLE=$UNSTABLE ./.ci/linux-build.sh
> > "
> > }
> >
> > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> > index 5a79a52daf..4c5361f3ca 100755
> > --- a/.ci/linux-build.sh
> > +++ b/.ci/linux-build.sh
> > @@ -9,6 +9,7 @@ COMMON_CFLAGS=""
> > OVN_CFLAGS=""
> > OPTS="$OPTS --enable-Werror"
> > JOBS=${JOBS:-"-j4"}
> > +RECHECK=${RECHECK:-"no"}
> >
> > function install_dpdk()
> > {
> > @@ -99,6 +100,17 @@ function configure_clang()
> > COMMON_CFLAGS="${COMMON_CFLAGS}
> > -Wno-error=unused-command-line-argument"
> > }
> >
> > +function run_tests()
> > +{
> > +if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
> > +TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=$RECHECK
> > +then
> > +# testsuite.log is necessary for debugging.
> > +cat */_build/sub/tests/testsuite.log
> > +return 1
> > +fi
> > +}
> > +
> > function execute_tests()
> > {
> > # 'distcheck' will reconfigure with required options.
> > @@ -106,27 +118,61 @@ function execute_tests()
> > configure_ovn
> >
> > export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
> > -if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
> > -TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=yes
> > -then
> > -# testsuite.log is necessary for debugging.
> > -cat */_build/sub/tests/testsuite.log
> > +
> > +local stable_rc=0
> > +local unstable_rc=0
> > +
> > +if ! SKIP_UNSTABLE=yes run_tests; then
> > +stable_rc=1
> > +fi
> > +
> > +if [ "$UNSTABLE" ]; then
> > +if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \
> > +run_tests; then
> > +unstable_rc=1
> > +fi
> > +fi
> > +
> > +if [[ $stable_rc -ne 0 ]] || [[ $unstable_rc -ne 0 ]]; then
> > exit 1
> > fi
> > }
> >
> > +function run_system_tests()
> > +{
> > +local type=$1
> > +local log_file=$2
> > +
> > +if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" \
> > +RECHECK=$RECHECK; then
> > +# $log_file is necessary for debugging.
> > +cat tests/$log_file
> > +return 1
> > +fi
> > +}
> > +
> > function execute_system_tests()
> > {
> > - type=$1
> > - log_file=$2
> > -
> > - configure_ovn $OPTS
> > - make $JOBS || { cat config.log; exit 1; }
> > - if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE"
> > RECHECK=yes; then
> > - # $log_file is necessary for debugging.
> > - cat tests/$log_file
> > - exit 1
> > - fi
> > +configure_ovn $OPTS
> > +make $JOBS || { cat config.log; exit 1; }
> > +
> > +local stable_rc=0
> > +local unstable_rc=0
> > +
> > +if ! SKIP_UNSTABLE=yes run_system_tests $@; then
> > +
Re: [ovs-dev] [PATCH ovn v3] controller: Don't artificially limit group and meter IDs to 16bit.
On Thu, Oct 26, 2023 at 7:37 AM Dumitru Ceara wrote:
>
> OVS actually supports way more. Detect the real number of groups and
> meters instead. To avoid preallocating huge bitmaps for the IDs,
> switch to id-pool instead (as suggested by Ilya).
>
> Reported-at: https://issues.redhat.com/browse/FDP-70
> Suggested-by: Ilya Maximets
> Signed-off-by: Dumitru Ceara
> ---
> V3:
> - Addressed Ilya's comments:
> - removed references to 'bitmap' in comments
> - fixed indentation & typos
> - moved "id-pool.h" include to C file
> - extended the OVS feature detection component to also check for the
> max number of groups.
> V2:
> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
> - renamed max_size to max_id, it's more accurate.
> ---
> controller/ofctrl.c | 2 +
> controller/ovn-controller.c | 6 +--
> include/ovn/features.h | 3 ++
> lib/extend-table.c | 81 --
> lib/extend-table.h | 13 --
> lib/features.c | 88 ++---
> tests/test-ovn.c| 4 +-
> 7 files changed, 140 insertions(+), 57 deletions(-)
Acked-by: Numan Siddique
Please see one comment/suggestion below.
Thanks
Numan
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 63b0aa975b..06f8b33232 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
> /* Clear existing groups, to match the state of the switch. */
> if (groups) {
> ovn_extend_table_clear(groups, true);
> +ovn_extend_table_reinit(groups, ovs_feature_max_select_groups_get());
> }
>
> /* Clear existing meters, to match the state of the switch. */
> if (meters) {
> ovn_extend_table_clear(meters, true);
> +ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
> ofctrl_meter_bands_clear();
> }
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index da7d145ed3..7a8ca38286 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node
> OVS_UNUSED,
> {
> struct ed_type_lflow_output *data = xzalloc(sizeof *data);
> ovn_desired_flow_table_init(&data->flow_table);
> -ovn_extend_table_init(&data->group_table);
> -ovn_extend_table_init(&data->meter_table);
> +ovn_extend_table_init(&data->group_table, "group-table", 0);
> +ovn_extend_table_init(&data->meter_table, "meter-table", 0);
> objdep_mgr_init(&data->lflow_deps_mgr);
> lflow_conj_ids_init(&data->conj_ids);
> uuidset_init(&data->objs_processed);
> @@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
> engine_set_force_recompute(true);
> }
>
> -if (br_int) {
> +if (br_int && ovs_feature_set_discovered()) {
> ct_zones_data = engine_get_data(&en_ct_zones);
> if (ct_zones_data && ofctrl_run(br_int, ovs_table,
> &ct_zones_data->pending)) {
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 7c3004b271..2c47ab766e 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
> bool ovs_feature_is_supported(enum ovs_feature_value feature);
> bool ovs_feature_support_run(const struct smap *ovs_capabilities,
> const char *br_name);
> +bool ovs_feature_set_discovered(void);
> +uint32_t ovs_feature_max_meters_get(void);
> +uint32_t ovs_feature_max_select_groups_get(void);
>
> #endif
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index ebb1a054cb..ed975a511e 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -17,9 +17,9 @@
> #include
> #include
>
> -#include "bitmap.h"
> #include "extend-table.h"
> #include "hash.h"
> +#include "id-pool.h"
> #include "lib/uuid.h"
> #include "openvswitch/vlog.h"
>
> @@ -30,13 +30,29 @@ ovn_extend_table_delete_desired(struct ovn_extend_table
> *table,
> struct ovn_extend_table_lflow_to_desired *l);
>
> void
> -ovn_extend_table_init(struct ovn_extend_table *table)
> +ovn_extend_table_init(struct ovn_extend_table *table, const char *table_name,
> + uint32_t max_id)
> {
> -table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
> -bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
> -hmap_init(&table->desired);
> -hmap_init(&table->lflow_to_desired);
> -hmap_init(&table->existing);
> +*table = (struct ovn_extend_table) {
> +.name = table_name,
> +.max_id = max_id,
> +/* Table id 0 is invalid, set id-pool base to 1. */
> +.table_ids = id_pool_create(1, max_id),
> +.desired = HMAP_INITIALIZER(&table->desired),
> +.lflow_to_desired = HMAP_INITIALIZER(&
Re: [ovs-dev] [PATCH v8 2/9] system-dpdk: Don't require hugetlbfs.
On Fri, Oct 27, 2023 at 5:31 PM David Marchand wrote: > > dpdk-testpmd does not need hugetlbfs backing as we don't require > multiprocess support in OVS unit tests. > > Switch to --in-memory and remove the (then unneeded) check on > hugetlbfs presence. > > Acked-by: Aaron Conole > Acked-by: Eelco Chaudron > Signed-off-by: David Marchand > --- > Changes since v7: > - reverted use of dynamic allocations and kept initial memory > reservation: this avoids random failures when testpmd and ovs-dpdk > try to allocate memory at the same time, Just a heads up on this change. While testing the v8 series, I hit random failures on the mtu unit tests. testpmd would fail to allocate memory in hugepages. I suspect this has to do with OVS allocating memory in // of testpmd. Reverting to an initial reserved memory makes the issue disappear so I removed this change and kept existing behavior. There may still be a race underneath but at least the situation is as bad as before. -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v8 8/9] system-dpdk: Rework cleanup for vhost-user client tests.
Those tests are subject to a race when a testpmd hosting the vhost-user server is stopped and OVS has enough time to detect the vhost-user socket drop and tries to reconnect to this socket. In such a situation, the tests can fail as the OVS process with the vhost-user client port complains with a warning log: 2023-09-08T13:15:18.160Z|00163|dpdk|INFO|VHOST_CONFIG: (.../005/dpdkvhostclient0) vhost peer closed 2023-09-08T13:15:18.160Z|00164|netdev_dpdk|INFO|vHost Device '.../005/dpdkvhostclient0' connection has been destroyed 2023-09-08T13:15:18.160Z|00165|dpdk|INFO|VHOST_CONFIG: (.../005/dpdkvhostclient0) vhost-user client: socket created, fd: 24 2023-09-08T13:15:18.160Z|00166|dpdk|WARN|VHOST_CONFIG: (.../005/dpdkvhostclient0) failed to connect: Connection refused 2023-09-08T13:15:18.160Z|00167|dpdk|INFO|VHOST_CONFIG: (.../005/dpdkvhostclient0) reconnecting... Invert the order of the cleanup steps. Signed-off-by: David Marchand Acked-by: Eelco Chaudron --- Changes since v6: - added this fix for spurious failures hit by Eelco, --- tests/system-dpdk.at | 15 +-- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index fd4a4b7d73..80277b24b6 100644 --- a/tests/system-dpdk.at +++ b/tests/system-dpdk.at @@ -222,10 +222,9 @@ AT_CHECK([test `ovs-vsctl get interface dpdkvhostuserclient0 statistics:tx_bytes $((`ovs-vsctl get interface dpdkvhostuserclient0 statistics:tx_q0_good_bytes` + dnl `ovs-vsctl get interface dpdkvhostuserclient0 statistics:tx_q1_good_bytes`))]) -OVS_DPDK_STOP_TESTPMD() - dnl Clean up AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) +OVS_DPDK_STOP_TESTPMD() OVS_DPDK_STOP_VSWITCHD(["dnl /VHOST_CONFIG: (.*dpdkvhostclient0) recvmsg failed/d /VHOST_CONFIG: (.*dpdkvhostclient0) failed to connect: No such file or directory/d @@ -649,10 +648,9 @@ AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9000]) AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) AT_CHECK([grep -E 'mtu=9000' stdout], [], [stdout]) -OVS_DPDK_STOP_TESTPMD() - dnl Clean up AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) +OVS_DPDK_STOP_TESTPMD() OVS_DPDK_STOP_VSWITCHD(["dnl /VHOST_CONFIG: (.*dpdkvhostclient0) failed to connect: No such file or directory/d"]) AT_CLEANUP @@ -694,10 +692,9 @@ AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=2000]) AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) AT_CHECK([grep -E 'mtu=2000' stdout], [], [stdout]) -OVS_DPDK_STOP_TESTPMD() - dnl Clean up AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) +OVS_DPDK_STOP_TESTPMD() OVS_DPDK_STOP_VSWITCHD(["dnl /VHOST_CONFIG: (.*dpdkvhostclient0) failed to connect: No such file or directory/d"]) AT_CLEANUP @@ -813,10 +810,9 @@ dnl Set MTU value above upper bound and check for error AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9711]) AT_CHECK([grep "dpdkvhostuserclient0: unsupported MTU 9711" ovs-vswitchd.log], [], [stdout]) -OVS_DPDK_STOP_TESTPMD() - dnl Clean up AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) +OVS_DPDK_STOP_TESTPMD() OVS_DPDK_STOP_VSWITCHD(["dnl /VHOST_CONFIG: (.*dpdkvhostclient0) failed to connect: No such file or directory/d /dpdkvhostuserclient0: unsupported MTU 9711/d @@ -859,10 +855,9 @@ dnl Set MTU value below lower bound and check for error AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=67]) AT_CHECK([grep "dpdkvhostuserclient0: unsupported MTU 67" ovs-vswitchd.log], [], [stdout]) -OVS_DPDK_STOP_TESTPMD() - dnl Clean up AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) +OVS_DPDK_STOP_TESTPMD() OVS_DPDK_STOP_VSWITCHD(["dnl /VHOST_CONFIG: (.*dpdkvhostclient0) failed to connect: No such file or directory/d /dpdkvhostuserclient0: unsupported MTU 67/d -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v8 9/9] system-dpdk: Run traffic tests.
Integrate system-traffic.at tests as part of check-dpdk. Some tests that can't work with the userspace datapath are skipped by overriding some OVS_CHECK_* macros. ADD_VETH is implemented using the net/af_xdp DPDK driver. Signed-off-by: David Marchand Acked-by: Eelco Chaudron --- Changes since v6: - fixed some checkpatch warning, Changes since v4: - switched to net/af_xdp, this removes the tweaking needed for net/tap, and it lets existing tool relying on kernel netdevs. veth offloading still needs some tweaking, Changes since v3: - reverted --dummy-numa and opted for configuring a number of rxqs relevant to the number of NUMA sockets, Changes since v2: - added ADD_VETH_IGNORE_LOGS and moved ignored error logs to OVS_TRAFFIC_VSWITCHD_STOP, - added --no-pci to DPDK options to avoid failing the tests when running in a vm with a virtio-net device, - faked a mono numa/mono core so that OVS requests at max 2 txqs on the net/tap port, --- .ci/dpdk-build.sh| 3 +- .github/workflows/build-and-test.yml | 2 +- tests/system-dpdk-macros.at | 77 tests/system-dpdk-testsuite.at | 2 + tests/system-dpdk.at | 3 -- 5 files changed, 82 insertions(+), 5 deletions(-) diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh index 35540f0694..aa83e44643 100755 --- a/.ci/dpdk-build.sh +++ b/.ci/dpdk-build.sh @@ -38,7 +38,8 @@ function build_dpdk() # any DPDK driver. # check-dpdk unit tests requires testpmd and some net/ driver. DPDK_OPTS="$DPDK_OPTS -Denable_apps=test-pmd" -DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio" +enable_drivers="net/null,net/af_xdp,net/tap,net/virtio" +DPDK_OPTS="$DPDK_OPTS -Denable_drivers=$enable_drivers" # Install DPDK using prefix. DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build" diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 4f62efb7c3..09654205e7 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -5,7 +5,7 @@ on: [push, pull_request] jobs: build-dpdk: env: - dependencies: gcc libnuma-dev ninja-build + dependencies: gcc libbpf-dev libnuma-dev ninja-build pkgconf CC: gcc DPDK_GIT: https://dpdk.org/git/dpdk-stable DPDK_VER: 22.11.1 diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at index 7fedfd6515..dcdfa55741 100644 --- a/tests/system-dpdk-macros.at +++ b/tests/system-dpdk-macros.at @@ -127,3 +127,80 @@ m4_define([OVS_DPDK_STOP_TESTPMD], [AT_CHECK([kill `cat testpmd.pid`]) OVS_WAIT([kill -0 `cat testpmd.pid`], [kill -9 `cat testpmd.pid`]) ]) + + +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [dbinit-aux-args]) +# +# Creates a database and starts ovsdb-server, starts ovs-vswitchd +# connected to that database, calls ovs-vsctl to create a bridge named +# br0 with predictable settings, passing 'vsctl-args' as additional +# commands to ovs-vsctl. If 'vsctl-args' causes ovs-vsctl to provide +# output (e.g. because it includes "create" commands) then 'vsctl-output' +# specifies the expected output after filtering through uuidfilt. +# 'dbinit-aux-args' are passed as additional commands to 'ovs-vsctl init' +# before starting ovs-vswitchd. +m4_define([OVS_TRAFFIC_VSWITCHD_START], + [ + OVS_DPDK_PRE_CHECK() + OVS_WAIT_WHILE([ip link show ovs-netdev]) + dnl For functional tests, no need for DPDK PCI probing. + OVS_DPDK_START([--no-pci], [--disable-system], [$3]) + dnl Add bridges, ports, etc. + OVS_WAIT_WHILE([ip link show br0]) + AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) +]) + + +# OVS_TRAFFIC_VSWITCHD_STOP([ALLOWLIST], [extra_cmds]) +# +# Gracefully stops ovs-vswitchd and ovsdb-server, checking their log files +# for messages with severity WARN or higher and signaling an error if any +# is present. The optional ALLOWLIST may contain shell-quoted "sed" +# commands to delete any warnings that are actually expected, e.g.: +# +# OVS_TRAFFIC_VSWITCHD_STOP(["/expected error/d"]) +# +# 'extra_cmds' are shell commands to be executed after OVS_VSWITCHD_STOP() is +# invoked. They can be used to perform additional cleanups such as name space +# removal. +m4_define([OVS_TRAFFIC_VSWITCHD_STOP], + [OVS_DPDK_STOP_VSWITCHD([$1]) + AT_CHECK([:; $2]) +]) + + +# Plug a veth into OVS via DPDK net/af_xdp. +m4_define([ADD_VETH], +[ AT_CHECK([ip link add $1 type veth peer name ovs-$1 || return 77]) + CONFIGURE_VETH_OFFLOADS([$1]) + AT_CHECK([ip link set $1 netns $2]) + AT_CHECK([ip link set dev ovs-$1 up]) + AT_CHECK([ovs-vsctl add-port $3 ovs-$1 -- \ +set interface ovs-$1 external-ids:iface-id="$1" -- \ +set interface ovs-$1 type=dpdk -- \ +set interface ovs-$1 options:dpdk-devargs=net_af_xdp$1,iface=ovs-$1]) + NS_CHECK_EXEC([$2], [ip addr add $4 dev $1 $7])
[ovs-dev] [PATCH v8 6/9] netdev-afxdp: Postpone libbpf logging helper registration.
When using net/af_xdp DPDK driver along OVS native AF_XDP support,
confusing logs are reported, like:
netdev_dpdk|INFO|Device 'net_af_xdpp0,iface=ovs-p0' attached to DPDK
dpif_netdev|INFO|PMD thread on numa_id: 0, core id: 11 created.
dpif_netdev|INFO|There are 1 pmd threads on numa node 0
dpdk|INFO|Device with port_id=0 already stopped
dpdk(pmd-c11/id:22)|INFO|PMD thread uses DPDK lcore 1.
netdev_dpdk|WARN|Rx checksum offload is not supported on port 0
netdev_afxdp|INFO|libbpf: elf: skipping unrecognized data section(6)
.xdp_run_config
netdev_afxdp|INFO|libbpf: elf: skipping unrecognized data section(7)
xdp_metadata
netdev_afxdp|INFO|libbpf: elf: skipping unrecognized data section(7)
xdp_metadata
netdev_afxdp|INFO|libbpf: elf: skipping unrecognized data section(7)
xdp_metadata
This comes from the fact that netdev-afxdp unconditionnally registers a
helper for logging libbpf messages.
Making both net/af_xdp and netdev-afxdp work at the same time seems
difficult, so at least, ensure that netdev-afxdp won't register this
helper unless a netdev is actually allocated.
Signed-off-by: David Marchand
Acked-by: Eelco Chaudron
---
lib/netdev-afxdp.c | 12 ++--
lib/netdev-afxdp.h | 1 -
lib/netdev-linux.c | 1 -
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc306..9884ccec4f 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -1195,18 +1195,18 @@ libbpf_print(enum libbpf_print_level level,
return 0;
}
-int netdev_afxdp_init(void)
-{
-libbpf_set_print(libbpf_print);
-return 0;
-}
-
int
netdev_afxdp_construct(struct netdev *netdev)
{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
struct netdev_linux *dev = netdev_linux_cast(netdev);
int ret;
+if (ovsthread_once_start(&once)) {
+libbpf_set_print(libbpf_print);
+ovsthread_once_done(&once);
+}
+
/* Configure common netdev-linux first. */
ret = netdev_linux_construct(netdev);
if (ret) {
diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
index e91cd102d2..6c5459f6e6 100644
--- a/lib/netdev-afxdp.h
+++ b/lib/netdev-afxdp.h
@@ -47,7 +47,6 @@ struct xsk_socket_info;
int netdev_afxdp_rxq_construct(struct netdev_rxq *rxq_);
void netdev_afxdp_rxq_destruct(struct netdev_rxq *rxq_);
-int netdev_afxdp_init(void);
int netdev_afxdp_construct(struct netdev *netdev_);
void netdev_afxdp_destruct(struct netdev *netdev_);
int netdev_afxdp_verify_mtu_size(const struct netdev *netdev, int mtu);
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index cca3408797..4538cdfe63 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -3754,7 +3754,6 @@ const struct netdev_class netdev_internal_class = {
#ifdef HAVE_AF_XDP
#define NETDEV_AFXDP_CLASS_COMMON \
-.init = netdev_afxdp_init, \
.construct = netdev_afxdp_construct,\
.destruct = netdev_afxdp_destruct, \
.get_stats = netdev_afxdp_get_stats,\
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v8 7/9] system-dpdk: Remove tap interfaces from vport MTU tests.
The unit tests for changing MTU with vhost-user ports are not using those tap interfaces. Signed-off-by: David Marchand --- Changes since v7: - added this patch after getting regressions in Intel CI because of the next patch, --- tests/system-dpdk.at | 14 -- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index 655e25ff13..fd4a4b7d73 100644 --- a/tests/system-dpdk.at +++ b/tests/system-dpdk.at @@ -636,9 +636,7 @@ AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout]) -OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1" \ ---vdev="net_tap0,iface=tap0"]) - +OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"]) OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log]) dnl Check default MTU value in the datapath @@ -683,9 +681,7 @@ AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout]) -OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1" \ ---vdev="net_tap0,iface=tap0"]) - +OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"]) OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log]) dnl Check MTU value in the datapath @@ -805,8 +801,7 @@ AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9702]) AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2 -OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1" \ ---vdev="net_tap0,iface=tap0"]) +OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"]) OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log]) @@ -852,8 +847,7 @@ AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout]) -OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1" \ ---vdev="net_tap0,iface=tap0"]) +OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"]) OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log]) -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v8 5/9] system-dpdk: Refactor OVS daemons helpers.
Align system-dpdk existing helpers to other common OVS helpers so they
can accept some optional arguments.
Introduce a OVS_DPDK_STOP_VSWITCHD wrapper around OVS_VSWITCHD_STOP to
catch dpdk related logs in a centralised fashion.
Signed-off-by: David Marchand
Acked-by: Eelco Chaudron
---
Changes since v6:
- did a minor cleanup on vhost-user client test log pattern (one entry
concerned vhost-user ports, not vhost-user *client* ports),
---
tests/system-dpdk-macros.at | 21 -
tests/system-dpdk.at| 158 +++-
2 files changed, 82 insertions(+), 97 deletions(-)
diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index 35d14bee8f..7fedfd6515 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -36,12 +36,13 @@ m4_define([OVS_DPDK_PRE_PHY_SKIP],
#
m4_define([OVS_DPDK_START],
[dnl start ovs dpdk
- OVS_DPDK_START_OVSDB()
+ OVS_DPDK_START_OVSDB($3)
dnl Enable DPDK functionality
AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
other_config:dpdk-init=true])
- OVS_DPDK_START_VSWITCHD($1)
+ OVS_DPDK_START_VSWITCHD([$1], [$2])
])
+
# OVS_DPDK_START_OVSDB()
#
# Create an empty database and start ovsdb-server.
@@ -60,9 +61,10 @@ m4_define([OVS_DPDK_START_OVSDB],
AT_CAPTURE_FILE([ovsdb-server.log])
dnl Initialize database.
- AT_CHECK([ovs-vsctl --no-wait init])
+ AT_CHECK([ovs-vsctl --no-wait init $1])
])
+
# OVS_DPDK_START_VSWITCHD()
#
# Add special configuration for dpdk-init. Start ovs-vswitchd.
@@ -72,12 +74,23 @@ m4_define([OVS_DPDK_START_VSWITCHD],
AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch .
other_config:dpdk-extra="--log-level=pmd.*:error $1"])
dnl Start ovs-vswitchd.
- AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn
-vofproto_dpif -vunixctl], [0], [stdout], [stderr])
+ AT_CHECK([ovs-vswitchd $2 --detach --no-chdir --pidfile --log-file -vvconn
-vofproto_dpif -vunixctl], [0], [stdout], [stderr])
AT_CAPTURE_FILE([ovs-vswitchd.log])
on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
])
+m4_define([OVS_DPDK_STOP_VSWITCHD],
+ [OVS_VSWITCHD_STOP([dnl
+$1";/does not exist. The Open vSwitch kernel module is probably not loaded./d
+/does not support MTU configuration,/d
+/EAL: No \(available\|free\) .*hugepages reported/d
+/Failed to enable flow control/d
+/Rx checksum offload is not supported on/d
+/TELEMETRY: No legacy callbacks, legacy socket not created/d"])
+])
+
+
# OVS_DPDK_CHECK_TESTPMD()
#
# Check dpdk-testpmd availability.
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 270587e2c0..655e25ff13 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -3,15 +3,6 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
AT_BANNER([OVS-DPDK unit tests])
-m4_define([SYSTEM_DPDK_ALLOWED_LOGS],[
-\@does not exist. The Open vSwitch kernel module is probably not loaded.@d
-\@does not support MTU configuration,@d
-\@EAL: No \(available\|free\) .*hugepages reported@d
-\@Failed to enable flow control@d
-\@Rx checksum offload is not supported on@d
-\@TELEMETRY: No legacy callbacks, legacy socket not created@d
-])
-
dnl CHECK_MEMPOOL_PARAM([mtu], [numa], [+line])
dnl
dnl Waits for logs to indicate that the user has configured a mempool
@@ -36,7 +27,7 @@ OVS_DPDK_START([--no-pci])
AT_CHECK([grep "DPDK Enabled - initializing..." ovs-vswitchd.log], [],
[stdout])
AT_CHECK([grep "EAL" ovs-vswitchd.log], [], [stdout])
AT_CHECK([grep "DPDK Enabled - initialized" ovs-vswitchd.log], [], [stdout])
-OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+OVS_DPDK_STOP_VSWITCHD
AT_CLEANUP
dnl --
@@ -58,7 +49,7 @@ sleep 2
dnl Clean up
AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+OVS_DPDK_STOP_VSWITCHD
AT_CLEANUP
dnl --
@@ -84,9 +75,8 @@ AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0)
reconnecting..." ov
dnl Clean up
AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
[stderr])
-OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
-\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No such file
or directory@d
-])")
+OVS_DPDK_STOP_VSWITCHD(["dnl
+/VHOST_CONFIG: (.*dpdkvhostclient0) failed to connect: No such file or
directory/d"])
AT_CLEANUP
dnl --
@@ -150,12 +140,11 @@ OVS_WAIT_UNTIL([grep "vHost Device
'$OVS_RUNDIR/dpdkvhostuser0' has been removed
dnl Clean up
AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuser0], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
-\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) recvmsg failed@d
-\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) failed to connect: No such file
or directory@d
-\@dpdkvhostuser ports
[ovs-dev] [PATCH v8 4/9] tests: Define a macro to skip tc relying tests.
Some unit tests expect that a OVS port has an associated netdevice on which they can hook tc. This will not be possible when testing the userspace datapath with DPDK. Introduce a helper (which will be overriden in system-dpdk tests) and use it in the existing tests. Acked-by: Aaron Conole Signed-off-by: David Marchand Acked-by: Eelco Chaudron --- Changes since v4: - as the traffic tests now use net/af_xdp, it is not required to skip tests relying on tcpdump anymore, --- tests/system-common-macros.at| 6 ++ tests/system-offloads-traffic.at | 6 +++--- tests/system-traffic.at | 6 +++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index 0077a8609c..0113aae8bd 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -297,6 +297,12 @@ m4_define([OVS_START_L7], # m4_define([OFPROTO_CLEAR_DURATION_IDLE], [[sed -e 's/duration=.*s,/duration=,/g' -e 's/idle_age=[0-9]*,/idle_age=,/g']]) +# OVS_CHECK_TC_QDISC() +# +# Macro to skip tests when tc qdisc can't be applied on a OVS port. +m4_define([OVS_CHECK_TC_QDISC], +[AT_SKIP_IF([test $HAVE_TC = no])]) + # OVS_CHECK_TUNNEL_TSO() # # Macro to be used in general tunneling tests that could be also diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 5ad6b4bfdf..0bedee7530 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -20,7 +20,7 @@ m4_define([OVS_CHECK_ACTIONS], [ m4_define([CHECK_TC_INGRESS_PPS], [ -AT_SKIP_IF([test $HAVE_TC = "no"]) +OVS_CHECK_TC_QDISC() AT_CHECK([ip link add ovs_tc_pps0 type veth peer name ovs_tc_pps1 dnl || exit 77]) on_exit 'ip link del ovs_tc_pps0' @@ -95,7 +95,7 @@ AT_CLEANUP AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled]) AT_KEYWORDS([ingress_policing]) -AT_SKIP_IF([test $HAVE_TC = "no"]) +OVS_CHECK_TC_QDISC() OVS_TRAFFIC_VSWITCHD_START() AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false]) AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) @@ -118,7 +118,7 @@ AT_CLEANUP AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled]) AT_KEYWORDS([ingress_policing]) -AT_SKIP_IF([test $HAVE_TC = "no"]) +OVS_CHECK_TC_QDISC() OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true]) AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) ADD_NAMESPACES(at_ns0) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 7ea4502028..a7d4ed83bd 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2321,7 +2321,7 @@ AT_CLEANUP AT_BANNER([QoS]) AT_SETUP([QoS - basic configuration]) -AT_SKIP_IF([test $HAVE_TC = no]) +OVS_CHECK_TC_QDISC() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -2355,7 +2355,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([QoS - 64bit]) -AT_SKIP_IF([test $HAVE_TC = no]) +OVS_CHECK_TC_QDISC() AT_SKIP_IF([test $HAVE_TCA_HTB_RATE64 = no]) OVS_TRAFFIC_VSWITCHD_START() @@ -2383,7 +2383,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([Ingress Policing - 64-bit]) -AT_SKIP_IF([test $HAVE_TC = no]) +OVS_CHECK_TC_QDISC() AT_SKIP_IF([test $HAVE_TCA_POLICE_PKTRATE64 = no]) OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(ns0) -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v8 3/9] ci: Run DPDK tests in GitHub Actions.
Let's enhance our coverage in the CI and run DPDK system tests.
A few DPDK drivers are enabled in DPDK compilation.
Put DPDK build in $PATH for dpdk-testpmd to be available.
sudo drops PATH= updates and -E alone does not seem to preserve this
variable.
Pass PATH=$PATH when running the tests, as a workaround.
Since those tests are run as root, the collection of logs is updated
accordingly.
In GHA, only two cores are available but some test rely on testpmd using
three lcores.
Add a DPDK_EAL_OPTIONS environment variable and use it to map all
testpmd lcores to core 1 (and leave core 0 alone for OVS main and PMD
threads).
Signed-off-by: David Marchand
Acked-by: Aaron Conole
Acked-by: Eelco Chaudron
---
Changes since v1:
- rebased after DPDK build has been moved out of linux-build.sh,
- restored running "normal" checks in the DPDK jobs,
---
.ci/dpdk-build.sh| 7 ---
.ci/linux-build.sh | 15 ++-
.github/workflows/build-and-test.yml | 7 ---
tests/system-dpdk-macros.at | 2 +-
4 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
index 02dcefef61..35540f0694 100755
--- a/.ci/dpdk-build.sh
+++ b/.ci/dpdk-build.sh
@@ -35,9 +35,10 @@ function build_dpdk()
DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
# OVS compilation and "normal" unit tests (run in the CI) do not depend on
-# any DPDK driver being present.
-# We can disable all drivers to save compilation time.
-DPDK_OPTS="$DPDK_OPTS -Ddisable_drivers=*/*"
+# any DPDK driver.
+# check-dpdk unit tests requires testpmd and some net/ driver.
+DPDK_OPTS="$DPDK_OPTS -Denable_apps=test-pmd"
+DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
# Install DPDK using prefix.
DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 8227a57487..aa2ecc5050 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -22,6 +22,9 @@ function install_dpdk()
# Export the following path for pkg-config to find the .pc file.
export PKG_CONFIG_PATH=$DPDK_LIB/pkgconfig/:$PKG_CONFIG_PATH
+# Expose dpdk binaries.
+export PATH=$(pwd)/dpdk-dir/build/bin:$PATH
+
if [ ! -f "${VERSION_FILE}" ]; then
echo "Could not find DPDK in $(pwd)/dpdk-dir"
return 1
@@ -113,7 +116,7 @@ fi
OPTS="${EXTRA_OPTS} ${OPTS} $*"
-if [ "$TESTSUITE" ]; then
+if [ "$TESTSUITE" = 'test' ]; then
# 'distcheck' will reconfigure with required options.
# Now we only need to prepare the Makefile without sparse-wrapped CC.
configure_ovs
@@ -123,6 +126,16 @@ if [ "$TESTSUITE" ]; then
TESTSUITEFLAGS=-j4 RECHECK=yes
else
build_ovs
+for testsuite in $TESTSUITE; do
+run_as_root=
+if [ "${testsuite##*dpdk}" != "$testsuite" ]; then
+sudo sh -c 'echo 1024 > /proc/sys/vm/nr_hugepages' || true
+[ "$(cat /proc/sys/vm/nr_hugepages)" = '1024' ]
+export DPDK_EAL_OPTIONS="--lcores 0@1,1@1,2@1"
+run_as_root="sudo -E PATH=$PATH"
+fi
+$run_as_root make $testsuite TESTSUITEFLAGS=-j4 RECHECK=yes
+done
fi
exit 0
diff --git a/.github/workflows/build-and-test.yml
b/.github/workflows/build-and-test.yml
index bc5494e863..4f62efb7c3 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -123,10 +123,10 @@ jobs:
opts: --enable-shared
- compiler: gcc
-testsuite:test
+testsuite:check check-dpdk
dpdk: dpdk
- compiler: clang
-testsuite:test
+testsuite:check check-dpdk
dpdk: dpdk
- compiler: gcc
@@ -213,7 +213,8 @@ jobs:
mkdir logs
cp config.log ./logs/
cp -r ./*/_build/sub/tests/testsuite.* ./logs/ || true
-tar -czvf logs.tgz logs/
+sudo cp -r ./tests/*testsuite.* ./logs/ || true
+sudo tar -czvf logs.tgz logs/
- name: upload logs on failure
if: failure() || cancelled()
diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index a176a57a4b..35d14bee8f 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -94,7 +94,7 @@ m4_define([OVS_DPDK_CHECK_TESTPMD],
m4_define([OVS_DPDK_START_TESTPMD],
[AT_CHECK([lscpu], [], [stdout])
AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3))
{printf "512,"}; print "512"}' > NUMA_NODE])
- eal_options="--in-memory --socket-mem="$(cat NUMA_NODE)"
--single-file-segments --no-pci"
+ eal_options="$DPDK_EAL_OPTIONS --in-memory --socket-mem="$(cat NUMA_NODE)"
--single-file-segments --no-pci"
options="$1"
test "$options" != "${options%% -- *}" || options="$options -- "
eal_options="$eal_options ${options%% -- *}"
--
2.41.0
[ovs-dev] [PATCH v8 2/9] system-dpdk: Don't require hugetlbfs.
dpdk-testpmd does not need hugetlbfs backing as we don't require
multiprocess support in OVS unit tests.
Switch to --in-memory and remove the (then unneeded) check on
hugetlbfs presence.
Acked-by: Aaron Conole
Acked-by: Eelco Chaudron
Signed-off-by: David Marchand
---
Changes since v7:
- reverted use of dynamic allocations and kept initial memory
reservation: this avoids random failures when testpmd and ovs-dpdk
try to allocate memory at the same time,
---
tests/system-dpdk-macros.at | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index 2cfd26d840..a176a57a4b 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -7,9 +7,6 @@ m4_define([OVS_DPDK_PRE_CHECK],
[dnl Check Hugepages
AT_CHECK([cat /proc/meminfo], [], [stdout])
AT_SKIP_IF([grep -E 'HugePages_Free: *0' stdout], [], [stdout])
- AT_CHECK([mount], [], [stdout])
- AT_CHECK([grep 'hugetlbfs' stdout], [], [stdout], [])
-
])
@@ -97,7 +94,7 @@ m4_define([OVS_DPDK_CHECK_TESTPMD],
m4_define([OVS_DPDK_START_TESTPMD],
[AT_CHECK([lscpu], [], [stdout])
AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3))
{printf "512,"}; print "512"}' > NUMA_NODE])
- eal_options="--socket-mem="$(cat NUMA_NODE)" --file-prefix page0
--single-file-segments --no-pci"
+ eal_options="--in-memory --socket-mem="$(cat NUMA_NODE)"
--single-file-segments --no-pci"
options="$1"
test "$options" != "${options%% -- *}" || options="$options -- "
eal_options="$eal_options ${options%% -- *}"
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v8 1/9] system-dpdk: Introduce helpers for testpmd.
Rather than copy/paste everywhere, introduce helpers to control
testpmd runs.
Rely on --stats-period (which outputs port stats every n seconds) so that
testpmd keeps running without expecting any user input.
Acked-by: Aaron Conole
Acked-by: Eelco Chaudron
Signed-off-by: David Marchand
---
Changes since v7:
- fixed -- detection in arguments passed to OVS_DPDK_START_TESTPMD,
- wrote the testpmd command to a debug file,
Changes since v1:
- fixed OVS_DPDK_START_TESTPMD passed arguments evaluation:: $@ -> $1,
---
tests/system-dpdk-macros.at | 38 +
tests/system-dpdk.at| 103 +---
2 files changed, 62 insertions(+), 79 deletions(-)
diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index 3920f08a5e..2cfd26d840 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -79,3 +79,41 @@ m4_define([OVS_DPDK_START_VSWITCHD],
AT_CAPTURE_FILE([ovs-vswitchd.log])
on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
])
+
+
+# OVS_DPDK_CHECK_TESTPMD()
+#
+# Check dpdk-testpmd availability.
+#
+m4_define([OVS_DPDK_CHECK_TESTPMD],
+ [AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null])
+])
+
+
+# OVS_DPDK_START_TESTPMD()
+#
+# Start dpdk-testpmd in background.
+#
+m4_define([OVS_DPDK_START_TESTPMD],
+ [AT_CHECK([lscpu], [], [stdout])
+ AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3))
{printf "512,"}; print "512"}' > NUMA_NODE])
+ eal_options="--socket-mem="$(cat NUMA_NODE)" --file-prefix page0
--single-file-segments --no-pci"
+ options="$1"
+ test "$options" != "${options%% -- *}" || options="$options -- "
+ eal_options="$eal_options ${options%% -- *}"
+ testpmd_options="-a --stats-period 2 ${options#* -- }"
+ echo "dpdk-testpmd $eal_options -- $testpmd_options" >testpmd.cmd
+ dpdk-testpmd $eal_options -- $testpmd_options >testpmd.log 2>&1 & \
+ echo $! > testpmd.pid
+ on_exit "kill -9 `cat testpmd.pid`"
+])
+
+
+# OVS_DPDK_STOP_TESTPMD()
+#
+# Stop background dpdk-testpmd.
+#
+m4_define([OVS_DPDK_STOP_TESTPMD],
+ [AT_CHECK([kill `cat testpmd.pid`])
+ OVS_WAIT([kill -0 `cat testpmd.pid`], [kill -9 `cat testpmd.pid`])
+])
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 0f58e85742..270587e2c0 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -97,13 +97,9 @@ dnl Ping vhost-user port
AT_SETUP([OVS-DPDK - ping vhost-user ports])
AT_KEYWORDS([dpdk])
OVS_DPDK_PRE_CHECK()
-AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null])
+OVS_DPDK_CHECK_TESTPMD()
OVS_DPDK_START([--no-pci])
-dnl Find number of sockets
-AT_CHECK([lscpu], [], [stdout])
-AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3))
{printf "512,"}; print "512"}' > NUMA_NODE])
-
dnl Add userspace bridge and attach it to OVS
AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set Interface
dpdkvhostuser0 \
@@ -125,12 +121,8 @@ ADD_NAMESPACES(ns1, ns2)
dnl Add veth device
ADD_VETH(tap1, ns2, br10, "172.31.110.12/24")
-dnl Execute testpmd in background
-on_exit "pkill -f -x -9 'tail -f /dev/null'"
-tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
- --vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostuser0" \
- --vdev="net_tap0,iface=tap0" --file-prefix page0 \
- --single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuser0.log 2>&1 &
+OVS_DPDK_START_TESTPMD([--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostuser0"
\
+--vdev="net_tap0,iface=tap0"])
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
OVS_WAIT_UNTIL([ip link show dev tap0 | grep -qw LOWER_UP])
@@ -151,8 +143,7 @@ AT_CHECK([ip netns exec ns2 ip link show], [], [stdout],
[stderr])
AT_CHECK([ip netns exec ns1 ping -c 4 -I tap0 172.31.110.12], [], [stdout],
[stderr])
-dnl Clean up the testpmd now
-pkill -f -x -9 'tail -f /dev/null'
+OVS_DPDK_STOP_TESTPMD()
dnl Wait for vhost-user handling the socket disconnect.
OVS_WAIT_UNTIL([grep "vHost Device '$OVS_RUNDIR/dpdkvhostuser0' has been
removed" ovs-vswitchd.log])
@@ -173,13 +164,9 @@ dnl Ping vhost-user-client port
AT_SETUP([OVS-DPDK - ping vhost-user-client ports])
AT_KEYWORDS([dpdk])
OVS_DPDK_PRE_CHECK()
-AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null])
+OVS_DPDK_CHECK_TESTPMD()
OVS_DPDK_START([--no-pci])
-dnl Find number of sockets
-AT_CHECK([lscpu], [], [stdout])
-AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3))
{printf "512,"}; print "512"}' > NUMA_NODE])
-
dnl Add userspace bridge and attach it to OVS
AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface \
@@ -200,13 +187,8 @@ ADD_NAMESPACES(ns1, ns2)
dnl Add veth device
ADD_VETH(tap1, ns2, br10, "172.31.110.12/24")
-dnl Execute testpmd in background
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 27/10/2023 14:38, Ilya Maximets wrote:
On 10/26/23 11:29, Jakob Meng wrote:
On 25.10.23 19:10, Ilya Maximets wrote:
On 10/24/23 11:21, Kevin Traynor wrote:
Using correct email for Simon this time
On 24/10/2023 10:19, Kevin Traynor wrote:
On 23/10/2023 10:11, Jakob Meng wrote:
On 20.10.23 12:02, Kevin Traynor wrote:
On 13/10/2023 10:07, jm...@redhat.com wrote:
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch series moves key-value pairs which are no valid options
from get_config() to the get_status() callback. The documentation in
vswitchd/vswitch.xml for status columns as well as tests have been
updated accordingly.
Compared to v4, the patch has been split into a series. Change requests
from Kevin Traynor have been incorporated, e.g. n_{r,t}xq will be
reported in dpkvhostclient status and tx-steering in the dpdk status
will be "unsupported" if the hw does not support steering traffic to
additional rxq.
The netdev dpdk classes no longer share a common get_config callback,
instead both the dpdk_class and the dpdk_vhost_client_class defines
their own callbacks.
Looks like you've lost the callback for the the vhost-user server ports.
(I noticed that in the code, but I didn't do a full review, so replying here.)
With "vhost-user server ports" you meant dpdk_vhost_class?
The get_config callback for dpdk_vhost_class has been dropped because it does
not have a set_config callback.
Ah, true. Please, add a note about that to the commit message.
For dpdk_vhost_client_class both config options
vhost-server-path and tx-retries-max are returned which were missed in
the previous patch version.
Jakob Meng (3):
netdev-dpdk: Sync and clean {get,set}_config() callbacks.
netdev-dummy: Sync and clean {get,set}_config() callbacks.
netdev-afxdp: Sync and clean {get,set}_config() callbacks.
Documentation/intro/install/afxdp.rst | 12 +--
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-afxdp.c | 21 +-
lib/netdev-afxdp.h | 1 +
lib/netdev-dpdk.c | 104 ++
lib/netdev-dummy.c | 19 -
lib/netdev-linux-private.h | 1 +
lib/netdev-linux.c | 4 +-
tests/pmd.at | 26 +++
tests/system-dpdk.at | 64 ++--
vswitchd/vswitch.xml | 25 ++-
11 files changed, 193 insertions(+), 88 deletions(-)
Hi Jakob,
The output looks good to me. Just a few minor code related comments on the code.
@previous reviewers/committers, if anyone else is intending to review before
Jakob respins for possibly the final version, please shout now!
As it is user visible change, it's probably worth to put a note in the NEWS so
users can quickly spot if they notice a change.
Best to mention the commands/output that changed ('ovs-appctl dpctl/show' and
'ovs-vsctl get Interface status') and say briefly that you've
aligned set_confg/get_config and updated status etc.
Suggest to not to bother mentioning specific netdevs and just do in one update,
maybe in first patch?
thanks,
Kevin.
Good idea. How about appending this to NEWS?
Post-v3.2.0
- OVSDB:
* ...
- ovs-appctl:
* 'ovs-appctl dpctl/show' has been changed to print valid config options
It is an appctl section, no need to repeat.
only: {configured,requested}_{rx,tx}_queues and 'rx_csum_offload' have
been dropped. Now, 'n_rxq' will be used for requested rx queues. Tx
queues cannot be changed by the user, hence 'requested_tx_queues' has
been dropped. 'lsc_interrupt_mode' has been renamed to option name
'dpdk-lsc-interrupt'.
Use 'ovs-vsctl get interface *** status' to query for values which
have
previously been returned by 'ovs-appctl dpctl/show'. For example, use
'ovs-vsctl get interface *** status:n_txq' to get what was previously
returned by 'configured_tx_queues'.
* 'ovs-appctl dpctl/show' will now print all available config options
like
'dpdk-devargs', '{rx,tx}-flow-ctrl-{autoneg}', 'vhost-server-path' and
'tx-retries-max' if they are set.
This doesn't seem to be entirely true. The flow control stuff
is always reported even if not set by the user. Should we maybe
avoid reporting default values for these somehow?
It is not handled consistently. If we decide so, we would also want to change reporting
of n_{r,t}xq_desc and all the others. Does anyone has strong opinions here? Maybe your
"maybe" was a strong opinion? 😄
The flow control is just the least used config and it is an on/o
Re: [ovs-dev] [PATCH v5] userspace: Support vxlan and geneve tso.
On Thu, Oct 26, 2023 at 3:40 AM Dexia Li via dev
wrote:
>
> For userspace datapath, this patch provides vxlan and geneve tunnel tso.
> Only support userspace vxlan or geneve tunnel, meanwhile support
> tunnel outter and inner csum offload. If netdev do not support offload
> features, there is a software fallback.If netdev do not support vxlan
> and geneve tso,packets will drop. Front-end devices can close offload
> features by ethtool also.
>
> Signed-off-by: Dexia Li
> ---
> lib/dp-packet.c | 41 +++-
> lib/dp-packet.h | 216
> lib/dpif-netdev.c | 4 +-
> lib/flow.c | 2 +-
> lib/netdev-dpdk.c | 88 ++--
> lib/netdev-dummy.c | 2 +-
> lib/netdev-native-tnl.c | 106 ++--
> lib/netdev-provider.h | 4 +
> lib/netdev.c| 35 +--
> lib/packets.c | 12 +--
> lib/packets.h | 6 +-
> tests/dpif-netdev.at| 4 +-
> 12 files changed, 461 insertions(+), 59 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index ed004c3b9..b5013da9f 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -543,16 +543,47 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct
> dp_packet *b2,
> return true;
> }
>
> +void
> +dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
> +uint64_t flags)
> +{
> +if (dp_packet_hwol_is_outer_ipv4_cksum(p)) {
> +if (!(flags & NETDEV_TX_OFFLOAD_OUTER_IP_CKSUM)) {
> +dp_packet_ip_set_header_csum(p, false);
> +dp_packet_ol_set_ip_csum_good(p);
> +dp_packet_hwol_reset_outer_ipv4_csum(p);
> +}
> +}
> +
> +if (!dp_packet_hwol_is_outer_UDP_cksum(p)) {
> +return;
> +}
> +
> +if (!(flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM)) {
> +packet_udp_complete_csum(p, false);
> +dp_packet_ol_set_l4_csum_good(p);
> +dp_packet_hwol_reset_outer_udp_csum(p);
> +}
> +}
> +
> /* Checks if the packet 'p' is compatible with netdev_ol_flags 'flags'
> * and if not, updates the packet with the software fall back. */
> void
> dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags)
> {
> +bool tnl_inner = false;
> +
> +if (dp_packet_hwol_is_tunnel_geneve(p) ||
> +dp_packet_hwol_is_tunnel_vxlan(p)) {
> +dp_packet_tnl_outer_ol_send_prepare(p, flags);
> +tnl_inner = true;
> +}
> +
> if (dp_packet_hwol_tx_ip_csum(p)) {
> if (dp_packet_ip_checksum_good(p)) {
> dp_packet_hwol_reset_tx_ip_csum(p);
> } else if (!(flags & NETDEV_TX_OFFLOAD_IPV4_CKSUM)) {
> -dp_packet_ip_set_header_csum(p);
> +dp_packet_ip_set_header_csum(p, tnl_inner);
> dp_packet_ol_set_ip_csum_good(p);
> dp_packet_hwol_reset_tx_ip_csum(p);
> }
> @@ -562,24 +593,24 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t
> flags)
> return;
> }
>
> -if (dp_packet_l4_checksum_good(p)) {
> +if (dp_packet_l4_checksum_good(p) && (!tnl_inner)) {
> dp_packet_hwol_reset_tx_l4_csum(p);
> return;
> }
>
> if (dp_packet_hwol_l4_is_tcp(p)
> && !(flags & NETDEV_TX_OFFLOAD_TCP_CKSUM)) {
> -packet_tcp_complete_csum(p);
> +packet_tcp_complete_csum(p, tnl_inner);
> dp_packet_ol_set_l4_csum_good(p);
> dp_packet_hwol_reset_tx_l4_csum(p);
> } else if (dp_packet_hwol_l4_is_udp(p)
> && !(flags & NETDEV_TX_OFFLOAD_UDP_CKSUM)) {
> -packet_udp_complete_csum(p);
> +packet_udp_complete_csum(p, tnl_inner);
> dp_packet_ol_set_l4_csum_good(p);
> dp_packet_hwol_reset_tx_l4_csum(p);
> } else if (!(flags & NETDEV_TX_OFFLOAD_SCTP_CKSUM)
> && dp_packet_hwol_l4_is_sctp(p)) {
> -packet_sctp_complete_csum(p);
> +packet_sctp_complete_csum(p, tnl_inner);
> dp_packet_ol_set_l4_csum_good(p);
> dp_packet_hwol_reset_tx_l4_csum(p);
> }
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 70ddf8aa4..80c7ab961 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -86,22 +86,47 @@ enum dp_packet_offload_mask {
> DEF_OL_FLAG(DP_PACKET_OL_TX_SCTP_CKSUM, RTE_MBUF_F_TX_SCTP_CKSUM, 0x800),
> /* Offload IP checksum. */
> DEF_OL_FLAG(DP_PACKET_OL_TX_IP_CKSUM, RTE_MBUF_F_TX_IP_CKSUM, 0x1000),
> +/* Offload packet is tunnel GENEVE. */
> +DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_GENEVE,
> +RTE_MBUF_F_TX_TUNNEL_GENEVE, 0x2000),
> +/* Offload packet is tunnel VXLAN. */
> +DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN,
> +RTE_MBUF_F_TX_TUNNEL_VXLAN, 0x4000),
> +/* Offload tunnel packet, out is ipv4 */
> +DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV4,
> +RTE_MBUF_F_TX_OUTER_IPV4, 0x8000),
> +/* Offload TUNNEL out ipv4 checksum */
> +DEF_OL_FLAG(DP_P
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
> From: Jakob Meng
>
> Add global option to output JSON from ovs-appctl cmds.
>
> This patch is an update of [0] with the following major changes:
> * The JSON-RPC API change is now backward compatible. One can use an
> updated client (ovs-appctl/dpctl) with an old server (ovs-vswitchd)
> and vice versa. Of course, JSON output only works when both are
> updated.
> * tests/pmd.at from forth patch now features an example of how the
> output looks like when a command does not support JSON output.
> * The patch has been split into a series of four. The first patch
> introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and
> necessary changes to the JSON-RPC API. It does not yet pass the
> output format to individual commands because that requires a lot
> of changes. Those changes have been split out into the third patch
> to increase readability of the series.
> * The second patch introduces equivalent changes to the Python files.
> * The third patch moves all commands to the updated functions in
> lib/unixctl.*, in particular unixctl_command_register() and the
> unixctl_cb_func type, as well as their Python counterparts. The
> output is still text-only (no json) for all commands.
> * The forth patch shows how JSON output could be implemented using
> 'dpif/show' as an example.
>
> The following paragraphs are taken from the previous patch revision
> and have been updated to changes mentioned above.
>
> For monitoring systems such as Prometheus it would be beneficial if OVS
> and OVS-DPDK would expose statistics in a machine-readable format.
> Several approaches like UNIX socket, OVSDB queries and JSON output from
> ovs-xxx tools have been proposed [2],[3]. This proof of concept
> describes one way how ovs-xxx tools could output JSON in addition to
> plain-text for humans.
>
> This patch follows an alternative approach to RFC [1] which
> implemented JSON output as a separate option for each command like
> 'dpif/show'. The option was called '-o|--output' in the latter. It
> has been renamed to '-f,--format' because ovs-appctl already has a
> short option '-o' which prints the available ovs-appctl options
> ('--option'). The new option name '-f,--format' is in line with
> ovsdb-client where it controls output formatting, too.
>
> An example call would be 'ovs-appctl --format json dpif/show' as
> shown in tests/pmd.at of the forth patch. By default, the output
> format is plain-text as before.
>
> With this patch, all commands announce their support for output
> formats when being registered with unixctl_command_register() from
> lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON.
> When a requested output format is not supported by a command, then
> process_command() in lib/unixctl.c will return an error. This is an
> advantage over the previous approach [1] where each command would have
> to parse the output format option and handle requests for unsupported
> output formats on its own.
>
> The question whether JSON output should be pretty printed and sorted
> remains. In most cases it would be unnecessary because machines
> consume the output or humans could use jq for pretty printing. However,
> it would make tests more readable (for humans) without having to use jq
> (which would require us to introduce a dependency on jq).
Hi Jakob,
I had some code-related comments on V1, which I do not see addressed or replied
to. Did you miss them? Anyway, I’ll go over my old review notes, and add them
to the split-up patch.
Cheers,
Eelco
>
> [0]
> https://patchwork.ozlabs.org/project/openvswitch/patch/20231020104320.1417664-2-jm...@redhat.com/
> [1]
> https://patchwork.ozlabs.org/project/openvswitch/patch/20231020092205.710399-2-jm...@redhat.com/
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1824861#c7
> [3]
> https://patchwork.ozlabs.org/project/openvswitch/patch/20230831131122.284913-1-rja...@redhat.com/
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng
>
> Jakob Meng (4):
> Add global option for JSON output to ovs-appctl/dpctl.
> python: Add global option for JSON output to Python tools.
> Migrate commands to extended unixctl API.
> ofproto: Add JSON output for 'dpif/show' command.
>
> lib/bfd.c | 16 ++-
> lib/cfm.c | 13 +-
> lib/command-line.c | 36 +
> lib/command-line.h | 10 ++
> lib/coverage.c | 11 +-
> lib/dpctl.c| 129 ++---
> lib/dpctl.h| 4 +
> lib/dpdk.c | 15 +-
> lib/dpif-netdev-perf.c | 1 +
> lib/dpif-netdev-perf.h | 1 +
> lib/dpif-netdev.c | 84 ++-
> lib/dpif-netlink.c | 6 +-
> lib/lacp.c | 7 +-
> lib/memory.c | 5 +-
> lib/netdev-dpdk.c | 16 ++-
> lib/netdev-dummy.c
Re: [ovs-dev] [RFC v3 2/4] python: Add global option for JSON output to Python tools.
On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
> From: Jakob Meng
>
> This patch introduces support for different output formats to the
> Python code, as did the previous commit for ovs-xxx tools like
> 'ovs-appctl --format json dpif/show'.
> In particular, tests/appctl.py gains a global option '-f,--format'
> which allows users to request JSON instead of plain-text for humans.
>
> This patch does not yet pass the choosen output format to commands.
> Doing so requires changes to all command_register() calls and all
> command callbacks. To improve readibility those changes have been
> split out into a follow up patch. Respectively, whenever an output
> format other than 'text' is choosen for tests/appctl.py, the script
> will fail.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng
Note reviewed anything yet, but I get some flake8 errors when compiling your
series:
pends.py build-aux/soexpand.py build-aux/xml2nroff' && \
flake8 $src --select=H231,H232,H233,H238 && \
flake8 $src
--ignore=E121,E123,E125,E126,E127,E128,E129,E131,E203,E722,W503,W504,F811,D,H,I
&& \
touch flake8-check
python/ovs/unixctl/__init__.py:25:50: E261 at least two spaces before inline
comment
python/ovs/unixctl/__init__.py:30:9: E265 block comment should start with '# '
python/ovs/unixctl/__init__.py:73:68: E261 at least two spaces before inline
comment
python/ovs/unixctl/server.py:114:27: F821 undefined name 'params'
python/ovs/unixctl/server.py:118:19: E111 indentation is not a multiple of 4
python/ovs/unixctl/server.py:148:51: E261 at least two spaces before inline
comment
python/ovs/unixctl/server.py:239:1: E302 expected 2 blank lines, found 1
make[1]: *** [Makefile:6795: flake8-check] Error 1
FYI if you install flake8 on your system, re-run ./configure it will pick these
up as part of your compile process (and make sure you add --enable-Werror).
//Eelco
> ---
> python/ovs/unixctl/__init__.py | 38 +++---
> python/ovs/unixctl/client.py | 20 --
> python/ovs/unixctl/server.py | 71 ++
> python/ovs/util.py | 9 +
> tests/appctl.py| 7 +++-
> 5 files changed, 112 insertions(+), 33 deletions(-)
>
> diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
> index 8ee312943..aa8969c61 100644
> --- a/python/ovs/unixctl/__init__.py
> +++ b/python/ovs/unixctl/__init__.py
> @@ -20,10 +20,14 @@ commands = {}
>
>
> class _UnixctlCommand(object):
> -def __init__(self, usage, min_args, max_args, callback, aux):
> +# TODO: Output format will be passed as 'fmt' to the command in later
> +# patch.
> +def __init__(self, usage, min_args, max_args, # fmt,
> + callback, aux):
> self.usage = usage
> self.min_args = min_args
> self.max_args = max_args
> +#self.fmt = fmt
> self.callback = callback
> self.aux = aux
>
> @@ -42,10 +46,12 @@ def _unixctl_help(conn, unused_argv, unused_aux):
> conn.reply(reply)
>
>
> -def command_register(name, usage, min_args, max_args, callback, aux):
> +def command_register_fmt(name, usage, min_args, max_args, fmt, callback,
> aux):
> """ Registers a command with the given 'name' to be exposed by the
> UnixctlServer. 'usage' describes the arguments to the command; it is used
> -only for presentation to the user in "help" output.
> +only for presentation to the user in "help" output. 'output_fmts' is a
> +bitmap that defines what output formats a command supports, e.g.
> +OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
>
> 'callback' is called when the command is received. It is passed a
> UnixctlConnection object, the list of arguments as unicode strings, and
> @@ -60,11 +66,33 @@ def command_register(name, usage, min_args, max_args,
> callback, aux):
> assert isinstance(usage, str)
> assert isinstance(min_args, int)
> assert isinstance(max_args, int)
> +assert isinstance(fmt, ovs.util.OutputFormat)
> assert callable(callback)
>
> if name not in commands:
> -commands[name] = _UnixctlCommand(usage, min_args, max_args, callback,
> - aux)
> +commands[name] = _UnixctlCommand(usage, min_args, max_args, # fmt,
> + callback, aux)
> +
> +
> +# TODO: command_register() will be replaced with command_register_fmt() in a
> +# later patch of this series. It is is kept temporarily to reduce the
> +# amount of changes in this patch.
> +def command_register(name, usage, min_args, max_args, callback, aux):
> +""" Registers a command with the given 'name' to be exposed by the
> +UnixctlServer. 'usage' describes the arguments to the command; it is used
> +only for presentation to the user in "help" output.
> +
> +'callback' is called when the command is received. It is passed a
> +UnixctlConn
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 10/26/23 11:29, Jakob Meng wrote:
> On 25.10.23 19:10, Ilya Maximets wrote:
>> On 10/24/23 11:21, Kevin Traynor wrote:
>>> Using correct email for Simon this time
>>>
>>> On 24/10/2023 10:19, Kevin Traynor wrote:
On 23/10/2023 10:11, Jakob Meng wrote:
> On 20.10.23 12:02, Kevin Traynor wrote:
>> On 13/10/2023 10:07, jm...@redhat.com wrote:
>>> From: Jakob Meng
>>>
>>> For better usability, the function pairs get_config() and
>>> set_config() for netdevs should be symmetric: Options which are
>>> accepted by set_config() should be returned by get_config() and the
>>> latter should output valid options for set_config() only.
>>>
>>> This patch series moves key-value pairs which are no valid options
>>> from get_config() to the get_status() callback. The documentation in
>>> vswitchd/vswitch.xml for status columns as well as tests have been
>>> updated accordingly.
>>>
>>> Compared to v4, the patch has been split into a series. Change requests
>>> from Kevin Traynor have been incorporated, e.g. n_{r,t}xq will be
>>> reported in dpkvhostclient status and tx-steering in the dpdk status
>>> will be "unsupported" if the hw does not support steering traffic to
>>> additional rxq.
>>> The netdev dpdk classes no longer share a common get_config callback,
>>> instead both the dpdk_class and the dpdk_vhost_client_class defines
>>> their own callbacks.
>> Looks like you've lost the callback for the the vhost-user server ports.
>> (I noticed that in the code, but I didn't do a full review, so replying
>> here.)
>
> With "vhost-user server ports" you meant dpdk_vhost_class?
>
> The get_config callback for dpdk_vhost_class has been dropped because it does
> not have a set_config callback.
Ah, true. Please, add a note about that to the commit message.
>
>>
>>> For dpdk_vhost_client_class both config options
>>> vhost-server-path and tx-retries-max are returned which were missed in
>>> the previous patch version.
>>>
>>> Jakob Meng (3):
>>> netdev-dpdk: Sync and clean {get,set}_config() callbacks.
>>> netdev-dummy: Sync and clean {get,set}_config() callbacks.
>>> netdev-afxdp: Sync and clean {get,set}_config() callbacks.
>>>
>>> Documentation/intro/install/afxdp.rst | 12 +--
>>> Documentation/topics/dpdk/phy.rst | 4 +-
>>> lib/netdev-afxdp.c | 21 +-
>>> lib/netdev-afxdp.h | 1 +
>>> lib/netdev-dpdk.c | 104
>>> ++
>>> lib/netdev-dummy.c | 19 -
>>> lib/netdev-linux-private.h | 1 +
>>> lib/netdev-linux.c | 4 +-
>>> tests/pmd.at | 26 +++
>>> tests/system-dpdk.at | 64 ++--
>>> vswitchd/vswitch.xml | 25 ++-
>>> 11 files changed, 193 insertions(+), 88 deletions(-)
>>>
>> Hi Jakob,
>>
>> The output looks good to me. Just a few minor code related comments on
>> the code.
>>
>> @previous reviewers/committers, if anyone else is intending to review
>> before Jakob respins for possibly the final version, please shout now!
>>
>> As it is user visible change, it's probably worth to put a note in the
>> NEWS so users can quickly spot if they notice a change.
>>
>> Best to mention the commands/output that changed ('ovs-appctl
>> dpctl/show' and 'ovs-vsctl get Interface status') and
>> say briefly that you've aligned set_confg/get_config and updated status
>> etc.
>>
>> Suggest to not to bother mentioning specific netdevs and just do in one
>> update, maybe in first patch?
>>
>> thanks,
>> Kevin.
> Good idea. How about appending this to NEWS?
>
> Post-v3.2.0
>
> - OVSDB:
> * ...
> - ovs-appctl:
> * 'ovs-appctl dpctl/show' has been changed to print valid config
> options
>> It is an appctl section, no need to repeat.
>>
> only: {configured,requested}_{rx,tx}_queues and
> 'rx_csum_offload' have
> been dropped. Now, 'n_rxq' will be used for requested rx queues.
> Tx
> queues cannot be changed by the user, hence
> 'requested_tx_queues' has
> been dropped. 'lsc_interrupt_mode' has been renamed to option
> name
> 'dpdk-lsc-interrupt'.
> Use 'ovs-vsctl get interface *** status' to query for values
> which have
> previously been returned by 'ovs-appctl dpctl/show'. For
> example, use
> 'ovs-vsctl get interface *** status:n_txq' to get what was
> previously
> returned by 'configured_tx_queues'.
> * 'ovs-appctl dpctl/show' will now pr
Re: [ovs-dev] [PATCH v7 8/8] system-dpdk: Run traffic tests.
On 27 Oct 2023, at 14:55, David Marchand wrote: > On Fri, Oct 27, 2023 at 1:45 PM Eelco Chaudron wrote: >> On 23 Oct 2023, at 10:18, David Marchand wrote: >> >>> Integrate system-traffic.at tests as part of check-dpdk. >>> >>> Some tests that can't work with the userspace datapath are skipped by >>> overriding some OVS_CHECK_* macros. >>> >>> ADD_VETH is implemented using the net/af_xdp DPDK driver. >>> >>> Signed-off-by: David Marchand >> >> This patch looks as good as it did last time, so >> >> Acked-by: Eelco Chaudron > > Thanks. > > >> >> I assume you will do one more revision based on some feedback on the first >> patch in the series. > > Yes, I will send one more revision. > > Apart from the comments on patch1, I have another issue to handle. > We hit some random yet frequent issues because of the previous patch > that reorders del-port and testpmd shutdown: this reordering opens a > little window during which some packets may be received by a pmd while > the port is deleted (see thread > https://mail.openvswitch.org/pipermail/ovs-dev/2023-October/408956.html). ACK, thanks for pointing me to this, as for some reason I did not have these messages :( //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 8/8] system-dpdk: Run traffic tests.
On Fri, Oct 27, 2023 at 1:45 PM Eelco Chaudron wrote: > On 23 Oct 2023, at 10:18, David Marchand wrote: > > > Integrate system-traffic.at tests as part of check-dpdk. > > > > Some tests that can't work with the userspace datapath are skipped by > > overriding some OVS_CHECK_* macros. > > > > ADD_VETH is implemented using the net/af_xdp DPDK driver. > > > > Signed-off-by: David Marchand > > This patch looks as good as it did last time, so > > Acked-by: Eelco Chaudron Thanks. > > I assume you will do one more revision based on some feedback on the first > patch in the series. Yes, I will send one more revision. Apart from the comments on patch1, I have another issue to handle. We hit some random yet frequent issues because of the previous patch that reorders del-port and testpmd shutdown: this reordering opens a little window during which some packets may be received by a pmd while the port is deleted (see thread https://mail.openvswitch.org/pipermail/ovs-dev/2023-October/408956.html). -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] editorconfig: Remove [*] section and trim_trailing_whitespace.
, Oct 27, 2023 at 14:10:
From: Jakob Meng
Wildcard sections [*] and [**] are unsafe because properties cannot be
applied safely to any filetype in general. For example, IDEs like
Visual Studio Code and KDevelop store configuration files in subfolders
like .vscode or .kdev4. Properties from wildcard sections also apply to
those files which is not safe in general.
Another example are patches created with 'git format-patch' which can
contain trailing whitespaces. When editing a patch, e.g. to fix a typo
in the title, trailing whitespaces should not be removed.
Property trim_trailing_whitespace should not be defined at all because
it is interpreted differently by editors. Some wipe whitespaces from
the whole file, others remove them from edited lines only and a few
change their behavior between releases [0]. Limiting the property to a
subset of files like *.c/*.h will not mitigate the issue:
Multiple definitions of a whitespace exist. Unicode considers a form
feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
follows this definition, causing the Kate editor identify a form feed
as a trailing whitespace and removing it from sources [3]. This breaks
patches when editors remove form feeds and thus causing broken patches
which cannot be applied cleanly.
Removing trim_trailing_whitespace will be a minor inconvienence, in
particular because utilities/checkpatch.py and thus 0-day Robot will
prevent trailing whitespaces for our definition of a whitespace.
[0]
https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
[1] https://en.wikipedia.org/wiki/Whitespace_character
[2]
https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
[3]
https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
Signed-off-by: Jakob Meng
---
.editorconfig | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/.editorconfig b/.editorconfig
index 685c72750..41ba51bf3 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -2,15 +2,18 @@
root = true
-[*]
-end_of_line = lf
-insert_final_newline = true
-trim_trailing_whitespace = true
-charset = utf-8
+# No wildcard sections [*] and [**] because properties cannot be
+# applied safely to any filetype in general.
+
+# Property trim_trailing_whitespace should not be defined at all
+# because it is interpreted differently by editors.
[*.{c,h}]
+charset = utf-8
+end_of_line = lf
indent_style = space
indent_size = 4
+insert_final_newline = true
max_line_length = 79
[include/linux/**.h]
--
2.39.2
Perfect, thanks Jakob!
Reviewed-by: Robin-Jarry
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.
On 27.10.23 12:29, Eelco Chaudron wrote:
>
> On 26 Oct 2023, at 14:34, Jakob Meng wrote:
>
>> On 26.10.23 14:21, Robin Jarry wrote:
>>> Jakob Meng, Oct 26, 2023 at 14:17:
On 26.10.23 13:52, Robin Jarry wrote:
> , Oct 26, 2023 at 13:07:
>> From: Jakob Meng
>>
>> Wildcard sections [*] and [**] are unsafe because properties cannot be
>> applied safely to any filetype in general. For example, IDEs like
>> Visual Studio Code and KDevelop store configuration files in subfolders
>> like .vscode or .kdev4. Properties from wildcard sections also apply to
>> those files which is not safe in general.
>> Another example are patches created with 'git format-patch' which can
>> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
>> in the title, trailing whitespaces should not be removed.
>>
>> Property trim_trailing_whitespace should not be defined at all because
>> it is interpreted differently by editors. Some wipe whitespaces from
>> the whole file, others remove them from edited lines only and a few
>> change their behavior between releases [0]. Limiting the property to a
>> subset of files like *.c/*.h will not mitigate the issue:
>>
>> Multiple definitions of a whitespace exist. Unicode considers a form
>> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
>> follows this definition, causing the Kate editor identify a form feed
>> as a trailing whitespace and removing it from sources [3]. This breaks
>> patches when editors remove form feeds and thus causing broken patches
>> which cannot be applied cleanly.
>>
>> Removing trim_trailing_whitespace will be a minor inconvienence, in
>> particular because utilities/checkpatch.py and thus 0-day Robot will
>> prevent trailing whitespaces for our definition of a whitespace.
>>
>> [0]
>> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
>> [1] https://en.wikipedia.org/wiki/Whitespace_character
>> [2]
>> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
>> [3]
>> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>>
>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>>
>> Signed-off-by: Jakob Meng
>> ---
>> .editorconfig | 34 +-
>> 1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/.editorconfig b/.editorconfig
>> index 685c72750..aebcf3a72 100644
>> --- a/.editorconfig
>> +++ b/.editorconfig
>> @@ -2,47 +2,71 @@
>>
>> root = true
>>
>> -[*]
>> -end_of_line = lf
>> -insert_final_newline = true
>> -trim_trailing_whitespace = true
>> -charset = utf-8
> Hi Jakob,
>
> I think you could keep these two options:
>
> end_of_line = lf
> charset = utf-8
>
You cannot decide this in general for all possible filetypes across all
possible dev platforms. Again, those properties in [*] would also apply to
non-ovs-owned files inside the source tree, e.g. created by IDEs. Unsafe.
Please don't.
>>> Ok fair enough.
>>>
> And probably adding insert_final_newline = true is not necessary. >
> checkpatch should complain if the final newline is missing.
I left it in .editorconfig because it is not causing trouble. But I can
remove it, if you want.
>>> I don't think it is important you can leave it or remove it.
>>>
>>> However I just realized that you could simply copy the settings in the
>>> [*.{c,h}] section as other sections will inherit from it unless they
>>> override something.
>> Oh, nice! IIUC this is actually covered by the EditorConfig spec [0]:
>>
>> "All found EditorConfig files are searched for sections with section names
>> matching the given filename."
>>
>> and
>>
>> "Files are read top to bottom and the most recent rules found take
>> precedence. If multiple EditorConfig files have matching sections, the rules
>> from the closer EditorConfig file are read last, so pairs in closer files
>> take precedence."
>>
>> and
>>
>> "For any pair, a value of unset removes the effect of that pair, even if it
>> has been set before. For example, add indent_size = unset to undefine the
>> indent_size pair (and use editor defaults)."
>>
>> The latter would not make sense if you could not inherit properties from
>> other sections.
>>
>> [0] https://spec.editorconfig.org/
> So I guess we can expect a new rev. Will mark this as changes requested for
> now.
Updated patch is out:
https://patchwork.ozlabs.org/project/openvswitch/patch/20231027121040.26751-1-jm...@redhat.com/
Only change is that properties get inherited as discussed above.
>
> Your patch should only remove insert_final_newline and >
> trim_trailing_whitespace from the de
Re: [ovs-dev] [PATCH v3] editorconfig: Remove [*] section and trim_trailing_whitespace.
On 27 Oct 2023, at 14:10, jm...@redhat.com wrote:
> From: Jakob Meng
>
> Wildcard sections [*] and [**] are unsafe because properties cannot be
> applied safely to any filetype in general. For example, IDEs like
> Visual Studio Code and KDevelop store configuration files in subfolders
> like .vscode or .kdev4. Properties from wildcard sections also apply to
> those files which is not safe in general.
> Another example are patches created with 'git format-patch' which can
> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
> in the title, trailing whitespaces should not be removed.
>
> Property trim_trailing_whitespace should not be defined at all because
> it is interpreted differently by editors. Some wipe whitespaces from
> the whole file, others remove them from edited lines only and a few
> change their behavior between releases [0]. Limiting the property to a
> subset of files like *.c/*.h will not mitigate the issue:
>
> Multiple definitions of a whitespace exist. Unicode considers a form
> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
> follows this definition, causing the Kate editor identify a form feed
> as a trailing whitespace and removing it from sources [3]. This breaks
> patches when editors remove form feeds and thus causing broken patches
> which cannot be applied cleanly.
>
> Removing trim_trailing_whitespace will be a minor inconvienence, in
> particular because utilities/checkpatch.py and thus 0-day Robot will
> prevent trailing whitespaces for our definition of a whitespace.
>
> [0]
> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
> [1] https://en.wikipedia.org/wiki/Whitespace_character
> [2]
> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
> [3]
> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>
> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>
> Signed-off-by: Jakob Meng
Thanks Jakob for fixing this! The changes look good to me.
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] editorconfig: Remove [*] section and trim_trailing_whitespace.
From: Jakob Meng
Wildcard sections [*] and [**] are unsafe because properties cannot be
applied safely to any filetype in general. For example, IDEs like
Visual Studio Code and KDevelop store configuration files in subfolders
like .vscode or .kdev4. Properties from wildcard sections also apply to
those files which is not safe in general.
Another example are patches created with 'git format-patch' which can
contain trailing whitespaces. When editing a patch, e.g. to fix a typo
in the title, trailing whitespaces should not be removed.
Property trim_trailing_whitespace should not be defined at all because
it is interpreted differently by editors. Some wipe whitespaces from
the whole file, others remove them from edited lines only and a few
change their behavior between releases [0]. Limiting the property to a
subset of files like *.c/*.h will not mitigate the issue:
Multiple definitions of a whitespace exist. Unicode considers a form
feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
follows this definition, causing the Kate editor identify a form feed
as a trailing whitespace and removing it from sources [3]. This breaks
patches when editors remove form feeds and thus causing broken patches
which cannot be applied cleanly.
Removing trim_trailing_whitespace will be a minor inconvienence, in
particular because utilities/checkpatch.py and thus 0-day Robot will
prevent trailing whitespaces for our definition of a whitespace.
[0]
https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
[1] https://en.wikipedia.org/wiki/Whitespace_character
[2]
https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
[3]
https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
Signed-off-by: Jakob Meng
---
.editorconfig | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/.editorconfig b/.editorconfig
index 685c72750..41ba51bf3 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -2,15 +2,18 @@
root = true
-[*]
-end_of_line = lf
-insert_final_newline = true
-trim_trailing_whitespace = true
-charset = utf-8
+# No wildcard sections [*] and [**] because properties cannot be
+# applied safely to any filetype in general.
+
+# Property trim_trailing_whitespace should not be defined at all
+# because it is interpreted differently by editors.
[*.{c,h}]
+charset = utf-8
+end_of_line = lf
indent_style = space
indent_size = 4
+insert_final_newline = true
max_line_length = 79
[include/linux/**.h]
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 8/8] system-dpdk: Run traffic tests.
On 23 Oct 2023, at 10:18, David Marchand wrote: > Integrate system-traffic.at tests as part of check-dpdk. > > Some tests that can't work with the userspace datapath are skipped by > overriding some OVS_CHECK_* macros. > > ADD_VETH is implemented using the net/af_xdp DPDK driver. > > Signed-off-by: David Marchand This patch looks as good as it did last time, so Acked-by: Eelco Chaudron I assume you will do one more revision based on some feedback on the first patch in the series. //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 7/8] system-dpdk: Rework cleanup for vhost-user client tests.
On 23 Oct 2023, at 10:18, David Marchand wrote: > Those tests are subject to a race when a testpmd hosting the vhost-user > server is stopped and OVS has enough time to detect the vhost-user socket > drop and tries to reconnect to this socket. > > In such a situation, the tests can fail as the OVS process with the > vhost-user client port complains with a warning log: > > 2023-09-08T13:15:18.160Z|00163|dpdk|INFO|VHOST_CONFIG: > (.../005/dpdkvhostclient0) vhost peer closed > 2023-09-08T13:15:18.160Z|00164|netdev_dpdk|INFO|vHost Device > '.../005/dpdkvhostclient0' connection has been destroyed > 2023-09-08T13:15:18.160Z|00165|dpdk|INFO|VHOST_CONFIG: > (.../005/dpdkvhostclient0) vhost-user client: socket created, fd: 24 > 2023-09-08T13:15:18.160Z|00166|dpdk|WARN|VHOST_CONFIG: > (.../005/dpdkvhostclient0) failed to connect: Connection refused > 2023-09-08T13:15:18.160Z|00167|dpdk|INFO|VHOST_CONFIG: > (.../005/dpdkvhostclient0) reconnecting... > > Invert the order of the cleanup steps. > > Signed-off-by: David Marchand Thanks for fixing my problems ;) Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] userspace: Support vxlan and geneve tso.
On 10/26/23 09:39, Dexia Li via dev wrote: > For userspace datapath, this patch provides vxlan and geneve tunnel tso. > Only support userspace vxlan or geneve tunnel, meanwhile support > tunnel outter and inner csum offload. If netdev do not support offload > features, there is a software fallback.If netdev do not support vxlan > and geneve tso,packets will drop. Front-end devices can close offload > features by ethtool also. > > Signed-off-by: Dexia Li > --- > lib/dp-packet.c | 41 +++- > lib/dp-packet.h | 216 > lib/dpif-netdev.c | 4 +- > lib/flow.c | 2 +- > lib/netdev-dpdk.c | 88 ++-- > lib/netdev-dummy.c | 2 +- > lib/netdev-native-tnl.c | 106 ++-- > lib/netdev-provider.h | 4 + > lib/netdev.c| 35 +-- > lib/packets.c | 12 +-- > lib/packets.h | 6 +- > tests/dpif-netdev.at| 4 +- > 12 files changed, 461 insertions(+), 59 deletions(-) Hi, Dexia Li. Thanks for working on this! I didn't review the patch and I'm not sure if Mike will have more comments, but the current version still breaks system tests. You may see the report from the Intel CI in patchwork. It looks like packets are not delivered anymore via ipv6 tunnels. Also, you may run these tests yourself with (as root): # make check-system-userspace It is the main testsuite that Intel CI is running. You may also check: # make check-dpdk # make check-system-tso These are the most relevant to your patch. You may find some more information here: https://docs.openvswitch.org/en/latest/topics/testing/ Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.
On 26 Oct 2023, at 14:34, Jakob Meng wrote:
> On 26.10.23 14:21, Robin Jarry wrote:
>> Jakob Meng, Oct 26, 2023 at 14:17:
>>> On 26.10.23 13:52, Robin Jarry wrote:
, Oct 26, 2023 at 13:07:
> From: Jakob Meng
>
> Wildcard sections [*] and [**] are unsafe because properties cannot be
> applied safely to any filetype in general. For example, IDEs like
> Visual Studio Code and KDevelop store configuration files in subfolders
> like .vscode or .kdev4. Properties from wildcard sections also apply to
> those files which is not safe in general.
> Another example are patches created with 'git format-patch' which can
> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
> in the title, trailing whitespaces should not be removed.
>
> Property trim_trailing_whitespace should not be defined at all because
> it is interpreted differently by editors. Some wipe whitespaces from
> the whole file, others remove them from edited lines only and a few
> change their behavior between releases [0]. Limiting the property to a
> subset of files like *.c/*.h will not mitigate the issue:
>
> Multiple definitions of a whitespace exist. Unicode considers a form
> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
> follows this definition, causing the Kate editor identify a form feed
> as a trailing whitespace and removing it from sources [3]. This breaks
> patches when editors remove form feeds and thus causing broken patches
> which cannot be applied cleanly.
>
> Removing trim_trailing_whitespace will be a minor inconvienence, in
> particular because utilities/checkpatch.py and thus 0-day Robot will
> prevent trailing whitespaces for our definition of a whitespace.
>
> [0]
> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
> [1] https://en.wikipedia.org/wiki/Whitespace_character
> [2]
> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
> [3]
> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>
> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>
> Signed-off-by: Jakob Meng
> ---
> .editorconfig | 34 +-
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/.editorconfig b/.editorconfig
> index 685c72750..aebcf3a72 100644
> --- a/.editorconfig
> +++ b/.editorconfig
> @@ -2,47 +2,71 @@
>
> root = true
>
> -[*]
> -end_of_line = lf
> -insert_final_newline = true
> -trim_trailing_whitespace = true
> -charset = utf-8
Hi Jakob,
I think you could keep these two options:
end_of_line = lf
charset = utf-8
>>>
>>> You cannot decide this in general for all possible filetypes across all
>>> possible dev platforms. Again, those properties in [*] would also apply to
>>> non-ovs-owned files inside the source tree, e.g. created by IDEs. Unsafe.
>>> Please don't.
>>
>> Ok fair enough.
>>
And probably adding insert_final_newline = true is not necessary. >
checkpatch should complain if the final newline is missing.
>>>
>>> I left it in .editorconfig because it is not causing trouble. But I can
>>> remove it, if you want.
>>
>> I don't think it is important you can leave it or remove it.
>>
>> However I just realized that you could simply copy the settings in the
>> [*.{c,h}] section as other sections will inherit from it unless they
>> override something.
>
> Oh, nice! IIUC this is actually covered by the EditorConfig spec [0]:
>
> "All found EditorConfig files are searched for sections with section names
> matching the given filename."
>
> and
>
> "Files are read top to bottom and the most recent rules found take
> precedence. If multiple EditorConfig files have matching sections, the rules
> from the closer EditorConfig file are read last, so pairs in closer files
> take precedence."
>
> and
>
> "For any pair, a value of unset removes the effect of that pair, even if it
> has been set before. For example, add indent_size = unset to undefine the
> indent_size pair (and use editor defaults)."
>
> The latter would not make sense if you could not inherit properties from
> other sections.
>
> [0] https://spec.editorconfig.org/
So I guess we can expect a new rev. Will mark this as changes requested for now.
>>
Your patch should only remove insert_final_newline and >
trim_trailing_whitespace from the default section.
>>>
>>>
> +# No wildcard sections [*] and [**] because properties cannot be
> +# applied safely to any filetype in general.
> +
> +# Property trim_trailing_whitespace should not be defined at all
> +# because it is interpreted differently by editors.
>
> [*.{c,h}]
>>
Re: [ovs-dev] [PATCH] python: Remove duplicate UnixctlClient implementation.
On 26 Oct 2023, at 14:10, jm...@redhat.com wrote:
> From: Jakob Meng
>
> The unixctl implementation in Python has been split into three parts in
> the past. During this process the UnixctlClient was duplicated, in
> python/ovs/unixctl/client.py and python/ovs/unixctl/server.py. This
> patch removes the duplicate from the latter.
>
> Fixes: 53cf9963ccc6 ("python: Break unixctl implementation into re...")
> Signed-off-by: Jakob Meng
Removing this duplicate code seems a reasonable thing to do ;)
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/2] vswitch.xml: Add entry for dpdkvhostuser userspace-tso.
On 23 Oct 2023, at 11:41, Kevin Traynor wrote:
> get_status for dpdkvhostuser(/client) netdev class may display
> userspace-tso status.
>
> Fixes: a5669fd51c9b ("netdev-dpdk: Drop TSO in case of conflicting virtio
> features.")
> Signed-off-by: Kevin Traynor
Thanks for updating this! The changes look good to me.
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/2] vswitch.xml: Add dpdkvhostuser group status.
On 23 Oct 2023, at 11:41, Kevin Traynor wrote:
> Add group for dpdkvhostuser(/client) netdev.
>
> Adding as a single group as they display the same status,
> one of which is 'mode' to indicate if it's client or server.
>
> Fixes: b2e8b12f8a82 ("netdev-dpdk: add vhost-user get_status.")
> Signed-off-by: Kevin Traynor
Thanks for updating this! The changes look good to me.
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/3] binding: handle pb->chassis and pb->up from if-status module
Hi Mark
Thanks for the review and the comments.
On Mon, Sep 25, 2023 at 9:48 PM Mark Michelson wrote:
>
> Hi Xavier,
>
> Functionality-wise, this looks good. I have a couple of small comments
> though.
>
> On 9/20/23 11:45, Xavier Simonart wrote:
> > Before this patch, if-status module handled pb->chassis and pb->up
> > for vif ports, but not for non-VIF ports.
> > This caused a few potential issues:
> > - It was difficult to check that a no-vif port has been previously claimed
> >as there was no state in if-status. Hence it was difficult to always
> > release
> >such a port when pb->chassis was set to a different chassis.
> >This issue will be fixed in a future patch.
> > - Releasing such ports caused ovn-controller to log warnings such as
> >"Trying to delete unknown ports".
> > - If sb is readonly at the time of the claim, it forced the module to
> > recompute.
> >
> > This patch fixed issues two and three by moving the work of setting
> > pb->chassis
> > and pb->up to the if-status module.
> >
> > Signed-off-by: Xavier Simonart
> >
> > ---
> > v2: Avoid clearing iface if already deleted
> > ---
> > controller/binding.c| 122 -
> > controller/binding.h| 18 +
> > controller/if-status.c | 130
> > controller/if-status.h | 9 ++-
> > controller/ovn-controller.c | 6 +-
> > tests/ovn.at| 94 ++
> > 6 files changed, 318 insertions(+), 61 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index dbd2d52b7..92ca3ebbe 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -1200,7 +1200,7 @@ local_binding_set_pb(struct shash *local_bindings,
> > const char *pb_name,
> >* Updates the 'pb->up' field only if it's explicitly set to 'false'.
> >* This is to ensure compatibility with older versions of ovn-northd.
> >*/
> > -static bool
> > +bool
> > claimed_lport_set_up(const struct sbrec_port_binding *pb,
> >const struct sbrec_port_binding *parent_pb,
> >bool sb_readonly)
> > @@ -1213,6 +1213,8 @@ claimed_lport_set_up(const struct sbrec_port_binding
> > *pb,
> > if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> > if (!sb_readonly) {
> > if (pb->n_up) {
> > +VLOG_INFO("Setting lport %s up in Southbound",
> > + pb->logical_port);
> > sbrec_port_binding_set_up(pb, &up, 1);
> > }
> > } else if (pb->n_up && !pb->up[0]) {
> > @@ -1347,39 +1349,26 @@ claim_lport(const struct sbrec_port_binding *pb,
> > }
> > update_tracked = true;
> >
> > -if (!notify_up) {
> > -if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> > -return false;
> > -}
> > -if (sb_readonly) {
> > -return false;
> > -}
> > -set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > -} else {
> > -if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > iface_rec,
> > - sb_readonly, can_bind);
> > -}
> > +if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
> > + sb_readonly, can_bind, notify_up,
> > + parent_pb);
> > register_claim_timestamp(pb->logical_port, now);
> > sset_find_and_delete(postponed_ports, pb->logical_port);
> > } else {
> > -if (!notify_up) {
> > -if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> > -return false;
> > -}
> > -} else {
> > -if ((pb->n_up && !pb->up[0]) ||
> > -!smap_get_bool(&iface_rec->external_ids,
> > - OVN_INSTALLED_EXT_ID, false)) {
> > -if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > - iface_rec, sb_readonly,
> > - can_bind);
> > -}
> > +if ((pb->n_up && !pb->up[0]) ||
> > +(notify_up && !smap_get_bool(&iface_rec->external_ids,
> > + OVN_INSTALLED_EXT_ID, false))) {
> > +if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > + iface_rec, sb_readonly,
> > + can_bind, notify_up,
> > + parent_pb);
> > }
> > }
> > } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> > if (!is_additional_chassis(pb, chassis_rec)) {
> >
[ovs-dev] [BUG] [ofpacts_equal_stringwise] ovs-ofctl replace xxx will cause flow_mod even if openflow is same
Hi all, I'm using ovs-dpdk version 2.17.1. Now I found ovs-ofctl replace xxx will cause flow_mod even if openflow is same. Detail: ``` [root@localhost ~]# cat flows in_port="p0",dl_vlan=11,ip,nw_proto=17,actions=output:vf2 [root@localhost ~]# cat flows2 in_port="p0",dl_vlan=11,ip,nw_proto=17,actions=mod_vlan_pcp(2),output:vf2 [root@localhost ~]# cat flows3 in_port="p0",dl_vlan=11,ip,nw_proto=17,actions=set_field:2->vlan_pcp,output:vf2 [root@localhost ~]# ovs-ofctl del-flows br-phy [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows # 1st add this openflow [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows # then add this openflow again # then this command will NOT cause flow_mods [root@localhost ~]# ovs-ofctl del-flows br-phy [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows2 # 1st add this openflow [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows2 # then add this openflow again # This will cause flow_mods # log: xxx|connmgr|INFO|br-phy<->unix#48: 1 flow_mods in the last 0 s (1 adds) [root@localhost ~]# ovs-ofctl del-flows br-phy [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows3 # 1st add this openflow [root@localhost ~]# ovs-ofctl -O OpenFlow13 replace-flows br-phy flows3 # then add this openflow again # this will NOT cause flow_mods [root@localhost ~]# ovs-ofctl replace-flows br-phy flows3 # This will cause flow_mods # log: xxx|connmgr|INFO|br-phy<->unix#48: 1 flow_mods in the last 0 s (1 adds) ``` The reason cause flow_mods is because these function, fte_version_equals - ofpacts_equal_stringwise . Which is different openflow version process between mod and set_field actions. As the flow_mods will cause ovs-vswitchd to delete megaflow and then add same megaflow again. So the rte_flow rule which is offload by this megaflow will delete and add. Then the network flow will interrupt and miss upcall. Is this a bug? As the openflow controller is NOT controlled by us. Which means the command is NOT controlled by us. But the ovs-ofctl and ovs-vswitchd is code by us. So how to deal with this? Is a good idea to change the behavior of fte_version_equals - ofpacts_equal_stringwise, which could process all version? Thank you~ Simon Jones ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 0/5] Fix some of Clang's static analyzer warnings.
On 10/23/23 16:21, Eelco Chaudron wrote: > This series fixes some of Clang's scan-build warnings reported. > > v2: >- Split patch two into two, i.e. put the actual fix in a separate patch. >- Updated commit messages. > > Eelco Chaudron (5): > general: Fix Clang's static analyzer 'Dead initialization' warnings. > general: Fix Clang's static analyzer 'Dead assignment' warnings. > ofp-table: Fix count_common_prefix_run() function. > ovsdb: Fix Clang's static analyzer 'func null dereference' warnings. > netdev-offload: Fix Clang's static analyzer 'Division by zero' warnings. > > > lib/dpif-netdev.c | 4 > lib/meta-flow.c | 4 ++-- > lib/netdev-offload.c| 3 ++- > lib/ofp-actions.c | 8 +--- > lib/ofp-monitor.c | 2 +- > lib/ofp-table.c | 2 +- > ovsdb/mutation.c| 14 -- > tests/test-id-fpool.c | 2 +- > tests/test-mpsc-queue.c | 2 +- > 9 files changed, 29 insertions(+), 12 deletions(-) Thanks, Eelco! For the set: Acked-by: Ilya Maximets Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Use dirhtml builder.
On 10/27/23 11:04, David Marchand wrote:
> On Thu, Oct 26, 2023 at 7:54 PM Ilya Maximets wrote:
>>
>> We used this builder before, but from the project configuration
>> on the website. ReadTheDocs doesn't allow to change it there
>> anymore and it doesn't allow to see the full name of the previously
>> used builder (!!), so I failed to migrate it to the config file.
>>
>> The result is that older link like:
>> https://docs.openvswitch.org/en/latest/howto/dpdk/
>> Now require .html:
>> https://docs.openvswitch.org/en/latest/howto/dpdk.html
>>
>> Fixing now by switching the builder back.
>>
>> Fixes: e388bd73b70d ("readthedocs: Add the configuration file.")
>> Reported-by: Antonin Bas
>> Reported-by: David Marchand
>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/310
>> Signed-off-by: Ilya Maximets
>> ---
>>
>> The version of the docs with the change applied can be
>> temporarily seen here:
>>https://igsilya-ovs.readthedocs.io/en/latest/
>>
>> .readthedocs.yaml | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/.readthedocs.yaml b/.readthedocs.yaml
>> index e481e64f1..7d505150e 100644
>> --- a/.readthedocs.yaml
>> +++ b/.readthedocs.yaml
>> @@ -14,6 +14,7 @@ build:
>> # Build documentation in the "Documentation/" directory with Sphinx.
>> sphinx:
>>configuration: Documentation/conf.py
>> + builder: "dirhtml"
>>
>> # Build all formats: HTML, PDF, ePub.
>> formats: all
>> --
>> 2.41.0
>>
>
> Reviewed-by: David Marchand
Thanks, Antonin, Eelco and David! Applied to master and 3.2.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Use dirhtml builder.
On Thu, Oct 26, 2023 at 7:54 PM Ilya Maximets wrote:
>
> We used this builder before, but from the project configuration
> on the website. ReadTheDocs doesn't allow to change it there
> anymore and it doesn't allow to see the full name of the previously
> used builder (!!), so I failed to migrate it to the config file.
>
> The result is that older link like:
> https://docs.openvswitch.org/en/latest/howto/dpdk/
> Now require .html:
> https://docs.openvswitch.org/en/latest/howto/dpdk.html
>
> Fixing now by switching the builder back.
>
> Fixes: e388bd73b70d ("readthedocs: Add the configuration file.")
> Reported-by: Antonin Bas
> Reported-by: David Marchand
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/310
> Signed-off-by: Ilya Maximets
> ---
>
> The version of the docs with the change applied can be
> temporarily seen here:
>https://igsilya-ovs.readthedocs.io/en/latest/
>
> .readthedocs.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/.readthedocs.yaml b/.readthedocs.yaml
> index e481e64f1..7d505150e 100644
> --- a/.readthedocs.yaml
> +++ b/.readthedocs.yaml
> @@ -14,6 +14,7 @@ build:
> # Build documentation in the "Documentation/" directory with Sphinx.
> sphinx:
>configuration: Documentation/conf.py
> + builder: "dirhtml"
>
> # Build all formats: HTML, PDF, ePub.
> formats: all
> --
> 2.41.0
>
Reviewed-by: David Marchand
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [dpdk-latest] sparse: Fix build with DPDK v23.11-rc1.
On 01/09/2023 13:19, Simon Horman wrote: On Thu, Aug 31, 2023 at 09:32:23AM +0200, David Marchand wrote: After DPDK started relying on compiler intrinsics in rte_cycles.h, sparse raises the following warning: libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I ../../include/sparse -I ../../include -m64 -I /usr/local/include -I /usr/include/x86_64-linux-gnu " cgcc -target=x86_64 -target=host_os_specs -D__MMX__=1 -D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE2__=1 -D__SSE__=1 -DHAVE_CONFIG_H -I. -I../.. -I ../../include -I ./include -I ../../lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow -Wmultistatement-macros -Wcast-align=strict -mssse3 -include rte_config.h -mrtm -I/home/runner/work/ovs/ovs/dpdk-dir/include -Werror -D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/netdev-dpdk.lo -MD -MP -MF lib/.deps/netdev-dpdk.Tpo -c ../../lib/netdev-dpdk.c -o lib/netdev-dpdk.o ../../lib/netdev-dpdk.c: note: in included file (through /usr/lib/gcc/x86_64-linux-gnu/9//include/x86intrin.h, /home/runner/work/ovs/ovs/dpdk-dir/include/rte_cycles.h, /home/runner/work/ovs/ovs/dpdk-dir/include/rte_spinlock.h, ...): /usr/lib/gcc/x86_64-linux-gnu/9//include/ia32intrin.h:114:10: error: undefined identifier '__builtin_ia32_rdtsc' Provide an empty implementation of __builtin_ia32_rdtsc() builtin. Signed-off-by: David Marchand --- Note: I am sending this early, but please wait before merging this in dpdk-latest as v23.11-rc1 is far from being ready. I am expecting more changes in EAL headers and I'll update this patch if hitting more issues. Hi David, It seems appropriate to mark this as deffered in patchwork, so I have done so. Patch looks find to me. Acked-by: Simon Horman Thanks David and Simon. Updated title of the commit to "sparse: Add some compiler intrinsics for DPDK build." after talking to David, as there was a (now fixed) unrelated build issue at the time of DPDK v23.11-rc1. Rebased dpdk-latest branch and applied this commit there. Kevin. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Use dirhtml builder.
On 26 Oct 2023, at 19:55, Ilya Maximets wrote:
> We used this builder before, but from the project configuration
> on the website. ReadTheDocs doesn't allow to change it there
> anymore and it doesn't allow to see the full name of the previously
> used builder (!!), so I failed to migrate it to the config file.
>
> The result is that older link like:
> https://docs.openvswitch.org/en/latest/howto/dpdk/
> Now require .html:
> https://docs.openvswitch.org/en/latest/howto/dpdk.html
>
> Fixing now by switching the builder back.
>
> Fixes: e388bd73b70d ("readthedocs: Add the configuration file.")
> Reported-by: Antonin Bas
> Reported-by: David Marchand
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/310
> Signed-off-by: Ilya Maximets
The changes look good to me. Verified your generated document and generated a
set locally.
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
