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@xxxxxxxxxxx>:
> On Thu, Sep 1, 2016 at 12:02 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>>  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