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

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

[yuankuiz]

 From: John Zhao <yuankuiz@codeaurora.org>

Variable tick_stopped returned by tick_nohz_tick_stopped
can have only true / forse values. Since the return type
of the tick_nohz_tick_stopped is also bool, variable
tick_stopped nice to have data type as bool in place of int.
Moreover, the executed instructions cost could be minimal
without potiential data type conversion.

Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
---
  kernel/time/tick-sched.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 6de959a..4d34309 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -48,8 +48,8 @@ struct tick_sched {
  	unsigned long			check_clocks;
  	enum tick_nohz_mode		nohz_mode;

+	bool				tick_stopped	: 1;
  	unsigned int			inidle		: 1;
-	unsigned int			tick_stopped	: 1;
  	unsigned int			idle_active	: 1;
  	unsigned int			do_timer_last	: 1;
  	unsigned int			got_idle_tick	: 1;

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

[yuankuiz]

 From: John Zhao <yuankuiz@codeaurora.org>

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 <yuankuiz@codeaurora.org>
---
  kernel/time/tick-sched.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 6de959a..4d34309 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -48,8 +48,8 @@ struct tick_sched {
  	unsigned long			check_clocks;
  	enum tick_nohz_mode		nohz_mode;

+	bool				tick_stopped	: 1;
  	unsigned int			inidle		: 1;
-	unsigned int			tick_stopped	: 1;
  	unsigned int			idle_active	: 1;
  	unsigned int			do_timer_last	: 1;
  	unsigned int			got_idle_tick	: 1;

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

[Thomas Gleixner]

On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:

> From: John Zhao <yuankuiz@codeaurora.org>
> 
> Variable tick_stopped returned by tick_nohz_tick_stopped
> can have only true / forse values. Since the return type
> of the tick_nohz_tick_stopped is also bool, variable
> tick_stopped nice to have data type as bool in place of int.

The data type is not int. It's part of an integer bitfield and occupies
exactly one bit of storage, while bool has an architecture dependend size
and is at least 1 byte, i.e. 8 bit. So with alignment effects you grew the
size of the data structure and therefore the cache foot print.

This is not about 'nice to have' ....

Thanks,

	tglx

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

[Rafael J. Wysocki]

On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
> From: John Zhao <yuankuiz@codeaurora.org>
>
> Variable tick_stopped returned by tick_nohz_tick_stopped
> can have only true / forse values. Since the return type
> of the tick_nohz_tick_stopped is also bool, variable
> tick_stopped nice to have data type as bool in place of int.
> Moreover, the executed instructions cost could be minimal
> without potiential data type conversion.
>
> Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
> ---
>  kernel/time/tick-sched.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 6de959a..4d34309 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -48,8 +48,8 @@ struct tick_sched {
>         unsigned long                   check_clocks;
>         enum tick_nohz_mode             nohz_mode;
>
> +       bool                            tick_stopped    : 1;
>         unsigned int                    inidle          : 1;
> -       unsigned int                    tick_stopped    : 1;
>         unsigned int                    idle_active     : 1;
>         unsigned int                    do_timer_last   : 1;
>         unsigned int                    got_idle_tick   : 1;

I don't think this is a good idea at all.

Please see https://lkml.org/lkml/2017/11/21/384 for example.

Thanks!

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

[yuankuiz]

On 2018-04-10 03:55 PM, Thomas Gleixner wrote:
> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
> 
>> From: John Zhao <yuankuiz@codeaurora.org>
>> 
>> Variable tick_stopped returned by tick_nohz_tick_stopped
>> can have only true / forse values. Since the return type
>> of the tick_nohz_tick_stopped is also bool, variable
>> tick_stopped nice to have data type as bool in place of int.
> 
> The data type is not int.
[ZJ] Yes. Per newest tip in branch of linux-pm-cpuidle, it is unsigned 
int with 1 bit in width.
There is typo in commit message, "int" at here should be "unsigned int"

> It's part of an integer bitfield and occupies
> exactly one bit of storage, while bool has an architecture dependend 
> size
> and is at least 1 byte, i.e. 8 bit. So with alignment effects you grew 
> the
> size of the data structure and therefore the cache foot print.
[ZJ] So, 1 bit in width is specified as:
+	bool				tick_stopped	: 1;
      This patch is based on the linux-pm-cpuidle branch which has
already gathered available space in the structure of tick_sched.

> 
> This is not about 'nice to have' ....
> 
> Thanks,
> 
> 	tglx

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

[yuankuiz]

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

> 
> Thanks!

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

[yuankuiz]

 From: John Zhao <yuankuiz@codeaurora.org>

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 <yuankuiz@codeaurora.org>
---
  kernel/time/tick-sched.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 6de959a..4d34309 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -48,8 +48,8 @@ struct tick_sched {
  	unsigned long			check_clocks;
  	enum tick_nohz_mode		nohz_mode;

+	bool				tick_stopped	: 1;
  	unsigned int			inidle		: 1;
-	unsigned int			tick_stopped	: 1;
  	unsigned int			idle_active	: 1;
  	unsigned int			do_timer_last	: 1;
  	unsigned int			got_idle_tick	: 1;

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

[yuankuiz]

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, yuankuiz@codeaurora.org wrote:
> From: John Zhao <yuankuiz@codeaurora.org>
> 
> 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 <yuankuiz@codeaurora.org>
> ---
>  kernel/time/tick-sched.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 6de959a..4d34309 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -48,8 +48,8 @@ struct tick_sched {
>  	unsigned long			check_clocks;
>  	enum tick_nohz_mode		nohz_mode;
> 
> +	bool				tick_stopped	: 1;
>  	unsigned int			inidle		: 1;
> -	unsigned int			tick_stopped	: 1;
>  	unsigned int			idle_active	: 1;
>  	unsigned int			do_timer_last	: 1;
>  	unsigned int			got_idle_tick	: 1;

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

[Thomas Gleixner]

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

This patch falls into the case 'pointless' because it adds extra storage
for no benefit at all.

Thanks,

	tglx

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

[yuankuiz]

Hi Thomas,

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

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

> 
> Thanks,
> 
> 	tglx

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

[Thomas Gleixner]

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

Groan. No. Care to look at the data structure? You create a new storage,
which is incidentally merged into the other bitfield by the compiler at a
different bit position, but there is no guarantee that a compiler does
that. It's free to use distinct storage for that bool based bit.

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

A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to be
evaluated as a bit. So there is a type conversion from BIT to bool required
because BIT != bool.

By chance the evaluation can be done by evaluating the byte in which the
bit is placed just because the compiler knows that the remaining bits are
not used. There is no guarantee that this is done, it happens to be true
for a particular compiler.

But that does not make it any more interesting. It just makes the code
harder to read and eventually leads to bigger storage.

Thanks,

	tglx

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

[Peter Zijlstra]

On Tue, Apr 10, 2018 at 06:07:17PM +0800, yuankuiz@codeaurora.org wrote:
> > > > > @@ -48,8 +48,8 @@ struct tick_sched {
> > > > >         unsigned long                   check_clocks;
> > > > >         enum tick_nohz_mode             nohz_mode;
> > > > >
> > > > > +       bool                            tick_stopped    : 1;
> > > > >         unsigned int                    inidle          : 1;
> > > > > -       unsigned int                    tick_stopped    : 1;
> > > > >         unsigned int                    idle_active     : 1;
> > > > >         unsigned int                    do_timer_last   : 1;
> > > > >         unsigned int                    got_idle_tick   : 1;
> > > >
> > > > I don't think this is a good idea at all.
> > > >
> > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
> > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the
> > > case of
> > > "Maybe".
> > 
> > This patch falls into the case 'pointless' because it adds extra storage
> [ZJ] 1 bit vs 1 bit. no more.

Since its a different type, the bitfields will not be merged. Also I'm
surprised a bitfield with base-type _Bool is even allowed.

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

Do you have any actual evidence for that? Is there a compiler stupid
enough to generate code to convert a bool to a 1bit value?

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

[Thomas Gleixner]

On Tue, 10 Apr 2018, Peter Zijlstra wrote:

> On Tue, Apr 10, 2018 at 06:07:17PM +0800, yuankuiz@codeaurora.org wrote:
> > > > > > @@ -48,8 +48,8 @@ struct tick_sched {
> > > > > >         unsigned long                   check_clocks;
> > > > > >         enum tick_nohz_mode             nohz_mode;
> > > > > >
> > > > > > +       bool                            tick_stopped    : 1;
> > > > > >         unsigned int                    inidle          : 1;
> > > > > > -       unsigned int                    tick_stopped    : 1;
> > > > > >         unsigned int                    idle_active     : 1;
> > > > > >         unsigned int                    do_timer_last   : 1;
> > > > > >         unsigned int                    got_idle_tick   : 1;
> > > > >
> > > > > I don't think this is a good idea at all.
> > > > >
> > > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
> > > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the
> > > > case of
> > > > "Maybe".
> > > 
> > > This patch falls into the case 'pointless' because it adds extra storage
> > [ZJ] 1 bit vs 1 bit. no more.
> 
> Since its a different type, the bitfields will not be merged. Also I'm
> surprised a bitfield with base-type _Bool is even allowed.
> 
> > > for no benefit at all.
> > [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() which is bool.
> > The benefit is no any potiential type conversion could be minded.
> 
> Do you have any actual evidence for that? Is there a compiler stupid
> enough to generate code to convert a bool to a 1bit value?

Sure, if you do:

> > > > > > +       bool                            tick_stopped    : 1;

which is stupidly allowed by the standard....

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

[Peter Zijlstra]

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.

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

[Peter Zijlstra]

On Tue, Apr 10, 2018 at 10:00:01AM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
> > +++ b/kernel/time/tick-sched.h
> > @@ -48,8 +48,8 @@ struct tick_sched {
> >         unsigned long                   check_clocks;
> >         enum tick_nohz_mode             nohz_mode;
> >
> > +       bool                            tick_stopped    : 1;
> >         unsigned int                    inidle          : 1;
> > -       unsigned int                    tick_stopped    : 1;
> >         unsigned int                    idle_active     : 1;
> >         unsigned int                    do_timer_last   : 1;
> >         unsigned int                    got_idle_tick   : 1;
> 
> I don't think this is a good idea at all.
> 
> Please see https://lkml.org/lkml/2017/11/21/384 for example.

Joe, apw, could we get Checkpatch to whinge about _Bool in composite
types? That should immediately also disqualify using it as the base type
of bitfields.

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

[yuankuiz]

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

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

> >> > for no benefit at all.
>> [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() which is 
>> bool.
>> The benefit is no any potiential type conversion could be minded.
> 
> A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to be
> evaluated as a bit. So there is a type conversion from BIT to bool 
> required
> because BIT != bool.
[ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
"If  the  value  0  or  1  is  stored  into  a  nonzero-width  bit-field 
  of  types
_Bool, the value of the bit-field shall compare equal to the value 
stored."
Obviously, it is nothing related to type conversion actually.
> 
> By chance the evaluation can be done by evaluating the byte in which 
> the
> bit is placed just because the compiler knows that the remaining bits 
> are
> not used. There is no guarantee that this is done, it happens to be 
> true
> for a particular compiler.
[ZJ] Actually, such as GCC owe that kind of guarantee to be promised by 
ABI.
> 
> But that does not make it any more interesting. It just makes the code
> harder to read and eventually leads to bigger storage.
[ZJ] To get the benctifit to be profiled, it is given as:
number of instructions of function tick_nohz_tick_stopped():
                         original: 17
                         patched:  14
      Which was saved is:
                movzbl	%al, %eax
                testl	%eax, %eax
                setne    %al
      Say, 3 / 17 = 17 % could be gained in the instruction executed for 
this function can be evaluated.

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

> 
> Thanks,
> 
> 	tglx

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

[yuankuiz]

Typo...

On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote:
> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>> > > > On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
>>> > > > > From: John Zhao <yuankuiz@codeaurora.org>
>>> > > > >
>>> > > > > Variable tick_stopped returned by tick_nohz_tick_stopped
>>> > > > > can have only true / false values. Since the return type
>>> > > > > of the tick_nohz_tick_stopped is also bool, variable
>>> > > > > tick_stopped nice to have data type as bool in place of unsigned int.
>>> > > > > Moreover, the executed instructions cost could be minimal
>>> > > > > without potiential data type conversion.
>>> > > > >
>>> > > > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
>>> > > > > ---
>>> > > > >  kernel/time/tick-sched.h | 2 +-
>>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>>> > > > >
>>> > > > > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
>>> > > > > index 6de959a..4d34309 100644
>>> > > > > --- a/kernel/time/tick-sched.h
>>> > > > > +++ b/kernel/time/tick-sched.h
>>> > > > > @@ -48,8 +48,8 @@ struct tick_sched {
>>> > > > >         unsigned long                   check_clocks;
>>> > > > >         enum tick_nohz_mode             nohz_mode;
>>> > > > >
>>> > > > > +       bool                            tick_stopped    : 1;
>>> > > > >         unsigned int                    inidle          : 1;
>>> > > > > -       unsigned int                    tick_stopped    : 1;
>>> > > > >         unsigned int                    idle_active     : 1;
>>> > > > >         unsigned int                    do_timer_last   : 1;
>>> > > > >         unsigned int                    got_idle_tick   : 1;
>>> > > >
>>> > > > I don't think this is a good idea at all.
>>> > > >
>>> > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
>>> > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the case of
>>> > > "Maybe".
>>> >
>>> > This patch falls into the case 'pointless' because it adds extra storage
>>> [ZJ] 1 bit vs 1 bit. no more.
>> 
>> Groan. No. Care to look at the data structure? You create a new 
>> storage,
> [ZJ] Say, {unsigned int, unsigned int, unsigned int, unsigned int,
> unsigned int} becomes
>           {bool        , unsigned int, unsigned int, unsigned int, 
> unsigned int}
> As specified by the rule No.10 at the section 6.7.2.1 of C99 TC2 as:
> "If enough space remains, a bit-field that immediately follows another
> bit-field in a
> structure shall be packed into adjacent bits of the same unit." What
> is the new storage so far?
> 
>> which is incidentally merged into the other bitfield by the compiler 
>> at a
>> different bit position, but there is no guarantee that a compiler does
>> that. It's free to use distinct storage for that bool based bit.
> [ZJ] Per the rule No.10 at section 6.7.2.1 of C99 TC2 as:
> " If insufficient space remains, whether  a  bit-field  that  does
> not  fit  is  put  into
> the  next  unit  or overlaps  adjacent  units  is 
> implementation-defined."
> So, implementation is never mind which type will be stored if any.
> 
>> >> > for no benefit at all.
>>> [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() which 
>>> is bool.
>>> The benefit is no any potiential type conversion could be minded.
>> 
>> A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to be
>> evaluated as a bit. So there is a type conversion from BIT to bool 
>> required
>> because BIT != bool.
> [ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
> "If  the  value  0  or  1  is  stored  into  a  nonzero-width
> bit-field  of  types
> _Bool, the value of the bit-field shall compare equal to the value 
> stored."
> Obviously, it is nothing related to type conversion actually.
>> 
>> By chance the evaluation can be done by evaluating the byte in which 
>> the
>> bit is placed just because the compiler knows that the remaining bits 
>> are
>> not used. There is no guarantee that this is done, it happens to be 
>> true
>> for a particular compiler.
> [ZJ] Actually, such as GCC owe that kind of guarantee to be promised by 
> ABI.
>> 
>> But that does not make it any more interesting. It just makes the code
>> harder to read and eventually leads to bigger storage.
> [ZJ] To get the benctifit to be profiled, it is given as:
> number of instructions of function tick_nohz_tick_stopped():
[ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an 
evaluation() as:
#include <stdio.h>
#include <stdbool.h>

struct tick_sched {
         unsigned int inidle             : 1;
         unsigned int tick_stopped       : 1;
};

bool get_status()
{
         struct tick_sched *ts;
         ts->tick_stopped = 1;
         return ts->tick_stopped;
}

int main()
{
         if (get_status()) return 0;
         return 0;
}

[ZJ] Toggle the declaration of tick_stopped in side of the tick_sched 
structure for comparison.


>                         original: 17
>                         patched:  14
>      Which was saved is:
>                movzbl	%al, %eax
>                testl	%eax, %eax
>                setne    %al
>      Say, 3 / 17 = 17 % could be gained in the instruction executed
> for this function can be evaluated.
> 
> Note:
>      The environment I used is:
>                OS : Ubuntu Desktop 16.04 LTS
>                gcc: 6.3.0                       (without optimization
> for in general purpose)
> 
>> 

Just FYI.

Thanks,
ZJ

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

[Joe Perches]

On Tue, 2018-04-10 at 14:33 +0200, Peter Zijlstra wrote:
> On Tue, Apr 10, 2018 at 10:00:01AM +0200, Rafael J. Wysocki wrote:
> > On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
> > > +++ b/kernel/time/tick-sched.h
> > > @@ -48,8 +48,8 @@ struct tick_sched {
> > >         unsigned long                   check_clocks;
> > >         enum tick_nohz_mode             nohz_mode;
> > > 
> > > +       bool                            tick_stopped    : 1;
> > >         unsigned int                    inidle          : 1;
> > > -       unsigned int                    tick_stopped    : 1;
> > >         unsigned int                    idle_active     : 1;
> > >         unsigned int                    do_timer_last   : 1;
> > >         unsigned int                    got_idle_tick   : 1;
> > 
> > I don't think this is a good idea at all.
> > 
> > Please see https://lkml.org/lkml/2017/11/21/384 for example.
> 
> Joe, apw, could we get Checkpatch to whinge about _Bool in composite
> types? That should immediately also disqualify using it as the base type
> of bitfields.

Whinging about bool <foo> : <x> seems entirely sensible
and straightforward to do.

I'm not so sure about bool in structs as a patch context
could be adding a bool to local stack definitions and
there's no real ability to determine if the bool is in a
struct or on the stack.

Also, I think there's nothing really wrong with using
bool in structs.  Steven Rostedt's rationale in
https://lkml.org/lkml/2017/11/21/207 isn't really right
as sizeof(int) is 4 not 1 and sizeof(bool) is 1 on arches
without alignment issues.  I believe when using gcc,
sizeof(bool) is always 1 and there may be alignment padding
added on some arches.  Dunno.

But I think the battle is already lost anyway.

git grep -P  '(?<!static|extern)\s+bool\s+\w+\s*;' include | wc -l
1543

[PATCH] checkpatch: whinge about bool bitfields

[Joe Perches]

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 <joe@perches.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
---
 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",

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

[Peter Zijlstra]

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.

[PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

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 <joe@perches.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
---
 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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Andrew Morton]

On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <joe@perches.com> 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",

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <joe@perches.com> 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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Andrew Morton]

On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches <joe@perches.com> wrote:

> On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <joe@perches.com> 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.

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

[yuankuiz]

++

On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote:
> Typo...
> 
> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote:
>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org>
>>>> > > > >
>>>> > > > > Variable tick_stopped returned by tick_nohz_tick_stopped
>>>> > > > > can have only true / false values. Since the return type
>>>> > > > > of the tick_nohz_tick_stopped is also bool, variable
>>>> > > > > tick_stopped nice to have data type as bool in place of unsigned int.
>>>> > > > > Moreover, the executed instructions cost could be minimal
>>>> > > > > without potiential data type conversion.
>>>> > > > >
>>>> > > > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
>>>> > > > > ---
>>>> > > > >  kernel/time/tick-sched.h | 2 +-
>>>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>> > > > >
>>>> > > > > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
>>>> > > > > index 6de959a..4d34309 100644
>>>> > > > > --- a/kernel/time/tick-sched.h
>>>> > > > > +++ b/kernel/time/tick-sched.h
>>>> > > > > @@ -48,8 +48,8 @@ struct tick_sched {
>>>> > > > >         unsigned long                   check_clocks;
>>>> > > > >         enum tick_nohz_mode             nohz_mode;
>>>> > > > >
>>>> > > > > +       bool                            tick_stopped    : 1;
>>>> > > > >         unsigned int                    inidle          : 1;
>>>> > > > > -       unsigned int                    tick_stopped    : 1;
>>>> > > > >         unsigned int                    idle_active     : 1;
>>>> > > > >         unsigned int                    do_timer_last   : 1;
>>>> > > > >         unsigned int                    got_idle_tick   : 1;
>>>> > > >
>>>> > > > I don't think this is a good idea at all.
>>>> > > >
>>>> > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
>>>> > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the case of
>>>> > > "Maybe".
>>>> >
>>>> > This patch falls into the case 'pointless' because it adds extra storage
>>>> [ZJ] 1 bit vs 1 bit. no more.
>>> 
>>> Groan. No. Care to look at the data structure? You create a new 
>>> storage,
>> [ZJ] Say, {unsigned int, unsigned int, unsigned int, unsigned int,
>> unsigned int} becomes
>>           {bool        , unsigned int, unsigned int, unsigned int, 
>> unsigned int}
>> As specified by the rule No.10 at the section 6.7.2.1 of C99 TC2 as:
>> "If enough space remains, a bit-field that immediately follows another
>> bit-field in a
>> structure shall be packed into adjacent bits of the same unit." What
>> is the new storage so far?
>> 
>>> which is incidentally merged into the other bitfield by the compiler 
>>> at a
>>> different bit position, but there is no guarantee that a compiler 
>>> does
>>> that. It's free to use distinct storage for that bool based bit.
>> [ZJ] Per the rule No.10 at section 6.7.2.1 of C99 TC2 as:
>> " If insufficient space remains, whether  a  bit-field  that  does
>> not  fit  is  put  into
>> the  next  unit  or overlaps  adjacent  units  is 
>> implementation-defined."
>> So, implementation is never mind which type will be stored if any.
>> 
>>> >> > for no benefit at all.
>>>> [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() which 
>>>> is bool.
>>>> The benefit is no any potiential type conversion could be minded.
>>> 
>>> A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to 
>>> be
>>> evaluated as a bit. So there is a type conversion from BIT to bool 
>>> required
>>> because BIT != bool.
>> [ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
>> "If  the  value  0  or  1  is  stored  into  a  nonzero-width
>> bit-field  of  types
>> _Bool, the value of the bit-field shall compare equal to the value 
>> stored."
>> Obviously, it is nothing related to type conversion actually.
>>> 
>>> By chance the evaluation can be done by evaluating the byte in which 
>>> the
>>> bit is placed just because the compiler knows that the remaining bits 
>>> are
>>> not used. There is no guarantee that this is done, it happens to be 
>>> true
>>> for a particular compiler.
>> [ZJ] Actually, such as GCC owe that kind of guarantee to be promised 
>> by ABI.
>>> 
>>> But that does not make it any more interesting. It just makes the 
>>> code
>>> harder to read and eventually leads to bigger storage.
>> [ZJ] To get the benctifit to be profiled, it is given as:
>> number of instructions of function tick_nohz_tick_stopped():
> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an 
> evaluation() as:
> #include <stdio.h>
> #include <stdbool.h>
> 
> struct tick_sched {
>         unsigned int inidle             : 1;
>         unsigned int tick_stopped       : 1;
> };
> 
> bool get_status()
> {
>         struct tick_sched *ts;
>         ts->tick_stopped = 1;
>         return ts->tick_stopped;
> }
> 
> int main()
> {
>         if (get_status()) return 0;
>         return 0;
> }
> 
> [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched
> structure for comparison.
> 
> 
>>                         original: 17
>>                         patched:  14
>>      Which was saved is:
>>                movzbl	%al, %eax
>>                testl	%eax, %eax
>>                setne    %al
>>      Say, 3 / 17 = 17 % could be gained in the instruction executed
>> for this function can be evaluated.
>> 
>> Note:
>>      The environment I used is:
>>                OS : Ubuntu Desktop 16.04 LTS
>>                gcc: 6.3.0                       (without optimization
>> for in general purpose)
>> 
>>> 
> 
> Just FYI.
> 
> Thanks,
> ZJ

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

[yuankuiz]

++
On 2018-04-11 07:09 AM, yuankuiz@codeaurora.org wrote:
> ++
> 
> On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote:
>> Typo...
>> 
>> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote:
>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
>>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org>
>>>>> > > > >
>>>>> > > > > Variable tick_stopped returned by tick_nohz_tick_stopped
>>>>> > > > > can have only true / false values. Since the return type
>>>>> > > > > of the tick_nohz_tick_stopped is also bool, variable
>>>>> > > > > tick_stopped nice to have data type as bool in place of unsigned int.
>>>>> > > > > Moreover, the executed instructions cost could be minimal
>>>>> > > > > without potiential data type conversion.
>>>>> > > > >
>>>>> > > > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
>>>>> > > > > ---
>>>>> > > > >  kernel/time/tick-sched.h | 2 +-
>>>>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> > > > >
>>>>> > > > > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
>>>>> > > > > index 6de959a..4d34309 100644
>>>>> > > > > --- a/kernel/time/tick-sched.h
>>>>> > > > > +++ b/kernel/time/tick-sched.h
>>>>> > > > > @@ -48,8 +48,8 @@ struct tick_sched {
>>>>> > > > >         unsigned long                   check_clocks;
>>>>> > > > >         enum tick_nohz_mode             nohz_mode;
>>>>> > > > >
>>>>> > > > > +       bool                            tick_stopped    : 1;
>>>>> > > > >         unsigned int                    inidle          : 1;
>>>>> > > > > -       unsigned int                    tick_stopped    : 1;
>>>>> > > > >         unsigned int                    idle_active     : 1;
>>>>> > > > >         unsigned int                    do_timer_last   : 1;
>>>>> > > > >         unsigned int                    got_idle_tick   : 1;
>>>>> > > >
>>>>> > > > I don't think this is a good idea at all.
>>>>> > > >
>>>>> > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
>>>>> > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the case of
>>>>> > > "Maybe".
>>>>> >
>>>>> > This patch falls into the case 'pointless' because it adds extra storage
>>>>> [ZJ] 1 bit vs 1 bit. no more.
>>>> 
>>>> Groan. No. Care to look at the data structure? You create a new 
>>>> storage,
>>> [ZJ] Say, {unsigned int, unsigned int, unsigned int, unsigned int,
>>> unsigned int} becomes
>>>           {bool        , unsigned int, unsigned int, unsigned int, 
>>> unsigned int}
>>> As specified by the rule No.10 at the section 6.7.2.1 of C99 TC2 as:
>>> "If enough space remains, a bit-field that immediately follows 
>>> another
>>> bit-field in a
>>> structure shall be packed into adjacent bits of the same unit." What
>>> is the new storage so far?
>>> 
>>>> which is incidentally merged into the other bitfield by the compiler 
>>>> at a
>>>> different bit position, but there is no guarantee that a compiler 
>>>> does
>>>> that. It's free to use distinct storage for that bool based bit.
>>> [ZJ] Per the rule No.10 at section 6.7.2.1 of C99 TC2 as:
>>> " If insufficient space remains, whether  a  bit-field  that  does
>>> not  fit  is  put  into
>>> the  next  unit  or overlaps  adjacent  units  is 
>>> implementation-defined."
>>> So, implementation is never mind which type will be stored if any.
>>> 
>>>> >> > for no benefit at all.
>>>>> [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() which 
>>>>> is bool.
>>>>> The benefit is no any potiential type conversion could be minded.
>>>> 
>>>> A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to 
>>>> be
>>>> evaluated as a bit. So there is a type conversion from BIT to bool 
>>>> required
>>>> because BIT != bool.
>>> [ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
>>> "If  the  value  0  or  1  is  stored  into  a  nonzero-width
>>> bit-field  of  types
>>> _Bool, the value of the bit-field shall compare equal to the value 
>>> stored."
>>> Obviously, it is nothing related to type conversion actually.
>>>> 
>>>> By chance the evaluation can be done by evaluating the byte in which 
>>>> the
>>>> bit is placed just because the compiler knows that the remaining 
>>>> bits are
>>>> not used. There is no guarantee that this is done, it happens to be 
>>>> true
>>>> for a particular compiler.
>>> [ZJ] Actually, such as GCC owe that kind of guarantee to be promised 
>>> by ABI.
>>>> 
>>>> But that does not make it any more interesting. It just makes the 
>>>> code
>>>> harder to read and eventually leads to bigger storage.
>>> [ZJ] To get the benctifit to be profiled, it is given as:
>>> number of instructions of function tick_nohz_tick_stopped():
>> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an 
>> evaluation() as:
>> #include <stdio.h>
>> #include <stdbool.h>
>> 
>> struct tick_sched {
>>         unsigned int inidle             : 1;
>>         unsigned int tick_stopped       : 1;
>> };
>> 
>> bool get_status()
>> {
>>         struct tick_sched *ts;
>>         ts->tick_stopped = 1;
>>         return ts->tick_stopped;
>> }
>> 
>> int main()
>> {
>>         if (get_status()) return 0;
>>         return 0;
>> }
>> 
>> [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched
>> structure for comparison.
>> 
>> 
>>>                         original: 17
>>>                         patched:  14
>>>      Which was saved is:
>>>                movzbl	%al, %eax
>>>                testl	%eax, %eax
>>>                setne    %al
>>>      Say, 3 / 17 = 17 % could be gained in the instruction executed
>>> for this function can be evaluated.
>>> 
>>> Note:
>>>      The environment I used is:
>>>                OS : Ubuntu Desktop 16.04 LTS
>>>                gcc: 6.3.0                       (without optimization
>>> for in general purpose)
>>> 
>>>> 
>> 
>> Just FYI.
>> 
>> Thanks,
>> ZJ

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Peter Zijlstra]

On Tue, Apr 10, 2018 at 03:00:11PM -0700, Andrew Morton wrote:
> On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <joe@perches.com> 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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Andrew Morton]

On Wed, 11 Apr 2018 10:15:02 +0200 Peter Zijlstra <peterz@infradead.org> 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 <joe@perches.com> wrote:
> > 
> > > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote:
> > > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches <joe@perches.com> 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(?).

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

(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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Peter Zijlstra]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

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.
>
>

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> 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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Peter Zijlstra]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Peter Zijlstra]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Andrea Parri]

Hi,

On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> 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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Peter Zijlstra]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Kalle Valo]

Andrea Parri <andrea.parri@amarulasolutions.com> writes:

> On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote:
>> 
>> * Peter Zijlstra <peterz@infradead.org> 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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Peter Zijlstra]

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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

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 webmaster@kernel.org, postmaster@kernel.org 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.

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Andrew Morton]

On Thu, 12 Apr 2018 14:08:32 +0200 Peter Zijlstra <peterz@infradead.org> 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...

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[yuankuiz]

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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

On Tue, 2018-04-17 at 17:07 +0800, yuankuiz@codeaurora.org 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

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

On Wed, 18 Apr 2018, Joe Perches wrote:

> On Tue, 2018-04-17 at 17:07 +0800, yuankuiz@codeaurora.org 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
>

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

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, yuankuiz@codeaurora.org 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;
};

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

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, yuankuiz@codeaurora.org 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

>
>

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[yuankuiz]

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, yuankuiz@codeaurora.org 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
> 
>> 
>> 

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[yuankuiz]

On 2018-04-19 02:48 PM, yuankuiz@codeaurora.org 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, yuankuiz@codeaurora.org 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
>> 
>>> 
>>> 

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[yuankuiz]

On 2018-04-19 06:42 PM, yuankuiz@codeaurora.org wrote:
> On 2018-04-19 02:48 PM, yuankuiz@codeaurora.org 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, yuankuiz@codeaurora.org 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
>>> 
>>>> 
>>>> 

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

[yuankuiz]

On 2018-04-11 07:20 AM, yuankuiz@codeaurora.org wrote:
> ++
> On 2018-04-11 07:09 AM, yuankuiz@codeaurora.org wrote:
>> ++
>> 
>> On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote:
>>> Typo...
>>> 
>>> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote:
>>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
>>>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org>
>>>>>> > > > >
>>>>>> > > > > Variable tick_stopped returned by tick_nohz_tick_stopped
>>>>>> > > > > can have only true / false values. Since the return type
>>>>>> > > > > of the tick_nohz_tick_stopped is also bool, variable
>>>>>> > > > > tick_stopped nice to have data type as bool in place of unsigned int.
>>>>>> > > > > Moreover, the executed instructions cost could be minimal
>>>>>> > > > > without potiential data type conversion.
>>>>>> > > > >
>>>>>> > > > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
>>>>>> > > > > ---
>>>>>> > > > >  kernel/time/tick-sched.h | 2 +-
>>>>>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>> > > > >
>>>>>> > > > > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
>>>>>> > > > > index 6de959a..4d34309 100644
>>>>>> > > > > --- a/kernel/time/tick-sched.h
>>>>>> > > > > +++ b/kernel/time/tick-sched.h
>>>>>> > > > > @@ -48,8 +48,8 @@ struct tick_sched {
>>>>>> > > > >         unsigned long                   check_clocks;
>>>>>> > > > >         enum tick_nohz_mode             nohz_mode;
>>>>>> > > > >
>>>>>> > > > > +       bool                            tick_stopped    : 1;
>>>>>> > > > >         unsigned int                    inidle          : 1;
>>>>>> > > > > -       unsigned int                    tick_stopped    : 1;
>>>>>> > > > >         unsigned int                    idle_active     : 1;
>>>>>> > > > >         unsigned int                    do_timer_last   : 1;
>>>>>> > > > >         unsigned int                    got_idle_tick   : 1;
>>>>>> > > >
>>>>>> > > > I don't think this is a good idea at all.
>>>>>> > > >
>>>>>> > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
>>>>>> > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the case of
>>>>>> > > "Maybe".
>>>>>> >
>>>>>> > This patch falls into the case 'pointless' because it adds extra storage
>>>>>> [ZJ] 1 bit vs 1 bit. no more.
>>>>> 
>>>>> Groan. No. Care to look at the data structure? You create a new 
>>>>> storage,
>>>> [ZJ] Say, {unsigned int, unsigned int, unsigned int, unsigned int,
>>>> unsigned int} becomes
>>>>           {bool        , unsigned int, unsigned int, unsigned int, 
>>>> unsigned int}
>>>> As specified by the rule No.10 at the section 6.7.2.1 of C99 TC2 as:
>>>> "If enough space remains, a bit-field that immediately follows 
>>>> another
>>>> bit-field in a
>>>> structure shall be packed into adjacent bits of the same unit." What
>>>> is the new storage so far?
[ZJ] Further prototyping has been given based on gcc for both of x86_64 
and armv8-a,
      unsigned int and bool share the same 4 bytes without the addtional 
storage for sure.
      Open this and welcome if any other difference behaviour could be 
captured.
>>>> 
>>>>> which is incidentally merged into the other bitfield by the 
>>>>> compiler at a
>>>>> different bit position, but there is no guarantee that a compiler 
>>>>> does
>>>>> that. It's free to use distinct storage for that bool based bit.
>>>> [ZJ] Per the rule No.10 at section 6.7.2.1 of C99 TC2 as:
>>>> " If insufficient space remains, whether  a  bit-field  that  does
>>>> not  fit  is  put  into
>>>> the  next  unit  or overlaps  adjacent  units  is 
>>>> implementation-defined."
>>>> So, implementation is never mind which type will be stored if any.
>>>> 
>>>>> >> > for no benefit at all.
>>>>>> [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() 
>>>>>> which is bool.
>>>>>> The benefit is no any potiential type conversion could be minded.
>>>>> 
>>>>> A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has to 
>>>>> be
>>>>> evaluated as a bit. So there is a type conversion from BIT to bool 
>>>>> required
>>>>> because BIT != bool.
>>>> [ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
>>>> "If  the  value  0  or  1  is  stored  into  a  nonzero-width
>>>> bit-field  of  types
>>>> _Bool, the value of the bit-field shall compare equal to the value 
>>>> stored."
>>>> Obviously, it is nothing related to type conversion actually.
>>>>> 
>>>>> By chance the evaluation can be done by evaluating the byte in 
>>>>> which the
>>>>> bit is placed just because the compiler knows that the remaining 
>>>>> bits are
>>>>> not used. There is no guarantee that this is done, it happens to be 
>>>>> true
>>>>> for a particular compiler.
>>>> [ZJ] Actually, such as GCC owe that kind of guarantee to be promised 
>>>> by ABI.
>>>>> 
>>>>> But that does not make it any more interesting. It just makes the 
>>>>> code
>>>>> harder to read and eventually leads to bigger storage.
>>>> [ZJ] To get the benctifit to be profiled, it is given as:
>>>> number of instructions of function tick_nohz_tick_stopped():
>>> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an 
>>> evaluation() as:
>>> #include <stdio.h>
>>> #include <stdbool.h>
>>> 
>>> struct tick_sched {
>>>         unsigned int inidle             : 1;
>>>         unsigned int tick_stopped       : 1;
>>> };
>>> 
>>> bool get_status()
>>> {
>>>         struct tick_sched *ts;
>>>         ts->tick_stopped = 1;
>>>         return ts->tick_stopped;
>>> }
>>> 
>>> int main()
>>> {
>>>         if (get_status()) return 0;
>>>         return 0;
>>> }
>>> 
>>> [ZJ] Toggle the declaration of tick_stopped in side of the tick_sched
>>> structure for comparison.
>>> 
>>> 
>>>>                         original: 17
>>>>                         patched:  14
>>>>      Which was saved is:
>>>>                movzbl	%al, %eax
>>>>                testl	%eax, %eax
>>>>                setne    %al
>>>>      Say, 3 / 17 = 17 % could be gained in the instruction executed
>>>> for this function can be evaluated.
>>>> 
>>>> Note:
>>>>      The environment I used is:
>>>>                OS : Ubuntu Desktop 16.04 LTS
>>>>                gcc: 6.3.0                       (without 
>>>> optimization
>>>> for in general purpose)
>>>> 
>>>>> 
>>> 
>>> Just FYI.
>>> 
>>> Thanks,
>>> ZJ

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

[yuankuiz]

On 2018-04-20 09:47 AM, yuankuiz@codeaurora.org wrote:
> On 2018-04-11 07:20 AM, yuankuiz@codeaurora.org wrote:
>> ++
>> On 2018-04-11 07:09 AM, yuankuiz@codeaurora.org wrote:
>>> ++
>>> 
>>> On 2018-04-10 10:49 PM, yuankuiz@codeaurora.org wrote:
>>>> Typo...
>>>> 
>>>> On 2018-04-10 10:08 PM, yuankuiz@codeaurora.org wrote:
>>>>> On 2018-04-10 07:06 PM, Thomas Gleixner wrote:
>>>>>> On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>>>>> On 2018-04-10 05:10 PM, Thomas Gleixner wrote:
>>>>>>> > On Tue, 10 Apr 2018, yuankuiz@codeaurora.org wrote:
>>>>>>> > > On 2018-04-10 04:00 PM, Rafael J. Wysocki wrote:
>>>>>>> > > > On Tue, Apr 10, 2018 at 9:33 AM,  <yuankuiz@codeaurora.org> wrote:
>>>>>>> > > > > From: John Zhao <yuankuiz@codeaurora.org>
>>>>>>> > > > >
>>>>>>> > > > > Variable tick_stopped returned by tick_nohz_tick_stopped
>>>>>>> > > > > can have only true / false values. Since the return type
>>>>>>> > > > > of the tick_nohz_tick_stopped is also bool, variable
>>>>>>> > > > > tick_stopped nice to have data type as bool in place of unsigned int.
>>>>>>> > > > > Moreover, the executed instructions cost could be minimal
>>>>>>> > > > > without potiential data type conversion.
>>>>>>> > > > >
>>>>>>> > > > > Signed-off-by: John Zhao <yuankuiz@codeaurora.org>
>>>>>>> > > > > ---
>>>>>>> > > > >  kernel/time/tick-sched.h | 2 +-
>>>>>>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>> > > > >
>>>>>>> > > > > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
>>>>>>> > > > > index 6de959a..4d34309 100644
>>>>>>> > > > > --- a/kernel/time/tick-sched.h
>>>>>>> > > > > +++ b/kernel/time/tick-sched.h
>>>>>>> > > > > @@ -48,8 +48,8 @@ struct tick_sched {
>>>>>>> > > > >         unsigned long                   check_clocks;
>>>>>>> > > > >         enum tick_nohz_mode             nohz_mode;
>>>>>>> > > > >
>>>>>>> > > > > +       bool                            tick_stopped    : 1;
>>>>>>> > > > >         unsigned int                    inidle          : 1;
>>>>>>> > > > > -       unsigned int                    tick_stopped    : 1;
>>>>>>> > > > >         unsigned int                    idle_active     : 1;
>>>>>>> > > > >         unsigned int                    do_timer_last   : 1;
>>>>>>> > > > >         unsigned int                    got_idle_tick   : 1;
>>>>>>> > > >
>>>>>>> > > > I don't think this is a good idea at all.
>>>>>>> > > >
>>>>>>> > > > Please see https://lkml.org/lkml/2017/11/21/384 for example.
>>>>>>> > > [ZJ] Thanks for this sharing. Looks like, this patch fall into the case of
>>>>>>> > > "Maybe".
>>>>>>> >
>>>>>>> > This patch falls into the case 'pointless' because it adds extra storage
>>>>>>> [ZJ] 1 bit vs 1 bit. no more.
>>>>>> 
>>>>>> Groan. No. Care to look at the data structure? You create a new 
>>>>>> storage,
>>>>> [ZJ] Say, {unsigned int, unsigned int, unsigned int, unsigned int,
>>>>> unsigned int} becomes
>>>>>           {bool        , unsigned int, unsigned int, unsigned int, 
>>>>> unsigned int}
>>>>> As specified by the rule No.10 at the section 6.7.2.1 of C99 TC2 
>>>>> as:
>>>>> "If enough space remains, a bit-field that immediately follows 
>>>>> another
>>>>> bit-field in a
>>>>> structure shall be packed into adjacent bits of the same unit." 
>>>>> What
>>>>> is the new storage so far?
> [ZJ] Further prototyping has been given based on gcc for both of
> x86_64 and armv8-a,
>      unsigned int and bool share the same 1 bytes without the
> addtional storage for sure.
>      Open this and welcome if any other difference behaviour could be 
> captured.
[ZJ] Typo.. change 4 bytes above to 1 byte actually.
>>>>> 
>>>>>> which is incidentally merged into the other bitfield by the 
>>>>>> compiler at a
>>>>>> different bit position, but there is no guarantee that a compiler 
>>>>>> does
>>>>>> that. It's free to use distinct storage for that bool based bit.
>>>>> [ZJ] Per the rule No.10 at section 6.7.2.1 of C99 TC2 as:
>>>>> " If insufficient space remains, whether  a  bit-field  that  does
>>>>> not  fit  is  put  into
>>>>> the  next  unit  or overlaps  adjacent  units  is 
>>>>> implementation-defined."
>>>>> So, implementation is never mind which type will be stored if any.
>>>>> 
>>>>>> >> > for no benefit at all.
>>>>>>> [ZJ] tick_stopped is returned by the tick_nohz_tick_stopped() 
>>>>>>> which is bool.
>>>>>>> The benefit is no any potiential type conversion could be minded.
>>>>>> 
>>>>>> A bit stays a bit. 'bool foo : 1;' or 'unsigned int foo : 1' has 
>>>>>> to be
>>>>>> evaluated as a bit. So there is a type conversion from BIT to bool 
>>>>>> required
>>>>>> because BIT != bool.
>>>>> [ZJ] Per the rule No.9 at section 6.7.2.1 of C99 TC2 as:
>>>>> "If  the  value  0  or  1  is  stored  into  a  nonzero-width
>>>>> bit-field  of  types
>>>>> _Bool, the value of the bit-field shall compare equal to the value 
>>>>> stored."
>>>>> Obviously, it is nothing related to type conversion actually.
>>>>>> 
>>>>>> By chance the evaluation can be done by evaluating the byte in 
>>>>>> which the
>>>>>> bit is placed just because the compiler knows that the remaining 
>>>>>> bits are
>>>>>> not used. There is no guarantee that this is done, it happens to 
>>>>>> be true
>>>>>> for a particular compiler.
>>>>> [ZJ] Actually, such as GCC owe that kind of guarantee to be 
>>>>> promised by ABI.
[ZJ] "-mone-byte-bool" could be used by alpha-linux-gcc to override the 
default bool size
      to become 1 byte for even Darwin / powerPC from it's manual.
>>>>>> 
>>>>>> But that does not make it any more interesting. It just makes the 
>>>>>> code
>>>>>> harder to read and eventually leads to bigger storage.
>>>>> [ZJ] To get the benctifit to be profiled, it is given as:
>>>>> number of instructions of function tick_nohz_tick_stopped():
>>>> [ZJ] Here, I used is not the tick_nohz_tick_stopped(), but an 
>>>> evaluation() as:
>>>> #include <stdio.h>
>>>> #include <stdbool.h>
>>>> 
>>>> struct tick_sched {
>>>>         unsigned int inidle             : 1;
>>>>         unsigned int tick_stopped       : 1;
>>>> };
>>>> 
>>>> bool get_status()
>>>> {
>>>>         struct tick_sched *ts;
>>>>         ts->tick_stopped = 1;
>>>>         return ts->tick_stopped;
>>>> }
>>>> 
>>>> int main()
>>>> {
>>>>         if (get_status()) return 0;
>>>>         return 0;
>>>> }
>>>> 
>>>> [ZJ] Toggle the declaration of tick_stopped in side of the 
>>>> tick_sched
>>>> structure for comparison.
>>>> 
>>>> 
>>>>>                         original: 17
>>>>>                         patched:  14
>>>>>      Which was saved is:
>>>>>                movzbl	%al, %eax
>>>>>                testl	%eax, %eax
>>>>>                setne    %al
>>>>>      Say, 3 / 17 = 17 % could be gained in the instruction executed
>>>>> for this function can be evaluated.
>>>>> 
>>>>> Note:
>>>>>      The environment I used is:
>>>>>                OS : Ubuntu Desktop 16.04 LTS
>>>>>                gcc: 6.3.0                       (without 
>>>>> optimization
>>>>> for in general purpose)
>>>>> 
>>>>>> 
>>>> 
>>>> Just FYI.
>>>> 
>>>> Thanks,
>>>> ZJ

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

[Joe Perches]

On Fri, 2018-04-20 at 14:44 +0800, yuankuiz@codeaurora.org wrote:
> On 2018-04-20 09:47 AM, yuankuiz@codeaurora.org wrote:
[]
> > [ZJ] Further prototyping has been given based on gcc for both of
> > x86_64 and armv8-a,
> >      unsigned int and bool share the same 1 bytes without the
> > addtional storage for sure.
> >      Open this and welcome if any other difference behaviour could be 
> > captured.
> 
> [ZJ] Typo.. change 4 bytes above to 1 byte actually.

Not really.

unsigned int is 4 and bool is generally 1.
Alignment padding after a bool may exist.

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

[yuankuiz]

On 2018-04-21 03:24 AM, Joe Perches wrote:
> On Fri, 2018-04-20 at 14:44 +0800, yuankuiz@codeaurora.org wrote:
>> On 2018-04-20 09:47 AM, yuankuiz@codeaurora.org wrote:
> []
>> > [ZJ] Further prototyping has been given based on gcc for both of
>> > x86_64 and armv8-a,
>> >      unsigned int and bool share the same 1 bytes without the
>> > addtional storage for sure.
>> >      Open this and welcome if any other difference behaviour could be
>> > captured.
>> 
>> [ZJ] Typo.. change 4 bytes above to 1 byte actually.
> 
> Not really.
> 
> unsigned int is 4 and bool is generally 1.
> Alignment padding after a bool may exist.
[ZJ] Depending on how to pack, the size was padded is variance. For 
example.
      In case of the "unsigned char" at the following, pack is happened 
and result 1 bytes.(if no more than 8 bits are used)
      In case of the "int" at the following, pack is happened but result 
4 bytes.
      I mean, I demo it but use the 1# case due for another thread 
discussion on the ichx_desc() so move a little bit from the tick_sched 
struct.