Re: [PATCH 1/4] pid: Replace pid bitmap implementation with IDR API
On Mon, Sep 25, 2017 at 8:50 PM, Oleg Nesterov wrote:
> On 09/25, Gargi Sharma wrote:
>>
>> @@ -285,10 +145,14 @@ void free_pid(struct pid *pid)
>> break;
>> }
>> }
>> - spin_unlock_irqrestore(&pidmap_lock, flags);
>>
>> - for (i = 0; i <= pid->level; i++)
>> - free_pidmap(pid->numbers + i);
>> + for (i = 0; i <= pid->level; i++) {
>> + struct upid *upid = pid->numbers + i;
>> + struct pid_namespace *ns = upid->ns;
>> +
>> + idr_remove(&ns->idr, upid->nr);
>> + }
>> + spin_unlock_irqrestore(&pidmap_lock, flags);
>
> Now that you moved the "free pidmap" code under pidmap_lock, we do not
> need 2 "for (i = 0; i <= pid->level; i++)" loops, you could simply add
> a single
>
> idr_remove(&ns->idr, upid->nr);
>
> line into the 1st loop ?
Yes, I can do that. Will make this change.
Thanks!
Gargi
>
> Oleg.
>
Re: [PATCH 1/4] pid: Replace pid bitmap implementation with IDR API
On Mon, Sep 25, 2017 at 8:29 PM, Oleg Nesterov wrote:
> On 09/25, Gargi Sharma wrote:
>>
>> void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>> {
>> - int nr;
>> + int nr = 2;
>> int rc;
>> struct task_struct *task, *me = current;
>> int init_pids = thread_group_leader(me) ? 1 : 2;
>> + struct pid *pid;
>>
>> /* Don't allow any more processes into the pid namespace */
>> disable_pid_allocation(pid_ns);
>> @@ -240,8 +230,8 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>>*
>>*/
>> read_lock(&tasklist_lock);
>> - nr = next_pidmap(pid_ns, 1);
>> - while (nr > 0) {
>> + pid = idr_get_next(&pid_ns->idr, &nr);
>> + while (pid) {
>> rcu_read_lock();
>>
>> task = pid_task(find_vpid(nr), PIDTYPE_PID);
>> @@ -250,7 +240,8 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>>
>> rcu_read_unlock();
>>
>> - nr = next_pidmap(pid_ns, nr);
>> + nr++;
>> + pid = idr_get_next(&pid_ns->idr, &nr);
>> }
>> read_unlock(&tasklist_lock);
>
> Then you should probably rewrite this code using
> idr_for_each_entry_continue() ?
Yes, I missed this macro in the idr library.
>
> And why do you need find_vpid(nr) if you already have "pid" ?
Thanks for the feedback! Yes, it is not needed anymore and can be removed.
Best,
Gargi
>
> Oleg.
>
Re: [PATCH 1/4] pid: Replace pid bitmap implementation with IDR API
On 09/25, Gargi Sharma wrote:
>
> @@ -285,10 +145,14 @@ void free_pid(struct pid *pid)
> break;
> }
> }
> - spin_unlock_irqrestore(&pidmap_lock, flags);
>
> - for (i = 0; i <= pid->level; i++)
> - free_pidmap(pid->numbers + i);
> + for (i = 0; i <= pid->level; i++) {
> + struct upid *upid = pid->numbers + i;
> + struct pid_namespace *ns = upid->ns;
> +
> + idr_remove(&ns->idr, upid->nr);
> + }
> + spin_unlock_irqrestore(&pidmap_lock, flags);
Now that you moved the "free pidmap" code under pidmap_lock, we do not
need 2 "for (i = 0; i <= pid->level; i++)" loops, you could simply add
a single
idr_remove(&ns->idr, upid->nr);
line into the 1st loop ?
Oleg.
Re: [PATCH 1/4] pid: Replace pid bitmap implementation with IDR API
On 09/25, Gargi Sharma wrote:
>
> void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> {
> - int nr;
> + int nr = 2;
> int rc;
> struct task_struct *task, *me = current;
> int init_pids = thread_group_leader(me) ? 1 : 2;
> + struct pid *pid;
>
> /* Don't allow any more processes into the pid namespace */
> disable_pid_allocation(pid_ns);
> @@ -240,8 +230,8 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>*
>*/
> read_lock(&tasklist_lock);
> - nr = next_pidmap(pid_ns, 1);
> - while (nr > 0) {
> + pid = idr_get_next(&pid_ns->idr, &nr);
> + while (pid) {
> rcu_read_lock();
>
> task = pid_task(find_vpid(nr), PIDTYPE_PID);
> @@ -250,7 +240,8 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
>
> rcu_read_unlock();
>
> - nr = next_pidmap(pid_ns, nr);
> + nr++;
> + pid = idr_get_next(&pid_ns->idr, &nr);
> }
> read_unlock(&tasklist_lock);
Then you should probably rewrite this code using idr_for_each_entry_continue() ?
And why do you need find_vpid(nr) if you already have "pid" ?
Oleg.
Re: [PATCH 1/4] pid: Replace pid bitmap implementation with IDR API
On Mon, 2017-09-25 at 08:56 -0400, Gargi Sharma wrote: > This patch replaces the current bitmap implemetation for > Process ID allocation. Functions that are no longer required, > for example, free_pidmap(), alloc_pidmap(), etc. are removed. > The rest of the functions are modified to use the IDR API. > The change was made to make the PID allocation less complex by > replacing custom code with calls to generic API. Nice work, Gargi! > Signed-off-by: Gargi Sharma > --- a/arch/powerpc/platforms/cell/spufs/sched.c > +++ b/arch/powerpc/platforms/cell/spufs/sched.c > @@ -1093,7 +1093,7 @@ static int show_spu_loadavg(struct seq_file *s, > void *private) > LOAD_INT(c), LOAD_FRAC(c), > count_active_contexts(), > atomic_read(&nr_spu_contexts), > - task_active_pid_ns(current)->last_pid); > + task_active_pid_ns(current)->idr.idr_next-1); > return 0; > } The repeated use of "idr.idr_next - 1" suggests that maybe this could be hidden behind a new IDR API, but that could be something for a follow-up patch. There already is the idr_get_cursor function, but you would still have to subtract 1 from the value returned by idr_get_cursor... Other than that nitpick, this patch looks good to me. Reviewed-by: Rik van Riel kind regards, Rik van Riel -- All Rights Reversed. signature.asc Description: This is a digitally signed message part
[PATCH 1/4] pid: Replace pid bitmap implementation with IDR API
This patch replaces the current bitmap implemetation for
Process ID allocation. Functions that are no longer required,
for example, free_pidmap(), alloc_pidmap(), etc. are removed.
The rest of the functions are modified to use the IDR API.
The change was made to make the PID allocation less complex by
replacing custom code with calls to generic API.
Signed-off-by: Gargi Sharma
---
arch/powerpc/platforms/cell/spufs/sched.c | 2 +-
fs/proc/loadavg.c | 2 +-
include/linux/pid_namespace.h | 14 +--
init/main.c | 2 +-
kernel/pid.c | 198 ++
kernel/pid_namespace.c| 39 +++---
6 files changed, 56 insertions(+), 201 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/sched.c
b/arch/powerpc/platforms/cell/spufs/sched.c
index 1fbb5da..d285aef 100644
--- a/arch/powerpc/platforms/cell/spufs/sched.c
+++ b/arch/powerpc/platforms/cell/spufs/sched.c
@@ -1093,7 +1093,7 @@ static int show_spu_loadavg(struct seq_file *s, void
*private)
LOAD_INT(c), LOAD_FRAC(c),
count_active_contexts(),
atomic_read(&nr_spu_contexts),
- task_active_pid_ns(current)->last_pid);
+ task_active_pid_ns(current)->idr.idr_next-1);
return 0;
}
diff --git a/fs/proc/loadavg.c b/fs/proc/loadavg.c
index 983fce5..9355f4d 100644
--- a/fs/proc/loadavg.c
+++ b/fs/proc/loadavg.c
@@ -23,7 +23,7 @@ static int loadavg_proc_show(struct seq_file *m, void *v)
LOAD_INT(avnrun[1]), LOAD_FRAC(avnrun[1]),
LOAD_INT(avnrun[2]), LOAD_FRAC(avnrun[2]),
nr_running(), nr_threads,
- task_active_pid_ns(current)->last_pid);
+ task_active_pid_ns(current)->idr.idr_next-1);
return 0;
}
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index b09136f..f4db4a7 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -9,15 +9,8 @@
#include
#include
#include
+#include
-struct pidmap {
- atomic_t nr_free;
- void *page;
-};
-
-#define BITS_PER_PAGE (PAGE_SIZE * 8)
-#define BITS_PER_PAGE_MASK (BITS_PER_PAGE-1)
-#define PIDMAP_ENTRIES ((PID_MAX_LIMIT+BITS_PER_PAGE-1)/BITS_PER_PAGE)
struct fs_pin;
@@ -29,9 +22,8 @@ enum { /* definitions for pid_namespace's hide_pid field */
struct pid_namespace {
struct kref kref;
- struct pidmap pidmap[PIDMAP_ENTRIES];
+ struct idr idr;
struct rcu_head rcu;
- int last_pid;
unsigned int nr_hashed;
struct task_struct *child_reaper;
struct kmem_cache *pid_cachep;
@@ -105,6 +97,6 @@ static inline int reboot_pid_ns(struct pid_namespace
*pid_ns, int cmd)
extern struct pid_namespace *task_active_pid_ns(struct task_struct *tsk);
void pidhash_init(void);
-void pidmap_init(void);
+void pid_idr_init(void);
#endif /* _LINUX_PID_NS_H */
diff --git a/init/main.c b/init/main.c
index 0ee9c686..9f4db20 100644
--- a/init/main.c
+++ b/init/main.c
@@ -667,7 +667,7 @@ asmlinkage __visible void __init start_kernel(void)
if (late_time_init)
late_time_init();
calibrate_delay();
- pidmap_init();
+ pid_idr_init();
anon_vma_init();
acpi_early_init();
#ifdef CONFIG_X86
diff --git a/kernel/pid.c b/kernel/pid.c
index 020dedb..ea22e89 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#define pid_hashfn(nr, ns) \
hash_long((unsigned long)nr + (unsigned long)ns, pidhash_shift)
@@ -53,12 +54,6 @@ int pid_max = PID_MAX_DEFAULT;
int pid_max_min = RESERVED_PIDS + 1;
int pid_max_max = PID_MAX_LIMIT;
-static inline int mk_pid(struct pid_namespace *pid_ns,
- struct pidmap *map, int off)
-{
- return (map - pid_ns->pidmap)*BITS_PER_PAGE + off;
-}
-
#define find_next_offset(map, off) \
find_next_zero_bit((map)->page, BITS_PER_PAGE, off)
@@ -70,10 +65,7 @@ static inline int mk_pid(struct pid_namespace *pid_ns,
*/
struct pid_namespace init_pid_ns = {
.kref = KREF_INIT(2),
- .pidmap = {
- [ 0 ... PIDMAP_ENTRIES-1] = { ATOMIC_INIT(BITS_PER_PAGE), NULL }
- },
- .last_pid = 0,
+ .idr = IDR_INIT,
.nr_hashed = PIDNS_HASH_ADDING,
.level = 0,
.child_reaper = &init_task,
@@ -101,138 +93,6 @@ EXPORT_SYMBOL_GPL(init_pid_ns);
static __cacheline_aligned_in_smp DEFINE_SPINLOCK(pidmap_lock);
-static void free_pidmap(struct upid *upid)
-{
- int nr = upid->nr;
- struct pidmap *map = upid->ns->pidmap + nr / BITS_PER_PAGE;
- int offset = nr & BITS_PER_PAGE_MASK;
-
- clear_bit(offset, map->page);
- atomic_inc(&map->nr_free);
-}
-
-/*
- * If we started walking pids at 'base', is 'a' seen before 'b'?
- */
-static
