From d4e1c5fb4416b382ee3233fe1df5ed045745cf5f Mon Sep 17 00:00:00 2001 From: Kirill Tkhai Date: Tue, 16 May 2017 19:27:07 +0300 Subject: [PATCH] forking: Use last_pid_mutex for synchronization during clone() 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. v2: Don't use flock() at all Signed-off-by: Kirill Tkhai Signed-off-by: Andrei Vagin --- criu/cr-restore.c | 20 +++----------------- criu/img-remote-proto.c | 16 ---------------- criu/namespaces.c | 33 +++++---------------------------- criu/pie/restorer.c | 34 ++++++++++++++-------------------- 4 files changed, 22 insertions(+), 81 deletions(-) diff --git a/criu/cr-restore.c b/criu/cr-restore.c index 72daf6697..efbc3cbe9 100644 --- a/criu/cr-restore.c +++ b/criu/cr-restore.c @@ -1231,7 +1231,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; }; @@ -1432,19 +1431,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) { @@ -1453,10 +1445,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); @@ -1725,9 +1714,6 @@ static int restore_task_with_children(void *_arg) if (current->pid->real < 0) 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; diff --git a/criu/img-remote-proto.c b/criu/img-remote-proto.c index 4f440976a..a5450e67c 100644 --- a/criu/img-remote-proto.c +++ b/criu/img-remote-proto.c @@ -616,28 +616,12 @@ void *accept_local_image_connections(void *port) prepare_fwd_rimg(); } - /* We need to flock the last pid file to avoid stealing pids - * from restore. - */ - int fd = open_proc_rw(PROC_GEN, LAST_PID_PATH); - if (fd < 0) - goto err; - - if (flock(fd, LOCK_EX)) { - pr_perror("Can't lock %s", LAST_PID_PATH); - goto err; - } - if (pthread_create( &tid, NULL, process_local_image_connection, (void *) wt)) { pr_perror("Unable to create worker thread"); goto err; } - if (flock(fd, LOCK_UN)) - pr_perror("Can't unlock %s", LAST_PID_PATH); - close(fd); - wt->tid = tid; add_worker(wt); } diff --git a/criu/namespaces.c b/criu/namespaces.c index fab5ca733..7877c034a 100644 --- a/criu/namespaces.c +++ b/criu/namespaces.c @@ -2627,7 +2627,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, i, lock_fd, transport_fd, saved_errno; + int pid_ns_fd, i, transport_fd, saved_errno; struct pstree_item *ns_reaper; struct ns_id *ns, *tmp; struct pid *pid; @@ -2645,26 +2645,7 @@ static int do_create_pid_ns_helper(void *arg, int sk, pid_t unused_pid) goto err; } - if (switch_ns(root_item->pid->real, &mnt_ns_desc, &mnt_ns_fd) < 0) { - pr_err("Can't set mnt_ns\n"); - 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); BUG_ON(transport_fd < 0); @@ -2676,18 +2657,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)) { pr_err("Can't set next pid using helper\n"); - flock(lock_fd, LOCK_UN); - close(lock_fd); + unlock_last_pid(); goto err; } child = fork(); - if (!child) { - close(lock_fd); + if (!child) exit(pid_ns_helper(ns, sk)); - } saved_errno = errno; - flock(lock_fd, LOCK_UN); - close(lock_fd); + unlock_last_pid(); if (child < 0) { errno = saved_errno; pr_perror("Can't fork"); diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c index 6549de5e9..ed3d8d898 100644 --- a/criu/pie/restorer.c +++ b/criu/pie/restorer.c @@ -1533,20 +1533,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; + int i, fd = -1; - 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; + 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; @@ -1554,13 +1552,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; } @@ -1570,7 +1569,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; } } @@ -1587,14 +1586,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);