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>