Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 12/17/18 20:42, Liam Mark wrote:
> On Sun, 16 Dec 2018, Alexey Skidanov wrote:
> 
>>
>>
>> On 12/16/18 7:20 AM, Liam Mark wrote:
>>> On Tue, 6 Feb 2018, Alexey Skidanov wrote:
>>>
>>>>
>>>>
>>>> On 02/07/2018 01:56 AM, Laura Abbott wrote:
>>>>> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>>>>>
>>>>>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>>>>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>>>>>> Any driver may access shared buffers, created by ion, using
>>>>>>>> dma_buf_vmap and
>>>>>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>>>>>> buffers into
>>>>>>>> the kernel virtual address space. The implementation of these API is
>>>>>>>> missing in
>>>>>>>> the current ion implementation.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Skidanov 
>>>>>>>> ---
>>>>>>>
>>>>>>> No review from any other Intel developers? :(
>>>>>> Will add.
>>>>>>>
>>>>>>> Anyway, what in-tree driver needs access to these functions?
>>>>>> I'm not sure that there are the in-tree drivers using these functions
>>>>>> and ion as> buffer exporter because they are not implemented in ion :)
>>>>>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>>>>>> other buffer exporters.
>>>>>
>>>>> It's still not clear why you need to implement these APIs.
>>>> How the importing kernel module may access the content of the buffer? :)
>>>> With the current ion implementation it's only possible by dma_buf_kmap,
>>>> mapping one page at a time. For pretty large buffers, it might have some
>>>> performance impact.
>>>> (Probably, the page by page mapping is the only way to access large
>>>> buffers on 32 bit systems, where the vmalloc range is very small. By the
>>>> way, the current ion dma_map_kmap doesn't really map only 1 page at a
>>>> time - it uses the result of vmap() that might fail on 32 bit systems.)
>>>>
>>>>> Are you planning to use Ion with GPU drivers? I'm especially
>>>>> interested in this if you have a non-Android use case.
>>>> Yes, my use case is the non-Android one. But not with GPU drivers.
>>>>>
>>>>> Thanks,
>>>>> Laura
>>>>
>>>> Thanks,
>>>> Alexey
>>>
>>> I was wondering if we could re-open the discussion on adding support to 
>>> ION for dma_buf_vmap.
>>> It seems like the patch was not taken as the reviewers wanted more 
>>> evidence of an upstream use case.
>>>
>>> Here would be my upstream usage argument for including dma_buf_vmap 
>>> support in ION.
>>>
>>> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
>>> of a kernel mapping for the buffer, unfortunately the resulting call to 
>>> alloc_vmap_area can be quite expensive and this has caused a performance 
>>> regression for certain clients when they have moved to the new version of 
>>> ION.
>>>
>>> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
>>> and generally isn't needed by clients. So if we remove the creation of the 
>>> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
>>> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
>>>
>>> An additional benefit of removing the creation of kernel mappings from 
>>> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
>>> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
>>> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
>>> go negative which could lead to undesired behavior.
>>>
>>> One disadvantage of the above change is that a kernel mapping is not 
>>> already created when a client calls dma_buf_kmap. So the following 
>>> dma_buf_kmap contract can't be satisfied.
>>>
>>> /**
>>> * dma_buf_kmap - Map a page of the buffer object into kernel address 
>>> space. The
>>> * same restrictions as for kmap and friends apply.
>>> * @dmabuf:  [in]buffer to map page 

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 12/16/18 7:20 AM, Liam Mark wrote:
> On Tue, 6 Feb 2018, Alexey Skidanov wrote:
> 
>>
>>
>> On 02/07/2018 01:56 AM, Laura Abbott wrote:
>>> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>>>
>>>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>>>> Any driver may access shared buffers, created by ion, using
>>>>>> dma_buf_vmap and
>>>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>>>> buffers into
>>>>>> the kernel virtual address space. The implementation of these API is
>>>>>> missing in
>>>>>> the current ion implementation.
>>>>>>
>>>>>> Signed-off-by: Alexey Skidanov 
>>>>>> ---
>>>>>
>>>>> No review from any other Intel developers? :(
>>>> Will add.
>>>>>
>>>>> Anyway, what in-tree driver needs access to these functions?
>>>> I'm not sure that there are the in-tree drivers using these functions
>>>> and ion as> buffer exporter because they are not implemented in ion :)
>>>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>>>> other buffer exporters.
>>>
>>> It's still not clear why you need to implement these APIs.
>> How the importing kernel module may access the content of the buffer? :)
>> With the current ion implementation it's only possible by dma_buf_kmap,
>> mapping one page at a time. For pretty large buffers, it might have some
>> performance impact.
>> (Probably, the page by page mapping is the only way to access large
>> buffers on 32 bit systems, where the vmalloc range is very small. By the
>> way, the current ion dma_map_kmap doesn't really map only 1 page at a
>> time - it uses the result of vmap() that might fail on 32 bit systems.)
>>
>>> Are you planning to use Ion with GPU drivers? I'm especially
>>> interested in this if you have a non-Android use case.
>> Yes, my use case is the non-Android one. But not with GPU drivers.
>>>
>>> Thanks,
>>> Laura
>>
>> Thanks,
>> Alexey
> 
> I was wondering if we could re-open the discussion on adding support to 
> ION for dma_buf_vmap.
> It seems like the patch was not taken as the reviewers wanted more 
> evidence of an upstream use case.
> 
> Here would be my upstream usage argument for including dma_buf_vmap 
> support in ION.
> 
> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
> of a kernel mapping for the buffer, unfortunately the resulting call to 
> alloc_vmap_area can be quite expensive and this has caused a performance 
> regression for certain clients when they have moved to the new version of 
> ION.
> 
> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
> and generally isn't needed by clients. So if we remove the creation of the 
> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
> 
> An additional benefit of removing the creation of kernel mappings from 
> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
> go negative which could lead to undesired behavior.
> 
> One disadvantage of the above change is that a kernel mapping is not 
> already created when a client calls dma_buf_kmap. So the following 
> dma_buf_kmap contract can't be satisfied.
> 
> /**
> * dma_buf_kmap - Map a page of the buffer object into kernel address 
> space. The
> * same restrictions as for kmap and friends apply.
> * @dmabuf:[in]buffer to map page from.
> * @page_num:  [in]page in PAGE_SIZE units to map.
> *
> * This call must always succeed, any necessary preparations that might 
> fail
> * need to be done in begin_cpu_access.
> */
> 
> But hopefully we can work around this by moving clients to dma_buf_vmap.
I think the problem is with the contract. We can't ensure that the call
is always succeeds regardless the implementation - any mapping might
fail. Probably this is why  *all* clients of dma_buf_kmap() check the
return value (so it's safe to return NULL in case of failure).

I would suggest to fix the contract and to keep the dma_buf_kmap()
support in ION.
> 
> Based on discussions at LPC here is what was proposed:
> - #1 Add support to ION for dma_buf_vmap and dma_buf_vunmap
> - #2 Move any existing ION clients over from using dma_buf_kmap to 
> dma_buf_vmap
> - #3 Deprecate support in ION for dma_buf_kmap?
> - #4 Make the above performance optimization to 
> ion_dma_buf_begin_cpu_access to remove the creation of a kernel mapping.
> 
> Thoughts?
> 
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Thanks,
Alexey

[PATCH v2] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

gen_pool_alloc_algo() uses different allocation functions implementing
different allocation algorithms. With gen_pool_first_fit_align() allocation
function, the returned address should be aligned on the requested boundary.

If chunk start address isn't aligned on the requested boundary, the
returned address isn't aligned too. The only way to get properly aligned
address is to initialize the pool with chunks aligned on the requested
boundary. If want to have an ability to allocate buffers aligned on
different boundaries (for example, 4K, 1MB, ...), the chunk start address
should be aligned on the max possible alignment.

This happens because gen_pool_first_fit_align() looks for properly aligned
memory block without taking into account the chunk start address alignment.

To fix this, we provide chunk start address to gen_pool_first_fit_align()
and change its implementation such that it starts looking for properly
aligned block with appropriate offset (exactly as it done in CMA).

Link: 
https://lkml.kernel.org/lkml/a170cf65-6884-3592-1de9-4c235888c...@intel.com
Signed-off-by: Alexey Skidanov 
---
Changes:
v2: 
Correct typos
Add more details to changelog

 include/linux/genalloc.h | 13 +++--
 lib/genalloc.c   | 20 
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 872f930..dd0a452 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -51,7 +51,8 @@ typedef unsigned long (*genpool_algo_t)(unsigned long *map,
unsigned long size,
unsigned long start,
unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool,
+   unsigned long start_addr);
 
 /*
  *  General purpose special memory pool descriptor.
@@ -131,24 +132,24 @@ extern void gen_pool_set_algo(struct gen_pool *pool, 
genpool_algo_t algo,
 
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_fixed_alloc(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_first_fit_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 
 extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
diff --git a/lib/genalloc.c b/lib/genalloc.c
index ca06adc..5deb25c 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -311,7 +311,7 @@ unsigned long gen_pool_alloc_algo(struct gen_pool *pool, 
size_t size,
end_bit = chunk_size(chunk) >> order;
 retry:
start_bit = algo(chunk->bits, end_bit, start_bit,
-nbits, data, pool);
+nbits, data, pool, chunk->start_addr);
if (start_bit >= end_bit)
continue;
remain = bitmap_set_ll(chunk->bits, start_bit, nbits);
@@ -525,7 +525,7 @@ EXPORT_SYMBOL(gen_pool_set_algo);
  */
 unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_addr)
 {
return bitmap_find_next_zero_area(map, size, start, nr, 0);
 }
@@ -543,16 +543,19 @@ EXPORT_SYMBOL(gen_pool_first_fit);
  */
 unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_addr)
 {
struct genpool_data_align *alignment;
-   unsigned long align_mask;
+   unsigned long align_mask, align_off;
int order;
 
alignment = data;
order = pool->min_alloc_order;
ali

[PATCH v2] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

gen_pool_alloc_algo() uses different allocation functions implementing
different allocation algorithms. With gen_pool_first_fit_align() allocation
function, the returned address should be aligned on the requested boundary.

If chunk start address isn't aligned on the requested boundary, the
returned address isn't aligned too. The only way to get properly aligned
address is to initialize the pool with chunks aligned on the requested
boundary. If want to have an ability to allocate buffers aligned on
different boundaries (for example, 4K, 1MB, ...), the chunk start address
should be aligned on the max possible alignment.

This happens because gen_pool_first_fit_align() looks for properly aligned
memory block without taking into account the chunk start address alignment.

To fix this, we provide chunk start address to gen_pool_first_fit_align()
and change its implementation such that it starts looking for properly
aligned block with appropriate offset (exactly as it done in CMA).

Link: 
https://lkml.kernel.org/lkml/a170cf65-6884-3592-1de9-4c235888c...@intel.com
Signed-off-by: Alexey Skidanov 
---
Changes:
v2: 
Correct typos
Add more details to changelog

 include/linux/genalloc.h | 13 +++--
 lib/genalloc.c   | 20 
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 872f930..dd0a452 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -51,7 +51,8 @@ typedef unsigned long (*genpool_algo_t)(unsigned long *map,
unsigned long size,
unsigned long start,
unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool,
+   unsigned long start_addr);
 
 /*
  *  General purpose special memory pool descriptor.
@@ -131,24 +132,24 @@ extern void gen_pool_set_algo(struct gen_pool *pool, 
genpool_algo_t algo,
 
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_fixed_alloc(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_first_fit_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 
 extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
diff --git a/lib/genalloc.c b/lib/genalloc.c
index ca06adc..5deb25c 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -311,7 +311,7 @@ unsigned long gen_pool_alloc_algo(struct gen_pool *pool, 
size_t size,
end_bit = chunk_size(chunk) >> order;
 retry:
start_bit = algo(chunk->bits, end_bit, start_bit,
-nbits, data, pool);
+nbits, data, pool, chunk->start_addr);
if (start_bit >= end_bit)
continue;
remain = bitmap_set_ll(chunk->bits, start_bit, nbits);
@@ -525,7 +525,7 @@ EXPORT_SYMBOL(gen_pool_set_algo);
  */
 unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_addr)
 {
return bitmap_find_next_zero_area(map, size, start, nr, 0);
 }
