On 14/08/2024 11:55, Eelco Chaudron wrote:
>
>
> On 14 Aug 2024, at 10:49, Eelco Chaudron wrote:
>
>> It is observed in some environments that there are much more ukeys than
>> actual DP flows. For example:
>>
>> $ ovs-appctl upcall/show
>> system@ovs-system:
>> flows : (current 7) (avg 6) (max 117) (limit 2125)
>> offloaded flows : 525
>> dump duration : 1063ms
>> ufid enabled : true
>>
>> 23: (keys 3612)
>> 24: (keys 3625)
>> 25: (keys 3485)
>>
>> The revalidator threads are busy revalidating the stale ukeys leading to
>> high CPU and long dump duration.
>>
>> This patch records the last dump timestamp. If the flow was not dumped
>> for at least twice the idle time, we can assume the datapath flow now
>> longer exists and the ukey should be deleted.
>
> Roi can you make sure the test passes on your system? I did 1024+ runs on my
> system and it passes each time. Also on GitHub it works, but your machine
> might be different, as it failed their in the past.
>
> Thanks,
>
> Eelco
Hi,
Everything seems to be working fine. the test pass for me.
Thanks,
Roi
>
>> Reported-by: Roi Dayan <[email protected]>
>> Co-authored-by: Han Zhou <[email protected]>
>> Co-authored-by: Roi Dayan <[email protected]>
>> Signed-off-by: Han Zhou <[email protected]>
>> Signed-off-by: Roi Dayan <[email protected]>
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
>>
>> v3: Rewrote fix to use actual dump state, and added a tests case.
>> ---
>> ofproto/ofproto-dpif-upcall.c | 14 ++++-
>> tests/system-offloads-traffic.at | 64 ++++++++++++++++++++
>> utilities/usdt-scripts/flow_reval_monitor.py | 2 +
>> 3 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 4d39bc5a7..5c130b694 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -57,6 +57,7 @@ COVERAGE_DEFINE(dumped_inconsistent_flow);
>> COVERAGE_DEFINE(dumped_new_flow);
>> COVERAGE_DEFINE(handler_duplicate_upcall);
>> COVERAGE_DEFINE(revalidate_missed_dp_flow);
>> +COVERAGE_DEFINE(revalidate_missing_dp_flow);
>> COVERAGE_DEFINE(ukey_dp_change);
>> COVERAGE_DEFINE(ukey_invalid_stat_reset);
>> COVERAGE_DEFINE(ukey_replace_contention);
>> @@ -278,6 +279,7 @@ enum flow_del_reason {
>> FDR_BAD_ODP_FIT, /* Bad ODP flow fit. */
>> FDR_FLOW_IDLE, /* Flow idle timeout. */
>> FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
>> + FDR_FLOW_MISSING_DP, /* Flow is missing from the datapath. */
>> FDR_FLOW_WILDCARDED, /* Flow needs a narrower wildcard mask. */
>> FDR_NO_OFPROTO, /* Bridge not found. */
>> FDR_PURGE, /* User requested flow deletion. */
>> @@ -315,6 +317,7 @@ struct udpif_key {
>> struct dpif_flow_stats stats OVS_GUARDED; /* Last known stats.*/
>> const char *dp_layer OVS_GUARDED; /* Last known dp_layer. */
>> long long int created OVS_GUARDED; /* Estimate of creation time.
>> */
>> + long long int last_dumped OVS_GUARDED; /* Flow last dump time. */
>> uint64_t dump_seq OVS_GUARDED; /* Tracks udpif->dump_seq. */
>> uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
>> enum ukey_state state OVS_GUARDED; /* Tracks ukey lifetime. */
>> @@ -1825,6 +1828,7 @@ ukey_create__(const struct nlattr *key, size_t key_len,
>> ukey->state_thread = ovsthread_id_self();
>> ukey->state_where = OVS_SOURCE_LOCATOR;
>> ukey->created = ukey->flow_time = time_msec();
>> + ukey->last_dumped = 0;
>> memset(&ukey->stats, 0, sizeof ukey->stats);
>> ukey->stats.used = used;
>> ukey->dp_layer = NULL;
>> @@ -2456,7 +2460,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
>> *ukey,
>> log_unexpected_stats_jump(ukey, stats);
>> }
>>
>> - if (need_revalidate) {
>> + if ((ukey->last_dumped ? ukey->last_dumped : ukey->created)
>> + < udpif->dpif->current_ms - (2 * ofproto_max_idle)) {
>> + /* If the flow was not dumped for at least twice the idle time,
>> + * we can assume the datapath flow now longer exists and the ukey
>> + * should be deleted. */
>> + COVERAGE_INC(revalidate_missing_dp_flow);
>> + *del_reason = FDR_FLOW_MISSING_DP;
>> + } else if (need_revalidate) {
>> if (should_revalidate(udpif, ukey, push.n_packets)) {
>> if (!ukey->xcache) {
>> ukey->xcache = xlate_cache_new();
>> @@ -2890,6 +2901,7 @@ revalidate(struct revalidator *revalidator)
>> continue;
>> }
>>
>> + ukey->last_dumped = now;
>> ukey->offloaded = f->attrs.offloaded;
>> if (!ukey->dp_layer
>> || (!dpif_synced_dp_layers(udpif->dpif)
>> diff --git a/tests/system-offloads-traffic.at
>> b/tests/system-offloads-traffic.at
>> index d1da33d96..a9416cbce 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -93,6 +93,70 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded
>> flows : [[1-9]]"], [0], [i
>> OVS_TRAFFIC_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> +AT_SETUP([offloads - Forced removed datapath entries])
>> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch .
>> other_config:hw-offload=true])
>> +
>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>> +
>> +ADD_NAMESPACES(at_ns0, at_ns1)
>> +
>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>> +AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore])
>> +
>> +AT_CHECK([ovs-appctl vlog/set ofproto_dpif_upcall:dbg])
>> +
>> +dnl Set idle timeout to 1 second.
>> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:max-idle=1000])
>> +
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -W 2 10.1.1.2 | FORMAT_PING],
>> [0], [dnl
>> +10 packets transmitted, 10 received, 0% packet loss, time 0ms
>> +])
>> +
>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep
>> "eth_type(0x0800)" | DUMP_CLEAN_SORTED], [0], [dnl
>> +in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756,
>> used:0.001s, actions:output
>> +in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756,
>> used:0.001s, actions:output
>> +])
>> +
>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep -q "arp" ],
>> [0])
>> +
>> +dnl Delete IPv4 entries, but keep the ARP ones.
>> +AT_CHECK([tc filter del dev ovs-p0 ingress protocol ip pref 2])
>> +AT_CHECK([tc filter del dev ovs-p1 ingress protocol ip pref 2])
>> +
>> +dnl Bring the remote ports down to avoid traffic.
>> +AT_CHECK([ip -n at_ns0 link set p0 down])
>> +AT_CHECK([ip -n at_ns1 link set p1 down])
>> +
>> +dnl Wait until the ARP flow has timed out.
>> +OVS_WAIT_UNTIL([test `ovs-appctl dpctl/dump-flows type=tc,offloaded | \
>> + grep "arp" | wc -l` -eq 0])
>> +
>> +dnl Set max-idle to 10 ms, so we are sure the max-idle * 2 has been reached.
>> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:max-idle=10])
>> +
>> +dnl Add and delete a port to force the revalidation.
>> +for i in `seq 3`; do
>> + REV_RECONFIGURE=$(ovs-appctl coverage/read-counter rev_reconfigure)
>> + AT_CHECK([ovs-vsctl add-port br0 ovs-p2 -- \
>> + set interface ovs-p2 type=internal])
>> + AT_CHECK([ovs-vsctl del-port ovs-p2 ])
>> + OVS_WAIT_UNTIL([test `ovs-appctl coverage/read-counter rev_reconfigure` \
>> + -gt $REV_RECONFIGURE])
>> +done
>> +
>> +dnl Wait for another full round of revalidation. After this all ukeys
>> should
>> +dnl be gone.
>> +AT_CHECK([ovs-appctl revalidator/wait], [0])
>> +AT_CHECK([ovs-appctl revalidator/wait], [0])
>> +
>> +dnl Make sure no more ukeys exists.
>> +AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
>> + grep -qv '0)'], [1])
>> +
>> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to flow_del (No such file or
>> directory)/d"])
>> +AT_CLEANUP
>> +
>> AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst -
>> offloads disabled])
>> AT_KEYWORDS([ingress_policing])
>> OVS_CHECK_TC_QDISC()
>> diff --git a/utilities/usdt-scripts/flow_reval_monitor.py
>> b/utilities/usdt-scripts/flow_reval_monitor.py
>> index 28479a565..1de06c73b 100755
>> --- a/utilities/usdt-scripts/flow_reval_monitor.py
>> +++ b/utilities/usdt-scripts/flow_reval_monitor.py
>> @@ -249,6 +249,7 @@ FdrReasons = IntEnum(
>> "FDR_BAD_ODP_FIT",
>> "FDR_FLOW_IDLE",
>> "FDR_FLOW_LIMIT",
>> + "FDR_FLOW_MISSING_DP",
>> "FDR_FLOW_WILDCARDED",
>> "FDR_NO_OFPROTO",
>> "FDR_PURGE",
>> @@ -265,6 +266,7 @@ FdrReasonStrings = {
>> FdrReasons.FDR_BAD_ODP_FIT: "Bad ODP flow fit",
>> FdrReasons.FDR_FLOW_IDLE: "Flow idle timeout",
>> FdrReasons.FDR_FLOW_LIMIT: "Kill all flows condition reached",
>> + FdrReasons.FDR_FLOW_MISSING_DP: "Flow is missing from the datapath",
>> FdrReasons.FDR_FLOW_WILDCARDED: "Flow needs a narrower wildcard mask",
>> FdrReasons.FDR_NO_OFPROTO: "Bridge not found",
>> FdrReasons.FDR_PURGE: "User requested flow deletion",
>> --
>> 2.45.2
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
