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. > > > > 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. > > > > 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. > > > > I did not looked further. > > Thank you for review, I definitely skimmed too fast. > > 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. >
So I produced 2 patches. First one is straightforward: remove shmrealloc as it is a no-op anyway: http://people.freebsd.org/~mjg/patches/sysvshm-remove-shmrealloc.patch 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 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 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. Is this acceptable? -- Mateusz Guzik <mjguzik gmail.com> _______________________________________________ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
