Re: [PATCH V5 11/17] blk-throttle: add a simple idle detection

[Tejun Heo <tj@kernel.org>]

On Thu, Dec 15, 2016 at 12:33:02PM -0800, Shaohua Li wrote:
>  /* Throttling is performed over 100ms slice and after that slice is renewed */
>  #define DFL_THROTL_SLICE (HZ / 10)
>  #define MAX_THROTL_SLICE (HZ / 5)
> +#define DFL_IDLE_THRESHOLD_SSD (50 * 1000) /* 50 us */
> +#define DFL_IDLE_THRESHOLD_HD (1000 * 1000) /* 1 ms */
> +#define MAX_IDLE_TIME (500L * 1000 * 1000) /* 500 ms */

Hmm... why are we capping idle time so low?  This is a value to be
configured by userland.  Does it make sense to cap it this low?  Also,
wouldn't it make sense to start with higher default value given that
the user has to explicitly enable low limit for it to be effective and
thus explicitly requesting best effort latency target which will be
added later?

I'm really uncomfortable with pitting these two knobs against each
other in the similar time ranges.  It's really difficult tell what
latency target of 25us means and predict its behavior and when the
idle timeout is 50us.  It's fine if some people fiddle with them but
it'd be great if the defaults clearly indicate that they're operating
in mostly separate time scales.

Thanks.

-- 
tejun