[PATCH] android: binder: use VM_ALLOC to get vm area.

[PATCH] android: binder: use VM_ALLOC to get vm area.

[Ganesh Mahendran]

VM_IOREMAP is used to access hardware through a mechanism called
I/O mapped memory. Android binder is a IPC machanism which will
not access I/O memory.

Also VM_IOREMAP has alignment requiement which may not needed in
binder.
----
__get_vm_area_node()
{
...
    if (flags & VM_IOREMAP)
        align = 1ul << clamp_t(int, fls_long(size),
           PAGE_SHIFT, IOREMAP_MAX_ORDER);
...
}
----

This patch use VM_ALLOC to get vm area.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 16288e7..3511d5c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		goto err_already_mapped;
 	}
 
-	area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
+	area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
 	if (area == NULL) {
 		ret = -ENOMEM;
 		failure_string = "get_vm_area";
-- 
1.9.1

Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

[Greg KH]

On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
> VM_IOREMAP is used to access hardware through a mechanism called
> I/O mapped memory. Android binder is a IPC machanism which will
> not access I/O memory.
> 
> Also VM_IOREMAP has alignment requiement which may not needed in
> binder.
> ----
> __get_vm_area_node()
> {
> ...
>     if (flags & VM_IOREMAP)
>         align = 1ul << clamp_t(int, fls_long(size),
>            PAGE_SHIFT, IOREMAP_MAX_ORDER);
> ...
> }
> ----
> 
> This patch use VM_ALLOC to get vm area.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> ---
>  drivers/android/binder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 16288e7..3511d5c 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>  		goto err_already_mapped;
>  	}
>  
> -	area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
> +	area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>  	if (area == NULL) {
>  		ret = -ENOMEM;
>  		failure_string = "get_vm_area";

What change have you noticed with this patch?  Have you tested it?
Found that previously reserved iomemory is now free for binder to use
where it wasn't?  What kind of change does the system now run as because
of this?

And are you _sure_ the alignment requirement isn't needed for binder?
Have you verified this with the userspace binder library?

This is messy, tricky, stuff, I'm loath to change it without loads of
testing having happened...

thanks,

greg k-h

Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

[Arve Hjønnevåg]

On Thu, Sep 1, 2016 at 12:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>> VM_IOREMAP is used to access hardware through a mechanism called
>> I/O mapped memory. Android binder is a IPC machanism which will
>> not access I/O memory.
>>
>> Also VM_IOREMAP has alignment requiement which may not needed in
>> binder.
>> ----
>> __get_vm_area_node()
>> {
>> ...
>>     if (flags & VM_IOREMAP)
>>         align = 1ul << clamp_t(int, fls_long(size),
>>            PAGE_SHIFT, IOREMAP_MAX_ORDER);
>> ...
>> }
>> ----
>>
>> This patch use VM_ALLOC to get vm area.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>> ---
>>  drivers/android/binder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 16288e7..3511d5c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>>               goto err_already_mapped;
>>       }
>>
>> -     area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>> +     area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>>       if (area == NULL) {
>>               ret = -ENOMEM;
>>               failure_string = "get_vm_area";
>
> What change have you noticed with this patch?  Have you tested it?
> Found that previously reserved iomemory is now free for binder to use
> where it wasn't?  What kind of change does the system now run as because
> of this?
>
> And are you _sure_ the alignment requirement isn't needed for binder?
> Have you verified this with the userspace binder library?
>
> This is messy, tricky, stuff, I'm loath to change it without loads of
> testing having happened...
>
> thanks,
>
> greg k-h

There is no alignment requirement on this area unless
cache_is_vipt_aliasing returns true. In that case the area needs to be
aligned with vma->vm_start which is done manually in the code right
after this allocation. If there are no other side effects of changing
this flag this change should be safe, but please run all the tests at
https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/
to test it.

-- 
Arve Hjønnevåg

Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

[Ganesh Mahendran]

Hi, Greg:

2016-09-02 3:02 GMT+08:00 Greg KH <gregkh@linuxfoundation.org>:
> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>> VM_IOREMAP is used to access hardware through a mechanism called
>> I/O mapped memory. Android binder is a IPC machanism which will
>> not access I/O memory.
>>
>> Also VM_IOREMAP has alignment requiement which may not needed in
>> binder.
>> ----
>> __get_vm_area_node()
>> {
>> ...
>>     if (flags & VM_IOREMAP)
>>         align = 1ul << clamp_t(int, fls_long(size),
>>            PAGE_SHIFT, IOREMAP_MAX_ORDER);
>> ...
>> }
>> ----
>>
>> This patch use VM_ALLOC to get vm area.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>> ---
>>  drivers/android/binder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 16288e7..3511d5c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>>               goto err_already_mapped;
>>       }
>>
>> -     area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>> +     area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>>       if (area == NULL) {
>>               ret = -ENOMEM;
>>               failure_string = "get_vm_area";
>
> What change have you noticed with this patch?  Have you tested it?
> Found that previously reserved iomemory is now free for binder to use
> where it wasn't?  What kind of change does the system now run as because
> of this?

I did some sanity test in Android system.  I will do more test using:
https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/

>
> And are you _sure_ the alignment requirement isn't needed for binder?
> Have you verified this with the userspace binder library?

I will do the binder test.

Thanks.

>
> This is messy, tricky, stuff, I'm loath to change it without loads of
> testing having happened...
>
> thanks,
>
> greg k-h

Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

[Ganesh Mahendran]

2016-09-02 3:59 GMT+08:00 Arve Hjønnevåg <arve@android.com>:
> On Thu, Sep 1, 2016 at 12:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>>> VM_IOREMAP is used to access hardware through a mechanism called
>>> I/O mapped memory. Android binder is a IPC machanism which will
>>> not access I/O memory.
>>>
>>> Also VM_IOREMAP has alignment requiement which may not needed in
>>> binder.
>>> ----
>>> __get_vm_area_node()
>>> {
>>> ...
>>>     if (flags & VM_IOREMAP)
>>>         align = 1ul << clamp_t(int, fls_long(size),
>>>            PAGE_SHIFT, IOREMAP_MAX_ORDER);
>>> ...
>>> }
>>> ----
>>>
>>> This patch use VM_ALLOC to get vm area.
>>>
>>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>>> ---
>>>  drivers/android/binder.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>>> index 16288e7..3511d5c 100644
>>> --- a/drivers/android/binder.c
>>> +++ b/drivers/android/binder.c
>>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>>>               goto err_already_mapped;
>>>       }
>>>
>>> -     area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>>> +     area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>>>       if (area == NULL) {
>>>               ret = -ENOMEM;
>>>               failure_string = "get_vm_area";
>>
>> What change have you noticed with this patch?  Have you tested it?
>> Found that previously reserved iomemory is now free for binder to use
>> where it wasn't?  What kind of change does the system now run as because
>> of this?
>>
>> And are you _sure_ the alignment requirement isn't needed for binder?
>> Have you verified this with the userspace binder library?
>>
>> This is messy, tricky, stuff, I'm loath to change it without loads of
>> testing having happened...
>>
>> thanks,
>>
>> greg k-h
>
> There is no alignment requirement on this area unless
> cache_is_vipt_aliasing returns true. In that case the area needs to be
> aligned with vma->vm_start which is done manually in the code right
> after this allocation. If there are no other side effects of changing
> this flag this change should be safe, but please run all the tests at
> https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/
> to test it.

Thanks for your suggestion.
I only did some android app performance test. I will do more binder test.

Thanks.

>
> --
> Arve Hjønnevåg