[PATCHv4] procfs: show hierarchy of pid namespace

[PATCHv4] procfs: show hierarchy of pid namespace

[Chen Hanxiao]

This patch will show the hierarchy of pid namespace
by /proc/pidns_hierarchy like:

[root@localhost ~]#cat /proc/pidns_hierarchy
/proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
/proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
/proc/1550/ns/pid

It shows the pid hierarchy below:

        init_pid_ns (not showed in /proc/pidns_hierarchy)
              │
┌──────────────┐
ns1                          ns2
│                            │
/proc/1550/ns/pid    /proc/18060/ns/pid
                              │
                              │
                             ns3
                              │
                     /proc/18102/ns/pid
                              │
                     ┌──────────┒
                     ns4                   ns5
                     │                    │
                     /proc/1534/ns/pid   /proc/1600/ns/pid

Every pid printed in pidns_hierarchy
is the init pid of that pid ns level.

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
v4: simplify pid collection and do some performance optimizamtion
    fix another race issue.
v3: fix a race issue and memory leak issue
v2: use a procfs text file instead of dirs under /proc

 fs/proc/Kconfig           |   6 ++
 fs/proc/Makefile          |   1 +
 fs/proc/pidns_hierarchy.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 fs/proc/pidns_hierarchy.c

diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 2183fcf..4bb111c 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -71,3 +71,9 @@ config PROC_PAGE_MONITOR
 	  /proc/pid/smaps, /proc/pid/clear_refs, /proc/pid/pagemap,
 	  /proc/kpagecount, and /proc/kpageflags. Disabling these
           interfaces will reduce the size of the kernel by approximately 4kb.
+
+config PROC_PID_HIERARCHY
+	bool "Enable /proc/pidns_hierarchy support" if EXPERT
+	depends on PROC_FS
+	help
+	  Show pid namespace hierarchy information
diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index 7151ea4..33e384b 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -30,3 +30,4 @@ proc-$(CONFIG_PROC_KCORE)	+= kcore.o
 proc-$(CONFIG_PROC_VMCORE)	+= vmcore.o
 proc-$(CONFIG_PRINTK)	+= kmsg.o
 proc-$(CONFIG_PROC_PAGE_MONITOR)	+= page.o
