cpuacct_task: exit cleanly on SIGVTALRM

PATCH New Stanislav Kholmanskikh 2 participants 4 messages Aug 31, 2016 – Sep 2, 2016

Share & tools

Stanislav Kholmanskikh

author

2016-08-31 10:21 UTC #0 lore raw

PATCH 1/1 cpuacct_task: exit cleanly on SIGVTALRM

We don't handle the SIGVTALRM signal in cpuacct_task, so the process gets terminated, and it makes cpuacct.sh generate a "Virtual timer expired" message for each cpuacct_task process.

In my opinion, these messages have no real value, and there is no need in keeping them in the output.

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 .../kernel/controllers/cpuacct/cpuacct_task.c      |   23 +++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

testcases/kernel/controllers/cpuacct/cpuacct_task.c +22 -1

index 0ca01d1..94aa4e6 100644
@@ -25,11 +25,21 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <unistd.h>
+#include <signal.h>
 #include <stdlib.h>
+#include <string.h>
+
+static volatile int got_signal;
+
+static void sig_handler(int signo)
+{
+	got_signal = 1;
+}
 
 int main(int argc, char **argv)
 {
 	FILE *f;
+	struct sigaction sa;
 
 	if (argc != 2) {
 		fprintf(stderr, "Usage: %s /cgroup/.../tasks\n", argv[0]);

@@ -44,9 +54,20 @@ int main(int argc, char **argv)
 
 	fprintf(f, "%i\n", getpid());
 	fclose(f);
+
+	memset(&sa, 0, sizeof(sa));
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = sig_handler;
+
+	if (sigaction(SIGVTALRM, &sa, NULL)) {
+		perror("sigaction failed");
+		return 1;
+	}
+
 	struct itimerval it = {.it_value = {.tv_sec = 0, .tv_usec = 10000}};
 
 	setitimer(ITIMER_VIRTUAL, &it, NULL);
-	for (;;);
+	while (!got_signal)
+		;
 	return 0;
 }

Signature · 1 line

1.7.1

Cyril Hrubis

2016-08-31 13:34 UTC #1 lore raw

PATCH 1/1 cpuacct_task: exit cleanly on SIGVTALRM

Hi!

Quoted 6 lines

We don't handle the SIGVTALRM signal in cpuacct_task, so the process gets terminated, and it makes cpuacct.sh generate a "Virtual timer expired" message for each cpuacct_task process.
In my opinion, these messages have no real value, and there is no need in keeping them in the output.

I was just too lazy to install the signal handler in this case given that the default action for the signal is to terminate the process...

Quoted 13 lines

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 .../kernel/controllers/cpuacct/cpuacct_task.c      |   23 +++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

testcases/kernel/controllers/cpuacct/cpuacct_task.c +0 -0

index 0ca01d1..94aa4e6 100644
@@ -25,11 +25,21 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <unistd.h>

testcases/kernel/controllers/cpuacct/cpuacct_task.c line 37

Quoted 10 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c +9 -0

+#include <signal.h>
 #include <stdlib.h>
+#include <string.h>
+
+static volatile int got_signal;
+
+static void sig_handler(int signo)
+{
+	got_signal = 1;
+}

We can as well do _exit(0); here. Then we don't have to add the global variable and the main loop will stay as for (;;);

Quoted 10 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c +1 -0

 int main(int argc, char **argv)
 {
 	FILE *f;
+	struct sigaction sa;
 
 	if (argc != 2) {
 		fprintf(stderr, "Usage: %s /cgroup/.../tasks\n", argv[0]);

@@ -44,9 +54,20 @@ int main(int argc, char **argv)
 
 	fprintf(f, "%i\n", getpid());

testcases/kernel/controllers/cpuacct/cpuacct_task.c int main(int argc, char **argv) · line 65

Quoted 10 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c +9 -0

 	fclose(f);
+
+	memset(&sa, 0, sizeof(sa));
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = sig_handler;
+
+	if (sigaction(SIGVTALRM, &sa, NULL)) {
+		perror("sigaction failed");
+		return 1;
+	}

I would just use simpler signal(SIGVTALRM, sig_handler); here.

testcases/kernel/controllers/cpuacct/cpuacct_task.c int main(int argc, char **argv) · line 73

Quoted 8 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c +2 -1

 	struct itimerval it = {.it_value = {.tv_sec = 0, .tv_usec = 10000}};
 
 	setitimer(ITIMER_VIRTUAL, &it, NULL);
-	for (;;);
+	while (!got_signal)
+		;
 	return 0;
 }

Other than that it's fine.

Signature · 2 lines

Cyril Hrubis
[email protected]

Stanislav Kholmanskikh

author

2016-08-31 14:26 UTC #2 lore raw

PATCH 1/1 cpuacct_task: exit cleanly on SIGVTALRM

Hi!

Quoted 29 lines · Cyril Hrubis

Hi!
Quoted 6 lines
We don't handle the SIGVTALRM signal in cpuacct_task, so the process gets terminated, and it makes cpuacct.sh generate a "Virtual timer expired" message for each cpuacct_task process.
In my opinion, these messages have no real value, and there is no need in keeping them in the output.
I was just too lazy to install the signal handler in this case given that the default action for the signal is to terminate the process...
Quoted 18 lines
Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
 .../kernel/controllers/cpuacct/cpuacct_task.c      |   23 +++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
testcases/kernel/controllers/cpuacct/cpuacct_task.c +4 -0
index 0ca01d1..94aa4e6 100644
@@ -25,11 +25,21 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <unistd.h>
+#include <signal.h>
 #include <stdlib.h>
+#include <string.h>
+
+static volatile int got_signal;

Quoted 8 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c line 37
Quoted 5 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +5 -0
+
+static void sig_handler(int signo)
+{
+	got_signal = 1;
+}
We can as well do _exit(0); here. Then we don't have to add the global variable and the main loop will stay as for (;;);

Quoted 10 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c +1 -0

 int main(int argc, char **argv)
 {
 	FILE *f;
+	struct sigaction sa;
 
 	if (argc != 2) {
 		fprintf(stderr, "Usage: %s /cgroup/.../tasks\n", argv[0]);

@@ -44,9 +54,20 @@ int main(int argc, char **argv)

Quoted 13 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c int main(int argc, char **argv) · line 65
Quoted 11 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +9 -0
 	fprintf(f, "%i\n", getpid());
 	fclose(f);
+
+	memset(&sa, 0, sizeof(sa));
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = sig_handler;
+
+	if (sigaction(SIGVTALRM, &sa, NULL)) {
+		perror("sigaction failed");
+		return 1;
+	}
I would just use simpler signal(SIGVTALRM, sig_handler); here.

I tend to use sigaction() everywhere, since the man page for signal() gives preference to sigaction() for the general case.

Quoted 12 lines

testcases/kernel/controllers/cpuacct/cpuacct_task.c int main(int argc, char **argv) · line 73
Quoted 9 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +2 -1
 	struct itimerval it = {.it_value = {.tv_sec = 0, .tv_usec = 10000}};
 
 	setitimer(ITIMER_VIRTUAL, &it, NULL);
-	for (;;);
+	while (!got_signal)
+		;
 	return 0;
 }
Other than that it's fine.

Stanislav Kholmanskikh

author

2016-09-02 07:57 UTC #3 lore raw

PATCH 1/1 cpuacct_task: exit cleanly on SIGVTALRM

Quoted 67 lines · Stanislav Kholmanskikh

Hi!
Quoted 29 lines · Cyril Hrubis
Hi!
Quoted 6 lines
We don't handle the SIGVTALRM signal in cpuacct_task, so the process gets terminated, and it makes cpuacct.sh generate a "Virtual timer expired" message for each cpuacct_task process.
In my opinion, these messages have no real value, and there is no need in keeping them in the output.
I was just too lazy to install the signal handler in this case given that the default action for the signal is to terminate the process...
Quoted 18 lines
Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
 .../kernel/controllers/cpuacct/cpuacct_task.c      |   23 +++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
testcases/kernel/controllers/cpuacct/cpuacct_task.c +4 -0
index 0ca01d1..94aa4e6 100644
@@ -25,11 +25,21 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <unistd.h>
+#include <signal.h>
 #include <stdlib.h>
+#include <string.h>
+
+static volatile int got_signal;
Quoted 8 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c line 37
Quoted 5 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +5 -0
+
+static void sig_handler(int signo)
+{
+	got_signal = 1;
+}
We can as well do _exit(0); here. Then we don't have to add the global variable and the main loop will stay as for (;;);
Ok
Quoted 10 lines
Quoted 10 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +1 -0
 int main(int argc, char **argv)
 {
 	FILE *f;
+	struct sigaction sa;
 
 	if (argc != 2) {
 		fprintf(stderr, "Usage: %s /cgroup/.../tasks\n", argv[0]);
@@ -44,9 +54,20 @@ int main(int argc, char **argv)
 
Quoted 13 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c int main(int argc, char **argv) · line 65
Quoted 11 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +9 -0
 	fprintf(f, "%i\n", getpid());
 	fclose(f);
+
+	memset(&sa, 0, sizeof(sa));
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = sig_handler;
+
+	if (sigaction(SIGVTALRM, &sa, NULL)) {
+		perror("sigaction failed");
+		return 1;
+	}
I would just use simpler signal(SIGVTALRM, sig_handler); here.

Quoted 16 lines

I tend to use sigaction() everywhere, since the man page for signal() gives preference to sigaction() for the general case.
Quoted 12 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c int main(int argc, char **argv) · line 73
Quoted 9 lines
testcases/kernel/controllers/cpuacct/cpuacct_task.c +2 -1
 	struct itimerval it = {.it_value = {.tv_sec = 0, .tv_usec = 10000}};
 
 	setitimer(ITIMER_VIRTUAL, &it, NULL);
-	for (;;);
+	while (!got_signal)
+		;
 	return 0;
 }
Other than that it's fine.

Updated the patch to use _exit(0) and pushed it.

Thank you.