Re: sysvshm: replace Giant with a local sx lock

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.
> > > 
> > > 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
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.

> 
> 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) ?

> 
> 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.

> 
> 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>