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

[Takashi Iwai]

On Sat, 25 Nov 2017 10:19:42 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> ALSA control core supports system call compatibility layer because some
> structures in ALSA control interface includes members of 'long' and
> 'pointer' types and change own layout according to ABIs. In this point,
> it's enough for the layer to have handlers for structures on ABIs with
> 'ILP32' data model.
> 
> A recent commit of 6236d8bb2afc ('ALSA: ctl: Fix ioctls for X32 ABI')
> clear that System V ABI for i386 architecture is unique than the other
> ABIs with 'ILP32' data model. On this ABI, machine type for storage class
> of 'long long' type has 4 bytes alignment. This is different from the
> other ABIs.
> 
> In current implementation of this layer, the same structure is used for
> i386 ABI and the other ABI with 'ILP32' data model except for x32 ABI.
> Macro is used to switch between these. But in a view of data model,
> it's better to define structure for i386 ABI uniquely against the
> other ABIs including x32 for simplicity.
> 
> Additionally, this layer includes some points to be improved:
>  - cancel allocation in user space from kernel land
>  - reduce kernel stack usage
> 
> This patchset is my attempts to improve the layer. For this purpose,
> this patchset introduces a local framework to describe consistent method
> to convert and process data for differences of structure layout.

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
	  

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.

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.


thanks,

Takashi