On Sun, 29 Mar 2020 20:16:47 +0200,
James Bottomley wrote:
>
> On Sun, 2020-03-29 at 17:50 +0000, George Spelvin wrote:
> > On Sun, Mar 29, 2020 at 07:13:33PM +0200, Takashi Iwai wrote:
> > > On Sun, 29 Mar 2020 14:11:29 +0200, George Spelvin wrote:
> > > > On Sun, Mar 29, 2020 at 09:52:23AM +0200, Takashi Iwai wrote:
> > > > > I thought the compiler already optimizes to the constant
> > > > > calculation
> > > > > for the above case?
> > > >
> > > > It optimizes that if the entire argument, including "seconds", is
> > > > a compile-time constant.
> > > >
> > > > However, given "msecs_to_jiffies(hdev->rpa_timeout * 1000);",
> > > > the computatin is non-trivial.
> > >
> > > Fair enough. But it's still a question whether an open code X * HZ
> > > is
> > > good at all...
> >
> > I'm sorry, I don't understand what you mean by "good at all" here.
> > The value computed is exactly the same.
>
> I think he means what the compiler does with it.
>
> We all assume that msecs_to_jiffies is properly optimized so there
> should be no need to open code it like you're proposing.
Yes, it'd be best if the compiler can handle it properly.
But also I meant to keep using the macro for consistency reason.
IIRC, we wanted to eliminate the explicit use of HZ in the past, and
it's how many lines have been converted with *_to_jiffies() calls.
I don't know whether the eliminate of HZ is still wished, but
reverting to the open code is a step backward for that.
thanks,
Takashi
On Mon, Mar 30, 2020 at 08:27:01AM +0200, Takashi Iwai wrote:
> On Sun, 29 Mar 2020 20:16:47 +0200, James Bottomley wrote:
>> We all assume that msecs_to_jiffies is properly optimized so there
>> should be no need to open code it like you're proposing.
>
> Yes, it'd be best if the compiler can handle it properly.
I've tried, and can't figure out how to get the compiler to detect this
special case and not invoke the general code. In particular, for a
variable x, __builtin_constant_p(x * 1000 % 1000) is false. Even if x is
signed and ANSI lets the compiler assume that overflow doesn't happen.
If you can do it, I'm most curious how!
> But also I meant to keep using the macro for consistency reason.
> IIRC, we wanted to eliminate the explicit use of HZ in the past, and
> it's how many lines have been converted with *_to_jiffies() calls.
> I don't know whether the eliminate of HZ is still wished, but
> reverting to the open code is a step backward for that.
Well, you could always add a secs_to_jiffies(x) wrapper. But given
that it expands to basically x * HZ, some people might wonder why
you're bothering.
I assumed that open-coding x * HZ was the preferred style, so that's
what I did.
On Mon, 30 Mar 2020 08:51:05 +0200,
George Spelvin wrote:
>
> On Mon, Mar 30, 2020 at 08:27:01AM +0200, Takashi Iwai wrote:
> > On Sun, 29 Mar 2020 20:16:47 +0200, James Bottomley wrote:
> >> We all assume that msecs_to_jiffies is properly optimized so there
> >> should be no need to open code it like you're proposing.
> >
> > Yes, it'd be best if the compiler can handle it properly.
>
> I've tried, and can't figure out how to get the compiler to detect this
> special case and not invoke the general code. In particular, for a
> variable x, __builtin_constant_p(x * 1000 % 1000) is false. Even if x is
> signed and ANSI lets the compiler assume that overflow doesn't happen.
>
> If you can do it, I'm most curious how!
Actually in the very early version of msecs_to_jiffies() was all
inlined, so the compiler could optimize such a case, I guess. Now it
was factored out to an external function in commit ca42aaf0c861, so it
became difficult.
> > But also I meant to keep using the macro for consistency reason.
> > IIRC, we wanted to eliminate the explicit use of HZ in the past, and
> > it's how many lines have been converted with *_to_jiffies() calls.
> > I don't know whether the eliminate of HZ is still wished, but
> > reverting to the open code is a step backward for that.
>
> Well, you could always add a secs_to_jiffies(x) wrapper. But given
> that it expands to basically x * HZ, some people might wonder why
> you're bothering.
Well, comparing with the expanded result doesn't make always sense.
With such a logic, you can argue why BIT(x) macro is needed, too.
After all, it's a matter of semantics.
> I assumed that open-coding x * HZ was the preferred style, so that's
> what I did.
That's my question, too -- whether the open code is preferred for this
particular purpose.
thanks,
Takashi