@@ -543,16 +543,19 @@ EXPORT_SYMBOL(gen_pool_first_fit);
  */
 unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_addr)
 {
struct genpool_data_align *alignment;
-   unsigned long align_mask;
+   unsigned long align_mask, align_off;
int order;
 
alignment = data;
order = pool->min_alloc_order;
ali

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/8/18 00:14, Andrew Morton wrote:
> On Wed, 7 Nov 2018 08:21:07 +0200 Alexey Skidanov  
> wrote:
> 
>>> Why does this need "fixing"?  Are there current callers which can
>>> misalign chunk_start_addr?  Or is there a requirement that future
>>> callers can misalign chunk_start_addr?
>>>
>> I work on adding aligned allocation support for ION heaps
>> (https://www.spinics.net/lists/linux-driver-devel/msg118754.html). Two
>> of these heaps use genpool internally.
>>
>> If you want to have an ability to allocate buffers aligned on different
>> boundaries (for example, 4K, 1MB, ...), this fix actually removes the
>> requirement for chunk_start_addr to be aligned on the max possible
>> alignment.
> 
> OK.  This sort of information should be in the changelog, please.  In
> much detail.
> 
Sure, I'll add this information and resend the patch.

Thanks,
Alexey

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/8/18 00:14, Andrew Morton wrote:
> On Wed, 7 Nov 2018 08:21:07 +0200 Alexey Skidanov  
> wrote:
> 
>>> Why does this need "fixing"?  Are there current callers which can
>>> misalign chunk_start_addr?  Or is there a requirement that future
>>> callers can misalign chunk_start_addr?
>>>
>> I work on adding aligned allocation support for ION heaps
>> (https://www.spinics.net/lists/linux-driver-devel/msg118754.html). Two
>> of these heaps use genpool internally.
>>
>> If you want to have an ability to allocate buffers aligned on different
>> boundaries (for example, 4K, 1MB, ...), this fix actually removes the
>> requirement for chunk_start_addr to be aligned on the max possible
>> alignment.
> 
> OK.  This sort of information should be in the changelog, please.  In
> much detail.
> 
Sure, I'll add this information and resend the patch.

Thanks,
Alexey

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/8/18 00:12, Andrew Morton wrote:
> On Wed, 7 Nov 2018 08:27:31 +0200 Alexey Skidanov  
> wrote:
> 
>>
>>
>> On 11/7/18 12:15 AM, Andrew Morton wrote:
>>> On Tue,  6 Nov 2018 14:20:53 +0200 Alexey Skidanov 
>>>  wrote:
>>>
>>>> On success, gen_pool_first_fit_align() returns the bit number such that
>>>> chunk_start_addr + (bit << order) is properly aligned. On failure,
>>>> the bitmap size parameter is returned.
>>>>
>>>> When the chunk_start_addr isn't aligned properly, the
>>>> chunk_start_addr + (bit << order) isn't aligned too.
>>>>
>>>> To fix this, gen_pool_first_fit_align() takes into account
>>>> the chunk_start_addr alignment and returns the bit value such that
>>>> chunk_start_addr + (bit << order) is properly aligned
>>>> (exactly as it done in CMA).
>>>>
>>>> ...
>>>>
>>>> --- a/include/linux/genalloc.h
>>>> +++ b/include/linux/genalloc.h
>>>>
>>>> ...
>>>>
>>>> +  struct gen_pool *pool, unsigned long start_add)
>>>>
>>>> ...
>>>>
>>>> +  struct gen_pool *pool, unsigned long start_add)
>>>>
>>>> ...
>>>>
>>>> +  struct gen_pool *pool, unsigned long start_add)
>>>>
>>>> ...
>>>>
>>>
>>> We have three typos here.  Which makes me wonder why we're passing the
>>> new argument and then not using it?
>>>
>> genpool uses allocation callbacks function that implement some
>> allocation strategy - bes fit, first fit, ... All of them has the same
>> type. The added chunk start_addr is used only in one of them -
>> gen_pool_first_fit_align()
> 
> OK, but the argument name here is start_add, not start_addr.
> 
Sure, I'll fix.

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/8/18 00:12, Andrew Morton wrote:
> On Wed, 7 Nov 2018 08:27:31 +0200 Alexey Skidanov  
> wrote:
> 
>>
>>
>> On 11/7/18 12:15 AM, Andrew Morton wrote:
>>> On Tue,  6 Nov 2018 14:20:53 +0200 Alexey Skidanov 
>>>  wrote:
>>>
>>>> On success, gen_pool_first_fit_align() returns the bit number such that
>>>> chunk_start_addr + (bit << order) is properly aligned. On failure,
>>>> the bitmap size parameter is returned.
>>>>
>>>> When the chunk_start_addr isn't aligned properly, the
>>>> chunk_start_addr + (bit << order) isn't aligned too.
>>>>
>>>> To fix this, gen_pool_first_fit_align() takes into account
>>>> the chunk_start_addr alignment and returns the bit value such that
>>>> chunk_start_addr + (bit << order) is properly aligned
>>>> (exactly as it done in CMA).
>>>>
>>>> ...
>>>>
>>>> --- a/include/linux/genalloc.h
>>>> +++ b/include/linux/genalloc.h
>>>>
>>>> ...
>>>>
>>>> +  struct gen_pool *pool, unsigned long start_add)
>>>>
>>>> ...
>>>>
>>>> +  struct gen_pool *pool, unsigned long start_add)
>>>>
>>>> ...
>>>>
>>>> +  struct gen_pool *pool, unsigned long start_add)
>>>>
>>>> ...
>>>>
>>>
>>> We have three typos here.  Which makes me wonder why we're passing the
>>> new argument and then not using it?
>>>
>> genpool uses allocation callbacks function that implement some
>> allocation strategy - bes fit, first fit, ... All of them has the same
>> type. The added chunk start_addr is used only in one of them -
>> gen_pool_first_fit_align()
> 
> OK, but the argument name here is start_add, not start_addr.
> 
Sure, I'll fix.

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/7/18 12:15 AM, Andrew Morton wrote:
> On Tue,  6 Nov 2018 14:20:53 +0200 Alexey Skidanov 
>  wrote:
> 
>> On success, gen_pool_first_fit_align() returns the bit number such that
>> chunk_start_addr + (bit << order) is properly aligned. On failure,
>> the bitmap size parameter is returned.
>>
>> When the chunk_start_addr isn't aligned properly, the
>> chunk_start_addr + (bit << order) isn't aligned too.
>>
>> To fix this, gen_pool_first_fit_align() takes into account
>> the chunk_start_addr alignment and returns the bit value such that
>> chunk_start_addr + (bit << order) is properly aligned
>> (exactly as it done in CMA).
>>
>> ...
>>
>> --- a/include/linux/genalloc.h
>> +++ b/include/linux/genalloc.h
>>
>> ...
>>
>> +struct gen_pool *pool, unsigned long start_add)
>>
>> ...
>>
>> +struct gen_pool *pool, unsigned long start_add)
>>
>> ...
>>
>> +struct gen_pool *pool, unsigned long start_add)
>>
>> ...
>>
> 
> We have three typos here.  Which makes me wonder why we're passing the
> new argument and then not using it?
> 
genpool uses allocation callbacks function that implement some
allocation strategy - bes fit, first fit, ... All of them has the same
type. The added chunk start_addr is used only in one of them -
gen_pool_first_fit_align()

Thanks,
Alexey

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/7/18 12:15 AM, Andrew Morton wrote:
> On Tue,  6 Nov 2018 14:20:53 +0200 Alexey Skidanov 
>  wrote:
> 
>> On success, gen_pool_first_fit_align() returns the bit number such that
>> chunk_start_addr + (bit << order) is properly aligned. On failure,
>> the bitmap size parameter is returned.
>>
>> When the chunk_start_addr isn't aligned properly, the
>> chunk_start_addr + (bit << order) isn't aligned too.
>>
>> To fix this, gen_pool_first_fit_align() takes into account
>> the chunk_start_addr alignment and returns the bit value such that
>> chunk_start_addr + (bit << order) is properly aligned
>> (exactly as it done in CMA).
>>
>> ...
>>
>> --- a/include/linux/genalloc.h
>> +++ b/include/linux/genalloc.h
>>
>> ...
>>
>> +struct gen_pool *pool, unsigned long start_add)
>>
>> ...
>>
>> +struct gen_pool *pool, unsigned long start_add)
>>
>> ...
>>
>> +struct gen_pool *pool, unsigned long start_add)
>>
>> ...
>>
> 
> We have three typos here.  Which makes me wonder why we're passing the
> new argument and then not using it?
> 
genpool uses allocation callbacks function that implement some
allocation strategy - bes fit, first fit, ... All of them has the same
type. The added chunk start_addr is used only in one of them -
gen_pool_first_fit_align()

