Re: [ovs-dev] [PATCH 0/2] Patches to branch for 3.4.
On 15.07.24 13:23, Ilya Maximets wrote: > The plan is to branch by the end of today. There is still a couple of > patch sets on the list that can / will be applied before that. > > Ilya Maximets (2): > Prepare for 3.4.0. > Prepare for post-3.4.0 (3.4.90). > > Documentation/faq/releases.rst | 1 + > NEWS | 6 +- > configure.ac | 2 +- > debian/changelog | 10 -- > debian/rules | 4 ++-- > 5 files changed, 17 insertions(+), 6 deletions(-) > Except for the Conntrack Helper Persistence version, the changes match the version bump to 3.2 back in the day. Ack on the series… Acked-by: Jakob Meng ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.
On 16.05.24 15:57, Mike Pattrick wrote: > This patch attempts to fix a large number of ubsan error messages that > take the following form: > > lib/netlink-notifier.c:237:13: runtime error: call to function > route_table_change through pointer to incorrect function type > 'void (*)(const void *, void *)' > > In Clang 17 the undefined behaviour sanatizer check for function > pointers was enabled by default, whereas it was previously disabled > while compiling C code. These warnings are a false positive in the case > of OVS, as our macros already check to make sure the function parameter > is the correct size. > > So that check is disabled in the single function that is causing all of > the errors. > > Signed-off-by: Mike Pattrick > --- > v2: Changed macro name > --- > include/openvswitch/compiler.h | 11 +++ > lib/ovs-rcu.c | 1 + > 2 files changed, 12 insertions(+) Thank you, Mike! This solves most issues I had when running OVS tests on Fedora Rawhide with Clang 18 and "-fsanitize=undefined". Acked-by: Jakob Meng ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 6/6] ofproto: Add JSON output for 'dpif/show' command.
On 13.04.24 00:55, Ilya Maximets wrote: > On 4/12/24 09:26, jm...@redhat.com wrote: >> From: Jakob Meng >> >> The 'dpif/show' command now supports machine-readable JSON output in >> addition to the plain-text output for humans. An example would be: >> >> ovs-appctl --format json dpif/show >> >> Reported-at: https://bugzilla.redhat.com/1824861 >> Signed-off-by: Jakob Meng >> --- >> NEWS | 1 + >> ofproto/ofproto-dpif.c | 124 + >> tests/pmd.at | 28 ++ >> 3 files changed, 142 insertions(+), 11 deletions(-) >> > Hi, Jakob. Thanks for v9! > > The general approach in the set seems reasonable, however I didn't > read the code carefully enough. I hope to do that once I'm back > from PTO in one week. > > Hi Ilya! Thank you for taking the time and reviewing all patches of the series ☺️ Very much appreciated! I agree with all you wrote and prepared a new patch series v10 [0]. I am curious to hear your opinion on the latest changes 👀 [0] https://patchwork.ozlabs.org/project/openvswitch/list/?series=407002&state=%2A&archive=both Best regards, Jakob ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 3/6] Migrate commands to extended unixctl API.
On 19.03.24 16:41, Eelco Chaudron wrote: > > On 19 Mar 2024, at 16:19, Ilya Maximets wrote: > >> On 3/19/24 15:54, Eelco Chaudron wrote: >>> >>> On 19 Mar 2024, at 15:46, Ilya Maximets wrote: >>> >>>> On 3/19/24 15:17, Eelco Chaudron wrote: >>>>> >>>>> On 19 Mar 2024, at 15:00, Ilya Maximets wrote: >>>>> >>>>>> On 3/19/24 14:41, Jakob Meng wrote: >>>>>>> >>>>>>> On 19.03.24 13:22, Ilya Maximets wrote: >>>>>>>> On 3/19/24 13:21, Ilya Maximets wrote: >>>>>>>>> On 3/19/24 13:17, Eelco Chaudron wrote: >>>>>>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote: >>>>>>>>>> >>>>>>>>>>> Hi! >>>>>>>>>>> >>>>>>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote: >>>>>>>>>>>> On 18 Jan 2024, at 16:26, jm...@redhat.com wrote: >>>>>>>>>>>> >>>>>>>>>>>>> ... >>>>>>>>>>>> Thank for the patch! What a beast to go trough ;) >>>>>>>>>>> Thank you for doing it anyway ☺️ >>>>>>>>>>> >>>>>>>>>>>> I believe the current approach is acceptable. However, we could >>>>>>>>>>>> also >>>>>>>>>>>> incorporate union callbacks: if registration only supports text, >>>>>>>>>>>> we would >>>>>>>>>>>> use callback A, and if multiple formats exist, we could employ the >>>>>>>>>>>> new style >>>>>>>>>>>> callback. This would mitigate the need for a significant overhaul. >>>>>>>>>>>> Just a >>>>>>>>>>>> suggestion, as I'm ok with the current approach. :) >>>>>>>>>>> This trades many-initial-changes-across-a-lot-of-files for a >>>>>>>>>>> more-complex-registration-and-callback-logic. >>>>>>>>>>> >>>>>>>>>>> We could also make struct unixctl_conn visible (move it from >>>>>>>>>>> lib/unixctl.c to lib/unixctl.h) and let callees read the output >>>>>>>>>>> format from its 'fmt' member. Then we would not have to change the >>>>>>>>>>> callback signature at all. >>>>>>>>>> Or have a utility function to return the format, which keeps the >>>>>>>>>> structure invisible. >>>>>>>>> +1, just add some access methods. >>>>>>>>> >>>>>>>>>>> I do not have a strong opinion here. What do you, Ilya and the >>>>>>>>>>> others think? >>>>>>>>>> I’ll wait for others to chime in... >>>>>>>>> I don't think changing all the callbacks is a good thing to do. >>>>>>>>> i.e. this patch should not exist or be very small. >>>>>>>> * or it should be very small. >>>>>>> How about unixctl_command_register? >>>>>>> >>>>>>> Should we change the existing function signature (like I did)? >>>>>>> Or add a second function, like unixctl_command_register_fmt? >>>>>>> Or should we add another function to set/change supported formats of an >>>>>>> already registered command? >>>>>>> >>>>>>> Example: >>>>>>> >>>>>>> // Get output format which the user has choosen (defaults to txt) >>>>>>> enum ovs_output_fmt >>>>>>> unixctl_command_get_output_format(struct unixctl_conn *); >>>>>>> >>>>>>> // Set output formats that a command supports >>>>>>> int >>>>>>> unixctl_command_set_output_formats(const char *name, unsigned int >>>>>>> output_fmts); >>>>>>> >>>>>>> Wdyt? >>>>>> I'm actually not sure why we need to register them differently in the >>>>>> first palace. Make the callback check what was the requested format >>>>>&g
Re: [ovs-dev] [PATCH v7 3/6] Migrate commands to extended unixctl API.
On 19.03.24 13:22, Ilya Maximets wrote: > On 3/19/24 13:21, Ilya Maximets wrote: >> On 3/19/24 13:17, Eelco Chaudron wrote: >>> >>> On 19 Mar 2024, at 13:11, Jakob Meng wrote: >>> >>>> Hi! >>>> >>>> On 15.03.24 11:19, Eelco Chaudron wrote: >>>>> On 18 Jan 2024, at 16:26, jm...@redhat.com wrote: >>>>> >>>>>> ... >>>>> Thank for the patch! What a beast to go trough ;) >>>> Thank you for doing it anyway ☺️ >>>> >>>>> I believe the current approach is acceptable. However, we could also >>>>> incorporate union callbacks: if registration only supports text, we would >>>>> use callback A, and if multiple formats exist, we could employ the new >>>>> style >>>>> callback. This would mitigate the need for a significant overhaul. Just a >>>>> suggestion, as I'm ok with the current approach. :) >>>> This trades many-initial-changes-across-a-lot-of-files for a >>>> more-complex-registration-and-callback-logic. >>>> >>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c >>>> to lib/unixctl.h) and let callees read the output format from its 'fmt' >>>> member. Then we would not have to change the callback signature at all. >>> Or have a utility function to return the format, which keeps the structure >>> invisible. >> +1, just add some access methods. >> >>>> I do not have a strong opinion here. What do you, Ilya and the others >>>> think? >>> I’ll wait for others to chime in... >> I don't think changing all the callbacks is a good thing to do. >> i.e. this patch should not exist or be very small. > * or it should be very small. How about unixctl_command_register? Should we change the existing function signature (like I did)? Or add a second function, like unixctl_command_register_fmt? Or should we add another function to set/change supported formats of an already registered command? Example: // Get output format which the user has choosen (defaults to txt) enum ovs_output_fmt unixctl_command_get_output_format(struct unixctl_conn *); // Set output formats that a command supports int unixctl_command_set_output_formats(const char *name, unsigned int output_fmts); Wdyt? > >>>>> Some small style comments highlighted below. >>>> I would like to have your eagle eyes...👀 Fixed them! >>>> >>>> Thanks, >>>> Jakob ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 3/6] Migrate commands to extended unixctl API.
Hi! On 15.03.24 11:19, Eelco Chaudron wrote: > On 18 Jan 2024, at 16:26, jm...@redhat.com wrote: > >> ... > Thank for the patch! What a beast to go trough ;) Thank you for doing it anyway ☺️ > I believe the current approach is acceptable. However, we could also > incorporate union callbacks: if registration only supports text, we would > use callback A, and if multiple formats exist, we could employ the new style > callback. This would mitigate the need for a significant overhaul. Just a > suggestion, as I'm ok with the current approach. :) This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic. We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all. I do not have a strong opinion here. What do you, Ilya and the others think? > Some small style comments highlighted below. I would like to have your eagle eyes...👀 Fixed them! Thanks, Jakob ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 2/6] python: Add global option for JSON output to Python tools.
On 15.03.24 11:16, Eelco Chaudron wrote:
> Hi Jakob,
>
> See some comments below.
>
> //Eelco
Thanks and comments below again 😄
Cheers,
Jakob
>> ...
>> --- a/tests/appctl.py
>> +++ b/tests/appctl.py
>> @@ -49,13 +49,30 @@ def main():
>> help="Arguments to the command.")
>> parser.add_argument("-T", "--timeout", metavar="SECS",
>> help="wait at most SECS seconds for a response")
>> +parser.add_argument("-f", "--format", metavar="FMT",
>> +help="Output format.", default="text",
>> +choices=[fmt.name.lower()
>> + for fmt in ovs.util.OutputFormat])
>> args = parser.parse_args()
>>
>> signal_alarm(int(args.timeout) if args.timeout else None)
>>
>> ovs.vlog.Vlog.init()
>> target = args.target
>> +format = ovs.util.OutputFormat[args.format.upper()]
>> client = connect_to_target(target)
>> +
>> +if format != ovs.util.OutputFormat.TEXT:
>> +err_no, error, _ = client.transact(
>> +"set-options", ["--format", args.format])
>> +
>> +if err_no:
>> +ovs.util.ovs_fatal(err_no, "%s: transaction error" % target)
>> +elif error is not None:
> I see this is a copy from below, but is the 'elif' ok here? Meaning can we not
> get both errors at the same time? The C code catches both.
>
> Also 'if err_no' we will still try the second transaction for the command.
> I think we should exit even in this case.
When we hit a transaction error ('if err_no'), the ovs.util.ovs_fatal() call
will sys.exit(1), so the elif branch will not be executed nor the second
command transaction?!
What did I miss?🙈
>> +sys.stderr.write(error)
>> +ovs.util.ovs_error(0, "%s: server returned an error" % target)
>> +sys.exit(2)
>> +
>> err_no, error, result = client.transact(args.command, args.argv)
>> client.close()
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On 15.03.24 11:15, Eelco Chaudron wrote:
> [...]
> Hi Jakob,
>
>
> Thank you for submitting this series; I believe it's a valuable addition to
> OVS! Apologies for the delayed response. I've reviewed the entire series, and
> most of the comments are minor change requests. I'll hold off on sending a
> new revision until Ilya has had a chance to review it, as he provided some
> comments on the previous revision.
>
> Cheers,
>
> Eelco
Thanks for your review ☺️ Some questions below..
Best,
Jakob
>> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
>> index ba0c172e6..02df8ba97 100644
>> --- a/utilities/ovs-appctl.c
>> +++ b/utilities/ovs-appctl.c
>> @@ -29,12 +29,22 @@
>> #include "jsonrpc.h"
>> #include "process.h"
>> #include "timeval.h"
>> +#include "svec.h"
>> #include "unixctl.h"
>> #include "util.h"
>> #include "openvswitch/vlog.h"
>>
>> static void usage(void);
>> -static const char *parse_command_line(int argc, char *argv[]);
>> +
>> +/* Parsed command line args. */
>> +struct cmdl_args {
>> +enum ovs_output_fmt format;
>> +char *target;
>> +};
>> +
>> +static struct cmdl_args *cmdl_args_create(void);
>> +static void cmdl_args_destroy(struct cmdl_args *);
>> +static struct cmdl_args *parse_command_line(int argc, char *argv[]);
>> static struct jsonrpc *connect_to_target(const char *target);
>>
>> int
>> @@ -43,30 +53,59 @@ main(int argc, char *argv[])
>> char *cmd_result, *cmd_error;
>> struct jsonrpc *client;
>> char *cmd, **cmd_argv;
>> -const char *target;
>> +struct cmdl_args *args;
>> int cmd_argc;
>> int error;
>> +struct svec opt_argv = SVEC_EMPTY_INITIALIZER;
> Can we keep the variables in reversed Christmas tree order?
>
>> set_program_name(argv[0]);
>>
>> /* Parse command line and connect to target. */
>> -target = parse_command_line(argc, argv);
>> -client = connect_to_target(target);
>> +args = parse_command_line(argc, argv);
>> +client = connect_to_target(args->target);
>> +
>> +/* Transact options request (if required) and process reply */
>> +if (args->format != OVS_OUTPUT_FMT_TEXT) {
>> +svec_add(&opt_argv, "--format");
>> +svec_add(&opt_argv, ovs_output_fmt_to_string(args->format));
>> +}
>> +svec_terminate(&opt_argv);
>> +
>> +if (opt_argv.n > 0) {
> You can use svec_is_empty() here.
>
>> +error = unixctl_client_transact(client, "set-options",
>> +opt_argv.n, opt_argv.names,
>> +&cmd_result, &cmd_error);
>> +
>> +if (error) {
>> +ovs_fatal(error, "%s: transaction error", args->target);
>> +}
>>
>> -/* Transact request and process reply. */
>> +if (cmd_error) {
>> +jsonrpc_close(client);
>> +fputs(cmd_error, stderr);
>> +ovs_error(0, "%s: server returned an error", args->target);
>> +exit(2);
>> +}
>> +
>> +free(cmd_result);
>> +free(cmd_error);
> cmd_error will never end up here, as you call exit(2) above, so the free
> should also move up.
The error handling of this set-options call is intentionally kept very similar
to the error handling of the later command call. Should I drop the
"free(cmd_error);" in both sections because we exit anyway? Should I move it up
in both cases?
> Should we not exit on a transaction error also? I think error handling might
> need some more cleanup.
We exit on transaction error with ovs_fatal?! Just like in the next command
call..
What kind of cleanup do you have in mind?
>> +}
>> +svec_destroy(&opt_argv);
>> +
>> +/* Transact command request and process reply. */
>> cmd = argv[optind++];
>> cmd_argc = argc - optind;
>> cmd_argv = cmd_argc ? argv + optind : NULL;
>> error = unixctl_client_transact(client, cmd, cmd_argc, cmd_argv,
>> &cmd_result, &cmd_error);
>> if (error) {
>> -ovs_fatal(error, "%s: transaction error", target);
>> +ovs_fatal(error, "%s: transaction error", args->target);
>> }
>>
>> if (cmd_error) {
>> jsonrpc_close(client);
>> fputs(cmd_error, stderr);
>> -ovs_error(0, "%s: server returned an error", target);
>> +ovs_error(0, "%s: server returned an error", args->target);
>> exit(2);
>> } else if (cmd_result) {
>> fputs(cmd_result, stdout);
>> @@ -74,6 +113,7 @@ main(int argc, char *argv[])
>> OVS_NOT_REACHED();
>> }
>>
>> +cmdl_args_destroy(args);
>> jsonrpc_close(client);
>> free(cmd_result);
>> free(cmd_error);
>> @@ -101,13 +141,34 @@ Common commands:\n\
>>vlog/reopenMake the program reopen its log file\n\
>> Other options:\n\
>>--timeout=SECS wait at most SECS seconds for a response\n\
>> + -f, --format=FMT Output format. One of: 'json', or 'text'\n\
>> + ('text',
Re: [ovs-dev] [PATCH v2] dpif-netdev: Increase MAX_RECIRC_DEPTH to 8.
On 16.02.24 00:03, Ilya Maximets wrote: > On 2/15/24 14:23, Simon Horman wrote: >> On Thu, Feb 15, 2024 at 09:16:38AM +0100, Jakob Meng wrote: >>> On 30.01.24 10:44, Simon Horman wrote: >>>> On Fri, Jan 26, 2024 at 02:24:51PM +0100, jm...@redhat.com wrote: >>>>> From: Jakob Meng >>>>> >>>>> In a scenario where OVN does load balancing and then SNAT with a OVS >>>>> userspace datapath [0], the recirc_depth may be greater than 6. In >>>>> that case, ovs-vswitchd might drop packets and raise warnings: >>>>> >>>>> dpif_netdev|WARN|Packet dropped. Max recirculation depth exceeded. >>>>> >>>>> Increasing MAX_RECIRC_DEPTH to 8 solves this issue. >>>>> >>>>> [0] >>>>> https://github.com/ovn-org/ovn/blob/dd5cd73e3df1bfb1a215cb45d1e2e03eff1d049a/tests/system-ovn-kmod.at#L740 >>>>> >>>>> Reported-at: https://issues.redhat.com/browse/FDP-251 >>>>> Signed-off-by: Jakob Meng >>>> Hi Jakob, >>>> >>>> I'm unsure what the considerations were when setting this limit, >>>> but I do note that it was increased once before, from 5 to 6, >>>> for an OVN use-case [1]. So this approach seems reasonable to me. >>>> >>>> Acked-by: Simon Horman >>>> >>>> [1] dpif-netdev: Set MAX_RECIRC_DEPTH to 6. >>>> https://github.com/openvswitch/ovs/commit/3f9d3836d63a >>>> >>> Hi Ilya, >>> can we include this patch in 3.3, please? >>> >>> It is required for more complex setups like OKD/OpenShift with userspace >>> datapaths. >> In the hope of moving this forwards: >> >> My feeling is that while on the one hand this is a wack-a-mole approach it >> does address a real problem in a well understood way, thus moving >> things in a positive direction. >> >> As for downside, there is a risk that this will blow the stack anyway >> or somehow cause resource exhaustion that didn't occur before. I didn't >> exercise the patch. But I do suspect that risk is minimal. And if it >> surfaces we will have to search for a better solution. >> >> But we don't have a better solution at this time - or at least I don't. >> So, other than what I assume is a small risk of regression, do we really >> lose anything by moving forward with this simple change at this time? > I think, it's fine for now. The extra 2 recirculations are very unlikely > to blow up the stack, especially since the default config is to have 2 MB > of pre-allocated and mlocked stack. > > So, applied for now. Also applied to 3.3 as I don't think this change > carries any risk at this time and it will make life of OVN developers/users > much easier. > > But we still need to think of a better way of solving this issue for the > future. > > Best regards, Ilya Maximets. > Thank you, Ilya and Simon! I have updated the ticket [0] accordingly. [0] https://issues.redhat.com/browse/FDP-251 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] dpif-netdev: Increase MAX_RECIRC_DEPTH to 8.
On 30.01.24 10:44, Simon Horman wrote: > On Fri, Jan 26, 2024 at 02:24:51PM +0100, jm...@redhat.com wrote: >> From: Jakob Meng >> >> In a scenario where OVN does load balancing and then SNAT with a OVS >> userspace datapath [0], the recirc_depth may be greater than 6. In >> that case, ovs-vswitchd might drop packets and raise warnings: >> >> dpif_netdev|WARN|Packet dropped. Max recirculation depth exceeded. >> >> Increasing MAX_RECIRC_DEPTH to 8 solves this issue. >> >> [0] >> https://github.com/ovn-org/ovn/blob/dd5cd73e3df1bfb1a215cb45d1e2e03eff1d049a/tests/system-ovn-kmod.at#L740 >> >> Reported-at: https://issues.redhat.com/browse/FDP-251 >> Signed-off-by: Jakob Meng > Hi Jakob, > > I'm unsure what the considerations were when setting this limit, > but I do note that it was increased once before, from 5 to 6, > for an OVN use-case [1]. So this approach seems reasonable to me. > > Acked-by: Simon Horman > > [1] dpif-netdev: Set MAX_RECIRC_DEPTH to 6. > https://github.com/openvswitch/ovs/commit/3f9d3836d63a > Hi Ilya, can we include this patch in 3.3, please? It is required for more complex setups like OKD/OpenShift with userspace datapaths. Best, Jakob ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] github: Bump Fedora version to 39.
On 29.01.24 23:33, Ilya Maximets wrote: > Fedora 37 reached EOL in November. Switch to the most recent version > to avoid potential CI failures in the future. > > Signed-off-by: Ilya Maximets > --- > .github/workflows/build-and-test.yml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/.github/workflows/build-and-test.yml > b/.github/workflows/build-and-test.yml > index ddb425580..fc7558148 100644 > --- a/.github/workflows/build-and-test.yml > +++ b/.github/workflows/build-and-test.yml > @@ -462,7 +462,7 @@ jobs: >build-linux-rpm: > name: linux rpm fedora > runs-on: ubuntu-latest > -container: fedora:37 > +container: fedora:39 > timeout-minutes: 30 > > strategy: Why not fedora:rawhide? We will not have to update this line anymore and we will catch breaking changes early, like the GCC14 issue. I assume there has been a discussion previously, but I cannot find it in my archives (which is not old)... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-atomic: Fix inclusion of Clang header by GCC 14.
On 18.01.24 15:59, Ilya Maximets wrote:
> GCC 14 started to advertise c_atomic extension, older versions didn't
> do that. Add check for __clang__, so GCC doesn't include headers
> designed for Clang.
>
> Another option would be to prefer stdatomic implementation instead,
> but some older versions of Clang are not able to use stdatomic.h
> supplied by GCC as described in commit:
> 07ece367fb5f ("ovs-atomic: Prefer Clang intrinsics over .")
>
> This change fixes OVS build with GCC on Fedora Rawhide (40).
>
> Reported-by: Jakob Meng
> Signed-off-by: Ilya Maximets
> ---
> lib/ovs-atomic.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/ovs-atomic.h b/lib/ovs-atomic.h
> index ab9ce6b2e..f140d25fe 100644
> --- a/lib/ovs-atomic.h
> +++ b/lib/ovs-atomic.h
> @@ -328,7 +328,7 @@
> #if __CHECKER__
> /* sparse doesn't understand some GCC extensions we use. */
> #include "ovs-atomic-pthreads.h"
> -#elif __has_extension(c_atomic)
> +#elif __clang__ && __has_extension(c_atomic)
> #include "ovs-atomic-clang.h"
> #elif HAVE_ATOMIC && __cplusplus >= 201103L
> #include "ovs-atomic-c++.h"
Tested with latest gcc 14 [0] from Fedora Rawhide and it fixed all my OVS
compilation issues. Thank you!
[0] gcc (GCC) 14.0.1 20240113 (Red Hat 14.0.1-0)
Acked-by: Jakob Meng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 3/6] Migrate commands to extended unixctl API.
On 18.01.24 11:17, 0-day Robot wrote: > Bleep bloop. Greetings Jakob Meng, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > error: Failed to merge in the changes. > hint: Use 'git am --show-current-patch=diff' to see the failed patch > Patch failed at 0001 Migrate commands to extended unixctl API. > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > > Patch skipped due to previous failure. > > Please check this out. If you feel there has been an error, please email > acon...@redhat.com > > Thanks, > 0-day Robot > Patch does not apply cleanly, have to rebase it to latest master 🙄 Archived this patch series and will submit a new one later.. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 3/6] Migrate commands to extended unixctl API.
On 17.01.24 20:50, Simon Horman wrote:
> On Wed, Jan 17, 2024 at 06:56:31PM +, Simon Horman wrote:
>> On Wed, Jan 17, 2024 at 01:55:36PM +0100, jm...@redhat.com wrote:
>>> From: Jakob Meng
>>>
>>> Previous commits introduced support different output formats to
>>> ovs-xxx tools and its Python equivalents. However, the commands
>>> were not yet migrated to the updated {unixctl_}command_register()
>>> functions and unixctl_cb_func function type in order to highlight
>>> the API changes only.
>>>
>>> This patch accomplishes this command migration in a single sweep.
>>> It replaces the old functions {unixctl_}command_register() in
>>> lib/unixctl.* and python/ovs/unixctl/__init__.py with their
>>> extended variants featuring the new 'output_fmts' parameter.
>>> All command registrations have been updated to announce what output
>>> formats each commands supports. No new output formats have been
>>> added, i.e. all commands still support OVS_OUTPUT_FMT_TEXT only.
>>>
>>> All command callbacks gained a 'enum ovs_output_fmt fmt OVS_UNUSED'
>>> argument to conform with the updated function type unixctl_cb_func
>>> in lib/unixctl.h. Without any new output formats being added, it is
>>> always ignored for now.
>>>
>>> Reported-at: https://bugzilla.redhat.com/1824861
>>> Signed-off-by: Jakob Meng
>> Recheck-request: github-robot
> Hi Jacob,
>
> Recheck triggered 2nd run of jobs.
> However, both the 1st and 2nd runs failed.
>
> e.g.
>
> 184: IPsec -- Libreswan (ipv4, geneve, defaultroute, psk) FAILED
> (ovs-macros.at:242)
> 185: IPsec -- Libreswan (ipv4, geneve, localip, psk) FAILED
> (ovs-macros.at:242)
> 186: IPsec -- Libreswan (ipv4, geneve, defaultroute, self-signed) FAILED
> (ovs-macros.at:242)
> 187: IPsec -- Libreswan (ipv4, geneve, defaultroute, ca-signed) FAILED
> (ovs-macros.at:242)
> 188: IPsec -- Libreswan (ipv4, gre, defaultroute, psk) FAILED
> (ovs-macros.at:242)
> 189: IPsec -- Libreswan (ipv4, vxlan, defaultroute, psk) FAILED
> (ovs-macros.at:242)
> 190: IPsec -- Libreswan (ipv6, vxlan, defaultroute, psk) FAILED
> (ovs-macros.at:242)
> 191: IPsec -- Libreswan (ipv6, vxlan, localip, psk) FAILED
> (ovs-macros.at:242)
> 192: IPsec -- Libreswan (ipv6, geneve, defaultroute, psk) FAILED
> (ovs-macros.at:242)o
>
> https://github.com/ovsrobot/ovs/actions/runs/7556373167/job/20587597728
>
> Ilya tells me he think this is because the ovs-monitor-ipsec script
> needs to be updated for the new API.
>
Argh, thank you! I missed command callbacks in two files 🙈 v6 [0] is on its
way..
[0]
https://patchwork.ozlabs.org/project/openvswitch/list/?series=390996&archive=both&state=*
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 6/6] unixctl: Pass output format as command args via JSON-RPC API.
On 03.01.24 00:57, Ilya Maximets wrote:
> On 11/16/23 11:41, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> A previous patch had changed the JSON-RPC API in lib/unixctl.* (and
>> its Python counterpart) in order to allow transporting the requested
>> output format from ovs-xxx tools to ovs-vswitchd across unix sockets:
>>
>> The meaning of 'method' and 'params' had be changed: 'method' would be
>> 'execute/v1' when an output format other than 'text' is requested. In
>> that case, the first parameter of the JSON array 'params' would be the
>> designated command, the second one the output format and the rest
>> will be command args.
>> That change allowed to transport the output format in a backward
>> compatible way: One could use an updated client (ovs-appctl) with an
>> old server (ovs-vswitchd) and vice versa. Of course, JSON output would
>> only work when both sides have been updated.
>>
>> This patch reverts the JSON-RPC API to the old behaviour: The command
>> will be copied to JSON-RPC's 'method' parameter while command args will
>> be copied to the 'params' parameter.
>> The client will inject the output format into the command args and the
>> server will parse and remove it before passing the args to the command
>> callback.
>>
>> The advantage of this (old) approach is a simpler JSON-RPC API but at
>> the cost of inferior usability: ovs-xxx tools are no longer able to
>> detect missing JSON support at the server side. Old servers will simply
>> pass the output format as an arg to the command which would then result
>> in an error such as 'command takes at most ... arguments' or a non-\
>> uniform error from the command callback.
> Hi, Jakob.
>
> I see you don't like the option of passing a format argument by filtering
> it out. :)
>
> Both solutions look like nasty workarounds at this point, some more, some
> less, but in the end they all are just abusing the system. So, I came
> to a conclusion that two possible "correct" solutions are:
>
> 1. Implement JSON-RPC v2: https://www.jsonrpc.org/specification
>This will give us an automatic protocol version verification and
>we can pass paramaters as object, with separated server and method
>parameters, e.g.
> "params": {
> "server-params": { "format": "json" },
> "method-params": [ arg1, arg2, ... ]
> }
>This is not a lightweight option to implement, will require proper
>validation, support for both versions of the spec, and potentially
>support for features that we do not actually need.
>
> 2. Just make 2 calls. Add a new internal "unixctl_set_options" method
>that will change the options for a current connection. E.g.
>{
>"method": "unixctl_set_options",
>"params": [ "format:json" ],
>"id": 1
>}
>If that method fails - no json format. If the method succeeds, server
>will reply with JSON results for this connections.
>The current connection options can be directly stored in the struct
>unixctl_conn. Implementation of the method can be just a normal
>jsonrpc method registered by the unixctl module itself.
>
>Perfect compatibility with old servers, they will reject the first
>request with a clear error. Old clients will not use this method
>and have no changes in workflow at all. Only clients that need
>JSON replies will need to use this method.
>
>Should be much easier to implement. SO, I'd vote for this option.
>
> Thoughts?
>
> Best regards, Ilya Maximets.
>
Well, I agree with all your points 😄 Making 2 calls is a good idea, will update
my patch. Thank you!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Documentation: Add section on inclusive language.
On 14.12.23 23:06, Aaron Conole wrote: > Simon Horman writes: > >> As a community we should strive to be inclusive. >> As such it seems appropriate to adopt an word list, >> to help guide the use of inclusive language. >> >> This patch proposes use of the Inclusive Naming Word List v1.0. >> >> Link: https://inclusivenaming.org/word-lists/ >> Signed-off-by: Simon Horman >> --- > I think it's a good practice to adopt a list that gets vetted. I think > the inclusive naming initiative is a good choice to adopt. > > For that: > > Acked-by: Aaron Conole > > Some terms that are on the 'replace immediately' list (such as > 'whitelist' and 'abort' - although the latter is considered as not a > first-order concern). I guess we should also include some kind of > effort to clean these up as well. > Thank you, Simon! FYI Simon did a survey on inclusive language and compared various DEI lists [0]. In particular, the IBM Inclusive IT Language Repo [1] and Inclusive Naming Word Lists v1.0 [2] stood out because they give explanations on why certain words should be replaced or kept. The IBM list covers more terms, though the Inclusive Naming Word Lists has better rationale. Most other sources simply compile lists without giving any rationale. [0] https://issues.redhat.com/browse/FDP-146 [1] https://github.com/IBM/IBMInclusiveITLanguage [2] https://inclusivenaming.org/word-lists/ Acked-by: Jakob Meng ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
Thank you again for all your input! Your comments have been incorporated into a
new patch series v4:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=382459&archive=both&state=*
On 25.10.23 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).
>
> Wdyt?
>
> [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
Re: [ovs-dev] [PATCH v8 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 14.11.23 12:17, Kevin Traynor wrote:
> On 13/11/2023 08:53, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> This patch series incorporates Ilya's comments for v7 and has been rebased
>> to master:
>> * fixed afxdp status descriptions in vswitchd/vswitch.xml
>> * be consistent with rx/Rx/RX/tx/Tx/TX in vswitchd/vswitch.xml
>>
>> Jakob Meng (3):
>> netdev-dummy: Sync and clean {get,set}_config() callbacks.
>> netdev-afxdp: Sync and clean {get,set}_config() callbacks.
>> netdev-dpdk: Sync and clean {get,set}_config() callbacks.
>>
>> Documentation/intro/install/afxdp.rst | 12 +--
>> Documentation/topics/dpdk/phy.rst | 4 +-
>> NEWS | 7 ++
>> lib/netdev-afxdp.c | 21 -
>> lib/netdev-afxdp.h | 1 +
>> lib/netdev-dpdk.c | 113 ++
>> 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 +-
>> 12 files changed, 209 insertions(+), 88 deletions(-)
>>
>> --
>> 2.39.2
>>
>
> Ilya's documentation comments from v7 were resolved in v8.
>
> Added Robin's RvB from earlier version to relevant patches.
>
> Applied. Thanks Jakob, Robin, Ilya and others who had comments on early
> versions.
>
Thank you so much 🥳
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 2/3] netdev-afxdp: Sync and clean {get, set}_config() callbacks.
On 03.11.23 22:44, Ilya Maximets wrote: > On 10/30/23 10:49, 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 >> also moves key-value pairs which are no valid options from get_config() >> to the get_status() callback. >> >> ... >> >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -3821,6 +3821,17 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 >> type=patch options:peer=p1 \ >> supported by hardware. >> >> >> + >> + >> + >> + AF_XDP netdev specific interface status options. > 'netdev' word seems redundant here. 'netdev' and 'interface' are > basically words that describe the same thing in this context. > And the 'interface' should be preferred in the user documentation, > as that is the entity users actually interact with via database. > >> + >> + >> + >> +XDP mode which was chosen. > The "was chosen" here is a bit ambiguous as it may be interpreted > as both "chosen by the user" and "chosen by OVS itself". > "currently in use" might be a better way to describe the meaning, > as it kind of was before. > > It might also be useful to reference the key="xdp-mode" /> for the description of the possible values. > >> + >> + >> + >> >> >> ack, ty, integrated into patch series v8: https://patchwork.ozlabs.org/project/openvswitch/list/?series=381909 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/1] python: Remove duplicate UnixctlClient implementation.
On 31.10.23 16:22, Simon Horman wrote:
> On Mon, Oct 30, 2023 at 10:02:59AM +0100, 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 registry,
>> client, and server.")
>> Signed-off-by: Jakob Meng
>> Acked-by: Simon Horman
>> Acked-by: Eelco Chaudron
> Thanks Jakob,
>
> I have applied this patch.
>
> I have not backported as, although it has a fixes tag, I don't believe
> that it fixes a user-visible bug. Please let me know if you would like
> me to reconsider.
>
Thank you, Simon!
Agreed, backporting it should not be necessary. The code itself is fine, just a
perfect duplicate.
___
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.
Hi Ilya, thanks for sharing your thoughts, always appreciated! ☺️ Please find comments below. On 28.10.23 00:05, Ilya Maximets wrote: > 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. With OVS-DPDK I wanted to highlight one of the potential use cases of this change. It came up in discussions and the OVS codebase. Will remove it, if it is frowned upon, no worry. > >>>> 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. JSON-RPC 1.0 defines 'params' as a "Array of objects to pass as arguments to the method." [0]. Putting JSON objects into that array is valid. Please have a look at the source code changes in unixctl.c. Hopefully it helps with understanding my approach: Basically, I changed the meaning of the JSON-RPC API. Previously, there was a 1-1 mapping between command+args and JSON-RPC's method+params. Now, command+args are part of 'params'. [0] https://www.jsonrpc.org/specification_v1 (After reading though your mail completely, I am thinking about reevaluating this decision as explained below.) > >>> 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
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 30.10.23 14:07, Ilya Maximets wrote: > On 10/30/23 10:54, Jakob Meng wrote: >> On 27.10.23 17:25, Kevin Traynor wrote: >>> 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: >>>>>> ... >>>>>> Maybe something along these lines: >>>>>> >>>>>> - ovs-appctl: >>>>>> * Output of 'dpctl/show' command no longer shows interface >>>>>> configuration >>>>>> status, only values of the actual configuration options, a.k.a. >>>>>> 'requested' configuration. The interface configuration status, >>>>>> a.k.a. 'configured' values, can be found in the 'status' column >>>>>> of >>>>>> the Interface table, i.e. with 'ovs-vsctl get interface <..> >>>>>> status'. >>>>>> Reported names adjusted accordingly. >>>>>> >>>>>> What do you think? >>>>> Simple and concise 👍 I will use that. However, we could add an example, >>>>> it could make it easier to grasp the meaning. >>>> I guess, the reference to 'configured' and 'requested' should be enough >>>> of an example. I think, if we would add an example, it should be very >>>> short, i.e. no longer than one extra line. This entry is already too long. >>>> >>>>> Should I put the NEWS change in one of the patches or in a separate patch >>>>> 4/4? >>>> I'd say since the netdev-dpdk changes are the most noticeable, add the >>>> NEWS change into netdev-dpdk patch, but move the patch itself to the >>>> end of the set, so the NEWS entry is correct. >>>> >>>> Having a NEWS entry as a separate patch is not a good practice as if >>>> impairs ability to git blame the NEWS file. >>>> >>> The plan above looks good to me too, >>> >>> Kevin. >> Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: >> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 >> >> (cover letter includes list of what was changed, but it's not listed in >> patchwork) > It actually is in the patchwork, you just need to 'expand' the 'Series' after > navigating to one of the patches: > > https://patchwork.ozlabs.org/project/openvswitch/cover/20231030095000.720854-1-jm...@redhat.com/ > > Best regards, Ilya Maximets. Ah, nice! Thank you ☺️ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
On 30.10.23 11:19, Eelco Chaudron wrote:
> On 30 Oct 2023, at 11:07, Jakob Meng wrote:
>
>> On 27.10.23 16:27, Eelco Chaudron wrote:
>>> 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.
>> Which one did I miss?
>>
>> The note on JSON pretty printing I left here as a remainder for the
>> discussion on last thursday. With the consensus on adding an extra f
Re: [ovs-dev] [RFC v3 2/4] python: Add global option for JSON output to Python tools.
On 27.10.23 15:52, Eelco Chaudron wrote: > 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). > Thank you, Eelco, for pointing this out. I was not aware that flake8 is actually incorporated into the build if available. This reminds me of something which bothers me about the OVS docs: It documents requirements and steps for building and installing OVS in great detail (e.g. [0]). It lists all kinds of configure flags one could use and software packages one could install and often gives rationale for it. But for starters this is overwhelming! Please gimme a brief list of commands to get started on common OSes such as Debian/Ubuntu and/or Fedora/RHEL. For example, put it at the beginning of [0]. A list which reflects what you and other experienced OVS devs typically use in their day to day work. Without experience in OVS, how else are starters supposed to know which configure flags to use and which software to install? [0] https://docs.openvswitch.org/en/latest/intro/install/general/ The benefit for you will hopefully be better patches, without basic shortcomings such as flake8 failures. Wdyt? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
On 27.10.23 16:27, Eelco Chaudron wrote:
> 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.
Which one did I miss?
The note on JSON pretty printing I left here as a remainder for the discussion
on last thursday. With the consensus on adding an extra flag for pretty
printing I will remove it in the next patch version.
You asked about freeing args->target which is done a couple of lines above in
"cmdl_args_destroy()".
The rest of your comments has been incorporated in v3 in one way or another 😉
Best,
Jakob
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
On 27.10.23 17:25, Kevin Traynor wrote: > 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: >>>> ... >>>> Maybe something along these lines: >>>> >>>> - ovs-appctl: >>>> * Output of 'dpctl/show' command no longer shows interface >>>> configuration >>>> status, only values of the actual configuration options, a.k.a. >>>> 'requested' configuration. The interface configuration status, >>>> a.k.a. 'configured' values, can be found in the 'status' column of >>>> the Interface table, i.e. with 'ovs-vsctl get interface <..> >>>> status'. >>>> Reported names adjusted accordingly. >>>> >>>> What do you think? >>> >>> Simple and concise 👍 I will use that. However, we could add an example, >>> it could make it easier to grasp the meaning. >> >> I guess, the reference to 'configured' and 'requested' should be enough >> of an example. I think, if we would add an example, it should be very >> short, i.e. no longer than one extra line. This entry is already too long. >> >>> >>> Should I put the NEWS change in one of the patches or in a separate patch >>> 4/4? >> >> I'd say since the netdev-dpdk changes are the most noticeable, add the >> NEWS change into netdev-dpdk patch, but move the patch itself to the >> end of the set, so the NEWS entry is correct. >> >> Having a NEWS entry as a separate patch is not a good practice as if >> impairs ability to git blame the NEWS file. >> > > The plan above looks good to me too, > > Kevin. Thank you both, Ilya and Kevin! I have incorporated your suggestions into v7: https://patchwork.ozlabs.org/project/openvswitch/list/?series=379901 (cover letter includes list of what was changed, but it's not listed in patchwork) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] python: Remove duplicate UnixctlClient implementation.
On 28.10.23 00:08, Ilya Maximets wrote:
> 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.
Missed that part in "Submitting patches" guide. Thank you for pointing out!
Patch v2 is out:
https://patchwork.ozlabs.org/project/openvswitch/patch/20231030090259.15251-2-jm...@redhat.com/
___
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.
Hi Aaron,
first and foremost: I am fully committed to make our OVS community more
inclusive. Replacing terminology that promotes racial and cultural bias is one
of many (!) steps in achieving that. For the objective there is a broad
consensus, on the practical implementation not so much, it seems.
IBM Academy of Technology conducted a evaluation of IT terminology and
published a guideline on which terms to replace including rationale [0]:
[0] https://github.com/IBM/IBMInclusiveITLanguage
For example, it replaces {black,white}list with {block,allow}list because:
"As a pair, "blacklist" and "whitelist" promote racial bias by implying that
black is bad and white is good. When the terms 'white' or 'black' are used in a
context where white is represented as good or black is represented as bad, this
usage reinforces a model that promotes racial bias." [0]
However, for "white space" there is "No change recommended" because:
"This term is not based on a good/bad binary and so does not fall under our
guiding principle for black and white terms (see "blacklist")." [0]
Precision of expression is part of inclusive language. In my commit message I
am using the term "white space" because that is exactly the technical term used
in Unicode [1], editors and our ecosystem. Using other words such as
empty-space or blank-space could be confusing for readers.
[1] https://en.wikipedia.org/wiki/Whitespace_character
Although disturbed at first, I appreciate that you brought this up. We should
discuss this with Simon and others to get a common understanding of what
inclusive language is in practice. I assume you do not advocate for banning the
words white and black completely from the English language, do you?
Best,
Jakob
On 27.10.23 23:53, Aaron Conole wrote:
> 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/ktextedit
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
Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.
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
Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.
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.
> 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.
> 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}]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = space
>> indent_size = 4
>> +insert_final_newline = true
>> max_line_length = 79
>>
>> [include/linux/**.h]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> indent_size = tab
>> +insert_final_newline = true
>> tab_width = 8
>>
>> [include/sparse/rte_*.h]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> +insert_final_newline = true
>> tab_width = 8
>>
>> [include/windows/getopt.h]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> indent_size = tab
>> +insert_final_newline = true
>> tab_width = 8
>>
>> [include/windows/netinet/{icmp6,ip6}.h]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> indent_size = tab
>> +insert_final_newline = true
>> tab_width = 8
>>
>> [lib/getopt_long.c]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> indent_size = tab
>> +insert_final_newline = true
>> tab_width = 8
>>
>> [lib/sflow*.{c,h}]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> indent_size = tab
>> +insert_final_newline = true
>> tab_width = 8
>>
>> [lib/strsep.c]
>> +charset = utf-8
>> +end_of_line = lf
>> indent_style = tab
>> indent_size = tab
>> +insert_final_newline = true
>> tab_width = 8
>> --
>> 2.39.2
>
>
>
___
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 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.*. 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. For (2.) see below. > > unixctl_command_register() in lib/unixctl.* has been cloned as > unixctl_command_register_fmt() in order to demonstrate the new API. > The latter will be replaced with the former in a later patch. The > new function gained an int argument called 'output_fmts' with which > commands have to announce their support for output formats. > > Function type unixctl_cb_func in lib/unixctl.h has gained a new arg > 'enum ovs_output_fmt fmt'. For now, it has been added as a comment > only for the huge changes reason mentioned earlier. The output format > which is passed via unix socket to ovs-vswitchd will be converted into > a value of type 'enum ovs_output_fmt' in lib/unixctl.c and then passed > to said 'fmt' arg of the choosen command handler (in a future patch). > When a requested output format is not supported by a command, > then process_command() in lib/unixctl.c will return an error. (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used: The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again. Wdyt? > This patch does not yet pass the choosen output format to commands. > Doing so requires changes to all unixctl_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 ovs-xxx tools, they will f
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On 25.10.23 20:50, Jakob Meng wrote:
> On 25.10.23 15:48, Mike Pattrick wrote:
>> On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry wrote:
>>> Eelco Chaudron, Oct 25, 2023 at 14:56:
>>>> On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
>>>>
>>>>> From: Jakob Meng
>>>>>
>>>>> A patch created with 'git format-patch' can contain trailing spaces.
>>>>> When editing a patch, e.g. to fix a typo in the title, the trailing
>>>>> spaces should not be removed. This becomes tricky when editors like
>>>>> Kate identify a space followed by form feed as a trailing space and
>>>>> remove both. This will result in a broken patch which cannot be applied
>>>>> cleanly. Although this case should likely be considered a editor bug,
>>>>> keeping trailing spaces in a patch is the right thing to do and also
>>>>> helps mitigating these kinds of editor bugs.
>>>>>
>>>>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>>>> This looks good to me, however as you mention this is more of an
>>>> editor configuration issue. It looks like leaving out any default
>>>> value will cause the editor to use its configured value.
>>>>
>>>> Acked-by: Eelco Chaudron
>>> It seems OK as well. But another safer option would have been to move
>>> the trim_trailing_whitespace = true option in specific sections. Or
>>> completely remove it since it will be caught by checkpatch.
>> I think it also makes sense to remove trim_trailing_whitespace from
>> the default section, but the current patch makes sense as a standalone
>> change.
>>
>> Acked-by: Mike Pattrick
> Thank you all for your feedback! You are right, I will change my patch ☺️
>
> We should completely remove the default section because we cannot set any
> reasonable defaults for all possible filetypes. For example, IDEs tend to
> write their own files to a subfolder like .vscode or .kdev4. A default
> section would apply to files in there, too, which is not safe in general.
>
> We also should not use insert_final_newline and trim_trailing_whitespace
> anywhere in .editorconfig! Editors interpret these lines differently, some
> will wipe whitespaces across the whole file, others will only remove them
> from lines being edited and others change their behavior between releases.
> Limiting those options to a subset like *.c/*.h will not help: As written in
> my other response, the definition of whitespace is ambiguous. Unicode
> considers form feed to be a whitespace [0], causing some editors to wipe form
> feeds when trim_trailing_whitespace is true which is not what we want in OVS.
> As Robin mentioned, we already have a test for our definition of whitespaces
> and thus we can avoid this whitespace mess by not using it in .editorconfig.
>
> [0] https://en.wikipedia.org/wiki/Whitespace_character
>
Updated patch is out 🥂
https://patchwork.ozlabs.org/project/openvswitch/patch/20231026110729.21961-1-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
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.
>
>>>>>> 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?
>>>>
>&g
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On 25.10.23 15:48, Mike Pattrick wrote:
> On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry wrote:
>> Eelco Chaudron, Oct 25, 2023 at 14:56:
>>> On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
>>>
>>>> From: Jakob Meng
>>>>
>>>> A patch created with 'git format-patch' can contain trailing spaces.
>>>> When editing a patch, e.g. to fix a typo in the title, the trailing
>>>> spaces should not be removed. This becomes tricky when editors like
>>>> Kate identify a space followed by form feed as a trailing space and
>>>> remove both. This will result in a broken patch which cannot be applied
>>>> cleanly. Although this case should likely be considered a editor bug,
>>>> keeping trailing spaces in a patch is the right thing to do and also
>>>> helps mitigating these kinds of editor bugs.
>>>>
>>>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>>> This looks good to me, however as you mention this is more of an
>>> editor configuration issue. It looks like leaving out any default
>>> value will cause the editor to use its configured value.
>>>
>>> Acked-by: Eelco Chaudron
>> It seems OK as well. But another safer option would have been to move
>> the trim_trailing_whitespace = true option in specific sections. Or
>> completely remove it since it will be caught by checkpatch.
> I think it also makes sense to remove trim_trailing_whitespace from
> the default section, but the current patch makes sense as a standalone
> change.
>
> Acked-by: Mike Pattrick
Thank you all for your feedback! You are right, I will change my patch ☺️
We should completely remove the default section because we cannot set any
reasonable defaults for all possible filetypes. For example, IDEs tend to write
their own files to a subfolder like .vscode or .kdev4. A default section would
apply to files in there, too, which is not safe in general.
We also should not use insert_final_newline and trim_trailing_whitespace
anywhere in .editorconfig! Editors interpret these lines differently, some will
wipe whitespaces across the whole file, others will only remove them from lines
being edited and others change their behavior between releases. Limiting those
options to a subset like *.c/*.h will not help: As written in my other
response, the definition of whitespace is ambiguous. Unicode considers form
feed to be a whitespace [0], causing some editors to wipe form feeds when
trim_trailing_whitespace is true which is not what we want in OVS. As Robin
mentioned, we already have a test for our definition of whitespaces and thus we
can avoid this whitespace mess by not using it in .editorconfig.
[0] https://en.wikipedia.org/wiki/Whitespace_character
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On 25.10.23 13:52, jm...@redhat.com wrote:
> From: Jakob Meng
>
> A patch created with 'git format-patch' can contain trailing spaces.
> When editing a patch, e.g. to fix a typo in the title, the trailing
> spaces should not be removed. This becomes tricky when editors like
> Kate identify a space followed by form feed as a trailing space and
> remove both. This will result in a broken patch which cannot be applied
> cleanly. Although this case should likely be considered a editor bug,
> keeping trailing spaces in a patch is the right thing to do and also
> helps mitigating these kinds of editor bugs.
Whether this is a editor bug or not is debatable. For example, Kate uses
QChar's isSpace() [0] to identify a "trailing space" [1] which considers a form
feed character (0x0c) a space.
In future Kate releases this issue will surface less often because Kate's
interpretation of 'trim_trailing_whitespaces' from .editorconfig has been
changed [3].
[0] https://doc.qt.io/qt-5/qchar.html#isSpace
[1]
https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
[2]
https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
[3]
https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>
> Signed-off-by: Jakob Meng
> ---
> .editorconfig | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/.editorconfig b/.editorconfig
> index 685c72750..5be5415da 100644
> --- a/.editorconfig
> +++ b/.editorconfig
> @@ -13,6 +13,10 @@ indent_style = space
> indent_size = 4
> max_line_length = 79
>
> +[*.{patch,diff}]
> +insert_final_newline = false
> +trim_trailing_whitespace = false
> +
> [include/linux/**.h]
> indent_style = tab
> indent_size = tab
___
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 25.10.23 11:59, 0-day Robot wrote: > Bleep bloop. Greetings Jakob Meng, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > checkpatch: > WARNING: Line lacks whitespace around operator > WARNING: Line lacks whitespace around operator > #696 FILE: utilities/ovs-appctl.c:112: > -f, --format=FMT Output format. One of: 'json', or 'text'\n\ > > Lines checked: 842, Warnings: 2, Errors: 0 > > > Please check this out. If you feel there has been an error, please email > acon...@redhat.com Shall we fix utilities/checkpatch.py or simply ignore this message? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 1/1] Add global option to output JSON from ovs-appctl cmds.
On 20.10.23 13:48, Eelco Chaudron wrote:
>
> On 20 Oct 2023, at 12:43, jm...@redhat.com wrote:
>
>> From: Jakob Meng
>>
>> This patch follows an alternative approach to RFC [0].
>>
>> 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 [1],[2]. This proof of concept
>> describes one way how ovs-xxx tools could output JSON in addition to
>> plain-text for humans.
>>
>> In the previous approach JSON output was implemented as a separate
>> option for each command such as 'dpif/show'. This patch adds a global
>> option '-f,--format' instead. An example call would be
>> 'ovs-appctl --format json dpif/show' as shown in tests/pmd.at.
>> By default, the output format is plain-text as before.
>>
>> In the previous patch the option was called '-o|--output' instead of
>> '-f,--format'. But 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.
>>
>> unixctl_command_register() in lib/unixctl.* gained a new int arg
>> 'output_fmts' with which commands have to announce their support for
>> output formats. All commands have been updated accordingly which is why
>> so many files had to be changed. Most announce OVS_OUTPUT_FMT_TEXT only
>> except for 'dpif/show' in ofproto/ofproto-dpif.c which also supports
>> OVS_OUTPUT_FMT_JSON.
>>
>> The JSON-RPC API in lib/unixctl.c has been changed to transport the
>> requested output format besides command and args. 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 always be
>> 'execute/v1' to ensure (in)compatibility between (mis)matching client
>> and server. The JSON-RPC 1.0 spec which is currently implemented,
>> defines 'params' as a JSON array. The first parameter will now be the
>> command, the second one the output format and the rest will be command
>> args.
>>
>> Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
>> 'enum ovs_output_fmt fmt'. The output format which is passed via unix
>> socket to ovs-vswitchd will be converted into a value of type
>> 'enum ovs_output_fmt' in lib/unixctl.c and then passed to said 'fmt'
>> arg of the choosen command handler. 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 [0] where each command
>> would have to parse the output format option and handle requests for
>> unsupported output formats on its own.
>>
>> A disadvantage to the previous approach is that the JSON-RPC API for
>> the socket communication had to be changed and all commands had to be
>> updated to announce their output format support. However, this is a
>> one-time change only: Whenever JSON output is to be implemend for
>> another command, OVS_OUTPUT_FMT_TEXT|OVS_OUTPUT_FMT_JSON would have to
>> be added to the unixctl_command_register() call and the command handler
>> would have to readout the fmt arg.
>>
>> 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).
>>
>> Wdyt?
> See my comments on the v1, which crossed:
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-October/408810.html
>
Thank you, Eelco ☺️
I have incorporated all your comments into a new patch series:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=379267&state=*
Cover letter explains it all but is not shown in patchwork...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Add the configuration file.
On 23.10.23 20:52, Jakob Meng wrote: > On 23.10.23 15:31, Ilya Maximets wrote: >> Since last month ReadTheDocs only supports building with a new >> configuration file provided in the repository itself: >> https://blog.readthedocs.com/migrate-configuration-v2/ >> >> So, all our documentation buids are failing for quite some time. >> >> Add the configuration file to unblock documentation updates. >> >> Need to remove the upper restriction on the sphinx version. >> sphinx 2.0 is very old at this point and pip fails to install >> it along with other dependencies on the rtd server. >> >> Note: Sphinx 2.0 moved from HTML4 to HTML5 renderer and tables >> no longer have borders by default. That should be addressed >> via CSS file in the ovs-sphinx-theme. > It was initially limited to 2.0 for RHEL7 support [0]. Is this still relevant? > > [0] > https://github.com/openvswitch/ovs/commit/59cf52e6d3307d5889335893fc941fe55cd3ed99 > bs 🙄 This commit [1] introduced the 2.0 limit but its not clear why. [1] https://github.com/openvswitch/ovs/commit/eeab45ffdc74325ba1aef893b1805e69867789da ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Add the configuration file.
On 23.10.23 15:31, Ilya Maximets wrote: > Since last month ReadTheDocs only supports building with a new > configuration file provided in the repository itself: > https://blog.readthedocs.com/migrate-configuration-v2/ > > So, all our documentation buids are failing for quite some time. > > Add the configuration file to unblock documentation updates. > > Need to remove the upper restriction on the sphinx version. > sphinx 2.0 is very old at this point and pip fails to install > it along with other dependencies on the rtd server. > > Note: Sphinx 2.0 moved from HTML4 to HTML5 renderer and tables > no longer have borders by default. That should be addressed > via CSS file in the ovs-sphinx-theme. It was initially limited to 2.0 for RHEL7 support [0]. Is this still relevant? [0] https://github.com/openvswitch/ovs/commit/59cf52e6d3307d5889335893fc941fe55cd3ed99 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
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. 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
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.
Wdyt?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 2/3] netdev-dummy: Sync and clean {get, set}_config() callbacks.
On 20.10.23 12:03, 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
>> also moves key-value pairs which are no valid options from get_config()
>> to the get_status() callback. The tests have been updated accordingly.
>>
>> Reported-at: https://bugzilla.redhat.com/1949855
>> Signed-off-by: Jakob Meng
>> ---
>> lib/netdev-dummy.c | 19 +++
>> tests/pmd.at | 26 +-
>> 2 files changed, 28 insertions(+), 17 deletions(-)
>>
>> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
>> index 1a54add87..fe82317d7 100644
>> --- a/lib/netdev-dummy.c
>> +++ b/lib/netdev-dummy.c
>> @@ -795,14 +795,25 @@ netdev_dummy_get_config(const struct netdev *dev,
>> struct smap *args)
>> dummy_packet_conn_get_config(&netdev->conn, args);
>> + /* pcap, rxq_pcap and tx_pcap cannot be recovered because filenames
>> have
>> + * been discarded after opening file descriptors */
>> +
>> + if (netdev->ol_ip_csum) {
>> + smap_add_format(args, "ol_ip_csum", "%s", "true");
>> + }
>> +
>> + if (netdev->ol_ip_csum_set_good) {
>> + smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
>> + }
>> +
>> /* 'dummy-pmd' specific config. */
>> if (!netdev_is_pmd(dev)) {
>> goto exit;
>> }
>> - smap_add_format(args, "requested_rx_queues", "%d",
>> netdev->requested_n_rxq);
>> - smap_add_format(args, "configured_rx_queues", "%d", dev->n_rxq);
>> - smap_add_format(args, "requested_tx_queues", "%d",
>> netdev->requested_n_txq);
>> - smap_add_format(args, "configured_tx_queues", "%d", dev->n_txq);
>> +
>> + smap_add_format(args, "n_rxq", "%d", netdev->requested_n_rxq);
>> + smap_add_format(args, "n_txq", "%d", netdev->requested_n_txq);
>> + smap_add_format(args, "numa_id", "%d", netdev->requested_numa_id);
>> exit:
>> ovs_mutex_unlock(&netdev->mutex);
>> diff --git a/tests/pmd.at b/tests/pmd.at
>> index 7bdaca9e7..df6adde6c 100644
>> --- a/tests/pmd.at
>> +++ b/tests/pmd.at
>> @@ -93,11 +93,11 @@ pmd thread numa_id core_id :
>> overhead: NOT AVAIL
>> ])
>> -AT_CHECK([ovs-appctl dpif/show | sed
>> 's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
>> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
>> dummy@ovs-dummy: hit:0 missed:0
>> br0:
>> br0 65534/100: (dummy-internal)
>> - p0 1/1: (dummy-pmd: configured_rx_queues=1,
>> configured_tx_queues=, requested_rx_queues=1,
>> requested_tx_queues=)
>> + p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
>> ])
>> OVS_VSWITCHD_STOP
>> @@ -111,11 +111,11 @@ CHECK_PMD_THREADS_CREATED()
>> AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=8])
>> -AT_CHECK([ovs-appctl dpif/show | sed
>> 's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
>> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
>> dummy@ovs-dummy: hit:0 missed:0
>> br0:
>> br0 65534/100: (dummy-internal)
>> - p0 1/1: (dummy-pmd: configured_rx_queues=8,
>> configured_tx_queues=, requested_rx_queues=8,
>> requested_tx_queues=)
>> + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0)
>> ])
>> AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed
>> SED_NUMA_CORE_PATTERN], [0], [dnl
>> @@ -144,11 +144,11 @@ OVS_VSWITCHD_START([add-port br0 p0 -- set Interface
>> p0 type=dummy-pmd options:n
>> CHECK_CPU_DISCOVERED(2)
>> CHECK_PMD_THREADS_CREATED()
>> -AT_CHECK([ovs-appctl dpif/show | sed
>> 's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
>> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
>> dummy@ovs-dummy: hit:0 missed:0
>> br0:
>> br0 65534/100: (dummy-internal)
>> - p0 1/1: (dummy-pmd: configured_rx_queues=8,
>> configured_tx_queues=, requested_rx_queues=8,
>> requested_tx_queues=)
>> + p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0)
>> ])
>> AT_CHE
Re: [ovs-dev] [PATCH v6 1/3] netdev-dpdk: Sync and clean {get, set}_config() callbacks.
Thanks for your feedback, Kevin! Added some notes inline.
On 20.10.23 12:03, 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 moves key-value pairs which are no valid options from
>> get_config() to the get_status() callback. For example, get_config()
>> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
>> previously. For requested rx queues the proper option name is n_rxq,
>> so requested_rx_queues has been renamed respectively. Tx queues
>> cannot be changed by the user, hence requested_tx_queues has been
>> dropped. Both configured_{rx,tx}_queues will be returned as
>> n_{r,t}xq in the get_status() callback.
>>
>> The documentation in vswitchd/vswitch.xml for status columns as well
>> as tests have been updated accordingly.
>>
>> Reported-at: https://bugzilla.redhat.com/1949855
>> Signed-off-by: Jakob Meng
>> ---
>> Documentation/topics/dpdk/phy.rst | 4 +-
>> lib/netdev-dpdk.c | 104 +-
>> tests/system-dpdk.at | 64 +++---
>> vswitchd/vswitch.xml | 14 +++-
>> 4 files changed, 127 insertions(+), 59 deletions(-)
>>
>> diff --git a/Documentation/topics/dpdk/phy.rst
>> b/Documentation/topics/dpdk/phy.rst
>> index f66b106c4..41cc3588a 100644
>> --- a/Documentation/topics/dpdk/phy.rst
>> +++ b/Documentation/topics/dpdk/phy.rst
>> @@ -198,7 +198,7 @@ Example::
>> a dedicated queue, it will be explicit::
>> $ ovs-vsctl get interface dpdk-p0 status
>> - {..., rx_steering=unsupported}
>> + {..., rx-steering=unsupported}
>> More details can often be found in ``ovs-vswitchd.log``::
>> @@ -499,7 +499,7 @@ its options::
>> $ ovs-appctl dpctl/show
>> [...]
>> - port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
>> dpdk-vf-mac=00:11:22:33:44:55, ...)
>> + port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
>> $ ovs-vsctl show
>> [...]
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 55700250d..56588f92a 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1905,31 +1905,32 @@ netdev_dpdk_get_config(const struct netdev *netdev,
>> struct smap *args)
>> ovs_mutex_lock(&dev->mutex);
>> - smap_add_format(args, "requested_rx_queues", "%d", dev->user_n_rxq);
>> - smap_add_format(args, "configured_rx_queues", "%d", netdev->n_rxq);
>> - smap_add_format(args, "requested_tx_queues", "%d",
>> dev->requested_n_txq);
>> - smap_add_format(args, "configured_tx_queues", "%d", netdev->n_txq);
>> - smap_add_format(args, "mtu", "%d", dev->mtu);
>> + smap_add_format(args, "dpdk-devargs", "%s", dev->devargs);
>> + smap_add_format(args, "n_rxq", "%d", dev->user_n_rxq);
>> + smap_add_format(args, "rx-flow-ctrl", "%s",
>> + dev->fc_conf.mode == RTE_ETH_FC_TX_PAUSE ||
>> + dev->fc_conf.mode == RTE_ETH_FC_FULL ? "true" :
>> "false");
>> + smap_add_format(args, "tx-flow-ctrl", "%s",
>> + dev->fc_conf.mode == RTE_ETH_FC_RX_PAUSE ||
>> + dev->fc_conf.mode == RTE_ETH_FC_FULL ? "true" :
>> "false");
>> + smap_add_format(args, "flow-ctrl-autoneg", "%s",
>> + dev->fc_conf.autoneg ? "true" : "false");
>> - if (dev->type == DPDK_DEV_ETH) {
>> - smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
>> - smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
>> - if (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) {
>> - smap_add(args, "rx_csum_offload", "true");
>> - } else {
>> - smap_add(args, "rx_csum_offload", "false");
>> - }
>> - if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
>> -
Re: [ovs-dev] [RFC] Add global option to output JSON from ovs-appctl cmds.
Superseeded by: https://patchwork.ozlabs.org/project/openvswitch/patch/20231020104320.1417664-2-jm...@redhat.com/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [RFC] Add options to output JSON from ovs-appctl commands.
Superseeded by: https://patchwork.ozlabs.org/project/openvswitch/patch/20231020092205.710399-2-jm...@redhat.com/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 12.10.23 17:39, Robin Jarry wrote: > Kevin Traynor, Oct 12, 2023 at 17:34: >> ok, 'rss' is documented as default, so maybe we don't need to display if it >> is in use by default, selected by user or as fallback. >> >> That would make things a bit easier as 'rx-steering:' is free to use to >> indicate the unsupported case. >> >> So maybe status could just have: >> 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports >> 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not support. >> >> If rx-steering is not reported, then it is using the default 'rss'. >> >> Robin, what do you think ? > > It may be surprising to users not to see any mention in get_config() when > explicitly setting rx-steering=rss but I don't see that as a common/standard > use case. So I think it should be OK. > Thank you Kevin and Robin ☺️ Your change requests have been incorporated in v6: https://patchwork.ozlabs.org/project/openvswitch/list/?series=377463 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] vswitch.xml: Add entry for dpdkvhostuser userspace-tso.
On 12.10.23 14:15, 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
> ---
> vswitchd/vswitch.xml | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 403b954c2..135b3c8c3 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3849,4 +3849,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Size of vring n.
>
> +
> +Status of userspace-tso.
> +
>
>
LGTM
Acked-by: Jakob Meng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] vswitch.xml: Add dpdkvhostuser group status.
On 12.10.23 14:15, 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
> ---
> vswitchd/vswitch.xml | 28
> 1 file changed, 28 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 1e2a1267d..403b954c2 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3822,4 +3822,32 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
>
>
> +
> +
> +
> + dpdkvhostuser and dpdkvhostuserclient
> + netdev specific interface status options.
> +
> +
> +Client or server vhost mode.
> +
> +
> +vhost features bitmap.
> +
> +
> +Number of vrings.
> +
> +
> +Vhost numa association.
> +
> +
> +Vhost socket path.
> +
> +
> + Status of connection.
> +
> +
> +Size of vring n.
> +
> +
>
>
LGTM
Acked-by: Jakob Meng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 11.10.23 18:48, Kevin Traynor wrote:
> On 11/10/2023 11:11, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> For better usability, the function pairs get_config() and
>> set_config() for each netdev 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 moves key-value pairs which are no valid options from
>> get_config() to the get_status() callback. For example, get_config()
>> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
>> previously. For requested rx queues the proper option name is n_rxq,
>> so requested_rx_queues has been renamed respectively. Tx queues
>> cannot be changed by the user, hence requested_tx_queues has been
>> dropped. Both configured_{rx,tx}_queues will be returned as
>> n_{r,t}xq in the get_status() callback.
>> vi yo
>
>> The documentation in vswitchd/vswitch.xml for status columns as well
>> as tests have been updated accordingly.
>>
>> Reported-at: https://bugzilla.redhat.com/1949855
>> Signed-off-by: Jakob Meng
>> ---
>> 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 | 49 +++-
>> 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, 150 insertions(+), 76 deletions(-)
>>
>
> Hi Jakob, some initial comments below.
>
Thank you ☺️
> It feels like a lot of changes in one patch, split across different netdevs.
> I wonder could it be broken up into patches for the different netdev types ?
> or even further like groups for related features, queues/rx-steering/flow
> control ? Not sure what works best without causing too much intermediate
> updates with UTs etc.
>
> Also, I'd suggest adding a cover letter with diff from previous version i.e.
> the thing I forgot last week :-)
>
Ack, will try to split this patch up for the next revision. But first, some
questions below ;)
>
>> diff --git a/Documentation/intro/install/afxdp.rst
>> b/Documentation/intro/install/afxdp.rst
>> index 51c24bf5b..5776614c8 100644
>> --- a/Documentation/intro/install/afxdp.rst
>> +++ b/Documentation/intro/install/afxdp.rst
>> @@ -219,14 +219,10 @@ Otherwise, enable debugging by::
>> ovs-appctl vlog/set netdev_afxdp::dbg
>> To check which XDP mode was chosen by ``best-effort``, you can look for
>> -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``::
>> -
>> - # ovs-appctl dpctl/show
>> - netdev@ovs-netdev:
>> - <...>
>> - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true,
>> - xdp-mode=best-effort,
>> - xdp-mode-in-use=native-with-zerocopy)
>> +``xdp-mode`` in the output of ``ovs-vsctl get interface INT
>> status:xdp-mode``::
>> +
>> + # ovs-vsctl get interface ens802f0 status:xdp-mode
>> + "native-with-zerocopy"
>> References
>> --
>> diff --git a/Documentation/topics/dpdk/phy.rst
>> b/Documentation/topics/dpdk/phy.rst
>> index f66b106c4..41cc3588a 100644
>> --- a/Documentation/topics/dpdk/phy.rst
>> +++ b/Documentation/topics/dpdk/phy.rst
>> @@ -198,7 +198,7 @@ Example::
>> a dedicated queue, it will be explicit::
>> $ ovs-vsctl get interface dpdk-p0 status
>> - {..., rx_steering=unsupported}
>> + {..., rx-steering=unsupported}
>>
>
> The fix is correct, but the meaning of unsupported is changed so text above
> (not shown in diff) is incorrect. Mentioning here but I think it will change
> in the status reporting and then the docs can be synced with that.
Good catch! We could expose "rx_steer_flows_num" but this would be less
self-explanatory than printing some kind of "unsupported". Any idea how to fix
this properly in the docs?
>
>> More details can often be found in ``ovs-vswitchd.log``::
>> @@ -499,7 +499,7 @@ its options::
>> $ ovs-appctl dpctl/show
>> [...]
>> - port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
&
Re: [ovs-dev] [PATCH, v4] netdev: Sync'ed and cleaned {get, set}_config().
On 09.10.23 15:27, Ilya Maximets wrote:
> On 10/8/23 10:00, Jakob Meng wrote:
>>
>> On 06.10.23 20:00, Ilya Maximets wrote:
>>> On 10/6/23 09:49, Jakob Meng wrote:
>>>> On 05.10.23 21:08, Ilya Maximets wrote:
>>>>> On 10/4/23 14:21, jm...@redhat.com wrote:
>>>>>> diff --git a/tests/pmd.at b/tests/pmd.at
>>>>>> index 7bdaca9e7..fb838286b 100644
>>>>>> --- a/tests/pmd.at
>>>>>> +++ b/tests/pmd.at
>>>>>> @@ -93,11 +93,11 @@ pmd thread numa_id core_id :
>>>>>>overhead: NOT AVAIL
>>>>>> ])
>>>>>>
>>>>>> -AT_CHECK([ovs-appctl dpif/show | sed
>>>>>> 's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
>>>>>> +AT_CHECK([ovs-appctl dpif/show | sed
>>>>>> 's/\(numa_id=\)[[0-9]]*/\1/g'], [0], [dnl
>>>>> Hi, Jakub. Could you explain what is going on here?
>>>>>
>>>>> I'm a bit confused. We cear out the number of tx_queues in some
>>>>> tests, because it is determined dynamically in the datapath. But
>>>>> why clearing the numa id instead? The numa id is not changing if
>>>>> not explicitly set in the port configuration, IIRC.
>>>>>
>>>>> Best regards, Ilya Maximets.
>>>> True, in pmd.at we use "--dummy-numa" to set the numa sockets and cores.
>>>> For most cases it is simply 0,0,0,0 or else numa_id is configured so
>>>> numa_id should be known to us (esp. because netdev-dummy simply stores the
>>>> numa id internally). I cleared numa_id because the surrounding code does
>>>> as well and for the sake of testing that the option is printed, it is
>>>> enough. But if you prefer I will replace with specific numa ids
>>>> in the next patch revision.
>>> We only clear numa_id in the PMD thread header but not in the port
>>> configuration. There is no point clearing it.
>> Will change it in next patchset but I am waiting for your full review first
>> ;)
>>> We do clear number of tx queues however, and I'm not sure why you
>>> switched to not doing that anymore. Looks like an unrelated change.
>> configured_tx_queues is no longer returned from get_config() and
>> requested_tx_queues has been renamed to n_txq.
>>
>> pmd.at uses netdev dummy. By default n_txq is initialized to 1 for netdevs
>> [0] and netdev-dummy copies this to requested_n_txq [1] which is then
>> returned as n_txq in netdev dummy's get_config().
>>
>> pmd.at does not change n_txq so we know that n_txq is always 1 and thus we
>> do not need the regex clearing?!
> True. I forgot I removed the implementation of .set_tx_multiq()
> callback from netdev-dummy 7 years ago [1]. :D
>
> So, yes, no clearing required in the current code. It will be
> needed only if we decide to implement this callback again in the
> future.
>
> [1] d66e4a5e7e10 ("netdev-dummy: Add n_txq option.")
7 years ago... 😱 At that time i was still coding in C++ and chasing the dream
of a phd😂
Send patch v5 without clearing *tx_queues nor numa_id:
https://patchwork.ozlabs.org/project/openvswitch/patch/20231011101105.21803-1-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v4] netdev: Sync'ed and cleaned {get, set}_config().
On 06.10.23 20:00, Ilya Maximets wrote: > On 10/6/23 09:49, Jakob Meng wrote: >> On 05.10.23 21:08, Ilya Maximets wrote: >>> On 10/4/23 14:21, jm...@redhat.com wrote: >>>> diff --git a/tests/pmd.at b/tests/pmd.at >>>> index 7bdaca9e7..fb838286b 100644 >>>> --- a/tests/pmd.at >>>> +++ b/tests/pmd.at >>>> @@ -93,11 +93,11 @@ pmd thread numa_id core_id : >>>>overhead: NOT AVAIL >>>> ]) >>>> >>>> -AT_CHECK([ovs-appctl dpif/show | sed >>>> 's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl >>>> +AT_CHECK([ovs-appctl dpif/show | sed >>>> 's/\(numa_id=\)[[0-9]]*/\1/g'], [0], [dnl >>> Hi, Jakub. Could you explain what is going on here? >>> >>> I'm a bit confused. We cear out the number of tx_queues in some >>> tests, because it is determined dynamically in the datapath. But >>> why clearing the numa id instead? The numa id is not changing if >>> not explicitly set in the port configuration, IIRC. >>> >>> Best regards, Ilya Maximets. >> True, in pmd.at we use "--dummy-numa" to set the numa sockets and cores. For >> most cases it is simply 0,0,0,0 or else numa_id is configured so numa_id >> should be known to us (esp. because netdev-dummy simply stores the numa id >> internally). I cleared numa_id because the surrounding code does as well and >> for the sake of testing that the option is printed, it is enough. But if you >> prefer I will replace with specific numa ids in the next patch >> revision. > We only clear numa_id in the PMD thread header but not in the port > configuration. There is no point clearing it. Will change it in next patchset but I am waiting for your full review first ;) > We do clear number of tx queues however, and I'm not sure why you > switched to not doing that anymore. Looks like an unrelated change. configured_tx_queues is no longer returned from get_config() and requested_tx_queues has been renamed to n_txq. pmd.at uses netdev dummy. By default n_txq is initialized to 1 for netdevs [0] and netdev-dummy copies this to requested_n_txq [1] which is then returned as n_txq in netdev dummy's get_config(). pmd.at does not change n_txq so we know that n_txq is always 1 and thus we do not need the regex clearing?! [0] https://github.com/openvswitch/ovs/blob/b78427639fa97ca46846c8bd9744f9006c77f641/lib/netdev.c#L439 [1] https://github.com/openvswitch/ovs/blob/b78427639fa97ca46846c8bd9744f9006c77f641/lib/netdev-dummy.c#L718 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/3] netdev-dpdk: Document rx-steering status options.
On 06.10.23 11:29, jm...@redhat.com wrote:
> From: Jakob Meng
>
> Fixes: fc06ea9a1883 ("netdev-dpdk: Add custom rx-steering configuration.")
> Signed-off-by: Jakob Meng
> ---
> vswitchd/vswitch.xml | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 006d1e6a4..1e2a1267d 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3806,6 +3806,20 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Ethernet address set for this VF interface. Only reported for
> dpdk
> VF representors.
>
> +
> +
> +Hardware Rx queue steering policy in use.
> +
> +
> +
> +ID of rx steering queue. Only reported if
> rx-steering
> +is supported by hardware.
> +
> +
> +
> +IDs of rss queues. Only reported if rx-steering is
> +supported by hardware.
> +
>
>
>
(Unchanged patch, adding acks from previous versions)
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
Acked-by: Kevin Traynor
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] netdev-dpdk: Document status options for VF MAC address.
On 06.10.23 11:29, jm...@redhat.com wrote:
> From: Jakob Meng
>
> Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address. ")
> Signed-off-by: Jakob Meng
> ---
> vswitchd/vswitch.xml | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index cfcde34ff..797fb05bf 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3805,6 +3805,10 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Device ID of PCI device.
>
>
> +
> +Ethernet address set for this VF interface. Only reported for
> dpdk
> +VF representors.
> +
>
>
>
(Unchanged patch, adding acks from previous versions)
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
Acked-by: Kevin Traynor
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-dpdk: Fixed DPDK specific interface status options.
On 06.10.23 11:17, Eelco Chaudron wrote:
>
> On 6 Oct 2023, at 11:10, Jakob Meng wrote:
>
> On 06.10.23 10:55, Eelco Chaudron wrote:
>> On 6 Oct 2023, at 9:21, Jakob Meng wrote:
>>
>>> On 05.10.23 17:53, Kevin Traynor wrote:
>>>> On 05/10/2023 13:52, Eelco Chaudron wrote:
>>>>> On 4 Oct 2023, at 10:31, jm...@redhat.com wrote:
>>>>>
>>>>>> From: Jakob Meng
>>>>>>
>>>>>> The status options pci-vendor_id and pci-device_id for dpdk netdevs
>>>>>> have been replaced by bus_info. This patch updates the documentation
>>>>>> in vswitchd/vswitch.xml accordingly.
>>>>>>
>>>>>> Fixes: a77c7796f23a ("dpdk: Update to use v22.11.1.")
>>>>> Thanks for fixing the documentation, however I think the commit
>>>>> message should somehow state this is related to the documentation.
>>>>>
>>>> Message it probably ok, but yeah title might mislead so should mention
>>>> docs.
>> I meant the subject of the commit message ;)
>>
>>> Maybe "netdev-dpdk: Update docs for interface info in
>>> vswitchd/vswitch.xml."?
>> This looks good to me! Maybe send a new rev with this subject fixed and
>> the added sign-off, and add my ACKs.
>>
>> Then Kevin, if you want, you can try to merge them.
> Thank you, done ☺️
>
> Patch v2:
> https://patchwork.ozlabs.org/project/openvswitch/patch/20231006090703.701871-1-jm...@redhat.com/
>
>
> One request, can you sent a v3 with for the whole series? If you make a
> change to a patch in a series, you should re-sent the entire series,
> including any ACKs added (or removed, if you made changes). This way we only
> need to track the latest version of the series, rather than each patch
> individually.
>
Done, v3 series:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=376419
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-dpdk: Fixed DPDK specific interface status options.
On 06.10.23 10:55, Eelco Chaudron wrote:
> On 6 Oct 2023, at 9:21, Jakob Meng wrote:
>
>> On 05.10.23 17:53, Kevin Traynor wrote:
>>> On 05/10/2023 13:52, Eelco Chaudron wrote:
>>>> On 4 Oct 2023, at 10:31, jm...@redhat.com wrote:
>>>>
>>>>> From: Jakob Meng
>>>>>
>>>>> The status options pci-vendor_id and pci-device_id for dpdk netdevs
>>>>> have been replaced by bus_info. This patch updates the documentation
>>>>> in vswitchd/vswitch.xml accordingly.
>>>>>
>>>>> Fixes: a77c7796f23a ("dpdk: Update to use v22.11.1.")
>>>> Thanks for fixing the documentation, however I think the commit message
>>>> should somehow state this is related to the documentation.
>>>>
>>> Message it probably ok, but yeah title might mislead so should mention docs.
> I meant the subject of the commit message ;)
>
>> Maybe "netdev-dpdk: Update docs for interface info in vswitchd/vswitch.xml."?
> This looks good to me! Maybe send a new rev with this subject fixed and the
> added sign-off, and add my ACKs.
>
> Then Kevin, if you want, you can try to merge them.
Thank you, done ☺️
Patch v2:
https://patchwork.ozlabs.org/project/openvswitch/patch/20231006090703.701871-1-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v4] netdev: Sync'ed and cleaned {get, set}_config().
On 05.10.23 21:08, Ilya Maximets wrote: > On 10/4/23 14:21, jm...@redhat.com wrote: >> diff --git a/tests/pmd.at b/tests/pmd.at >> index 7bdaca9e7..fb838286b 100644 >> --- a/tests/pmd.at >> +++ b/tests/pmd.at >> @@ -93,11 +93,11 @@ pmd thread numa_id core_id : >>overhead: NOT AVAIL >> ]) >> >> -AT_CHECK([ovs-appctl dpif/show | sed >> 's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl >> +AT_CHECK([ovs-appctl dpif/show | sed >> 's/\(numa_id=\)[[0-9]]*/\1/g'], [0], [dnl > Hi, Jakub. Could you explain what is going on here? > > I'm a bit confused. We cear out the number of tx_queues in some > tests, because it is determined dynamically in the datapath. But > why clearing the numa id instead? The numa id is not changing if > not explicitly set in the port configuration, IIRC. > > Best regards, Ilya Maximets. True, in pmd.at we use "--dummy-numa" to set the numa sockets and cores. For most cases it is simply 0,0,0,0 or else numa_id is configured so numa_id should be known to us (esp. because netdev-dummy simply stores the numa id internally). I cleared numa_id because the surrounding code does as well and for the sake of testing that the option is printed, it is enough. But if you prefer I will replace with specific numa ids in the next patch revision. Best, Jakob ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-dpdk: Fixed DPDK specific interface status options.
On 05.10.23 17:53, Kevin Traynor wrote:
> On 05/10/2023 13:52, Eelco Chaudron wrote:
>> On 4 Oct 2023, at 10:31, jm...@redhat.com wrote:
>>
>>> From: Jakob Meng
>>>
>>> The status options pci-vendor_id and pci-device_id for dpdk netdevs
>>> have been replaced by bus_info. This patch updates the documentation
>>> in vswitchd/vswitch.xml accordingly.
>>>
>>> Fixes: a77c7796f23a ("dpdk: Update to use v22.11.1.")
>>
>> Thanks for fixing the documentation, however I think the commit message
>> should somehow state this is related to the documentation.
>>
>
> Message it probably ok, but yeah title might mislead so should mention docs.
Maybe "netdev-dpdk: Update docs for interface info in vswitchd/vswitch.xml."?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v3] netdev: Sync'ed and cleaned {get, set}_config().
On 05.10.23 09:42, Simon Horman wrote:
> On Wed, Oct 04, 2023 at 02:27:17PM +0200, Jakob Meng wrote:
>> On 04.10.23 12:18, Ilya Maximets wrote:
>>> On 10/2/23 16:44, David Marchand wrote:
>>>> On Mon, Oct 2, 2023 at 1:52 PM Simon Horman wrote:
>>>>> On Wed, Sep 27, 2023 at 03:24:07PM +0200, jm...@redhat.com wrote:
>>>>>> From: Jakob Meng
>>>>>>
>>>>>> For better usability, the function pairs get_config() and
>>>>>> set_config() for each netdev 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 moves key-value pairs which are no valid options from
>>>>>> get_config() to the get_status() callback. For example, get_config()
>>>>>> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
>>>>>> previously. For requested rx queues the proper option name is n_rxq,
>>>>>> so requested_rx_queues has been renamed respectively. Tx queues
>>>>>> cannot be changed by the user, hence requested_tx_queues has been
>>>>>> dropped. Both configured_{rx,tx}_queues will be returned as
>>>>>> n_{r,t}xq in the get_status() callback.
>>>>>>
>>>>>> The documentation in vswitchd/vswitch.xml for status columns as well
>>>>>> as tests have been updated accordingly.
>>>>>>
>>>>>> Reported-at: https://bugzilla.redhat.com/1949855
>>>>>> Signed-off-by: Jakob Meng
>>>>> Hi Jacob,
>>>>>
>>>>> I see some CI failures with this change,
>>>>> but I'm unsure if they are related to the change or not.
>>>>>
>>>>> Could you take a look?
>>>> I can reproduce issues on vhost-user port mtu tests.
>>>>
>>>> I understand we have a race now.
>>>>
>>>> Previously, the test was calling an explicit dpctl/show which
>>>> synchronuously invoked each port get_config(), then matching this
>>>> command output against mtu=9000.
>>>> Now with this patch, the test looks at the interface status field in ovsdb.
>>>> My understanding is that this status field may not have been refreshed
>>>> *yet* (or there is another issue and I missed some sync point..).
>>>>
>>>>
>>>> Adding a sleep 1 before "ovs-vsctl get Interface dpdkvhostuserclient0
>>>> mtu" does the trick for me...
>>>>
>>>> I tested adding a check on the vhost-user port link status (see below).
>>>> Link status of a vhu port is supposed to be up once the port has been
>>>> reconfigured (vhost_reconfigured == which means that
>>>> netdev_dpdk_mempool_configure() has been called).
>>>>
>>>> This seems to work, though I never noticed a "down" status in my runs.
>>>> So maybe adding one ovs-vsctl command was enough to put some delay in the
>>>> test..
>>>>
>>>> $ git diff
>>>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
>>>> index 95aa3e906b..59fec6903f 100644
>>>> --- a/tests/system-dpdk.at
>>>> +++ b/tests/system-dpdk.at
>>>> @@ -749,6 +749,7 @@ tail -f /dev/null | dpdk-testpmd
>>>> --socket-mem="$(cat NUMA_NODE)" --no-pci\
>>>> --single-file-segments -- -a
>>>>> $OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>>>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing"
>>>> ovs-vswitchd.log])
>>>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0
>>>> link_state | grep -w up])
>>>>
>>>> dnl Check MTU value in the datapath
>>>> AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>>>> @@ -887,6 +888,7 @@ tail -f /dev/null | dpdk-testpmd
>>>> --socket-mem="$(cat NUMA_NODE)" --no-pci\
>>>> --single-file-segments -- -a
>>>>> $OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>>>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing"
>>>> ovs-vswitchd.log])
>>>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0
>>>> link_state | grep -w up])
>>>>
>>>> dnl Check MTU value in the datapath
>>>> AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>>> Thanks, David. Yeah, there is a slight delay for the database update.
>>> Waiting for the port to come 'up' seems reasonable.
>>>
>>> Best regards, Ilya Maximets.
>>>
>> Thank you all for your suggestions and David for finding a fix for the DPDK
>> tests ☺️
>>
>> I have incorporated everything into patch v4 which is out now:
>>
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20231004122145.734313-1-jm...@redhat.com/
>>
> Thanks, I concur that v4 addresses the issues discussed here.
>
Yes, v4 applies all of your suggestions: It includes David's fix to wait until
ports are up and follows Ilya's suggestions to skip reporting default values
for config options, to fix indentation and to drop the docs for dummy
ports/datapaths.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v3] netdev: Sync'ed and cleaned {get, set}_config().
On 04.10.23 12:18, Ilya Maximets wrote:
> On 10/2/23 16:44, David Marchand wrote:
>> On Mon, Oct 2, 2023 at 1:52 PM Simon Horman wrote:
>>> On Wed, Sep 27, 2023 at 03:24:07PM +0200, jm...@redhat.com wrote:
>>>> From: Jakob Meng
>>>>
>>>> For better usability, the function pairs get_config() and
>>>> set_config() for each netdev 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 moves key-value pairs which are no valid options from
>>>> get_config() to the get_status() callback. For example, get_config()
>>>> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
>>>> previously. For requested rx queues the proper option name is n_rxq,
>>>> so requested_rx_queues has been renamed respectively. Tx queues
>>>> cannot be changed by the user, hence requested_tx_queues has been
>>>> dropped. Both configured_{rx,tx}_queues will be returned as
>>>> n_{r,t}xq in the get_status() callback.
>>>>
>>>> The documentation in vswitchd/vswitch.xml for status columns as well
>>>> as tests have been updated accordingly.
>>>>
>>>> Reported-at: https://bugzilla.redhat.com/1949855
>>>> Signed-off-by: Jakob Meng
>>> Hi Jacob,
>>>
>>> I see some CI failures with this change,
>>> but I'm unsure if they are related to the change or not.
>>>
>>> Could you take a look?
>> I can reproduce issues on vhost-user port mtu tests.
>>
>> I understand we have a race now.
>>
>> Previously, the test was calling an explicit dpctl/show which
>> synchronuously invoked each port get_config(), then matching this
>> command output against mtu=9000.
>> Now with this patch, the test looks at the interface status field in ovsdb.
>> My understanding is that this status field may not have been refreshed
>> *yet* (or there is another issue and I missed some sync point..).
>>
>>
>> Adding a sleep 1 before "ovs-vsctl get Interface dpdkvhostuserclient0
>> mtu" does the trick for me...
>>
>> I tested adding a check on the vhost-user port link status (see below).
>> Link status of a vhu port is supposed to be up once the port has been
>> reconfigured (vhost_reconfigured == which means that
>> netdev_dpdk_mempool_configure() has been called).
>>
>> This seems to work, though I never noticed a "down" status in my runs.
>> So maybe adding one ovs-vsctl command was enough to put some delay in the
>> test..
>>
>> $ git diff
>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
>> index 95aa3e906b..59fec6903f 100644
>> --- a/tests/system-dpdk.at
>> +++ b/tests/system-dpdk.at
>> @@ -749,6 +749,7 @@ tail -f /dev/null | dpdk-testpmd
>> --socket-mem="$(cat NUMA_NODE)" --no-pci\
>> --single-file-segments -- -a
>>> $OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0
>> link_state | grep -w up])
>>
>> dnl Check MTU value in the datapath
>> AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
>> @@ -887,6 +888,7 @@ tail -f /dev/null | dpdk-testpmd
>> --socket-mem="$(cat NUMA_NODE)" --no-pci\
>> --single-file-segments -- -a
>>> $OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0
>> link_state | grep -w up])
>>
>> dnl Check MTU value in the datapath
>> AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
> Thanks, David. Yeah, there is a slight delay for the database update.
> Waiting for the port to come 'up' seems reasonable.
>
> Best regards, Ilya Maximets.
>
Thank you all for your suggestions and David for finding a fix for the DPDK
tests ☺️
I have incorporated everything into patch v4 which is out now:
https://patchwork.ozlabs.org/project/openvswitch/patch/20231004122145.734313-1-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/3] netdev-dpdk: Document rx-steering status options.
On 04.10.23 10:32, jm...@redhat.com wrote:
> From: Jakob Meng
>
> Fixes: fc06ea9a1883 ("netdev-dpdk: Add custom rx-steering configuration.")
> ---
> vswitchd/vswitch.xml | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 006d1e6a4..1e2a1267d 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3806,6 +3806,20 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Ethernet address set for this VF interface. Only reported for
> dpdk
> VF representors.
>
> +
> +
> +Hardware Rx queue steering policy in use.
> +
> +
> +
> +ID of rx steering queue. Only reported if
> rx-steering
> +is supported by hardware.
> +
> +
> +
> +IDs of rss queues. Only reported if rx-steering is
> +supported by hardware.
> +
>
>
>
Signed-off-by: Jakob Meng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-dpdk: Fixed DPDK specific interface status options.
On 04.10.23 10:31, jm...@redhat.com wrote:
> From: Jakob Meng
>
> The status options pci-vendor_id and pci-device_id for dpdk netdevs
> have been replaced by bus_info. This patch updates the documentation
> in vswitchd/vswitch.xml accordingly.
>
> Fixes: a77c7796f23a ("dpdk: Update to use v22.11.1.")
> ---
> vswitchd/vswitch.xml | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 797fb05bf..006d1e6a4 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3797,12 +3797,9 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Interface description string.
>
>
> -
> -Vendor ID of PCI device.
> -
> -
> -
> -Device ID of PCI device.
> +
> +Bus name and bus info such as Vendor ID and Device ID of PCI
> +device.
>
>
>
Signed-off-by: Jakob Meng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] netdev-dpdk: Document status options for VF MAC address.
On 04.10.23 10:26, jm...@redhat.com wrote:
> From: Jakob Meng
>
> Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address. ")
> ---
> vswitchd/vswitch.xml | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index cfcde34ff..797fb05bf 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3805,6 +3805,10 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Device ID of PCI device.
>
>
> +
> +Ethernet address set for this VF interface. Only reported for
> dpdk
> + VF representors.
> +
>
>
>
Signed-off-by: Jakob Meng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev: Fixed DPDK specific interface status options.
On 29.09.23 17:05, Kevin Traynor wrote:
> On 28/09/2023 08:50, Simon Horman wrote:
>> On Wed, Sep 27, 2023 at 01:46:19PM +0200, jm...@redhat.com wrote:
>>> From: Jakob Meng
>>>
>>> The documentation in vswitchd/vswitch.xml for status columns has been
>>> updated to reflect recent and not so recent changes to dpdk netdevs.
>>> For example, status columns pci-vendor_id and pci-device_id have been
>>> replaced with bus_info in order to sync changes from [0].
>>>
>>> [0]
>>> https://github.com/openvswitch/ovs/commit/a77c7796f23a76190b61e2109a009df980253b0f
>>
>> Hi Jakob,
>>
>> I wonder if it would be useful to provide the following fixes tags and
>> reword the patch description slightly to take into account their presence.
>>
>
> +1. Also, not exactly a critical backport, but if backporting they would be
> for slightly different branches, so could split the fixes into a 1/2 and 2/2.
>
> (btw, I noticed that v3 of the "netdev: Sync'ed and cleaned {get,
> set}_config()." series also still contains these fixes - I think you meant to
> remove from there)
>
>>
>> Fixes: a77c7796f23a ("dpdk: Update to use v22.11.1.")
>
> relevant for branches 3.1/3.2
>
>> Fixes: fc06ea9a1883 ("netdev-dpdk: Add custom rx-steering configuration.")
>>
>
> relevant for branch 3.2
>
Thank you for your feedback ☺️ Patch has been split into:
[ovs-dev,3/3] netdev-dpdk: Document rx-steering status options.
<https://patchwork.ozlabs.org/project/openvswitch/patch/20231004083221.29507-1-jm...@redhat.com/>
https://patchwork.ozlabs.org/project/openvswitch/patch/20231004082628.28712-1-jm...@redhat.com/
[ovs-dev,2/3] netdev-dpdk: Fixed DPDK specific interface status options.
<https://patchwork.ozlabs.org/project/openvswitch/patch/20231004083124.29206-1-jm...@redhat.com/>
https://patchwork.ozlabs.org/project/openvswitch/patch/20231004083124.29206-1-jm...@redhat.com/
[ovs-dev,1/3] netdev-dpdk: Document status options for VF MAC address.
<https://patchwork.ozlabs.org/project/openvswitch/patch/20231004082628.28712-1-jm...@redhat.com/>
https://patchwork.ozlabs.org/project/openvswitch/patch/20231004083221.29507-1-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v2] netdev: Sync'ed and cleaned {get, set}_config() and get_status()
CI discovered some code locations I had missed previously, so I had to upload a new patch v3: https://patchwork.ozlabs.org/project/openvswitch/patch/20230927132406.2076833-1-jm...@redhat.com/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v2] netdev: Sync'ed and cleaned {get, set}_config() and get_status()
On 27.09.23 13:16, David Marchand wrote: > On Wed, Sep 27, 2023 at 12:11 PM Jakob Meng wrote: >> The documentation in vswitchd/vswitch.xml for status columns has been >> updated accordingly. Status columns pci-vendor_id and pci-device_id >> have been replaced with bus_info in order to sync changes from [0]. > Quick comment. > This part should probably go to a separate patch (this is backport > material) in a next revision. As suggested by David, I have moved the doc fixes related to previous commits to a separate patch: https://patchwork.ozlabs.org/project/openvswitch/patch/20230927114619.2068486-1-jm...@redhat.com/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev: Sync'ed and cleaned {get, set}_config() and get_status()
On 27.09.23 13:16, David Marchand wrote: > On Wed, Sep 27, 2023 at 12:11 PM Jakob Meng wrote: >> The documentation in vswitchd/vswitch.xml for status columns has been >> updated accordingly. Status columns pci-vendor_id and pci-device_id >> have been replaced with bus_info in order to sync changes from [0]. > Quick comment. > This part should probably go to a separate patch (this is backport > material) in a next revision. > > ack, done: https://patchwork.ozlabs.org/project/openvswitch/patch/20230927114619.2068486-1-jm...@redhat.com/ thank you ☺️ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] netdev: Sync'ed and cleaned {get, set}_config() and get_status()
For better usability, the function pairs get_config() and
set_config() for each netdev 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 moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns has been
updated accordingly. Status columns pci-vendor_id and pci-device_id
have been replaced with bus_info in order to sync changes from [0].
[0]
https://github.com/openvswitch/ovs/commit/a77c7796f23a76190b61e2109a009df980253b0f
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
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 | 49 ++
lib/netdev-dummy.c| 17 --
lib/netdev-linux-private.h| 2 +
lib/netdev-linux.c| 4 +-
tests/bridge.at | 14 ++---
tests/dpctl.at| 12 ++---
tests/mcast-snooping.at | 6 +--
tests/mpls-xlate.at | 34 ++--
tests/netdev-type.at | 4 +-
tests/nsh.at | 20
tests/ofproto-dpif.at | 36 ++---
tests/ovs-ofctl.at| 4 +-
tests/ovs-vswitchd.at | 6 +--
tests/packet-type-aware.at| 50 +-
tests/pmd.at | 42 +++
tests/system-layer3-tunnels.at| 8 +--
tests/tunnel-push-pop-ipv6.at | 18 +++
tests/tunnel-push-pop.at | 18 +++
tests/tunnel.at | 74 +--
vswitchd/vswitch.xml | 69 +++--
24 files changed, 310 insertions(+), 215 deletions(-)
diff --git a/Documentation/intro/install/afxdp.rst
b/Documentation/intro/install/afxdp.rst
index 51c24bf5b..4153e5bc0 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -219,14 +219,10 @@ Otherwise, enable debugging by::
ovs-appctl vlog/set netdev_afxdp::dbg
To check which XDP mode was chosen by ``best-effort``, you can look for
-``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``::
-
- # ovs-appctl dpctl/show
- netdev@ovs-netdev:
-<...>
-port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true,
- xdp-mode=best-effort,
- xdp-mode-in-use=native-with-zerocopy)
+``xdp-mode`` in the output of ``ovs-vsctl get interface NAME
status:xdp-mode``::
+
+ # ovs-vsctl get interface ens802f0 status:xdp-mode
+ "native-with-zerocopy"
References
--
diff --git a/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc30..3bd3dff4a 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct
smap *args)
ovs_mutex_lock(&dev->mutex);
smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name);
-smap_add_format(args, "xdp-mode-in-use", "%s",
-xdp_modes[dev->xdp_mode_in_use].name);
smap_add_format(args, "use-need-wakeup", "%s",
dev->use_need_wakeup ? "true" : "false");
ovs_mutex_unlock(&dev->mutex);
@@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev,
return error;
}
+
+int
