ANDROID: binder: change down_write to down_read

PATCH In review Minchan Kim 4 participants 10 messages May 7, 2018 – May 17, 2018 Android drivers

Share & tools

Minchan Kim

author

2018-05-07 14:15 UTC #0 lore raw

PATCH v6 1/1 ANDROID: binder: change down_write to down_read

binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.

Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.

Cc Ganesh Mahendran <[email protected]>

Cc Joe Perches <[email protected]>

Cc Arve Hjønnevåg <[email protected]>

Cc Todd Kjos <[email protected]>

Reviewed-by Martijn Coenen <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Reviewed-by Joel Fernandes (Google) <[email protected]>

---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)

drivers/android/binder.c +3 -1

index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;

drivers/android/binder_alloc.c +3 -3

index 5a426c877dfb..4f382d51def1 100644
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
 		vma = alloc->vma;
 	}

@@ -288,7 +288,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		/* vm_insert_page does not seem to increment the refcount */
 	}
 	if (mm) {
-		up_write(&mm->mmap_sem);
+		up_read(&mm->mmap_sem);
 		mmput(mm);
 	}
 	return 0;

@@ -321,7 +321,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 	}
 err_no_vma:
 	if (mm) {
-		up_write(&mm->mmap_sem);
+		up_read(&mm->mmap_sem);
 		mmput(mm);
 	}
 	return vma ? -ENOMEM : -ESRCH;

Signature · 1 line

2.17.0.441.gb46fe60e1d-goog

Joel Fernandes

Reviewed-by on v6 1/1

2018-05-07 17:28 UTC #1 lore raw

Quoted 47 lines · Minchan Kim

binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644

drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222

Quoted 5 lines

drivers/android/binder_alloc.c +1 -1

@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);

Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).

Otherwise looks good to me:

Reviewed-by Joel Fernandes (Google) <[email protected]>

thanks,

- Joel

Minchan Kim

author

2018-05-08 10:51 UTC #2 lore raw

Quoted 50 lines · Joel Fernandes

Quoted 49 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 

Quoted 7 lines

drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 3 lines
drivers/android/binder_alloc.c +1 -1
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).

It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).

Quoted 3 lines

Otherwise looks good to me:
Reviewed-by Joel Fernandes (Google) <[email protected]>

Thanks, Joel!

Joel Fernandes

2018-05-08 23:08 UTC #3 lore raw

Quoted 59 lines · Minchan Kim

Quoted 50 lines · Joel Fernandes
Quoted 49 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
Quoted 7 lines
drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 3 lines
drivers/android/binder_alloc.c +1 -1
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).

Quoted 2 lines

It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).

When you say unmap, are you talking about pages being unmapped from the VMA or something else?

For the collapse/split part, the binder VMA (vm_area_struct) itself isn't changed after the initial mmap (AFAIK) so I don't see a need for protection there.

Again, I'm not against the patch but thought its good to clarify for myself what the usage of the lock is, here.

thanks,

- Joel

Minchan Kim

author

2018-05-09 06:40 UTC #4 lore raw

Quoted 60 lines · Joel Fernandes

Quoted 59 lines · Minchan Kim
Quoted 57 lines · Joel Fernandes
Quoted 47 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 5 lines
drivers/android/binder_alloc.c +1 -1
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).

Quoted 5 lines

Quoted 2 lines
It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).
When you say unmap, are you talking about pages being unmapped from the VMA or something else?

I mean to destroy vm_area_struct itself as well as unmap pages.

Quoted 4 lines

For the collapse/split part, the binder VMA (vm_area_struct) itself isn't changed after the initial mmap (AFAIK) so I don't see a need for protection there.

There is no way to unmap in runtime? What happens if some buggy applications do unmap by mistake?
Cannot we access those B's vma from A context?
If B process is exiting, the VMA would be gone while A is accessing.

Quoted 3 lines

Again, I'm not against the patch but thought its good to clarify for myself what the usage of the lock is, here.

I know. :)

Joel Fernandes

2018-05-09 23:19 UTC #5 lore raw

Quoted 70 lines · Minchan Kim

