linux-kernel - Re: [PATCH v8 01/10] fs: Allow fine-grained control of folio sizes

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: <4b6ca560-03e7-4fd2-8b4b-acfc75bdb925@arm.com>
Date: Thu, 4 Jul 2024 16:52:13 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Matthew Wilcox <willy@...radead.org>
Cc: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>, david@...morbit.com,
 chandan.babu@...cle.com, djwong@...nel.org, brauner@...nel.org,
 akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
 yang@...amperecomputing.com, linux-mm@...ck.org, john.g.garry@...cle.com,
 linux-fsdevel@...r.kernel.org, hare@...e.de, p.raghav@...sung.com,
 mcgrof@...nel.org, gost.dev@...sung.com, cl@...amperecomputing.com,
 linux-xfs@...r.kernel.org, hch@....de, Zi Yan <zi.yan@...t.com>
Subject: Re: [PATCH v8 01/10] fs: Allow fine-grained control of folio sizes

On 04/07/2024 16:20, Matthew Wilcox wrote:
> On Thu, Jul 04, 2024 at 01:23:20PM +0100, Ryan Roberts wrote:
>>> -	AS_LARGE_FOLIO_SUPPORT = 6,
>>
>> nit: this removed enum is still referenced in a comment further down the file.
> 
> Thanks.  Pankaj, let me know if you want me to send you a patch or if
> you'll do it directly.
> 
>>> +	/* Bits 16-25 are used for FOLIO_ORDER */
>>> +	AS_FOLIO_ORDER_BITS = 5,
>>> +	AS_FOLIO_ORDER_MIN = 16,
>>> +	AS_FOLIO_ORDER_MAX = AS_FOLIO_ORDER_MIN + AS_FOLIO_ORDER_BITS,
>>
>> nit: These 3 new enums seem a bit odd.
> 
> Yes, this is "too many helpful suggestions" syndrome.  It made a lot
> more sense originally.

Well now you can add my "helpful" suggestion to that list :)

> 
> https://lore.kernel.org/linux-fsdevel/ZlUQcEaP3FDXpCge@dread.disaster.area/
> 
>>> +static inline void mapping_set_folio_order_range(struct address_space *mapping,
>>> +						 unsigned int min,
>>> +						 unsigned int max)
>>> +{
>>> +	if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
>>> +		return;
>>> +
>>> +	if (min > MAX_PAGECACHE_ORDER)
>>> +		min = MAX_PAGECACHE_ORDER;
>>> +	if (max > MAX_PAGECACHE_ORDER)
>>> +		max = MAX_PAGECACHE_ORDER;
>>> +	if (max < min)
>>> +		max = min;
>>
>> It seems strange to silently clamp these? Presumably for the bs>ps usecase,
>> whatever values are passed in are a hard requirement? So wouldn't want them to
>> be silently reduced. (Especially given the recent change to reduce the size of
>> MAX_PAGECACHE_ORDER to less then PMD size in some cases).
> 
> Hm, yes.  We should probably make this return an errno.  Including
> returning an errno for !IS_ENABLED() and min > 0.

Right.

> 
>>> -	if (new_order < MAX_PAGECACHE_ORDER) {
>>> +	if (new_order < mapping_max_folio_order(mapping)) {
>>>  		new_order += 2;
>>> -		new_order = min_t(unsigned int, MAX_PAGECACHE_ORDER, new_order);
>>> +		new_order = min(mapping_max_folio_order(mapping), new_order);
>>>  		new_order = min_t(unsigned int, new_order, ilog2(ra->size));
>>
>> I wonder if its possible that ra->size could ever be less than
>> mapping_min_folio_order()? Do you need to handle that?
> 
> I think we take care of that in later patches? 

Yes I saw that once I got to it. You can ignore this.

> This patch is mostly
> about honouring the max properly and putting in the infrastructure for
> the min, but not doing all the necessary work for min.