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:   Wed, 11 Jan 2023 08:29:20 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Al Viro <viro@zeniv.linux.org.uk>
CC:     Jeff Moyer <jmoyer@redhat.com>,
        "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Benjamin LaHaise <bcrl@kvack.org>, <linux-aio@kvack.org>,
        <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        "Venkataramanan, Anirudh" <anirudh.venkataramanan@intel.com>
Subject: Re: [PATCH v2] fs/aio: Replace kmap{,_atomic}() with
 kmap_local_page()
Message-ID: <Y77j4ItupAhjh76P@iweiny-desk3>
References: <20230109175629.9482-1-fmdefrancesco@gmail.com>
 <Y73+xKXDELSd14p1@ZenIV>
 <x498ri9ma5n.fsf@segfault.boston.devel.redhat.com>
 <Y77dkghufF6GVq1Y@ZenIV>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y77dkghufF6GVq1Y@ZenIV>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?wuq4zA/ipppKiEhLzs9bAxSs0w9O7EktWvgVzSKDzGjvm8cIAz4KK5pqZDnK?=
 =?us-ascii?Q?TFm28xfatGHik9VrPjPyjakEXYjlqDjfOLRGkxbrzJaJzK7nNkJYvDj2sKdV?=
 =?us-ascii?Q?GTwvb9Xtp41RMIKeD1Os8PpVReGjIKfaNSIjj3jYg4FvbJdaoFsbC4E4So/K?=
 =?us-ascii?Q?/e89IetoTRaZVNCSr6PaEhyrz1yFZFDowIJXy5JFXuPbPkfLqbmrKAJDmBYs?=
 =?us-ascii?Q?RBMfGG7gVt1aDgNPkfHsK8w+VXG6N3WYALU6CywPErUKIWS72GNV5JW/59Uz?=
 =?us-ascii?Q?hebhcJq3vYvY4euREHdT+B4IoLHaeEjemTo/CfBngsHgH0y6G+YTTss74t7y?=
 =?us-ascii?Q?fMWnn5Rku3StRfPGsp3yDrJTYM/Y+/FCKxndSrdRkOQUM9+3eZx2K6Hi9Idu?=
 =?us-ascii?Q?Krn+XZi8VXPE/x8rQvQhQ1PtaKV9BdmjBXGjNLJAV5VudTaaADPhkhtvoAuX?=
 =?us-ascii?Q?M30S+PGDIbySC3bm8srCmf9a8OoUXrXxHYDdhxRATdh5fBDm+aXZ7DvBTLTI?=
 =?us-ascii?Q?tcAgwRCZZZh9/idrBQjmuTQ+aHJ+Zf78qkty2R47O8u159QYQxGFy96xzCer?=
 =?us-ascii?Q?lyMKFe/NXMqi9EuQwE1jclhKYRUdp7l+PYcDxN2YrwYY2Rnwr6lWu6s5YGlZ?=
 =?us-ascii?Q?kI0mZcyeIcMqvUVw0REx1XmSsPd9Onspb3HDso0OhYgAp1EjFaap+4anxgfC?=
 =?us-ascii?Q?6ErHY8iV5N2Fo/jroJkum2LfDWhrPCM0UeANMnHsAijULeZ/wtXbbiJId86Q?=
 =?us-ascii?Q?0eKhBkPhW4brR7ezKgJfQBHZeYoin3caLYOoJgSVs2A28yDCm1IcaQOI310N?=
 =?us-ascii?Q?Kp/G/l/k9Fd+qrT+ASVy9Zyerix1ERY3xLalOsUoYLBqkmjcDAujN2yQmTn7?=
 =?us-ascii?Q?WTAZJ9t1YI1AwOWwJXEJ+kcJW9kLFnwRfTMmUuS4IKPdPTaw6TnMI95a4Oos?=
 =?us-ascii?Q?JhLTUfDjkxzDMatcGJ30gBqirVtYni3hWRUnj30FXm/IGS2CJMelxVFAaS+X?=
 =?us-ascii?Q?tPYVxAh6tHKH0ugBm+VM3MI1GkKNXYgcH8XNEVEZhxg1MG9RF0skNiEBSDSL?=
 =?us-ascii?Q?h5P/2nnDoku9IxEqnzjJsqPsHlQiTfl3Hliz2fQ9+NvcI3IKeMNkd6Zv9HLj?=
 =?us-ascii?Q?lK9Dt4+3vNpIicopIzXy66dnzILX8JobW8khmzu+4KqfEETNZaNMlLozScqh?=
 =?us-ascii?Q?XdMtfr8t7TaFP4uoLPdIdTbIMF/8S6cD231aUwOVMA6oqaOU5i/jMcqEnQCr?=
 =?us-ascii?Q?QDtj8c31bsp7BOFql+EpjQAfOKMSxWuCDHYmPPdZcVzoQDuYeziD8jiXgT6v?=
 =?us-ascii?Q?VBpOQg7XlJOmamDE+qiSnRC/W+1JFJLNIM7bzv6gsmuEYNalnzvpVpA3oHez?=
 =?us-ascii?Q?HbSgpviYquU+5HRE5mZHugqBfv+OxDHAY4ZF480VDUnVtJGJ2PbWkBAJhqYb?=
 =?us-ascii?Q?mI+pXU8mY7BSG3A1YKNXTN2bPuPD1iDH30o3dlGNt8SLzDQzru1jCG4WmWMO?=
 =?us-ascii?Q?52XKbOj8B6jo/QGPtfbSpPjme/GnkaGwcHCeczMVzbGxnNv+Eocq56r8jpQM?=
 =?us-ascii?Q?UAlZpcqJzOgCVNCs7dhQrxfGJf1C0/IjI+2tstRE?=
