linux-kernel - Re: [PATCH 01/11] mm/ksm: Convert get_ksm

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 for Android: free password hash cracker in your pocket

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

Date: Thu, 21 Mar 2024 10:07:58 +0800
From: Alex Shi <seakeel@...il.com>
To: Matthew Wilcox <willy@...radead.org>, alexs@...nel.org
Cc: Izik Eidus <izik.eidus@...ellosystems.com>,
 Andrea Arcangeli <aarcange@...hat.com>, Hugh Dickins <hughd@...gle.com>,
 Chris Wright <chrisw@...s-sol.org>, kasong@...cent.com,
 Andrew Morton <akpm@...ux-foundation.org>,
 "open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/11] mm/ksm: Convert get_ksm_page to return a folio



On 3/20/24 8:54 PM, Matthew Wilcox wrote:
> On Wed, Mar 20, 2024 at 03:40:37PM +0800, alexs@...nel.org wrote:
>> -static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
>> +static void *get_ksm_page(struct ksm_stable_node *stable_node,
>>  				 enum get_ksm_page_flags flags)
> 
> I am really not a fan of returning void * instead of a page or a
> folio.  Particularly since you rename this function at the end anyway!
> 
> You should do it like this:
> 
> In this patch, convert get_ksm_page() to get_ksm_folio() and add:
> 
> static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
> 		enum get_ksm_page_flags flags)
> {
> 	struct folio *folio = get_ksm_folio(node, flags);
> 	return &folio->page;
> }
> 
> Then convert each call-site to get_ksm_folio(), and finally delete
> get_ksm_page().  That way you're always converting each caller to
> the exact code you want it to look like, and your reiewrs don't have to
> keep three patches in their head at once as they review each place.
> 
> Also, I think this should be ksm_get_folio(), not get_ksm_folio().
> Seems to fit better.
> 
>> @@ -949,32 +949,32 @@ static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
>>  		 * in the ref_freeze section of __remove_mapping(); but Anon
>>  		 * page->mapping reset to NULL later, in free_pages_prepare().
> 
> Could you fix page->mapping to folio->mapping in the comment?
> 

Thanks for comment! I will take your suggestion and resend soon. 

Best regards!