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