[PATCH] lib/test_free_pages: Add basic progress indicators

[PATCH] lib/test_free_pages: Add basic progress indicators

[Geert Uytterhoeven]

The test module to check that free_pages() does not leak memory does not
provide any feedback whatsoever its state or progress, but may take some
time on slow machines.  Add the printing of messages upon starting each
phase of the test, and upon completion.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 lib/test_free_pages.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/test_free_pages.c b/lib/test_free_pages.c
index 074e76bd76b2b3c9..25ae1ac2624ae481 100644
--- a/lib/test_free_pages.c
+++ b/lib/test_free_pages.c
@@ -5,6 +5,8 @@
  * Author: Matthew Wilcox <willy@infradead.org>
  */
 
+#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
+
 #include <linux/gfp.h>
 #include <linux/mm.h>
 #include <linux/module.h>
@@ -26,8 +28,11 @@ static void test_free_pages(gfp_t gfp)
 
 static int m_in(void)
 {
+	pr_info("Testing with GFP_KERNEL\n");
 	test_free_pages(GFP_KERNEL);
+	pr_info("Testing with GFP_KERNEL | __GFP_COMP\n");
 	test_free_pages(GFP_KERNEL | __GFP_COMP);
+	pr_info("Test completed\n");
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] lib/test_free_pages: Add basic progress indicators

[Matthew Wilcox]

On Sun, Oct 18, 2020 at 04:04:45PM +0200, Geert Uytterhoeven wrote:
> The test module to check that free_pages() does not leak memory does not
> provide any feedback whatsoever its state or progress, but may take some
> time on slow machines.  Add the printing of messages upon starting each
> phase of the test, and upon completion.

It's not supposed to take a long time.  Can you crank down that 1000 *
1000 to something more appropriate?

Re: [PATCH] lib/test_free_pages: Add basic progress indicators

[Geert Uytterhoeven]

Hi Matthew,

On Sun, Oct 18, 2020 at 4:25 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Sun, Oct 18, 2020 at 04:04:45PM +0200, Geert Uytterhoeven wrote:
> > The test module to check that free_pages() does not leak memory does not
> > provide any feedback whatsoever its state or progress, but may take some
> > time on slow machines.  Add the printing of messages upon starting each
> > phase of the test, and upon completion.
>
> It's not supposed to take a long time.  Can you crank down that 1000 *

It took 1m11s on ARAnyM, running on an i7-8700K.
Real hardware may even take longer.

> 1000 to something more appropriate?

What would be a suitable value? You do want to see it "leak gigabytes
of memory and probably OOM your system" if something's wrong,
so decreasing the value a lot may not be a good idea?

Regardless, if it OOMs, I think you do want to see this happens
while running this test.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] lib/test_free_pages: Add basic progress indicators

[Matthew Wilcox]

On Sun, Oct 18, 2020 at 04:39:27PM +0200, Geert Uytterhoeven wrote:
> Hi Matthew,
> 
> On Sun, Oct 18, 2020 at 4:25 PM Matthew Wilcox <willy@infradead.org> wrote:
> > On Sun, Oct 18, 2020 at 04:04:45PM +0200, Geert Uytterhoeven wrote:
> > > The test module to check that free_pages() does not leak memory does not
> > > provide any feedback whatsoever its state or progress, but may take some
> > > time on slow machines.  Add the printing of messages upon starting each
> > > phase of the test, and upon completion.
> >
> > It's not supposed to take a long time.  Can you crank down that 1000 *
> 
> It took 1m11s on ARAnyM, running on an i7-8700K.
> Real hardware may even take longer.

71 seconds is clearly too long.  0.7 seconds would be fine, so 10 * 1000
would be appropriate, but then that's only 320MB which might not be
enough to notice on a modern machine.

> > 1000 to something more appropriate?
> 
> What would be a suitable value? You do want to see it "leak gigabytes
> of memory and probably OOM your system" if something's wrong,
> so decreasing the value a lot may not be a good idea?
> 
> Regardless, if it OOMs, I think you do want to see this happens
> while running this test.

How about scaling with the amount of memory on the machine?

This might cause problems on machines with terabytes of memory.
Maybe we should cap it at a terabyte?

