Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 30 Jul 2022 18:40:54 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
CC:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Benjamin LaHaise <bcrl@kvack.org>,
        <linux-fsdevel@vger.kernel.org>, <linux-aio@kvack.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] aio: Replace kmap{,_atomic}() with kmap_local_page()
Message-ID: <YuXdprlqIFF1uCUS@iweiny-desk3>
References: <20220706233328.18582-1-fmdefrancesco@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220706233328.18582-1-fmdefrancesco@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?5A4/A2wLHUqnm9DDXeFgauGOc/cWOVxKod6sJq8XHbGdmeE7Ll3L4baEOuIQ?=
 =?us-ascii?Q?bi99LJ6+fIgsjaEftvQLmxc9ho1GcKIAcD7pvX5Jh23Xq9ym4XdvR8mx1EuN?=
 =?us-ascii?Q?EFJGaccVIG+fr4wGURb4t4Y27XjpJ6eO48nRGfgIpoQtDfsDfpsjwr+G4ah7?=
 =?us-ascii?Q?PL6LsIvMnLSNBd9NXB+hZFDATi22gIyq4+EyT1Zd33NUG25i76/kVsdyZI9V?=
 =?us-ascii?Q?t3ZZEv2HJdj4iAok3z0PiyNgHh21b4sRiz57IOvSm/wKynjr+cZx8VxBNUxK?=
 =?us-ascii?Q?foBlkG4uKx29Gj8eInBNfZjahKyGnJAtjVYstZpYNaxWdtdlwB/ned7MHarM?=
 =?us-ascii?Q?/6mff+FaSvQQSEhbbv64Zi16T78Mhm4jZVvcAikQ+qkInpylciGCiDahyxCg?=
 =?us-ascii?Q?99D93ITuPGldQmCJorPYKTZKdwrzzLTui4TUysXQ482HRzD7axQZge6tXFcn?=
 =?us-ascii?Q?6e2WxZCtw0iXnZyW+ebhZ4whTBu1fJmMlWPQNPxHIqkgG3iv9DfpbwPBluVE?=
 =?us-ascii?Q?pqiNbnb+gycMXegmXXhWPN6c7VmFU2Q2QSff7PucjzIL7vUc7P/haD42uwB5?=
 =?us-ascii?Q?s3gMWy3r+/DnlGntJCfBeG8ejLOk4Uxa29xObb0IY1lE/ti+oOqx7H8ZlFjB?=
 =?us-ascii?Q?ePH5TcmrOXYRpV+tOQysxQl/5u5m7ZrRvD5g378aNRKOxzmwSRdym/kta9Zg?=
 =?us-ascii?Q?2S+Yv96x13HpXFhjrxgsBmNEaQorpc9s/WdtwaYKgIdhvCr6QZHUecg8xOjU?=
 =?us-ascii?Q?eXDlFjJNL1C6Ou6GlHZoWAjE+8ABq6PCVWC7lojfcnTmoZUbibfc6HQXMgx6?=
 =?us-ascii?Q?2wLnILaZSBvLy3DvAeBug04C/BtLbLZKpW7y30hXGgJsfPSS88yTBEQbdWSd?=
 =?us-ascii?Q?reNUNqS7NrB4LtFaad6MzqiCGGuEcpQnFkxZEBMZ2RaRjF9xYuxB7ybnfmJq?=
 =?us-ascii?Q?JscxatEBMn5YdAjJyD2QEwDjC97T2r5KOKsSWlORtfot1cUlSrzwQ3GuhYOT?=
 =?us-ascii?Q?66bfxDAhiYvKqTBUKDz14xWsT1hdp5C3aSfpsEXQ0QJy4N1kXju/QaHQJ67W?=
 =?us-ascii?Q?RoZXd4CLoo/dgYJmoCeX/N25OS72eSn/TeYwfl4X5B4a7b5nBqqecJGaQyZo?=
 =?us-ascii?Q?2LRbkoBAjgza1V/J3VfnOpo6i4kxDD28buYeUDqc6giBeHz3sDyuDqBoDNFN?=
 =?us-ascii?Q?eVgnOKZ6uz9QwTTnF0hkGTbQxVkJtgh2d+4r8CQsd99mTJ2FaJiXrRGfVRT4?=
 =?us-ascii?Q?osV8m0BcSUcD8rF5ryzNqm22dGqGnG663Hwsuz59ccWp//j/V9HZcPuE0uAE?=
 =?us-ascii?Q?Oru7EK7FwwSIePd5BegVruGdQ6JhE9/D1Z0tRwh6Sutt1q+vFtxyf2yVJOrS?=
 =?us-ascii?Q?LPtVffhZg0sdcWnh5XxRmsUbshQ/NZnhETut1jzy2dwskucQRDuVdYFi0E53?=
 =?us-ascii?Q?iGqvLrhXD60MIffBlVfj9QeVacVZYSvysstJenl1fZ/iJrkW5iPd5r6jZJFe?=
 =?us-ascii?Q?pgyWXpogZ+cwbSlLEMQrSVcoJW2nVE+CQZHHE3zq7onWyTmljyYSExyAB5bm?=
 =?us-ascii?Q?gUFNbuh5kVNpcs1GnHzfVpny1UJH9ODBdFJU33wH?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 20d09680-500a-4e8e-2daf-08da7295b86e
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jul 2022 01:40:59.7518
 (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: fga5kkX63TfK6P6LCwq9/xkl4of2XrOplVXg5K6pZWYMBlgrsJqAPC1iQ8lL2muJNiy78LQPNJsw6hObijHglg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR11MB4392
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jul 07, 2022 at 01:33:28AM +0200, Fabio M. De Francesco wrote:
> The use of kmap() and kmap_atomic() are being deprecated in favor of
> kmap_local_page().
> 
> With kmap_local_page(), the mappings are per thread, CPU local and not
> globally visible. Furthermore, the mappings can be acquired from any
> context (including interrupts).
> 
> Therefore, use kmap_local_page() in aio.c because these mappings are per
> thread, CPU local, and not globally visible.
> 
> Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with
> HIGHMEM64GB enabled.
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

I'm not sure why this slipped by for so long.

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

> ---
> 
> I've tested with "./check -g aio". The tests in this group fail 3/26
> times, with and without my patch. Therefore, these changes don't introduce
> further errors.
> 
>  fs/aio.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 3c249b938632..343fea0c6d1a 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -567,7 +567,7 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events)
>  	ctx->user_id = ctx->mmap_base;
>  	ctx->nr_events = nr_events; /* trusted copy */
>  
> -	ring = kmap_atomic(ctx->ring_pages[0]);
> +	ring = kmap_local_page(ctx->ring_pages[0]);
>  	ring->nr = nr_events;	/* user copy */
>  	ring->id = ~0U;
>  	ring->head = ring->tail = 0;
> @@ -575,7 +575,7 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events)
>  	ring->compat_features = AIO_RING_COMPAT_FEATURES;
>  	ring->incompat_features = AIO_RING_INCOMPAT_FEATURES;
>  	ring->header_length = sizeof(struct aio_ring);
> -	kunmap_atomic(ring);
> +	kunmap_local(ring);
>  	flush_dcache_page(ctx->ring_pages[0]);
>  
>  	return 0;
> @@ -678,9 +678,9 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
>  					 * we are protected from page migration
>  					 * changes ring_pages by ->ring_lock.
>  					 */
> -					ring = kmap_atomic(ctx->ring_pages[0]);
> +					ring = kmap_local_page(ctx->ring_pages[0]);
>  					ring->id = ctx->id;
> -					kunmap_atomic(ring);
> +					kunmap_local(ring);
>  					return 0;
>  				}
>  
> @@ -1024,9 +1024,9 @@ static void user_refill_reqs_available(struct kioctx *ctx)
>  		 * against ctx->completed_events below will make sure we do the
>  		 * safe/right thing.
>  		 */
> -		ring = kmap_atomic(ctx->ring_pages[0]);
> +		ring = kmap_local_page(ctx->ring_pages[0]);
>  		head = ring->head;
> -		kunmap_atomic(ring);
> +		kunmap_local(ring);
>  
>  		refill_reqs_available(ctx, head, ctx->tail);
>  	}
> @@ -1132,12 +1132,12 @@ static void aio_complete(struct aio_kiocb *iocb)
>  	if (++tail >= ctx->nr_events)
>  		tail = 0;
>  
> -	ev_page = kmap_atomic(ctx->ring_pages[pos / AIO_EVENTS_PER_PAGE]);
> +	ev_page = kmap_local_page(ctx->ring_pages[pos / AIO_EVENTS_PER_PAGE]);
>  	event = ev_page + pos % AIO_EVENTS_PER_PAGE;
>  
>  	*event = iocb->ki_res;
>  
> -	kunmap_atomic(ev_page);
> +	kunmap_local(ev_page);
>  	flush_dcache_page(ctx->ring_pages[pos / AIO_EVENTS_PER_PAGE]);
>  
>  	pr_debug("%p[%u]: %p: %p %Lx %Lx %Lx\n", ctx, tail, iocb,
> @@ -1151,10 +1151,10 @@ static void aio_complete(struct aio_kiocb *iocb)
>  
>  	ctx->tail = tail;
>  
> -	ring = kmap_atomic(ctx->ring_pages[0]);
> +	ring = kmap_local_page(ctx->ring_pages[0]);
>  	head = ring->head;
>  	ring->tail = tail;
> -	kunmap_atomic(ring);
> +	kunmap_local(ring);
>  	flush_dcache_page(ctx->ring_pages[0]);
>  
>  	ctx->completed_events++;
> @@ -1214,10 +1214,10 @@ static long aio_read_events_ring(struct kioctx *ctx,
>  	mutex_lock(&ctx->ring_lock);
>  
>  	/* Access to ->ring_pages here is protected by ctx->ring_lock. */
> -	ring = kmap_atomic(ctx->ring_pages[0]);
> +	ring = kmap_local_page(ctx->ring_pages[0]);
>  	head = ring->head;
>  	tail = ring->tail;
> -	kunmap_atomic(ring);
> +	kunmap_local(ring);
>  
>  	/*
>  	 * Ensure that once we've read the current tail pointer, that
> @@ -1249,10 +1249,10 @@ static long aio_read_events_ring(struct kioctx *ctx,
>  		avail = min(avail, nr - ret);
>  		avail = min_t(long, avail, AIO_EVENTS_PER_PAGE - pos);
>  
> -		ev = kmap(page);
> +		ev = kmap_local_page(page);
>  		copy_ret = copy_to_user(event + ret, ev + pos,
>  					sizeof(*ev) * avail);
> -		kunmap(page);
> +		kunmap_local(ev);
>  
>  		if (unlikely(copy_ret)) {
>  			ret = -EFAULT;
> @@ -1264,9 +1264,9 @@ static long aio_read_events_ring(struct kioctx *ctx,
>  		head %= ctx->nr_events;
>  	}
>  
> -	ring = kmap_atomic(ctx->ring_pages[0]);
> +	ring = kmap_local_page(ctx->ring_pages[0]);
>  	ring->head = head;
> -	kunmap_atomic(ring);
> +	kunmap_local(ring);
>  	flush_dcache_page(ctx->ring_pages[0]);
>  
>  	pr_debug("%li  h%u t%u\n", ret, head, tail);
> -- 
> 2.36.1
>