2012-10-03 06:49:10

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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.


2012-10-03 07:00:12

by Josh Triplett

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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

2012-10-03 07:53:13

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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.

2012-10-03 11:12:19

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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 <[email protected]>
> 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

2012-10-03 14:01:57

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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

2012-10-03 14:46:44

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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

2012-10-03 15:14:12

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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

2012-10-03 15:24:00

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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.

2012-10-03 15:38:20

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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.

2012-10-04 00:32:44

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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

2012-10-04 00:55:04

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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

2012-10-04 00:55:17

by Michel Lespinasse

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

On Wed, Oct 3, 2012 at 7:46 AM, Daniel Santos <[email protected]> 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).

--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

2012-10-04 02:33:13

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 7/10] compiler{,-gcc4}.h: Introduce __flatten function attribute

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