* [IGT PATCH 2/2] aux: Use IGT version of system() call to run rtcwake
2017-10-12 13:22 [IGT PATCH 1/2] aux: Suspend signal helper for shell commands Imre Deak
@ 2017-10-12 13:22 ` Imre Deak
2017-10-12 13:57 ` Chris Wilson
2017-10-12 13:51 ` [IGT PATCH 1/2] aux: Suspend signal helper for shell commands Chris Wilson
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Imre Deak @ 2017-10-12 13:22 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
The clone() system call has difficulty to make progress if interrupted
frequently by the signal helper process. At least on an APL, like in the
Bugzilla ticket below, this can introduce minutes of overhead to a
single system() call (leading to a global CI timeout). To get rid of the
overhead suspend the signal helper process for the duration of the
system() call, which is provided already by igt_system().
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103160
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
lib/igt_aux.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 36dfab43..a1c5e2cf 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -794,7 +794,7 @@ static void suspend_via_rtcwake(enum igt_suspend_state state)
*/
snprintf(cmd, sizeof(cmd), "rtcwake -n -s %d -m %s " SQUELCH,
delay, suspend_state_name[state]);
- ret = system(cmd);
+ ret = igt_system(cmd);
igt_require_f(ret == 0, "rtcwake test failed with %i\n"
"This failure could mean that something is wrong with "
"the rtcwake tool or how your distro is set up.\n",
@@ -802,7 +802,7 @@ static void suspend_via_rtcwake(enum igt_suspend_state state)
snprintf(cmd, sizeof(cmd), "rtcwake -s %d -m %s ",
delay, suspend_state_name[state]);
- ret = system(cmd);
+ ret = igt_system(cmd);
igt_assert_f(ret == 0,
"rtcwake failed with %i\n"
"Check dmesg for further details.\n",
--
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [IGT PATCH 2/2] aux: Use IGT version of system() call to run rtcwake
2017-10-12 13:22 ` [IGT PATCH 2/2] aux: Use IGT version of system() call to run rtcwake Imre Deak
@ 2017-10-12 13:57 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-10-12 13:57 UTC (permalink / raw)
To: Imre Deak, intel-gfx; +Cc: Daniel Vetter
Quoting Imre Deak (2017-10-12 14:22:07)
> The clone() system call has difficulty to make progress if interrupted
> frequently by the signal helper process. At least on an APL, like in the
> Bugzilla ticket below, this can introduce minutes of overhead to a
> single system() call (leading to a global CI timeout). To get rid of the
> overhead suspend the signal helper process for the duration of the
> system() call, which is provided already by igt_system().
The alternative is to pull small binaries into ourselves, e.g.
converting system("modprobe") to libkmod calls fixed a bunch of similar
issues (under kasan they were slow even without interruptions!).
We could absorb rtcwake in a couple of hundred lines, if the system() is
slow enough to be interrupted, it's slow. ;)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103160
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [IGT PATCH 1/2] aux: Suspend signal helper for shell commands
2017-10-12 13:22 [IGT PATCH 1/2] aux: Suspend signal helper for shell commands Imre Deak
2017-10-12 13:22 ` [IGT PATCH 2/2] aux: Use IGT version of system() call to run rtcwake Imre Deak
@ 2017-10-12 13:51 ` Chris Wilson
2017-10-12 13:52 ` Chris Wilson
2017-10-12 17:52 ` Daniel Vetter
3 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-10-12 13:51 UTC (permalink / raw)
To: Imre Deak, intel-gfx; +Cc: Daniel Vetter
Quoting Imre Deak (2017-10-12 14:22:06)
> The clone() system call with a larger executable (like /bin/sh) may have
> difficulty to make progress on some platforms if interrupted frequently.
> So suspend the signal helper process for the duration of the syscall.
> This is needed to solve an actual problem by the next patch.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> lib/igt_aux.c | 38 ++++++++++++++++++++++++++++++++++++++
> lib/igt_aux.h | 2 ++
> lib/igt_core.c | 9 +++++++++
> 3 files changed, 49 insertions(+)
>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index fa6594c3..36dfab43 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -350,6 +350,44 @@ void igt_stop_signal_helper(void)
> sig_stat = 0;
> }
>
> +/**
> + * igt_suspend_signal_helper:
> + *
> + * Suspends the child process spawned with igt_fork_signal_helper().
> + *
> + * This should be called before code that has difficulty to make progress if
> + * interrupted frequently, like the clone() syscall spawning a large
> + * executable.
* igt_resume_signal_helper() must be called after the critical section
* to restart interruptions for the test.
> + */
> +void igt_suspend_signal_helper(void)
> +{
> + int status;
> +
> + if (!signal_helper.running)
> + return;
> +
> + kill(signal_helper.pid, SIGSTOP);
> + while (waitpid(signal_helper.pid, &status, WUNTRACED) == -1 &&
> + errno == EINTR)
> + ;
> +}
> +
> +/**
> + * igt_suspend_signal_helper:
igt_resume_signal_helper
> + *
> + * Resumes the child process spawned with igt_fork_signal_helper().
> + *
> + * This should be paired with igt_suspend_signal_helper() and called after the
> + * problematic code sensitive to signals.
> + */
> +void igt_resume_signal_helper(void)
> +{
> + if (!signal_helper.running)
> + return;
> +
> + kill(signal_helper.pid, SIGCONT);
> +}
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [IGT PATCH 1/2] aux: Suspend signal helper for shell commands
2017-10-12 13:22 [IGT PATCH 1/2] aux: Suspend signal helper for shell commands Imre Deak
2017-10-12 13:22 ` [IGT PATCH 2/2] aux: Use IGT version of system() call to run rtcwake Imre Deak
2017-10-12 13:51 ` [IGT PATCH 1/2] aux: Suspend signal helper for shell commands Chris Wilson
@ 2017-10-12 13:52 ` Chris Wilson
2017-10-13 11:22 ` Imre Deak
2017-10-12 17:52 ` Daniel Vetter
3 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2017-10-12 13:52 UTC (permalink / raw)
To: Imre Deak, intel-gfx; +Cc: Daniel Vetter
Quoting Imre Deak (2017-10-12 14:22:06)
> The clone() system call with a larger executable (like /bin/sh) may have
One thing to note it's not the target executable size that's
problematic, it's ours. It is our mm that is being forked.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [IGT PATCH 1/2] aux: Suspend signal helper for shell commands
2017-10-12 13:52 ` Chris Wilson
@ 2017-10-13 11:22 ` Imre Deak
0 siblings, 0 replies; 7+ messages in thread
From: Imre Deak @ 2017-10-13 11:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx
On Thu, Oct 12, 2017 at 02:52:40PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2017-10-12 14:22:06)
> > The clone() system call with a larger executable (like /bin/sh) may have
>
> One thing to note it's not the target executable size that's
> problematic, it's ours. It is our mm that is being forked.
Hm, right didn't think this through. And based on that I tried now
vfork(), but that doesn't work as-is due to how we do our output/error
piping.
I also forgot igt_system_quiet(), will change that too.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [IGT PATCH 1/2] aux: Suspend signal helper for shell commands
2017-10-12 13:22 [IGT PATCH 1/2] aux: Suspend signal helper for shell commands Imre Deak
` (2 preceding siblings ...)
2017-10-12 13:52 ` Chris Wilson
@ 2017-10-12 17:52 ` Daniel Vetter
3 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2017-10-12 17:52 UTC (permalink / raw)
To: Imre Deak; +Cc: Daniel Vetter, intel-gfx
On Thu, Oct 12, 2017 at 04:22:06PM +0300, Imre Deak wrote:
> The clone() system call with a larger executable (like /bin/sh) may have
> difficulty to make progress on some platforms if interrupted frequently.
> So suspend the signal helper process for the duration of the syscall.
> This is needed to solve an actual problem by the next patch.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Your patch prefix is wrong, which means CI wont' pick this up. Please
resubmit with i-g-t as patch thing per our CONTRIBUTING file.
Thanks, Daniel
PS: Yes we'll soon have a separate mailing list for igt to avoid this kind
of fun.
-Daniel
> ---
> lib/igt_aux.c | 38 ++++++++++++++++++++++++++++++++++++++
> lib/igt_aux.h | 2 ++
> lib/igt_core.c | 9 +++++++++
> 3 files changed, 49 insertions(+)
>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index fa6594c3..36dfab43 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -350,6 +350,44 @@ void igt_stop_signal_helper(void)
> sig_stat = 0;
> }
>
> +/**
> + * igt_suspend_signal_helper:
> + *
> + * Suspends the child process spawned with igt_fork_signal_helper().
> + *
> + * This should be called before code that has difficulty to make progress if
> + * interrupted frequently, like the clone() syscall spawning a large
> + * executable.
> + */
> +void igt_suspend_signal_helper(void)
> +{
> + int status;
> +
> + if (!signal_helper.running)
> + return;
> +
> + kill(signal_helper.pid, SIGSTOP);
> + while (waitpid(signal_helper.pid, &status, WUNTRACED) == -1 &&
> + errno == EINTR)
> + ;
> +}
> +
> +/**
> + * igt_suspend_signal_helper:
> + *
> + * Resumes the child process spawned with igt_fork_signal_helper().
> + *
> + * This should be paired with igt_suspend_signal_helper() and called after the
> + * problematic code sensitive to signals.
> + */
> +void igt_resume_signal_helper(void)
> +{
> + if (!signal_helper.running)
> + return;
> +
> + kill(signal_helper.pid, SIGCONT);
> +}
> +
> static struct igt_helper_process shrink_helper;
> static void __attribute__((noreturn)) shrink_helper_process(int fd, pid_t pid)
> {
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index 499a1679..688ad1b8 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -55,6 +55,8 @@ extern int num_trash_bos;
> /* generally useful helpers */
> void igt_fork_signal_helper(void);
> void igt_stop_signal_helper(void);
> +void igt_suspend_signal_helper(void);
> +void igt_resume_signal_helper(void);
>
> void igt_fork_shrink_helper(int fd);
> void igt_stop_shrink_helper(void);
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 950ea9b0..ea4b5fa6 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -2282,6 +2282,13 @@ int igt_system(const char *command)
> if (pipe(errpipe) < 0)
> goto err;
>
> + /*
> + * The clone() system call forking a large executable has difficulty
> + * to make progress if interrupted too frequently, so suspend it for
> + * the time of the syscall.
> + */
> + igt_suspend_signal_helper();
> +
> igt_fork_helper(&process) {
> close(outpipe[0]);
> close(errpipe[0]);
> @@ -2298,6 +2305,8 @@ int igt_system(const char *command)
> exit(EXIT_FAILURE);
> }
>
> + igt_resume_signal_helper();
> +
> close(outpipe[1]);
> close(errpipe[1]);
>
> --
> 2.13.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread