[RFC 0/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[RFC 0/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Frank A. Cancio Bello]

Improves the documentation of buffer_size_kb by clarifying how is
calculated the number of allocated pages for the ring buffer.

** Do not apply the second patch **. It's just for illustration
purposes.

Frank A. Cancio Bello (2):
  docs: ftrace: Clarify the RAM impact of buffer_size_kb
  ** do not apply this patch ** Just for illustration purposes

 Documentation/trace/ftrace.rst | 13 +++++++++++--
 kernel/trace/ring_buffer.c     |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.17.1

[RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Frank A. Cancio Bello]

The current text could mislead the user into believing that the number
of pages allocated by each CPU ring buffer is calculated by the round
up of the division: buffer_size_kb / PAGE_SIZE.

Clarify that the number of pages allocated is the round up of the
division: buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE). Add an
example that shows how the number of pages allocated could be off by
5 pages more compared with how the current text suggests it should be.

Suggested-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Signed-off-by: Frank A. Cancio Bello <frank@generalsoftwareinc.com>
---
 Documentation/trace/ftrace.rst | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
index e3060eedb22d..ec2c4eff95a6 100644
--- a/Documentation/trace/ftrace.rst
+++ b/Documentation/trace/ftrace.rst
@@ -188,8 +188,17 @@ of ftrace. Here is a list of some of the key files:
 	If the last page allocated has room for more bytes
 	than requested, the rest of the page will be used,
 	making the actual allocation bigger than requested or shown.
-	( Note, the size may not be a multiple of the page size
-	due to buffer management meta-data. )
+
+        The number of pages allocated for each CPU buffer may not
+        be the same than the round up of the division:
+        buffer_size_kb / PAGE_SIZE. This is because part of each page is
+        used to store a page header with metadata. E.g. with
+        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
+        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
+        page header with metadata) the number of pages allocated for each
+        CPU buffer is 1029, not 1024. The formula for calculating the
+        number of pages allocated for each CPU buffer is the round up of:
+        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).
 
 	Buffer sizes for individual CPUs may vary
 	(see "per_cpu/cpu0/buffer_size_kb" below), and if they do
-- 
2.17.1

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Steven Rostedt]

On Wed, 13 Nov 2019 11:32:36 -0500
"Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:

> The current text could mislead the user into believing that the number
> of pages allocated by each CPU ring buffer is calculated by the round
> up of the division: buffer_size_kb / PAGE_SIZE.
> 
> Clarify that the number of pages allocated is the round up of the
> division: buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE). Add an
> example that shows how the number of pages allocated could be off by
> 5 pages more compared with how the current text suggests it should be.
> 
> Suggested-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Signed-off-by: Frank A. Cancio Bello <frank@generalsoftwareinc.com>
> ---
>  Documentation/trace/ftrace.rst | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
> index e3060eedb22d..ec2c4eff95a6 100644
> --- a/Documentation/trace/ftrace.rst
> +++ b/Documentation/trace/ftrace.rst
> @@ -188,8 +188,17 @@ of ftrace. Here is a list of some of the key files:
>  	If the last page allocated has room for more bytes
>  	than requested, the rest of the page will be used,
>  	making the actual allocation bigger than requested or shown.
> -	( Note, the size may not be a multiple of the page size
> -	due to buffer management meta-data. )

The above is not untrue ;-)

> +
> +        The number of pages allocated for each CPU buffer may not
> +        be the same than the round up of the division:
> +        buffer_size_kb / PAGE_SIZE. This is because part of each page is
> +        used to store a page header with metadata. E.g. with
> +        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
> +        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
> +        page header with metadata) the number of pages allocated for each
> +        CPU buffer is 1029, not 1024. The formula for calculating the
> +        number of pages allocated for each CPU buffer is the round up of:
> +        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).

I have no problem with this patch, but the concern of documenting the
implementation here, which will most likely not be updated if the
implementation is ever changed, which is why I was vague to begin with.

But it may never be changed as that code has been like that for a
decade now.

-- Steve


>  
>  	Buffer sizes for individual CPUs may vary
>  	(see "per_cpu/cpu0/buffer_size_kb" below), and if they do

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Joel Fernandes]

