Re: [ovs-dev] [PATCH] netdev-offload-tc: Disable offload of IPv6 fragments.
On Fri, Jul 29, 2022 at 10:38:36PM +0200, Ilya Maximets wrote:
> OVS kernel datapath and TC are parsing IPv6 fragments differently.
> For IPv6 later fragments, according to the original design [1], OVS
> always sets nw_proto=44 (IPPROTO_FRAMENT), regardless of the type
> of the L4 protocol.
>
> This leads to situation where flow for nw_proto=44 gets installed
> to TC, but packets can not match on it, causing all IPv6 later
> fragments to go to userspace significantly degrading performance.
>
> Disabling offload for such packets, so the flow can be installed
> to the OVS kernel datapath instead. Disabling for all IPv6 fragments
> including the first one, because it doesn't make a lot of sense to
> handle them separately. It may also cause potential problems with
> conntrack trying to re-assemble a packet from fragments handled by
> different datapaths (first in HW, later in OVS kernel).
>
> Checking both 'nw_proto' and 'nw_frag' as classifier might decide
> to match only on one of them and also nw_proto will not be 44 for
> the first fragment.
>
> The issue was hidden for some time due to incorrect behavior of the
> OVS kernel datapath that was recently fixed in kernel commit:
>
> 12378a5a75e3 ("net: openvswitch: fix parsing of nw_proto for IPv6 fragments")
>
> To allow offloading in the future either flow dissector in TC
> should be changed to parse packets in the same way as OVS does,
> or parsing in OVS kernel and userspace should be made configurable,
> so users can opt-in to the behavior change. Silent change of the
> behavior (change by default) is not an option, because existing
> OpenFlow pipelines may depend on a certain behavior.
>
> [1] https://docs.openvswitch.org/en/latest/topics/design/#fragments
>
> Fixes: 83e866067ea6 ("netdev-tc-offloads: Add support for IP fragmentation")
> Signed-off-by: Ilya Maximets
Reviewed-by: Marcelo Ricardo Leitner
> ---
> lib/netdev-offload-tc.c | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 29d0e63eb..9d37881a1 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1459,6 +1459,21 @@ parse_put_flow_set_action(struct tc_flower *flower,
> struct tc_action *action,
> return 0;
> }
>
> +static bool
> +is_ipv6_fragment_and_masked(const struct flow *key, const struct flow *mask)
> +{
> + if (key->dl_type != htons(ETH_P_IPV6)) {
> + return false;
> + }
> + if (mask->nw_proto && key->nw_proto == IPPROTO_FRAGMENT) {
> + return true;
> + }
> + if (key->nw_frag & (mask->nw_frag & FLOW_NW_FRAG_ANY)) {
> + return true;
> + }
> + return false;
> +}
> +
> static int
> test_key_and_mask(struct match *match)
> {
> @@ -1541,6 +1556,11 @@ test_key_and_mask(struct match *match)
> return EOPNOTSUPP;
> }
>
> +if (is_ipv6_fragment_and_masked(key, mask)) {
> +VLOG_DBG_RL(, "offloading of IPv6 fragments isn't supported");
> +return EOPNOTSUPP;
> +}
> +
> if (!is_all_zeros(mask, sizeof *mask)) {
> VLOG_DBG_RL(, "offloading isn't supported, unknown attribute");
> return EOPNOTSUPP;
> @@ -2099,7 +2119,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> memset(>arp_tha, 0, sizeof mask->arp_tha);
> }
>
> -if (is_ip_any(key)) {
> +if (is_ip_any(key) && !is_ipv6_fragment_and_masked(key, mask)) {
> flower.key.ip_proto = key->nw_proto;
> flower.mask.ip_proto = mask->nw_proto;
> mask->nw_proto = 0;
> --
> 2.34.3
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/1] packets: Re-calculate IPv6 checksum only for first frag upon modify.
On 8/4/22 19:51, Ilya Maximets wrote:
> On 7/13/22 22:47, Mike Pattrick wrote:
>> On Tue, Jun 7, 2022 at 11:00 AM Salem Sol wrote:
>>>
>>> In case of modifying an IPv6 packet src/dst address the L4 checksum should
>>> be
>>> recalculated only for the first frag. Currently it's done for all frags,
>>> leading to incorrect reassembled packet checksum.
>>> Fix it by adding a new flag to recalculate the checksum only for last frag.
>>
>> Should be "only for the first frag."
>>
>> Acked-by: Mike Pattrick .
>
> Thanks! Applied and backported down to 2.13.
>
> Best regards, Ilya Maximets.
Hrm. There is a problem with the test. It works with upstream 5.19
kernel, but it fails on some older distribution kernels. Both kernel
and userspace tests fail, because p0 namespace replies only to a first
fragmented ICMP request.
Does it require any specific fix in the kernel to work correctly?
The only relevant recent kernel change I found is:
d9b5ae5c1b24 ("openvswitch: Fix setting ipv6 fields causing hw csum failure")
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn branch-21.12 0/4] Avoid unnecessary deletion & recreation during restart.
On Thu, Aug 4, 2022 at 10:12 AM Mark Michelson wrote: > > This is a backport of Han Zhou's patch series "Avoid unnecessary > deletion & recreation during restart" for branch-21.12. The original > series can be found at > https://patchwork.ozlabs.org/project/ovn/list/?series=311230 . > > Apologies if I did not handle the sign-offs on the commits correctly. > I'm not sure what the protocol is when backporting a patch series > authored by someone else. Since it's a backport I suppose you can also add the Acked-by tags from the main commit while applying. Also please add the original commits in the commit message. Normally I'd do - git cherry-pick -x and it would add the below line in the commit message (cherry picked from commit x) Please note that you also need to backport this commit https://github.com/ovn-org/ovn/commit/db15cf29a1f9857b55389f424c5d747406550cb7 to fix a regression caused by this patch series. With the commit db15cf29 also backported - For the entire series Acked-by: Numan Siddique Thanks Numan > > Han Zhou (4): > ofctrl: Wakeup when entering S_UPDATE_FLOWS. > ofctrl: Support ovn-ofctrl-wait-before-clear to reduce down time > during upgrade. > ofctrl.c: Include group changes to bundle. > ofctrl.c: Use bundle to avoid data plane downtime during the first > flow installation. > > controller/ofctrl.c | 275 +--- > controller/ofctrl.h | 4 +- > controller/ovn-controller.8.xml | 34 > controller/ovn-controller.c | 6 +- > tests/ovn-controller.at | 55 ++- > 5 files changed, 277 insertions(+), 97 deletions(-) > > -- > 2.37.1 > > ___ > 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] python: Fix E275 missing whitespace after keyword.
On 8/4/22 17:56, Dumitru Ceara wrote: > On 8/4/22 17:51, Ilya Maximets wrote: >> On 8/4/22 17:47, Dumitru Ceara wrote: >>> On 8/4/22 16:33, Mike Pattrick wrote: On Thu, Aug 4, 2022 at 9:56 AM Ilya Maximets wrote: > > With just released flake8 5.0 we're getting a bunch of E275 errors: > > utilities/bugtool/ovs-bugtool.in:959:23: E275 missing whitespace after > keyword > tests/test-ovsdb.py:623:11: E275 missing whitespace after keyword > python/setup.py:105:8: E275 missing whitespace after keyword > python/setup.py:106:8: E275 missing whitespace after keyword > make[2]: *** [flake8-check] Error 1 > > This breaks CI on branches below 2.16. We don't see a problem right > now on newer branches because we're installing extra dependencies > that backtrack flake8 down to 4.1 or even 3.9. > > Signed-off-by: Ilya Maximets Looks good to me! Acked-by: Mike Pattrick >>> >>> I'm still getting: >>> >>> python/ovs/db/idl.py:145:15: E275 missing whitespace after keyword >>> python/ovs/db/idl.py:167:15: E275 missing whitespace after keyword >>> >>> Because of "assert" being a keyword too. >> >> OK. I only fixed ones that failed CI on branch-2.15 and below >> and this code is fairly new. >> >> I can fold the following in while applying the change, If that's OK: >> >> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py >> index b87099ff5..8f13d1f55 100644 >> --- a/python/ovs/db/idl.py >> +++ b/python/ovs/db/idl.py >> @@ -142,7 +142,7 @@ class ConditionState(object): >> >> class IdlTable(object): >> def __init__(self, idl, table): >> -assert(isinstance(table, ovs.db.schema.TableSchema)) >> +assert isinstance(table, ovs.db.schema.TableSchema) >> self._table = table >> self.need_table = False >> self.rows = custom_index.IndexedRows(self) >> @@ -164,7 +164,7 @@ class IdlTable(object): >> >> @condition.setter >> def condition(self, condition): >> -assert(isinstance(condition, list)) >> +assert isinstance(condition, list) >> self.idl.cond_change(self.name, condition) >> >> @classmethod >> --- >> >> What do you think? > > Looks good to me, thanks! > > Acked-by: Dumitru Ceara > Thanks! Applied to all supported branches down to 2.13. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/3] netdev-offload-tc: Improved debug logs for bad/unused bit masks.
On 7/29/22 16:30, Ilya Maximets wrote: > Emerged from Roi's suggestion while discussing tunnel offload fixes. > It will probably make sense to backport these down to 2.17 as it is > debug-only and 2.17 will be our new LTS. > > Ilya Maximets (3): > dynamic-string: Add function for a sparse hex dump. > netdev-offload-tc: Print unused mask bits on failure. > tc: Use sparse hex dump while printing inconsistencies. > > include/openvswitch/dynamic-string.h | 2 ++ > lib/dynamic-string.c | 36 +--- > lib/netdev-offload-tc.c | 12 +- > lib/tc.c | 12 +- > 4 files changed, 47 insertions(+), 15 deletions(-) > Thanks, Roi, for review! Applied and backported down to 2.17 since it's a debug-only change, so it will be easier to debug issues on a future LTS branch. best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-offloads-traffic: Fix waiting for netcat indefinitely.
On 8/3/22 16:58, Mike Pattrick wrote:
> On Thu, Jul 28, 2022 at 2:27 PM Ilya Maximets wrote:
>>
>> $NC_EOF_OPT should be used to avoid some netcat implementations
>> to wait indefinitely.
>>
>> This fixes the check-offloads testsuite hanging in Ubuntu 22.04.
>>
>> Fixes: 5660b89a309d ("dpif-netlink: Offloading meter to tc police action")
>> Signed-off-by: Ilya Maximets
>
> Acked-by: Mike Pattrick
Thanks! Applied this one to master and branch-3.0.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netlink: Fix incorrect bit shift in compat mode.
On 8/3/22 17:28, Mike Pattrick wrote:
> On Thu, Jul 28, 2022 at 11:43 AM Ilya Maximets wrote:
>>
>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior in
>> lib/dpif-netlink.c:1077:40: runtime error:
>>left shift of 1 by 31 places cannot be represented in type 'int'
>>
>> #0 0x73fc31 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1077:40
>> #1 0x73fc31 in dpif_netlink_port_add lib/dpif-netlink.c:1132:17
>> #2 0x2c1745 in dpif_port_add lib/dpif.c:597:13
>> #3 0x07b279 in port_add ofproto/ofproto-dpif.c:3957:17
>> #4 0x01b209 in ofproto_port_add ofproto/ofproto.c:2124:13
>> #5 0xfdbfce in iface_do_create vswitchd/bridge.c:2066:13
>> #6 0xfdbfce in iface_create vswitchd/bridge.c:2109:13
>> #7 0xfdbfce in bridge_add_ports__ vswitchd/bridge.c:1173:21
>> #8 0xfb5319 in bridge_add_ports vswitchd/bridge.c:1189:5
>> #9 0xfb5319 in bridge_reconfigure vswitchd/bridge.c:901:9
>> #10 0xfae0f9 in bridge_run vswitchd/bridge.c:3334:9
>> #11 0xfe67dd in main vswitchd/ovs-vswitchd.c:129:9
>> #12 0x4b6d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
>> #13 0x4b6e3f in __libc_start_main
>> (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
>> #14 0x562594eed024 in _start (vswitchd/ovs-vswitchd+0x787024)
>>
>> Fixes: 526df7d8543f ("tunnel: Provide framework for tunnel extensions for
>> VXLAN-GBP and others")
>> Signed-off-by: Ilya Maximets
>
> Acked-by: Mike Pattrick
Thanks! Applied and backported down to 2.13.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] checkpatch: Add check for a Fixes tag.
On 8/2/22 09:20, Pai G, Sunil wrote: > > >> -Original Message- >> From: dev On Behalf Of Ilya Maximets >> Sent: Tuesday, July 26, 2022 7:00 PM >> To: ovs-dev@openvswitch.org >> Cc: Ilya Maximets >> Subject: [ovs-dev] [PATCH] checkpatch: Add check for a Fixes tag. >> >> A new check for common mistakes while formatting a 'Fixes:' tag. >> >> Signed-off-by: Ilya Maximets >> --- >> tests/checkpatch.at | 69 + >> utilities/checkpatch.py | 9 ++ >> 2 files changed, 78 insertions(+) >> > > Thanks Ilya! this will save some review time. > Changes LGTM, > Acked-by: Sunil Pai G Applied. Thanks! Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] python: use setuptools instead of distutils
On 7/11/22 14:24, Mike Pattrick wrote: > On Wed, Jun 29, 2022 at 11:31 AM Timothy Redaelli > wrote: >> >> On Python 3.12, distutils will be removed and it's currently (3.10+) >> deprecated (see PEP 632). >> >> Since the suggested and simplest replacement is setuptools, this commit >> replaces distutils to use setuptools instead. >> >> setuptools < 59.0 doesn't have setuptools.errors and so, in this case, >> distutils.errors is still used. >> >> Signed-off-by: Timothy Redaelli > > LGTM! > > Acked-by: Mike Pattrick Thanks! Applied and backported down to 2.17 to avoid issues on a future LTS branch. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/1] packets: Re-calculate IPv6 checksum only for first frag upon modify.
On 7/13/22 22:47, Mike Pattrick wrote: > On Tue, Jun 7, 2022 at 11:00 AM Salem Sol wrote: >> >> In case of modifying an IPv6 packet src/dst address the L4 checksum should be >> recalculated only for the first frag. Currently it's done for all frags, >> leading to incorrect reassembled packet checksum. >> Fix it by adding a new flag to recalculate the checksum only for last frag. > > Should be "only for the first frag." > > Acked-by: Mike Pattrick . Thanks! Applied and backported down to 2.13. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH branch-2.17 RESEND] netdev-linux: set correct action for packets that passed policer
Referenced commit changed policer action type from TC_ACT_UNSPEC (continue)
to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5
driver at the time validated action type and always assumed 'continue', the
breakage wasn't caught until later validation code was added. The change
also broke valid configuration when sending from offload-capable device to
non-offload capable. For example, when sending from mlx5 VF to OvS bridge
netdevice the traffic that passed matchall classifier with policer could no
longer match the following flower rule in software:
filter protocol all pref 1 matchall chain 0
filter protocol all pref 1 matchall chain 0 handle 0x1
in_hw (rule hit 7863)
action order 1: police 0x1 rate 32Mbit burst 1000Kb mtu 64Kb action
drop/pipe overhead 0b
ref 1 bind 1 installed 17 sec firstused 17 sec
Action statistics:
Sent 152199634 bytes 102550 pkt (dropped 1315, overlimits 1315 requeues
0)
Sent software 74612172 bytes 51275 pkt
Sent hardware 77587462 bytes 51275 pkt
backlog 0b 0p requeues 0
used_hw_stats delayed
filter protocol ip pref 3 flower chain 0
filter protocol ip pref 3 flower chain 0 handle 0x1
dst_mac aa:94:1f:f2:f8:44
src_mac e4:00:01:08:00:02
eth_type ipv4
ip_flags nofrag
not_in_hw
action order 1: skbedit ptype host pipe
index 1 ref 1 bind 1 installed 6 sec used 6 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 2: mirred (Ingress Redirect to device br-ovs) stolen
index 1 ref 1 bind 1 installed 6 sec used 6 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie 401a9c8b3d403c62240d3eb5e21c1604
no_percpu
Fix the issue by restoring policer action type to 'continue'.
Fixes: c2567e533f8a ("add port-based ingress policing based packet-per-second
rate-limiting")
Signed-off-by: Vlad Buslov
---
lib/netdev-linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index c9cf8c7892f1..067e0175612b 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2626,7 +2626,7 @@ static void
nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset,
size_t act_offset)
{
-nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_PIPE);
+nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_UNSPEC);
nl_msg_end_nested(request, offset);
nl_msg_end_nested(request, act_offset);
}
--
2.36.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2.17] netdev-linux: set correct action for packets that passed policer
On Thu 04 Aug 2022 at 19:12, Ilya Maximets wrote: > On 8/4/22 19:09, 0-day Robot wrote: >> Bleep bloop. Greetings Vlad Buslov, I am a robot and I have tried out your >> patch. >> Thanks for your contribution. >> >> I encountered some error that I wasn't expecting. See the details below. >> >> >> git-am: >> error: Failed to merge in the changes. > > FYI, you need a full branch name, i.e. 'branch-2.17' instead of just '2.17' > for the robot to apply to correct branch. Got it. Resending with the correct branch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2.17] netdev-linux: set correct action for packets that passed policer
On 8/4/22 19:09, 0-day Robot wrote: > Bleep bloop. Greetings Vlad Buslov, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > error: Failed to merge in the changes. FYI, you need a full branch name, i.e. 'branch-2.17' instead of just '2.17' for the robot to apply to correct branch. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2.17] netdev-linux: set correct action for packets that passed policer
Bleep bloop. Greetings Vlad Buslov, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 netdev-linux: set correct action for packets that passed policer When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On Thu 04 Aug 2022 at 10:24, Ilya Maximets wrote: > On 8/4/22 09:52, Simon Horman wrote: >> On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote: >>> On 8/4/22 09:18, Simon Horman wrote: On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: > Referenced commit changed policer action type from TC_ACT_UNSPEC > (continue) > to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 > driver at the time validated action type and always assumed 'continue', > the > breakage wasn't caught until later validation code was added. The change > also broke valid configuration when sending from offload-capable device to > non-offload capable. For example, when sending from mlx5 VF to OvS bridge > netdevice the traffic that passed matchall classifier with policer could > no > longer match the following flower rule in software: ... > Notes: > Changes V2 -> V3: > > - Refactored the fix to set action TC_ACT_UNSPEC only for > matchall/basic > classifiers. > > Changes V1 -> V2: > > - Rebase on latest master. ... Hi Vlad, thanks for addressing our concerns. We have reviewed and tested this patch and do not observe any problems for our use-cases: in particular mirred after meter (police). Acked-by: Simon Horman I'd also be happy to apply this to the upstream tree if there are no objections from others. >>> >>> No objections from my side. >>> >>> Regarding backports, AFAICT, we'll need that change down to 2.16, right? >>> Can code from v1 be used on 2.17 and 2.16 or do we need a separate backport >>> patch? >> >> I think that the complexity that v3 addresses arises from metering support >> which allows other actions to follow a police action (meter). >> >> So, assuming metering is not present in 2.17 and 2.16, and given that it >> turns out that UNSPEC is fine for both PPS and BPS, then I suspect we >> can go for a simple backport-patch which simply changes TC_ACT_PIPE >> to TC_ACT_UNSPEC in nl_msg_act_police_end_nest(). >> >> This would need some testing, IMHO, and I may end up eating the words >> above. > > If that works, sounds good to me. > > Vlad, could you also test this approach with 2.17 ? Tested and sent the fix for 2.17 branch. > > I think, the best way forward will be to apply v3 to master and branch-3.0 > now. Once the approach above for 2.17/2.16 is tested to work or some other > solution is chosen, one of you could post the backport patch, so it can be > reviewed/applied. > > What do you think? > > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2.17] netdev-linux: set correct action for packets that passed policer
Referenced commit changed policer action type from TC_ACT_UNSPEC (continue)
to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5
driver at the time validated action type and always assumed 'continue', the
breakage wasn't caught until later validation code was added. The change
also broke valid configuration when sending from offload-capable device to
non-offload capable. For example, when sending from mlx5 VF to OvS bridge
netdevice the traffic that passed matchall classifier with policer could no
longer match the following flower rule in software:
filter protocol all pref 1 matchall chain 0
filter protocol all pref 1 matchall chain 0 handle 0x1
in_hw (rule hit 7863)
action order 1: police 0x1 rate 32Mbit burst 1000Kb mtu 64Kb action
drop/pipe overhead 0b
ref 1 bind 1 installed 17 sec firstused 17 sec
Action statistics:
Sent 152199634 bytes 102550 pkt (dropped 1315, overlimits 1315 requeues
0)
Sent software 74612172 bytes 51275 pkt
Sent hardware 77587462 bytes 51275 pkt
backlog 0b 0p requeues 0
used_hw_stats delayed
filter protocol ip pref 3 flower chain 0
filter protocol ip pref 3 flower chain 0 handle 0x1
dst_mac aa:94:1f:f2:f8:44
src_mac e4:00:01:08:00:02
eth_type ipv4
ip_flags nofrag
not_in_hw
action order 1: skbedit ptype host pipe
index 1 ref 1 bind 1 installed 6 sec used 6 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 2: mirred (Ingress Redirect to device br-ovs) stolen
index 1 ref 1 bind 1 installed 6 sec used 6 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie 401a9c8b3d403c62240d3eb5e21c1604
no_percpu
Fix the issue by restoring policer action type to 'continue'.
Fixes: c2567e533f8a ("add port-based ingress policing based packet-per-second
rate-limiting")
Signed-off-by: Vlad Buslov
---
lib/netdev-linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index c9cf8c7892f1..067e0175612b 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2626,7 +2626,7 @@ static void
nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset,
size_t act_offset)
{
-nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_PIPE);
+nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_UNSPEC);
nl_msg_end_nested(request, offset);
nl_msg_end_nested(request, act_offset);
}
--
2.36.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On Thu 04 Aug 2022 at 08:52, Simon Horman wrote: > On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote: >> On 8/4/22 09:18, Simon Horman wrote: >> > On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: >> >> Referenced commit changed policer action type from TC_ACT_UNSPEC >> >> (continue) >> >> to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 >> >> driver at the time validated action type and always assumed 'continue', >> >> the >> >> breakage wasn't caught until later validation code was added. The change >> >> also broke valid configuration when sending from offload-capable device to >> >> non-offload capable. For example, when sending from mlx5 VF to OvS bridge >> >> netdevice the traffic that passed matchall classifier with policer could >> >> no >> >> longer match the following flower rule in software: >> > >> > ... >> > >> >> Notes: >> >> Changes V2 -> V3: >> >> >> >> - Refactored the fix to set action TC_ACT_UNSPEC only for >> >> matchall/basic >> >> classifiers. >> >> >> >> Changes V1 -> V2: >> >> >> >> - Rebase on latest master. >> > >> > ... >> > >> > Hi Vlad, >> > >> > thanks for addressing our concerns. We have reviewed and tested this patch >> > and do not observe any problems for our use-cases: in particular mirred >> > after meter (police). >> > >> > Acked-by: Simon Horman >> > >> > I'd also be happy to apply this to the upstream tree >> > if there are no objections from others. >> >> No objections from my side. >> >> Regarding backports, AFAICT, we'll need that change down to 2.16, right? >> Can code from v1 be used on 2.17 and 2.16 or do we need a separate backport >> patch? > > I think that the complexity that v3 addresses arises from metering support > which allows other actions to follow a police action (meter). > > So, assuming metering is not present in 2.17 and 2.16, and given that it > turns out that UNSPEC is fine for both PPS and BPS, then I suspect we > can go for a simple backport-patch which simply changes TC_ACT_PIPE > to TC_ACT_UNSPEC in nl_msg_act_police_end_nest(). > > This would need some testing, IMHO, and I may end up eating the words > above. Looking at the code it seem indeed to be the case. Sending the fix for 2.17 branch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovsdb: Don't store rows that didn't change in transaction history.
Transaction history is used to construct database updates for clients.
But if the row didn't change it will never be used for monitor updates,
because ovsdb_monitor_changes_classify() will always return
OVSDB_CHANGES_NO_EFFECT. So, ovsdb_monitor_history_change_cb()
will never add it to the update.
This condition is very common for rows with references. While
processing strong references in ovsdb_txn_adjust_atom_refs() the
whole destination row will be cloned into transaction just to update
the reference counter. If this row will not be changed later in
the transaction, it will just stay in that state and will be added
to the transaction history. Since the data didn't change, both 'old'
and 'new' datums will be the same and equal to one in the database.
So, we're keeping 2 copies of the same row in memory and we are
never using them. In this case, we should just not add them to the
transaction history in the first place.
This change should save some space in the transaction history in case
of transactions with rows with big number of strong references.
This should also speed up the processing since we will not clone
these rows for transaction history and will not count their atoms.
Testing shows about 5-10% performance improvement in ovn-heater
test scenarios.
'n_atoms' counter for transaction adjusted to count only changed
rows, so we will have accurate value for a number of atoms in the
history.
Signed-off-by: Ilya Maximets
---
The function 'ovsdb_txn_clone' is going to be renamed as part
of the other patch, so it will make more sense:
https://patchwork.ozlabs.org/project/openvswitch/patch/20220802132333.2258491-1-i.maxim...@ovn.org/
ovsdb/transaction.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 6864fe099..450ac28cf 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -42,7 +42,7 @@ struct ovsdb_txn {
struct ovs_list txn_tables; /* Contains "struct ovsdb_txn_table"s. */
struct ds comment;
struct uuid txnid; /* For clustered and relay modes. It is the eid. */
-size_t n_atoms;/* Number of atoms in all transaction rows. */
+size_t n_atoms;/* Number of atoms in all changed transaction rows. */
ssize_t n_atoms_diff; /* Difference between number of added and
* removed atoms. */
};
@@ -987,9 +987,17 @@ static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
count_atoms(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row)
{
struct ovsdb_table *table = txn_row->table;
+size_t n_columns = shash_count(>schema->columns);
ssize_t n_atoms_old = 0, n_atoms_new = 0;
struct shash_node *node;
+if (txn_row->old && txn_row->new
+&& bitmap_is_all_zeros(txn_row->changed, n_columns)) {
+/* This row didn't change, it has nothing to contribute
+ * to atom counters. */
+return NULL;
+}
+
SHASH_FOR_EACH (node, >schema->columns) {
const struct ovsdb_column *column = node->data;
const struct ovsdb_type *type = >type;
@@ -1115,6 +1123,14 @@ ovsdb_txn_clone(const struct ovsdb_txn *txn)
struct ovsdb_txn_row *r;
HMAP_FOR_EACH (r, hmap_node, >txn_rows) {
size_t n_columns = shash_count(>table->schema->columns);
+
+if (r->old && r->new
+&& bitmap_is_all_zeros(r->changed, n_columns)) {
+/* The row is neither old or new and no columns changed.
+ * No need to store in the history. */
+continue;
+}
+
struct ovsdb_txn_row *r_cloned =
xzalloc(offsetof(struct ovsdb_txn_row, changed)
+ bitmap_n_bytes(n_columns));
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] netlink-conntrack: Do not fail to parse if optional TCP protocol attributes are not found.
Some of the CTA_PROTOINFO_TCP nested attributes are not always
included in the received message, but the parsing logic considers them
as required, failing in case they are not found.
This was observed while monitoring some connections by reading the
events sent by conntrack:
./ovstest test-netlink-conntrack monitor
[...]
2022-08-04T09:39:02Z|7|netlink_conntrack|ERR|Could not parse nested TCP
protoinfo
options. Possibly incompatible Linux kernel version.
2022-08-04T09:39:02Z|8|netlink_notifier|WARN|unexpected netlink message
contents
[...]
All the TCP DELETE/DESTROY events fail to parse with the message
above.
Fix it by turning the relevant attributes to optional.
Signed-off-by: Paolo Valerio
---
- [1] is the related piece of code that skips flags and wscale for the
destroy evts.
[1]
https://github.com/torvalds/linux/blob/master/net/netfilter/nf_conntrack_proto_tcp.c#L1202
---
lib/netlink-conntrack.c | 45 +++--
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 78f1bf60b..4fcde9ba1 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -672,13 +672,13 @@ nl_ct_parse_protoinfo_tcp(struct nlattr *nla,
static const struct nl_policy policy[] = {
[CTA_PROTOINFO_TCP_STATE] = { .type = NL_A_U8, .optional = false },
[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL] = { .type = NL_A_U8,
-.optional = false },
+.optional = true },
[CTA_PROTOINFO_TCP_WSCALE_REPLY] = { .type = NL_A_U8,
- .optional = false },
+ .optional = true },
[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL] = { .type = NL_A_U16,
- .optional = false },
+ .optional = true },
[CTA_PROTOINFO_TCP_FLAGS_REPLY] = { .type = NL_A_U16,
-.optional = false },
+.optional = true },
};
struct nlattr *attrs[ARRAY_SIZE(policy)];
bool parsed;
@@ -695,20 +695,29 @@ nl_ct_parse_protoinfo_tcp(struct nlattr *nla,
* connection, but our structures store a separate state for
* each endpoint. Here we duplicate the state. */
protoinfo->tcp.state_orig = protoinfo->tcp.state_reply = state;
-protoinfo->tcp.wscale_orig = nl_attr_get_u8(
-attrs[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL]);
-protoinfo->tcp.wscale_reply = nl_attr_get_u8(
-attrs[CTA_PROTOINFO_TCP_WSCALE_REPLY]);
-flags_orig =
-nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL],
- sizeof *flags_orig);
-protoinfo->tcp.flags_orig =
-ip_ct_tcp_flags_to_dpif(flags_orig->flags);
-flags_reply =
-nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_REPLY],
- sizeof *flags_reply);
-protoinfo->tcp.flags_reply =
-ip_ct_tcp_flags_to_dpif(flags_reply->flags);
+
+if (attrs[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL]) {
+protoinfo->tcp.wscale_orig =
+nl_attr_get_u8(attrs[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL]);
+}
+if (attrs[CTA_PROTOINFO_TCP_WSCALE_REPLY]) {
+protoinfo->tcp.wscale_reply =
+nl_attr_get_u8(attrs[CTA_PROTOINFO_TCP_WSCALE_REPLY]);
+}
+if (attrs[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL]) {
+flags_orig =
+nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL],
+ sizeof *flags_orig);
+protoinfo->tcp.flags_orig =
+ip_ct_tcp_flags_to_dpif(flags_orig->flags);
+}
+if (attrs[CTA_PROTOINFO_TCP_FLAGS_REPLY]) {
+flags_reply =
+nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_REPLY],
+ sizeof *flags_reply);
+protoinfo->tcp.flags_reply =
+ip_ct_tcp_flags_to_dpif(flags_reply->flags);
+}
} else {
VLOG_ERR_RL(, "Could not parse nested TCP protoinfo options. "
"Possibly incompatible Linux kernel version.");
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] ct-dpif: Do not show flag key if empty.
This patch avoids to show flags_orig/flags_reply key if they have no value.
E.g., the following:
NEW tcp,orig=([...]),reply=([...]),id=1800618864,
status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120,
protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7,
wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM,flags_reply=)
becomes:
NEW tcp,orig=([...]),reply=([...]),id=1800618864,
status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120,
protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7,
wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM)
Signed-off-by: Paolo Valerio
---
lib/ct-dpif.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index cfc2315e3..f1a375523 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -512,10 +512,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds,
protoinfo->tcp.wscale_orig,
protoinfo->tcp.wscale_reply);
}
-ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig,
- tcp_flags);
-ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply,
- tcp_flags);
+
+if (protoinfo->tcp.flags_orig) {
+ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig,
+ tcp_flags);
+}
+
+if (protoinfo->tcp.flags_reply) {
+ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply,
+ tcp_flags);
+}
}
static void
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] python: Fix E275 missing whitespace after keyword.
On 8/4/22 17:51, Ilya Maximets wrote: > On 8/4/22 17:47, Dumitru Ceara wrote: >> On 8/4/22 16:33, Mike Pattrick wrote: >>> On Thu, Aug 4, 2022 at 9:56 AM Ilya Maximets wrote: With just released flake8 5.0 we're getting a bunch of E275 errors: utilities/bugtool/ovs-bugtool.in:959:23: E275 missing whitespace after keyword tests/test-ovsdb.py:623:11: E275 missing whitespace after keyword python/setup.py:105:8: E275 missing whitespace after keyword python/setup.py:106:8: E275 missing whitespace after keyword make[2]: *** [flake8-check] Error 1 This breaks CI on branches below 2.16. We don't see a problem right now on newer branches because we're installing extra dependencies that backtrack flake8 down to 4.1 or even 3.9. Signed-off-by: Ilya Maximets >>> >>> Looks good to me! >>> >>> Acked-by: Mike Pattrick >>> >> >> I'm still getting: >> >> python/ovs/db/idl.py:145:15: E275 missing whitespace after keyword >> python/ovs/db/idl.py:167:15: E275 missing whitespace after keyword >> >> Because of "assert" being a keyword too. > > OK. I only fixed ones that failed CI on branch-2.15 and below > and this code is fairly new. > > I can fold the following in while applying the change, If that's OK: > > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > index b87099ff5..8f13d1f55 100644 > --- a/python/ovs/db/idl.py > +++ b/python/ovs/db/idl.py > @@ -142,7 +142,7 @@ class ConditionState(object): > > class IdlTable(object): > def __init__(self, idl, table): > -assert(isinstance(table, ovs.db.schema.TableSchema)) > +assert isinstance(table, ovs.db.schema.TableSchema) > self._table = table > self.need_table = False > self.rows = custom_index.IndexedRows(self) > @@ -164,7 +164,7 @@ class IdlTable(object): > > @condition.setter > def condition(self, condition): > -assert(isinstance(condition, list)) > +assert isinstance(condition, list) > self.idl.cond_change(self.name, condition) > > @classmethod > --- > > What do you think? Looks good to me, thanks! Acked-by: Dumitru Ceara ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] python: Fix E275 missing whitespace after keyword.
On 8/4/22 17:47, Dumitru Ceara wrote: > On 8/4/22 16:33, Mike Pattrick wrote: >> On Thu, Aug 4, 2022 at 9:56 AM Ilya Maximets wrote: >>> >>> With just released flake8 5.0 we're getting a bunch of E275 errors: >>> >>> utilities/bugtool/ovs-bugtool.in:959:23: E275 missing whitespace after >>> keyword >>> tests/test-ovsdb.py:623:11: E275 missing whitespace after keyword >>> python/setup.py:105:8: E275 missing whitespace after keyword >>> python/setup.py:106:8: E275 missing whitespace after keyword >>> make[2]: *** [flake8-check] Error 1 >>> >>> This breaks CI on branches below 2.16. We don't see a problem right >>> now on newer branches because we're installing extra dependencies >>> that backtrack flake8 down to 4.1 or even 3.9. >>> >>> Signed-off-by: Ilya Maximets >> >> Looks good to me! >> >> Acked-by: Mike Pattrick >> > > I'm still getting: > > python/ovs/db/idl.py:145:15: E275 missing whitespace after keyword > python/ovs/db/idl.py:167:15: E275 missing whitespace after keyword > > Because of "assert" being a keyword too. OK. I only fixed ones that failed CI on branch-2.15 and below and this code is fairly new. I can fold the following in while applying the change, If that's OK: diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py index b87099ff5..8f13d1f55 100644 --- a/python/ovs/db/idl.py +++ b/python/ovs/db/idl.py @@ -142,7 +142,7 @@ class ConditionState(object): class IdlTable(object): def __init__(self, idl, table): -assert(isinstance(table, ovs.db.schema.TableSchema)) +assert isinstance(table, ovs.db.schema.TableSchema) self._table = table self.need_table = False self.rows = custom_index.IndexedRows(self) @@ -164,7 +164,7 @@ class IdlTable(object): @condition.setter def condition(self, condition): -assert(isinstance(condition, list)) +assert isinstance(condition, list) self.idl.cond_change(self.name, condition) @classmethod --- What do you think? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn]OVN-CI: ovn unit tests run in parallel jobs.
Bleep bloop. Greetings Mohammad Heib, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 90 characters long (recommended limit is 79)
#38 FILE: .ci/linux-build.sh:58:
if ! sudo make -j4 check-kernel TESTSUITEFLAGS="$TESTSUITEFLAGS"
RECHECK=yes; then
WARNING: Line is 101 characters long (recommended limit is 79)
#72 FILE: .github/workflows/test.yml:39:
- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
WARNING: Line is 100 characters long (recommended limit is 79)
#73 FILE: .github/workflows/test.yml:40:
- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
WARNING: Line is 100 characters long (recommended limit is 79)
#74 FILE: .github/workflows/test.yml:41:
- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
WARNING: Line is 99 characters long (recommended limit is 79)
#75 FILE: .github/workflows/test.yml:42:
- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
WARNING: Line is 127 characters long (recommended limit is 79)
#76 FILE: .github/workflows/test.yml:43:
- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" }
WARNING: Line is 126 characters long (recommended limit is 79)
#77 FILE: .github/workflows/test.yml:44:
- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=yes,ovn_monitor_all=no" }
WARNING: Line is 126 characters long (recommended limit is 79)
#78 FILE: .github/workflows/test.yml:45:
- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=no,ovn_monitor_all=yes" }
WARNING: Line is 125 characters long (recommended limit is 79)
#79 FILE: .github/workflows/test.yml:46:
- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=no,ovn_monitor_all=no" }
WARNING: Line is 119 characters long (recommended limit is 79)
#80 FILE: .github/workflows/test.yml:47:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
WARNING: Line is 118 characters long (recommended limit is 79)
#81 FILE: .github/workflows/test.yml:48:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
WARNING: Line is 118 characters long (recommended limit is 79)
#82 FILE: .github/workflows/test.yml:49:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
WARNING: Line is 117 characters long (recommended limit is 79)
#83 FILE: .github/workflows/test.yml:50:
- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
WARNING: Line is 121 characters long (recommended limit is 79)
#84 FILE: .github/workflows/test.yml:51:
- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
WARNING: Line is 120 characters long (recommended limit is 79)
#85 FILE: .github/workflows/test.yml:52:
- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
WARNING: Line is 120 characters long (recommended limit is 79)
#86 FILE: .github/workflows/test.yml:53:
- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
WARNING: Line is 119 characters long (recommended limit is 79)
#87 FILE: .github/workflows/test.yml:54:
- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
WARNING: Line is 108 characters long (recommended limit is 79)
#88 FILE: .github/workflows/test.yml:55:
- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
WARNING: Line is 107 characters long (recommended limit is 79)
#89 FILE: .github/workflows/test.yml:56:
- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
WARNING: Line is 107 characters long (recommended limit is 79)
#90 FILE: .github/workflows/test.yml:57:
- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
WARNING: Line is 106 characters long (recommended limit is 79)
#91 FILE: .github/workflows/test.yml:58:
- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
Lines checked: 97, Warnings: 21, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
Re: [ovs-dev] [PATCH] python: Fix E275 missing whitespace after keyword.
On 8/4/22 16:33, Mike Pattrick wrote:
> On Thu, Aug 4, 2022 at 9:56 AM Ilya Maximets wrote:
>>
>> With just released flake8 5.0 we're getting a bunch of E275 errors:
>>
>> utilities/bugtool/ovs-bugtool.in:959:23: E275 missing whitespace after
>> keyword
>> tests/test-ovsdb.py:623:11: E275 missing whitespace after keyword
>> python/setup.py:105:8: E275 missing whitespace after keyword
>> python/setup.py:106:8: E275 missing whitespace after keyword
>> make[2]: *** [flake8-check] Error 1
>>
>> This breaks CI on branches below 2.16. We don't see a problem right
>> now on newer branches because we're installing extra dependencies
>> that backtrack flake8 down to 4.1 or even 3.9.
>>
>> Signed-off-by: Ilya Maximets
>
> Looks good to me!
>
> Acked-by: Mike Pattrick
>
I'm still getting:
python/ovs/db/idl.py:145:15: E275 missing whitespace after keyword
python/ovs/db/idl.py:167:15: E275 missing whitespace after keyword
Because of "assert" being a keyword too.
I'm running:
$ flake8 --version
5.0.3 (mccabe: 0.7.0, pycodestyle: 2.9.0, pyflakes: 2.5.0) CPython 3.6.8 on
Linux
Regards,
Dumitru
>> ---
>> python/setup.py | 4 ++--
>> tests/test-ovsdb.py | 2 +-
>> utilities/bugtool/ovs-bugtool.in | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/python/setup.py b/python/setup.py
>> index 062901ddd..27684c404 100644
>> --- a/python/setup.py
>> +++ b/python/setup.py
>> @@ -124,6 +124,6 @@ except BuildFailed:
>> print("Retrying the build without the C extension.")
>> print("*" * 75)
>>
>> -del(setup_args['cmdclass'])
>> -del(setup_args['ext_modules'])
>> +del setup_args['cmdclass']
>> +del setup_args['ext_modules']
>> setuptools.setup(**setup_args)
>> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
>> index 853264f22..402cacbe9 100644
>> --- a/tests/test-ovsdb.py
>> +++ b/tests/test-ovsdb.py
>> @@ -620,7 +620,7 @@ def update_condition(idl, commands):
>> commands = commands[len("condition "):].split(";")
>> for command in commands:
>> command = command.split(" ")
>> -if(len(command) != 2):
>> +if len(command) != 2:
>> sys.stderr.write("Error parsing condition %s\n" % command)
>> sys.exit(1)
>>
>> diff --git a/utilities/bugtool/ovs-bugtool.in
>> b/utilities/bugtool/ovs-bugtool.in
>> index fa62cbe94..fee0de853 100755
>> --- a/utilities/bugtool/ovs-bugtool.in
>> +++ b/utilities/bugtool/ovs-bugtool.in
>> @@ -956,7 +956,7 @@ def load_plugins(just_capabilities=False, filter=None):
>> filters = []
>> else:
>> filters = filters_tmp.split(',')
>> -if not(filter is None or filter in filters):
>> +if not (filter is None or filter in filters):
>> continue
>> if el.tagName == "files":
>> newest_first = getBoolAttr(el, 'newest_first')
>> --
>> 2.34.3
>>
>> ___
>> 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
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 4/6] northd: Add MAC binding aging mechanism
On Thu, Aug 4, 2022 at 5:23 PM Numan Siddique wrote:
> On Tue, Jul 26, 2022 at 10:13 AM Ihar Hrachyshka
> wrote:
> >
> > Acked-By: Ihar Hrachyshka
> >
> > On Wed, Jul 20, 2022 at 4:50 AM Ales Musil wrote:
> > >
>
> Hi Ales,
>
> I couldn't get a chance to thoroughly review the entire series, but
> overall it looks fine to me.
>
> I've a few comments in this patch.
>
>
Hi Numan,
thank you for the review, please see my reply below.
>
>
>
> > > Add MAC binding aging mechanism, that utilizes
> > > the timestamp column of MAC_Binding table.
> > > When the MAC binding exceeds the threshold it is
> > > removed from SB DB, this is postponed only in case
> > > we receive update ARP with update to MAC address.
> > >
> > > The threshold is configurable via option
> > > "mac_binding_age_threshold" that can be specified
> > > for each logical router. The option is defaulting to
> > > 0 which means that by default the aging is disabled
> > > and the MAC binding rows will be persisted the same
> > > way as before.
> > >
> > > Reported-at: https://bugzilla.redhat.com/2084668
> > > Signed-off-by: Ales Musil
> > > ---
> > > v3: Rebase on top of current main.
> > > Update according to the final conclusion.
> > > ---
> > > northd/automake.mk | 2 +
> > > northd/inc-proc-northd.c | 13
> > > northd/mac-binding-aging.c | 151 +
> > > northd/mac-binding-aging.h | 33
> > > ovn-nb.xml | 7 ++
> > > 5 files changed, 206 insertions(+)
> > > create mode 100644 northd/mac-binding-aging.c
> > > create mode 100644 northd/mac-binding-aging.h
> > >
> > > diff --git a/northd/automake.mk b/northd/automake.mk
> > > index 4862ec7b7..81582867d 100644
> > > --- a/northd/automake.mk
> > > +++ b/northd/automake.mk
> > > @@ -1,6 +1,8 @@
> > > # ovn-northd
> > > bin_PROGRAMS += northd/ovn-northd
> > > northd_ovn_northd_SOURCES = \
> > > + northd/mac-binding-aging.c \
> > > + northd/mac-binding-aging.h \
> > > northd/northd.c \
> > > northd/northd.h \
> > > northd/ovn-northd.c \
> > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > index 43093cb5a..4a3699106 100644
> > > --- a/northd/inc-proc-northd.c
> > > +++ b/northd/inc-proc-northd.c
> > > @@ -22,9 +22,11 @@
> > > #include "ip-mcast-index.h"
> > > #include "static-mac-binding-index.h"
> > > #include "lib/inc-proc-eng.h"
> > > +#include "lib/mac-binding-index.h"
> > > #include "lib/ovn-nb-idl.h"
> > > #include "lib/ovn-sb-idl.h"
> > > #include "mcast-group-index.h"
> > > +#include "northd/mac-binding-aging.h"
> > > #include "openvswitch/poll-loop.h"
> > > #include "openvswitch/vlog.h"
> > > #include "inc-proc-northd.h"
> > > @@ -149,6 +151,8 @@ enum sb_engine_node {
> > > * avoid sparse errors. */
> > > static ENGINE_NODE(northd, "northd");
> > > static ENGINE_NODE(lflow, "lflow");
> > > +static ENGINE_NODE(mac_binding_aging, "mac_binding_aging");
> > > +static ENGINE_NODE(mac_binding_aging_waker,
> "mac_binding_aging_waker");
> > >
> > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > >struct ovsdb_idl_loop *sb)
> > > @@ -211,12 +215,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > engine_add_input(_northd, _sb_load_balancer, NULL);
> > > engine_add_input(_northd, _sb_fdb, NULL);
> > > engine_add_input(_northd, _sb_static_mac_binding, NULL);
> > > +engine_add_input(_mac_binding_aging, _sb_mac_binding, NULL);
> > > +engine_add_input(_mac_binding_aging, _northd, NULL);
> > > +engine_add_input(_mac_binding_aging,
> _mac_binding_aging_waker, NULL);
> > > engine_add_input(_lflow, _nb_bfd, NULL);
> > > engine_add_input(_lflow, _sb_bfd, NULL);
> > > engine_add_input(_lflow, _sb_logical_flow, NULL);
> > > engine_add_input(_lflow, _sb_multicast_group, NULL);
> > > engine_add_input(_lflow, _sb_igmp_group, NULL);
> > > engine_add_input(_lflow, _northd, NULL);
> > > +engine_add_input(_lflow, _mac_binding_aging, NULL);
> > >
> > > struct engine_arg engine_arg = {
> > > .nb_idl = nb->idl,
> > > @@ -235,6 +243,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > chassis_hostname_index_create(sb->idl);
> > > struct ovsdb_idl_index *sbrec_static_mac_binding_by_lport_ip
> > > = static_mac_binding_index_create(sb->idl);
> > > +struct ovsdb_idl_index *sbrec_mac_binding_by_datapath
> > > += mac_binding_by_datapath_index_create(sb->idl);
> > >
> > > engine_init(_lflow, _arg);
> > >
> > > @@ -256,6 +266,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > engine_ovsdb_node_add_index(_sb_static_mac_binding,
> > >
> "sbrec_static_mac_binding_by_lport_ip",
> > > sbrec_static_mac_binding_by_lport_ip);
> > > +engine_ovsdb_node_add_index(_sb_mac_binding,
> > > +
[ovs-dev] [PATCH ovn]OVN-CI: ovn unit tests run in parallel jobs.
Ovn unit tests supported matrix size has been increased
after adding support to monitor_all and northd_parallelization
options recently, and that increased the execution time of the ovn-ci jobs.
This patch aims to reduce the execution time of those jobs by splitting
them into smaller jobs that runs in parallel and each one will execute
a subset of unit test.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2114862
Signed-off-by: Mohammad Heib
---
.ci/linux-build.sh | 9 +++--
.github/workflows/test.yml | 26 +-
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index dc32564fa..2b0782aea 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -47,10 +47,15 @@ else
fi
if [ "$TESTSUITE" ]; then
+TESTSUITEFLAGS=""
+if [[ ! -z $TESTSUITE_KW ]]; then
+TESTSUITEFLAGS="-k $TESTSUITE_KW"
+fi
+
if [ "$TESTSUITE" = "system-test" ]; then
configure_ovn $OPTS
make -j4 || { cat config.log; exit 1; }
-if ! sudo make -j4 check-kernel RECHECK=yes; then
+if ! sudo make -j4 check-kernel TESTSUITEFLAGS="$TESTSUITEFLAGS"
RECHECK=yes; then
# system-kmod-testsuite.log is necessary for debugging.
cat tests/system-kmod-testsuite.log
exit 1
@@ -62,7 +67,7 @@ if [ "$TESTSUITE" ]; then
export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" -j4 \
-TESTSUITEFLAGS="-j4" RECHECK=yes
+TESTSUITEFLAGS="$TESTSUITEFLAGS -j4" RECHECK=yes
then
# testsuite.log is necessary for debugging.
cat */_build/sub/tests/testsuite.log
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 3b7283229..83d7c2386 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -24,6 +24,7 @@ jobs:
M32: ${{ matrix.cfg.m32 }}
OPTS:${{ matrix.cfg.opts }}
TESTSUITE: ${{ matrix.cfg.testsuite }}
+ TESTSUITE_KW: ${{ matrix.cfg.testsuite_kw }}
SANITIZERS: ${{ matrix.cfg.sanitizers }}
name: linux ${{ join(matrix.cfg.*, ' ') }}
@@ -35,11 +36,26 @@ jobs:
cfg:
- { compiler: gcc, opts: --disable-ssl }
- { compiler: clang, opts: --disable-ssl }
-- { compiler: gcc, testsuite: test }
-- { compiler: gcc, testsuite: system-test }
-- { compiler: clang, testsuite: test, sanitizers: sanitizers }
-- { compiler: gcc, testsuite: test, libs: -ljemalloc }
-- { compiler: clang, testsuite: test, libs: -ljemalloc }
+- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
+- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
+- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
+- { compiler: gcc, testsuite: test, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
+- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=yes,ovn_monitor_all=yes" }
+- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=yes,ovn_monitor_all=no" }
+- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=no,ovn_monitor_all=yes" }
+- { compiler: clang, testsuite: test, sanitizers: sanitizers,
testsuite_kw: "parallelization=no,ovn_monitor_all=no" }
+- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
+- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
+- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
+- { compiler: gcc, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
+- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
+- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
+- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
+- { compiler: clang, testsuite: test, libs: -ljemalloc, testsuite_kw:
"parallelization=no,ovn_monitor_all=no" }
+- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=yes" }
+- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=yes,ovn_monitor_all=no" }
+- { compiler: gcc, testsuite: system-test, testsuite_kw:
"parallelization=no,ovn_monitor_all=yes" }
+- { compiler: gcc, testsuite: system-test, testsuite_kw:
Re: [ovs-dev] [PATCH ovn v3 4/6] northd: Add MAC binding aging mechanism
On Tue, Jul 26, 2022 at 10:13 AM Ihar Hrachyshka wrote:
>
> Acked-By: Ihar Hrachyshka
>
> On Wed, Jul 20, 2022 at 4:50 AM Ales Musil wrote:
> >
Hi Ales,
I couldn't get a chance to thoroughly review the entire series, but
overall it looks fine to me.
I've a few comments in this patch.
> > Add MAC binding aging mechanism, that utilizes
> > the timestamp column of MAC_Binding table.
> > When the MAC binding exceeds the threshold it is
> > removed from SB DB, this is postponed only in case
> > we receive update ARP with update to MAC address.
> >
> > The threshold is configurable via option
> > "mac_binding_age_threshold" that can be specified
> > for each logical router. The option is defaulting to
> > 0 which means that by default the aging is disabled
> > and the MAC binding rows will be persisted the same
> > way as before.
> >
> > Reported-at: https://bugzilla.redhat.com/2084668
> > Signed-off-by: Ales Musil
> > ---
> > v3: Rebase on top of current main.
> > Update according to the final conclusion.
> > ---
> > northd/automake.mk | 2 +
> > northd/inc-proc-northd.c | 13
> > northd/mac-binding-aging.c | 151 +
> > northd/mac-binding-aging.h | 33
> > ovn-nb.xml | 7 ++
> > 5 files changed, 206 insertions(+)
> > create mode 100644 northd/mac-binding-aging.c
> > create mode 100644 northd/mac-binding-aging.h
> >
> > diff --git a/northd/automake.mk b/northd/automake.mk
> > index 4862ec7b7..81582867d 100644
> > --- a/northd/automake.mk
> > +++ b/northd/automake.mk
> > @@ -1,6 +1,8 @@
> > # ovn-northd
> > bin_PROGRAMS += northd/ovn-northd
> > northd_ovn_northd_SOURCES = \
> > + northd/mac-binding-aging.c \
> > + northd/mac-binding-aging.h \
> > northd/northd.c \
> > northd/northd.h \
> > northd/ovn-northd.c \
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index 43093cb5a..4a3699106 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -22,9 +22,11 @@
> > #include "ip-mcast-index.h"
> > #include "static-mac-binding-index.h"
> > #include "lib/inc-proc-eng.h"
> > +#include "lib/mac-binding-index.h"
> > #include "lib/ovn-nb-idl.h"
> > #include "lib/ovn-sb-idl.h"
> > #include "mcast-group-index.h"
> > +#include "northd/mac-binding-aging.h"
> > #include "openvswitch/poll-loop.h"
> > #include "openvswitch/vlog.h"
> > #include "inc-proc-northd.h"
> > @@ -149,6 +151,8 @@ enum sb_engine_node {
> > * avoid sparse errors. */
> > static ENGINE_NODE(northd, "northd");
> > static ENGINE_NODE(lflow, "lflow");
> > +static ENGINE_NODE(mac_binding_aging, "mac_binding_aging");
> > +static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker");
> >
> > void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >struct ovsdb_idl_loop *sb)
> > @@ -211,12 +215,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_add_input(_northd, _sb_load_balancer, NULL);
> > engine_add_input(_northd, _sb_fdb, NULL);
> > engine_add_input(_northd, _sb_static_mac_binding, NULL);
> > +engine_add_input(_mac_binding_aging, _sb_mac_binding, NULL);
> > +engine_add_input(_mac_binding_aging, _northd, NULL);
> > +engine_add_input(_mac_binding_aging, _mac_binding_aging_waker,
> > NULL);
> > engine_add_input(_lflow, _nb_bfd, NULL);
> > engine_add_input(_lflow, _sb_bfd, NULL);
> > engine_add_input(_lflow, _sb_logical_flow, NULL);
> > engine_add_input(_lflow, _sb_multicast_group, NULL);
> > engine_add_input(_lflow, _sb_igmp_group, NULL);
> > engine_add_input(_lflow, _northd, NULL);
> > +engine_add_input(_lflow, _mac_binding_aging, NULL);
> >
> > struct engine_arg engine_arg = {
> > .nb_idl = nb->idl,
> > @@ -235,6 +243,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > chassis_hostname_index_create(sb->idl);
> > struct ovsdb_idl_index *sbrec_static_mac_binding_by_lport_ip
> > = static_mac_binding_index_create(sb->idl);
> > +struct ovsdb_idl_index *sbrec_mac_binding_by_datapath
> > += mac_binding_by_datapath_index_create(sb->idl);
> >
> > engine_init(_lflow, _arg);
> >
> > @@ -256,6 +266,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_ovsdb_node_add_index(_sb_static_mac_binding,
> > "sbrec_static_mac_binding_by_lport_ip",
> > sbrec_static_mac_binding_by_lport_ip);
> > +engine_ovsdb_node_add_index(_sb_mac_binding,
> > +"sbrec_mac_binding_by_datapath",
> > +sbrec_mac_binding_by_datapath);
> > }
> >
> > void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
> > diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c
> > new file mode 100644
> > index 0..8af477ff1
> > --- /dev/null
> > +++
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 8/4/22 16:57, Paul Blakey wrote:
> On 29/07/2022 17:53, Ilya Maximets wrote:
>> @@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower *flower,
>> struct tc_action *action,
>> Â action->encap.ttl = nl_attr_get_u8(tun_attr);
>> Â }
>> Â break;
>> +Â Â Â case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
>> +   /* XXX: This is wrong! We're ignoring the DF flag configuration
>> + * requested by the user. However, TC for now has no way to
>> pass
>> + * that flag and it is set by default, meaning tunnel offloading
>> + * will not work if 'options:df_default=false' is not set.
>> + * Keeping incorrect behavior for now. */
>> +Â Â Â }
>> +Â Â Â break;
>> +Â Â Â case OVS_TUNNEL_KEY_ATTR_CSUM: {
>> +Â Â Â attr_csum_present = true;
>> +Â Â Â action->encap.no_csum = !nl_attr_get_u8(tun_attr);>
> Hi, OVS_TUNNEL_KEY_ATTR_CSUM is a flag (zero size, not u8), so maybe you meant
> setting action->encap.no_csum = 0; instead
Hmm, you're right. Got confused with TCA_TUNNEL_KEY_NO_CSUM, I guess.
Will fix in v3. Thanks!
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 8/2/22 09:13, Roi Dayan wrote:
>
>
> On 2022-08-02 9:53 AM, Roi Dayan wrote:
>>
>>
>> On 2022-08-01 9:31 AM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-29 5:53 PM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
  OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
  OVS_TUNNEL_KEY_ATTR_OAM
  OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
  OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using
tc interface")
Signed-off-by: Ilya Maximets
---
 lib/dpif-netlink.c | 14 +-
 lib/netdev-offload-tc.c | 61 -
 lib/netdev-offload.h   | 3 --
 3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
 size_t left;
 struct netdev *dev;
 struct offload_info info;
-Â Â Â ovs_be16 dst_port = 0;
-Â Â Â uint8_t csum_on = false;
 int err;
 info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
 return EOPNOTSUPP;
 }
-Â Â Â /* Get tunnel dst port */
+Â Â Â /* Check the output port for a tunnel. */
 NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
 if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
-Â Â Â const struct netdev_tunnel_config *tnl_cfg;
 struct netdev *outdev;
 odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
 err = EOPNOTSUPP;
 goto out;
 }
-Â Â Â tnl_cfg = netdev_get_tunnel_config(outdev);
-Â Â Â if (tnl_cfg && tnl_cfg->dst_port != 0) {
-Â Â Â dst_port = tnl_cfg->dst_port;
-Â Â Â }
-Â Â Â if (tnl_cfg) {
-Â Â Â csum_on = tnl_cfg->csum;
-Â Â Â }
 netdev_close(outdev);
 }
 }
