Messages in this thread |  | | From | "Fabio M. De Francesco" <> | Subject | Re: [PATCH 3/3] binder: Use kmap_local_page() in binder_alloc_get_page() | Date | Sun, 24 Apr 2022 11:39:57 +0200 |
| |
On sabato 23 aprile 2022 18:02:48 CEST Christophe JAILLET wrote: > Hi, > > Le 23/04/2022 à 12:24, Fabio M. De Francesco a écrit : > > The use of kmap_atomic() is being deprecated in favor of kmap_local_page() > > where it is feasible. Each call of kmap_atomic() in the kernel creates > > a non-preemptible section and disable pagefaults. This could be a source > > of unwanted latency, so it should be only used if it is absolutely > > required, otherwise kmap_local_page() should be preferred. > > > > With kmap_local_page(), the mapping is per thread, CPU local and not > > globally visible. Furthermore, the mapping can be acquired from any context > > (including interrupts). > > > > Therefore, use kmap_local_page() / kunmap_local() in place of > > kmap_atomic() / kunmap_atomic(). > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > drivers/android/binder_alloc.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/ binder_alloc.c > > index 0875c463c002..058595cc7ff0 100644 > > --- a/drivers/android/binder_alloc.c > > +++ b/drivers/android/binder_alloc.c > > @@ -1250,17 +1250,17 @@ static int binder_alloc_do_buffer_copy(struct binder_alloc *alloc, > > page = binder_alloc_get_page(alloc, buffer, > > buffer_offset, &pgoff); > > size = min_t(size_t, bytes, PAGE_SIZE - pgoff); > > - base_ptr = kmap_atomic(page); > > + base_ptr = kmap_local_page(page); > > tmpptr = base_ptr + pgoff; > > if (to_buffer) > > memcpy(tmpptr, ptr, size); > > else > > memcpy(ptr, tmpptr, size); > > in the same spirit as patch 1/3, memcpy_to_page() and memcpy_from_page() > looks good candidate to avoid code duplication.
Hello Christophe, Todd,
I had thought to use memcpy_to_page() and memcpy_from_page() (exactly as I did in other conversions I have been working on during the latest couple of weeks).
However, I decided to avoid to use them for I should also have deleted the comment which is before "kunmap_local(base_ptr);".
I don't know how much Maintainers think it is necessary to make readers notice that "kunmap_local() takes care of flushing the cache []" (exactly as kunmap_atomic() does). Actually I'd delete that comment that looks redundant and unnecessary to me, but I cannot know if Todd wants it to remain there.
@Todd: Can you please say what you think about this topic? Should I leave the patch as-is or I should use memcpy_{to,from}_page() and delete that comment?
I won't send any v2 unless I have your confirmation.
Thanks,
Fabio
> > Not checked in details, but looks mostly the same. > > Just my 2c. > > CJ > > > /* > > - * kunmap_atomic() takes care of flushing the cache > > + * kunmap_local() takes care of flushing the cache > > * if this device has VIVT cache arch > > */ > > - kunmap_atomic(base_ptr); > > + kunmap_local(base_ptr); > > bytes -= size; > > pgoff = 0; > > ptr = ptr + size; > >
|  |