Re: svn commit: r332072 - head/sys/sys

[Roger Pau Monné]

On Fri, Apr 06, 2018 at 03:12:08AM +1000, Bruce Evans wrote:
> On Thu, 5 Apr 2018, Warner Losh wrote:
> 
> > On Thu, Apr 5, 2018 at 9:46 AM, Roger Pau Monné  wrote:
> > 
> > > On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:
> > > > On Thu, 2018-04-05 at 14:31 +, Roger Pau Monné wrote:
> > > > > Log:
> > > > >   introduce GiB and MiB macros
> > > > > ...
> > > > > +/* Unit conversion macros. */
> > > > > +#define GiB(v) (v ## ULL << 30)
> > > > > +#define MiB(v) (v ## ULL << 20)
> > > > > +
> > > > >  #endif /* _SYS_PARAM_H_ */
> > > > 
> > > > These names don't make it clear whether the conversion is bytes->GiB or
> > > > GiB->bytes.  The names seem way too generic for a public namespace in a
> > > > file as heavily included behind your back as param.h is.
> > > > 
> > > > Also, this completely reasonable usage won't work, likely with
> > > > confusing compile error messages:
> > > > 
> > > >   int bytes, gibytes;
> > > >   ...
> > > >   bytes = GiB(gibytes);
> > > 
> > > I find those helpful for their specific usage. I could introduce
> > > static inline functions like:
> > > 
> > > size_t gb_to_bytes(size_t)...
> > > 
> > > But I assume this is also going to cause further discussion.
> 
> Yes, it gives even more namespace pollution and type errors.  Macros
> at least don't expose their internals if they are not used.
> 
> size_t is actually already part of the undocumented namespace pollution
> in .
> 
> The type errors are restriction to just one type in another way.  Type-
> generic APIs that avoid such restrictions are much harder to implement
> using inline functions than macros.
> 
> > Yea, traditional macro names would be "gibtob" and "btogib" but I didn't
> > just reply to bikeshed a name:
> > 
> > But you don't need to specify a type, consider the current btodb macro:
> > #define btodb(bytes)/* calculates (bytes / DEV_BSIZE)
> > */ \
> >(sizeof (bytes) > sizeof(long) \
> > ? (daddr_t)((unsigned long long)(bytes) >> DEV_BSHIFT) \
> > : (daddr_t)((unsigned long)(bytes) >> DEV_BSHIFT))
> > 
> > which shows how to do this in a macro, which is orthogonal to any name you
> > may choose. I can also bikeshed function vs macro :)
> 
> This macro is mostly my mistake in 1995-1996.  The long long abominations
> in it were supposed to be temporary (until C99 standardized something
> better).  It was originally MD for i386 only and then the sizes of almost
> all types are known and fixed so it is easier to hard-code minimal sizes
> that work.  The optimization of avoiding using 64-bit types was more needed
> in 1995-1996 since CPUs were slower and compilers did less strength reduction.
> 
> btodb() is much easier than dbtob() since it shifts right, so it can't
> overflow unless the cast of 'bytes' is wrong so that it truncations.
> dbtob() doesn't try hard to be optimal.  It just always upcasts to
> off_t.
> 
> jake later convinced me (in connection with his PAE and sparc64 work) that
> it should be the caller's responsibility to avoid overflow.  Any casts in
> the macro limits it to the types in it.  This is why the page to byte
> conversion macros don't have any casts in them.  PAE usually needs 64-bit
> results, but this would just be a pessimization for normal i386, and
> deciding the casts in the macro as above is complicated.
> 
> So correct GB() macros would look like ((v) << 30), where the caller must
> cast v to a large enough type.  E.g., for variable v which might be larger
> than 4, on 32-bit systems, the caller must write something like
> GB((uintmax_t)v).  But it is easier for writing to just multiply v by 1G.
> This is also easier for reading since it is unclear that GB() is even a
> conversion or which direction it goes in.  A longer descriptive name would
> be about as clear and long as an explicit multiplication.
> 
> I usually write 1G as ((type)1024 * 1024 * 1024) since the decimal and
> even hex values of 1G have too many digits to be clear, and
> multiplication is clearer than shifting and allows the type to be in
> the factor.
> 
> Disk block size conversions need to use macros since the DEV_BSIZE = 512
> was variable in theory (in practice this is now a fixed virtual size).
> Conversions to G don't need macros since the magic number in them is no
> more magic than the G in their name.

