Re: [PATCH v2 0/2] Replace PID bitmap allocation with IDR API

From: Eric W. Biederman
Date: Wed Sep 27 2017 - 13:18:23 EST


Gargi Sharma <gs051095@xxxxxxxxx> writes:

> This patch series replaces kernel bitmap implementation of PID allocation
> with IDR API. These patches are written to simplify the kernel by replacing custom code with calls to generic code.
>
> The following are the stats for pid and pid_namespace object files
> before and after the replacement. There is a noteworthy change between
> the IDR and bitmap implementation.
>
> Before
> text data bss dec hex filename
> 8447 3894 64 12405 3075 kernel/pid.o
> After
> text data bss dec hex filename
> 3301 304 0 3605 e15 kernel/pid.o
>
> Before
> text data bss dec hex filename
> 5692 1842 192 7726 1e2e kernel/pid_namespace.o
> After
> text data bss dec hex filename
> 2870 216 16 3102 c1e kernel/pid_namespace.o
>
> There wasn't a considerable difference between the time required for
> allocation of PIDs to the processes.

How about the time to call readdir on /proc?

How many processes were you testing with?

Why just the bitmap? Why not update the hash table as well.
An rbtree or an rhashtable might be better.

Hmm. Oh look there is patch 2/2 and you do replace the hashtable with
the idr as well. Now I am very interested in a comparison of data
structures.

How does the runtime memory footprint of your new pid hash
implementation compare to the old memory footprint?

There are a lot of options in this space and it does not sound like you
have looked at the options very thoroughly.

I am a little worried that in the quest to reuse code you may have made the
total amount of code executed larger and more susceptible to more cache line
misses.

>From what Oleg has pointed out and from your the holes in your
description I am generally leery of this patchset as the attention to
detail was appears lower than necessary for this to be more than a proof
of concept.

Eric


> ---
> Changes in v2:
> - Removed redundant IDR function that was introduced
> in the previous patchset.
> - Renamed PIDNS_HASH_ADDING
> - Used idr_for_each_entry_continue()
> - Used idr_find() to lookup pids
>
> Gargi Sharma (2):
> pid: Replace pid bitmap implementation with IDR API
> pid: Remove pidhash
>
> arch/powerpc/platforms/cell/spufs/sched.c | 2 +-
> fs/proc/loadavg.c | 2 +-
> include/linux/init_task.h | 1 -
> include/linux/pid.h | 2 -
> include/linux/pid_namespace.h | 18 +--
> init/main.c | 3 +-
> kernel/fork.c | 2 +-
> kernel/pid.c | 239 +++++-------------------------
> kernel/pid_namespace.c | 54 +++----
> 9 files changed, 68 insertions(+), 255 deletions(-)