diff mbox series

[-v3,2/3] resource: Make alloc_free_mem_region() works for iomem_resource

Message ID 20240906030713.204292-3-ying.huang@intel.com
State New
Headers show
Series resource: Fix region_intersects() vs add_memory_driver_managed() | expand

Commit Message

Huang, Ying Sept. 6, 2024, 3:07 a.m. UTC 
During developing a kunit test case for region_intersects(), some fake
resources need to be inserted into iomem_resource.  To do that, a
resource hole needs to be found first in iomem_resource.

However, alloc_free_mem_region() cannot work for iomem_resource now.
Because the start address to check cannot be 0 to detect address
wrapping 0 in gfr_continue(), while iomem_resource.start == 0.  To
make alloc_free_mem_region() works for iomem_resource, gfr_start() is
changed to avoid to return 0 even if base->start == 0.  We don't need
to check 0 as start address.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Baoquan He <bhe@redhat.com>
---
 kernel/resource.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Hildenbrand Sept. 9, 2024, 7:04 a.m. UTC | #1 
On 06.09.24 05:07, Huang Ying wrote:
> During developing a kunit test case for region_intersects(), some fake
> resources need to be inserted into iomem_resource.  To do that, a
> resource hole needs to be found first in iomem_resource.
> 
> However, alloc_free_mem_region() cannot work for iomem_resource now.
> Because the start address to check cannot be 0 to detect address
> wrapping 0 in gfr_continue(), while iomem_resource.start == 0.  To
> make alloc_free_mem_region() works for iomem_resource, gfr_start() is
> changed to avoid to return 0 even if base->start == 0.  We don't need
> to check 0 as start address.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Alistair Popple <apopple@nvidia.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Baoquan He <bhe@redhat.com>
> ---
>   kernel/resource.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 235dc77f8add..035ef16c1a66 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1873,7 +1873,7 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
>   		return end - size + 1;
>   	}
>   
> -	return ALIGN(base->start, align);

You should add a comment here. But I do find what you are doing here 
quite confusing.

Above you write: "We don't need to check 0 as start address." -- why? To 
make the code extra confusing? :)

/* Never return address 0, because XXX. */
if (!base->start)
	retrn align;
return ALIGN(base->start, align);


And i still haven't understood XXX. For whom exactly is address 0 a problem?

> +	return ALIGN(max(base->start, align), align);
>   }
>   
>   static bool gfr_continue(struct resource *base, resource_size_t addr,
Huang, Ying Sept. 9, 2024, 7:07 a.m. UTC | #2 
Hi, David,

David Hildenbrand <david@redhat.com> writes:

> On 06.09.24 05:07, Huang Ying wrote:
>> During developing a kunit test case for region_intersects(), some fake
>> resources need to be inserted into iomem_resource.  To do that, a
>> resource hole needs to be found first in iomem_resource.
>> However, alloc_free_mem_region() cannot work for iomem_resource now.
>> Because the start address to check cannot be 0 to detect address
>> wrapping 0 in gfr_continue(), while iomem_resource.start == 0.  To
>> make alloc_free_mem_region() works for iomem_resource, gfr_start() is
>> changed to avoid to return 0 even if base->start == 0.  We don't need
>> to check 0 as start address.
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Alison Schofield <alison.schofield@intel.com>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Alistair Popple <apopple@nvidia.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Baoquan He <bhe@redhat.com>
>> ---
>>   kernel/resource.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 235dc77f8add..035ef16c1a66 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1873,7 +1873,7 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
>>   		return end - size + 1;
>>   	}
>>   -	return ALIGN(base->start, align);
>
> You should add a comment here. But I do find what you are doing here
> quite confusing.

Sure.  And sorry for confusing words.

> Above you write: "We don't need to check 0 as start address." -- why?
> To make the code extra confusing? :)

After the change, we will not return "0" from gfr_start().  So we cannot
check "0" as start address.  And I think nobody need to check "0", so it
should be OK to do that.

> /* Never return address 0, because XXX. */
> if (!base->start)
> 	retrn align;
> return ALIGN(base->start, align);
>
>
> And i still haven't understood XXX. For whom exactly is address 0 a problem?

Because the following lines in gfr_continue()

	/*
	 * In the ascend case be careful that the last increment by
	 * @size did not wrap 0.
	 */
	return addr > addr - size &&
	       addr <= min_t(resource_size_t, base->end,
			     (1ULL << MAX_PHYSMEM_BITS) - 1);

If addr == 0, then addr < addr - size.  gfr_continue() will return
false, and we will not check any address.

