LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] mm: expland documentation over __read_mostly
@ 2018-05-07 23:15 Luis R. Rodriguez
  2018-05-08  0:20 ` Randy Dunlap
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Luis R. Rodriguez @ 2018-05-07 23:15 UTC (permalink / raw)
  To: tglx, arnd, cl
  Cc: keescook, luto, longman, viro, willy, ebiederm, linux-arch,
	linux-mm, linux-kernel, Luis R. Rodriguez

__read_mostly can easily be misused by folks, its not meant for
just read-only data. There are performance reasons for using it, but
we also don't provide any guidance about its use. Provide a bit more
guidance over it use.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/linux/cache.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Every now and then we get a patch suggesting to use __read_mostly for
something new or old but with no justifications. Add a bit more
verbiage to help guide its users.

Is this sufficient documentation to at least ask for a reason in the commit
log as to why its being used for new entries? Or should we be explicit and
ask for such justifications in commit logs? Taken from prior discussions
with Christoph Lameter [0] over its use.

[0] https://lkml.kernel.org/r/alpine.DEB.2.11.1504301343190.28879@gentwo.org

diff --git a/include/linux/cache.h b/include/linux/cache.h
index 750621e41d1c..62bc5adc0ed5 100644
--- a/include/linux/cache.h
+++ b/include/linux/cache.h
@@ -15,8 +15,14 @@
 
 /*
  * __read_mostly is used to keep rarely changing variables out of frequently
- * updated cachelines. If an architecture doesn't support it, ignore the
- * hint.
+ * updated cachelines. Its use should be reserved for data that is used
+ * frequently in hot paths. Performance traces can help decide when to use
+ * this. You want __read_mostly data to be tightly packed, so that in the
+ * best case multiple frequently read variables for a hot path will be next
+ * to each other in order to reduce the number of cachelines needed to
+ * execute a critial path. We should be mindful and selective if its use.
+ *
+ * If an architecture doesn't support it, ignore the hint.
  */
 #ifndef __read_mostly
 #define __read_mostly
-- 
2.17.0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-07 23:15 [PATCH] mm: expland documentation over __read_mostly Luis R. Rodriguez
@ 2018-05-08  0:20 ` Randy Dunlap
  2018-05-08  3:23 ` Joel Fernandes
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Randy Dunlap @ 2018-05-08  0:20 UTC (permalink / raw)
  To: Luis R. Rodriguez, tglx, arnd, cl
  Cc: keescook, luto, longman, viro, willy, ebiederm, linux-arch,
	linux-mm, linux-kernel

On 05/07/2018 04:15 PM, Luis R. Rodriguez wrote:
> __read_mostly can easily be misused by folks, its not meant for
> just read-only data. There are performance reasons for using it, but
> we also don't provide any guidance about its use. Provide a bit more
> guidance over it use.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  include/linux/cache.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Every now and then we get a patch suggesting to use __read_mostly for
> something new or old but with no justifications. Add a bit more
> verbiage to help guide its users.
> 
> Is this sufficient documentation to at least ask for a reason in the commit
> log as to why its being used for new entries? Or should we be explicit and
> ask for such justifications in commit logs? Taken from prior discussions
> with Christoph Lameter [0] over its use.
> 
> [0] https://lkml.kernel.org/r/alpine.DEB.2.11.1504301343190.28879@gentwo.org
> 
> diff --git a/include/linux/cache.h b/include/linux/cache.h
> index 750621e41d1c..62bc5adc0ed5 100644
> --- a/include/linux/cache.h
> +++ b/include/linux/cache.h
> @@ -15,8 +15,14 @@
>  
>  /*
>   * __read_mostly is used to keep rarely changing variables out of frequently
> - * updated cachelines. If an architecture doesn't support it, ignore the
> - * hint.
> + * updated cachelines. Its use should be reserved for data that is used
> + * frequently in hot paths. Performance traces can help decide when to use
> + * this. You want __read_mostly data to be tightly packed, so that in the
> + * best case multiple frequently read variables for a hot path will be next
> + * to each other in order to reduce the number of cachelines needed to
> + * execute a critial path. We should be mindful and selective if its use.

                                                                 of its use.

> + *
> + * If an architecture doesn't support it, ignore the hint.
>   */
>  #ifndef __read_mostly
>  #define __read_mostly
> 

-- 
~Randy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-07 23:15 [PATCH] mm: expland documentation over __read_mostly Luis R. Rodriguez
  2018-05-08  0:20 ` Randy Dunlap
