criu: dump and restore a thread name

Submitted by Andrei Vagin on Feb. 1, 2018, 7:29 p.m.

Details

Message ID 20180201192906.21635-1-avagin@openvz.org
State Accepted
Series "criu: dump and restore a thread name"
Headers show

Commit Message

Andrei Vagin Feb. 1, 2018, 7:29 p.m.
From: Andrei Vagin <avagin@virtuozzo.com>

Reported-by: 志平 林 <larry.lin@outlook.com>
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
---
 criu/cr-dump.c          |  3 +++
 criu/cr-restore.c       |  5 +++++
 criu/include/parasite.h |  1 +
 criu/include/restorer.h |  1 +
 criu/pie/parasite.c     | 18 +++++++++++++++---
 criu/pie/restorer.c     |  6 ++++++
 images/core.proto       |  1 +
 7 files changed, 32 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-dump.c b/criu/cr-dump.c
index aec726dad..fd7743d80 100644
--- a/criu/cr-dump.c
+++ b/criu/cr-dump.c
@@ -732,6 +732,9 @@  int dump_thread_core(int pid, CoreEntry *core, const struct parasite_dump_thread
 			tc->has_pdeath_sig = true;
 			tc->pdeath_sig = ti->pdeath_sig;
 		}
+		tc->comm = xstrdup(ti->comm);
+		if (tc->comm == NULL)
+			return -1;
 	}
 
 	return ret;
diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index c6c9a7daf..86c793ee9 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -3723,6 +3723,11 @@  static int sigreturn_restore(pid_t pid, struct task_restore_args *task_args, uns
 		if (construct_sigframe(sigframe, sigframe, blkset, tcore))
 			goto err;
 
+		if (thread_args[i].comm)
+			strncpy(thread_args[i].comm, tcore->thread_core->comm, TASK_COMM_LEN);
+		else
+			strncpy(thread_args[i].comm, core->tc->comm, TASK_COMM_LEN);
+
 		if (thread_args[i].pid[0] != pid)
 			core_entry__free_unpacked(tcore, NULL);
 
diff --git a/criu/include/parasite.h b/criu/include/parasite.h
index 9de0d2e9a..9e2e1498e 100644
--- a/criu/include/parasite.h
+++ b/criu/include/parasite.h
@@ -170,6 +170,7 @@  struct parasite_dump_thread {
 	tls_t				tls;
 	stack_t				sas;
 	int				pdeath_sig;
+	char				comm[TASK_COMM_LEN];
 	struct parasite_dump_creds	creds[0];
 };
 
diff --git a/criu/include/restorer.h b/criu/include/restorer.h
index 15307d9c0..60089667a 100644
--- a/criu/include/restorer.h
+++ b/criu/include/restorer.h
@@ -100,6 +100,7 @@  struct thread_restore_args {
 
 	bool				check_only;
 	struct thread_creds_args	*creds_args;
+	char				comm[TASK_COMM_LEN];
 } __aligned(64);
 
 typedef long (*thread_restore_fcall_t) (struct thread_restore_args *args);
diff --git a/criu/pie/parasite.c b/criu/pie/parasite.c
index 7a48f324e..c7d5fd7eb 100644
--- a/criu/pie/parasite.c
+++ b/criu/pie/parasite.c
@@ -172,16 +172,28 @@  static int dump_thread_common(struct parasite_dump_thread *ti)
 
 	arch_get_tls(&ti->tls);
 	ret = sys_prctl(PR_GET_TID_ADDRESS, (unsigned long) &ti->tid_addr, 0, 0, 0);
-	if (ret)
+	if (ret) {
+		pr_err("Unable to get the clear_child_tid address: %d\n", ret);
 		goto out;
+	}
 
 	ret = sys_sigaltstack(NULL, &ti->sas);
-	if (ret)
+	if (ret) {
+		pr_err("Unable to get signal stack context: %d\n", ret);
 		goto out;
+	}
 
 	ret = sys_prctl(PR_GET_PDEATHSIG, (unsigned long)&ti->pdeath_sig, 0, 0, 0);
-	if (ret)
+	if (ret) {
+		pr_err("Unable to get the parent death signal: %d\n", ret);
+		goto out;
+	}
+
+	ret = sys_prctl(PR_GET_NAME, (unsigned long) &ti->comm, 0, 0, 0);
+	if (ret) {
+		pr_err("Unable to get the thread name: %d\n", ret);
 		goto out;
+	}
 
 	ret = dump_creds(ti->creds);
 out:
diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c
index 091026103..73e868f97 100644
--- a/criu/pie/restorer.c
+++ b/criu/pie/restorer.c
@@ -549,6 +549,12 @@  long __export_restore_thread(struct thread_restore_args *args)
 	if (ret)
 		goto core_restore_end;
 
+	ret = sys_prctl(PR_SET_NAME, (unsigned long) &args->comm, 0, 0, 0);
+	if (ret) {
+		pr_err("Unable to set a thread name: %d\n", ret);
+		goto core_restore_end;
+	}
+
 	pr_info("%ld: Restored\n", sys_gettid());
 
 	restore_finish_stage(task_entries_local, CR_STATE_RESTORE);
diff --git a/images/core.proto b/images/core.proto
index 0291fae68..7eb17b466 100644
--- a/images/core.proto
+++ b/images/core.proto
@@ -87,6 +87,7 @@  message thread_core_entry {
 
 	optional signal_queue_entry	signals_p	= 9;
 	optional creds_entry		creds		= 10;
+	optional string			comm		= 11;
 }
 
 message task_rlimits_entry {

Comments

Dmitry Safonov Feb. 1, 2018, 9:02 p.m.
2018-02-01 19:29 GMT+00:00 Andrei Vagin <avagin@openvz.org>:
> From: Andrei Vagin <avagin@virtuozzo.com>
>
> Reported-by: 志平 林 <larry.lin@outlook.com>
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>

Looks good.
We could get it from procfs on pre-dump...
But I guess, it's a trade-off between 3 syscalls from criu
and 1 syscall in parasite, so I don't feel the difference.

How about your rule for 1 new feature = 1 new test? ;-)
Also some small nits below.

>         ret = sys_prctl(PR_GET_PDEATHSIG, (unsigned long)&ti->pdeath_sig, 0, 0, 0);
> -       if (ret)
> +       if (ret) {
> +               pr_err("Unable to get the parent death signal: %d\n", ret);
> +               goto out;
> +       }
> +
> +       ret = sys_prctl(PR_GET_NAME, (unsigned long) &ti->comm, 0, 0, 0);
> +       if (ret) {
> +               pr_err("Unable to get the thread name: %d\n", ret);
>                 goto out;
> +       }

We could omit getting thread's name if it's a thread leader.
(just less on a syscall per each task on the dump).
We can live without thread's name if it's the same as leader
(potentialy saving a bit of space in image, removing allocation on
dump, not bothering on restore about the name).

> @@ -549,6 +549,12 @@ long __export_restore_thread(struct thread_restore_args *args)
>         if (ret)
>                 goto core_restore_end;
>
> +       ret = sys_prctl(PR_SET_NAME, (unsigned long) &args->comm, 0, 0, 0);
> +       if (ret) {
> +               pr_err("Unable to set a thread name: %d\n", ret);
> +               goto core_restore_end;
> +       }
> +
>         pr_info("%ld: Restored\n", sys_gettid());
>
>         restore_finish_stage(task_entries_local, CR_STATE_RESTORE);

We could omit restoring:
1. Thread leader (we already restored that)
2. If threads have the same name as the leader.
Potentially saving a syscall per-restored thread.
Andrey Vagin Feb. 1, 2018, 9:29 p.m.
On Thu, Feb 01, 2018 at 09:02:32PM +0000, Dmitry Safonov wrote:
> 2018-02-01 19:29 GMT+00:00 Andrei Vagin <avagin@openvz.org>:
> > From: Andrei Vagin <avagin@virtuozzo.com>
> >
> > Reported-by: 志平 林 <larry.lin@outlook.com>
> > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> 
> Looks good.
> We could get it from procfs on pre-dump...
> But I guess, it's a trade-off between 3 syscalls from criu
> and 1 syscall in parasite, so I don't feel the difference.
> 
> How about your rule for 1 new feature = 1 new test? ;-)

