linux-kernel - Re: [PATCH 08/18] io-controller: idle for sometime on sync queue before expiring it

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] [day] [month] [year] [list]

Message-ID: <4A305C52.7040804@cn.fujitsu.com>
Date:	Thu, 11 Jun 2009 09:22:26 +0800
From:	Gui Jianfeng <guijianfeng@...fujitsu.com>
To:	Vivek Goyal <vgoyal@...hat.com>
CC:	nauman@...gle.com, dpshah@...gle.com, lizf@...fujitsu.com,
	mikew@...gle.com, fchecconi@...il.com, paolo.valente@...more.it,
	jens.axboe@...cle.com, 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,
	linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org, righi.andrea@...il.com,
	agk@...hat.com, dm-devel@...hat.com, snitzer@...hat.com,
	m-ikeda@...jp.nec.com, akpm@...ux-foundation.org
Subject: Re: [PATCH 08/18] io-controller: idle for sometime on sync queue
 before expiring it

Vivek Goyal wrote:
> On Wed, Jun 10, 2009 at 09:30:38AM +0800, Gui Jianfeng wrote:
>> Vivek Goyal wrote:
>>> On Tue, Jun 09, 2009 at 03:56:38PM +0800, Gui Jianfeng wrote:
>>>> Vivek Goyal wrote:
>>>> ...
>>>>> +ssize_t elv_fairness_store(struct request_queue *q, const char *name,
>>>>> +			  size_t count)
>>>>> +{
>>>>> +	struct elv_fq_data *efqd;
>>>>> +	unsigned int data;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	char *p = (char *)name;
>>>>> +
>>>>> +	data = simple_strtoul(p, &p, 10);
>>>>> +
>>>>> +	if (data < 0)
>>>>> +		data = 0;
>>>>> +	else if (data > INT_MAX)
>>>>> +		data = INT_MAX;
>>>>   Hi Vivek,
>>>>
>>>>   data might overflow on 64 bit systems. In addition, since "fairness" is nothing 
>>>>   more than a switch, just let it be.
>>>>
>>>> Signed-off-by: Gui Jianfeng <guijianfeng@...fujitsu.com>
>>>> ---
>>> Hi Gui,
>>>
>>> How about following patch? Currently this should apply at the end of the
>>> patch series. If it looks good, I will merge the changes in higher level
>>> patches.
>>   This patch seems good to me. Some trivial issues comment below.
>>
>>> Thanks
>>> Vivek
>>>
>>> o Previously common layer elevator parameters were appearing as request
>>>   queue parameters in sysfs. But actually these are io scheduler parameters
>>>   in hiearchical mode. Fix it.
>>>
>>> o Use macros to define multiple sysfs C functions doing the same thing. Code
>>>   borrowed from CFQ. Helps reduce the number of lines of by 140.
>>>
>>> Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
>> ...	\
>>> +}
>>> +SHOW_FUNCTION(elv_fairness_show, efqd->fairness, 0);
>>> +EXPORT_SYMBOL(elv_fairness_show);
>>> +SHOW_FUNCTION(elv_slice_idle_show, efqd->elv_slice_idle, 1);
>>> +EXPORT_SYMBOL(elv_slice_idle_show);
>>> +SHOW_FUNCTION(elv_async_slice_idle_show, efqd->elv_async_slice_idle, 1);
>>> +EXPORT_SYMBOL(elv_async_slice_idle_show);
>>> +SHOW_FUNCTION(elv_slice_sync_show, efqd->elv_slice[1], 1);
>>> +EXPORT_SYMBOL(elv_slice_sync_show);
>>> +SHOW_FUNCTION(elv_slice_async_show, efqd->elv_slice[0], 1);
>>> +EXPORT_SYMBOL(elv_slice_async_show);
>>> +#undef SHOW_FUNCTION
>>> +
>>> +#define STORE_FUNCTION(__FUNC, __PTR, MIN, MAX, __CONV)			\
>>> +ssize_t __FUNC(struct elevator_queue *e, const char *page, size_t count)	\
>>> +{									\
>>> +	struct elv_fq_data *efqd = &e->efqd;				\
>>> +	unsigned int __data;						\
>>> +	int ret = elv_var_store(&__data, (page), count);		\
>>   Since simple_strtoul returns unsigned long, it's better to make __data 
>>   be that type.
>>
> 
> I just took it from CFQ. BTW, what's the harm here in truncating unsigned
> long to int? Anyway for our variables we are not expecting any value 
> bigger than unsigned int and if it is, we expect to truncate it?
> 
>>> +	if (__data < (MIN))						\
>>> +		__data = (MIN);						\
>>> +	else if (__data > (MAX))					\
>>> +		__data = (MAX);						\
>>> +	if (__CONV)							\
>>> +		*(__PTR) = msecs_to_jiffies(__data);			\
>>> +	else								\
>>> +		*(__PTR) = __data;					\
>>> +	return ret;							\
>>> +}
>>> +STORE_FUNCTION(elv_fairness_store, &efqd->fairness, 0, 1, 0);
>>> +EXPORT_SYMBOL(elv_fairness_store);
>>> +STORE_FUNCTION(elv_slice_idle_store, &efqd->elv_slice_idle, 0, UINT_MAX, 1);
>>   Do we need to set an actual max limitation rather than UINT_MAX for these entries?
> 
> Again these are the same values CFQ was using.  Do you have a better upper
> limit in mind? Until and unless there is strong objection to UINT_MAX, we
> can stick to what CFQ has been doing so far.

  Ok, I don't have strong opinion about the above things.

> 
> Thanks
> Vivek
> 
> 
> 

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