Hi Rik,
On 10/02, Rik van Riel wrote:
Gargi and I are looking at that code, and trying to figure out
exactly what needs to be done to make all of this correct.
see another email I sent to Gargi a minute ago,
2) With pid_ns_prepare_proc out of the way, we can put all the code from below where the call to pid_ns_prepare_proc is now (except error handing) into the main loop of pid allocation, so we can do all that stuff under the pidmap_lock:
for (i = ns->level; i >= 0; i--) {
...
idr_alloc_cyclic(...)
get_pid_ns(ns);
atomic_set(&pid->count, 1);
for (...)
INIT_HLIST_HEAD(...) ns->nr_allocated++;
...
}
I do not see how this can fix the problem with not-fully-initialized
pid returned by find_pid_ns().
As for PIDNS_ADDING/PIDNS_HASH_ADDING, _perhaps_ we can cleanup this
logic
a bit and do the check earlier, but imo this needs another/separate
change.
I'd suggest to keep the current logic and the order of initialization
and
just do
for (i = ns->level; i >= 0; i--) {
...
// do not expose the new pid to find_pid_ns() until it
// is fully initialized
nr = idr_alloc_cyclic(&tmp->idr, /*pid*/ NULL, ...);
...
}
...
spin_lock_irq(&pidmap_lock);
if (!(ns->nr_hashed & PIDNS_HASH_ADDING))
goto out_unlock;
for ( ; upid >= pid->numbers; --upid) {
- hlist_add_head_rcu(&upid->pid_chain,
- &pid_hash[pid_hashfn(upid->nr, upid-
ns)]);+ // finally make it visible to find_pid_ns()
+ idr_replace(upid->ns-idr, pid, upid->nr);
upid->ns->nr_hashed++;
}
spin_unlock_irq(&pidmap_lock);
Or I missed something?
On Mon, 2017-10-02 at 17:21 +0200, Oleg Nesterov wrote:
Hi Rik,
On 10/02, Rik van Riel wrote:
Gargi and I are looking at that code, and trying to figure out
exactly what needs to be done to make all of this correct.
see another email I sent to Gargi a minute ago,
2) With pid_ns_prepare_proc out of the way, we can put all the code
from below where the call to pid_ns_prepare_proc is now (except
error handing) into the main loop of pid allocation, so we can
do all that stuff under the pidmap_lock:
for (i = ns->level; i >= 0; i--) {
...
idr_alloc_cyclic(...)
get_pid_ns(ns);
atomic_set(&pid->count, 1);
for (...)
INIT_HLIST_HEAD(...)
ns->nr_allocated++;
...
}
I do not see how this can fix the problem with not-fully-initialized
pid returned by find_pid_ns().
As for PIDNS_ADDING/PIDNS_HASH_ADDING, _perhaps_ we can cleanup this
logic
a bit and do the check earlier, but imo this needs another/separate
change.
I'd suggest to keep the current logic and the order of initialization
and
just do
for (i = ns->level; i >= 0; i--) {
...
// do not expose the new pid to find_pid_ns() until it
// is fully initialized
nr = idr_alloc_cyclic(&tmp->idr, /*pid*/ NULL, ...);
...
}
...
spin_lock_irq(&pidmap_lock);
if (!(ns->nr_hashed & PIDNS_HASH_ADDING))
goto out_unlock;
for ( ; upid >= pid->numbers; --upid) {
- hlist_add_head_rcu(&upid->pid_chain,
- &pid_hash[pid_hashfn(upid->nr, upid-
ns)]);+ // finally make it visible to find_pid_ns()
+ idr_replace(upid->ns-idr, pid, upid->nr);
upid->ns->nr_hashed++;
}
spin_unlock_irq(&pidmap_lock);
Or I missed something?
You are right, that would both fix the problem, and keep the errorI understand better now. Thanks!
paths relatively simple.
Gargi, what do you think?
--
All Rights Reversed.
Sysop: | Keyop |
---|---|
Location: | Huddersfield, West Yorkshire, UK |
Users: | 285 |
Nodes: | 16 (2 / 14) |
Uptime: | 70:52:53 |
Calls: | 6,488 |
Calls today: | 1 |
Files: | 12,096 |
Messages: | 5,275,625 |