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

Alistair Popple
2021-03-29T01:38:00Z

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
>