X-MS-Exchange-CrossTenant-Network-Message-Id: afd7db0f-6d8a-4028-705e-08daf3f100e2
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jan 2023 16:29:25.7759
 (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: zA7dATwrOEMctZ+lotdKT/JsbAT6cv7KE42eXZSGzFbOPscGGTgBBc5NXbFJ+J23A0oQauLEvK3nfMr4X/2/KQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB7101
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 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 Wed, Jan 11, 2023 at 04:02:26PM +0000, Al Viro wrote:
> On Wed, Jan 11, 2023 at 09:13:40AM -0500, Jeff Moyer wrote:
> > Hi, Al,
> > 
> > Al Viro <viro@zeniv.linux.org.uk> writes:
> > 
> > > On Mon, Jan 09, 2023 at 06:56:29PM +0100, Fabio M. De Francesco wrote:
> > >
> > >> -	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]);
> > >
> > > I wonder if it would be more readable as memcpy_to_page(), actually...
> > 
> > I'm not sure I understand what you're suggesting.
> 
> 	memcpy_to_page(ctx->ring_pages[0], 0, &(struct aio_ring){
> 			.nr = nr_events, .id = ~0U, .magic = AIO_RING_MAGIC,
> 			.compat_features = AIO_RING_COMPAT_FEATURES,
> 			.in_compat_features = AIO_RING_INCOMPAT_FEATURES,
> 			.header_length = sizeof(struct aio_ring)},
> 			sizeof(struct aio_ring));
> 
> instead of the lines from kmap_atomic to flush_dcache_page...

For us mere mortals I think this may parse easier as:

	struct aio_ring r;

...
	r = (struct aio_ring) {
		.nr = nr_events,
		.id = ~0U,
		.magic = AIO_RING_MAGIC,
		.compat_features = AIO_RING_COMPAT_FEATURES,
		.incompat_features = AIO_RING_INCOMPAT_FEATURES,
		.header_length = sizeof(struct aio_ring),
	};

	memcpy_to_page(ctx->ring_pages[0], 0, &r, sizeof(r));
...

Is there any concern with the extra assignments to the stack only to copy into
the page?  I guess this is not a critical path?

Ira

> > 
> > >>  
> > >>  	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);
> > >
> > > Incidentally, does it need flush_dcache_page()?
> > 
> > Yes, good catch.