On 16.04.21 06:19, Dan Williams wrote:
On Thu, Apr 15, 2021 at 7:58 PM Alistair Popple <apopple@xxxxxxxxxx> 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
