| Message ID | 95477c93db187bab6da8a8ba7c57836868446179.camel@perches.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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",
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.
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.
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.
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(?).
(Adding Julia Lawall) On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > We already have some 500 bools-in-structs I got at least triple that only in include/ so I expect there are at probably an order of magnitude more than 500 in the kernel. I suppose some cocci script could count the actual number of instances. A regex can not. > and the owners of that code will > be wondering whether they should change them, and whether they should > apply those remove-bool-in-struct patches which someone sent them. Which is why the warning is --strict only > So... can we please get some clarity here? > ... > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) > > hm, Linus suggests that instead of using > > bool mybool; > > we should use > > unsigned mybool:1; > > However that introduces the risk that alterations of mybool will use > nonatomic rmw operations. > > unsigned myboolA:1; > unsigned myboolB:1; > > so > > foo->myboolA = 1; > > could scribble on concurrent alterations of foo->myboolB. I think. Without barriers, that could happen anyway. To me, the biggest problem with conversions from bool to bitfield is logical. ie: unsigned int.singlebitfield = 4; is not the same result as bool = 4; because of implicit truncation vs boolean conversion so a direct change of bool use in structs to unsigned would also require logic analysis.
On Wed, Apr 11, 2018 at 09:29:59AM -0700, Andrew Morton wrote: > > OK. I guess. But I'm not really seeing some snappy description which > helps people understand why checkpatch is warning about this. "Results in architecture dependent layout." is the best short sentence I can come up with. > We already have some 500 bools-in-structs and the owners of that code > will be wondering whether they should change them, and whether they > should apply those remove-bool-in-struct patches which someone sent > them. I still have room in my /dev/null mailbox for pure checkpatch patches. > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) Yes, we really should not use lkml.org for references. Sadly google displays it very prominently when you search for something. > hm, Linus suggests that instead of using > > bool mybool; > > we should use > > unsigned mybool:1; > > However that introduces the risk that alterations of mybool will use > nonatomic rmw operations. > > unsigned myboolA:1; > unsigned myboolB:1; > > so > > foo->myboolA = 1; > > could scribble on concurrent alterations of foo->myboolB. I think. So that is true of u8 on Alpha <EV56 too. If you want concurrent, you had better know what you're doing. > I guess that risk is also present if myboolA and myboolB were `bool', > too. The compiler could do any old thing with them including, perhaps, > using a single-bit bitfield(?). The smallest addressable type in C is a byte, so while _Bool may be larger than a byte, it cannot be smaller. Otherwise we could not write: _Bool var; _Boll *ptr = &var; Which is something that comes apart with bitfields.
On Wed, 11 Apr 2018, Joe Perches wrote: > (Adding Julia Lawall) > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > > We already have some 500 bools-in-structs > > I got at least triple that only in include/ > so I expect there are at probably an order > of magnitude more than 500 in the kernel. > > I suppose some cocci script could count the > actual number of instances. A regex can not. I got 12667. I'm not sure to understand the issue. Will using a bitfield help if there are no other bitfields in the structure? julia > > > and the owners of that code will > > be wondering whether they should change them, and whether they should > > apply those remove-bool-in-struct patches which someone sent them. > > Which is why the warning is --strict only > > > So... can we please get some clarity here? > > > > ... > > > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) > > > > hm, Linus suggests that instead of using > > > > bool mybool; > > > > we should use > > > > unsigned mybool:1; > > > > However that introduces the risk that alterations of mybool will use > > nonatomic rmw operations. > > > > unsigned myboolA:1; > > unsigned myboolB:1; > > > > so > > > > foo->myboolA = 1; > > > > could scribble on concurrent alterations of foo->myboolB. I think. > > Without barriers, that could happen anyway. > > To me, the biggest problem with conversions > from bool to bitfield is logical. ie: > > unsigned int.singlebitfield = 4; > > is not the same result as > > bool = 4; > > because of implicit truncation vs boolean conversion > so a direct change of bool use in structs to unsigned > would also require logic analysis. > >
On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote: > On Wed, 11 Apr 2018, Joe Perches wrote: > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > > > We already have some 500 bools-in-structs > > > > I got at least triple that only in include/ > > so I expect there are at probably an order > > of magnitude more than 500 in the kernel. > > > > I suppose some cocci script could count the > > actual number of instances. A regex can not. > > I got 12667. Could you please post the cocci script? > I'm not sure to understand the issue. Will using a bitfield help if there > are no other bitfields in the structure? IMO, not really. The primary issue is described by Linus here: https://lkml.org/lkml/2017/11/21/384 I personally do not find a significant issue with uncontrolled sizes of bool in kernel structs as all of the kernel structs are transitory and not written out to storage. I suppose bool bitfields are also OK, but for the RMW required. Using unsigned int :1 bitfield instead of bool :1 has the negative of truncation so that the uint has to be set with !! instead of a simple assign.
On Wed, 11 Apr 2018, Joe Perches wrote: > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote: > > On Wed, 11 Apr 2018, Joe Perches wrote: > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > > > > We already have some 500 bools-in-structs > > > > > > I got at least triple that only in include/ > > > so I expect there are at probably an order > > > of magnitude more than 500 in the kernel. > > > > > > I suppose some cocci script could count the > > > actual number of instances. A regex can not. > > > > I got 12667. > > Could you please post the cocci script? Sure. julia Command line: spatch.opt boolinstruct.cocci -j 40 --very-quiet --no-includes --include-headers /run/shm/linux-next --use-idutils This was tested on: struct foo { bool a; bool b,c; int r; }; struct { bool a; bool b,c; int r; } x; ---------------------- @initialize:ocaml@ @@ let ctr = ref 0 @r@ identifier i,x; position p; @@ struct i { ... bool x@p; ... } @script:ocaml@ _p << r.p; @@ ctr := !ctr + 1 @s@ identifier x; position p; @@ struct { ... bool x@p; ... } @script:ocaml@ _p << s.p; @@ ctr := !ctr + 1 @finalize:ocaml@ ctrs << merge.ctr; @@ ctr := 0; List.iter (function c -> ctr := !c + !ctr) ctrs; Printf.printf "%d\n" !ctr
* Peter Zijlstra <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
On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote:
> lkml.org is nice in emails that have a short expected life time and relevance -
I like lkml.org's archive (although it's not without its problems), but
the site suffers from serious availability issues -- it is down a lot,
which is _really_ tedious.
On Wed, Apr 11, 2018 at 11:42:20PM -0700, Joe Perches wrote: > I personally do not find a significant issue with > uncontrolled sizes of bool in kernel structs as > all of the kernel structs are transitory and not > written out to storage. People that care about cache locality, false sharing and other such things really care about structure layout. Growing a structure into another cacheline can be a significant performance hit -- cache misses hurt.
Hi, On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote: > > * Peter Zijlstra <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
On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > Is there a better or recommended way to reference posts on LKML in commit > messages? (I do like the idea of linking to previous discussions, results, > ...) Yes: https://lkml.kernel.org/r/$MSGID that has the added benefit that it immediately includes the msg-id, so even if you don't have tubes, you can search for it in your local mailboxes. Also, since we (kernel.org) control the redirection (currently marc.info) we can always point it to a life archive.
Andrea Parri <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.
On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote: > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > > Is there a better or recommended way to reference posts on LKML in commit > > messages? (I do like the idea of linking to previous discussions, results, > > ...) > > Yes: > > https://lkml.kernel.org/r/$MSGID > > that has the added benefit that it immediately includes the msg-id, so > even if you don't have tubes, you can search for it in your local > mailboxes. > > Also, since we (kernel.org) control the redirection (currently > marc.info) we can always point it to a life archive. Message IDs are not useful unless you subscribe and keep your emails. It'd be _much_ nicer if vger.kernel.org stored every email it sent and had a search mechanism available rather than relying on external systems.
On Thu, Apr 12, 2018 at 05:01:37AM -0700, Joe Perches wrote: > On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote: > > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > > > Is there a better or recommended way to reference posts on LKML in commit > > > messages? (I do like the idea of linking to previous discussions, results, > > > ...) > > > > Yes: > > > > https://lkml.kernel.org/r/$MSGID > > > > that has the added benefit that it immediately includes the msg-id, so > > even if you don't have tubes, you can search for it in your local > > mailboxes. > > > > Also, since we (kernel.org) control the redirection (currently > > marc.info) we can always point it to a life archive. > > Message IDs are not useful unless you subscribe and > keep your emails. I happen to do so.. > It'd be _much_ nicer if vger.kernel.org stored every email > it sent and had a search mechanism available rather than > relying on external systems. People are looking at that afaik.
On Thu, 2018-04-12 at 14:08 +0200, Peter Zijlstra wrote: > On Thu, Apr 12, 2018 at 05:01:37AM -0700, Joe Perches wrote: > > On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote: > > > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > > > > Is there a better or recommended way to reference posts on LKML in commit > > > > messages? (I do like the idea of linking to previous discussions, results, > > > > ...) > > > > > > Yes: > > > > > > https://lkml.kernel.org/r/$MSGID > > > > > > that has the added benefit that it immediately includes the msg-id, so > > > even if you don't have tubes, you can search for it in your local > > > mailboxes. > > > > > > Also, since we (kernel.org) control the redirection (currently > > > marc.info) we can always point it to a life archive. > > > > Message IDs are not useful unless you subscribe and > > keep your emails. > > I happen to do so.. As do I for mailing list threads I reply to, but keeping all email threads locally is not not practicable on limited storage systems. > > It'd be _much_ nicer if vger.kernel.org stored every email > > it sent and had a search mechanism available rather than > > relying on external systems. > > People are looking at that afaik. Looking and doing are unfortunately different. Back when gmane died a couple years ago, I made the same suggestion to 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.
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...
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
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",
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(+)