> Il giorno 11 giu 2020, alle ore 16:39, Jan Kara <jack@xxxxxxx> ha scritto:
>
> On Thu 11-06-20 16:11:10, Paolo Valente wrote:
>>
>>
>>> Il giorno 5 giu 2020, alle ore 16:16, Jan Kara <jack@xxxxxxx> ha scritto:
>>>
>>> Currently whenever bfq queue has a request queued we add now -
>>> last_completion_time to the think time statistics. This is however
>>> misleading in case the process is able to submit several requests in
>>> parallel because e.g. if the queue has request completed at time T0 and
>>> then queues new requests at times T1, T2, then we will add T1-T0 and
>>> T2-T0 to think time statistics which just doesn't make any sence (the
>>> queue's think time is penalized by the queue being able to submit more
>>> IO).
>>
>> I've thought about this issue several times. My concern is that, by
>> updating the think time only when strictly meaningful, we reduce the
>> number of samples. In some cases, we may reduce it to a very low
>> value. For this reason, so far I have desisted from changing the
>> current scheme. IOW, I opted for dirtier statistics to avoid the risk
>> of too scarse statistics. Any feedback is very welcome.
>
> I understand the concern.
Hi,
sorry for the sidereal delay.
> But:
>
> a) I don't think adding these samples to statistics helps in any way (you
> cannot improve the prediction power of the statistics by including in it
> some samples that are not directly related to the thing you try to
> predict). And think time is used to predict the answer to the question: If
> bfq queue becomes idle, how long will it take for new request to arrive? So
> second and further requests are simply irrelevant.
>
Yes, you are super right in theory.
Unfortunately this may not mean that your patch will do only good, for
the concerns in my previous email.
So, here is a proposal to move forward:
1) I test your patch on my typical set of
latency/guaranteed-bandwidth/total-throughput benchmarks
2) You test your patch on a significant set of benchmarks in mmtests
What do you think?
Thanks,
Paolo
> b) From my testing with 4 fio sequential readers (the workload mentioned in
> the previous patch) this patch caused a noticeable change in computed think
> time and that allowed fio processes to be reliably marked as having short
> think time (without this patch they were oscilating between short ttime and
> not short ttime) and consequently achieving better throughput because we
> were idling for new requests from these bfq queues. Note that this was with
> somewhat aggressive slice_idle setting (2ms). Having slice_idle >= 4ms was
> enough hide the ttime computation issue but still this demonstrates that
> these bogus samples noticeably raise computed average.
>
> Honza
>
>>> So add to think time statistics only time intervals when the queue
>>> had no IO pending.
>>>
>>> Signed-off-by: Jan Kara <jack@xxxxxxx>
>>> ---
>>> block/bfq-iosched.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>> index c66c3eaa9e26..4b1c9c5f57b6 100644
>>> --- a/block/bfq-iosched.c
>>> +++ b/block/bfq-iosched.c
>>> @@ -5192,8 +5192,16 @@ static void bfq_update_io_thinktime(struct bfq_data *bfqd,
>>> struct bfq_queue *bfqq)
>>> {
>>> struct bfq_ttime *ttime = &bfqq->ttime;
>>> - u64 elapsed = ktime_get_ns() - bfqq->ttime.last_end_request;
>>> -
>>> + u64 elapsed;
>>> +
>>> + /*
>>> + * We are really interested in how long it takes for the queue to
>>> + * become busy when there is no outstanding IO for this queue. So
>>> + * ignore cases when the bfq queue has already IO queued.
>>> + */
>>> + if (bfqq->dispatched || bfq_bfqq_busy(bfqq))
>>> + return;
>>> + elapsed = ktime_get_ns() - bfqq->ttime.last_end_request;
>>> elapsed = min_t(u64, elapsed, 2ULL * bfqd->bfq_slice_idle);
>>>
>>> ttime->ttime_samples = (7*ttime->ttime_samples + 256) / 8;
>>> --
>>> 2.16.4
>>>
>>
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
