From: ebiederm@xmission.com (Eric W. Biederman)
To: Gargi Sharma <gs051095@gmail.com>
Cc: linux-kernel@vger.kernel.org, riel@surriel.com, julia.lawall@lip6.fr,
        akpm@linux-foundation.org, mingo@kernel.org, pasha.tatashin@oracle.com,
        ktkhai@virtuozzo.com, oleg@redhat.com
References: <cover.1506488388.git.gs051095@gmail.com>
Date: Wed, 27 Sep 2017 12:18:08 -0500
In-Reply-To: <cover.1506488388.git.gs051095@gmail.com> (Gargi Sharma's message
        of "Wed, 27 Sep 2017 01:06:01 -0400")
Message-ID: <87ing4vz5b.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v2 0/2] Replace PID bitmap allocation with IDR API
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3016
Lines: 77

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(-)