+proc-$(CONFIG_PROC_PID_HIERARCHY)	+= pidns_hierarchy.o
diff --git a/fs/proc/pidns_hierarchy.c b/fs/proc/pidns_hierarchy.c
new file mode 100644
index 0000000..621661b
--- /dev/null
+++ b/fs/proc/pidns_hierarchy.c
@@ -0,0 +1,218 @@
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/proc_fs.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/pid_namespace.h>
+#include <linux/seq_file.h>
+#include <linux/mutex.h>
+
+/*
+ *  /proc/pidns_hierarchy
+ *
+ *  show the hierarchy of pid namespace
+ */
+
+#define NS_HIERARCHY	"pidns_hierarchy"
+
+static LIST_HEAD(pidns_list);
+static LIST_HEAD(pidns_tree);
+static DEFINE_MUTEX(pidns_list_lock);
+
+/* list for host pid collection */
+struct pidns_list {
+	struct list_head list;
+	struct pid *pid;
+};
+
+static void free_pidns_list(struct list_head *head)
+{
+	struct pidns_list *tmp, *pos;
+
+	list_for_each_entry_safe(pos, tmp, head, list) {
+		list_del(&pos->list);
+		kfree(pos);
+	}
+}
+
+static int
+pidns_list_add(struct pid *pid, struct list_head *list_head,
+		struct pid_namespace *curr_ns)
+{
+	struct pidns_list *ent;
+	struct pid_namespace *ns;
+
+	if (is_child_reaper(pid)) {
+		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+		if (!ent)
+			return -ENOMEM;
+
+		ent->pid = pid;
+		ns = pid->numbers[pid->level].ns;
+		if (curr_ns) {
+			/* add pids who is the child of curr_ns */
+			for (; ns != NULL; ns = ns->parent)
+				if (ns == curr_ns)
+					list_add_tail(&ent->list, list_head);
+		} else {
+			list_add_tail(&ent->list, list_head);
+		}
+	}
+
+	return 0;
+}
+
+static int
+pidns_list_filter(void)
+{
+	struct pidns_list *pos, *pos_t;
+	struct pid_namespace *ns0, *ns1;
+	struct pid *pid0, *pid1;
+	int rc, flag = 0;
+
+	/* screen pid with relationship
+	 * in pidns_list, we may add pids like
+	 * ns0   ns1   ns2
+	 * pid1->pid2->pid3
+	 * we should screen pid1, pid2 and keep pid3
+	 */
+	list_for_each_entry(pos, &pidns_list, list) {
+		list_for_each_entry(pos_t, &pidns_list, list) {
+			flag = 0;
+			pid0 = pos->pid;
+			pid1 = pos_t->pid;
+			ns0 = pid0->numbers[pid0->level].ns;
+			ns1 = pid1->numbers[pid1->level].ns;
+			if (pos->pid->level < pos_t->pid->level)
+				for (; ns1 != NULL; ns1 = ns1->parent)
+					if (ns0 == ns1) {
+						flag = 1;
+						break;
+					}
+			if (flag == 1)
+				break;
+		}
+
+		if (flag == 0) {
+			rc = pidns_list_add(pos->pid, &pidns_tree, NULL);
+			if (rc)
+				goto out;
+		}
+	}
+
+	/* Now all usefull stuffs are in pidns_tree, free pidns_list*/
+	free_pidns_list(&pidns_list);
+
+	return 0;
+
+out:
+	free_pidns_list(&pidns_tree);
+	return rc;
+}
+
+/* collect pids in pidns_list,
+ * then remove duplicated ones,
+ * add the rest to pidns_tree
+ */
+static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
+{
+	struct pid *pid;
+	struct task_struct *p;
+	int rc;
+
+	/* collect pid in differet ns */
+	for_each_process(p) {
+		pid = task_pid(p);
+		if (pid && (pid->level > 0)) {
+			rc = pidns_list_add(pid, &pidns_list, curr_ns);
+			if (rc)
+				goto out;
+		}
+	}
+
+	/* screen duplicate pids from list pidns_list
+	* and form a new list pidns_tree
+	*/
+	rc = pidns_list_filter();
+	if (rc)
+		goto out;
+
+	return 0;
+
+out:
+	free_pidns_list(&pidns_list);
+	return rc;
+}
+
+static int nslist_proc_show(struct seq_file *m, void *v)
+{
+	struct pidns_list *pos;
+	struct pid_namespace *ns, *curr_ns;
+	struct pid *pid;
+	char pid_buf[32];
+	int i, curr_level;
+	int rc;
+
+	curr_ns = task_active_pid_ns(current);
+
+	mutex_lock(&pidns_list_lock);
+	rcu_read_lock();
+	rc = proc_pidns_list_refresh(curr_ns);
+	if (rc) {
+		rcu_read_unlock();
+		mutex_unlock(&pidns_list_lock);
+		return rc;
+	}
+
+	/* print pid namespace hierarchy */
+	list_for_each_entry(pos, &pidns_tree, list) {
+		pid = pos->pid;
+		curr_level = -1;
+		ns = pid->numbers[pid->level].ns;
+		/* Check whether a pid has relationship with current ns */
+		for (; ns != NULL; ns = ns->parent)
+			if (ns == curr_ns)
+				curr_level = curr_ns->level;
+
+		if (curr_level == -1)
+			continue;
+
+		for (i = curr_level + 1; i <= pid->level; i++) {
+			ns = pid->numbers[i].ns;
+			/* show PID '1' in specific pid ns */
+			snprintf(pid_buf, 32, "/proc/%u/ns/pid",
+				pid_vnr(find_pid_ns(1, ns)));
+			seq_printf(m, "%s ", pid_buf);
+		}
+
+		seq_putc(m, '\n');
+	}
+
+	free_pidns_list(&pidns_tree);
+	rcu_read_unlock();
+	mutex_unlock(&pidns_list_lock);
+
+	return 0;
+}
+
+static int nslist_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nslist_proc_show, NULL);
+}
+
+static const struct file_operations proc_nspid_nslist_fops = {
+	.open		= nslist_proc_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init pidns_hierarchy_init(void)
+{
+	proc_create(NS_HIERARCHY, S_IWUGO,
+		NULL, &proc_nspid_nslist_fops);
+
+	return 0;
+}
+fs_initcall(pidns_hierarchy_init);
-- 
1.9.0

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

[PATCHv4] procfs: show hierarchy of pid namespace

[Chen Hanxiao]

This patch will show the hierarchy of pid namespace
by /proc/pidns_hierarchy like:

[root@localhost ~]#cat /proc/pidns_hierarchy
/proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
/proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
/proc/1550/ns/pid

It shows the pid hierarchy below:

        init_pid_ns (not showed in /proc/pidns_hierarchy)
              │
┌──────────────┐
ns1                          ns2
│                            │
/proc/1550/ns/pid    /proc/18060/ns/pid
                              │
                              │
                             ns3
                              │
                     /proc/18102/ns/pid
                              │
                     ┌──────────┒
                     ns4                   ns5
                     │                    │
                     /proc/1534/ns/pid   /proc/1600/ns/pid

Every pid printed in pidns_hierarchy
is the init pid of that pid ns level.

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
v4: simplify pid collection and do some performance optimizamtion
    fix another race issue.
v3: fix a race issue and memory leak issue
v2: use a procfs text file instead of dirs under /proc

 fs/proc/Kconfig           |   6 ++
 fs/proc/Makefile          |   1 +
 fs/proc/pidns_hierarchy.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 fs/proc/pidns_hierarchy.c

diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 2183fcf..4bb111c 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -71,3 +71,9 @@ config PROC_PAGE_MONITOR
 	  /proc/pid/smaps, /proc/pid/clear_refs, /proc/pid/pagemap,
 	  /proc/kpagecount, and /proc/kpageflags. Disabling these
           interfaces will reduce the size of the kernel by approximately 4kb.
+
+config PROC_PID_HIERARCHY
+	bool "Enable /proc/pidns_hierarchy support" if EXPERT
+	depends on PROC_FS
+	help
+	  Show pid namespace hierarchy information
diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index 7151ea4..33e384b 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -30,3 +30,4 @@ proc-$(CONFIG_PROC_KCORE)	+= kcore.o
 proc-$(CONFIG_PROC_VMCORE)	+= vmcore.o
 proc-$(CONFIG_PRINTK)	+= kmsg.o
 proc-$(CONFIG_PROC_PAGE_MONITOR)	+= page.o
+proc-$(CONFIG_PROC_PID_HIERARCHY)	+= pidns_hierarchy.o
diff --git a/fs/proc/pidns_hierarchy.c b/fs/proc/pidns_hierarchy.c
new file mode 100644
index 0000000..621661b
--- /dev/null
+++ b/fs/proc/pidns_hierarchy.c
@@ -0,0 +1,218 @@
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/proc_fs.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/pid_namespace.h>
+#include <linux/seq_file.h>
+#include <linux/mutex.h>
+
+/*
+ *  /proc/pidns_hierarchy
+ *
+ *  show the hierarchy of pid namespace
+ */
+
+#define NS_HIERARCHY	"pidns_hierarchy"
+
+static LIST_HEAD(pidns_list);
+static LIST_HEAD(pidns_tree);
+static DEFINE_MUTEX(pidns_list_lock);
+
+/* list for host pid collection */
+struct pidns_list {
+	struct list_head list;
+	struct pid *pid;
+};
+
+static void free_pidns_list(struct list_head *head)
+{
+	struct pidns_list *tmp, *pos;
+
+	list_for_each_entry_safe(pos, tmp, head, list) {
+		list_del(&pos->list);
+		kfree(pos);
+	}
+}
+
+static int
+pidns_list_add(struct pid *pid, struct list_head *list_head,
+		struct pid_namespace *curr_ns)
+{
+	struct pidns_list *ent;
+	struct pid_namespace *ns;
+
+	if (is_child_reaper(pid)) {
+		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+		if (!ent)
+			return -ENOMEM;
+
+		ent->pid = pid;
+		ns = pid->numbers[pid->level].ns;
+		if (curr_ns) {
+			/* add pids who is the child of curr_ns */
+			for (; ns != NULL; ns = ns->parent)
+				if (ns == curr_ns)
+					list_add_tail(&ent->list, list_head);
+		} else {
+			list_add_tail(&ent->list, list_head);
+		}
+	}
+
+	return 0;
+}
+
+static int
+pidns_list_filter(void)
+{
+	struct pidns_list *pos, *pos_t;
+	struct pid_namespace *ns0, *ns1;
+	struct pid *pid0, *pid1;
+	int rc, flag = 0;
+
+	/* screen pid with relationship
+	 * in pidns_list, we may add pids like
+	 * ns0   ns1   ns2
+	 * pid1->pid2->pid3
+	 * we should screen pid1, pid2 and keep pid3
+	 */
+	list_for_each_entry(pos, &pidns_list, list) {
+		list_for_each_entry(pos_t, &pidns_list, list) {
+			flag = 0;
+			pid0 = pos->pid;
+			pid1 = pos_t->pid;
+			ns0 = pid0->numbers[pid0->level].ns;
+			ns1 = pid1->numbers[pid1->level].ns;
+			if (pos->pid->level < pos_t->pid->level)
+				for (; ns1 != NULL; ns1 = ns1->parent)
+					if (ns0 == ns1) {
+						flag = 1;
+						break;
+					}
+			if (flag == 1)
+				break;
+		}
+
+		if (flag == 0) {
+			rc = pidns_list_add(pos->pid, &pidns_tree, NULL);
+			if (rc)
+				goto out;
+		}
+	}
+
+	/* Now all usefull stuffs are in pidns_tree, free pidns_list*/
+	free_pidns_list(&pidns_list);
+
+	return 0;
+
+out:
+	free_pidns_list(&pidns_tree);
+	return rc;
+}
+
+/* collect pids in pidns_list,
+ * then remove duplicated ones,
+ * add the rest to pidns_tree
+ */
+static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
+{
+	struct pid *pid;
+	struct task_struct *p;
+	int rc;
+
+	/* collect pid in differet ns */
+	for_each_process(p) {
+		pid = task_pid(p);
+		if (pid && (pid->level > 0)) {
+			rc = pidns_list_add(pid, &pidns_list, curr_ns);
+			if (rc)
+				goto out;
+		}
+	}
+
+	/* screen duplicate pids from list pidns_list
+	* and form a new list pidns_tree
+	*/
+	rc = pidns_list_filter();
+	if (rc)
+		goto out;
+
+	return 0;
+
+out:
+	free_pidns_list(&pidns_list);
+	return rc;
+}
+
+static int nslist_proc_show(struct seq_file *m, void *v)
+{
+	struct pidns_list *pos;
+	struct pid_namespace *ns, *curr_ns;
+	struct pid *pid;
+	char pid_buf[32];
+	int i, curr_level;
+	int rc;
+
+	curr_ns = task_active_pid_ns(current);
+
+	mutex_lock(&pidns_list_lock);
+	rcu_read_lock();
+	rc = proc_pidns_list_refresh(curr_ns);
+	if (rc) {
+		rcu_read_unlock();
+		mutex_unlock(&pidns_list_lock);
+		return rc;
+	}
+
+	/* print pid namespace hierarchy */
+	list_for_each_entry(pos, &pidns_tree, list) {
+		pid = pos->pid;
+		curr_level = -1;
+		ns = pid->numbers[pid->level].ns;
+		/* Check whether a pid has relationship with current ns */
+		for (; ns != NULL; ns = ns->parent)
+			if (ns == curr_ns)
+				curr_level = curr_ns->level;
+
+		if (curr_level == -1)
+			continue;
+
+		for (i = curr_level + 1; i <= pid->level; i++) {
+			ns = pid->numbers[i].ns;
+			/* show PID '1' in specific pid ns */
+			snprintf(pid_buf, 32, "/proc/%u/ns/pid",
+				pid_vnr(find_pid_ns(1, ns)));
+			seq_printf(m, "%s ", pid_buf);
+		}
+
+		seq_putc(m, '\n');
+	}
+
+	free_pidns_list(&pidns_tree);
+	rcu_read_unlock();
+	mutex_unlock(&pidns_list_lock);
+
+	return 0;
+}
+
+static int nslist_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nslist_proc_show, NULL);
+}
+
+static const struct file_operations proc_nspid_nslist_fops = {
+	.open		= nslist_proc_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init pidns_hierarchy_init(void)
+{
+	proc_create(NS_HIERARCHY, S_IWUGO,
+		NULL, &proc_nspid_nslist_fops);
+
+	return 0;
+}
+fs_initcall(pidns_hierarchy_init);
-- 
1.9.0

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Richard Weinberger]

Am 08.10.2014 11:56, schrieb Chen Hanxiao:
> This patch will show the hierarchy of pid namespace
> by /proc/pidns_hierarchy like:
> 
> [root@localhost ~]#cat /proc/pidns_hierarchy
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> /proc/1550/ns/pid

A proc file that prints paths of other proc files, srsly? ;)
I didn't follow the whole discussion but why is this not
a directory containing symbolic links to other pid files in /proc/<PID>/ns/pid?

Thanks,
//richard

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Richard Weinberger]

Am 08.10.2014 11:56, schrieb Chen Hanxiao:
> This patch will show the hierarchy of pid namespace
> by /proc/pidns_hierarchy like:
> 
> [root@localhost ~]#cat /proc/pidns_hierarchy
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> /proc/1550/ns/pid

A proc file that prints paths of other proc files, srsly? ;)
I didn't follow the whole discussion but why is this not
a directory containing symbolic links to other pid files in /proc/<PID>/ns/pid?

Thanks,
//richard

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Oleg Nesterov]

Sorry if this was already discussed, I have to admit that I ignored
the previous discussion ;) And it is possible I misread this patch
completely.

On 10/08, Chen Hanxiao wrote:
>
> This patch will show the hierarchy of pid namespace
> by /proc/pidns_hierarchy like:
>
> [root@localhost ~]#cat /proc/pidns_hierarchy
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> /proc/1550/ns/pid

Well, personally I too think that the filenames look a bit strange,
can't it just print the numbers?

And, iiuc what this patch does... perhaps in this case we should
simply add "struct list_head children" into struct pid_namespace?
In this case the patch will be really simple. I dunno.

> +pidns_list_add(struct pid *pid, struct list_head *list_head,
> +		struct pid_namespace *curr_ns)
> +{
> +	struct pidns_list *ent;
> +	struct pid_namespace *ns;
> +
> +	if (is_child_reaper(pid)) {
> +		ent = kmalloc(sizeof(*ent), GFP_KERNEL);

GFP_KERNEL under rcu_read_lock(). This is not safe without
CONFIG_PREEMPT_RCU.

> +		if (!ent)
> +			return -ENOMEM;
> +
> +		ent->pid = pid;
> +		ns = pid->numbers[pid->level].ns;
> +		if (curr_ns) {
> +			/* add pids who is the child of curr_ns */
> +			for (; ns != NULL; ns = ns->parent)
> +				if (ns == curr_ns)
> +					list_add_tail(&ent->list, list_head);

afaics, it doesn't make sense to continue after list_add() ?

> +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
> +{
> +	struct pid *pid;
> +	struct task_struct *p;
> +	int rc;
> +
> +	/* collect pid in differet ns */
> +	for_each_process(p) {

Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
makes more sense?

> +		pid = task_pid(p);

Well, in theory you need barrier() here. Or perhaps we should add
ACCESS_ONCE() into task_pid()...

And imho it would be better to declare pidns_list/pidns_tree locally
in nslist_proc_show() and pass them to the callees.

Oleg.

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Oleg Nesterov]

Sorry if this was already discussed, I have to admit that I ignored
the previous discussion ;) And it is possible I misread this patch
completely.

On 10/08, Chen Hanxiao wrote:
>
> This patch will show the hierarchy of pid namespace
> by /proc/pidns_hierarchy like:
>
> [root@localhost ~]#cat /proc/pidns_hierarchy
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> /proc/1550/ns/pid

Well, personally I too think that the filenames look a bit strange,
can't it just print the numbers?

And, iiuc what this patch does... perhaps in this case we should
simply add "struct list_head children" into struct pid_namespace?
In this case the patch will be really simple. I dunno.

> +pidns_list_add(struct pid *pid, struct list_head *list_head,
> +		struct pid_namespace *curr_ns)
> +{
> +	struct pidns_list *ent;
> +	struct pid_namespace *ns;
> +
> +	if (is_child_reaper(pid)) {
> +		ent = kmalloc(sizeof(*ent), GFP_KERNEL);

GFP_KERNEL under rcu_read_lock(). This is not safe without
CONFIG_PREEMPT_RCU.

> +		if (!ent)
> +			return -ENOMEM;
> +
> +		ent->pid = pid;
> +		ns = pid->numbers[pid->level].ns;
> +		if (curr_ns) {
> +			/* add pids who is the child of curr_ns */
> +			for (; ns != NULL; ns = ns->parent)
> +				if (ns == curr_ns)
> +					list_add_tail(&ent->list, list_head);

afaics, it doesn't make sense to continue after list_add() ?

> +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
> +{
> +	struct pid *pid;
> +	struct task_struct *p;
> +	int rc;
> +
> +	/* collect pid in differet ns */
> +	for_each_process(p) {

Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
makes more sense?

> +		pid = task_pid(p);

Well, in theory you need barrier() here. Or perhaps we should add
ACCESS_ONCE() into task_pid()...

And imho it would be better to declare pidns_list/pidns_tree locally
in nslist_proc_show() and pass them to the callees.

Oleg.

RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

> -----Original Message-----
> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Wednesday, October 08, 2014 6:35 PM
> To: Chen, Hanxiao/陈 晗霄; containers@lists.linux-foundation.org;
> linux-kernel@vger.kernel.org
> Cc: Serge Hallyn; Eric W. Biederman; Oleg Nesterov; David Howells; Richard
> Weinberger; Pavel Emelyanov; Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> Am 08.10.2014 11:56, schrieb Chen Hanxiao:
> > This patch will show the hierarchy of pid namespace
> > by /proc/pidns_hierarchy like:
> >
> > [root@localhost ~]#cat /proc/pidns_hierarchy
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> > /proc/1550/ns/pid
> 
> A proc file that prints paths of other proc files, srsly? ;)

Yes, sounds weird  though.

> I didn't follow the whole discussion but why is this not
> a directory containing symbolic links to other pid files in /proc/<PID>/ns/pid?

In the v1 version it’s a directory, and contained symlinks to /proc/<PID>/ns/pid
But we found that is not so easy to use:
a) dirs looks like a snapshot
refreshing it needs a lot of unnecessary codes.

b) dirs did not provide more info than proc file
	What we really need is the <PID>, and we could get it from
	proc file.
	When we read the file, we refresh it at that time.

Thanks,
- Chen

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1556 bytes --]



> -----Original Message-----
> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Wednesday, October 08, 2014 6:35 PM
> To: Chen, Hanxiao/陈 晗霄; containers@lists.linux-foundation.org;
> linux-kernel@vger.kernel.org
> Cc: Serge Hallyn; Eric W. Biederman; Oleg Nesterov; David Howells; Richard
> Weinberger; Pavel Emelyanov; Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> Am 08.10.2014 11:56, schrieb Chen Hanxiao:
> > This patch will show the hierarchy of pid namespace
> > by /proc/pidns_hierarchy like:
> >
> > [root@localhost ~]#cat /proc/pidns_hierarchy
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> > /proc/1550/ns/pid
> 
> A proc file that prints paths of other proc files, srsly? ;)

Yes, sounds weird  though.

> I didn't follow the whole discussion but why is this not
> a directory containing symbolic links to other pid files in /proc/<PID>/ns/pid?

In the v1 version it’s a directory, and contained symlinks to /proc/<PID>/ns/pid
But we found that is not so easy to use:
a) dirs looks like a snapshot
refreshing it needs a lot of unnecessary codes.

b) dirs did not provide more info than proc file
	What we really need is the <PID>, and we could get it from
	proc file.
	When we read the file, we refresh it at that time.

Thanks,
- Chen

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

> -----Original Message-----
> From: Oleg Nesterov [mailto:oleg@redhat.com]
> Sent: Wednesday, October 08, 2014 11:13 PM
> To: Chen, Hanxiao/陈 晗霄
> Cc: containers@lists.linux-foundation.org; linux-kernel@vger.kernel.org; Serge
> Hallyn; Eric W. Biederman; David Howells; Richard Weinberger; Pavel Emelyanov;
> Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> Sorry if this was already discussed, I have to admit that I ignored
> the previous discussion ;) And it is possible I misread this patch
> completely.
> 
> On 10/08, Chen Hanxiao wrote:
> >
> > This patch will show the hierarchy of pid namespace
> > by /proc/pidns_hierarchy like:
> >
> > [root@localhost ~]#cat /proc/pidns_hierarchy
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> > /proc/1550/ns/pid
> 
> Well, personally I too think that the filenames look a bit strange,
> can't it just print the numbers?