>> +	return ALIGN(max(base->start, align), align);
>>   }
>>     static bool gfr_continue(struct resource *base, resource_size_t
>> addr,

--
Best Regards,
Huang, Ying
David Hildenbrand Sept. 9, 2024, 8:04 a.m. UTC | #3 
On 09.09.24 09:07, Huang, Ying wrote:
> Hi, David,
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 06.09.24 05:07, Huang Ying wrote:
>>> During developing a kunit test case for region_intersects(), some fake
>>> resources need to be inserted into iomem_resource.  To do that, a
>>> resource hole needs to be found first in iomem_resource.
>>> However, alloc_free_mem_region() cannot work for iomem_resource now.
>>> Because the start address to check cannot be 0 to detect address
>>> wrapping 0 in gfr_continue(), while iomem_resource.start == 0.  To
>>> make alloc_free_mem_region() works for iomem_resource, gfr_start() is
>>> changed to avoid to return 0 even if base->start == 0.  We don't need
>>> to check 0 as start address.
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Cc: Dave Jiang <dave.jiang@intel.com>
>>> Cc: Alison Schofield <alison.schofield@intel.com>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> Cc: Alistair Popple <apopple@nvidia.com>
>>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Baoquan He <bhe@redhat.com>
>>> ---
>>>    kernel/resource.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>> index 235dc77f8add..035ef16c1a66 100644
>>> --- a/kernel/resource.c
>>> +++ b/kernel/resource.c
>>> @@ -1873,7 +1873,7 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
>>>    		return end - size + 1;
>>>    	}
>>>    -	return ALIGN(base->start, align);
>>
>> You should add a comment here. But I do find what you are doing here
>> quite confusing.
> 
> Sure.  And sorry for confusing words.
> 
>> Above you write: "We don't need to check 0 as start address." -- why?
>> To make the code extra confusing? :)
> 
> After the change, we will not return "0" from gfr_start().  So we cannot
> check "0" as start address.  And I think nobody need to check "0", so it
> should be OK to do that.
> 
>> /* Never return address 0, because XXX. */
>> if (!base->start)
>> 	return align;
>> return ALIGN(base->start, align);
>>
>>
>> And i still haven't understood XXX. For whom exactly is address 0 a problem?
> 
> Because the following lines in gfr_continue()
> 
> 	/*
> 	 * In the ascend case be careful that the last increment by
> 	 * @size did not wrap 0.
> 	 */
> 	return addr > addr - size &&
> 	       addr <= min_t(resource_size_t, base->end,
> 			     (1ULL << MAX_PHYSMEM_BITS) - 1);
> 
> If addr == 0, then addr < addr - size.  gfr_continue() will return
> false, and we will not check any address.

Thanks, that makes things cleaner. I think it might be better to just rework
the retying logic, to detect wraps based on the old and new address. That
would require a bit more work, something like that should probably handle all
possible corner case. Dan wrote that code, so I'll leave it up to him to decide
how to handle that :)


diff --git a/kernel/resource.c b/kernel/resource.c
index 9f747bb7cd031..2cd054c8277e8 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1819,40 +1819,48 @@ EXPORT_SYMBOL(resource_list_free);
  #define GFR_REQUEST_REGION	(1UL << 1)
  #define GFR_DEFAULT_ALIGN (1UL << PA_SECTION_SHIFT)
  
-static resource_size_t gfr_start(struct resource *base, resource_size_t size,
-				 resource_size_t align, unsigned long flags)
+static bool gfr_start(struct resource *base, resource_size_t *addr,
+		resource_size_t size, resource_size_t align, unsigned long flags)
  {
+	resource_size_t start_addr;
+
  	if (flags & GFR_DESCENDING) {
  		resource_size_t end;
  
  		end = min_t(resource_size_t, base->end,
  			    (1ULL << MAX_PHYSMEM_BITS) - 1);
-		return end - size + 1;
+		start_addr = end - size + 1;
+		if (start_addr > end || start_addr < base->start)
+			return false;
+	} else {
+		start_addr = ALIGN(base->start, align);
+		if (start_addr < base->start || start_addr > base->end)
+			return false;
  	}
-
-	return ALIGN(base->start, align);
+	*addr = start_addr;
+	return true;
  }
  
-static bool gfr_continue(struct resource *base, resource_size_t addr,
-			 resource_size_t size, unsigned long flags)
+static bool gfr_continue(struct resource *base, resource_size_t *addr,
+			 resource_size_t align, unsigned long flags)
  {
-	if (flags & GFR_DESCENDING)
-		return addr > size && addr >= base->start;
-	/*
-	 * In the ascend case be careful that the last increment by
-	 * @size did not wrap 0.
-	 */
-	return addr > addr - size &&
-	       addr <= min_t(resource_size_t, base->end,
-			     (1ULL << MAX_PHYSMEM_BITS) - 1);
-}
+	resource_size_t new_addr;
  
-static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
-				unsigned long flags)
-{
-	if (flags & GFR_DESCENDING)
-		return addr - size;
-	return addr + size;
+	if (flags & GFR_DESCENDING) {
+		new_addr = *addr - align;
+		if (new_addr > *addr || new_addr < base->start)
+			return false;
+	} else {
+		resource_size_t end;
+
+		end = min_t(resource_size_t, base->end,
+			    (1ULL << MAX_PHYSMEM_BITS) - 1);
+		new_addr = *addr + align;
+		if (new_addr < *addr || new_addr > end)
+			return false;
+	}
+	*addr = new_addr;
+	return true;
  }
  
  static void remove_free_mem_region(void *_res)
@@ -1893,9 +1901,9 @@ get_free_mem_region(struct device *dev, struct resource *base,
  	}
  
  	write_lock(&resource_lock);
-	for (addr = gfr_start(base, size, align, flags);
-	     gfr_continue(base, addr, align, flags);
-	     addr = gfr_next(addr, align, flags)) {
+	if (!gfr_start(base, &addr, size, align, flags))
+		goto unlock;
+	do {
  		if (__region_intersects(base, addr, size, 0, IORES_DESC_NONE) !=
  		    REGION_DISJOINT)
  			continue;
@@ -1939,7 +1947,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
  		}
  
  		return res;
-	}
+	} while (gfr_continue(base, &addr, align, flags));
+unlock:
  	write_unlock(&resource_lock);
  
  	if (flags & GFR_REQUEST_REGION) {
diff mbox series

Patch

diff --git a/kernel/resource.c b/kernel/resource.c
index 235dc77f8add..035ef16c1a66 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1873,7 +1873,7 @@  static resource_size_t gfr_start(struct resource *base, resource_size_t size,
 		return end - size + 1;
 	}
 
-	return ALIGN(base->start, align);
+	return ALIGN(max(base->start, align), align);
 }
 
 static bool gfr_continue(struct resource *base, resource_size_t addr,