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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
