Date: Wed, 5 Nov 2014 03:19:58 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Hans Petter Selasky <hselasky@freebsd.org> Subject: Re: svn commit: r274088 - head/sys/kern Message-ID: <20141105023323.O1105@besplex.bde.org> In-Reply-To: <20141104114041.GA21297@dft-labs.eu> References: <201411041129.sA4BTnwX030600@svn.freebsd.org> <20141104114041.GA21297@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141105023323.O1105>