* [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative.
@ 2021-05-17 15:53 Chi Wu
2021-05-18 3:42 ` chi wu
0 siblings, 1 reply; 6+ messages in thread
From: Chi Wu @ 2021-05-17 15:53 UTC (permalink / raw)
To: akpm; +Cc: jack, linux-kernel, tan.hu, Chi Wu
The value of percpu_counter_read() may become negative after
running percpu_counter_sum() in fprop_reflect_period_percpu().
The value of variable 'num' will be zero in fprop_fraction_percpu()
when using percpu_counter_read_positive(), but if using the abs of
percpu_counter_read() will be close to the correct value.
Signed-off-by: Chi Wu <wuchi.zero@gmail.com>
---
lib/flex_proportions.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
index 451543937524..3ac79ca2c441 100644
--- a/lib/flex_proportions.c
+++ b/lib/flex_proportions.c
@@ -147,7 +147,7 @@ void fprop_fraction_single(struct fprop_global *p,
seq = read_seqcount_begin(&p->sequence);
fprop_reflect_period_single(p, pl);
num = pl->events;
- den = percpu_counter_read_positive(&p->events);
+ den = abs(percpu_counter_read(&p->events));
} while (read_seqcount_retry(&p->sequence, seq));
/*
@@ -209,7 +209,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
val = percpu_counter_sum(&pl->events);
percpu_counter_add_batch(&pl->events,
- -val + (val >> (period-pl->period)), PROP_BATCH);
+ -val + (val >> (period - pl->period)), PROP_BATCH);
} else
percpu_counter_set(&pl->events, 0);
pl->period = period;
@@ -234,8 +234,8 @@ void fprop_fraction_percpu(struct fprop_global *p,
do {
seq = read_seqcount_begin(&p->sequence);
fprop_reflect_period_percpu(p, pl);
- num = percpu_counter_read_positive(&pl->events);
- den = percpu_counter_read_positive(&p->events);
+ num = abs(percpu_counter_read(&pl->events));
+ den = abs(percpu_counter_read(&p->events));
} while (read_seqcount_retry(&p->sequence, seq));
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative.
2021-05-17 15:53 [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative Chi Wu
@ 2021-05-18 3:42 ` chi wu
2021-05-18 8:59 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: chi wu @ 2021-05-18 3:42 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jan Kara, linux-kernel, tan.hu
Chi Wu <wuchi.zero@gmail.com> 于2021年5月17日周一 下午11:53写道：
>
> The value of percpu_counter_read() may become negative after
> running percpu_counter_sum() in fprop_reflect_period_percpu().
> The value of variable 'num' will be zero in fprop_fraction_percpu()
> when using percpu_counter_read_positive(), but if using the abs of
> percpu_counter_read() will be close to the correct value.
>
I realized that I was wrong as follow:
(a) the decay rule is broken, the negative means the difference in
decay here.
(b) as the target event increasing, proportion of the event will
decrease to 0 firstly and then it will increase. The logic is bad.
1. abs(-50) / abs(100) = 50% //+50 to 2
2. abs(0) / abs(150) = 0 % //+50 to 3
3. abs(50)/abs(200) = 25%
Anyway, the percpu_counter_sum() had cost a lost performance,
may be we could get a little benefits from that. So could we add a
variable to stroe the decay value, we will get the value when
percpu_counter_read() is negative?
Thanks.
> Signed-off-by: Chi Wu <wuchi.zero@gmail.com>
> ---
> lib/flex_proportions.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
> index 451543937524..3ac79ca2c441 100644
> --- a/lib/flex_proportions.c
> +++ b/lib/flex_proportions.c
> @@ -147,7 +147,7 @@ void fprop_fraction_single(struct fprop_global *p,
> seq = read_seqcount_begin(&p->sequence);
> fprop_reflect_period_single(p, pl);
> num = pl->events;
> - den = percpu_counter_read_positive(&p->events);
> + den = abs(percpu_counter_read(&p->events));
> } while (read_seqcount_retry(&p->sequence, seq));
>
> /*
> @@ -209,7 +209,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
> val = percpu_counter_sum(&pl->events);
>
> percpu_counter_add_batch(&pl->events,
> - -val + (val >> (period-pl->period)), PROP_BATCH);
> + -val + (val >> (period - pl->period)), PROP_BATCH);
> } else
> percpu_counter_set(&pl->events, 0);
> pl->period = period;
> @@ -234,8 +234,8 @@ void fprop_fraction_percpu(struct fprop_global *p,
> do {
> seq = read_seqcount_begin(&p->sequence);
> fprop_reflect_period_percpu(p, pl);
> - num = percpu_counter_read_positive(&pl->events);
> - den = percpu_counter_read_positive(&p->events);
> + num = abs(percpu_counter_read(&pl->events));
> + den = abs(percpu_counter_read(&p->events));
> } while (read_seqcount_retry(&p->sequence, seq));
>
> /*
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative.
2021-05-18 3:42 ` chi wu
@ 2021-05-18 8:59 ` Jan Kara
2021-05-18 10:22 ` chi wu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2021-05-18 8:59 UTC (permalink / raw)
To: chi wu; +Cc: Andrew Morton, Jan Kara, linux-kernel, tan.hu
On Tue 18-05-21 11:42:53, chi wu wrote:
> Chi Wu <wuchi.zero@gmail.com> 于2021年5月17日周一 下午11:53写道：
> >
> > The value of percpu_counter_read() may become negative after
> > running percpu_counter_sum() in fprop_reflect_period_percpu().
> > The value of variable 'num' will be zero in fprop_fraction_percpu()
> > when using percpu_counter_read_positive(), but if using the abs of
> > percpu_counter_read() will be close to the correct value.
> >
>
> I realized that I was wrong as follow:
> (a) the decay rule is broken, the negative means the difference in
> decay here.
> (b) as the target event increasing, proportion of the event will
> decrease to 0 firstly and then it will increase. The logic is bad.
> 1. abs(-50) / abs(100) = 50% //+50 to 2
> 2. abs(0) / abs(150) = 0 % //+50 to 3
> 3. abs(50)/abs(200) = 25%
>
> Anyway, the percpu_counter_sum() had cost a lost performance,
> may be we could get a little benefits from that. So could we add a
> variable to stroe the decay value, we will get the value when
> percpu_counter_read() is negative?
The result of percpu_counter_read() is inherently inexact (but fast! ;). It
can be upto number_of_cpus * counter_batch away from the real counter
value. But do you observe any practical problems with this inaccuracy on
your system? Sure, cache memory won't be split among devices exactly
according to writeout proportion but that usually does not matter.
Honza
> > ---
> > lib/flex_proportions.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
> > index 451543937524..3ac79ca2c441 100644
> > --- a/lib/flex_proportions.c
> > +++ b/lib/flex_proportions.c
> > @@ -147,7 +147,7 @@ void fprop_fraction_single(struct fprop_global *p,
> > seq = read_seqcount_begin(&p->sequence);
> > fprop_reflect_period_single(p, pl);
> > num = pl->events;
> > - den = percpu_counter_read_positive(&p->events);
> > + den = abs(percpu_counter_read(&p->events));
> > } while (read_seqcount_retry(&p->sequence, seq));
> >
> > /*
> > @@ -209,7 +209,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
> > val = percpu_counter_sum(&pl->events);
> >
> > percpu_counter_add_batch(&pl->events,
> > - -val + (val >> (period-pl->period)), PROP_BATCH);
> > + -val + (val >> (period - pl->period)), PROP_BATCH);
> > } else
> > percpu_counter_set(&pl->events, 0);
> > pl->period = period;
> > @@ -234,8 +234,8 @@ void fprop_fraction_percpu(struct fprop_global *p,
> > do {
> > seq = read_seqcount_begin(&p->sequence);
> > fprop_reflect_period_percpu(p, pl);
> > - num = percpu_counter_read_positive(&pl->events);
> > - den = percpu_counter_read_positive(&p->events);
> > + num = abs(percpu_counter_read(&pl->events));
> > + den = abs(percpu_counter_read(&p->events));
> > } while (read_seqcount_retry(&p->sequence, seq));
> >
> > /*
> > --
> > 2.17.1
> >
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative.
2021-05-18 8:59 ` Jan Kara
@ 2021-05-18 10:22 ` chi wu
2021-05-18 10:39 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: chi wu @ 2021-05-18 10:22 UTC (permalink / raw)
To: Jan Kara; +Cc: Andrew Morton, linux-kernel, tan.hu
Jan Kara <jack@suse.cz> 于2021年5月18日周二 下午4:59写道：
>
> On Tue 18-05-21 11:42:53, chi wu wrote:
> > Chi Wu <wuchi.zero@gmail.com> 于2021年5月17日周一 下午11:53写道：
> > >
> > > The value of percpu_counter_read() may become negative after
> > > running percpu_counter_sum() in fprop_reflect_period_percpu().
> > > The value of variable 'num' will be zero in fprop_fraction_percpu()
> > > when using percpu_counter_read_positive(), but if using the abs of
> > > percpu_counter_read() will be close to the correct value.
> > >
> >
> > I realized that I was wrong as follow:
> > (a) the decay rule is broken, the negative means the difference in
> > decay here.
> > (b) as the target event increasing, proportion of the event will
> > decrease to 0 firstly and then it will increase. The logic is bad.
> > 1. abs(-50) / abs(100) = 50% //+50 to 2
> > 2. abs(0) / abs(150) = 0 % //+50 to 3
> > 3. abs(50)/abs(200) = 25%
> >
> > Anyway, the percpu_counter_sum() had cost a lost performance,
> > may be we could get a little benefits from that. So could we add a
> > variable to stroe the decay value, we will get the value when
> > percpu_counter_read() is negative?
>
> The result of percpu_counter_read() is inherently inexact (but fast! ;). It
> can be upto number_of_cpus * counter_batch away from the real counter
> value. But do you observe any practical problems with this inaccuracy on
> your system? Sure, cache memory won't be split among devices exactly
> according to writeout proportion but that usually does not matter.
>
> Honza
>
Thanks, Got it.
Just try to optimize the fuse (with strictlimit feature)performance
issue: The writing thread will be paused and runs slowly, when the
proportion of fuse-bdi is 0.
The issue is normal,and one of reasons is the characteristics of
percpu_counter batch. Even the pages are writeout, we may be could not
get the real proportion value due to side effects of counter
performance. It's just a slight disappointment.
> > > ---
> > > lib/flex_proportions.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
> > > index 451543937524..3ac79ca2c441 100644
> > > --- a/lib/flex_proportions.c
> > > +++ b/lib/flex_proportions.c
> > > @@ -147,7 +147,7 @@ void fprop_fraction_single(struct fprop_global *p,
> > > seq = read_seqcount_begin(&p->sequence);
> > > fprop_reflect_period_single(p, pl);
> > > num = pl->events;
> > > - den = percpu_counter_read_positive(&p->events);
> > > + den = abs(percpu_counter_read(&p->events));
> > > } while (read_seqcount_retry(&p->sequence, seq));
> > >
> > > /*
> > > @@ -209,7 +209,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
> > > val = percpu_counter_sum(&pl->events);
> > >
> > > percpu_counter_add_batch(&pl->events,
> > > - -val + (val >> (period-pl->period)), PROP_BATCH);
> > > + -val + (val >> (period - pl->period)), PROP_BATCH);
> > > } else
> > > percpu_counter_set(&pl->events, 0);
> > > pl->period = period;
> > > @@ -234,8 +234,8 @@ void fprop_fraction_percpu(struct fprop_global *p,
> > > do {
> > > seq = read_seqcount_begin(&p->sequence);
> > > fprop_reflect_period_percpu(p, pl);
> > > - num = percpu_counter_read_positive(&pl->events);
> > > - den = percpu_counter_read_positive(&p->events);
> > > + num = abs(percpu_counter_read(&pl->events));
> > > + den = abs(percpu_counter_read(&p->events));
> > > } while (read_seqcount_retry(&p->sequence, seq));
> > >
> > > /*
> > > --
> > > 2.17.1
> > >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative.
2021-05-18 10:22 ` chi wu
@ 2021-05-18 10:39 ` Jan Kara
2021-05-18 11:55 ` chi wu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2021-05-18 10:39 UTC (permalink / raw)
To: chi wu; +Cc: Jan Kara, Andrew Morton, linux-kernel, tan.hu
On Tue 18-05-21 18:22:05, chi wu wrote:
> Jan Kara <jack@suse.cz> 于2021年5月18日周二 下午4:59写道：
> >
> > On Tue 18-05-21 11:42:53, chi wu wrote:
> > > Chi Wu <wuchi.zero@gmail.com> 于2021年5月17日周一 下午11:53写道：
> > > >
> > > > The value of percpu_counter_read() may become negative after
> > > > running percpu_counter_sum() in fprop_reflect_period_percpu().
> > > > The value of variable 'num' will be zero in fprop_fraction_percpu()
> > > > when using percpu_counter_read_positive(), but if using the abs of
> > > > percpu_counter_read() will be close to the correct value.
> > > >
> > >
> > > I realized that I was wrong as follow:
> > > (a) the decay rule is broken, the negative means the difference in
> > > decay here.
> > > (b) as the target event increasing, proportion of the event will
> > > decrease to 0 firstly and then it will increase. The logic is bad.
> > > 1. abs(-50) / abs(100) = 50% //+50 to 2
> > > 2. abs(0) / abs(150) = 0 % //+50 to 3
> > > 3. abs(50)/abs(200) = 25%
> > >
> > > Anyway, the percpu_counter_sum() had cost a lost performance,
> > > may be we could get a little benefits from that. So could we add a
> > > variable to stroe the decay value, we will get the value when
> > > percpu_counter_read() is negative?
> >
> > The result of percpu_counter_read() is inherently inexact (but fast! ;). It
> > can be upto number_of_cpus * counter_batch away from the real counter
> > value. But do you observe any practical problems with this inaccuracy on
> > your system? Sure, cache memory won't be split among devices exactly
> > according to writeout proportion but that usually does not matter.
> >
> > Honza
> >
>
> Thanks, Got it.
> Just try to optimize the fuse (with strictlimit feature)performance
> issue: The writing thread will be paused and runs slowly, when the
> proportion of fuse-bdi is 0.
> The issue is normal,and one of reasons is the characteristics of
> percpu_counter batch. Even the pages are writeout, we may be could not
> get the real proportion value due to side effects of counter
> performance. It's just a slight disappointment.
Well, you can tune 'min_ratio' of the fuse-bdi to avoid problems with these
near-zero states... To always give the bdi some breathing room for ramping
up.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative.
2021-05-18 10:39 ` Jan Kara
@ 2021-05-18 11:55 ` chi wu
0 siblings, 0 replies; 6+ messages in thread
From: chi wu @ 2021-05-18 11:55 UTC (permalink / raw)
To: Jan Kara; +Cc: Andrew Morton, linux-kernel, tan.hu
Jan Kara <jack@suse.cz> 于2021年5月18日周二 下午6:39写道：
>
> On Tue 18-05-21 18:22:05, chi wu wrote:
> > Jan Kara <jack@suse.cz> 于2021年5月18日周二 下午4:59写道：
> > >
> > > On Tue 18-05-21 11:42:53, chi wu wrote:
> > > > Chi Wu <wuchi.zero@gmail.com> 于2021年5月17日周一 下午11:53写道：
> > > > >
> > > > > The value of percpu_counter_read() may become negative after
> > > > > running percpu_counter_sum() in fprop_reflect_period_percpu().
> > > > > The value of variable 'num' will be zero in fprop_fraction_percpu()
> > > > > when using percpu_counter_read_positive(), but if using the abs of
> > > > > percpu_counter_read() will be close to the correct value.
> > > > >
> > > >
> > > > I realized that I was wrong as follow:
> > > > (a) the decay rule is broken, the negative means the difference in
> > > > decay here.
> > > > (b) as the target event increasing, proportion of the event will
> > > > decrease to 0 firstly and then it will increase. The logic is bad.
> > > > 1. abs(-50) / abs(100) = 50% //+50 to 2
> > > > 2. abs(0) / abs(150) = 0 % //+50 to 3
> > > > 3. abs(50)/abs(200) = 25%
> > > >
> > > > Anyway, the percpu_counter_sum() had cost a lost performance,
> > > > may be we could get a little benefits from that. So could we add a
> > > > variable to stroe the decay value, we will get the value when
> > > > percpu_counter_read() is negative?
> > >
> > > The result of percpu_counter_read() is inherently inexact (but fast! ;). It
> > > can be upto number_of_cpus * counter_batch away from the real counter
> > > value. But do you observe any practical problems with this inaccuracy on
> > > your system? Sure, cache memory won't be split among devices exactly
> > > according to writeout proportion but that usually does not matter.
> > >
> > > Honza
> > >
> >
> > Thanks, Got it.
> > Just try to optimize the fuse (with strictlimit feature)performance
> > issue: The writing thread will be paused and runs slowly, when the
> > proportion of fuse-bdi is 0.
> > The issue is normal,and one of reasons is the characteristics of
> > percpu_counter batch. Even the pages are writeout, we may be could not
> > get the real proportion value due to side effects of counter
> > performance. It's just a slight disappointment.
>
> Well, you can tune 'min_ratio' of the fuse-bdi to avoid problems with these
> near-zero states... To always give the bdi some breathing room for ramping
> up.
>
Thanks, it is useful to me.
> Honza
>
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-18 11:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-17 15:53 [PATCH] lib/flex_proportions.c: Use abs() when percpu_counter is negative Chi Wu
2021-05-18 3:42 ` chi wu
2021-05-18 8:59 ` Jan Kara
2021-05-18 10:22 ` chi wu
2021-05-18 10:39 ` Jan Kara
2021-05-18 11:55 ` chi wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox