RE: [PATCHv3 1/2] procfs: show hierarchy of pid namespace

Chen, Hanxiao
2014-09-25T09:45:47Z

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

Hi,

> -----Original Message-----
> From: Mateusz Guzik [mailto:mguzik@redhat.com]
> Sent: Thursday, September 25, 2014 1:45 AM
> On Wed, Sep 24, 2014 at 06:00:26PM +0800, Chen Hanxiao wrote:
> > +static int
> > +pidns_list_filter(void)
> > +{
> > + struct pidns_list *pos, *pos_t;
> > + struct pid_namespace *ns0, *ns1;
> > + struct pid *pid0, *pid1;
> > + int flag = 0;
> > + int rc;
> > +
> > + /* 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) {
>
> In the previous thread I tried to note this will be terribly inefficient
> to use and adding a list of children to pid_namespace struct would deal
> with the problem.

If that, we had to add a children list, maybe another sibling list
in pid_namespace struct and maintain them.
That cost too much.

For this feature, I think we'd better not touch pid_namespace struct.
As we need not to know the hierarchy so frequently,
I think such kind of inefficient is acceptable.

>
> > + 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);
> > + if (rc)
> > + goto out;
> > + }
> > + }
> > +
> > + /* Now all usefull stuff 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(void)
> > +{
> > + struct pid *pid;
> > + struct task_struct *p;
> > + int rc;
> > +
> > + /* collect pid in differet ns */
> > + rcu_read_lock();
> > + for_each_process(p) {
> > + pid = task_pid(p);
> > + if (pid && (pid->level > 0)) {
> > + rc = pidns_list_add(pid, &pidns_list);
> > + 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;
> > + rcu_read_unlock();
> > +
> > + return 0;
> > +
> > +out:
> > + free_pidns_list(&pidns_list);
> > + rcu_read_unlock();
> > + 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);
> > + rc = proc_pidns_list_refresh();
> > + if (rc) {
> > + mutex_unlock(&pidns_list_lock);
> > + return rc;
> > + }
> > +
> > + /* print pid namespace hierarchy */
> > + list_for_each_entry(pos, &pidns_tree, list) {
>
> What keeps pid_namespace's safe to use? Similarly to previous patch,
> here we hit a place where the code is not protected with rcu and
> structures were just plugged into the list.
>
Will fix.
All list should be protected by rcu lock.

> Recreating the list for each open seems quite unnecessary as well.
>
> One could work around that by caching generated output and having a
> generation counter for namespaces to know whether the content is stale.
> But that still does not seem right.
>
That will bring another issue:
We had to *keep* that list and update it
even if we don't open pidns_hierarchy.

This patch try to solve this when open /proc/pidns_hierarchy by:
a) recreated the list when open
b) show it in a procfs text file
c) drop that list

> It looks like in the original thread someone suggested hooking this up
> under proc as a directory tree which sounds much better to me.

Dir trees provide the same information as proc text files did:
a symlink name like /proc/PID/ns/pid.

Refresh dir trees needs a lot of codes too.
So a procfs text file is a better choice.

Thanks,
- Chen

>
> Just my $0,03.
>
> > + 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);
> > + mutex_unlock(&pidns_list_lock);
> > +
> > + return 0;
> > +}
>
> --
> Mateusz Guzik
ÿôèº{.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¥