Messages in this thread | | | Subject | Re: [PATCH v4] kernel/resource: Fix locking in request_free_mem_region | From | David Hildenbrand <> | Date | Fri, 16 Apr 2021 10:18:02 +0200 |
| |
On 16.04.21 06:19, Dan Williams wrote: > On Thu, Apr 15, 2021 at 7:58 PM Alistair Popple <apopple@nvidia.com> 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. >> >> 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: >> >> /* >> * mm/hmm.c reserves physical addresses which then >> * become unavailable to other users. Conflicts are >> * not expected. Warn to aid debugging if encountered. >> */ >> if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) { >> pr_warn("Unaddressable device %s %pR conflicts with %pR", >> conflict->name, conflict, res); >> >> To fix this create versions of region_intersects() and >> request_mem_region() that allow the caller to take the appropriate lock >> such that it may be held over the required calls. >> >> Instead of creating another version of devm_request_mem_region() that >> doesn't take the lock open-code it to allow the caller to pre-allocate >> the required memory prior to taking the lock. >> >> On some architectures and kernel configurations revoke_iomem() also >> calls resource code so cannot be called with the resource lock held. >> Therefore call it only after dropping the lock. > > The patch is difficult to read because too many things are being > changed at once, and the changelog seems to confirm that. Can you try > breaking this down into a set of incremental changes? Not only will > this ease review it will distribute any regressions over multiple > bisection targets. > > Something like: > > * Refactor region_intersects() to allow external locking > * Refactor __request_region() to allow external locking > * Push revoke_iomem() down into... > * Fix resource_lock usage in [devm_]request_free_mem_region()
+1
-- Thanks,
David / dhildenb
| |