* [RFC 0/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
@ 2019-11-13 16:31 Frank A. Cancio Bello
2019-11-13 16:32 ` [RFC 1/2] " Frank A. Cancio Bello
2019-11-13 16:33 ` [RFC 2/2] ** do not apply this patch ** Just for illustration purposes Frank A. Cancio Bello
0 siblings, 2 replies; 12+ messages in thread
From: Frank A. Cancio Bello @ 2019-11-13 16:31 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar, Jonathan Corbet, linux-doc, linux-kernel
Cc: joel, saiprakash.ranjan
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-13 16:31 [RFC 0/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb Frank A. Cancio Bello
@ 2019-11-13 16:32 ` Frank A. Cancio Bello
2019-11-13 16:37 ` Steven Rostedt
2019-11-13 16:33 ` [RFC 2/2] ** do not apply this patch ** Just for illustration purposes Frank A. Cancio Bello
1 sibling, 1 reply; 12+ messages in thread
From: Frank A. Cancio Bello @ 2019-11-13 16:32 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar, Jonathan Corbet, linux-doc, linux-kernel
Cc: joel, saiprakash.ranjan
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC 2/2] ** do not apply this patch ** Just for illustration purposes
2019-11-13 16:31 [RFC 0/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb Frank A. Cancio Bello
2019-11-13 16:32 ` [RFC 1/2] " Frank A. Cancio Bello
@ 2019-11-13 16:33 ` Frank A. Cancio Bello
2019-11-13 21:06 ` kbuild test robot
2019-11-16 10:01 ` kbuild test robot
1 sibling, 2 replies; 12+ messages in thread
From: Frank A. Cancio Bello @ 2019-11-13 16:33 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar, Jonathan Corbet, linux-doc, linux-kernel
Cc: joel, saiprakash.ranjan
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-13 16:32 ` [RFC 1/2] " Frank A. Cancio Bello
@ 2019-11-13 16:37 ` Steven Rostedt
2019-11-14 20:20 ` Joel Fernandes
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2019-11-13 16:37 UTC (permalink / raw)
To: Frank A. Cancio Bello
Cc: Ingo Molnar, Jonathan Corbet, linux-doc, linux-kernel, joel,
saiprakash.ranjan
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] ** do not apply this patch ** Just for illustration purposes
2019-11-13 16:33 ` [RFC 2/2] ** do not apply this patch ** Just for illustration purposes Frank A. Cancio Bello
@ 2019-11-13 21:06 ` kbuild test robot
2019-11-16 10:01 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2019-11-13 21:06 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 9198 bytes --]
Hi "Frank,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on tip/perf/core]
[cannot apply to v5.4-rc7 next-20191113]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Frank-A-Cancio-Bello/docs-ftrace-Clarify-the-RAM-impact-of-buffer_size_kb/20191114-040608
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 295c52ee1485e4dee660fc1a0e6ceed6c803c9d3
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:15,
from include/asm-generic/bug.h:19,
from arch/x86/include/asm/bug.h:83,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from include/linux/ring_buffer.h:5,
from include/linux/trace_events.h:6,
from kernel/trace/ring_buffer.c:7:
kernel/trace/ring_buffer.c: In function 'ring_buffer_resize':
include/linux/kern_levels.h:5:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'unsigned int' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:9:20: note: in expansion of macro 'KERN_SOH'
#define KERN_ALERT KERN_SOH "1" /* action must be taken immediately */
^~~~~~~~
>> kernel/trace/ring_buffer.c:1733:9: note: in expansion of macro 'KERN_ALERT'
printk(KERN_ALERT "PAGE_SIZE: %lu, BUF_PAGE_HDR_SIZE: %lu, size: %lu, nr_pages: %ld",
^~~~~~~~~~
kernel/trace/ring_buffer.c:1733:58: note: format string is defined here
printk(KERN_ALERT "PAGE_SIZE: %lu, BUF_PAGE_HDR_SIZE: %lu, size: %lu, nr_pages: %ld",
~~^
%u
vim +/KERN_ALERT +1733 kernel/trace/ring_buffer.c
1703
1704 /**
1705 * ring_buffer_resize - resize the ring buffer
1706 * @buffer: the buffer to resize.
1707 * @size: the new size.
1708 * @cpu_id: the cpu buffer to resize
1709 *
1710 * Minimum size is 2 * BUF_PAGE_SIZE.
1711 *
1712 * Returns 0 on success and < 0 on failure.
1713 */
1714 int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
1715 int cpu_id)
1716 {
1717 struct ring_buffer_per_cpu *cpu_buffer;
1718 unsigned long nr_pages;
1719 int cpu, err = 0;
1720
1721 /*
1722 * Always succeed@resizing a non-existent buffer:
1723 */
1724 if (!buffer)
1725 return size;
1726
1727 /* Make sure the requested buffer exists */
1728 if (cpu_id != RING_BUFFER_ALL_CPUS &&
1729 !cpumask_test_cpu(cpu_id, buffer->cpumask))
1730 return size;
1731
1732 nr_pages = DIV_ROUND_UP(size, BUF_PAGE_SIZE);
> 1733 printk(KERN_ALERT "PAGE_SIZE: %lu, BUF_PAGE_HDR_SIZE: %lu, size: %lu, nr_pages: %ld",
1734 PAGE_SIZE, BUF_PAGE_HDR_SIZE, size, nr_pages);
1735
1736 /* we need a minimum of two pages */
1737 if (nr_pages < 2)
1738 nr_pages = 2;
1739
1740 size = nr_pages * BUF_PAGE_SIZE;
1741
1742 /*
1743 * Don't succeed if resizing is disabled, as a reader might be
1744 * manipulating the ring buffer and is expecting a sane state while
1745 * this is true.
1746 */
1747 if (atomic_read(&buffer->resize_disabled))
1748 return -EBUSY;
1749
1750 /* prevent another thread from changing buffer sizes */
1751 mutex_lock(&buffer->mutex);
1752
1753 if (cpu_id == RING_BUFFER_ALL_CPUS) {
1754 /* calculate the pages to update */
1755 for_each_buffer_cpu(buffer, cpu) {
1756 cpu_buffer = buffer->buffers[cpu];
1757
1758 cpu_buffer->nr_pages_to_update = nr_pages -
1759 cpu_buffer->nr_pages;
1760 /*
1761 * nothing more to do for removing pages or no update
1762 */
1763 if (cpu_buffer->nr_pages_to_update <= 0)
1764 continue;
1765 /*
1766 * to add pages, make sure all new pages can be
1767 * allocated without receiving ENOMEM
1768 */
1769 INIT_LIST_HEAD(&cpu_buffer->new_pages);
1770 if (__rb_allocate_pages(cpu_buffer->nr_pages_to_update,
1771 &cpu_buffer->new_pages, cpu)) {
1772 /* not enough memory for new pages */
1773 err = -ENOMEM;
1774 goto out_err;
1775 }
1776 }
1777
1778 get_online_cpus();
1779 /*
1780 * Fire off all the required work handlers
1781 * We can't schedule on offline CPUs, but it's not necessary
1782 * since we can change their buffer sizes without any race.
1783 */
1784 for_each_buffer_cpu(buffer, cpu) {
1785 cpu_buffer = buffer->buffers[cpu];
1786 if (!cpu_buffer->nr_pages_to_update)
1787 continue;
1788
1789 /* Can't run something on an offline CPU. */
1790 if (!cpu_online(cpu)) {
1791 rb_update_pages(cpu_buffer);
1792 cpu_buffer->nr_pages_to_update = 0;
1793 } else {
1794 schedule_work_on(cpu,
1795 &cpu_buffer->update_pages_work);
1796 }
1797 }
1798
1799 /* wait for all the updates to complete */
1800 for_each_buffer_cpu(buffer, cpu) {
1801 cpu_buffer = buffer->buffers[cpu];
1802 if (!cpu_buffer->nr_pages_to_update)
1803 continue;
1804
1805 if (cpu_online(cpu))
1806 wait_for_completion(&cpu_buffer->update_done);
1807 cpu_buffer->nr_pages_to_update = 0;
1808 }
1809
1810 put_online_cpus();
1811 } else {
1812 /* Make sure this CPU has been initialized */
1813 if (!cpumask_test_cpu(cpu_id, buffer->cpumask))
1814 goto out;
1815
1816 cpu_buffer = buffer->buffers[cpu_id];
1817
1818 if (nr_pages == cpu_buffer->nr_pages)
1819 goto out;
1820
1821 cpu_buffer->nr_pages_to_update = nr_pages -
1822 cpu_buffer->nr_pages;
1823
1824 INIT_LIST_HEAD(&cpu_buffer->new_pages);
1825 if (cpu_buffer->nr_pages_to_update > 0 &&
1826 __rb_allocate_pages(cpu_buffer->nr_pages_to_update,
1827 &cpu_buffer->new_pages, cpu_id)) {
1828 err = -ENOMEM;
1829 goto out_err;
1830 }
1831
1832 get_online_cpus();
1833
1834 /* Can't run something on an offline CPU. */
1835 if (!cpu_online(cpu_id))
1836 rb_update_pages(cpu_buffer);
1837 else {
1838 schedule_work_on(cpu_id,
1839 &cpu_buffer->update_pages_work);
1840 wait_for_completion(&cpu_buffer->update_done);
1841 }
1842
1843 cpu_buffer->nr_pages_to_update = 0;
1844 put_online_cpus();
1845 }
1846
1847 out:
1848 /*
1849 * The ring buffer resize can happen with the ring buffer
1850 * enabled, so that the update disturbs the tracing as little
1851 * as possible. But if the buffer is disabled, we do not need
1852 * to worry about that, and we can take the time to verify
1853 * that the buffer is not corrupt.
1854 */
1855 if (atomic_read(&buffer->record_disabled)) {
1856 atomic_inc(&buffer->record_disabled);
1857 /*
1858 * Even though the buffer was disabled, we must make sure
1859 * that it is truly disabled before calling rb_check_pages.
1860 * There could have been a race between checking
1861 * record_disable and incrementing it.
1862 */
1863 synchronize_rcu();
1864 for_each_buffer_cpu(buffer, cpu) {
1865 cpu_buffer = buffer->buffers[cpu];
1866 rb_check_pages(cpu_buffer);
1867 }
1868 atomic_dec(&buffer->record_disabled);
1869 }
1870
1871 mutex_unlock(&buffer->mutex);
1872 return size;
1873
1874 out_err:
1875 for_each_buffer_cpu(buffer, cpu) {
1876 struct buffer_page *bpage, *tmp;
1877
1878 cpu_buffer = buffer->buffers[cpu];
1879 cpu_buffer->nr_pages_to_update = 0;
1880
1881 if (list_empty(&cpu_buffer->new_pages))
1882 continue;
1883
1884 list_for_each_entry_safe(bpage, tmp, &cpu_buffer->new_pages,
1885 list) {
1886 list_del_init(&bpage->list);
1887 free_buffer_page(bpage);
1888 }
1889 }
1890 mutex_unlock(&buffer->mutex);
1891 return err;
1892 }
1893 EXPORT_SYMBOL_GPL(ring_buffer_resize);
1894
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28149 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-13 16:37 ` Steven Rostedt
@ 2019-11-14 20:20 ` Joel Fernandes
2019-11-14 21:36 ` Steven Rostedt
0 siblings, 1 reply; 12+ messages in thread
From: Joel Fernandes @ 2019-11-14 20:20 UTC (permalink / raw)
To: Steven Rostedt
Cc: Frank A. Cancio Bello, Ingo Molnar, Jonathan Corbet, linux-doc,
linux-kernel, saiprakash.ranjan
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-14 20:20 ` Joel Fernandes
@ 2019-11-14 21:36 ` Steven Rostedt
2019-11-15 4:24 ` Frank A. Cancio Bello
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2019-11-14 21:36 UTC (permalink / raw)
To: Joel Fernandes
Cc: Frank A. Cancio Bello, Ingo Molnar, Jonathan Corbet, linux-doc,
linux-kernel, saiprakash.ranjan
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-14 21:36 ` Steven Rostedt
@ 2019-11-15 4:24 ` Frank A. Cancio Bello
2019-11-15 13:30 ` Steven Rostedt
0 siblings, 1 reply; 12+ messages in thread
From: Frank A. Cancio Bello @ 2019-11-15 4:24 UTC (permalink / raw)
To: Steven Rostedt
Cc: Joel Fernandes, Ingo Molnar, Jonathan Corbet, linux-doc,
linux-kernel, saiprakash.ranjan
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-15 4:24 ` Frank A. Cancio Bello
@ 2019-11-15 13:30 ` Steven Rostedt
2019-11-15 15:59 ` Frank A. Cancio Bello
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2019-11-15 13:30 UTC (permalink / raw)
To: Frank A. Cancio Bello
Cc: Joel Fernandes, Ingo Molnar, Jonathan Corbet, linux-doc,
linux-kernel, saiprakash.ranjan
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-15 13:30 ` Steven Rostedt
@ 2019-11-15 15:59 ` Frank A. Cancio Bello
2019-11-15 16:03 ` Steven Rostedt
0 siblings, 1 reply; 12+ messages in thread
From: Frank A. Cancio Bello @ 2019-11-15 15:59 UTC (permalink / raw)
To: Steven Rostedt
Cc: Joel Fernandes, Ingo Molnar, Jonathan Corbet, linux-doc,
linux-kernel, saiprakash.ranjan
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb
2019-11-15 15:59 ` Frank A. Cancio Bello
@ 2019-11-15 16:03 ` Steven Rostedt
0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2019-11-15 16:03 UTC (permalink / raw)
To: Frank A. Cancio Bello
Cc: Joel Fernandes, Ingo Molnar, Jonathan Corbet, linux-doc,
linux-kernel, saiprakash.ranjan
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] ** do not apply this patch ** Just for illustration purposes
2019-11-13 16:33 ` [RFC 2/2] ** do not apply this patch ** Just for illustration purposes Frank A. Cancio Bello
2019-11-13 21:06 ` kbuild test robot
@ 2019-11-16 10:01 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2019-11-16 10:01 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]
Hi "Frank,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on tip/perf/core]
[cannot apply to v5.4-rc7 next-20191114]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Frank-A-Cancio-Bello/docs-ftrace-Clarify-the-RAM-impact-of-buffer_size_kb/20191114-040608
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 295c52ee1485e4dee660fc1a0e6ceed6c803c9d3
config: i386-randconfig-a002-20191115 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from <command-line>:0:0:
kernel/trace/ring_buffer.c: In function 'ring_buffer_resize':
>> kernel/trace/ring_buffer.c:125:43: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'unsigned int' [-Wformat=]
#define BUF_PAGE_HDR_SIZE offsetof(struct buffer_data_page, data)
^
include/linux/compiler_types.h:129:54: note: in definition of macro '__compiler_offsetof'
#define __compiler_offsetof(a, b) __builtin_offsetof(a, b)
^
>> kernel/trace/ring_buffer.c:125:27: note: in expansion of macro 'offsetof'
#define BUF_PAGE_HDR_SIZE offsetof(struct buffer_data_page, data)
^
>> kernel/trace/ring_buffer.c:1734:20: note: in expansion of macro 'BUF_PAGE_HDR_SIZE'
PAGE_SIZE, BUF_PAGE_HDR_SIZE, size, nr_pages);
^
vim +125 kernel/trace/ring_buffer.c
a358324466b171 Steven Rostedt 2008-11-11 124
499e547057f5bb Steven Rostedt 2012-02-22 @125 #define BUF_PAGE_HDR_SIZE offsetof(struct buffer_data_page, data)
033601a32b2012 Steven Rostedt 2008-11-21 126
:::::: The code at line 125 was first introduced by commit
:::::: 499e547057f5bba5cd6f87ebe59b05d0c59da905 tracing/ring-buffer: Only have tracing_on disable tracing buffers
:::::: TO: Steven Rostedt <srostedt@redhat.com>
:::::: CC: Steven Rostedt <rostedt@goodmis.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28029 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-11-16 10:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13 16:31 [RFC 0/2] docs: ftrace: Clarify the RAM impact of buffer_size_kb Frank A. Cancio Bello
2019-11-13 16:32 ` [RFC 1/2] " Frank A. Cancio Bello
2019-11-13 16:37 ` Steven Rostedt
2019-11-14 20:20 ` Joel Fernandes
2019-11-14 21:36 ` Steven Rostedt
2019-11-15 4:24 ` Frank A. Cancio Bello
2019-11-15 13:30 ` Steven Rostedt
2019-11-15 15:59 ` Frank A. Cancio Bello
2019-11-15 16:03 ` Steven Rostedt
2019-11-13 16:33 ` [RFC 2/2] ** do not apply this patch ** Just for illustration purposes Frank A. Cancio Bello
2019-11-13 21:06 ` kbuild test robot
2019-11-16 10:01 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.