Yes, let's print PID numbers only.
> 
> And, iiuc what this patch does... perhaps in this case we should
> simply add "struct list_head children" into struct pid_namespace?
> In this case the patch will be really simple. I dunno.
> 

If we had a children list in pid_namespace,
all we had to do is a iteration from pid 1 of current ns.
That would be nice.

> > +pidns_list_add(struct pid *pid, struct list_head *list_head,
> > +		struct pid_namespace *curr_ns)
> > +{
> > +	struct pidns_list *ent;
> > +	struct pid_namespace *ns;
> > +
> > +	if (is_child_reaper(pid)) {
> > +		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> 
> GFP_KERNEL under rcu_read_lock(). This is not safe without
> CONFIG_PREEMPT_RCU.

It should be GFP_ATOMIC, Matesuz have already pointed out
and I'v changed it in v3.
Sorry for that mistake.

> 
> > +		if (!ent)
> > +			return -ENOMEM;
> > +
> > +		ent->pid = pid;
> > +		ns = pid->numbers[pid->level].ns;
> > +		if (curr_ns) {
> > +			/* add pids who is the child of curr_ns */
> > +			for (; ns != NULL; ns = ns->parent)
> > +				if (ns == curr_ns)
> > +					list_add_tail(&ent->list, list_head);
> 
> afaics, it doesn't make sense to continue after list_add() ?

Oops, we need a break here.

> 
> > +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
> > +{
> > +	struct pid *pid;
> > +	struct task_struct *p;
> > +	int rc;
> > +
> > +	/* collect pid in differet ns */
> > +	for_each_process(p) {
> 
> Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> makes more sense?

Only tasks from our ns is valid.
But how could find_ge_pid() do that?

nr = 1;
while (nr < PID_MAX_LIMIT) {
	find_ge_pid(nr, curr_ns);
	list_add();
	nr++;
}
Perhaps that's not a good way.

> 
> > +		pid = task_pid(p);
> 
> Well, in theory you need barrier() here. Or perhaps we should add
> ACCESS_ONCE() into task_pid()...

You mean modify task_pid as:
return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);

> 
> And imho it would be better to declare pidns_list/pidns_tree locally
> in nslist_proc_show() and pass them to the callees.

That's a good idea.
Will changed in the next version.

Thanks,
- Chen

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3304 bytes --]



> -----Original Message-----
> From: Oleg Nesterov [mailto:oleg@redhat.com]
> Sent: Wednesday, October 08, 2014 11:13 PM
> To: Chen, Hanxiao/³Â êÏÏö
> Cc: containers@lists.linux-foundation.org; linux-kernel@vger.kernel.org; Serge
> Hallyn; Eric W. Biederman; David Howells; Richard Weinberger; Pavel Emelyanov;
> Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> Sorry if this was already discussed, I have to admit that I ignored
> the previous discussion ;) And it is possible I misread this patch
> completely.
> 
> On 10/08, Chen Hanxiao wrote:
> >
> > This patch will show the hierarchy of pid namespace
> > by /proc/pidns_hierarchy like:
> >
> > [root@localhost ~]#cat /proc/pidns_hierarchy
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1534/ns/pid
> > /proc/18060/ns/pid /proc/18102/ns/pid /proc/1600/ns/pid
> > /proc/1550/ns/pid
> 
> Well, personally I too think that the filenames look a bit strange,
> can't it just print the numbers?

Yes, let's print PID numbers only.
> 
> And, iiuc what this patch does... perhaps in this case we should
> simply add "struct list_head children" into struct pid_namespace?
> In this case the patch will be really simple. I dunno.
> 

If we had a children list in pid_namespace,
all we had to do is a iteration from pid 1 of current ns.
That would be nice.

> > +pidns_list_add(struct pid *pid, struct list_head *list_head,
> > +		struct pid_namespace *curr_ns)
> > +{
> > +	struct pidns_list *ent;
> > +	struct pid_namespace *ns;
> > +
> > +	if (is_child_reaper(pid)) {
> > +		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> 
> GFP_KERNEL under rcu_read_lock(). This is not safe without
> CONFIG_PREEMPT_RCU.

It should be GFP_ATOMIC, Matesuz have already pointed out
and I'v changed it in v3.
Sorry for that mistake.

> 
> > +		if (!ent)
> > +			return -ENOMEM;
> > +
> > +		ent->pid = pid;
> > +		ns = pid->numbers[pid->level].ns;
> > +		if (curr_ns) {
> > +			/* add pids who is the child of curr_ns */
> > +			for (; ns != NULL; ns = ns->parent)
> > +				if (ns == curr_ns)
> > +					list_add_tail(&ent->list, list_head);
> 
> afaics, it doesn't make sense to continue after list_add() ?

Oops, we need a break here.

> 
> > +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns)
> > +{
> > +	struct pid *pid;
> > +	struct task_struct *p;
> > +	int rc;
> > +
> > +	/* collect pid in differet ns */
> > +	for_each_process(p) {
> 
> Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> makes more sense?

Only tasks from our ns is valid.
But how could find_ge_pid() do that?

nr = 1;
while (nr < PID_MAX_LIMIT) {
	find_ge_pid(nr, curr_ns);
	list_add();
	nr++;
}
Perhaps that's not a good way.

> 
> > +		pid = task_pid(p);
> 
> Well, in theory you need barrier() here. Or perhaps we should add
> ACCESS_ONCE() into task_pid()...

You mean modify task_pid as:
return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);

> 
> And imho it would be better to declare pidns_list/pidns_tree locally
> in nslist_proc_show() and pass them to the callees.

That's a good idea.
Will changed in the next version.

Thanks,
- Chen

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Oleg Nesterov]

On 10/09, Chen, Hanxiao wrote:
>
> > From: Oleg Nesterov [mailto:oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> >
> > Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> > makes more sense?
>
> Only tasks from our ns is valid.
> But how could find_ge_pid() do that?
>
> nr = 1;
> while (nr < PID_MAX_LIMIT) {
> 	find_ge_pid(nr, curr_ns);
> 	list_add();
> 	nr++;
> }

something like this, except list_add() should obviously depend on
is_child_reaper() check.

This can be more optimal in sub-namespaces, you do not need to abuse
the global process list.

And if you change this code to use get_pid/put_pid, then you do not
need to hold rcu_read_lock() throughout, you only need it around
find_ge_pid + get_pid.

At the same time, for_each_process() in the global namespace can be
faster if there are a lot of sub-threads.

> Perhaps that's not a good way.

OK, I won't insist.

although it would be nice to know why do you think this is bad.

> > > +		pid = task_pid(p);
> >
> > Well, in theory you need barrier() here. Or perhaps we should add
> > ACCESS_ONCE() into task_pid()...
>
> You mean modify task_pid as:
> return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);

Yes. But not now an not in this patch of course. I'd suggest to add
barrier() just in case.


> > And imho it would be better to declare pidns_list/pidns_tree locally
> > in nslist_proc_show() and pass them to the callees.
>
> That's a good idea.
> Will changed in the next version.

Good. And I forgot to mention, in this case you do not need pidns_list_lock
at all afaics.

Oleg.

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Oleg Nesterov]

On 10/09, Chen, Hanxiao wrote:
>
> > From: Oleg Nesterov [mailto:oleg@redhat.com]
> >
> > Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> > makes more sense?
>
> Only tasks from our ns is valid.
> But how could find_ge_pid() do that?
>
> nr = 1;
> while (nr < PID_MAX_LIMIT) {
> 	find_ge_pid(nr, curr_ns);
> 	list_add();
> 	nr++;
> }

something like this, except list_add() should obviously depend on
is_child_reaper() check.

This can be more optimal in sub-namespaces, you do not need to abuse
the global process list.

And if you change this code to use get_pid/put_pid, then you do not
need to hold rcu_read_lock() throughout, you only need it around
find_ge_pid + get_pid.

At the same time, for_each_process() in the global namespace can be
faster if there are a lot of sub-threads.

> Perhaps that's not a good way.

OK, I won't insist.

although it would be nice to know why do you think this is bad.

> > > +		pid = task_pid(p);
> >
> > Well, in theory you need barrier() here. Or perhaps we should add
> > ACCESS_ONCE() into task_pid()...
>
> You mean modify task_pid as:
> return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);

Yes. But not now an not in this patch of course. I'd suggest to add
barrier() just in case.


> > And imho it would be better to declare pidns_list/pidns_tree locally
> > in nslist_proc_show() and pass them to the callees.
>
> That's a good idea.
> Will changed in the next version.

Good. And I forgot to mention, in this case you do not need pidns_list_lock
at all afaics.

Oleg.

RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

> -----Original Message-----
> From: Oleg Nesterov [mailto:oleg@redhat.com]
> Sent: Friday, October 10, 2014 5:34 AM
> To: Chen, Hanxiao/陈 晗霄
> Cc: containers@lists.linux-foundation.org; linux-kernel@vger.kernel.org; Serge
> Hallyn; Eric W. Biederman; David Howells; Richard Weinberger; Pavel Emelyanov;
> Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> On 10/09, Chen, Hanxiao wrote:
> >
> > > From: Oleg Nesterov [mailto:oleg@redhat.com]
> > >
> > > Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> > > makes more sense?
> >
> > Only tasks from our ns is valid.
> > But how could find_ge_pid() do that?
> >
> > nr = 1;
> > while (nr < PID_MAX_LIMIT) {
> > 	find_ge_pid(nr, curr_ns);
> > 	list_add();
> > 	nr++;
> > }
> 
> something like this, except list_add() should obviously depend on
> is_child_reaper() check.
> 
> This can be more optimal in sub-namespaces, you do not need to abuse
> the global process list.
> 
> And if you change this code to use get_pid/put_pid, then you do not
> need to hold rcu_read_lock() throughout, you only need it around
> find_ge_pid + get_pid.
> 
> At the same time, for_each_process() in the global namespace can be
> faster if there are a lot of sub-threads.
> 
> > Perhaps that's not a good way.
> 
> OK, I won't insist.
> 
> although it would be nice to know why do you think this is bad.
> 
I worried about it may slower in global namespace.
But it will provide a great convenient way when query pid hierarchy
when not in init_pid_ns.


> > > > +		pid = task_pid(p);
> > >
> > > Well, in theory you need barrier() here. Or perhaps we should add
> > > ACCESS_ONCE() into task_pid()...
> >
> > You mean modify task_pid as:
> > return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);
> 
> Yes. But not now an not in this patch of course. I'd suggest to add
> barrier() just in case.
> 
We can get rid of task_pid when we use find_ge_pid.
> 
> > > And imho it would be better to declare pidns_list/pidns_tree locally
> > > in nslist_proc_show() and pass them to the callees.
> >
> > That's a good idea.
> > Will changed in the next version.
> 
> Good. And I forgot to mention, in this case you do not need pidns_list_lock
> at all afaics.

Thanks for your comments.
I'll post a new patch using find_ge_pid + get_pid 

Thanks,
- Chen

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

RE: [PATCHv4] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2514 bytes --]



