From owner-freebsd-arch@freebsd.org Wed Nov 30 21:35:24 2016 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A806BC5D9A1 for ; Wed, 30 Nov 2016 21:35:24 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 3C8CC1DAA for ; Wed, 30 Nov 2016 21:35:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c110-21-100-78.carlnfd1.nsw.optusnet.com.au [110.21.100.78]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 361B51042861; Thu, 1 Dec 2016 08:05:55 +1100 (AEDT) Date: Thu, 1 Dec 2016 08:05:54 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik cc: freebsd-arch@freebsd.org Subject: Re: __read_only in the kernel In-Reply-To: <20161130011033.GA20999@dft-labs.eu> Message-ID: <20161201070246.S1023@besplex.bde.org> References: <20161127212503.GA23218@dft-labs.eu> <20161130011033.GA20999@dft-labs.eu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=BKLDlBYG c=1 sm=1 tr=0 a=uGjuzT6u7JdBDS7kH8taPg==:117 a=uGjuzT6u7JdBDS7kH8taPg==:17 a=kj9zAlcOel0A:10 a=zKwlznhDgV4ePe7pBxgA:9 a=5hRXF1yCUoz3ZGdL:21 a=PV8UcHJ9p_xyYgZ2:21 a=CjuIK1q_8ugA:10 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Nov 2016 21:35:24 -0000 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 . 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. 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 don't need any linker support to work, since they create sections as necessary. So the above definition in 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 or better if kernel-only. It is close to needing a parameter for alignment. Bruce