* Re: + pid-delete-struct-pidmap-nr_free.patch added to -mm tree
[not found] <59d2c81b.sN3iiDcgxwFHtFRg%akpm@linux-foundation.org>
@ 2017-10-03 16:15 ` Oleg Nesterov
2017-10-03 17:39 ` Gargi Sharma
0 siblings, 1 reply; 3+ messages in thread
From: Oleg Nesterov @ 2017-10-03 16:15 UTC (permalink / raw)
To: akpm
Cc: adobriyan, ebiederm, keescook, mm-commits, linux-kernel,
Gargi Sharma, Rik van Riel
On 10/02, Andrew Morton wrote:
>
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Subject: pid: delete struct pidmap::nr_free
>
> There is a check in pid allocation code to skip a full page:
>
> if (likely(atomic_read(&map->nr_free))) {
> ...
>
> In practice it doesn't do anything. To skip a pidmap page one has to have
> 32K consecutive pids allocated at the same time which doesn't happen.
>
> Currently the price is _every_ fork/exit on every system being slower than
> necessary.
Agreed, I too never understood how can this counter help.
Add Gargi and Rik, the next version of "Replace PID bitmap allocation with IDR
API" can conflict with this and the previous pid-delete-reserved_pids.patch.
Oleg.
>
> Link: http://lkml.kernel.org/r/20170909203649.GB4791@avx2
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> include/linux/pid_namespace.h | 1 -
> kernel/pid.c | 28 ++++++++++------------------
> kernel/pid_namespace.c | 6 ------
> 3 files changed, 10 insertions(+), 25 deletions(-)
>
> diff -puN include/linux/pid_namespace.h~pid-delete-struct-pidmap-nr_free include/linux/pid_namespace.h
> --- a/include/linux/pid_namespace.h~pid-delete-struct-pidmap-nr_free
> +++ a/include/linux/pid_namespace.h
> @@ -11,7 +11,6 @@
> #include <linux/ns_common.h>
>
> struct pidmap {
> - atomic_t nr_free;
> void *page;
> };
>
> diff -puN kernel/pid.c~pid-delete-struct-pidmap-nr_free kernel/pid.c
> --- a/kernel/pid.c~pid-delete-struct-pidmap-nr_free
> +++ a/kernel/pid.c
> @@ -68,9 +68,6 @@ static inline int mk_pid(struct pid_name
> */
> struct pid_namespace init_pid_ns = {
> .kref = KREF_INIT(2),
> - .pidmap = {
> - [ 0 ... PIDMAP_ENTRIES-1] = { ATOMIC_INIT(BITS_PER_PAGE), NULL }
> - },
> .last_pid = 0,
> .nr_hashed = PIDNS_HASH_ADDING,
> .level = 0,
> @@ -106,7 +103,6 @@ static void free_pidmap(struct upid *upi
> int offset = nr & BITS_PER_PAGE_MASK;
>
> clear_bit(offset, map->page);
> - atomic_inc(&map->nr_free);
> }
>
> /*
> @@ -181,20 +177,17 @@ static int alloc_pidmap(struct pid_names
> if (unlikely(!map->page))
> return -ENOMEM;
> }
> - if (likely(atomic_read(&map->nr_free))) {
> - for ( ; ; ) {
> - if (!test_and_set_bit(offset, map->page)) {
> - atomic_dec(&map->nr_free);
> - set_last_pid(pid_ns, last, pid);
> - return pid;
> - }
> - offset = find_next_offset(map, offset);
> - if (offset >= BITS_PER_PAGE)
> - break;
> - pid = mk_pid(pid_ns, map, offset);
> - if (pid >= pid_max)
> - break;
> + for (;;) {
> + if (!test_and_set_bit(offset, map->page)) {
> + set_last_pid(pid_ns, last, pid);
> + return pid;
> }
> + offset = find_next_offset(map, offset);
> + if (offset >= BITS_PER_PAGE)
> + break;
> + pid = mk_pid(pid_ns, map, offset);
> + if (pid >= pid_max)
> + break;
> }
> if (map < &pid_ns->pidmap[(pid_max-1)/BITS_PER_PAGE]) {
> ++map;
> @@ -591,7 +584,6 @@ void __init pidmap_init(void)
> init_pid_ns.pidmap[0].page = kzalloc(PAGE_SIZE, GFP_KERNEL);
> /* Reserve PID 0. We never call free_pidmap(0) */
> set_bit(0, init_pid_ns.pidmap[0].page);
> - atomic_dec(&init_pid_ns.pidmap[0].nr_free);
>
> init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> diff -puN kernel/pid_namespace.c~pid-delete-struct-pidmap-nr_free kernel/pid_namespace.c
> --- a/kernel/pid_namespace.c~pid-delete-struct-pidmap-nr_free
> +++ a/kernel/pid_namespace.c
> @@ -98,7 +98,6 @@ static struct pid_namespace *create_pid_
> struct pid_namespace *ns;
> unsigned int level = parent_pid_ns->level + 1;
> struct ucounts *ucounts;
> - int i;
> int err;
>
> err = -EINVAL;
> @@ -139,11 +138,6 @@ static struct pid_namespace *create_pid_
> INIT_WORK(&ns->proc_work, proc_cleanup_work);
>
> set_bit(0, ns->pidmap[0].page);
> - atomic_set(&ns->pidmap[0].nr_free, BITS_PER_PAGE - 1);
> -
> - for (i = 1; i < PIDMAP_ENTRIES; i++)
> - atomic_set(&ns->pidmap[i].nr_free, BITS_PER_PAGE);
> -
> return ns;
>
> out_free_map:
> _
>
> Patches currently in -mm which might be from adobriyan@gmail.com are
>
> proc-uninline-name_to_int.patch
> proc-use-do-while-in-name_to_int.patch
> seq_file-delete-small-value-optimization.patch
> pid-delete-reserved_pids.patch
> pid-delete-struct-pidmap-nr_free.patch
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: + pid-delete-struct-pidmap-nr_free.patch added to -mm tree
2017-10-03 16:15 ` + pid-delete-struct-pidmap-nr_free.patch added to -mm tree Oleg Nesterov
@ 2017-10-03 17:39 ` Gargi Sharma
2017-10-03 21:56 ` Alexey Dobriyan
0 siblings, 1 reply; 3+ messages in thread
From: Gargi Sharma @ 2017-10-03 17:39 UTC (permalink / raw)
To: Oleg Nesterov
Cc: akpm, adobriyan, Eric W. Biederman, keescook, mm-commits,
linux-kernel, Rik van Riel
On Tue, Oct 3, 2017 at 9:45 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 10/02, Andrew Morton wrote:
>>
>> From: Alexey Dobriyan <adobriyan@gmail.com>
>> Subject: pid: delete struct pidmap::nr_free
>>
>> There is a check in pid allocation code to skip a full page:
>>
>> if (likely(atomic_read(&map->nr_free))) {
>> ...
>>
>> In practice it doesn't do anything. To skip a pidmap page one has to have
>> 32K consecutive pids allocated at the same time which doesn't happen.
>>
>> Currently the price is _every_ fork/exit on every system being slower than
>> necessary.
>
> Agreed, I too never understood how can this counter help.
>
> Add Gargi and Rik, the next version of "Replace PID bitmap allocation with IDR
> API" can conflict with this and the previous pid-delete-reserved_pids.patch.
I think this patch will become obsolete as pidmap will be removed. As
for the 1/2
patch of Alexey's series, I'll incorporate it so that rolled over PIDs
start from 1
instead of RESERVED_PIDS.
Thanks!
Gargi
>
> Oleg.
>
>>
>> Link: http://lkml.kernel.org/r/20170909203649.GB4791@avx2
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>> include/linux/pid_namespace.h | 1 -
>> kernel/pid.c | 28 ++++++++++------------------
>> kernel/pid_namespace.c | 6 ------
>> 3 files changed, 10 insertions(+), 25 deletions(-)
>>
>> diff -puN include/linux/pid_namespace.h~pid-delete-struct-pidmap-nr_free include/linux/pid_namespace.h
>> --- a/include/linux/pid_namespace.h~pid-delete-struct-pidmap-nr_free
>> +++ a/include/linux/pid_namespace.h
>> @@ -11,7 +11,6 @@
>> #include <linux/ns_common.h>
>>
>> struct pidmap {
>> - atomic_t nr_free;
>> void *page;
>> };
>>
>> diff -puN kernel/pid.c~pid-delete-struct-pidmap-nr_free kernel/pid.c
>> --- a/kernel/pid.c~pid-delete-struct-pidmap-nr_free
>> +++ a/kernel/pid.c
>> @@ -68,9 +68,6 @@ static inline int mk_pid(struct pid_name
>> */
>> struct pid_namespace init_pid_ns = {
>> .kref = KREF_INIT(2),
>> - .pidmap = {
>> - [ 0 ... PIDMAP_ENTRIES-1] = { ATOMIC_INIT(BITS_PER_PAGE), NULL }
>> - },
>> .last_pid = 0,
>> .nr_hashed = PIDNS_HASH_ADDING,
>> .level = 0,
>> @@ -106,7 +103,6 @@ static void free_pidmap(struct upid *upi
>> int offset = nr & BITS_PER_PAGE_MASK;
>>
>> clear_bit(offset, map->page);
>> - atomic_inc(&map->nr_free);
>> }
>>
>> /*
>> @@ -181,20 +177,17 @@ static int alloc_pidmap(struct pid_names
>> if (unlikely(!map->page))
>> return -ENOMEM;
>> }
>> - if (likely(atomic_read(&map->nr_free))) {
>> - for ( ; ; ) {
>> - if (!test_and_set_bit(offset, map->page)) {
>> - atomic_dec(&map->nr_free);
>> - set_last_pid(pid_ns, last, pid);
>> - return pid;
>> - }
>> - offset = find_next_offset(map, offset);
>> - if (offset >= BITS_PER_PAGE)
>> - break;
>> - pid = mk_pid(pid_ns, map, offset);
>> - if (pid >= pid_max)
>> - break;
>> + for (;;) {
>> + if (!test_and_set_bit(offset, map->page)) {
>> + set_last_pid(pid_ns, last, pid);
>> + return pid;
>> }
>> + offset = find_next_offset(map, offset);
>> + if (offset >= BITS_PER_PAGE)
>> + break;
>> + pid = mk_pid(pid_ns, map, offset);
>> + if (pid >= pid_max)
>> + break;
>> }
>> if (map < &pid_ns->pidmap[(pid_max-1)/BITS_PER_PAGE]) {
>> ++map;
>> @@ -591,7 +584,6 @@ void __init pidmap_init(void)
>> init_pid_ns.pidmap[0].page = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> /* Reserve PID 0. We never call free_pidmap(0) */
>> set_bit(0, init_pid_ns.pidmap[0].page);
>> - atomic_dec(&init_pid_ns.pidmap[0].nr_free);
>>
>> init_pid_ns.pid_cachep = KMEM_CACHE(pid,
>> SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
>> diff -puN kernel/pid_namespace.c~pid-delete-struct-pidmap-nr_free kernel/pid_namespace.c
>> --- a/kernel/pid_namespace.c~pid-delete-struct-pidmap-nr_free
>> +++ a/kernel/pid_namespace.c
>> @@ -98,7 +98,6 @@ static struct pid_namespace *create_pid_
>> struct pid_namespace *ns;
>> unsigned int level = parent_pid_ns->level + 1;
>> struct ucounts *ucounts;
>> - int i;
>> int err;
>>
>> err = -EINVAL;
>> @@ -139,11 +138,6 @@ static struct pid_namespace *create_pid_
>> INIT_WORK(&ns->proc_work, proc_cleanup_work);
>>
>> set_bit(0, ns->pidmap[0].page);
>> - atomic_set(&ns->pidmap[0].nr_free, BITS_PER_PAGE - 1);
>> -
>> - for (i = 1; i < PIDMAP_ENTRIES; i++)
>> - atomic_set(&ns->pidmap[i].nr_free, BITS_PER_PAGE);
>> -
>> return ns;
>>
>> out_free_map:
>> _
>>
>> Patches currently in -mm which might be from adobriyan@gmail.com are
>>
>> proc-uninline-name_to_int.patch
>> proc-use-do-while-in-name_to_int.patch
>> seq_file-delete-small-value-optimization.patch
>> pid-delete-reserved_pids.patch
>> pid-delete-struct-pidmap-nr_free.patch
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: + pid-delete-struct-pidmap-nr_free.patch added to -mm tree
2017-10-03 17:39 ` Gargi Sharma
@ 2017-10-03 21:56 ` Alexey Dobriyan
0 siblings, 0 replies; 3+ messages in thread
From: Alexey Dobriyan @ 2017-10-03 21:56 UTC (permalink / raw)
To: Gargi Sharma
Cc: Oleg Nesterov, akpm, Eric W. Biederman, keescook, mm-commits,
linux-kernel, Rik van Riel
On Tue, Oct 03, 2017 at 11:09:08PM +0530, Gargi Sharma wrote:
> On Tue, Oct 3, 2017 at 9:45 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 10/02, Andrew Morton wrote:
> >>
> >> From: Alexey Dobriyan <adobriyan@gmail.com>
> >> Subject: pid: delete struct pidmap::nr_free
> >>
> >> There is a check in pid allocation code to skip a full page:
> >>
> >> if (likely(atomic_read(&map->nr_free))) {
> >> ...
> >>
> >> In practice it doesn't do anything. To skip a pidmap page one has to have
> >> 32K consecutive pids allocated at the same time which doesn't happen.
> >>
> >> Currently the price is _every_ fork/exit on every system being slower than
> >> necessary.
> >
> > Agreed, I too never understood how can this counter help.
> >
> > Add Gargi and Rik, the next version of "Replace PID bitmap allocation with IDR
> > API" can conflict with this and the previous pid-delete-reserved_pids.patch.
>
> I think this patch will become obsolete as pidmap will be removed. As
> for the 1/2
> patch of Alexey's series, I'll incorporate it so that rolled over PIDs
^^^^^^^^^^^
> start from 1 instead of RESERVED_PIDS.
I saw IDR pids, lots of code removed indeed.
Incorporation is not really necessary as RESERVED_PIDS is an independent change.
Just send it as 2/2 or whatever the number or rebase on top of -mm.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-03 21:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <59d2c81b.sN3iiDcgxwFHtFRg%akpm@linux-foundation.org>
2017-10-03 16:15 ` + pid-delete-struct-pidmap-nr_free.patch added to -mm tree Oleg Nesterov
2017-10-03 17:39 ` Gargi Sharma
2017-10-03 21:56 ` Alexey Dobriyan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox