[ovs-dev] [PATCH v5] ofproto-dpif-upcall: Wait for valid hw flow stats before applying min-revalidate-pps
Depending on the driver implementation, it can take from 0.2 seconds up to 2 seconds before offloaded flow statistics are updated. This is true for both TC and rte_flow-based offloading. 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, by default, before assuming no packets where received during this period. Reviewed-by: Simon Horman Signed-off-by: Eelco Chaudron --- 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. - v4: Add a configurable option. After looking at multiple vendor implementation for both TC and rte_flow I came to the conclusion that the delay is roughly between 0.2 and 2 seconds. Updated commit message. - v5: Rebased to latest upstream master. Made the key parameter const in should_revalidate(). ofproto/ofproto-dpif-upcall.c | 26 -- ofproto/ofproto-provider.h|4 ofproto/ofproto.c |9 + ofproto/ofproto.h |2 ++ vswitchd/bridge.c |3 +++ vswitchd/vswitch.xml | 12 6 files changed, 46 insertions(+), 10 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index fc94078cb..60c273a1d 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2100,10 +2100,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, const 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 (!ofproto_min_revalidate_pps) { return true; @@ -2134,8 +2136,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 < ofproto_offloaded_stats_delay)) { +/* The flow is receiving more than min-revalidate-pps, so keep it. + * Or it's a hardware offloaded flow that might take up to X seconds + * to update its statistics. Until we are sure the statistics had a + * chance to be updated, also keep it. */ return true; } return false; @@ -2339,7 +2345,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; @@ -2358,7 +2364,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 { @@ -2374,7 +2380,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; } @@ -2774,6 +2780,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 @@ -2805,8 +2812,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; @@ -2891,7 +2897,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) COVERAGE_INC(revalidate_missed_dp_flow); memcpy(&stats, &ukey->stats, sizeof stats);
Re: [ovs-dev] [PATCH v5] ofproto-dpif-upcall: Wait for valid hw flow stats before applying min-revalidate-pps
On Mon, Feb 27, 2023 at 03:58:04PM +0100, Eelco Chaudron wrote: > Depending on the driver implementation, it can take from 0.2 seconds > up to 2 seconds before offloaded flow statistics are updated. This is > true for both TC and rte_flow-based offloading. 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, by default, before assuming no > packets where received during this period. > > Reviewed-by: Simon Horman > Signed-off-by: Eelco Chaudron > --- > > 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. > - v4: Add a configurable option. > After looking at multiple vendor implementation for both TC and > rte_flow I came to the conclusion that the delay is roughly between > 0.2 and 2 seconds. Updated commit message. > - v5: Rebased to latest upstream master. > Made the key parameter const in should_revalidate(). Thanks, LGTM. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] ofproto-dpif-upcall: Wait for valid hw flow stats before applying min-revalidate-pps
On 2/27/23 15:58, Eelco Chaudron wrote: > Depending on the driver implementation, it can take from 0.2 seconds > up to 2 seconds before offloaded flow statistics are updated. This is > true for both TC and rte_flow-based offloading. 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, by default, before assuming no > packets where received during this period. > > Reviewed-by: Simon Horman > Signed-off-by: Eelco Chaudron > --- > > 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. > - v4: Add a configurable option. > After looking at multiple vendor implementation for both TC and > rte_flow I came to the conclusion that the delay is roughly between > 0.2 and 2 seconds. Updated commit message. > - v5: Rebased to latest upstream master. > Made the key parameter const in should_revalidate(). > > ofproto/ofproto-dpif-upcall.c | 26 -- > ofproto/ofproto-provider.h|4 > ofproto/ofproto.c |9 + > ofproto/ofproto.h |2 ++ > vswitchd/bridge.c |3 +++ > vswitchd/vswitch.xml | 12 > 6 files changed, 46 insertions(+), 10 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index fc94078cb..60c273a1d 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2100,10 +2100,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, const 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 (!ofproto_min_revalidate_pps) { > return true; > @@ -2134,8 +2136,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 < ofproto_offloaded_stats_delay)) { > +/* The flow is receiving more than min-revalidate-pps, so keep it. > + * Or it's a hardware offloaded flow that might take up to X seconds > + * to update its statistics. Until we are sure the statistics had a > + * chance to be updated, also keep it. */ > return true; > } > return false; > @@ -2339,7 +2345,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; > @@ -2358,7 +2364,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 { > @@ -2374,7 +2380,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; > } > @@ -2774,6 +2780,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 > @@ -2805,8 +2812,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); > } >
Re: [ovs-dev] [PATCH v5] ofproto-dpif-upcall: Wait for valid hw flow stats before applying min-revalidate-pps
On 3 Mar 2023, at 22:20, Ilya Maximets wrote: > On 2/27/23 15:58, Eelco Chaudron wrote: >> Depending on the driver implementation, it can take from 0.2 seconds >> up to 2 seconds before offloaded flow statistics are updated. This is >> true for both TC and rte_flow-based offloading. 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, by default, before assuming no >> packets where received during this period. >> >> Reviewed-by: Simon Horman >> Signed-off-by: Eelco Chaudron >> --- >> >> 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. >> - v4: Add a configurable option. >> After looking at multiple vendor implementation for both TC and >> rte_flow I came to the conclusion that the delay is roughly between >> 0.2 and 2 seconds. Updated commit message. >> - v5: Rebased to latest upstream master. >> Made the key parameter const in should_revalidate(). >> >> ofproto/ofproto-dpif-upcall.c | 26 -- >> ofproto/ofproto-provider.h|4 >> ofproto/ofproto.c |9 + >> ofproto/ofproto.h |2 ++ >> vswitchd/bridge.c |3 +++ >> vswitchd/vswitch.xml | 12 >> 6 files changed, 46 insertions(+), 10 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index fc94078cb..60c273a1d 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -2100,10 +2100,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, const 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 (!ofproto_min_revalidate_pps) { >> return true; >> @@ -2134,8 +2136,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 < ofproto_offloaded_stats_delay)) { >> +/* The flow is receiving more than min-revalidate-pps, so keep it. >> + * Or it's a hardware offloaded flow that might take up to X seconds >> + * to update its statistics. Until we are sure the statistics had a >> + * chance to be updated, also keep it. */ >> return true; >> } >> return false; >> @@ -2339,7 +2345,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; >> @@ -2358,7 +2364,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 { >> @@ -2374,7 +2380,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; >> } >> @@ -2774,6 +2780,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 >> @@ -2805,8 +2812,7 @@ revalidate(struct revalidator *revalidator) >> result = UKEY_DELETE; >> } else { >> result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, >> - reval_seq, &recir