Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 8 May 2020 11:20:03 +0200
From:   Jan Kara <jack@suse.cz>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Tan Hu <tan.hu@zte.com.cn>, linux-kernel@vger.kernel.org,
        xue.zhihong@zte.com.cn, wang.yi59@zte.com.cn,
        wang.liang82@zte.com.cn, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH] lib/flex_proportions.c: aging counts when fraction
 smaller than max_frac/FPROP_FRAC_BASE
Message-ID: <20200508092003.GC20446@quack2.suse.cz>
References: <1588746088-38605-1-git-send-email-tan.hu@zte.com.cn>
 <20200507164614.4a816d2aa1b341be937b128a@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200507164614.4a816d2aa1b341be937b128a@linux-foundation.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for CC Andrew.

On Thu 07-05-20 16:46:14, Andrew Morton wrote:
> On Wed, 6 May 2020 14:21:28 +0800 Tan Hu <tan.hu@zte.com.cn> wrote:
> 
> > If the given type has fraction smaller than max_frac/FPROP_FRAC_BASE,
> > __fprop_inc_percpu_max should follow the design formula and aging
> > fraction too.
> > 
> > Signed-off-by: Tan Hu <tan.hu@zte.com.cn>
> > ---
> >  lib/flex_proportions.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
> > index 7852bfff5..451543937 100644
> > --- a/lib/flex_proportions.c
> > +++ b/lib/flex_proportions.c
> > @@ -266,8 +266,7 @@ void __fprop_inc_percpu_max(struct fprop_global *p,
> >  		if (numerator >
> >  		    (((u64)denominator) * max_frac) >> FPROP_FRAC_SHIFT)
> >  			return;
> > -	} else
> > -		fprop_reflect_period_percpu(p, pl);
> > -	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
> > -	percpu_counter_add(&p->events, 1);
> > +	}
> > +
> > +	__fprop_inc_percpu(p, pl);

So the code is actually correct as is because if max_frac <
FPROP_FRAC_BASE, we call fprop_fraction_percpu() which calls
fprop_reflect_period_percpu(). So the 'else' is there to avoid calling the
function twice. That being said calling fprop_reflect_period_percpu() twice
as we would with your patch does no big harm as we'd just quickly bail on
pl->period == p->period test. So I think the code is easier to understand
the way you suggest without significant downside. But please update the
changelog to explain that this is just code cleanup (with the above
reasoning) and not a functional fix.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR