| Message ID | CANiq72=rgT9cPWH64Tf_8gAU09EiH5FvGqFUdOjV=spVbM5m8Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [GIT,PULL,linux-next] Add Compiler Attributes tree | expand |
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.
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.
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
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
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
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?
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
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
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
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
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
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
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
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
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
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
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
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