From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760168AbcAUUdg (ORCPT ); Thu, 21 Jan 2016 15:33:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59678 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751896AbcAUUdd (ORCPT ); Thu, 21 Jan 2016 15:33:33 -0500 Date: Thu, 21 Jan 2016 15:33:32 -0500 From: Vivek Goyal To: Shaohua Li Cc: linux-kernel@vger.kernel.org, axboe@kernel.dk, tj@kernel.org, jmoyer@redhat.com, Kernel-team@fb.com Subject: Re: [RFC 2/3] blk-throttling: weight based throttling Message-ID: <20160121203332.GD8379@redhat.com> References: <9ea3761d048bf6b5f36c3d57e3eecbce2780b6c3.1453308862.git.shli@fb.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9ea3761d048bf6b5f36c3d57e3eecbce2780b6c3.1453308862.git.shli@fb.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 20, 2016 at 09:49:18AM -0800, Shaohua Li wrote: > We know total bandwidth of a disk and can calculate cgroup's bandwidth > percentage against disk bandwidth according to its weight. We can easily > calculate cgroup bandwidth. > > Signed-off-by: Shaohua Li > --- > block/blk-throttle.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 134 insertions(+), 1 deletion(-) > > diff --git a/block/blk-throttle.c b/block/blk-throttle.c > index 2149a1d..b3f847d 100644 > --- a/block/blk-throttle.c > +++ b/block/blk-throttle.c > @@ -12,6 +12,9 @@ > #include > #include "blk.h" > > +#define MAX_WEIGHT (1000) > +#define WEIGHT_RATIO_SHIFT (12) > +#define WEIGHT_RATIO (1 << WEIGHT_RATIO_SHIFT) > /* Max dispatch from a group in 1 round */ > static int throtl_grp_quantum = 8; > > @@ -74,6 +77,10 @@ struct throtl_service_queue { > unsigned int nr_pending; /* # queued in the tree */ > unsigned long first_pending_disptime; /* disptime of the first tg */ > struct timer_list pending_timer; /* fires on first_pending_disptime */ > + > + unsigned int weight; > + unsigned int children_weight; > + unsigned int ratio; Will it be better to call it "share" instead of "ratio". It is basically a measure of % disk share of the group and share seems more intuitive. [..] > +static void tg_update_bps(struct throtl_grp *tg) > +{ > + struct throtl_service_queue *sq, *parent_sq; > + > + sq = &tg->service_queue; > + parent_sq = sq->parent_sq; > + > + if (!tg->td->weight_based || !parent_sq) > + return; > + sq->ratio = max_t(unsigned int, > + parent_sq->ratio * sq->weight / parent_sq->children_weight, > + 1); > + It might be good to decouple updation of "share/ratio" and updation of bps. Change of share can happen any time either weight is changed or an active group is queue/dequeued and we don't have to do it every time a bio is submitted. > + tg->bps[READ] = max_t(uint64_t, > + (queue_bandwidth(tg->td, READ) * sq->ratio) >> > + WEIGHT_RATIO_SHIFT, > + 1024); > + tg->bps[WRITE] = max_t(uint64_t, > + (queue_bandwidth(tg->td, WRITE) * sq->ratio) >> > + WEIGHT_RATIO_SHIFT, > + 1024); > +} > + > +static void tg_update_ratio(struct throtl_grp *tg) > +{ > + struct throtl_data *td = tg->td; > + struct cgroup_subsys_state *pos_css; > + struct blkcg_gq *blkg; > + > + blkg_for_each_descendant_pre(blkg, pos_css, td->queue->root_blkg) { Is it possible to traverse only the affected subtree instead of whole tree of groups. Because if weight is updated on a group, then we just need to traverse the subtree under that group's parent. [..] > @@ -1415,6 +1546,7 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg, > > sq = &tg->service_queue; > > + tg_update_bps(tg); Updating bps for every bio submitted sounds like a lot. We probably could do it when first bio gets queued in the group and then refresh it at some regular interval. Say when next set of dispatch happens from group we could update bandwidth of group after dispatch. Thanks Vivek