From owner-freebsd-arch@freebsd.org Thu Dec 29 11:46:00 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 F3076C96021 for ; Thu, 29 Dec 2016 11:46:00 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x244.google.com (mail-wm0-x244.google.com [IPv6:2a00:1450:400c:c09::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 87D62144E for ; Thu, 29 Dec 2016 11:46:00 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x244.google.com with SMTP id c85so27941170wmi.1 for ; Thu, 29 Dec 2016 03:46:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=SV8LlheFMZZI9Tm7RatZ+AcuQi/sHsOqI0qrEjVLff4=; b=qAT/xydr+oOvkZO0YWmIYGsYS4/vZbNzrvJczJpWZ7FY0oYYCsLz+GsdrnlHG0TbjM cVYfFrezjANJY/0AkQthe1GoOnK/BvVjdeCPxRol/Ey5hVAhDGMcQCvDojEGBGB9EDp+ XO21d6jJl0v2FXsMd/G48g1YD/TYbihvMyqmJMPbcrHsZilaIGRQM+1ySk9+tyH0Vcy7 nK1EURVrMN2kUkO5YKlo3hsKDukDc1AMp5PT8L9OuIGI0fRM2bia8hmeDSJVU5MUpnDK 5VRplIZYUfK6xIa5+v9idw66i5FYc8Up80+Jyk8kLMgq4y7umQZl0cLIYNgKhj8+jajo uKZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=SV8LlheFMZZI9Tm7RatZ+AcuQi/sHsOqI0qrEjVLff4=; b=At1v1RTcR4eC6fx0h09XBNnTaSmB05BsmqIR4bhjOOCTgZ9r+rZ/eDuJpSX2SoFwpP SlVXfe91GCHLnmqTVmYbWts9e/Zzm0b487/2vxTt1t+7J7Dz0mbX8XsrIiVde1ca08ky Df6bKim95+omb5qx3UAAPmDw0S1BpahANNwgiK0PYcJQC2e4ZdUmKxjaZLTZKmFKamjB jW67T9btuMpCs39aP3nxyZ/ApRz7m+ytgEOlmTqRR2dfnMF5T+mOeWSGxuPAtX1F228g chj78VOBOcAphD78STxCp/vcOsnIcVOgLhcHsaPbX3Q868fDGkSaGjqDkCrQLRM2RQ5n +nHg== X-Gm-Message-State: AIkVDXK89dgah76UV/o5Sy8RViUKgb3ak4CGhD/vsqzX26i9bxKJkbuBxaL66519Ei9PWg== X-Received: by 10.28.131.72 with SMTP id f69mr34605417wmd.135.1483011958632; Thu, 29 Dec 2016 03:45:58 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id w18sm65172277wme.9.2016.12.29.03.45.57 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 29 Dec 2016 03:45:57 -0800 (PST) Date: Thu, 29 Dec 2016 12:45:55 +0100 From: Mateusz Guzik To: Bruce Evans Cc: freebsd-arch@freebsd.org Subject: Re: __read_only in the kernel Message-ID: <20161229114554.GA29676@dft-labs.eu> References: <20161127212503.GA23218@dft-labs.eu> <20161130011033.GA20999@dft-labs.eu> <20161201070246.S1023@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161201070246.S1023@besplex.bde.org> User-Agent: Mutt/1.5.21 (2010-09-15) 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: Thu, 29 Dec 2016 11:46:01 -0000 On Thu, Dec 01, 2016 at 08:05:54AM +1100, Bruce Evans wrote: > On Wed, 30 Nov 2016, Mateusz Guzik wrote: > >>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. > [..] > 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. > I wanted to avoid providing the definition for archs which don't have the linker script bit and this was the only header I found which is md and effectively always included. Indeed it seems the section is harmless even without the explicit support. > >>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. > With the indended use grouping side-by-side is beneficial - as the vars in question are supposed to be rarely modified, there is no problem with them sharing a cache line. Making them all use dedicated lines would only waste memory. That said, what about the patch below. I also grepped the tree and found 2 surprises, handled in the patch. diff --git a/sys/compat/linuxkpi/common/include/linux/compiler.h b/sys/compat/linuxkpi/common/include/linux/compiler.h index c780abc..c32f1fa 100644 --- a/sys/compat/linuxkpi/common/include/linux/compiler.h +++ b/sys/compat/linuxkpi/common/include/linux/compiler.h @@ -67,7 +67,6 @@ #define typeof(x) __typeof(x) #define uninitialized_var(x) x = x -#define __read_mostly __attribute__((__section__(".data.read_mostly"))) #define __always_unused __unused #define __must_check __result_use_check diff --git a/sys/conf/ldscript.amd64 b/sys/conf/ldscript.amd64 index 5d86b03..d87d607 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 : diff --git a/sys/dev/drm2/drm_os_freebsd.h b/sys/dev/drm2/drm_os_freebsd.h index dc01c6a..11c9feb 100644 --- a/sys/dev/drm2/drm_os_freebsd.h +++ b/sys/dev/drm2/drm_os_freebsd.h @@ -80,7 +80,6 @@ typedef void irqreturn_t; #define __init #define __exit -#define __read_mostly #define BUILD_BUG_ON(x) CTASSERT(!(x)) #define BUILD_BUG_ON_NOT_POWER_OF_2(x) diff --git a/sys/sys/systm.h b/sys/sys/systm.h index a1ce9b4..5f646ff 100644 --- a/sys/sys/systm.h +++ b/sys/sys/systm.h @@ -445,4 +445,6 @@ extern void (*softdep_ast_cleanup)(void); void counted_warning(unsigned *counter, const char *msg); +#define __read_mostly __section(".data.read_mostly") + #endif /* !_SYS_SYSTM_H_ */ -- Mateusz Guzik