> -----Original Message-----
> From: Oleg Nesterov [mailto:oleg@redhat.com]
> Sent: Friday, October 10, 2014 5:34 AM
> To: Chen, Hanxiao/³Â êÏÏö
> Cc: containers@lists.linux-foundation.org; linux-kernel@vger.kernel.org; Serge
> Hallyn; Eric W. Biederman; David Howells; Richard Weinberger; Pavel Emelyanov;
> Vasiliy Kulikov; Mateusz Guzik
> Subject: Re: [PATCHv4] procfs: show hierarchy of pid namespace
> 
> On 10/09, Chen, Hanxiao wrote:
> >
> > > From: Oleg Nesterov [mailto:oleg@redhat.com]
> > >
> > > Hmm. We only want the tasks from our namespace, yes? Perhaps find_ge_pid()
> > > makes more sense?
> >
> > Only tasks from our ns is valid.
> > But how could find_ge_pid() do that?
> >
> > nr = 1;
> > while (nr < PID_MAX_LIMIT) {
> > 	find_ge_pid(nr, curr_ns);
> > 	list_add();
> > 	nr++;
> > }
> 
> something like this, except list_add() should obviously depend on
> is_child_reaper() check.
> 
> This can be more optimal in sub-namespaces, you do not need to abuse
> the global process list.
> 
> And if you change this code to use get_pid/put_pid, then you do not
> need to hold rcu_read_lock() throughout, you only need it around
> find_ge_pid + get_pid.
> 
> At the same time, for_each_process() in the global namespace can be
> faster if there are a lot of sub-threads.
> 
> > Perhaps that's not a good way.
> 
> OK, I won't insist.
> 
> although it would be nice to know why do you think this is bad.
> 
I worried about it may slower in global namespace.
But it will provide a great convenient way when query pid hierarchy
when not in init_pid_ns.


