Date: Wed, 15 May 2013 22:33:37 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: current@freebsd.org Subject: Re: sysvshm: replace Giant with a local sx lock Message-ID: <20130515193337.GX3047@kib.kiev.ua> In-Reply-To: <20130514201305.GA15195@dft-labs.eu> References: <20130423203823.GA6346@dft-labs.eu> <20130423205532.GE67273@kib.kiev.ua> <20130423213621.GB6346@dft-labs.eu> <20130514201305.GA15195@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--6+tq7hoORRLxZXV8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, May 14, 2013 at 10:13:05PM +0200, Mateusz Guzik wrote: > On Tue, Apr 23, 2013 at 11:36:21PM +0200, Mateusz Guzik wrote: > > On Tue, Apr 23, 2013 at 11:55:32PM +0300, Konstantin Belousov wrote: > > > On Tue, Apr 23, 2013 at 10:38:23PM +0200, Mateusz Guzik wrote: > > > > I would like to replace Giant with a local sx lock in sysvshm code. > > > > Looked really straightforward so maybe I missed something. > > >=20 > > > At very least, the shmget_existing() is no longer functional. > > > The sx is owned around tsleep(), and thus a progress cannot be made > > > by other thread, which needs the same sx lock. > > >=20 > > > Use of the SHMSEG_REMOVED in the shmget_allocate_segment() does > > > not make any sense in your patch, since sleeping malloc allocation > > > owns sx and prevent other threads from finding the segment. > > >=20 > > > I did not looked further. > >=20 > > Thank you for review, I definitely skimmed too fast. > >=20 > > Looks like this code has some bugs as it is already, e.g. kern_shmat > > does not re-check for NULL p->p_vmspace->vm_shm after malloc. > >=20 >=20 > So I produced 2 patches. >=20 > First one is straightforward: remove shmrealloc as it is a no-op anyway: > http://people.freebsd.org/~mjg/patches/sysvshm-remove-shmrealloc.patch Would it be better to change the kern.ipc.shmmni to CTLFLAG_RW instead ? The need for reboot to tune the number of identifiers is a bug. >=20 > Second one replaces Giant with sx lock and removes current support for > allocations that dropped Giant. Unfortunately I didn't have any good > ideas how to split this patch: > http://people.freebsd.org/~mjg/patches/sysvshm-giant-sx2.patch This seems to change the application-visible behaviour. When shmctl(IPC_RMID) is performed on the segment which is currently mapped, the existing mappings could still stay around for the undefined period of time. The present kernel code would put a process to sleep when attempt is made to call shmget() for the key. Your patch results in the shmget() for the removed key succeeding, it seems. Are you aware of sx_sleep(9) ? >=20 > Bugs in current support: > - p->p_vmspace->vm_shm allocation race (2 threads) in shmat > - vm_map_find can drop Giant, not taken into account in shmat > - lack of clean up if vm_pager_allocate fails I would consider fixing this more important, and actually unrelated to the supposed Giant replacement. >=20 > While it is possible to fix all these problems, I think sx lock that is > not dropped is ok to use here and it simplifies the implementation. >=20 > Is this acceptable? > --=20 > Mateusz Guzik <mjguzik gmail.com> --6+tq7hoORRLxZXV8 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (FreeBSD) iQIcBAEBAgAGBQJRk+MQAAoJEJDCuSvBvK1BI0oQAIgmG4Dqr3Dge4atGTDv7mdv qH58ZXIULByg3Ras0CI/pQGXhzbAel4pSPqs96IzDdbPiZD/Z4iFe7DBF+KXKW3c uwsANh7nyeDpPuPs2dsh9ADQbOUlHWSoS+gUIQBxER5sHwdMcwmeQjl83S1oxt0N 0mT6xzPBEBIAc0tnj7tYRgRQeocqMyQ0oImxFQMGmj+B2z0lTU67d3nJgE04ApIh XhOxTCqQhBi9UREwkJGQVhKTt/G17bfmZMwbijRVMZD3yjmmK2BSHglxPPofwRmj SWqefsfpr8VT0jc5jlxqiKyT8ZhAc39TZFotau5G/9TvFy3uSWitKLu3jqThposB cXG5q5obRUdjjgeCBIXMRvNvKFjU66qHLgpKo4VTHXCqezRSpI9Y69py94VqAlNT JdR6JuM/zIEjrGJnqOzMaJKzWXhAdp25cV7k7DppA4xKmoR0YGqIOh5zARNxUiHc 0i+hySvXBmqx0GTTdXfkv06pOu7Bz5hUhBOdL5G9e5Rx/tdhVBSsRpoWR6+viDG3 UkKPEP1NlUpk0EMuVfmo5GVuC0ZYLTJBnAsJiz2fu8qcJJrJp6ulYy/BSXunqAh7 2JzY2JCOPoGykJ9wVW3FophTnWtI4BNEb6VRB1ThbqTv0Fj8+QNTsnOiQpWwDFhg +X8VIGX+rYt+0TIqleWu =jYva -----END PGP SIGNATURE----- --6+tq7hoORRLxZXV8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130515193337.GX3047>