On 21 Feb 2023, at 11:47, Simon Horman wrote:
> On Tue, Feb 14, 2023 at 02:39:25PM +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. >> >> Signed-off-by: Eelco Chaudron <[email protected]> > > Some minor nits inline, but this looks good to me. > > Reviewed-by: Simon Horman <[email protected]> Thanks for the review! I’ve fixed the nits and will send out a v5, keeping your reviewed-by. Cheers, Eelco > >> 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. >> --- >> ofproto/ofproto-dpif-upcall.c | 26 ++++++++++++++++---------- > > nit: there was some fuzz in ofproto-dpif-upcall. when applying this patch. > >> 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 db7570ee2..24ac6806c 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, struct udpif_key *ukey, >> + uint64_t packets) > > nit: can ukey be const? > >> + OVS_REQUIRES(ukey->mutex) >> { >> long long int metric, now, duration; >> + long long int used = ukey->stats.used; >> >> if (!ofproto_min_revalidate_pps) { >> return true; > > ... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