You can see that I attached this patch to a feature request email, I'm
not going to commit this patch in this form. You can consider that it is
RFC;).

I will fix your other comments in the next version.

> Also some small nits below.
> 
> >         ret = sys_prctl(PR_GET_PDEATHSIG, (unsigned long)&ti->pdeath_sig, 0, 0, 0);
> > -       if (ret)
> > +       if (ret) {
> > +               pr_err("Unable to get the parent death signal: %d\n", ret);
> > +               goto out;
> > +       }
> > +
> > +       ret = sys_prctl(PR_GET_NAME, (unsigned long) &ti->comm, 0, 0, 0);
> > +       if (ret) {
> > +               pr_err("Unable to get the thread name: %d\n", ret);
> >                 goto out;
> > +       }
> 
> We could omit getting thread's name if it's a thread leader.
> (just less on a syscall per each task on the dump).
> We can live without thread's name if it's the same as leader
> (potentialy saving a bit of space in image, removing allocation on
> dump, not bothering on restore about the name).

> 
> > @@ -549,6 +549,12 @@ long __export_restore_thread(struct thread_restore_args *args)
> >         if (ret)
> >                 goto core_restore_end;
> >
> > +       ret = sys_prctl(PR_SET_NAME, (unsigned long) &args->comm, 0, 0, 0);
> > +       if (ret) {
> > +               pr_err("Unable to set a thread name: %d\n", ret);
> > +               goto core_restore_end;
> > +       }
> > +
> >         pr_info("%ld: Restored\n", sys_gettid());
> >
> >         restore_finish_stage(task_entries_local, CR_STATE_RESTORE);
> 
> We could omit restoring:
> 1. Thread leader (we already restored that)
> 2. If threads have the same name as the leader.
> Potentially saving a syscall per-restored thread.
> 
> -- 
>              Dmitry