From: John Zhao <[email protected]>
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 <[email protected]>
---
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;
From: John Zhao <[email protected]>
Variable tick_stopped returned by tick_nohz_tick_stopped
can only be 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 int.
Moreover, the executed instructions cost could be minimal
without potiential data type conversion.
Signed-off-by: John Zhao <[email protected]>
---
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;
On Tue, 10 Apr 2018, [email protected] wrote:
> From: John Zhao <[email protected]>
>
> 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, <[email protected]> wrote:
> From: John Zhao <[email protected]>
>
> 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 <[email protected]>
> ---
> 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, [email protected] wrote:
>
>> From: John Zhao <[email protected]>
>>
>> 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, <[email protected]> wrote:
>> From: John Zhao <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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!
From: John Zhao <[email protected]>
Variable tick_stopped returned by tick_nohz_tick_stopped
can only be 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
the 'unsigned int'.
Moreover, the executed instructions cost could be minimal
without potiential data type conversion.
Signed-off-by: John Zhao <[email protected]>
---
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;
Subject and commit message have been updated due for typo.
This patch is based on the tip of linux-pm-cpuild branch.
Thanks
On 2018-04-10 04:51 PM, [email protected] wrote:
> From: John Zhao <[email protected]>
>
> Variable tick_stopped returned by tick_nohz_tick_stopped
> can only be 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
> the 'unsigned int'.
> Moreover, the executed instructions cost could be minimal
> without potiential data type conversion.
>
> Signed-off-by: John Zhao <[email protected]>
> ---
> 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;
On Tue, 10 Apr 2018, [email protected] wrote:
> On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
> > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
> > > From: John Zhao <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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, [email protected] wrote:
>> On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>> > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>> > > From: John Zhao <[email protected]>
>> > >
>> > > 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 <[email protected]>
>> > > ---
>> > > 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, [email protected] wrote:
> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
> > On Tue, 10 Apr 2018, [email protected] wrote:
> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
> > > > > From: John Zhao <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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, [email protected] 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, [email protected] 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, <[email protected]> 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, [email protected] wrote:
>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>> > On Tue, 10 Apr 2018, [email protected] wrote:
>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>> > > > > From: John Zhao <[email protected]>
>> > > > >
>> > > > > 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 <[email protected]>
>> > > > > ---
>> > > > > 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, [email protected] wrote:
> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>> On Tue, 10 Apr 2018, [email protected] wrote:
>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>> > On Tue, 10 Apr 2018, [email protected] wrote:
>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>>> > > > > From: John Zhao <[email protected]>
>>> > > > >
>>> > > > > 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 <[email protected]>
>>> > > > > ---
>>> > > > > 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, <[email protected]> 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
Using bool in a bitfield isn't a good idea as the
alignment behavior is arch implementation defined.
Suggest using unsigned int or u<8|16|32> instead.
Signed-off-by: Joe Perches <[email protected]>
Suggested-by: Peter Zijlstra <[email protected]>
---
scripts/checkpatch.pl | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eb534d48140e..e16d6713f236 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6251,6 +6251,12 @@ sub process {
}
}
+# check for bool bitfields
+ if ($sline =~ /^.\s+bool\s*$Ident\s*:\s*\d+\s*;/) {
+ WARN("BOOL_BITFIELD",
+ "Avoid using bool as bitfield. Prefer bool bitfields as unsigned int or u<8|16|32>\n" . $herecurr);
+ }
+
# check for semaphores initialized locked
if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
WARN("CONSIDER_COMPLETION",
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.
A struct with a bool member can have different sizes on various
architectures because neither bool size nor alignment is standardized.
So emit a message on the use of bool in structs only in .h files and
not .c files.
There is the real possibility that this test could have a false positive
when a bool is declared as an automatic, so limit the test to .h files
where the only false positive is for declarations in static inline functions.
Signed-off-by: Joe Perches <[email protected]>
Suggested-by: Peter Zijlstra <[email protected]>
---
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e16d6713f236..24618dffc5cb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6257,6 +6257,13 @@ sub process {
"Avoid using bool as bitfield. Prefer bool bitfields as unsigned int or u<8|16|32>\n" . $herecurr);
}
+# check for bool use in .h files
+ if ($realfile =~ /\.h$/ &&
+ $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
+ CHK("BOOL_MEMBER",
+ "Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n" . $herecurr);
+ }
+
# check for semaphores initialized locked
if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
WARN("CONSIDER_COMPLETION",
--
2.15.0
On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <[email protected]> wrote:
> A struct with a bool member can have different sizes on various
> architectures because neither bool size nor alignment is standardized.
>
> So emit a message on the use of bool in structs only in .h files and
> not .c files.
>
> There is the real possibility that this test could have a false positive
> when a bool is declared as an automatic, so limit the test to .h files
> where the only false positive is for declarations in static inline functions.
What's wrong with bools in structs? The changelog should be
self-contained, please. At least add a link in the changelog (of the
lkml.kernel.org/r/MSGID variety), but a link in the changelog is risky
because the reader may be offline or the server may be down.
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6257,6 +6257,13 @@ sub process {
> "Avoid using bool as bitfield. Prefer bool bitfields as unsigned int or u<8|16|32>\n" . $herecurr);
> }
>
> +# check for bool use in .h files
> + if ($realfile =~ /\.h$/ &&
> + $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
> + CHK("BOOL_MEMBER",
> + "Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n" . $herecurr);
And... the server is down. Am unable to understand or review this patch!
> + }
> +
> # check for semaphores initialized locked
> if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
> WARN("CONSIDER_COMPLETION",
On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <[email protected]> wrote:
>
> > A struct with a bool member can have different sizes on various
> > architectures because neither bool size nor alignment is standardized.
>
> What's wrong with bools in structs?
See above.
On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches <[email protected]> wrote:
> On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <[email protected]> wrote:
> >
> > > A struct with a bool member can have different sizes on various
> > > architectures because neither bool size nor alignment is standardized.
> >
> > What's wrong with bools in structs?
>
> See above.
Yeah, but so what? `long' has different sizes on different
architectures too.
++
On 2018-04-10 10:49 PM, [email protected] wrote:
> Typo...
>
> On 2018-04-10 10:08 PM, [email protected] wrote:
>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>> On Tue, 10 Apr 2018, [email protected] wrote:
>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>> > On Tue, 10 Apr 2018, [email protected] wrote:
>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>>>> > > > > From: John Zhao <[email protected]>
>>>> > > > >
>>>> > > > > 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 <[email protected]>
>>>> > > > > ---
>>>> > > > > 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, [email protected] wrote:
> ++
>
> On 2018-04-10 10:49 PM, [email protected] wrote:
>> Typo...
>>
>> On 2018-04-10 10:08 PM, [email protected] wrote:
>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>>> On Tue, 10 Apr 2018, [email protected] wrote:
>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>>> > On Tue, 10 Apr 2018, [email protected] wrote:
>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>>>>> > > > > From: John Zhao <[email protected]>
>>>>> > > > >
>>>>> > > > > 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 <[email protected]>
>>>>> > > > > ---
>>>>> > > > > 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, Apr 10, 2018 at 03:00:11PM -0700, Andrew Morton wrote:
> On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches <[email protected]> wrote:
>
> > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <[email protected]> wrote:
> > >
> > > > A struct with a bool member can have different sizes on various
> > > > architectures because neither bool size nor alignment is standardized.
> > >
> > > What's wrong with bools in structs?
> >
> > See above.
>
> Yeah, but so what? `long' has different sizes on different
> architectures too.
Right, so we have ILP32/LP64 for all our 32/64 bit archs respectively.
So only 2 possible variations to consider, and if you know your bitness
you know your layout.
(+- some really unfortunate alignment exceptions, the worst of which
Arnd recently removed, hooray!)
But neither says anything about sizeof(_Bool), and the standard leaves
it undefined and only mandates it is large enough to store either 0 or
1 (and I suspect this vagueness is because there are architectures that
either have no byte addressibility or it's more expensive than word
addressibility).
Typically GCC chooses a single byte to represent _Bool, but there are no
guarantees. This means that when you care about structure layout (as we
all really should) things go wobbly when you use _Bool.
If GCC were to guarantee a 1 byte _Bool for all Linux ABIs we could
reconsider.
On Wed, 11 Apr 2018 10:15:02 +0200 Peter Zijlstra <[email protected]> wrote:
> On Tue, Apr 10, 2018 at 03:00:11PM -0700, Andrew Morton wrote:
> > On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches <[email protected]> wrote:
> >
> > > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> > > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <[email protected]> wrote:
> > > >
> > > > > A struct with a bool member can have different sizes on various
> > > > > architectures because neither bool size nor alignment is standardized.
> > > >
> > > > What's wrong with bools in structs?
> > >
> > > See above.
> >
> > Yeah, but so what? `long' has different sizes on different
> > architectures too.
>
> Right, so we have ILP32/LP64 for all our 32/64 bit archs respectively.
> So only 2 possible variations to consider, and if you know your bitness
> you know your layout.
>
> (+- some really unfortunate alignment exceptions, the worst of which
> Arnd recently removed, hooray!)
>
> But neither says anything about sizeof(_Bool), and the standard leaves
> it undefined and only mandates it is large enough to store either 0 or
> 1 (and I suspect this vagueness is because there are architectures that
> either have no byte addressibility or it's more expensive than word
> addressibility).
>
> Typically GCC chooses a single byte to represent _Bool, but there are no
> guarantees. This means that when you care about structure layout (as we
> all really should) things go wobbly when you use _Bool.
>
> If GCC were to guarantee a 1 byte _Bool for all Linux ABIs we could
> reconsider.
OK. I guess. But I'm not really seeing some snappy description which
helps people understand why checkpatch is warning about this. We
already have some 500 bools-in-structs and the owners of that code will
be wondering whether they should change them, and whether they should
apply those remove-bool-in-struct patches which someone sent them.
So... can we please get some clarity here?
...
(ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
hm, Linus suggests that instead of using
bool mybool;
we should use
unsigned mybool:1;
However that introduces the risk that alterations of mybool will use
nonatomic rmw operations.
unsigned myboolA:1;
unsigned myboolB:1;
so
foo->myboolA = 1;
could scribble on concurrent alterations of foo->myboolB. I think.
I guess that risk is also present if myboolA and myboolB were `bool',
too. The compiler could do any old thing with them including, perhaps,
using a single-bit bitfield(?).
(Adding Julia Lawall)
On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> We already have some 500 bools-in-structs
I got at least triple that only in include/
so I expect there are at probably an order
of magnitude more than 500 in the kernel.
I suppose some cocci script could count the
actual number of instances. A regex can not.
> and the owners of that code will
> be wondering whether they should change them, and whether they should
> apply those remove-bool-in-struct patches which someone sent them.
Which is why the warning is --strict only
> So... can we please get some clarity here?
> ...
>
> (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
>
> hm, Linus suggests that instead of using
>
> bool mybool;
>
> we should use
>
> unsigned mybool:1;
>
> However that introduces the risk that alterations of mybool will use
> nonatomic rmw operations.
>
> unsigned myboolA:1;
> unsigned myboolB:1;
>
> so
>
> foo->myboolA = 1;
>
> could scribble on concurrent alterations of foo->myboolB. I think.
Without barriers, that could happen anyway.
To me, the biggest problem with conversions
from bool to bitfield is logical. ie:
unsigned int.singlebitfield = 4;
is not the same result as
bool = 4;
because of implicit truncation vs boolean conversion
so a direct change of bool use in structs to unsigned
would also require logic analysis.
On Wed, Apr 11, 2018 at 09:29:59AM -0700, Andrew Morton wrote:
>
> OK. I guess. But I'm not really seeing some snappy description which
> helps people understand why checkpatch is warning about this.
"Results in architecture dependent layout."
is the best short sentence I can come up with.
> We already have some 500 bools-in-structs and the owners of that code
> will be wondering whether they should change them, and whether they
> should apply those remove-bool-in-struct patches which someone sent
> them.
I still have room in my /dev/null mailbox for pure checkpatch patches.
> (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
Yes, we really should not use lkml.org for references. Sadly google
displays it very prominently when you search for something.
> hm, Linus suggests that instead of using
>
> bool mybool;
>
> we should use
>
> unsigned mybool:1;
>
> However that introduces the risk that alterations of mybool will use
> nonatomic rmw operations.
>
> unsigned myboolA:1;
> unsigned myboolB:1;
>
> so
>
> foo->myboolA = 1;
>
> could scribble on concurrent alterations of foo->myboolB. I think.
So that is true of u8 on Alpha <EV56 too. If you want concurrent, you
had better know what you're doing.
> I guess that risk is also present if myboolA and myboolB were `bool',
> too. The compiler could do any old thing with them including, perhaps,
> using a single-bit bitfield(?).
The smallest addressable type in C is a byte, so while _Bool may be
larger than a byte, it cannot be smaller. Otherwise we could not write:
_Bool var;
_Boll *ptr = &var;
Which is something that comes apart with bitfields.
On Wed, 11 Apr 2018, Joe Perches wrote:
> (Adding Julia Lawall)
>
> On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > We already have some 500 bools-in-structs
>
> I got at least triple that only in include/
> so I expect there are at probably an order
> of magnitude more than 500 in the kernel.
>
> I suppose some cocci script could count the
> actual number of instances. A regex can not.
I got 12667.
I'm not sure to understand the issue. Will using a bitfield help if there
are no other bitfields in the structure?
julia
>
> > and the owners of that code will
> > be wondering whether they should change them, and whether they should
> > apply those remove-bool-in-struct patches which someone sent them.
>
> Which is why the warning is --strict only
>
> > So... can we please get some clarity here?
>
>
> > ...
> >
> > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
> >
> > hm, Linus suggests that instead of using
> >
> > bool mybool;
> >
> > we should use
> >
> > unsigned mybool:1;
> >
> > However that introduces the risk that alterations of mybool will use
> > nonatomic rmw operations.
> >
> > unsigned myboolA:1;
> > unsigned myboolB:1;
> >
> > so
> >
> > foo->myboolA = 1;
> >
> > could scribble on concurrent alterations of foo->myboolB. I think.
>
> Without barriers, that could happen anyway.
>
> To me, the biggest problem with conversions
> from bool to bitfield is logical. ie:
>
> unsigned int.singlebitfield = 4;
>
> is not the same result as
>
> bool = 4;
>
> because of implicit truncation vs boolean conversion
> so a direct change of bool use in structs to unsigned
> would also require logic analysis.
>
>
On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> On Wed, 11 Apr 2018, Joe Perches wrote:
> > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > We already have some 500 bools-in-structs
> >
> > I got at least triple that only in include/
> > so I expect there are at probably an order
> > of magnitude more than 500 in the kernel.
> >
> > I suppose some cocci script could count the
> > actual number of instances. A regex can not.
>
> I got 12667.
Could you please post the cocci script?
> I'm not sure to understand the issue. Will using a bitfield help if there
> are no other bitfields in the structure?
IMO, not really.
The primary issue is described by Linus here:
https://lkml.org/lkml/2017/11/21/384
I personally do not find a significant issue with
uncontrolled sizes of bool in kernel structs as
all of the kernel structs are transitory and not
written out to storage.
I suppose bool bitfields are also OK, but for the
RMW required.
Using unsigned int :1 bitfield instead of bool :1
has the negative of truncation so that the uint
has to be set with !! instead of a simple assign.
On Wed, 11 Apr 2018, Joe Perches wrote:
> On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > We already have some 500 bools-in-structs
> > >
> > > I got at least triple that only in include/
> > > so I expect there are at probably an order
> > > of magnitude more than 500 in the kernel.
> > >
> > > I suppose some cocci script could count the
> > > actual number of instances. A regex can not.
> >
> > I got 12667.
>
> Could you please post the cocci script?
Sure.
julia
Command line:
spatch.opt boolinstruct.cocci -j 40 --very-quiet --no-includes --include-headers /run/shm/linux-next --use-idutils
This was tested on:
struct foo {
bool a;
bool b,c;
int r;
};
struct {
bool a;
bool b,c;
int r;
} x;
----------------------
@initialize:ocaml@
@@
let ctr = ref 0
@r@
identifier i,x;
position p;
@@
struct i {
...
bool x@p;
...
}
@script:ocaml@
_p << r.p;
@@
ctr := !ctr + 1
@s@
identifier x;
position p;
@@
struct {
...
bool x@p;
...
}
@script:ocaml@
_p << s.p;
@@
ctr := !ctr + 1
@finalize:ocaml@
ctrs << merge.ctr;
@@
ctr := 0;
List.iter (function c -> ctr := !c + !ctr) ctrs;
Printf.printf "%d\n" !ctr
* Peter Zijlstra <[email protected]> wrote:
> I still have room in my /dev/null mailbox for pure checkpatch patches.
>
> > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
>
> Yes, we really should not use lkml.org for references. Sadly google
> displays it very prominently when you search for something.
lkml.org is nice in emails that have a short expected life time and relevance -
but it probably shouldn't be used for permanent references such as kernel
messages, code comments and Git log entries.
Thanks,
Ingo
On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote:
> lkml.org is nice in emails that have a short expected life time and relevance -
I like lkml.org's archive (although it's not without its problems), but
the site suffers from serious availability issues -- it is down a lot,
which is _really_ tedious.
On Wed, Apr 11, 2018 at 11:42:20PM -0700, Joe Perches wrote:
> I personally do not find a significant issue with
> uncontrolled sizes of bool in kernel structs as
> all of the kernel structs are transitory and not
> written out to storage.
People that care about cache locality, false sharing and other such
things really care about structure layout.
Growing a structure into another cacheline can be a significant
performance hit -- cache misses hurt.
Hi,
On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote:
>
> * Peter Zijlstra <[email protected]> wrote:
>
> > I still have room in my /dev/null mailbox for pure checkpatch patches.
> >
> > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
> >
> > Yes, we really should not use lkml.org for references. Sadly google
> > displays it very prominently when you search for something.
>
> lkml.org is nice in emails that have a short expected life time and relevance -
> but it probably shouldn't be used for permanent references such as kernel
> messages, code comments and Git log entries.
Is there a better or recommended way to reference posts on LKML in commit
messages? (I do like the idea of linking to previous discussions, results,
...)
Andrea
>
> Thanks,
>
> Ingo
On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote:
> Is there a better or recommended way to reference posts on LKML in commit
> messages? (I do like the idea of linking to previous discussions, results,
> ...)
Yes:
https://lkml.kernel.org/r/$MSGID
that has the added benefit that it immediately includes the msg-id, so
even if you don't have tubes, you can search for it in your local
mailboxes.
Also, since we (kernel.org) control the redirection (currently
marc.info) we can always point it to a life archive.
Andrea Parri <[email protected]> writes:
> On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote:
>>
>> * Peter Zijlstra <[email protected]> wrote:
>>
>> > I still have room in my /dev/null mailbox for pure checkpatch patches.
>> >
>> > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning)
>> >
>> > Yes, we really should not use lkml.org for references. Sadly google
>> > displays it very prominently when you search for something.
>>
>> lkml.org is nice in emails that have a short expected life time and relevance -
>> but it probably shouldn't be used for permanent references such as kernel
>> messages, code comments and Git log entries.
>
> Is there a better or recommended way to reference posts on LKML in commit
> messages? (I do like the idea of linking to previous discussions, results,
> ...)
I like lkml.kernel.org, for example a link to your message would be:
https://lkml.kernel.org/r/20180412093521.GA6427@andrea
BTW, I think it would be a good idea to add a hostname to your
Message-Id.
--
Kalle Valo
On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote:
> On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote:
> > Is there a better or recommended way to reference posts on LKML in commit
> > messages? (I do like the idea of linking to previous discussions, results,
> > ...)
>
> Yes:
>
> https://lkml.kernel.org/r/$MSGID
>
> that has the added benefit that it immediately includes the msg-id, so
> even if you don't have tubes, you can search for it in your local
> mailboxes.
>
> Also, since we (kernel.org) control the redirection (currently
> marc.info) we can always point it to a life archive.
Message IDs are not useful unless you subscribe and
keep your emails.
It'd be _much_ nicer if vger.kernel.org stored every email
it sent and had a search mechanism available rather than
relying on external systems.
On Thu, Apr 12, 2018 at 05:01:37AM -0700, Joe Perches wrote:
> On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote:
> > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote:
> > > Is there a better or recommended way to reference posts on LKML in commit
> > > messages? (I do like the idea of linking to previous discussions, results,
> > > ...)
> >
> > Yes:
> >
> > https://lkml.kernel.org/r/$MSGID
> >
> > that has the added benefit that it immediately includes the msg-id, so
> > even if you don't have tubes, you can search for it in your local
> > mailboxes.
> >
> > Also, since we (kernel.org) control the redirection (currently
> > marc.info) we can always point it to a life archive.
>
> Message IDs are not useful unless you subscribe and
> keep your emails.
I happen to do so..
> It'd be _much_ nicer if vger.kernel.org stored every email
> it sent and had a search mechanism available rather than
> relying on external systems.
People are looking at that afaik.
On Thu, 2018-04-12 at 14:08 +0200, Peter Zijlstra wrote:
> On Thu, Apr 12, 2018 at 05:01:37AM -0700, Joe Perches wrote:
> > On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote:
> > > > Is there a better or recommended way to reference posts on LKML in commit
> > > > messages? (I do like the idea of linking to previous discussions, results,
> > > > ...)
> > >
> > > Yes:
> > >
> > > https://lkml.kernel.org/r/$MSGID
> > >
> > > that has the added benefit that it immediately includes the msg-id, so
> > > even if you don't have tubes, you can search for it in your local
> > > mailboxes.
> > >
> > > Also, since we (kernel.org) control the redirection (currently
> > > marc.info) we can always point it to a life archive.
> >
> > Message IDs are not useful unless you subscribe and
> > keep your emails.
>
> I happen to do so..
As do I for mailing list threads I reply to, but keeping all email
threads locally is not not practicable on limited storage systems.
> > It'd be _much_ nicer if vger.kernel.org stored every email
> > it sent and had a search mechanism available rather than
> > relying on external systems.
>
> People are looking at that afaik.
Looking and doing are unfortunately different.
Back when gmane died a couple years ago, I made the same suggestion
to [email protected], [email protected] and cc'd lkml
https://lkml.org/lkml/2016/8/3/484
Never heard back.
Maybe it's the proper time now to revisit this.
On Thu, 12 Apr 2018 14:08:32 +0200 Peter Zijlstra <[email protected]> wrote:
> > It'd be _much_ nicer if vger.kernel.org stored every email
> > it sent and had a search mechanism available rather than
> > relying on external systems.
>
> People are looking at that afaik.
I have linux-kernel mboxes going back to October 2000, which I can send
to whoever-that-is if needed for importation purposes...
On Wed, 11 Apr 2018, Joe Perches wrote:
> On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > We already have some 500 bools-in-structs
> > >
> > > I got at least triple that only in include/
> > > so I expect there are at probably an order
> > > of magnitude more than 500 in the kernel.
> > >
> > > I suppose some cocci script could count the
> > > actual number of instances. A regex can not.
> >
> > I got 12667.
>
> Could you please post the cocci script?
>
> > I'm not sure to understand the issue. Will using a bitfield help if there
> > are no other bitfields in the structure?
>
> IMO, not really.
>
> The primary issue is described by Linus here:
> https://lkml.org/lkml/2017/11/21/384
>
> I personally do not find a significant issue with
> uncontrolled sizes of bool in kernel structs as
> all of the kernel structs are transitory and not
> written out to storage.
>
> I suppose bool bitfields are also OK, but for the
> RMW required.
>
> Using unsigned int :1 bitfield instead of bool :1
> has the negative of truncation so that the uint
> has to be set with !! instead of a simple assign.
At least with gcc 5.4.0, a number of structures become larger with
unsigned int :1. bool:1 seems to mostly solve this problem. The structure
ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger with
both approaches.
julia
Hi julia,
On 2018-04-15 05:19 AM, Julia Lawall wrote:
> On Wed, 11 Apr 2018, Joe Perches wrote:
>
>> On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
>> > On Wed, 11 Apr 2018, Joe Perches wrote:
>> > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
>> > > > We already have some 500 bools-in-structs
>> > >
>> > > I got at least triple that only in include/
>> > > so I expect there are at probably an order
>> > > of magnitude more than 500 in the kernel.
>> > >
>> > > I suppose some cocci script could count the
>> > > actual number of instances. A regex can not.
>> >
>> > I got 12667.
>>
>> Could you please post the cocci script?
>>
>> > I'm not sure to understand the issue. Will using a bitfield help if there
>> > are no other bitfields in the structure?
>>
>> IMO, not really.
>>
>> The primary issue is described by Linus here:
>> https://lkml.org/lkml/2017/11/21/384
>>
>> I personally do not find a significant issue with
>> uncontrolled sizes of bool in kernel structs as
>> all of the kernel structs are transitory and not
>> written out to storage.
>>
>> I suppose bool bitfields are also OK, but for the
>> RMW required.
>>
>> Using unsigned int :1 bitfield instead of bool :1
>> has the negative of truncation so that the uint
>> has to be set with !! instead of a simple assign.
>
> At least with gcc 5.4.0, a number of structures become larger with
> unsigned int :1. bool:1 seems to mostly solve this problem. The
> structure
> ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> with
> both approaches.
[ZJ] Hopefully, this could make it better in your environment.
IMHO, this is just for double check.
diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
index 4f6d643..b46e170 100644
--- a/drivers/gpio/gpio-ich.c
+++ b/drivers/gpio/gpio-ich.c
@@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
#define ICHX_READ(reg, base_res) inl((reg) + (base_res)->start)
struct ichx_desc {
+ /* GPO_BLINK is available on this chipset */
+ bool uses_gpe0:1;
+
+ /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
*/
+ bool uses_gpe0:1;
+
+ /*
+ * Some chipsets don't let reading output values on GPIO_LVL
register
+ * this option allows driver caching written output values
+ */
+ bool use_outlvl_cache:1;
+
/* Max GPIO pins the chipset can have */
uint ngpio;
@@ -77,24 +89,12 @@ struct ichx_desc {
const u8 (*regs)[3];
const u8 *reglen;
- /* GPO_BLINK is available on this chipset */
- bool have_blink;
-
- /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
*/
- bool uses_gpe0;
-
/* USE_SEL is bogus on some chipsets, eg 3100 */
u32 use_sel_ignore[3];
/* Some chipsets have quirks, let these use their own
request/get */
int (*request)(struct gpio_chip *chip, unsigned offset);
int (*get)(struct gpio_chip *chip, unsigned offset);
-
- /*
- * Some chipsets don't let reading output values on GPIO_LVL
register
- * this option allows driver caching written output values
- */
- bool use_outlvl_cache;
};
ZJ
On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
> Hi julia,
>
> On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > On Wed, 11 Apr 2018, Joe Perches wrote:
> >
> > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > We already have some 500 bools-in-structs
> > > > >
> > > > > I got at least triple that only in include/
> > > > > so I expect there are at probably an order
> > > > > of magnitude more than 500 in the kernel.
> > > > >
> > > > > I suppose some cocci script could count the
> > > > > actual number of instances. A regex can not.
> > > >
> > > > I got 12667.
> > >
> > > Could you please post the cocci script?
> > >
> > > > I'm not sure to understand the issue. Will using a bitfield help if there
> > > > are no other bitfields in the structure?
> > >
> > > IMO, not really.
> > >
> > > The primary issue is described by Linus here:
> > > https://lkml.org/lkml/2017/11/21/384
> > >
> > > I personally do not find a significant issue with
> > > uncontrolled sizes of bool in kernel structs as
> > > all of the kernel structs are transitory and not
> > > written out to storage.
> > >
> > > I suppose bool bitfields are also OK, but for the
> > > RMW required.
> > >
> > > Using unsigned int :1 bitfield instead of bool :1
> > > has the negative of truncation so that the uint
> > > has to be set with !! instead of a simple assign.
> >
> > At least with gcc 5.4.0, a number of structures become larger with
> > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > structure
> > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > with
> > both approaches.
>
> [ZJ] Hopefully, this could make it better in your environment.
> IMHO, this is just for double check.
I doubt this is actually better or smaller code.
Check the actual object code using objdump and the
struct alignment using pahole.
> diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> index 4f6d643..b46e170 100644
> --- a/drivers/gpio/gpio-ich.c
> +++ b/drivers/gpio/gpio-ich.c
> @@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
> #define ICHX_READ(reg, base_res) inl((reg) + (base_res)->start)
>
> struct ichx_desc {
> + /* GPO_BLINK is available on this chipset */
> + bool uses_gpe0:1;
> +
> + /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> */
> + bool uses_gpe0:1;
> +
> + /*
> + * Some chipsets don't let reading output values on GPIO_LVL
> register
> + * this option allows driver caching written output values
> + */
> + bool use_outlvl_cache:1;
> +
> /* Max GPIO pins the chipset can have */
> uint ngpio;
>
> @@ -77,24 +89,12 @@ struct ichx_desc {
> const u8 (*regs)[3];
> const u8 *reglen;
>
> - /* GPO_BLINK is available on this chipset */
> - bool have_blink;
> -
> - /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> */
> - bool uses_gpe0;
> -
> /* USE_SEL is bogus on some chipsets, eg 3100 */
> u32 use_sel_ignore[3];
>
> /* Some chipsets have quirks, let these use their own
> request/get */
> int (*request)(struct gpio_chip *chip, unsigned offset);
> int (*get)(struct gpio_chip *chip, unsigned offset);
> -
> - /*
> - * Some chipsets don't let reading output values on GPIO_LVL
> register
> - * this option allows driver caching written output values
> - */
> - bool use_outlvl_cache;
> };
>
>
> ZJ
On Wed, 18 Apr 2018, Joe Perches wrote:
> On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
> > Hi julia,
> >
> > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > >
> > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > We already have some 500 bools-in-structs
> > > > > >
> > > > > > I got at least triple that only in include/
> > > > > > so I expect there are at probably an order
> > > > > > of magnitude more than 500 in the kernel.
> > > > > >
> > > > > > I suppose some cocci script could count the
> > > > > > actual number of instances. A regex can not.
> > > > >
> > > > > I got 12667.
> > > >
> > > > Could you please post the cocci script?
> > > >
> > > > > I'm not sure to understand the issue. Will using a bitfield help if there
> > > > > are no other bitfields in the structure?
> > > >
> > > > IMO, not really.
> > > >
> > > > The primary issue is described by Linus here:
> > > > https://lkml.org/lkml/2017/11/21/384
> > > >
> > > > I personally do not find a significant issue with
> > > > uncontrolled sizes of bool in kernel structs as
> > > > all of the kernel structs are transitory and not
> > > > written out to storage.
> > > >
> > > > I suppose bool bitfields are also OK, but for the
> > > > RMW required.
> > > >
> > > > Using unsigned int :1 bitfield instead of bool :1
> > > > has the negative of truncation so that the uint
> > > > has to be set with !! instead of a simple assign.
> > >
> > > At least with gcc 5.4.0, a number of structures become larger with
> > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > structure
> > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > with
> > > both approaches.
> >
> > [ZJ] Hopefully, this could make it better in your environment.
> > IMHO, this is just for double check.
>
> I doubt this is actually better or smaller code.
>
> Check the actual object code using objdump and the
> struct alignment using pahole.
I didn't have a chance to try it, but it looks quite likely to result in a
smaller data structure based on the other examples that I looked at.
julia
>
> > diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> > index 4f6d643..b46e170 100644
> > --- a/drivers/gpio/gpio-ich.c
> > +++ b/drivers/gpio/gpio-ich.c
> > @@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
> > #define ICHX_READ(reg, base_res) inl((reg) + (base_res)->start)
> >
> > struct ichx_desc {
> > + /* GPO_BLINK is available on this chipset */
> > + bool uses_gpe0:1;
> > +
> > + /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > + bool uses_gpe0:1;
> > +
> > + /*
> > + * Some chipsets don't let reading output values on GPIO_LVL
> > register
> > + * this option allows driver caching written output values
> > + */
> > + bool use_outlvl_cache:1;
> > +
> > /* Max GPIO pins the chipset can have */
> > uint ngpio;
> >
> > @@ -77,24 +89,12 @@ struct ichx_desc {
> > const u8 (*regs)[3];
> > const u8 *reglen;
> >
> > - /* GPO_BLINK is available on this chipset */
> > - bool have_blink;
> > -
> > - /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > - bool uses_gpe0;
> > -
> > /* USE_SEL is bogus on some chipsets, eg 3100 */
> > u32 use_sel_ignore[3];
> >
> > /* Some chipsets have quirks, let these use their own
> > request/get */
> > int (*request)(struct gpio_chip *chip, unsigned offset);
> > int (*get)(struct gpio_chip *chip, unsigned offset);
> > -
> > - /*
> > - * Some chipsets don't let reading output values on GPIO_LVL
> > register
> > - * this option allows driver caching written output values
> > - */
> > - bool use_outlvl_cache;
> > };
> >
> >
> > ZJ
>
On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>
> On Wed, 18 Apr 2018, Joe Perches wrote:
>
> > On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
> > > Hi julia,
> > >
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > >
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > >
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > >
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances. A regex can not.
> > > > > >
> > > > > > I got 12667.
> > > > >
> > > > > Could you please post the cocci script?
> > > > >
> > > > > > I'm not sure to understand the issue. Will using a bitfield help if there
> > > > > > are no other bitfields in the structure?
> > > > >
> > > > > IMO, not really.
> > > > >
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > >
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > >
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > >
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > >
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > >
> > > [ZJ] Hopefully, this could make it better in your environment.
> > > IMHO, this is just for double check.
> >
> > I doubt this is actually better or smaller code.
> >
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
>
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.
I _really_ doubt there is any difference in size between the
below in any architecture
struct foo {
int bar;
bool baz:1;
int qux;
};
and
struct foo {
int bar;
bool baz;
int qux;
};
Where there would be a difference in size is
struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};
and
struct foo {
int bar;
bool baz1;
bool baz2;
int qux;
};
On Wed, 18 Apr 2018, Joe Perches wrote:
> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
> >
> > On Wed, 18 Apr 2018, Joe Perches wrote:
> >
> > > On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
> > > > Hi julia,
> > > >
> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > >
> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > > We already have some 500 bools-in-structs
> > > > > > > >
> > > > > > > > I got at least triple that only in include/
> > > > > > > > so I expect there are at probably an order
> > > > > > > > of magnitude more than 500 in the kernel.
> > > > > > > >
> > > > > > > > I suppose some cocci script could count the
> > > > > > > > actual number of instances. A regex can not.
> > > > > > >
> > > > > > > I got 12667.
> > > > > >
> > > > > > Could you please post the cocci script?
> > > > > >
> > > > > > > I'm not sure to understand the issue. Will using a bitfield help if there
> > > > > > > are no other bitfields in the structure?
> > > > > >
> > > > > > IMO, not really.
> > > > > >
> > > > > > The primary issue is described by Linus here:
> > > > > > https://lkml.org/lkml/2017/11/21/384
> > > > > >
> > > > > > I personally do not find a significant issue with
> > > > > > uncontrolled sizes of bool in kernel structs as
> > > > > > all of the kernel structs are transitory and not
> > > > > > written out to storage.
> > > > > >
> > > > > > I suppose bool bitfields are also OK, but for the
> > > > > > RMW required.
> > > > > >
> > > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > > has the negative of truncation so that the uint
> > > > > > has to be set with !! instead of a simple assign.
> > > > >
> > > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > > structure
> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > > with
> > > > > both approaches.
> > > >
> > > > [ZJ] Hopefully, this could make it better in your environment.
> > > > IMHO, this is just for double check.
> > >
> > > I doubt this is actually better or smaller code.
> > >
> > > Check the actual object code using objdump and the
> > > struct alignment using pahole.
> >
> > I didn't have a chance to try it, but it looks quite likely to result in a
> > smaller data structure based on the other examples that I looked at.
>
> I _really_ doubt there is any difference in size between the
> below in any architecture
>
> struct foo {
> int bar;
> bool baz:1;
> int qux;
> };
>
> and
>
> struct foo {
> int bar;
> bool baz;
> int qux;
> };
>
> Where there would be a difference in size is
>
> struct foo {
> int bar;
> bool baz1:1;
> bool baz2:1;
> int qux;
> };
>
> and
>
> struct foo {
> int bar;
> bool baz1;
> bool baz2;
>
> int qux;
> };
In the situation of the example there are two bools together in the middle
of the structure and one at the end. Somehow, even converting to bool:1
increases the size. But it seems plausible that putting all three bools
together and converting them all to :1 would reduce the size. I don't
know. The size increase (more than 8 bytes) seems out of proportion for 3
bools.
I was able to check around 3000 structures that were not declared with any
attributes, that don't declare named types internally, and that are
compiled for x86. Around 10% become smaller whn using bool:1, typically
by at most 8 bytes.
julia
>
>
On 2018-04-19 01:16 PM, Julia Lawall wrote:
> On Wed, 18 Apr 2018, Joe Perches wrote:
>
>> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>> >
>> > On Wed, 18 Apr 2018, Joe Perches wrote:
>> >
>> > > On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
>> > > > Hi julia,
>> > > >
>> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
>> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
>> > > > >
>> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
>> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
>> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
>> > > > > > > > > We already have some 500 bools-in-structs
>> > > > > > > >
>> > > > > > > > I got at least triple that only in include/
>> > > > > > > > so I expect there are at probably an order
>> > > > > > > > of magnitude more than 500 in the kernel.
>> > > > > > > >
>> > > > > > > > I suppose some cocci script could count the
>> > > > > > > > actual number of instances. A regex can not.
>> > > > > > >
>> > > > > > > I got 12667.
>> > > > > >
>> > > > > > Could you please post the cocci script?
>> > > > > >
>> > > > > > > I'm not sure to understand the issue. Will using a bitfield help if there
>> > > > > > > are no other bitfields in the structure?
>> > > > > >
>> > > > > > IMO, not really.
>> > > > > >
>> > > > > > The primary issue is described by Linus here:
>> > > > > > https://lkml.org/lkml/2017/11/21/384
>> > > > > >
>> > > > > > I personally do not find a significant issue with
>> > > > > > uncontrolled sizes of bool in kernel structs as
>> > > > > > all of the kernel structs are transitory and not
>> > > > > > written out to storage.
>> > > > > >
>> > > > > > I suppose bool bitfields are also OK, but for the
>> > > > > > RMW required.
>> > > > > >
>> > > > > > Using unsigned int :1 bitfield instead of bool :1
>> > > > > > has the negative of truncation so that the uint
>> > > > > > has to be set with !! instead of a simple assign.
>> > > > >
>> > > > > At least with gcc 5.4.0, a number of structures become larger with
>> > > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
>> > > > > structure
>> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
>> > > > > with
>> > > > > both approaches.
>> > > >
>> > > > [ZJ] Hopefully, this could make it better in your environment.
>> > > > IMHO, this is just for double check.
>> > >
>> > > I doubt this is actually better or smaller code.
>> > >
>> > > Check the actual object code using objdump and the
>> > > struct alignment using pahole.
>> >
>> > I didn't have a chance to try it, but it looks quite likely to result in a
>> > smaller data structure based on the other examples that I looked at.
>>
>> I _really_ doubt there is any difference in size between the
>> below in any architecture
>>
>> struct foo {
>> int bar;
>> bool baz:1;
>> int qux;
>> };
>>
>> and
>>
>> struct foo {
>> int bar;
>> bool baz;
>> int qux;
>> };
>>
>> Where there would be a difference in size is
>>
>> struct foo {
>> int bar;
>> bool baz1:1;
>> bool baz2:1;
>> int qux;
>> };
>>
>> and
>>
>> struct foo {
>> int bar;
>> bool baz1;
>> bool baz2;
>>
>> int qux;
>> };
>
> In the situation of the example there are two bools together in the
> middle
> of the structure and one at the end. Somehow, even converting to
> bool:1
> increases the size. But it seems plausible that putting all three
> bools
> together and converting them all to :1 would reduce the size. I don't
> know. The size increase (more than 8 bytes) seems out of proportion
> for 3
> bools.
[ZJ] Typically, addition saving is due for difference padding.
>
> I was able to check around 3000 structures that were not declared with
> any
> attributes, that don't declare named types internally, and that are
> compiled for x86. Around 10% become smaller whn using bool:1,
> typically
> by at most 8 bytes.
>
> julia
>
>>
>>
On 2018-04-19 02:48 PM, [email protected] wrote:
> On 2018-04-19 01:16 PM, Julia Lawall wrote:
>> On Wed, 18 Apr 2018, Joe Perches wrote:
>>
>>> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>>> >
>>> > On Wed, 18 Apr 2018, Joe Perches wrote:
>>> >
>>> > > On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
>>> > > > Hi julia,
>>> > > >
>>> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
>>> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
>>> > > > >
>>> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
>>> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
>>> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
>>> > > > > > > > > We already have some 500 bools-in-structs
>>> > > > > > > >
>>> > > > > > > > I got at least triple that only in include/
>>> > > > > > > > so I expect there are at probably an order
>>> > > > > > > > of magnitude more than 500 in the kernel.
>>> > > > > > > >
>>> > > > > > > > I suppose some cocci script could count the
>>> > > > > > > > actual number of instances. A regex can not.
>>> > > > > > >
>>> > > > > > > I got 12667.
>>> > > > > >
>>> > > > > > Could you please post the cocci script?
>>> > > > > >
>>> > > > > > > I'm not sure to understand the issue. Will using a bitfield help if there
>>> > > > > > > are no other bitfields in the structure?
>>> > > > > >
>>> > > > > > IMO, not really.
>>> > > > > >
>>> > > > > > The primary issue is described by Linus here:
>>> > > > > > https://lkml.org/lkml/2017/11/21/384
>>> > > > > >
>>> > > > > > I personally do not find a significant issue with
>>> > > > > > uncontrolled sizes of bool in kernel structs as
>>> > > > > > all of the kernel structs are transitory and not
>>> > > > > > written out to storage.
>>> > > > > >
>>> > > > > > I suppose bool bitfields are also OK, but for the
>>> > > > > > RMW required.
>>> > > > > >
>>> > > > > > Using unsigned int :1 bitfield instead of bool :1
>>> > > > > > has the negative of truncation so that the uint
>>> > > > > > has to be set with !! instead of a simple assign.
>>> > > > >
>>> > > > > At least with gcc 5.4.0, a number of structures become larger with
>>> > > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
>>> > > > > structure
>>> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
>>> > > > > with
>>> > > > > both approaches.
>>> > > >
>>> > > > [ZJ] Hopefully, this could make it better in your environment.
>>> > > > IMHO, this is just for double check.
>>> > >
>>> > > I doubt this is actually better or smaller code.
>>> > >
>>> > > Check the actual object code using objdump and the
>>> > > struct alignment using pahole.
>>> >
>>> > I didn't have a chance to try it, but it looks quite likely to result in a
>>> > smaller data structure based on the other examples that I looked at.
>>>
>>> I _really_ doubt there is any difference in size between the
>>> below in any architecture
>>>
>>> struct foo {
>>> int bar;
>>> bool baz:1;
>>> int qux;
>>> };
>>>
>>> and
>>>
>>> struct foo {
>>> int bar;
>>> bool baz;
>>> int qux;
>>> };
>>>
>>> Where there would be a difference in size is
>>>
>>> struct foo {
>>> int bar;
>>> bool baz1:1;
>>> bool baz2:1;
>>> int qux;
>>> };
>>>
>>> and
>>>
>>> struct foo {
>>> int bar;
>>> bool baz1;
>>> bool baz2;
>>>
>>> int qux;
>>> };
[ZJ] Even though, two bool:1 are grouped in the #3, finally 4 bytes are
padded
due for int is the most significant in the type size.
At least, they are all the same per x86 and arm based on gcc.(12
bytes).
>>
>> In the situation of the example there are two bools together in the
>> middle
>> of the structure and one at the end. Somehow, even converting to
>> bool:1
>> increases the size. But it seems plausible that putting all three
>> bools
>> together and converting them all to :1 would reduce the size. I don't
>> know. The size increase (more than 8 bytes) seems out of proportion
>> for 3
>> bools.
> [ZJ] Typically, addition saving is due for difference padding.
>>
>> I was able to check around 3000 structures that were not declared with
>> any
>> attributes, that don't declare named types internally, and that are
>> compiled for x86. Around 10% become smaller whn using bool:1,
>> typically
>> by at most 8 bytes.
[ZJ] As my example, int (*)() requested 8 bytes in x86 arch, then 8
bytes is similiar to that.
While it request 4 bytes in arm arch. Typically, my previous
example struct can
reach to 32 bytes in x86 arch(compared to 40 bytes for original
version).
Similarly, 20 bytes in arm arch(compared to 24 bytes per original
version).
>>
>> julia
>>
>>>
>>>
On 2018-04-19 06:42 PM, [email protected] wrote:
> On 2018-04-19 02:48 PM, [email protected] wrote:
>> On 2018-04-19 01:16 PM, Julia Lawall wrote:
>>> On Wed, 18 Apr 2018, Joe Perches wrote:
>>>
>>>> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>>>> >
>>>> > On Wed, 18 Apr 2018, Joe Perches wrote:
>>>> >
>>>> > > On Tue, 2018-04-17 at 17:07 +0800, [email protected] wrote:
>>>> > > > Hi julia,
>>>> > > >
>>>> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
>>>> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
>>>> > > > >
>>>> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
>>>> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
>>>> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
>>>> > > > > > > > > We already have some 500 bools-in-structs
>>>> > > > > > > >
>>>> > > > > > > > I got at least triple that only in include/
>>>> > > > > > > > so I expect there are at probably an order
>>>> > > > > > > > of magnitude more than 500 in the kernel.
>>>> > > > > > > >
>>>> > > > > > > > I suppose some cocci script could count the
>>>> > > > > > > > actual number of instances. A regex can not.
>>>> > > > > > >
>>>> > > > > > > I got 12667.
>>>> > > > > >
>>>> > > > > > Could you please post the cocci script?
>>>> > > > > >
>>>> > > > > > > I'm not sure to understand the issue. Will using a bitfield help if there
>>>> > > > > > > are no other bitfields in the structure?
>>>> > > > > >
>>>> > > > > > IMO, not really.
>>>> > > > > >
>>>> > > > > > The primary issue is described by Linus here:
>>>> > > > > > https://lkml.org/lkml/2017/11/21/384
>>>> > > > > >
>>>> > > > > > I personally do not find a significant issue with
>>>> > > > > > uncontrolled sizes of bool in kernel structs as
>>>> > > > > > all of the kernel structs are transitory and not
>>>> > > > > > written out to storage.
>>>> > > > > >
>>>> > > > > > I suppose bool bitfields are also OK, but for the
>>>> > > > > > RMW required.
>>>> > > > > >
>>>> > > > > > Using unsigned int :1 bitfield instead of bool :1
>>>> > > > > > has the negative of truncation so that the uint
>>>> > > > > > has to be set with !! instead of a simple assign.
>>>> > > > >
>>>> > > > > At least with gcc 5.4.0, a number of structures become larger with
>>>> > > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
>>>> > > > > structure
>>>> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
>>>> > > > > with
>>>> > > > > both approaches.
>>>> > > >
>>>> > > > [ZJ] Hopefully, this could make it better in your environment.
>>>> > > > IMHO, this is just for double check.
>>>> > >
>>>> > > I doubt this is actually better or smaller code.
>>>> > >
>>>> > > Check the actual object code using objdump and the
>>>> > > struct alignment using pahole.
>>>> >
>>>> > I didn't have a chance to try it, but it looks quite likely to result in a
>>>> > smaller data structure based on the other examples that I looked at.
>>>>
>>>> I _really_ doubt there is any difference in size between the
>>>> below in any architecture
>>>>
>>>> struct foo {
>>>> int bar;
>>>> bool baz:1;
>>>> int qux;
>>>> };
>>>>
>>>> and
>>>>
>>>> struct foo {
>>>> int bar;
>>>> bool baz;
>>>> int qux;
>>>> };
>>>>
>>>> Where there would be a difference in size is
>>>>
>>>> struct foo {
>>>> int bar;
>>>> bool baz1:1;
>>>> bool baz2:1;
>>>> int qux;
>>>> };
>>>>
>>>> and
>>>>
>>>> struct foo {
>>>> int bar;
>>>> bool baz1;
>>>> bool baz2;
>>>>
>>>> int qux;
>>>> };
> [ZJ] Even though, two bool:1 are grouped in the #3, finally 4 bytes are
> padded
> due for int is the most significant in the type size.
> At least, they are all the same per x86 and arm based on gcc.(12
> bytes).
[ZJ] However, #3 could be difference to #4 if compiling it if the size
of (_Bool)
is a bigger value(4 bytes maybe available in Alpha EV45 for ex.).
>>>
>>> In the situation of the example there are two bools together in the
>>> middle
>>> of the structure and one at the end. Somehow, even converting to
>>> bool:1
>>> increases the size. But it seems plausible that putting all three
>>> bools
>>> together and converting them all to :1 would reduce the size. I
>>> don't
>>> know. The size increase (more than 8 bytes) seems out of proportion
>>> for 3
>>> bools.
>> [ZJ] Typically, addition saving is due for difference padding.
>>>
>>> I was able to check around 3000 structures that were not declared
>>> with any
>>> attributes, that don't declare named types internally, and that are
>>> compiled for x86. Around 10% become smaller whn using bool:1,
>>> typically
>>> by at most 8 bytes.
> [ZJ] As my example, int (*)() requested 8 bytes in x86 arch, then 8
> bytes is similiar to that.
> While it request 4 bytes in arm arch. Typically, my previous
> example struct can
> reach to 32 bytes in x86 arch(compared to 40 bytes for original
> version).
> Similarly, 20 bytes in arm arch(compared to 24 bytes per original
> version).
>>>
>>> julia
>>>
>>>>
>>>>
On 2018-04-11 07:20 AM, [email protected] wrote:
> ++
> On 2018-04-11 07:09 AM, [email protected] wrote:
>> ++
>>
>> On 2018-04-10 10:49 PM, [email protected] wrote:
>>> Typo...
>>>
>>> On 2018-04-10 10:08 PM, [email protected] wrote:
>>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>>>> On Tue, 10 Apr 2018, [email protected] wrote:
>>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>>>> > On Tue, 10 Apr 2018, [email protected] wrote:
>>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>>>>>> > > > > From: John Zhao <[email protected]>
>>>>>> > > > >
>>>>>> > > > > 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 <[email protected]>
>>>>>> > > > > ---
>>>>>> > > > > 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, [email protected] wrote:
> On 2018-04-11 07:20 AM, [email protected] wrote:
>> ++
>> On 2018-04-11 07:09 AM, [email protected] wrote:
>>> ++
>>>
>>> On 2018-04-10 10:49 PM, [email protected] wrote:
>>>> Typo...
>>>>
>>>> On 2018-04-10 10:08 PM, [email protected] wrote:
>>>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>>>>> On Tue, 10 Apr 2018, [email protected] wrote:
>>>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>>>>> > On Tue, 10 Apr 2018, [email protected] wrote:
>>>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM, <[email protected]> wrote:
>>>>>>> > > > > From: John Zhao <[email protected]>
>>>>>>> > > > >
>>>>>>> > > > > 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 <[email protected]>
>>>>>>> > > > > ---
>>>>>>> > > > > 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, [email protected] wrote:
> On 2018-04-20 09:47 AM, [email protected] 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, [email protected] wrote:
>> On 2018-04-20 09:47 AM, [email protected] 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.