[00/24] ALSA: ctl: refactoring compat layer

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

In the end of this message, I attach a diff for this idea, on the last
patch of this set. When applying, the binary is shrunk.

$ size snd.ko.master
snd.ko.master  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26244      0 <-
.init.text                    470      0
.exit.text                     45      0
.altinstr_replacement          20      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      1784      0 <-
.rodata.str1.1                733      0
.rodata.str1.8                559      0
__ksymtab_strings            1133      0
.modinfo                      423      0
__param                       120      0
.orc_unwind_ip               5224      0
.orc_unwind                  7836      0
.smp_locks                     24      0
.altinstructions               52      0
.data                         480      0
__bug_table                    12      0
.gnu.linkonce.this_module     832      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       49519

$ size snd.ko.compat
sound/core/snd.ko  :
section                      size   addr
.note.gnu.build-id             36      0
.text                       26020      0 <-
.init.text                    470      0
.exit.text                     45      0
__ksymtab                     832      0
__ksymtab_gpl                  96      0
.rodata                      1976      0 <-
.rodata.str1.1                763      0
.rodata.str1.8                551      0
__ksymtab_strings            1133      0
.modinfo                      424      0
__param                       120      0
.orc_unwind_ip               5004      0
.orc_unwind                  7506      0
.smp_locks                     24      0
.data                         480      0
__bug_table                    24      0
.gnu.linkonce.this_module     832      0
.bss                         2240      0
.comment                      324      0
.note.GNU-stack                 0      0
Total                       48900

             v4.15-rc1   compat
.text:      26244       26020
.rodata:    1784        1976
total:      49519       48900

If we can accept the negative points, I'll post the revised version.

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

  	int err;
@@ -571,51 +524,35 @@ static long snd_ctl_ioctl_compat(struct file 
*file, unsigned int cmd,
  		return snd_ctl_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
  	}

-	/* Allocate a buffer to convert layout of structure for native ABI. */
-	buf = kzalloc(_IOC_SIZE(handlers[i].orig_cmd), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
  	/* Allocate an alternative buffer to copy from/to user space. */
  	size = _IOC_SIZE(handlers[i].cmd);
-	data = kzalloc(size, GFP_KERNEL);
-	if (!data) {
-		kfree(buf);
-		return -ENOMEM;
-	}

  	if (handlers[i].cmd & IOC_IN) {
-		if (copy_from_user(data, compat_ptr(arg), size)) {
-			err = -EFAULT;
-			goto end;
-		}
+		if (copy_from_user(&data, compat_ptr(arg), size))
+			return -EFAULT;
  	}

-	err = handlers[i].deserialize(ctl_file, buf, data);
+	err = handlers[i].deserialize(ctl_file, &buf, &data);
  	if (err < 0)
-		goto end;
+		return err;

-	err = handlers[i].func(ctl_file, buf);
+	err = handlers[i].func(ctl_file, &buf);
  	if (err < 0)
-		goto end;
+		return err;

-	err = handlers[i].serialize(ctl_file, data, buf);
+	err = handlers[i].serialize(ctl_file, &data, &buf);
  	if (err >= 0) {
  		if (handlers[i].cmd & IOC_OUT) {
-			if (copy_to_user(compat_ptr(arg), data, size))
+			if (copy_to_user(compat_ptr(arg), &data, size))
  				err = -EFAULT;
  		}
  	}

  	if (err < 0) {
  		if (cmd == SNDRV_CTL_IOCTL_ELEM_ADD_32 ||
-		    cmd == SNDRV_CTL_IOCTL_ELEM_REPLACE_32) {
-			struct snd_ctl_elem_info *info = buf;
-			snd_ctl_remove_user_ctl(ctl_file, &info->id);
-		}
+		    cmd == SNDRV_CTL_IOCTL_ELEM_REPLACE_32)
+			snd_ctl_remove_user_ctl(ctl_file, &buf.elem_info.id);
  	}
-end:
-	kfree(data);
-	kfree(buf);
+
  	return err;
  }
========== 8< ----------


Thanks

Takashi Sakamoto


[00/24] ALSA: ctl: refactoring compat layer

Commit Message

Comments

Patch