On Wed, Nov 13, 2019 at 11:37:30AM -0500, Steven Rostedt wrote:
> On Wed, 13 Nov 2019 11:32:36 -0500
> "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:
[snip]
> > +
> > +        The number of pages allocated for each CPU buffer may not
> > +        be the same than the round up of the division:
> > +        buffer_size_kb / PAGE_SIZE. This is because part of each page is
> > +        used to store a page header with metadata. E.g. with
> > +        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
> > +        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
> > +        page header with metadata) the number of pages allocated for each
> > +        CPU buffer is 1029, not 1024. The formula for calculating the
> > +        number of pages allocated for each CPU buffer is the round up of:
> > +        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).
> 
> I have no problem with this patch, but the concern of documenting the
> implementation here, which will most likely not be updated if the
> implementation is ever changed, which is why I was vague to begin with.
> 
> But it may never be changed as that code has been like that for a
> decade now.

Agreed. To give some context, Frank is an outreachy intern I am working with and
one of his starter tasks was to understand the ring buffer's basics.  I asked
him to send a patch since I thought he mentioned there was an error in the
documnentation. It looks like all that was missing is some explanation which
the deleted text in brackets above should already cover.

Steve, your call if you want this patch. Looks like Frank understands the
page header taking up some space, so one of the goals of the exercise is
accomplished ;-)

thanks,

 - Joel

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Steven Rostedt]

On Thu, 14 Nov 2019 15:20:59 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Wed, Nov 13, 2019 at 11:37:30AM -0500, Steven Rostedt wrote:
> > On Wed, 13 Nov 2019 11:32:36 -0500
> > "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:  
> [snip]
> > > +
> > > +        The number of pages allocated for each CPU buffer may not
> > > +        be the same than the round up of the division:
> > > +        buffer_size_kb / PAGE_SIZE. This is because part of each page is
> > > +        used to store a page header with metadata. E.g. with
> > > +        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
> > > +        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
> > > +        page header with metadata) the number of pages allocated for each
> > > +        CPU buffer is 1029, not 1024. The formula for calculating the
> > > +        number of pages allocated for each CPU buffer is the round up of:
> > > +        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).  
> > 
> > I have no problem with this patch, but the concern of documenting the
> > implementation here, which will most likely not be updated if the
> > implementation is ever changed, which is why I was vague to begin with.
> > 
> > But it may never be changed as that code has been like that for a
> > decade now.  
> 
> Agreed. To give some context, Frank is an outreachy intern I am working with and
> one of his starter tasks was to understand the ring buffer's basics.  I asked
> him to send a patch since I thought he mentioned there was an error in the
> documnentation. It looks like all that was missing is some explanation which
> the deleted text in brackets above should already cover.
> 
> Steve, your call if you want this patch. Looks like Frank understands the
> page header taking up some space, so one of the goals of the exercise is
> accomplished ;-)

Yes agreed, what was written was not wrong (thus understood). But the
more I think about this, the less I like the implementation details in
the documentation directory. Now I am looking forward for some other
patches from Frank, and perhaps he could add some comments in
ring_buffer.c about this. ;-)

-- Steve

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Frank A. Cancio Bello]

On Thu, Nov 14, 2019 at 04:36:39PM -0500, Steven Rostedt wrote:
> On Thu, 14 Nov 2019 15:20:59 -0500
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > On Wed, Nov 13, 2019 at 11:37:30AM -0500, Steven Rostedt wrote:
> > > On Wed, 13 Nov 2019 11:32:36 -0500
> > > "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:  
> > [snip]
> > > > +
> > > > +        The number of pages allocated for each CPU buffer may not
> > > > +        be the same than the round up of the division:
> > > > +        buffer_size_kb / PAGE_SIZE. This is because part of each page is
> > > > +        used to store a page header with metadata. E.g. with
> > > > +        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
> > > > +        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
> > > > +        page header with metadata) the number of pages allocated for each
> > > > +        CPU buffer is 1029, not 1024. The formula for calculating the
> > > > +        number of pages allocated for each CPU buffer is the round up of:
> > > > +        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).  
> > > 
> > > I have no problem with this patch, but the concern of documenting the
> > > implementation here, which will most likely not be updated if the
> > > implementation is ever changed, which is why I was vague to begin with.
> > > 
> > > But it may never be changed as that code has been like that for a
> > > decade now.  
> > 
> > Agreed. To give some context, Frank is an outreachy intern I am working with and
> > one of his starter tasks was to understand the ring buffer's basics.  I asked
> > him to send a patch since I thought he mentioned there was an error in the
> > documnentation. It looks like all that was missing is some explanation which
> > the deleted text in brackets above should already cover.
> > 

Not exactly in my opinion ;) The deleted text was not the problem. I
just deleted it because with the added text it turns to be redundant.

The issue that I found with the documentation (maybe just to my
newbie's eyes) is in this part:

"The trace buffers are allocated in pages (blocks of memory that the
kernel uses for allocation, usually 4 KB in size). If the last page
allocated has room for more bytes than requested, the rest of the
page will be used, making the actual allocation bigger than requested
or shown."

