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