[CRIU] [PATCH 3/5] pstree: prepare to store all pid-s in rb-tree
Andrew Vagin
avagin at virtuozzo.com
Thu Mar 10 10:23:06 PST 2016
On Thu, Mar 10, 2016 at 10:33:50AM +0300, Pavel Emelyanov wrote:
> On 03/09/2016 09:43 PM, Andrew Vagin wrote:
> > On Wed, Mar 09, 2016 at 02:02:39PM +0300, Pavel Emelyanov wrote:
> >> On 02/19/2016 09:13 PM, Andrey Vagin wrote:
> >>> From: Andrew Vagin <avagin at virtuozzo.com>
> >>>
> >>> It will allow us to find a free pid and will speed up
> >>> searching an item by pid.
> >>>
> >>> The current algorithm to searching a free pid may return
> >>> a value bigger that kernel.max_pid.
> >>>
> >>> Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
> >>> ---
> >>> criu/files-reg.c | 17 +++++++--------
> >>> criu/include/image.h | 2 ++
> >>> criu/include/pid.h | 3 +++
> >>> criu/include/pstree.h | 2 ++
> >>> criu/pstree.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++----
> >>> 5 files changed, 69 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/criu/files-reg.c b/criu/files-reg.c
> >>> index 96921b8..e47cb76 100644
> >>> --- a/criu/files-reg.c
> >>> +++ b/criu/files-reg.c
> >>> @@ -357,18 +357,17 @@ static int open_remap_dead_process(struct reg_file_info *rfi,
> >>> {
> >>> struct pstree_item *helper;
> >>>
> >>> - for_each_pstree_item(helper) {
> >>> - /* don't need to add multiple tasks */
> >>> - if (helper->pid.virt == rfe->remap_id) {
> >>> - pr_info("Skipping helper for restoring /proc/%d; pid exists\n", rfe->remap_id);
> >>> - return 0;
> >>> - }
> >>> - }
> >>> -
> >>> - helper = alloc_pstree_helper();
> >>> + helper = insert_item(rfe->remap_id);
> >>> if (!helper)
> >>> return -1;
> >>>
> >>> + if (helper->pid.state != TASK_UNDEF) {
> >>> + pr_info("Skipping helper for restoring /proc/%d; pid exists\n", rfe->remap_id);
> >>> + return 0;
> >>> + }
> >>> +
> >>> + init_pstree_helper(helper);
> >>> +
> >>> helper->sid = root_item->sid;
> >>> helper->pgid = root_item->pgid;
> >>> helper->pid.virt = rfe->remap_id;
> >>> diff --git a/criu/include/image.h b/criu/include/image.h
> >>> index 305febf..f141915 100644
> >>> --- a/criu/include/image.h
> >>> +++ b/criu/include/image.h
> >>> @@ -112,10 +112,12 @@
> >>>
> >>> #define TASK_COMM_LEN 16
> >>>
> >>> +#define TASK_UNDEF 0x0
> >>> #define TASK_ALIVE 0x1
> >>> #define TASK_DEAD 0x2
> >>> #define TASK_STOPPED 0x3
> >>> #define TASK_HELPER 0x4
> >>> +#define TASK_THREAD 0x5
> >>>
> >>> #define CR_PARENT_LINK "parent"
> >>>
> >>> diff --git a/criu/include/pid.h b/criu/include/pid.h
> >>> index 2a709a2..533be88 100644
> >>> --- a/criu/include/pid.h
> >>> +++ b/criu/include/pid.h
> >>> @@ -2,6 +2,7 @@
> >>> #define __CR_PID_H__
> >>>
> >>> #include "stdbool.h"
> >>> +#include "rbtree.h"
> >>>
> >>> struct pid {
> >>> /*
> >>> @@ -19,6 +20,8 @@ struct pid {
> >>> pid_t virt;
> >>>
> >>> int state; /* TASK_XXX constants */
> >>> +
> >>> + struct rb_node node;
> >>> };
> >>>
> >>> /*
> >>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
> >>> index fcc1158..7bef810 100644
> >>> --- a/criu/include/pstree.h
> >>> +++ b/criu/include/pstree.h
> >>> @@ -68,6 +68,8 @@ extern struct pstree_item *__alloc_pstree_item(bool rst);
> >>> extern struct pstree_item *alloc_pstree_helper(void);
> >>> extern void init_pstree_helper(struct pstree_item *ret);
> >>>
> >>> +extern struct pstree_item *insert_item(pid_t pid);
> >>> +
> >>> extern struct pstree_item *root_item;
> >>> extern struct pstree_item *pstree_item_next(struct pstree_item *item);
> >>> #define for_each_pstree_item(pi) \
> >>> diff --git a/criu/pstree.c b/criu/pstree.c
> >>> index 2688547..1be8c48 100644
> >>> --- a/criu/pstree.c
> >>> +++ b/criu/pstree.c
> >>> @@ -17,6 +17,7 @@
> >>> #include "images/pstree.pb-c.h"
> >>>
> >>> struct pstree_item *root_item;
> >>> +struct rb_root root_rb;
> >>
> >> Bad name for global variable.
> >
> > It should be static
> >
> >>
> >>>
> >>> #define CLONE_ALLNS (CLONE_NEWPID | CLONE_NEWNET | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_NEWNS | CLONE_NEWUSER)
> >>>
> >>> @@ -384,6 +385,50 @@ static int prepare_pstree_for_shell_job(void)
> >>> return 0;
> >>> }
> >>>
> >>> +static struct pid *insert_pid(pid_t pid, struct pid *pid_node)
> >>
> >> Why struct pid's value is not enough and we have to pass pid_t pid here?
> >
> > pid_node is used to insert a thread entry
>
> Huh? Let's check, see below:
>
> >>
> >>> +{
> >>> + struct rb_node *node = root_rb.rb_node;
> >>> + struct rb_node **new = &root_rb.rb_node;
> >>> + struct rb_node *parent = NULL;
> >>> +
> >>> + while (node) {
> >>> + struct pid *this = rb_entry(node, struct pid, node);
> >>> +
> >>> + parent = *new;
> >>> + if (pid < this->virt)
> >>> + node = node->rb_left, new = &((*new)->rb_left);
> >>> + else if (pid > this->virt)
> >>> + node = node->rb_right, new = &((*new)->rb_right);
> >>> + else
> >>> + return this;
> >>> + }
>
> If we get here the node is NULL, correct?
>
> >>> +
> >>> + if (!node) {
Here is a mistake. pid_node should be instead of node. I fixed this bug
in my tree already. Thanks.
>
> And we dive into this if branch.
>
> >>> + struct pstree_item *item;
> >>> +
> >>> + item = alloc_pstree_item_with_rst();
> >>> + if (item == NULL)
> >>> + return NULL;
> >>> +
> >>> + item->pid.virt = pid;
> >>> + pid_node = &item->pid;
>
> Here we override whatever value is in the pid_node.
>
> >>> + }
> >>> + rb_link_and_balance(&root_rb, &pid_node->node, parent, new);
> >>> + return pid_node;
>
> And return it. So what's the point in two args for this function then?
>
> >>> +}
> >>> +
> >>> +struct pstree_item *insert_item(pid_t pid)
> >>
> >> This is search_item, rather than insert_ isn't it?
> >
> > Its inserts a new entry, if it isn't there yet.
>
> OK, so this is neither pure insert nor pure search. Please, find some
> appropriate name.
>
> > We call this function for pid, sid, pgid.
>
> Even sids?!
>
> >>
> >>> +{
> >>> + struct pid *node;;
> >>> +
> >>> + node = insert_pid(pid, NULL);
> >>> + if (!node)
> >>> + return NULL;
> >>> + BUG_ON(node->state == TASK_THREAD);
> >>> +
> >>> + return container_of(node, struct pstree_item, pid);
> >>> +}
> >>> +
> >>> static int read_pstree_image(void)
> >>> {
> >>> int ret = 0, i;
> >>> @@ -404,18 +449,22 @@ static int read_pstree_image(void)
> >>> break;
> >>>
> >>> ret = -1;
> >>> - pi = alloc_pstree_item_with_rst();
> >>> + pi = insert_item(e->pid);
> >>> if (pi == NULL)
> >>> break;
> >>>
> >>> + if (insert_item(e->pgid) == NULL)
> >>> + break;
> >>> + if (insert_item(e->sid) == NULL)
> >>> + break;
> >>> +
> >>
> >> Please, explain this.
> >
> > * We need to know about all used pids to allocate pids for helpers.
> > * Actually we will neet to add helpers for these pids too to restore
> > these sid and pgid.
>
> If we insert an item for e->sid, would the corresponding pstree_item (if
> it exists) pick one up?
Yes, it would
>
> >>
> >>> pi->pid.virt = e->pid;
> >>> max_pid = max((int)e->pid, max_pid);
> >>> -
> >>> pi->pgid = e->pgid;
> >>> max_pid = max((int)e->pgid, max_pid);
> >>> -
> >>> pi->sid = e->sid;
> >>> max_pid = max((int)e->sid, max_pid);
> >>> + pi->pid.state = TASK_ALIVE;
> >>>
> >>> if (e->ppid == 0) {
> >>> if (root_item) {
> >>> @@ -465,7 +514,8 @@ static int read_pstree_image(void)
> >>> for (i = 0; i < e->n_threads; i++) {
> >>> pi->threads[i].real = -1;
> >>> pi->threads[i].virt = e->threads[i];
> >>> - max_pid = max((int)e->threads[i], max_pid);
> >>> + pi->threads[i].state = TASK_THREAD;
> >>> + insert_pid(pi->threads[i].virt, &pi->threads[i]);
> >>> }
> >>>
> >>> task_entries->nr_threads += e->n_threads;
> >>>
> >>
> > .
> >
>
More information about the CRIU
mailing list