On 1/3/23 18:13, Eelco Chaudron wrote: > When the ukey's action set changes, it could caus the flow to use a > 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 <[email protected]> > --- > ofproto/ofproto-dpif-upcall.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index ad9635496..feaf58575 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -53,11 +53,13 @@ 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(upcall_flow_limit_hit); > COVERAGE_DEFINE(upcall_flow_limit_kill); > +COVERAGE_DEFINE(upcall_ukey_contention); > +COVERAGE_DEFINE(upcall_ukey_replace); > + > > /* A thread that reads upcalls from dpif, forwards each upcall's packet, > * and possibly sets up a kernel flow as a cache. */ > @@ -287,6 +289,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. */ > @@ -1766,6 +1769,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 = "ovs"; > ukey->xcache = NULL; > > ukey->offloaded = false; > @@ -2758,6 +2762,16 @@ revalidate(struct revalidator *revalidator) > continue; > } > > + if (strcmp(ukey->dp_layer, f->attrs.dp_layer)) { > + /* The dp_layer has changed this is probably due to an > earlier > + * revalidate cycle moving it to/from hw offload. In this > case > + * we should reset the ukey stored statistics, as they are > + * from the deleted DP flow. */ > + COVERAGE_INC(ukey_dp_change); > + ukey->dp_layer = f->attrs.dp_layer; > + memset(&ukey->stats, 0, sizeof ukey->stats);
Wouldn't this mess up OpenFlow stats down the line? Especially if we accept a patch from Balazs that reverts an overflow workaround. Also, I would guess that this will mess up stats for netdev-offload-dpdk, because dpif-netdev hides all the stats shenanigans from the upper layers while correctly exposing the dp_layer. It's doing that by summing the userspace datapath stats for that flow with offloaded ones. See the function get_dpif_flow_status() for details. I'd say, dpif-netlink should take care of correct stats management instead. This will probably mean tracking stats delta in both dpif-netlink and netdev-offload-tc. Note that netdev-offload-tc should still be able to report correct stats to dpif-netdev as well. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
