linux-kernel - Re: [PATCH] block: make iolatency avg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180801001326.GA34556@dennisz-mbp.dhcp.thefacebook.com>
Date:   Tue, 31 Jul 2018 17:13:28 -0700
From:   Dennis Zhou <dennisszhou@...il.com>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Tejun Heo <tj@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Josef Bacik <jbacik@...com>, kernel-team@...com,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block: make iolatency avg_lat exponentially decay

Hi Johannes,

On Tue, Jul 31, 2018 at 05:21:50PM -0400, Johannes Weiner wrote:
> Hi Dennis,
> 
> this generally looks good to me. Just two small nit picks:
> 
> On Tue, Jul 31, 2018 at 01:36:47PM -0700, Dennis Zhou wrote:
> > @@ -135,6 +135,24 @@ struct iolatency_grp {
> >  	struct child_latency_info child_lat;
> >  };
> >  
> > +#define BLKIOLATENCY_MIN_WIN_SIZE (100 * NSEC_PER_MSEC)
> > +#define BLKIOLATENCY_MAX_WIN_SIZE NSEC_PER_SEC
> > +/*
> > + * These are the constants used to fake the fixed-point moving average
> > + * calculation just like load average. The latency window is bucketed to
> > + * try to approximately calculate average latency for the last 1 minute.
> > + */
> > +#define BLKIOLATENCY_NR_EXP_FACTORS 5
> > +#define BLKIOLATENCY_EXP_BUCKET_SIZE (BLKIOLATENCY_MAX_WIN_SIZE / \
> > +				      (BLKIOLATENCY_NR_EXP_FACTORS - 1))
> > +static const u64 iolatency_exp_factors[BLKIOLATENCY_NR_EXP_FACTORS] = {
> > +	2045, // exp(1/600) - 600 samples
> > +	2039, // exp(1/240) - 240 samples
> > +	2031, // exp(1/120) - 120 samples
> > +	2023, // exp(1/80)  - 80 samples
> > +	2014, // exp(1/60)  - 60 samples
> 
> Might be useful to drop the FIXED_1 name in a comment here. It says
> "fixed-point", and "load average", but since the numbers are directly
> in relationship to that constant, it'd be good to name it I think.
> 

I've added a comment in v2 that points out FIXED_1 and mentions its
value is 2048. I also explained a little more about the samples and
binding the 1/exp window.

> > +	exp_idx = min_t(int, BLKIOLATENCY_NR_EXP_FACTORS - 1,
> > +			iolat->cur_win_nsec / BLKIOLATENCY_EXP_BUCKET_SIZE);
> > +	CALC_LOAD(iolat->total_lat_avg, iolatency_exp_factors[exp_idx],
> > +		  stat.mean);
> 
> The load average keeps the running value in fixed point presentation
> to avoid rounding errors. I guess because this is IO time in ns, the
> values are so much higher than the FIXED_1 denominator (2048) that
> rounding errors are negligible, and we don't need to bother with it.
> 
> Can you mention that in a comment, please?

I've added what you said here in a comment above this.

Thanks,
Dennis