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;
Subject: Re: [PATCH v2] kernel/resource: Fix locking in
 request_free_mem_region
To:     Alistair Popple <apopple@nvidia.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
References: <20210326012035.3853-1-apopple@nvidia.com>
 <9eef1283-28a3-845e-0e3e-80b763c9ec59@redhat.com>
 <3158185.bARUjMUeyn@nvdebian>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat GmbH
Message-ID: <88bca3c7-7958-7bab-317d-1918e47061ee@redhat.com>
Date:   Mon, 29 Mar 2021 11:27:47 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <3158185.bARUjMUeyn@nvdebian>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?

Ah, yes, thanks - that makes sense.

> 
>>>
>>> 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

Right, you would want to return -EBUSY, -ENOMEM,... from 
__request_region() - which somehow seems like the right thing to do 
considering that we can have both types of errors already.

> 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).

Fair enough, but I doubt the starvation is a real issue ...

> 
> I also don't think what I am proposing is particularly complex. I agree the

Well, it adds another 42 LOC to kernel/resource.c for a rather special 
case that just needs a better return value from __request_region() to 
make a decision.

> 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
>>
> 
> 
> 
> 


-- 
Thanks,

David / dhildenb