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