diff --git a/lib/test_free_pages.c b/lib/test_free_pages.c
index 074e76bd76b2..aa18fa52290a 100644
--- a/lib/test_free_pages.c
+++ b/lib/test_free_pages.c
@@ -9,11 +9,11 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 
-static void test_free_pages(gfp_t gfp)
+static void test_free_pages(gfp_t gfp, unsigned long totalram)
 {
-	unsigned int i;
+	unsigned long i, max = totalram / 8;
 
-	for (i = 0; i < 1000 * 1000; i++) {
+	for (i = 0; i < max; i++) {
 		unsigned long addr = __get_free_pages(gfp, 3);
 		struct page *page = virt_to_page(addr);
 
@@ -26,8 +26,11 @@ static void test_free_pages(gfp_t gfp)
 
 static int m_in(void)
 {
-	test_free_pages(GFP_KERNEL);
-	test_free_pages(GFP_KERNEL | __GFP_COMP);
+	struct sysinfo si;
+
+	si_meminfo(&si);
+	test_free_pages(GFP_KERNEL, si.totalram);
+	test_free_pages(GFP_KERNEL | __GFP_COMP, si.totalram);
 
 	return 0;
 }

Re: [PATCH] lib/test_free_pages: Add basic progress indicators

[Mike Rapoport]

On Sun, Oct 18, 2020 at 04:01:46PM +0100, Matthew Wilcox wrote:
> On Sun, Oct 18, 2020 at 04:39:27PM +0200, Geert Uytterhoeven wrote:
> > Hi Matthew,
> > 
> > On Sun, Oct 18, 2020 at 4:25 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > On Sun, Oct 18, 2020 at 04:04:45PM +0200, Geert Uytterhoeven wrote:
> > > > The test module to check that free_pages() does not leak memory does not
> > > > provide any feedback whatsoever its state or progress, but may take some
> > > > time on slow machines.  Add the printing of messages upon starting each
> > > > phase of the test, and upon completion.
> > >
> > > It's not supposed to take a long time.  Can you crank down that 1000 *
> > 
> > It took 1m11s on ARAnyM, running on an i7-8700K.
> > Real hardware may even take longer.
> 
> 71 seconds is clearly too long.  0.7 seconds would be fine, so 10 * 1000
> would be appropriate, but then that's only 320MB which might not be
> enough to notice on a modern machine.
> 
> > > 1000 to something more appropriate?
> > 
> > What would be a suitable value? You do want to see it "leak gigabytes
> > of memory and probably OOM your system" if something's wrong,
> > so decreasing the value a lot may not be a good idea?
> > 
> > Regardless, if it OOMs, I think you do want to see this happens
> > while running this test.
> 
> How about scaling with the amount of memory on the machine?
> 
> This might cause problems on machines with terabytes of memory.
> Maybe we should cap it at a terabyte?

On ARAnyM wih 782 MBytes of RAM running on i7-8650U it takes ~1.75
seconds.
Still, I think adding some verbosity to the test wouldn't hurt ;-)

> diff --git a/lib/test_free_pages.c b/lib/test_free_pages.c
> index 074e76bd76b2..aa18fa52290a 100644
> --- a/lib/test_free_pages.c
> +++ b/lib/test_free_pages.c
> @@ -9,11 +9,11 @@
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  
> -static void test_free_pages(gfp_t gfp)
> +static void test_free_pages(gfp_t gfp, unsigned long totalram)
>  {
> -	unsigned int i;
> +	unsigned long i, max = totalram / 8;
>  
> -	for (i = 0; i < 1000 * 1000; i++) {
> +	for (i = 0; i < max; i++) {
>  		unsigned long addr = __get_free_pages(gfp, 3);
>  		struct page *page = virt_to_page(addr);
>  
> @@ -26,8 +26,11 @@ static void test_free_pages(gfp_t gfp)
>  
>  static int m_in(void)
>  {
> -	test_free_pages(GFP_KERNEL);
> -	test_free_pages(GFP_KERNEL | __GFP_COMP);
> +	struct sysinfo si;
> +
> +	si_meminfo(&si);
> +	test_free_pages(GFP_KERNEL, si.totalram);
> +	test_free_pages(GFP_KERNEL | __GFP_COMP, si.totalram);
>  
>  	return 0;
>  }

-- 
Sincerely yours,
Mike.

Re: [PATCH] lib/test_free_pages: Add basic progress indicators

[Matthew Wilcox]

On Sun, Oct 18, 2020 at 08:12:52PM +0300, Mike Rapoport wrote:
> On Sun, Oct 18, 2020 at 04:01:46PM +0100, Matthew Wilcox wrote:
> > On Sun, Oct 18, 2020 at 04:39:27PM +0200, Geert Uytterhoeven wrote:
> > > Hi Matthew,
> > > 
> > > On Sun, Oct 18, 2020 at 4:25 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > On Sun, Oct 18, 2020 at 04:04:45PM +0200, Geert Uytterhoeven wrote:
> > > > > The test module to check that free_pages() does not leak memory does not
> > > > > provide any feedback whatsoever its state or progress, but may take some
> > > > > time on slow machines.  Add the printing of messages upon starting each
> > > > > phase of the test, and upon completion.
> > > >
> > > > It's not supposed to take a long time.  Can you crank down that 1000 *
> > > 
> > > It took 1m11s on ARAnyM, running on an i7-8700K.
> > > Real hardware may even take longer.
> > 
> > 71 seconds is clearly too long.  0.7 seconds would be fine, so 10 * 1000
> > would be appropriate, but then that's only 320MB which might not be
> > enough to notice on a modern machine.
> > 
> > > > 1000 to something more appropriate?
> > > 
> > > What would be a suitable value? You do want to see it "leak gigabytes
> > > of memory and probably OOM your system" if something's wrong,
> > > so decreasing the value a lot may not be a good idea?
> > > 
> > > Regardless, if it OOMs, I think you do want to see this happens
> > > while running this test.
> > 
> > How about scaling with the amount of memory on the machine?
> > 
> > This might cause problems on machines with terabytes of memory.
> > Maybe we should cap it at a terabyte?
> 
> On ARAnyM wih 782 MBytes of RAM running on i7-8650U it takes ~1.75
> seconds.

That seems like a somewhat unusual configuration.  I think it's pretty
strange to find an actual m68k with more than 128MB of memory.  I mean,
I can set up my laptop to believe it has 64TB of memory, and this will
run slowly, but I don't think it's any real problem.

> Still, I think adding some verbosity to the test wouldn't hurt ;-)

I prefer the unix philosophy of only emitting messages if something's
wrong.

Re: [PATCH] lib/test_free_pages: Add basic progress indicators

[Geert Uytterhoeven]

Hi Matthew,

On Mon, Oct 19, 2020 at 4:05 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Sun, Oct 18, 2020 at 08:12:52PM +0300, Mike Rapoport wrote:
> > On Sun, Oct 18, 2020 at 04:01:46PM +0100, Matthew Wilcox wrote:
> > > On Sun, Oct 18, 2020 at 04:39:27PM +0200, Geert Uytterhoeven wrote:
> > > > On Sun, Oct 18, 2020 at 4:25 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > On Sun, Oct 18, 2020 at 04:04:45PM +0200, Geert Uytterhoeven wrote:
> > > > > > The test module to check that free_pages() does not leak memory does not
> > > > > > provide any feedback whatsoever its state or progress, but may take some
> > > > > > time on slow machines.  Add the printing of messages upon starting each
> > > > > > phase of the test, and upon completion.
> > > > >
> > > > > It's not supposed to take a long time.  Can you crank down that 1000 *
> > > >
> > > > It took 1m11s on ARAnyM, running on an i7-8700K.
> > > > Real hardware may even take longer.
> > >
> > > 71 seconds is clearly too long.  0.7 seconds would be fine, so 10 * 1000
> > > would be appropriate, but then that's only 320MB which might not be
> > > enough to notice on a modern machine.
> > >
> > > > > 1000 to something more appropriate?
> > > >
> > > > What would be a suitable value? You do want to see it "leak gigabytes
> > > > of memory and probably OOM your system" if something's wrong,
> > > > so decreasing the value a lot may not be a good idea?
> > > >
> > > > Regardless, if it OOMs, I think you do want to see this happens
> > > > while running this test.
> > >
> > > How about scaling with the amount of memory on the machine?
> > >
> > > This might cause problems on machines with terabytes of memory.
> > > Maybe we should cap it at a terabyte?
> >
> > On ARAnyM wih 782 MBytes of RAM running on i7-8650U it takes ~1.75
> > seconds.
>
> That seems like a somewhat unusual configuration.  I think it's pretty
> strange to find an actual m68k with more than 128MB of memory.  I mean,
> I can set up my laptop to believe it has 64TB of memory, and this will
> run slowly, but I don't think it's any real problem.

Have you tried it on one of your parisc boxes? They tend to have quite
some RAM, compared to CPU speed.

I gave the unmodified test a try on a 1.5 GHz Cortex-A15, which is still
a decent arm32 system, and it took 4.25s.

> > Still, I think adding some verbosity to the test wouldn't hurt ;-)
>
> I prefer the unix philosophy of only emitting messages if something's
> wrong.

This is not a normal program, but a test.
Alternatively, convert it to kunit, so we'll at least be aware when
it starts running, and when it has completed ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds