| Message ID | 1348874411-28288-8-git-send-email-daniel.santos@pobox.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Sep 28, 2012 at 06:20:08PM -0500, Daniel Santos wrote: > --- a/include/linux/compiler-gcc4.h > +++ b/include/linux/compiler-gcc4.h > @@ -15,7 +15,12 @@ > > #if GCC_VERSION >= 40102 > # define __compiletime_object_size(obj) __builtin_object_size(obj, 0) > -#endif > + > +/* flatten introduced in 4.1, but broken in 4.6.0 (gcc bug #48731)*/ > +# if GCC_VERSION != 40600 > +# define __flatten __attribute__((flatten)) > +# endif > +#endif /* GCC_VERSION >= 40102 */ Same comment as before: why 40102 rather than 40100? - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/28/2012 07:26 PM, Josh Triplett wrote: > On Fri, Sep 28, 2012 at 06:20:08PM -0500, Daniel Santos wrote: >> --- a/include/linux/compiler-gcc4.h >> +++ b/include/linux/compiler-gcc4.h >> @@ -15,7 +15,12 @@ >> >> #if GCC_VERSION >= 40102 >> # define __compiletime_object_size(obj) __builtin_object_size(obj, 0) >> -#endif >> + >> +/* flatten introduced in 4.1, but broken in 4.6.0 (gcc bug #48731)*/ >> +# if GCC_VERSION != 40600 >> +# define __flatten __attribute__((flatten)) >> +# endif >> +#endif /* GCC_VERSION >= 40102 */ > Same comment as before: why 40102 rather than 40100? Again, I'm presuming building with 4.1.0 or 4.1.1 will always have the build broken earlier. I don't have any objections to changing this, but it would seem that the issue with the __weak attribute needs to be resolved. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 28, 2012 at 07:38:32PM -0500, Daniel Santos wrote: > On 09/28/2012 07:26 PM, Josh Triplett wrote: > > On Fri, Sep 28, 2012 at 06:20:08PM -0500, Daniel Santos wrote: > >> --- a/include/linux/compiler-gcc4.h > >> +++ b/include/linux/compiler-gcc4.h > >> @@ -15,7 +15,12 @@ > >> > >> #if GCC_VERSION >= 40102 > >> # define __compiletime_object_size(obj) __builtin_object_size(obj, 0) > >> -#endif > >> + > >> +/* flatten introduced in 4.1, but broken in 4.6.0 (gcc bug #48731)*/ > >> +# if GCC_VERSION != 40600 > >> +# define __flatten __attribute__((flatten)) > >> +# endif > >> +#endif /* GCC_VERSION >= 40102 */ > > Same comment as before: why 40102 rather than 40100? > Again, I'm presuming building with 4.1.0 or 4.1.1 will always have the > build broken earlier. I don't have any objections to changing this, but > it would seem that the issue with the __weak attribute needs to be resolved. That issue doesn't relate to __flatten, though; it only relates to __weak. Since __flatten (and __compiletime_object_size) will work fine on 4.1.0 and 4.1.1, don't exclude them just because the definition for __weak elsewhere in the file excludes them. That just makes it harder for anyone who might want to work on the issue with __weak. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 28 Sep 2012, Josh Triplett wrote: > That issue doesn't relate to __flatten, though; it only relates to > __weak. Since __flatten (and __compiletime_object_size) will work fine > on 4.1.0 and 4.1.1, don't exclude them just because the definition for > __weak elsewhere in the file excludes them. That just makes it harder > for anyone who might want to work on the issue with __weak. > Nack to the patch since there are no users of it; there's no need to define every possible gcc function attribute. If anything actually needs to use __attribute__((flatten)), then it can introduce it. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 02, 2012 at 11:49:03PM -0700, David Rientjes wrote: > On Fri, 28 Sep 2012, Josh Triplett wrote: > > > That issue doesn't relate to __flatten, though; it only relates to > > __weak. Since __flatten (and __compiletime_object_size) will work fine > > on 4.1.0 and 4.1.1, don't exclude them just because the definition for > > __weak elsewhere in the file excludes them. That just makes it harder > > for anyone who might want to work on the issue with __weak. > > > > Nack to the patch since there are no users of it; there's no need to > define every possible gcc function attribute. If anything actually needs > to use __attribute__((flatten)), then it can introduce it. This patch series started out as part of another patch series by Daniel Santos that makes use of __flatten; I think Daniel plans to have that patch series depend on this one. Thus, I think it makes sense to introduce __flatten at this point. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2 Oct 2012, Josh Triplett wrote: > This patch series started out as part of another patch series by Daniel > Santos that makes use of __flatten; I think Daniel plans to have that > patch series depend on this one. Thus, I think it makes sense to > introduce __flatten at this point. > Daniel, please introduce __flatten in the patch series that uses it, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/2012 02:53 AM, David Rientjes wrote: > On Tue, 2 Oct 2012, Josh Triplett wrote: > >> This patch series started out as part of another patch series by Daniel >> Santos that makes use of __flatten; I think Daniel plans to have that >> patch series depend on this one. Thus, I think it makes sense to >> introduce __flatten at this point. >> > > Daniel, please introduce __flatten in the patch series that uses it, > thanks. That isn't going to work. I split my patches out into three sets because, otherwise, the list of maintainers that must be CCed exceeds the allowable size of the LKML server and my messages get tagged as spam, causing untold confusion as messages reach maintainers, but not the LKML. Please note from the summary of this patch set (https://lkml.org/lkml/2012/9/28/1136) > This patch set is a dependency of the generic red-black tree patch set, which > I have now split up into three smaller sets. And the patch set this depends upon was submitted 9/28 as well (https://lkml.org/lkml/2012/9/28/1183) and the summary starts with this text: > This patch set depends upon the following: > * Cleanup & new features for compiler*.h and bug.h > * kernel-doc bug fixes (for generating docs) If I move this patch to the other patch set, scripts/get_maintainers.pl will give me a list longer than the LKML administrator will allow for recipients (1024 bytes max) On 09/28/2012 12:46 PM, David Miller wrote: > From: Daniel Santos <danielfsantos@att.net> > Date: Fri, 28 Sep 2012 09:22:52 -0500 > >> Hello. I'm trying to send a patch set and people in my TO list are >> receiving the emails, but not the LKML. > You can't have email fields larger than 1024 characters, that CC: > list is way too large. There is never any legitimate reason to > CC: so many people, and we block such long email fields since > %99.99999 they indicate spam. If you have any other reasonable suggestions, please post them. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-03 at 06:20 -0500, Daniel Santos wrote: > > Daniel, please introduce __flatten in the patch series that uses it, > > thanks. > That isn't going to work. I split my patches out into three sets > because, otherwise, the list of maintainers that must be CCed exceeds > the allowable size of the LKML server and my messages get tagged as > spam, causing untold confusion as messages reach maintainers, but not > the LKML. Please note from the summary of this patch set > (https://lkml.org/lkml/2012/9/28/1136) That's not a valid reason to not included it with the other patch series. > > This patch set is a dependency of the generic red-black tree patch set, which > > I have now split up into three smaller sets. > And the patch set this depends upon was submitted 9/28 as well > (https://lkml.org/lkml/2012/9/28/1183) and the summary starts with this > text: > > This patch set depends upon the following: > > * Cleanup & new features for compiler*.h and bug.h > > * kernel-doc bug fixes (for generating docs) > If I move this patch to the other patch set, > scripts/get_maintainers.pl will give me a list longer than the LKML > administrator will allow for recipients (1024 bytes max) You don't need to use get_maintainers. It's more of a help tool to find maintainers and not something that is mandatory. Not everyone that has ever touched one of these files needs to be Cc'd. Please move the patch to the patch series where it is used. Otherwise it confuses reviewers as it did here. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/2012 09:01 AM, Steven Rostedt wrote: > On Wed, 2012-10-03 at 06:20 -0500, Daniel Santos wrote: > >>> Daniel, please introduce __flatten in the patch series that uses it, >>> thanks. >> That isn't going to work. I split my patches out into three sets >> because, otherwise, the list of maintainers that must be CCed exceeds >> the allowable size of the LKML server and my messages get tagged as >> spam, causing untold confusion as messages reach maintainers, but not >> the LKML. Please note from the summary of this patch set >> (https://lkml.org/lkml/2012/9/28/1136) > That's not a valid reason to not included it with the other patch > series. > >>> This patch set is a dependency of the generic red-black tree patch set, which >>> I have now split up into three smaller sets. >> And the patch set this depends upon was submitted 9/28 as well >> (https://lkml.org/lkml/2012/9/28/1183) and the summary starts with this >> text: >>> This patch set depends upon the following: >>> * Cleanup & new features for compiler*.h and bug.h >>> * kernel-doc bug fixes (for generating docs) >> If I move this patch to the other patch set, >> scripts/get_maintainers.pl will give me a list longer than the LKML >> administrator will allow for recipients (1024 bytes max) > You don't need to use get_maintainers. It's more of a help tool to find > maintainers and not something that is mandatory. Not everyone that has > ever touched one of these files needs to be Cc'd. > > Please move the patch to the patch series where it is used. Otherwise it > confuses reviewers as it did here. Ok then, but this would also apply to the addition of these macros as well: BUILD_BUG_ON_NON_CONST BUILD_BUG42 BUILD_BUG_ON_NON_CONST42 Should these then also be moved? Should I only CC those who have responded to these patches and whomever is in the MAINTAINERS file then? Thanks Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-03 at 09:46 -0500, Daniel Santos wrote: > > Please move the patch to the patch series where it is used. Otherwise it > > confuses reviewers as it did here. > Ok then, but this would also apply to the addition of these macros as well: > BUILD_BUG_ON_NON_CONST > BUILD_BUG42 > BUILD_BUG_ON_NON_CONST42 > > Should these then also be moved? If they are only used by the other patch set, sure. > Should I only CC those who have responded to these patches and whomever > is in the MAINTAINERS file then? Yep. I personally never use the get_maintainers script. I first check the MAINTAINERS file. If the subsystem I'm working on exists there, I only email those that are listed there, including any mailing lists that are mentioned (as well as LKML). If it's not listed, I then do a git log and see who does the most sign offs to changes there, and to what kind of changes. I usually ignore the trivial stuff. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-03 at 11:14 -0400, Steven Rostedt wrote: > > Yep. I personally never use the get_maintainers script. I first check > the MAINTAINERS file. If the subsystem I'm working on exists there, I > only email those that are listed there, including any mailing lists that > are mentioned (as well as LKML). If it's not listed, I then do a git log > and see who does the most sign offs to changes there, and to what kind > of changes. I usually ignore the trivial stuff. I also tend to suggest doing git-blame to see who touched the code being changed last. As a maintainer I frequently get to fwd/bounce patches because of missing CCs like that. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-03 at 11:14 -0400, Steven Rostedt wrote: > I first check > the MAINTAINERS file. If the subsystem I'm working on exists there, I > only email those that are listed there, including any mailing lists that > are mentioned (as well as LKML). If it's not listed, I then do a git log > and see who does the most sign offs to changes there, and to what kind > of changes. I usually ignore the trivial stuff. Funny because that's what the script does too. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-10-03 at 08:38 -0700, Joe Perches wrote: > On Wed, 2012-10-03 at 11:14 -0400, Steven Rostedt wrote: > > I first check > > the MAINTAINERS file. If the subsystem I'm working on exists there, I > > only email those that are listed there, including any mailing lists that > > are mentioned (as well as LKML). If it's not listed, I then do a git log > > and see who does the most sign offs to changes there, and to what kind > > of changes. I usually ignore the trivial stuff. > > Funny because that's what the script does too. > Really? It ignores the trivial stuff and only adds people that seem to actually do real work on the file? If that's the case, I doubt that it would have caused the huge Cc list that Daniel sent out. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/2012 07:32 PM, Steven Rostedt wrote: > On Wed, 2012-10-03 at 08:38 -0700, Joe Perches wrote: >> On Wed, 2012-10-03 at 11:14 -0400, Steven Rostedt wrote: >>> I first check >>> the MAINTAINERS file. If the subsystem I'm working on exists there, I >>> only email those that are listed there, including any mailing lists that >>> are mentioned (as well as LKML). If it's not listed, I then do a git log >>> and see who does the most sign offs to changes there, and to what kind >>> of changes. I usually ignore the trivial stuff. >> >> Funny because that's what the script does too. >> > > Really? It ignores the trivial stuff and only adds people that seem to > actually do real work on the file? > > If that's the case, I doubt that it would have caused the huge Cc list > that Daniel sent out. > > -- Steve Well, you run that on: * include/linux/bug.h, * include/linux/compiler{,-gcc{,3,4}}.h, * include/linux/rbtree.h, * tools/testing/selftests, * Documentation/DocBook/kernel-api.tmpl and * scripts/kernel-doc, and it really starts to add up (that's currently 23 on mmotm, not including myself). Now, add people already involved in the patch set, etc., and it makes its way to 30. That's why I split the patch set up. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 7:46 AM, Daniel Santos <danielfsantos@att.net> wrote: > On 10/03/2012 09:01 AM, Steven Rostedt wrote: >> You don't need to use get_maintainers. It's more of a help tool to find >> maintainers and not something that is mandatory. Not everyone that has >> ever touched one of these files needs to be Cc'd. >> >> Please move the patch to the patch series where it is used. Otherwise it >> confuses reviewers as it did here. > Ok then, but this would also apply to the addition of these macros as well: > BUILD_BUG_ON_NON_CONST > BUILD_BUG42 > BUILD_BUG_ON_NON_CONST42 > > Should these then also be moved? Yes, this would actually make things easier. > Should I only CC those who have responded to these patches and whomever > is in the MAINTAINERS file then? There is no strong rule here, but generally get_maintainers returns too many people. You want to trim down the list to something shorter; a dozen people is the most I would consider (but for most patches a half-dozen is already plenty).
On Wed, 2012-10-03 at 20:32 -0400, Steven Rostedt wrote: > On Wed, 2012-10-03 at 08:38 -0700, Joe Perches wrote: > > On Wed, 2012-10-03 at 11:14 -0400, Steven Rostedt wrote: > > > I first check > > > the MAINTAINERS file. If the subsystem I'm working on exists there, I > > > only email those that are listed there, including any mailing lists that > > > are mentioned (as well as LKML). If it's not listed, I then do a git log > > > and see who does the most sign offs to changes there, and to what kind > > > of changes. I usually ignore the trivial stuff. > > > > Funny because that's what the script does too. > > > > Really? It ignores the trivial stuff and only adds people that seem to > actually do real work on the file? Fundamentally, yes. It's not as good as even a semi-skilled person of course. > If that's the case, I doubt that it would have caused the huge Cc list > that Daniel sent out. Multiple files per patch, large recipient lists. I generally use --no-git and --nogit-fallback -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h index ad610f2..5a0897e 100644 --- a/include/linux/compiler-gcc4.h +++ b/include/linux/compiler-gcc4.h @@ -15,7 +15,12 @@ #if GCC_VERSION >= 40102 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0) -#endif + +/* flatten introduced in 4.1, but broken in 4.6.0 (gcc bug #48731)*/ +# if GCC_VERSION != 40600 +# define __flatten __attribute__((flatten)) +# endif +#endif /* GCC_VERSION >= 40102 */ #if GCC_VERSION >= 40300 /* Mark functions as cold. gcc will assume any path leading to a call diff --git a/include/linux/compiler.h b/include/linux/compiler.h index fd455aa..268aeb6 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -244,6 +244,10 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect); #define __always_inline inline #endif +#ifndef __flatten +#define __flatten +#endif + #endif /* __KERNEL__ */ /*
For gcc 4.1 & later, expands to __attribute__((flatten)) which forces the compiler to inline everything it can into the function. This is useful in combination with noinline when you want to control the depth of inlining, or create a single function where inline expansions will occur. (see http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bflatten_007d-function-attribute-2512) Normally, it's best to leave this type of thing up to the compiler. However, the generic rbtree code uses inline functions just to be able to inject compile-time constant data that specifies how the caller wants the function to behave (via struct rb_relationship). This data can be thought of as the template parameters of a C++ templatized function. Since some of these functions, once expanded, become quite large, gcc sometimes decides not to perform some important inlining, in one case, even generating a few bytes more code by not doing so. (Note: I have not eliminated the possibility that this was an optimization bug, but the flatten attribute fixes it in either case.) Combining __flatten and noinline insures that important optimizations occur in these cases and that the inline expansion occurs in exactly one place, thus not leading to unnecissary bloat. However, it also can eliminate some opportunities for optimization should gcc otherwise decide the function its self is a good candidate for inlining. Signed-off-by: Daniel Santos <daniel.santos@pobox.com> --- include/linux/compiler-gcc4.h | 7 ++++++- include/linux/compiler.h | 4 ++++ 2 files changed, 10 insertions(+), 1 deletions(-)