[PATCH] genalloc: Make the avail variable an atomic64_t

[PATCH] genalloc: Make the avail variable an atomic64_t

[sbates]

From: Stephen Bates <sbates@raithlin.com>

If the amount of resources allocated to a gen_pool exceeds 2^32 then
the avail atomic overflows and this causes problems when clients try
and borrow resources from the pool.

Add the <linux/atomic.h> header to pull in atomic64 operations on
platforms that do not support them natively.

Signed-off-by: Stephen Bates <sbates@raithlin.com>
---
 include/linux/genalloc.h |  3 ++-
 lib/genalloc.c           | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 6dfec4d..b327c31 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -32,6 +32,7 @@
 
 #include <linux/types.h>
 #include <linux/spinlock_types.h>
+#include <linux/atomic.h>
 
 struct device;
 struct device_node;
@@ -71,7 +72,7 @@ struct gen_pool {
  */
 struct gen_pool_chunk {
 	struct list_head next_chunk;	/* next chunk in pool */
-	atomic_t avail;
+	atomic64_t avail;
 	phys_addr_t phys_addr;		/* physical starting address of memory chunk */
 	unsigned long start_addr;	/* start address of memory chunk */
 	unsigned long end_addr;		/* end address of memory chunk (inclusive) */
diff --git a/lib/genalloc.c b/lib/genalloc.c
index 144fe6b..a97df2b 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -194,7 +194,7 @@ int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phy
 	chunk->phys_addr = phys;
 	chunk->start_addr = virt;
 	chunk->end_addr = virt + size - 1;
-	atomic_set(&chunk->avail, size);
+	atomic64_set(&chunk->avail, size);
 
 	spin_lock(&pool->lock);
 	list_add_rcu(&chunk->next_chunk, &pool->chunks);
@@ -304,7 +304,7 @@ unsigned long gen_pool_alloc_algo(struct gen_pool *pool, size_t size,
 	nbits = (size + (1UL << order) - 1) >> order;
 	rcu_read_lock();
 	list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) {
-		if (size > atomic_read(&chunk->avail))
+		if (size > atomic64_read(&chunk->avail))
 			continue;
 
 		start_bit = 0;
@@ -324,7 +324,7 @@ unsigned long gen_pool_alloc_algo(struct gen_pool *pool, size_t size,
 
 		addr = chunk->start_addr + ((unsigned long)start_bit << order);
 		size = nbits << order;
-		atomic_sub(size, &chunk->avail);
+		atomic64_sub(size, &chunk->avail);
 		break;
 	}
 	rcu_read_unlock();
@@ -390,7 +390,7 @@ void gen_pool_free(struct gen_pool *pool, unsigned long addr, size_t size)
 			remain = bitmap_clear_ll(chunk->bits, start_bit, nbits);
 			BUG_ON(remain);
 			size = nbits << order;
-			atomic_add(size, &chunk->avail);
+			atomic64_add(size, &chunk->avail);
 			rcu_read_unlock();
 			return;
 		}
@@ -464,7 +464,7 @@ size_t gen_pool_avail(struct gen_pool *pool)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk)
-		avail += atomic_read(&chunk->avail);
+		avail += atomic64_read(&chunk->avail);
 	rcu_read_unlock();
 	return avail;
 }
-- 
2.7.4

Re: [PATCH] genalloc: Make the avail variable an atomic64_t

[Logan Gunthorpe]

On 25/10/17 09:32 AM, sbates@raithlin.com wrote:
> From: Stephen Bates <sbates@raithlin.com>
> 
> If the amount of resources allocated to a gen_pool exceeds 2^32 then
> the avail atomic overflows and this causes problems when clients try
> and borrow resources from the pool.
> 
> Add the <linux/atomic.h> header to pull in atomic64 operations on
> platforms that do not support them natively.
> 
> Signed-off-by: Stephen Bates <sbates@raithlin.com>

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

This looks pretty straightforward to me.

Logan

Re: [PATCH] genalloc: Make the avail variable an atomic64_t

[Daniel Mentz]

I found that genalloc is very slow for large chunk sizes because
bitmap_find_next_zero_area has to grind through that entire bitmap.
Hence, I recommend avoiding genalloc for large chunk sizes.

I'm thinking how this would behave on a 32 bit ARM platform

On Wed, Oct 25, 2017 at 8:32 AM,  <sbates@raithlin.com> wrote:
> --- a/lib/genalloc.c
> +++ b/lib/genalloc.c
> @@ -194,7 +194,7 @@ int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phy
>         chunk->phys_addr = phys;
>         chunk->start_addr = virt;
>         chunk->end_addr = virt + size - 1;
> -       atomic_set(&chunk->avail, size);
> +       atomic64_set(&chunk->avail, size);

Isn't size defined as a size_t type which is 32 bit wide on ARM? How
can you ever set chunk->avail to anything larger than 2^32 - 1?

> @@ -464,7 +464,7 @@ size_t gen_pool_avail(struct gen_pool *pool)
>
>         rcu_read_lock();
>         list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk)
> -               avail += atomic_read(&chunk->avail);
> +               avail += atomic64_read(&chunk->avail);

