Re: sysvshm: replace Giant with a local sx lock

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>