Re: [PATCH] time: tick-sched: use bool for tick_stopped

From: yuankuiz
Date: Tue Apr 10 2018 - 10:08:17 EST


On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
On Tue, 10 Apr 2018, yuankuiz@xxxxxxxxxxxxxx wrote:
On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
> On Tue, 10 Apr 2018, yuankuiz@xxxxxxxxxxxxxx wrote:
> > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
> > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@xxxxxxxxxxxxxx> wrote:
> > > > From: John Zhao <yuankuiz@xxxxxxxxxxxxxx>
> > > >
> > > > Variable tick_stopped returned by tick_nohz_tick_stopped
> > > > can have only true / false values. Since the return type
> > > > of the tick_nohz_tick_stopped is also bool, variable
> > > > tick_stopped nice to have data type as bool in place of unsigned int.
> > > > Moreover, the executed instructions cost could be minimal
> > > > without potiential data type conversion.
> > > >
> > > > Signed-off-by: John Zhao <yuankuiz@xxxxxxxxxxxxxx>
> > > > ---
> > > > kernel/time/tick-sched.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> > > > index 6de959a..4d34309 100644
> > > > --- a/kernel/time/tick-sched.h
> > > > +++ b/kernel/time/tick-sched.h
> > > > @@ -48,8 +48,8 @@ struct tick_sched {
> > > > unsigned long check_clocks;
> > > > enum tick_nohz_mode nohz_mode;
> > > >
> > > > + bool tick_stopped : 1;
> > > > unsigned int inidle : 1;
> > > > - unsigned int tick_stopped : 1;
> > > > unsigned int idle_active : 1;
> > > > unsigned int do_timer_last : 1;
> > > > unsigned int got_idle_tick : 1;
> > >
> > > I don't think this is a good idea at all.
> > >
> > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
> > [ZJ] Thanks for this sharing. Looks like, this patch fall into the case of
> > "Maybe".
>
> This patch falls into the case 'pointless' because it adds extra storage
[ZJ] 1 bit vs 1 bit. no more.

Groan. No. Care to look at the data structure? You create a new storage,
[ZJ] Say, {unsigned int, unsigned int, unsigned int, unsigned int, unsigned int} becomes
{bool , unsigned int, unsigned int, unsigned int, unsigned int}
As specified by the rule No.10 at the section 6.7.2.1 of C99 TC2 as:
"If enough space remains, a bit-field that immediately follows another bit-field in a
structure shall be packed into adjacent bits of the same unit." What is the new storage so far?

which is incidentally merged into the other bitfield by the compiler at a
different bit position, but there is no guarantee that a compiler does
that. It's free to use distinct storage for that bool based bit.
[ZJ] Per the rule No.10 at section 6.7.2.1 of C99 TC2 as:
" If insufficient space remains, whether a bit-field that does not fit is put into
the next unit or overlaps adjacent units is implementation-defined."
So, implementation is never mind which type will be stored if any.

>> > for no benefit at all.
[ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() which is bool.
The benefit is no any potiential type conversion could be minded.

A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to be
evaluated as a bit. So there is a type conversion from BIT to bool required
because BIT != bool.
[ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
"If the value 0 or 1 is stored into a nonzero-width bit-field of types
_Bool, the value of the bit-field shall compare equal to the value stored."
Obviously, it is nothing related to type conversion actually.

By chance the evaluation can be done by evaluating the byte in which the
bit is placed just because the compiler knows that the remaining bits are
not used. There is no guarantee that this is done, it happens to be true
for a particular compiler.
[ZJ] Actually, such as GCC owe that kind of guarantee to be promised by ABI.

But that does not make it any more interesting. It just makes the code
harder to read and eventually leads to bigger storage.
[ZJ] To get the benctifit to be profiled, it is given as:
number of instructions of function tick_nohz_tick_stopped():
original: 17
patched: 14
Which was saved is:
movzbl %al, %eax
testl %eax, %eax
setne %al
Say, 3 / 17 = 17 % could be gained in the instruction executed for this function can be evaluated.

Note:
The environment I used is:
OS : Ubuntu Desktop 16.04 LTS
gcc: 6.3.0 (without optimization for in general purpose)


Thanks,

tglx