Re: [PATCH v2] kernel/resource: Fix locking in request_free_mem_region

[David Hildenbrand <david@redhat.com>]

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