@ 2018-05-08  3:23 ` Joel Fernandes
  2018-05-08  8:28 ` David Howells
  2018-05-08 12:54 ` Christopher Lameter
  3 siblings, 0 replies; 11+ messages in thread
From: Joel Fernandes @ 2018-05-08  3:23 UTC (permalink / raw)
  To: Luis R. Rodriguez, tglx, arnd, cl
  Cc: keescook, luto, longman, viro, willy, ebiederm, linux-arch,
	linux-mm, linux-kernel



On May 7, 2018 4:15:06 PM PDT, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>__read_mostly can easily be misused by folks, its not meant for
>just read-only data. There are performance reasons for using it, but
>we also don't provide any guidance about its use. Provide a bit more
>guidance over it use.
>
>Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>---
> include/linux/cache.h | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>Every now and then we get a patch suggesting to use __read_mostly for
>something new or old but with no justifications. Add a bit more
>verbiage to help guide its users.
>
>Is this sufficient documentation to at least ask for a reason in the
>commit
>log as to why its being used for new entries? Or should we be explicit
>and
>ask for such justifications in commit logs? Taken from prior
>discussions
>with Christoph Lameter [0] over its use.
>
>[0]
>https://lkml.kernel.org/r/alpine.DEB.2.11.1504301343190.28879@gentwo.org
>
>diff --git a/include/linux/cache.h b/include/linux/cache.h
>index 750621e41d1c..62bc5adc0ed5 100644
>--- a/include/linux/cache.h
>+++ b/include/linux/cache.h
>@@ -15,8 +15,14 @@
> 
> /*
>* __read_mostly is used to keep rarely changing variables out of
>frequently
>- * updated cachelines. If an architecture doesn't support it, ignore
>the
>- * hint.
>+ * updated cachelines. Its use should be reserved for data that is
>used
>+ * frequently in hot paths. Performance traces can help decide when to
>use
>+ * this. You want __read_mostly data to be tightly packed, so that in
>the
>+ * best case multiple frequently read variables for a hot path will be
>next
>+ * to each other in order to reduce the number of cachelines needed to
>+ * execute a critial path. We should be mindful and selective if its

Nit: in its use.

- Joel


>use.
>+ *
>+ * If an architecture doesn't support it, ignore the hint.
>  */
> #ifndef __read_mostly
> #define __read_mostly

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-07 23:15 [PATCH] mm: expland documentation over __read_mostly Luis R. Rodriguez
  2018-05-08  0:20 ` Randy Dunlap
  2018-05-08  3:23 ` Joel Fernandes
@ 2018-05-08  8:28 ` David Howells
  2018-05-08 11:23   ` Matthew Wilcox
  2018-05-08 12:54 ` Christopher Lameter
  3 siblings, 1 reply; 11+ messages in thread
From: David Howells @ 2018-05-08  8:28 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: dhowells, Luis R. Rodriguez, tglx, arnd, cl, keescook, luto,
	longman, viro, willy, ebiederm, linux-arch, linux-mm,
	linux-kernel

Randy Dunlap <rdunlap@infradead.org> wrote:

> > + * execute a critial path. We should be mindful and selective if its use.
> 
>                                                                  of its use.

                                                                   in its use.

David

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-08  8:28 ` David Howells
@ 2018-05-08 11:23   ` Matthew Wilcox
  2018-05-08 15:39     ` Randy Dunlap
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2018-05-08 11:23 UTC (permalink / raw)
  To: David Howells
  Cc: Randy Dunlap, Luis R. Rodriguez, tglx, arnd, cl, keescook, luto,
	longman, viro, ebiederm, linux-arch, linux-mm, linux-kernel

On Tue, May 08, 2018 at 09:28:14AM +0100, David Howells wrote:
> Randy Dunlap <rdunlap@infradead.org> wrote:
> 
> > > + * execute a critial path. We should be mindful and selective if its use.
> > 
> >                                                                  of its use.
> 
>                                                                    in its use.
								     with its use.

Nah, just kidding.  Let's go with "in".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-07 23:15 [PATCH] mm: expland documentation over __read_mostly Luis R. Rodriguez
                   ` (2 preceding siblings ...)
  2018-05-08  8:28 ` David Howells
@ 2018-05-08 12:54 ` Christopher Lameter
  3 siblings, 0 replies; 11+ messages in thread