Quoted 60 lines · Joel Fernandes
Quoted 59 lines · Minchan Kim
Quoted 57 lines · Joel Fernandes
Quoted 47 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 5 lines
drivers/android/binder_alloc.c +1 -1
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).
Quoted 5 lines
Quoted 2 lines
It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).
When you say unmap, are you talking about pages being unmapped from the VMA or something else?
I mean to destroy vm_area_struct itself as well as unmap pages.
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
For the collapse/split part, the binder VMA (vm_area_struct) itself isn't changed after the initial mmap (AFAIK) so I don't see a need for protection there.
There is no way to unmap in runtime? What happens if some buggy applications do unmap by mistake?
Cannot we access those B's vma from A context?
If B process is exiting, the VMA would be gone while A is accessing.

If the VMA is gone, then why is it a good idea to map pages into it anyway?

About the unmap at runtime part, your commit message was a bit confusing. You said "every binder buffers should be mapped in advance by binder_mmap." but I think the new binder shrinker mechanism doesn't make that true anymore.

The unmap by mistake point is a valid one I guess.

thanks,

- Joel

Minchan Kim

2018-05-15 06:27 UTC #6 lore raw

Hi Joel,

Sorry for the late response. I was off.

Quoted 75 lines · Joel Fernandes

Quoted 74 lines · Minchan Kim
Quoted 60 lines · Joel Fernandes
Quoted 59 lines · Minchan Kim
Quoted 57 lines · Joel Fernandes
Quoted 47 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 5 lines
drivers/android/binder_alloc.c +1 -1
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).
Quoted 5 lines
Quoted 2 lines
It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).
When you say unmap, are you talking about pages being unmapped from the VMA or something else?
I mean to destroy vm_area_struct itself as well as unmap pages.
Quoted 4 lines
For the collapse/split part, the binder VMA (vm_area_struct) itself isn't changed after the initial mmap (AFAIK) so I don't see a need for protection there.

Quoted 6 lines

Quoted 4 lines
There is no way to unmap in runtime? What happens if some buggy applications do unmap by mistake?
Cannot we access those B's vma from A context?
If B process is exiting, the VMA would be gone while A is accessing.
If the VMA is gone, then why is it a good idea to map pages into it anyway?

Hmm, sorry about that. I couldn't understand your point.

Quoted 4 lines

About the unmap at runtime part, your commit message was a bit confusing. You said "every binder buffers should be mapped in advance by binder_mmap." but I think the new binder shrinker mechanism doesn't make that true anymore.

It's good point. I didn't know know that.
When I see binder_vm_fault, it emits SIGBUS. That means shrinker cannot zap pages process is using, I think. IOW, every pages for binder are mapped at mmap time and is never mapped in runtime by page fault. Right?

Thanks.

Quoted 7 lines

The unmap by mistake point is a valid one I guess.
thanks,
- Joel

Martijn Coenen

2018-05-15 07:46 UTC #7 lore raw

Quoted 80 lines · Minchan Kim

Hi Joel,
Sorry for the late response. I was off.
Quoted 75 lines · Joel Fernandes
Quoted 74 lines · Minchan Kim
Quoted 60 lines · Joel Fernandes
Quoted 59 lines · Minchan Kim
Quoted 57 lines · Joel Fernandes
Quoted 47 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
            failure_string = "bad vm_flags";
            goto err_bad_arg;
    }
-   vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+   vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+   vma->vm_flags &= ~VM_MAYWRITE;
+
    vma->vm_ops = &binder_vm_ops;
    vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 5 lines
drivers/android/binder_alloc.c +1 -1
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
            mm = alloc->vma_vm_mm;
 
    if (mm) {
-           down_write(&mm->mmap_sem);
+           down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).
Quoted 5 lines
Quoted 2 lines
It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).
When you say unmap, are you talking about pages being unmapped from the VMA or something else?
I mean to destroy vm_area_struct itself as well as unmap pages.
Quoted 4 lines
For the collapse/split part, the binder VMA (vm_area_struct) itself isn't changed after the initial mmap (AFAIK) so I don't see a need for protection there.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
There is no way to unmap in runtime? What happens if some buggy applications do unmap by mistake?
Cannot we access those B's vma from A context?
If B process is exiting, the VMA would be gone while A is accessing.
If the VMA is gone, then why is it a good idea to map pages into it anyway?
Hmm, sorry about that. I couldn't understand your point.

