[GIT PULL linux-next] Add Compiler Attributes tree

[GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Stephen,

The Compiler Attributes series has been stable for 10+ days. To
increase testing before 4.20, I would to request it being picked up
for -next.

The changes w.r.t. v5 in the LKML:

  - Rebased on top of next-20180928, which required removing
aligned_largest, which was removed by 9503cd9cbaba
("include/linux/compiler*.h: add version detection to
asm_volatile_goto").
  - Added latest Reviewed-by's and Tested-by's.

Thanks!

Cheers,
Miguel

The following changes since commit 4794a36bf08dfa89fe636e5080db9d8350e255dd:

  Add linux-next specific files for 20180928 (2018-09-28 15:26:51 +1000)

are available in the Git repository at:

  https://github.com/ojeda/linux.git compiler-attributes

for you to fetch changes up to dbce062c0b519db1cdad8d87ab46851f0be6bdea:

  Compiler Attributes: ext4: remove local __nonstring definition
(2018-10-02 15:11:26 +0200)

----------------------------------------------------------------
Miguel Ojeda (15):
      Compiler Attributes: remove unused attributes
      Compiler Attributes: always use the extra-underscores syntax
      Compiler Attributes: remove unneeded tests
      Compiler Attributes: homogenize __must_be_array
      Compiler Attributes: remove unneeded sparse (__CHECKER__) tests
      Compiler Attributes: add missing SPDX ID in compiler_types.h
      Compiler Attributes: use feature checks instead of version checks
      Compiler Attributes: KENTRY used twice the "used" attribute
      Compiler Attributes: remove uses of __attribute__ from compiler.h
      Compiler Attributes: add Doc/process/programming-language.rst
      Compiler Attributes: add MAINTAINERS entry
      Compiler Attributes: add support for __nonstring (gcc >= 8)
      Compiler Attributes: enable -Wstringop-truncation on W=1 (gcc >= 8)
      Compiler Attributes: auxdisplay: panel: use __nonstring
      Compiler Attributes: ext4: remove local __nonstring definition

 Documentation/process/index.rst                |   1 +
 Documentation/process/programming-language.rst |  45 +++++
 MAINTAINERS                                    |   5 +
 drivers/auxdisplay/panel.c                     |   7 +-
 fs/ext4/ext4.h                                 |   9 -
 include/linux/compiler-clang.h                 |   5 -
 include/linux/compiler-gcc.h                   |  70 +------
 include/linux/compiler-intel.h                 |   9 -
 include/linux/compiler.h                       |  19 +-
 include/linux/compiler_attributes.h            | 257 +++++++++++++++++++++++++
 include/linux/compiler_types.h                 | 100 ++--------
 scripts/Makefile.extrawarn                     |   1 +
 12 files changed, 340 insertions(+), 188 deletions(-)
 create mode 100644 Documentation/process/programming-language.rst
 create mode 100644 include/linux/compiler_attributes.h

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

Hi Miguel,

On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> The Compiler Attributes series has been stable for 10+ days. To
> increase testing before 4.20, I would to request it being picked up
> for -next.
> 
> The changes w.r.t. v5 in the LKML:
> 
>   - Rebased on top of next-20180928, which required removing

Unfortunately, trees/branches included in linux-next must be based on
something stable (usually Linus' tree, but it could be another
tree/branch that is included in linux-next that does not rebase).
Linux-next itself rebases every day, so snything based on it would drag
in a previous version of all the other trees :-(

> aligned_largest, which was removed by 9503cd9cbaba
> ("include/linux/compiler*.h: add version detection to
> asm_volatile_goto").

That commit is from Andrew's patch series which also rebases (usually
at least every week), so you cannot depend on it.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Nick Desaulniers]

On Tue, Oct 2, 2018 at 2:11 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Miguel,
>
> On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > The Compiler Attributes series has been stable for 10+ days. To
> > increase testing before 4.20, I would to request it being picked up
> > for -next.
> >
> > The changes w.r.t. v5 in the LKML:
> >
> >   - Rebased on top of next-20180928, which required removing
>
> Unfortunately, trees/branches included in linux-next must be based on
> something stable (usually Linus' tree, but it could be another
> tree/branch that is included in linux-next that does not rebase).
> Linux-next itself rebases every day, so snything based on it would drag
> in a previous version of all the other trees :-(

I think of this like a branch that's force pushed to.  Can't base
other branches or trees off of it cause it's always moving/force
rewriting history.

>
> > aligned_largest, which was removed by 9503cd9cbaba
> > ("include/linux/compiler*.h: add version detection to
> > asm_volatile_goto").
>
> That commit is from Andrew's patch series which also rebases (usually
> at least every week), so you cannot depend on it.

Miguel, you should be able to drop that patch from your set then,
since Andrew's -mm tree flows into this -next tree as well, IIUC.
We'll take up that patch from there.

-- 
Thanks,
~Nick Desaulniers

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Nick,

On Tue, Oct 2, 2018 at 11:16 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Oct 2, 2018 at 2:11 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > Hi Miguel,
> >
> > On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > The Compiler Attributes series has been stable for 10+ days. To
> > > increase testing before 4.20, I would to request it being picked up
> > > for -next.
> > >
> > > The changes w.r.t. v5 in the LKML:
> > >
> > >   - Rebased on top of next-20180928, which required removing
> >
> > Unfortunately, trees/branches included in linux-next must be based on
> > something stable (usually Linus' tree, but it could be another
> > tree/branch that is included in linux-next that does not rebase).
> > Linux-next itself rebases every day, so snything based on it would drag
> > in a previous version of all the other trees :-(
>
> I think of this like a branch that's force pushed to.  Can't base
> other branches or trees off of it cause it's always moving/force
> rewriting history.

As I have read, -next is supposed to be a vision of what the merge
window will look like after merging everything, i.e. ideally -rc1. For
that to work for files out-of-tree (like these ones, which are not
maintained by a single tree), changes should be allowed to be stacked
on each other; otherwise, we cannot handle conflicts :-(

>
> >
> > > aligned_largest, which was removed by 9503cd9cbaba
> > > ("include/linux/compiler*.h: add version detection to
> > > asm_volatile_goto").
> >
> > That commit is from Andrew's patch series which also rebases (usually
> > at least every week), so you cannot depend on it.
>
> Miguel, you should be able to drop that patch from your set then,
> since Andrew's -mm tree flows into this -next tree as well, IIUC.
> We'll take up that patch from there.

Not sure what you mean by "drop that patch". There is no patch to
drop, there is a conflict with a change already in -next.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Dominique Martinet]

Miguel Ojeda wrote on Wed, Oct 03, 2018:
> As I have read, -next is supposed to be a vision of what the merge
> window will look like after merging everything, i.e. ideally -rc1. For
> that to work for files out-of-tree (like these ones, which are not
> maintained by a single tree), changes should be allowed to be stacked
> on each other; otherwise, we cannot handle conflicts :-(

The rule is the same as with a regular mainline pull; I don't have the
reference at hand but in some recent-ish pull request Linus said he
prefers the stable version with the conflict, and optionally you can
provide a second branch with the conflict resolved for reference, but
the pull request should be based on something stable even if it has
conflicts

If there is a conflict Stefen will resolve it like Linus/Greg would, and
the resolved bit will be carried over everyday so it's not much more
work -- exactly like a regular pull request for inclusion in the main
tree :)


(Found another mail arguing for stable base[1], but can't find the mail I
was thinking of)
 [1] http://lkml.kernel.org/r/CA+55aFw+dwofadgvzrM-UCMSih+f1choCwW+xFFM3aPjoRQX_g@mail.gmail.com

-- 
Dominique Martinet

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

Hi Miguel,

On Wed, 3 Oct 2018 00:36:52 +0200 Dominique Martinet <asmadeus@codewreck.org> wrote:
>
> Miguel Ojeda wrote on Wed, Oct 03, 2018:
> > As I have read, -next is supposed to be a vision of what the merge
> > window will look like after merging everything, i.e. ideally -rc1. For
> > that to work for files out-of-tree (like these ones, which are not
> > maintained by a single tree), changes should be allowed to be stacked
> > on each other; otherwise, we cannot handle conflicts :-(  
> 
> The rule is the same as with a regular mainline pull; I don't have the
> reference at hand but in some recent-ish pull request Linus said he
> prefers the stable version with the conflict, and optionally you can
> provide a second branch with the conflict resolved for reference, but
> the pull request should be based on something stable even if it has
> conflicts
> 
> If there is a conflict Stefen will resolve it like Linus/Greg would, and
> the resolved bit will be carried over everyday so it's not much more
> work -- exactly like a regular pull request for inclusion in the main
> tree :)

Exactly what Dominique said.  I will fix up the conflict (unless it is
a very complex conflict, in which case the author(s) should help) and
the Linus (or Greg) will do the same.  If you do depend on a patch in
Andrew's series, what happens if that patch does not get sent to Linus
during the merge window or Linus rejects it?

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Stephen,

On Wed, Oct 3, 2018 at 1:00 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Miguel,
>
> On Wed, 3 Oct 2018 00:36:52 +0200 Dominique Martinet <asmadeus@codewreck.org> wrote:
> >
> > Miguel Ojeda wrote on Wed, Oct 03, 2018:
> > > As I have read, -next is supposed to be a vision of what the merge
> > > window will look like after merging everything, i.e. ideally -rc1. For
> > > that to work for files out-of-tree (like these ones, which are not
> > > maintained by a single tree), changes should be allowed to be stacked
> > > on each other; otherwise, we cannot handle conflicts :-(
> >
> > The rule is the same as with a regular mainline pull; I don't have the
> > reference at hand but in some recent-ish pull request Linus said he
> > prefers the stable version with the conflict, and optionally you can
> > provide a second branch with the conflict resolved for reference, but
> > the pull request should be based on something stable even if it has
> > conflicts
> >
> > If there is a conflict Stefen will resolve it like Linus/Greg would, and
> > the resolved bit will be carried over everyday so it's not much more
> > work -- exactly like a regular pull request for inclusion in the main
> > tree :)
>
> Exactly what Dominique said.  I will fix up the conflict (unless it is
> a very complex conflict, in which case the author(s) should help) and
> the Linus (or Greg) will do the same.  If you do depend on a patch in
> Andrew's series, what happens if that patch does not get sent to Linus
> during the merge window or Linus rejects it?

This doesn't depend on anything. Not sure what is all the fuss about
-- people got confused into thinking we had to drop a patch for some
reason. As explained in the first email, I simply rebased v5 (which is
based on top of rcX) to resolve the conflict myself (i.e. it does
*not* depend on changes in -next). If you are the one solving
conflicts yourself (which is what I asked in my second email), there
is no problem to begin with; I will simply send v6 to you and we are
done.

When I sent the first email, I assumed that changes in -next were
supposed to be clean -- my mistake, but please document somewhere how
-next works! Specially that you are rerere'ing conflicts and
re-resolving them every day.

Then the discussion shifted to what to do with changes that actually
depend on other changes.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Steven Rostedt]

On Wed, 3 Oct 2018 14:34:28 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> When I sent the first email, I assumed that changes in -next were
> supposed to be clean -- my mistake, but please document somewhere how
> -next works! Specially that you are rerere'ing conflicts and
> re-resolving them every day.

Yes, a document in the Documentation directory on how to work with
linux-next may be a good idea. Something we can point people to when
they start using linux-next. I know there was something in the initial
email, but I've lost that.

-- Steve

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

HI Dominique,

On Wed, Oct 3, 2018 at 12:37 AM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Miguel Ojeda wrote on Wed, Oct 03, 2018:
> > As I have read, -next is supposed to be a vision of what the merge
> > window will look like after merging everything, i.e. ideally -rc1. For
> > that to work for files out-of-tree (like these ones, which are not
> > maintained by a single tree), changes should be allowed to be stacked
> > on each other; otherwise, we cannot handle conflicts :-(
>
> The rule is the same as with a regular mainline pull; I don't have the
> reference at hand but in some recent-ish pull request Linus said he

That is actually the first problem: there is no
reference/documentation at hand. :-P

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Steven Rostedt]

On Wed, 3 Oct 2018 14:14:28 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> HI Dominique,
> 
> On Wed, Oct 3, 2018 at 12:37 AM Dominique Martinet
> <asmadeus@codewreck.org> wrote:
> >
> > Miguel Ojeda wrote on Wed, Oct 03, 2018:  
> > > As I have read, -next is supposed to be a vision of what the merge
> > > window will look like after merging everything, i.e. ideally -rc1. For
> > > that to work for files out-of-tree (like these ones, which are not
> > > maintained by a single tree), changes should be allowed to be stacked
> > > on each other; otherwise, we cannot handle conflicts :-(  
> >
> > The rule is the same as with a regular mainline pull; I don't have the
> > reference at hand but in some recent-ish pull request Linus said he  
> 
> That is actually the first problem: there is no
> reference/documentation at hand. :-P
>

Yes, and one of the topics for Maintainers Summit is to create one. One
is actually in the works led by Dan Williams.

-- Steve

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Theodore Y. Ts'o]

On Wed, Oct 03, 2018 at 12:12:10AM +0200, Miguel Ojeda wrote:
> As I have read, -next is supposed to be a vision of what the merge
> window will look like after merging everything, i.e. ideally -rc1. For
> that to work for files out-of-tree (like these ones, which are not
> maintained by a single tree), changes should be allowed to be stacked
> on each other; otherwise, we cannot handle conflicts :-(

In general, best practice is to base tree on an -rcX commit.  I
usually will use something like -rc4 which is after most of the major
changes have gone in.  This tends to reduce conflicts for most git
trees.

There are times when a commit in one tree needs to depend on a commit
in another tree.  What to do depends on the circumstances.

One solution is to base one subsystem's git tree on another
subsystem's git tree --- *if* that git tree is one that doesn't get
rebase.  You'll need to talk to the subsystem maintainer to find out
whether or not that's the case.  But that's actually not a great
solution, because what can happen is if the tree A is based on tree B,
and there is something in tree B which Linus objects to, tree B won't
get pulled.  And since tree A depends on tree B, Linus will refuse to
pull tree A as well.  We recently had a case where a subsystem pull
got delayed by a full development cycle because of this.

So another solution is to simply evade the problem.  If the reason why
tree A needs to depend on tree B is that tree B is using some
interface which has changed, if it's a minor change, then Stephen will
fix it up when he pulls the changes; just as Linus will.

If the issue is that there is some common infrastructure which two git
tree needs, what will often happen is that just those patches which
provide the new infastructure will get put on a branch based on -rcX
on one of the git trees.  And then the subsystems will base their work
on that sub-branche.  For example, suppose the file system developers
have collectively decided that there should be a new fallocate(2)
flag, FALLOC_FL_DONT_RANDOMLY_LOSE (ala RFC 748).  The work to define
and enable that feature in the VFS layer might be placed on the
randomly-lose git branch on xfs.git.  And then the xfs and ext4
development branches will both be based on the randomly-lose branch.

Yet another solution is to arrange the code changes to avoid needing
commits that might conflict.  For example, in fs/ext4/ext4.h, I very
deliberately did this.

/* Until this gets included into linux/compiler-gcc.h */
#ifndef __nonstring
#if defined(GCC_VERSION) && (GCC_VERSION >= 80000)
#define __nonstring __attribute__((nonstring))
#else
#define __nonstring
#endif
#endif

You included a cleanup patch to remove it in your git tree, but it
wasn't actually necessary.  If there was a merge conflict, it would be
simple enough to just drop your cleanup patch, since I had carefully
used #ifndef __nonstring... #endif.  So the idea was that if someone
defined __nonstring somewhere else, it wouldn't break the build with a
duplicate #define since it was protected with an #ifndef.

I didn't mind that you included a cleanup patch; but I set things up
so that it would not be necessary, since often the best way to solve a
merge conflict is by avoiding the need for the change (in some other
git tree) in the first place.  :-)

Cheers,

					- Ted

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Ted,

On Wed, Oct 3, 2018 at 1:25 AM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Wed, Oct 03, 2018 at 12:12:10AM +0200, Miguel Ojeda wrote:
> > As I have read, -next is supposed to be a vision of what the merge
> > window will look like after merging everything, i.e. ideally -rc1. For
> > that to work for files out-of-tree (like these ones, which are not
> > maintained by a single tree), changes should be allowed to be stacked
> > on each other; otherwise, we cannot handle conflicts :-(
>
> In general, best practice is to base tree on an -rcX commit.  I
> usually will use something like -rc4 which is after most of the major
> changes have gone in.  This tends to reduce conflicts for most git
> trees.

Sure. That is what I do for mainline PRs to Linus. As you say, it
works fine for mostly independent trees. But it doesn't (that well)
for out-of-tree stuff or for stuff spanning several trees.

>
> There are times when a commit in one tree needs to depend on a commit
> in another tree.  What to do depends on the circumstances.
>

Exactly. And for this case, I simply assumed Stephen wanted a clean
series to apply on top of the latest next-* tag (same way we base
stuff on top of -rc*s). Note that this is *not* really a "tree"
collecting changes/development, it is a patch series, i.e. what is
important is only the range-diff.

What has surprised me is that -next does not allow for range-diffs
(patches/commits/...) to be inside -next, maybe applied after all the
normal merges of trees. I simply assumed it did, given what I could
find about -next (which does not seem to be documented properly, by
the way).

>
> So another solution is to simply evade the problem.  If the reason why
> tree A needs to depend on tree B is that tree B is using some
> interface which has changed, if it's a minor change, then Stephen will
> fix it up when he pulls the changes; just as Linus will.

While this is a simple conflict, I don't really agree with release
maintainers having to fix conflicts on the fly (even if it is a single
trivial line), but that is an orthogonal discussion...

> Yet another solution is to arrange the code changes to avoid needing
> commits that might conflict.  For example, in fs/ext4/ext4.h, I very
> deliberately did this.
>
> /* Until this gets included into linux/compiler-gcc.h */
> #ifndef __nonstring
> #if defined(GCC_VERSION) && (GCC_VERSION >= 80000)
> #define __nonstring __attribute__((nonstring))
> #else
> #define __nonstring
> #endif
> #endif
>
> You included a cleanup patch to remove it in your git tree, but it
> wasn't actually necessary.  If there was a merge conflict, it would be
> simple enough to just drop your cleanup patch, since I had carefully
> used #ifndef __nonstring... #endif.  So the idea was that if someone
> defined __nonstring somewhere else, it wouldn't break the build with a
> duplicate #define since it was protected with an #ifndef.
>
> I didn't mind that you included a cleanup patch; but I set things up
> so that it would not be necessary, since often the best way to solve a
> merge conflict is by avoiding the need for the change (in some other
> git tree) in the first place.  :-)

Alright, I am not sure how to answer this without sounding
"aggressive" and maybe I misunderstood something you tried to point
out, so I apologize in advance. There are several points here:

  * The merge conflict isn't related to this (but let's assume it was,
since you pointed this out as an example -- I guess; although I am not
sure why).

  * Thanks for explaining, but I think most people here understand how
#ifndef works :-) Also note that we already had guards for some
attributes that needed per-compiler implementations.

  * The patch is actually necessary:

    - Several people argued that examples should be present when I
introduced __nonstring earlier in the series. While I don't think they
are mandatory for this case, they are good to have, and since they
were anyway requested, I happily added them.

    - Changes should include everything related to them (as far as
possible, i.e. extreme examples aside). Adding __nonstring and not
removing the ext4 part would simply be leaving undone work (which has
to be done sooner or later). To be honest, it could have even been
done in the same commit and I would say it is logically OK (even if
not great).

    - Related to that: I think the outer #ifndef shouldn't have been
included to begin with, because it is either wrong or dead code: if we
end up having a __nonstring, it should be removed at the same time
(like this series does); and if __nonstring isn't there (like now), it
isn't guarding against anything. At worst, it could hide a mismatch in
the definitions.

    - I promised to you I would clean it up :-)

You seem to argue that it is better to avoid merge conflicts rather
than doing a proper series. Well, I think we should really try to
avoid conflicts pushing down the """quality""" of
patches/commits/series.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Theodore Y. Ts'o]

On Wed, Oct 03, 2018 at 01:54:05PM +0200, Miguel Ojeda wrote:
> 
> Exactly. And for this case, I simply assumed Stephen wanted a clean
> series to apply on top of the latest next-* tag (same way we base
> stuff on top of -rc*s). Note that this is *not* really a "tree"
> collecting changes/development, it is a patch series, i.e. what is
> important is only the range-diff.
> 
> What has surprised me is that -next does not allow for range-diffs
> (patches/commits/...) to be inside -next, maybe applied after all the
> normal merges of trees. I simply assumed it did, given what I could
> find about -next (which does not seem to be documented properly, by
> the way).

Part of the reason why I wrote my long message was because the -next
tree usage has been mostly by convention.  Better documentation would
be a good thing; I think the main reason why we don't have it is that
historically, the people who submit direct pull requests to Linus
already know how things work.  Obviously that's not a great
assumption, but in fact there are a large number of things about what
is needed to be a subsystem maintainer which needs to be documented,
and the linux-next tree is just one part of it.  (And perhaps not even
the biggest part.)

Historically there was one source of changes into the linux-next tree
which was done as quilt-style patch series, and that was Andrew
Morton's mmotm tree.  Part of the problem is that it's a lot more work
to consume a patch series, and so these days while Andrew still uses a
patch series, he exports a git tree[1] which is what I believe Stephen
and Linus use.  (For that matter, I use a patch series[2] as well, but
the public interface is the ext4.git tree on git.kernel.org.)

[1] https://www.ozlabs.org/~akpm/mmotm/mmotm-readme.txt
[2] https://ext4.wiki.kernel.org/index.php/Ext4_patchsets

> While this is a simple conflict, I don't really agree with release
> maintainers having to fix conflicts on the fly (even if it is a single
> trivial line), but that is an orthogonal discussion...

Sure; there are some srtong reasons for it, but we can save that for
the Maintainer's Handbook discussion.  I will say that this is
something where attempts to avoid Linus needing to fix conflicts on
the fly, such as a rebase right before a PULL request, violates
Linus's rules which has in the past resulted in him expressing a
"strong" correction e-mail to Maintainers since it was assumed that
they really should have known better.  Linus has said he doesn't want
to yell at Maintainers, so we can support him by getting the
Maintainer's Handbook out there.  :-)

> Alright, I am not sure how to answer this without sounding
> "aggressive" and maybe I misunderstood something you tried to point
> out, so I apologize in advance. There are several points here:
> 
>   * The merge conflict isn't related to this (but let's assume it was,
> since you pointed this out as an example -- I guess; although I am not
> sure why).

I thought I was clear that I used it as an example, but my apologies
if that didn't come across.

>     - Changes should include everything related to them (as far as
> possible, i.e. extreme examples aside). Adding __nonstring and not
> removing the ext4 part would simply be leaving undone work (which has
> to be done sooner or later). To be honest, it could have even been
> done in the same commit and I would say it is logically OK (even if
> not great).
> ...
>
> You seem to argue that it is better to avoid merge conflicts rather
> than doing a proper series. Well, I think we should really try to
> avoid conflicts pushing down the """quality""" of
> patches/commits/series.

And I think this is where the disagreement lies, which is why I
bothered to send my long missive in the first place.  From my point of
view, and I suspect many maintainers share this, avoiding merge
conflicts is way important than making sure everything is "done" in a
single patch series.  There have been plenty of changes where cleanup
is done latter, and/or where preparatory patches are done in one
release, and the big change happens in the next release, 3 months
later.

That's because some of the alternatives, such as basing your git tree
on an unstable head branch, such as linux-next, or a last minute
rebase which means that the actual patches that which gets the pull
rebase hasn't been tested or soaked in linux-next, are far worse
because it can lead to lower quality git trees getting merged during
the merge window.

There is a tension here between maximizing the "quality" of a patch
series, and maximizing the "quality" of the git trees that feed into
the merge window.  Or perhaps the better way of saying this is
minimizing the risk of bad changes getting integreted during the merge
window.  Some technical debt which gets cleaned up in the next release
is in my view a very low price to pay in order to minimize risk.

Part of this is because I've had to waste time debugging changes made
to the ext4 sources which came in via someone else's git tree that
were "obviously correct" or "trivial changes" --- but they weren't.
Most of the time they get caught as part of linux-next testing, but
not always.  So sometimes the interests of one subsystem maintainer
can end up conflicting with the interests of the patch series author,
or the interests of another subsystem maintainer.  And that's why we
have some of these "cultural understandings", many of which we clearly
need to document.

(And yes, in this case I didn't object to the cleanup being in your
patch series; normally I don't care, unless it actually break things.
I was just trying to make the point from my perspective, nothing
*required* that the change be in your git tree, and if it *was*
causing the problem, I had gone out of my way to make sure to make it
easy for you to drop the change; and from my perspective I was doing
you --- and me :-) --- a favor when I added the outer #ifndef, which I
had done with full consideration, specifically for this reason.  Even
if the definition was different, my definition *had* been fully tested
with over a 27+ VM hours of regression testing, and if it turned out
that they were different, cleaning that up three months from now in
the next release is just *fine* as far as I'm concerned.)

Cheers,

						- Ted

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Ted,

On Wed, Oct 3, 2018 at 5:33 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Wed, Oct 03, 2018 at 01:54:05PM +0200, Miguel Ojeda wrote:
> >
> > Exactly. And for this case, I simply assumed Stephen wanted a clean
> > series to apply on top of the latest next-* tag (same way we base
> > stuff on top of -rc*s). Note that this is *not* really a "tree"
> > collecting changes/development, it is a patch series, i.e. what is
> > important is only the range-diff.
> >
> > What has surprised me is that -next does not allow for range-diffs
> > (patches/commits/...) to be inside -next, maybe applied after all the
> > normal merges of trees. I simply assumed it did, given what I could
> > find about -next (which does not seem to be documented properly, by
> > the way).
>
> Part of the reason why I wrote my long message was because the -next
> tree usage has been mostly by convention.  Better documentation would
> be a good thing; I think the main reason why we don't have it is that
> historically, the people who submit direct pull requests to Linus
> already know how things work.  Obviously that's not a great
> assumption, but in fact there are a large number of things about what
> is needed to be a subsystem maintainer which needs to be documented,
> and the linux-next tree is just one part of it.  (And perhaps not even
> the biggest part.)

It not a bad assumption either -- I happen to be one a very odd
maintainer because I have not been actively contributing for years so
I missed the -next implementation, so it is fine, I don't blame anyone
for my mistake; still a few lines would be fine: who is the
maintainer, where to send the patches, who should be Cc'd, etc. :-)

>
> Historically there was one source of changes into the linux-next tree
> which was done as quilt-style patch series, and that was Andrew
> Morton's mmotm tree.  Part of the problem is that it's a lot more work
> to consume a patch series, and so these days while Andrew still uses a
> patch series, he exports a git tree[1] which is what I believe Stephen
> and Linus use.  (For that matter, I use a patch series[2] as well, but
> the public interface is the ext4.git tree on git.kernel.org.)

Hm... I agree that patches (i.e. referring to the email workflow) are
a pain, but I am not sure why the actual "patches" (i.e. referring to
the commits/diffs) need to be harder to work with. Maybe we don't have
the tools for that yet. For instance, the new git range-diff command
is actually quite good to compare versions of series after rebasing.

>
> [1] https://www.ozlabs.org/~akpm/mmotm/mmotm-readme.txt
> [2] https://ext4.wiki.kernel.org/index.php/Ext4_patchsets
>
> > While this is a simple conflict, I don't really agree with release
> > maintainers having to fix conflicts on the fly (even if it is a single
> > trivial line), but that is an orthogonal discussion...
>
> Sure; there are some srtong reasons for it, but we can save that for
> the Maintainer's Handbook discussion.  I will say that this is
> something where attempts to avoid Linus needing to fix conflicts on
> the fly, such as a rebase right before a PULL request, violates
> Linus's rules which has in the past resulted in him expressing a
> "strong" correction e-mail to Maintainers since it was assumed that
> they really should have known better.  Linus has said he doesn't want
> to yell at Maintainers, so we can support him by getting the
> Maintainer's Handbook out there.  :-)

Yeah, I actually got the correction myself :-) I understand Linus'
reasons about the benefits of using the merges (and I agree that
information shouldn't be dropped); but I think there has to be a
better way to handle conflicts. (See below).

>
> > Alright, I am not sure how to answer this without sounding
> > "aggressive" and maybe I misunderstood something you tried to point
> > out, so I apologize in advance. There are several points here:
> >
> >   * The merge conflict isn't related to this (but let's assume it was,
> > since you pointed this out as an example -- I guess; although I am not
> > sure why).
>
> I thought I was clear that I used it as an example, but my apologies
> if that didn't come across.

