| Message ID | 891d4f632fbff5052e11f2d0b6fac35d@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: > From: John Zhao <yuankuiz@codeaurora.org> > > Variable tick_stopped returned by tick_nohz_tick_stopped > can have only true / forse 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 int. The data type is not int. It's part of an integer bitfield and occupies exactly one bit of storage, while bool has an architecture dependend size and is at least 1 byte, i.e. 8 bit. So with alignment effects you grew the size of the data structure and therefore the cache foot print. This is not about 'nice to have' .... Thanks, tglx
On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: > From: John Zhao <yuankuiz@codeaurora.org> > > Variable tick_stopped returned by tick_nohz_tick_stopped > can have only true / forse 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 int. > Moreover, the executed instructions cost could be minimal > without potiential data type conversion. > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org> > --- > 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. Thanks!
On 2018-04-10 03:55 PM, Thomas Gleixner wrote: > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: > >> From: John Zhao <yuankuiz@codeaurora.org> >> >> Variable tick_stopped returned by tick_nohz_tick_stopped >> can have only true / forse 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 int. > > The data type is not int. [ZJ] Yes. Per newest tip in branch of linux-pm-cpuidle, it is unsigned int with 1 bit in width. There is typo in commit message, "int" at here should be "unsigned int" > It's part of an integer bitfield and occupies > exactly one bit of storage, while bool has an architecture dependend > size > and is at least 1 byte, i.e. 8 bit. So with alignment effects you grew > the > size of the data structure and therefore the cache foot print. [ZJ] So, 1 bit in width is specified as: + bool tick_stopped : 1; This patch is based on the linux-pm-cpuidle branch which has already gathered available space in the structure of tick_sched. > > This is not about 'nice to have' .... > > Thanks, > > tglx
On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >> From: John Zhao <yuankuiz@codeaurora.org> >> >> Variable tick_stopped returned by tick_nohz_tick_stopped >> can have only true / forse 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 int. >> Moreover, the executed instructions cost could be minimal >> without potiential data type conversion. >> >> Signed-off-by: John Zhao <yuankuiz@codeaurora.org> >> --- >> 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". > > Thanks!
On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: > > > From: John Zhao <yuankuiz@codeaurora.org> > > > > > > Variable tick_stopped returned by tick_nohz_tick_stopped > > > can have only true / forse 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 int. > > > Moreover, the executed instructions cost could be minimal > > > without potiential data type conversion. > > > > > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org> > > > --- > > > 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 for no benefit at all. Thanks, tglx
Hi Thomas, On 2018-04-10 05:10 PM, Thomas Gleixner wrote: > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >> On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >> > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >> > > From: John Zhao <yuankuiz@codeaurora.org> >> > > >> > > 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@codeaurora.org> >> > > --- >> > > 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. > 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. > > Thanks, > > tglx
On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: > On 2018-04-10 05:10 PM, Thomas Gleixner wrote: > > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: > > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: > > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: > > > > > From: John Zhao <yuankuiz@codeaurora.org> > > > > > > > > > > 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@codeaurora.org> > > > > > --- > > > > > 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, 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. > > 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. 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. But that does not make it any more interesting. It just makes the code harder to read and eventually leads to bigger storage. Thanks, tglx
On Tue, Apr 10, 2018 at 06:07:17PM +0800, yuankuiz@codeaurora.org wrote: > > > > > @@ -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. Since its a different type, the bitfields will not be merged. Also I'm surprised a bitfield with base-type _Bool is even allowed. > > 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. Do you have any actual evidence for that? Is there a compiler stupid enough to generate code to convert a bool to a 1bit value?
On Tue, 10 Apr 2018, Peter Zijlstra wrote: > On Tue, Apr 10, 2018 at 06:07:17PM +0800, yuankuiz@codeaurora.org wrote: > > > > > > @@ -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. > > Since its a different type, the bitfields will not be merged. Also I'm > surprised a bitfield with base-type _Bool is even allowed. > > > > 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. > > Do you have any actual evidence for that? Is there a compiler stupid > enough to generate code to convert a bool to a 1bit value? Sure, if you do: > > > > > > + bool tick_stopped : 1; which is stupidly allowed by the standard....
On Tue, Apr 10, 2018 at 02:07:32PM +0200, Thomas Gleixner wrote: > On Tue, 10 Apr 2018, Peter Zijlstra wrote: > > Do you have any actual evidence for that? Is there a compiler stupid > > enough to generate code to convert a bool to a 1bit value? > > Sure, if you do: > > > > > > > > + bool tick_stopped : 1; > > which is stupidly allowed by the standard.... The code-gen changes are because of the layout change, not because of the type change.
On Tue, Apr 10, 2018 at 10:00:01AM +0200, Rafael J. Wysocki wrote: > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: > > +++ 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. Joe, apw, could we get Checkpatch to whinge about _Bool in composite types? That should immediately also disqualify using it as the base type of bitfields.
On 2018-04-10 07:06 PM, Thomas Gleixner wrote: > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >> On 2018-04-10 05:10 PM, Thomas Gleixner wrote: >> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >> > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >> > > > > From: John Zhao <yuankuiz@codeaurora.org> >> > > > > >> > > > > 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@codeaurora.org> >> > > > > --- >> > > > > 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
Typo... On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote: > On 2018-04-10 07:06 PM, Thomas Gleixner wrote: >> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote: >>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >>> > > > > From: John Zhao <yuankuiz@codeaurora.org> >>> > > > > >>> > > > > 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@codeaurora.org> >>> > > > > --- >>> > > > > 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(): [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an evaluation() as: #include <stdio.h> #include <stdbool.h> struct tick_sched { unsigned int inidle : 1; unsigned int tick_stopped : 1; }; bool get_status() { struct tick_sched *ts; ts->tick_stopped = 1; return ts->tick_stopped; } int main() { if (get_status()) return 0; return 0; } [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched structure for comparison. > 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) > >> Just FYI. Thanks, ZJ
On Tue, 2018-04-10 at 14:33 +0200, Peter Zijlstra wrote: > On Tue, Apr 10, 2018 at 10:00:01AM +0200, Rafael J. Wysocki wrote: > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: > > > +++ 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. > > Joe, apw, could we get Checkpatch to whinge about _Bool in composite > types? That should immediately also disqualify using it as the base type > of bitfields. Whinging about bool <foo> : <x> seems entirely sensible and straightforward to do. I'm not so sure about bool in structs as a patch context could be adding a bool to local stack definitions and there's no real ability to determine if the bool is in a struct or on the stack. Also, I think there's nothing really wrong with using bool in structs. Steven Rostedt's rationale in https://lkml.org/lkml/2017/11/21/207 isn't really right as sizeof(int) is 4 not 1 and sizeof(bool) is 1 on arches without alignment issues. I believe when using gcc, sizeof(bool) is always 1 and there may be alignment padding added on some arches. Dunno. But I think the battle is already lost anyway. git grep -P '(?<!static|extern)\s+bool\s+\w+\s*;' include | wc -l 1543
On Tue, Apr 10, 2018 at 08:14:54AM -0700, Joe Perches wrote: > Whinging about bool <foo> : <x> seems entirely sensible > and straightforward to do. > > I'm not so sure about bool in structs as a patch context > could be adding a bool to local stack definitions and > there's no real ability to determine if the bool is in a > struct or on the stack. > > Also, I think there's nothing really wrong with using > bool in structs. Steven Rostedt's rationale in > https://lkml.org/lkml/2017/11/21/207 isn't really right > as sizeof(int) is 4 not 1 and sizeof(bool) is 1 on arches > without alignment issues. I believe when using gcc, > sizeof(bool) is always 1 and there may be alignment padding > added on some arches. Dunno. C std simply does not define sizeof(_Bool) and leaves it up to architecture ABI, therefore I refuse to use _Bool in composite types, because I care about layout. Also, not all architectures can do byte addressing, see Alpha <EV56 and for those _Bool would have to be a whole word (the existence of such architectures likely influenced the vague definition of _Bool in the first place). > But I think the battle is already lost anyway. > > git grep -P '(?<!static|extern)\s+bool\s+\w+\s*;' include | wc -l > 1543 Yes I know, doesn't mean we shouldn't discourage it for new code; also Linus.
++ On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote: > Typo... > > On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote: >> On 2018-04-10 07:06 PM, Thomas Gleixner wrote: >>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote: >>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >>>> > > > > From: John Zhao <yuankuiz@codeaurora.org> >>>> > > > > >>>> > > > > 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@codeaurora.org> >>>> > > > > --- >>>> > > > > 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(): > [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an > evaluation() as: > #include <stdio.h> > #include <stdbool.h> > > struct tick_sched { > unsigned int inidle : 1; > unsigned int tick_stopped : 1; > }; > > bool get_status() > { > struct tick_sched *ts; > ts->tick_stopped = 1; > return ts->tick_stopped; > } > > int main() > { > if (get_status()) return 0; > return 0; > } > > [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched > structure for comparison. > > >> 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) >> >>> > > Just FYI. > > Thanks, > ZJ
++ On 2018-04-11 07:09 AM, yuankuiz@codeaurora.org wrote: > ++ > > On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote: >> Typo... >> >> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote: >>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote: >>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote: >>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org> >>>>> > > > > >>>>> > > > > 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@codeaurora.org> >>>>> > > > > --- >>>>> > > > > 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(): >> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an >> evaluation() as: >> #include <stdio.h> >> #include <stdbool.h> >> >> struct tick_sched { >> unsigned int inidle : 1; >> unsigned int tick_stopped : 1; >> }; >> >> bool get_status() >> { >> struct tick_sched *ts; >> ts->tick_stopped = 1; >> return ts->tick_stopped; >> } >> >> int main() >> { >> if (get_status()) return 0; >> return 0; >> } >> >> [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched >> structure for comparison. >> >> >>> 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) >>> >>>> >> >> Just FYI. >> >> Thanks, >> ZJ
On 2018-04-11 07:20 AM, yuankuiz@codeaurora.org wrote: > ++ > On 2018-04-11 07:09 AM, yuankuiz@codeaurora.org wrote: >> ++ >> >> On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote: >>> Typo... >>> >>> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote: >>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote: >>>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote: >>>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >>>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org> >>>>>> > > > > >>>>>> > > > > 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@codeaurora.org> >>>>>> > > > > --- >>>>>> > > > > 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? [ZJ] Further prototyping has been given based on gcc for both of x86_64 and armv8-a, unsigned int and bool share the same 4 bytes without the addtional storage for sure. Open this and welcome if any other difference behaviour could be captured. >>>> >>>>> 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(): >>> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an >>> evaluation() as: >>> #include <stdio.h> >>> #include <stdbool.h> >>> >>> struct tick_sched { >>> unsigned int inidle : 1; >>> unsigned int tick_stopped : 1; >>> }; >>> >>> bool get_status() >>> { >>> struct tick_sched *ts; >>> ts->tick_stopped = 1; >>> return ts->tick_stopped; >>> } >>> >>> int main() >>> { >>> if (get_status()) return 0; >>> return 0; >>> } >>> >>> [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched >>> structure for comparison. >>> >>> >>>> 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) >>>> >>>>> >>> >>> Just FYI. >>> >>> Thanks, >>> ZJ
On 2018-04-20 09:47 AM, yuankuiz@codeaurora.org wrote: > On 2018-04-11 07:20 AM, yuankuiz@codeaurora.org wrote: >> ++ >> On 2018-04-11 07:09 AM, yuankuiz@codeaurora.org wrote: >>> ++ >>> >>> On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote: >>>> Typo... >>>> >>>> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote: >>>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote: >>>>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote: >>>>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote: >>>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote: >>>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <yuankuiz@codeaurora.org> wrote: >>>>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org> >>>>>>> > > > > >>>>>>> > > > > 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@codeaurora.org> >>>>>>> > > > > --- >>>>>>> > > > > 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? > [ZJ] Further prototyping has been given based on gcc for both of > x86_64 and armv8-a, > unsigned int and bool share the same 1 bytes without the > addtional storage for sure. > Open this and welcome if any other difference behaviour could be > captured. [ZJ] Typo.. change 4 bytes above to 1 byte actually. >>>>> >>>>>> 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. [ZJ] "-mone-byte-bool" could be used by alpha-linux-gcc to override the default bool size to become 1 byte for even Darwin / powerPC from it's manual. >>>>>> >>>>>> 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(): >>>> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an >>>> evaluation() as: >>>> #include <stdio.h> >>>> #include <stdbool.h> >>>> >>>> struct tick_sched { >>>> unsigned int inidle : 1; >>>> unsigned int tick_stopped : 1; >>>> }; >>>> >>>> bool get_status() >>>> { >>>> struct tick_sched *ts; >>>> ts->tick_stopped = 1; >>>> return ts->tick_stopped; >>>> } >>>> >>>> int main() >>>> { >>>> if (get_status()) return 0; >>>> return 0; >>>> } >>>> >>>> [ZJ] Toggle the declaration of tick_stopped in side of the >>>> tick_sched >>>> structure for comparison. >>>> >>>> >>>>> 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) >>>>> >>>>>> >>>> >>>> Just FYI. >>>> >>>> Thanks, >>>> ZJ
On Fri, 2018-04-20 at 14:44 +0800, yuankuiz@codeaurora.org wrote: > On 2018-04-20 09:47 AM, yuankuiz@codeaurora.org wrote: [] > > [ZJ] Further prototyping has been given based on gcc for both of > > x86_64 and armv8-a, > > unsigned int and bool share the same 1 bytes without the > > addtional storage for sure. > > Open this and welcome if any other difference behaviour could be > > captured. > > [ZJ] Typo.. change 4 bytes above to 1 byte actually. Not really. unsigned int is 4 and bool is generally 1. Alignment padding after a bool may exist.
On 2018-04-21 03:24 AM, Joe Perches wrote: > On Fri, 2018-04-20 at 14:44 +0800, yuankuiz@codeaurora.org wrote: >> On 2018-04-20 09:47 AM, yuankuiz@codeaurora.org wrote: > [] >> > [ZJ] Further prototyping has been given based on gcc for both of >> > x86_64 and armv8-a, >> > unsigned int and bool share the same 1 bytes without the >> > addtional storage for sure. >> > Open this and welcome if any other difference behaviour could be >> > captured. >> >> [ZJ] Typo.. change 4 bytes above to 1 byte actually. > > Not really. > > unsigned int is 4 and bool is generally 1. > Alignment padding after a bool may exist. [ZJ] Depending on how to pack, the size was padded is variance. For example. In case of the "unsigned char" at the following, pack is happened and result 1 bytes.(if no more than 8 bits are used) In case of the "int" at the following, pack is happened but result 4 bytes. I mean, I demo it but use the 1# case due for another thread discussion on the ichx_desc() so move a little bit from the tick_sched struct.
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;