Messages in this thread | | | From | (Eric W. Biederman) | Date | Wed, 27 Sep 2017 12:18:08 -0500 | Subject | Re: [PATCH v2 0/2] Replace PID bitmap allocation with IDR API |
| |
Gargi Sharma <gs051095@gmail.com> 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(-)
| |