> > > > +		pid = task_pid(p);
> > >
> > > Well, in theory you need barrier() here. Or perhaps we should add
> > > ACCESS_ONCE() into task_pid()...
> >
> > You mean modify task_pid as:
> > return ACCESS_ONCE(task->pids[PIDTYPE_PID].pid;);
> 
> Yes. But not now an not in this patch of course. I'd suggest to add
> barrier() just in case.
> 
We can get rid of task_pid when we use find_ge_pid.
> 
> > > And imho it would be better to declare pidns_list/pidns_tree locally
> > > in nslist_proc_show() and pass them to the callees.
> >
> > That's a good idea.
> > Will changed in the next version.
> 
> Good. And I forgot to mention, in this case you do not need pidns_list_lock
> at all afaics.

Thanks for your comments.
I'll post a new patch using find_ge_pid + get_pid 

Thanks,
- Chen

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Oleg Nesterov]

On 10/13, Chen, Hanxiao wrote:
>
> > At the same time, for_each_process() in the global namespace can be
> > faster if there are a lot of sub-threads.
> >
> > > Perhaps that's not a good way.
> >
> > OK, I won't insist.
> >
> > although it would be nice to know why do you think this is bad.
> >
> I worried about it may slower in global namespace.
> But it will provide a great convenient way when query pid hierarchy
> when not in init_pid_ns.