Thanks,
Alexey

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/7/18 12:11 AM, Andrew Morton wrote:
> On Tue,  6 Nov 2018 14:20:53 +0200 Alexey Skidanov 
>  wrote:
> 
>> On success, gen_pool_first_fit_align() returns the bit number such that
>> chunk_start_addr + (bit << order) is properly aligned. On failure,
>> the bitmap size parameter is returned.
>>
>> When the chunk_start_addr isn't aligned properly, the
>> chunk_start_addr + (bit << order) isn't aligned too.
>>
>> To fix this, gen_pool_first_fit_align() takes into account
>> the chunk_start_addr alignment and returns the bit value such that
>> chunk_start_addr + (bit << order) is properly aligned
>> (exactly as it done in CMA).
> 
> Why does this need "fixing"?  Are there current callers which can
> misalign chunk_start_addr?  Or is there a requirement that future
> callers can misalign chunk_start_addr?
> 
I work on adding aligned allocation support for ION heaps
(https://www.spinics.net/lists/linux-driver-devel/msg118754.html). Two
of these heaps use genpool internally.

If you want to have an ability to allocate buffers aligned on different
boundaries (for example, 4K, 1MB, ...), this fix actually removes the
requirement for chunk_start_addr to be aligned on the max possible
alignment.

Thanks,
Alexey

Re: [PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On 11/7/18 12:11 AM, Andrew Morton wrote:
> On Tue,  6 Nov 2018 14:20:53 +0200 Alexey Skidanov 
>  wrote:
> 
>> On success, gen_pool_first_fit_align() returns the bit number such that
>> chunk_start_addr + (bit << order) is properly aligned. On failure,
>> the bitmap size parameter is returned.
>>
>> When the chunk_start_addr isn't aligned properly, the
>> chunk_start_addr + (bit << order) isn't aligned too.
>>
>> To fix this, gen_pool_first_fit_align() takes into account
>> the chunk_start_addr alignment and returns the bit value such that
>> chunk_start_addr + (bit << order) is properly aligned
>> (exactly as it done in CMA).
> 
> Why does this need "fixing"?  Are there current callers which can
> misalign chunk_start_addr?  Or is there a requirement that future
> callers can misalign chunk_start_addr?
> 
I work on adding aligned allocation support for ION heaps
(https://www.spinics.net/lists/linux-driver-devel/msg118754.html). Two
of these heaps use genpool internally.

If you want to have an ability to allocate buffers aligned on different
boundaries (for example, 4K, 1MB, ...), this fix actually removes the
requirement for chunk_start_addr to be aligned on the max possible
alignment.

Thanks,
Alexey

[PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On success, gen_pool_first_fit_align() returns the bit number such that
chunk_start_addr + (bit << order) is properly aligned. On failure,
the bitmap size parameter is returned.

When the chunk_start_addr isn't aligned properly, the
chunk_start_addr + (bit << order) isn't aligned too.

To fix this, gen_pool_first_fit_align() takes into account
the chunk_start_addr alignment and returns the bit value such that
chunk_start_addr + (bit << order) is properly aligned
(exactly as it done in CMA).

Link: 
https://lkml.kernel.org/lkml/a170cf65-6884-3592-1de9-4c235888c...@intel.com
Signed-off-by: Alexey Skidanov 
---
 include/linux/genalloc.h | 13 +++--
 lib/genalloc.c   | 20 
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 872f930..dd0a452 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -51,7 +51,8 @@ typedef unsigned long (*genpool_algo_t)(unsigned long *map,
unsigned long size,
unsigned long start,
unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool,
+   unsigned long start_addr);
 
 /*
  *  General purpose special memory pool descriptor.
@@ -131,24 +132,24 @@ extern void gen_pool_set_algo(struct gen_pool *pool, 
genpool_algo_t algo,
 
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_fixed_alloc(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_first_fit_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 
 extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
diff --git a/lib/genalloc.c b/lib/genalloc.c
index ca06adc..033817a 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -311,7 +311,7 @@ unsigned long gen_pool_alloc_algo(struct gen_pool *pool, 
size_t size,
end_bit = chunk_size(chunk) >> order;
 retry:
start_bit = algo(chunk->bits, end_bit, start_bit,
-nbits, data, pool);
+nbits, data, pool, chunk->start_addr);
if (start_bit >= end_bit)
continue;
remain = bitmap_set_ll(chunk->bits, start_bit, nbits);
@@ -525,7 +525,7 @@ EXPORT_SYMBOL(gen_pool_set_algo);
  */
 unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_add)
 {
return bitmap_find_next_zero_area(map, size, start, nr, 0);
 }
@@ -543,16 +543,19 @@ EXPORT_SYMBOL(gen_pool_first_fit);
  */
 unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_addr)
 {
struct genpool_data_align *alignment;
-   unsigned long align_mask;
+   unsigned long align_mask, align_off;
int order;
 
alignment = data;
order = pool->min_alloc_order;
align_mask = ((alignment->align + (1UL << order) - 1) >> order) - 1;
-   return bitmap_find_next_zero_area(map, size, start, nr, align_mask);
+   align_off = (start_addr & (alignment->align - 1)) >> order;
+
+   return bitmap_find_next_zero_area_off(map, size, start, nr,
+ align_mask, align_off);
 }
 EXPORT_SYMBOL(gen_pool_first_fit_align);
 
@@ -567,7 +570,7 @@ EXPORT_SYMBOL(gen_pool_first_fit_align);
  */
 unsigned long gen_pool_fixed_alloc(unsigned long *map, unsigned long siz

[PATCH] lib/genaloc: Fix allocation of aligned buffer from non-aligned chunk

[Alexey Skidanov]

On success, gen_pool_first_fit_align() returns the bit number such that
chunk_start_addr + (bit << order) is properly aligned. On failure,
the bitmap size parameter is returned.

When the chunk_start_addr isn't aligned properly, the
chunk_start_addr + (bit << order) isn't aligned too.

To fix this, gen_pool_first_fit_align() takes into account
the chunk_start_addr alignment and returns the bit value such that
chunk_start_addr + (bit << order) is properly aligned
(exactly as it done in CMA).

Link: 
https://lkml.kernel.org/lkml/a170cf65-6884-3592-1de9-4c235888c...@intel.com
Signed-off-by: Alexey Skidanov 
---
 include/linux/genalloc.h | 13 +++--
 lib/genalloc.c   | 20 
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 872f930..dd0a452 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -51,7 +51,8 @@ typedef unsigned long (*genpool_algo_t)(unsigned long *map,
unsigned long size,
unsigned long start,
unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool,
+   unsigned long start_addr);
 
 /*
  *  General purpose special memory pool descriptor.
@@ -131,24 +132,24 @@ extern void gen_pool_set_algo(struct gen_pool *pool, 
genpool_algo_t algo,
 
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_fixed_alloc(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_first_fit_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 
 extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
unsigned long size, unsigned long start, unsigned int nr,
-   void *data, struct gen_pool *pool);
+   void *data, struct gen_pool *pool, unsigned long start_addr);
 
 extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool);
+   struct gen_pool *pool, unsigned long start_addr);
 
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
diff --git a/lib/genalloc.c b/lib/genalloc.c
index ca06adc..033817a 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -311,7 +311,7 @@ unsigned long gen_pool_alloc_algo(struct gen_pool *pool, 
size_t size,
end_bit = chunk_size(chunk) >> order;
 retry:
start_bit = algo(chunk->bits, end_bit, start_bit,
-nbits, data, pool);
+nbits, data, pool, chunk->start_addr);
if (start_bit >= end_bit)
continue;
remain = bitmap_set_ll(chunk->bits, start_bit, nbits);
@@ -525,7 +525,7 @@ EXPORT_SYMBOL(gen_pool_set_algo);
  */
 unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_add)
 {
return bitmap_find_next_zero_area(map, size, start, nr, 0);
 }
@@ -543,16 +543,19 @@ EXPORT_SYMBOL(gen_pool_first_fit);
  */
 unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
unsigned long start, unsigned int nr, void *data,
-   struct gen_pool *pool)
+   struct gen_pool *pool, unsigned long start_addr)
 {
struct genpool_data_align *alignment;
-   unsigned long align_mask;
+   unsigned long align_mask, align_off;
int order;
 
alignment = data;
order = pool->min_alloc_order;
align_mask = ((alignment->align + (1UL << order) - 1) >> order) - 1;
-   return bitmap_find_next_zero_area(map, size, start, nr, align_mask);
+   align_off = (start_addr & (alignment->align - 1)) >> order;
+
+   return bitmap_find_next_zero_area_off(map, size, start, nr,
+ align_mask, align_off);
 }
 EXPORT_SYMBOL(gen_pool_first_fit_align);
 
