From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755426Ab1BNDUo (ORCPT ); Sun, 13 Feb 2011 22:20:44 -0500 Received: from cn.fujitsu.com ([222.73.24.84]:62320 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751917Ab1BNDUk (ORCPT ); Sun, 13 Feb 2011 22:20:40 -0500 Message-ID: <4D589F81.2050408@cn.fujitsu.com> Date: Mon, 14 Feb 2011 11:20:33 +0800 From: Gui Jianfeng User-Agent: Thunderbird 2.0.0.24 (Windows/20100228) MIME-Version: 1.0 To: Vivek Goyal CC: Jens Axboe , Shaohua Li , lkml , Chad Talbott , Divyesh Shah Subject: Re: [PATCH 5/6 v4] cfq-iosched: CFQ group hierarchical scheduling and use_hierarchy interface References: <4D51ED26.8050809@cn.fujitsu.com> <4D539821.1090703@cn.fujitsu.com> <20110210205722.GB2600@redhat.com> In-Reply-To: <20110210205722.GB2600@redhat.com> X-MIMETrack: Itemize by SMTP Server on mailserver/fnst(Release 8.5.1FP4|July 25, 2010) at 2011-02-14 11:19:42, Serialize by Router on mailserver/fnst(Release 8.5.1FP4|July 25, 2010) at 2011-02-14 11:19:42, Serialize complete at 2011-02-14 11:19:42 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Vivek Goyal wrote: > On Thu, Feb 10, 2011 at 03:47:45PM +0800, Gui Jianfeng wrote: >> CFQ group hierarchical scheduling and use_hierarchy interface. >> > > Hi Gui, > > I have done a quick high level review. Some minor comments inline. > > [..] >> struct cfq_data { >> struct request_queue *queue; >> - /* Root service tree for cfq_groups */ >> - struct cfq_rb_root grp_service_tree; >> struct cfq_group root_group; >> >> + /* cfq group schedule in flat or hierarchy manner. */ >> + bool use_hierarchy; >> + > > This seems to be redundant now? Nobody is using it? > >> /* >> * The priority currently being served >> */ >> @@ -246,6 +251,9 @@ struct cfq_data { >> unsigned long workload_expires; >> struct cfq_group *serving_group; >> >> + /* Service tree for cfq group flat scheduling mode. */ >> + struct cfq_rb_root grp_service_tree; > > Above comment is misleading. This service tree is now used both for > flat as well as hierarhical mode. > > [..] >> static void >> cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg) >> { >> - struct cfq_rb_root *st = &cfqd->grp_service_tree; >> struct cfq_entity *cfqe = &cfqg->cfqe; >> - struct cfq_entity *__cfqe; >> struct rb_node *n; >> + struct cfq_entity *entity; >> + struct cfq_rb_root *st; >> + struct cfq_group *__cfqg; >> >> cfqg->nr_cfqq++; >> + >> if (!RB_EMPTY_NODE(&cfqe->rb_node)) >> return; >> >> /* >> - * Currently put the group at the end. Later implement something >> - * so that groups get lesser vtime based on their weights, so that >> - * if group does not loose all if it was not continously backlogged. >> + * Enqueue this group and its ancestors onto their service tree. >> */ >> - n = rb_last(&st->rb); >> - if (n) { >> - __cfqe = rb_entry_entity(n); >> - cfqe->vdisktime = __cfqe->vdisktime + CFQ_IDLE_DELAY; >> - } else >> - cfqe->vdisktime = st->min_vdisktime; >> + while (cfqe) { >> + if (!RB_EMPTY_NODE(&cfqe->rb_node)) >> + return; >> >> - cfq_entity_service_tree_add(st, cfqe); >> + /* >> + * Currently put the group at the end. Later implement >> + * something so that groups get lesser vtime based on >> + * their weights, so that if group does not loose all >> + * if it was not continously backlogged. >> + */ > > Can we use vdisktime boost logic for groups also? I think it can be a separate > patch in the series (the last one). Keeping it as a separate patch will > also help you to coordinate with chad's patch. > >> + st = cfqe->service_tree; > > Group entity set their service tree when they get allocated and retain > this pointer even when they get deleted from serivce tree. Queue entities > seem to have it NULL when they get deleted from service tree and it > gets set again when queue is getting inserted. It would be nice if we > can fix this discrepancy and keep it consistent. I think clearing up > cfqe->service_tree is a better idea and then calculate it again for > group also. Vivek, Currently, cfq queue might change workload type and io class, so we need to recalculate its service_tree. But for cfq groups, IMHO we don't need to add this complexity for the time being. I think we can add this change as soon as different io classes or workload types are introduced. How do you think? Thanks, Gui