Messages in this thread | | | Date | Fri, 12 Aug 2011 19:07:18 +0800 | From | Wu Fengguang <> | Subject | Re: [PATCH 2/5] writeback: dirty position control |
| |
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
| |