I think Joel may have meant that if the VMA is gone (forex because the process unmaps or dies), we shouldn't allocate pages into that VMA anymore. I don't think we do that currently - we check if the vma is still valid before allocating.

Quoted 10 lines

Quoted 5 lines
About the unmap at runtime part, your commit message was a bit confusing. You said "every binder buffers should be mapped in advance by binder_mmap." but I think the new binder shrinker mechanism doesn't make that true anymore.
It's good point. I didn't know know that.
When I see binder_vm_fault, it emits SIGBUS. That means shrinker cannot zap pages process is using, I think. IOW, every pages for binder are mapped at mmap time and is never mapped in runtime by page fault. Right?

Right - the address range is allocated once, and an initial amount of pages is mapped into it. For every transaction into that process, we will see if there's enough pages, and if not allocate so that we have enough of them - so this is not done by page fault. The shrinker won't touch pages for which a transaction is in progress. Of course a process itself could still try to read from an unallocated address, but in that case returning SIGBUS and having that process crash seems fine.

I'm also not sure the read lock is needed, but I would need to read a whole lot more code to convince myself it's not.

Quoted 10 lines

Thanks.
Quoted 7 lines
The unmap by mistake point is a valid one I guess.
thanks,
- Joel

Minchan Kim

2018-05-15 08:56 UTC #8 lore raw

On Tue, May 15, 2018 at 09:46:01AM +0200, Martijn Coenen wrote:

< snip >

Quoted 9 lines

Quoted 8 lines
Quoted 3 lines
About the unmap at runtime part, your commit message was a bit confusing. You said "every binder buffers should be mapped in advance by binder_mmap." but I think the new binder shrinker mechanism doesn't make that true anymore.
It's good point. I didn't know know that.
When I see binder_vm_fault, it emits SIGBUS. That means shrinker cannot zap pages process is using, I think. IOW, every pages for binder are mapped at mmap time and is never mapped in runtime by page fault. Right?

Quoted 8 lines

Right - the address range is allocated once, and an initial amount of pages is mapped into it. For every transaction into that process, we will see if there's enough pages, and if not allocate so that we have enough of them - so this is not done by page fault. The shrinker won't touch pages for which a transaction is in progress. Of course a process itself could still try to read from an unallocated address, but in that case returning SIGBUS and having that process crash seems fine.

Thanks for the confirmation.

Quoted 3 lines

I'm also not sure the read lock is needed, but I would need to read a whole lot more code to convince myself it's not.

For page zapping, we shouldn't need mmap_sem write lock. We should replace it with down_read/write, too.

Thanks.

Joel Fernandes

2018-05-17 03:23 UTC #9 lore raw

In reply to Minchan Kim

Quoted 80 lines · Minchan Kim

Hi Joel,
Sorry for the late response. I was off.
Quoted 75 lines · Joel Fernandes
Quoted 74 lines · Minchan Kim
Quoted 60 lines · Joel Fernandes
Quoted 59 lines · Minchan Kim
Quoted 57 lines · Joel Fernandes
Quoted 47 lines · Minchan Kim
binder_update_page_range needs down_write of mmap_sem because vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless it is set. However, when I profile binder working, it seems every binder buffers should be mapped in advance by binder_mmap. It means we could set VM_MIXEDMAP in binder_mmap time which is already hold a mmap_sem as down_write so binder_update_page_range doesn't need to hold a mmap_sem as down_write.
Please use proper API down_read. It would help mmap_sem contention problem as well as fixing down_write abuse.
Ganesh Mahendran tested app launching and binder throughput test and he said he couldn't find any problem and I did binder latency test per Greg KH request(Thanks Martijn to teach me how I can do) I cannot find any problem, too.
Cc Ganesh Mahendran <[email protected]>
Cc Joe Perches <[email protected]>
Cc Arve Hjønnevåg <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reviewed-by Martijn Coenen <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 4 +++-
 drivers/android/binder_alloc.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +3 -1