For me that "suggests" the interpretation that the number of pages
allocated in the current implementation correspond with the round
integer division of buffer_size_kb / PAGE_SIZE, which is inaccurate
(for 5 pages in the example that I mentioned).


> > Steve, your call if you want this patch. Looks like Frank understands the
> > page header taking up some space, so one of the goals of the exercise is
> > accomplished ;-)
> 
> Yes agreed, what was written was not wrong (thus understood). But the
> more I think about this, the less I like the implementation details in
> the documentation directory.

Understood and agreed. It is funny that what I spotted as "a problem"
was precisely an incomplete description of the implementation (the
sentences that I quoted above). What do you think about removing
those two sentences?

> Now I am looking forward for some other
> patches from Frank, and perhaps he could add some comments in
> ring_buffer.c about this. ;-)
>

You can count on it. I'm just starting to learn. I'm very grateful
that Joel took me under his wing ;) and with the time I hope to be
able to give back more to the community with the help of experts like
you Steve.

Thank you, Steve and Joel, for such quick feedback!
frank a.

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Steven Rostedt]

On Thu, 14 Nov 2019 23:24:28 -0500
"Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:

> On Thu, Nov 14, 2019 at 04:36:39PM -0500, Steven Rostedt wrote:
> > On Thu, 14 Nov 2019 15:20:59 -0500
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> >   
> > > On Wed, Nov 13, 2019 at 11:37:30AM -0500, Steven Rostedt wrote:  
> > > > On Wed, 13 Nov 2019 11:32:36 -0500
> > > > "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:    
> > > [snip]  
> > > > > +
> > > > > +        The number of pages allocated for each CPU buffer may not
> > > > > +        be the same than the round up of the division:
> > > > > +        buffer_size_kb / PAGE_SIZE. This is because part of each page is
> > > > > +        used to store a page header with metadata. E.g. with
> > > > > +        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
> > > > > +        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
> > > > > +        page header with metadata) the number of pages allocated for each
> > > > > +        CPU buffer is 1029, not 1024. The formula for calculating the
> > > > > +        number of pages allocated for each CPU buffer is the round up of:
> > > > > +        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).    
> > > > 
> > > > I have no problem with this patch, but the concern of documenting the
> > > > implementation here, which will most likely not be updated if the
> > > > implementation is ever changed, which is why I was vague to begin with.
> > > > 
> > > > But it may never be changed as that code has been like that for a
> > > > decade now.    
> > > 
> > > Agreed. To give some context, Frank is an outreachy intern I am working with and
> > > one of his starter tasks was to understand the ring buffer's basics.  I asked
> > > him to send a patch since I thought he mentioned there was an error in the
> > > documnentation. It looks like all that was missing is some explanation which
> > > the deleted text in brackets above should already cover.
> > >   
> 
> Not exactly in my opinion ;) The deleted text was not the problem. I
> just deleted it because with the added text it turns to be redundant.
> 
> The issue that I found with the documentation (maybe just to my
> newbie's eyes) is in this part:
> 
> "The trace buffers are allocated in pages (blocks of memory that the
> kernel uses for allocation, usually 4 KB in size). If the last page
> allocated has room for more bytes than requested, the rest of the
> page will be used, making the actual allocation bigger than requested
> or shown."
> 
> For me that "suggests" the interpretation that the number of pages
> allocated in the current implementation correspond with the round
> integer division of buffer_size_kb / PAGE_SIZE, which is inaccurate
> (for 5 pages in the example that I mentioned).

If you would like, you could reword that to something more accurate,
but still not detailing the implementation.

> Understood and agreed. It is funny that what I spotted as "a problem"
> was precisely an incomplete description of the implementation (the
> sentences that I quoted above). What do you think about removing
> those two sentences?

I wouldn't remove them, just reword them to something you find more
accurate.

-- Steve

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Frank A. Cancio Bello]

