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:   Mon, 29 Mar 2021 12:37:49 +1100
Message-ID: <3158185.bARUjMUeyn@nvdebian>
In-Reply-To: <9eef1283-28a3-845e-0e3e-80b763c9ec59@redhat.com>
References: <20210326012035.3853-1-apopple@nvidia.com> <9eef1283-28a3-845e-0e3e-80b763c9ec59@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Mar 2021 01:37:54.4793
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 0ec5b4c1-05fc-4af5-f2a4-08d8f2534645
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: BN8NAM11FT061.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3160
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

> >
> > region_intersects() obtains a read lock before walking the resource tree
> > to protect against concurrent changes. However it drops the lock prior
> > to returning. This means by the time request_mem_region() is called in
> > request_free_mem_region() another thread may have already reserved the
> > requested region resulting in unexpected failures and a message in the
> > kernel log from hitting this condition:
> 
> I am confused. Why can't we return an error to the caller and let the
> caller continue searching? This feels much simpler than what you propose
> here. What am I missing?

The search occurs as part of the allocation. To allocate memory free space 
needs to be located and allocated as a single operation. However in this case 
the lock is dropped between locating a free region and allocating it resulting 
in an extra debug check firing and subsequent failure.

I did originally consider just allowing the caller to retry, but in the end it 
didn't seem any simpler. Callers would have to differentiate between transient 
and permanent failures and figure out how often to retry and no doubt each 
caller would do this differently. There is also the issue of starvation if one 
thread constantly looses the race to allocate after the search. Overall it 
seems simpler to me to just have a call that allocates a region (or fails due 
to lack of free space).

I also don't think what I am proposing is particularly complex. I agree the 
diff makes it look complex, but at a high level all I'm doing is moving the 
locking to outer function calls. It ends up looking more complex because there 
are some memory allocations which need reordering, but I don't think if things 
were originally written this way it would be considered complex.

 - Alistair

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