On 21 Feb 2023, at 12:15, Simon Horman wrote:
Thanks for the review Simon, see responses inline. Will send out a v4 soon. //Eelco > On Fri, Feb 03, 2023 at 12:12:12PM +0100, Eelco Chaudron wrote: >> When the ukey's action set changes, it could caus the flow to use a > > nit: s/caus/cause/ Will fix >> different datapath, for example, when it moves from tc to kernel. >> This will cause the the cached previous datapath statistics to be used. >> >> This change will reset the cached statistics when a change in >> datapath is discovered. >> >> Signed-off-by: Eelco Chaudron <echau...@redhat.com> > > ... > >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >> index 12477a24f..2795ccc81 100644 >> --- a/lib/dpif-provider.h >> +++ b/lib/dpif-provider.h >> @@ -338,6 +338,15 @@ struct dpif_class { >> int (*offload_stats_get)(struct dpif *dpif, >> struct netdev_custom_stats *stats); >> >> + /* This function will return true if the specific dpif implementation >> + * synchronizes the various datapath implementation layers, i.e., the >> + * dpif's layer in combination with the underlying netdev offload >> layers. >> + * For example, dpif-netlink does not sync its kernel flows with the tc >> + * ones, i.e., only one gets installed. On the other hand, dpif-netdev >> + * installs both flows, and internally keeps track of both, and >> represents > > nit: s/and internally/internally/ Will fix > >> + * them as one. */ >> + bool (*synced_dp_layers)(struct dpif *dpif); >> + > > Both implementations of this callback simply return a boolean, > without an logic. I don't know if it makes sense, but it occurs > to me that synced_dp_layers could be a bool rather than a function pointer. Changed to a bool as you suggested. > ... > >> diff --git a/lib/dpif.h b/lib/dpif.h >> index 6cb4dae6d..129cbf6a1 100644 >> --- a/lib/dpif.h >> +++ b/lib/dpif.h > > ... > >> @@ -54,11 +54,14 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall); >> COVERAGE_DEFINE(dumped_duplicate_flow); >> COVERAGE_DEFINE(dumped_new_flow); >> COVERAGE_DEFINE(handler_duplicate_upcall); >> -COVERAGE_DEFINE(upcall_ukey_contention); >> -COVERAGE_DEFINE(upcall_ukey_replace); >> COVERAGE_DEFINE(revalidate_missed_dp_flow); >> +COVERAGE_DEFINE(ukey_dp_change); >> +COVERAGE_DEFINE(ukey_invalid_stat_reset); >> COVERAGE_DEFINE(upcall_flow_limit_hit); >> COVERAGE_DEFINE(upcall_flow_limit_kill); >> +COVERAGE_DEFINE(upcall_ukey_contention); >> +COVERAGE_DEFINE(upcall_ukey_replace); >> + >> > > nit: now there are two blank lines here. Will fix >> /* A thread that reads upcalls from dpif, forwards each upcall's packet, >> * and possibly sets up a kernel flow as a cache. */ >> @@ -288,6 +291,7 @@ struct udpif_key { >> >> struct ovs_mutex mutex; /* Guards the following. */ >> 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. >> */ >> uint64_t dump_seq OVS_GUARDED; /* Tracks udpif->dump_seq. */ >> uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */ >> @@ -1771,6 +1775,7 @@ ukey_create__(const struct nlattr *key, size_t key_len, >> ukey->created = ukey->flow_time = time_msec(); >> memset(&ukey->stats, 0, sizeof ukey->stats); >> ukey->stats.used = used; >> + ukey->dp_layer = NULL; >> ukey->xcache = NULL; >> >> ukey->offloaded = false; >> @@ -2356,6 +2361,13 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key >> *ukey, >> ? stats->n_bytes - ukey->stats.n_bytes >> : 0); >> >> + if (stats->n_packets < ukey->stats.n_packets && >> + ukey->stats.n_packets < (UINT64_MAX / 4 * 3)) { > > Is this logic used elsewhere in the code-base? > Perhaps it warrants a helper. Added a MACRO for this… >> + /* Report cases where the packet counter is lower than the previous >> + * instance, but exclude the potential wrapping of an uint64_t. */ >> + COVERAGE_INC(ukey_invalid_stat_reset); >> + } >> + >> if (need_revalidate) { >> if (should_revalidate(udpif, push.n_packets, ukey->stats.used)) { >> if (!ukey->xcache) { >> @@ -2469,6 +2481,15 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, >> size_t n_ops) >> push->tcp_flags = stats->tcp_flags | op->ukey->stats.tcp_flags; >> push->n_packets = stats->n_packets - op->ukey->stats.n_packets; >> push->n_bytes = stats->n_bytes - op->ukey->stats.n_bytes; >> + >> + if (stats->n_packets < op->ukey->stats.n_packets && >> + op->ukey->stats.n_packets < (UINT64_MAX / 4 * 3)) { >> + /* Report cases where the packet counter is lower than the >> + * previous instance, but exclude the potential wrapping of >> an > > ... yes, here it is :) > >> + * uint64_t. */ >> + COVERAGE_INC(ukey_invalid_stat_reset); >> + } >> + >> ovs_mutex_unlock(&op->ukey->mutex); >> } else { >> push = stats; > > ... _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev