| Message ID | 1735610174-37467-2-git-send-email-fuqiang.wng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | some optimize in dirtylimit_throttle_pct | expand |
On Tue, Dec 31, 2024 at 9:56 AM fuqiang wang <fuqiang.wng@gmail.com> wrote: > The current dirtylimit_throttle_pct trace event is triggered when the > throttle time is adjusted linearly. Modify the trace event so that it > can record non-linear adjustments. Please target the optimization mentioned above in a single patch. . > Additionally, since the throttle time > might be adjusted again at the end of the dirtylimit_set_throttle > function, move the trace event to after this process and calculate the > final adjustment time and sleep percent. > If need, I'd advise dividing this into a different patch. > > This patch can fix the following issue: > > 1. The current dirty rate at 1000MB/s and the dirty limit value at > 10000MB/s, before merge this patch, this trace event will print: > > CPU[2] throttle percent: 98, throttle adjust time 191590 us > CPU[2] throttle percent: 98, throttle adjust time 191002 us > CPU[2] throttle percent: 98, throttle adjust time 191002 us > > After merge this patch, there will be no print. > > 2. The current dirty rate is 1000MB/s and the dirty limit rate value is > 333MB/s, before merge this patch, this trace event will print: > > CPU[3] throttle percent: 98, throttle adjust time 32666634 us > > It will only print linear adjustment, and the adjust time > will be larger and only have positive values. > > After merge this patch, print as following: > CPU[2] throttle percent: 97, throttle adjust time 128766 us > CPU[2] throttle percent: 94, throttle adjust time -61131 us > CPU[2] throttle percent: 92, throttle adjust time -16634 us > ... > CPU[2] throttle percent: 74, throttle adjust time -390 us > CPU[2] throttle percent: 73, throttle adjust time -390 us > > Signed-off-by: wangfuqiang49 <wangfuqiang49@jd.com> > --- > system/dirtylimit.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/system/dirtylimit.c b/system/dirtylimit.c > index 7c071248bb..c7f663e5b9 100644 > --- a/system/dirtylimit.c > +++ b/system/dirtylimit.c > @@ -281,31 +281,30 @@ static void dirtylimit_set_throttle(CPUState *cpu, > { > int64_t ring_full_time_us = 0; > uint64_t sleep_pct = 0; > + uint64_t throttle_pct = 0; > uint64_t throttle_us = 0; > + int64_t throtlle_us_old = cpu->throttle_us_per_full; > > if (current == 0) { > cpu->throttle_us_per_full = 0; > - return; > + goto end; > The current dirty rate is zero, indicating nothing needs to be done, including displaying the trace event. return directly seems justified. > } > > ring_full_time_us = dirtylimit_dirty_ring_full_time(current); > > if (dirtylimit_need_linear_adjustment(quota, current)) { > if (quota < current) { > - sleep_pct = (current - quota) * 100 / current; > s/sleep_pct/throttle_pci/ , why ? > + throttle_pct = (current - quota) * 100 / current; > throttle_us = > - ring_full_time_us * sleep_pct / (double)(100 - sleep_pct); > + ring_full_time_us * throttle_pct / (double)(100 - > throttle_pct); > cpu->throttle_us_per_full += throttle_us; > } else { > - sleep_pct = (quota - current) * 100 / quota; > + throttle_pct = (quota - current) * 100 / quota; > throttle_us = > - ring_full_time_us * sleep_pct / (double)(100 - sleep_pct); > + ring_full_time_us * throttle_pct / (double)(100 - > throttle_pct); > cpu->throttle_us_per_full -= throttle_us; > } > > - trace_dirtylimit_throttle_pct(cpu->cpu_index, > - sleep_pct, > - throttle_us); > } else { > if (quota < current) { > cpu->throttle_us_per_full += ring_full_time_us / 10; > @@ -323,6 +322,19 @@ static void dirtylimit_set_throttle(CPUState *cpu, > ring_full_time_us * DIRTYLIMIT_THROTTLE_PCT_MAX); > > cpu->throttle_us_per_full = MAX(cpu->throttle_us_per_full, 0); > + > +end: > + if (cpu->throttle_us_per_full - throtlle_us_old) { > + if (current) { > + sleep_pct = ring_full_time_us * 100 / (ring_full_time_us + > + cpu->throttle_us_per_full); > + } else { > + sleep_pct = 0; > + } > + trace_dirtylimit_throttle_pct(cpu->cpu_index, > + sleep_pct, > + cpu->throttle_us_per_full - > + throtlle_us_old); } > This changes the interface of the trace event, We can keep the throttle_us and add the delta meanwhile: trace_dirtylimit_throttle_pct(cpu->cpu_index, sleep_pct, throttle_us, cpu->throttle_us_per_full - throtlle_us_old) } > > static void dirtylimit_adjust_throttle(CPUState *cpu) > -- > 2.47.0 > >
On 2025/1/6 21:38, fuqiang wang wrote: I'm sorry for getting back to you after a few days. > 在 2025/1/3 00:40, Yong Huang 写道: > > > > > > On Tue, Dec 31, 2024 at 9:56 AM fuqiang wang <fuqiang.wng@gmail.com> wrote: > > > > The current dirtylimit_throttle_pct trace event is triggered when the > > throttle time is adjusted linearly. Modify the trace event so that it > > can record non-linear adjustments. > > > > > > Please target the optimization mentioned above in a single patch. > > . Thank you for your suggestions. I will do it in next version. > > > > Additionally, since the throttle time > > might be adjusted again at the end of the dirtylimit_set_throttle > > function, move the trace event to after this process and calculate the > > final adjustment time and sleep percent. > > > > > > If need, I'd advise dividing this into a different patch. > > Thanks. I will do it. > > > > This patch can fix the following issue: > > > > 1. The current dirty rate at 1000MB/s and the dirty limit value at > > 10000MB/s, before merge this patch, this trace event will print: > > > > CPU[2] throttle percent: 98, throttle adjust time 191590 us > > CPU[2] throttle percent: 98, throttle adjust time 191002 us > > CPU[2] throttle percent: 98, throttle adjust time 191002 us > > > > After merge this patch, there will be no print. > > > > 2. The current dirty rate is 1000MB/s and the dirty limit rate value is > > 333MB/s, before merge this patch, this trace event will print: > > > > CPU[3] throttle percent: 98, throttle adjust time 32666634 us > > > > It will only print linear adjustment, and the adjust time > > will be larger and only have positive values. > > > > After merge this patch, print as following: > > CPU[2] throttle percent: 97, throttle adjust time 128766 us > > CPU[2] throttle percent: 94, throttle adjust time -61131 us > > CPU[2] throttle percent: 92, throttle adjust time -16634 us > > ... > > CPU[2] throttle percent: 74, throttle adjust time -390 us > > CPU[2] throttle percent: 73, throttle adjust time -390 us > > > > Signed-off-by: wangfuqiang49 <wangfuqiang49@jd.com> > > --- > > system/dirtylimit.c | 28 ++++++++++++++++++++-------- > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > diff --git a/system/dirtylimit.c b/system/dirtylimit.c > > index 7c071248bb..c7f663e5b9 100644 > > --- a/system/dirtylimit.c > > +++ b/system/dirtylimit.c > > @@ -281,31 +281,30 @@ static void dirtylimit_set_throttle(CPUState *cpu, > > { > > int64_t ring_full_time_us = 0; > > uint64_t sleep_pct = 0; > > + uint64_t throttle_pct = 0; > > uint64_t throttle_us = 0; > > + int64_t throtlle_us_old = cpu->throttle_us_per_full; > > > > if (current == 0) { > > cpu->throttle_us_per_full = 0; > > - return; > > + goto end; > > > > > > The current dirty rate is zero, indicating nothing needs to be done, > > > > including displaying the trace event. return directly seems justified. My original goal is to report every change in throttle_us_per_full using the trace event including when throttle_us_per_full is zero. When testing test/migration/stress, it generally occurs when the stress process migrates to another core. I would like to seek your advice again on whether this is necessary. > > > > } > > > > ring_full_time_us = dirtylimit_dirty_ring_full_time(current); > > > > if (dirtylimit_need_linear_adjustment(quota, current)) { > > if (quota < current) { > > - sleep_pct = (current - quota) * 100 / current; > > > > > > s/sleep_pct/throttle_pci/ , why ? I modified the sleep_pct variable to record the actual percentage of sleep time, while the original sleep_pct records the percentage of each sleep time adjustment. However, this doesn't seem to be a good change. How about keeping the original sleep_pct variable and adding a new sleep_pct_full variable to store the actual percentage of sleep time? > > > > + throttle_pct = (current - quota) * 100 / current; > > throttle_us = > > - ring_full_time_us * sleep_pct / (double)(100 - sleep_pct); > > + ring_full_time_us * throttle_pct / (double)(100 - > > throttle_pct); > > cpu->throttle_us_per_full += throttle_us; > > } else { > > - sleep_pct = (quota - current) * 100 / quota; > > + throttle_pct = (quota - current) * 100 / quota; > > throttle_us = > > - ring_full_time_us * sleep_pct / (double)(100 - sleep_pct); > > + ring_full_time_us * throttle_pct / (double)(100 - > > throttle_pct); > > cpu->throttle_us_per_full -= throttle_us; > > } > > > > - trace_dirtylimit_throttle_pct(cpu->cpu_index, > > - sleep_pct, > > - throttle_us); > > } else { > > if (quota < current) { > > cpu->throttle_us_per_full += ring_full_time_us / 10; > > @@ -323,6 +322,19 @@ static void dirtylimit_set_throttle(CPUState *cpu, > > ring_full_time_us * DIRTYLIMIT_THROTTLE_PCT_MAX); > > > > cpu->throttle_us_per_full = MAX(cpu->throttle_us_per_full, 0); > > + > > +end: > > + if (cpu->throttle_us_per_full - throtlle_us_old) { > > + if (current) { > > + sleep_pct = ring_full_time_us * 100 / (ring_full_time_us + > > + cpu->throttle_us_per_full); I'm very sorry, it seems I sent an old version of the patch. It should be: + if (cpu->throttle_us_per_full - throtlle_us_old) { + if (current) { + sleep_pct = cpu->throttle_us_per_full * 100 / (ring_full_time_us + + cpu->throttle_us_per_full); The test records in the commit message are based on the new version. > > + } else { > > + sleep_pct = 0; > > + } > > + trace_dirtylimit_throttle_pct(cpu->cpu_index, > > + sleep_pct, > > + cpu->throttle_us_per_full - > > + throtlle_us_old); } > > > > > > This changes the interface of the trace event, We can keep > > the throttle_us and add the delta meanwhile: > > trace_dirtylimit_throttle_pct(cpu->cpu_index, > > sleep_pct, > > throttle_us, > > cpu->throttle_us_per_full - throtlle_us_old) > > > > } > > > > static void dirtylimit_adjust_throttle(CPUState *cpu) > > -- > > 2.47.0 > > > > > > > > -- > > Best regards "cpu->throttle_us_per_full - throttle_us_old represents the adjustment value for throttle us in this update." And throttle_us is as well. However, throttle_us is always a positive value and now it includes both positive and negative values. The change to throttle_us alone doesn't seem to alter the interface of trace_event. However, as mentioned earlier, the meaning of sleep_pct has changed. Previously, it was used to indicate the percentage of the current adjustment, and now it is used to represent the actual percentage of sleep time.(I'm not sure if I misunderstood your meaning or if I have a misunderstanding of the code.) So, is it necessary to also report the adjustment of sleep time in this update? I would appreciate any suggestions you might have. fuqiang THX
diff --git a/system/dirtylimit.c b/system/dirtylimit.c index 7c071248bb..c7f663e5b9 100644 --- a/system/dirtylimit.c +++ b/system/dirtylimit.c @@ -281,31 +281,30 @@ static void dirtylimit_set_throttle(CPUState *cpu, { int64_t ring_full_time_us = 0; uint64_t sleep_pct = 0; + uint64_t throttle_pct = 0; uint64_t throttle_us = 0; + int64_t throtlle_us_old = cpu->throttle_us_per_full; if (current == 0) { cpu->throttle_us_per_full = 0; - return; + goto end; } ring_full_time_us = dirtylimit_dirty_ring_full_time(current); if (dirtylimit_need_linear_adjustment(quota, current)) { if (quota < current) { - sleep_pct = (current - quota) * 100 / current; + throttle_pct = (current - quota) * 100 / current; throttle_us = - ring_full_time_us * sleep_pct / (double)(100 - sleep_pct); + ring_full_time_us * throttle_pct / (double)(100 - throttle_pct); cpu->throttle_us_per_full += throttle_us; } else { - sleep_pct = (quota - current) * 100 / quota; + throttle_pct = (quota - current) * 100 / quota; throttle_us = - ring_full_time_us * sleep_pct / (double)(100 - sleep_pct); + ring_full_time_us * throttle_pct / (double)(100 - throttle_pct); cpu->throttle_us_per_full -= throttle_us; } - trace_dirtylimit_throttle_pct(cpu->cpu_index, - sleep_pct, - throttle_us); } else { if (quota < current) { cpu->throttle_us_per_full += ring_full_time_us / 10; @@ -323,6 +322,19 @@ static void dirtylimit_set_throttle(CPUState *cpu, ring_full_time_us * DIRTYLIMIT_THROTTLE_PCT_MAX); cpu->throttle_us_per_full = MAX(cpu->throttle_us_per_full, 0); + +end: + if (cpu->throttle_us_per_full - throtlle_us_old) { + if (current) { + sleep_pct = ring_full_time_us * 100 / (ring_full_time_us + + cpu->throttle_us_per_full); + } else { + sleep_pct = 0; + } + trace_dirtylimit_throttle_pct(cpu->cpu_index, + sleep_pct, + cpu->throttle_us_per_full - + throtlle_us_old); } } static void dirtylimit_adjust_throttle(CPUState *cpu)
The current dirtylimit_throttle_pct trace event is triggered when the throttle time is adjusted linearly. Modify the trace event so that it can record non-linear adjustments. Additionally, since the throttle time might be adjusted again at the end of the dirtylimit_set_throttle function, move the trace event to after this process and calculate the final adjustment time and sleep percent. This patch can fix the following issue: 1. The current dirty rate at 1000MB/s and the dirty limit value at 10000MB/s, before merge this patch, this trace event will print: CPU[2] throttle percent: 98, throttle adjust time 191590 us CPU[2] throttle percent: 98, throttle adjust time 191002 us CPU[2] throttle percent: 98, throttle adjust time 191002 us After merge this patch, there will be no print. 2. The current dirty rate is 1000MB/s and the dirty limit rate value is 333MB/s, before merge this patch, this trace event will print: CPU[3] throttle percent: 98, throttle adjust time 32666634 us It will only print linear adjustment, and the adjust time will be larger and only have positive values. After merge this patch, print as following: CPU[2] throttle percent: 97, throttle adjust time 128766 us CPU[2] throttle percent: 94, throttle adjust time -61131 us CPU[2] throttle percent: 92, throttle adjust time -16634 us ... CPU[2] throttle percent: 74, throttle adjust time -390 us CPU[2] throttle percent: 73, throttle adjust time -390 us Signed-off-by: wangfuqiang49 <wangfuqiang49@jd.com> --- system/dirtylimit.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)