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 <[email protected]>
>> 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.
>> - 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);
>> 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(). */
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index a84ddc1d0..71341a01f 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -541,6 +541,10 @@ extern unsigned ofproto_max_revalidator;
>> * duration exceeds half of max-revalidator config variable. */
>> extern unsigned ofproto_min_revalidate_pps;
>>
>> +/* Worst case delay (in ms) it might take before statistics of offloaded
>> flows
>> + * are updated. This is used when calculating the min revalidate pps. */
>
> This comment is confusing, because 'min revalidate pps' is a configuration
> option and we do not calculate it. We should say 'Offloaded flows younger
> than this delay will always be revalidated regardless of
> ofproto_min_revalidate_pps'.
> Or something along these lines.
Thanks will use your phrase and fix the below occurrences as well.
Will send out a v6 soon.
//Eelco
>> +extern unsigned ofproto_offloaded_stats_delay;
>> +
>> /* Number of upcall handler and revalidator threads. Only affects the
>> * ofproto-dpif implementation. */
>> extern uint32_t n_handlers, n_revalidators;
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 863b34d25..69fece4c5 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -311,6 +311,7 @@ unsigned ofproto_flow_limit = OFPROTO_FLOW_LIMIT_DEFAULT;
>> unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT;
>> unsigned ofproto_max_revalidator = OFPROTO_MAX_REVALIDATOR_DEFAULT;
>> unsigned ofproto_min_revalidate_pps = OFPROTO_MIN_REVALIDATE_PPS_DEFAULT;
>> +unsigned ofproto_offloaded_stats_delay = OFPROTO_OFFLOADED_STATS_DELAY;
>>
>> uint32_t n_handlers, n_revalidators;
>>
>> @@ -727,6 +728,14 @@ ofproto_set_min_revalidate_pps(unsigned
>> min_revalidate_pps)
>> ofproto_min_revalidate_pps = min_revalidate_pps;
>> }
>>
>> +/* Set worst case delay (in ms) it might take before statistics of offloaded
>> + * flows are updated. This is used when calculating the min revalidate pps.
>> */
>
> Same here.
>
>> +void
>> +ofproto_set_offloaded_stats_delay(unsigned offloaded_stats_delay)
>> +{
>> + ofproto_offloaded_stats_delay = offloaded_stats_delay;
>> +}
>> +
>> /* If forward_bpdu is true, the NORMAL action will forward frames with
>> * reserved (e.g. STP) destination Ethernet addresses. if forward_bpdu is
>> false,
>> * the NORMAL action will drop these frames. */
>> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
>> index 4e15167ab..fa7973ac7 100644
>> --- a/ofproto/ofproto.h
>> +++ b/ofproto/ofproto.h
>> @@ -311,6 +311,7 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
>> #define OFPROTO_MAX_IDLE_DEFAULT 10000 /* ms */
>> #define OFPROTO_MAX_REVALIDATOR_DEFAULT 500 /* ms */
>> #define OFPROTO_MIN_REVALIDATE_PPS_DEFAULT 5
>> +#define OFPROTO_OFFLOADED_STATS_DELAY 2000 /* ms */
>>
>> const char *ofproto_port_open_type(const struct ofproto *,
>> const char *port_type);
>> @@ -340,6 +341,7 @@ void ofproto_set_flow_limit(unsigned limit);
>> void ofproto_set_max_idle(unsigned max_idle);
>> void ofproto_set_max_revalidator(unsigned max_revalidator);
>> void ofproto_set_min_revalidate_pps(unsigned min_revalidate_pps);
>> +void ofproto_set_offloaded_stats_delay(unsigned offloaded_stats_delay);
>> void ofproto_set_forward_bpdu(struct ofproto *, bool forward_bpdu);
>> void ofproto_set_mac_table_config(struct ofproto *, unsigned idle_time,
>> size_t max_entries);
>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>> index abf2afe57..c35c16c8d 100644
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -832,6 +832,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
>> *ovs_cfg)
>> ofproto_set_min_revalidate_pps(
>> smap_get_uint(&ovs_cfg->other_config, "min-revalidate-pps",
>> OFPROTO_MIN_REVALIDATE_PPS_DEFAULT));
>> + ofproto_set_offloaded_stats_delay(
>> + smap_get_uint(&ovs_cfg->other_config, "offloaded-stats-delay",
>> + OFPROTO_OFFLOADED_STATS_DELAY));
>> ofproto_set_vlan_limit(smap_get_int(&ovs_cfg->other_config,
>> "vlan-limit",
>> LEGACY_MAX_VLAN_HEADERS));
>> ofproto_set_bundle_idle_timeout(smap_get_uint(&ovs_cfg->other_config,
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 05ac1fbe5..fde19838d 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -216,6 +216,18 @@
>> </p>
>> </column>
>>
>> + <column name="other_config" key="offloaded-stats-delay"
>> + type='{"type": "integer", "minInteger": 0}'>
>> + <p>
>> + Set worst case delay (in ms) it might take before statistics of
>> + offloaded flows are updated. This is used when calculating the
>> + min-revalidate-pps.
>
> Same here.
>
> Nit: Please use xml markup to refer options. i.e.:
> <ref column="other_config" key="min-revalidate-pps"/>
>
> Would still be great if we didn't need this patch at all,
> but I guess we stuck with it for now.
>
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