From: Christopher Lameter @ 2018-05-08 12:54 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: tglx, arnd, keescook, luto, longman, viro, willy, ebiederm,
	linux-arch, linux-mm, linux-kernel

On Mon, 7 May 2018, Luis R. Rodriguez wrote:

> __read_mostly can easily be misused by folks, its not meant for
> just read-only data. There are performance reasons for using it, but
> we also don't provide any guidance about its use. Provide a bit more
> guidance over it use.

Acked-by: Christoph Lameter <cl@linux.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-08 11:23   ` Matthew Wilcox
@ 2018-05-08 15:39     ` Randy Dunlap
  2018-05-08 18:17       ` Luis R. Rodriguez
  0 siblings, 1 reply; 11+ messages in thread
From: Randy Dunlap @ 2018-05-08 15:39 UTC (permalink / raw)
  To: Matthew Wilcox, David Howells
  Cc: Luis R. Rodriguez, tglx, arnd, cl, keescook, luto, longman, viro,
	ebiederm, linux-arch, linux-mm, linux-kernel

On 05/08/2018 04:23 AM, Matthew Wilcox wrote:
> On Tue, May 08, 2018 at 09:28:14AM +0100, David Howells wrote:
>> Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>>>> + * execute a critial path. We should be mindful and selective if its use.
>>>
>>>                                                                  of its use.
>>
>>                                                                    in its use.
> 								     with its use.
> 
> Nah, just kidding.  Let's go with "in".
> 

Yeah, no, I don't care.  Just flip a 3-sided coin.

-- 
~Randy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2018-05-08 15:39     ` Randy Dunlap
@ 2018-05-08 18:17       ` Luis R. Rodriguez
  0 siblings, 0 replies; 11+ messages in thread
From: Luis R. Rodriguez @ 2018-05-08 18:17 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Matthew Wilcox, David Howells, Luis R. Rodriguez, tglx, arnd, cl,
	keescook, luto, longman, viro, ebiederm, linux-arch, linux-mm,
	linux-kernel

On Tue, May 08, 2018 at 08:39:30AM -0700, Randy Dunlap wrote:
> On 05/08/2018 04:23 AM, Matthew Wilcox wrote:
> > On Tue, May 08, 2018 at 09:28:14AM +0100, David Howells wrote:
> >> Randy Dunlap <rdunlap@infradead.org> wrote:
> >>
> >>>> + * execute a critial path. We should be mindful and selective if its use.
> >>>
> >>>                                                                  of its use.
> >>
> >>                                                                    in its use.
> > 								     with its use.
> > 
> > Nah, just kidding.  Let's go with "in".
> > 
> 
> Yeah, no, I don't care.  Just flip a 3-sided coin.

Heh, the coin says "of".

I also added:

 * ie: if you're going to use it please supply a *good* justification in your   
 * commit log.

Sending v2.

  Luis

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2020-05-06 23:13 Luis Chamberlain
  2020-05-06 23:36 ` Rafael Aquini
