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