diff mbox

mm: expland documentation over __read_mostly

Message ID 20180507231506.4891-1-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Luis Chamberlain May 7, 2018, 11:15 p.m. UTC
__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

Comments

Randy Dunlap May 8, 2018, 12:20 a.m. UTC | #1
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
>
Joel Fernandes May 8, 2018, 3:23 a.m. UTC | #2
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
David Howells May 8, 2018, 8:28 a.m. UTC | #3
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
Matthew Wilcox May 8, 2018, 11:23 a.m. UTC | #4
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".
Christoph Lameter (Ampere) May 8, 2018, 12:54 p.m. UTC | #5
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>
Randy Dunlap May 8, 2018, 3:39 p.m. UTC | #6
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.
Luis Chamberlain May 8, 2018, 6:17 p.m. UTC | #7
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
diff mbox

Patch

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