[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