No apologies needed!

>
> >     - Changes should include everything related to them (as far as
> > possible, i.e. extreme examples aside). Adding __nonstring and not
> > removing the ext4 part would simply be leaving undone work (which has
> > to be done sooner or later). To be honest, it could have even been
> > done in the same commit and I would say it is logically OK (even if
> > not great).
> > ...
> >
> > You seem to argue that it is better to avoid merge conflicts rather
> > than doing a proper series. Well, I think we should really try to
> > avoid conflicts pushing down the """quality""" of
> > patches/commits/series.
>
> And I think this is where the disagreement lies, which is why I
> bothered to send my long missive in the first place.  From my point of
> view, and I suspect many maintainers share this, avoiding merge
> conflicts is way important than making sure everything is "done" in a
> single patch series.  There have been plenty of changes where cleanup
> is done latter, and/or where preparatory patches are done in one
> release, and the big change happens in the next release, 3 months
> later.

I agree with that being a problem for huge changes and/or developed in
parallel. Still, if we have -next (which is meant precisely to test
integration), we should not have to reduce nor split the amount of
work on the trees just to solve integration in most cases, no? That is
my point. (See again below).

>
> That's because some of the alternatives, such as basing your git tree
> on an unstable head branch, such as linux-next, or a last minute
> rebase which means that the actual patches that which gets the pull
> rebase hasn't been tested or soaked in linux-next, are far worse
> because it can lead to lower quality git trees getting merged during
> the merge window.
>
> There is a tension here between maximizing the "quality" of a patch
> series, and maximizing the "quality" of the git trees that feed into
> the merge window.  Or perhaps the better way of saying this is
> minimizing the risk of bad changes getting integreted during the merge
> window.  Some technical debt which gets cleaned up in the next release
> is in my view a very low price to pay in order to minimize risk.

