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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ZfrcihNz74lDDg1J@casper.infradead.org>
Date: Wed, 20 Mar 2024 12:54:34 +0000
From: Matthew Wilcox <willy@...radead.org>
To: 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 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?