Message-ID: <rwnvktudfyagql35wimoeshpghoccv2stbk72uukdlnmr6iagm@xwknjlczowox>
Date: Tue, 2 Sep 2025 10:19:27 +0200
From: Alejandro Colomar <alx@...nel.org>
To: Usama Arif <usamaarif642@...il.com>
Cc: linux-man@...r.kernel.org, david@...hat.com, 
	lorenzo.stoakes@...cle.com, hannes@...xchg.org, baohua@...nel.org, shakeel.butt@...ux.dev, 
	ziy@...dia.com, laoar.shao@...il.com, baolin.wang@...ux.alibaba.com, 
	Liam.Howlett@...cle.com, linux-kernel@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH] PR_*ET_THP_DISABLE.2const: document addition of
 PR_THP_DISABLE_EXCEPT_ADVISED

Hi Usama,

On Mon, Sep 01, 2025 at 05:58:39PM +0100, Usama Arif wrote:
> Thanks for the quick review! Its my first time writing a man page so was apologies
> if there were some basic mistakes in formatting.

No problem.  Formatting issues are usual, and expected.  :)

> >> @@ -373,7 +373,9 @@ nor can it be stack memory or backed by a DAX-enabled device
> >>  (unless the DAX device is hot-plugged as System RAM).
> >>  The process must also not have
> >>  .B PR_SET_THP_DISABLE
> >> -set (see
> >> +set without the
> >> +.B PR_THP_DISABLE_EXCEPT_ADVISED
> >> +flag (see
> >>  .BR prctl (2)).
> > 
> > Double negation is confusing.  Please rephrase to something like
> > 
> > 	The process can have X set
> > 	only if Y is also set.
> > 
> 
> Yes, makes sense, will change to belwow in the next revision:
> 
> The process can have
> .B PR_SET_THP_DISABLE
> set only if
> .B PR_THP_DISABLE_EXCEPT_ADVISED
> flag is set (see
> .BR prctl (2)).

Sounds good.

> >>  .IP
> >>  The
> >> diff --git a/man/man2const/PR_GET_THP_DISABLE.2const b/man/man2const/PR_GET_THP_DISABLE.2const
> >> index 38ff3b370..df239700f 100644
> >> --- a/man/man2const/PR_GET_THP_DISABLE.2const
> >> +++ b/man/man2const/PR_GET_THP_DISABLE.2const
> >> @@ -6,7 +6,7 @@
> >>  .SH NAME
> >>  PR_GET_THP_DISABLE
> >>  \-
> >> -get the state of the "THP disable" flag for the calling thread
> >> +get the state of the "THP disable" flags for the calling thread
> >>  .SH LIBRARY
> >>  Standard C library
> >>  .RI ( libc ,\~ \-lc )
> >> @@ -18,13 +18,21 @@ Standard C library
> >>  .B int prctl(PR_GET_THP_DISABLE, 0L, 0L, 0L, 0L);
> >>  .fi
> >>  .SH DESCRIPTION
> >> -Return the current setting of
> >> -the "THP disable" flag for the calling thread:
> >> -either 1, if the flag is set, or 0, if it is not.
> >> +Returns a value whose bits indicate how THP-disable is configured
> > 
> > s/Returns/Return/
> > 
> 
> ack

The thing is, if we were writing it from scratch, I wouldn't have a
preference.  However, given it already uses Return, I don't see strong
reasons to change it.

If you still prefer to change it, though, feel free to insist.

> >> +for the calling thread.
> >> +The returned value is interpreted as follows:
> >> +.P
> >> +.nf
> >> +.B "Bits"
> >> +.B " 1 0  Value  Description"
> >> + 0 0    0    No THP-disable behaviour specified.
> >> + 0 1    1    THP is entirely disabled for this process.
> >> + 1 1    3    THP-except-advised mode is set for this process.
> > 
> > We should probably use a table with .TS/.TE.  See examples of this in
> > other manual pages for how to use that (or read tbl(1) if you want).
> > 
> > If you don't know how to use that, I can do it myself.  tbl(1) is a bit
> > weird.
> 
> 
> I tried below, and it seemed to look ok in the output, but please let me know if
> its ok:
> 
> .TS
> allbox;
> cb cb cb l
> c c c l.
> Bit 1	Bit 0	Value	Description
> 0	0	0	No THP-disable behaviour specified.
> 0	1	1	THP is entirely disabled for this process.
> 1	1	3	THP-except-advised mode is set for this process.
> .TE

At first glance, looks good.

> >> @@ -15,24 +15,62 @@ Standard C library
> >>  .BR "#include <linux/prctl.h>" "  /* Definition of " PR_* " constants */"
> >>  .B #include <sys/prctl.h>
> >>  .P
> >> -.BI "int prctl(PR_SET_THP_DISABLE, long " flag ", 0L, 0L, 0L);"
> >> +.BI "int prctl(PR_SET_THP_DISABLE, long " thp_disable ", unsigned long " flags ", 0L, 0L);"
> > 
> > Hmmm, I'm reading this weirdly.
> > 
> > Old code doing prctl(PR_SET_THP_DIABLE, 1, 0L, 0L, 0L); would be
> > transformed from setting the flag before, to now using 0L as flags?
> > 
> > Or how is backwards compatibility handled?
> > 
> 
> 
> Its still backwards compatible. The name of the arguments is changed, but the arg values have not.
> Before you could do 2 things:
> 
> prctl(PR_SET_THP_DISABLE, 0, 0, 0, 0); // to reset THP setting.
> prctl(PR_SET_THP_DISABLE, 1, 0, 0, 0); // to disable THPs completely.
> 
> Now in addition to the 2 calls above, you can do:
> prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, 0, 0); // to disable THPs except madvise.

Thanks!  This helped me understand it.  I think this would be useful in
the commit message.

> > 
> > See man-pages(7) ("Lists"):
> > 
> >        There should always be exactly 2 spaces between the list  symbol
> >        and  the  elements.   This doesn't apply to "tagged paragraphs",
> >        which use the default indentation rules.
> > 
> > (If you grep(1) around, you'll see that number everywhere.)
> > 
> >> +Global THP controls are set to "always" or "madvise" and
> >> +.BR madvise (...,
> >> +.BR MADV_HUGEPAGE )
> > 
> > I'd say
> > 
> > 	.I madvise(..., MADV_HUGEPAGE)
> > 
> > as an inlined expression, which goes in full italics; that's simpler.
> 
> This results in the entire line being underlined, which is probably not what
> not what we want?

The entire function call is underlined; see man-pages(7):

       Expressions, if not written on a separate indented line,  should
       be  specified  in italics.  Again, the use of nonbreaking spaces
       may be appropriate if the  expression  is  inlined  with  normal
       text.

Which reminds me to prevent the line from breaking at that expression:

	.I \%madvise(...,\~MADV_HUGEPAGE)


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es>
Use port 80 (that is, <...:80/>).

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists