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

[Takashi Sakamoto]

On Nov 29 2017 16:12, Takashi Iwai wrote:
 >> 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.

Yep. It's a reason to allocate on heap, instead of usage of stack, in my
patchset.

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

It's definitely preferable. No objections I have. In this point, it's
better to apply better refactoring to current ALSA control core because
kernel stack is used for some medium structures.

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

Hm, OK. There might be a space to re-design my local framework for this
purpose.

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

$ find arch -name Kconfig | xargs grep -l COMPAT
arch/parisc/Kconfig
arch/arm/Kconfig
arch/mips/Kconfig
arch/arm64/Kconfig
arch/Kconfig
arch/s390/Kconfig
arch/sparc/Kconfig
arch/x86/Kconfig
arch/powerpc/Kconfig
arch/tile/Kconfig

One of my friend who is a debian user uses Tile-Gx machine, but I've
never used it. Far from it, I've never used most of the architectures
actually...

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

(The compat layer is for 64 bit machine. I cannot imagine sparc64
machine for desktop use...)


Thanks

Takashi Sakamoto