[PATCH v2 0/2] Disable Android low memory killer

[PATCH v2 0/2] Disable Android low memory killer

[tim.gore]

From: Tim Gore <tim.gore@intel.com>

For some tests that put pressure on memory, the Android
lowmemorykiller needs to be disabled for the test to run to
completion. The first patch is a simple bit of preparation
to ensure that all (well written) "simple" tests exit via a
call to igt_exit, in the same way as tests with subtests do.
This is to make sure we can clean up by re-enabling the
lowmemorykiller.
The second patch is to disable the Android lowmemorykiller
during the common initialisation code (in oom_adjust_for_doom
to be exact) and to re-enstate it in igt_exit.

v1: As above

v2: Remove the call to disable the lowmemorykiller from
    oom_adjust_for_doom. lowmemorykiller is not disabled
    by default now; it is up to each individual test to
    call low_mem_killer_disable() if it needs to.

Tim Gore (2):
  lib/igt_core: make single/simple tests use igt_exit
  lib/igt_core.c: add function to disable lowmemorykiller

 lib/igt_core.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++---
 lib/igt_core.h         |  2 +-
 tests/igt_simulation.c |  2 +-
 3 files changed, 77 insertions(+), 6 deletions(-)

-- 
2.1.1

[PATCH v2 1/2] lib/igt_core: make single/simple tests use igt_exit

[tim.gore]

From: Tim Gore <tim.gore@intel.com>

Currently tests that use igt_simple_main will simply call
"exit()" if they pass, making it difficult to ensure that
any required cleanup is done. At present this is not an
issue, but it will be when I submit a patch to turn off the
lowmemorykiller for all tests.

Signed-off-by: Tim Gore <tim.gore@intel.com>
---
 lib/igt_core.c         | 9 +++++----
 lib/igt_core.h         | 2 +-
 tests/igt_simulation.c | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 5d41468..9344815 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -641,7 +641,7 @@ bool igt_only_list_subtests(void)
 static bool skipped_one = false;
 static bool succeeded_one = false;
 static bool failed_one = false;
-static int igt_exitcode;
+static int igt_exitcode = IGT_EXIT_SUCCESS;
 
 static void exit_subtest(const char *) __attribute__((noreturn));
 static void exit_subtest(const char *result)
@@ -692,7 +692,8 @@ void igt_skip(const char *f, ...)
 		assert(in_fixture);
 		__igt_fixture_end();
 	} else {
-		exit(IGT_EXIT_SKIP);
+		igt_exitcode = IGT_EXIT_SKIP;
+		igt_exit();
 	}
 }
 
@@ -786,7 +787,7 @@ void igt_fail(int exitcode)
 			__igt_fixture_end();
 		}
 
-		exit(exitcode);
+		igt_exit();
 	}
 }
 
@@ -857,7 +858,7 @@ void igt_exit(void)
 		exit(IGT_EXIT_SUCCESS);
 
 	if (!test_with_subtests)
-		exit(IGT_EXIT_SUCCESS);
+		exit(igt_exitcode);
 
 	/* Calling this without calling one of the above is a failure */
 	assert(skipped_one || succeeded_one || failed_one);
