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

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

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

[Mathieu Desnoyers]

- On Oct 26, 2017, at 12:20 AM, Stephen Bates sba...@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(>avail, size);
>>> +   atomic64_set(>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, >chunks, next_chunk)
>>> -   avail += atomic_read(>avail);
>>> +   avail += atomic64_read(>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

[Mathieu Desnoyers]

- On Oct 26, 2017, at 12:20 AM, Stephen Bates sba...@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(>avail, size);
>>> +   atomic64_set(>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, >chunks, next_chunk)
>>> -   avail += atomic_read(>avail);
>>> +   avail += atomic64_read(>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]

> 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(>avail, size);
>> +   atomic64_set(>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, >chunks, next_chunk)
>> -   avail += atomic_read(>avail);
>> +   avail += atomic64_read(>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

[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(>avail, size);
>> +   atomic64_set(>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, >chunks, next_chunk)
>> -   avail += atomic_read(>avail);
>> +   avail += atomic64_read(>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

[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

[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

[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,   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(>avail, size);
> +   atomic64_set(>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, >chunks, next_chunk)
> -   avail += atomic_read(>avail);
> +   avail += atomic64_read(>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

[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,   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(>avail, size);
> +   atomic64_set(>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, >chunks, next_chunk)
> -   avail += atomic_read(>avail);
> +   avail += atomic64_read(>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 09:32 AM, sba...@raithlin.com wrote:

From: Stephen Bates 

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  header to pull in atomic64 operations on
platforms that do not support them natively.

Signed-off-by: Stephen Bates 


Reviewed-by: Logan Gunthorpe 

This looks pretty straightforward to me.

Logan

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

[Logan Gunthorpe]

On 25/10/17 09:32 AM, sba...@raithlin.com wrote:

From: Stephen Bates 

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  header to pull in atomic64 operations on
platforms that do not support them natively.

Signed-off-by: Stephen Bates 


Reviewed-by: Logan Gunthorpe 

This looks pretty straightforward to me.

Logan

[PATCH] genalloc: Make the avail variable an atomic64_t

[sbates]

From: Stephen Bates 

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  header to pull in atomic64 operations on
platforms that do not support them natively.

Signed-off-by: Stephen Bates 
---
 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 
 #include 
+#include 
 
 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(>avail, size);
+   atomic64_set(>avail, size);
 
spin_lock(>lock);
list_add_rcu(>next_chunk, >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, >chunks, next_chunk) {
-   if (size > atomic_read(>avail))
+   if (size > atomic64_read(>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, >avail);
+   atomic64_sub(size, >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, >avail);
+   atomic64_add(size, >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, >chunks, next_chunk)
-   avail += atomic_read(>avail);
+   avail += atomic64_read(>avail);
rcu_read_unlock();
return avail;
 }
-- 
2.7.4

[PATCH] genalloc: Make the avail variable an atomic64_t

[sbates]

From: Stephen Bates 

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  header to pull in atomic64 operations on
platforms that do not support them natively.

Signed-off-by: Stephen Bates 
---
 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 
 #include 
+#include 
 
 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(>avail, size);
+   atomic64_set(>avail, size);
 
spin_lock(>lock);
list_add_rcu(>next_chunk, >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, >chunks, next_chunk) {
-   if (size > atomic_read(>avail))
+   if (size > atomic64_read(>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, >avail);
+   atomic64_sub(size, >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, >avail);
+   atomic64_add(size, >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, >chunks, next_chunk)
-   avail += atomic_read(>avail);
+   avail += atomic64_read(>avail);
rcu_read_unlock();
return avail;
 }
-- 
2.7.4