[PATCH v5 1/5] PID namespace translation support

[Ákos Uzonyi]

On Wed, 5 Aug 2020 at 00:31, Dmitry V. Levin <ldv at altlinux.org> wrote:
> On Tue, Aug 04, 2020 at 07:52:46PM +0200, Ákos Uzonyi wrote:
> > On Mon, 3 Aug 2020 at 23:43, Dmitry V. Levin <ldv at altlinux.org> wrote:
> > > On Mon, Aug 03, 2020 at 09:19:12PM +0200, Ákos Uzonyi wrote:
> > > [...]
> > > > +void
> > > > +pidns_init(void)
> > > > +{
> > > > +     if (proc_data_cache)
> > > > +             return;
> > > > +
> > > > +     pid_max = INT_MAX;
> > > > +     if (read_int_from_file("/proc/sys/kernel/pid_max", &pid_max) < 0)
> > > > +             debug_func_perror_msg("reading /proc/sys/kernel/pid_max");
> > > > +     pid_max_size = ilog2_32(pid_max - 1) + 1;
> > > > +     pid_max_size_lg = ilog2_32(pid_max_size - 1) + 1;
> > > > +
> > > > +     for (int i = 0; i < PT_COUNT; i++)
> > > > +             ns_pid_to_proc_pid[i] = trie_create(64, ptr_sz_lg, 10, 10, 0);
> > >
> > > Has the numeric constant 64 been chosen because
> > > sizeof(struct tcb.pid_ns) * 8 == 64?
> >
> > Yes.
> > For making it clear in the code, which one would be preferred?
> > - Using a named constant
>
> It's always nice to have a good name for a constant.
>
> > - Using the expression you wrote above
>
> Strictly speaking, that was not a valid C expression, but
>         sizeof_field(struct tcb, pid_ns) * 8
> is a valid one.

As tcb.pid_ns is just one of the variables where ns ids are stored, I
might prefer using sizeof(uint64_t):
#define NS_ID_SIZE sizeof(uint64_t)
or maybe just simply:
#define NS_ID_SIZE 64

> > - Adding a comment about it
> >
> > > > +
> > > > +     proc_data_cache = trie_create(pid_max_size, ptr_sz_lg, 10, 10, 0);
> > > > +}
> > > > +
> > > > +static void
> > > > +put_proc_pid(uint64_t ns, int ns_pid, enum pid_type type, int proc_pid)
> > > > +{
> > > > +     struct trie *b = (struct trie *) (uintptr_t) trie_get(ns_pid_to_proc_pid[type], ns);
> > > > +     if (!b) {
> > > > +             b = trie_create(pid_max_size, pid_max_size_lg, 10, 10, 0);
> > > > +             trie_set(ns_pid_to_proc_pid[type], ns, (uint64_t) (uintptr_t) b);
> > > > +     }
> > > > +     trie_set(b, ns_pid, proc_pid);
> > > > +}
> > >
> > > In all three cases trie_create is invoked with node_key_bits
> > > and data_block_key_bits arguments equal to 10.
> > >
> > > Could you shed some light on the choice of this numeric constant, please?
> >
> > Unfortunately, I have no strong reason why 10 was chosen. It's just a
> > number that seemed to be ok. But thinking a bit more about it, I
> > realized maybe a smaller number would be better. 10 means node size is
> > 8KB (with 8 byte pointers), which seems to be a lot of memory usage,
>
> Should the number depend on the size of pointer then?
> Or should it depend on pid_max?

I think it's easier to use a constant.

> > as PIDs are sparse.
>
> What do you mean by that?

I just meant that usually a few pids are distributed over a large
interval, so it's a waste allocating large nodes, as usually only a
few pointers will be used in them.

> > Maybe for example 4 could be a better choice, so
> > nodes are small, and lookup times are still not that bad. But I'm
> > still not sure what explanation could I write to the chosen constant.
>
> Even if it just feels right, try to explain in the comment why it feels right.

OK, I'll try to come up with something.