Re: [ovs-dev] RFC testsuite: fmt_pkt performance improvements
On 6/9/23 18:19, Ihar Hrachyshka wrote: > This thread is to discuss improvements that would allow OVN (and maybe > OVS?) to adopt fmt_pkt (frontend to scapy packet constructor) in the test > suite. > > (For those not aware of what it is, please consult this commit: > > https://github.com/ovn-org/ovn/commit/d8c0d3ae4f3a293737d662a865761235ed9ea1cf > > There are a number of examples of usage of the tool in the OVN tree, please > `grep` for `fmt_pkt`. > > tl;dr it allows us to get rid of hex strings that define packets under test > and replace them with symbolic representations of packet format. > > Here is what I sent recently to my colleagues that captures my current > thinking. What I'm looking for is: feedback on the ideas listed, maybe also > their relative priorities. I'm also looking for any other ideas that would > improve scapy without destroying its value in speed of test composition and > consumption by human beings. > > I may or may not work on these improvements in the near future, but > regardless, let's at least agree on the direction this tool should take. > > === > > **Opinion**: fmt_pkt is great. > > **Problem**: it’s slow: every time it’s invoked, a `python` interpreter is > started, all `scapy` modules are loaded, then a packet string > representation is produced. This takes a long time. If we want to adopt it > wider for the test suite, we'll have to make it quicker. > > ``` > stack@ubuntu2204:~/neutron$ time python3 -c 'from scapy.all import *' > > real0m0.551s > user0m0.454s > sys 0m0.093s > ``` > > If a test does it a number of times, the total clock time runs to a minute > for a single test case. It doesn't scale. > > **Ideas** > > 1. Don't import all modules from `scapy`. > > Initial tests suggest that tailoring the list of `scapy` modules to import > significantly reduces the time spent to produce a string for a packet. > > ``` > stack@ubuntu2204:~/neutron$ time python3 -c 'from scapy.all import *' > > real0m0.551s > user0m0.454s > sys 0m0.093s > stack@ubuntu2204:~/neutron$ time python3 -c 'from scapy.layers import l2' > > real0m0.190s > user0m0.161s > sys 0m0.025s > ``` > > In the example above, we shaved 2/3 of the time. > > How the list of imported modules is passed into `fmt_pkt` is subject to > discussion. > > - One way is making the caller pass the list - or a macro that refers to a > pre-defined list - to each call. This can be further hidden from the caller > by introducing special helpers for specific types of packets, e.g. have > `fmt_l2_pkt` or `fmt_tcp_pkt` depending on how specific we would like to be. > - Another way is making the test case declare the list of modules of > interest before using `fmt_pkt` once at the start, then assume this > case-scoped global setting is applied to all consequent `fmt_pkt` calls in > the test case. > > 2. Avoid duplicate module imports by running a `scapy` wrapper daemon. > > The daemon would start `python`, load the (necessary?) `scapy` modules, > open a UNIX socket and run a simple server that will: > > - receive a symbolic string from `fmt_pkt` call that represents the packet; > - run the transformation step to produce a string that represents the > packet byte stream; > - return the byte stream through the socket. > > Since the test suite is executed in public CI on patches posted to ML by > unauthorized users, the daemon must expect - and deny - transformation for > anything that doesn't look like a `scapy` packet definition. > > Whether the daemon should be scoped to a test case or to the whole run is > something to discuss. > > - On one hand, scoping the daemon to a test case simplifies the cleanup and > tracking of the process. (We can make sure it's killed during the regular > CLEANUP phase for a particular test case.) > - On the other hand, running the same daemon for all test cases shaves time > spent to start it and import `scapy` modules to near-zero (we could say > it's `O(1)` if we feel fancy.) I don't know if having a global daemon for > the whole test suite run complicates the cleanup procedure. > > If we go with per-case daemon mode, then we should at least not start it > for test cases that won't use `fmt_pkt` at all. I don't know which > mechanism should be used to track these. Perhaps there should be a macro > that tags tests for this setup step. > > 3. If `fmt_pkt` proves to be useful and successful in broader scope and > gets adopted in most of test cases, we may consider making daemon startup > the default behavior for a test suite run. > > 4. If this happens, we should as well remove the `HAVE_SCAPY` macro and > assume it's always installed on a test node. > > === > > Looking forward to any comments, Thanks for putting this together Ihar! I don't have a lot of comments but, if I get a vote, I vote for option 3 above. Regards, Dumitru > > Cheers, > Ihar > ___ > dev mailing list >
Re: [ovs-dev] [PATCH v5] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions
On 9 Jun 2023, at 17:05, Mike Pattrick wrote:
> Several xlate actions used in recursive translation currently store a
> large amount of information on the stack. This can result in handler
> threads quickly running out of stack space despite before
> xlate_resubmit_resource_check() is able to terminate translation. This
> patch reduces stack usage by over 3kb from several translation actions.
>
> This patch also moves some trace function from do_xlate_actions into its
> own function to reduce stack usage.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779
> Signed-off-by: Mike Pattrick
> Reviewed-by: Simon Horman
Changes look good to me, one small nit below, but not related to your changes.
So if no more revs are needed, we can leave it as is.
Acked-by: Eelco Chaudron
> ---
> Since v1:
> - Refactored code into specialized functions and renamed variables for
> improved readability.
> Since v2:
> - Removed inline keywords
> Since v3:
> - Improved formatting.
> Since v4:
> - v4 patch was an incorrect revision
> ---
> ofproto/ofproto-dpif-xlate.c | 211 ++-
> 1 file changed, 135 insertions(+), 76 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..9e6d5138d 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -501,6 +501,82 @@ ctx_cancel_freeze(struct xlate_ctx *ctx)
>
> static void finish_freezing(struct xlate_ctx *ctx);
>
> +/* These functions and structure are used to save stack space in actions that
> + * need to retain a large amount of xlate_ctx state. */
> +struct xretained_state {
> +union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
> +uint64_t actset_stub[1024 / 8];
> +struct ofpbuf old_stack;
> +struct ofpbuf old_action_set;
> +struct flow old_flow;
> +struct flow old_base;
> +struct flow_tnl flow_tnl_mask;
> +};
> +
> +/* The return of this function must be freed by xretain_state_restore(). */
> +static struct xretained_state *
> +xretain_state_save(struct xlate_ctx *ctx)
> +{
> +struct xretained_state *retained = xmalloc(sizeof *retained);
> +
> +retained->old_flow = ctx->xin->flow;
> +retained->old_stack = ctx->stack;
> +retained->old_action_set = ctx->action_set;
> +ofpbuf_use_stub(>stack, retained->new_stack,
> +sizeof retained->new_stack);
> +ofpbuf_use_stub(>action_set, retained->actset_stub,
> +sizeof retained->actset_stub);
> +
> +return retained;
> +}
> +
> +static void
> +xretain_tunnel_mask_save(struct xlate_ctx *ctx,
> + struct xretained_state *retained)
> +{
> +retained->flow_tnl_mask = ctx->wc->masks.tunnel;
> +}
> +
> +static void
> +xretain_base_flow_save(const struct xlate_ctx *ctx,
> + struct xretained_state *retained)
> +{
> +retained->old_base = ctx->base_flow;
> +}
> +
> +static void
> +xretain_base_flow_restore(struct xlate_ctx *ctx,
> + const struct xretained_state *retained)
> +{
> +ctx->base_flow = retained->old_base;
> +}
> +
> +static void
> +xretain_flow_restore(struct xlate_ctx *ctx,
> + const struct xretained_state *retained)
> +{
> +ctx->xin->flow = retained->old_flow;
> +}
> +
> +static void
> +xretain_tunnel_mask_restore(struct xlate_ctx *ctx,
> +const struct xretained_state *retained)
> +{
> +ctx->wc->masks.tunnel = retained->flow_tnl_mask;
> +}
> +
> +static void
> +xretain_state_restore(struct xlate_ctx *ctx, struct xretained_state
> *retained)
> +{
> +ctx->xin->flow = retained->old_flow;
> +ofpbuf_uninit(>action_set);
> +ctx->action_set = retained->old_action_set;
> +ofpbuf_uninit(>stack);
> +ctx->stack = retained->old_stack;
> +
> +free(retained);
> +}
> +
> /* A controller may use OFPP_NONE as the ingress port to indicate that
> * it did not arrive on a "real" port. 'ofpp_none_bundle' exists for
> * when an input bundle is needed for validation (e.g., mirroring or
> @@ -3915,20 +3991,17 @@ static void
> patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
>struct xport *out_dev, bool is_last_action)
> {
> -struct flow *flow = >xin->flow;
> -struct flow old_flow = ctx->xin->flow;
> -struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel;
> +struct ovs_list *old_trace = ctx->xin->trace;
> bool old_conntrack = ctx->conntracked;
> +struct flow *flow = >xin->flow;
> bool old_was_mpls = ctx->was_mpls;
> +struct xretained_state *retained_state;
> ovs_version_t old_version = ctx->xin->tables_version;
> -struct ofpbuf old_stack = ctx->stack;
> -uint8_t new_stack[1024];
> -struct ofpbuf old_action_set = ctx->action_set;
> -struct ovs_list *old_trace = ctx->xin->trace;
> -uint64_t actset_stub[1024 / 8];
nit: Maybe re-order the variable declaration
Re: [ovs-dev] [PATCH ovn] ci: ovn-kubernetes: Figure out dependencies dynamically.
Dumitru Ceara writes: > On 6/22/23 13:56, Aaron Conole wrote: >> Dumitru Ceara writes: >> >>> This avoids manual intervention when upstream ovn-kubernetes changes >>> its dependency versions. >>> >>> Signed-off-by: Patryk Diak >>> Co-authored-by: Patryk Diak >>> Signed-off-by: Dumitru Ceara >>> --- >> >> LGTM overall. It's also the only patch to have successfully built >> recently, so I assume it will be important to apply for other patches to >> succeed. >> > > Thanks for the review! Yes, all other patches were failing ovn-kube CI > when building the container image, effectively keeping us in the dark > wrt effects on ovn-kubernetes. > >>> .ci/ovn-kubernetes/Dockerfile| 16 +++--- >>> .ci/ovn-kubernetes/prepare.sh| 11 ++ >>> .github/workflows/ovn-kubernetes.yml | 31 +--- >> >> We got some warnings on this file due to line lengths. Maybe we should >> exclude yml from the line length check. WDYT? >> > > Sure, I can post a patch for that. Should I prepare a patch for OVS > though? We normally try to keep our checkpatch version in sync with the > OVS one. Please do :) Then they can just be sync'd after, I guess. >>> 3 files changed, 39 insertions(+), 19 deletions(-) >> >> Acked-by: Aaron Conole >> > > I pushed this patch to main. Hopefully the CI turns green soon. > > Regards, > Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12] netdev-dpdk: Add custom rx-steering configuration.
On 22/06/2023 14:06, Kevin Traynor wrote:
On 21/06/2023 20:24, Robin Jarry wrote:
Some control protocols are used to maintain link status between
forwarding engines (e.g. LACP). When the system is not sized properly,
the PMD threads may not be able to process all incoming traffic from the
configured Rx queues. When a signaling packet of such protocols is
dropped, it can cause link flapping, worsening the situation.
Use the RTE flow API to redirect these protocols into a dedicated Rx
queue. The assumption is made that the ratio between control protocol
traffic and user data traffic is very low and thus this dedicated Rx
queue will never get full. Re-program the RSS redirection table to only
use the other Rx queues.
The additional Rx queue will be assigned a PMD core like any other Rx
queue. Polling that extra queue may introduce increased latency and
a slight performance penalty at the benefit of preventing link flapping.
This feature must be enabled per port on specific protocols via the
rx-steering option. This option takes "rss" followed by a "+" separated
list of protocol names. It is only supported on ethernet ports. This
feature is experimental.
If the user has already configured multiple Rx queues on the port, an
additional one will be allocated for control packets. If the hardware
cannot satisfy the number of requested Rx queues, the last Rx queue will
be assigned for control plane. If only one Rx queue is available, the
rx-steering feature will be disabled. If the hardware does not support
the RTE flow matchers/actions, the rx-steering feature will be
completely disabled on the port.
It cannot be enabled when other_config:hw-offload=true as it may
conflict with the offloaded RTE flows. Similarly, if hw-offload is
enabled, custom rx-steering will be forcibly disabled on all ports.
Example use:
ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \
set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \
set interface phy0 options:rx-steering=rss+lacp -- \
set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \
set interface phy1 options:rx-steering=rss+lacp
As a starting point, only one protocol is supported: LACP. Other
protocols can be added in the future. NIC compatibility should be
checked.
To validate that this works as intended, I used a traffic generator to
generate random traffic slightly above the machine capacity at line rate
on a two ports bond interface. OVS is configured to receive traffic on
two VLANs and pop/push them in a br-int bridge based on tags set on
patch ports.
+--+
| DUT |
|++|
|| br-int ||
in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11
||||
in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10
|| patch10patch11 ||
|+---|---|+|
|| | |
|+---|---|+|
|| patch00patch01 ||
|| tag:10tag:20 ||
||||
|| br-phy || default flow, action=NORMAL
||||
|| bond0|| balance-slb, lacp=passive, lacp-time=fast
||phy0 phy1 ||
|+--|-|---+|
+---|-|+
| |
+---|-|+
| port0 port1 | balance L3/L4, lacp=active, lacp-time=fast
| lag | mode trunk VLANs 10, 20
| |
|switch|
| |
| vlan 10vlan 20 | mode access
| port2 port3 |
+-|--|-+
| |
+-|--|-+
| tgen0 tgen1 | Random traffic that is properly balanced
| | across the bond ports in both directions.
| traffic generator |
+--+
Without rx-steering, the bond0 links are randomly switching to
"defaulted" when one of the LACP packets sent by the switch is dropped
because the RX queues are full and the PMD threads did not process them
fast enough. When that happens, all traffic must go through a single
link which causes above line rate traffic to be dropped.
~# ovs-appctl lacp/show-stats bond0
bond0 statistics
member: phy0:
TX PDUs: 347246
RX PDUs: 14865
RX Bad PDUs: 0
RX Marker Request PDUs: 0
Link Expired: 168
Link Defaulted: 0
Carrier Status Changed: 0
member: phy1:
TX PDUs: 347245
RX PDUs: 14919
RX Bad PDUs: 0
RX Marker Request PDUs: 0
Link Expired: 147
Link Defaulted: 1
Carrier Status Changed: 0
When rx-steering is enabled, no LACP packet is dropped and the bond
links remain enabled at all times, maximizing the throughput. Neither
the "Link Expired" nor the "Link Defaulted" counters are incremented
anymore.
This feature may be
Re: [ovs-dev] [PATCH v12] netdev-dpdk: Add custom rx-steering configuration.
Hi Kevin, Kevin Traynor, Jun 22, 2023 at 15:06: > Hi Robin, > > As discussed offline, the issue is fixed in v12. In the case where > offload has failed on device A, and device B triggers set_config the > set_config for device B will see that device B has the correct number of > queues (user_n_rxq), so there will not be a reconfigure of *that* > device. I tested this by faking some failed offloads with gdb. > > If a reconfigure for device A does occur for some other reason, then it > will attempt to apply the config again, (presumably) the offload will > fail again and the normal retry occurs without offload. This seems the > correct behaviour to me. Somehow I did assume that netdev_dpdk_reconfigure() would be called for all ports regardless if their configuration had changed. Good to know that the problem is now fixed. > For v12, I also tested some basic increasing decreasing rxqs, enabling > hwol and it's working as expected. > > thanks, > Kevin. > > Acked-by: Kevin Traynor Thanks a lot! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12] netdev-dpdk: Add custom rx-steering configuration.
On 21/06/2023 20:24, Robin Jarry wrote: Some control protocols are used to maintain link status between forwarding engines (e.g. LACP). When the system is not sized properly, the PMD threads may not be able to process all incoming traffic from the configured Rx queues. When a signaling packet of such protocols is dropped, it can cause link flapping, worsening the situation. Use the RTE flow API to redirect these protocols into a dedicated Rx queue. The assumption is made that the ratio between control protocol traffic and user data traffic is very low and thus this dedicated Rx queue will never get full. Re-program the RSS redirection table to only use the other Rx queues. The additional Rx queue will be assigned a PMD core like any other Rx queue. Polling that extra queue may introduce increased latency and a slight performance penalty at the benefit of preventing link flapping. This feature must be enabled per port on specific protocols via the rx-steering option. This option takes "rss" followed by a "+" separated list of protocol names. It is only supported on ethernet ports. This feature is experimental. If the user has already configured multiple Rx queues on the port, an additional one will be allocated for control packets. If the hardware cannot satisfy the number of requested Rx queues, the last Rx queue will be assigned for control plane. If only one Rx queue is available, the rx-steering feature will be disabled. If the hardware does not support the RTE flow matchers/actions, the rx-steering feature will be completely disabled on the port. It cannot be enabled when other_config:hw-offload=true as it may conflict with the offloaded RTE flows. Similarly, if hw-offload is enabled, custom rx-steering will be forcibly disabled on all ports. Example use: ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \ set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \ set interface phy0 options:rx-steering=rss+lacp -- \ set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \ set interface phy1 options:rx-steering=rss+lacp As a starting point, only one protocol is supported: LACP. Other protocols can be added in the future. NIC compatibility should be checked. To validate that this works as intended, I used a traffic generator to generate random traffic slightly above the machine capacity at line rate on a two ports bond interface. OVS is configured to receive traffic on two VLANs and pop/push them in a br-int bridge based on tags set on patch ports. +--+ | DUT | |++| || br-int || in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11 |||| in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10 || patch10patch11 || |+---|---|+| || | | |+---|---|+| || patch00patch01 || || tag:10tag:20 || |||| || br-phy || default flow, action=NORMAL |||| || bond0|| balance-slb, lacp=passive, lacp-time=fast ||phy0 phy1 || |+--|-|---+| +---|-|+ | | +---|-|+ | port0 port1 | balance L3/L4, lacp=active, lacp-time=fast | lag | mode trunk VLANs 10, 20 | | |switch| | | | vlan 10vlan 20 | mode access | port2 port3 | +-|--|-+ | | +-|--|-+ | tgen0 tgen1 | Random traffic that is properly balanced | | across the bond ports in both directions. | traffic generator | +--+ Without rx-steering, the bond0 links are randomly switching to "defaulted" when one of the LACP packets sent by the switch is dropped because the RX queues are full and the PMD threads did not process them fast enough. When that happens, all traffic must go through a single link which causes above line rate traffic to be dropped. ~# ovs-appctl lacp/show-stats bond0 bond0 statistics member: phy0: TX PDUs: 347246 RX PDUs: 14865 RX Bad PDUs: 0 RX Marker Request PDUs: 0 Link Expired: 168 Link Defaulted: 0 Carrier Status Changed: 0 member: phy1: TX PDUs: 347245 RX PDUs: 14919 RX Bad PDUs: 0 RX Marker Request PDUs: 0 Link Expired: 147 Link Defaulted: 1 Carrier Status Changed: 0 When rx-steering is enabled, no LACP packet is dropped and the bond links remain enabled at all times, maximizing the throughput. Neither the "Link Expired" nor the "Link Defaulted" counters are incremented anymore. This feature may be considered as "QoS". However, it does not work by limiting the rate of traffic explicitly. It only
Re: [ovs-dev] [PATCH ovn] ci: ovn-kubernetes: Figure out dependencies dynamically.
On 6/22/23 14:45, Dumitru Ceara wrote: > On 6/22/23 13:56, Aaron Conole wrote: >> Dumitru Ceara writes: >> >>> This avoids manual intervention when upstream ovn-kubernetes changes >>> its dependency versions. >>> >>> Signed-off-by: Patryk Diak >>> Co-authored-by: Patryk Diak >>> Signed-off-by: Dumitru Ceara >>> --- >> >> LGTM overall. It's also the only patch to have successfully built >> recently, so I assume it will be important to apply for other patches to >> succeed. >> > > Thanks for the review! Yes, all other patches were failing ovn-kube CI > when building the container image, effectively keeping us in the dark > wrt effects on ovn-kubernetes. > >>> .ci/ovn-kubernetes/Dockerfile| 16 +++--- >>> .ci/ovn-kubernetes/prepare.sh| 11 ++ >>> .github/workflows/ovn-kubernetes.yml | 31 +--- >> >> We got some warnings on this file due to line lengths. Maybe we should >> exclude yml from the line length check. WDYT? >> > > Sure, I can post a patch for that. Should I prepare a patch for OVS > though? We normally try to keep our checkpatch version in sync with the > OVS one. > >>> 3 files changed, 39 insertions(+), 19 deletions(-) >> >> Acked-by: Aaron Conole >> > > I pushed this patch to main. Hopefully the CI turns green soon. > I forgot to mention: I added Patryk to the list of authors in AUTHORS.rst. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ci: ovn-kubernetes: Figure out dependencies dynamically.
On 6/22/23 13:56, Aaron Conole wrote: > Dumitru Ceara writes: > >> This avoids manual intervention when upstream ovn-kubernetes changes >> its dependency versions. >> >> Signed-off-by: Patryk Diak >> Co-authored-by: Patryk Diak >> Signed-off-by: Dumitru Ceara >> --- > > LGTM overall. It's also the only patch to have successfully built > recently, so I assume it will be important to apply for other patches to > succeed. > Thanks for the review! Yes, all other patches were failing ovn-kube CI when building the container image, effectively keeping us in the dark wrt effects on ovn-kubernetes. >> .ci/ovn-kubernetes/Dockerfile| 16 +++--- >> .ci/ovn-kubernetes/prepare.sh| 11 ++ >> .github/workflows/ovn-kubernetes.yml | 31 +--- > > We got some warnings on this file due to line lengths. Maybe we should > exclude yml from the line length check. WDYT? > Sure, I can post a patch for that. Should I prepare a patch for OVS though? We normally try to keep our checkpatch version in sync with the OVS one. >> 3 files changed, 39 insertions(+), 19 deletions(-) > > Acked-by: Aaron Conole > I pushed this patch to main. Hopefully the CI turns green soon. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v13] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back
On 22 Jun 2023, at 13:29, Ilya Maximets wrote:
> On 6/22/23 11:30, Eelco Chaudron wrote:
>>
>>
>> On 22 Jun 2023, at 11:15, Ilya Maximets wrote:
>>
>>> On 6/22/23 10:50, Eelco Chaudron wrote:
On 30 May 2023, at 9:32, Eelco Chaudron wrote:
> On 26 May 2023, at 22:51, Ilya Maximets wrote:
>
>> On 5/26/23 15:09, Eelco Chaudron wrote:
>>>
>>>
>>> On 26 May 2023, at 14:03, Balazs Nemeth wrote:
>>>
The only way that stats->{n_packets,n_bytes} would decrease is due to
an
overflow, or if there are bugs in how statistics are handled. In the
past, there were multiple issues that caused a jump backward. A
workaround was in place to set the statistics to 0 in that case. When
this happened while the revalidator was under heavy load, the
workaround
had an unintended side effect where should_revalidate returned false
causing the flow to be removed because the metric it calculated was
based on a bogus value. Since many of those bugs have now been
identified and resolved, there is no need to set the statistics to 0.
In
addition, the (unlikely) overflow still needs to be handled
appropriately. If an unexpected jump does happen, just log it as a
warning.
Signed-off-by: Balazs Nemeth
>>>
>>> Thanks for making the final change! Here is an example of the log
>>> message for others reviewing:
>>>
>>> 2023-05-26T12:55:07.590Z|3|ofproto_dpif_upcall(revalidator14)|WARN|Unexpected
>>> jump in packet stats from 0 to 1 when handling ukey
>>> ufid:90c082b0-7aa6-442a-9b86-04d10fe9ede6
>>> recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0)
>>> ,ct_mark(0),ct_label(0),eth(src=6e:48:c8:77:d3:8c,dst=33:33:00:00:00:16),eth_type(0x86dd),ipv6(src=::,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no),key32(40
>>> 00),icmpv6(type=143,code=0), actions:1,3
>>>
>>> One nit on a missing “,” compared to the dp flow dump, but I think Ilya
>>> can add this on commit.
>>>
>>> Acked-by: Eelco Chaudron
>>
>> Thanks! I added the comma and applied the patch.
>
> Are you planning on back porting this, as all other relevant patches
> where?
Ilya did you maybe miss this email? Question still applies ;)
>>>
>>> Yeah, sorry, it fell through the cracks.
>>>
>>> I'm a bit worried about backporting this one, because the
>>> original reason for the stats jump is still not clear and
>>> so might still exist. I guess, we could backport to 3.1,
>>> but I'd like not to backport to LTS.
>>>
>>> What do you think?
>>
>> I’m happy with getting this in 3.1 ;)
>
> OK. Backported to 3.1 now.
>
> Best regards, Ilya Maximets.
Thanks!!
//Eelco
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ci: ovn-kubernetes: Figure out dependencies dynamically.
Dumitru Ceara writes: > This avoids manual intervention when upstream ovn-kubernetes changes > its dependency versions. > > Signed-off-by: Patryk Diak > Co-authored-by: Patryk Diak > Signed-off-by: Dumitru Ceara > --- LGTM overall. It's also the only patch to have successfully built recently, so I assume it will be important to apply for other patches to succeed. > .ci/ovn-kubernetes/Dockerfile| 16 +++--- > .ci/ovn-kubernetes/prepare.sh| 11 ++ > .github/workflows/ovn-kubernetes.yml | 31 +--- We got some warnings on this file due to line lengths. Maybe we should exclude yml from the line length check. WDYT? > 3 files changed, 39 insertions(+), 19 deletions(-) Acked-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v13] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back
On 6/22/23 11:30, Eelco Chaudron wrote:
>
>
> On 22 Jun 2023, at 11:15, Ilya Maximets wrote:
>
>> On 6/22/23 10:50, Eelco Chaudron wrote:
>>>
>>>
>>> On 30 May 2023, at 9:32, Eelco Chaudron wrote:
>>>
On 26 May 2023, at 22:51, Ilya Maximets wrote:
> On 5/26/23 15:09, Eelco Chaudron wrote:
>>
>>
>> On 26 May 2023, at 14:03, Balazs Nemeth wrote:
>>
>>> The only way that stats->{n_packets,n_bytes} would decrease is due to an
>>> overflow, or if there are bugs in how statistics are handled. In the
>>> past, there were multiple issues that caused a jump backward. A
>>> workaround was in place to set the statistics to 0 in that case. When
>>> this happened while the revalidator was under heavy load, the workaround
>>> had an unintended side effect where should_revalidate returned false
>>> causing the flow to be removed because the metric it calculated was
>>> based on a bogus value. Since many of those bugs have now been
>>> identified and resolved, there is no need to set the statistics to 0. In
>>> addition, the (unlikely) overflow still needs to be handled
>>> appropriately. If an unexpected jump does happen, just log it as a
>>> warning.
>>>
>>> Signed-off-by: Balazs Nemeth
>>
>> Thanks for making the final change! Here is an example of the log
>> message for others reviewing:
>>
>> 2023-05-26T12:55:07.590Z|3|ofproto_dpif_upcall(revalidator14)|WARN|Unexpected
>> jump in packet stats from 0 to 1 when handling ukey
>> ufid:90c082b0-7aa6-442a-9b86-04d10fe9ede6
>> recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0)
>> ,ct_mark(0),ct_label(0),eth(src=6e:48:c8:77:d3:8c,dst=33:33:00:00:00:16),eth_type(0x86dd),ipv6(src=::,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no),key32(40
>> 00),icmpv6(type=143,code=0), actions:1,3
>>
>> One nit on a missing “,” compared to the dp flow dump, but I think Ilya
>> can add this on commit.
>>
>> Acked-by: Eelco Chaudron
>
> Thanks! I added the comma and applied the patch.
Are you planning on back porting this, as all other relevant patches where?
>>>
>>> Ilya did you maybe miss this email? Question still applies ;)
>>
>> Yeah, sorry, it fell through the cracks.
>>
>> I'm a bit worried about backporting this one, because the
>> original reason for the stats jump is still not clear and
>> so might still exist. I guess, we could backport to 3.1,
>> but I'd like not to backport to LTS.
>>
>> What do you think?
>
> I’m happy with getting this in 3.1 ;)
OK. Backported to 3.1 now.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] dpif-netdev: Update meter timestamp when dp_netdev_run_meter been called.
Hi Ilya,
Thanks for reviewing our code.
We have 20+ threads indeed, so taking the same meter lock makes 'now'
not updated timely.
Using lockless meter to police traffic will be a perfect solution. :)
Thanks a lot.
On 6/22/2023 6:35 AM, Ilya Maximets wrote:
On 6/21/23 18:07, Ilya Maximets wrote:
On 5/31/23 17:41,mit...@outlook.com wrote:
From: Lin Huang
Currently, a meter's timestamp 'now' is set by 'pmd->ctx.now' which updated
by pmd_thread_ctx_time_update().
Before processing of the new packet batch:
- dpif_netdev_execute()
- dp_netdev_process_rxq_port()
There is a problem when user want to police the rate to a low pps by meter.
For example, When the traffic is more than 3Mpps, policing the traffic to
1M pps, the real rate will be 3Mpps not 1Mpps.
The key point is that a meter's timestamp isn't update in real time.
For example, PMD thread A and B are polled at the same time (t1).
Thread A starts to run meter to police traffic. At the same time, thread B
pause at 'ovs_mutex_lock(>lock)' to wait thread A release the meter
lock. This elapsed a lot of time, especially we have more than 10 PMD threads.
Shouldn't the infrequent time update cause more drops instead?
Also, having 10+ threads waiting on the same lock doesn't sound like
a particularly efficient setup. If the locking here indeed a problem,
maybe you can try the following patch in your scenario:
https://patchwork.ozlabs.org/project/openvswitch/patch/20230621223220.2073152-1-i.maxim...@ovn.org/
?
Best regards, Ilya Maximets.
After thread A release the meter lock, thread B start to count the time diff.
- long_delta_t = now - meter->used' --> long_delta_t = t1 - t1 = 0.
- band->bucket += (uint64_t) delta_t * band->rate --> band->bucket = 0.
- band_exceeded_pkt = band->bucket / 1000; --> band_exceeded_pkt = 0.
Fix this problem by update the meter timestamp every time.
Test-by: Zhang Yuhuang
Signed-off-by: Lin Huang
---
lib/dpif-netdev.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 70b953ae6..dbb275cf8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7169,12 +7169,13 @@ dpif_netdev_meter_get_features(const struct dpif * dpif
OVS_UNUSED,
* that exceed a band are dropped in-place. */
static void
dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
-uint32_t meter_id, long long int now)
+uint32_t meter_id)
{
struct dp_meter *meter;
struct dp_meter_band *band;
struct dp_packet *packet;
long long int long_delta_t; /* msec */
+long long int now;
uint32_t delta_t; /* msec */
const size_t cnt = dp_packet_batch_size(packets_);
uint32_t bytes, volume;
@@ -7197,8 +7198,12 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
dp_packet_batch *packets_,
memset(exceeded_rate, 0, cnt * sizeof *exceeded_rate);
ovs_mutex_lock(>lock);
+
+/* Update now */
+now = time_msec();
+
/* All packets will hit the meter at the same time. */
-long_delta_t = now / 1000 - meter->used / 1000; /* msec */
+long_delta_t = now - meter->used; /* msec */
if (long_delta_t < 0) {
/* This condition means that we have several threads fighting for a
@@ -9170,8 +9175,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
*packets_,
}
case OVS_ACTION_ATTR_METER:
-dp_netdev_run_meter(pmd->dp, packets_, nl_attr_get_u32(a),
-pmd->ctx.now);
+dp_netdev_run_meter(pmd->dp, packets_, nl_attr_get_u32(a));
break;
case OVS_ACTION_ATTR_PUSH_VLAN:
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/2] debian: Run system testsuites as autopkgtest.
On Tue, Jun 20, 2023 at 6:09 PM Ilya Maximets wrote: > > On 6/20/23 10:08, Frode Nordahl wrote: > > The autopkgtests [0][1] are relevant in an upstream context > > because an Open vSwitch contributor may want to have a quick > > way of running the upstream system testsuites on recent > > Debian/Ubuntu releases in an automated and contained manner. > > > > During the Debian/Ubuntu/upstream package source sync work [2], a > > relatively naive autopkgtest was added. It had been around since > > Open vSwitch was initially packaged many years ago. > > > > Replace the autopkgtest with a test that runs all the upstream > > system testsuites instead. > > > > To run the autopkgtest, take a look at [1] for prerequisites then: > > > > ./boot.sh && \ > > ./configure \ > > --prefix=/usr \ > > --localstatedir=/var \ > > sysconfdir=/etc \ > > --with-dpdk=shared && \ > > make debian-source > > autopkgtest \ > > --env DEB_BUILD_OPTIONS="afxdp nocheck parallel=32" \ > > openvswitch_3.1.90-1.dsc \ > > -- qemu \ > > --cpus 32 \ > > --ram-size 8129 \ > > autopkgtest-mantic-amd64.img > > > > 0: https://wiki.debian.org/ContinuousIntegration/autopkgtest > > 1: https://packaging.ubuntu.com/html/auto-pkg-test.html > > 2: https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/396219.html > > > > Signed-off-by: Frode Nordahl > > --- > > .gitignore| 1 + > > debian/automake.mk| 31 ++- > > debian/rules | 5 ++ > > debian/tests/afxdp| 1 + > > debian/tests/control | 38 ++-- > > debian/tests/dpdk | 46 +- > > debian/tests/kernel | 1 + > > debian/tests/offloads | 1 + > > debian/tests/openflow.py | 66 -- > > debian/tests/run-tests.sh | 160 ++ > > debian/tests/system-userspace | 1 + > > debian/tests/vanilla | 29 -- > > 12 files changed, 230 insertions(+), 150 deletions(-) > > create mode 12 debian/tests/afxdp > > mode change 100755 => 12 debian/tests/dpdk > > create mode 12 debian/tests/kernel > > create mode 12 debian/tests/offloads > > delete mode 100755 debian/tests/openflow.py > > create mode 100755 debian/tests/run-tests.sh > > create mode 12 debian/tests/system-userspace > > delete mode 100755 debian/tests/vanilla > > > > diff --git a/.gitignore b/.gitignore > > index 26ed8d3d0..3d4f17b8a 100644 > > --- a/.gitignore > > +++ b/.gitignore > > @@ -75,6 +75,7 @@ OvsDpInterface.h > > testsuite.tmp.orig > > /rpm/ > > /openvswitch*.tar.gz > > +/openvswitch*.dsc > > /tests/lcov/ > > /Documentation/_build > > /.venv > > diff --git a/debian/automake.mk b/debian/automake.mk > > index 3195964c9..3e0a38edd 100644 > > --- a/debian/automake.mk > > +++ b/debian/automake.mk > > @@ -61,10 +61,13 @@ EXTRA_DIST += \ > > debian/rules \ > > debian/source/format \ > > debian/source/lintian-overrides \ > > + debian/tests/afxdp \ > > debian/tests/control \ > > debian/tests/dpdk \ > > - debian/tests/openflow.py \ > > - debian/tests/vanilla \ > > + debian/tests/kernel \ > > + debian/tests/offloads \ > > + debian/tests/run-tests.sh \ > > + debian/tests/system-userspace \ > > debian/watch > > > > check-debian-changelog-version: > > @@ -125,7 +128,6 @@ CLEANFILES += debian/control > > debian: debian/copyright debian/control > > .PHONY: debian > > > > - > > debian-deb: debian > > @if test X"$(srcdir)" != X"$(top_builddir)"; then > > \ > > echo "Debian packages should be built from $(abs_srcdir)/"; > > \ > > @@ -144,3 +146,26 @@ else > > $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \ > > fakeroot debian/rules binary > > endif > > + > > +debian-source: debian > > + @if test X"$(srcdir)" != X"$(top_builddir)"; then > > \ > > + echo "Debian packages should be built from $(abs_srcdir)/"; > > \ > > + exit 1; > > \ > > + fi > > + cp $(srcdir)/debian/control.in $(srcdir)/debian/control > > + $(update_deb_copyright) > > + $(update_deb_control_afxdp) > > + $(update_deb_control_dpdk) > > + $(AM_V_GEN) $(MAKE) distdir > > + cp $(srcdir)/debian/control $(srcdir)/debian/copyright \ > > + $(distdir)/debian/ > > + $(AM_V_GEN) tardir=$(distdir) && $(am__tar) | \ > > + eval GZIP= gzip $(GZIP_ENV) \ > > + -c >$(PACKAGE_NAME)_$(PACKAGE_VERSION).orig.tar.gz > > + cd $(distdir); \ > > + $(AM_V_GEN) dpkg-source --compression=gzip -b . > > + $(am__post_remove_distdir) > > + > > +DISTCLEANFILES += $(PACKAGE_NAME)_$(PACKAGE_VERSION)-1.debian.tar.gz \ > > +
Re: [ovs-dev] [PATCH v2 1/2] debian: Conditionally build package with AF_XDP.
On Tue, Jun 20, 2023 at 6:03 PM Ilya Maximets wrote: > > On 6/20/23 16:13, Frode Nordahl wrote: > > On Tue, Jun 20, 2023 at 3:44 PM Ilya Maximets wrote: > >> > >> On 6/20/23 10:08, Frode Nordahl wrote: > >>> Allow building the upstream Open vSwitch deb package with AF_XDP > >>> datapath enabled by passing in the string 'afxdp' in the > >>> DEB_BUILD_OPTIONS environment variable. > >> > >> Hi, Frode. Thanks for the patch! > > > > Thank you for taking the time to review, Ilya. Much appreciated! > > > >> Can we also update the debian-deb make target to pass this argument > >> whenever we HAVE_AF_XDP ? And maybe add a CI job for it? > >> Documentation/intro/install/debian.rst may also need some updates. > > > > Enabling AF_XDP whenever we HAVE_AF_XDP would be my preference, > > we have enabled the build by default in recent development releases of > > Debian/Ubuntu. I was just continuing the caution set out in the commit [0] > > that added the default build behavior to autoconf. > > We enabled AF_XDP support in Fedora by default [2], so I don't see a > reason to not do the same for Ubuntu/Debian whenever required versions > on libxdp/libbpf are available. > > [2] https://github.com/openvswitch/ovs/commit/9736b971b > > > > > We have had no reported issues so far for the builds where this has been > > enabled, and from the work done on this patch it appears the detection > > works good and produces buildable artifacts on systems without the required > > libraries. > > Good to know. > > > > > If that is what we want we could probably do without the > > extra DEB_BUILD_OPTION though, and produce a debian/control with the > > appropriate build-/runtime dependencies when we HAVE_AF_XDP and let > > autoconf/automake dtrt in the package build. > > > > What do you think? > > Sounds good to me. Maybe provide a 'noafxdp' option similar to > 'nodpdk' ? But I guess it's fine without it either. Yes, that sounds reasonable to me. > > I'll also take a look at docs and CI for this. Speaking of CI, the next commit in the series adds tests, so we could in essence just run those. I'll check if I can find a good way to run those on GHA. In light of that, the fact that not adding Depends was not caught was a bit disappointing. It is due to the need to include all the build dependencies to run the test, which incidentally make the packaged binaries run even when dependencies are not declared. I'll see if I either can remove the build deps after producing the testsuite or see if I can make the OVS build system build only the testsuite and not the rest of the tree. > > 0: https://github.com/openvswitch/ovs/commit/e44e8034318 > > > >> Some more comments below. > >> > >>> > >>> Signed-off-by: Frode Nordahl > >>> --- > >>> debian/automake.mk | 30 ++ > >>> debian/control.in | 2 ++ > >>> debian/rules | 8 +++- > >>> 3 files changed, 31 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/debian/automake.mk b/debian/automake.mk > >>> index 7b2afafae..3195964c9 100644 > >>> --- a/debian/automake.mk > >>> +++ b/debian/automake.mk > >>> @@ -95,17 +95,29 @@ CLEANFILES += debian/copyright > >>> > >>> > >>> if DPDK_NETDEV > >>> -update_deb_control = \ > >>> - $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \ > >>> - < $(srcdir)/debian/control.in > debian/control > >>> +update_deb_control_dpdk = \ > >>> + $(AM_V_GEN) sed -i 's/^\# DPDK_NETDEV //' \ > >>> + $(srcdir)/debian/control > >>> else > >>> -update_deb_control = \ > >>> - $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \ > >>> - < $(srcdir)/debian/control.in > debian/control > >>> +update_deb_control_dpdk = \ > >>> + $(AM_V_GEN) sed -i '/^\# DPDK_NETDEV /d' \ > >>> + $(srcdir)/debian/control > >>> +endif > >>> + > >>> +if HAVE_AF_XDP > >>> +update_deb_control_afxdp = \ > >>> + $(AM_V_GEN) sed -i 's/^\# HAVE_AF_XDP //' \ > >>> + $(srcdir)/debian/control > >>> +else > >>> +update_deb_control_afxdp = \ > >>> + $(AM_V_GEN) sed -i '/^\# HAVE_AF_XDP /d' \ > >>> + $(srcdir)/debian/control > >>> endif > >>> > >>> debian/control: $(srcdir)/debian/control.in Makefile > >>> - $(update_deb_control) > >>> + cp $(srcdir)/debian/control.in $(srcdir)/debian/control > >>> + $(update_deb_control_afxdp) > >>> + $(update_deb_control_dpdk) > >>> > >>> CLEANFILES += debian/control > >>> > >>> @@ -120,8 +132,10 @@ debian-deb: debian > >>> exit 1; > >>> \ > >>> fi > >>> $(MAKE) distclean > >>> + cp $(srcdir)/debian/control.in $(srcdir)/debian/control > >>> $(update_deb_copyright) > >>> - $(update_deb_control) > >>> + $(update_deb_control_afxdp) > >>> + $(update_deb_control_dpdk) > >>> $(AM_V_GEN) fakeroot debian/rules clean > >>> if DPDK_NETDEV > >>> $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \ > >>> diff --git
Re: [ovs-dev] [PATCH v13] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back
On 22 Jun 2023, at 11:15, Ilya Maximets wrote:
> On 6/22/23 10:50, Eelco Chaudron wrote:
>>
>>
>> On 30 May 2023, at 9:32, Eelco Chaudron wrote:
>>
>>> On 26 May 2023, at 22:51, Ilya Maximets wrote:
>>>
On 5/26/23 15:09, Eelco Chaudron wrote:
>
>
> On 26 May 2023, at 14:03, Balazs Nemeth wrote:
>
>> The only way that stats->{n_packets,n_bytes} would decrease is due to an
>> overflow, or if there are bugs in how statistics are handled. In the
>> past, there were multiple issues that caused a jump backward. A
>> workaround was in place to set the statistics to 0 in that case. When
>> this happened while the revalidator was under heavy load, the workaround
>> had an unintended side effect where should_revalidate returned false
>> causing the flow to be removed because the metric it calculated was
>> based on a bogus value. Since many of those bugs have now been
>> identified and resolved, there is no need to set the statistics to 0. In
>> addition, the (unlikely) overflow still needs to be handled
>> appropriately. If an unexpected jump does happen, just log it as a
>> warning.
>>
>> Signed-off-by: Balazs Nemeth
>
> Thanks for making the final change! Here is an example of the log message
> for others reviewing:
>
> 2023-05-26T12:55:07.590Z|3|ofproto_dpif_upcall(revalidator14)|WARN|Unexpected
> jump in packet stats from 0 to 1 when handling ukey
> ufid:90c082b0-7aa6-442a-9b86-04d10fe9ede6
> recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0)
> ,ct_mark(0),ct_label(0),eth(src=6e:48:c8:77:d3:8c,dst=33:33:00:00:00:16),eth_type(0x86dd),ipv6(src=::,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no),key32(40
> 00),icmpv6(type=143,code=0), actions:1,3
>
> One nit on a missing “,” compared to the dp flow dump, but I think Ilya
> can add this on commit.
>
> Acked-by: Eelco Chaudron
Thanks! I added the comma and applied the patch.
>>>
>>> Are you planning on back porting this, as all other relevant patches where?
>>
>> Ilya did you maybe miss this email? Question still applies ;)
>
> Yeah, sorry, it fell through the cracks.
>
> I'm a bit worried about backporting this one, because the
> original reason for the stats jump is still not clear and
> so might still exist. I guess, we could backport to 3.1,
> but I'd like not to backport to LTS.
>
> What do you think?
I’m happy with getting this in 3.1 ;)
Thanks,
Eelco
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v13] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back
On 6/22/23 10:50, Eelco Chaudron wrote:
>
>
> On 30 May 2023, at 9:32, Eelco Chaudron wrote:
>
>> On 26 May 2023, at 22:51, Ilya Maximets wrote:
>>
>>> On 5/26/23 15:09, Eelco Chaudron wrote:
On 26 May 2023, at 14:03, Balazs Nemeth wrote:
> The only way that stats->{n_packets,n_bytes} would decrease is due to an
> overflow, or if there are bugs in how statistics are handled. In the
> past, there were multiple issues that caused a jump backward. A
> workaround was in place to set the statistics to 0 in that case. When
> this happened while the revalidator was under heavy load, the workaround
> had an unintended side effect where should_revalidate returned false
> causing the flow to be removed because the metric it calculated was
> based on a bogus value. Since many of those bugs have now been
> identified and resolved, there is no need to set the statistics to 0. In
> addition, the (unlikely) overflow still needs to be handled
> appropriately. If an unexpected jump does happen, just log it as a
> warning.
>
> Signed-off-by: Balazs Nemeth
Thanks for making the final change! Here is an example of the log message
for others reviewing:
2023-05-26T12:55:07.590Z|3|ofproto_dpif_upcall(revalidator14)|WARN|Unexpected
jump in packet stats from 0 to 1 when handling ukey
ufid:90c082b0-7aa6-442a-9b86-04d10fe9ede6
recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0)
,ct_mark(0),ct_label(0),eth(src=6e:48:c8:77:d3:8c,dst=33:33:00:00:00:16),eth_type(0x86dd),ipv6(src=::,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no),key32(40
00),icmpv6(type=143,code=0), actions:1,3
One nit on a missing “,” compared to the dp flow dump, but I think Ilya
can add this on commit.
Acked-by: Eelco Chaudron
>>>
>>> Thanks! I added the comma and applied the patch.
>>
>> Are you planning on back porting this, as all other relevant patches where?
>
> Ilya did you maybe miss this email? Question still applies ;)
Yeah, sorry, it fell through the cracks.
I'm a bit worried about backporting this one, because the
original reason for the stats jump is still not clear and
so might still exist. I guess, we could backport to 3.1,
but I'd like not to backport to LTS.
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 v13] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back
On 30 May 2023, at 9:32, Eelco Chaudron wrote:
> On 26 May 2023, at 22:51, Ilya Maximets wrote:
>
>> On 5/26/23 15:09, Eelco Chaudron wrote:
>>>
>>>
>>> On 26 May 2023, at 14:03, Balazs Nemeth wrote:
>>>
The only way that stats->{n_packets,n_bytes} would decrease is due to an
overflow, or if there are bugs in how statistics are handled. In the
past, there were multiple issues that caused a jump backward. A
workaround was in place to set the statistics to 0 in that case. When
this happened while the revalidator was under heavy load, the workaround
had an unintended side effect where should_revalidate returned false
causing the flow to be removed because the metric it calculated was
based on a bogus value. Since many of those bugs have now been
identified and resolved, there is no need to set the statistics to 0. In
addition, the (unlikely) overflow still needs to be handled
appropriately. If an unexpected jump does happen, just log it as a
warning.
Signed-off-by: Balazs Nemeth
>>>
>>> Thanks for making the final change! Here is an example of the log message
>>> for others reviewing:
>>>
>>> 2023-05-26T12:55:07.590Z|3|ofproto_dpif_upcall(revalidator14)|WARN|Unexpected
>>> jump in packet stats from 0 to 1 when handling ukey
>>> ufid:90c082b0-7aa6-442a-9b86-04d10fe9ede6
>>> recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0)
>>> ,ct_mark(0),ct_label(0),eth(src=6e:48:c8:77:d3:8c,dst=33:33:00:00:00:16),eth_type(0x86dd),ipv6(src=::,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no),key32(40
>>> 00),icmpv6(type=143,code=0), actions:1,3
>>>
>>> One nit on a missing “,” compared to the dp flow dump, but I think Ilya can
>>> add this on commit.
>>>
>>> Acked-by: Eelco Chaudron
>>
>> Thanks! I added the comma and applied the patch.
>
> Are you planning on back porting this, as all other relevant patches where?
Ilya did you maybe miss this email? Question still applies ;)
> Cheers,
>
> Eelco
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Reduce size of the SB monitor condition.
On 6/21/23 16:50, Han Zhou wrote:
> On Wed, Jun 21, 2023 at 2:48 AM Dumitru Ceara wrote:
>>
>> We don't need to explicitly add port bindings that were already bound
>> locally. We implicitly get those because we monitor the datapaths
>> they're attached to.
>>
>> When performing an ovn-heater 500-node density-heavy scale test [0], with
>> conditional monitoring enabled, the unreasonably long poll intervals on
>> the Southbound database (the ones that took more than one second) are
>> measured as:
>>
>> -
>>Count MinMax Median Mean 95 percentile
>> -
>> 56.0 1010.0 2664.0 1455.5 1544.9 2163.0
>> 77.0 1015.0 3460.0 1896.0 1858.2 2907.8
>> 69.0 1010.0 3118.0 1618.0 1688.1 2582.4
>> -
>>202.0 1010.0 3460.0 1610.0 1713.3 2711.5
>>
>> Compared to the baseline results (also with conditional monitoring
>> enabled):
>> -
>>Count MinMax Median Mean 95 percentile
>> -
>>141.0 1003.018338.0 1721.0 2625.4 7626.0
>>151.0 1019.080297.0 1808.0 3410.7 9089.0
>>165.0 1007.050736.0 2354.0 3067.7 7309.8
>> -
>>457.0 1003.080297.0 2131.0 3044.6 7799.6
>>
>> We see significant improvement on the server side. This is explained
>> by the fact that now the Southbound database server doesn't need to
>> apply as many conditions as before when filtering individual monitor
>> contents.
>>
>> Note: Sub-ports - OVN switch ports with parent_port set - have to be
>> monitored unconditionally as we cannot efficiently determine their local
>> datapaths without including all local OVS interfaces in the monitor.
>>
>> [0]
> https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2139194
>> Reported-by: Ilya Maximets
>> Signed-off-by: Dumitru Ceara
>> ---
>> V2:
>> - Addressed Han's comments:
>> - changed commit log: removed assumption about sub-ports.
>> - added ovn-nb documentation note about sub-ports monitoring.
>> - added ovn-controller documentation note about sub-ports monitoring.
>> - added TODO item.
>> - Added NEWS item.
>> ---
>> NEWS| 3 +++
>> TODO.rst| 6 ++
>> controller/binding.c| 2 +-
>> controller/binding.h| 2 +-
>> controller/ovn-controller.8.xml | 6 ++
>> controller/ovn-controller.c | 19 ---
>> ovn-nb.xml | 6 +-
>> 7 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 482970eeeb..8275877f99 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -7,6 +7,9 @@ Post v23.06.0
>> DHCPv4 "hostname" (12) option.
>>- Support to create/update MAC_Binding when GARP received from VTEP
> (RAMP)
>> switch on l3dgw port.
>> + - To allow optimizing ovn-controller's monitor conditions for the
> regular
>> +VIF case, ovn-controller now unconditionally monitors all sub-ports
>> +(ports with parent_port set).
>>
>> OVN v23.06.0 - 01 Jun 2023
>> --
>> diff --git a/TODO.rst b/TODO.rst
>> index dff163e70b..b790a9fadf 100644
>> --- a/TODO.rst
>> +++ b/TODO.rst
>> @@ -124,3 +124,9 @@ OVN To-do List
>> * Load Balancer templates
>>
>>* Support combining the VIP IP and port into a single template
> variable.
>> +
>> +* ovn-controller conditional monitoring
>> +
>> + * Improve sub-ports (with parent_port set) conditional monitoring;
> these
>> +are currently unconditionally monitored, even if ovn-monitor-all is
>> +set to false.
>> diff --git a/controller/binding.c b/controller/binding.c
>> index 486095ca79..359ad6d660 100644
>> --- a/controller/binding.c
>> +++ b/controller/binding.c
>> @@ -785,7 +785,7 @@ local_binding_data_destroy(struct local_binding_data
> *lbinding_data)
>> }
>>
>> const struct sbrec_port_binding *
>> -local_binding_get_primary_pb(struct shash *local_bindings,
>> +local_binding_get_primary_pb(const struct shash *local_bindings,
>> const char *pb_name)
>> {
>> struct local_binding *lbinding =
>> diff --git a/controller/binding.h b/controller/binding.h
>> index 0e57f02ee5..319cbd263c 100644
>> --- a/controller/binding.h
>> +++ b/controller/binding.h
>> @@ -150,7 +150,7 @@ void local_binding_data_init(struct
> local_binding_data *);
>> void
