From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751193AbdJBNfc (ORCPT ); Mon, 2 Oct 2017 09:35:32 -0400 Received: from shelob.surriel.com ([96.67.55.147]:51034 "EHLO shelob.surriel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751036AbdJBNfb (ORCPT ); Mon, 2 Oct 2017 09:35:31 -0400 Message-ID: <1506951313.21121.119.camel@surriel.com> Subject: Re: [PATCH v2 2/2] pid: Remove pidhash From: Rik van Riel To: Oleg Nesterov , Gargi Sharma Cc: linux-kernel@vger.kernel.org, julia.lawall@lip6.fr, akpm@linux-foundation.org, mingo@kernel.org, pasha.tatashin@oracle.com, ktkhai@virtuozzo.com Date: Mon, 02 Oct 2017 09:35:13 -0400 In-Reply-To: <20170927154543.GA3788@redhat.com> References: <20170927154543.GA3788@redhat.com> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-OH+pNKcFWSUA0l492evz" X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-OH+pNKcFWSUA0l492evz Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-09-27 at 17:45 +0200, Oleg Nesterov wrote: > On 09/27, Gargi Sharma wrote: > >=20 > > -#define find_next_offset(map, off) =09 > > \ > > - find_next_zero_bit((map)->page, BITS_PER_PAGE, > > off) > > - >=20 > this should go into the previous patch, but this is minor... >=20 > > @@ -208,12 +200,10 @@ struct pid *alloc_pid(struct pid_namespace > > *ns) > > =C2=A0 > > =C2=A0 upid =3D pid->numbers + ns->level; > > =C2=A0 spin_lock_irq(&pidmap_lock); > > - if (!(ns->nr_hashed & PIDNS_HASH_ADDING)) > > + if (!(ns->pid_allocated & PIDNS_ADDING)) > > =C2=A0 goto out_unlock; > > =C2=A0 for ( ; upid >=3D pid->numbers; --upid) { > > - hlist_add_head_rcu(&upid->pid_chain, > > - &pid_hash[pid_hashfn(upid->nr, > > upid->ns)]); > > - upid->ns->nr_hashed++; > > + upid->ns->pid_allocated++; >=20 > No, this is wrong. >=20 > It is too late to check PIDNS_HASH_ADDING/PIDNS_ADDING and increment > pid_allocated, > once we call idr_alloc_cyclic() this pid is already "hashed" in that > it can be found > by find_pid_ns() with this patch applied. >=20 > And of course, it is too late to do atomic_set(&pid->count, 1) and > initialize > pid->tasks[type] lists by the same reason. Hi Oleg, 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. We are thinking something along these lines: 1) First, check if this is a new namespace (PIDNS_ADDING), and do the call to pid_ns_prepare_proc, before we even call idr_alloc. Maybe something like: if (unlikely(ns->nr_allocated =3D=3D PIDNS_ADDING)) { if (pid_ns_prepare_proc(ns)) { disable_pid_allocations(ns); goto out_free_ns; } 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 =3D ns->level; i >=3D 0; i--) { ... idr_alloc_cyclic(...) get_pid_ns(ns); atomic_set(&pid->count, 1); for (...) INIT_HLIST_HEAD(...) ns->nr_allocated++; ... } Would that resolve your objection, or are we barking up the wrong tree? --=20 All Rights Reversed. --=-OH+pNKcFWSUA0l492evz Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJZ0kCSAAoJEM553pKExN6DtH0H/13RuHgjmsXRWrk78VQTgkkp 8FHXelhjLPyydeEAndAkil7aFoDxmf3wlnVmenXHSENmj50OtcF9/WBuj91vn0kd XhYTl7QlFU/+7/TJtmppUqyt+0EZMCkDzlOOr0Uc+QGPhPwD8fmJ55UrbdDhTg5j sf2dW1jE/Aiomx0Gs77c3E2Ux+rC4NA8kcp0Bklgymlg0onZWJUOd6DMQzG+fPbV bX18QMkVYAqjcvBijS1vZn/IH736cdhyfXWuR8MqLaZJn5F5f9nHCybKgo+uMGrY CKFpBSKX3TPxFFsoxjiRyRBdurXky1hilnkdjeLG29vatFM/ZRtXA1poRaGri7I= =vUBt -----END PGP SIGNATURE----- --=-OH+pNKcFWSUA0l492evz--