Skip to contentSkip to conversation

ANDROID: binder: change down_write to down_read

PATCH Superseded Minchan Kim 3 participants 4 messages Mar 28, 2018 Android drivers

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Minchan Kim

author
#0 lore raw

PATCH 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 bider_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.

Android suffers from mmap_sem contention so let's reduce mmap_sem down_write.

Cc Martijn Coenen <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 2 +-
 drivers/android/binder_alloc.c | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +1 -1
index 764b63a5aade..9a14c6dd60c4 100644
@@ -4722,7 +4722,7 @@ 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) & ~VM_MAYWRITE;
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
drivers/android/binder_alloc.c +5 -3
index 5a426c877dfb..a184bf12eb15 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;
 	}
 

@@ -229,6 +229,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		goto err_no_vma;
 	}
 
+	WARN_ON_ONCE(vma && !(vma->vm_flags & VM_MIXEDMAP));
+
 	for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
 		int ret;
 		bool on_lru;

@@ -288,7 +290,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 +323,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.rc1.321.gba9d0f2565-goog
maintainer
#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 bider_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.

Android suffers from mmap_sem contention so let's reduce mmap_sem down_write.

Cc Martijn Coenen <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 2 +-
 drivers/android/binder_alloc.c | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +1 -1
index 764b63a5aade..9a14c6dd60c4 100644
@@ -4722,7 +4722,7 @@ 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) & ~VM_MAYWRITE;
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
drivers/android/binder_alloc.c +1 -1
index 5a426c877dfb..a184bf12eb15 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;
 	}

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

Quoted 4 lines
drivers/android/binder_alloc.c +1 -0
@@ -229,6 +229,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		goto err_no_vma;
 	}
 
+	WARN_ON_ONCE(vma && !(vma->vm_flags & VM_MIXEDMAP));

What can we do with this warning? What about systems that run in 'panic on warn' mode? Is this something we should handle directly instead?

thanks,

greg k-h

author
#2 lore raw
Quoted 47 lines · Greg Kroah-Hartman
Quoted 46 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 bider_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.

Android suffers from mmap_sem contention so let's reduce mmap_sem down_write.

Cc Martijn Coenen <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 2 +-
 drivers/android/binder_alloc.c | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +1 -1
index 764b63a5aade..9a14c6dd60c4 100644
@@ -4722,7 +4722,7 @@ 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) & ~VM_MAYWRITE;
 	vma->vm_ops = &binder_vm_ops;
 	vma->vm_private_data = proc;
drivers/android/binder_alloc.c +1 -1
index 5a426c877dfb..a184bf12eb15 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;
 	}
Quoted 8 lines

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

Quoted 5 lines
drivers/android/binder_alloc.c +1 -0
@@ -229,6 +229,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
 		goto err_no_vma;
 	}
 
+	WARN_ON_ONCE(vma && !(vma->vm_flags & VM_MIXEDMAP));

What can we do with this warning? What about systems that run in 'panic on warn' mode? Is this something we should handle directly instead?

Hmm, it seems we don't need above warning there because vm_insert_page will hit BUG_ON in that case. I will remove it at respin. Thanks for the review, Greg.

I will wait other binder maintainer's opinion about below question.

Quoted 2 lines
Quoted 2 lines

it seems
every binder buffers should be mapped in advance by binder_mmap.

#3 lore raw
In reply to Minchan Kim
Quoted 4 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.

Yeah this is correct - before doing any binder transactions binder_mmap() must be called, and we do fail transactions if that hasn't been done yet. LGTM once you're removed the WARN_ON.

Martijn

Quoted 72 lines

It means we could set VM_MIXEDMAP in bider_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.

Android suffers from mmap_sem contention so let's reduce mmap_sem down_write.

Cc Martijn Coenen <[email protected]>
Cc Todd Kjos <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/android/binder.c       | 2 +-
 drivers/android/binder_alloc.c | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/android/binder.c +1 -1
index 764b63a5aade..9a14c6dd60c4 100644
@@ -4722,7 +4722,7 @@ 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) & ~VM_MAYWRITE;
        vma->vm_ops = &binder_vm_ops;
        vma->vm_private_data = proc;
drivers/android/binder_alloc.c +5 -4
index 5a426c877dfb..a184bf12eb15 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;
        }
 

@@ -229,6 +229,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
                goto err_no_vma;
        }
 
+       WARN_ON_ONCE(vma && !(vma->vm_flags & VM_MIXEDMAP));
+
        for (page_addr = start; page_addr < end; page_addr += PAGE_SIZE) {
                int ret;
                bool on_lru;

@@ -288,7 +290,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 +323,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;
--

2.17.0.rc1.321.gba9d0f2565-goog