From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5E00BC433F2 for ; Mon, 27 Jul 2020 08:42:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id EB65C206E7 for ; Mon, 27 Jul 2020 08:42:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="GqIWWP15" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EB65C206E7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 462C76B0002; Mon, 27 Jul 2020 04:42:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 413976B0003; Mon, 27 Jul 2020 04:42:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B38E6B0005; Mon, 27 Jul 2020 04:42:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0009.hostedemail.com [216.40.44.9]) by kanga.kvack.org (Postfix) with ESMTP id 11F0B6B0002 for ; Mon, 27 Jul 2020 04:42:39 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 9BA05180AD81F for ; Mon, 27 Jul 2020 08:42:38 +0000 (UTC) X-FDA: 77083214796.12.milk76_0f0a9fb26f5f Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin12.hostedemail.com (Postfix) with ESMTP id 6E4C8180555DE for ; Mon, 27 Jul 2020 08:42:38 +0000 (UTC) X-HE-Tag: milk76_0f0a9fb26f5f X-Filterd-Recvd-Size: 8052 Received: from esa2.hc3370-68.iphmx.com (esa2.hc3370-68.iphmx.com [216.71.145.153]) by imf40.hostedemail.com (Postfix) with ESMTP for ; Mon, 27 Jul 2020 08:42:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1595839358; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=2o9INdDVMeRMpvPPdJXUmq8eiLdRA5WJoeGCeiRGYrU=; b=GqIWWP15K8qlV7kIHrwZuOTUfnKXg3sPU5i6B7R6EGMf6A8u+FsksD4A FPvtL70VgMBhGGfMhmWNXoqLZIR5TX9zOoyIEIfyZl2LzAHvmIEeK3tkl zaxnzsIPqPknDRGyGqyMKTSqro9Gha+hGFghW02RlY6RyixIubcXdhLO6 A=; Authentication-Results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: Yca7UCFJpaeOVZ9HKKhOTpqfBIiKAriZvcgYQaPo0HE6fOZZvkR2AsF9VPPYu+epj83BLXOj3k gl4P3yNaDa+q84isI/NKLZRu5XiC5bGaEOEF3FTgoj+iZcb/ix9957FLQMorJuzTQAIMY4mlrr +Np4yzWrMWMuXFxxLOYqckYRLVib17RgIt6eFK+1pbXrqnh8A0gbggdDiPM/afSmih9XF5fKrg wxNMorQafr287qc0Qw8RptqovfS5+FeO5tPZwpnc2Guobc2FdC+j1QTLSlRwnKGOD1mALt5D0y MwQ= X-SBRS: 2.7 X-MesageID: 23253209 X-Ironport-Server: esa2.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.75,402,1589256000"; d="scan'208";a="23253209" Date: Mon, 27 Jul 2020 10:42:16 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Boris Ostrovsky CC: David Hildenbrand , , Juergen Gross , Stefano Stabellini , Wei Liu , Oleksandr Andrushchenko , David Airlie , "Yan Yankovskyi" , , "Michal Hocko" , , Daniel Vetter , , Dan Williams , Dan Carpenter Subject: Re: [PATCH v2 4/4] xen: add helpers to allocate unpopulated memory Message-ID: <20200727084216.GO7191@Air-de-Roger> References: <20200724124241.48208-1-roger.pau@citrix.com> <20200724124241.48208-5-roger.pau@citrix.com> <1778c97f-3a69-8280-141c-879814dd213f@redhat.com> <1fd1d29e-5c10-0c29-0628-b79807f81de6@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <1fd1d29e-5c10-0c29-0628-b79807f81de6@oracle.com> X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To AMSPEX02CL02.citrite.net (10.69.22.126) X-Rspamd-Queue-Id: 6E4C8180555DE X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Jul 24, 2020 at 12:36:33PM -0400, Boris Ostrovsky wrote: > On 7/24/20 10:34 AM, David Hildenbrand wrote: > > CCing Dan > > > > On 24.07.20 14:42, Roger Pau Monne wrote: > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include > >> + > >> +#include > >> +#include > >> + > >> +static DEFINE_MUTEX(lock); > >> +static LIST_HEAD(list); > >> +static unsigned int count; > >> + > >> +static int fill(unsigned int nr_pages) >=20 >=20 > Less generic names? How about=C2=A0 list_lock, pg_list, pg_count, > fill_pglist()? (But these are bad too, so maybe you can come up with > something better) OK, I have to admit I like using such short names when the code allows to, for example this code is so simple that it didn't seem to warrant using longer names. Will rename on next version. > >> +{ > >> + struct dev_pagemap *pgmap; > >> + void *vaddr; > >> + unsigned int i, alloc_pages =3D round_up(nr_pages, PAGES_PER_SECTI= ON); > >> + int nid, ret; > >> + > >> + pgmap =3D kzalloc(sizeof(*pgmap), GFP_KERNEL); > >> + if (!pgmap) > >> + return -ENOMEM; > >> + > >> + pgmap->type =3D MEMORY_DEVICE_DEVDAX; > >> + pgmap->res.name =3D "XEN SCRATCH"; >=20 >=20 > Typically iomem resources only capitalize first letters. >=20 >=20 > >> + pgmap->res.flags =3D IORESOURCE_MEM | IORESOURCE_BUSY; > >> + > >> + ret =3D allocate_resource(&iomem_resource, &pgmap->res, > >> + alloc_pages * PAGE_SIZE, 0, -1, > >> + PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL); >=20 >=20 > Are we not going to end up with a whole bunch of "Xen scratch" resource > ranges for each miss in the page list? Or do we expect them to get merg= ed? PAGES_PER_SECTION is IMO big enough to prevent ending up with a lot of separated ranges. I think the value is 32 or 64MiB on x86, so while we are likely to end up with more than one resource added, I don't think it's going to be massive. >=20 > >> + if (ret < 0) { > >> + pr_err("Cannot allocate new IOMEM resource\n"); > >> + kfree(pgmap); > >> + return ret; > >> + } > >> + > >> + nid =3D memory_add_physaddr_to_nid(pgmap->res.start); >=20 >=20 > Should we consider page range crossing node boundaries? I'm not sure whether this is possible (I would think allocate_resource should return a range from a single node), but then it would greatly complicate the code to perform the memremap_pages, as we would have to split the region into multiple dev_pagemap structs. FWIW the current code in the balloon driver does exactly the same (which doesn't mean it's correct, but that's where I got the logic from). > >> + > >> +#ifdef CONFIG_XEN_HAVE_PVMMU > >> + /* > >> + * We don't support PV MMU when Linux and Xen is using > >> + * different page granularity. > >> + */ > >> + BUILD_BUG_ON(XEN_PAGE_SIZE !=3D PAGE_SIZE); > >> + > >> + /* > >> + * memremap will build page tables for the new memory so > >> + * the p2m must contain invalid entries so the correct > >> + * non-present PTEs will be written. > >> + * > >> + * If a failure occurs, the original (identity) p2m entries > >> + * are not restored since this region is now known not to > >> + * conflict with any devices. > >> + */ > >> + if (!xen_feature(XENFEAT_auto_translated_physmap)) { > >> + xen_pfn_t pfn =3D PFN_DOWN(pgmap->res.start); > >> + > >> + for (i =3D 0; i < alloc_pages; i++) { > >> + if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) { > >> + pr_warn("set_phys_to_machine() failed, no memory added\n"); > >> + release_resource(&pgmap->res); > >> + kfree(pgmap); > >> + return -ENOMEM; > >> + } > >> + } > >> + } > >> +#endif > >> + > >> + vaddr =3D memremap_pages(pgmap, nid); > >> + if (IS_ERR(vaddr)) { > >> + pr_err("Cannot remap memory range\n"); > >> + release_resource(&pgmap->res); > >> + kfree(pgmap); > >> + return PTR_ERR(vaddr); > >> + } > >> + > >> + for (i =3D 0; i < alloc_pages; i++) { > >> + struct page *pg =3D virt_to_page(vaddr + PAGE_SIZE * i); > >> + > >> + BUG_ON(!virt_addr_valid(vaddr + PAGE_SIZE * i)); > >> + list_add(&pg->lru, &list); > >> + count++; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +/** > >> + * xen_alloc_unpopulated_pages - alloc unpopulated pages > >> + * @nr_pages: Number of pages > >> + * @pages: pages returned > >> + * @return 0 on success, error otherwise > >> + */ > >> +int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page = **pages) > >> +{ > >> + unsigned int i; > >> + int ret =3D 0; > >> + > >> + mutex_lock(&lock); > >> + if (count < nr_pages) { > >> + ret =3D fill(nr_pages); >=20 >=20 > (nr_pages - count) ? Yup, already fixed as Juergen also pointed it out. > >> + > >> +#ifdef CONFIG_XEN_PV > >> +static int __init init(void) > >> +{ > >> + unsigned int i; > >> + > >> + if (!xen_domain()) > >> + return -ENODEV; > >> + > >> + /* > >> + * Initialize with pages from the extra memory regions (see > >> + * arch/x86/xen/setup.c). > >> + */ >=20 >=20 > This loop will be executing only for PV guests so we can just bail out > for non-PV guests here. Sure. Thanks, Roger.