diff --git a/lib/igt_core.h b/lib/igt_core.h
index d74cbf9..974a2fb 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -188,7 +188,7 @@ void igt_simple_init_parse_opts(int argc, char **argv,
 	int main(int argc, char **argv) { \
 		igt_simple_init(argc, argv); \
 		igt_tokencat(__real_main, __LINE__)(); \
-		exit(0); \
+		igt_exit(); \
 	} \
 	static void igt_tokencat(__real_main, __LINE__)(void) \
 
diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c
index 13b2da9..e588959 100644
--- a/tests/igt_simulation.c
+++ b/tests/igt_simulation.c
@@ -65,7 +65,7 @@ static int do_fork(void)
 
 			igt_skip_on_simulation();
 
-			exit(0);
+			igt_exit();
 		} else {
 			if (list_subtests)
 				igt_subtest_init(2, argv_list);
-- 
2.1.1

Re: [PATCH v2 1/2] lib/igt_core: make single/simple tests use igt_exit

[Daniel Vetter]

On Mon, Sep 29, 2014 at 01:34:30PM +0100, tim.gore@intel.com wrote:
> From: Tim Gore <tim.gore@intel.com>
> 
> Currently tests that use igt_simple_main will simply call
> "exit()" if they pass, making it difficult to ensure that
> any required cleanup is done. At present this is not an
> issue, but it will be when I submit a patch to turn off the
> lowmemorykiller for all tests.
> 
> Signed-off-by: Tim Gore <tim.gore@intel.com>

Merged, with the api doc for igt_exit update. Please don't forget to
adjust documentation!

Thanks, Daniel
> ---
>  lib/igt_core.c         | 9 +++++----
>  lib/igt_core.h         | 2 +-
>  tests/igt_simulation.c | 2 +-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 5d41468..9344815 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -641,7 +641,7 @@ bool igt_only_list_subtests(void)
>  static bool skipped_one = false;
>  static bool succeeded_one = false;
>  static bool failed_one = false;
> -static int igt_exitcode;
> +static int igt_exitcode = IGT_EXIT_SUCCESS;
>  
>  static void exit_subtest(const char *) __attribute__((noreturn));
>  static void exit_subtest(const char *result)
> @@ -692,7 +692,8 @@ void igt_skip(const char *f, ...)
>  		assert(in_fixture);
>  		__igt_fixture_end();
>  	} else {
> -		exit(IGT_EXIT_SKIP);
> +		igt_exitcode = IGT_EXIT_SKIP;
> +		igt_exit();
>  	}
>  }
>  
> @@ -786,7 +787,7 @@ void igt_fail(int exitcode)
>  			__igt_fixture_end();
>  		}
>  
> -		exit(exitcode);
> +		igt_exit();
>  	}
>  }
>  
> @@ -857,7 +858,7 @@ void igt_exit(void)
>  		exit(IGT_EXIT_SUCCESS);
>  
>  	if (!test_with_subtests)
> -		exit(IGT_EXIT_SUCCESS);
> +		exit(igt_exitcode);
>  
>  	/* Calling this without calling one of the above is a failure */
>  	assert(skipped_one || succeeded_one || failed_one);
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index d74cbf9..974a2fb 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -188,7 +188,7 @@ void igt_simple_init_parse_opts(int argc, char **argv,
>  	int main(int argc, char **argv) { \
>  		igt_simple_init(argc, argv); \
>  		igt_tokencat(__real_main, __LINE__)(); \
> -		exit(0); \
> +		igt_exit(); \
>  	} \
>  	static void igt_tokencat(__real_main, __LINE__)(void) \
>  
> diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c
> index 13b2da9..e588959 100644
> --- a/tests/igt_simulation.c
> +++ b/tests/igt_simulation.c
> @@ -65,7 +65,7 @@ static int do_fork(void)
>  
>  			igt_skip_on_simulation();
>  
> -			exit(0);
> +			igt_exit();
>  		} else {
>  			if (list_subtests)
>  				igt_subtest_init(2, argv_list);
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 1/2] lib/igt_core: make single/simple tests use igt_exit

[Gore, Tim]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, September 29, 2014 3:58 PM
> To: Gore, Tim
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 1/2] lib/igt_core: make single/simple tests
> use igt_exit
> 
> On Mon, Sep 29, 2014 at 01:34:30PM +0100, tim.gore@intel.com wrote:
> > From: Tim Gore <tim.gore@intel.com>
> >
> > Currently tests that use igt_simple_main will simply call "exit()" if
> > they pass, making it difficult to ensure that any required cleanup is
> > done. At present this is not an issue, but it will be when I submit a
> > patch to turn off the lowmemorykiller for all tests.
> >
> > Signed-off-by: Tim Gore <tim.gore@intel.com>
> 
> Merged, with the api doc for igt_exit update. Please don't forget to adjust
> documentation!
> 
> Thanks, Daniel

Fair comment about keeping docs up to date  but bit confused by the last sentence.
You mention some some docs in a previous mail, in your personal area on freedesktop,
are those the ones you mean. Do I have write access to those ! There's a docs directory
in igt, but this just has an xml file that refers to others that I cannot find.
 All hints gratefully received.
  Tim

