Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:
> Hi Andy,
>
> Thanks for the review.
>
> On 26/08/24 23:14, Andy Shevchenko wrote:
>> On Mon, Aug 26, 2024 at 08:38:17PM +0530, Devarsh Thakkar wrote:
>>> Add below rounding related macros:
>>>
>>> round_closest_up(x, y) : Rounds x to the closest multiple of y where y is a
>>> power of 2, with a preference to round up in case two nearest values are
>>> possible.
>>>
>>> round_closest_down(x, y) : Rounds x to the closest multiple of y where y is
>>> a power of 2, with a preference to round down in case two nearest values
>>> are possible.
>>>
>>> roundclosest(x, y) : Rounds x to the closest multiple of y, this macro
>>> should generally be used only when y is not multiple of 2 as otherwise
>>> round_closest* macros should be used which are much faster.
>> 
>> I understand the point, but if you need to send a v3, please explain
>> the equivalency between roundclosest() and one (or both?) of the
>> round_closest_*() in case the argument is power-of-2.
>> 
>
> The equivalency between roundclosest w.r.t round_closest is same as
> equivalency between existing macros rounddown w.r.t round_down. Functionally
> both are same but the former is recommended to be used only for the scenario
> where multiple is not power of 2 and latter is faster but is strictly for the
> scenario where multiple is power of 2. I think the same is already summarized
> well in commit message and further elaborated in the patch itself as part of
> header file comments [1] so I personally don't think any update is required
> w.r.t this.

I still don't think rounddown vs. round_down naming is a good example to
model anything after.

I have yet to hear a single compelling argument in favor of having a
single underscore in the middle of a name having implications about the
inputs of a macro/function.

The macros being added here are at 2 or 3 in Rusty's scale [1]. We could
aim for 6 (The name tells you how to use it), but also do opportunistic
8 (The compiler will warn if you get it wrong) for compile-time
constants.

As-is, these, and round_up/round_down, are just setting a trap for an
unsuspecting kernel developer to fall into.


BR,
Jani.


[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html


>
> [1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com
>
> Regards
> Devarsh

-- 
Jani Nikula, Intel