This reverts commit 44e602b4e52f70f04620bbbf4fe46ecb40170bde.
This caused a performance regression particularly when pages are getting
reclaimed. We don't need to acquire the mmap_lock to determine when the
binder buffer has been fully initialized. A subsequent patch will bring
back the lockless approach for this.
[cmllamas: resolved trivial conflicts with renaming of alloc->mm]
Cc: Liam Howlett <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Signed-off-by: Carlos Llamas <[email protected]>
---
drivers/android/binder_alloc.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 55a3c3c2409f..92c814ec44fe 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -380,15 +380,12 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
size_t size, data_offsets_size;
int ret;
- mmap_read_lock(alloc->mm);
if (!binder_alloc_get_vma(alloc)) {
- mmap_read_unlock(alloc->mm);
binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
"%d: binder_alloc_buf, no vma\n",
alloc->pid);
return ERR_PTR(-ESRCH);
}
- mmap_read_unlock(alloc->mm);
data_offsets_size = ALIGN(data_size, sizeof(void *)) +
ALIGN(offsets_size, sizeof(void *));
@@ -916,25 +913,17 @@ void binder_alloc_print_pages(struct seq_file *m,
* Make sure the binder_alloc is fully initialized, otherwise we might
* read inconsistent state.
*/
-
- mmap_read_lock(alloc->mm);
- if (binder_alloc_get_vma(alloc) == NULL) {
- mmap_read_unlock(alloc->mm);
- goto uninitialized;
- }
-
- mmap_read_unlock(alloc->mm);
- for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
- page = &alloc->pages[i];
- if (!page->page_ptr)
- free++;
- else if (list_empty(&page->lru))
- active++;
- else
- lru++;
+ if (binder_alloc_get_vma(alloc) != NULL) {
+ for (i = 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
+ page = &alloc->pages[i];
+ if (!page->page_ptr)
+ free++;
+ else if (list_empty(&page->lru))
+ active++;
+ else
+ lru++;
+ }
}
-
-uninitialized:
mutex_unlock(&alloc->mutex);
seq_printf(m, " pages: %d:%d:%d\n", active, lru, free);
seq_printf(m, " pages high watermark: %zu\n", alloc->pages_high);
--
2.40.0.634.g4ca3ef3211-goog
Bring back the original lockless design in binder_alloc to determine
whether the buffer setup has been completed by the ->mmap() handler.
However, this time use smp_load_acquire() and smp_store_release() to
wrap all the ordering in a single macro call.
Also, add comments to make it evident that binder uses alloc->vma to
determine when the binder_alloc has been fully initialized. In these
scenarios acquiring the mmap_lock is not required.
Cc: Liam Howlett <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Signed-off-by: Carlos Llamas <[email protected]>
---
drivers/android/binder_alloc.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index eb082b33115b..9d166e10315e 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -309,17 +309,16 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
return vma ? -ENOMEM : -ESRCH;
}
+static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
+ struct vm_area_struct *vma)
+{
+ smp_store_release(&alloc->vma, vma);
+}
+
static inline struct vm_area_struct *binder_alloc_get_vma(
struct binder_alloc *alloc)
{
- struct vm_area_struct *vma = NULL;
-
- if (alloc->vma) {
- /* Look at description in binder_alloc_set_vma */
- smp_rmb();
- vma = alloc->vma;
- }
- return vma;
+ return smp_load_acquire(&alloc->vma);
}
static bool debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
@@ -382,6 +381,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
size_t size, data_offsets_size;
int ret;
+ /* Check binder_alloc is fully initialized */
if (!binder_alloc_get_vma(alloc)) {
binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
"%d: binder_alloc_buf, no vma\n",
@@ -777,7 +777,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
buffer->free = 1;
binder_insert_free_buffer(alloc, buffer);
alloc->free_async_space = alloc->buffer_size / 2;
- alloc->vma = vma;
+
+ /* Signal binder_alloc is fully initialized */
+ binder_alloc_set_vma(alloc, vma);
return 0;
@@ -959,7 +961,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
*/
void binder_alloc_vma_close(struct binder_alloc *alloc)
{
- alloc->vma = 0;
+ binder_alloc_set_vma(alloc, NULL);
}
/**
--
2.40.0.634.g4ca3ef3211-goog
On Mon, Apr 24, 2023 at 08:55:46PM +0000, Carlos Llamas wrote:
> This reverts commit 44e602b4e52f70f04620bbbf4fe46ecb40170bde.
>
> This caused a performance regression particularly when pages are getting
> reclaimed. We don't need to acquire the mmap_lock to determine when the
> binder buffer has been fully initialized. A subsequent patch will bring
> back the lockless approach for this.
>
> [cmllamas: resolved trivial conflicts with renaming of alloc->mm]
>
> Cc: Liam Howlett <[email protected]>
> Cc: Suren Baghdasaryan <[email protected]>
> Signed-off-by: Carlos Llamas <[email protected]>
> ---
> drivers/android/binder_alloc.c | 31 ++++++++++---------------------
> 1 file changed, 10 insertions(+), 21 deletions(-)
Why is this series "RFC"? What needs to be done to be able to submit it
as a real patch series?
Also, as the commits you are reverting are in older kernels, please
properly cc: stable in the signed-off-by area, and add a fixes: tag for
the commit you are reverting when you resend these as a real series.
thanks,
greg k-h
On Tue, Apr 25, 2023 at 07:19:20AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 24, 2023 at 08:55:46PM +0000, Carlos Llamas wrote:
> > This reverts commit 44e602b4e52f70f04620bbbf4fe46ecb40170bde.
> >
> > This caused a performance regression particularly when pages are getting
> > reclaimed. We don't need to acquire the mmap_lock to determine when the
> > binder buffer has been fully initialized. A subsequent patch will bring
> > back the lockless approach for this.
> >
> > [cmllamas: resolved trivial conflicts with renaming of alloc->mm]
> >
> > Cc: Liam Howlett <[email protected]>
> > Cc: Suren Baghdasaryan <[email protected]>
> > Signed-off-by: Carlos Llamas <[email protected]>
> > ---
> > drivers/android/binder_alloc.c | 31 ++++++++++---------------------
> > 1 file changed, 10 insertions(+), 21 deletions(-)
>
> Why is this series "RFC"? What needs to be done to be able to submit it
> as a real patch series?
No real reason. Just wanted to get feedback from the mm folks first.
I'll go ahead and submit the v1.
>
> Also, as the commits you are reverting are in older kernels, please
> properly cc: stable in the signed-off-by area, and add a fixes: tag for
> the commit you are reverting when you resend these as a real series.
Sounds good, thanks!