-Â Â Â info.tp_dst_port = dst_port;
-Â Â Â info.tunnel_csum_on = csum_on;
 info.recirc_id_shared_with_tc = (dpif->user_features
  & OVS_DP_F_TC_RECIRC_SHARING);
 err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,11 @@ static int
 parse_put_flow_set_action(struct tc_flower *flower, struct tc_action
*action,
   const struct nlattr *set, size_t set_len)
 {
+Â Â Â static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
 const struct nlattr *tunnel;
 const struct nlattr *tun_attr;
 size_t tun_left, tunnel_len;
+Â Â Â bool attr_csum_present;
 if (nl_attr_type(set) == OVS_KEY_ATTR_MPLS) {
 return parse_mpls_set_action(flower, action, set);
@@
Re: [ovs-dev] [PATCH ovn] run CI for every commit when merging several commits, not for the latest.
Bleep bloop. Greetings Igor Zhukov, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 88 characters long (recommended limit is 79)
#25 FILE: .github/workflows/ovn-kubernetes.yml:12:
group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.run_id }}
WARNING: Line is 88 characters long (recommended limit is 79)
#38 FILE: .github/workflows/test.yml:11:
group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.run_id }}
Lines checked: 44, 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
[ovs-dev] [PATCH ovn] run CI for every commit when merging several commits, not for the latest.
From: Igor Zhukov
Docs:
https://docs.github.com/en/actions/using-jobs/using-concurrency#example-using-a-fallback-value
Test: https://github.com/fsb4000/ovn/actions and https://imgur.com/a/mUBQWSO
Signed-off-by: Igor Zhukov
---
.github/workflows/ovn-kubernetes.yml | 2 +-
.github/workflows/test.yml | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/ovn-kubernetes.yml
b/.github/workflows/ovn-kubernetes.yml
index 03f35d7a3..32eb2350b 100644
--- a/.github/workflows/ovn-kubernetes.yml
+++ b/.github/workflows/ovn-kubernetes.yml
@@ -9,7 +9,7 @@ on:
- cron: '0 0 * * 0'
concurrency:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.run_id }}
cancel-in-progress: true
env:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 3b7283229..76a7ce090 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -8,7 +8,7 @@ on:
- cron: '0 0 * * 0'
concurrency:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.run_id }}
cancel-in-progress: true
jobs:
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 29/07/2022 17:53, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 61 -
lib/netdev-offload.h| 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
-ovs_be16 dst_port = 0;
-uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
return EOPNOTSUPP;
}
-/* Get tunnel dst port */
+/* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
-const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
-tnl_cfg = netdev_get_tunnel_config(outdev);
-if (tnl_cfg && tnl_cfg->dst_port != 0) {
-dst_port = tnl_cfg->dst_port;
-}
-if (tnl_cfg) {
-csum_on = tnl_cfg->csum;
-}
netdev_close(outdev);
}
}
-info.tp_dst_port = dst_port;
-info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,11 @@ static int
parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action,
const struct nlattr *set, size_t set_len)
{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
const struct nlattr *tunnel;
const struct nlattr *tun_attr;
size_t tun_left, tunnel_len;
+bool attr_csum_present;
if (nl_attr_type(set) == OVS_KEY_ATTR_MPLS) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
tunnel = nl_attr_get(set);
tunnel_len = nl_attr_get_size(set);
+attr_csum_present = false;
action->type = TC_ACT_ENCAP;
action->encap.id_present = false;
flower->action_count++;
@@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
action->encap.ttl = nl_attr_get_u8(tun_attr);
}
break;
+case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
+/* XXX: This is wrong! We're ignoring the DF flag configuration
+ *
Re: [ovs-dev] [PATCH] python: Fix E275 missing whitespace after keyword.
On Thu, Aug 4, 2022 at 9:56 AM Ilya Maximets wrote:
>
> With just released flake8 5.0 we're getting a bunch of E275 errors:
>
> utilities/bugtool/ovs-bugtool.in:959:23: E275 missing whitespace after
> keyword
> tests/test-ovsdb.py:623:11: E275 missing whitespace after keyword
> python/setup.py:105:8: E275 missing whitespace after keyword
> python/setup.py:106:8: E275 missing whitespace after keyword
> make[2]: *** [flake8-check] Error 1
>
> This breaks CI on branches below 2.16. We don't see a problem right
> now on newer branches because we're installing extra dependencies
> that backtrack flake8 down to 4.1 or even 3.9.
>
> Signed-off-by: Ilya Maximets
Looks good to me!
Acked-by: Mike Pattrick
> ---
> python/setup.py | 4 ++--
> tests/test-ovsdb.py | 2 +-
> utilities/bugtool/ovs-bugtool.in | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/python/setup.py b/python/setup.py
> index 062901ddd..27684c404 100644
> --- a/python/setup.py
> +++ b/python/setup.py
> @@ -124,6 +124,6 @@ except BuildFailed:
> print("Retrying the build without the C extension.")
> print("*" * 75)
>
> -del(setup_args['cmdclass'])
> -del(setup_args['ext_modules'])
> +del setup_args['cmdclass']
> +del setup_args['ext_modules']
> setuptools.setup(**setup_args)
> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
> index 853264f22..402cacbe9 100644
> --- a/tests/test-ovsdb.py
> +++ b/tests/test-ovsdb.py
> @@ -620,7 +620,7 @@ def update_condition(idl, commands):
> commands = commands[len("condition "):].split(";")
> for command in commands:
> command = command.split(" ")
> -if(len(command) != 2):
> +if len(command) != 2:
> sys.stderr.write("Error parsing condition %s\n" % command)
> sys.exit(1)
>
> diff --git a/utilities/bugtool/ovs-bugtool.in
> b/utilities/bugtool/ovs-bugtool.in
> index fa62cbe94..fee0de853 100755
> --- a/utilities/bugtool/ovs-bugtool.in
> +++ b/utilities/bugtool/ovs-bugtool.in
> @@ -956,7 +956,7 @@ def load_plugins(just_capabilities=False, filter=None):
> filters = []
> else:
> filters = filters_tmp.split(',')
> -if not(filter is None or filter in filters):
> +if not (filter is None or filter in filters):
> continue
> if el.tagName == "files":
> newest_first = getBoolAttr(el, 'newest_first')
> --
> 2.34.3
>
> ___
> 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 ovn v2 2/4] ovn-controller: Add a generic way to check if the daemon started recently.
On 7/25/22 23:34, Han Zhou wrote:
> In some cases we need to know if ovn-controller started long enough and
> has enough iterations of input processing, primarily to ensure it has
> downloaded and handled a complete initial view of the SB DB (and of
> course the local OVS DB), so that it won't delete things too early by
> mistake based on incomplete data.
>
> The mechanism will be used in follow up patches.
>
> Suggested-by: Dumitru Ceara
> Signed-off-by: Han Zhou
> ---
> controller/ovn-controller.c | 22 +---
> lib/inc-proc-eng.c | 11
> lib/inc-proc-eng.h | 4 +++
> lib/ovn-util.c | 50 +
> lib/ovn-util.h | 4 +++
> 5 files changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 2e9138036..8fc554201 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -93,6 +93,7 @@ static unixctl_cb_func debug_dump_lflow_conj_ids;
> static unixctl_cb_func lflow_cache_flush_cmd;
> static unixctl_cb_func lflow_cache_show_stats_cmd;
> static unixctl_cb_func debug_delay_nb_cfg_report;
> +static unixctl_cb_func debug_ignore_startup_delay;
>
> #define DEFAULT_BRIDGE_NAME "br-int"
> #define DEFAULT_DATAPATH "system"
> @@ -863,11 +864,12 @@ static void
> store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn *ovs_txn,
> const struct sbrec_chassis_private *chassis,
> const struct ovsrec_bridge *br_int,
> - unsigned int delay_nb_cfg_report, int64_t startup_ts)
> + unsigned int delay_nb_cfg_report)
> {
> struct ofctrl_acked_seqnos *acked_nb_cfg_seqnos =
> ofctrl_acked_seqnos_get(ofctrl_seq_type_nb_cfg);
> uint64_t cur_cfg = acked_nb_cfg_seqnos->last_acked;
> +int64_t startup_ts = daemon_startup_ts();
>
> if (ovs_txn && br_int
> && startup_ts != smap_get_ullong(_int->external_ids,
> @@ -3796,6 +3798,9 @@ main(int argc, char *argv[])
> debug_dump_lflow_conj_ids,
> _output_data->conj_ids);
>
> +unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
> + debug_ignore_startup_delay, NULL);
> +
> unsigned int ovs_cond_seqno = UINT_MAX;
> unsigned int ovnsb_cond_seqno = UINT_MAX;
> unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
> @@ -3817,7 +3822,6 @@ main(int argc, char *argv[])
> /* Main loop. */
> exiting = false;
> restart = false;
> -int64_t startup_ts = time_wall_msec();
> bool sb_monitor_all = false;
> while (!exiting) {
> memory_run();
> @@ -3997,6 +4001,10 @@ main(int argc, char *argv[])
> }
> stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
> time_msec());
> +if (engine_has_updated()) {
> +daemon_started_recently_countdown();
> +}
> +
> ct_zones_data = engine_get_data(_ct_zones);
> if (ovs_idl_txn) {
> if (ct_zones_data) {
> @@ -4159,7 +4167,7 @@ main(int argc, char *argv[])
> }
>
> store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private,
> - br_int, delay_nb_cfg_report, startup_ts);
> + br_int, delay_nb_cfg_report);
>
> if (pending_pkt.conn) {
> struct ed_type_addr_sets *as_data =
> @@ -4633,3 +4641,11 @@ debug_dump_lflow_conj_ids(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
> unixctl_command_reply(conn, ds_cstr(_ids_dump));
> ds_destroy(_ids_dump);
> }
> +
> +static void
> +debug_ignore_startup_delay(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *arg
> OVS_UNUSED)
> +{
> +daemon_started_recently_ignore();
> +unixctl_command_reply(conn, NULL);
> +}
Can we avoid this new CLI by using a combination of "ovn-appctl
inc-engine/recompute" and "ovn-appctl time/warp"? As far as I can tell
this is only used in the self test.
Thanks,
Dumitru
> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> index 575b774ae..2e2b31033 100644
> --- a/lib/inc-proc-eng.c
> +++ b/lib/inc-proc-eng.c
> @@ -312,6 +312,17 @@ engine_has_run(void)
> return false;
> }
>
> +bool
> +engine_has_updated(void)
> +{
> +for (size_t i = 0; i < engine_n_nodes; i++) {
> +if (engine_nodes[i]->state == EN_UPDATED) {
> +return true;
> +}
> +}
> +return false;
> +}
> +
> bool
> engine_aborted(void)
> {
> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> index 9bfab1f7c..dc365dc18 100644
> --- a/lib/inc-proc-eng.h
> +++ b/lib/inc-proc-eng.h
> @@ -238,6 +238,10 @@ bool engine_node_changed(struct engine_node *node);
> /* Return true if the
Re: [ovs-dev] [PATCH ovn branch-21.12 4/4] ofctrl.c: Use bundle to avoid data plane downtime during the first flow installation.
Bleep bloop. Greetings Mark Michelson, 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: Comment with 'xxx' marker #136 FILE: controller/ofctrl.c:2433: * XXX: Ideally, we should include the meter deletion and Lines checked: 182, Warnings: 1, 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 ovn branch-21.12 2/4] ofctrl: Support ovn-ofctrl-wait-before-clear to reduce down time during upgrade.
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 84 characters long (recommended limit is 79) #321 FILE: controller/ovn-controller.8.xml:289: ovn-appctl -t ovn-controller stopwatch/show flow-generation Lines checked: 439, Warnings: 1, 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
[ovs-dev] [PATCH ovn branch-21.12 4/4] ofctrl.c: Use bundle to avoid data plane downtime during the first flow installation.
From: Han Zhou
Now in ofctrl we wait in S_WAIT_BEFORE_CLEAR for the initial flow
compute to complete before clearing the existing flows, to reduce the
data plane down time during ovn-controller restart. However, the flow
installation takes a long time when the flow number is huge, which still
leads to a long data plane interruption after we clearing the flows in
S_CLEAR_FLOWS and before the new flow installation completes.
This patch moves the initial flow/group/meter deletion from
run_S_CLEAR_FLOWS to ofctrl_put() the first time when we install flows
to OVS after the transition from S_CLEAR_FLOWS state, and combine the
initial flow/group deletion and the new flow/group installation to a
bundle, so that OVS atomically switch from the old flows to the new ones
without any gap.
Ideally we should include meters in the bundle as well, but OVS doesn't
support meter mod in the bundle yet.
The new order of the operations in ofctrl_put becomes:
- clear meters (if it is first run after S_CLEAR_FLOWS)
- add new meters
- bundle
- clear flows (if it is first run after S_CLEAR_FLOWS)
- clear groups (if it is first run after S_CLEAR_FLOWS)
- add new groups
- flow changes
- delete old groups
- delete old meters
Tested with ~200k ovs flows generated by ovn-controller on a system with 8
Armv8 A72 cores, with ovn-ofctrl-wait-before-clear set to 8000.
Without this patch, there were ~5 seconds ping failures during ovn-controller
restart.
With this patch, there is no ping failure observed with 100ms interval
(ping -i 0.1).
Signed-off-by: Han Zhou
---
controller/ofctrl.c | 78 ++---
1 file changed, 46 insertions(+), 32 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index ff5679d00..3fce4db27 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -366,9 +366,11 @@ static void ofctrl_meter_bands_clear(void);
* S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
static enum mf_field_id mff_ovn_geneve;
-/* Indicates if flows need to be reinstalled for scenarios when ovs
- * is restarted, even if there is no change in the desired flow table. */
-static bool need_reinstall_flows;
+/* Indicates if we just went through the S_CLEAR_FLOWS state, which means we
+ * need to perform a one time deletion for all the existing flows, groups and
+ * meters. This can happen during initialization or OpenFlow reconnection
+ * (e.g. after OVS restart). */
+static bool ofctrl_initial_clear;
static ovs_be32 queue_msg(struct ofpbuf *);
@@ -634,25 +636,10 @@ run_S_CLEAR_FLOWS(void)
{
VLOG_DBG("clearing all flows");
-need_reinstall_flows = true;
-/* Send a flow_mod to delete all flows. */
-struct ofputil_flow_mod fm = {
-.table_id = OFPTT_ALL,
-.command = OFPFC_DELETE,
-};
-minimatch_init_catchall();
-queue_msg(encode_flow_mod());
-minimatch_destroy();
-
-/* Send a group_mod to delete all groups. */
-struct ofputil_group_mod gm;
-memset(, 0, sizeof gm);
-gm.command = OFPGC11_DELETE;
-gm.group_id = OFPG_ALL;
-gm.command_bucket_id = OFPG15_BUCKET_ALL;
-ovs_list_init();
-queue_msg(encode_group_mod());
-ofputil_uninit_group_mod();
+/* Set the flag so that the ofctrl_run() can clear the existing flows,
+ * groups and meters. We clear them in ofctrl_run() right before the new
+ * ones are installed to avoid data plane downtime. */
+ofctrl_initial_clear = true;
/* Clear installed_flows, to match the state of the switch. */
ovn_installed_flow_table_clear();
@@ -662,13 +649,6 @@ run_S_CLEAR_FLOWS(void)
ovn_extend_table_clear(groups, true);
}
-/* Send a meter_mod to delete all meters. */
-struct ofputil_meter_mod mm;
-memset(, 0, sizeof mm);
-mm.command = OFPMC13_DELETE;
-mm.meter.meter_id = OFPM13_ALL;
-queue_msg(encode_meter_mod());
-
/* Clear existing meters, to match the state of the switch. */
if (meters) {
ovn_extend_table_clear(meters, true);
@@ -2405,7 +2385,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
static uint64_t old_req_cfg = 0;
bool need_put = false;
if (lflows_changed || pflows_changed || skipped_last_time ||
-need_reinstall_flows) {
+ofctrl_initial_clear) {
need_put = true;
old_req_cfg = req_cfg;
} else if (req_cfg != old_req_cfg) {
@@ -2434,8 +2414,6 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
return;
}
-need_reinstall_flows = false;
-
/* OpenFlow messages to send to the switch to bring it up-to-date. */
struct ovs_list msgs = OVS_LIST_INITIALIZER();
@@ -2450,6 +2428,19 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
}
}
+if (ofctrl_initial_clear) {
+/* Send a meter_mod to delete all meters.
+ * XXX: Ideally, we should include the meter deletion and
+ *
[ovs-dev] [PATCH ovn branch-21.12 0/4] Avoid unnecessary deletion & recreation during restart.
This is a backport of Han Zhou's patch series "Avoid unnecessary deletion & recreation during restart" for branch-21.12. The original series can be found at https://patchwork.ozlabs.org/project/ovn/list/?series=311230 . Apologies if I did not handle the sign-offs on the commits correctly. I'm not sure what the protocol is when backporting a patch series authored by someone else. Han Zhou (4): ofctrl: Wakeup when entering S_UPDATE_FLOWS. ofctrl: Support ovn-ofctrl-wait-before-clear to reduce down time during upgrade. ofctrl.c: Include group changes to bundle. ofctrl.c: Use bundle to avoid data plane downtime during the first flow installation. controller/ofctrl.c | 275 +--- controller/ofctrl.h | 4 +- controller/ovn-controller.8.xml | 34 controller/ovn-controller.c | 6 +- tests/ovn-controller.at | 55 ++- 5 files changed, 277 insertions(+), 97 deletions(-) -- 2.37.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-21.12 3/4] ofctrl.c: Include group changes to bundle.
From: Han Zhou
Move the group changes to the same bundle as the OF flow changes.
The steps in ofctrl_put were:
- add groups
- add meters
- bundle
- change flows
- delete groups
- delete meters
Now it becomes:
- add meters
- bundle
- add groups
- change flows
- delete groups
- delete meters
It is required for a future change that needs all operations in a
bundle. Ideally we should add meters to the bundle as well but it is not
supported yet by OVS.
Signed-off-by: Han Zhou
---
controller/ofctrl.c | 110
1 file changed, 61 insertions(+), 49 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index ecabbcc9b..ff5679d00 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1737,21 +1737,24 @@ encode_flow_mod(struct ofputil_flow_mod *fm)
return ofputil_encode_flow_mod(fm, OFPUTIL_P_OF15_OXM);
}
-static void
-add_flow_mod(struct ofputil_flow_mod *fm,
- struct ofputil_bundle_ctrl_msg *bc,
- struct ovs_list *msgs)
+static struct ofpbuf *
+encode_bundle_add(struct ofpbuf *msg, struct ofputil_bundle_ctrl_msg *bc)
{
-struct ofpbuf *msg = encode_flow_mod(fm);
struct ofputil_bundle_add_msg bam = {
.bundle_id = bc->bundle_id,
.flags = bc->flags,
.msg = msg->data,
};
-struct ofpbuf *bundle_msg;
-
-bundle_msg = ofputil_encode_bundle_add(OFP15_VERSION, );
+return ofputil_encode_bundle_add(OFP15_VERSION, );
+}
+static void
+add_flow_mod(struct ofputil_flow_mod *fm,
+ struct ofputil_bundle_ctrl_msg *bc,
+ struct ovs_list *msgs)
+{
+struct ofpbuf *msg = encode_flow_mod(fm);
+struct ofpbuf *bundle_msg = encode_bundle_add(msg, bc);
ofpbuf_delete(msg);
ovs_list_push_back(msgs, _msg->list_node);
}
@@ -1765,13 +1768,18 @@ encode_group_mod(const struct ofputil_group_mod *gm)
}
static void
-add_group_mod(struct ofputil_group_mod *gm, struct ovs_list *msgs)
+add_group_mod(struct ofputil_group_mod *gm,
+ struct ofputil_bundle_ctrl_msg *bc,
+ struct ovs_list *msgs)
{
struct ofpbuf *msg = encode_group_mod(gm);
-if (msg->size <= UINT16_MAX) {
-ovs_list_push_back(msgs, >list_node);
+if ((msg->size + sizeof(struct ofp14_bundle_ctrl_msg)) <= UINT16_MAX) {
+struct ofpbuf *bundle_msg = encode_bundle_add(msg, bc);
+ofpbuf_delete(msg);
+ovs_list_push_back(msgs, _msg->list_node);
return;
}
+
/* This group mod request is too large to fit in a single OF message
* since the header can only specify a 16-bit size. We need to break
* this into multiple group_mod requests.
@@ -1793,7 +1801,9 @@ add_group_mod(struct ofputil_group_mod *gm, struct
ovs_list *msgs)
* the size of the buckets, we will not put too many in our new group_mod
* message.
*/
-size_t max_buckets = ((UINT16_MAX - sizeof *ogm) / bucket_size) / 2;
+size_t max_buckets = ((UINT16_MAX - sizeof *ogm -
+ sizeof(struct ofp14_bundle_ctrl_msg)) / bucket_size)
+ / 2;
ovs_assert(max_buckets < ovs_list_size(>buckets));
@@ -1822,14 +1832,16 @@ add_group_mod(struct ofputil_group_mod *gm, struct
ovs_list *msgs)
ovs_list_splice(, >list_node, >buckets);
struct ofpbuf *orig = encode_group_mod(gm);
-ovs_list_push_back(msgs, >list_node);
+struct ofpbuf *bundle_msg = encode_bundle_add(orig, bc);
+ofpbuf_delete(orig);
+ovs_list_push_back(msgs, _msg->list_node);
/* We call this recursively just in case our new
* INSERT_BUCKET/REMOVE_BUCKET group_mod is still too
* large for an OF message. This will allow for it to
* be broken into pieces, too.
*/
-add_group_mod(, msgs);
+add_group_mod(, bc, msgs);
ofputil_uninit_group_mod();
}
@@ -2438,29 +2450,6 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
}
}
-/* Iterate through all the desired groups. If there are new ones,
- * add them to the switch. */
-struct ovn_extend_table_info *desired;
-EXTEND_TABLE_FOR_EACH_UNINSTALLED (desired, groups) {
-/* Create and install new group. */
-struct ofputil_group_mod gm;
-enum ofputil_protocol usable_protocols;
-char *group_string = xasprintf("group_id=%"PRIu32",%s",
- desired->table_id,
- desired->name);
-char *error = parse_ofp_group_mod_str(, OFPGC15_ADD, group_string,
- NULL, NULL, _protocols);
-if (!error) {
-add_group_mod(, );
-} else {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_ERR_RL(, "new group %s %s", error, group_string);
-free(error);
-}
-free(group_string);
-ofputil_uninit_group_mod();
-
[ovs-dev] [PATCH ovn branch-21.12 2/4] ofctrl: Support ovn-ofctrl-wait-before-clear to reduce down time during upgrade.
From: Han Zhou
Whenever OpenFlow connection between ovn-controller and OVS is
connected/reconnected, typically during ovn-controller/OVS
restart/upgrade, ovn-controller would:
1. clears all the existing flows after the initial hand-shaking
2. compute the new flows
3. install the new flows to OVS.
In large scale environments when there are a big number of flows
needs to be computed by ovn-controller, the step 2 and 3 above can take
very long time (from seconds to minutes, depending on the scale and
topology), which would cause a data plane down time.
The purpose of this patch is to reduce data plane down time during
ovn-controller restart/upgrade. It adds a new state S_WAIT_BEFORE_CLEAR
to the ofctrl state machine, so that ovn-controller waits for a period
of time before clearing the existing OVS flows while it is computing the
new flows. Ideally, ovn-controller should clear the flows after it
completes the new flows computing. However, it is difficult for
ovn-controller to judge if it has generated all the new flows (or the
flows that are sufficient to avoid down time), because flows computation
depends on iterations of SB monitor data processing and condition
changes. So, instead of try to determine by ovn-controller itself, this
patch provides a configuration "ovn-ofctrl-wait-before-clear" for users
to determine the feasible time to wait, according to the scale. As
mentioned in the document, the output of
ovn-appctl -t ovn-controller inc-engine/recompute
ovn-appctl -t ovn-controller stopwatch/show flow-generation
can help users determining what value to set.
Tested with ~200k ovs flows generated by ovn-controller on a system with 8
Armv8 A72 cores, with ovn-ofctrl-wait-before-clear set to 7000 (the
stopwatch/show flow-generation Maximum is 3596ms).
Without this patch, there were ~13 seconds ping failures during ovn-controller
restart.
With this patch, the ping failure reduced to ~6 seconds.
(The ~6 seconds down time was introduced by step 3 - install the new
flows to ovs, which will be addressed in future patches)
Signed-off-by: Han Zhou
---
controller/ofctrl.c | 87 -
controller/ofctrl.h | 4 +-
controller/ovn-controller.8.xml | 34 +
controller/ovn-controller.c | 6 +--
tests/ovn-controller.at | 55 -
5 files changed, 168 insertions(+), 18 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 72707baf9..ecabbcc9b 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -47,6 +47,7 @@
#include "physical.h"
#include "openvswitch/rconn.h"
#include "socket-util.h"
+#include "timeval.h"
#include "util.h"
#include "vswitch-idl.h"
@@ -272,6 +273,7 @@ static unsigned int seqno;
STATE(S_NEW)\
STATE(S_TLV_TABLE_REQUESTED)\
STATE(S_TLV_TABLE_MOD_SENT) \
+STATE(S_WAIT_BEFORE_CLEAR) \
STATE(S_CLEAR_FLOWS)\
STATE(S_UPDATE_FLOWS)
enum ofctrl_state {
@@ -314,6 +316,14 @@ static uint64_t cur_cfg;
/* Current state. */
static enum ofctrl_state state;
+/* The time (ms) to stay in the state S_WAIT_BEFORE_CLEAR. Read from
+ * external_ids: ovn-ofctrl-wait-before-clear. */
+static unsigned int wait_before_clear_time = 0;
+
+/* The time when the state S_WAIT_BEFORE_CLEAR should complete.
+ * If the timer is not started yet, it is set to 0. */
+static long long int wait_before_clear_expire = 0;
+
/* Transaction IDs for messages in flight to the switch. */
static ovs_be32 xid, xid2;
@@ -419,18 +429,19 @@ recv_S_NEW(const struct ofp_header *oh OVS_UNUSED,
* If we receive an NXT_TLV_TABLE_REPLY:
*
* - If it contains our tunnel metadata option, assign its field ID to
- * mff_ovn_geneve and transition to S_CLEAR_FLOWS.
+ * mff_ovn_geneve and transition to S_WAIT_BEFORE_CLEAR.
*
* - Otherwise, if there is an unused tunnel metadata field ID, send
* NXT_TLV_TABLE_MOD and OFPT_BARRIER_REQUEST, and transition to
* S_TLV_TABLE_MOD_SENT.
*
* - Otherwise, log an error, disable Geneve, and transition to
- * S_CLEAR_FLOWS.
+ * S_WAIT_BEFORE_CLEAR.
*
* If we receive an OFPT_ERROR:
*
- * - Log an error, disable Geneve, and transition to S_CLEAR_FLOWS. */
+ * - Log an error, disable Geneve, and transition to S_WAIT_BEFORE_CLEAR.
+ */
static void
run_S_TLV_TABLE_REQUESTED(void)
@@ -457,7 +468,7 @@ process_tlv_table_reply(const struct
ofputil_tlv_table_reply *reply)
return false;
} else {
mff_ovn_geneve = MFF_TUN_METADATA0 + map->index;
-state = S_CLEAR_FLOWS;
+state = S_WAIT_BEFORE_CLEAR;
return true;
}
}
@@ -524,7 +535,7 @@ recv_S_TLV_TABLE_REQUESTED(const struct ofp_header *oh,
enum ofptype type,
/* Error path. */
mff_ovn_geneve = 0;
-
[ovs-dev] [PATCH ovn branch-21.12 1/4] ofctrl: Wakeup when entering S_UPDATE_FLOWS.
From: Han Zhou In theory it is possible that ovn-controller tried to install flows by calling ofctrl_put() while ofctrl is at a state other than S_UPDATE_FLOWS thus not able to install. So, when entering S_UPDATE_FLOWS, we should immediately wakeup the main loop so that any pending flows can be installed without being delayed. Signed-off-by: Han Zhou --- controller/ofctrl.c | 4 1 file changed, 4 insertions(+) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index c0a04ec50..72707baf9 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -644,6 +644,10 @@ run_S_CLEAR_FLOWS(void) } state = S_UPDATE_FLOWS; + +/* Give a chance for the main loop to call ofctrl_put() in case there were + * pending flows waiting ofctrl state change to S_UPDATE_FLOWS. */ +poll_immediate_wake(); } static void -- 2.37.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 4/4] vif-plug.c: Use daemon_started_recently() to replace the prime counter.
On 7/25/22 23:34, Han Zhou wrote:
> Also remove the reset mechanism when DB is reconnected, because at DB
> reconnection the data in IDL would not reset.
>
> Signed-off-by: Han Zhou
> ---
Hi Han,
I noticed that with this change the "VIF plugging" test starts failing
when running with conditional monitoring disabled:
777: ovn-controller - VIF plugging -- ovn-northd -- parallelization=no
-- ovn_monitor_all=yes ok
775: ovn-controller - VIF plugging -- ovn-northd -- parallelization=yes
-- ovn_monitor_all=yes ok
778: ovn-controller - VIF plugging -- ovn-northd -- parallelization=no
-- ovn_monitor_all=no FAILED (ovs-macros.at:255)
776: ovn-controller - VIF plugging -- ovn-northd -- parallelization=yes
-- ovn_monitor_all=no FAILED (ovs-macros.at:255)
> controller/ovn-controller.c | 1 -
> controller/vif-plug.c | 20 ++--
> 2 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 8fc554201..8bb18fc23 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3874,7 +3874,6 @@ main(int argc, char *argv[])
> if (!new_ovnsb_cond_seqno) {
> VLOG_INFO("OVNSB IDL reconnected, force recompute.");
> engine_set_force_recompute(true);
> -vif_plug_reset_idl_prime_counter();
> }
> ovnsb_cond_seqno = new_ovnsb_cond_seqno;
> }
> diff --git a/controller/vif-plug.c b/controller/vif-plug.c
> index c6fbe7e59..38348bf54 100644
> --- a/controller/vif-plug.c
> +++ b/controller/vif-plug.c
> @@ -532,22 +532,14 @@ vif_plug_handle_iface(const struct ovsrec_interface
> *iface_rec,
> * completeness of the initial data downloading we need this counter so that
> we
> * do not erronously unplug ports because the data is just not loaded yet.
> */
> -#define VIF_PLUG_PRIME_IDL_COUNT_SEEED 10
> -static int vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
> -
> -void
> -vif_plug_reset_idl_prime_counter(void)
> -{
> -vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
> -}
> -
We should remove this from the .h file too.
> void
> vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
> struct vif_plug_ctx_out *vif_plug_ctx_out)
> {
> -if (vif_plug_prime_idl_count && --vif_plug_prime_idl_count > 0) {
> -VLOG_DBG("vif_plug_run: vif_plug_prime_idl_count=%d, will not unplug
> "
> - "ports in this iteration.", vif_plug_prime_idl_count);
> +bool delay_plug = daemon_started_recently();
> +if (delay_plug) {
> +VLOG_DBG("vif_plug_run: daemon started recently, will not unplug "
> + "ports in this iteration.");
> }
>
> if (!vif_plug_ctx_in->chassis_rec) {
> @@ -557,7 +549,7 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
> OVSREC_INTERFACE_TABLE_FOR_EACH (iface_rec,
> vif_plug_ctx_in->iface_table) {
> vif_plug_handle_iface(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out,
> - !vif_plug_prime_idl_count);
> + !delay_plug);
> }
>
> struct sbrec_port_binding *target =
> @@ -573,7 +565,7 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
> enum en_lport_type lport_type = get_lport_type(pb);
> if (lport_type == LP_VIF) {
> vif_plug_handle_lport_vif(pb, vif_plug_ctx_in, vif_plug_ctx_out,
> - !vif_plug_prime_idl_count);
> + !delay_plug);
> }
> }
> sbrec_port_binding_index_destroy_row(target);
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] python: Fix E275 missing whitespace after keyword.
With just released flake8 5.0 we're getting a bunch of E275 errors:
utilities/bugtool/ovs-bugtool.in:959:23: E275 missing whitespace after keyword
tests/test-ovsdb.py:623:11: E275 missing whitespace after keyword
python/setup.py:105:8: E275 missing whitespace after keyword
python/setup.py:106:8: E275 missing whitespace after keyword
make[2]: *** [flake8-check] Error 1
This breaks CI on branches below 2.16. We don't see a problem right
now on newer branches because we're installing extra dependencies
that backtrack flake8 down to 4.1 or even 3.9.
Signed-off-by: Ilya Maximets
---
python/setup.py | 4 ++--
tests/test-ovsdb.py | 2 +-
utilities/bugtool/ovs-bugtool.in | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/python/setup.py b/python/setup.py
index 062901ddd..27684c404 100644
--- a/python/setup.py
+++ b/python/setup.py
@@ -124,6 +124,6 @@ except BuildFailed:
print("Retrying the build without the C extension.")
print("*" * 75)
-del(setup_args['cmdclass'])
-del(setup_args['ext_modules'])
+del setup_args['cmdclass']
+del setup_args['ext_modules']
setuptools.setup(**setup_args)
diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index 853264f22..402cacbe9 100644
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -620,7 +620,7 @@ def update_condition(idl, commands):
commands = commands[len("condition "):].split(";")
for command in commands:
command = command.split(" ")
-if(len(command) != 2):
+if len(command) != 2:
sys.stderr.write("Error parsing condition %s\n" % command)
sys.exit(1)
diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index fa62cbe94..fee0de853 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -956,7 +956,7 @@ def load_plugins(just_capabilities=False, filter=None):
filters = []
else:
filters = filters_tmp.split(',')
-if not(filter is None or filter in filters):
+if not (filter is None or filter in filters):
continue
if el.tagName == "files":
newest_first = getBoolAttr(el, 'newest_first')
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Cancel previous runs in the PR when you push new commits
Yes, some tests needed. I will try to test later today.
> On 8/4/22 15:19, Igor Zhukov wrote:
>
>> Hi. Yes, I confirm.
>> I'm sorry :(
>
> No problem. :)
>
>> Probably removing
>> `|| github.ref`
>> will help.
>
> Makes sense to me to limit it to PRs. But what would be the generated
> group name for a plain push event (no PR)?
>
> Thanks,
> Dumitru
>
>>> Hi guys,
>>>
>>> It seems to me that this is breaking a bit the patchwork + ovsrobot
>>> workflow. For example, for a series of patches the robot will push them
>>> one by one, to trigger test runs with each individual patch.
>>>
>>> Taking a random patch series as example:
>>> https://patchwork.ozlabs.org/project/ovn/list/?series=311230
>>>
>>> Patch 4/4 failed some tests:
>>> https://mail.openvswitch.org/pipermail/ovs-build/2022-July/024055.html
>>> https://github.com/ovsrobot/ovn/runs/7509525379?check_suite_focus=true
>>>
>>> And this caused the runs for patches 1-3/4 to be cancelled:
>>> https://github.com/ovsrobot/ovn/actions/runs/2735541538
>>> https://github.com/ovsrobot/ovn/actions/runs/2735538404
>>> https://github.com/ovsrobot/ovn/actions/runs/2735531275
>>>
>>> I think it's quite useful to run tests on each individual patch of the
>>> series. It makes bisecting a failure easier.
>>>
>>> Also, it might be a personal preference, but it gives me more confidence
>>> if I see a patch series in patchwork having more green icons than red.
>>>
>>> Do you guys have any other thoughts on this matter?
>>>
>>> Thanks,
>>> Dumitru
>>>
>>> On 7/14/22 22:02, Numan Siddique wrote:
>>>
On Wed, Jul 13, 2022 at 6:53 AM Ales Musil wrote:
> Ok, thanks.
>
> Acked-by: Ales Musil
Thanks. I applied this patch to the main branch.
Numan
> On Wed, Jul 13, 2022 at 1:50 PM Igor Zhukov wrote:
>
>> Yes, as far as I understand.
>> I found some github repos also use it:
>> https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12
>>
>>> Hi Igor,
>>>
>>> IIUC this applies only to PR right? I mean there's no harm in having
>>> that
>>>
>>> just to be sure.
>>>
>>> Thanks,
>>>
>>> Ales
>>>
>>> On Wed, Jul 13, 2022 at 1:25 PM Igor Zhukov wrote:
>>>
From: Igor Zhukov
While implementing https://github.com/ovn-org/ovn/pull/139 I sometimes
>>
>> pushed
>>
several commits quickly and after that I noticed that previous run was
>>
>> still in
>>
progress and the most recent run was waiting in line.
I googled some solutions and
I found the answer: https://stackoverflow.com/a/72408109/4544798
Github docs:
>>
>> https://docs.github.com/en/actions/using-jobs/using-concurrency
>>
Signed-off-by: Igor Zhukov
Submitted-at: https://github.com/ovn-org/ovn/pull/145
---
.github/workflows/ovn-kubernetes.yml | 4
.github/workflows/test.yml | 4
2 files changed, 8 insertions(+)
diff --git a/.github/workflows/ovn-kubernetes.yml
>>
>> b/.github/workflows/ovn-kubernetes.yml
>>
index c05bbd3f9..431e47660 100644
--- a/.github/workflows/ovn-kubernetes.yml
+++ b/.github/workflows/ovn-kubernetes.yml
@@ -8,6 +8,10 @@ on:
# Run Sunday at midnight
- cron: '0 0 * * 0'
+concurrency:
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
>>
>> github.ref }}
>>
+ cancel-in-progress: true
+
env:
GO_VERSION: "1.17.6"
K8S_VERSION: v1.23.3
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index e0de7c60e..56e8ba870 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -7,6 +7,10 @@ on:
# Run Sunday at midnight
- cron: '0 0 * * 0'
+concurrency:
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
>>
>> github.ref }}
>>
+ cancel-in-progress: true
+
jobs:
build-linux:
env:
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>> --
>>>
>>> Ales Musil
>>>
>>> Senior Software Engineer - OVN Core
>>>
>>> Red Hat EMEA
>>>
>>> amu...@redhat.com
>>>
>>> IM: amusil
>
> --
Re: [ovs-dev] [PATCH ovn] Cancel previous runs in the PR when you push new commits
On 8/4/22 15:19, Igor Zhukov wrote:
> Hi. Yes, I confirm.
> I'm sorry :(
No problem. :)
>
> Probably removing
> `|| github.ref`
> will help.
Makes sense to me to limit it to PRs. But what would be the generated
group name for a plain push event (no PR)?
Thanks,
Dumitru
>
>> Hi guys,
>>
>> It seems to me that this is breaking a bit the patchwork + ovsrobot
>> workflow. For example, for a series of patches the robot will push them
>> one by one, to trigger test runs with each individual patch.
>>
>> Taking a random patch series as example:
>> https://patchwork.ozlabs.org/project/ovn/list/?series=311230
>>
>> Patch 4/4 failed some tests:
>> https://mail.openvswitch.org/pipermail/ovs-build/2022-July/024055.html
>> https://github.com/ovsrobot/ovn/runs/7509525379?check_suite_focus=true
>>
>> And this caused the runs for patches 1-3/4 to be cancelled:
>> https://github.com/ovsrobot/ovn/actions/runs/2735541538
>> https://github.com/ovsrobot/ovn/actions/runs/2735538404
>> https://github.com/ovsrobot/ovn/actions/runs/2735531275
>>
>> I think it's quite useful to run tests on each individual patch of the
>> series. It makes bisecting a failure easier.
>>
>> Also, it might be a personal preference, but it gives me more confidence
>> if I see a patch series in patchwork having more green icons than red.
>>
>> Do you guys have any other thoughts on this matter?
>>
>> Thanks,
>> Dumitru
>>
>> On 7/14/22 22:02, Numan Siddique wrote:
>>
>>> On Wed, Jul 13, 2022 at 6:53 AM Ales Musil wrote:
>>>
Ok, thanks.
Acked-by: Ales Musil
>>>
>>> Thanks. I applied this patch to the main branch.
>>>
>>> Numan
>>>
On Wed, Jul 13, 2022 at 1:50 PM Igor Zhukov wrote:
> Yes, as far as I understand.
> I found some github repos also use it:
> https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12
>
>> Hi Igor,
>>
>> IIUC this applies only to PR right? I mean there's no harm in having that
>>
>> just to be sure.
>>
>> Thanks,
>>
>> Ales
>>
>> On Wed, Jul 13, 2022 at 1:25 PM Igor Zhukov wrote:
>>
>>> From: Igor Zhukov
>>>
>>> While implementing https://github.com/ovn-org/ovn/pull/139 I sometimes
>
> pushed
>
>>> several commits quickly and after that I noticed that previous run was
>
> still in
>
>>> progress and the most recent run was waiting in line.
>>>
>>> I googled some solutions and
>>>
>>> I found the answer: https://stackoverflow.com/a/72408109/4544798
>>>
>>> Github docs:
>
> https://docs.github.com/en/actions/using-jobs/using-concurrency
>
>>> Signed-off-by: Igor Zhukov
>>>
>>> Submitted-at: https://github.com/ovn-org/ovn/pull/145
>>>
>>> ---
>>>
>>> .github/workflows/ovn-kubernetes.yml | 4
>>>
>>> .github/workflows/test.yml | 4
>>>
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/.github/workflows/ovn-kubernetes.yml
>
> b/.github/workflows/ovn-kubernetes.yml
>
>>> index c05bbd3f9..431e47660 100644
>>>
>>> --- a/.github/workflows/ovn-kubernetes.yml
>>>
>>> +++ b/.github/workflows/ovn-kubernetes.yml
>>>
>>> @@ -8,6 +8,10 @@ on:
>>>
>>> # Run Sunday at midnight
>>>
>>> - cron: '0 0 * * 0'
>>>
>>> +concurrency:
>>>
>>> + group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
>
> github.ref }}
>
>>> + cancel-in-progress: true
>>>
>>> +
>>>
>>> env:
>>>
>>> GO_VERSION: "1.17.6"
>>>
>>> K8S_VERSION: v1.23.3
>>>
>>> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
>>>
>>> index e0de7c60e..56e8ba870 100644
>>>
>>> --- a/.github/workflows/test.yml
>>>
>>> +++ b/.github/workflows/test.yml
>>>
>>> @@ -7,6 +7,10 @@ on:
>>>
>>> # Run Sunday at midnight
>>>
>>> - cron: '0 0 * * 0'
>>>
>>> +concurrency:
>>>
>>> + group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
>
> github.ref }}
>
>>> + cancel-in-progress: true
>>>
>>> +
>>>
>>> jobs:
>>>
>>> build-linux:
>>>
>>> env:
>>>
>>> --
>>>
>>> 2.30.2
>>>
>>> ___
>>>
>>> dev mailing list
>>>
>>> d...@openvswitch.org
>>>
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>> --
>>
>> Ales Musil
>>
>> Senior Software Engineer - OVN Core
>>
>> Red Hat EMEA
>>
>> amu...@redhat.com
>>
>> IM: amusil
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.com IM: amusil
___
dev mailing list
Re: [ovs-dev] [PATCH ovn] Cancel previous runs in the PR when you push new commits
Hi. Yes, I confirm.
I'm sorry :(
Probably removing
`|| github.ref`
will help.
> Hi guys,
>
> It seems to me that this is breaking a bit the patchwork + ovsrobot
> workflow. For example, for a series of patches the robot will push them
> one by one, to trigger test runs with each individual patch.
>
> Taking a random patch series as example:
> https://patchwork.ozlabs.org/project/ovn/list/?series=311230
>
> Patch 4/4 failed some tests:
> https://mail.openvswitch.org/pipermail/ovs-build/2022-July/024055.html
> https://github.com/ovsrobot/ovn/runs/7509525379?check_suite_focus=true
>
> And this caused the runs for patches 1-3/4 to be cancelled:
> https://github.com/ovsrobot/ovn/actions/runs/2735541538
> https://github.com/ovsrobot/ovn/actions/runs/2735538404
> https://github.com/ovsrobot/ovn/actions/runs/2735531275
>
> I think it's quite useful to run tests on each individual patch of the
> series. It makes bisecting a failure easier.
>
> Also, it might be a personal preference, but it gives me more confidence
> if I see a patch series in patchwork having more green icons than red.
>
> Do you guys have any other thoughts on this matter?
>
> Thanks,
> Dumitru
>
> On 7/14/22 22:02, Numan Siddique wrote:
>
>> On Wed, Jul 13, 2022 at 6:53 AM Ales Musil wrote:
>>
>>> Ok, thanks.
>>>
>>> Acked-by: Ales Musil
>>
>> Thanks. I applied this patch to the main branch.
>>
>> Numan
>>
>>> On Wed, Jul 13, 2022 at 1:50 PM Igor Zhukov wrote:
>>>
Yes, as far as I understand.
I found some github repos also use it:
https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12
> Hi Igor,
>
> IIUC this applies only to PR right? I mean there's no harm in having that
>
> just to be sure.
>
> Thanks,
>
> Ales
>
> On Wed, Jul 13, 2022 at 1:25 PM Igor Zhukov wrote:
>
>> From: Igor Zhukov
>>
>> While implementing https://github.com/ovn-org/ovn/pull/139 I sometimes
pushed
>> several commits quickly and after that I noticed that previous run was
still in
>> progress and the most recent run was waiting in line.
>>
>> I googled some solutions and
>>
>> I found the answer: https://stackoverflow.com/a/72408109/4544798
>>
>> Github docs:
https://docs.github.com/en/actions/using-jobs/using-concurrency
>> Signed-off-by: Igor Zhukov
>>
>> Submitted-at: https://github.com/ovn-org/ovn/pull/145
>>
>> ---
>>
>> .github/workflows/ovn-kubernetes.yml | 4
>>
>> .github/workflows/test.yml | 4
>>
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/.github/workflows/ovn-kubernetes.yml
b/.github/workflows/ovn-kubernetes.yml
>> index c05bbd3f9..431e47660 100644
>>
>> --- a/.github/workflows/ovn-kubernetes.yml
>>
>> +++ b/.github/workflows/ovn-kubernetes.yml
>>
>> @@ -8,6 +8,10 @@ on:
>>
>> # Run Sunday at midnight
>>
>> - cron: '0 0 * * 0'
>>
>> +concurrency:
>>
>> + group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
>> + cancel-in-progress: true
>>
>> +
>>
>> env:
>>
>> GO_VERSION: "1.17.6"
>>
>> K8S_VERSION: v1.23.3
>>
>> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
>>
>> index e0de7c60e..56e8ba870 100644
>>
>> --- a/.github/workflows/test.yml
>>
>> +++ b/.github/workflows/test.yml
>>
>> @@ -7,6 +7,10 @@ on:
>>
>> # Run Sunday at midnight
>>
>> - cron: '0 0 * * 0'
>>
>> +concurrency:
>>
>> + group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
>> + cancel-in-progress: true
>>
>> +
>>
>> jobs:
>>
>> build-linux:
>>
>> env:
>>
>> --
>>
>> 2.30.2
>>
>> ___
>>
>> dev mailing list
>>
>> d...@openvswitch.org
>>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA
>
> amu...@redhat.com
>
> IM: amusil
>>>
>>> --
>>>
>>> Ales Musil
>>>
>>> Senior Software Engineer - OVN Core
>>>
>>> Red Hat EMEA
>>>
>>> amu...@redhat.com IM: amusil
>>>
>>> ___
>>> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Cancel previous runs in the PR when you push new commits
Hi guys,
It seems to me that this is breaking a bit the patchwork + ovsrobot
workflow. For example, for a series of patches the robot will push them
one by one, to trigger test runs with each individual patch.
Taking a random patch series as example:
https://patchwork.ozlabs.org/project/ovn/list/?series=311230
Patch 4/4 failed some tests:
https://mail.openvswitch.org/pipermail/ovs-build/2022-July/024055.html
https://github.com/ovsrobot/ovn/runs/7509525379?check_suite_focus=true
And this caused the runs for patches 1-3/4 to be cancelled:
https://github.com/ovsrobot/ovn/actions/runs/2735541538
https://github.com/ovsrobot/ovn/actions/runs/2735538404
https://github.com/ovsrobot/ovn/actions/runs/2735531275
I think it's quite useful to run tests on each individual patch of the
series. It makes bisecting a failure easier.
Also, it might be a personal preference, but it gives me more confidence
if I see a patch series in patchwork having more green icons than red.
Do you guys have any other thoughts on this matter?
Thanks,
Dumitru
On 7/14/22 22:02, Numan Siddique wrote:
> On Wed, Jul 13, 2022 at 6:53 AM Ales Musil wrote:
>>
>> Ok, thanks.
>>
>> Acked-by: Ales Musil
>>
>
> Thanks. I applied this patch to the main branch.
>
> Numan
>
>> On Wed, Jul 13, 2022 at 1:50 PM Igor Zhukov wrote:
>>
>>> Yes, as far as I understand.
>>> I found some github repos also use it:
>>> https://github.com/TeamAmaze/AmazeFileManager/blob/release/3.7/.github/workflows/android-feature.yml#L10-L12
>>>
Hi Igor,
IIUC this applies only to PR right? I mean there's no harm in having that
just to be sure.
Thanks,
Ales
On Wed, Jul 13, 2022 at 1:25 PM Igor Zhukov wrote:
> From: Igor Zhukov
>
> While implementing https://github.com/ovn-org/ovn/pull/139 I sometimes
>>> pushed
>
> several commits quickly and after that I noticed that previous run was
>>> still in
>
> progress and the most recent run was waiting in line.
>
> I googled some solutions and
>
> I found the answer: https://stackoverflow.com/a/72408109/4544798
>
> Github docs:
>>> https://docs.github.com/en/actions/using-jobs/using-concurrency
>
> Signed-off-by: Igor Zhukov
>
> Submitted-at: https://github.com/ovn-org/ovn/pull/145
>
> ---
>
> .github/workflows/ovn-kubernetes.yml | 4
>
> .github/workflows/test.yml | 4
>
> 2 files changed, 8 insertions(+)
>
> diff --git a/.github/workflows/ovn-kubernetes.yml
>>> b/.github/workflows/ovn-kubernetes.yml
>
> index c05bbd3f9..431e47660 100644
>
> --- a/.github/workflows/ovn-kubernetes.yml
>
> +++ b/.github/workflows/ovn-kubernetes.yml
>
> @@ -8,6 +8,10 @@ on:
>
> # Run Sunday at midnight
>
> - cron: '0 0 * * 0'
>
> +concurrency:
>
> + group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
>>> github.ref }}
>
> + cancel-in-progress: true
>
> +
>
> env:
>
> GO_VERSION: "1.17.6"
>
> K8S_VERSION: v1.23.3
>
> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
>
> index e0de7c60e..56e8ba870 100644
>
> --- a/.github/workflows/test.yml
>
> +++ b/.github/workflows/test.yml
>
> @@ -7,6 +7,10 @@ on:
>
> # Run Sunday at midnight
>
> - cron: '0 0 * * 0'
>
> +concurrency:
>
> + group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
>>> github.ref }}
>
> + cancel-in-progress: true
>
> +
>
> jobs:
>
> build-linux:
>
> env:
>
> --
>
> 2.30.2
>
> ___
>
> dev mailing list
>
> d...@openvswitch.org
>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.com
IM: amusil
>>>
>>>
>>
>> --
>>
>> Ales Musil
>>
>> Senior Software Engineer - OVN Core
>>
>> Red Hat EMEA
>>
>> amu...@redhat.comIM: amusil
>>
>> ___
>> 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
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On Thu, Aug 04, 2022 at 09:59:24AM +0100, Simon Horman wrote:
> On Thu, Aug 04, 2022 at 10:24:52AM +0200, Ilya Maximets wrote:
> > On 8/4/22 09:52, Simon Horman wrote:
> > > On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote:
> > >> On 8/4/22 09:18, Simon Horman wrote:
...
> > >>> I'd also be happy to apply this to the upstream tree
> > >>> if there are no objections from others.
> > >>
> > >> No objections from my side.
> > >>
> > >> Regarding backports, AFAICT, we'll need that change down to 2.16, right?
> > >> Can code from v1 be used on 2.17 and 2.16 or do we need a separate
> > >> backport patch?
> > >
> > > I think that the complexity that v3 addresses arises from metering support
> > > which allows other actions to follow a police action (meter).
> > >
> > > So, assuming metering is not present in 2.17 and 2.16, and given that it
> > > turns out that UNSPEC is fine for both PPS and BPS, then I suspect we
> > > can go for a simple backport-patch which simply changes TC_ACT_PIPE
> > > to TC_ACT_UNSPEC in nl_msg_act_police_end_nest().
> > >
> > > This would need some testing, IMHO, and I may end up eating the words
> > > above.
> >
> > If that works, sounds good to me.
> >
> > Vlad, could you also test this approach with 2.17 ?
> >
> > I think, the best way forward will be to apply v3 to master and branch-3.0
> > now. Once the approach above for 2.17/2.16 is tested to work or some other
> > solution is chosen, one of you could post the backport patch, so it can be
> > reviewed/applied.
> >
> > What do you think?
>
> Yes, I agree we should move forwards on applying this to branch-3.0,
> and tackle 2.17/2.16 separately.
Patch pushed to.
1. main
d9268782af4d ("netdev-linux: set correct action for packets that passed
policer")
https://github.com/openvswitch/ovs/commit/d9268782af4d7fe8872393c98c22f39bfc124610
2. branch-3.0
5ce25c4b75c1 ("netdev-linux: set correct action for packets that passed
policer")
https://github.com/openvswitch/ovs/commit/5ce25c4b75c1d09e13ef8566c5f86728e2a4c5b1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On Thu, Aug 04, 2022 at 10:24:52AM +0200, Ilya Maximets wrote: > On 8/4/22 09:52, Simon Horman wrote: > > On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote: > >> On 8/4/22 09:18, Simon Horman wrote: > >>> On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: > Referenced commit changed policer action type from TC_ACT_UNSPEC > (continue) > to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 > driver at the time validated action type and always assumed 'continue', > the > breakage wasn't caught until later validation code was added. The change > also broke valid configuration when sending from offload-capable device > to > non-offload capable. For example, when sending from mlx5 VF to OvS bridge > netdevice the traffic that passed matchall classifier with policer could > no > longer match the following flower rule in software: > >>> > >>> ... > >>> > Notes: > Changes V2 -> V3: > > - Refactored the fix to set action TC_ACT_UNSPEC only for > matchall/basic > classifiers. > > Changes V1 -> V2: > > - Rebase on latest master. > >>> > >>> ... > >>> > >>> Hi Vlad, > >>> > >>> thanks for addressing our concerns. We have reviewed and tested this patch > >>> and do not observe any problems for our use-cases: in particular mirred > >>> after meter (police). > >>> > >>> Acked-by: Simon Horman > >>> > >>> I'd also be happy to apply this to the upstream tree > >>> if there are no objections from others. > >> > >> No objections from my side. > >> > >> Regarding backports, AFAICT, we'll need that change down to 2.16, right? > >> Can code from v1 be used on 2.17 and 2.16 or do we need a separate > >> backport patch? > > > > I think that the complexity that v3 addresses arises from metering support > > which allows other actions to follow a police action (meter). > > > > So, assuming metering is not present in 2.17 and 2.16, and given that it > > turns out that UNSPEC is fine for both PPS and BPS, then I suspect we > > can go for a simple backport-patch which simply changes TC_ACT_PIPE > > to TC_ACT_UNSPEC in nl_msg_act_police_end_nest(). > > > > This would need some testing, IMHO, and I may end up eating the words > > above. > > If that works, sounds good to me. > > Vlad, could you also test this approach with 2.17 ? > > I think, the best way forward will be to apply v3 to master and branch-3.0 > now. Once the approach above for 2.17/2.16 is tested to work or some other > solution is chosen, one of you could post the backport patch, so it can be > reviewed/applied. > > What do you think? Yes, I agree we should move forwards on applying this to branch-3.0, and tackle 2.17/2.16 separately. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On 8/4/22 09:52, Simon Horman wrote: > On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote: >> On 8/4/22 09:18, Simon Horman wrote: >>> On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: Referenced commit changed policer action type from TC_ACT_UNSPEC (continue) to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 driver at the time validated action type and always assumed 'continue', the breakage wasn't caught until later validation code was added. The change also broke valid configuration when sending from offload-capable device to non-offload capable. For example, when sending from mlx5 VF to OvS bridge netdevice the traffic that passed matchall classifier with policer could no longer match the following flower rule in software: >>> >>> ... >>> Notes: Changes V2 -> V3: - Refactored the fix to set action TC_ACT_UNSPEC only for matchall/basic classifiers. Changes V1 -> V2: - Rebase on latest master. >>> >>> ... >>> >>> Hi Vlad, >>> >>> thanks for addressing our concerns. We have reviewed and tested this patch >>> and do not observe any problems for our use-cases: in particular mirred >>> after meter (police). >>> >>> Acked-by: Simon Horman >>> >>> I'd also be happy to apply this to the upstream tree >>> if there are no objections from others. >> >> No objections from my side. >> >> Regarding backports, AFAICT, we'll need that change down to 2.16, right? >> Can code from v1 be used on 2.17 and 2.16 or do we need a separate backport >> patch? > > I think that the complexity that v3 addresses arises from metering support > which allows other actions to follow a police action (meter). > > So, assuming metering is not present in 2.17 and 2.16, and given that it > turns out that UNSPEC is fine for both PPS and BPS, then I suspect we > can go for a simple backport-patch which simply changes TC_ACT_PIPE > to TC_ACT_UNSPEC in nl_msg_act_police_end_nest(). > > This would need some testing, IMHO, and I may end up eating the words > above. If that works, sounds good to me. Vlad, could you also test this approach with 2.17 ? I think, the best way forward will be to apply v3 to master and branch-3.0 now. Once the approach above for 2.17/2.16 is tested to work or some other solution is chosen, one of you could post the backport patch, so it can be reviewed/applied. What do you think? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Use datapath groups for southbound load balancers.
On 8/3/22 16:43, Ilya Maximets wrote: > Some CMSes, like ovn-kubernetes, tend to create load balancers attached > to a big number of logical switches or routers. For each of these > load balancers northd creates a record in Load_Balancer table of the > Southbound database with all the logical datapaths (switches, routers) > listed in the 'datapaths' column. All these logical datapaths are > references to datapath bindings. With large number of load balancers > like these applied to the same set of load balancers, the size of > the Southbound database can grow significantly and these references > can take up to 90% of all the traffic between Sb DB and ovn-controllers. > > For example, while creating 3 load balancers (1 for tcp, udp and sctp) > in a 250-node cluster in ovn-heater cluster-density test, the database > update sent to every ovn-controller is about 40 KB in size, out of > which 36 KB are just UUIDs of 250 logical datapaths repeated 3 times. > > Introducing a new column 'datapath_group' in a Load_Balancer table, > so we can create a group once and just refer to it from each load > balancer. This saves a lot of CPU time, memory and network bandwidth. > Re-using already existing Logical_DP_Group table to store these groups. > > In 250 node cluster-density test with ovn-heater that creates 30K load > balancers applied to all 250 logical switches the following improvement > was observed: > > - Southbound database size decreased from 310 MB to 118 MB. > - CPU time on Southbound ovsdb-server process decreased by a >factor of 7, from ~35 minutes per server to ~5. > - Memory consumption on Southbound ovsdb-server process decreased >from 12 GB (peaked at ~14) RSS down to ~1 GB (peaked at ~2). > - Memory consumption on ovn-controller processes decreased >by 400 MB on each node, from 1300 MB to 900 MB. Similar decrease >observed for ovn-northd as well. > > We're adding some extra work to ovn-northd process with this change > to find/create datapath groups. CPU time in the test above increased > by ~10%, but overall round trip time for changes in OVN to be > propagated to OVN controllers is still noticeably lower due to > improvements in other components like Sb DB. Actually, I was using the data from the prliminary run for this commit message and also looked into a wrong place while checking northd performance numbers :/ . Re-run with the version of the code in this patch shows 12% performance improvement on ovn-northd looking at actual length of poll intervals. So, northd-related paragraph above shuold be thrown away and replaced with one more bullet in observed improvements: - Poll intervals on ovn-northd reduced by 12%. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On Thu, Aug 04, 2022 at 09:33:15AM +0200, Ilya Maximets wrote: > On 8/4/22 09:18, Simon Horman wrote: > > On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: > >> Referenced commit changed policer action type from TC_ACT_UNSPEC (continue) > >> to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 > >> driver at the time validated action type and always assumed 'continue', the > >> breakage wasn't caught until later validation code was added. The change > >> also broke valid configuration when sending from offload-capable device to > >> non-offload capable. For example, when sending from mlx5 VF to OvS bridge > >> netdevice the traffic that passed matchall classifier with policer could no > >> longer match the following flower rule in software: > > > > ... > > > >> Notes: > >> Changes V2 -> V3: > >> > >> - Refactored the fix to set action TC_ACT_UNSPEC only for > >> matchall/basic > >> classifiers. > >> > >> Changes V1 -> V2: > >> > >> - Rebase on latest master. > > > > ... > > > > Hi Vlad, > > > > thanks for addressing our concerns. We have reviewed and tested this patch > > and do not observe any problems for our use-cases: in particular mirred > > after meter (police). > > > > Acked-by: Simon Horman > > > > I'd also be happy to apply this to the upstream tree > > if there are no objections from others. > > No objections from my side. > > Regarding backports, AFAICT, we'll need that change down to 2.16, right? > Can code from v1 be used on 2.17 and 2.16 or do we need a separate backport > patch? I think that the complexity that v3 addresses arises from metering support which allows other actions to follow a police action (meter). So, assuming metering is not present in 2.17 and 2.16, and given that it turns out that UNSPEC is fine for both PPS and BPS, then I suspect we can go for a simple backport-patch which simply changes TC_ACT_PIPE to TC_ACT_UNSPEC in nl_msg_act_police_end_nest(). This would need some testing, IMHO, and I may end up eating the words above. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On 8/4/22 09:18, Simon Horman wrote: > On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: >> Referenced commit changed policer action type from TC_ACT_UNSPEC (continue) >> to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 >> driver at the time validated action type and always assumed 'continue', the >> breakage wasn't caught until later validation code was added. The change >> also broke valid configuration when sending from offload-capable device to >> non-offload capable. For example, when sending from mlx5 VF to OvS bridge >> netdevice the traffic that passed matchall classifier with policer could no >> longer match the following flower rule in software: > > ... > >> Notes: >> Changes V2 -> V3: >> >> - Refactored the fix to set action TC_ACT_UNSPEC only for matchall/basic >> classifiers. >> >> Changes V1 -> V2: >> >> - Rebase on latest master. > > ... > > Hi Vlad, > > thanks for addressing our concerns. We have reviewed and tested this patch > and do not observe any problems for our use-cases: in particular mirred > after meter (police). > > Acked-by: Simon Horman > > I'd also be happy to apply this to the upstream tree > if there are no objections from others. No objections from my side. Regarding backports, AFAICT, we'll need that change down to 2.16, right? Can code from v1 be used on 2.17 and 2.16 or do we need a separate backport patch? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: set correct action for packets that passed policer
On Wed, Aug 03, 2022 at 12:32:46PM +0200, Vlad Buslov wrote: > Referenced commit changed policer action type from TC_ACT_UNSPEC (continue) > to TC_ACT_PIPE. However, since neither TC hardware offload layer nor mlx5 > driver at the time validated action type and always assumed 'continue', the > breakage wasn't caught until later validation code was added. The change > also broke valid configuration when sending from offload-capable device to > non-offload capable. For example, when sending from mlx5 VF to OvS bridge > netdevice the traffic that passed matchall classifier with policer could no > longer match the following flower rule in software: ... > Notes: > Changes V2 -> V3: > > - Refactored the fix to set action TC_ACT_UNSPEC only for matchall/basic > classifiers. > > Changes V1 -> V2: > > - Rebase on latest master. ... Hi Vlad, thanks for addressing our concerns. We have reviewed and tested this patch and do not observe any problems for our use-cases: in particular mirred after meter (police). Acked-by: Simon Horman I'd also be happy to apply this to the upstream tree if there are no objections from others. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