@@ -567,7 +570,7 @@ EXPORT_SYMBOL(gen_pool_first_fit_align);
  */
 unsigned long gen_pool_fixed_alloc(unsigned long *map, unsigned long siz

Re: lib/genalloc

[Alexey Skidanov]

On 11/2/18 11:16 PM, Daniel Mentz wrote:
> On Fri, Nov 2, 2018 at 1:55 PM Alexey Skidanov
>  wrote:
>>
>>
>>
>> On 11/2/18 9:17 PM, Daniel Mentz wrote:
>>> On Thu, Nov 1, 2018 at 10:07 AM Alexey Skidanov
>>>  wrote:
>>>> On 11/1/18 18:48, Stephen  Bates wrote:
>>>>>>I use gen_pool_first_fit_align() as pool allocation algorithm 
>>>>>> allocating
>>>>>>buffers with requested alignment. But if a chunk base address is not
>>>>>>aligned to the requested alignment(from some reason), the returned
>>>>>>address is not aligned too.
>>>>>
>>>>> Alexey
>>>>>
>>>>> Can you try using gen_pool_first_fit_order_align()? Will that give you 
>>>>> the alignment you need?
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>> I think it will not help me. Let's assume that the chunk base address is
>>>> 0x2F40 and I want to allocate 16MB aligned buffer. I get back the
>>>> 0x2F40. I think it happens because of this string in the
>>>> gen_pool_alloc_algo():
>>>>
>>>> addr = chunk->start_addr + ((unsigned long)start_bit << order);
>>>>
>>>> and the gen_pool_first_fit_align() implementation that doesn't take into
>>>> account the "incorrect" chunk base alignment.
>>>
>>> gen_pool_first_fit_align() has no information about the chunk base
>>> alignment. Hence, it can't take it into account.
>>>
>>> How do you request the alignment in your code?
>>>
>>> I agree with your analysis that gen_pool_first_fit_align() performs
>>> alignment only with respect to the start of the chunk not the memory
>>> address that gen_pool_alloc_algo() returns. I guess a solution would
>>> be to only add chunks that satisfy all your alignment requirements. In
>>> your case, you must only add chunks that are 16MB aligned.
>>> I am unsure whether this is by design, but I believe it's the way that
>>> the code currently works.
>>>
>>
>> Daniel,
>>
>> I think the better solution is to use bitmap_find_next_zero_area_off()
>> that receives the bit offset (CMA allocator uses it to solve the same
>> issue). Of course, we need to pass the chunk base address to the
>> gen_pool_first_fit_align().
>>
>> What do you think?
> 
> Yeah, I guess you could extend genpool_algo_t to include the
> information you need i.e. the offset and then provide a modified
> version of gen_pool_first_fit_align() that does take your offset into
> account. I wouldn't change gen_pool_first_fit_align(), though, because
> existing users might depend on the current behavior.
> 
I think that the "fixed" version of gen_pool_first_fit_align() is less
restrictive with respect to chunk base address - it will work correctly
with arbitrary aligned chunks.

Thanks,
Alexey

Re: lib/genalloc

[Alexey Skidanov]

On 11/2/18 11:16 PM, Daniel Mentz wrote:
> On Fri, Nov 2, 2018 at 1:55 PM Alexey Skidanov
>  wrote:
>>
>>
>>
>> On 11/2/18 9:17 PM, Daniel Mentz wrote:
>>> On Thu, Nov 1, 2018 at 10:07 AM Alexey Skidanov
>>>  wrote:
>>>> On 11/1/18 18:48, Stephen  Bates wrote:
>>>>>>I use gen_pool_first_fit_align() as pool allocation algorithm 
>>>>>> allocating
>>>>>>buffers with requested alignment. But if a chunk base address is not
>>>>>>aligned to the requested alignment(from some reason), the returned
>>>>>>address is not aligned too.
>>>>>
>>>>> Alexey
>>>>>
>>>>> Can you try using gen_pool_first_fit_order_align()? Will that give you 
>>>>> the alignment you need?
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>> I think it will not help me. Let's assume that the chunk base address is
>>>> 0x2F40 and I want to allocate 16MB aligned buffer. I get back the
>>>> 0x2F40. I think it happens because of this string in the
>>>> gen_pool_alloc_algo():
>>>>
>>>> addr = chunk->start_addr + ((unsigned long)start_bit << order);
>>>>
>>>> and the gen_pool_first_fit_align() implementation that doesn't take into
>>>> account the "incorrect" chunk base alignment.
>>>
>>> gen_pool_first_fit_align() has no information about the chunk base
>>> alignment. Hence, it can't take it into account.
>>>
>>> How do you request the alignment in your code?
>>>
>>> I agree with your analysis that gen_pool_first_fit_align() performs
>>> alignment only with respect to the start of the chunk not the memory
>>> address that gen_pool_alloc_algo() returns. I guess a solution would
>>> be to only add chunks that satisfy all your alignment requirements. In
>>> your case, you must only add chunks that are 16MB aligned.
>>> I am unsure whether this is by design, but I believe it's the way that
>>> the code currently works.
>>>
>>
>> Daniel,
>>
>> I think the better solution is to use bitmap_find_next_zero_area_off()
>> that receives the bit offset (CMA allocator uses it to solve the same
>> issue). Of course, we need to pass the chunk base address to the
>> gen_pool_first_fit_align().
>>
>> What do you think?
> 
> Yeah, I guess you could extend genpool_algo_t to include the
> information you need i.e. the offset and then provide a modified
> version of gen_pool_first_fit_align() that does take your offset into
> account. I wouldn't change gen_pool_first_fit_align(), though, because
> existing users might depend on the current behavior.
> 
I think that the "fixed" version of gen_pool_first_fit_align() is less
restrictive with respect to chunk base address - it will work correctly
with arbitrary aligned chunks.

Thanks,
Alexey

Re: lib/genalloc

[Alexey Skidanov]

On 11/2/18 9:17 PM, Daniel Mentz wrote:
> On Thu, Nov 1, 2018 at 10:07 AM Alexey Skidanov
>  wrote:
>> On 11/1/18 18:48, Stephen  Bates wrote:
>>>>I use gen_pool_first_fit_align() as pool allocation algorithm allocating
>>>>buffers with requested alignment. But if a chunk base address is not
>>>>aligned to the requested alignment(from some reason), the returned
>>>>address is not aligned too.
>>>
>>> Alexey
>>>
>>> Can you try using gen_pool_first_fit_order_align()? Will that give you the 
>>> alignment you need?
>>>
>>> Stephen
>>>
>>>
>> I think it will not help me. Let's assume that the chunk base address is
>> 0x2F40 and I want to allocate 16MB aligned buffer. I get back the
>> 0x2F40. I think it happens because of this string in the
>> gen_pool_alloc_algo():
>>
>> addr = chunk->start_addr + ((unsigned long)start_bit << order);
>>
>> and the gen_pool_first_fit_align() implementation that doesn't take into
>> account the "incorrect" chunk base alignment.
> 
> gen_pool_first_fit_align() has no information about the chunk base
> alignment. Hence, it can't take it into account.
> 
> How do you request the alignment in your code?
> 
> I agree with your analysis that gen_pool_first_fit_align() performs
> alignment only with respect to the start of the chunk not the memory
> address that gen_pool_alloc_algo() returns. I guess a solution would
> be to only add chunks that satisfy all your alignment requirements. In
> your case, you must only add chunks that are 16MB aligned.
> I am unsure whether this is by design, but I believe it's the way that
> the code currently works.
> 

Daniel,

I think the better solution is to use bitmap_find_next_zero_area_off()
that receives the bit offset (CMA allocator uses it to solve the same
issue). Of course, we need to pass the chunk base address to the
gen_pool_first_fit_align().

What do you think?

Thanks,
Alexey

Re: lib/genalloc

[Alexey Skidanov]

On 11/2/18 9:17 PM, Daniel Mentz wrote:
> On Thu, Nov 1, 2018 at 10:07 AM Alexey Skidanov
>  wrote:
>> On 11/1/18 18:48, Stephen  Bates wrote:
>>>>I use gen_pool_first_fit_align() as pool allocation algorithm allocating
>>>>buffers with requested alignment. But if a chunk base address is not
>>>>aligned to the requested alignment(from some reason), the returned
>>>>address is not aligned too.
>>>
>>> Alexey
>>>
>>> Can you try using gen_pool_first_fit_order_align()? Will that give you the 
>>> alignment you need?
>>>
>>> Stephen
>>>
>>>
>> I think it will not help me. Let's assume that the chunk base address is
>> 0x2F40 and I want to allocate 16MB aligned buffer. I get back the
>> 0x2F40. I think it happens because of this string in the
>> gen_pool_alloc_algo():
>>
>> addr = chunk->start_addr + ((unsigned long)start_bit << order);
>>
>> and the gen_pool_first_fit_align() implementation that doesn't take into
>> account the "incorrect" chunk base alignment.
> 
> gen_pool_first_fit_align() has no information about the chunk base
> alignment. Hence, it can't take it into account.
> 
> How do you request the alignment in your code?
> 
> I agree with your analysis that gen_pool_first_fit_align() performs
> alignment only with respect to the start of the chunk not the memory
> address that gen_pool_alloc_algo() returns. I guess a solution would
> be to only add chunks that satisfy all your alignment requirements. In
> your case, you must only add chunks that are 16MB aligned.
> I am unsure whether this is by design, but I believe it's the way that
> the code currently works.
> 

Daniel,

I think the better solution is to use bitmap_find_next_zero_area_off()
that receives the bit offset (CMA allocator uses it to solve the same
issue). Of course, we need to pass the chunk base address to the
gen_pool_first_fit_align().

What do you think?

Thanks,
Alexey

Re: lib/genalloc

[Alexey Skidanov]

On 11/1/18 18:48, Stephen  Bates wrote:
>>I use gen_pool_first_fit_align() as pool allocation algorithm allocating
>>buffers with requested alignment. But if a chunk base address is not
>>aligned to the requested alignment(from some reason), the returned
>>address is not aligned too.
> 
> Alexey
> 
> Can you try using gen_pool_first_fit_order_align()? Will that give you the 
> alignment you need?
> 
> Stephen
> 
> 
I think it will not help me. Let's assume that the chunk base address is
0x2F40 and I want to allocate 16MB aligned buffer. I get back the
0x2F40. I think it happens because of this string in the
gen_pool_alloc_algo():

addr = chunk->start_addr + ((unsigned long)start_bit << order);

and the gen_pool_first_fit_align() implementation that doesn't take into
account the "incorrect" chunk base alignment.

It might be easily fixed, by rounding the start address up (0x2F40
-> 0x3000) and start looking from this, aligned, address.

Thanks,
Alexey

Re: lib/genalloc

[Alexey Skidanov]

On 11/1/18 18:48, Stephen  Bates wrote:
>>I use gen_pool_first_fit_align() as pool allocation algorithm allocating
>>buffers with requested alignment. But if a chunk base address is not
>>aligned to the requested alignment(from some reason), the returned
>>address is not aligned too.
> 
> Alexey
> 
> Can you try using gen_pool_first_fit_order_align()? Will that give you the 
> alignment you need?
> 
> Stephen
> 
> 
I think it will not help me. Let's assume that the chunk base address is
0x2F40 and I want to allocate 16MB aligned buffer. I get back the
0x2F40. I think it happens because of this string in the
gen_pool_alloc_algo():

addr = chunk->start_addr + ((unsigned long)start_bit << order);

and the gen_pool_first_fit_align() implementation that doesn't take into
account the "incorrect" chunk base alignment.

It might be easily fixed, by rounding the start address up (0x2F40
-> 0x3000) and start looking from this, aligned, address.

Thanks,
Alexey

lib/genalloc

[Alexey Skidanov]

Hi,

I use gen_pool_first_fit_align() as pool allocation algorithm allocating
buffers with requested alignment. But if a chunk base address is not
aligned to the requested alignment(from some reason), the returned
address is not aligned too.

The reason is the allocation algorithm works on bitmap, omitting the
base address. Is this behavior by design?

Thanks,
Alexey

lib/genalloc

[Alexey Skidanov]

Hi,

I use gen_pool_first_fit_align() as pool allocation algorithm allocating
buffers with requested alignment. But if a chunk base address is not
aligned to the requested alignment(from some reason), the returned
address is not aligned too.

The reason is the allocation algorithm works on bitmap, omitting the
base address. Is this behavior by design?

Thanks,
Alexey

Re: [PATCH] lib/scatterlist: Make sg_page_count() accessible to other modules

[Alexey Skidanov]

On 02/13/2018 10:14 PM, Andy Shevchenko wrote:
> On Sat, Feb 10, 2018 at 8:06 PM, Alexey Skidanov
> <alexey.skida...@intel.com> wrote:
>> Currently, sg_page_count() may be used only inside the scatterlist.c file.
>>
>> However, the same calculation is done outside of scatterlist.c file
>> causing to code duplication.
>>
>> To fix this, we move the sg_page_count() to the scatterlist.h file, making it
>> accessible to other modules.
> 
> Can you provide an example of potential (or existing?) use case?
> 
This one, for example - drivers/staging/android/ion/ion_heap.c:
for_each_sg(table->sgl, sg, table->nents, i) {
int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
struct page *page = sg_page(sg);
BUG_ON(i >= npages);
for (j = 0; j < npages_this_entry; j++)
*(tmp++) = page++;
}

One more is in the patch I have submitted for review.
>>
>> Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
>> ---
>>  include/linux/scatterlist.h | 5 +
>>  lib/scatterlist.c   | 5 -
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
>> index b7c8325..fe28148 100644
>> --- a/include/linux/scatterlist.h
>> +++ b/include/linux/scatterlist.h
>> @@ -248,6 +248,11 @@ static inline void *sg_virt(struct scatterlist *sg)
>> return page_address(sg_page(sg)) + sg->offset;
>>  }
>>
>> +static inline int sg_page_count(struct scatterlist *sg)
>> +{
>> +   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
>> +}
>> +
>>  int sg_nents(struct scatterlist *sg);
>>  int sg_nents_for_len(struct scatterlist *sg, u64 len);
>>  struct scatterlist *sg_next(struct scatterlist *);
>> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
>> index 7c1c55f..4a59131 100644
>> --- a/lib/scatterlist.c
>> +++ b/lib/scatterlist.c
>> @@ -486,11 +486,6 @@ void __sg_page_iter_start(struct sg_page_iter *piter,
>>  }
>>  EXPORT_SYMBOL(__sg_page_iter_start);
>>
>> -static int sg_page_count(struct scatterlist *sg)
>> -{
>> -   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
>> -}
>> -
>>  bool __sg_page_iter_next(struct sg_page_iter *piter)
>>  {
>> if (!piter->__nents || !piter->sg)
>> --
>> 2.7.4
>>
> 
> 
> 

Re: [PATCH] lib/scatterlist: Make sg_page_count() accessible to other modules

[Alexey Skidanov]

On 02/13/2018 10:14 PM, Andy Shevchenko wrote:
> On Sat, Feb 10, 2018 at 8:06 PM, Alexey Skidanov
>  wrote:
>> Currently, sg_page_count() may be used only inside the scatterlist.c file.
>>
>> However, the same calculation is done outside of scatterlist.c file
>> causing to code duplication.
>>
>> To fix this, we move the sg_page_count() to the scatterlist.h file, making it
>> accessible to other modules.
> 
> Can you provide an example of potential (or existing?) use case?
> 
This one, for example - drivers/staging/android/ion/ion_heap.c:
for_each_sg(table->sgl, sg, table->nents, i) {
int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
struct page *page = sg_page(sg);
BUG_ON(i >= npages);
for (j = 0; j < npages_this_entry; j++)
*(tmp++) = page++;
}

One more is in the patch I have submitted for review.
>>
>> Signed-off-by: Alexey Skidanov 
>> ---
>>  include/linux/scatterlist.h | 5 +
>>  lib/scatterlist.c   | 5 -
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
>> index b7c8325..fe28148 100644
>> --- a/include/linux/scatterlist.h
>> +++ b/include/linux/scatterlist.h
>> @@ -248,6 +248,11 @@ static inline void *sg_virt(struct scatterlist *sg)
>> return page_address(sg_page(sg)) + sg->offset;
>>  }
>>
>> +static inline int sg_page_count(struct scatterlist *sg)
>> +{
>> +   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
>> +}
>> +
>>  int sg_nents(struct scatterlist *sg);
>>  int sg_nents_for_len(struct scatterlist *sg, u64 len);
>>  struct scatterlist *sg_next(struct scatterlist *);
>> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
>> index 7c1c55f..4a59131 100644
>> --- a/lib/scatterlist.c
>> +++ b/lib/scatterlist.c
>> @@ -486,11 +486,6 @@ void __sg_page_iter_start(struct sg_page_iter *piter,
>>  }
>>  EXPORT_SYMBOL(__sg_page_iter_start);
>>
>> -static int sg_page_count(struct scatterlist *sg)
>> -{
>> -   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
>> -}
>> -
>>  bool __sg_page_iter_next(struct sg_page_iter *piter)
>>  {
>> if (!piter->__nents || !piter->sg)
>> --
>> 2.7.4
>>
> 
> 
> 

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 10:46 PM, Laura Abbott wrote:
> On 02/12/2018 12:22 PM, Alexey Skidanov wrote:
>>
>>
>> On 02/12/2018 09:52 PM, Laura Abbott wrote:
>>> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>>>
>>>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>>>> requested
>>>>>> allocation alignment. CMA heap allocates buffers aligned on buffer
>>>>>> size
>>>>>> page order.
>>>>>>
>>>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>>>> cases,
>>>>>> providing specific alignment may reduce the external memory
>>>>>> fragmentation
>>>>>> and in some cases it may avoid the allocation request failure.
>>>>>>
>>>>> I really do not want to bring this back as part of the regular
>>>>> ABI.
>>>> Yes, I know it was removed in 4.12.
>>>> Having an alignment parameter that gets used for exactly
>>>>> one heap only leads to confusion (which is why it was removed
>>>>> from the ABI in the first place).
>>>> You are correct regarding the CMA heap. But, probably it may be used by
>>>> custom heap as well.
>>>
>>> I can think of a lot of instances where it could be used but
>>> ultimately there needs to be an actual in kernel user who wants
>>> it.
>>>
>>>>> The alignment came from the behavior of the DMA APIs. Do you
>>>>> actually need to specify any alignment from userspace or do
>>>>> you only need page size?
>>>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>>>> decide
>>>
>>> I'm really not convinced changing the ABI yet again just to let
>>> the user decide is actually worth it. If we can manage it, I'd
>>> much rather see a proposal that doesn't change the ABI.
>> I didn't actually change the ABI - I just use the "unused" member:
>> struct ion_allocation_data {
>> @@ -80,7 +79,7 @@ struct ion_allocation_data {
>>  __u32 heap_id_mask;
>>  __u32 flags;
>>  __u32 fd;
>> -   __u32 unused;
>> +   __u32 align;
>>   };
>>
> 
> Something that was previously unused is now being used. That's a change
> in how the structure is interpreted which is an ABI change, especially
> since we haven't been checking that the __unused field is set
> to 0.
Yes you are correct. I just realized that it isn't even backward
compatible.
> 
>> As an alternative, I may add __u64 heap_specific_param - but this will
>> change the ABI. But, probably it makes the ABI more generic?
> 
> Why does the ABI need to be more generic? If we change the ABI
> we're stuck with it and I'd like to approach it as the very last
> resort.
> 
> Thanks,
> Laura
It seems, that there is no way to do it without some ABI change?

Thanks,
Alexey

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 10:46 PM, Laura Abbott wrote:
> On 02/12/2018 12:22 PM, Alexey Skidanov wrote:
>>
>>
>> On 02/12/2018 09:52 PM, Laura Abbott wrote:
>>> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>>>
>>>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>>>> requested
>>>>>> allocation alignment. CMA heap allocates buffers aligned on buffer
>>>>>> size
>>>>>> page order.
>>>>>>
>>>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>>>> cases,
>>>>>> providing specific alignment may reduce the external memory
>>>>>> fragmentation
>>>>>> and in some cases it may avoid the allocation request failure.
>>>>>>
>>>>> I really do not want to bring this back as part of the regular
>>>>> ABI.
>>>> Yes, I know it was removed in 4.12.
>>>> Having an alignment parameter that gets used for exactly
>>>>> one heap only leads to confusion (which is why it was removed
>>>>> from the ABI in the first place).
>>>> You are correct regarding the CMA heap. But, probably it may be used by
>>>> custom heap as well.
>>>
>>> I can think of a lot of instances where it could be used but
>>> ultimately there needs to be an actual in kernel user who wants
>>> it.
>>>
>>>>> The alignment came from the behavior of the DMA APIs. Do you
>>>>> actually need to specify any alignment from userspace or do
>>>>> you only need page size?
>>>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>>>> decide
>>>
>>> I'm really not convinced changing the ABI yet again just to let
>>> the user decide is actually worth it. If we can manage it, I'd
>>> much rather see a proposal that doesn't change the ABI.
>> I didn't actually change the ABI - I just use the "unused" member:
>> struct ion_allocation_data {
>> @@ -80,7 +79,7 @@ struct ion_allocation_data {
>>  __u32 heap_id_mask;
>>  __u32 flags;
>>  __u32 fd;
>> -   __u32 unused;
>> +   __u32 align;
>>   };
>>
> 
> Something that was previously unused is now being used. That's a change
> in how the structure is interpreted which is an ABI change, especially
> since we haven't been checking that the __unused field is set
> to 0.
Yes you are correct. I just realized that it isn't even backward
compatible.
> 
>> As an alternative, I may add __u64 heap_specific_param - but this will
>> change the ABI. But, probably it makes the ABI more generic?
> 
> Why does the ABI need to be more generic? If we change the ABI
> we're stuck with it and I'd like to approach it as the very last
> resort.
> 
> Thanks,
> Laura
It seems, that there is no way to do it without some ABI change?

Thanks,
Alexey

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 09:52 PM, Laura Abbott wrote:
> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>
>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>> requested
>>>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>>>> page order.
>>>>
>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>> cases,
>>>> providing specific alignment may reduce the external memory
>>>> fragmentation
>>>> and in some cases it may avoid the allocation request failure.
>>>>
>>> I really do not want to bring this back as part of the regular
>>> ABI.
>> Yes, I know it was removed in 4.12.
>> Having an alignment parameter that gets used for exactly
>>> one heap only leads to confusion (which is why it was removed
>>> from the ABI in the first place).
>> You are correct regarding the CMA heap. But, probably it may be used by
>> custom heap as well.
> 
> I can think of a lot of instances where it could be used but
> ultimately there needs to be an actual in kernel user who wants
> it.
> 
>>> The alignment came from the behavior of the DMA APIs. Do you
>>> actually need to specify any alignment from userspace or do
>>> you only need page size?
>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>> decide
> 
> I'm really not convinced changing the ABI yet again just to let
> the user decide is actually worth it. If we can manage it, I'd
> much rather see a proposal that doesn't change the ABI.
I didn't actually change the ABI - I just use the "unused" member:
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
-   __u32 unused;
+   __u32 align;
 };

As an alternative, I may add __u64 heap_specific_param - but this will
change the ABI. But, probably it makes the ABI more generic?
> 
>>> Thanks,
>>> Laura
>>>
>> Thanks,
>> Alexey
>>
> 

Thanks,
Alexey

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 09:52 PM, Laura Abbott wrote:
> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>
>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>> requested
>>>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>>>> page order.
>>>>
>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>> cases,
>>>> providing specific alignment may reduce the external memory
>>>> fragmentation
>>>> and in some cases it may avoid the allocation request failure.
>>>>
>>> I really do not want to bring this back as part of the regular
>>> ABI.
>> Yes, I know it was removed in 4.12.
>> Having an alignment parameter that gets used for exactly
>>> one heap only leads to confusion (which is why it was removed
>>> from the ABI in the first place).
>> You are correct regarding the CMA heap. But, probably it may be used by
>> custom heap as well.
> 
> I can think of a lot of instances where it could be used but
> ultimately there needs to be an actual in kernel user who wants
> it.
> 
>>> The alignment came from the behavior of the DMA APIs. Do you
>>> actually need to specify any alignment from userspace or do
>>> you only need page size?
>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>> decide
> 
> I'm really not convinced changing the ABI yet again just to let
> the user decide is actually worth it. If we can manage it, I'd
> much rather see a proposal that doesn't change the ABI.
I didn't actually change the ABI - I just use the "unused" member:
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
-   __u32 unused;
+   __u32 align;
 };

As an alternative, I may add __u64 heap_specific_param - but this will
change the ABI. But, probably it makes the ABI more generic?
> 
>>> Thanks,
>>> Laura
>>>
>> Thanks,
>> Alexey
>>
> 

Thanks,
Alexey

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 08:42 PM, Laura Abbott wrote:
> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>> Current ion defined allocation ioctl doesn't allow to specify the
>> requested
>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>> page order.
>>
>> Sometimes, the alignment requirement is less restrictive. In such cases,
>> providing specific alignment may reduce the external memory fragmentation
>> and in some cases it may avoid the allocation request failure.
>>
> 
> I really do not want to bring this back as part of the regular
> ABI. 
Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly
> one heap only leads to confusion (which is why it was removed
> from the ABI in the first place).
You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.
> 
> The alignment came from the behavior of the DMA APIs. Do you
> actually need to specify any alignment from userspace or do
> you only need page size?
Yes. If CMA gives it for free, I would suggest to let the ion user to decide
> 
> Thanks,
> Laura
> 
Thanks,
Alexey

>> To fix this, we add an alignment parameter to the allocation ioctl.
>>
>> Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
>> ---
>>   drivers/staging/android/ion/ion-ioctl.c |  3 ++-
>>   drivers/staging/android/ion/ion.c   | 14 +-
>>   drivers/staging/android/ion/ion.h   |  9 ++---
>>   drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
>>   drivers/staging/android/ion/ion_chunk_heap.c    |  3 ++-
>>   drivers/staging/android/ion/ion_cma_heap.c  | 18 --
>>   drivers/staging/android/ion/ion_system_heap.c   |  6 --
>>   drivers/staging/android/uapi/ion.h  |  7 +++
>>   8 files changed, 40 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index c789893..9fe022b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>     fd = ion_alloc(data.allocation.len,
>>  data.allocation.heap_id_mask,
>> -   data.allocation.flags);
>> +   data.allocation.flags,
>> +   data.allocation.align);
>>   if (fd < 0)
>>   return fd;
>>   diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index f480885..35ddc7a 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
>>   static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
>>   struct ion_device *dev,
>>   unsigned long len,
>> -    unsigned long flags)
>> +    unsigned long flags,
>> +    unsigned int align)
>>   {
>>   struct ion_buffer *buffer;
>>   int ret;
>> @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct
>> ion_heap *heap,
>>   buffer->heap = heap;
>>   buffer->flags = flags;
>>   -    ret = heap->ops->allocate(heap, buffer, len, flags);
>> +    ret = heap->ops->allocate(heap, buffer, len, flags, align);
>>     if (ret) {
>>   if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
>>   goto err2;
>>     ion_heap_freelist_drain(heap, 0);
>> -    ret = heap->ops->allocate(heap, buffer, len, flags);
>> +    ret = heap->ops->allocate(heap, buffer, len, flags, align);
>>   if (ret)
>>   goto err2;
>>   }
>> @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
>>   .unmap = ion_dma_buf_kunmap,
>>   };
>>   -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int
>> flags)
>> +int ion_alloc(size_t len,
>> +  unsigned int heap_id_mask,
>> +  unsigned int flags,
>> +  unsigned int align)
>>   {
>>   struct ion_device *dev = internal_dev;
>>   struct ion_buffer *buffer = NULL;
>> @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int
>> heap_id_mask, unsigned int flags)
>>   /* if the caller didn't specify this heap id */
>>   i

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 08:42 PM, Laura Abbott wrote:
> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>> Current ion defined allocation ioctl doesn't allow to specify the
>> requested
>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>> page order.
>>
>> Sometimes, the alignment requirement is less restrictive. In such cases,
>> providing specific alignment may reduce the external memory fragmentation
>> and in some cases it may avoid the allocation request failure.
>>
> 
> I really do not want to bring this back as part of the regular
> ABI. 
Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly
> one heap only leads to confusion (which is why it was removed
> from the ABI in the first place).
You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.
> 
> The alignment came from the behavior of the DMA APIs. Do you
> actually need to specify any alignment from userspace or do
> you only need page size?
Yes. If CMA gives it for free, I would suggest to let the ion user to decide
> 
> Thanks,
> Laura
> 
Thanks,
Alexey