> > ---
> >  lib/igt_core.c         | 9 +++++----
> >  lib/igt_core.h         | 2 +-
> >  tests/igt_simulation.c | 2 +-
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c index 5d41468..9344815
> > 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -641,7 +641,7 @@ bool igt_only_list_subtests(void)  static bool
> > skipped_one = false;  static bool succeeded_one = false;  static bool
> > failed_one = false; -static int igt_exitcode;
> > +static int igt_exitcode = IGT_EXIT_SUCCESS;
> >
> >  static void exit_subtest(const char *) __attribute__((noreturn));
> > static void exit_subtest(const char *result) @@ -692,7 +692,8 @@ void
> > igt_skip(const char *f, ...)
> >  		assert(in_fixture);
> >  		__igt_fixture_end();
> >  	} else {
> > -		exit(IGT_EXIT_SKIP);
> > +		igt_exitcode = IGT_EXIT_SKIP;
> > +		igt_exit();
> >  	}
> >  }
> >
> > @@ -786,7 +787,7 @@ void igt_fail(int exitcode)
> >  			__igt_fixture_end();
> >  		}
> >
> > -		exit(exitcode);
> > +		igt_exit();
> >  	}
> >  }
> >
> > @@ -857,7 +858,7 @@ void igt_exit(void)
> >  		exit(IGT_EXIT_SUCCESS);
> >
> >  	if (!test_with_subtests)
> > -		exit(IGT_EXIT_SUCCESS);
> > +		exit(igt_exitcode);
> >
> >  	/* Calling this without calling one of the above is a failure */
> >  	assert(skipped_one || succeeded_one || failed_one); diff --git
> > a/lib/igt_core.h b/lib/igt_core.h index d74cbf9..974a2fb 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -188,7 +188,7 @@ void igt_simple_init_parse_opts(int argc, char
> **argv,
> >  	int main(int argc, char **argv) { \
> >  		igt_simple_init(argc, argv); \
> >  		igt_tokencat(__real_main, __LINE__)(); \
> > -		exit(0); \
> > +		igt_exit(); \
> >  	} \
> >  	static void igt_tokencat(__real_main, __LINE__)(void) \
> >
> > diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c index
> > 13b2da9..e588959 100644
> > --- a/tests/igt_simulation.c
> > +++ b/tests/igt_simulation.c
> > @@ -65,7 +65,7 @@ static int do_fork(void)
> >
> >  			igt_skip_on_simulation();
> >
> > -			exit(0);
> > +			igt_exit();
> >  		} else {
> >  			if (list_subtests)
> >  				igt_subtest_init(2, argv_list);
> > --
> > 2.1.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 1/2] lib/igt_core: make single/simple tests use igt_exit

[Daniel Vetter]

On Mon, Sep 29, 2014 at 03:18:10PM +0000, Gore, Tim wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, September 29, 2014 3:58 PM
> > To: Gore, Tim
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v2 1/2] lib/igt_core: make single/simple tests
> > use igt_exit
> > 
> > On Mon, Sep 29, 2014 at 01:34:30PM +0100, tim.gore@intel.com wrote:
> > > From: Tim Gore <tim.gore@intel.com>
> > >
> > > Currently tests that use igt_simple_main will simply call "exit()" if
> > > they pass, making it difficult to ensure that any required cleanup is
> > > done. At present this is not an issue, but it will be when I submit a
> > > patch to turn off the lowmemorykiller for all tests.
> > >
> > > Signed-off-by: Tim Gore <tim.gore@intel.com>
> > 
> > Merged, with the api doc for igt_exit update. Please don't forget to adjust
> > documentation!
> > 
> > Thanks, Daniel
> 
> Fair comment about keeping docs up to date  but bit confused by the last sentence.
> You mention some some docs in a previous mail, in your personal area on freedesktop,
> are those the ones you mean. Do I have write access to those ! There's a docs directory
> in igt, but this just has an xml file that refers to others that I cannot find.
>  All hints gratefully received.

$ ./autogen.sh --enable-gtk-doc
$ make

