Date: Fri, 24 Oct 2014 22:45:46 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Rui Paulo <rpaulo@me.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Rui Paulo <rpaulo@FreeBSD.org> Subject: Re: svn commit: r273598 - in head: include sys/dev/acpica Message-ID: <20141024194546.GE1877@kib.kiev.ua> In-Reply-To: <33decfcd-e77c-4e4c-8161-9f4a232213c6@me.com> References: <33decfcd-e77c-4e4c-8161-9f4a232213c6@me.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Oct 24, 2014 at 07:33:07PM +0000, Rui Paulo wrote: > On Oct 24, 2014, at 12:20 PM, Konstantin Belousov <kostikbel@gmail.com> wrote: > > On Fri, Oct 24, 2014 at 06:39:16PM +0000, Rui Paulo wrote: > > Author: rpaulo > > Date: Fri Oct 24 18:39:15 2014 > > New Revision: 273598 > > URL: https://svnweb.freebsd.org/changeset/base/273598 > > > > Log: > > HPET: create /dev/hpetN as a way to access HPET from userland. > > > > In some cases, TSC is broken and special applications might benefit > > from memory mapping HPET and reading the registers to count time. > > Most often the main HPET counter is 32-bit only[1], so this only gives > > the application a 300 second window based on the default HPET > > interval. > > Other applications, such as Intel's DPDK, expect /dev/hpet to be > > present and use it to count time as well. > > > > Although we have an almost userland version of gettimeofday() which > > uses rdtsc in userland, it's not always possible to use it, depending > > on how broken the multi-socket hardware is. > Yes, and hpet userland mapping would be better handled through the same > fake-vdso framework. As designed, it has discriminator to inform > userspace about algorithm, and can happilly utilize HPET timecounter > automatically mapped by kernel into the process address space. > > I'm aware of that, but I found the vdso a bit confusing and decided to work on that later. > > > +static int > > +hpet_open(struct cdev *cdev, int oflags, int devtype, struct thread *td) > > +{ > > + struct hpet_softc *sc; > > + > > + sc = cdev->si_drv1; > > + if (!sc->mmap_allow) > > + return (EPERM); > > + if (atomic_cmpset_32(&sc->devinuse, 0, 1) == 0) > > + return (EBUSY); > This is extra-weird. > The devinuse business disallows simultaneous opens, which prevents > other process from opening and mapping. But if the original caller > does mmap and close, second process now is allowed to open and mmap. > > That said, why do you need this devinuse at all ? > > Hmm, I wanted to avoid multiple mmap's, but that doesn't work like you said. I may just remove this restriction. > This is probably best. > > +static int > > +hpet_mmap(struct cdev *cdev, vm_ooffset_t offset, vm_paddr_t *paddr, > > + int nprot, vm_memattr_t *memattr) > > +{ > > + struct hpet_softc *sc; > > + > > + sc = cdev->si_drv1; > > + if (offset > rman_get_size(sc->mem_res)) > > + return (EINVAL); > > + if (!sc->mmap_allow_write && (nprot & PROT_WRITE)) > > + return (EPERM); > > + *paddr = rman_get_start(sc->mem_res) + offset; > What is the memattr for the backing page ? Is it set to non-cached > mode somehow ? I was not able to find place where would this happen. > > I expect it to be set to non-cached since it's a device anyway, but I don't know where it is. During my testing, I did not see any problems with cached values, though. > I am not claiming that it is wrong, only that I do not see an easy reason why it is right. Just printing the *memattr would provide the confidence. > > + sc->pdev = make_dev(&hpet_cdevsw, 0, UID_ROOT, GID_WHEEL, > > + 0600, "hpet%d", device_get_unit(dev)); > > + if (sc->pdev) { > > + sc->pdev->si_drv1 = sc; > > + sc->mmap_allow = 1; > > + TUNABLE_INT_FETCH("hw.acpi.hpet.mmap_allow", > > + &sc->mmap_allow); > > + sc->mmap_allow_write = 1; > > + TUNABLE_INT_FETCH("hw.acpi.hpet.mmap_allow_write", > > + &sc->mmap_allow_write); > > + SYSCTL_ADD_INT(device_get_sysctl_ctx(dev), > > + SYSCTL_CHILDREN(device_get_sysctl_tree(dev)), > > + OID_AUTO, "mmap_allow", > > + CTLFLAG_RW, &sc->mmap_allow, 0, > > + "Allow userland to memory map HPET"); > Why is mmap_allow is per-instance, while mmap_allow_write taken from > the global tunable ? > > Are you asking why there's no sysctl for it? IMO the allow-write must be controllable per-instance, or just managed by /dev/hpet* unix permissions. Having one global knob, which is consulted at the module load, is not flexible. What is the use-case for writing to HPET page ? To manually micro-adjust the timer ? Then, you probably want to disable write for instance used by system timecounter or eventtimer, but allow for HPET utilized by other consumers.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141024194546.GE1877>