>> To fix this, we add an alignment parameter to the allocation ioctl.
>>
>> Signed-off-by: Alexey Skidanov 
>> ---
>>   drivers/staging/android/ion/ion-ioctl.c |  3 ++-
>>   drivers/staging/android/ion/ion.c   | 14 +-
>>   drivers/staging/android/ion/ion.h   |  9 ++---
>>   drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
>>   drivers/staging/android/ion/ion_chunk_heap.c    |  3 ++-
>>   drivers/staging/android/ion/ion_cma_heap.c  | 18 --
>>   drivers/staging/android/ion/ion_system_heap.c   |  6 --
>>   drivers/staging/android/uapi/ion.h  |  7 +++
>>   8 files changed, 40 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index c789893..9fe022b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>     fd = ion_alloc(data.allocation.len,
>>  data.allocation.heap_id_mask,
>> -   data.allocation.flags);
>> +   data.allocation.flags,
>> +   data.allocation.align);
>>   if (fd < 0)
>>   return fd;
>>   diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index f480885..35ddc7a 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
>>   static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
>>   struct ion_device *dev,
>>   unsigned long len,
>> -    unsigned long flags)
>> +    unsigned long flags,
>> +    unsigned int align)
>>   {
>>   struct ion_buffer *buffer;
>>   int ret;
>> @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct
>> ion_heap *heap,
>>   buffer->heap = heap;
>>   buffer->flags = flags;
>>   -    ret = heap->ops->allocate(heap, buffer, len, flags);
>> +    ret = heap->ops->allocate(heap, buffer, len, flags, align);
>>     if (ret) {
>>   if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
>>   goto err2;
>>     ion_heap_freelist_drain(heap, 0);
>> -    ret = heap->ops->allocate(heap, buffer, len, flags);
>> +    ret = heap->ops->allocate(heap, buffer, len, flags, align);
>>   if (ret)
>>   goto err2;
>>   }
>> @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
>>   .unmap = ion_dma_buf_kunmap,
>>   };
>>   -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int
>> flags)
>> +int ion_alloc(size_t len,
>> +  unsigned int heap_id_mask,
>> +  unsigned int flags,
>> +  unsigned int align)
>>   {
>>   struct ion_device *dev = internal_dev;
>>   struct ion_buffer *buffer = NULL;
>> @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int
>> heap_id_mask, unsigned int flags)
>>   /* if the caller didn't specify this heap id */
>>   if (!((1 << heap

[PATCH] lib/scatterlist: Make sg_page_count() accessible to other modules

[Alexey Skidanov]

Currently, sg_page_count() may be used only inside the scatterlist.c file.

However, the same calculation is done outside of scatterlist.c file
causing to code duplication.

To fix this, we move the sg_page_count() to the scatterlist.h file, making it
accessible to other modules.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
 include/linux/scatterlist.h | 5 +
 lib/scatterlist.c   | 5 -
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index b7c8325..fe28148 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -248,6 +248,11 @@ static inline void *sg_virt(struct scatterlist *sg)
return page_address(sg_page(sg)) + sg->offset;
 }
 
+static inline int sg_page_count(struct scatterlist *sg)
+{
+   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
+}
+
 int sg_nents(struct scatterlist *sg);
 int sg_nents_for_len(struct scatterlist *sg, u64 len);
 struct scatterlist *sg_next(struct scatterlist *);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 7c1c55f..4a59131 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -486,11 +486,6 @@ void __sg_page_iter_start(struct sg_page_iter *piter,
 }
 EXPORT_SYMBOL(__sg_page_iter_start);
 
-static int sg_page_count(struct scatterlist *sg)
-{
-   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
-}
-
 bool __sg_page_iter_next(struct sg_page_iter *piter)
 {
if (!piter->__nents || !piter->sg)
-- 
2.7.4

[PATCH] lib/scatterlist: Make sg_page_count() accessible to other modules

[Alexey Skidanov]

Currently, sg_page_count() may be used only inside the scatterlist.c file.

However, the same calculation is done outside of scatterlist.c file
causing to code duplication.

To fix this, we move the sg_page_count() to the scatterlist.h file, making it
accessible to other modules.

Signed-off-by: Alexey Skidanov 
---
 include/linux/scatterlist.h | 5 +
 lib/scatterlist.c   | 5 -
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index b7c8325..fe28148 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -248,6 +248,11 @@ static inline void *sg_virt(struct scatterlist *sg)
return page_address(sg_page(sg)) + sg->offset;
 }
 
+static inline int sg_page_count(struct scatterlist *sg)
+{
+   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
+}
+
 int sg_nents(struct scatterlist *sg);
 int sg_nents_for_len(struct scatterlist *sg, u64 len);
 struct scatterlist *sg_next(struct scatterlist *);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 7c1c55f..4a59131 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -486,11 +486,6 @@ void __sg_page_iter_start(struct sg_page_iter *piter,
 }
 EXPORT_SYMBOL(__sg_page_iter_start);
 
-static int sg_page_count(struct scatterlist *sg)
-{
-   return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
-}
-
 bool __sg_page_iter_next(struct sg_page_iter *piter)
 {
if (!piter->__nents || !piter->sg)
-- 
2.7.4

[PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

Current ion defined allocation ioctl doesn't allow to specify the requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.

To fix this, we add an alignment parameter to the allocation ioctl.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
 drivers/staging/android/ion/ion-ioctl.c |  3 ++-
 drivers/staging/android/ion/ion.c   | 14 +-
 drivers/staging/android/ion/ion.h   |  9 ++---
 drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
 drivers/staging/android/ion/ion_chunk_heap.c|  3 ++-
 drivers/staging/android/ion/ion_cma_heap.c  | 18 --
 drivers/staging/android/ion/ion_system_heap.c   |  6 --
 drivers/staging/android/uapi/ion.h  |  7 +++
 8 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index c789893..9fe022b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
 
fd = ion_alloc(data.allocation.len,
   data.allocation.heap_id_mask,
-  data.allocation.flags);
+  data.allocation.flags,
+  data.allocation.align);
if (fd < 0)
return fd;
 
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..35ddc7a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
 static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
unsigned long len,
-   unsigned long flags)
+   unsigned long flags,
+   unsigned int align)
 {
struct ion_buffer *buffer;
int ret;
@@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
buffer->heap = heap;
buffer->flags = flags;
 
-   ret = heap->ops->allocate(heap, buffer, len, flags);
+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
 
if (ret) {
if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
goto err2;
 
ion_heap_freelist_drain(heap, 0);
-   ret = heap->ops->allocate(heap, buffer, len, flags);
+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
if (ret)
goto err2;
}
@@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap = ion_dma_buf_kunmap,
 };
 
-int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
+int ion_alloc(size_t len,
+ unsigned int heap_id_mask,
+ unsigned int flags,
+ unsigned int align)
 {
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
@@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, 
unsigned int flags)
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
continue;
-   buffer = ion_buffer_create(heap, dev, len, flags);
+   buffer = ion_buffer_create(heap, dev, len, flags, align);
if (!IS_ERR(buffer))
break;
}
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index f5f9cd6..0c161d2 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -123,8 +123,10 @@ struct ion_device {
  */
 struct ion_heap_ops {
int (*allocate)(struct ion_heap *heap,
-   struct ion_buffer *buffer, unsigned long len,
-   unsigned long flags);
+   struct ion_buffer *buffer,
+   unsigned long len,
+   unsigned long flags,
+   unsigned int align);
void (*free)(struct ion_buffer *buffer);
void * (*map_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
void (*unmap_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
@@ -228,7 +230,8 @@ int ion_heap_pages_zero(struct page *page, size_t size, 
pgprot_t pgprot);
 
 int ion_alloc(size_t len,
  

[PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

Current ion defined allocation ioctl doesn't allow to specify the requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.

To fix this, we add an alignment parameter to the allocation ioctl.

Signed-off-by: Alexey Skidanov 
---
 drivers/staging/android/ion/ion-ioctl.c |  3 ++-
 drivers/staging/android/ion/ion.c   | 14 +-
 drivers/staging/android/ion/ion.h   |  9 ++---
 drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
 drivers/staging/android/ion/ion_chunk_heap.c|  3 ++-
 drivers/staging/android/ion/ion_cma_heap.c  | 18 --
 drivers/staging/android/ion/ion_system_heap.c   |  6 --
 drivers/staging/android/uapi/ion.h  |  7 +++
 8 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index c789893..9fe022b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
 
fd = ion_alloc(data.allocation.len,
   data.allocation.heap_id_mask,
-  data.allocation.flags);
+  data.allocation.flags,
+  data.allocation.align);
if (fd < 0)
return fd;
 
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..35ddc7a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
 static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
unsigned long len,
-   unsigned long flags)
+   unsigned long flags,
+   unsigned int align)
 {
struct ion_buffer *buffer;
int ret;
@@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
buffer->heap = heap;
buffer->flags = flags;
 
-   ret = heap->ops->allocate(heap, buffer, len, flags);
+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
 
if (ret) {
if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
goto err2;
 
ion_heap_freelist_drain(heap, 0);
-   ret = heap->ops->allocate(heap, buffer, len, flags);
+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
if (ret)
goto err2;
}
@@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap = ion_dma_buf_kunmap,
 };
 
-int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
+int ion_alloc(size_t len,
+ unsigned int heap_id_mask,
+ unsigned int flags,
+ unsigned int align)
 {
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
@@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, 
unsigned int flags)
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
continue;
-   buffer = ion_buffer_create(heap, dev, len, flags);
+   buffer = ion_buffer_create(heap, dev, len, flags, align);
if (!IS_ERR(buffer))
break;
}
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index f5f9cd6..0c161d2 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -123,8 +123,10 @@ struct ion_device {
  */
 struct ion_heap_ops {
int (*allocate)(struct ion_heap *heap,
-   struct ion_buffer *buffer, unsigned long len,
-   unsigned long flags);
+   struct ion_buffer *buffer,
+   unsigned long len,
+   unsigned long flags,
+   unsigned int align);
void (*free)(struct ion_buffer *buffer);
void * (*map_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
void (*unmap_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
@@ -228,7 +230,8 @@ int ion_heap_pages_zero(struct page *page, size_t size, 
pgprot_t pgprot);
 
 int ion_alloc(size_t len,
  unsigned int heap_id_mask,
- 

dma-buf CPU access

[Alexey Skidanov]

Hi,

According to the CPU access instructions in the dma-buf.c, I understand
that on 32 bit platforms, due to the limited number of persistent kernel
mappings or limited vmalloc range, the CPU access pattern should look
like this:

dma_buf_start_cpu_access();

void *ptr = dma_buf_kmap();

/*Access the page pointed by ptr*/

dma_buf_kunmap(ptr);

dma_buf_end_cpu_access();


or


dma_buf_start_cpu_access();

void *ptr = dma_buf_vmap();

/*Access the buffer pointed by ptr*/

dma_buf_vunmap(ptr);

dma_buf_end_cpu_access();


But on 64 bit platforms, there are no such restrictions (there is no
highmem at all). So, frequently accessed buffer, may be mapped once, but
every access should be surrounded by :

dma_buf_start_cpu_access();

/*Access the buffer pointed by ptr*/

dma_buf_end_cpu_access()

Am I correct?

Thanks,
Alexey

dma-buf CPU access

[Alexey Skidanov]

Hi,

According to the CPU access instructions in the dma-buf.c, I understand
that on 32 bit platforms, due to the limited number of persistent kernel
mappings or limited vmalloc range, the CPU access pattern should look
like this:

dma_buf_start_cpu_access();

void *ptr = dma_buf_kmap();

/*Access the page pointed by ptr*/

dma_buf_kunmap(ptr);

dma_buf_end_cpu_access();


or


dma_buf_start_cpu_access();

void *ptr = dma_buf_vmap();

/*Access the buffer pointed by ptr*/

dma_buf_vunmap(ptr);

dma_buf_end_cpu_access();


But on 64 bit platforms, there are no such restrictions (there is no
highmem at all). So, frequently accessed buffer, may be mapped once, but
every access should be surrounded by :

dma_buf_start_cpu_access();

/*Access the buffer pointed by ptr*/

dma_buf_end_cpu_access()

Am I correct?

Thanks,
Alexey

[PATCH] mm: Free CMA pages to the buddy allocator instead of per-cpu-pagelists

[Alexey Skidanov]

The current code frees pages to the per-cpu-pagelists (pcp) according to
their migrate type. The exception is isolated pages that are freed
directly to the buddy allocator.

The pages are marked as isolate to indicate the buddy allocator that
they are not supposed to be allocated as opposite to the pages located
in the pcp lists that are immediate candidates for the upcoming
allocation requests.

This was likely an oversight when the CMA migrate type was added. As a
result of this, freed CMA pages go to the MIGRATE_MOVABLE per-cpu-list.
This sometime leads to CMA page allocation instead of Movable one,
increasing the probability of CMA page pining, which may cause to CMA
allocation failure. In addition, there is no gain to free CMA page to
the pcp because the CMA pages mainly allocated for DMA purpose.

To fix this, we free CMA page directly to the buddy allocator. This
avoids the CMA page allocation instead of MOVABLE one. Actually, the CMA
pages are very similar to the isolated ones - both of them should not be
supposed as immediate candidates for upcoming allocation requests and
thus shouldn't be freed to pcp. I've audited all the other checks for
isolated pageblocks to ensure that this mistake was not repeated elsewhere.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
 mm/page_alloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 59d5921..9a76b68 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2644,7 +2644,8 @@ static void free_unref_page_commit(struct page *page, 
unsigned long pfn)
 * excessively into the page allocator
 */
if (migratetype >= MIGRATE_PCPTYPES) {
-   if (unlikely(is_migrate_isolate(migratetype))) {
+   if (unlikely(is_migrate_isolate(migratetype) ||
+   is_migrate_cma(migratetype))) {
free_one_page(zone, page, pfn, 0, migratetype);
return;
}
-- 
2.7.4

[PATCH] mm: Free CMA pages to the buddy allocator instead of per-cpu-pagelists

[Alexey Skidanov]

The current code frees pages to the per-cpu-pagelists (pcp) according to
their migrate type. The exception is isolated pages that are freed
directly to the buddy allocator.

The pages are marked as isolate to indicate the buddy allocator that
they are not supposed to be allocated as opposite to the pages located
in the pcp lists that are immediate candidates for the upcoming
allocation requests.

This was likely an oversight when the CMA migrate type was added. As a
result of this, freed CMA pages go to the MIGRATE_MOVABLE per-cpu-list.
This sometime leads to CMA page allocation instead of Movable one,
increasing the probability of CMA page pining, which may cause to CMA
allocation failure. In addition, there is no gain to free CMA page to
the pcp because the CMA pages mainly allocated for DMA purpose.

To fix this, we free CMA page directly to the buddy allocator. This
avoids the CMA page allocation instead of MOVABLE one. Actually, the CMA
pages are very similar to the isolated ones - both of them should not be
supposed as immediate candidates for upcoming allocation requests and
thus shouldn't be freed to pcp. I've audited all the other checks for
isolated pageblocks to ensure that this mistake was not repeated elsewhere.

Signed-off-by: Alexey Skidanov 
---
 mm/page_alloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 59d5921..9a76b68 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2644,7 +2644,8 @@ static void free_unref_page_commit(struct page *page, 
unsigned long pfn)
 * excessively into the page allocator
 */
if (migratetype >= MIGRATE_PCPTYPES) {
-   if (unlikely(is_migrate_isolate(migratetype))) {
+   if (unlikely(is_migrate_isolate(migratetype) ||
+   is_migrate_cma(migratetype))) {
free_one_page(zone, page, pfn, 0, migratetype);
return;
}
-- 
2.7.4

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

On 02/07/2018 05:32 PM, Laura Abbott wrote:
> On 02/07/2018 07:10 AM, Alexey Skidanov wrote:
>>
>>
>> On 02/07/2018 04:58 PM, Laura Abbott wrote:
>>> On 02/06/2018 11:05 PM, Alexey Skidanov wrote:
>>>>
>>>>
>>>>> Yup, you've hit upon a key problem. Having fallbacks be stable
>>>>> was always a problem and the recommendation these days is to
>>>>> not rely on them. You can specify a heap at a time and fallback
>>>>> manually if you want that behavior.
>>>>>
>>>>> If you have a proposal to make fallbacks work reliably without
>>>>> overly complicating the ABI I'm happy to review it.
>>>>>
>>>>> Thanks,
>>>>> Laura
>>>>>
>>>> I think it's possible to "automate" the "manual fallback" behavior. But
>>>> the real issues is using heap id to specify the particular heap object.
>>>>
>>>> Current API (allocation IOCTL) requires to specify the particular heap
>>>> object by using heap id. From the other hand, the user space doesn't
>>>> control the heaps creation order and heap id assignment. So it may be
>>>> tricky, especially when more than one object of the same heap type is
>>>> created automatically.
>>>>
>>>> Thanks,
>>>> Alexey
>>>>
>>>>
>>>
>>> The query ioctl is designed to get the heap ID information without
>>> needing to rely on the linking order or anything else defined in
>>> the kernel.
>>>
>>> Thanks,
>>> Laura
>>
>> That is true. But if we have 2 *automatically created* heaps of the same
>> type, how userspace can distinguish between them?
>>
>> Thanks,
>> Alexey
>>
> 
> The query ioctl also gives the name which should be different
> for each heap. It's not ideal but the name/heap type are the best
> way to differentiate between heaps without resorting to hard
> coding.
> 
> Thanks,
> Laura

You are correct ... It will work (assuming that user space developer
knows where to look for the name :) )

So, the userspace may pass the list of pairs [heap type, name] (as part
of allocation ioctl) defining the fallback order.

Thanks,
Alexey

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

On 02/07/2018 05:32 PM, Laura Abbott wrote:
> On 02/07/2018 07:10 AM, Alexey Skidanov wrote:
>>
>>
>> On 02/07/2018 04:58 PM, Laura Abbott wrote:
>>> On 02/06/2018 11:05 PM, Alexey Skidanov wrote:
>>>>
>>>>
>>>>> Yup, you've hit upon a key problem. Having fallbacks be stable
>>>>> was always a problem and the recommendation these days is to
>>>>> not rely on them. You can specify a heap at a time and fallback
>>>>> manually if you want that behavior.
>>>>>
>>>>> If you have a proposal to make fallbacks work reliably without
>>>>> overly complicating the ABI I'm happy to review it.
>>>>>
>>>>> Thanks,
>>>>> Laura
>>>>>
>>>> I think it's possible to "automate" the "manual fallback" behavior. But
>>>> the real issues is using heap id to specify the particular heap object.
>>>>
>>>> Current API (allocation IOCTL) requires to specify the particular heap
>>>> object by using heap id. From the other hand, the user space doesn't
>>>> control the heaps creation order and heap id assignment. So it may be
>>>> tricky, especially when more than one object of the same heap type is
>>>> created automatically.
>>>>
>>>> Thanks,
>>>> Alexey
>>>>
>>>>
>>>
>>> The query ioctl is designed to get the heap ID information without
>>> needing to rely on the linking order or anything else defined in
>>> the kernel.
>>>
>>> Thanks,
>>> Laura
>>
>> That is true. But if we have 2 *automatically created* heaps of the same
>> type, how userspace can distinguish between them?
>>
>> Thanks,
>> Alexey
>>
> 
> The query ioctl also gives the name which should be different
> for each heap. It's not ideal but the name/heap type are the best
> way to differentiate between heaps without resorting to hard
> coding.
> 
> Thanks,
> Laura

You are correct ... It will work (assuming that user space developer
knows where to look for the name :) )

So, the userspace may pass the list of pairs [heap type, name] (as part
of allocation ioctl) defining the fallback order.

Thanks,
Alexey

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

On 02/07/2018 04:58 PM, Laura Abbott wrote:
> On 02/06/2018 11:05 PM, Alexey Skidanov wrote:
>>
>>
>>> Yup, you've hit upon a key problem. Having fallbacks be stable
>>> was always a problem and the recommendation these days is to
>>> not rely on them. You can specify a heap at a time and fallback
>>> manually if you want that behavior.
>>>
>>> If you have a proposal to make fallbacks work reliably without
>>> overly complicating the ABI I'm happy to review it.
>>>
>>> Thanks,
>>> Laura
>>>
>> I think it's possible to "automate" the "manual fallback" behavior. But
>> the real issues is using heap id to specify the particular heap object.
>>
>> Current API (allocation IOCTL) requires to specify the particular heap
>> object by using heap id. From the other hand, the user space doesn't
>> control the heaps creation order and heap id assignment. So it may be
>> tricky, especially when more than one object of the same heap type is
>> created automatically.
>>
>> Thanks,
>> Alexey
>>
>>
> 
> The query ioctl is designed to get the heap ID information without
> needing to rely on the linking order or anything else defined in
> the kernel.
> 
> Thanks,
> Laura

That is true. But if we have 2 *automatically created* heaps of the same
type, how userspace can distinguish between them?

Thanks,
Alexey

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

On 02/07/2018 04:58 PM, Laura Abbott wrote:
> On 02/06/2018 11:05 PM, Alexey Skidanov wrote:
>>
>>
>>> Yup, you've hit upon a key problem. Having fallbacks be stable
>>> was always a problem and the recommendation these days is to
>>> not rely on them. You can specify a heap at a time and fallback
>>> manually if you want that behavior.
>>>
>>> If you have a proposal to make fallbacks work reliably without
>>> overly complicating the ABI I'm happy to review it.
>>>
>>> Thanks,
>>> Laura
>>>
>> I think it's possible to "automate" the "manual fallback" behavior. But
>> the real issues is using heap id to specify the particular heap object.
>>
>> Current API (allocation IOCTL) requires to specify the particular heap
>> object by using heap id. From the other hand, the user space doesn't
>> control the heaps creation order and heap id assignment. So it may be
>> tricky, especially when more than one object of the same heap type is
>> created automatically.
>>
>> Thanks,
>> Alexey
>>
>>
> 
> The query ioctl is designed to get the heap ID information without
> needing to rely on the linking order or anything else defined in
> the kernel.
> 
> Thanks,
> Laura

That is true. But if we have 2 *automatically created* heaps of the same
type, how userspace can distinguish between them?

Thanks,
Alexey

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

> Yup, you've hit upon a key problem. Having fallbacks be stable
> was always a problem and the recommendation these days is to
> not rely on them. You can specify a heap at a time and fallback
> manually if you want that behavior.
> 
> If you have a proposal to make fallbacks work reliably without
> overly complicating the ABI I'm happy to review it.
> 
> Thanks,
> Laura
> 
I think it's possible to "automate" the "manual fallback" behavior. But
the real issues is using heap id to specify the particular heap object.

Current API (allocation IOCTL) requires to specify the particular heap
object by using heap id. From the other hand, the user space doesn't
control the heaps creation order and heap id assignment. So it may be
tricky, especially when more than one object of the same heap type is
created automatically.

Thanks,
Alexey

Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

> Yup, you've hit upon a key problem. Having fallbacks be stable
> was always a problem and the recommendation these days is to
> not rely on them. You can specify a heap at a time and fallback
> manually if you want that behavior.
> 
> If you have a proposal to make fallbacks work reliably without
> overly complicating the ABI I'm happy to review it.
> 
> Thanks,
> Laura
> 
I think it's possible to "automate" the "manual fallback" behavior. But
the real issues is using heap id to specify the particular heap object.

Current API (allocation IOCTL) requires to specify the particular heap
object by using heap id. From the other hand, the user space doesn't
control the heaps creation order and heap id assignment. So it may be
tricky, especially when more than one object of the same heap type is
created automatically.

Thanks,
Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 02/07/2018 01:56 AM, Laura Abbott wrote:
> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>
>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>> Any driver may access shared buffers, created by ion, using
>>>> dma_buf_vmap and
>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>> buffers into
>>>> the kernel virtual address space. The implementation of these API is
>>>> missing in
>>>> the current ion implementation.
>>>>
>>>> Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
>>>> ---
>>>
>>> No review from any other Intel developers? :(
>> Will add.
>>>
>>> Anyway, what in-tree driver needs access to these functions?
>> I'm not sure that there are the in-tree drivers using these functions
>> and ion as> buffer exporter because they are not implemented in ion :)
>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>> other buffer exporters.
> 
> It's still not clear why you need to implement these APIs.
How the importing kernel module may access the content of the buffer? :)
With the current ion implementation it's only possible by dma_buf_kmap,
mapping one page at a time. For pretty large buffers, it might have some
performance impact.
(Probably, the page by page mapping is the only way to access large
buffers on 32 bit systems, where the vmalloc range is very small. By the
way, the current ion dma_map_kmap doesn't really map only 1 page at a
time - it uses the result of vmap() that might fail on 32 bit systems.)

> Are you planning to use Ion with GPU drivers? I'm especially
> interested in this if you have a non-Android use case.
Yes, my use case is the non-Android one. But not with GPU drivers.
> 
> Thanks,
> Laura

Thanks,
Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 02/07/2018 01:56 AM, Laura Abbott wrote:
> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>
>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>> Any driver may access shared buffers, created by ion, using
>>>> dma_buf_vmap and
>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>> buffers into
>>>> the kernel virtual address space. The implementation of these API is
>>>> missing in
>>>> the current ion implementation.
>>>>
>>>> Signed-off-by: Alexey Skidanov 
>>>> ---
>>>
>>> No review from any other Intel developers? :(
>> Will add.
>>>
>>> Anyway, what in-tree driver needs access to these functions?
>> I'm not sure that there are the in-tree drivers using these functions
>> and ion as> buffer exporter because they are not implemented in ion :)
>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>> other buffer exporters.
> 
> It's still not clear why you need to implement these APIs.
How the importing kernel module may access the content of the buffer? :)
With the current ion implementation it's only possible by dma_buf_kmap,
mapping one page at a time. For pretty large buffers, it might have some
performance impact.
(Probably, the page by page mapping is the only way to access large
buffers on 32 bit systems, where the vmalloc range is very small. By the
way, the current ion dma_map_kmap doesn't really map only 1 page at a
time - it uses the result of vmap() that might fail on 32 bit systems.)

> Are you planning to use Ion with GPU drivers? I'm especially
> interested in this if you have a non-Android use case.
Yes, my use case is the non-Android one. But not with GPU drivers.
> 
> Thanks,
> Laura

Thanks,
Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 01/31/2018 03:00 PM, Greg KH wrote:

On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:

Any driver may access shared buffers, created by ion, using dma_buf_vmap and
dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
the kernel virtual address space. The implementation of these API is missing in
the current ion implementation.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---


No review from any other Intel developers? :(

Will add.


Anyway, what in-tree driver needs access to these functions?
I'm not sure that there are the in-tree drivers using these functions 
and ion as buffer exporter because they are not implemented in ion :) 
But there are some in-tree drivers using these APIs (gpu drivers) with 
other buffer exporters.


And are you sure that you don't need to do any "real" logic in the
vmap/vunmap calls?  That feels like there would be some reference
counting problems here.
dma_buf_start_cpu_access is called before the call to dma_buf_vmap. It 
actually increments the reference count. dma_buf_end_cpu_access is 
called after the dma_buf_vunmap. It actually decrements the reference 
count.>>

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
  {
  }
  
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)

+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;


Just call ion_dma_buf_kmap(dmabuf, 0)?

Sure.


Again, please get this reviewed by someone else in Intel first.  Don't
ignore the resources you have, to do so would be foolish :)

Sure. Will do.


thanks,

greg k-h



Thanks,
Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 01/31/2018 03:00 PM, Greg KH wrote:

On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:

Any driver may access shared buffers, created by ion, using dma_buf_vmap and
dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
the kernel virtual address space. The implementation of these API is missing in
the current ion implementation.

Signed-off-by: Alexey Skidanov 
---


No review from any other Intel developers? :(

Will add.


Anyway, what in-tree driver needs access to these functions?
I'm not sure that there are the in-tree drivers using these functions 
and ion as buffer exporter because they are not implemented in ion :) 
But there are some in-tree drivers using these APIs (gpu drivers) with 
other buffer exporters.


And are you sure that you don't need to do any "real" logic in the
vmap/vunmap calls?  That feels like there would be some reference
counting problems here.
dma_buf_start_cpu_access is called before the call to dma_buf_vmap. It 
actually increments the reference count. dma_buf_end_cpu_access is 
called after the dma_buf_vunmap. It actually decrements the reference 
count.>>

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
  {
  }
  
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)

+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;


Just call ion_dma_buf_kmap(dmabuf, 0)?

Sure.


Again, please get this reviewed by someone else in Intel first.  Don't
ignore the resources you have, to do so would be foolish :)

Sure. Will do.


thanks,

greg k-h



Thanks,
Alexey

[PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

Any driver may access shared buffers, created by ion, using dma_buf_vmap and
dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
the kernel virtual address space. The implementation of these API is missing in
the current ion implementation.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
 drivers/staging/android/ion/ion.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap_atomic = ion_dma_buf_kunmap,
.map = ion_dma_buf_kmap,
.unmap = ion_dma_buf_kunmap,
+   .vmap = ion_dma_buf_vmap,
+   .vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

[PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

Any driver may access shared buffers, created by ion, using dma_buf_vmap and
dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
the kernel virtual address space. The implementation of these API is missing in
the current ion implementation.

Signed-off-by: Alexey Skidanov 
---
 drivers/staging/android/ion/ion.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap_atomic = ion_dma_buf_kunmap,
.map = ion_dma_buf_kmap,
.unmap = ion_dma_buf_kunmap,
+   .vmap = ion_dma_buf_vmap,
+   .vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

[PATCH v2] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

dma_buf_vmap and dma_buf_vunmap allow drivers to access buffers, created by ion.

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
Changes in v1:
 - Added changelog text 


 drivers/staging/android/ion/ion.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap_atomic = ion_dma_buf_kunmap,
.map = ion_dma_buf_kmap,
.unmap = ion_dma_buf_kunmap,
+   .vmap = ion_dma_buf_vmap,
+   .vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

[PATCH v2] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

dma_buf_vmap and dma_buf_vunmap allow drivers to access buffers, created by ion.

Signed-off-by: Alexey Skidanov 
---
Changes in v1:
 - Added changelog text 


 drivers/staging/android/ion/ion.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap_atomic = ion_dma_buf_kunmap,
.map = ion_dma_buf_kmap,
.unmap = ion_dma_buf_kunmap,
+   .vmap = ion_dma_buf_vmap,
+   .vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

[PATCH] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

Signed-off-by: Alexey Skidanov <alexey.skida...@intel.com>
---
 drivers/staging/android/ion/ion.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap_atomic = ion_dma_buf_kunmap,
.map = ion_dma_buf_kmap,
.unmap = ion_dma_buf_kunmap,
+   .vmap = ion_dma_buf_vmap,
+   .vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

[PATCH] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

Signed-off-by: Alexey Skidanov 
---
 drivers/staging/android/ion/ion.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, 
unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+   struct ion_buffer *buffer = dmabuf->priv;
+
+   return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap_atomic = ion_dma_buf_kunmap,
.map = ion_dma_buf_kmap,
.unmap = ion_dma_buf_kunmap,
+   .vmap = ion_dma_buf_vmap,
+   .vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

Hi,

According to my understanding, the allocation fall back order
completely depends on heap->id that is assigned during the heap
creation:
   plist_for_each_entry(heap, >heaps, node) {
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
continue;
buffer = ion_buffer_create(heap, dev, len, flags);
if (!IS_ERR(buffer))
break;
}

On creation, each heap is added to the priority list according to the
priority assigned:

...
static int heap_id;
...
void ion_device_add_heap(struct ion_heap *heap)
{
...
heap->id = heap_id++;
...
}


The order of creation is the order of linkage defined in the Makefile.
Thus, by default, we have:

heap id 2, type ION_HEAP_TYPE_DMA
heap id 1, type ION_HEAP_TYPE_SYSTEM
heap id 0, type ION_HEAP_TYPE_SYSTEM_CONTIG

Changing the linkage order:
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index bb30bf8..e05052c 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_ION) +=   ion.o ion-ioctl.o ion_heap.o
-obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
 obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
 obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
 obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o

I get the following order:

heap id 2, type ION_HEAP_TYPE_SYSTEM
heap id 1, type ION_HEAP_TYPE_SYSTEM_CONTIG
heap id 0, type ION_HEAP_TYPE_DMA

So, if the user specifies more than 1 heap in the heap_id_mask during
allocation, the allocation fall back order completely depends on the
order of linkage. Probably, it's better to let the user to define the
fall back order (and NOT to be dependent on the linkage order at all)
?

Thanks,
Alexey

staging: ion: ION allocation fall back order depends on heap linkage order

[Alexey Skidanov]

Hi,

According to my understanding, the allocation fall back order
completely depends on heap->id that is assigned during the heap
creation:
   plist_for_each_entry(heap, >heaps, node) {
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
continue;
buffer = ion_buffer_create(heap, dev, len, flags);
if (!IS_ERR(buffer))
break;
}

On creation, each heap is added to the priority list according to the
priority assigned:

...
static int heap_id;
...
void ion_device_add_heap(struct ion_heap *heap)
{
...
heap->id = heap_id++;
...
}


The order of creation is the order of linkage defined in the Makefile.
Thus, by default, we have:

heap id 2, type ION_HEAP_TYPE_DMA
heap id 1, type ION_HEAP_TYPE_SYSTEM
heap id 0, type ION_HEAP_TYPE_SYSTEM_CONTIG

Changing the linkage order:
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index bb30bf8..e05052c 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_ION) +=   ion.o ion-ioctl.o ion_heap.o
-obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
 obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
 obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
 obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
+obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o

I get the following order:

heap id 2, type ION_HEAP_TYPE_SYSTEM
heap id 1, type ION_HEAP_TYPE_SYSTEM_CONTIG
heap id 0, type ION_HEAP_TYPE_DMA

So, if the user specifies more than 1 heap in the heap_id_mask during
allocation, the allocation fall back order completely depends on the
order of linkage. Probably, it's better to let the user to define the
fall back order (and NOT to be dependent on the linkage order at all)
?

Thanks,
Alexey