I see this tension as a missing piece of the puzzle. I am not an
expert on the kernel, but I worked in the past on a system similar to
what Stephen is doing: merging many quasi-independent trees at CERN
(but it was not Git in that case, but CVS; yeah... :-).

The problem we had there was similar: 1000+ projects with changes
depending on each other in many, many cases. We had a "release
manager" (similar to what Stephen is doing) sorting it all out in time
to make the release required for the CMS detector -- they suffered a
lot...

We ended up designing a system to make cross-project changes called
"tagsets" (equivalent to the Next/* files included in linux-next). I
also added support for assigning dependencies to other pending (or
done) tagsets in a graph. Cycles in the graph meant changes had to go
in at the same time. The dependencies were plotted in an SVG graph
drawn by Graphviz. All this in a webapp to make it convenient for
remote developers and release managers. Fun times :-)

This served as a transition step from CVS to Git (and nowadays I think
everything is done with standard PRs in GitHub AFAIK). However, it
seems we are having the same problem. Although it is much easier to
handle (less trees, git rerere, better tooling, etc.), Stephen still
has a custom system to deal with all the trees, so it seems there *is*
something missing. (...continuing below using the example)

>
> Part of this is because I've had to waste time debugging changes made
> to the ext4 sources which came in via someone else's git tree that
> were "obviously correct" or "trivial changes" --- but they weren't.
> Most of the time they get caught as part of linux-next testing, but
> not always.  So sometimes the interests of one subsystem maintainer
> can end up conflicting with the interests of the patch series author,
> or the interests of another subsystem maintainer.  And that's why we
> have some of these "cultural understandings", many of which we clearly
> need to document.

(...from above) Yes, indeed, but the point is that changes should be
able to be done in different parts of the kernel crossing trees and
decided by everyone.

We have this problem only because we have ended up dividing the kernel
in so many almost-independent trees (and mailing lists) that, in the
end, it is like having several independent projects; which then in
turn requires a custom system to sort out everything again (which is
the same problem we faced at CMS).

So either a) we are missing a system to actually handle this kind of
mega-project composed of smaller projects (like Stephen's, or the
already-retired one at CMS); or b) we are missing way to discuss
changes globally that scales (so that splitting trees is not required
and the problem goes away).

Just to be clear, I don't have a magical solution for this :-) I am
just saying that it looks very, very familiar to me and that, if we
had that something, those problems about wasting time debugging
changes made to different trees shouldn't be there; and therefore we
wouldn't need to drop patches, leave dead code for defensive reasons,
rejecting work, creating technical debt, etc.

Maybe the "solution" should be a new webapp to sort out kernel
development better. Maybe it should be a better discussion platform
other than email. Maybe it should be some Git support for managing
many dependent "subtrees" with possible dependencies, preparing
"releases", doing "super-merges" of all trees when ready (e.g. merge
window) -- basically linux-next and Stephen's scripts, but kept in the
repository itself; so that Linus could simply "trigger" the full merge
on the merge window, etc.

>
> (And yes, in this case I didn't object to the cleanup being in your
> patch series; normally I don't care, unless it actually break things.
> I was just trying to make the point from my perspective, nothing
> *required* that the change be in your git tree, and if it *was*
> causing the problem, I had gone out of my way to make sure to make it
> easy for you to drop the change; and from my perspective I was doing
> you --- and me :-) --- a favor when I added the outer #ifndef, which I
> had done with full consideration, specifically for this reason.  Even
> if the definition was different, my definition *had* been fully tested
> with over a 27+ VM hours of regression testing, and if it turned out
> that they were different, cleaning that up three months from now in
> the next release is just *fine* as far as I'm concerned.)

Yes, of course your change was tested, and that is a good thing. My
point is simply that it is still misleading, and it is still better to
simply do it kernel-wide if possible (which goes back to: we currently
have effectively independent projects going on). The change would have
been tested by you the same way if it had been applied kernel-wide.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

On Wed, Oct 3, 2018 at 11:23 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Wed, Oct 3, 2018 at 5:33 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> > had done with full consideration, specifically for this reason.  Even
> > if the definition was different, my definition *had* been fully tested
> > with over a 27+ VM hours of regression testing, and if it turned out

I forgot to mention that if the definitions were different, it could
have caused a problem, because your definition wouldn't apply, so your
27+ hours of testing wouldn't have mattered :-P Without the #ifndef,
we would have at least got a redefinition warning about it.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Theodore Y. Ts'o]

On Wed, Oct 03, 2018 at 11:41:08PM +0200, Miguel Ojeda wrote:
> 
> I forgot to mention that if the definitions were different, it could
> have caused a problem, because your definition wouldn't apply, so your
> 27+ hours of testing wouldn't have mattered :-P Without the #ifndef,
> we would have at least got a redefinition warning about it.

In this case, yes.  Again, I emphasize that I was using the ext4.h
cleanup as an *example*.  The point I was trying to make was that your
change did *not* do a full set of deep ext4 regression tests because
the your change didn't go through the ext4.git tree.  I have seen
cases where "simple mechanical changes" were anything but, and while
the kernel *compiled* it resulted in the file system not working.
(And in the worst case, it resulted actual data loss or file system
corruption.)

My test scripts are public, and in fact I've gone out of my way to
make them easy for other people to run them, with documentation[1][2],
slide sets[3], etc.  But the vast majority of people who submit
patches to ext4.git don't bother --- and as far as I know *no one* who
sends ext4 changes via other git trees *ever* bothered.  (And, for one
recent patchset where the ext4 developer spent a lot of time using
kvm-xfstests before submission, accepting that set of changes was easy
and was a joy.  I'm a big believe in tests.)

[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/gce-xfstests.md
[2] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
[3] https://thunk.org/gce-xfstests

As far as what you want to do, there are solutions, but they require a
radically different way of developing.  For example, inside Google,
for the non-public sources (e.g., excluding Android, Chrome OS, etc.)
there is a single source tree, with thousands of projects.  They use
common set of C++ utility libraries/classes, and there is a
standardized build system (bazel[4]), and a standardized way of
writing tests.  More importantly, there is a fully automated
continuous testing system which will automatically trigger and run the
appropriate tests --- at the moment when the patch is submitted into
the Google's custom code review system --- and the results of the test
are automatically submitted as comments in the code review system, so
the automated testing is integrated into the code review.  For changes
that affected all or substantially all projects, it's possible to run
tests across the entire source tree using automated, centralized
resources, and even then it can take a significantlly non-trivial (as
in days) of calendar time running on many systems in parallel.

[4] https://bazel.build/

So if you are willing to completely standardize your testing
infrastructure and devote utterly lavish amounts of automated testing
which is deeply integrated into a standardized, non-email code review
system, the challenge you have identified *has* been solved.  But
trust me when I say that it is a very non-trivial thing to do, and it
requires a lot of very strict code development practices that are
imposed on all Google C++ developers working in the common tree (which
is probably 90%+ of the SWE's inside Google).  I'm not at all
convinced that it could be adopted or imposed on the kernel
development process.  In fact, I'm quite confident it could not be.

I actually think the Linux Kernel's approach of carefully segregating
how code and headers to avoid merge conflicts (and worse, semantic
conflicts that may git merge and build succesfully, but then blow up
in subtle ways leading to user data loss) is actually a pretty good
way of doing things.  It works 99.99% of the the commits.  True, it
wasn't optimal for the changes you were trying to make; but your
experience is the exception, not the rule.

The risk here is that it's very easy to engineer changes in processes
for corner cases, and which make things much worse (either taking more
time, or more effort, or being less reliable) for the common case.

Cheers,

					- Ted

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Ted,

On Thu, Oct 4, 2018 at 7:02 AM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> In this case, yes.  Again, I emphasize that I was using the ext4.h
> cleanup as an *example*.  The point I was trying to make was that your
> change did *not* do a full set of deep ext4 regression tests because
> the your change didn't go through the ext4.git tree.  I have seen

Because it shouldn't matter from which tree they come from to get the
tests run. *That* is the problem: effectively we have independent
projects in the kernel. If you tell me that my change did not run the
ext4 tests (even if I Cc'd you or maybe some ML, etc.), it means there
is something wrong.

> cases where "simple mechanical changes" were anything but, and while
> the kernel *compiled* it resulted in the file system not working.
> (And in the worst case, it resulted actual data loss or file system
> corruption.)

Yes, I have also experienced one-liners breaking entire projects.
Ideally, any change should be assumed to break everything unless
proved otherwise, and I agree with having as much testing as possible
-- see above for the actual problem here.

> My test scripts are public, and in fact I've gone out of my way to
> make them easy for other people to run them, with documentation[1][2],
> slide sets[3], etc.  But the vast majority of people who submit
> patches to ext4.git don't bother --- and as far as I know *no one* who
> sends ext4 changes via other git trees *ever* bothered.  (And, for one
> recent patchset where the ext4 developer spent a lot of time using
> kvm-xfstests before submission, accepting that set of changes was easy
> and was a joy.  I'm a big believe in tests.)

I believe you -- but actually I would *expect* that. Nobody runs them
because nobody has either the time, the expertise, or feels the need
to do so. It is not a matter of "being easy to run". It is simply that
people won't do it unless you force them to (and if you force them,
you will lose some contributions).

The solution is not providing slides, documentation, or making it
fool-proof. The solution, in my view, is avoiding people having to do
the tests. Currently, I thought we do that in the kernel by Cc'ing
whoever is a maintainer of the code and letting that person do
whatever is required for those changes. Of course, you can automate
this in many fancy ways as many projects/companies have done (and as
you describe for Google).

However...

> So if you are willing to completely standardize your testing
> infrastructure and devote utterly lavish amounts of automated testing

...I don't agree. I don't think this follows from the above
discussion. While release management and testing are topics that go
together (pretty much always), they are two different, independent
problems. I am talking here about the need for some "software" to do
the release management (i.e. merging many trees, doing integration,
etc.), not the testing side of it (which can change irrespective of
the release management side; e.g. may have different triggers, may
have different requirements, may use different systems to help, etc.).

Actually, it is fairly easy to prove: Pick Documentation/ and imagine
it was only plain text files without processing required (as in the
old days). For that "project", you don't need any testing whatsoever,
but you still need to do release integration. For ext4, you may want
to use your testing scripts. For drivers/foo/bar.c, maybe the only way
of testing is plugging the hardware and seeing that it works. So,
different projects, different testing mechanisms; but same release
integration at the end of the day.

> which is deeply integrated into a standardized, non-email code review
> system, the challenge you have identified *has* been solved.  But

Of course it has been "solved". All projects with hundreds of
contributors/subprojects/... have to deal with the same issues, and
they have to keep moving forward at the end of the day. The point is
not how Google, or CERN, or anybody else does it. The point is that
there should be a better way to handle this which makes kernel
development better.

I explained the issue we faced at CERN years ago simply because it was
very similar to this (merging given tags of different trees,
basically). I am not sure how/if your Google example is relevant here,
since it is a completely custom system, it is very different to what
the kernel does, etc. I understand you are trying to say "this is no
news, there are other ways of solving it, it is solved at Google"; but
with my "example" I was not trying to say "we solved it at CERN" or
trying to say I am a "guru"; I was simply stating that it seems there
could be a project coming out of this to solve this space *in the
context of the kernel workflow*.

> trust me when I say that it is a very non-trivial thing to do, and it
> requires a lot of very strict code development practices that are
> imposed on all Google C++ developers working in the common tree (which
> is probably 90%+ of the SWE's inside Google).  I'm not at all
> convinced that it could be adopted or imposed on the kernel
> development process.  In fact, I'm quite confident it could not be.

This paragraph seems to follow from the fact that you connect
heavily-automated testing with release integration. I don't
necessarily agree with that as explained above, so I think I could cut
here this part of the discussion, but anyway, here it goes: coding
development practices are not completely tied to release integration.
Same as testing: i.e. you *can* do release integration (and heavy
testing) without strict coding development guidelines. We actually do
that at the kernel nowadays (and we did it at CERN too).

>
> I actually think the Linux Kernel's approach of carefully segregating
> how code and headers to avoid merge conflicts (and worse, semantic
> conflicts that may git merge and build succesfully, but then blow up

Not sure how the kernel is preventing semantic conflicts (in different
ways than other projects, I mean). What do you mean?

> in subtle ways leading to user data loss) is actually a pretty good
> way of doing things.  It works 99.99% of the the commits.  True, it

I would say many commits are local enough that it is not really a
problem. The issue is non-local commits, and I am not that confident
it works for 99.99% of those.

> wasn't optimal for the changes you were trying to make; but your
> experience is the exception, not the rule.

Sure, but regardless of how well it works, the fact is that Stephen is
managing some custom (and AFAIK, ad hoc) scripts. That usually means
there is something that may be factorized out.

>
> The risk here is that it's very easy to engineer changes in processes
> for corner cases, and which make things much worse (either taking more
> time, or more effort, or being less reliable) for the common case.

Agreed, but changes are not necessarily bad, i.e. they are not
guaranteed to be worse than the previous state. Actually, I would
argue we could reach a better state of the art for *all*
commits/cases, not only corner ones.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

[Miguel Ojeda]

Hi Stephen,

On Tue, Oct 2, 2018 at 11:11 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Miguel,
>
> On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > The Compiler Attributes series has been stable for 10+ days. To
> > increase testing before 4.20, I would to request it being picked up
> > for -next.
> >
> > The changes w.r.t. v5 in the LKML:
> >
> >   - Rebased on top of next-20180928, which required removing
>
> Unfortunately, trees/branches included in linux-next must be based on
> something stable (usually Linus' tree, but it could be another
> tree/branch that is included in linux-next that does not rebase).
> Linux-next itself rebases every day, so snything based on it would drag
> in a previous version of all the other trees :-(

I assumed you could apply changes as a diff/patches/cherry-pick, not
as a merge, for those that went on top of others (so that at the new
merge window, conflicts were already solved). Otherwise, why are
next-* tags/branches provided anyway?

>
> > aligned_largest, which was removed by 9503cd9cbaba
> > ("include/linux/compiler*.h: add version detection to
> > asm_volatile_goto").
>
> That commit is from Andrew's patch series which also rebases (usually
> at least every week), so you cannot depend on it.

Then who is solving the conflict?

Thanks!

Cheers,
Miguel