[alsa-devel] [PATCH 00/24] ALSA: ctl: refactoring compat layer

[Takashi Iwai]

On Wed, 29 Nov 2017 03:18:57 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Nov 28 2017 22:48, Takashi Iwai wrote:
> > Well, both the code size and the code performance are rather the basic
> > question.  Usually a code refactoring is done because of performance,
> > not because of the code itself.  IOW, the refactoring is based on the
> > implicit rule -- a better code performs better.  If it's not true, the
> > only exception is about the portability.  But I don't think that your
> > patchset would improve that dramatically.  It's the point making me
> > hesitating to apply the patches.
> >
> > In short, the patch result doesn't look convincing enough.  At least,
> > I'd like to see an improvement in the resultant binary.
> 
> Hm. For these concerns, I can propose additional improvements to this
> patchset.
>  * Use kernel stack for buffers to convert layout of structure
>   - (pros) This is for your concern about an overhead to use additional
>     kernel heap. Additionally, this can reduce one member from handler
>     structure because no need to calculate original buffer size.
>   - (cons) two buffers for purse consumes the kernel stack by around
>     2000 bytes.

I'm afraid that this increase isn't acceptable.
The current compat code was originally with stack, too, but had to be
converted later to kmalloc for reducing the stack size usage.  Now the
new code would even double.

Although the requirement of stack usage reduction came from 4k stack
and the condition was relaxed nowadays after giving up the idea, the
stack is still precious and we are not allowed to consume too much.
As a rule of thumb, maybe 100 to 200 bytes would be maximum per
function.

>  * Change some functions in 'control.c' to get 'void *' as an argument.
>   - (pros) these functions can get callback from 'control_compat.c'
>     directly. As a result, .text section can be decreased.
>   - (cons) these functions lose type information for the argument. This
>     also can fake static code analyzer, perhaps.

This isn't fascinating, either, but more acceptable than the former.
(Read: let's think of other ways, but this can be still taken as a
 last resort.)

BTW, if we go in that way, I guess the idea of using the conversion
table can be applied to 64bit native stuff, too.  Basically
memdup_user() and and copy_to_user() calls correspond to deserialize
and serialize.  Using a common helper in both sides might reduce the
code size, and it makes the change to void* more demanded.

> 
> >>> Also, did you actually test with all possible architectures, not only
> >>> trusting the written spec?  It should be relatively easy to set up the
> >>> cross-compilation just only for checking this stuff.  You don't need
> >>> to build the whole kernel.
> >>
> >> I've already done it. To check size of structure and offsets of members
> >> on it, I've generate assembly list, like:
> >>
> >> $ cat test.c
> >> #include <stdio.h>
> >> #include <stdlib.h>
> >> #include <stddef.h>
> >> #include <sound/asound.h>
> >> struct snd_ctl_elem_value_32 {
> >>      ...
> >> } __attribute__((packed));
> >> int main(void)
> >> {
> >>      printf("%zd\n", offsetof(struct snd_ctl_elem_value, value));
> >>      return EXIT_SUCCESS;
> >> }
> >> $ x86_64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B 4 printf
> >>      .cfi_def_cfa_register 6
> >>      movl    $72, %esi
> >>      leaq    .LC0(%rip), %rdi
> >>      movl    $0, %eax
> >>      call    printf at PLT
> >> $ aarch64-linux-gnu-gcc-7 -S -o - /tmp/test.c | grep -B1 printf
> >>      mov x1, 72
> >>      bl  printf
> >
> > There are lots of other architectures supporting 64/32 compatibility,
> > e.g. s390, powerpc, sparc, etc.  That's my concern.  At least these
> > major active architectures have to be checked (maybe MIPS, too).
> 
> If you'd like me to check more architectures, I'll do it. Please send
> a list of architectures to investigate.

You can grep COMPAT definition in Kconfig in arch/*.  mips and tile
seem to have the compat layer, too.


> But in practical points, I'll
> omit s390, sparc and sparc64 because nowadays we have no products
> based on these architectures for consumer use. At least, in data
> center, sound devices are needless.

Don't underestimate it.  There are always crazy users, and the sound
usage isn't always tied with the real hardware but also with a VM or
data processing.  (And also there are still desktop users of SPARC
boxen :)


thanks,

Takashi