Re: [PATCH v3 1/7] binder: create userspace-to-binder-buffer copy function

[Todd Kjos]

On Thu, Feb 14, 2019 at 1:25 PM Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
>
> On Thu, Feb 14, 2019 at 03:53:54PM -0500, Joel Fernandes wrote:
> > On Thu, Feb 14, 2019 at 3:42 PM Todd Kjos <tkjos@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Feb 14, 2019 at 11:45 AM Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
> > [snip]
> > > > > + * check_buffer() - verify that buffer/offset is safe to access
> > > > > + * @alloc: binder_alloc for this proc
> > > > > + * @buffer: binder buffer to be accessed
> > > > > + * @offset: offset into @buffer data
> > > > > + * @bytes: bytes to access from offset
> > > > > + *
> > > > > + * Check that the @offset/@bytes are within the size of the given
> > > > > + * @buffer and that the buffer is currently active and not freeable.
> > > > > + * Offsets must also be multiples of sizeof(u32). The kernel is
> > > >
> > > > In all callers of binder_alloc_copy_user_to_buffer, the alignment of offsets
> > > > is set to sizeof(void *). Then shouldn't this function check for sizeof(void *)
> > > > alignment instead of u32?
> > >
> > > But there are other callers of check_buffer() later in the series that
> > > don't require pointer-size alignment. u32 alignment is consistent with
> > > the alignment requirements of the binder driver before this change.
> > > The copy functions don't actually need to insist on alignment, but
> > > these binder buffer objects have always used u32 alignment which has
> > > been checked in the driver. If user code misaligned it, then errors
> > > are returned. The alignment checks are really to be consistent with
> > > previous binder driver behavior.
> >
> > Got it, thanks.
>
> One more thing I wanted to ask is, kmap() will now cause global lock
> contention because of using spin_lock due to kmap_high().
>
> Previously the binder driver was made to not use global lock (as you had
> done). Now these paths will start global locking on 32-bit architectures.
> Would that degrade performance?

There was a lot of concern about 32-bit performance both for
lock-contention and the cost of map/unmap operations. Of course,
32-bit systems are also where the primary win is -- namely avoiding
vmalloc space depletion. So there was a several months-long evaluation
period on 32-bit devices by a silicon vendor who did a lot of testing
across a broad set of benchmarks / workloads to verify the performance
costs are acceptable. We also ran tests to try to exhaust the kmap
space with multiple large buffers.

The testing did find that there is some performance degradation for
large buffer transfers, but there are no cases where this
significantly impacted a meaningful user workload.

>
> Are we not using kmap_atomic() in this patch because of any concern that the
> kmap fixmap space is limited and may run out?

We're not using the atomic version here since we can't guarantee that
the loop will be atomic since we are calling copy_from_user(). Later
in the series, other cases do use kmap_atomic().

>
> thanks,
>
>  - Joel
>
>