Re: [PATCH 05/16] blkio: Implement per cfq group latency target and busy queue avg

[Corrado Zoccolo]

On Fri, Nov 13, 2009 at 5:15 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Fri, Nov 13, 2009 at 10:18:15AM -0500, Vivek Goyal wrote:
>> On Fri, Nov 13, 2009 at 11:46:49AM +0100, Corrado Zoccolo wrote:
>> > On Fri, Nov 13, 2009 at 12:32 AM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> > > Âstatic inline void
>> > > @@ -441,10 +445,13 @@ cfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>> > > Â Â Â Âif (cfqd->cfq_latency) {
>> > > Â Â Â Â Â Â Â Â/* interested queues (we consider only the ones with the same
>> > > Â Â Â Â Â Â Â Â * priority class) */
>> > This comment needs to be updated
>>
>> Sure. Will do. Now the interested queues are the one with same priority
>> class with-in group.
>>
>> > > Â Â Â Â Â Â Â Â * priority class) */
>> > > - Â Â Â Â Â Â Â unsigned iq = cfq_get_avg_queues(cfqd, cfq_class_rt(cfqq));
>> > > + Â Â Â Â Â Â Â unsigned iq = cfq_group_get_avg_queues(cfqd, cfqq->cfqg,
>> > > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â cfq_class_rt(cfqq));
>> > > Â Â Â Â Â Â Â Âunsigned sync_slice = cfqd->cfq_slice[1];
>> > > Â Â Â Â Â Â Â Âunsigned expect_latency = sync_slice * iq;
>> > > - Â Â Â Â Â Â Â if (expect_latency > cfq_target_latency) {
>> > > + Â Â Â Â Â Â Â unsigned group_target_lat = cfq_target_latency/cfqd->nr_groups;
>> >
>> > I'm not sure that we should divide the target latency evenly among groups.
>> > Groups with different weights will have different percentage of time
>> > in each 300ms round, so probably we should consider it here.
>> >
>>
>> Taking group weight into account will be more precise thing. So may be
>> I can keep track of total weight on the service tree and determine
>> group target latency as proportion of total weight.
>>
>> Âgroup_target_lat = group_weight * cfq_target_latency/total_weight_of_groups
>>
>
> Here is the patch I generated on top of all the patches in series.
>
> o Determine group target latency in proportion to group weight instead of
> Âjust number of groups.

Great.
I have only one concern, regarding variable naming:
group_target_lat is a bit misleading. The fact is that it will be
larger for higher weight groups, so people could ask why are you
giving more latency to higher weight group...
Actually, it is the group share of the scheduling round, so you should
name it accordingly.

>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> Âblock/cfq-iosched.c | Â 21 +++++++++++++++++----
> Â1 file changed, 17 insertions(+), 4 deletions(-)
>
> Index: linux7/block/cfq-iosched.c
> ===================================================================
> --- linux7.orig/block/cfq-iosched.c   2009-11-13 09:48:38.000000000 -0500
> +++ linux7/block/cfq-iosched.c Â2009-11-13 11:06:22.000000000 -0500
> @@ -81,6 +81,7 @@ struct cfq_rb_root {
> Â Â Â Âunsigned count;
> Â Â Â Âu64 min_vdisktime;
> Â Â Â Âstruct rb_node *active;
> + Â Â Â unsigned total_weight;
> Â};
> Â#define CFQ_RB_ROOT Â Â(struct cfq_rb_root) { RB_ROOT, NULL, 0, 0, }
>
> @@ -521,18 +522,28 @@ static inline unsigned cfq_group_get_avg
> Â Â Â Âreturn cfqg->busy_queues_avg[rt];
> Â}
>
> +static inline unsigned
> +cfq_group_latency(struct cfq_data *cfqd, struct cfq_group *cfqg)
> +{
> + Â Â Â struct cfq_rb_root *st = &cfqd->grp_service_tree;
> +
> + Â Â Â return cfq_target_latency * cfqg->weight / st->total_weight;
> +}
> +
> Âstatic inline void
> Âcfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> Â{
> Â Â Â Âunsigned slice = cfq_prio_to_slice(cfqd, cfqq);
> Â Â Â Âif (cfqd->cfq_latency) {
> - Â Â Â Â Â Â Â /* interested queues (we consider only the ones with the same
> - Â Â Â Â Â Â Â Â* priority class) */
> + Â Â Â Â Â Â Â /*
> + Â Â Â Â Â Â Â Â* interested queues (we consider only the ones with the same
> + Â Â Â Â Â Â Â Â* priority class in the cfq group)
> + Â Â Â Â Â Â Â Â*/
> Â Â Â Â Â Â Â Âunsigned iq = cfq_group_get_avg_queues(cfqd, cfqq->cfqg,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcfq_class_rt(cfqq));
> Â Â Â Â Â Â Â Âunsigned sync_slice = cfqd->cfq_slice[1];
> Â Â Â Â Â Â Â Âunsigned expect_latency = sync_slice * iq;
> - Â Â Â Â Â Â Â unsigned group_target_lat = cfq_target_latency/cfqd->nr_groups;
> + Â Â Â Â Â Â Â unsigned group_target_lat = cfq_group_latency(cfqd, cfqq->cfqg);
>
> Â Â Â Â Â Â Â Âif (expect_latency > group_target_lat) {
> Â Â Â Â Â Â Â Â Â Â Â Âunsigned base_low_slice = 2 * cfqd->cfq_slice_idle;
> @@ -799,6 +810,7 @@ cfq_group_service_tree_add(struct cfq_da
> Â Â Â Â__cfq_group_service_tree_add(st, cfqg);
> Â Â Â Âcfqg->on_st = true;
> Â Â Â Âcfqd->nr_groups++;
> + Â Â Â st->total_weight += cfqg->weight;
> Â}
>
> Âstatic void
> @@ -819,6 +831,7 @@ cfq_group_service_tree_del(struct cfq_da
> Â Â Â Âcfq_log_cfqg(cfqd, cfqg, "del_from_rr group");
> Â Â Â Âcfqg->on_st = false;
> Â Â Â Âcfqd->nr_groups--;
> + Â Â Â st->total_weight -= cfqg->weight;
> Â Â Â Âif (!RB_EMPTY_NODE(&cfqg->rb_node))
> Â Â Â Â Â Â Â Âcfq_rb_erase(&cfqg->rb_node, st);
> Â Â Â Âcfqg->saved_workload_slice = 0;
> @@ -2033,7 +2046,7 @@ static void choose_service_tree(struct c
> Â Â Â Â * proportional to the number of queues in that workload, over
> Â Â Â Â * all the queues in the same priority class
> Â Â Â Â */
> - Â Â Â group_target_latency = cfq_target_latency/cfqd->nr_groups;
> + Â Â Â group_target_latency = cfq_group_latency(cfqd, cfqg);
>
> Â Â Â Âslice = group_target_latency * count /
> Â Â Â Â Â Â Â Âmax_t(unsigned, cfqg->busy_queues_avg[cfqd->serving_prio],
>



-- 
__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:czoccolo@xxxxxxxxx
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/