Date: Thu, 1 Dec 2016 08:05:54 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-arch@freebsd.org Subject: Re: __read_only in the kernel Message-ID: <20161201070246.S1023@besplex.bde.org> In-Reply-To: <20161130011033.GA20999@dft-labs.eu> References: <20161127212503.GA23218@dft-labs.eu> <20161130011033.GA20999@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 30 Nov 2016, Mateusz Guzik wrote: > I officially threaten to commit this by Friday. It has too high a density of style bugs to commit. >> diff --git a/sys/amd64/include/param.h b/sys/amd64/include/param.h >> index a619e395..ab66e79 100644 >> --- a/sys/amd64/include/param.h >> +++ b/sys/amd64/include/param.h >> @@ -152,4 +152,6 @@ >> #define INKERNEL(va) (((va) >= DMAP_MIN_ADDRESS && (va) < DMAP_MAX_ADDRESS) \ >> || ((va) >= VM_MIN_KERNEL_ADDRESS && (va) < VM_MAX_KERNEL_ADDRESS)) >> >> +#define __read_mostly __attribute__((__section__(".data.read_mostly"))) >> + >> #endif /* !_AMD64_INCLUDE_PARAM_H_ */ 1. Hard-coded gccism: __attribute__(). 1a. Non-use of the FreeBSD macro __section() whose purpose is to make it easy to avoid using __attribute__() for precisely what it is used for here. 2. Misplaced definition. Such definitions go in <sys/cdefs.h>. This one has no dependencies on amd64 except possibly for bugs elsewhere, but is only in an amd64 header. __section() has no dependencies on anything, but its contents may be MD. Here the contents are not very MD. <sys/cdefs.h> already refers to the magic sections ".gnu.warning." # sym without even using its own macro (it uses hard-coded __asm__() and also doesn't even use the preferred FreeBSD spelling __asm()). It isn't clear what prevents these hard-coded gccisms from being syntax errors in the lint case where __section() is defined as empty to avoid syntax errors. According to userland tests, section statements like the above and the ones in <sys/cdefs.h> don't need any linker support to work, since they create sections as necessary. So the above definition in <sys/cdefs.h> should almost perfectly for all arches, even without linker support. Style bug (1) is smaller if it is there. >> diff --git a/sys/conf/ldscript.amd64 b/sys/conf/ldscript.amd64 >> index 5d86b03..ae98447 100644 >> --- a/sys/conf/ldscript.amd64 >> +++ b/sys/conf/ldscript.amd64 >> @@ -151,6 +151,11 @@ SECTIONS >> KEEP (*(.gnu.linkonce.d.*personality*)) >> } >> .data1 : { *(.data1) } >> + .data_read_mostly : >> + { >> + *(.data.read_mostly) >> + . = ALIGN(64); >> + } >> _edata = .; PROVIDE (edata = .); >> __bss_start = .; >> .bss : For arches without this linker support, the variables would be grouped but not aligned so much. Aligning the subsection seems to be useless anyway. This only aligns the first variable in the subsection. Most variables are still only aligned according to their natural or specified alignment. This is rarely as large as 64. But I think variables in the subsection can be more aligned than the subsection. If they had to be (as in a.out), then it is the responsibility of the linker to align the subsection to more than the default if a single variable in the subsection needs more than the default. >> diff --git a/sys/sys/param.h b/sys/sys/param.h >> index cf38985..dcd9526 100644 >> --- a/sys/sys/param.h >> +++ b/sys/sys/param.h >> @@ -360,4 +360,8 @@ __END_DECLS >> */ >> #define __PAST_END(array, offset) (((__typeof__(*(array)) *)(array))[offset]) >> >> +#ifndef __read_mostly >> +#define __read_mostly >> +#endif >> + >> #endif /* _SYS_PARAM_H_ */ Since the definition was misplaced in only the amd64 param.h, you needed this hack to define it for other arches. The general definition would be misplaced here too, like most definitions in this file. __read_mostly() is not a system parameter. Fortunately, it also doesn't use any system parameters, so it can go in <sys/cdefs.h> or better <sys/systm.h> if kernel-only. It is close to needing a parameter for alignment. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161201070246.S1023>