| Message ID | 20210419070109.4780-3-apopple@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/3] kernel/resource: Allow region_intersects users to hold resource_lock | expand |
On 19.04.21 09:01, 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 before calling request_mem_region() to allocate the > region. > > However the resource_lock is dropped between these two calls meaning by the > time request_mem_region() is called in request_free_mem_region() another > thread may have already reserved the requested region. This results 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); > > These unexpected failures can be corrected by holding resource_lock across > the two calls. This also requires memory allocation to be performed prior > to taking the lock. > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > --- > kernel/resource.c | 45 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 38 insertions(+), 7 deletions(-) > > diff --git a/kernel/resource.c b/kernel/resource.c > index 75f8da722497..e8468e867495 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1801,25 +1801,56 @@ static struct resource *__request_free_mem_region(struct device *dev, > { > resource_size_t end, addr; > struct resource *res; > + struct region_devres *dr = NULL; > > size = ALIGN(size, 1UL << PA_SECTION_SHIFT); > end = min_t(unsigned long, base->end, (1UL << MAX_PHYSMEM_BITS) - 1); > addr = end - size + 1UL; > > + res = alloc_resource(GFP_KERNEL); > + if (!res) > + return ERR_PTR(-ENOMEM); > + > + if (dev) { > + dr = devres_alloc(devm_region_release, > + sizeof(struct region_devres), GFP_KERNEL); > + if (!dr) { > + free_resource(res); > + return ERR_PTR(-ENOMEM); > + } > + } > + > + write_lock(&resource_lock); > for (; addr > size && addr >= base->start; addr -= size) { > - if (region_intersects(addr, size, 0, IORES_DESC_NONE) != > + if (__region_intersects(addr, size, 0, IORES_DESC_NONE) != > REGION_DISJOINT) > continue; > > - if (dev) > - res = devm_request_mem_region(dev, addr, size, name); > - else > - res = request_mem_region(addr, size, name); > - if (!res) > - return ERR_PTR(-ENOMEM); > + if (!__request_region_locked(res, &iomem_resource, addr, size, > + name, 0)) > + break; > + > + if (dev) { > + dr->parent = &iomem_resource; > + dr->start = addr; > + dr->n = size; > + devres_add(dev, dr); > + } > + > res->desc = IORES_DESC_DEVICE_PRIVATE_MEMORY; > + write_unlock(&resource_lock); > + > + /* > + * A driver is claiming this region so revoke any mappings. > + */ > + revoke_iomem(res); > return res; > } > + write_unlock(&resource_lock); > + > + free_resource(res); > + if (dr) > + devres_free(dr); > > return ERR_PTR(-ERANGE); > } > LGTM Reviewed-by: David Hildenbrand <david@redhat.com>
diff --git a/kernel/resource.c b/kernel/resource.c index 75f8da722497..e8468e867495 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1801,25 +1801,56 @@ static struct resource *__request_free_mem_region(struct device *dev, { resource_size_t end, addr; struct resource *res; + struct region_devres *dr = NULL; size = ALIGN(size, 1UL << PA_SECTION_SHIFT); end = min_t(unsigned long, base->end, (1UL << MAX_PHYSMEM_BITS) - 1); addr = end - size + 1UL; + res = alloc_resource(GFP_KERNEL); + if (!res) + return ERR_PTR(-ENOMEM); + + if (dev) { + dr = devres_alloc(devm_region_release, + sizeof(struct region_devres), GFP_KERNEL); + if (!dr) { + free_resource(res); + return ERR_PTR(-ENOMEM); + } + } + + write_lock(&resource_lock); for (; addr > size && addr >= base->start; addr -= size) { - if (region_intersects(addr, size, 0, IORES_DESC_NONE) != + if (__region_intersects(addr, size, 0, IORES_DESC_NONE) != REGION_DISJOINT) continue; - if (dev) - res = devm_request_mem_region(dev, addr, size, name); - else - res = request_mem_region(addr, size, name); - if (!res) - return ERR_PTR(-ENOMEM); + if (!__request_region_locked(res, &iomem_resource, addr, size, + name, 0)) + break; + + if (dev) { + dr->parent = &iomem_resource; + dr->start = addr; + dr->n = size; + devres_add(dev, dr); + } + res->desc = IORES_DESC_DEVICE_PRIVATE_MEMORY; + write_unlock(&resource_lock); + + /* + * A driver is claiming this region so revoke any mappings. + */ + revoke_iomem(res); return res; } + write_unlock(&resource_lock); + + free_resource(res); + if (dr) + devres_free(dr); return ERR_PTR(-ERANGE); }
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 before calling request_mem_region() to allocate the region. However the resource_lock is dropped between these two calls meaning by the time request_mem_region() is called in request_free_mem_region() another thread may have already reserved the requested region. This results 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); These unexpected failures can be corrected by holding resource_lock across the two calls. This also requires memory allocation to be performed prior to taking the lock. Signed-off-by: Alistair Popple <apopple@nvidia.com> --- kernel/resource.c | 45 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-)