[CRIU] [PATCH 3/3] forking: Use last_pid_mutex for synchronization during clone()
Andrei Vagin
avagin at virtuozzo.com
Fri May 12 14:24:39 PDT 2017
On Thu, May 11, 2017 at 05:55:21PM +0300, Kirill Tkhai wrote:
> Before this patch we used flock to order task creation,
> but this way is not good. It took 5 syscalls to synchronize
> a creation of a single child:
>
> 1)open()
> 2)flock(LOCK_EX)
> 3)flock(LOCK_UN)
> 4)close() in parent
> 5)close() in child
>
> The patch introduces more effective way for synchronization,
> which executes 2 syscalls only. We use last_pid_mutex,
> and the syscalls number sounds definitely better.
>
> In order to synchronize with parallel CRIU, we still
> take ns_last_pid flock. After talking to @xemul,
> we remain the ns_last_pid file locked all the restore time,
> which is done once by criu task. In further, if we need
> to relax this, we may take the flock() globally on
> CR_STATE_FORKING stage only, and synchronize thread
> creation with flock() locally, but now we don't need that.
>
> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
> ---
> criu/cr-restore.c | 41 +++++++++++++++++++++--------------------
> criu/namespaces.c | 23 +++++------------------
> criu/pie/restorer.c | 36 +++++++++++++++---------------------
> 3 files changed, 41 insertions(+), 59 deletions(-)
>
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index ec80c54b4..e4c7fad36 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -1169,7 +1169,6 @@ static int restore_one_task(int pid, CoreEntry *core)
> struct cr_clone_arg {
> struct pstree_item *item;
> unsigned long clone_flags;
> - int fd;
>
> CoreEntry *core;
> };
> @@ -1378,19 +1377,12 @@ static inline int fork_with_pid(struct pstree_item *item)
>
> pr_info("Forking task with %d pid (flags 0x%lx)\n", pid, ca.clone_flags);
>
> - ca.fd = open_proc_rw(PROC_GEN, LAST_PID_PATH);
> - if (ca.fd < 0)
> - goto err;
> -
> wait_pid_ns_helper_prepared(pid_ns, item->pid);
>
> if (set_pid_ns_for_children(pid_ns, item->pid) < 0)
> - goto err_close;
> + goto err;
>
> - if (flock(ca.fd, LOCK_EX)) {
> - pr_perror("%d: Can't lock %s", pid, LAST_PID_PATH);
> - goto err_close;
> - }
> + lock_last_pid();
>
> ret = do_fork_with_pid(item, pid_ns, &ca);
> if (ret < 0) {
> @@ -1399,10 +1391,7 @@ static inline int fork_with_pid(struct pstree_item *item)
> }
>
> err_unlock:
> - if (flock(ca.fd, LOCK_UN))
> - pr_perror("%d: Can't unlock %s", pid, LAST_PID_PATH);
> -err_close:
> - close(ca.fd);
> + unlock_last_pid();
> err:
> if (ca.core)
> core_entry__free_unpacked(ca.core, NULL);
> @@ -1702,9 +1691,6 @@ static int restore_task_with_children(void *_arg)
> goto err;
> }
>
> - if ( !(ca->clone_flags & CLONE_FILES))
> - close_safe(&ca->fd);
> -
> ret = clone_service_fd(rsti(current)->service_fd_id);
> if (ret)
> goto err;
> @@ -2080,7 +2066,7 @@ static int write_restored_pid(void)
> static int restore_root_task(struct pstree_item *init)
> {
> enum trace_flags flag = TRACE_ALL;
> - int ret, fd, mnt_ns_fd = -1;
> + int ret, fd, mnt_ns_fd = -1, lock_fd = -1;
> int root_seized = 0;
> struct pstree_item *item;
>
> @@ -2130,11 +2116,19 @@ static int restore_root_task(struct pstree_item *init)
> if (prepare_namespace_before_tasks())
> return -1;
>
> + lock_fd = open_proc_rw(PROC_GEN, LAST_PID_PATH);
> + if (lock_fd < 0)
> + goto out;
> + if (flock(lock_fd, LOCK_EX)) {
> + pr_perror("Can't flock ns_last_pid");
> + goto out_close;
> + }
We need to lock ns_last_pid only for a case when processes are restored
in a current pid namespace.
> +
> __restore_switch_stage_nw(CR_STATE_ROOT_TASK);
>
> ret = fork_with_pid(init);
> if (ret < 0)
> - goto out;
> + goto out_unlock;
>
> restore_origin_ns_hook();
>
> @@ -2286,6 +2280,9 @@ static int restore_root_task(struct pstree_item *init)
> ret = restore_switch_stage(CR_STATE_RESTORE_CREDS);
> BUG_ON(ret);
>
> + if (flock(lock_fd, LOCK_UN) || close_safe(&lock_fd))
> + pr_perror("Can't unlock or close ns_last_pid");
> +
> timing_stop(TIME_RESTORE);
>
> ret = catch_tasks(root_seized, &flag);
> @@ -2354,7 +2351,11 @@ static int restore_root_task(struct pstree_item *init)
> if (vpid(pi) > 0)
> kill(vpid(pi), SIGKILL);
> }
> -
> +out_unlock:
> + if (lock_fd >= 0 && flock(lock_fd, LOCK_UN))
> + pr_perror("Can't unlock ns_last_pid");
> +out_close:
> + close_safe(&lock_fd);
> out:
> fini_cgroup();
> depopulate_roots_yard(mnt_ns_fd, true);
> diff --git a/criu/namespaces.c b/criu/namespaces.c
> index 7a38dfd4a..2a4e1327c 100644
> --- a/criu/namespaces.c
> +++ b/criu/namespaces.c
> @@ -2620,7 +2620,7 @@ static int pid_ns_helper(struct ns_id *ns, int sk)
>
> static int do_create_pid_ns_helper(void *arg, int sk, pid_t unused_pid)
> {
> - int pid_ns_fd, mnt_ns_fd, fd, i, lock_fd, transport_fd;
> + int pid_ns_fd, mnt_ns_fd, fd, i, transport_fd;
> struct ns_id *ns, *tmp;
> struct pid *pid;
> pid_t child;
> @@ -2660,21 +2660,12 @@ static int do_create_pid_ns_helper(void *arg, int sk, pid_t unused_pid)
> goto err;
> }
>
> - lock_fd = open("/proc/" LAST_PID_PATH, O_RDONLY);
> - if (lock_fd < 0) {
> - pr_perror("Unable to open /proc/" LAST_PID_PATH);
> - goto err;
> - }
> -
> if (restore_ns(mnt_ns_fd, &mnt_ns_desc) < 0) {
> pr_err("Can't restore ns\n");
> goto err;
> }
>
> - if (flock(lock_fd, LOCK_EX)) {
> - close(lock_fd);
> - pr_perror("Can't lock %s", LAST_PID_PATH);
> - }
> + lock_last_pid();
>
> transport_fd = get_service_fd(TRANSPORT_FD_OFF);
> /*
> @@ -2684,18 +2675,14 @@ static int do_create_pid_ns_helper(void *arg, int sk, pid_t unused_pid)
> */
> for (i = pid->level - 2, tmp = ns->parent; i >= 0; i--, tmp = tmp->parent)
> if (request_set_next_pid(tmp->id, pid->ns[i].virt, transport_fd)) {
> + unlock_last_pid();
> pr_err("Can't set next pid using helper\n");
> - flock(lock_fd, LOCK_UN);
> - close(lock_fd);
> goto err;
> }
> child = fork();
> - if (!child) {
> - close(lock_fd);
> + if (!child)
> exit(pid_ns_helper(ns, sk));
> - }
> - flock(lock_fd, LOCK_UN);
> - close(lock_fd);
> + unlock_last_pid();
> if (child < 0) {
> pr_perror("Can't fork");
> goto err;
> diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c
> index 6e2f7f4d7..eee683bac 100644
> --- a/criu/pie/restorer.c
> +++ b/criu/pie/restorer.c
> @@ -1440,20 +1440,18 @@ long __export_restore_task(struct task_restore_args *args)
> CLONE_THREAD | CLONE_SYSVSEM | CLONE_FS;
> long last_pid_len;
> long parent_tid;
> - int i, fd;
> -
> - fd = sys_openat(args->proc_fd, LAST_PID_PATH, O_RDWR, 0);
> - if (fd < 0) {
> - pr_err("can't open last pid fd %d\n", fd);
> - goto core_restore_end;
> + int i, fd = -1;
> +
> + if (thread_args[0].pid[1] == 0) {
> + /* One level pid ns hierarhy */
> + fd = sys_openat(args->proc_fd, LAST_PID_PATH, O_RDWR, 0);
> + if (fd < 0) {
> + pr_err("can't open last pid fd %d\n", fd);
> + goto core_restore_end;
> + }
> }
>
> - ret = sys_flock(fd, LOCK_EX);
> - if (ret) {
> - pr_err("Can't lock last_pid %d\n", fd);
> - sys_close(fd);
> - goto core_restore_end;
> - }
> + mutex_lock(&task_entries_local->last_pid_mutex);
>
> for (i = 0; i < args->nr_threads; i++) {
> char last_pid_buf[16], *s;
> @@ -1461,13 +1459,14 @@ long __export_restore_task(struct task_restore_args *args)
> if (thread_args[i].pid[0] == args->t->pid[0])
> continue;
>
> - if (thread_args[i].pid[1] == 0) {
> + if (fd >= 0) {
> /* One level pid ns hierarhy */
> last_pid_len = std_vprint_num(last_pid_buf, sizeof(last_pid_buf), thread_args[i].pid[0] - 1, &s);
> sys_lseek(fd, 0, SEEK_SET);
> ret = sys_write(fd, s, last_pid_len);
> if (ret < 0) {
> pr_err("Can't set last_pid %ld/%s\n", ret, last_pid_buf);
> + mutex_unlock(&task_entries_local->last_pid_mutex);
> sys_close(fd);
> goto core_restore_end;
> }
> @@ -1477,7 +1476,7 @@ long __export_restore_task(struct task_restore_args *args)
> break;
> if (request_set_next_pid(args->pid_ns_id[k], thread_args[i].pid[k], args->transport_fd) < 0) {
> pr_err("Can't request to set pid\n");
> - sys_close(fd);
> + mutex_unlock(&task_entries_local->last_pid_mutex);
> goto core_restore_end;
> }
> }
> @@ -1494,14 +1493,9 @@ long __export_restore_task(struct task_restore_args *args)
> RUN_CLONE_RESTORE_FN(ret, clone_flags, new_sp, parent_tid, thread_args, args->clone_restore_fn);
> }
>
> - ret = sys_flock(fd, LOCK_UN);
> - if (ret) {
> - pr_err("Can't unlock last_pid %ld\n", ret);
> + mutex_unlock(&task_entries_local->last_pid_mutex);
> + if (fd >= 0)
> sys_close(fd);
> - goto core_restore_end;
> - }
> -
> - sys_close(fd);
> }
>
> restore_rlims(args);
>
More information about the CRIU
mailing list