... and you have them, too.

I just upload them for convience somewhere until someone gets less lazy
than me and makes an autobuilder+proper location for this.
-Daniel
>   Tim
> 
> > > ---
> > >  lib/igt_core.c         | 9 +++++----
> > >  lib/igt_core.h         | 2 +-
> > >  tests/igt_simulation.c | 2 +-
> > >  3 files changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c index 5d41468..9344815
> > > 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -641,7 +641,7 @@ bool igt_only_list_subtests(void)  static bool
> > > skipped_one = false;  static bool succeeded_one = false;  static bool
> > > failed_one = false; -static int igt_exitcode;
> > > +static int igt_exitcode = IGT_EXIT_SUCCESS;
> > >
> > >  static void exit_subtest(const char *) __attribute__((noreturn));
> > > static void exit_subtest(const char *result) @@ -692,7 +692,8 @@ void
> > > igt_skip(const char *f, ...)
> > >  		assert(in_fixture);
> > >  		__igt_fixture_end();
> > >  	} else {
> > > -		exit(IGT_EXIT_SKIP);
> > > +		igt_exitcode = IGT_EXIT_SKIP;
> > > +		igt_exit();
> > >  	}
> > >  }
> > >
> > > @@ -786,7 +787,7 @@ void igt_fail(int exitcode)
> > >  			__igt_fixture_end();
> > >  		}
> > >
> > > -		exit(exitcode);
> > > +		igt_exit();
> > >  	}
> > >  }
> > >
> > > @@ -857,7 +858,7 @@ void igt_exit(void)
> > >  		exit(IGT_EXIT_SUCCESS);
> > >
> > >  	if (!test_with_subtests)
> > > -		exit(IGT_EXIT_SUCCESS);
> > > +		exit(igt_exitcode);
> > >
> > >  	/* Calling this without calling one of the above is a failure */
> > >  	assert(skipped_one || succeeded_one || failed_one); diff --git
> > > a/lib/igt_core.h b/lib/igt_core.h index d74cbf9..974a2fb 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -188,7 +188,7 @@ void igt_simple_init_parse_opts(int argc, char
> > **argv,
> > >  	int main(int argc, char **argv) { \
> > >  		igt_simple_init(argc, argv); \
> > >  		igt_tokencat(__real_main, __LINE__)(); \
> > > -		exit(0); \
> > > +		igt_exit(); \
> > >  	} \
> > >  	static void igt_tokencat(__real_main, __LINE__)(void) \
> > >
> > > diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c index
> > > 13b2da9..e588959 100644
> > > --- a/tests/igt_simulation.c
> > > +++ b/tests/igt_simulation.c
> > > @@ -65,7 +65,7 @@ static int do_fork(void)
> > >
> > >  			igt_skip_on_simulation();
> > >
> > > -			exit(0);
> > > +			igt_exit();
> > >  		} else {
> > >  			if (list_subtests)
> > >  				igt_subtest_init(2, argv_list);
> > > --
> > > 2.1.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 2/2] lib/igt_core.c: add function to disable lowmemorykiller

[tim.gore]

From: Tim Gore <tim.gore@intel.com>

Several IGT tests put a lot of pressure on memory and
when running these tests on Android they tend to get
killed by the lowmemorykiller. The lowmemorykiller really
is not usefull in this context and is just preventing the
test from doing its job. This commit adds a function to
disable the lowmemorykiller by writing "9999" to its
oom adj parameter, which means it will never "select"
any process to kill. The normal linux oom killer is still
there to protect the kernel.
This function is not called by default; it is up to each
individual test to call this function if it might trigger
the lowmemorykiller.
The igt_exit() now includes a call to this function to
re-enable the lowmemorykiller if it has been disabled.
A test may also re-enable the lowmemorykiller once it is
finished since this is benign if called more than once.

Signed-off-by: Tim Gore <tim.gore@intel.com>
---
 lib/igt_core.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 9344815..172a204 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -326,6 +326,72 @@ static void print_usage(const char *help_str, bool output_on_stderr)
 		fprintf(f, "%s\n", help_str);
 }
 
