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é <royger at freebsd.org> wrote:
>
>> On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:
>>> On Thu, 2018-04-05 at 14:31 +0000, 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 <sys/param.h>.

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