Re: svn commit: r330285 - head/sys/sys
On 03/03/2018 05:21, Konstantin Belousov wrote:
On Sat, Mar 03, 2018 at 01:47:41PM +1100, Bruce Evans wrote:
On Fri, 2 Mar 2018, Konstantin Belousov wrote:
On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote:
...
I think use of _Nonnull attributes in the threading functions may also
be a waste (I introduced them mostly to be compatible with Android).
FWIW, Dragonfly sprinkled some restrict there recently:
http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492
Just in case someone is considering more cleanups.
This is not a cleanup for me, but a needed change. Right now x86
copyouts are implemented in asm, so whatever damage is done to the
prototypes, only effect is at the caller side. In my work, i386 copyouts
are done in C, so it starts matter.
That seems slow, especially for small sizes as are common for syscall args
(in 1 of my versions, copyin() of args is optimized to fuword() in a loop,
and fuword() is optimized to not use pcb_onfault, so it is not much more
than 1 memory access. However, in your i386 version this optimization
would be negative since the slow part is switching the map, so fuword()
should never be used to access multiple words).
Yes. I already explained it in private, the current choice for i386 is
either to be neglected very fast, or to get this change to still be a
Tier 1 32 bit platform. The change is to make 4/4g split for UVA/KVA.
In particular, the change ensures that it is possible to self-host i386
for forthcoming years, which is not practical for armv7 now and would be
less so with clang grow.
In other news, my system already boots single-user on SMP machine and
I have torture tests like setting invalid %ss segment by sigreturn(2),
work. There is (much) more to come, but I am happy how the patch
progressed so far.
Very nice.
However, copyinstr() and
copystr() should never have been "optimized" by writing them in asm. On
x86, their asm is badly written so they are slower than simple C versions
except on 8088's and maybe 8086's and maybe on the original i386. (8088's
were limited mainly by instruction bandwidth and the original i386 wasn't
much better, so short CISC instructions like lodsb and stosb tended to be
faster than larger separate instructions despite their large setup overheads.
Sure, copyinstr() is rewritten in C. The current version of copyout stuff
is there:
https://kib.kiev.ua/git/gitweb.cgi?p=deviant2.git;a=blob;f=sys/i386/i386/copyout.c;h=9747c06a84d7d2b5faac946f5de57f6a34d96c8c;hb=refs/heads/pcid
Also I looked at the dragonfly commit because I become curious what do you
mean by threading functions. The first example was
intpthread_attr_getguardsize(const pthread_attr_t * __restrict,
- size_t *);
+ size_t * __restrict);
POSIX agrees with the dragonfly change, but I do not understand it.
Aliasing rules already disallow the first and second arguments to point
to the same memory, because they have different types.
(1) thread_attr_t is opaque, so the types might be the same.
(2) pthread_attr_t might be a pointer to a struct/union containing a size_t.
(3) perhaps other reasons. I'm not sure how 'restrict interacts with global
variables or even it it prevents the interaction in (2). A previous
discussion showed that const doesn't make types different enough to
prevent aliasing. Similarly for volatile.
Similarly for other pointers to {opaque, struct/union, or even integer} types.
size_t can't be aliased to int, but it can be aliased to any unsigned type
in C and to any unsigned type not smaller than uint16_t in POSIX (POSIX
but not C requires u_char == uint8_t, so size_t can't be u_char in POSIX
but it can be u_char in C).
I can only summarize it as 'there is no use to have restrict on the
pthread_attr_getguardsize() arguments'.
Well, I'll admit I don't understand well the advantages and that's why I
brought up a pointer to the changes instead of working on them. Usually,
standards compliance is reason enough for such change though.
Pedro.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r330285 - head/sys/sys
On Sat, 3 Mar 2018, Konstantin Belousov wrote:
On Sat, Mar 03, 2018 at 01:47:41PM +1100, Bruce Evans wrote:
On Fri, 2 Mar 2018, Konstantin Belousov wrote:
On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote:
...
I think use of _Nonnull attributes in the threading functions may also
be a waste (I introduced them mostly to be compatible with Android).
FWIW, Dragonfly sprinkled some restrict there recently:
http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492
Just in case someone is considering more cleanups.
This is not a cleanup for me, but a needed change. Right now x86
copyouts are implemented in asm, so whatever damage is done to the
prototypes, only effect is at the caller side. In my work, i386 copyouts
are done in C, so it starts matter.
That seems slow, especially for small sizes as are common for syscall args
(in 1 of my versions, copyin() of args is optimized to fuword() in a loop,
and fuword() is optimized to not use pcb_onfault, so it is not much more
than 1 memory access. However, in your i386 version this optimization
would be negative since the slow part is switching the map, so fuword()
should never be used to access multiple words).
Yes. I already explained it in private, the current choice for i386 is
either to be neglected very fast, or to get this change to still be a
Tier 1 32 bit platform. The change is to make 4/4g split for UVA/KVA.
In particular, the change ensures that it is possible to self-host i386
for forthcoming years, which is not practical for armv7 now and would be
less so with clang grow.
I use i386 since it is 10-20% faster than amd64 for my applications,
and don't like changes that fix this by slowing down the fast case. My
applications don't include clang.
...
Also I looked at the dragonfly commit because I become curious what do you
mean by threading functions. The first example was
intpthread_attr_getguardsize(const pthread_attr_t * __restrict,
- size_t *);
+ size_t * __restrict);
POSIX agrees with the dragonfly change, but I do not understand it.
Aliasing rules already disallow the first and second arguments to point
to the same memory, because they have different types.
(1) thread_attr_t is opaque, so the types might be the same.
(2) pthread_attr_t might be a pointer to a struct/union containing a size_t.
(3) perhaps other reasons. I'm not sure how 'restrict interacts with global
variables or even it it prevents the interaction in (2). A previous
discussion showed that const doesn't make types different enough to
prevent aliasing. Similarly for volatile.
Similarly for other pointers to {opaque, struct/union, or even integer} types.
size_t can't be aliased to int, but it can be aliased to any unsigned type
in C and to any unsigned type not smaller than uint16_t in POSIX (POSIX
but not C requires u_char == uint8_t, so size_t can't be u_char in POSIX
but it can be u_char in C).
I can only summarize it as 'there is no use to have restrict on the
pthread_attr_getguardsize() arguments'.
No, the summary is 'POSIX is correct to declare almost all arg pointers
as restrict, since it is not useful for new APIs to allow aliases, and
restrict must be used so that implementations can take advantage of this
if they want'.
Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r330285 - head/sys/sys
On Sat, Mar 03, 2018 at 01:47:41PM +1100, Bruce Evans wrote:
> On Fri, 2 Mar 2018, Konstantin Belousov wrote:
>
> > On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote:
> >> ...
> >> I think use of _Nonnull attributes in the threading functions may also
> >> be a waste (I introduced them mostly to be compatible with Android).
> >> FWIW, Dragonfly sprinkled some restrict there recently:
> >>
> >> http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492
> >>
> >> Just in case someone is considering more cleanups.
> >
> > This is not a cleanup for me, but a needed change. Right now x86
> > copyouts are implemented in asm, so whatever damage is done to the
> > prototypes, only effect is at the caller side. In my work, i386 copyouts
> > are done in C, so it starts matter.
>
> That seems slow, especially for small sizes as are common for syscall args
> (in 1 of my versions, copyin() of args is optimized to fuword() in a loop,
> and fuword() is optimized to not use pcb_onfault, so it is not much more
> than 1 memory access. However, in your i386 version this optimization
> would be negative since the slow part is switching the map, so fuword()
> should never be used to access multiple words).
Yes. I already explained it in private, the current choice for i386 is
either to be neglected very fast, or to get this change to still be a
Tier 1 32 bit platform. The change is to make 4/4g split for UVA/KVA.
In particular, the change ensures that it is possible to self-host i386
for forthcoming years, which is not practical for armv7 now and would be
less so with clang grow.
In other news, my system already boots single-user on SMP machine and
I have torture tests like setting invalid %ss segment by sigreturn(2),
work. There is (much) more to come, but I am happy how the patch
progressed so far.
> However, copyinstr() and
> copystr() should never have been "optimized" by writing them in asm. On
> x86, their asm is badly written so they are slower than simple C versions
> except on 8088's and maybe 8086's and maybe on the original i386. (8088's
> were limited mainly by instruction bandwidth and the original i386 wasn't
> much better, so short CISC instructions like lodsb and stosb tended to be
> faster than larger separate instructions despite their large setup overheads.
Sure, copyinstr() is rewritten in C. The current version of copyout stuff
is there:
https://kib.kiev.ua/git/gitweb.cgi?p=deviant2.git;a=blob;f=sys/i386/i386/copyout.c;h=9747c06a84d7d2b5faac946f5de57f6a34d96c8c;hb=refs/heads/pcid
>
> > Also I looked at the dragonfly commit because I become curious what do you
> > mean by threading functions. The first example was
> > intpthread_attr_getguardsize(const pthread_attr_t * __restrict,
> > - size_t *);
> > + size_t * __restrict);
> > POSIX agrees with the dragonfly change, but I do not understand it.
> > Aliasing rules already disallow the first and second arguments to point
> > to the same memory, because they have different types.
>
> (1) thread_attr_t is opaque, so the types might be the same.
> (2) pthread_attr_t might be a pointer to a struct/union containing a size_t.
> (3) perhaps other reasons. I'm not sure how 'restrict interacts with global
> variables or even it it prevents the interaction in (2). A previous
> discussion showed that const doesn't make types different enough to
> prevent aliasing. Similarly for volatile.
>
> Similarly for other pointers to {opaque, struct/union, or even integer} types.
> size_t can't be aliased to int, but it can be aliased to any unsigned type
> in C and to any unsigned type not smaller than uint16_t in POSIX (POSIX
> but not C requires u_char == uint8_t, so size_t can't be u_char in POSIX
> but it can be u_char in C).
I can only summarize it as 'there is no use to have restrict on the
pthread_attr_getguardsize() arguments'.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r330285 - head/sys/sys
On Fri, 2 Mar 2018, Konstantin Belousov wrote:
On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote:
...
I think use of _Nonnull attributes in the threading functions may also
be a waste (I introduced them mostly to be compatible with Android).
FWIW, Dragonfly sprinkled some restrict there recently:
http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492
Just in case someone is considering more cleanups.
This is not a cleanup for me, but a needed change. Right now x86
copyouts are implemented in asm, so whatever damage is done to the
prototypes, only effect is at the caller side. In my work, i386 copyouts
are done in C, so it starts matter.
That seems slow, especially for small sizes as are common for syscall args
(in 1 of my versions, copyin() of args is optimized to fuword() in a loop,
and fuword() is optimized to not use pcb_onfault, so it is not much more
than 1 memory access. However, in your i386 version this optimization
would be negative since the slow part is switching the map, so fuword()
should never be used to access multiple words). However, copyinstr() and
copystr() should never have been "optimized" by writing them in asm. On
x86, their asm is badly written so they are slower than simple C versions
except on 8088's and maybe 8086's and maybe on the original i386. (8088's
were limited mainly by instruction bandwidth and the original i386 wasn't
much better, so short CISC instructions like lodsb and stosb tended to be
faster than larger separate instructions despite their large setup overheads.
Also I looked at the dragonfly commit because I become curious what do you
mean by threading functions. The first example was
intpthread_attr_getguardsize(const pthread_attr_t * __restrict,
- size_t *);
+ size_t * __restrict);
POSIX agrees with the dragonfly change, but I do not understand it.
Aliasing rules already disallow the first and second arguments to point
to the same memory, because they have different types.
(1) thread_attr_t is opaque, so the types might be the same.
(2) pthread_attr_t might be a pointer to a struct/union containing a size_t.
(3) perhaps other reasons. I'm not sure how 'restrict interacts with global
variables or even it it prevents the interaction in (2). A previous
discussion showed that const doesn't make types different enough to
prevent aliasing. Similarly for volatile.
Similarly for other pointers to {opaque, struct/union, or even integer} types.
size_t can't be aliased to int, but it can be aliased to any unsigned type
in C and to any unsigned type not smaller than uint16_t in POSIX (POSIX
but not C requires u_char == uint8_t, so size_t can't be u_char in POSIX
but it can be u_char in C).
Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r330285 - head/sys/sys
On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote: > (cc in Eitan as he may be interested in the extra restrict cases) > > > On 02/03/2018 11:47, Konstantin Belousov wrote: > > Author: kib > > Date: Fri Mar 2 16:47:02 2018 > > New Revision: 330285 > > URL: https://svnweb.freebsd.org/changeset/base/330285 > > > > Log: > >Remove _Nonnull attributes from user addresses arguments for > >copyout(9) family. > > > >The addresses are user-controllable, and if the process ABI allows > >mapping at zero, then the zero address is meaningful, contradicting > >the definition of _Nonnull. In any case, it does not require any > >special code to handle NULL udaddr. > > > > FWIW, the _Nonnull attributes didn't do much at all beyond producing a > warning. > They replaced the GNU __nonnull() attributes which were much more dangerous. > I am OK with seeing both gone here though. > > >It is not clear if __restrict makes sense as well, since kaddr and > >udaddr point to different address spaces, so equal numeric values of > >the pointers do not imply aliasing and a legitimate. But leave it for > >later. > > > >copyinstr(9) does not have its user address argument annotated. > > I think use of _Nonnull attributes in the threading functions may also > be a waste (I introduced them mostly to be compatible with Android). > FWIW, Dragonfly sprinkled some restrict there recently: > > http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492 > > Just in case someone is considering more cleanups. This is not a cleanup for me, but a needed change. Right now x86 copyouts are implemented in asm, so whatever damage is done to the prototypes, only effect is at the caller side. In my work, i386 copyouts are done in C, so it starts matter. Also I looked at the dragonfly commit because I become curious what do you mean by threading functions. The first example was intpthread_attr_getguardsize(const pthread_attr_t * __restrict, - size_t *); + size_t * __restrict); POSIX agrees with the dragonfly change, but I do not understand it. Aliasing rules already disallow the first and second arguments to point to the same memory, because they have different types. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r330285 - head/sys/sys
On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote: > (cc in Eitan as he may be interested in the extra restrict cases) > > > On 02/03/2018 11:47, Konstantin Belousov wrote: > > Author: kib > > Date: Fri Mar 2 16:47:02 2018 > > New Revision: 330285 > > URL: https://svnweb.freebsd.org/changeset/base/330285 > > > > Log: > >Remove _Nonnull attributes from user addresses arguments for > >copyout(9) family. > > > >The addresses are user-controllable, and if the process ABI allows > >mapping at zero, then the zero address is meaningful, contradicting > >the definition of _Nonnull. In any case, it does not require any > >special code to handle NULL udaddr. > > > > FWIW, the _Nonnull attributes didn't do much at all beyond producing a > warning. > They replaced the GNU __nonnull() attributes which were much more dangerous. > I am OK with seeing both gone here though. Even if the process ABI doesn't allow mapping at NULL, we have code that depends on copyout(NULL, foo, 0) being a nop. -- Brooks signature.asc Description: PGP signature
Re: svn commit: r330285 - head/sys/sys
(cc in Eitan as he may be interested in the extra restrict cases) On 02/03/2018 11:47, Konstantin Belousov wrote: Author: kib Date: Fri Mar 2 16:47:02 2018 New Revision: 330285 URL: https://svnweb.freebsd.org/changeset/base/330285 Log: Remove _Nonnull attributes from user addresses arguments for copyout(9) family. The addresses are user-controllable, and if the process ABI allows mapping at zero, then the zero address is meaningful, contradicting the definition of _Nonnull. In any case, it does not require any special code to handle NULL udaddr. FWIW, the _Nonnull attributes didn't do much at all beyond producing a warning. They replaced the GNU __nonnull() attributes which were much more dangerous. I am OK with seeing both gone here though. It is not clear if __restrict makes sense as well, since kaddr and udaddr point to different address spaces, so equal numeric values of the pointers do not imply aliasing and a legitimate. But leave it for later. copyinstr(9) does not have its user address argument annotated. I think use of _Nonnull attributes in the threading functions may also be a waste (I introduced them mostly to be compatible with Android). FWIW, Dragonfly sprinkled some restrict there recently: http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492 Just in case someone is considering more cleanups. Cheers, Pedro. Sponsored by:The FreeBSD Foundation MFC after: 1 week Modified: head/sys/sys/systm.h Modified: head/sys/sys/systm.h == --- head/sys/sys/systm.hFri Mar 2 16:31:23 2018(r330284) +++ head/sys/sys/systm.hFri Mar 2 16:47:02 2018(r330285) @@ -277,14 +277,14 @@ int copystr(const void * _Nonnull __restrict kfaddr, int copyinstr(const void * __restrict udaddr, void * _Nonnull __restrict kaddr, size_t len, size_t * __restrict lencopied); -intcopyin(const void * _Nonnull __restrict udaddr, +intcopyin(const void * __restrict udaddr, void * _Nonnull __restrict kaddr, size_t len); -intcopyin_nofault(const void * _Nonnull __restrict udaddr, +intcopyin_nofault(const void * __restrict udaddr, void * _Nonnull __restrict kaddr, size_t len); int copyout(const void * _Nonnull __restrict kaddr, - void * _Nonnull __restrict udaddr, size_t len); + void * __restrict udaddr, size_t len); int copyout_nofault(const void * _Nonnull __restrict kaddr, - void * _Nonnull __restrict udaddr, size_t len); + void * __restrict udaddr, size_t len); int fubyte(volatile const void *base); long fuword(volatile const void *base); ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r330285 - head/sys/sys
Author: kib Date: Fri Mar 2 16:47:02 2018 New Revision: 330285 URL: https://svnweb.freebsd.org/changeset/base/330285 Log: Remove _Nonnull attributes from user addresses arguments for copyout(9) family. The addresses are user-controllable, and if the process ABI allows mapping at zero, then the zero address is meaningful, contradicting the definition of _Nonnull. In any case, it does not require any special code to handle NULL udaddr. It is not clear if __restrict makes sense as well, since kaddr and udaddr point to different address spaces, so equal numeric values of the pointers do not imply aliasing and a legitimate. But leave it for later. copyinstr(9) does not have its user address argument annotated. Sponsored by: The FreeBSD Foundation MFC after:1 week Modified: head/sys/sys/systm.h Modified: head/sys/sys/systm.h == --- head/sys/sys/systm.hFri Mar 2 16:31:23 2018(r330284) +++ head/sys/sys/systm.hFri Mar 2 16:47:02 2018(r330285) @@ -277,14 +277,14 @@ int copystr(const void * _Nonnull __restrict kfaddr, intcopyinstr(const void * __restrict udaddr, void * _Nonnull __restrict kaddr, size_t len, size_t * __restrict lencopied); -intcopyin(const void * _Nonnull __restrict udaddr, +intcopyin(const void * __restrict udaddr, void * _Nonnull __restrict kaddr, size_t len); -intcopyin_nofault(const void * _Nonnull __restrict udaddr, +intcopyin_nofault(const void * __restrict udaddr, void * _Nonnull __restrict kaddr, size_t len); intcopyout(const void * _Nonnull __restrict kaddr, - void * _Nonnull __restrict udaddr, size_t len); + void * __restrict udaddr, size_t len); intcopyout_nofault(const void * _Nonnull __restrict kaddr, - void * _Nonnull __restrict udaddr, size_t len); + void * __restrict udaddr, size_t len); intfubyte(volatile const void *base); long fuword(volatile const void *base); ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