+
+/* Some of the IGT tests put quite a lot of pressure on memory and when
+ * running on Android they are sometimes killed by the Android low memory killer.
+ * The low memory killer really isn't usefull in this context and has no
+ * interaction with the gpu driver that we are testing, so the following
+ * function is used to disable it by modifying one of its module parameters.
+ * We still have the normal linux oom killer to protect the kernel.
+ * Apparently it is also possible for the lowmemorykiller to get included
+ * in some linux distributions; so rather than check for Android we directly
+ * check for the existence of the module parameter we want to adjust.
+ */
+void low_mem_killer_disable(bool disable)
+{
+	static const char* adj_fname="/sys/module/lowmemorykiller/parameters/adj";
+	static const char no_lowmem_killer[] = "9999";
+	int fd;
+	struct stat buf;
+	/* The following must persist across invocations */
+	static char prev_adj_scores[256];
+	static int adj_scores_len = 0;
+	static bool is_disabled = false;
+
+	/* check to see if there is something to do */
+	if (!(disable ^ is_disabled))
+		return;
+
+	/* capture the permissions bits for the lowmemkiller adj pseudo-file.
+	   Bail out if the stat fails; it probably means that there is no
+	   lowmemorykiller, but in any case we're doomed. */
+	if (stat (adj_fname, &buf))
+	{
+		igt_assert(errno == ENOENT);
+		return;
+	}
+	/* make sure the file can be read/written - by default it is write-only */
+	chmod (adj_fname, S_IRUSR | S_IWUSR);
+
+	if (disable)
+	{
+		/* read the current oom adj parameters for lowmemorykiller */
+		fd = open(adj_fname, O_RDWR);
+		igt_assert(fd != -1);
+		adj_scores_len = read(fd, (void*)prev_adj_scores, 255);
+		igt_assert(adj_scores_len > 0);
+
+		/* writing 9999 to this module parameter effectively diables the
+		 * low memory killer. This is not a real file, so we dont need to
+		 * seek to the start or truncate it */
+		write(fd, no_lowmem_killer, sizeof(no_lowmem_killer));
+		close(fd);
+		is_disabled = true;
+	}
+	else
+	{
+		/* just re-enstate the original settings */
+		fd = open(adj_fname, O_WRONLY);
+		igt_assert(fd != -1);
+		write(fd, prev_adj_scores, adj_scores_len);
+		close(fd);
+		is_disabled = false;
+	}
+
+	/* re-enstate the file permissions */
+	chmod (adj_fname, buf.st_mode);
+}
+
 static void oom_adjust_for_doom(void)
 {
 	int fd;
@@ -334,6 +400,7 @@ static void oom_adjust_for_doom(void)
 	fd = open("/proc/self/oom_score_adj", O_WRONLY);
 	igt_assert(fd != -1);
 	igt_assert(write(fd, always_kill, sizeof(always_kill)) == sizeof(always_kill));
+	close(fd);
 }
 
 static int common_init(int argc, char **argv,
@@ -848,6 +915,9 @@ void igt_exit(void)
 {
 	igt_exit_called = true;
 
+	/* re-enstate the low mem killer in case we disabled it */
+	low_mem_killer_disable(false);
+
 	if (run_single_subtest && !run_single_subtest_found) {
 		igt_warn("Unknown subtest: %s\n", run_single_subtest);
 		exit(IGT_EXIT_INVALID);
-- 
2.1.1

Re: [PATCH v2 0/2] Disable Android low memory killer

[Daniel Vetter]

On Mon, Sep 29, 2014 at 01:34:29PM +0100, tim.gore@intel.com wrote:
> From: Tim Gore <tim.gore@intel.com>
> 
> For some tests that put pressure on memory, the Android
> lowmemorykiller needs to be disabled for the test to run to
> completion. The first patch is a simple bit of preparation
> to ensure that all (well written) "simple" tests exit via a
> call to igt_exit, in the same way as tests with subtests do.
> This is to make sure we can clean up by re-enabling the
> lowmemorykiller.
> The second patch is to disable the Android lowmemorykiller
> during the common initialisation code (in oom_adjust_for_doom
> to be exact) and to re-enstate it in igt_exit.
> 
> v1: As above
> 
> v2: Remove the call to disable the lowmemorykiller from
>     oom_adjust_for_doom. lowmemorykiller is not disabled
>     by default now; it is up to each individual test to
>     call low_mem_killer_disable() if it needs to.

See my late replies (I was off for an extended w/e). Summary:
- I think we should just do this unconditionally since it's a hack and
  pointless to burden tests with it.
- proper exit handler and you can gc patch 1.

Cheers, Daniel
> 
> Tim Gore (2):
>   lib/igt_core: make single/simple tests use igt_exit
>   lib/igt_core.c: add function to disable lowmemorykiller
> 
>  lib/igt_core.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++---
>  lib/igt_core.h         |  2 +-
>  tests/igt_simulation.c |  2 +-
>  3 files changed, 77 insertions(+), 6 deletions(-)
> 
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 0/2] Disable Android low memory killer

[Gore, Tim]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, September 29, 2014 2:35 PM
> To: Gore, Tim
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 0/2] Disable Android low memory killer
> 
> On Mon, Sep 29, 2014 at 01:34:29PM +0100, tim.gore@intel.com wrote:
> > From: Tim Gore <tim.gore@intel.com>
> >
> > For some tests that put pressure on memory, the Android
> > lowmemorykiller needs to be disabled for the test to run to
> > completion. The first patch is a simple bit of preparation to ensure
> > that all (well written) "simple" tests exit via a call to igt_exit, in
> > the same way as tests with subtests do.
> > This is to make sure we can clean up by re-enabling the
> > lowmemorykiller.
> > The second patch is to disable the Android lowmemorykiller during the
> > common initialisation code (in oom_adjust_for_doom to be exact) and to
> > re-enstate it in igt_exit.
> >
> > v1: As above
> >
> > v2: Remove the call to disable the lowmemorykiller from
> >     oom_adjust_for_doom. lowmemorykiller is not disabled
> >     by default now; it is up to each individual test to
> >     call low_mem_killer_disable() if it needs to.
> 
> See my late replies (I was off for an extended w/e). Summary:
> - I think we should just do this unconditionally since it's a hack and
>   pointless to burden tests with it.
> - proper exit handler and you can gc patch 1.
> 
> Cheers, Daniel

OK. Igt already has an exit handler, although it just checks that igt_exit has been called,
and does not get installed for simple tests. Would you be OK with me extending this
exit handler to re-instate the low memory killer, or do want to keep the possibility of
simple tests calling exit() rather than igt_exit(). ?

I agree that the lowmemorykiller seems to be the problem, but don't feel confident
to go changing it yet.

  Tim
> >
> > Tim Gore (2):
> >   lib/igt_core: make single/simple tests use igt_exit
> >   lib/igt_core.c: add function to disable lowmemorykiller
> >
> >  lib/igt_core.c         | 79
> +++++++++++++++++++++++++++++++++++++++++++++++---
> >  lib/igt_core.h         |  2 +-
> >  tests/igt_simulation.c |  2 +-
> >  3 files changed, 77 insertions(+), 6 deletions(-)
> >
> > --
> > 2.1.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 0/2] Disable Android low memory killer

