Re: [PATCH 2/5] writeback: dirty position control

Wu Fengguang
2011-08-12T11:07:26Z

On Fri, Aug 12, 2011 at 05:45:33PM +0800, Peter Zijlstra wrote:
> On Fri, 2011-08-12 at 13:45 +0800, Wu Fengguang wrote:
> > Code is
> >
> > unsigned long freerun = (thresh + bg_thresh) / 2;
> >
> > setpoint = (limit + freerun) / 2;
> > pos_ratio = abs(dirty - setpoint);
> > pos_ratio <<= BANDWIDTH_CALC_SHIFT;
> > do_div(pos_ratio, limit - setpoint + 1);
>
> Why do you use do_div()? from the code those things are unsigned long,
> and you can divide that just fine.

Because pos_ratio was "unsigned long long"..

> Also, there's div64_s64 that can do signed divides for s64 types.
> That'll loose the extra conditionals you used for abs and putting the
> sign back.

Ah ok, good to know that :)

> > x = pos_ratio;
> > pos_ratio = pos_ratio * x >> BANDWIDTH_CALC_SHIFT;
> > pos_ratio = pos_ratio * x >> BANDWIDTH_CALC_SHIFT;
>
> So on 32bit with unsigned long that gets 32=2*(10+b) bits for x, that
> solves to 6, which isn't going to be enough I figure since
> (dirty-setpoint) !< 64.
>
> So you really need to use u64/s64 types here, unsigned long just won't
> do, with u64 you have 64=2(10+b) 22 bits for x, which should fit.

Sure, here is the updated code:

long long pos_ratio; /* for scaling up/down the rate limit */
long x;

if (unlikely(dirty >= limit))
return 0;

/*
* global setpoint
*
* setpoint - dirty 3
* f(dirty) := 1 + (----------------)
* limit - setpoint
*
* it's a 3rd order polynomial that subjects to
*
* (1) f(freerun) = 2.0 => rampup base_rate reasonably fast
* (2) f(setpoint) = 1.0 => the balance point
* (3) f(limit) = 0 => the hard limit
* (4) df/dx < 0 => negative feedback control
* (5) the closer to setpoint, the smaller |df/dx| (and the reverse),
* => fast response on large errors; small oscillation near setpoint
*/
setpoint = (limit + freerun) / 2;
pos_ratio = (setpoint - dirty) << RATELIMIT_CALC_SHIFT;
pos_ratio = div_s64(pos_ratio, limit - setpoint + 1);
x = pos_ratio;
pos_ratio = pos_ratio * x >> RATELIMIT_CALC_SHIFT;
pos_ratio = pos_ratio * x >> RATELIMIT_CALC_SHIFT;
pos_ratio += 1 << RATELIMIT_CALC_SHIFT;

Thanks,
Fengguang