| Message ID | 167292339956.820938.1778910291937768406.stgit@ebuild |
|---|---|
| State | Accepted |
| Commit | 6c24851f433a13e99fc48a0cd8a0e90ab873f901 |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-upcall: Use last known stats ukey stats on revalidate missed dp flows. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 1/5/23 08:56, Eelco Chaudron wrote: > Instead of using all zero stats when executing a revalidate for missed > dp flows, use the last known stats to avoid odd statistics being used. > > As these zero stats are stored in the ukey, the next time revalidate_ukey() > is called the delta between the new stats and the zero stats is used, which > would cause an additional increase in total packets/bytes. Okay, this makes a lot more sense. If we set it to 0 then it would be as if all the stats were suddenly dropped to 0 which is not the case. Using the the last known value makes more sense. > > In addition if the 'ofproto-dpif-upcall: Don't set statistics to 0 when > they jump back' patch get's applied, the packet count will become > even more exotic, as 0 - 'a few packets' results in a huge value > for stats to be pushed. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Acked-by: Michael Santana <msantana@redhat.com> > --- > ofproto/ofproto-dpif-upcall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index be3c894b4..886ba54b8 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2879,7 +2879,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) > } else { > struct dpif_flow_stats stats; > COVERAGE_INC(revalidate_missed_dp_flow); > - memset(&stats, 0, sizeof stats); > + memcpy(&stats, &ukey->stats, sizeof stats); > result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, > reval_seq, &recircs); > } >
On 1/11/23 19:36, Michael Santana wrote: > > > On 1/5/23 08:56, Eelco Chaudron wrote: >> Instead of using all zero stats when executing a revalidate for missed >> dp flows, use the last known stats to avoid odd statistics being used. >> >> As these zero stats are stored in the ukey, the next time revalidate_ukey() >> is called the delta between the new stats and the zero stats is used, which >> would cause an additional increase in total packets/bytes. > Okay, this makes a lot more sense. If we set it to 0 then it would be as if all the stats were suddenly dropped to 0 which is not the case. Using the the last known value makes more sense. >> >> In addition if the 'ofproto-dpif-upcall: Don't set statistics to 0 when >> they jump back' patch get's applied, the packet count will become >> even more exotic, as 0 - 'a few packets' results in a huge value >> for stats to be pushed. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > Acked-by: Michael Santana <msantana@redhat.com> >> --- >> ofproto/ofproto-dpif-upcall.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index be3c894b4..886ba54b8 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -2879,7 +2879,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) >> } else { >> struct dpif_flow_stats stats; >> COVERAGE_INC(revalidate_missed_dp_flow); >> - memset(&stats, 0, sizeof stats); >> + memcpy(&stats, &ukey->stats, sizeof stats); >> result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, >> reval_seq, &recircs); >> } >> I don't know which branch this patch is based on, but it's certainly not master or anything down to 2.17. And I'm not sure how our CI was able to apply it... Anyway, I rebased it manually and applied. Backported down to 2.17. Thanks! Best regards, Ilya Maximets.
On 15 Feb 2023, at 21:04, Ilya Maximets wrote: > On 1/11/23 19:36, Michael Santana wrote: >> >> >> On 1/5/23 08:56, Eelco Chaudron wrote: >>> Instead of using all zero stats when executing a revalidate for missed >>> dp flows, use the last known stats to avoid odd statistics being used. >>> >>> As these zero stats are stored in the ukey, the next time revalidate_ukey() >>> is called the delta between the new stats and the zero stats is used, which >>> would cause an additional increase in total packets/bytes. >> Okay, this makes a lot more sense. If we set it to 0 then it would be as if all the stats were suddenly dropped to 0 which is not the case. Using the the last known value makes more sense. >>> >>> In addition if the 'ofproto-dpif-upcall: Don't set statistics to 0 when >>> they jump back' patch get's applied, the packet count will become >>> even more exotic, as 0 - 'a few packets' results in a huge value >>> for stats to be pushed. >>> >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> Acked-by: Michael Santana <msantana@redhat.com> >>> --- >>> ofproto/ofproto-dpif-upcall.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >>> index be3c894b4..886ba54b8 100644 >>> --- a/ofproto/ofproto-dpif-upcall.c >>> +++ b/ofproto/ofproto-dpif-upcall.c >>> @@ -2879,7 +2879,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) >>> } else { >>> struct dpif_flow_stats stats; >>> COVERAGE_INC(revalidate_missed_dp_flow); >>> - memset(&stats, 0, sizeof stats); >>> + memcpy(&stats, &ukey->stats, sizeof stats); >>> result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, >>> reval_seq, &recircs); >>> } >>> > > I don't know which branch this patch is based on, but it's > certainly not master or anything down to 2.17. And I'm not > sure how our CI was able to apply it... > > Anyway, I rebased it manually and applied. > Backported down to 2.17. Thanks, yes I might have based this on top of some of the other revalidator fixes I was working on :( > Thanks! > > Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index be3c894b4..886ba54b8 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2879,7 +2879,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) } else { struct dpif_flow_stats stats; COVERAGE_INC(revalidate_missed_dp_flow); - memset(&stats, 0, sizeof stats); + memcpy(&stats, &ukey->stats, sizeof stats); result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, reval_seq, &recircs); }
Instead of using all zero stats when executing a revalidate for missed dp flows, use the last known stats to avoid odd statistics being used. As these zero stats are stored in the ukey, the next time revalidate_ukey() is called the delta between the new stats and the zero stats is used, which would cause an additional increase in total packets/bytes. In addition if the 'ofproto-dpif-upcall: Don't set statistics to 0 when they jump back' patch get's applied, the packet count will become even more exotic, as 0 - 'a few packets' results in a huge value for stats to be pushed. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- ofproto/ofproto-dpif-upcall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)