[Daniel Vetter]

On Mon, Sep 29, 2014 at 01:47:49PM +0000, Gore, Tim wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, September 29, 2014 2:35 PM
> > To: Gore, Tim
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v2 0/2] Disable Android low memory killer
> > 
> > On Mon, Sep 29, 2014 at 01:34:29PM +0100, tim.gore@intel.com wrote:
> > > From: Tim Gore <tim.gore@intel.com>
> > >
> > > For some tests that put pressure on memory, the Android
> > > lowmemorykiller needs to be disabled for the test to run to
> > > completion. The first patch is a simple bit of preparation to ensure
> > > that all (well written) "simple" tests exit via a call to igt_exit, in
> > > the same way as tests with subtests do.
> > > This is to make sure we can clean up by re-enabling the
> > > lowmemorykiller.
> > > The second patch is to disable the Android lowmemorykiller during the
> > > common initialisation code (in oom_adjust_for_doom to be exact) and to
> > > re-enstate it in igt_exit.
> > >
> > > v1: As above
> > >
> > > v2: Remove the call to disable the lowmemorykiller from
> > >     oom_adjust_for_doom. lowmemorykiller is not disabled
> > >     by default now; it is up to each individual test to
> > >     call low_mem_killer_disable() if it needs to.
> > 
> > See my late replies (I was off for an extended w/e). Summary:
> > - I think we should just do this unconditionally since it's a hack and
> >   pointless to burden tests with it.
> > - proper exit handler and you can gc patch 1.
> > 
> > Cheers, Daniel
> 
> OK. Igt already has an exit handler, although it just checks that igt_exit has been called,
> and does not get installed for simple tests. Would you be OK with me extending this
> exit handler to re-instate the low memory killer, or do want to keep the possibility of
> simple tests calling exit() rather than igt_exit(). ?

