Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
From:   Alistair Popple <apopple@nvidia.com>
To:     David Hildenbrand <david@redhat.com>
CC:     <linux-kernel@vger.kernel.org>, <akpm@linux-foundation.org>,
        <daniel.vetter@ffwll.ch>, <dan.j.williams@intel.com>,
        <gregkh@linuxfoundation.org>, <jhubbard@nvidia.com>,
        <jglisse@redhat.com>, <linux-mm@kvack.org>
Subject: Re: [PATCH v2] kernel/resource: Fix locking in request_free_mem_region
Date:   Wed, 31 Mar 2021 17:19:44 +1100
Message-ID: <12736273.ONR6GAMRWp@nvdebian>
In-Reply-To: <e910441c-73a7-b57e-1330-ead65c4ff412@redhat.com>
References: <20210326012035.3853-1-apopple@nvidia.com> <3158185.bARUjMUeyn@nvdebian> <e910441c-73a7-b57e-1330-ead65c4ff412@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2021 06:19:48.8528
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3df7faac-2604-4808-3b72-08d8f40cfcb9
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT025.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3622
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tuesday, 30 March 2021 8:13:32 PM AEDT David Hildenbrand wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 29.03.21 03:37, Alistair Popple wrote:
> > On Friday, 26 March 2021 7:57:51 PM AEDT David Hildenbrand wrote:
> >> On 26.03.21 02:20, Alistair Popple wrote:
> >>> request_free_mem_region() is used to find an empty range of physical
> >>> addresses for hotplugging ZONE_DEVICE memory. It does this by iterating
> >>> over the range of possible addresses using region_intersects() to see if
> >>> the range is free.
> >>
> >> Just a high-level question: how does this iteract with memory
> >> hot(un)plug? IOW, how defines and manages the "range of possible
> >> addresses" ?
> >
> > Both the driver and the maximum physical address bits available define the
> > range of possible addresses for device private memory. From
> > __request_free_mem_region():
> >
> > end = min_t(unsigned long, base->end, (1UL << MAX_PHYSMEM_BITS) - 1);
> > addr = end - size + 1UL;
> >
> > There is no lower address range bound here so it is effectively zero. The 
code
> > will try to allocate the highest possible physical address first and 
continue
> > searching down for a free block. Does that answer your question?
> 
> Oh, sorry, the fist time I had a look I got it wrong - I thought (1UL <<
> MAX_PHYSMEM_BITS) would be the lower address limit. That looks indeed
> problematic to me.
> 
> You might end up reserving an iomem region that could be used e.g., by
> memory hotplug code later. If someone plugs a DIMM or adds memory via
> different approaches (virtio-mem), memory hotplug (via add_memory())
> would fail.
> 
> You never should be touching physical memory area reserved for memory
> hotplug, i.e., via SRAT.
> 
> What is the expectation here?

Most drivers call request_free_mem_region() with iomem_resource as the base. 
So zone device private pages currently tend to get allocated from the top of 
that.

By definition ZONE_DEVICE private pages are unaddressable from the CPU. So in 
terms of expectation I think all that is really required for ZONE_DEVICE 
private pages (at least for Nouveau) is a valid range of physical addresses 
that allow page_to_pfn() and pfn_to_page() to work correctly. To make this 
work drivers add the pages via memremap_pages() -> pagemap_range() -> 
add_pages().

 - Alistair

> --
> Thanks,
> 
> David / dhildenb
>