avail is defined as size_t (32 bit). Aren't you going to overflow that variable?

Re: [PATCH] genalloc: Make the avail variable an atomic64_t

[Logan Gunthorpe]

On 25/10/17 11:55 AM, Daniel Mentz wrote:
> avail is defined as size_t (32 bit). Aren't you going to overflow that variable?

I think the point is to get support for 64-bit systems (ie. ARM64 and 
x64. We're working with large PCI BARs and need some way to allocate 
memory from them. You aren't likely to ever get >4GB chunks on 32-bit 
systems but you certainly can on 64-bit systems.

Logan

Re: [PATCH] genalloc: Make the avail variable an atomic64_t

[Stephen Bates]

> I found that genalloc is very slow for large chunk sizes because
> bitmap_find_next_zero_area has to grind through that entire bitmap.
> Hence, I recommend avoiding genalloc for large chunk sizes.

Thanks for the feedback Daniel! We have been doing 16GiB without any noticeable issues.

> I'm thinking how this would behave on a 32 bit ARM platform

I don’t think people would be doing such big allocations on 32 bit (ARM systems). It would not make sense for them to be doing >4GB anyway.

>> --- a/lib/genalloc.c
>> +++ b/lib/genalloc.c
>> @@ -194,7 +194,7 @@ int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phy
>>         chunk->phys_addr = phys;
>>         chunk->start_addr = virt;
>>         chunk->end_addr = virt + size - 1;
>> -       atomic_set(&chunk->avail, size);
>> +       atomic64_set(&chunk->avail, size);

> Isn't size defined as a size_t type which is 32 bit wide on ARM? How
> can you ever set chunk->avail to anything larger than 2^32 - 1?

I did consider changing this type but it seems like there would never be a need to set this value to more than 4GiB on 32 bit systems.

>> @@ -464,7 +464,7 @@ size_t gen_pool_avail(struct gen_pool *pool)
>>
>>         rcu_read_lock();
>>         list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk)
>> -               avail += atomic_read(&chunk->avail);
>> +               avail += atomic64_read(&chunk->avail);
>
>avail is defined as size_t (32 bit). Aren't you going to overflow that variable?

Again, I don’t think people on 32 bit systems will be doing >4GB assignments so it would not be an issue.

Stephen

Re: [PATCH] genalloc: Make the avail variable an atomic64_t

[Mathieu Desnoyers]

----- On Oct 26, 2017, at 12:20 AM, Stephen Bates sbates@raithlin.com wrote:

>> I found that genalloc is very slow for large chunk sizes because
>> bitmap_find_next_zero_area has to grind through that entire bitmap.
>> Hence, I recommend avoiding genalloc for large chunk sizes.
> 
> Thanks for the feedback Daniel! We have been doing 16GiB without any noticeable
> issues.
> 
>> I'm thinking how this would behave on a 32 bit ARM platform
> 
> I don’t think people would be doing such big allocations on 32 bit (ARM
> systems). It would not make sense for them to be doing >4GB anyway.
> 
>>> --- a/lib/genalloc.c
>>> +++ b/lib/genalloc.c
>>> @@ -194,7 +194,7 @@ int gen_pool_add_virt(struct gen_pool *pool, unsigned long
>>> virt, phys_addr_t phy
>>>         chunk->phys_addr = phys;
>>>         chunk->start_addr = virt;
>>>         chunk->end_addr = virt + size - 1;
>>> -       atomic_set(&chunk->avail, size);
>>> +       atomic64_set(&chunk->avail, size);
> 
>> Isn't size defined as a size_t type which is 32 bit wide on ARM? How
>> can you ever set chunk->avail to anything larger than 2^32 - 1?
> 
> I did consider changing this type but it seems like there would never be a need
> to set this value to more than 4GiB on 32 bit systems.
> 
>>> @@ -464,7 +464,7 @@ size_t gen_pool_avail(struct gen_pool *pool)
>>>
>>>         rcu_read_lock();
>>>         list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk)
>>> -               avail += atomic_read(&chunk->avail);
>>> +               avail += atomic64_read(&chunk->avail);
>>
>>avail is defined as size_t (32 bit). Aren't you going to overflow that variable?
> 
> Again, I don’t think people on 32 bit systems will be doing >4GB assignments so
> it would not be an issue.

We have atomic_long_t for that. Please use it instead. It will be
64-bit on 64-bit archs, and 32-bit on 32-bit archs, which seems to
fit your purpose here.

Thanks,

Mathieu


> 
> Stephen

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] genalloc: Make the avail variable an atomic64_t

[Stephen Bates]

> We have atomic_long_t for that. Please use it instead. It will be
> 64-bit on 64-bit archs, and 32-bit on 32-bit archs, which seems to
> fit your purpose here.

Thanks you Mathieu! Yes atomic_long_t looks perfect for this and addresses Daniel’s concerns for 32 bit systems. I’ll prepare a v2 with that change.

Stephen