Yes, it is not clear what is actually better, so ...

> I'll post a new patch using find_ge_pid + get_pid

Only if you think this makes more sense. I do not have a strong opinion.

And we can change the implementation at any time. The real problem is
the API this patch adds. I leave this to you and other reviewers who
understand the problem space better ;) Just I think that the changelog
could say a bit more, to explain why do we need this patch. IOW, explain
the problem and how/why this patch helps.

Oleg.

Re: [PATCHv4] procfs: show hierarchy of pid namespace

[Oleg Nesterov]

On 10/13, Chen, Hanxiao wrote:
>
> > At the same time, for_each_process() in the global namespace can be
> > faster if there are a lot of sub-threads.
> >
> > > Perhaps that's not a good way.
> >
> > OK, I won't insist.
> >
> > although it would be nice to know why do you think this is bad.
> >
> I worried about it may slower in global namespace.
> But it will provide a great convenient way when query pid hierarchy
> when not in init_pid_ns.

Yes, it is not clear what is actually better, so ...

> I'll post a new patch using find_ge_pid + get_pid

Only if you think this makes more sense. I do not have a strong opinion.

And we can change the implementation at any time. The real problem is
the API this patch adds. I leave this to you and other reviewers who
understand the problem space better ;) Just I think that the changelog
could say a bit more, to explain why do we need this patch. IOW, explain
the problem and how/why this patch helps.

Oleg.