Received-SPF: pass (google.com: domain of linux-kernel+bounces-7401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 20 Dec 2023 11:53:34 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>,
	"Andrew Morton" <akpm@linux-foundation.org>, <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>, Ira Weiny <ira.weiny@intel.com>
CC: Ira Weiny <ira.weiny@intel.com>
Subject: Re: [PATCH] mm/memory: Replace kmap() with kmap_local_page()
Message-ID: <6583463e8de5c_30a94294fb@iweiny-mobl.notmuch>
References: <20231215084417.2002370-1-fabio.maria.de.francesco@linux.intel.com>
 <657fbdb5db945_126a129483@iweiny-mobl.notmuch>
 <4255260.irdbgypaU6@fdefranc-mobl3>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <4255260.irdbgypaU6@fdefranc-mobl3>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1A+Js4ucjDmMEJ/Q8jAz21viY4eukhXhM+/+abW/xuASjwE6vUjAmA+0BlBp?=
 =?us-ascii?Q?nYwg7Z20a7saF7mJIJdv4KfnELVFmSQk1vx5JDllsfScnAl0dU9AIdj4D28r?=
 =?us-ascii?Q?dX3uutV4FzvunykH9ekVRJQuqXFRXzN2Td+nQSYnUx1i2DWf6l/hU3nbxcKs?=
 =?us-ascii?Q?kjEy7QTNjhriq9Sbr5YrLG5r62sNQ8zdozpVWGH1hfqIdIPCZIq59UPvIyt6?=
 =?us-ascii?Q?og77OCl55JosFSjtCpRwZUENu1+phlISANYuVbu8BYhi9ev5r+fECWPL/IJJ?=
 =?us-ascii?Q?/TXkHQX2Tg5+xDYSGrrnkpI/h7NbLrSpg6/gjrfmeTx1+LI2BBi2ZxPB2dTu?=
 =?us-ascii?Q?9ynMtoBj6wI8aWuNmeCpG83jxmDLBWYIp/li8iO1PgrHScK1PcP7NDbX9F6H?=
 =?us-ascii?Q?hsP29apqV17q0R9ogzkkmyx4qTUqI2ro2VwuKbSArKmFe6cfpM4nT/vCMnMa?=
 =?us-ascii?Q?1ACfsA+T4ulwFTCg1jmGvhSFR4YKsIq4Mi6cPasV7tZFWwhDNpbcv7Yfc/xB?=
 =?us-ascii?Q?LKOYvHEsWszuJg2i9fVMWsxhdfHID19++9EO9ifH1/lRLNqoDE2Sc1xxGPfn?=
 =?us-ascii?Q?w1yp2l7LFt9r0vwaYCI7ow0Vb69Vtj6ODgwJrG9/ZUtg1U2OLE4AEJ/1yxub?=
 =?us-ascii?Q?iYyUpRDcBFuYvYpWhI0jXk1rEGmjWYgqJ1PLBi56hjynMcAVKkmBCO2ZJM4s?=
 =?us-ascii?Q?kLP3iVnrecqfNdx6YEa8CIpeAIU/Aj4g/42atwWQvHzqmBe6h3dTzNKO6acC?=
 =?us-ascii?Q?skEJfUla6MD9qfh7euHK2VXSKAMPZF6wV19jY41qE6kN0A7HxE8zKJ3HNs8h?=
 =?us-ascii?Q?Qe+vJEC1HqgKVTJkknQdE2WjyD6x63313prbXjh9zpNvUigPuGbN27ZOfviw?=
 =?us-ascii?Q?brzwAnN+Y2d1JEhiJCGaoUXNvRVszj/30Q8r74vDhaDvwIQaStm9hFttc/xd?=
 =?us-ascii?Q?iZTKiAOSeZm3Zr9inyMSBF2JZaslbwLgxFOH2Z4v3Fjzj557Ty90sMCvcKVP?=
 =?us-ascii?Q?vgyj7rXRSu+okUul2P6s6nuhJ2YPtou+VGzWiFzbTiCUF9DFGAEnFuPvMp2l?=
 =?us-ascii?Q?QKxXgvCblq48CJFkrqoN7MOZiktImnmEe8YkKQsa3EsbBKvubvMJvHzjHRBs?=
 =?us-ascii?Q?j22ko/mzSNwBF36XXT0WwxHwg3jJIa1oVjMlEcSLDB6y4CQwzuf/DiKflpm5?=
 =?us-ascii?Q?yFOLLk/ua9c6IXD+dIsVFrJLt8cN0SCHl+mMEjQVByYVX5zP7xPg/Bok6Uyb?=
 =?us-ascii?Q?2gLN+ohCxpIlbABbqIP4n0WVhASXzBCrXfdzAzHludiSwYIV806EQCyWReYr?=
 =?us-ascii?Q?nL+vT2xJWhxO5gPR3AQ3tEFJYZQuiFo5Cu9NE9W65CWd/XGMmg8Uxp2JLrpK?=
 =?us-ascii?Q?tL52zJ3NkeOTWQOnjwi5/rNV/pN/5u3oc34b6MCLYn4K8XvRLx9rIQfD1xot?=
 =?us-ascii?Q?dV5WsTfWBrf03e6sLLQWAdPrxSb6EEgrOXXGDsj2TWP+E4UJ1W5+ryy+r7p+?=
 =?us-ascii?Q?Bo/20Xjblcw4k6WfIEHDb+REQXqiUFhYVU6qJaIldlpYc5I71sgVbVezJDmT?=
 =?us-ascii?Q?Yw60nhPJ1oyd2pEQj8O9QJXcTAf2xmynOIOsJEwfydeNP9GAw/FqAc5Lz8oV?=
 =?us-ascii?Q?XEq+qVZppU3tBqtTk8L3H4wZkf8VJcf7cUpKbuNH5Col?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bb85b2b8-d30b-4e89-c399-08dc01955cd0
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2023 19:53:40.0769
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yh06Whh52GhfizYth6IC8xZbZC4Dp/S6ptwYAWE+057O42Sg2y0xYyPrPJbqmepjNS8c13tZehqSuxKs0nI8TQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR11MB8079
X-OriginatorOrg: intel.com

Fabio M. De Francesco wrote:
> On Monday, 18 December 2023 04:34:13 CET Ira Weiny wrote:
> > Fabio M. De Francesco wrote:
> > 
> > [snip]
> > 
> > > Cc: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Fabio M. De Francesco
> > > <fabio.maria.de.francesco@linux.intel.com> ---
> > > 
> > >  mm/memory.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 7d9f6b685032..88377a107fbe 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -5852,7 +5852,7 @@ static int __access_remote_vm(struct mm_struct *mm,
> > > unsigned long addr,> 
> > >  			if (bytes > PAGE_SIZE-offset)
> > >  			
> > >  				bytes = PAGE_SIZE-offset;
> > > 
> > > -			maddr = kmap(page);
> > > +			maddr = kmap_local_page(page);
> > > 
> > >  			if (write) {
> > >  			
> > >  				copy_to_user_page(vma, page, addr,
> > >  				
> > >  						  maddr + offset, buf, 
> bytes);
> > > 
> > > @@ -5861,8 +5861,7 @@ static int __access_remote_vm(struct mm_struct *mm,
> > > unsigned long addr,> 
> > >  				copy_from_user_page(vma, page, addr,
> > >  				
> > >  						    buf, maddr + offset, 
> bytes);
> > >  			
> > >  			}
> > > 
> > > -			kunmap(page);
> > > -			put_page(page);
> > > +			unmap_and_put_page(page, maddr);
> > 
> > Does this really have the same functionality?
> > 
> > Ira
> 
> Do you have any specific reasons to say that? 
> 
> The unmap_and_put_page() helper was created by Al Viro (it initially was 
> put_and_unmap_page() and I sent a patch to rename it to the current name). He 
> noticed that we have lots of kunmap_local() followed by put_page(). 
> 
> The current implementation has then been changed (Matthew did it, if I 
> remember correctly).
> 
> My understanding of the current implementation is that unmap_and_put_page() 
> calls folio_release_kmap(), taking as arguments the folio which the page 
> belongs to and the kernel virtual address returned by kmap_local_page().
> 
> folio_release_kmap() calls kunmap_local() and then folio_put(). The last is 
> called on the folio obtained by the unmap_and_put_page() wrapper and, if I'm 
> not wrong, it releases refcounts on folios like put_page() does on pages.

This is where my consternation came from.  I saw the folio_put() and did
not realize that get_page() now calls folio_get().

> 
> Am I missing something?

Nope, I just did not have time to trace code yesterday.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>