Date: Sat, 24 Jan 2015 17:51:17 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Ian Lepore <ian@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64 Message-ID: <20150124155117.GW42409@kib.kiev.ua> In-Reply-To: <20150124154240.GV42409@kib.kiev.ua> References: <201501241251.t0OCpGa8053192@svn.freebsd.org> <1422111397.1038.53.camel@freebsd.org> <20150124154240.GV42409@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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. 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) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150124155117.GW42409>