linux-kernel - Re: [PATCH 10/19] io-conroller: Prepare elevator layer for single queue schedulers

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]

Date:	Fri, 12 Jun 2009 08:37:25 +0800
From:	Gui Jianfeng <guijianfeng@...fujitsu.com>
To:	Vivek Goyal <vgoyal@...hat.com>
CC:	linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org, dm-devel@...hat.com,
	jens.axboe@...cle.com, nauman@...gle.com, dpshah@...gle.com,
	lizf@...fujitsu.com, mikew@...gle.com, fchecconi@...il.com,
	paolo.valente@...more.it, ryov@...inux.co.jp,
	fernando@....ntt.co.jp, s-uchida@...jp.nec.com, taka@...inux.co.jp,
	jmoyer@...hat.com, dhaval@...ux.vnet.ibm.com,
	balbir@...ux.vnet.ibm.com, righi.andrea@...il.com,
	m-ikeda@...jp.nec.com, jbaron@...hat.com, agk@...hat.com,
	snitzer@...hat.com, akpm@...ux-foundation.org, peterz@...radead.org
Subject: Re: [PATCH 10/19] io-conroller: Prepare elevator layer for single
 queue schedulers

Vivek Goyal wrote:
> On Thu, Jun 11, 2009 at 04:10:55PM +0800, Gui Jianfeng wrote:
>> Vivek Goyal wrote:
>> ...
>>>  
>>>  /*
>>> @@ -1296,6 +1302,13 @@ void io_group_chain_link(struct request_queue *q, void *key,
>>>  		iog = io_cgroup_lookup_group(iocg, key);
>>>  		io_group_set_parent(prev, iog);
>>>  	}
>>> +
>>> +	if (unlikely(efqd->only_root_group))
>>> +		/*
>>> +		 * TODO: Take care of force expiry of existing queue before
>>> +		 * new queue is queued.
>>> +		 */
>>> +		efqd->only_root_group = 0;
>>   Hi Vivek,
>>
>>   This flag isn't set back when all child groups go away. Am i missing something?
>>   BTW, why not just determine "only root group" by cgroup itself? Although there might be 
>>   some impact if cgroup is built but no request goes into it. but i think this isn't a big
>>   deal. How about the following patch?
>>
> 
> Hi Gui,
> 
> Determining if there are any children present or not from cgroup sounds like
> a good idea. Just that cost of the operation now has increased. I am not
> sure how significant that is. But for the time being we can stick to your
> implementation. 

  I don't introduce any extra locking here, so i guess the cost is very limited.

> 
> One question inline below.
> 
>> Signed-off-by: Gui Jianfeng <guijianfeng@...fujitsu.com>
>> ---
>>  block/elevator-fq.c |   21 ++++++++++-----------
>>  block/elevator-fq.h |    1 -
>>  2 files changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/elevator-fq.c b/block/elevator-fq.c
>> index a516dce..f33155c 100644
>> --- a/block/elevator-fq.c
>> +++ b/block/elevator-fq.c
>> @@ -76,7 +76,6 @@ void elv_del_ioq_busy(struct elevator_queue *e, struct io_queue *ioq,
>>  void elv_activate_ioq(struct io_queue *ioq, int add_front);
>>  void elv_deactivate_ioq(struct elv_fq_data *efqd, struct io_queue *ioq,
>>  					int requeue);
>> -
>>  static int bfq_update_next_active(struct io_sched_data *sd)
>>  {
>>  	struct io_group *iog;
>> @@ -1131,6 +1130,14 @@ struct io_cgroup io_root_cgroup = {
>>  	.ioprio_class = IO_DEFAULT_GRP_CLASS,
>>  };
>>  
>> +static int is_only_root_group(void)
>> +{
>> +	if (list_empty(&io_root_cgroup.css.cgroup->children))
>> +		return 1;
>> +
> 
> Do we need some kind of locking here to make sure cgroup->children list is not
> being modified?

  Even if the children list is modified, i think this is not a big problem, and just
  get a mis-judgement for one time. Anyway, children list changing is rarely happens.
  For this corner case, IMHO, there's no need to introduce the cgroup lock, for this 
  lock costs too much.


-- 
Regards
Gui Jianfeng

--
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/