Date: Sun, 25 Jan 2015 10:07:00 -0700 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers <src-committers@freebsd.org>, Ian Lepore <ian@freebsd.org> Subject: Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64 Message-ID: <DB07559A-5EE9-4495-ABBC-19D6E45B99EF@bsdimp.com> In-Reply-To: <20150124155117.GW42409@kib.kiev.ua> References: <201501241251.t0OCpGa8053192@svn.freebsd.org> <1422111397.1038.53.camel@freebsd.org> <20150124154240.GV42409@kib.kiev.ua> <20150124155117.GW42409@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jan 24, 2015, at 8:51 AM, Konstantin Belousov <kostikbel@gmail.com> = wrote: >=20 > 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 >>>>=20 >>>> 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. >>>>=20 >>>=20 >>> 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? >>=20 >> I see, thank you for noting this. >>=20 >> 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. >>=20 >> I think dump exclusive sx around whole memrw() should be enough. >>=20 >> 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 ? >>=20 >> P.S. mips uses uiomove_fromphys(), avoiding transient mapping, >> and sparc allocates KVA when needed. >=20 > 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> >=20 > @@ -72,6 +73,9 @@ MALLOC_DEFINE(M_MEMDESC, "memdesc", "memory range = descriptors"); >=20 > struct mem_range_softc mem_range_softc; >=20 > +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 =3D (int)uio->uio_offset & PAGE_MASK; > c =3D (u_int)(PAGE_SIZE - ((int)iov->iov_base & = PAGE_MASK)); > @@ -114,6 +119,7 @@ memrw(struct cdev *dev, struct uio *uio, int = flags) > c =3D min(c, (u_int)iov->iov_len); > error =3D uiomove((caddr_t)&_tmppt[o], (int)c, = uio); > pmap_qremove((vm_offset_t)_tmppt, 1); > + sx_xunlock(&tmppt_lock); > continue; > } > else if (dev2unit(dev) =3D=3D CDEV_MINOR_KMEM) { >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?DB07559A-5EE9-4495-ABBC-19D6E45B99EF>