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

Takashi Iwai tiwai at suse.de
Tue Nov 28 14:48:54 CET 2017


On Tue, 28 Nov 2017 14:32:51 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Nov 28 2017 05:40, Takashi Iwai wrote:
> > Basically I like this kind of refactoring, but I hesitate applying at
> > this time.  First off, the patches make codes bigger in both source
> > codes and binaries:
> >
> >   sound/core/control.c        | 242 ++++++++-----
> >   sound/core/control_compat.c | 837 
> +++++++++++++++++++++++++-------------------
> >   2 files changed, 630 insertions(+), 449 deletions(-)
> >
> > (original)
> >   % size sound.ko
> >     text    data     bss     dec     hex filename
> >    59494    2188    6272   67954   10972 sound/core/snd.ko
> >
> > (patched)
> >   % size sound.ko
> >     text    data     bss     dec     hex filename
> >    60186    2200    6272   68658   10c32 sound/core/snd.ko
> 
> Yes. This patchset increases binary size. In my build envoronment
> (make x86_64_defconfig + disable debug configurations), they're:
> 
> (at v4.15-rc1[1])
> $ size -A /tmp/snd.ko.master
> /tmp/snd.ko.master  :
> section                      size   addr
> .note.gnu.build-id             36      0
> .text                       26244      0 <-
> .init.text                    470      0
> .exit.text                     45      0
> .altinstr_replacement          20      0
> __ksymtab                     832      0
> __ksymtab_gpl                  96      0
> .rodata                      1784      0 <-
> .rodata.str1.1                733      0
> .rodata.str1.8                559      0
> __ksymtab_strings            1133      0
> .modinfo                      423      0
> __param                       120      0
> .orc_unwind_ip               5220      0
> .orc_unwind                  7830      0
> .smp_locks                     24      0
> .altinstructions               52      0
> .data                         480      0
> __bug_table                    12      0
> .gnu.linkonce.this_module     768      0
> .bss                         2240      0
> .comment                      324      0
> .note.GNU-stack                 0      0
> Total                       49445
> 
> (with whole this patchset, at a branch in my repo[2])
> $ size -A /tmp/snd.ko.compat
> /tmp/snd.ko.compat  :
> section                      size   addr
> .note.gnu.build-id             36      0
> .text                       26692      0 <-
> .init.text                    470      0
> .exit.text                     45      0
> __ksymtab                     832      0
> __ksymtab_gpl                  96      0
> .rodata                      2104      0 <-
> .rodata.str1.1                733      0
> .rodata.str1.8                511      0
> __ksymtab_strings            1133      0
> .modinfo                      424      0
> __param                       120      0
> .orc_unwind_ip               5212      0
> .orc_unwind                  7818      0
> .smp_locks                     24      0
> .data                         480      0
> __bug_table                    12      0
> .gnu.linkonce.this_module     768      0
> .bss                         2240      0
> .comment                      324      0
> .note.GNU-stack                 0      0
> Total                       50074
> 
> Both of sections of '.rodata' and '.text' are increased. As a summary:
> 
>             v4.15-rc1   compat
> .text:      26244       26692
> .rodata:    1784        2104
> total:      49445       50074
> 
> The increase of former comes from 'handlers[]' in
> 'snd_ctl_ioctl_compat()' and this is unavoided. On the other hand, the
> increase of latter comes from a series of function named as
> 'ctl_compat_ioctl_xxx()'. If 'control.c' got proper arrangements, the
> increase can be suppressed somehow.
> 
> Actually, this patchset has a next patchset, to apply refactoring the
> file[3] (I should have added some comments about it, but they were not
> prepared when posting this patchset, sorry...). In the unposted
> patchset, the similar idea as this patchset is introduced; i.e. handler,
> allocator and deallocator. As a result:
> 
> (with the unposted patchset, at a branch in my repo[3])
> /tmp/snd.ko.unlocked  :
> section                      size   addr
> .note.gnu.build-id             36      0
> .text                       26164      0 <-
> .init.text                    470      0
> .exit.text                     45      0
> __ksymtab                     832      0
> __ksymtab_gpl                  96      0
> .rodata                      2328      0 <-
> .rodata.str1.1                733      0
> .rodata.str1.8                511      0
> __ksymtab_strings            1133      0
> .modinfo                      424      0
> __param                       120      0
> .orc_unwind_ip               5252      0
> .orc_unwind                  7878      0
> .smp_locks                     24      0
> .data                         480      0
> __bug_table                    12      0
> .gnu.linkonce.this_module     768      0
> .bss                         2240      0
> .comment                      324      0
> .note.GNU-stack                 0      0
> Total                       49870
> 
> As a summary:
>             v4.15-rc1   compat  unlocked
> .text:      26244       26692   26164
> .rodata:    1784        2104    2428
> total:      49445       50074   49870
> 
> The .text section decreases its size, while .rodata increases. Total
> size of included sections increases +0.8%. Of course, any debug sections
> are excluded from the calculation.
> 
> In my opinion, in a point of binary size, the increase is enough
> acceptable.
> 
> > Another slight concern is that the new code requires one extra kmalloc
> > at each execution. The kctl ioctls aren't very hot code path, but
> > they can be called yet relatively frequently, hence a slowdown isn't
> > good unless the modification brings significantly (read
> > "dramatically") improvement of code readability.
> 
> Hm. I guess that you actually found some advantages of this patchset to
> increase code readability and maintainability. The additional step for
> allocation on kernel space belongs to optimization domain, however as
> you noted code path via ALSA control interface is not designed for
> severe use cases such as real-time data transmission. We can be
> optimistic for the overhead as long as it's not so large.

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.


> > 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).


thanks,

Takashi


More information about the Alsa-devel mailing list