Oops, missed that the exit handler stuff doesn't work for simple tests.
Yeah, in that case rolling out igt_exit for simple tests makes sense
indeed.
-Daniel

> 
> I agree that the lowmemorykiller seems to be the problem, but don't feel confident
> to go changing it yet.
> 
>   Tim
> > >
> > > Tim Gore (2):
> > >   lib/igt_core: make single/simple tests use igt_exit
> > >   lib/igt_core.c: add function to disable lowmemorykiller
> > >
> > >  lib/igt_core.c         | 79
> > +++++++++++++++++++++++++++++++++++++++++++++++---
> > >  lib/igt_core.h         |  2 +-
> > >  tests/igt_simulation.c |  2 +-
> > >  3 files changed, 77 insertions(+), 6 deletions(-)
> > >
> > > --
> > > 2.1.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 0/2] Disable Android low memory killer

[Gore, Tim]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, September 29, 2014 3:55 PM
> To: Gore, Tim
> Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 0/2] Disable Android low memory killer
> 
> On Mon, Sep 29, 2014 at 01:47:49PM +0000, Gore, Tim wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Monday, September 29, 2014 2:35 PM
> > > To: Gore, Tim
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH v2 0/2] Disable Android low memory
> > > killer
> > >
> > > On Mon, Sep 29, 2014 at 01:34:29PM +0100, tim.gore@intel.com wrote:
> > > > From: Tim Gore <tim.gore@intel.com>
> > > >
> > > > For some tests that put pressure on memory, the Android
> > > > lowmemorykiller needs to be disabled for the test to run to
> > > > completion. The first patch is a simple bit of preparation to
> > > > ensure that all (well written) "simple" tests exit via a call to
> > > > igt_exit, in the same way as tests with subtests do.
> > > > This is to make sure we can clean up by re-enabling the
> > > > lowmemorykiller.
> > > > The second patch is to disable the Android lowmemorykiller during
> > > > the common initialisation code (in oom_adjust_for_doom to be
> > > > exact) and to re-enstate it in igt_exit.
> > > >
> > > > v1: As above
> > > >
> > > > v2: Remove the call to disable the lowmemorykiller from
> > > >     oom_adjust_for_doom. lowmemorykiller is not disabled
> > > >     by default now; it is up to each individual test to
> > > >     call low_mem_killer_disable() if it needs to.
> > >
> > > See my late replies (I was off for an extended w/e). Summary:
> > > - I think we should just do this unconditionally since it's a hack and
> > >   pointless to burden tests with it.
> > > - proper exit handler and you can gc patch 1.
> > >
> > > Cheers, Daniel
> >
> >So should I re-submit v1 of my second patch, to disable the
      lowmemorykiller by default?

         Tim 

Re: [PATCH v2 0/2] Disable Android low memory killer

[Daniel Vetter]

On Tue, Sep 30, 2014 at 01:17:55PM +0000, Gore, Tim wrote:
> So should I re-submit v1 of my second patch, to disable the
> lowmemorykiller by default?

Yeah, but using the exit handler support from igt and adding a big comment
that we just hack around issues with that and that in theory i915
shrinkers and the lowmemory killer should properly work together. I think
that's the sanest approach to this mess.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch