svn commit: r274088 - head/sys/kern

[Bruce Evans]

On Tue, 4 Nov 2014, Mateusz Guzik wrote:

> On Tue, Nov 04, 2014 at 11:29:49AM +0000, Hans Petter Selasky wrote:
>> Author: hselasky
>> Date: Tue Nov  4 11:29:49 2014
>> New Revision: 274088
>> URL: https://svnweb.freebsd.org/changeset/base/274088
>>
>> Log:
>>   Simplify logic a bit. Ensure data buffer is properly aligned,
>>   especially for platforms where unaligned access is not allowed. Make
>>   it possible to override the small buffer size.
>>
>>   A simple continuous read string test using libusb showed a reduction
>>   in CPU usage from roughly 10% to less than 1% using a dual-core GHz
>>   CPU, when the malloc() operation was skipped for small buffers.
>>
>>   MFC after:	2 weeks
>>
>> Modified:
>>   head/sys/kern/sys_generic.c
>>
>> Modified: head/sys/kern/sys_generic.c
>> ==============================================================================
>> --- head/sys/kern/sys_generic.c	Tue Nov  4 10:25:52 2014	(r274087)
>> +++ head/sys/kern/sys_generic.c	Tue Nov  4 11:29:49 2014	(r274088)
>> @@ -646,10 +646,13 @@ struct ioctl_args {
>>  int
>>  sys_ioctl(struct thread *td, struct ioctl_args *uap)
>>  {
>> +#ifndef SYS_IOCTL_SMALL_SIZE
>> +#define	SYS_IOCTL_SMALL_SIZE 128
>> +#endif

This shouldn't be ifdefed.  It doesn't even have the full complexity for
an option (it isn't in conf/options).

>> +	u_char smalldata[SYS_IOCTL_SMALL_SIZE] __aligned(8);
>
> Should not you align to word size instead?

Neither.  The alignment should be the same as given by malloc(9).  The
precise value is undocumented.  It seems to be determined by malloc()
using uma_zcreate() with align = UMA_ALIGN_PTR.  This is bogus since
UMA_ALIGN_PTR only guarantees enough alignment for pointers but malloc()
is required to support all object types.  The definition UMA_ALIGN_PTR
is also bogus, since it assumes that all pointers have the same alignment
requirements and that these are given by the size of the special pointer
type 'void *'.  But this works, because all machines are vaxes.  It gives
the register size on all machines.

Another unsuitable alignment is by the MD ALIGNBYTES macro.  This is
(sizeof(register_t) - 1) on all arches except mips 32-bit where it is 7
sparc64 where it is 15.  On arm, it is 3, but arm also has
STACKALIGNBYTES = 7.  The register size is really too small to use for
malloc() on 32-bit arches.  Its technical correctness depends on no
C objects having more than 32-bit alignment.  On i386, int64_t and
double should have 64-bit alignment, but this is not required unless
CFLAGS includes -malign-double which breaks the ABI in userland and
is irrelevant for the kernel.

> Also maybe it would be beneficial to derive the limit from KSTACK_PAGES
> (something silly like KSTACK_PAGES * 32 or something), which I believe would
> address earlier complaints.

No, that would reward bloat in KSTACK_PAGES.  KSTACK_PAGES depends on this,
not the other way round.

>> @@ -682,10 +685,10 @@ sys_ioctl(struct thread *td, struct ioct
>>  			data = (void *)&arg;
>>  			size = 0;
>>  		} else {
>> -			if (size <= sizeof(smalldata))
>> -				data = smalldata;
>> -			else
>> +			if (size > SYS_IOCTL_SMALL_SIZE)
>>  				data = malloc((u_long)size, M_IOCTLOPS, M_WAITOK);
>> +			else
>> +				data = smalldata;
>>  		}
>>  	} else
>>  		data = (void *)&uap->data;

The previous order was better.

The malloc() line still has 2 style bugs.

Bruce