On Fri, Nov 15, 2019 at 08:30:00AM -0500, Steven Rostedt wrote:
> On Thu, 14 Nov 2019 23:24:28 -0500
> "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:
> 
> > On Thu, Nov 14, 2019 at 04:36:39PM -0500, Steven Rostedt wrote:
> > > On Thu, 14 Nov 2019 15:20:59 -0500
> > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > >   
> > > > On Wed, Nov 13, 2019 at 11:37:30AM -0500, Steven Rostedt wrote:  
> > > > > On Wed, 13 Nov 2019 11:32:36 -0500
> > > > > "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:    
> > > > [snip]  
> > > > > > +
> > > > > > +        The number of pages allocated for each CPU buffer may not
> > > > > > +        be the same than the round up of the division:
> > > > > > +        buffer_size_kb / PAGE_SIZE. This is because part of each page is
> > > > > > +        used to store a page header with metadata. E.g. with
> > > > > > +        buffer_size_kb=4096 (kilobytes), a PAGE_SIZE=4096 bytes and a
> > > > > > +        BUF_PAGE_HDR_SIZE=16 bytes (BUF_PAGE_HDR_SIZE is the size of the
> > > > > > +        page header with metadata) the number of pages allocated for each
> > > > > > +        CPU buffer is 1029, not 1024. The formula for calculating the
> > > > > > +        number of pages allocated for each CPU buffer is the round up of:
> > > > > > +        buffer_size_kb / (PAGE_SIZE - BUF_PAGE_HDR_SIZE).    
> > > > > 
> > > > > I have no problem with this patch, but the concern of documenting the
> > > > > implementation here, which will most likely not be updated if the
> > > > > implementation is ever changed, which is why I was vague to begin with.
> > > > > 
> > > > > But it may never be changed as that code has been like that for a
> > > > > decade now.    
> > > > 
> > > > Agreed. To give some context, Frank is an outreachy intern I am working with and
> > > > one of his starter tasks was to understand the ring buffer's basics.  I asked
> > > > him to send a patch since I thought he mentioned there was an error in the
> > > > documnentation. It looks like all that was missing is some explanation which
> > > > the deleted text in brackets above should already cover.
> > > >   
> > 
> > Not exactly in my opinion ;) The deleted text was not the problem. I
> > just deleted it because with the added text it turns to be redundant.
> > 
> > The issue that I found with the documentation (maybe just to my
> > newbie's eyes) is in this part:
> > 
> > "The trace buffers are allocated in pages (blocks of memory that the
> > kernel uses for allocation, usually 4 KB in size). If the last page
> > allocated has room for more bytes than requested, the rest of the
> > page will be used, making the actual allocation bigger than requested
> > or shown."
> > 
> > For me that "suggests" the interpretation that the number of pages
> > allocated in the current implementation correspond with the round
> > integer division of buffer_size_kb / PAGE_SIZE, which is inaccurate
> > (for 5 pages in the example that I mentioned).
> 
> If you would like, you could reword that to something more accurate,
> but still not detailing the implementation.
> 
> > Understood and agreed. It is funny that what I spotted as "a problem"
> > was precisely an incomplete description of the implementation (the
> > sentences that I quoted above). What do you think about removing
> > those two sentences?
> 
> I wouldn't remove them, just reword them to something you find more
> accurate.
> 

I feel that adding:

"A few extra pages may be allocated to accommodate buffer management
meta-data."

between the two sentences that I quoted will address the issue. If
that is OK with you I will proceed to package this change in a new
patchset along with a few fixes of typos that I spotted in other
parts of the doc.

thanks one more time for your quick response.
frank a.

Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb

[Steven Rostedt]

On Fri, 15 Nov 2019 10:59:55 -0500
"Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:

> I feel that adding:
> 
> "A few extra pages may be allocated to accommodate buffer management
> meta-data."
> 
> between the two sentences that I quoted will address the issue. If
> that is OK with you I will proceed to package this change in a new
> patchset along with a few fixes of typos that I spotted in other
> parts of the doc.

Yep, I'm OK with that.

Thanks!

-- Steve

[RFC 2/2] ** do not apply this patch ** Just for illustration purposes

[Frank A. Cancio Bello]

Prints a message that will allow us to evaluate the number of pages
allocated by each CPU buffer as well the main values that participate
in its calculation.

 $ echo 0 > /sys/kernel/debug/tracing/tracing_on
 $ echo 4096 > /sys/kernel/debug/tracing/buffer_size_kb
   .... e.g. of output:
   PAGE_SIZE: 4096, BUF_PAGE_HDR_SIZE: 16, size: 4194304, nr_pages: 1029

Signed-off-by: Frank A. Cancio Bello <frank@generalsoftwareinc.com>
---
 kernel/trace/ring_buffer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 66358d66c933..c10b6bcb29b9 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1730,6 +1730,8 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
 		return size;
 
 	nr_pages = DIV_ROUND_UP(size, BUF_PAGE_SIZE);
+	printk(KERN_ALERT "PAGE_SIZE: %lu, BUF_PAGE_HDR_SIZE: %lu, size: %lu, nr_pages: %ld",
+	       PAGE_SIZE, BUF_PAGE_HDR_SIZE, size, nr_pages);
 
 	/* we need a minimum of two pages */
 	if (nr_pages < 2)
-- 
2.17.1