linux-kernel - Re: [PATCH 07/20] blkio: Provide capablity to enqueue/dequeue group entities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091104165434.GD2870@redhat.com>
Date:	Wed, 4 Nov 2009 11:54:34 -0500
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Jeff Moyer <jmoyer@...hat.com>
Cc:	linux-kernel@...r.kernel.org, jens.axboe@...cle.com,
	nauman@...gle.com, dpshah@...gle.com, lizf@...fujitsu.com,
	ryov@...inux.co.jp, fernando@....ntt.co.jp, s-uchida@...jp.nec.com,
	taka@...inux.co.jp, guijianfeng@...fujitsu.com,
	balbir@...ux.vnet.ibm.com, righi.andrea@...il.com,
	m-ikeda@...jp.nec.com, akpm@...ux-foundation.org, riel@...hat.com,
	kamezawa.hiroyu@...fujitsu.com
Subject: Re: [PATCH 07/20] blkio: Provide capablity to enqueue/dequeue
	group entities

On Wed, Nov 04, 2009 at 10:34:57AM -0500, Jeff Moyer wrote:
> Vivek Goyal <vgoyal@...hat.com> writes:
> 
> > o This patch embeds cfq_entity object in cfq_group and provides helper routines
> >   so that group entities can be scheduled.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
> > ---
> >  block/cfq-iosched.c |  110 +++++++++++++++++++++++++++++++++++++++++++--------
> >  1 files changed, 93 insertions(+), 17 deletions(-)
> >
> > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> > index bc99163..8ec8a82 100644
> > --- a/block/cfq-iosched.c
> > +++ b/block/cfq-iosched.c
> > @@ -79,6 +79,7 @@ struct cfq_service_tree {
> >  #define CFQ_RB_ROOT	(struct cfq_service_tree) { RB_ROOT, NULL, 0, NULL}
> >  
> >  struct cfq_sched_data {
> > +	unsigned int nr_active;
> >  	struct cfq_service_tree service_tree[IO_IOPRIO_CLASSES];
> >  };
> >  
> > @@ -89,6 +90,10 @@ struct cfq_entity {
> >  	struct cfq_service_tree *st;
> >  	unsigned short ioprio_class;
> >  	bool ioprio_class_changed;
> > +	struct cfq_entity *parent;
> > +	bool on_st;
> > +	/* Points to the sched_data of group entity. Null for cfqq */
> > +	struct cfq_sched_data *my_sd;
> 
> Why my_sd?  None of the other members required a my.  ;-)
> 

Because in rest of the code, we have been using "struct cfq_sched_data *sd"
to mean pointer to scheduling data entity/queue/group is queued. 

In this case entity is embedded inside a cfq_group object and it is
hosting a sched_data tree. So "*sd" means the sched_data the entity is
queued on and "*my_sd" means the sched_data entity is hosting.

> > +static inline struct cfq_entity *parent_entity(struct cfq_entity *cfqe)
> > +{
> > +	return cfqe->parent;
> > +}
> 
> Wow, is this really necessary for a pointer dereference?
> 

I guess no. As core scheduling code is inspired from CFS, it is coming
from there. :-). Will get rid of it in next posting.

> > +#ifdef CONFIG_CFQ_GROUP_IOSCHED
> > +/* check for entity->parent so that loop is not executed for root entity. */
> > +#define for_each_entity(entity)	\
> > +	for (; entity && entity->parent; entity = entity->parent)
> 
> See, you don't use it here

:-)

> 
> > @@ -660,7 +723,13 @@ static void enqueue_cfqe(struct cfq_entity *cfqe)
> >  
> >  static void enqueue_cfqq(struct cfq_queue *cfqq)
> >  {
> > -	enqueue_cfqe(&cfqq->entity);
> > +	struct cfq_entity *cfqe = &cfqq->entity;
> > +
> > +	for_each_entity(cfqe) {
> > +		if (cfqe->on_st)
> > +			break;
> > +		enqueue_cfqe(cfqe);
> > +	}
> >  }
> 
> Maybe I'm slow, but I would have benefitted from a comment above that
> function stating that we have to walk up the tree to make sure that the
> parent is also scheduled.

Sure, will put a comment here.

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/