Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.
On Wed, Oct 25, 2023 at 11:33 PM Ilya Maximets wrote: > > On 10/25/23 11:45, Simon Horman wrote: > > On Sat, Oct 21, 2023 at 05:04:48PM +0200, Frode Nordahl wrote: > >> Many system tests currently use ping with the combination of a > >> low packet count (-c 3), short interval between sends (-i 0.3) > >> and a _deadline_ of 2 seconds (-d 2). > >> > >> This combination of options may lead to a situation where more > >> than count packets are sent however ping will stop when count > >> packets are received. This results in a failed test due to how > >> the result is checked, for example: > >> > >> ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING > >> @@ -1,2 +1,2 @@ > >> -3 packets transmitted, 3 received, 0% packet loss, time 0ms > >> +4 packets transmitted, 3 received, 25% packet loss, time 0ms > >> > >> To reiterate, in the above example there is no packet loss, but > >> ping stops after _receiving_ 3 packets, not bothering with > >> waiting for the response to the fourth packet it just sent out. > >> > >> If we look at the iputils ping manual for the -w deadline option > >> we can read that this is expected behavior: > >> > >>> Specify a timeout, in seconds, before ping exits regardless of > >>> how many packets have been sent or received. In this case ping > >>> does not stop after count packet are sent, it waits either for > >>> deadline expire or until count probes are answered or for some > >>> error notification from network. > >> > >> To avoid these kinds of failures in checks where a response is > >> expected, we replace ping -w with ping -W. > >> > >> We keep ping -w for checks where it is expected to NOT get a > >> response. > >> > >> Signed-off-by: Frode Nordahl > > > > Thanks Frode, > > > > TIL about -w and -W. > > I learned about -W as well. :) > > Thanks, Frode, for figuring out the cause of these failures! I've seen > them before, but didn't dig too deep to find a cause. OVN also has them > from time to time. yw, we run all the system tests in an automated fashion as part of the openvswitch package regression testing, and we see them quite frequently, most likely due to the load of the CI infrastructure. > Though I'm not sure if -W is the right choice. Reading the description: > >-W timeout >Time to wait for a response, in seconds. The >option affects only timeout in absence of any >responses, otherwise ping waits for two RTTs. >Real number allowed with dot as a decimal >separator (regardless locale setup). 0 means >infinite timeout. > > And I don't really like the 'in absence of ANY responses' part of it. > > So, IIUC, if we send 3 packets, first gets replied and the other two > are dropped somewhere, ping will ignore the timeout and will wait > indefinitely. Unfortunately, OVS gives the first packet a special > treatment, so potential for this scenario to happen is rather high. > This may break CI systems, getting them stuck testing one patch. And > it doesn't seem like we can mix -w and -W, at least the behavior is > not really defined in this case. It also says "otherwise ping waits for two RTTs.", so it will not wait indefinitely. The documentation is a bit convoluted though so I went to look so that we can be sure about what it will do. On arrival of the first packet, ping will gather various information [0] which will be used to compute the RTT [1], which is used when initializing the waittime [2][3]. So it appears to me -W would cover the scenario laid out above, i.e. if we get one reply quickly and the rest are lost, the computed RTT would have a ping exit within a reasonable timeframe. Even if the first response comes near the timeout value, the RTT would not be more than 6 seconds for a -W of 3. 0: https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping.c#L1654-L1660 1: https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L761 2: https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L599 3: https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L263-L268 > Would be really nice to use fping instead that has simple and very > straightforward arguments without side effects, but once again RHEL > doesn't package it... > > Maybe we could use '$ timeout 2 ping6 -q -c 3 -i 0.3 fc00::3' instead? That would also work, either option works for me, what would be your preference? > Another option might be to slightly reduce the deadline, so the 4th > packet will not be sent. But that sounds fragile. Agreed. -- Frode Nordahl > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests/system-traffic: Ensure no name resolution for tcpdump.
On 10/25/23 11:43, Simon Horman wrote: > On Sat, Oct 21, 2023 at 01:22:08AM +0200, Frode Nordahl wrote: >> Depending on system configuration, executing tcpdump without the >> -n parameter, may prolong the execution time for tcpdump while it >> attempts name resolution. >> >> This delay may in turn lead to test failures due to contents of >> tables to check being evicted. >> >> We recently started to see this problem with the >> "conntrack -IPv6 ICMP6 Related with SNAT" test. >> >> For consistency, this patch adds the -n parameter to all tcpdump >> calls in system-traffic.at. >> >> Signed-off-by: Frode Nordahl > > Thanks Frode, > > I think that in general tcpdump -n is good practice unless lookup > is really needed. > > Acked-by: Simon Horman Thanks, Frode and Simon! Applied and backported down to 2.17, since it's a test fix. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.
On 10/25/23 11:45, Simon Horman wrote: > On Sat, Oct 21, 2023 at 05:04:48PM +0200, Frode Nordahl wrote: >> Many system tests currently use ping with the combination of a >> low packet count (-c 3), short interval between sends (-i 0.3) >> and a _deadline_ of 2 seconds (-d 2). >> >> This combination of options may lead to a situation where more >> than count packets are sent however ping will stop when count >> packets are received. This results in a failed test due to how >> the result is checked, for example: >> >> ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING >> @@ -1,2 +1,2 @@ >> -3 packets transmitted, 3 received, 0% packet loss, time 0ms >> +4 packets transmitted, 3 received, 25% packet loss, time 0ms >> >> To reiterate, in the above example there is no packet loss, but >> ping stops after _receiving_ 3 packets, not bothering with >> waiting for the response to the fourth packet it just sent out. >> >> If we look at the iputils ping manual for the -w deadline option >> we can read that this is expected behavior: >> >>> Specify a timeout, in seconds, before ping exits regardless of >>> how many packets have been sent or received. In this case ping >>> does not stop after count packet are sent, it waits either for >>> deadline expire or until count probes are answered or for some >>> error notification from network. >> >> To avoid these kinds of failures in checks where a response is >> expected, we replace ping -w with ping -W. >> >> We keep ping -w for checks where it is expected to NOT get a >> response. >> >> Signed-off-by: Frode Nordahl > > Thanks Frode, > > TIL about -w and -W. I learned about -W as well. :) Thanks, Frode, for figuring out the cause of these failures! I've seen them before, but didn't dig too deep to find a cause. OVN also has them from time to time. Though I'm not sure if -W is the right choice. Reading the description: -W timeout Time to wait for a response, in seconds. The option affects only timeout in absence of any responses, otherwise ping waits for two RTTs. Real number allowed with dot as a decimal separator (regardless locale setup). 0 means infinite timeout. And I don't really like the 'in absence of ANY responses' part of it. So, IIUC, if we send 3 packets, first gets replied and the other two are dropped somewhere, ping will ignore the timeout and will wait indefinitely. Unfortunately, OVS gives the first packet a special treatment, so potential for this scenario to happen is rather high. This may break CI systems, getting them stuck testing one patch. And it doesn't seem like we can mix -w and -W, at least the behavior is not really defined in this case. Would be really nice to use fping instead that has simple and very straightforward arguments without side effects, but once again RHEL doesn't package it... Maybe we could use '$ timeout 2 ping6 -q -c 3 -i 0.3 fc00::3' instead? Another option might be to slightly reduce the deadline, so the 4th packet will not be sent. But that sounds fragile. Best regards, Ilya Maximets. ___ 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 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 ovn] northd: Support CIDR-based MAC binding aging threshold.
On 10/24/23 21:35, Han Zhou wrote:
> Enhance MAC_Binding aging to allow CIDR-based threshold configurations.
> This enables distinct threshold settings for different IP ranges,
> applying the longest prefix matching for overlapping ranges.
>
> A common use case involves setting a default threshold for all IPs, while
> disabling aging for a specific range and potentially excluding a subset
> within that range.
>
> Signed-off-by: Han Zhou
> ---
> northd/aging.c | 297 +---
> northd/aging.h | 3 +
> northd/northd.c | 11 +-
> ovn-nb.xml | 63 +-
> tests/ovn.at| 60 ++
> 5 files changed, 413 insertions(+), 21 deletions(-)
>
> diff --git a/northd/aging.c b/northd/aging.c
> index f626c72c8ca3..e5868211a63b 100644
> --- a/northd/aging.c
> +++ b/northd/aging.c
> @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct aging_waker
> *waker, int64_t next_wake_ms)
> }
> }
>
> +struct threshold_entry {
> +union {
> +ovs_be32 ipv4;
> +struct in6_addr ipv6;
> +} prefix;
> +bool is_v4;
Hi, Han. Thanks for the patch!
Not a full review, but I wonder if it would be cleaner to replace
all the structure members above with a single 'struct in6_addr prefix;'
and store ipv4 addresses as ipv6-mapped ipv4. This will allow to use
a single array for storing the entries as well. May save some lines
of code.
What do you think?
Also, this patch, probably, needs a NEWS entry.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Add the configuration file.
On 10/25/23 17:23, Ilya Maximets wrote: > On 10/25/23 15:20, Aaron Conole wrote: >> Ilya Maximets writes: >> >>> 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. >>> >>> Signed-off-by: Ilya Maximets >>> --- >> >> Looks like it stopped working about a month ago. Good future idea for a >> bot to monitor this part. >> >> Anyway: >> >> Acked-by: Aaron Conole > > Thanks! Applied. Backported this to 3.2 as well, because rtd also builds the 'stable' version from the latest tag. It will rebuild the docs once the new tag (v3.2.2) is available. > > And the build passed! > > Best regards, Ilya Maximets. ___ 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 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.)
> For dpdk_vhost_client_class both config options
> vhost-server-path and tx-retries-max are returned which were missed in
> the previous patch version.
>
> Jakob Meng (3):
> netdev-dpdk: Sync and clean {get,set}_config() callbacks.
> netdev-dummy: Sync and clean {get,set}_config() callbacks.
> netdev-afxdp: Sync and clean {get,set}_config() callbacks.
>
> Documentation/intro/install/afxdp.rst | 12 +--
> Documentation/topics/dpdk/phy.rst | 4 +-
> lib/netdev-afxdp.c | 21 +-
> lib/netdev-afxdp.h | 1 +
> lib/netdev-dpdk.c | 104 ++
> lib/netdev-dummy.c | 19 -
> lib/netdev-linux-private.h | 1 +
> lib/netdev-linux.c | 4 +-
> tests/pmd.at | 26 +++
> tests/system-dpdk.at | 64 ++--
> vswitchd/vswitch.xml | 25 ++-
> 11 files changed, 193 insertions(+), 88 deletions(-)
>
Hi Jakob,
The output looks good to me. Just a few minor code related comments on the
code.
@previous reviewers/committers, if anyone else is intending to review
before Jakob respins for possibly the final version, please shout now!
As it is user visible change, it's probably worth to put a note in the
NEWS so users can quickly spot if they notice a change.
Best to mention the commands/output that changed ('ovs-appctl dpctl/show'
and 'ovs-vsctl get Interface status') and say briefly
that you've aligned set_confg/get_config and updated status etc.
Suggest to not to bother mentioning specific netdevs and just do in one
update, maybe in first patch?
thanks,
Kevin.
>>>
>>> Good idea. How about appending this to NEWS?
>>>
>>> Post-v3.2.0
>>>
>>> - OVSDB:
>>> * ...
>>> - ovs-appctl:
>>> * 'ovs-appctl dpctl/show' has been changed to print valid config
>>> options
It is an appctl section, no need to repeat.
>>> only: {configured,requested}_{rx,tx}_queues and 'rx_csum_offload'
>>> have
>>> been dropped. Now, 'n_rxq' will be used for requested rx queues. Tx
>>> queues cannot be changed by the user, hence 'requested_tx_queues'
>>> has
>>> been dropped. 'lsc_interrupt_mode' has been renamed to option name
>>> 'dpdk-lsc-interrupt'.
>>> Use 'ovs-vsctl get interface *** status' to query for values which
>>> have
>>> previously been returned by 'ovs-appctl dpctl/show'. For example,
>>> use
>>> 'ovs-vsctl get interface *** status:n_txq' to get what was
>>> previously
>>> returned by 'configured_tx_queues'.
>>> * 'ovs-appctl dpctl/show' will now print all available config options
>>> like
>>> 'dpdk-devargs', '{rx,tx}-flow-ctrl-{autoneg}', 'vhost-server-path'
>>> and
>>> 'tx-retries-max' if they are set.
This doesn't seem to be entirely true. The flow control stuff
is always reported even if not set by the user. Should we maybe
avoid reporting default values for these somehow?
>>>
>>>
>>> Wdyt?
>>>
>>
>> I was thinking something shorter without mentioning individual configs
>> would be sufficient, but I can see the benefit of listing the individual
>> changed configs th
Re: [ovs-dev] [PATCH] README: Add documentation build status badge.
On 10/25/23 15:22, Aaron Conole wrote: > Ilya Maximets writes: > >> This should make it a little more visible that documentation >> build fails on ReadTheDocs. >> >> Signed-off-by: Ilya Maximets >> --- > > NEAT! I guess without the other readthedocs patch the badge will show > failure. But it does add a way to monitor the docs build. > > Acked-by: Aaron Conole Thanks! Applied. And it's already green. :) Best regards, Ilya Maximets. > >> README.rst | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/README.rst b/README.rst >> index e6c0d3d30..a2c234f4d 100644 >> --- a/README.rst >> +++ b/README.rst >> @@ -12,6 +12,8 @@ Open vSwitch >> :target: https://ci.appveyor.com/project/blp/ovs/history >> .. image:: https://api.cirrus-ci.com/github/openvswitch/ovs.svg >> :target: https://cirrus-ci.com/github/openvswitch/ovs >> +.. image:: >> https://readthedocs.org/projects/openvswitch/badge/?version=latest >> +:target: https://docs.openvswitch.org/en/latest/ >> >> What is Open vSwitch? >> - > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Add the configuration file.
On 10/25/23 15:20, Aaron Conole wrote: > Ilya Maximets writes: > >> 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. >> >> Signed-off-by: Ilya Maximets >> --- > > Looks like it stopped working about a month ago. Good future idea for a > bot to monitor this part. > > Anyway: > > Acked-by: Aaron Conole Thanks! Applied. And the build passed! Best regards, Ilya Maximets. ___ 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 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
>
> What do you think?
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 1/8] system-dpdk: Introduce helpers for testpmd.
On Wed, Oct 25, 2023 at 2:50 PM Aaron Conole wrote:
>
> David Marchand writes:
>
> > On Mon, Oct 23, 2023 at 10:20 AM David Marchand
> > wrote:
> >> +# OVS_DPDK_CHECK_TESTPMD()
> >> +#
> >> +# Check dpdk-testpmd availability.
> >> +#
> >> +m4_define([OVS_DPDK_CHECK_TESTPMD],
> >> + [AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null])
> >> +])
> >> +
> >> +
> >> +# OVS_DPDK_START_TESTPMD()
> >> +#
> >> +# Start dpdk-testpmd in background.
> >> +#
> >> +m4_define([OVS_DPDK_START_TESTPMD],
> >> + [AT_CHECK([lscpu], [], [stdout])
> >> + AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while
> >> (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE])
> >> + eal_options="--socket-mem="$(cat NUMA_NODE)" --file-prefix page0
> >> --single-file-segments --no-pci"
> >> + options="$1"
> >> + [ "$options" != "${options%% -- *}" ] || options="$options -- "
> >
> > I realised, looking at a generated dpdk testsuite file (while trying
> > to understand a Intel CI failure), that this syntax above is wrong.
> >
> > It is not a big problem, since testpmd (/getopt) does not complain
> > about such a trailing --.
> > Yet, better to avoid [] and instead use a "if test ...; then ...; fi"
> > construct.
>
> I think we need to use [[...]] to get the behavior you intend under m4,
> but also we can use test (since '[' is usually either an alias or a
> reimplementation of 'test').
I don't mind.
I see both uses of "test " and some [[]] in tests/.
But this file only used test so far... so I would tend to go with it.
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] README: Add documentation build status badge.
Ilya Maximets writes: > This should make it a little more visible that documentation > build fails on ReadTheDocs. > > Signed-off-by: Ilya Maximets > --- NEAT! I guess without the other readthedocs patch the badge will show failure. But it does add a way to monitor the docs build. Acked-by: Aaron Conole > README.rst | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/README.rst b/README.rst > index e6c0d3d30..a2c234f4d 100644 > --- a/README.rst > +++ b/README.rst > @@ -12,6 +12,8 @@ Open vSwitch > :target: https://ci.appveyor.com/project/blp/ovs/history > .. image:: https://api.cirrus-ci.com/github/openvswitch/ovs.svg > :target: https://cirrus-ci.com/github/openvswitch/ovs > +.. image:: https://readthedocs.org/projects/openvswitch/badge/?version=latest > +:target: https://docs.openvswitch.org/en/latest/ > > What is Open vSwitch? > - ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] readthedocs: Add the configuration file.
Ilya Maximets writes: > 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. > > Signed-off-by: Ilya Maximets > --- Looks like it stopped working about a month ago. Good future idea for a bot to monitor this part. Anyway: Acked-by: Aaron Conole > .readthedocs.yaml | 24 > Documentation/requirements.txt | 2 +- > Makefile.am| 1 + > 3 files changed, 26 insertions(+), 1 deletion(-) > create mode 100644 .readthedocs.yaml > > diff --git a/.readthedocs.yaml b/.readthedocs.yaml > new file mode 100644 > index 0..e481e64f1 > --- /dev/null > +++ b/.readthedocs.yaml > @@ -0,0 +1,24 @@ > +# .readthedocs.yaml > +# Read the Docs configuration file. > +# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details. > + > +# Required. > +version: 2 > + > +# Set the OS, Python version, etc. > +build: > + os: ubuntu-22.04 > + tools: > +python: "3.12" > + > +# Build documentation in the "Documentation/" directory with Sphinx. > +sphinx: > + configuration: Documentation/conf.py > + > +# Build all formats: HTML, PDF, ePub. > +formats: all > + > +# Declare the Python requirements. > +python: > + install: > + - requirements: Documentation/requirements.txt > diff --git a/Documentation/requirements.txt b/Documentation/requirements.txt > index 77130c6e0..77f44bd76 100644 > --- a/Documentation/requirements.txt > +++ b/Documentation/requirements.txt > @@ -1,2 +1,2 @@ > -sphinx>=1.1,<2.0 > +sphinx>=1.1 > ovs_sphinx_theme>=1.0,<1.1 > diff --git a/Makefile.am b/Makefile.am > index 439e2bf6d..94f488d18 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -84,6 +84,7 @@ EXTRA_DIST = \ > .cirrus.yml \ > .editorconfig \ > .github/workflows/build-and-test.yml \ > + .readthedocs.yaml \ > appveyor.yml \ > boot.sh \ > poc/builders/Vagrantfile \ ___ 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.
Jakob Meng writes: > 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? Ideally both - since we should be able to catch that we're in a string in many instances. That said, it gets a bit more complicated in practice because inserting without a leading '"' or trailing '"' means we don't have the context (unless we do something like apply the patch and then scan the lines keeping the context). We usually these kinds of errors since they are obviously tool limitations. ___ 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.
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.
What do you think?
___
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 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 5/6] ct-dpif: Enforce CT zone limit protection.
On 10/18/23 09:56, Ales Musil wrote:
> Make sure that if any zone limit was set via DB
> all zones are forced to be set there also. This
> is done by tracking which datapath has zone limit
> protection and it is reflected in the dpctl command.
>
> If the datapath is protected the dpctl command will
> return permission error.
>
> Signed-off-by: Ales Musil
> ---
> v5: Rebase on top of current master.
> Address comments from Ilya:
> - Add more user friendly error message to the dpctl.
> - Fix style related problems.
> v4: Rebase on top of current master.
> Make the protection datapath wide.
> ---
> lib/ct-dpif.c | 27 +++
> lib/ct-dpif.h | 2 ++
> lib/dpctl.c| 12
> ofproto/ofproto-dpif.c | 14 ++
> ofproto/ofproto-provider.h | 5 +
> ofproto/ofproto.c | 11 +++
> ofproto/ofproto.h | 2 ++
> tests/system-traffic.at| 36
> vswitchd/bridge.c | 7 +++
> 9 files changed, 116 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 2ee045164..41d2dc4d7 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -23,6 +23,7 @@
> #include "openvswitch/ofp-ct.h"
> #include "openvswitch/ofp-parse.h"
> #include "openvswitch/vlog.h"
> +#include "sset.h"
>
> VLOG_DEFINE_THIS_MODULE(ct_dpif);
>
> @@ -32,6 +33,10 @@ struct flags {
> const char *name;
> };
>
> +/* Protection for CT zone limit per datapath. */
> +static struct sset ct_limit_protection =
> +SSET_INITIALIZER(&ct_limit_protection);
> +
> static void ct_dpif_format_counters(struct ds *,
> const struct ct_dpif_counters *);
> static void ct_dpif_format_timestamp(struct ds *,
> @@ -1064,3 +1069,25 @@ ct_dpif_get_features(struct dpif *dpif, enum
> ct_features *features)
> ? dpif->dpif_class->ct_get_features(dpif, features)
> : EOPNOTSUPP);
> }
> +
> +void
> +ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected)
> +{
> +if (sset_contains(&ct_limit_protection, dpif->full_name) == protected) {
> +return;
> +}
> +
> +if (protected) {
> +sset_add(&ct_limit_protection, dpif->full_name);
> +} else {
> +sset_find_and_delete(&ct_limit_protection, dpif->full_name);
> +}
> +VLOG_INFO("The CT zone limit protection is %s for \"%s\".",
> + protected ? "enabled" : "disabled", dpif->full_name);
This message is only useful for users who already use dpctl.
And they will see the error while trying to use it. I don't
think we need this message in the log file.
> +}
> +
> +bool
> +ct_dpif_is_zone_limit_protected(struct dpif *dpif)
> +{
> +return sset_contains(&ct_limit_protection, dpif->full_name);
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index c8a7c155e..c3786d5ae 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -350,5 +350,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif *dpif,
> uint32_t tp_id,
> uint16_t dl_type, uint8_t nw_proto,
> char **tp_name, bool *is_generic);
> int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features);
> +void ct_dpif_set_zone_limit_protection(struct dpif *, bool protected);
> +bool ct_dpif_is_zone_limit_protected(struct dpif *);
>
> #endif /* CT_DPIF_H */
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index a8c654747..8c87ff9e8 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2234,6 +2234,12 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
> }
>
> +if (ct_dpif_is_zone_limit_protected(dpif)) {
> +ds_put_cstr(&ds, "the zone limits are set via DB");
I'd suggest something more user-friendly, e.g. "The zone limits are set
via database, use 'ovs-vsctl set-zone-limit <...>' instead."
> +error = EPERM;
> +goto error;
> +}
> +
> error = ct_dpif_set_limits(dpif, &zone_limits);
> if (!error) {
> ct_dpif_free_zone_limits(&zone_limits);
> @@ -2310,6 +2316,12 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> }
> }
>
> +if (ct_dpif_is_zone_limit_protected(dpif)) {
> +ds_put_cstr(&ds, "the zone limits are set via DB");
The same here, but with 'del-zone-limit'.
> +error = EPERM;
> +goto error;
> +}
> +
> error = ct_dpif_del_limits(dpif, &zone_limits);
> if (!error) {
> goto out;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6a931a806..7c5360b67 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5663,6 +5663,19 @@ ct_zone_limits_commit(struct dpif_backer *backer)
> }
> }
>
> +static void
> +ct_zone_limit_protection_update(const char *datapath_type, bool protected)
> +{
> +struct dpif_backer *backer = shash_find_data(&all_dp
Re: [ovs-dev] [PATCH v7 1/8] system-dpdk: Introduce helpers for testpmd.
David Marchand writes:
> On Mon, Oct 23, 2023 at 10:20 AM David Marchand
> wrote:
>> +# OVS_DPDK_CHECK_TESTPMD()
>> +#
>> +# Check dpdk-testpmd availability.
>> +#
>> +m4_define([OVS_DPDK_CHECK_TESTPMD],
>> + [AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null])
>> +])
>> +
>> +
>> +# OVS_DPDK_START_TESTPMD()
>> +#
>> +# Start dpdk-testpmd in background.
>> +#
>> +m4_define([OVS_DPDK_START_TESTPMD],
>> + [AT_CHECK([lscpu], [], [stdout])
>> + AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while
>> (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE])
>> + eal_options="--socket-mem="$(cat NUMA_NODE)" --file-prefix page0
>> --single-file-segments --no-pci"
>> + options="$1"
>> + [ "$options" != "${options%% -- *}" ] || options="$options -- "
>
> I realised, looking at a generated dpdk testsuite file (while trying
> to understand a Intel CI failure), that this syntax above is wrong.
>
> It is not a big problem, since testpmd (/getopt) does not complain
> about such a trailing --.
> Yet, better to avoid [] and instead use a "if test ...; then ...; fi"
> construct.
I think we need to use [[...]] to get the behavior you intend under m4,
but also we can use test (since '[' is usually either an alias or a
reimplementation of 'test').
WDYT?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 4/6] vswitchd, ofproto-dpif: Propagate the CT limit from database.
On 10/18/23 09:56, Ales Musil wrote:
> Propagate the CT limit that is present in the DB into
> datapath. The limit is currently only propagated on change
> and can be overwritten by the dpctl commands.
>
> Signed-off-by: Ales Musil
> ---
> v5: Rebase on top of current master.
> Address comments from Ilya:
> - Make sure the zones are always removed.
> - Fix style related problems.
> - Make sure the limit is initialized to -1.
> v4: Rebase on top of current master.
> Make sure that the values from DB are propagated only if set. That
> applies to both limit and policies.
> ---
> ofproto/ofproto-dpif.c | 39 +
> ofproto/ofproto-dpif.h | 5
> ofproto/ofproto-provider.h | 4 +++
> ofproto/ofproto.c | 12
> ofproto/ofproto.h | 2 ++
> tests/system-traffic.at| 54 +++
> vswitchd/bridge.c | 58 +++---
> 7 files changed, 164 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ba5706f6a..6a931a806 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -220,6 +220,7 @@ static void ofproto_unixctl_init(void);
> static void ct_zone_config_init(struct dpif_backer *backer);
> static void ct_zone_config_uninit(struct dpif_backer *backer);
> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> +static void ct_zone_limits_commit(struct dpif_backer *backer);
>
> static inline struct ofproto_dpif *
> ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -513,6 +514,7 @@ type_run(const char *type)
>
> process_dpif_port_changes(backer);
> ct_zone_timeout_policy_sweep(backer);
> +ct_zone_limits_commit(backer);
>
> return 0;
> }
> @@ -5522,6 +5524,8 @@ ct_zone_config_init(struct dpif_backer *backer)
> cmap_init(&backer->ct_zones);
> hmap_init(&backer->ct_tps);
> ovs_list_init(&backer->ct_tp_kill_list);
> +ovs_list_init(&backer->ct_zone_limits_to_add);
> +ovs_list_init(&backer->ct_zone_limits_to_del);
> clear_existing_ct_timeout_policies(backer);
> }
>
> @@ -5545,6 +5549,8 @@ ct_zone_config_uninit(struct dpif_backer *backer)
> id_pool_destroy(backer->tp_ids);
> cmap_destroy(&backer->ct_zones);
> hmap_destroy(&backer->ct_tps);
> +ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> +ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> }
>
> static void
> @@ -5625,6 +5631,38 @@ ct_del_zone_timeout_policy(const char *datapath_type,
> uint16_t zone_id)
> }
> }
>
> +static void
> +ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
> + int64_t *limit)
> +{
> +struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> + datapath_type);
> +if (!backer) {
> +return;
> +}
> +
> +if (limit) {
> +ct_dpif_push_zone_limit(&backer->ct_zone_limits_to_add, zone_id,
> +*limit, 0);
> +} else {
> +ct_dpif_push_zone_limit(&backer->ct_zone_limits_to_del, zone_id, 0,
> 0);
> +}
> +}
> +
> +static void
> +ct_zone_limits_commit(struct dpif_backer *backer)
> +{
> +if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> +ct_dpif_set_limits(backer->dpif, &backer->ct_zone_limits_to_add);
> +ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> +}
> +
> +if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
> +ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
> +ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> +}
> +}
> +
> static void
> get_datapath_cap(const char *datapath_type, struct smap *cap)
> {
> @@ -6914,4 +6952,5 @@ const struct ofproto_class ofproto_dpif_class = {
> ct_flush, /* ct_flush */
> ct_set_zone_timeout_policy,
> ct_del_zone_timeout_policy,
> +ct_zone_limit_update,
> };
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d8e0cd37a..b863dd6fc 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -284,6 +284,11 @@ struct dpif_backer {
> feature than 'bt_support'. */
>
> struct atomic_count tnl_count;
> +
> +struct ovs_list ct_zone_limits_to_add; /* CT zone limits queued for
> + * addition into datapath. */
> +struct ovs_list ct_zone_limits_to_del; /* CT zone limt queued for
> + * deletion from datapath. */
> };
>
> /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 9f7b8b6e8..33fb99280 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1921,6 +1921,10 @@ struct
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] [PATCH v5 3/6] ovs-vsctl: Add limit to CT zone.
On 10/18/23 09:56, Ales Musil wrote:
> Add limit to the CT zone DB table with ovs-vsctl
> helper methods. The limit has two special values
> besides any number, 0 is unlimited and empty limit
> is to leave the value untouched in the datapath.
>
> This is preparation step and the value is not yet
> propagated to the datapath.
>
> Signed-off-by: Ales Musil
> ---
> v5: Rebase on top of current master.
> Address comments from Ilya:
> - Use only single command for setting zone and default limit.
> - Correct the errors in the man page.
> - Use references for the column description.
> v4: Rebase on top of current master.
> Address comments from Ilya:
> - Make sure that the NEWS is clear on what has been added.
> - Make the usage of --may-exist and --if-exists more intuitive for the
> new commands.
> - Some cosmetics.
> Add command and column for default limit.
> ---
> NEWS | 5 ++
> tests/ovs-vsctl.at | 96 -
> utilities/ovs-vsctl.8.in | 35 +++--
> utilities/ovs-vsctl.c | 142 +++--
> vswitchd/vswitch.ovsschema | 14 +++-
> vswitchd/vswitch.xml | 13
> 6 files changed, 289 insertions(+), 16 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 7bc27b687..d5c9d9f04 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,11 @@ Post-v3.2.0
> - ovs-appctl:
> * Added support removal of default CT zone limit, e.g.
> "ovs-appctl dpctl/ct-del-limits default".
> + - ovs-vsctl:
> + * New commands 'add-zone-limit', 'del-zone-limit' and 'list-zone-limit'
I see the set-zone-limit in the code.
FWIW, I think the 'set' semantics is better for this case than 'add'.
> + to manage the maximum number of connections in conntrack zones via
> + a new 'limit' column in the 'CT_Zone' database table and
> + 'ct_zone_default_limit' column in the 'Datapath' table.
>
>
> v3.2.0 - 17 Aug 2023
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index a368bff6e..43d0ec80b 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -975,6 +975,67 @@ AT_CHECK(
>[0], [stdout])
> AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:10, Timeout
> Policies: system default
> ])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=10])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev zone=1 limit=1])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 1, Limit: 1
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist set-zone-limit netdev zone=1 limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 1, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=1])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev zone=10 limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=10 icmp_first=1
> icmp_reply=2])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> +Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> +Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev zone=10 limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> +Zone:10, Timeout Policies: system default
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev default limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Default limit: 5
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist set-zone-limit netdev default
> limit=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Default limit: 10
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev default])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-limit netdev default])])
> +
>
> AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0
> 'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])],
> [0], [stdout])
> AT_CHECK([RUN_OVS_VSCTL([list-dp-cap system])], [0], [recirc=true
> @@ -1113,16 +1174,47 @@ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1
> icmp_first=1 icmp_reply=2])
> ])
> AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])])
> AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp
Re: [ovs-dev] [ovs-build] |fail| pw1853561 [ovs-dev, v7, 8/8] system-dpdk: Run traffic tests.
On Wed, Oct 25, 2023 at 1:18 PM Ilya Maximets wrote: > On 10/25/23 12:09, David Marchand wrote: > >>> 2023-10-23T15:02:13.756Z|00082|bridge|INFO|bridge br10: deleted interface > >>> dpdkvhostuserclient0 on port 1 > >>> 2023-10-23T15:02:13.756Z|00083|dpif_netdev|INFO|PMD thread on numa_id: 1, > >>> core id: 88 destroyed. > >>> 2023-10-23T15:02:13.772Z|2|dpdk(pmd-c88/id:103)|INFO|PMD thread > >>> released DPDK lcore 2. > >>> 2023-10-23T15:02:13.778Z|00084|dpif_netdev|INFO|PMD thread on numa_id: 0, > >>> core id: 21 destroyed. > >>> 2023-10-23T15:02:13.778Z|2|ofproto_dpif_xlate(pmd-c21/id:102)|WARN|received > >>> packet on unknown port 1 on bridge br10 while processing > >>> icmp6,in_port=1,vlan_tci=0x,dl_src=ca:76:e9:ff:a2:09,dl_dst=33:33:00:00:00:02,ipv6_src=fe80::c876:e9ff:feff:a209,ipv6_dst=ff02::2,ipv6_label=0x0,nw_tos=0,nw_ecn=0,nw_ttl=255,nw_frag=no,icmp_type=133,icmp_code=0 > >>> 2023-10-23T15:02:13.791Z|3|dpdk(pmd-c21/id:102)|INFO|PMD thread > >>> released DPDK lcore 1. > >>> 2023-10-23T15:02:13.801Z|00085|dpdk|INFO|VHOST_CONFIG: > >>> (/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0) free > >>> connfd 95 > >>> 2023-10-23T15:02:13.801Z|00086|netdev_dpdk|INFO|vHost Device > >>> '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' not > >>> found > > > > I am a bit puzzled at this report. > > It is similar to > > https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/396325.html. > > > > I understand this shows a race in OVS cleaning up sequence, with some > > packet (triggering an upcall) received by a pmd on a port that is not > > referenced in the ofproto bridge anymore. > > Why did it show up again? This is probably due to my patch 7 in the v7 > > series which lets testpmd sends packets while deleting the vhu port. > > > > The easiest (laziest?) for me is probably to drop this patch 7 and > > instead waive warnings about a vhu socket reconnection... > > The packets are coming from the kernel interface on the other side > of testpmd, right? In that case, can we just bring that interface > down before removing OVS port to prevent random ipv6 traffic from > flowing around? Another similar option might be to set admin state > DOWN on the OVS side for the vhost-user port. Putting down the tap iface should do the job yes. But now I wonder why we need such a setup with testpmd + a tap in the mtu unit tests: no packet is being actively injected by the unit tests themselves. I get that testpmd will make sure that the vhost-user client port is running in a "nominal" situation when changing the mtu, so ok to keep it. But can we remove those tap iface from testpmd (for those MTU tests)? > > > But I find it strange that there is a window in which OVS pmd threads > > still poll packets (and complain) while the ports are being removed. > > OpenFlow ports are getting removed before their backing datapath ports, > so there is always a small window where packets can arrive on datapath > ports that do not have associated OpenFlow port numbers anymore. > Reversing this might not be an option due to reference counting, but I > don't remember exactly. > > Same applies to upcalls in kenrel datapath, because packets can be queued > for upcall while the port is getting removed. And it's even trickier to > fix that for a kernel, because it's done fully asynchronously. Ok, thanks for the context / explanations. -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
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.")
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
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 2/6] dpctl: Allow the default CT zone limit to de deleted.
On 10/18/23 09:56, Ales Musil wrote: > Add optional argument to dpctl ct-del-limits called > "default", which allows to remove the default limit > making it effectively system default. > > Signed-off-by: Ales Musil > --- > v5: Rebase on top of current master. > Address comments from Ilya: > - Correct the NEWS entry. > - Fix style related problems. > --- > NEWS| 3 +++ > lib/dpctl.c | 21 +++-- > tests/system-traffic.at | 26 ++ > 3 files changed, 44 insertions(+), 6 deletions(-) Hi, Ales. Thanks for the patch! It may need some extra changes in zone_limit_delete() though. While removing a defualt zone it will now log zone '-1', which is not particularly user-friendly. Also, the second time we'll try to remove the default limit the command will fail unable to find the entry for the default zone. It probably shouldn't fail, because default zone limit is always there, even if unlimited. We do always report a default value even if it wasn't configured. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 1/6] ct-dpif: Handle default zone limit the same way as other limits.
On 10/18/23 09:56, Ales Musil wrote:
> Internally handle default CT zone limit as other limits that
> can be passed via the list with special value -1. Currently,
> the -1 is treated by both datapaths as default, add static
> asserts to make sure that this remains the case in the future.
> This allows us to easily delete the default zone limit.
>
> Signed-off-by: Ales Musil
> ---
> v5: Rebase on top of current master.
> Address comments from Ilya:
> - Fix some typos.
> - Use OVS_ZONE_LIMIT_DEFAULT_ZONE instead of special constant.
> - Do not relay on DEFAULT_ZONE being -1 for the limit list.
> - Fix wrong netlink message.
> ---
> lib/conntrack.c | 2 +-
> lib/conntrack.h | 5 +++--
> lib/ct-dpif.c | 28 +++-
> lib/ct-dpif.h | 14 ++
> lib/dpctl.c | 15 ---
> lib/dpif-netdev.c | 21 ++---
> lib/dpif-netlink.c | 26 +-
> lib/dpif-provider.h | 16 +++-
> 8 files changed, 51 insertions(+), 76 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 47a443fba..31f00a127 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -398,7 +398,7 @@ zone_limit_clean(struct conntrack *ct, struct zone_limit
> *zl)
> }
>
> int
> -zone_limit_delete(struct conntrack *ct, uint16_t zone)
> +zone_limit_delete(struct conntrack *ct, int32_t zone)
> {
> ovs_mutex_lock(&ct->ct_lock);
> struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 57d5159b6..a8df4f78b 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -122,7 +122,8 @@ struct timeout_policy {
>
> enum {
> INVALID_ZONE = -2,
> -DEFAULT_ZONE = -1, /* Default zone for zone limit management. */
> +DEFAULT_ZONE = OVS_ZONE_LIMIT_DEFAULT_ZONE, /* Default zone for zone
> + * limit management. */
> MIN_ZONE = 0,
> MAX_ZONE = 0x,
> };
We may want to add an assertion here that DEFAULT_ZONE doesn't
overlap with other values.
> @@ -154,6 +155,6 @@ struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
> struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
> int32_t zone);
> int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit);
> -int zone_limit_delete(struct conntrack *ct, uint16_t zone);
> +int zone_limit_delete(struct conntrack *ct, int32_t zone);
>
> #endif /* conntrack.h */
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index f59c6e560..2ee045164 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -398,23 +398,19 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool
> *enabled)
> }
>
> int
> -ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> - const struct ovs_list *zone_limits)
> +ct_dpif_set_limits(struct dpif *dpif, const struct ovs_list *zone_limits)
> {
> return (dpif->dpif_class->ct_set_limits
> -? dpif->dpif_class->ct_set_limits(dpif, default_limit,
> - zone_limits)
> +? dpif->dpif_class->ct_set_limits(dpif, zone_limits)
> : EOPNOTSUPP);
> }
>
> int
> -ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
> - const struct ovs_list *zone_limits_in,
> +ct_dpif_get_limits(struct dpif *dpif, const struct ovs_list *zone_limits_in,
> struct ovs_list *zone_limits_out)
> {
> return (dpif->dpif_class->ct_get_limits
> -? dpif->dpif_class->ct_get_limits(dpif, default_limit,
> - zone_limits_in,
> +? dpif->dpif_class->ct_get_limits(dpif, zone_limits_in,
>zone_limits_out)
> : EOPNOTSUPP);
> }
> @@ -854,7 +850,7 @@ ct_dpif_format_tcp_stat(struct ds * ds, int tcp_state,
> int conn_per_state)
>
>
> void
> -ct_dpif_push_zone_limit(struct ovs_list *zone_limits, uint16_t zone,
> +ct_dpif_push_zone_limit(struct ovs_list *zone_limits, int32_t zone,
> uint32_t limit, uint32_t count)
> {
> struct ct_dpif_zone_limit *zone_limit = xmalloc(sizeof *zone_limit);
> @@ -928,15 +924,21 @@ error:
> }
>
> void
> -ct_dpif_format_zone_limits(uint32_t default_limit,
> - const struct ovs_list *zone_limits, struct ds *ds)
> +ct_dpif_format_zone_limits(const struct ovs_list *zone_limits, struct ds *ds)
> {
> struct ct_dpif_zone_limit *zone_limit;
>
> -ds_put_format(ds, "default limit=%"PRIu32, default_limit);
> +LIST_FOR_EACH (zone_limit, node, zone_limits) {
> +if (zone_limit->zone == OVS_ZONE_LIMIT_DEFAULT_ZONE) {
> +ds_put_format(ds, "default limit=%"PRIu32, zone_limit->limit);
> +}
> +}
>
> LIST_FOR_EACH (zone_limit, node, zone_limits) {
> -ds_put_
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] [ovs-build] |fail| pw1853561 [ovs-dev, v7, 8/8] system-dpdk: Run traffic tests.
On 10/25/23 12:09, David Marchand wrote: > Forwarding to dev@ > > On Mon, Oct 23, 2023 at 6:05 PM wrote: >>> 2023-10-23T15:02:12.622Z|00063|dpdk|INFO|VHOST_CONFIG: >>> (/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0) virtio >>> is now ready for processing. >>> 2023-10-23T15:02:12.622Z|00064|netdev_dpdk|INFO|vHost Device >>> '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' has >>> been added on numa node 0 >>> 2023-10-23T15:02:13.592Z|00074|dpif_netdev|INFO|Performing pmd to rx queue >>> assignment using cycles algorithm. >>> 2023-10-23T15:02:13.592Z|00075|dpif_netdev|INFO|Core 21 on numa node 0 >>> assigned port 'dpdkvhostuserclient0' rx queue 0 (measured processing cycles >>> 0). >>> 2023-10-23T15:02:13.592Z|1|netdev_dpdk(ovs_vhost2)|INFO|State of queue >>> 0 ( tx_qid 0 ) of vhost device >>> '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' >>> changed to 'enabled' >>> 2023-10-23T15:02:13.592Z|2|netdev_dpdk(ovs_vhost2)|INFO|State of queue >>> 1 ( rx_qid 0 ) of vhost device >>> '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' >>> changed to 'enabled' >>> 2023-10-23T15:02:13.595Z|00076|unixctl|DBG|received request dpctl/show[], >>> id=0 >>> 2023-10-23T15:02:13.596Z|00077|unixctl|DBG|replying with success, id=0: >>> "netdev@ovs-netdev: >>> lookups: hit:0 missed:2 lost:0 >>> flows: 2 >>> port 0: ovs-netdev (tap) >>> port 1: br10 (tap) >>> port 2: dpdkvhostuserclient0 (dpdkvhostuserclient: >>> configured_rx_queues=1, configured_tx_queues=1, mtu=9000, >>> requested_rx_queues=1, requested_tx_queues=1) >>> " >>> 2023-10-23T15:02:13.715Z|00078|dpif_netdev|INFO|Performing pmd to rx queue >>> assignment using cycles algorithm. >>> 2023-10-23T15:02:13.715Z|00079|dpif_netdev|INFO|Core 21 on numa node 0 >>> assigned port 'dpdkvhostuserclient0' rx queue 0 (measured processing cycles >>> 0). >>> 2023-10-23T15:02:13.728Z|00080|unixctl|DBG|received request dpctl/show[], >>> id=0 >>> 2023-10-23T15:02:13.728Z|00081|unixctl|DBG|replying with success, id=0: >>> "netdev@ovs-netdev: >>> lookups: hit:0 missed:2 lost:0 >>> flows: 2 >>> port 0: ovs-netdev (tap) >>> port 1: br10 (tap) >>> port 2: dpdkvhostuserclient0 (dpdkvhostuserclient: >>> configured_rx_queues=1, configured_tx_queues=1, mtu=2000, >>> requested_rx_queues=1, requested_tx_queues=1) >>> " >>> 2023-10-23T15:02:13.756Z|00082|bridge|INFO|bridge br10: deleted interface >>> dpdkvhostuserclient0 on port 1 >>> 2023-10-23T15:02:13.756Z|00083|dpif_netdev|INFO|PMD thread on numa_id: 1, >>> core id: 88 destroyed. >>> 2023-10-23T15:02:13.772Z|2|dpdk(pmd-c88/id:103)|INFO|PMD thread >>> released DPDK lcore 2. >>> 2023-10-23T15:02:13.778Z|00084|dpif_netdev|INFO|PMD thread on numa_id: 0, >>> core id: 21 destroyed. >>> 2023-10-23T15:02:13.778Z|2|ofproto_dpif_xlate(pmd-c21/id:102)|WARN|received >>> packet on unknown port 1 on bridge br10 while processing >>> icmp6,in_port=1,vlan_tci=0x,dl_src=ca:76:e9:ff:a2:09,dl_dst=33:33:00:00:00:02,ipv6_src=fe80::c876:e9ff:feff:a209,ipv6_dst=ff02::2,ipv6_label=0x0,nw_tos=0,nw_ecn=0,nw_ttl=255,nw_frag=no,icmp_type=133,icmp_code=0 >>> 2023-10-23T15:02:13.791Z|3|dpdk(pmd-c21/id:102)|INFO|PMD thread >>> released DPDK lcore 1. >>> 2023-10-23T15:02:13.801Z|00085|dpdk|INFO|VHOST_CONFIG: >>> (/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0) free >>> connfd 95 >>> 2023-10-23T15:02:13.801Z|00086|netdev_dpdk|INFO|vHost Device >>> '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' not >>> found > > I am a bit puzzled at this report. > It is similar to > https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/396325.html. > > I understand this shows a race in OVS cleaning up sequence, with some > packet (triggering an upcall) received by a pmd on a port that is not > referenced in the ofproto bridge anymore. > Why did it show up again? This is probably due to my patch 7 in the v7 > series which lets testpmd sends packets while deleting the vhu port. > > The easiest (laziest?) for me is probably to drop this patch 7 and > instead waive warnings about a vhu socket reconnection... The packets are coming from the kernel interface on the other side of testpmd, right? In that case, can we just bring that interface down before removing OVS port to prevent random ipv6 traffic from flowing around? Another similar option might be to set admin state DOWN on the OVS side for the vhost-user port. > But I find it strange that there is a window in which OVS pmd threads > still poll packets (and complain) while the ports are being removed. OpenFlow ports are getting removed before their backing datapath ports, so there is always a small window where packets can arrive on datapath ports that do not have associated OpenFlow port numbers anymore. Reversing this might not be an option due to reference counting, but I don't remember exactly. Same applies to upcalls in kenrel
Re: [ovs-dev] [ovs-build] |fail| pw1853561 [ovs-dev, v7, 8/8] system-dpdk: Run traffic tests.
Forwarding to dev@ On Mon, Oct 23, 2023 at 6:05 PM wrote: > > 2023-10-23T15:02:12.622Z|00063|dpdk|INFO|VHOST_CONFIG: > > (/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0) virtio > > is now ready for processing. > > 2023-10-23T15:02:12.622Z|00064|netdev_dpdk|INFO|vHost Device > > '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' has > > been added on numa node 0 > > 2023-10-23T15:02:13.592Z|00074|dpif_netdev|INFO|Performing pmd to rx queue > > assignment using cycles algorithm. > > 2023-10-23T15:02:13.592Z|00075|dpif_netdev|INFO|Core 21 on numa node 0 > > assigned port 'dpdkvhostuserclient0' rx queue 0 (measured processing cycles > > 0). > > 2023-10-23T15:02:13.592Z|1|netdev_dpdk(ovs_vhost2)|INFO|State of queue > > 0 ( tx_qid 0 ) of vhost device > > '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' > > changed to 'enabled' > > 2023-10-23T15:02:13.592Z|2|netdev_dpdk(ovs_vhost2)|INFO|State of queue > > 1 ( rx_qid 0 ) of vhost device > > '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' > > changed to 'enabled' > > 2023-10-23T15:02:13.595Z|00076|unixctl|DBG|received request dpctl/show[], > > id=0 > > 2023-10-23T15:02:13.596Z|00077|unixctl|DBG|replying with success, id=0: > > "netdev@ovs-netdev: > > lookups: hit:0 missed:2 lost:0 > > flows: 2 > > port 0: ovs-netdev (tap) > > port 1: br10 (tap) > > port 2: dpdkvhostuserclient0 (dpdkvhostuserclient: > > configured_rx_queues=1, configured_tx_queues=1, mtu=9000, > > requested_rx_queues=1, requested_tx_queues=1) > > " > > 2023-10-23T15:02:13.715Z|00078|dpif_netdev|INFO|Performing pmd to rx queue > > assignment using cycles algorithm. > > 2023-10-23T15:02:13.715Z|00079|dpif_netdev|INFO|Core 21 on numa node 0 > > assigned port 'dpdkvhostuserclient0' rx queue 0 (measured processing cycles > > 0). > > 2023-10-23T15:02:13.728Z|00080|unixctl|DBG|received request dpctl/show[], > > id=0 > > 2023-10-23T15:02:13.728Z|00081|unixctl|DBG|replying with success, id=0: > > "netdev@ovs-netdev: > > lookups: hit:0 missed:2 lost:0 > > flows: 2 > > port 0: ovs-netdev (tap) > > port 1: br10 (tap) > > port 2: dpdkvhostuserclient0 (dpdkvhostuserclient: > > configured_rx_queues=1, configured_tx_queues=1, mtu=2000, > > requested_rx_queues=1, requested_tx_queues=1) > > " > > 2023-10-23T15:02:13.756Z|00082|bridge|INFO|bridge br10: deleted interface > > dpdkvhostuserclient0 on port 1 > > 2023-10-23T15:02:13.756Z|00083|dpif_netdev|INFO|PMD thread on numa_id: 1, > > core id: 88 destroyed. > > 2023-10-23T15:02:13.772Z|2|dpdk(pmd-c88/id:103)|INFO|PMD thread > > released DPDK lcore 2. > > 2023-10-23T15:02:13.778Z|00084|dpif_netdev|INFO|PMD thread on numa_id: 0, > > core id: 21 destroyed. > > 2023-10-23T15:02:13.778Z|2|ofproto_dpif_xlate(pmd-c21/id:102)|WARN|received > > packet on unknown port 1 on bridge br10 while processing > > icmp6,in_port=1,vlan_tci=0x,dl_src=ca:76:e9:ff:a2:09,dl_dst=33:33:00:00:00:02,ipv6_src=fe80::c876:e9ff:feff:a209,ipv6_dst=ff02::2,ipv6_label=0x0,nw_tos=0,nw_ecn=0,nw_ttl=255,nw_frag=no,icmp_type=133,icmp_code=0 > > 2023-10-23T15:02:13.791Z|3|dpdk(pmd-c21/id:102)|INFO|PMD thread > > released DPDK lcore 1. > > 2023-10-23T15:02:13.801Z|00085|dpdk|INFO|VHOST_CONFIG: > > (/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0) free > > connfd 95 > > 2023-10-23T15:02:13.801Z|00086|netdev_dpdk|INFO|vHost Device > > '/root/ovs-dev/tests/system-dpdk-testsuite.dir/017/dpdkvhostclient0' not > > found I am a bit puzzled at this report. It is similar to https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/396325.html. I understand this shows a race in OVS cleaning up sequence, with some packet (triggering an upcall) received by a pmd on a port that is not referenced in the ofproto bridge anymore. Why did it show up again? This is probably due to my patch 7 in the v7 series which lets testpmd sends packets while deleting the vhu port. The easiest (laziest?) for me is probably to drop this patch 7 and instead waive warnings about a vhu socket reconnection... But I find it strange that there is a window in which OVS pmd threads still poll packets (and complain) while the ports are being removed. Opinions? -- David Marchand ___ 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.
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 Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 5/5] netdev-offload: Fix Clang's static analyzer 'Division by zero' warnings.
On Mon, Oct 23, 2023 at 04:22:47PM +0200, Eelco Chaudron wrote:
> When enabling DPDK with the configure the below, ovs-vswitchd will crash.
>
> ovs-vsctl set Open_vSwitch . other_config:n-offload-threads=0
> ovs-vsctl set Open_vSwitch . other_config:hw-offload=true
>
> This issue arises because setting the 'n-offload-threads' value to zero
> is not a supported configuration. This fix addresses this by implementing
> a check to ensure a valid 'n-offload-threads' value, both during
> configuration and statistics gathering.
>
> Fixes: 62c2d8a67543 ("netdev-offload: Add multi-thread API.")
> Signed-off-by: Eelco Chaudron
Hi Eelco,
Thanks for addressing my concerns regarding the patch description in v1.
This looks good to me.
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/5] ovsdb: Fix Clang's static analyzer 'func null dereference' warnings.
On Mon, Oct 23, 2023 at 04:22:39PM +0200, Eelco Chaudron wrote: > In the existing code, there is no existing path that would result > in a crash. Therefore, this code is currently implemented to > satisfy Clang's requirements. Nevertheless, it serves the > additional purpose of preventing issues with potential new use > cases of the ovsdb_mutation_set_execute() API. > > Signed-off-by: Eelco Chaudron Hi Eelco, Thanks for addressing my concerns regarding the patch description in v1. This looks good to me. Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 3/5] ofp-table: Fix count_common_prefix_run() function.
On Mon, Oct 23, 2023 at 04:22:32PM +0200, Eelco Chaudron wrote:
> It appears that an issue existed in the count_common_prefix_run()
> function from the beginning. This problem came to light while
> addressing 'Dead assignment' warnings identified by the Clang
> static analyzer.
>
> Instead of updating the extra_prefix_len with the current (next)
> value, the next value was inadvertently updated with extra_prefix_len.
> This patch rectifies this behavior.
>
> Fixes: 95a5454c5110 ("ofp-print: Abbreviate lists of fields in table features
> output.")
> Signed-off-by: Eelco Chaudron
Thanks Eelco,
I agree with the reasoning in the commit message.
Reviewed-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/5] general: Fix Clang's static analyzer 'Dead assignment' warnings.
On Mon, Oct 23, 2023 at 04:22:24PM +0200, Eelco Chaudron wrote: > This patch addresses a 'Dead assignment' warning by designating > the variable as OVS_UNUSED. We opted for this approach instead > of comparing it to the sizeof(struct ...) method because of > concerns related to code clarity. > > Signed-off-by: Eelco Chaudron Thanks, I confirmed that this addresses warnings generated by clang-tidy. And I agree this is a reasonable approach. Acked-by: Simon Horman ___ 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 25 Oct 2023, at 11:41, Jakob Meng wrote:
> 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...
Thanks, will at it to my review list.
//Eelco
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.
On Sat, Oct 21, 2023 at 05:04:48PM +0200, Frode Nordahl wrote: > Many system tests currently use ping with the combination of a > low packet count (-c 3), short interval between sends (-i 0.3) > and a _deadline_ of 2 seconds (-d 2). > > This combination of options may lead to a situation where more > than count packets are sent however ping will stop when count > packets are received. This results in a failed test due to how > the result is checked, for example: > > ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING > @@ -1,2 +1,2 @@ > -3 packets transmitted, 3 received, 0% packet loss, time 0ms > +4 packets transmitted, 3 received, 25% packet loss, time 0ms > > To reiterate, in the above example there is no packet loss, but > ping stops after _receiving_ 3 packets, not bothering with > waiting for the response to the fourth packet it just sent out. > > If we look at the iputils ping manual for the -w deadline option > we can read that this is expected behavior: > > > Specify a timeout, in seconds, before ping exits regardless of > > how many packets have been sent or received. In this case ping > > does not stop after count packet are sent, it waits either for > > deadline expire or until count probes are answered or for some > > error notification from network. > > To avoid these kinds of failures in checks where a response is > expected, we replace ping -w with ping -W. > > We keep ping -w for checks where it is expected to NOT get a > response. > > Signed-off-by: Frode Nordahl Thanks Frode, TIL about -w and -W. Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests/system-traffic: Ensure no name resolution for tcpdump.
On Sat, Oct 21, 2023 at 01:22:08AM +0200, Frode Nordahl wrote: > Depending on system configuration, executing tcpdump without the > -n parameter, may prolong the execution time for tcpdump while it > attempts name resolution. > > This delay may in turn lead to test failures due to contents of > tables to check being evicted. > > We recently started to see this problem with the > "conntrack -IPv6 ICMP6 Related with SNAT" test. > > For consistency, this patch adds the -n parameter to all tcpdump > calls in system-traffic.at. > > Signed-off-by: Frode Nordahl Thanks Frode, I think that in general tcpdump -n is good practice unless lookup is really needed. Acked-by: Simon Horman ___ 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
[ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.
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.
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.
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 fail.
By default, the output format is plain-text as before.
In popular tools like kubectl the option for output control is usually
called '-o|--output' instead of '-f,--format'. But ovs-appctl already
has an short option '-o' which prints the available ovs-appctl options
('--option'). The now choosen name also better alines with ovsdb-client
where '-f,--format' controls output formatting.
Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng
---
lib/command-line.c | 36 ++
lib/command-line.h | 10 ++
lib/dpctl.h| 4 +
lib/unixctl.c | 260 -
lib/unixctl.h | 17 ++-
tests/pmd.at | 5 +
utilities/ovs-appctl.c | 65 ---
utilities/ovs-dpctl.c | 12 ++
8 files changed, 337 insertions(+), 72 deletions(-)
diff --git a/lib/command-line.c b/lib/command-line.c
index 967f4f5d5..775e0430a 100644
--- a/lib/command-line.c
+++ b/lib/command-line.c
@@ -24,6 +24,7 @@
#include "ovs-thread.h"
#include "util.h"
#include "openvswitch/vlog.h"
+#include "openvswitch/json.h"
VLOG_DEFINE_THIS_MODULE(command_line);
@@ -53,6 +54,41 @@ ovs_cmdl_long_options_to_short_options(const struct option
options[])
return xstrdup(short_options);
}
+const char *
+ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
+{
+switch (fmt) {
+case OVS_OUTPUT_FMT_TEXT:
+return "text";
+
+case OVS_OUTPUT_FMT_JSON:
+return "json";
+
+default:
+return NULL;
+}
+}
+
+struct json *
+ovs_output_fmt_to_json(enum ovs_output_fmt fmt)
+{
+const char *string = ovs_output_fmt_to_string(fmt);
+return string ? json_string_create(string) : NULL;
+}
+
+bool
+ovs_output_fmt_from_string(const char *string, enum ovs_output_fmt *fmt)
+{
+if (!strcmp(string, "text")) {
+*fmt = OVS_OUTPUT_FMT_TEXT;
+} else if (!strcmp(string, "json")) {
+*fmt = OVS_OUTPUT_FMT_JSON;
+} else {
+return false;
+}
+return true;
+}
+
static char * OVS_WARN_UNUSED_RESULT
build_short_options(const struct option *long_options)
{
diff --git a/lib/command-line.h b/lib/command-line.h
index fc2a2690f..494274cec 100644
--- a/lib/command-line.h
+++ b/lib/command-line.h
@@ -20,6 +20,7 @@
/* Utilities for command-line parsing. */
#include "compiler.h"
+#include
struct option;
@@ -46,6 +47,15 @@ st
[ovs-dev] [RFC v3 2/4] python: Add global option for JSON output to Python tools.
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
---
python/ovs/unixctl/__init__.py | 38 +++---
python/ovs/unixctl/client.py | 20 --
python/ovs/unixctl/server.py | 71 ++
python/ovs/util.py | 9 +
tests/appctl.py| 7 +++-
5 files changed, 112 insertions(+), 33 deletions(-)
diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
index 8ee312943..aa8969c61 100644
--- a/python/ovs/unixctl/__init__.py
+++ b/python/ovs/unixctl/__init__.py
@@ -20,10 +20,14 @@ commands = {}
class _UnixctlCommand(object):
-def __init__(self, usage, min_args, max_args, callback, aux):
+# TODO: Output format will be passed as 'fmt' to the command in later
+# patch.
+def __init__(self, usage, min_args, max_args, # fmt,
+ callback, aux):
self.usage = usage
self.min_args = min_args
self.max_args = max_args
+#self.fmt = fmt
self.callback = callback
self.aux = aux
@@ -42,10 +46,12 @@ def _unixctl_help(conn, unused_argv, unused_aux):
conn.reply(reply)
-def command_register(name, usage, min_args, max_args, callback, aux):
+def command_register_fmt(name, usage, min_args, max_args, fmt, callback, aux):
""" Registers a command with the given 'name' to be exposed by the
UnixctlServer. 'usage' describes the arguments to the command; it is used
-only for presentation to the user in "help" output.
+only for presentation to the user in "help" output. 'output_fmts' is a
+bitmap that defines what output formats a command supports, e.g.
+OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
'callback' is called when the command is received. It is passed a
UnixctlConnection object, the list of arguments as unicode strings, and
@@ -60,11 +66,33 @@ def command_register(name, usage, min_args, max_args,
callback, aux):
assert isinstance(usage, str)
assert isinstance(min_args, int)
assert isinstance(max_args, int)
+assert isinstance(fmt, ovs.util.OutputFormat)
assert callable(callback)
if name not in commands:
-commands[name] = _UnixctlCommand(usage, min_args, max_args, callback,
- aux)
+commands[name] = _UnixctlCommand(usage, min_args, max_args, # fmt,
+ callback, aux)
+
+
+# TODO: command_register() will be replaced with command_register_fmt() in a
+# later patch of this series. It is is kept temporarily to reduce the
+# amount of changes in this patch.
+def command_register(name, usage, min_args, max_args, callback, aux):
+""" Registers a command with the given 'name' to be exposed by the
+UnixctlServer. 'usage' describes the arguments to the command; it is used
+only for presentation to the user in "help" output.
+
+'callback' is called when the command is received. It is passed a
+UnixctlConnection object, the list of arguments as unicode strings, and
+'aux'. Normally 'callback' should reply by calling
+UnixctlConnection.reply() or UnixctlConnection.reply_error() before it
+returns, but if the command cannot be handled immediately, then it can
+defer the reply until later. A given connection can only process a single
+request at a time, so a reply must be made eventually to avoid blocking
+that connection."""
+
+command_register_fmt(name, usage, min_args, max_args,
+ ovs.util.OutputFormat.TEXT, callback, aux)
def socket_name_from_target(target):
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..29a5f3df9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -26,13 +26,20 @@ class UnixctlClient(object):
assert isinstance(conn, ovs.jsonrpc.Connection)
self._conn = conn
-def transact(self, command, argv):
+def transact(self, command, argv, fmt):
assert isinstance(command, str)
assert isinstance(argv, list)
for arg in argv:
assert isinstance(arg, str)
+assert isinstance(fmt, ovs.util.OutputFormat)
+plain_rpc = (fmt == ovs.util.OutputFormat.TEXT)
+
+if plai
[ovs-dev] [RFC v3 4/4] ofproto: Add JSON output for 'dpif/show' command.
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
---
ofproto/ofproto-dpif.c | 129 -
tests/pmd.at | 29 +
2 files changed, 145 insertions(+), 13 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 516ec6280..bc5434b76 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -20,6 +20,7 @@
#include "bond.h"
#include "bundle.h"
#include "byte-order.h"
+#include "command-line.h"
#include "connectivity.h"
#include "connmgr.h"
#include "coverage.h"
@@ -6360,8 +6361,103 @@ done:
return changed;
}
+static struct json *
+dpif_show_backer_json(const struct dpif_backer *backer)
+{
+struct json *json_backer = json_object_create();
+
+/* add dpif name to json */
+json_object_put_string(json_backer, "name", dpif_name(backer->dpif));
+
+/* add dpif stats to json */
+struct dpif_dp_stats dp_stats;
+dpif_get_dp_stats(backer->dpif, &dp_stats);
+struct json *json_dp_stats = json_object_create();
+json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
+json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
+ dp_stats.n_missed);
+json_object_put(json_backer, "stats", json_dp_stats);
+
+/* add ofprotos to json */
+struct json *json_ofprotos = json_array_create_empty();
+struct shash ofproto_shash;
+shash_init(&ofproto_shash);
+const struct shash_node **ofprotos = get_ofprotos(&ofproto_shash);
+for (size_t i = 0; i < shash_count(&ofproto_shash); i++) {
+struct ofproto_dpif *ofproto = ofprotos[i]->data;
+
+if (ofproto->backer != backer) {
+continue;
+}
+
+struct json *json_ofproto = json_object_create();
+
+/* add ofproto name to json array */
+json_object_put_string(json_ofproto, "name", ofproto->up.name);
+
+/* add ofproto ports to json array */
+struct json *json_ofproto_ports = json_array_create_empty();
+const struct shash_node **ports;
+ports = shash_sort(&ofproto->up.port_by_name);
+for (size_t j = 0; j < shash_count(&ofproto->up.port_by_name); j++) {
+const struct shash_node *port = ports[j];
+struct ofport *ofport = port->data;
+
+struct json * json_ofproto_port = json_object_create();
+/* add ofproto port netdev name to json sub array */
+json_object_put_string(json_ofproto_port, "netdev_name",
+ netdev_get_name(ofport->netdev));
+/* add ofproto port ofp port to json sub array*/
+json_object_put_format(json_ofproto_port, "ofp_port", "%u",
+ ofport->ofp_port);
+
+/* add ofproto port odp port to json sub array*/
+odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
+ ofport->ofp_port);
+if (odp_port != ODPP_NONE) {
+json_object_put_format(json_ofproto_port, "odp_port",
+ "%"PRIu32, odp_port);
+} else {
+json_object_put_string(json_ofproto_port, "odp_port", "none");
+}
+
+/* add ofproto port netdev type to json sub array */
+json_object_put_string(json_ofproto_port, "netdev_type",
+ netdev_get_type(ofport->netdev));
+
+/* add ofproto port config to json sub array */
+struct json *json_port_config = json_object_create();
+struct smap config;
+smap_init(&config);
+if (!netdev_get_config(ofport->netdev, &config)) {
+struct smap_node *node;
+
+SMAP_FOR_EACH (node, &config) {
+json_object_put_string(json_port_config, node->key,
+ node->value);
+}
+}
+smap_destroy(&config);
+json_object_put(json_ofproto_port, "netdev_config",
+json_port_config);
+
+json_array_add(json_ofproto_ports, json_ofproto_port);
+} /* end of ofproto port(s) */
+
+free(ports);
+json_object_put(json_ofproto, "ports", json_ofproto_ports);
+
+json_array_add(json_ofprotos, json_ofproto);
+} /* end of ofproto(s) */
+shash_destroy(&ofproto_shash);
+free(ofprotos);
+
+json_object_put(json_backer, "ofprotos", json_ofprotos);
+return json_backer;
+}
+
static void
-dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
+dpif_show_backer_text(const struct dpif_backer *backer, struct ds *ds)
{
const struct shash_node **ofprotos;
[ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.
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 | 15 +-
lib/dpif-netdev-perf.c | 1 +
lib/dpif-netdev-perf.h | 1 +
lib/dpif-netdev.c | 84 ++-
lib/dpif-netlink.c | 6 +-
lib/lacp.c | 7 +-
lib/memory.c | 5 +-
lib/netdev-dpdk.c | 16 ++-
lib/netdev-dummy.c | 30 ++--
lib/netdev-native-tnl.c| 4 +-
lib/netdev-native-tnl.h| 4 +-
lib/netdev-vport.c | 1 +
lib/odp-execute.c | 10 +-
lib/ovs-lldp.c | 16 ++-
lib/ovs-router.c | 26 ++--
lib/rstp.c | 22 +--
lib/stopwatch.c| 10 +-
lib/stp.c | 20 +--
lib/timeval.c | 13 +-
Re: [ovs-dev] [PATCH 1/2] vswitch.xml: Add dpdkvhostuser group status.
On Fri, Oct 20, 2023 at 08:00:21PM +0100, Kevin Traynor wrote:
> Add group for dpdkvhostuser(/client) netdev.
>
> Adding as a single group as they display the same status,
> one of which is 'mode' to indicate if it's client or server.
>
> Fixes: b2e8b12f8a82 ("netdev-dpdk: add vhost-user get_status.")
> Signed-off-by: Kevin Traynor
Thanks Kevin,
I verified that this change correlates with the cited commit,
that the documentation seems logical (to me),
and that both the CI and checkpatch are happy.
Acked-by: Simon Horman
___
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 Fri, Oct 20, 2023 at 08:00:22PM +0100, Kevin Traynor wrote:
> get_status for dpdkvhostuser(/client) netdev class may display
> userspace-tso status.
>
> Fixes: a5669fd51c9b ("netdev-dpdk: Drop TSO in case of conflicting virtio
> features.")
> Signed-off-by: Kevin Traynor
Thanks Kevin,
I verified that this change correlatess with the cited fix,
that the documentation seems logical (to me),
and that both the CI and checkpatch are happy.
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/1] ofproto-dpif-trace: Improve conjunctive match tracing.
On Tue, Oct 24, 2023 at 10:40:43PM +0200, Ilya Maximets wrote: > On 10/3/23 09:41, Nobuhiro MIKI wrote: > > On 2023/10/02 20:41, Simon Horman wrote: > >> + Mike Pattrick > >> > >> On Mon, Sep 25, 2023 at 06:09:00PM +0900, Nobuhiro MIKI wrote: > >>> A conjunctive flow consists of two or more multiple flows with > >>> conjunction actions. When input to the ofproto/trace command > >>> matches a conjunctive flow, it outputs flows of all dimensions. > >>> > >>> Signed-off-by: Nobuhiro MIKI > >>> --- > >>> v3: > >>> * Remove struct flow changes. > >>> * Use struct 'cls_rule' instead of struct 'flow'. > >>> * Add priority and id conditionals for 'soft' arrays. > >>> * Use 'minimask' in struct 'cls_rule' as mask. > >> > >> Hi Miki-san, > >> > > > > Hi Simon-san, > > > > Thanks for your review. > > > >> I am not seeing minimask used in this patch. > > Sorry, it was a mistake. > > By the way, the test results seem to indicate that > > the mask is working correctly in the trace output. > > > >> > >>> * Use hmapx instead of ovs_list to store conj flows. > >>> * Passe 'conj_flows' as an argument only when tracing. > >> > >> Other than the minimask question above, it seems to me that > >> Ilya's review of v2 has been addressed. > >> > >> ... > >> > >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > >> > >> ... > >> > >>> @@ -918,6 +941,8 @@ xlate_report_table(const struct xlate_ctx *ctx, > >>> struct rule_dpif *rule, > >>> ctx->xin->trace = &oftrace_report(ctx->xin->trace, OFT_TABLE, > >>>ds_cstr(&s))->subs; > >>> ds_destroy(&s); > >>> + > >>> +xlate_report_conj_matches(ctx); > >>> } > >>> > >>> /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx' > >> > >>> @@ -4653,7 +4678,8 @@ xlate_table_action(struct xlate_ctx *ctx, > >>> ofp_port_t in_port, uint8_t table_id, > >>> ctx->xin->resubmit_stats, > >>> &ctx->table_id, in_port, > >>> may_packet_in, > >>> honor_table_miss, > >>> - ctx->xin->xcache); > >>> + ctx->xin->xcache, > >>> + &ctx->xout->conj_flows); > >> > >> As per my comment here [1] on another patch, I'm concerned about functions > >> with too many (say more than 6 arguments). For one thing. AFAIK, there is > >> the overhead of needing to pass arguments the stack rather than purely in > >> registers. For another, I think there are questions of readability and > >> maintainability. > >> > >> It is already the case before this patch, although this patch does > >> make it slightly worse. > >> > >> [1] Re: [PATCH v3] ofproto-dpif-mirror: Add support for pre-selection > >> filter > >> https://mail.openvswitch.org/pipermail/ovs-dev/2023-October/408228.html > >> > >> ... > > > > Agreed. > > > > It seems to me that there are approaches such as grouping arguments together > > as some kind of structure, or separating responsibilities by dividing one > > function into multiple functions. > > > > I will do some digging. > > But, any refactoring requires knowledge of existing data structures, so > > if you have any insights, I would appreciate your comments. > > While reduction of the number of arguments is a good thing, I don't really > see a good way to combine the arguments in this particular case at the moment. > So, maybe we can keep it as-is for now. But I agree that it's definitely a > good spot for potential future improvements. Thanks, no objections on my side from addressing this as a follow-up. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] Implement ovstest test-odp hexify-keys [--bad-csum].
On Fri, Oct 20, 2023 at 09:19:21AM -0400, Ihar Hrachyshka wrote:
> On Fri, Oct 20, 2023 at 8:40 AM Simon Horman wrote:
>
> > On Thu, Oct 19, 2023 at 09:00:39AM -0400, Ihar Hrachyshka wrote:
> > > On 10/19/23 7:14 AM, Simon Horman wrote:
> > > > On Thu, Oct 19, 2023 at 02:57:26AM +, Ihar Hrachyshka wrote:
> > > > > The command reads a flow string and an optional additional payload on
> > > > > stdin and produces a hex-string representation of the corresponding
> > > > > frame on stdout. It may receive more than a single input line.
> > > > >
> > > > > The command may be useful in tests, where we may need to produce hex
> > > > > frame payloads to compare observed frames against.
> > > > >
> > > > > As an example of the tool use, a single test case is converted to it.
> > > > > The test uses both normal and --bad-csum behavior of the tool,
> > > > > confirming it works as advertised.
> > > > >
> > > > > Signed-off-by: Ihar Hrachyshka
> > > > ...
> > > >
> > > > > @@ -3318,7 +3320,7 @@ packet_expand(struct dp_packet *p, const
> > struct flow *flow, size_t size)
> > > > >* from 'l7'. */
> > > > > void
> > > > > flow_compose(struct dp_packet *p, const struct flow *flow,
> > > > > - const void *l7, size_t l7_len)
> > > > > + const void *l7, size_t l7_len, bool bad_csum)
> > > > > {
> > > > > /* Add code to this function (or its callees) for emitting new
> > fields or
> > > > >* protocols. (This isn't essential, so it can be skipped for
> > initial
> > > > > @@ -3370,7 +3372,12 @@ flow_compose(struct dp_packet *p, const
> > struct flow *flow,
> > > > > /* Checksum has already been zeroed by put_zeros call. */
> > > > > ip->ip_csum = csum(ip, sizeof *ip);
> > > > >
> > > > > -dp_packet_ol_set_ip_csum_good(p);
> > > > > +if (bad_csum) {
> > > > > +ip->ip_csum++;
> > > > Hi Ihar,
> > > >
> > > > I am curious to know why '++'.
> > >
> > > I'm producing a checksum that is invalid. Since Internet checksum is
> > "16-bit
> > > ones' complement of the ones' complement sum", I assume that flipping any
> > > bit in it will result in an invalid result. Is it not the case? (++
> > could be
> > > anything else that guarantees a bit flip in the field. Nothing special
> > about
> > > ++ itself.)
> >
> > Yes, that is correct AFAIK.
> >
> > > And yes, I see that gcc is warning about this line in CI. So this line
> > may
> > > need adjustment for this reason.
> >
> > Thanks
> >
> > > > I am also wondering if we could re-cast this as 'skip-csum-verify' or
> > > > similar, rather than 'bad-csum'. After all, as it's not verified it
> > might
> > > > actually be correct.
> > > I am struggling to see how this could be "correct" after ++ on the final
> > > (valid) checksum. It may be that I miss something basic about how
> > checksums
> > > work though, let me know.
> >
> > I guess my question is really this: what is the goal and use-case for
> > this option?
> >
> >
> The patch includes an example test case that is converted to the tool. The
> test case 1) generates a packet with a garbled checksum; 2) pushes it
> through datapath; 3) confirms that the output has a proper checksum
> (fixed). That's why I needed a way to generate a "broken IPv4 checksum"
> packet - to confirm the behavior.
Thanks, I understand now. And I agree that 'bad-csum' is an appropriate
name for this option. Sorry for the noise.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
