Can vm_mmap()/vm_map_remove() be called with giant held? (linuxolator dvb patches)

[Kostik Belousov]

On Fri, Feb 18, 2011 at 09:55:42PM +0100, Juergen Lock wrote:
> I have finally got back to this and did the style and vm_map_remove()
> return value handling fixes, updated the patches in-place:
> 
> 	http://people.freebsd.org/~nox/dvb/linux-dvb.patch
> 
> (for head)
> 
> 	http://people.freebsd.org/~nox/dvb/linux-dvb-8.patch
> 
> (for 8.)
> 
> On Sun, Jan 30, 2011 at 12:54:48AM +0100, Juergen Lock wrote:
> > On Sat, Jan 29, 2011 at 10:51:05PM +0200, Kostik Belousov wrote:
> > > On Sat, Jan 29, 2011 at 09:10:00PM +0100, Juergen Lock wrote:
> > > > Hi!
> > > > 
> > > >  I was kinda hoping to be able to post a correct patch in public but
> > > > getting an answer to ${Subject} seems to be more difficult than I
> > > > thought... :)  So, does anyone here know?  copyout_map() and
> > > You do not need Giant locked for vm_map* functions.
> > > 
> > The question was more do I need to drop it first before calling them...
> > 
> > > > copyout_unmap() are copied from ksyms_map() from sys/dev/ksyms/ksyms.c
> > > > - should there maybe be global versions instead of two static copies
> > > > each, and what would be good names?  And giant is taken by linux_ioctl()
> > > Would you make a patch for this ?
> > > 
> >  Heh if you want me to...  Where should they go and are my name choices ok?
> > 
>  I haven't done this yet so people can keep patching linux.ko in-place
> without having to build a new kernel too...
Separate build of linux.ko is not quite supported action. I would greatly
prefer to have the move of these two functions before the rest of the
patch comes in. Together with conversion of other users.

I propose to put it into vm/vm_glue.c.

> 
> > > > in the same source file before calling the parts I added.  So here
> > > > comes the patch, it is to add support for dvb ioctls to the linuxolator
> > > > as discussed on -emulation earlier in this thread:
> > > > 
> > > > 	http://lists.freebsd.org/pipermail/freebsd-multimedia/2011-January/011575.html
> > > > 
> > > > (patch also at:
> > > > 
> > > > 	http://people.freebsd.org/~nox/dvb/linux-dvb.patch
> > > > 
> > > > and a version for 8, which is what I tested with w_scan on dvb-s2
> > > > and dvb-t, and Andrew Gallatin also tested it with SageTV:
> > > > 
> > > > 	http://people.freebsd.org/~nox/dvb/linux-dvb-8.patch
> > > > 
> > > > )
> > > > 
> 
> > > > +	/*
> > > > +	 * Map somewhere after heap in process memory.
> > > > +	 */
> > > > +	PROC_LOCK(td->td_proc);
> > > > +	*addr = round_page((vm_offset_t)vms->vm_daddr +
> > > > +	    lim_max(td->td_proc, RLIMIT_DATA));
> > > > +	PROC_UNLOCK(td->td_proc);
> > > Are you sure that this is needed ? Why not leave the address selection
> > > to the VM ?
> > > 
> >  I don't know, maybe sys/dev/ksyms/ksyms.c has a reason?
> 
>  How would I leave the address selection to the VM?  Just trying
> to initialize *addr to (vm_offset_t)NULL there caused the patch to
> stop working.
I believe you should do
	*addr = 0;
	vm_mmap(map, addr);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20110219/442055b4/attachment.pgp