> On Jan 24, 2015, at 8:51 AM, Konstantin Belousov <kostik...@gmail.com> wrote:
>
> On Sat, Jan 24, 2015 at 05:42:40PM +0200, Konstantin Belousov wrote:
>> On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote:
>>> On Sat, 2015-01-24 at 12:51 +0000, Konstantin Belousov wrote:
>>>> Author: kib
>>>> Date: Sat Jan 24 12:51:15 2015
>>>> New Revision: 277643
>>>> URL: https://svnweb.freebsd.org/changeset/base/277643
>>>>
>>>> Log:
>>>> Remove Giant from /dev/mem and /dev/kmem. It is definitely not needed
>>>> for i386, and from the code inspection, nothing in the
>>>> arm/mips/sparc64 implementations depends on it.
>>>>
>>>
>>> I'm not sure I agree with that. On arm the memrw() implementation uses
>>> a single statically-allocated page of kva space into which it maps each
>>> physical page in turn in the main loop. What prevents preemption or
>>> multicore access to /dev/mem from trying to use that single page for
>>> multiple operations at once?
>>
>> I see, thank you for noting this.
>>
>> But, I do not think that Giant is a solution for the problem. uiomove()
>> call accesses userspace, which may fault and cause sleep. If the
>> thread sleeps, the Giant is automatically dropped, so there is no real
>> protection.
>>
>> I think dump exclusive sx around whole memrw() should be enough.
>>
>> I can revert the commit for now, or I can leave it as is while
>> writing the patch with sx and waiting for somebody review. What
>> would you prefer ?
>>
>> P.S. mips uses uiomove_fromphys(), avoiding transient mapping,
>> and sparc allocates KVA when needed.
>
> Like this.
So why a sx lock and not a mutex?
Warner
> diff --git a/sys/arm/arm/mem.c b/sys/arm/arm/mem.c
> index 30d4b1d..58b0d25 100644
> --- a/sys/arm/arm/mem.c
> +++ b/sys/arm/arm/mem.c
> @@ -55,6 +55,7 @@ __FBSDID("$FreeBSD$");
> #include <sys/mutex.h>
> #include <sys/proc.h>
> #include <sys/signalvar.h>
> +#include <sys/sx.h>
> #include <sys/systm.h>
> #include <sys/uio.h>
>
> @@ -72,6 +73,9 @@ MALLOC_DEFINE(M_MEMDESC, "memdesc", "memory range
> descriptors");
>
> struct mem_range_softc mem_range_softc;
>
> +static struct sx tmppt_lock;
> +SX_SYSINIT(tmppt, &tmppt_lock, "mem4map");
> +
> /* ARGSUSED */
> int
> memrw(struct cdev *dev, struct uio *uio, int flags)
> @@ -107,6 +111,7 @@ memrw(struct cdev *dev, struct uio *uio, int flags)
> }
> if (!address_valid)
> return (EINVAL);
> + sx_xlock(&tmppt_lock);
> pmap_kenter((vm_offset_t)_tmppt, v);
> o = (int)uio->uio_offset & PAGE_MASK;
> c = (u_int)(PAGE_SIZE - ((int)iov->iov_base &
> PAGE_MASK));
> @@ -114,6 +119,7 @@ memrw(struct cdev *dev, struct uio *uio, int flags)
> c = min(c, (u_int)iov->iov_len);
> error = uiomove((caddr_t)&_tmppt[o], (int)c, uio);
> pmap_qremove((vm_offset_t)_tmppt, 1);
> + sx_xunlock(&tmppt_lock);
> continue;
> }
> else if (dev2unit(dev) == CDEV_MINOR_KMEM) {
>
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