I personally find the following chunk:

if (addr < GiB(4))
...

Much more easier to read and parse than:

if (addr < (4 * 1024 * 1024 * 1024))
...

But I won't insist anymore.

I will revert this and introduce the macros locally where I need them.

Roger.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r332072 - head/sys/sys

[Bruce Evans]

On Thu, 5 Apr 2018, Warner Losh wrote:


On Thu, Apr 5, 2018 at 9:46 AM, Roger Pau Monn??  wrote:


On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:

On Thu, 2018-04-05 at 14:31 +, Roger Pau Monn?? wrote:

Log:
  introduce GiB and MiB macros
...
+/* Unit conversion macros. */
+#define GiB(v) (v ## ULL << 30)
+#define MiB(v) (v ## ULL << 20)
+
 #endif /* _SYS_PARAM_H_ */


These names don't make it clear whether the conversion is bytes->GiB or
GiB->bytes.  The names seem way too generic for a public namespace in a
file as heavily included behind your back as param.h is.

Also, this completely reasonable usage won't work, likely with
confusing compile error messages:

  int bytes, gibytes;
  ...
  bytes = GiB(gibytes);


I find those helpful for their specific usage. I could introduce
static inline functions like:

size_t gb_to_bytes(size_t)...

But I assume this is also going to cause further discussion.


Yes, it gives even more namespace pollution and type errors.  Macros
at least don't expose their internals if they are not used.

size_t is actually already part of the undocumented namespace pollution
in .

The type errors are restriction to just one type in another way.  Type-
generic APIs that avoid such restrictions are much harder to implement
using inline functions than macros.


Yea, traditional macro names would be "gibtob" and "btogib" but I didn't
just reply to bikeshed a name:

But you don't need to specify a type, consider the current btodb macro:
#define btodb(bytes)/* calculates (bytes / DEV_BSIZE)
*/ \
   (sizeof (bytes) > sizeof(long) \
? (daddr_t)((unsigned long long)(bytes) >> DEV_BSHIFT) \
: (daddr_t)((unsigned long)(bytes) >> DEV_BSHIFT))

which shows how to do this in a macro, which is orthogonal to any name you
may choose. I can also bikeshed function vs macro :)


This macro is mostly my mistake in 1995-1996.  The long long abominations
in it were supposed to be temporary (until C99 standardized something
better).  It was originally MD for i386 only and then the sizes of almost
all types are known and fixed so it is easier to hard-code minimal sizes
that work.  The optimization of avoiding using 64-bit types was more needed
in 1995-1996 since CPUs were slower and compilers did less strength reduction.

btodb() is much easier than dbtob() since it shifts right, so it can't
overflow unless the cast of 'bytes' is wrong so that it truncations.
dbtob() doesn't try hard to be optimal.  It just always upcasts to
off_t.

jake later convinced me (in connection with his PAE and sparc64 work) that
it should be the caller's responsibility to avoid overflow.  Any casts in
the macro limits it to the types in it.  This is why the page to byte
conversion macros don't have any casts in them.  PAE usually needs 64-bit
results, but this would just be a pessimization for normal i386, and
deciding the casts in the macro as above is complicated.

So correct GB() macros would look like ((v) << 30), where the caller must
cast v to a large enough type.  E.g., for variable v which might be larger
than 4, on 32-bit systems, the caller must write something like
GB((uintmax_t)v).  But it is easier for writing to just multiply v by 1G.
This is also easier for reading since it is unclear that GB() is even a
conversion or which direction it goes in.  A longer descriptive name would
be about as clear and long as an explicit multiplication.

I usually write 1G as ((type)1024 * 1024 * 1024) since the decimal and
even hex values of 1G have too many digits to be clear, and
multiplication is clearer than shifting and allows the type to be in
the factor.

Disk block size conversions need to use macros since the DEV_BSIZE = 512
was variable in theory (in practice this is now a fixed virtual size).
Conversions to G don't need macros since the magic number in them is no
more magic than the G in their name.

Bruce___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r332072 - head/sys/sys

[Roger Pau Monné]

On Thu, Apr 05, 2018 at 09:58:36AM -0600, Warner Losh wrote:
> On Thu, Apr 5, 2018 at 9:46 AM, Roger Pau Monné  wrote:
> 
> > On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:
> > > On Thu, 2018-04-05 at 14:31 +, Roger Pau Monné wrote:
> > > > Author: royger
> > > > Date: Thu Apr  5 14:31:54 2018
> > > > New Revision: 332072
> > > > URL: https://svnweb.freebsd.org/changeset/base/332072
> > > >
> > > > Log:
> > > >   introduce GiB and MiB macros
> > > >
> > > >   This macros convert from GiB or MiB into bytes.
> > > >
> > > >   Sponsored by: Citrix Systems R
> > > >
> > > > Modified:
> > > >   head/sys/sys/param.h
> > > >
> > > > Modified: head/sys/sys/param.h
> > > > 
> > ==
> > > > --- head/sys/sys/param.hThu Apr  5 14:25:39 2018(r332071)
> > > > +++ head/sys/sys/param.hThu Apr  5 14:31:54 2018(r332072)
> > > > @@ -362,4 +362,8 @@ __END_DECLS
> > > >   */
> > > >  #define __PAST_END(array, offset) (((__typeof__(*(array))
> > *)(array))[offset])
> > > >
> > > > +/* Unit conversion macros. */
> > > > +#define GiB(v) (v ## ULL << 30)
> > > > +#define MiB(v) (v ## ULL << 20)
> > > > +
> > > >  #endif /* _SYS_PARAM_H_ */
> > > >
> > >
> > > These names don't make it clear whether the conversion is bytes->GiB or
> > > GiB->bytes.  The names seem way too generic for a public namespace in a
> > > file as heavily included behind your back as param.h is.
> > >
> > > Also, this completely reasonable usage won't work, likely with
> > > confusing compile error messages:
> > >
> > >   int bytes, gibytes;
> > >   ...
> > >   bytes = GiB(gibytes);
> >
> > I find those helpful for their specific usage. I could introduce
> > static inline functions like:
> >
> > size_t gb_to_bytes(size_t)...
> >
> > But I assume this is also going to cause further discussion.
> >
> 
> Yea, traditional macro names would be "gibtob" and "btogib" but I didn't
> just reply to bikeshed a name:
> 
> But you don't need to specify a type, consider the current btodb macro:
> #define btodb(bytes)/* calculates (bytes / DEV_BSIZE)
> */ \
> (sizeof (bytes) > sizeof(long) \
>  ? (daddr_t)((unsigned long long)(bytes) >> DEV_BSHIFT) \
>  : (daddr_t)((unsigned long)(bytes) >> DEV_BSHIFT))
> 
> which shows how to do this in a macro, which is orthogonal to any name you
> may choose. I can also bikeshed function vs macro :)

I was just going to remove those from here and place them in the files
where I use them, but I can also change them to:

#define gibtob(gib) ((unsigned long long)(gib) << 30)
#define mibtob(mib) ((unsigned long long)(mib) << 20)

If it's not going to start a bikeshed.

Thanks, Roger.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r332072 - head/sys/sys

[Warner Losh]

On Thu, Apr 5, 2018 at 9:46 AM, Roger Pau Monné  wrote:

> On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:
> > On Thu, 2018-04-05 at 14:31 +, Roger Pau Monné wrote:
> > > Author: royger
> > > Date: Thu Apr  5 14:31:54 2018
> > > New Revision: 332072
> > > URL: https://svnweb.freebsd.org/changeset/base/332072
> > >
> > > Log:
> > >   introduce GiB and MiB macros
> > >
> > >   This macros convert from GiB or MiB into bytes.
> > >
> > >   Sponsored by: Citrix Systems R
> > >
> > > Modified:
> > >   head/sys/sys/param.h
> > >
> > > Modified: head/sys/sys/param.h
> > > 
> ==
> > > --- head/sys/sys/param.hThu Apr  5 14:25:39 2018(r332071)
> > > +++ head/sys/sys/param.hThu Apr  5 14:31:54 2018(r332072)
> > > @@ -362,4 +362,8 @@ __END_DECLS
> > >   */
> > >  #define __PAST_END(array, offset) (((__typeof__(*(array))
> *)(array))[offset])
> > >
> > > +/* Unit conversion macros. */
> > > +#define GiB(v) (v ## ULL << 30)
> > > +#define MiB(v) (v ## ULL << 20)
> > > +
> > >  #endif /* _SYS_PARAM_H_ */
> > >
> >
> > These names don't make it clear whether the conversion is bytes->GiB or
> > GiB->bytes.  The names seem way too generic for a public namespace in a
> > file as heavily included behind your back as param.h is.
> >
> > Also, this completely reasonable usage won't work, likely with
> > confusing compile error messages:
> >
> >   int bytes, gibytes;
> >   ...
> >   bytes = GiB(gibytes);
>
> I find those helpful for their specific usage. I could introduce
> static inline functions like:
>
> size_t gb_to_bytes(size_t)...
>
> But I assume this is also going to cause further discussion.
>

Yea, traditional macro names would be "gibtob" and "btogib" but I didn't
just reply to bikeshed a name:

But you don't need to specify a type, consider the current btodb macro:
#define btodb(bytes)/* calculates (bytes / DEV_BSIZE)
*/ \
(sizeof (bytes) > sizeof(long) \
 ? (daddr_t)((unsigned long long)(bytes) >> DEV_BSHIFT) \
 : (daddr_t)((unsigned long)(bytes) >> DEV_BSHIFT))

which shows how to do this in a macro, which is orthogonal to any name you
may choose. I can also bikeshed function vs macro :)

Warner
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r332072 - head/sys/sys

[Roger Pau Monné]

On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:
> On Thu, 2018-04-05 at 14:31 +, Roger Pau Monné wrote:
> > Author: royger
> > Date: Thu Apr  5 14:31:54 2018
> > New Revision: 332072
> > URL: https://svnweb.freebsd.org/changeset/base/332072
> > 
> > Log:
> >   introduce GiB and MiB macros
> >   
> >   This macros convert from GiB or MiB into bytes.
> >   
> >   Sponsored by: Citrix Systems R
> > 
> > Modified:
> >   head/sys/sys/param.h
> > 
> > Modified: head/sys/sys/param.h
> > ==
> > --- head/sys/sys/param.hThu Apr  5 14:25:39 2018(r332071)
> > +++ head/sys/sys/param.hThu Apr  5 14:31:54 2018(r332072)
> > @@ -362,4 +362,8 @@ __END_DECLS
> >   */
> >  #define __PAST_END(array, offset) (((__typeof__(*(array)) 
> > *)(array))[offset])
> >  
> > +/* Unit conversion macros. */
> > +#define GiB(v) (v ## ULL << 30)
> > +#define MiB(v) (v ## ULL << 20)
> > +
> >  #endif /* _SYS_PARAM_H_ */
> > 
> 
> These names don't make it clear whether the conversion is bytes->GiB or
> GiB->bytes.  The names seem way too generic for a public namespace in a
> file as heavily included behind your back as param.h is.
> 
> Also, this completely reasonable usage won't work, likely with
> confusing compile error messages:
> 
>   int bytes, gibytes;
>   ...
>   bytes = GiB(gibytes);

I find those helpful for their specific usage. I could introduce
static inline functions like:

size_t gb_to_bytes(size_t)...

But I assume this is also going to cause further discussion.

Roger.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r332072 - head/sys/sys

[Ian Lepore]

On Thu, 2018-04-05 at 14:31 +, Roger Pau Monné wrote:
> Author: royger
> Date: Thu Apr  5 14:31:54 2018
> New Revision: 332072
> URL: https://svnweb.freebsd.org/changeset/base/332072
> 
> Log:
>   introduce GiB and MiB macros
>   
>   This macros convert from GiB or MiB into bytes.
>   
>   Sponsored by: Citrix Systems R
> 
> Modified:
>   head/sys/sys/param.h
> 
> Modified: head/sys/sys/param.h
> ==
> --- head/sys/sys/param.h  Thu Apr  5 14:25:39 2018(r332071)
> +++ head/sys/sys/param.h  Thu Apr  5 14:31:54 2018(r332072)
> @@ -362,4 +362,8 @@ __END_DECLS
>   */
>  #define __PAST_END(array, offset) (((__typeof__(*(array)) *)(array))[offset])
>  
> +/* Unit conversion macros. */
> +#define GiB(v) (v ## ULL << 30)
> +#define MiB(v) (v ## ULL << 20)
> +
>  #endif   /* _SYS_PARAM_H_ */
> 

These names don't make it clear whether the conversion is bytes->GiB or
GiB->bytes.  The names seem way too generic for a public namespace in a
file as heavily included behind your back as param.h is.

Also, this completely reasonable usage won't work, likely with
confusing compile error messages:

  int bytes, gibytes;
  ...
  bytes = GiB(gibytes);

-- Ian

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r332072 - head/sys/sys

[Bruce Evans]

On Thu, 5 Apr 2018, [UTF-8] Roger Pau Monn?? wrote:


Log:
 introduce GiB and MiB macros

 This macros convert from GiB or MiB into bytes.


This is undocumented namspace pollution with bad names and worse types.

The better names GB and MB would be more likely to conflicted with
code not written by disk marketers.


Modified: head/sys/sys/param.h
==
--- head/sys/sys/param.hThu Apr  5 14:25:39 2018(r332071)
+++ head/sys/sys/param.hThu Apr  5 14:31:54 2018(r332072)
@@ -362,4 +362,8 @@ __END_DECLS
 */
#define __PAST_END(array, offset) (((__typeof__(*(array)) *)(array))[offset])


Old style bug: space instead of tab after #define.



+/* Unit conversion macros. */
+#define GiB(v) (v ## ULL << 30)
+#define MiB(v) (v ## ULL << 20)
+


New style bugs:
- space instead of tab after #define
- use of the long long abomination

Type error:
- the abomination doesn't have the same type of carefully typedefed types
  like vm_size_t on any supported arch.


#endif  /* _SYS_PARAM_H_ */


Old style bugs:
- tab instead of space before comment on #endif.
- backwards comment on #endif.

Bruce___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r332072 - head/sys/sys

[Roger Pau Monné]

Author: royger
Date: Thu Apr  5 14:31:54 2018
New Revision: 332072
URL: https://svnweb.freebsd.org/changeset/base/332072

Log:
  introduce GiB and MiB macros
  
  This macros convert from GiB or MiB into bytes.
  
  Sponsored by: Citrix Systems R

Modified:
  head/sys/sys/param.h

Modified: head/sys/sys/param.h
==
--- head/sys/sys/param.hThu Apr  5 14:25:39 2018(r332071)
+++ head/sys/sys/param.hThu Apr  5 14:31:54 2018(r332072)
@@ -362,4 +362,8 @@ __END_DECLS
  */
 #define __PAST_END(array, offset) (((__typeof__(*(array)) *)(array))[offset])
 
+/* Unit conversion macros. */
+#define GiB(v) (v ## ULL << 30)
+#define MiB(v) (v ## ULL << 20)
+
 #endif /* _SYS_PARAM_H_ */
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"