Re: RFC: New userspace fetch/store API
> On Mar 7, 2019, at 11:21 AM, Klaus Klein wrote: > >> No, I'm removing the old API completely. > > Let me rephrase. :-) Your proposal didn't mention any header file; > I guess that means it'll remain then. Oh, yes, :-) -- thorpej
Re: RFC: New userspace fetch/store API
On Thu, Mar 07, 2019 at 10:56:04AM -0800, Jason Thorpe wrote: > > > > On Mar 7, 2019, at 2:59 AM, Klaus Klein wrote: > > > > On Sat, Feb 23, 2019 at 03:26:34PM -0800, Jason Thorpe wrote: > > > >> A project I’m working on has a need for a proper “int” size fetch / store > >> on all platforms, and it seems best to just tidy the whole mess up. So, I > >> am proposing a new replacement user fetch/store API. > > > > Cool. For completeness' sake, I suppose the new API is going to live > > alongside the old fetch/store API (for the time being) in ? > > No, I'm removing the old API completely. Let me rephrase. :-) Your proposal didn't mention any header file; I guess that means it'll remain then. - Klaus
Re: RFC: New userspace fetch/store API
> On Mar 7, 2019, at 2:59 AM, Klaus Klein wrote: > > On Sat, Feb 23, 2019 at 03:26:34PM -0800, Jason Thorpe wrote: > >> A project I’m working on has a need for a proper “int” size fetch / store on >> all platforms, and it seems best to just tidy the whole mess up. So, I am >> proposing a new replacement user fetch/store API. > > Cool. For completeness' sake, I suppose the new API is going to live > alongside the old fetch/store API (for the time being) in ? No, I'm removing the old API completely. -- thorpej
Re: RFC: New userspace fetch/store API
On Sat, Feb 23, 2019 at 03:26:34PM -0800, Jason Thorpe wrote: > A project I’m working on has a need for a proper “int” size fetch / store on > all platforms, and it seems best to just tidy the whole mess up. So, I am > proposing a new replacement user fetch/store API. Cool. For completeness' sake, I suppose the new API is going to live alongside the old fetch/store API (for the time being) in ? - Klaus
Re: RFC: New userspace fetch/store API
> On Feb 27, 2019, at 3:01 PM, Jason Thorpe wrote: > > I've made a bunch of progress on this, so far converting every architecture > except for ia64 at this point ... I may punt on that and let someone else > worry about fixing it up (i.e. I'll add stubs that just error out) .. I > refreshed the toolchains on my builder overnight, and am going to build test > kernels for most of the platforms we support. I'll test what I have hardware > for, and what I can quickly set up simulators for, but I'm going to need help > from others to get this all tested. > > Things that I can easily test directly: > > -- amd64 (using a VMware VM) > -- i386 (using a VMware VM) > -- aarch64 (using a Pinebook) > -- arm (using an ARMv6 Raspberry Pi) -- will exercise the ARMv4 and > pre-ARMv4 code paths in this case. > > I'm going to spend some time over the next few evenings getting Qemu > emulators up and running for smattering of systems (although I have been > having trouble getting Qemu guest networking working on a macOS host). What > I plan to test under Qemu: > > -- mips (32-bit, at least) > -- powerpc > -- sparc > -- sparc64 UPDATE: I have successfully tested: -- aarch64 (Qemu aarch64) -- amd64 (x86_64 VMware VM) -- arm (Qemu arm; tested armv3[no ldrh] and armv4[yes ldrh] code paths -- i386 (x86 VMware VM) -- sparc (Qemu sparc) I have setups to test, but am having troubles with: -- sparc64 -- but Qemu is giving me trouble with disk / network, so need to get that fixed first -- vax -- but my SIMH network setup stopped working, so I need to figure that out -- mips -- but gxemul doesn't seem to have good host networking capabilities, and evbmips MALTA hangs in Qemu when attaching gt0 Platforms where a good emulation environment seems to exist, but I haven't set one up yet: -- powerpc (Qemu?) Platforms I need help with due to lack of good emulation environments: -- alpha (I have no clue how well SIMH's alpha simulator works) -- hppa (Qemu doesn't support system emulation of hppa) -- m68k (no good simulation environment that I can run, AFAICT) -- sh3 (seems we don't support the boards that Qemu emulates, and also lack of networking in gxemul Dreamcast emulation) And in the WTF category: -- or1k -- port is incomplete -- riscv -- port is incomplete -- ia64 -- I decided to accept the pain and learn some ia64 assembly, but I don't know the state of the port, so... ? -- usermode ... ?? ia64 compiles, at least. So, any volunteers who can help me test alpha, hppa, m68k, and sh3? Let's start with those for now, since I don't have any clear path to test them myself. Thanks! -- thorpej
Re: RFC: New userspace fetch/store API
Jason Thorpe wrote:
> > Please prefix the size with 'u' if you operate on unsigned types:
> >
> > ufetch_u{8,16,32,64} and the same for ustore_*
>
> Even at the expense of being different from atomic_ops(3)?
>
atomic_ops(3) has atomic_cas_u{int,long}; ufetch_32() vs ufetch_u32()
is less of the concerns, so up to you.
--
Mindaugas
Re: RFC: New userspace fetch/store API
> On Feb 26, 2019, at 2:03 PM, Simon Burge wrote: > > Jason Thorpe wrote: > >> ... whether or not disc >> brakes are appropriate for road racing bicycles. > > Having recently converted, the answer is "yes". No debate needed :) I guess I'm just a luddite, but I do have disc brakes on my tandem :-) ANYWAY... I've made a bunch of progress on this, so far converting every architecture except for ia64 at this point ... I may punt on that and let someone else worry about fixing it up (i.e. I'll add stubs that just error out) .. I refreshed the toolchains on my builder overnight, and am going to build test kernels for most of the platforms we support. I'll test what I have hardware for, and what I can quickly set up simulators for, but I'm going to need help from others to get this all tested. Things that I can easily test directly: -- amd64 (using a VMware VM) -- i386 (using a VMware VM) -- aarch64 (using a Pinebook) -- arm (using an ARMv6 Raspberry Pi) -- will exercise the ARMv4 and pre-ARMv4 code paths in this case. I'm going to spend some time over the next few evenings getting Qemu emulators up and running for smattering of systems (although I have been having trouble getting Qemu guest networking working on a macOS host). What I plan to test under Qemu: -- mips (32-bit, at least) -- powerpc -- sparc -- sparc64 ...and if I get around to setting up SIMH, I will test vax myself, as well. I'll need help with everything else... ping me off list if you can help! Happily, I have an ATF unit test that exercises all of new functions, so anyone willing to volunteer to help me test it will have an easy time of it. -- thorpej
Re: RFC: New userspace fetch/store API
Jason Thorpe wrote: > ... whether or not disc > brakes are appropriate for road racing bicycles. Having recently converted, the answer is "yes". No debate needed :) Back to the function names... Cheers, Simon.
Re: RFC: New userspace fetch/store API
> On Feb 26, 2019, at 9:41 AM, Mouse wrote: > >> Memory store to initialize error > > Heh. Well spotted. > >> Itâ??s 2 loads and a store in either case, [...] > > ...though one does better when there _are_ multiple calls whose error > checks can be collapsed. ...and having personally audited every call site for these in the kernel recently, that basically never happens. >> and the second pattern is contrary to just about everything else in >> the kernel. > > True, though many, perhaps most, of those patterns are historical > relics from the days when memory references were comparatively cheap. > This is not to say that they necessarily should be discarded, just that > "It's Tradition!" is not necessarily a conversation-closer. At the same time, one mustn't arbitrarily micro-optimize every tiny little thing at the expense of code flow and API consistency. Trust me, I completely understand the "death by a thousand tiny cuts" problem (it's something I deal with constantly at $DayJob), but this isn't one of those. One also must consider what the intended use of these APIs are... while they are general purpose, any time you're fetching a single memory cell from another address space, you've already decided that the code isn't performance-critical. Indeed, in the first intended use case of ufetch_int(), what started me down this rabbit hole in the first place, is the _slow path_ of a contended mutex acquisition. In any case, I'm not going to spend any more time debating how values and errors are returned from these functions ... our time is better spent arguing about things that actually matter, such as wether or not "uint" should appear in the function names and whether or not disc brakes are appropriate for road racing bicycles. -- thorpej
Re: RFC: New userspace fetch/store API
> Memory store to initialize error Heh. Well spotted. > Itâ??s 2 loads and a store in either case, [...] ...though one does better when there _are_ multiple calls whose error checks can be collapsed. > and the second pattern is contrary to just about everything else in > the kernel. True, though many, perhaps most, of those patterns are historical relics from the days when memory references were comparatively cheap. This is not to say that they necessarily should be discarded, just that "It's Tradition!" is not necessarily a conversation-closer. > Nevermind that itâ??s patently absurd to be micro-optimizing this > particular thing because it completely ignores all of the other stuff > that these routines do behind the curtain that absolutely dwarfs what > weâ??re talking about here. True enough. Personally, I am less concerned about fixing this particular case than I am about fixing the mental attitudes and habits that led to it (to the extent that "fixing" is an appropriate word, of course). My own preference, here, would be the suggestion I saw upthread about returning a struct. There seems to be a resistance to using structs by value; I speculate that it is largely holdover from the days when structs by value were expensive, with perhaps some from the even older days when structs by value weren't even supported yet. The struct is easily the fewest memory references of anything I've seen suggested, and I see no reason to avoid it except habit. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: RFC: New userspace fetch/store API
> On Feb 26, 2019, at 7:58 AM, Mouse wrote: > int err = 0; long hisflags = ufetch_64(flag1p, ) | ufetch_64(flag2p, ); > if (err) return EFAULT; > do_something(hisflags); > >>> I like this, because it swaps the cost of the value that is always >>> needed (which was expensive) versus the one that isn't expected >>> often (the error case, was cheap). > >> Huh? The code always has to access err to work correctly. You don't >> save anything. > > Let's look at the no-error (presumably common) case: So, let’s look at the EVEN MORE COMMON case of only a single ufetch call at any particular call site: if ((error = ufetch_64(flag1p, ))) return error; do_something(val1); ufetch_64 does a memory load from flag1p ufetch_64 does a memory store to Memory load from to pass argument to do_something() Versus: error = 0; val1 = ufetch_64(flag1p, ); if (error) return error; do_something(val1); Memory store to initialize error ufetch_64 does a memory load from flag1p Memory load from to check error condition It’s 2 loads and a store in either case, and the second pattern is contrary to just about everything else in the kernel. Nevermind that it’s patently absurd to be micro-optimizing this particular thing because it completely ignores all of the other stuff that these routines do behind the curtain that absolutely dwarfs what we’re talking about here. -- thorpej
Re: RFC: New userspace fetch/store API
>>> int err = 0; >>> long hisflags = ufetch_64(flag1p, ) | ufetch_64(flag2p, ); >>> if (err) return EFAULT; >>> do_something(hisflags); >> I like this, because it swaps the cost of the value that is always >> needed (which was expensive) versus the one that isn't expected >> often (the error case, was cheap). > Huh? The code always has to access err to work correctly. You don't > save anything. Let's look at the no-error (presumably common) case: if (ufetch_64(flag1p,)) return(EFAULT); if (ufetch_64(flag2p,)) return(EFAULT); do_something(val1|val2) -> ufetch_64 fetches first value ufetch_64 writes through its second argument, returns false ufetch_64 fetches second value ufetch_64 writes through its second argument, returns false caller reads val1 caller reads val2 caller computes val1|val2 caller does something with it versus uint64_t hisflags = ufetch_64(flag1p, ) | ufetch_64(flag2p, ); if (err) return EFAULT; do_something(hisflags); -> ufetch_64 fetches first value ufetch_64 doesn't write through its second argument, returns val1 ufetch_64 fetches second value ufetch_64 doesn't write through its second argument, returns val2 caller computes val1|val2 caller reads err, tests it, discovers no error caller does something with it Looks to me as though the second way saves two writes and one read. Even if you test error after each call, instead of once after the whole sequence, you save two writes. What am I missing? (The reading of val1 and val2 in the first case, and the calls to ufetch_64 in the second case, may be switched, but that doesn't affect the counts.) /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: RFC: New userspace fetch/store API
On Tue, Feb 26, 2019 at 01:59:42PM +0100, Rhialto wrote: > On Mon 25 Feb 2019 at 18:10:20 +, Eduardo Horvath wrote: > > I'd do something like: > > > > uint64_t ufetch_64(const uint64_t *uaddr, int *errp); > > > > where *errp needs to be initialized to zero and is set on fault so you can > > do: > > > > int err = 0; > > long hisflags = ufetch_64(flag1p, ) | ufetch_64(flag2p, ); > > > > if (err) return EFAULT; > > > > do_something(hisflags); > > I like this, because it swaps the cost of the value that is always > needed (which was expensive) versus the one that isn't expected often > (the error case, was cheap). Huh? The code always has to access err to work correctly. You don't save anything. Joerg
Re: RFC: New userspace fetch/store API
On Mon 25 Feb 2019 at 18:10:20 +, Eduardo Horvath wrote: > I'd do something like: > > uint64_t ufetch_64(const uint64_t *uaddr, int *errp); > > where *errp needs to be initialized to zero and is set on fault so you can > do: > > int err = 0; > long hisflags = ufetch_64(flag1p, ) | ufetch_64(flag2p, ); > > if (err) return EFAULT; > > do_something(hisflags); I like this, because it swaps the cost of the value that is always needed (which was expensive) versus the one that isn't expected often (the error case, was cheap). With this change, the return value can be stored in a register. If you take the address of the variable where you want the value to be, it can't be a register. But the error case hopefully doesn't occur often, so the error code likely doesn't even have to be stored into. > Eduardo -Olaf. -- ___ Olaf 'Rhialto' Seibert -- "What good is a Ring of Power \X/ rhialto/at/falu.nl -- if you're unable...to Speak." - Agent Elrond signature.asc Description: PGP signature
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 3:48 PM, Mouse wrote: > > C does not have a `pointer' type; what it has is a `pointer to > OTHER-TYPE' for each type OTHER-TYPE. > > On all NetBSD ports I'm aware of, this is a distinction without a > difference, since all memory is the same and all pointers are just > memory addresses. Whether this is something to care about depends on > how much you are willing to depend on the compiler letting you get away > with things that C does not, strictly, promise. There are a lot of places in the NetBSD code base that make the assumption that “void *” is a substitute for a pointer to any other type, and that any pointer type can be converted to any number of integer types, including, but not limited to, “intptr_t”, “uintptr_t”, and “vaddr_t”. So, when I say “MI”, I’m referring to the NetBSD notion of “MI”, which includes a number of baked-in assumptions about implementation. > [still thorpej, but another message] >> If weâ??re concerned about portability of the things using this API, >> then we simply specify the alignment to be sizeof(type). That check >> is straight-forward in MI C. > > Not really. There is no MI way to check the alignment of a pointer. > Pointers cannot be arguments to & or %, and there is nothing MI about > converting a pointer to an integer. > > Again, on all NetBSD ports I'm aware of, the situation is nicer; it > would be reasonable to say that it's straightforward in MI NetBSD code, > but it's not really MI C. See above. -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 25, 2019, at 9:18 AM, Andrew Cagney wrote: > > So the out-of-band error check becomes the slow memory write? > I don't see it as a problem as the data would presumably be written to > the write-back cached's stack (besides, if the function is short, LTO > will eliminate it). > > FWIW, I suspect the most "efficient" way to return the value+error as > a struct - ABIs would return the pair in registers or, failing that, > pass a single stack address - but I don't think this interface should > go there. Agreed, this is way off in the weeds of “premature optimization”. Besides, why make this function “weird” compared to everything else that just returns an error code in a normal way? -- thorpej
Re: RFC: New userspace fetch/store API
On Mon, 25 Feb 2019, Andrew Cagney wrote: > On Mon, 25 Feb 2019 at 11:35, Eduardo Horvath wrote: > > > > On Sat, 23 Feb 2019, Jason Thorpe wrote: > > > > > int ufetch_8(const uint8_t *uaddr, uint8_t *valp); > > > int ufetch_16(const uint16_t *uaddr, uint16_t *valp); > > > int ufetch_32(const uint32_t *uaddr, uint32_t *valp); > > > #ifdef _LP64 > > > int ufetch_64(const uint64_t *uaddr, uint64_t *valp); > > > #endif > > > > etc. > > > > I'd prefer to return the fetched value and have an out of band error > > check. > > > > With this API the routine does a userland load, then a store into kernel > > memory. Then the kernel needs to reload that value. On modern CPUs > > memory accesses tend to be sloow. > > So the out-of-band error check becomes the slow memory write? Even cached accesses are slower than register accesses. And the compiler is limited in what it can do to reorder the instruction stream while maintaining C language semantics. > I don't see it as a problem as the data would presumably be written to > the write-back cached's stack (besides, if the function is short, LTO > will eliminate it). The compiler can eliminate the function but need to keep the defined C language synchronization points so the memory accesses limit the ability of the compiler to properly optimize the code. Plus, there are a number of applications where you may want to do a series of userland fetches in a row and operate on them. In this case it would be much easier to do the series and only check for success when you're done. And all the existing code expects the value to be returned by the function not in one of the parameters, so existing code would require less rewriting. I'd do something like: uint64_t ufetch_64(const uint64_t *uaddr, int *errp); where *errp needs to be initialized to zero and is set on fault so you can do: int err = 0; long hisflags = ufetch_64(flag1p, ) | ufetch_64(flag2p, ); if (err) return EFAULT; do_something(hisflags); Eduardo
Re: RFC: New userspace fetch/store API
On Mon, 25 Feb 2019 at 11:35, Eduardo Horvath wrote: > > On Sat, 23 Feb 2019, Jason Thorpe wrote: > > > int ufetch_8(const uint8_t *uaddr, uint8_t *valp); > > int ufetch_16(const uint16_t *uaddr, uint16_t *valp); > > int ufetch_32(const uint32_t *uaddr, uint32_t *valp); > > #ifdef _LP64 > > int ufetch_64(const uint64_t *uaddr, uint64_t *valp); > > #endif > > etc. > > I'd prefer to return the fetched value and have an out of band error > check. > > With this API the routine does a userland load, then a store into kernel > memory. Then the kernel needs to reload that value. On modern CPUs > memory accesses tend to be sloow. So the out-of-band error check becomes the slow memory write? I don't see it as a problem as the data would presumably be written to the write-back cached's stack (besides, if the function is short, LTO will eliminate it). FWIW, I suspect the most "efficient" way to return the value+error as a struct - ABIs would return the pair in registers or, failing that, pass a single stack address - but I don't think this interface should go there.
Re: RFC: New userspace fetch/store API
On Sat, 23 Feb 2019, Jason Thorpe wrote: > int ufetch_8(const uint8_t *uaddr, uint8_t *valp); > int ufetch_16(const uint16_t *uaddr, uint16_t *valp); > int ufetch_32(const uint32_t *uaddr, uint32_t *valp); > #ifdef _LP64 > int ufetch_64(const uint64_t *uaddr, uint64_t *valp); > #endif etc. I'd prefer to return the fetched value and have an out of band error check. With this API the routine does a userland load, then a store into kernel memory. Then the kernel needs to reload that value. On modern CPUs memory accesses tend to be sloow. Eduardo
Re: RFC: New userspace fetch/store API
[thorpej] > My intent was to cover the â??naturalâ??[*] fixed-size types for a > particular arch, and then provide convenience aliases for the common > basic C types: char, short, int, long, pointer. C does not have a `pointer' type; what it has is a `pointer to OTHER-TYPE' for each type OTHER-TYPE. On all NetBSD ports I'm aware of, this is a distinction without a difference, since all memory is the same and all pointers are just memory addresses. Whether this is something to care about depends on how much you are willing to depend on the compiler letting you get away with things that C does not, strictly, promise. [still thorpej, but another message] > If weâ??re concerned about portability of the things using this API, > then we simply specify the alignment to be sizeof(type). That check > is straight-forward in MI C. Not really. There is no MI way to check the alignment of a pointer. Pointers cannot be arguments to & or %, and there is nothing MI about converting a pointer to an integer. Again, on all NetBSD ports I'm aware of, the situation is nicer; it would be reasonable to say that it's straightforward in MI NetBSD code, but it's not really MI C. [dholland, replying to the second of the above quotes] > No, even if you know what the alignment's supposed to be, you can't > legally check it. Well, you can check it, certainly. But such checking works, when it does, only because the compiler happens to implement certain implementation-dependent aspects of C in whichever way the checking code you use expects. [dholland, same message] > Also, these days you can expect the compiler to simply remove such > checks on the grounds that all pointers to types with required > alignment are expected to be already aligned. Such a compiler is not suitable for compiling kernels, because of exactly this sort of thing. Compilers have a lot of latitude according to the C spec, yes, but compilers that take advantage of it (and cannot be told to not do so) are not suitable for building implementation infrastructure, such as even malloc(), never mind a kernel. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
re: RFC: New userspace fetch/store API
> > On Feb 23, 2019, at 4:04 PM, matthew green wrote: > > > > i like this. the current API is ... odd. > > Oh any thoughts on have intrsafe or not? if we can get away without it, that would be best. how hard is the single(?) caller to fix? :-) .mrg.
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 1:42 PM, Mindaugas Rasiukevicius wrote:
>
>> Outstanding question before I go too far down the rabbit hole: should I
>> bother with the “intrsafe” variants? The only application for it in the
>> tree right now is in the profiling code, as an optimization to avoid
>> taking an AST when it’s time to bump the counters.
>
> Please do not over-engineer it. IIRC, there are not that many use cases
> of this API, the need for more variants didn't really increase over the
> decade(s) and there is no need to provide a vast spectrum of functions.
> They can be added later, if there will be a need.
Again, I’m happy to eliminate the _intrsafe variants. The only use is in the
profiling code, and it’s just to avoid taking an AST.
> Also, are there any uses of byte and short operations? If not, then I
> think these variants should be eliminated. Nowadays, I would argue that
> generally there should be no good reason to choose sub-word operations;
> stick with the word size (it helps with the alignment too) or more.
There are lots of uses in MD code, and a few in MI code (ureadc() and
mincore(), for example). I figured having the same API when it’s just MD code
would be worthwhile.
>> These functions may block (to service a page fault), and are NOT safe to
>> call from any interrupt context.
>>
>
> Please make sure you add the asserts.
I’ll make C wrappers for all of these, and have the MD guts prefixed with an
underscore so I can centralize the assertion logic.
>> The implementation is entirely in architecture-dependent code. The
>> following fetch primitives must be supplied:
>>
>> int ufetch_8(const uint8_t *uaddr, uint8_t *valp);
>> int ufetch_16(const uint16_t *uaddr, uint16_t *valp);
>> int ufetch_32(const uint32_t *uaddr, uint32_t *valp);
>> #ifdef _LP64
>> int ufetch_64(const uint64_t *uaddr, uint64_t *valp);
>> #endif
>
> Please prefix the size with 'u' if you operate on unsigned types:
>
> ufetch_u{8,16,32,64} and the same for ustore_*
Even at the expense of being different from atomic_ops(3)?
-- thorpej
Re: RFC: New userspace fetch/store API
On Sun, 24 Feb 2019 at 10:38, Jason Thorpe wrote:
>
>
> > On Feb 24, 2019, at 7:05 AM, Andrew Cagney wrote:
> >
> > Can there be an inefficient default, implemented using something like
> > copy{in,out}() (yes I note the point about alignment). I sometimes
> > wonder of NetBSD's lost its way in terms of portability - it seems to
> > be accumulating an increasing number of redundant yet required
> > primitives.
>
> Honestly, it’s not adding that many — most of the ones specified in my
> proposal were required before (to varying degrees) just with different names
> and return semantics. And most of the “new” ones in my proposal as simple
> aliases. Furthermore, on the two implementations I’ve done so far, a great
> deal of the code is macro-ized to reduce the redundant typing, and even the
> “intrsafe” ones share most of the code by using a different (macro-ized)
> prologue and then jumping into the body of the non-intrsafe variant. It’s
> really not that much code.
Right.. But can it be done once in generic code using information
available while compiling?
> As far as “inefficient default” … depending on how copyin / copyout are
> implemented, it’s also an issue of correctness. At least the 32-bit and
> 64-bit variants would need to be atomic with respect to other loads / stores
> to the same address. This is one property that I need for my own use.
And that's a hard requirement. What of the other way around - copy*()
implemented using fetch_*().
> If there really is a concern about the number of operations here,I would be
> more than happy to drop the _8 and _16 (and thus char and short) widths from
> the formal API… but then other MI code would need to change to lose that
> functionality (in the case of the profiling code that uses fuswintr() and
> suswintr()), or to use heavier means of doing the same thing (e.g. copyin or
> copyout of a single byte).
>
> -- thorpej
>
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 8:26 PM, matthew green wrote: > >>> On Feb 23, 2019, at 4:04 PM, matthew green wrote: >>> >>> i like this. the current API is ... odd. >> >> Oh any thoughts on have intrsafe or not? > > if we can get away without it, that would be best. how hard is > the single(?) caller to fix? :-) Not at all — just make it use the “got a page fault” case always. I’ve done it in my local tree. -- thorpej
Re: RFC: New userspace fetch/store API
Jason Thorpe wrote:
> The existing fetch(9) / store(9) APIs have some problems. Specifically:
>
> ==> Their return semantics mean that fuword() cannot disambiguate between
> an error condition (-1) and a legitimate fetch of -1 from user space.
>
> ==> “word” is poorly defined. For all practical purposes, it means
> “long”, and there is thus no way to fetch/store an “int” value on LP64
> platforms.
>
> ==> There are lots of legacy aliases lying about that are no longer
> meaningful (I-space and D-space variants, for example).
Thanks for working on this. Actually, I started the same revamp with a
similar API (suggested by matt@ at that time) a bunch of years ago.. but
lost interest and didn't finish.
> Outstanding question before I go too far down the rabbit hole: should I
> bother with the “intrsafe” variants? The only application for it in the
> tree right now is in the profiling code, as an optimization to avoid
> taking an AST when it’s time to bump the counters.
Please do not over-engineer it. IIRC, there are not that many use cases
of this API, the need for more variants didn't really increase over the
decade(s) and there is no need to provide a vast spectrum of functions.
They can be added later, if there will be a need.
Also, are there any uses of byte and short operations? If not, then I
think these variants should be eliminated. Nowadays, I would argue that
generally there should be no good reason to choose sub-word operations;
stick with the word size (it helps with the alignment too) or more.
>
> These functions may block (to service a page fault), and are NOT safe to
> call from any interrupt context.
>
Please make sure you add the asserts.
> The implementation is entirely in architecture-dependent code. The
> following fetch primitives must be supplied:
>
> int ufetch_8(const uint8_t *uaddr, uint8_t *valp);
> int ufetch_16(const uint16_t *uaddr, uint16_t *valp);
> int ufetch_32(const uint32_t *uaddr, uint32_t *valp);
> #ifdef _LP64
> int ufetch_64(const uint64_t *uaddr, uint64_t *valp);
> #endif
Please prefix the size with 'u' if you operate on unsigned types:
ufetch_u{8,16,32,64} and the same for ustore_*
> The following aliases must also be provided, mapped to the appropriate
> fixed-size primitives:
>
> int ufetch_char(const unsigned char *uaddr, unsigned char *valp);
> int ufetch_short(const unsigned short *uaddr, unsigned short *valp);
> int ufetch_int(const unsigned int *uaddr, unsigned int *valp);
> int ufetch_long(const unsigned long *uaddr, unsigned long *valp);
> int ufetch_ptr(const void **uaddr, void **valp);
>
> The following store primitives must be suppled:
>
> int ustore_8(uint8_t *uaddr, uint8_t val);
> int ustore_16(uint16_t *uaddr, uint16_t val);
> int ustore_32(uint32_t *uaddr, uint32_t val);
> #ifdef _LP64
> int ustore_64(uint64_t *uaddr, uint64_t val);
> #endif
>
> The following aliases must also be provided, mapped to the appropriate
> fixed-size primitives:
>
> int ustore_char(unsigned char *uaddr, unsigned char val);
> int ustore_short(unsigned short *uaddr, unsigned short val);
> int ustore_int(unsigned int *uaddr, unsigned int val);
> int ustore_long(unsigned long *uaddr, unsigned long val);
> int ustore_ptr(void **uaddr, void *val);
The same applies here (e.g. ustore_int becomes ustore_uint).
--
Mindaugas
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 10:28 AM, David Holland wrote: > > No, even if you know what the alignment's supposed to be, you can't > legally check it. Or maybe you can, but it's in any event tangled in a > bunch of language-lawyering. > > Also, these days you can expect the compiler to simply remove such > checks on the grounds that all pointers to types with required > alignment are expected to be already aligned. Grumble, mumble… ok, fair. > The ones I remember looking at (which is not all of them, and probably > not alpha) didn't test onfault until they got to we_re_toast, meaning > any fault is already covered. But in any event it's free at runtime > (the sense of free I meant) because you only get to it when a trap > would otherwise be triggering a panic. I guess I’ll audit them as part of the process. > It's only not free on architectures where there are special > instructions for accessing usermode and they need to be explicitly > checked for failure rather than using on-fault logic; in that case you > might also need an explicit alignment check to avoid allowing bad > pointers to trigger a panic. That check should be in the MD code for > such ports. (If there even are any... I think there exist machines > that work this way, not sure we run on any of them.) m68k uses special instructions to access user space (well, “the other address space”) … but they also don’t have alignment constraints. Hilarious! -- thorpej
Re: RFC: New userspace fetch/store API
On Sun, Feb 24, 2019 at 08:47:12AM -0800, Jason Thorpe wrote: > > > On Feb 24, 2019, at 8:42 AM, David Holland > > wrote: > > Given that the alignment check _is_ free on some architectures, and > > that dealing with alignment in machine-independent C is problematic, > > that doesn't seem like the right way? > > If we?re concerned about portability of the things using this API, > then we simply specify the alignment to be sizeof(type). That > check is straight-forward in MI C. No, even if you know what the alignment's supposed to be, you can't legally check it. Or maybe you can, but it's in any event tangled in a bunch of language-lawyering. Also, these days you can expect the compiler to simply remove such checks on the grounds that all pointers to types with required alignment are expected to be already aligned. > The alignment check is not necessary ?free? in MD code ? a > mis-aligned address would likely trigger an alignment fault (SIGBUS > code path) rather than a page fault (SIGSEGV code path) in the trap > handler. Certainly, it didn?t seem like the alpha trap handler > handled a misaligned address in this case at all. This isn?t the > end of the world, but it means putting ?onfault? logic in more than > one code path in the trap handler. The ones I remember looking at (which is not all of them, and probably not alpha) didn't test onfault until they got to we_re_toast, meaning any fault is already covered. But in any event it's free at runtime (the sense of free I meant) because you only get to it when a trap would otherwise be triggering a panic. It's only not free on architectures where there are special instructions for accessing usermode and they need to be explicitly checked for failure rather than using on-fault logic; in that case you might also need an explicit alignment check to avoid allowing bad pointers to trigger a panic. That check should be in the MD code for such ports. (If there even are any... I think there exist machines that work this way, not sure we run on any of them.) -- David A. Holland dholl...@netbsd.org
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 10:07 AM, Brian Buhrow wrote: > > hello. It strikes me that if you've already written the code to fetch > and store 8, 16, 32 and sometimes 64 bit values, it should be committed. I am pretty sure most platforms already have the 8- and 16-bit variants, just under different names. I did have to write new implementations for alpha, as they were there-but-not-fully-implemented. -- thorpej
Re: RFC: New userspace fetch/store API
hello. It strikes me that if you've already written the code to fetch and store 8, 16, 32 and sometimes 64 bit values, it should be committed. If other OS's have this functionality, then it means applications wil use it and subsequent porting of such applications to NetBSD will be easier if the functionality is there. It may seem like I'm arguing we should do it because others did, and perhaps I am to some extent, but having an easy platform to port aplications to makes netBSD much more attractive than it would be if a bunch of functionality that's expected by application authors is missing. Just my 2 cents. -thanks -Brian
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 9:40 AM, Jason Thorpe wrote: > > 32-bit on every platform, and 64-bit on LP64 platforms (for pointers). > > I’m fine with that, so long as we’re willing to “give up” on the other things > that use byte / short / in-interrupt. I will note, however, that some platforms are still going to need the byte / short variants for their own MD purposes (m68k trap handler, for example). If this is the direction we decide to go, I will make them use the same naming pattern, and will protect their prototypes inside a __HAVE_USER_FETCH_STORE_16 or whatever. -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 9:12 AM, Joerg Sonnenberger wrote: > > I'm far more inclined to just want the limited API with as many > constraints as reasonable, e.g. an atomic 32bit fetch or store and > that's it. Maybe add 64bit variants, but no more. No point really in > having a lot of dead weight around. 32-bit on every platform, and 64-bit on LP64 platforms (for pointers). I’m fine with that, so long as we’re willing to “give up” on the other things that use byte / short / in-interrupt. -- thorpej
Re: RFC: New userspace fetch/store API
On Sat, Feb 23, 2019 at 04:40:40PM -0800, Jason Thorpe wrote: > >> i like this. the current API is ... odd. > >> > >> can you add alignment documentation? eg, if only to say that it > >> must support aligned or unaligned accesses for the relevant > >> datatypes. not sure what we need currently, ie, if unaligned is > >> needed because that happens today, or we can define it as wrong > >> in this API. > > > > Yah, I?ll make that clear. It?s implicit in the current API (alpha would > > blow up otherwise), but I will make it explicit. > > I added the following: > > > All addresses MUST be naturally-aligned for their type. If unaligned > access is required, use copy(9). > > > OK? Given that the predominant use of these is to fetch from user-supplied pointers, which can be crap, it would best to include an alignment check; otherwise ~all call sites will require explicit checks, which in addition to being error-prone start running into annoying portability questions. (On at least some platforms the alignment check is free, also, because an alignment fault during the access will be caught by the copyin fault logic.) -- David A. Holland dholl...@netbsd.org
Re: RFC: New userspace fetch/store API
[Sorry, I meant to respond to multiple points in my prior message, but the coffee hasn’t fully kicked in yet…] > On Feb 24, 2019, at 7:05 AM, Andrew Cagney wrote: > > On Sat, 23 Feb 2019 at 18:26, Jason Thorpe wrote: > >> A project I’m working on has a need for a proper “int” size fetch / store on >> all platforms, and it seems best to just tidy the whole mess up. So, I am >> proposing a new replacement user fetch/store API. > > Can you be more specific on why "int" rather than a more abstract type > (we've things like size_t ptrdiff_t et.al. all kicking around) - I'm > curious. In my particular case, it’s the kernel portion of an API that on other platforms uses an “int” in this particular argument slot. For the sake of not only API compatibility as well as COMPAT_* compatibility, “int” is what I decided to go with for my particular use case. My intent was to cover the “natural”[*] fixed-size types for a particular arch, and then provide convenience aliases for the common basic C types: char, short, int, long, pointer. The latter are aliases (and I wanted to place the aliases in a centralized location, subr_copy.c, but the assembler / linker doesn’t like aliasing symbols in a different compilation unit). [*] pre-BWX alpha of course has to emulate 8- and 16-bit accesses. >> int ufetch_8(const uint8_t *uaddr, uint8_t *valp); > > To follow through with Edgar Fuß's point. If I were looking for a > function to transfer a uint8_t I'd expect it to be called > fetch_uint8(). As I mentioned in my reply to Edgar, this is to be consistent with atomic_ops(3), which in turn used the same names as Solaris. > I do find passing pointers as both the addr and the dest confusing; > but you've added a const which should help get the order right. Yes, well, one of the problems with the old fetch(9) API is that “error condition” and “value” are returned in the same place, and thus there are ambiguous cases. -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 7:05 AM, Andrew Cagney wrote:
>
> Can there be an inefficient default, implemented using something like
> copy{in,out}() (yes I note the point about alignment). I sometimes
> wonder of NetBSD's lost its way in terms of portability - it seems to
> be accumulating an increasing number of redundant yet required
> primitives.
Honestly, it’s not adding that many — most of the ones specified in my proposal
were required before (to varying degrees) just with different names and return
semantics. And most of the “new” ones in my proposal as simple aliases.
Furthermore, on the two implementations I’ve done so far, a great deal of the
code is macro-ized to reduce the redundant typing, and even the “intrsafe” ones
share most of the code by using a different (macro-ized) prologue and then
jumping into the body of the non-intrsafe variant. It’s really not that much
code.
As far as “inefficient default” … depending on how copyin / copyout are
implemented, it’s also an issue of correctness. At least the 32-bit and 64-bit
variants would need to be atomic with respect to other loads / stores to the
same address. This is one property that I need for my own use.
If there really is a concern about the number of operations here,I would be
more than happy to drop the _8 and _16 (and thus char and short) widths from
the formal API… but then other MI code would need to change to lose that
functionality (in the case of the profiling code that uses fuswintr() and
suswintr()), or to use heavier means of doing the same thing (e.g. copyin or
copyout of a single byte).
-- thorpej
Re: RFC: New userspace fetch/store API
On Sat, 23 Feb 2019 at 18:26, Jason Thorpe wrote:
> A project I’m working on has a need for a proper “int” size fetch / store on
> all platforms, and it seems best to just tidy the whole mess up. So, I am
> proposing a new replacement user fetch/store API.
Can you be more specific on why "int" rather than a more abstract type
(we've things like size_t ptrdiff_t et.al. all kicking around) - I'm
curious.
> I have implemented the new API for alpha and amd64, and am putting together a
> test harness to exercise all aspects of the new API. Once that is done, I’ll
> tackle the remaining architectures.
> The implementation is entirely in architecture-dependent code. The following
> fetch primitives must be supplied:
As a corollary to KRE's point, why?
Can there be an inefficient default, implemented using something like
copy{in,out}() (yes I note the point about alignment). I sometimes
wonder of NetBSD's lost its way in terms of portability - it seems to
be accumulating an increasing number of redundant yet required
primitives.
> int ufetch_8(const uint8_t *uaddr, uint8_t *valp);
To follow through with Edgar Fuß's point. If I were looking for a
function to transfer a uint8_t I'd expect it to be called
fetch_uint8().
> int ufetch_16(const uint16_t *uaddr, uint16_t *valp);
> int ufetch_32(const uint32_t *uaddr, uint32_t *valp);
> #ifdef _LP64
> int ufetch_64(const uint64_t *uaddr, uint64_t *valp);
> #endif
>
> The following aliases must also be provided, mapped to the appropriate
> fixed-size primitives:
>
> int ufetch_char(const unsigned char *uaddr, unsigned char *valp);
> int ufetch_short(const unsigned short *uaddr, unsigned short *valp);
> int ufetch_int(const unsigned int *uaddr, unsigned int *valp);
> int ufetch_long(const unsigned long *uaddr, unsigned long *valp);
> int ufetch_ptr(const void **uaddr, void **valp);
I do find passing pointers as both the addr and the dest confusing;
but you've added a const which should help get the order right.
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 2:34 AM, Edgar Fuß wrote: > >> The following aliases must also be provided, mapped to the appropriate >> fixed-size primitives: >> >> int ufetch_char(const unsigned char *uaddr, unsigned char *valp); >> int ufetch_short(const unsigned short *uaddr, unsigned short *valp); >> int ufetch_int(const unsigned int *uaddr, unsigned int *valp); >> int ufetch_long(const unsigned long *uaddr, unsigned long *valp); > I would have expected the argument of a funtion called xxx_short etc. > to be a short etc., not an unsigned short etc. > Does it make sense to either make the argument char/short/int/long or > change the name to ufetch_uXXX? My proposed names match the pattern used by atomic_ops(3) (which in turn matches how Solaris named those routines). In general I would favor consistency with atomic_ops(3) for consistency’s sake, but I’m not wedded to the names and will change them if the consensus is that they should change. -- thorpej
Re: RFC: New userspace fetch/store API
> The following aliases must also be provided, mapped to the appropriate > fixed-size primitives: > > int ufetch_char(const unsigned char *uaddr, unsigned char *valp); > int ufetch_short(const unsigned short *uaddr, unsigned short *valp); > int ufetch_int(const unsigned int *uaddr, unsigned int *valp); > int ufetch_long(const unsigned long *uaddr, unsigned long *valp); I would have expected the argument of a funtion called xxx_short etc. to be a short etc., not an unsigned short etc. Does it make sense to either make the argument char/short/int/long or change the name to ufetch_uXXX?
Re: RFC: New userspace fetch/store API
> On Feb 23, 2019, at 6:35 PM, Robert Elz wrote: > > fuword/fuiword/suword/suiword (etc) are all ancient, and I would > have expected, largely deprecated these days - but what's wrong > with copyin/copyout ?Those could be optimised for particular > data sizes if there was some real need for that. Hey Robert — The copyin / copyout API contract is like bcopy(); arbitrary byte alignment and no guarantees that the load / store to the specified memory location will be of any specific size, or will even be a single load or store operation. I thought there would value in having something like the old fetch / store API, which did provide those sorts of API guarantees, just freshened up a little. Furthermore, there isn’t a provision for using copyin / copyout in interrupt context (e.g. fuswintr()) … Now, it may be that is’s time to abandon that… but I’ll stick with the above argument. -- thorpej
Re: RFC: New userspace fetch/store API
fuword/fuiword/suword/suiword (etc) are all ancient, and I would have expected, largely deprecated these days - but what's wrong with copyin/copyout ?Those could be optimised for particular data sizes if there was some real need for that. kre
Re: RFC: New userspace fetch/store API
> On Feb 23, 2019, at 4:19 PM, Jason Thorpe wrote: >> On Feb 23, 2019, at 4:04 PM, matthew green wrote: >> >> i like this. the current API is ... odd. >> >> can you add alignment documentation? eg, if only to say that it >> must support aligned or unaligned accesses for the relevant >> datatypes. not sure what we need currently, ie, if unaligned is >> needed because that happens today, or we can define it as wrong >> in this API. > > Yah, I’ll make that clear. It’s implicit in the current API (alpha would > blow up otherwise), but I will make it explicit. I added the following: All addresses MUST be naturally-aligned for their type. If unaligned access is required, use copy(9). OK? -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 23, 2019, at 4:04 PM, matthew green wrote: > > i like this. the current API is ... odd. Oh… any thoughts on “have intrsafe or not”? -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 23, 2019, at 4:04 PM, matthew green wrote: > > i like this. the current API is ... odd. > > can you add alignment documentation? eg, if only to say that it > must support aligned or unaligned accesses for the relevant > datatypes. not sure what we need currently, ie, if unaligned is > needed because that happens today, or we can define it as wrong > in this API. Yah, I’ll make that clear. It’s implicit in the current API (alpha would blow up otherwise), but I will make it explicit. -- thorpej
re: RFC: New userspace fetch/store API
i like this. the current API is ... odd. can you add alignment documentation? eg, if only to say that it must support aligned or unaligned accesses for the relevant datatypes. not sure what we need currently, ie, if unaligned is needed because that happens today, or we can define it as wrong in this API. thanks. .mrg.