@ 2020-05-06 23:44 ` Matthew Wilcox
  1 sibling, 0 replies; 11+ messages in thread
From: Matthew Wilcox @ 2020-05-06 23:44 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: cl, akpm, arnd, aquini, keescook, linux-mm, linux-arch, linux-kernel

On Wed, May 06, 2020 at 11:13:53PM +0000, Luis Chamberlain wrote:
> + * execute a critial path. We should be mindful and selective of its use.

"critical"


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mm: expland documentation over __read_mostly
  2020-05-06 23:13 Luis Chamberlain
@ 2020-05-06 23:36 ` Rafael Aquini
  2020-05-06 23:44 ` Matthew Wilcox
  1 sibling, 0 replies; 11+ messages in thread
From: Rafael Aquini @ 2020-05-06 23:36 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: cl, akpm, arnd, willy, keescook, linux-mm, linux-arch, linux-kernel

On Wed, May 06, 2020 at 11:13:53PM +0000, Luis Chamberlain wrote:
> __read_mostly can easily be misused by folks, its not meant for
> just read-only data. There are performance reasons for using it, but
> we also don't provide any guidance about its use. Provide a bit more
> guidance over it use.
               s/it/its

same goes for the subject, as I think there is a minor typo: s/expland/expand

> 
> Acked-by: Christoph Lameter <cl@linux.com>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> 
> I sent this 2 years ago, but it fell through the cracks. This time
> I'm adding Andrew Morton now, the fix0r-of-falling-through-the-cracks.
> 
> Resending as I just saw a patch which doesn't clearly justifiy the
> merits of the use of __read_mostly on it.
> 

That would be my fault! (sorry) given the rationale below, the patch I sent
really doesn't need the hint. Thanks for the extra bit of education here.

(not an excuse) In a glance over the source tree, though, it seems most 
of the hinting cases are doing it in the misguided way.


>  include/linux/cache.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/cache.h b/include/linux/cache.h
> index 750621e41d1c..8106fb304fa7 100644
> --- a/include/linux/cache.h
> +++ b/include/linux/cache.h
> @@ -15,8 +15,14 @@
>  
>  /*
>   * __read_mostly is used to keep rarely changing variables out of frequently
> - * updated cachelines. If an architecture doesn't support it, ignore the
> - * hint.
> + * updated cachelines. Its use should be reserved for data that is used
> + * frequently in hot paths. Performance traces can help decide when to use
> + * this. You want __read_mostly data to be tightly packed, so that in the
> + * best case multiple frequently read variables for a hot path will be next
> + * to each other in order to reduce the number of cachelines needed to
> + * execute a critial path. We should be mindful and selective of its use.
> + * ie: if you're going to use it please supply a *good* justification in your
> + * commit log
>   */
>  #ifndef __read_mostly
>  #define __read_mostly
> -- 
> 2.25.1
> 

Acked-by: Rafael Aquini <aquini@redhat.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] mm: expland documentation over __read_mostly
@ 2020-05-06 23:13 Luis Chamberlain
  2020-05-06 23:36 ` Rafael Aquini
  2020-05-06 23:44 ` Matthew Wilcox
  0 siblings, 2 replies; 11+ messages in thread
From: Luis Chamberlain @ 2020-05-06 23:13 UTC (permalink / raw)
  To: cl, akpm
  Cc: arnd, willy, aquini, keescook, linux-mm, linux-arch,
	linux-kernel, Luis Chamberlain

__read_mostly can easily be misused by folks, its not meant for
just read-only data. There are performance reasons for using it, but
we also don't provide any guidance about its use. Provide a bit more
guidance over it use.

Acked-by: Christoph Lameter <cl@linux.com>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

I sent this 2 years ago, but it fell through the cracks. This time
I'm adding Andrew Morton now, the fix0r-of-falling-through-the-cracks.

Resending as I just saw a patch which doesn't clearly justifiy the
merits of the use of __read_mostly on it.

 include/linux/cache.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/cache.h b/include/linux/cache.h
index 750621e41d1c..8106fb304fa7 100644
--- a/include/linux/cache.h
+++ b/include/linux/cache.h
@@ -15,8 +15,14 @@
 
 /*
  * __read_mostly is used to keep rarely changing variables out of frequently
- * updated cachelines. If an architecture doesn't support it, ignore the
- * hint.
+ * updated cachelines. Its use should be reserved for data that is used
+ * frequently in hot paths. Performance traces can help decide when to use
+ * this. You want __read_mostly data to be tightly packed, so that in the
+ * best case multiple frequently read variables for a hot path will be next
+ * to each other in order to reduce the number of cachelines needed to
+ * execute a critial path. We should be mindful and selective of its use.
+ * ie: if you're going to use it please supply a *good* justification in your
+ * commit log
  */
 #ifndef __read_mostly
 #define __read_mostly
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-06 23:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-07 23:15 [PATCH] mm: expland documentation over __read_mostly Luis R. Rodriguez
2018-05-08  0:20 ` Randy Dunlap
2018-05-08  3:23 ` Joel Fernandes
2018-05-08  8:28 ` David Howells
2018-05-08 11:23   ` Matthew Wilcox
2018-05-08 15:39     ` Randy Dunlap
2018-05-08 18:17       ` Luis R. Rodriguez
2018-05-08 12:54 ` Christopher Lameter
2020-05-06 23:13 Luis Chamberlain
2020-05-06 23:36 ` Rafael Aquini
2020-05-06 23:44 ` Matthew Wilcox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).