On 03/01/2023 15:36, Eelco Chaudron wrote:
> Depending on the driver implementation it can take up to 2 seconds before
> offloaded flow statistics are updated. This is causing a problem with
> min-revalidate-pps, as old statistic values are used during this period.
>
> This fix will wait for at least 2 seconds before assuming no packets
> where received during this period.
>
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
>
> Changes:
> - v2: Use existing ukey->offloaded so the revalidate_missed_dp_flow case is
> also covered.
> - v3: Added OVS_REQUIRES(ukey->mutex) to should_revalidate() to keep
> thread-safety-analysis happy.
>
> ofproto/ofproto-dpif-upcall.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index ad9635496..be3c894b4 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2094,10 +2094,12 @@ ukey_delete(struct umap *umap, struct udpif_key *ukey)
> }
>
> static bool
> -should_revalidate(const struct udpif *udpif, uint64_t packets,
> - long long int used)
> +should_revalidate(const struct udpif *udpif, struct udpif_key *ukey,
> + uint64_t packets)
> + OVS_REQUIRES(ukey->mutex)
> {
> long long int metric, now, duration;
> + long long int used = ukey->stats.used;
>
> if (!used) {
> /* Always revalidate the first time a flow is dumped. */
> @@ -2124,8 +2126,12 @@ should_revalidate(const struct udpif *udpif, uint64_t
> packets,
> duration = now - used;
> metric = duration / packets;
>
> - if (metric < 1000 / ofproto_min_revalidate_pps) {
> - /* The flow is receiving more than min-revalidate-pps, so keep it. */
> + if (metric < 1000 / ofproto_min_revalidate_pps ||
> + (ukey->offloaded && duration < 2000)) {
> + /* The flow is receiving more than min-revalidate-pps, so keep it.
> + * Or it's a hardware offloaded flow that might take up to 2 seconds
> + * to update its statistics. Until we are sure the statistics had a
> + * chance to be updated, also keep it. */
> return true;
> }
> return false;
> @@ -2323,7 +2329,7 @@ static enum reval_result
> revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
> const struct dpif_flow_stats *stats,
> struct ofpbuf *odp_actions, uint64_t reval_seq,
> - struct recirc_refs *recircs, bool offloaded)
> + struct recirc_refs *recircs)
> OVS_REQUIRES(ukey->mutex)
> {
> bool need_revalidate = ukey->reval_seq != reval_seq;
> @@ -2342,7 +2348,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> : 0);
>
> if (need_revalidate) {
> - if (should_revalidate(udpif, push.n_packets, ukey->stats.used)) {
> + if (should_revalidate(udpif, ukey, push.n_packets)) {
> if (!ukey->xcache) {
> ukey->xcache = xlate_cache_new();
> } else {
> @@ -2358,7 +2364,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
>
> /* Stats for deleted flows will be attributed upon flow deletion. Skip.
> */
> if (result != UKEY_DELETE) {
> - xlate_push_stats(ukey->xcache, &push, offloaded);
> + xlate_push_stats(ukey->xcache, &push, ukey->offloaded);
> ukey->stats = *stats;
> ukey->reval_seq = reval_seq;
> }
> @@ -2758,6 +2764,7 @@ revalidate(struct revalidator *revalidator)
> continue;
> }
>
> + ukey->offloaded = f->attrs.offloaded;
> already_dumped = ukey->dump_seq == dump_seq;
> if (already_dumped) {
> /* The flow has already been handled during this flow dump
> @@ -2789,8 +2796,7 @@ revalidate(struct revalidator *revalidator)
> result = UKEY_DELETE;
> } else {
> result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
> - reval_seq, &recircs,
> - f->attrs.offloaded);
> + reval_seq, &recircs);
> }
> ukey->dump_seq = dump_seq;
>
> @@ -2875,7 +2881,7 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> COVERAGE_INC(revalidate_missed_dp_flow);
> memset(&stats, 0, sizeof stats);
> result = revalidate_ukey(udpif, ukey, &stats,
> &odp_actions,
> - reval_seq, &recircs, false);
> + reval_seq, &recircs);
> }
> if (result != UKEY_KEEP) {
> /* Clears 'recircs' if filled by revalidate_ukey(). */
>
Acked-by: Roi Dayan <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