index 4eab5be3d00f..7b8e96f60719 100644
@@ -4730,7 +4730,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 		failure_string = "bad vm_flags";
 		goto err_bad_arg;
 	}
-	vma->vm_flags = (vma->vm_flags | VM_DONTCOPY) & ~VM_MAYWRITE;
+	vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
 
drivers/android/binder_alloc.c +0 -0
index 5a426c877dfb..4f382d51def1 100644
drivers/android/binder_alloc.c static int binder_update_page_range(struct binder_alloc *alloc, int allocate, · line 222
Quoted 5 lines
drivers/android/binder_alloc.c +1 -1
@@ -219,7 +219,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		mm = alloc->vma_vm_mm;
 
 	if (mm) {
-		down_write(&mm->mmap_sem);
+		down_read(&mm->mmap_sem);
Nice. Is there a need to hold the reader-lock at all here? Just curious what else is it protecting (here or in vm_insert_page).
Quoted 5 lines
Quoted 2 lines
It should protect vm_area_struct. IOW, when we try insert page into virtual address area, vma shouldn't be changed(ie, unmap/collapse/split).
When you say unmap, are you talking about pages being unmapped from the VMA or something else?
I mean to destroy vm_area_struct itself as well as unmap pages.
Quoted 4 lines
For the collapse/split part, the binder VMA (vm_area_struct) itself isn't changed after the initial mmap (AFAIK) so I don't see a need for protection there.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
There is no way to unmap in runtime? What happens if some buggy applications do unmap by mistake?
Cannot we access those B's vma from A context?
If B process is exiting, the VMA would be gone while A is accessing.
If the VMA is gone, then why is it a good idea to map pages into it anyway?
Hmm, sorry about that. I couldn't understand your point.

Sorry, I was just saying that we shouldn't hit a case where a process is dead and we are trying to map pages into its VMA.

But considering this is a read mostly usecase, I think keeping the read lock as you are doing would be fine for performance.

Quoted 8 lines

Quoted 3 lines
About the unmap at runtime part, your commit message was a bit confusing. You said "every binder buffers should be mapped in advance by binder_mmap." but I think the new binder shrinker mechanism doesn't make that true anymore.
It's good point. I didn't know know that.
When I see binder_vm_fault, it emits SIGBUS. That means shrinker cannot zap pages process is using, I think. IOW, every pages for binder are mapped at mmap time and is never mapped in runtime by page fault. Right?

Hmm, so the shrinker doesn't touch pages that the process are using. The shrinker maintains an LRU list which is populated only once pages are not needed anymore.

Before the shrinker was added, the pages would be unmapped and freed when they were no longer needed. After the shrinker was added, they are kept allocated and mapped, and are added to an LRU list. The exact diff that does this is from commit f2517eb76f ("android: binder: Add global lru shrinker to binder") is below.

Coming back the point of pages mapped in advance, my assertion is - both in previous code before the shrinker was added and in current code, the pages are not all to be mapped in advance. Before it was aggressively freed and unmapped, now its a lazy approach and I believe the point is to avoid contention on locks used in the aggressive approach.

thanks,

- Joel
-----------------

@@ -264,16 +289,21 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
free_range:
for (page_addr = end - PAGE_SIZE; page_addr >= start;
page_addr -= PAGE_SIZE) {
+ bool ret;
+
page = &alloc->pages[(page_addr - alloc->buffer) / PAGE_SIZE];
- if (vma)
- zap_page_range(vma, (uintptr_t)page_addr +
- alloc->user_buffer_offset, PAGE_SIZE);
+
+ ret = list_lru_add(&binder_alloc_lru, &page->lru);
+ WARN_ON(!ret);
+ continue;
+
err_vm_insert_page_failed:
unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
err_map_kernel_failed:
- __free_page(*page);
- *page = NULL;
+ __free_page(page->page_ptr);
+ page->page_ptr = NULL;
err_alloc_page_failed:
+err_page_ptr_cleared:
;
}
err_no_vma: