Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 8, 2018 at 12:14 PM Theodore Y. Ts'o wrote: > > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > > > I know at least StGit mail does not grok that "#"notation. I've > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > > preferred over "# " if only because it can be used to track > > fixes to commits that have been backported to stable. Is there any > > reason for "# " to continue in a world where we have "Fixes:"? > > The main annoyance I have with Fixes is because it can be a pain to > figure out what the "# " would be. Something like: > > % tag --contains DEADBEEF | grep ^v | head > > doesn't work because kernel version numbers don't sort obviously. So > v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't > do the right. Unless I'm misunderstanding, I think you want: git describe --contains $COMMIT --match=v[345]* ...which should give you the latest tagged kernel according to that match spec.
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 8, 2018 at 12:14 PM Theodore Y. Ts'o wrote: > > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > > > I know at least StGit mail does not grok that "#"notation. I've > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > > preferred over "# " if only because it can be used to track > > fixes to commits that have been backported to stable. Is there any > > reason for "# " to continue in a world where we have "Fixes:"? > > The main annoyance I have with Fixes is because it can be a pain to > figure out what the "# " would be. Something like: > > % tag --contains DEADBEEF | grep ^v | head > > doesn't work because kernel version numbers don't sort obviously. So > v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't > do the right. Unless I'm misunderstanding, I think you want: git describe --contains $COMMIT --match=v[345]* ...which should give you the latest tagged kernel according to that match spec.
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, 8 Nov 2018, Greg KH wrote:
> On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> > Fixes: 1234567890AB ("subsys/comp: Short summary")
> > Backport-to: 4.14
>
> Ick, no. Just stick to the "Fixes:" tag. My scripts can now track when
> a patch is backported to a stable tree so that I know to apply it to
> older ones despite the original patch showing up in a newer release.
Fine with me.
I just did a few stats.
commits between 4.19-rc1 and 4.19-final:
Total commits: 2098
Total Cc: stable 334
Cc: stable 11935%
Cc: stable #kver10 3%
Cc: stable Revert 0 0%
Cc: stable Fixes 12337%
Cc: stable Fixes #kver8124%
Total Fixes only584
Fixes only > 4.1822739%
Fixes only <= 4.1835761%
and between 4.19 and todays top of tree:
Total commits: 12947
Total Cc: stable 251
Cc: stable8534%
Cc: stable #kver2811%
Cc: stable reverts 2 1%
Cc: stable Fixes 7931%
Cc: stable Fixes #kver5522%
Total Fixes only835
Fixes only > 4.1983099%
Fixes only <= 4.19 5 1%
Not much change in the 'cc stable' ratio for those without any information
and for those with Fixes tag and #kver.
The cc stable + kver and the cc stable + Fixes differ, but their sum is
roughly the same.
But the real interesting change is that between 4.19-rc1 and 4.19-final the
number of Fixes only (no CC stable) commits which fix a commit in 4.18 or
earlier is rather high, while the same category on post 4.19 is minimal.
I'll run a larger and more detailed scan to figure out whether there is a
trend or this is just random.
Thanks,
tglx
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, 8 Nov 2018, Greg KH wrote:
> On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> > Fixes: 1234567890AB ("subsys/comp: Short summary")
> > Backport-to: 4.14
>
> Ick, no. Just stick to the "Fixes:" tag. My scripts can now track when
> a patch is backported to a stable tree so that I know to apply it to
> older ones despite the original patch showing up in a newer release.
Fine with me.
I just did a few stats.
commits between 4.19-rc1 and 4.19-final:
Total commits: 2098
Total Cc: stable 334
Cc: stable 11935%
Cc: stable #kver10 3%
Cc: stable Revert 0 0%
Cc: stable Fixes 12337%
Cc: stable Fixes #kver8124%
Total Fixes only584
Fixes only > 4.1822739%
Fixes only <= 4.1835761%
and between 4.19 and todays top of tree:
Total commits: 12947
Total Cc: stable 251
Cc: stable8534%
Cc: stable #kver2811%
Cc: stable reverts 2 1%
Cc: stable Fixes 7931%
Cc: stable Fixes #kver5522%
Total Fixes only835
Fixes only > 4.1983099%
Fixes only <= 4.19 5 1%
Not much change in the 'cc stable' ratio for those without any information
and for those with Fixes tag and #kver.
The cc stable + kver and the cc stable + Fixes differ, but their sum is
roughly the same.
But the real interesting change is that between 4.19-rc1 and 4.19-final the
number of Fixes only (no CC stable) commits which fix a commit in 4.18 or
earlier is rather high, while the same category on post 4.19 is minimal.
I'll run a larger and more detailed scan to figure out whether there is a
trend or this is just random.
Thanks,
tglx
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 05:19:47PM -0500, Theodore Y. Ts'o wrote: > On Thu, Nov 08, 2018 at 01:04:49PM -0800, Greg KH wrote: > > > (Also note that even with fast SSD's and/or everything in page cache, > > > runnning "tag --contains " will take a good 3-4 seconds, and > > > if the git packs are not in the page cache, and/or you're unfortunate > > > enough to have your git trees on an HDD it's not pretty.) > > > > I recommend the "static cache" or whatever that thing is called, that > > helps out a _LOT_ with stuff like this. For the kernel tree, which is > > never rebased, it speeds up this so much. > > At the risk of asking a stupid question, which cache is this? I don't > think it's the untrackedCache; is it the BitmapHashCache? It's the 'commitGraph', here's the article I was trying to remember I learned this from: https://blogs.msdn.microsoft.com/devops/2018/06/25/supercharging-the-git-commit-graph/ greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 05:19:47PM -0500, Theodore Y. Ts'o wrote: > On Thu, Nov 08, 2018 at 01:04:49PM -0800, Greg KH wrote: > > > (Also note that even with fast SSD's and/or everything in page cache, > > > runnning "tag --contains " will take a good 3-4 seconds, and > > > if the git packs are not in the page cache, and/or you're unfortunate > > > enough to have your git trees on an HDD it's not pretty.) > > > > I recommend the "static cache" or whatever that thing is called, that > > helps out a _LOT_ with stuff like this. For the kernel tree, which is > > never rebased, it speeds up this so much. > > At the risk of asking a stupid question, which cache is this? I don't > think it's the untrackedCache; is it the BitmapHashCache? It's the 'commitGraph', here's the article I was trying to remember I learned this from: https://blogs.msdn.microsoft.com/devops/2018/06/25/supercharging-the-git-commit-graph/ greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 01:04:49PM -0800, Greg KH wrote: > > (Also note that even with fast SSD's and/or everything in page cache, > > runnning "tag --contains " will take a good 3-4 seconds, and > > if the git packs are not in the page cache, and/or you're unfortunate > > enough to have your git trees on an HDD it's not pretty.) > > I recommend the "static cache" or whatever that thing is called, that > helps out a _LOT_ with stuff like this. For the kernel tree, which is > never rebased, it speeds up this so much. At the risk of asking a stupid question, which cache is this? I don't think it's the untrackedCache; is it the BitmapHashCache? Thanks, - Ted
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 01:04:49PM -0800, Greg KH wrote: > > (Also note that even with fast SSD's and/or everything in page cache, > > runnning "tag --contains " will take a good 3-4 seconds, and > > if the git packs are not in the page cache, and/or you're unfortunate > > enough to have your git trees on an HDD it's not pretty.) > > I recommend the "static cache" or whatever that thing is called, that > helps out a _LOT_ with stuff like this. For the kernel tree, which is > never rebased, it speeds up this so much. At the risk of asking a stupid question, which cache is this? I don't think it's the untrackedCache; is it the BitmapHashCache? Thanks, - Ted
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 01:06:15PM -0800, Greg KH wrote:
> On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> > On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> > > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > > > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra
> > > > wrote:
> > > > >
> > > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > > > + - Cc: ``cc-ed-person ``
> > > > > > > +
> > > > > > > + If the patch should be backported to stable, then please add
> > > > > > > a '``Cc:
> > > > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when
> > > > > > > sending your
> > > > > > > + mail.
> > > > > >
> > > > > > Can I suggest a more canonical form:
> > > > > >
> > > > > > Cc: # v4.18 and later kernels
> > > > > >
> > > > > > It would be nice if people adding Cc: stable lines would actually
> > > > > > try to
> > > > > > figure out which exact kernel versions are affected.
> > > >
> > > > I know at least StGit mail does not grok that "#"notation. I've
> > > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > > > preferred over "# " if only because it can be used to track
> > > > fixes to commits that have been backported to stable. Is there any
> > > > reason for "# " to continue in a world where we have "Fixes:"?
> > >
> > > I sometimes have fixes that need to be different for different past
> > > releases. And there have been cases where RCU patches would apply and
> > > build cleanly against releases for which it was not appropriate, but
> > > would have some low-probability failure. Which meant that it could be
> > > expected to pass light testing. :-/
> > >
> > > So I sometimes need a way of saying which versions a given patch applies
> > > to, independent of the version into which the bug was introduced.
> >
> > I can understand that you want to limit the scope of automatic backports.
> >
> > But we really should try to always use of the Fixes: tag. In most cases the
> > SHA1 of the commit in the fixes tag defines the backport scope.
> >
> > For the rare cases where the buggy commit is really old, but you want to
> > limit the backport scope for a reason then I really like to avoid to
> > overload the Cc stable tag and have a dedicated tag instead. Something
> > like:
> >
> > Fixes: 1234567890AB ("subsys/comp: Short summary")
> > Backport-to: 4.14
>
> Ick, no. Just stick to the "Fixes:" tag. My scripts can now track when
> a patch is backported to a stable tree so that I know to apply it to
> older ones despite the original patch showing up in a newer release.
>
> And yes, those scripts are new, as Sasha is about to point out all of
> the places where I missed this in the past :)
Here's the script if others are curious:
https://github.com/gregkh/gregkh-linux/blob/master/scripts/fix_in_what_release
Yes, I know it's horrid, I abuse the fact that 'git grep' is very fast
on the stable-queue repo :)
thanks,
greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 01:06:15PM -0800, Greg KH wrote:
> On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> > On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> > > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > > > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra
> > > > wrote:
> > > > >
> > > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > > > + - Cc: ``cc-ed-person ``
> > > > > > > +
> > > > > > > + If the patch should be backported to stable, then please add
> > > > > > > a '``Cc:
> > > > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when
> > > > > > > sending your
> > > > > > > + mail.
> > > > > >
> > > > > > Can I suggest a more canonical form:
> > > > > >
> > > > > > Cc: # v4.18 and later kernels
> > > > > >
> > > > > > It would be nice if people adding Cc: stable lines would actually
> > > > > > try to
> > > > > > figure out which exact kernel versions are affected.
> > > >
> > > > I know at least StGit mail does not grok that "#"notation. I've
> > > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > > > preferred over "# " if only because it can be used to track
> > > > fixes to commits that have been backported to stable. Is there any
> > > > reason for "# " to continue in a world where we have "Fixes:"?
> > >
> > > I sometimes have fixes that need to be different for different past
> > > releases. And there have been cases where RCU patches would apply and
> > > build cleanly against releases for which it was not appropriate, but
> > > would have some low-probability failure. Which meant that it could be
> > > expected to pass light testing. :-/
> > >
> > > So I sometimes need a way of saying which versions a given patch applies
> > > to, independent of the version into which the bug was introduced.
> >
> > I can understand that you want to limit the scope of automatic backports.
> >
> > But we really should try to always use of the Fixes: tag. In most cases the
> > SHA1 of the commit in the fixes tag defines the backport scope.
> >
> > For the rare cases where the buggy commit is really old, but you want to
> > limit the backport scope for a reason then I really like to avoid to
> > overload the Cc stable tag and have a dedicated tag instead. Something
> > like:
> >
> > Fixes: 1234567890AB ("subsys/comp: Short summary")
> > Backport-to: 4.14
>
> Ick, no. Just stick to the "Fixes:" tag. My scripts can now track when
> a patch is backported to a stable tree so that I know to apply it to
> older ones despite the original patch showing up in a newer release.
>
> And yes, those scripts are new, as Sasha is about to point out all of
> the places where I missed this in the past :)
Here's the script if others are curious:
https://github.com/gregkh/gregkh-linux/blob/master/scripts/fix_in_what_release
Yes, I know it's horrid, I abuse the fact that 'git grep' is very fast
on the stable-queue repo :)
thanks,
greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra
> > > wrote:
> > > >
> > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > > + - Cc: ``cc-ed-person ``
> > > > > > +
> > > > > > + If the patch should be backported to stable, then please add a
> > > > > > '``Cc:
> > > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when
> > > > > > sending your
> > > > > > + mail.
> > > > >
> > > > > Can I suggest a more canonical form:
> > > > >
> > > > > Cc: # v4.18 and later kernels
> > > > >
> > > > > It would be nice if people adding Cc: stable lines would actually try
> > > > > to
> > > > > figure out which exact kernel versions are affected.
> > >
> > > I know at least StGit mail does not grok that "#"notation. I've
> > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > > preferred over "# " if only because it can be used to track
> > > fixes to commits that have been backported to stable. Is there any
> > > reason for "# " to continue in a world where we have "Fixes:"?
> >
> > I sometimes have fixes that need to be different for different past
> > releases. And there have been cases where RCU patches would apply and
> > build cleanly against releases for which it was not appropriate, but
> > would have some low-probability failure. Which meant that it could be
> > expected to pass light testing. :-/
> >
> > So I sometimes need a way of saying which versions a given patch applies
> > to, independent of the version into which the bug was introduced.
>
> I can understand that you want to limit the scope of automatic backports.
>
> But we really should try to always use of the Fixes: tag. In most cases the
> SHA1 of the commit in the fixes tag defines the backport scope.
>
> For the rare cases where the buggy commit is really old, but you want to
> limit the backport scope for a reason then I really like to avoid to
> overload the Cc stable tag and have a dedicated tag instead. Something
> like:
>
> Fixes: 1234567890AB ("subsys/comp: Short summary")
> Backport-to: 4.14
Ick, no. Just stick to the "Fixes:" tag. My scripts can now track when
a patch is backported to a stable tree so that I know to apply it to
older ones despite the original patch showing up in a newer release.
And yes, those scripts are new, as Sasha is about to point out all of
the places where I missed this in the past :)
thanks,
greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra
> > > wrote:
> > > >
> > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > > + - Cc: ``cc-ed-person ``
> > > > > > +
> > > > > > + If the patch should be backported to stable, then please add a
> > > > > > '``Cc:
> > > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when
> > > > > > sending your
> > > > > > + mail.
> > > > >
> > > > > Can I suggest a more canonical form:
> > > > >
> > > > > Cc: # v4.18 and later kernels
> > > > >
> > > > > It would be nice if people adding Cc: stable lines would actually try
> > > > > to
> > > > > figure out which exact kernel versions are affected.
> > >
> > > I know at least StGit mail does not grok that "#"notation. I've
> > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > > preferred over "# " if only because it can be used to track
> > > fixes to commits that have been backported to stable. Is there any
> > > reason for "# " to continue in a world where we have "Fixes:"?
> >
> > I sometimes have fixes that need to be different for different past
> > releases. And there have been cases where RCU patches would apply and
> > build cleanly against releases for which it was not appropriate, but
> > would have some low-probability failure. Which meant that it could be
> > expected to pass light testing. :-/
> >
> > So I sometimes need a way of saying which versions a given patch applies
> > to, independent of the version into which the bug was introduced.
>
> I can understand that you want to limit the scope of automatic backports.
>
> But we really should try to always use of the Fixes: tag. In most cases the
> SHA1 of the commit in the fixes tag defines the backport scope.
>
> For the rare cases where the buggy commit is really old, but you want to
> limit the backport scope for a reason then I really like to avoid to
> overload the Cc stable tag and have a dedicated tag instead. Something
> like:
>
> Fixes: 1234567890AB ("subsys/comp: Short summary")
> Backport-to: 4.14
Ick, no. Just stick to the "Fixes:" tag. My scripts can now track when
a patch is backported to a stable tree so that I know to apply it to
older ones despite the original patch showing up in a newer release.
And yes, those scripts are new, as Sasha is about to point out all of
the places where I missed this in the past :)
thanks,
greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 03:14:25PM -0500, Theodore Y. Ts'o wrote: > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > > > I know at least StGit mail does not grok that "#"notation. I've > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > > preferred over "# " if only because it can be used to track > > fixes to commits that have been backported to stable. Is there any > > reason for "# " to continue in a world where we have "Fixes:"? > > The main annoyance I have with Fixes is because it can be a pain to > figure out what the "# " would be. Something like: > > % tag --contains DEADBEEF | grep ^v | head > > doesn't work because kernel version numbers don't sort obviously. So > v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't > do the right. > > I suppose it wouldn't be that hard to write a perl/python script that > correctly sorts kernel version numbers, but when the "# " is > present, I find it convenient. 'sort -V' should help you out here, no need to write anything new :) > (Also note that even with fast SSD's and/or everything in page cache, > runnning "tag --contains " will take a good 3-4 seconds, and > if the git packs are not in the page cache, and/or you're unfortunate > enough to have your git trees on an HDD it's not pretty.) I recommend the "static cache" or whatever that thing is called, that helps out a _LOT_ with stuff like this. For the kernel tree, which is never rebased, it speeds up this so much. thanks, greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 03:14:25PM -0500, Theodore Y. Ts'o wrote: > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > > > I know at least StGit mail does not grok that "#"notation. I've > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > > preferred over "# " if only because it can be used to track > > fixes to commits that have been backported to stable. Is there any > > reason for "# " to continue in a world where we have "Fixes:"? > > The main annoyance I have with Fixes is because it can be a pain to > figure out what the "# " would be. Something like: > > % tag --contains DEADBEEF | grep ^v | head > > doesn't work because kernel version numbers don't sort obviously. So > v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't > do the right. > > I suppose it wouldn't be that hard to write a perl/python script that > correctly sorts kernel version numbers, but when the "# " is > present, I find it convenient. 'sort -V' should help you out here, no need to write anything new :) > (Also note that even with fast SSD's and/or everything in page cache, > runnning "tag --contains " will take a good 3-4 seconds, and > if the git packs are not in the page cache, and/or you're unfortunate > enough to have your git trees on an HDD it's not pretty.) I recommend the "static cache" or whatever that thing is called, that helps out a _LOT_ with stuff like this. For the kernel tree, which is never rebased, it speeds up this so much. thanks, greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, 8 Nov 2018, Theodore Y. Ts'o wrote: > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > > > I know at least StGit mail does not grok that "#"notation. I've > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > > preferred over "# " if only because it can be used to track > > fixes to commits that have been backported to stable. Is there any > > reason for "# " to continue in a world where we have "Fixes:"? > > The main annoyance I have with Fixes is because it can be a pain to > figure out what the "# " would be. Something like: > > % tag --contains DEADBEEF | grep ^v | head > > doesn't work because kernel version numbers don't sort obviously. So > v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't > do the right. > > I suppose it wouldn't be that hard to write a perl/python script that > correctly sorts kernel version numbers, but when the "# " is > present, I find it convenient. > > (Also note that even with fast SSD's and/or everything in page cache, > runnning "tag --contains " will take a good 3-4 seconds, and > if the git packs are not in the page cache, and/or you're unfortunate > enough to have your git trees on an HDD it's not pretty.) Fair enough. But as I said before we please let us have both the fixes tag and a decicated backport-to one and make the latter mandatory. It if's not there and the patch has a 'Cc: stable' tag it's easy enough to reject it. The the submitter or the maintainer who decides that a patch needs to be backported to stable has to wait the 4 seconds and add that information. Thanks, tglx
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, 8 Nov 2018, Theodore Y. Ts'o wrote: > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > > > I know at least StGit mail does not grok that "#"notation. I've > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > > preferred over "# " if only because it can be used to track > > fixes to commits that have been backported to stable. Is there any > > reason for "# " to continue in a world where we have "Fixes:"? > > The main annoyance I have with Fixes is because it can be a pain to > figure out what the "# " would be. Something like: > > % tag --contains DEADBEEF | grep ^v | head > > doesn't work because kernel version numbers don't sort obviously. So > v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't > do the right. > > I suppose it wouldn't be that hard to write a perl/python script that > correctly sorts kernel version numbers, but when the "# " is > present, I find it convenient. > > (Also note that even with fast SSD's and/or everything in page cache, > runnning "tag --contains " will take a good 3-4 seconds, and > if the git packs are not in the page cache, and/or you're unfortunate > enough to have your git trees on an HDD it's not pretty.) Fair enough. But as I said before we please let us have both the fixes tag and a decicated backport-to one and make the latter mandatory. It if's not there and the patch has a 'Cc: stable' tag it's easy enough to reject it. The the submitter or the maintainer who decides that a patch needs to be backported to stable has to wait the 4 seconds and add that information. Thanks, tglx
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > I know at least StGit mail does not grok that "#"notation. I've > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > preferred over "# " if only because it can be used to track > fixes to commits that have been backported to stable. Is there any > reason for "# " to continue in a world where we have "Fixes:"? The main annoyance I have with Fixes is because it can be a pain to figure out what the "# " would be. Something like: % tag --contains DEADBEEF | grep ^v | head doesn't work because kernel version numbers don't sort obviously. So v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't do the right. I suppose it wouldn't be that hard to write a perl/python script that correctly sorts kernel version numbers, but when the "# " is present, I find it convenient. (Also note that even with fast SSD's and/or everything in page cache, runnning "tag --contains " will take a good 3-4 seconds, and if the git packs are not in the page cache, and/or you're unfortunate enough to have your git trees on an HDD it's not pretty.) - Ted
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > > I know at least StGit mail does not grok that "#"notation. I've > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > preferred over "# " if only because it can be used to track > fixes to commits that have been backported to stable. Is there any > reason for "# " to continue in a world where we have "Fixes:"? The main annoyance I have with Fixes is because it can be a pain to figure out what the "# " would be. Something like: % tag --contains DEADBEEF | grep ^v | head doesn't work because kernel version numbers don't sort obviously. So v4.10 comes before v4.3 using a normal sort, and even sort -n doesn't do the right. I suppose it wouldn't be that hard to write a perl/python script that correctly sorts kernel version numbers, but when the "# " is present, I find it convenient. (Also note that even with fast SSD's and/or everything in page cache, runnning "tag --contains " will take a good 3-4 seconds, and if the git packs are not in the page cache, and/or you're unfortunate enough to have your git trees on an HDD it's not pretty.) - Ted
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra
> > > wrote:
> > > >
> > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > > + - Cc: ``cc-ed-person ``
> > > > > > +
> > > > > > + If the patch should be backported to stable, then please add a
> > > > > > '``Cc:
> > > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when
> > > > > > sending your
> > > > > > + mail.
> > > > >
> > > > > Can I suggest a more canonical form:
> > > > >
> > > > > Cc: # v4.18 and later kernels
> > > > >
> > > > > It would be nice if people adding Cc: stable lines would actually try
> > > > > to
> > > > > figure out which exact kernel versions are affected.
> > >
> > > I know at least StGit mail does not grok that "#"notation. I've
> > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > > preferred over "# " if only because it can be used to track
> > > fixes to commits that have been backported to stable. Is there any
> > > reason for "# " to continue in a world where we have "Fixes:"?
> >
> > I sometimes have fixes that need to be different for different past
> > releases. And there have been cases where RCU patches would apply and
> > build cleanly against releases for which it was not appropriate, but
> > would have some low-probability failure. Which meant that it could be
> > expected to pass light testing. :-/
> >
> > So I sometimes need a way of saying which versions a given patch applies
> > to, independent of the version into which the bug was introduced.
>
> I can understand that you want to limit the scope of automatic backports.
>
> But we really should try to always use of the Fixes: tag. In most cases the
> SHA1 of the commit in the fixes tag defines the backport scope.
>
> For the rare cases where the buggy commit is really old, but you want to
> limit the backport scope for a reason then I really like to avoid to
> overload the Cc stable tag and have a dedicated tag instead. Something
> like:
>
> Fixes: 1234567890AB ("subsys/comp: Short summary")
> Backport-to: 4.14
>
> and have that backport tag right under the Fixes tag. If the Backport-to
> tag is ommitted, the SHA1 defines the scope, but I'm fine with making it
> mandatory.
>
> If there is really the special RCU case where each and every stable version
> needs some special treatment then say:
>
> Backport-to: Manual
>
> or whatever sensible word would express it correctly.
>
> The Fixes tag is really valuable when you need to make connections and I
> know that the people who are looking into safety-critical Linux value the
> tag because it can be used for tracking and for metrics.
Indeed, I do need to get my act together with the Fixes tag. And I am
happy with whatever format would limit backports appropriately.
Thanx, Paul
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 08:58:32PM +0100, Thomas Gleixner wrote:
> On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> > On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra
> > > wrote:
> > > >
> > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > > + - Cc: ``cc-ed-person ``
> > > > > > +
> > > > > > + If the patch should be backported to stable, then please add a
> > > > > > '``Cc:
> > > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when
> > > > > > sending your
> > > > > > + mail.
> > > > >
> > > > > Can I suggest a more canonical form:
> > > > >
> > > > > Cc: # v4.18 and later kernels
> > > > >
> > > > > It would be nice if people adding Cc: stable lines would actually try
> > > > > to
> > > > > figure out which exact kernel versions are affected.
> > >
> > > I know at least StGit mail does not grok that "#"notation. I've
> > > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > > preferred over "# " if only because it can be used to track
> > > fixes to commits that have been backported to stable. Is there any
> > > reason for "# " to continue in a world where we have "Fixes:"?
> >
> > I sometimes have fixes that need to be different for different past
> > releases. And there have been cases where RCU patches would apply and
> > build cleanly against releases for which it was not appropriate, but
> > would have some low-probability failure. Which meant that it could be
> > expected to pass light testing. :-/
> >
> > So I sometimes need a way of saying which versions a given patch applies
> > to, independent of the version into which the bug was introduced.
>
> I can understand that you want to limit the scope of automatic backports.
>
> But we really should try to always use of the Fixes: tag. In most cases the
> SHA1 of the commit in the fixes tag defines the backport scope.
>
> For the rare cases where the buggy commit is really old, but you want to
> limit the backport scope for a reason then I really like to avoid to
> overload the Cc stable tag and have a dedicated tag instead. Something
> like:
>
> Fixes: 1234567890AB ("subsys/comp: Short summary")
> Backport-to: 4.14
>
> and have that backport tag right under the Fixes tag. If the Backport-to
> tag is ommitted, the SHA1 defines the scope, but I'm fine with making it
> mandatory.
>
> If there is really the special RCU case where each and every stable version
> needs some special treatment then say:
>
> Backport-to: Manual
>
> or whatever sensible word would express it correctly.
>
> The Fixes tag is really valuable when you need to make connections and I
> know that the people who are looking into safety-critical Linux value the
> tag because it can be used for tracking and for metrics.
Indeed, I do need to get my act together with the Fixes tag. And I am
happy with whatever format would limit backports appropriately.
Thanx, Paul
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra wrote:
> > >
> > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > + - Cc: ``cc-ed-person ``
> > > > > +
> > > > > + If the patch should be backported to stable, then please add a
> > > > > '``Cc:
> > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending
> > > > > your
> > > > > + mail.
> > > >
> > > > Can I suggest a more canonical form:
> > > >
> > > > Cc: # v4.18 and later kernels
> > > >
> > > > It would be nice if people adding Cc: stable lines would actually try to
> > > > figure out which exact kernel versions are affected.
> >
> > I know at least StGit mail does not grok that "#"notation. I've
> > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > preferred over "# " if only because it can be used to track
> > fixes to commits that have been backported to stable. Is there any
> > reason for "# " to continue in a world where we have "Fixes:"?
>
> I sometimes have fixes that need to be different for different past
> releases. And there have been cases where RCU patches would apply and
> build cleanly against releases for which it was not appropriate, but
> would have some low-probability failure. Which meant that it could be
> expected to pass light testing. :-/
>
> So I sometimes need a way of saying which versions a given patch applies
> to, independent of the version into which the bug was introduced.
I can understand that you want to limit the scope of automatic backports.
But we really should try to always use of the Fixes: tag. In most cases the
SHA1 of the commit in the fixes tag defines the backport scope.
For the rare cases where the buggy commit is really old, but you want to
limit the backport scope for a reason then I really like to avoid to
overload the Cc stable tag and have a dedicated tag instead. Something
like:
Fixes: 1234567890AB ("subsys/comp: Short summary")
Backport-to: 4.14
and have that backport tag right under the Fixes tag. If the Backport-to
tag is ommitted, the SHA1 defines the scope, but I'm fine with making it
mandatory.
If there is really the special RCU case where each and every stable version
needs some special treatment then say:
Backport-to: Manual
or whatever sensible word would express it correctly.
The Fixes tag is really valuable when you need to make connections and I
know that the people who are looking into safety-critical Linux value the
tag because it can be used for tracking and for metrics.
Thanks,
tglx
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, 8 Nov 2018, Paul E. McKenney wrote:
> On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote:
> > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra wrote:
> > >
> > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote:
> > > > > + - Cc: ``cc-ed-person ``
> > > > > +
> > > > > + If the patch should be backported to stable, then please add a
> > > > > '``Cc:
> > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending
> > > > > your
> > > > > + mail.
> > > >
> > > > Can I suggest a more canonical form:
> > > >
> > > > Cc: # v4.18 and later kernels
> > > >
> > > > It would be nice if people adding Cc: stable lines would actually try to
> > > > figure out which exact kernel versions are affected.
> >
> > I know at least StGit mail does not grok that "#"notation. I've
> > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is
> > preferred over "# " if only because it can be used to track
> > fixes to commits that have been backported to stable. Is there any
> > reason for "# " to continue in a world where we have "Fixes:"?
>
> I sometimes have fixes that need to be different for different past
> releases. And there have been cases where RCU patches would apply and
> build cleanly against releases for which it was not appropriate, but
> would have some low-probability failure. Which meant that it could be
> expected to pass light testing. :-/
>
> So I sometimes need a way of saying which versions a given patch applies
> to, independent of the version into which the bug was introduced.
I can understand that you want to limit the scope of automatic backports.
But we really should try to always use of the Fixes: tag. In most cases the
SHA1 of the commit in the fixes tag defines the backport scope.
For the rare cases where the buggy commit is really old, but you want to
limit the backport scope for a reason then I really like to avoid to
overload the Cc stable tag and have a dedicated tag instead. Something
like:
Fixes: 1234567890AB ("subsys/comp: Short summary")
Backport-to: 4.14
and have that backport tag right under the Fixes tag. If the Backport-to
tag is ommitted, the SHA1 defines the scope, but I'm fine with making it
mandatory.
If there is really the special RCU case where each and every stable version
needs some special treatment then say:
Backport-to: Manual
or whatever sensible word would express it correctly.
The Fixes tag is really valuable when you need to make connections and I
know that the people who are looking into safety-critical Linux value the
tag because it can be used for tracking and for metrics.
Thanks,
tglx
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra wrote: > > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > > > + - Cc: ``cc-ed-person `` > > > > + > > > > + If the patch should be backported to stable, then please add a > > > > '``Cc: > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending > > > > your > > > > + mail. > > > > > > Can I suggest a more canonical form: > > > > > > Cc: # v4.18 and later kernels > > > > > > It would be nice if people adding Cc: stable lines would actually try to > > > figure out which exact kernel versions are affected. > > I know at least StGit mail does not grok that "#"notation. I've > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > preferred over "# " if only because it can be used to track > fixes to commits that have been backported to stable. Is there any > reason for "# " to continue in a world where we have "Fixes:"? I sometimes have fixes that need to be different for different past releases. And there have been cases where RCU patches would apply and build cleanly against releases for which it was not appropriate, but would have some low-probability failure. Which meant that it could be expected to pass light testing. :-/ So I sometimes need a way of saying which versions a given patch applies to, independent of the version into which the bug was introduced. Thanx, Paul
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra wrote: > > > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > > > + - Cc: ``cc-ed-person `` > > > > + > > > > + If the patch should be backported to stable, then please add a > > > > '``Cc: > > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending > > > > your > > > > + mail. > > > > > > Can I suggest a more canonical form: > > > > > > Cc: # v4.18 and later kernels > > > > > > It would be nice if people adding Cc: stable lines would actually try to > > > figure out which exact kernel versions are affected. > > I know at least StGit mail does not grok that "#"notation. I've > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > preferred over "# " if only because it can be used to track > fixes to commits that have been backported to stable. Is there any > reason for "# " to continue in a world where we have "Fixes:"? I sometimes have fixes that need to be different for different past releases. And there have been cases where RCU patches would apply and build cleanly against releases for which it was not appropriate, but would have some low-probability failure. Which meant that it could be expected to pass light testing. :-/ So I sometimes need a way of saying which versions a given patch applies to, independent of the version into which the bug was introduced. Thanx, Paul
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > I know at least StGit mail does not grok that "#"notation. I've > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > preferred over "# " if only because it can be used to track > fixes to commits that have been backported to stable. Yeah, FWIW, we do that for our SLES kernels. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 09:19:33AM -0800, Dan Williams wrote: > I know at least StGit mail does not grok that "#"notation. I've > stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is > preferred over "# " if only because it can be used to track > fixes to commits that have been backported to stable. Yeah, FWIW, we do that for our SLES kernels. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra wrote: > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > > + - Cc: ``cc-ed-person `` > > > + > > > + If the patch should be backported to stable, then please add a '``Cc: > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > > > + mail. > > > > Can I suggest a more canonical form: > > > > Cc: # v4.18 and later kernels > > > > It would be nice if people adding Cc: stable lines would actually try to > > figure out which exact kernel versions are affected. I know at least StGit mail does not grok that "#"notation. I've stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is preferred over "# " if only because it can be used to track fixes to commits that have been backported to stable. Is there any reason for "# " to continue in a world where we have "Fixes:"?
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 8, 2018 at 1:13 AM Peter Zijlstra wrote: > > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > > + - Cc: ``cc-ed-person `` > > > + > > > + If the patch should be backported to stable, then please add a '``Cc: > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > > > + mail. > > > > Can I suggest a more canonical form: > > > > Cc: # v4.18 and later kernels > > > > It would be nice if people adding Cc: stable lines would actually try to > > figure out which exact kernel versions are affected. I know at least StGit mail does not grok that "#"notation. I've stopped using it in favor of a "Fixes:" tag. I would think "Fixes:" is preferred over "# " if only because it can be used to track fixes to commits that have been backported to stable. Is there any reason for "# " to continue in a world where we have "Fixes:"?
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 10:12:51AM +0100, Peter Zijlstra wrote: > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > > + - Cc: ``cc-ed-person `` > > > + > > > + If the patch should be backported to stable, then please add a '``Cc: > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > > > + mail. > > > > Can I suggest a more canonical form: > > > > Cc: # v4.18 and later kernels > > > > It would be nice if people adding Cc: stable lines would actually try to > > figure out which exact kernel versions are affected. > > I think Greg actually prefers we use sta...@kernel.org, which is a > /dev/null target. His bot will then pick up on the tag once it hits > Linus' tree. I really do not care either way what address is used. Both work fine. thanks, greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 10:12:51AM +0100, Peter Zijlstra wrote: > On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > > + - Cc: ``cc-ed-person `` > > > + > > > + If the patch should be backported to stable, then please add a '``Cc: > > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > > > + mail. > > > > Can I suggest a more canonical form: > > > > Cc: # v4.18 and later kernels > > > > It would be nice if people adding Cc: stable lines would actually try to > > figure out which exact kernel versions are affected. > > I think Greg actually prefers we use sta...@kernel.org, which is a > /dev/null target. His bot will then pick up on the tag once it hits > Linus' tree. I really do not care either way what address is used. Both work fine. thanks, greg k-h
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > + - Cc: ``cc-ed-person `` > > + > > + If the patch should be backported to stable, then please add a '``Cc: > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > > + mail. > > Can I suggest a more canonical form: > > Cc: # v4.18 and later kernels > > It would be nice if people adding Cc: stable lines would actually try to > figure out which exact kernel versions are affected. I think Greg actually prefers we use sta...@kernel.org, which is a /dev/null target. His bot will then pick up on the tag once it hits Linus' tree.
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Thu, Nov 08, 2018 at 08:40:12AM +0100, Ingo Molnar wrote: > > + - Cc: ``cc-ed-person `` > > + > > + If the patch should be backported to stable, then please add a '``Cc: > > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > > + mail. > > Can I suggest a more canonical form: > > Cc: # v4.18 and later kernels > > It would be nice if people adding Cc: stable lines would actually try to > figure out which exact kernel versions are affected. I think Greg actually prefers we use sta...@kernel.org, which is a /dev/null target. His bot will then pick up on the tag once it hits Linus' tree.
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> +Line breaks
> +^^^
> +
> +Restricting line length to 80 characters makes deeply indented code hard to
> +read. Consider breaking out code into helper functions to avoid excessive
> +line breaking.
> +
> +The 80 character rule is not a strict rule, so please use common sense when
> +breaking lines. Especially format strings should never be broken up.
Might make sense to explain that:
+ The reason for that rule is that if for example we have this printk line:
+
+ if (uh_oh()) {
+ pr_info("Something really bad happened, danger"
+ "danger, blue smoke reported!\n");
+ }
+
+ People would see this message in the syslog:
+
+ Thu Nov 8 09:22:33: Something really bad happened, dangerdanger, blue
smoke reported!
+
+ And chances are that in sheer panic they'd type the most distinctive
+ part of that text as a search pattern for the kernel source tree:
+
+ $ git grep -i 'dangerdanger'
+ $
+
+ ... and they'd get absolutely no match on that string due to the
+ col80 broken format string, and confusion and frustration would rise,
+ in addition to growing amounts of blue smoke.
+
+ We don't want that, so just write out the single line:
+ if (uh_oh())
+ pr_info("Something really bad happened, danger danger,
blue smoke reported!\n");
+
+ Also note two other advantages of writing it like this:
+
+ - We saved two curly braces.
+ - We also added a proper space to 'danger danger' which was the original
intended message.
?
> +
> +When splitting function declarations or function calls, then please align
> +the first argument in the second line with the first argument in the first
> +line::
> +
> + static int long_function_name(struct foobar *barfoo, unsigned int id,
> + unsigned int offset)
> + {
> +
> + if (!id) {
> + ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID,
> +offset);
BTW., in this particular case I think small violations of col80 rule are
even more readable, i.e.:
> + ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID, offset);
And note that in this example we used 78 colums so we didn't even violate
the col80 rule. ;-)
Thanks,
Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> +Line breaks
> +^^^
> +
> +Restricting line length to 80 characters makes deeply indented code hard to
> +read. Consider breaking out code into helper functions to avoid excessive
> +line breaking.
> +
> +The 80 character rule is not a strict rule, so please use common sense when
> +breaking lines. Especially format strings should never be broken up.
Might make sense to explain that:
+ The reason for that rule is that if for example we have this printk line:
+
+ if (uh_oh()) {
+ pr_info("Something really bad happened, danger"
+ "danger, blue smoke reported!\n");
+ }
+
+ People would see this message in the syslog:
+
+ Thu Nov 8 09:22:33: Something really bad happened, dangerdanger, blue
smoke reported!
+
+ And chances are that in sheer panic they'd type the most distinctive
+ part of that text as a search pattern for the kernel source tree:
+
+ $ git grep -i 'dangerdanger'
+ $
+
+ ... and they'd get absolutely no match on that string due to the
+ col80 broken format string, and confusion and frustration would rise,
+ in addition to growing amounts of blue smoke.
+
+ We don't want that, so just write out the single line:
+ if (uh_oh())
+ pr_info("Something really bad happened, danger danger,
blue smoke reported!\n");
+
+ Also note two other advantages of writing it like this:
+
+ - We saved two curly braces.
+ - We also added a proper space to 'danger danger' which was the original
intended message.
?
> +
> +When splitting function declarations or function calls, then please align
> +the first argument in the second line with the first argument in the first
> +line::
> +
> + static int long_function_name(struct foobar *barfoo, unsigned int id,
> + unsigned int offset)
> + {
> +
> + if (!id) {
> + ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID,
> +offset);
BTW., in this particular case I think small violations of col80 rule are
even more readable, i.e.:
> + ret = longer_function_name(barfoo, DEFAULT_BARFOO_ID, offset);
And note that in this example we used 78 colums so we didn't even violate
the col80 rule. ;-)
Thanks,
Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > +Commit notifications > + > + > +The tip tree is monitored by a bot for new commits. The bot sends an email > +for each new commit to a dedicated mailing list > +(``linux-tip-comm...@vger.kernel.org``) and Cc's all people who are > +mentioned in one of the commit tags. It uses the email message id from the > +Link tag at the end of the tag list to set the In-Reply-To email header so > +the message is properly threaded with the patch submission email. s/id /ID > + > +The maintainers try to reply to the submitter when merging a patch, but > +they sometimes forget or it does not fit the workflow of the moment. While > +the bot message is purely mechanical assume it implies a 'Thank you! > +Applied.'. s/The maintainers /The -tip maintainers and submaintainers s/is purely mechanical assume it implies a 'Thank you! Applied.' /is purely mechanical, it also implies a 'Thank you! Applied.' message. ... added a missing comma, plus there's nothing to assume, that the thank-you note is implied is a given! :-) Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > +Commit notifications > + > + > +The tip tree is monitored by a bot for new commits. The bot sends an email > +for each new commit to a dedicated mailing list > +(``linux-tip-comm...@vger.kernel.org``) and Cc's all people who are > +mentioned in one of the commit tags. It uses the email message id from the > +Link tag at the end of the tag list to set the In-Reply-To email header so > +the message is properly threaded with the patch submission email. s/id /ID > + > +The maintainers try to reply to the submitter when merging a patch, but > +they sometimes forget or it does not fit the workflow of the moment. While > +the bot message is purely mechanical assume it implies a 'Thank you! > +Applied.'. s/The maintainers /The -tip maintainers and submaintainers s/is purely mechanical assume it implies a 'Thank you! Applied.' /is purely mechanical, it also implies a 'Thank you! Applied.' message. ... added a missing comma, plus there's nothing to assume, that the thank-you note is implied is a given! :-) Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > +Namespaces > +^^ > + > +To improve readability and to allow easy grepping for information the usage > +of consistent namespaces is important. The namespace should be used as a > +prefix for globally visible (inline) functions and variables. A simple rule > +for chosing a namespace prefix is to combine the subsystem and the > +component name, e.g. 'x86_comp\_', 'sched\_', 'irq\_', 'mutex\_', etc. For > +static functions and variables the namespace prefix can be omitted. > + > +Also be careful when using vendor names in namespaces. There is no value of > +having 'xxx_vendor\_' or 'vendor_xxx_` as prefix for all static functions > +of a vendor specific file as it is already clear that the code is vendor > +specific. Aside of that vendor names should only be used when it is really > +vendor specific functionality. > + > +As always apply common sense and aim for consistency and readability. I'd also suggest adding: > +This also includes static file scope functions that are immediately put into > +globally visible driver templates - it's useful for those symbols to carry a > +good prefix as well, for backtrace readability. > + > +Truly local functions, only called by other local functions, can have > +shorter descriptive names - our primary concern is greppability and > +backtrace readability. Beyond the driver-template aspect also note the backtrace readability argument I added: good namespaces are not just about greppability. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > +Namespaces > +^^ > + > +To improve readability and to allow easy grepping for information the usage > +of consistent namespaces is important. The namespace should be used as a > +prefix for globally visible (inline) functions and variables. A simple rule > +for chosing a namespace prefix is to combine the subsystem and the > +component name, e.g. 'x86_comp\_', 'sched\_', 'irq\_', 'mutex\_', etc. For > +static functions and variables the namespace prefix can be omitted. > + > +Also be careful when using vendor names in namespaces. There is no value of > +having 'xxx_vendor\_' or 'vendor_xxx_` as prefix for all static functions > +of a vendor specific file as it is already clear that the code is vendor > +specific. Aside of that vendor names should only be used when it is really > +vendor specific functionality. > + > +As always apply common sense and aim for consistency and readability. I'd also suggest adding: > +This also includes static file scope functions that are immediately put into > +globally visible driver templates - it's useful for those symbols to carry a > +good prefix as well, for backtrace readability. > + > +Truly local functions, only called by other local functions, can have > +shorter descriptive names - our primary concern is greppability and > +backtrace readability. Beyond the driver-template aspect also note the backtrace readability argument I added: good namespaces are not just about greppability. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> +Variable types
> +^^
> +
> +Please use the proper u8, u16, u32, u64 types for variables which are meant
> +to describe hardware or are used as arguments for functions which access
> +hardware. These types are clearly defining the bit width and avoid
> +truncation, expansion and 32/64 bit confusion.
> +
> +u64 is also recommended in code which would become ambiguous for 32bit when
> +'unsigned long' would be used instead. While in such situations 'unsigned
> +long long' could be used as well, u64 is shorter and also clearly shows
> +that the operation is required to be 64bit wide independent of the target
> +CPU.
> +
> +Please use 'unsigned int' instead of 'unsigned'.
s/for 32bit
/for 32-bit kernels
s/64bit wide
/64 bits wide
> +Constants
> +^
> +
> +Please do not use literal (hexa)decimal numbers in code or initializers.
> +Either use proper defines which have descriptive names or consider using
> +an enum.
I believe there should be an exception for 'obvious' literal values like
0 and 1.
I.e. the above is mostly a rule that is intended to cover undocumented
'magic' numbers.
I.e. how about this wording:
+Constants
+^
+
+Please do not use magic literal (hexa)decimal numbers when interfacing
+with hardware where the number has an unclear origin in code or
+initializers. I.e. "no magic numbers".
+
+Either use proper defines which have descriptive names or use an enum.
+
+Using obvious 0/1 literal values is fine in most cases.
?
> +
> +
> +Struct declarations and initializers
> +
> +
> +Struct declarations should align the struct member names in a tabular
> +fashion::
> +
> + struct bar_order {
> + unsigned intguest_id;
> + int ordered_item;
> + struct menu *menu;
> + };
> +
> +Please avoid documenting struct members within the declaration, because
> +this often results in strangely formatted comments and the struct members
> +become obfuscated::
> +
> + struct bar_order {
> + unsigned intguest_id; /* Unique guest id */
[ Sidenote: there's whitespace damage (extra spaces) in the text here. ]
> + int ordered_item;
> + /* Pointer to a menu instance which contains all the drinks */
> + struct menu *menu;
> + };
> +
> +Instead, please consider using the kernel-doc format in a comment preceding
> +the struct declaration, which is easier to read and has the added advantage
> +of including the information in the kernel documentation, for example, as
> +follows::
I disagree slightly here. While adding kernel-doc format is fine of
course, so are in-line comments which I frequently use.
This form is particularly helpful for more complex structures. Have a
look at 'struct fpu' for example:
/*
* Highest level per task FPU state data structure that
* contains the FPU register state plus various FPU
* state fields:
*/
struct fpu {
/*
* @last_cpu:
*
* Records the last CPU on which this context was loaded into
* FPU registers. (In the lazy-restore case we might be
* able to reuse FPU registers across multiple context switches
* this way, if no intermediate task used the FPU.)
*
* A value of -1 is used to indicate that the FPU state in context
* memory is newer than the FPU state in registers, and that the
* FPU state should be reloaded next time the task is run.
*/
unsigned intlast_cpu;
/*
* @initialized:
*
* This flag indicates whether this context is initialized: if the task
* is not running then we can restore from this context, if the task
* is running then we should save into this context.
*/
unsigned char initialized;
/*
* @state:
*
* In-memory copy of all FPU registers that we save/restore
* over context switches. If the task is using the FPU then
* the registers in the FPU are more recent than this state
* copy. If the task context-switches away then they get
* saved here and represent the FPU state.
*/
union fpregs_state state;
/*
* WARNING: 'state' is dynamically-sized. Do not put
* anything after it here.
*/
};
The in-line freestanding comments is perfectly structured and readable as
well, and this is analogous to the 'freestanding comments' style for C
statements.
We also have occasional examples where tail comments are fine, such as:
/*
* The legacy x87 FPU state format, as saved by FSAVE and
* restored by the FRSTOR instructions:
*/
struct fregs_state {
u32 cwd;/* FPU Control Word */
u32 swd;/* FPU
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> +Variable types
> +^^
> +
> +Please use the proper u8, u16, u32, u64 types for variables which are meant
> +to describe hardware or are used as arguments for functions which access
> +hardware. These types are clearly defining the bit width and avoid
> +truncation, expansion and 32/64 bit confusion.
> +
> +u64 is also recommended in code which would become ambiguous for 32bit when
> +'unsigned long' would be used instead. While in such situations 'unsigned
> +long long' could be used as well, u64 is shorter and also clearly shows
> +that the operation is required to be 64bit wide independent of the target
> +CPU.
> +
> +Please use 'unsigned int' instead of 'unsigned'.
s/for 32bit
/for 32-bit kernels
s/64bit wide
/64 bits wide
> +Constants
> +^
> +
> +Please do not use literal (hexa)decimal numbers in code or initializers.
> +Either use proper defines which have descriptive names or consider using
> +an enum.
I believe there should be an exception for 'obvious' literal values like
0 and 1.
I.e. the above is mostly a rule that is intended to cover undocumented
'magic' numbers.
I.e. how about this wording:
+Constants
+^
+
+Please do not use magic literal (hexa)decimal numbers when interfacing
+with hardware where the number has an unclear origin in code or
+initializers. I.e. "no magic numbers".
+
+Either use proper defines which have descriptive names or use an enum.
+
+Using obvious 0/1 literal values is fine in most cases.
?
> +
> +
> +Struct declarations and initializers
> +
> +
> +Struct declarations should align the struct member names in a tabular
> +fashion::
> +
> + struct bar_order {
> + unsigned intguest_id;
> + int ordered_item;
> + struct menu *menu;
> + };
> +
> +Please avoid documenting struct members within the declaration, because
> +this often results in strangely formatted comments and the struct members
> +become obfuscated::
> +
> + struct bar_order {
> + unsigned intguest_id; /* Unique guest id */
[ Sidenote: there's whitespace damage (extra spaces) in the text here. ]
> + int ordered_item;
> + /* Pointer to a menu instance which contains all the drinks */
> + struct menu *menu;
> + };
> +
> +Instead, please consider using the kernel-doc format in a comment preceding
> +the struct declaration, which is easier to read and has the added advantage
> +of including the information in the kernel documentation, for example, as
> +follows::
I disagree slightly here. While adding kernel-doc format is fine of
course, so are in-line comments which I frequently use.
This form is particularly helpful for more complex structures. Have a
look at 'struct fpu' for example:
/*
* Highest level per task FPU state data structure that
* contains the FPU register state plus various FPU
* state fields:
*/
struct fpu {
/*
* @last_cpu:
*
* Records the last CPU on which this context was loaded into
* FPU registers. (In the lazy-restore case we might be
* able to reuse FPU registers across multiple context switches
* this way, if no intermediate task used the FPU.)
*
* A value of -1 is used to indicate that the FPU state in context
* memory is newer than the FPU state in registers, and that the
* FPU state should be reloaded next time the task is run.
*/
unsigned intlast_cpu;
/*
* @initialized:
*
* This flag indicates whether this context is initialized: if the task
* is not running then we can restore from this context, if the task
* is running then we should save into this context.
*/
unsigned char initialized;
/*
* @state:
*
* In-memory copy of all FPU registers that we save/restore
* over context switches. If the task is using the FPU then
* the registers in the FPU are more recent than this state
* copy. If the task context-switches away then they get
* saved here and represent the FPU state.
*/
union fpregs_state state;
/*
* WARNING: 'state' is dynamically-sized. Do not put
* anything after it here.
*/
};
The in-line freestanding comments is perfectly structured and readable as
well, and this is analogous to the 'freestanding comments' style for C
statements.
We also have occasional examples where tail comments are fine, such as:
/*
* The legacy x87 FPU state format, as saved by FSAVE and
* restored by the FRSTOR instructions:
*/
struct fregs_state {
u32 cwd;/* FPU Control Word */
u32 swd;/* FPU
Re: [patch 2/2] Documentation/process: Add tip tree handbook
Lemme fill in the scheduler and locking/atomics bits as well: > +The tip tree contains the following subsystems: > + > + - **x86 architecture** > + > + The x86 architecture development takes place in the tip tree except > + for the x86 KVM and XEN specific parts which are maintained in the > + corresponding subsystems and routed directly to mainline from > + there. It's still good practice to Cc the x86 maintainers on > + x86-specific KVM and XEN patches. > + > + Some x86 subsystems have their own maintainers in addition to the > + overall x86 maintainers. Please Cc the overall x86 maintainers on > + patches touching files in arch/x86 even when they are not called out > + by the MAINTAINER file. > + > + Note, that ``x...@kernel.org`` is not a mailing list. It is merely a > + mail alias which distributes mails to the x86 top-level maintainer > + team. Please always Cc the Linux Kernel mailing list (LKML) > + ``linux-kernel@vger.kernel.org``, otherwise your mail ends up only in > + the private inboxes of the maintainers. > + > + - **Scheduler** Scheduler development takes place in the -tip tree, in the sched/core branch - with occasional sub-topic trees for work-in-progress patch-sets. > + > + - **Locking and atomics** Locking development (including atomics and other synchronization primitives that are connected to locking) takes place in the -tip tree, in the locking/core branch - with occasional sub-topic trees for work-in-progress patch-sets. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
Lemme fill in the scheduler and locking/atomics bits as well: > +The tip tree contains the following subsystems: > + > + - **x86 architecture** > + > + The x86 architecture development takes place in the tip tree except > + for the x86 KVM and XEN specific parts which are maintained in the > + corresponding subsystems and routed directly to mainline from > + there. It's still good practice to Cc the x86 maintainers on > + x86-specific KVM and XEN patches. > + > + Some x86 subsystems have their own maintainers in addition to the > + overall x86 maintainers. Please Cc the overall x86 maintainers on > + patches touching files in arch/x86 even when they are not called out > + by the MAINTAINER file. > + > + Note, that ``x...@kernel.org`` is not a mailing list. It is merely a > + mail alias which distributes mails to the x86 top-level maintainer > + team. Please always Cc the Linux Kernel mailing list (LKML) > + ``linux-kernel@vger.kernel.org``, otherwise your mail ends up only in > + the private inboxes of the maintainers. > + > + - **Scheduler** Scheduler development takes place in the -tip tree, in the sched/core branch - with occasional sub-topic trees for work-in-progress patch-sets. > + > + - **Locking and atomics** Locking development (including atomics and other synchronization primitives that are connected to locking) takes place in the -tip tree, in the locking/core branch - with occasional sub-topic trees for work-in-progress patch-sets. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > + - Signed-off-by: ``Patch handler `` > + > + SOBs after the author SOB are from people handling and transporting the > + patch, but were not involved in development. If the handler made > + modifications to the patch or the changelog, then this should be > + mentioned **after** the changelog text and **above** all commit tags in > + the following format:: > + > + ... changelog text ends. > + > + [ handler: Replaced foo by bar and updated changelog ] > + > + First-tag: . > + > + Note the two empty new lines which separate the changelog text and the > + commit tags from that notice. Even after a decade of introducing Git I still see Signed-off-by used as an Acked-by or Reviewed-by substitutes, so I'd suggest adding this small explanation as well: + SOB chains should reflect the *real* route a patch took as it was + propagated to us, with the first SOB entry signalling primary + authorship of a single author. Acks should be given as Acked-by + lines and review approvals as Reviewed-by lines. > + If a patch is sent to the mailing list by a handler then the author has > + to be noted in the first line of the changelog with:: > + > + From: ``Author `` > + > + Changelog text starts here > + > + so the authorship is preserved. The 'From:' line has to be followed by a > + empty newline. If that 'From:' line is missing, then the patch would be > + attributed to the person who sent (transported) it. The 'From:' line is > + automatically removed when the patch is applied and does not show up in > + the final git changelog. It merely affects the authorship information of > + the resulting git commit. s/(transported) /(transported, handled) to connect the text with the whole 'handler' language used before? and since we are not talking about the 'git command', maybe also: s/git /Git ? > + - Cc: ``cc-ed-person `` > + > + If the patch should be backported to stable, then please add a '``Cc: > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > + mail. Can I suggest a more canonical form: Cc: # v4.18 and later kernels It would be nice if people adding Cc: stable lines would actually try to figure out which exact kernel versions are affected. Also the '<>' form makes it easier to read and my email client will also syntax highlight it in that case. ;-) > + - Link: ``https://link/to/information`` > + > + For referring to email on LKML or other kernel mailing lists, please use > + the lkml.kernel.org redirector URL:: s/referring to email /referring to an email > + > + https://lkml.kernel.org/r/email-message@id > + > + The kernel.org redirector is considered a stable URL unlike other email > + archives. s/URL unlike /URL, unlike ? Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > + - Signed-off-by: ``Patch handler `` > + > + SOBs after the author SOB are from people handling and transporting the > + patch, but were not involved in development. If the handler made > + modifications to the patch or the changelog, then this should be > + mentioned **after** the changelog text and **above** all commit tags in > + the following format:: > + > + ... changelog text ends. > + > + [ handler: Replaced foo by bar and updated changelog ] > + > + First-tag: . > + > + Note the two empty new lines which separate the changelog text and the > + commit tags from that notice. Even after a decade of introducing Git I still see Signed-off-by used as an Acked-by or Reviewed-by substitutes, so I'd suggest adding this small explanation as well: + SOB chains should reflect the *real* route a patch took as it was + propagated to us, with the first SOB entry signalling primary + authorship of a single author. Acks should be given as Acked-by + lines and review approvals as Reviewed-by lines. > + If a patch is sent to the mailing list by a handler then the author has > + to be noted in the first line of the changelog with:: > + > + From: ``Author `` > + > + Changelog text starts here > + > + so the authorship is preserved. The 'From:' line has to be followed by a > + empty newline. If that 'From:' line is missing, then the patch would be > + attributed to the person who sent (transported) it. The 'From:' line is > + automatically removed when the patch is applied and does not show up in > + the final git changelog. It merely affects the authorship information of > + the resulting git commit. s/(transported) /(transported, handled) to connect the text with the whole 'handler' language used before? and since we are not talking about the 'git command', maybe also: s/git /Git ? > + - Cc: ``cc-ed-person `` > + > + If the patch should be backported to stable, then please add a '``Cc: > + sta...@vger.kernel.org``' tag, but do not Cc stable when sending your > + mail. Can I suggest a more canonical form: Cc: # v4.18 and later kernels It would be nice if people adding Cc: stable lines would actually try to figure out which exact kernel versions are affected. Also the '<>' form makes it easier to read and my email client will also syntax highlight it in that case. ;-) > + - Link: ``https://link/to/information`` > + > + For referring to email on LKML or other kernel mailing lists, please use > + the lkml.kernel.org redirector URL:: s/referring to email /referring to an email > + > + https://lkml.kernel.org/r/email-message@id > + > + The kernel.org redirector is considered a stable URL unlike other email > + archives. s/URL unlike /URL, unlike ? Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> + - Fixes: 12char-SHA1 ("sub/sys: Original subject line")
> +
> + A Fixes tag should be added even for changes which do not need to be
> + backported to stable kernels, i.e. when addressing a recently introduced
> + issue which only affects tip or the current head of mainline. These tags
> + are helpful to identify the original commit and are much more valuable
> + than prominently mentioning the commit which introduced a problem in the
> + text of the changelog itself because they can be automatically
> + extracted.
> +
> + The following example illustrates the difference::
> +
> + Commit
> +
> + abcdef012345678 ("x86/xxx: Replace foo with bar")
> +
> + left an unused instance of variable foo around. Remove it.
> +
> + Signed-off-by: J.Dev
> +
> + Please say instead::
> +
> + The recent replacement of foo with bar left an unused instance of
> + variable foo around. Remove it.
> +
> + Fixes: abcdef012345678 ("x86/xxx: Replace foo with bar")
> + Signed-off-by: J.Dev
Let me extend this policy element, I frequently write out commits in the
changelog itself *as well*, because that's where I utilize it myself when
reading other people's changelogs.
I.e. I would convert this:
The recent replacement of left with right left an unused instance of
variable left around. Remove it.
Fixes: abcdef012345678 ("x86/xxx: Replace 'left' with 'right')
Signed-off-by: J.Dev
... to the following form:
Two years ago the following commit:
abcdef012345678 ("x86/xxx: Replace foo with bar")
... left an unused instance of the variable 'left' around. Remove it.
Fixes: abcdef012345678 ("x86/xxx: Replace 'left' with 'right')
Signed-off-by: J.Dev
This changelog style, while more verbose, has a couple of advantages:
- It's a bad practice to force the reader to go the tags sections, fish
out a commit ID, just to be able to see the original commit.
Especially with longer changelogs and with changelogs mentioning
multiple source commits in-lining the commit ID is useful.
- Also note how this style allows for human-readable time information to
be inserted - which can be important to backporters. While an unused
variable warning might not be backported, in other cases the time
information can be useful in prioritizing the backporting.
- Also note another pet peeve of mine: the quotation marks around the
variable names 'left' and 'right'. I changed the variable names to
English words that are ambiguous in free-flowing changelog text, just
to illustrate how important it can be to escape them for better
readability.
The 'Fixes' tag is mainly a standard tag that backporter tooling can
search for - otherwise for human readers the in-line explanation is more
useful.
I really trivial cases the inlining can be skipped and only a 'Fixes' tag
is perfectly sufficient.
Thanks,
Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> + - Fixes: 12char-SHA1 ("sub/sys: Original subject line")
> +
> + A Fixes tag should be added even for changes which do not need to be
> + backported to stable kernels, i.e. when addressing a recently introduced
> + issue which only affects tip or the current head of mainline. These tags
> + are helpful to identify the original commit and are much more valuable
> + than prominently mentioning the commit which introduced a problem in the
> + text of the changelog itself because they can be automatically
> + extracted.
> +
> + The following example illustrates the difference::
> +
> + Commit
> +
> + abcdef012345678 ("x86/xxx: Replace foo with bar")
> +
> + left an unused instance of variable foo around. Remove it.
> +
> + Signed-off-by: J.Dev
> +
> + Please say instead::
> +
> + The recent replacement of foo with bar left an unused instance of
> + variable foo around. Remove it.
> +
> + Fixes: abcdef012345678 ("x86/xxx: Replace foo with bar")
> + Signed-off-by: J.Dev
Let me extend this policy element, I frequently write out commits in the
changelog itself *as well*, because that's where I utilize it myself when
reading other people's changelogs.
I.e. I would convert this:
The recent replacement of left with right left an unused instance of
variable left around. Remove it.
Fixes: abcdef012345678 ("x86/xxx: Replace 'left' with 'right')
Signed-off-by: J.Dev
... to the following form:
Two years ago the following commit:
abcdef012345678 ("x86/xxx: Replace foo with bar")
... left an unused instance of the variable 'left' around. Remove it.
Fixes: abcdef012345678 ("x86/xxx: Replace 'left' with 'right')
Signed-off-by: J.Dev
This changelog style, while more verbose, has a couple of advantages:
- It's a bad practice to force the reader to go the tags sections, fish
out a commit ID, just to be able to see the original commit.
Especially with longer changelogs and with changelogs mentioning
multiple source commits in-lining the commit ID is useful.
- Also note how this style allows for human-readable time information to
be inserted - which can be important to backporters. While an unused
variable warning might not be backported, in other cases the time
information can be useful in prioritizing the backporting.
- Also note another pet peeve of mine: the quotation marks around the
variable names 'left' and 'right'. I changed the variable names to
English words that are ambiguous in free-flowing changelog text, just
to illustrate how important it can be to escape them for better
readability.
The 'Fixes' tag is mainly a standard tag that backporter tooling can
search for - otherwise for human readers the in-line explanation is more
useful.
I really trivial cases the inlining can be skipped and only a 'Fixes' tag
is perfectly sufficient.
Thanks,
Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > +Backtraces in changelogs > + > + > +Backtraces can be useful to document the call chain which led to a > +problem. Though not all back traces are really valuable because the call > +chain is unique and obvious, e.g. in early boot code. Just copying the full > +dmesg output is adding a lot of distracting information like timestamps, > +module lists, register and stack dumps etc. > + > +Reducing the backtrace to the real relevant data helps to concentrate on > +the issue and not being distracted by destilling the relevant information > +out of the dump. Here is an example of a well trimmed backtrace:: > + > + unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x > + 0064) at rIP: 0xae059994 (native_write_msr+0x4/0x20) > + Call Trace: > + mba_wrmsr+0x41/0x80 > + update_domains+0x125/0x130 > + rdtgroup_mkdir+0x270/0x500 Yeah, so I frequently simplify such backtraces even more, i.e.: > + unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x > 0064) at rIP: 0xae059994 (native_write_msr()) > + > + Call Trace: > +mba_wrmsr() > +update_domains() > +rdtgroup_mkdir() Note how the actual MSR contents and the attempted operation's parameters are important, the actual hexadecimal offsets of the function call backtrace are not. They are useful when trying to do fuzzy version matching and in the occasional case when there's a question about which exact call chain it is - but those are 0.01% cases really. See for example this recent commit: commit e4a02ed2aaf447fa849e3254bfdb3b9b01e1e520 (origin/locking-urgent-for-linus, locking-urgent-for-linus) Author: Guenter Roeck Date: Tue Oct 2 14:48:49 2018 -0700 locking/ww_mutex: Fix runtime warning in the WW mutex selftest If CONFIG_WW_MUTEX_SELFTEST=y is enabled, booting an image in an arm64 virtual machine results in the following traceback if 8 CPUs are enabled: DEBUG_LOCKS_WARN_ON(__owner_task(owner) != current) WARNING: CPU: 2 PID: 537 at kernel/locking/mutex.c:1033 __mutex_unlock_slowpath+0x1a8/0x2e0 ... Call trace: __mutex_unlock_slowpath() ww_mutex_unlock() test_cycle_work() process_one_work() worker_thread() kthread() ret_from_fork() If requesting b_mutex fails with -EDEADLK, the error variable is reassigned to the return value from calling ww_mutex_lock on a_mutex again. If this call fails, a_mutex is not locked. It is, however, unconditionally unlocked subsequently, causing the reported warning. Fix the problem by using two error variables. With this change, the selftest still fails as follows: cyclic deadlock not resolved, ret[7/8] = -35 However, the traceback is gone. The C-style writing of the backtrace is more readable than listing the offsets. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote: > +Backtraces in changelogs > + > + > +Backtraces can be useful to document the call chain which led to a > +problem. Though not all back traces are really valuable because the call > +chain is unique and obvious, e.g. in early boot code. Just copying the full > +dmesg output is adding a lot of distracting information like timestamps, > +module lists, register and stack dumps etc. > + > +Reducing the backtrace to the real relevant data helps to concentrate on > +the issue and not being distracted by destilling the relevant information > +out of the dump. Here is an example of a well trimmed backtrace:: > + > + unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x > + 0064) at rIP: 0xae059994 (native_write_msr+0x4/0x20) > + Call Trace: > + mba_wrmsr+0x41/0x80 > + update_domains+0x125/0x130 > + rdtgroup_mkdir+0x270/0x500 Yeah, so I frequently simplify such backtraces even more, i.e.: > + unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x > 0064) at rIP: 0xae059994 (native_write_msr()) > + > + Call Trace: > +mba_wrmsr() > +update_domains() > +rdtgroup_mkdir() Note how the actual MSR contents and the attempted operation's parameters are important, the actual hexadecimal offsets of the function call backtrace are not. They are useful when trying to do fuzzy version matching and in the occasional case when there's a question about which exact call chain it is - but those are 0.01% cases really. See for example this recent commit: commit e4a02ed2aaf447fa849e3254bfdb3b9b01e1e520 (origin/locking-urgent-for-linus, locking-urgent-for-linus) Author: Guenter Roeck Date: Tue Oct 2 14:48:49 2018 -0700 locking/ww_mutex: Fix runtime warning in the WW mutex selftest If CONFIG_WW_MUTEX_SELFTEST=y is enabled, booting an image in an arm64 virtual machine results in the following traceback if 8 CPUs are enabled: DEBUG_LOCKS_WARN_ON(__owner_task(owner) != current) WARNING: CPU: 2 PID: 537 at kernel/locking/mutex.c:1033 __mutex_unlock_slowpath+0x1a8/0x2e0 ... Call trace: __mutex_unlock_slowpath() ww_mutex_unlock() test_cycle_work() process_one_work() worker_thread() kthread() ret_from_fork() If requesting b_mutex fails with -EDEADLK, the error variable is reassigned to the return value from calling ww_mutex_lock on a_mutex again. If this call fails, a_mutex is not locked. It is, however, unconditionally unlocked subsequently, causing the reported warning. Fix the problem by using two error variables. With this change, the selftest still fails as follows: cyclic deadlock not resolved, ret[7/8] = -35 However, the traceback is gone. The C-style writing of the backtrace is more readable than listing the offsets. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Ingo Molnar wrote: > With tail comments the code looks like this: > > res = dostuff(); /* We explain something here. */ > > seed = 1; /* Another explanation. */ > > mod_timer(_object->our_timer, jiffies + OUR_INTERVAL); /* We like > to talk */ > > res = check_stuff(our_object); /* We explain something here. */ > if (res) > return -EINVAL; > > interval = nontrivial_calculation(); /* Another explanation. */ > > mod_timer(_object->our_timer, jiffies + interval); /* This doesn't > race, because. */ > > ... while with freestanding comments it's: > > /* We explain something here: */ > res = check_stuff(our_object); > if (res) > return -EINVAL; > > /* Another explanation: */ > interval = nontrivial_calculation(); > > /* This doesn't race with init_our_stuff(), because: */ > mod_timer(_object->our_timer, jiffies + interval); > > This comment placement style has several advantages: > > - Comments precede actual code - while in tail comments it's exactly > the wrong way around. > > - We don't create over-long lines nor artificially short tail comments > just because we were trying to stay within the col80 limit with the > "this doesn't race" comment. The full-line comment was able to > explain that the race is with init_our_stuff(). > > - Freestanding oneliner comments are much better aligned as well: note > how ever comment starts at the exact same column, making it very easy > to read (or not to read) these comments. > > - In short: predictable visual parsing rules and proper semantic > ordering of information is good, random joining of typographical > elements just to create marginally more compact source code is bad. > > - Just *look* at the tail comments example: it's a visually diffuse, > jumble of statements and misaligned comments with good structure. Forgot to mention the role of punctuation: - Also note how punctuation actually *helps* the integration of comments and code. The ":" will direct attention to the code that follows, making it clear that the sentence explains the next statement. There are exceptions for when different type of punctuation is a better choice, for example: /* TODO: convert the code to our newest calc API ASAP. */ interval = nontrivial_calculation(); Here we don't explain the statement but document a TODO entry. Also, it's frequent practice to use multi-line comments to explain the next section of code, with a particular note about the first statement. Proper punctuation helps there too: /* * Establish the timeout interval and use it to set up * the timer of our object. The object is going to be * freed when the last reference is released. * * Note, the initial calculation is non-trivial, because * our timeout rules have complexity A), B) and C): */ interval = nontrivial_calculation(); Note how part of the multi-line comment describes overall principles of the code to follow, while the last sentence describes a noteworthy aspect of the next C statement. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Ingo Molnar wrote: > With tail comments the code looks like this: > > res = dostuff(); /* We explain something here. */ > > seed = 1; /* Another explanation. */ > > mod_timer(_object->our_timer, jiffies + OUR_INTERVAL); /* We like > to talk */ > > res = check_stuff(our_object); /* We explain something here. */ > if (res) > return -EINVAL; > > interval = nontrivial_calculation(); /* Another explanation. */ > > mod_timer(_object->our_timer, jiffies + interval); /* This doesn't > race, because. */ > > ... while with freestanding comments it's: > > /* We explain something here: */ > res = check_stuff(our_object); > if (res) > return -EINVAL; > > /* Another explanation: */ > interval = nontrivial_calculation(); > > /* This doesn't race with init_our_stuff(), because: */ > mod_timer(_object->our_timer, jiffies + interval); > > This comment placement style has several advantages: > > - Comments precede actual code - while in tail comments it's exactly > the wrong way around. > > - We don't create over-long lines nor artificially short tail comments > just because we were trying to stay within the col80 limit with the > "this doesn't race" comment. The full-line comment was able to > explain that the race is with init_our_stuff(). > > - Freestanding oneliner comments are much better aligned as well: note > how ever comment starts at the exact same column, making it very easy > to read (or not to read) these comments. > > - In short: predictable visual parsing rules and proper semantic > ordering of information is good, random joining of typographical > elements just to create marginally more compact source code is bad. > > - Just *look* at the tail comments example: it's a visually diffuse, > jumble of statements and misaligned comments with good structure. Forgot to mention the role of punctuation: - Also note how punctuation actually *helps* the integration of comments and code. The ":" will direct attention to the code that follows, making it clear that the sentence explains the next statement. There are exceptions for when different type of punctuation is a better choice, for example: /* TODO: convert the code to our newest calc API ASAP. */ interval = nontrivial_calculation(); Here we don't explain the statement but document a TODO entry. Also, it's frequent practice to use multi-line comments to explain the next section of code, with a particular note about the first statement. Proper punctuation helps there too: /* * Establish the timeout interval and use it to set up * the timer of our object. The object is going to be * freed when the last reference is released. * * Note, the initial calculation is non-trivial, because * our timeout rules have complexity A), B) and C): */ interval = nontrivial_calculation(); Note how part of the multi-line comment describes overall principles of the code to follow, while the last sentence describes a noteworthy aspect of the next C statement. Thanks, Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> +Coding style notes
> +--
> +
> +Comment style
> +^
> +
> +Sentences in comments start with a uppercase letter.
> +
> +Single line comments::
> +
> + /* This is a single line comment */
> +
> +Multi-line comments::
> +
> + /*
> + * This is a properly formatted
> + * multi-line comment.
> + *
> + * Larger multi-line comments should be split into paragraphs.
> + */
> +
> +No tail comments:
> +
> + Please refrain from using tail comments. Tail comments disturb the
> + reading flow in almost all contexts, but especially in code::
> +
> + if (somecondition_is_true) /* Don't put a comment here */
> + dostuff(); /* Neither here */
> +
> + seed = MAGIC_CONSTANT; /* Nor here */
> +
> + Use freestanding comments instead::
> +
> + /* This condition is not obvious without a comment */
> + if (somecondition_is_true) {
> + /* This really needs to be documented */
> + dostuff();
> + }
> +
> + /* This magic initialization needs a comment. Maybe not? */
> + seed = MAGIC_CONSTANT;
Yeah, so I think a better way to visualize and explain the 'no tail
comments' guideline in -tip is not these more complex code flows, but the
totally simple linear flows of statements.
With tail comments the code looks like this:
res = dostuff(); /* We explain something here. */
seed = 1; /* Another explanation. */
mod_timer(_object->our_timer, jiffies + OUR_INTERVAL); /* We like
to talk */
res = check_stuff(our_object); /* We explain something here. */
if (res)
return -EINVAL;
interval = nontrivial_calculation(); /* Another explanation. */
mod_timer(_object->our_timer, jiffies + interval); /* This doesn't
race, because. */
... while with freestanding comments it's:
/* We explain something here: */
res = check_stuff(our_object);
if (res)
return -EINVAL;
/* Another explanation: */
interval = nontrivial_calculation();
/* This doesn't race with init_our_stuff(), because: */
mod_timer(_object->our_timer, jiffies + interval);
This comment placement style has several advantages:
- Comments precede actual code - while in tail comments it's exactly
the wrong way around.
- We don't create over-long lines nor artificially short tail comments
just because we were trying to stay within the col80 limit with the
"this doesn't race" comment. The full-line comment was able to
explain that the race is with init_our_stuff().
- Freestanding oneliner comments are much better aligned as well: note
how ever comment starts at the exact same column, making it very easy
to read (or not to read) these comments.
- In short: predictable visual parsing rules and proper semantic
ordering of information is good, random joining of typographical
elements just to create marginally more compact source code is bad.
- Just *look* at the tail comments example: it's a visually diffuse,
jumble of statements and misaligned comments with good structure.
Do you want me to send a delta patch, or an edited document?
Thanks,
Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
* Thomas Gleixner wrote:
> +Coding style notes
> +--
> +
> +Comment style
> +^
> +
> +Sentences in comments start with a uppercase letter.
> +
> +Single line comments::
> +
> + /* This is a single line comment */
> +
> +Multi-line comments::
> +
> + /*
> + * This is a properly formatted
> + * multi-line comment.
> + *
> + * Larger multi-line comments should be split into paragraphs.
> + */
> +
> +No tail comments:
> +
> + Please refrain from using tail comments. Tail comments disturb the
> + reading flow in almost all contexts, but especially in code::
> +
> + if (somecondition_is_true) /* Don't put a comment here */
> + dostuff(); /* Neither here */
> +
> + seed = MAGIC_CONSTANT; /* Nor here */
> +
> + Use freestanding comments instead::
> +
> + /* This condition is not obvious without a comment */
> + if (somecondition_is_true) {
> + /* This really needs to be documented */
> + dostuff();
> + }
> +
> + /* This magic initialization needs a comment. Maybe not? */
> + seed = MAGIC_CONSTANT;
Yeah, so I think a better way to visualize and explain the 'no tail
comments' guideline in -tip is not these more complex code flows, but the
totally simple linear flows of statements.
With tail comments the code looks like this:
res = dostuff(); /* We explain something here. */
seed = 1; /* Another explanation. */
mod_timer(_object->our_timer, jiffies + OUR_INTERVAL); /* We like
to talk */
res = check_stuff(our_object); /* We explain something here. */
if (res)
return -EINVAL;
interval = nontrivial_calculation(); /* Another explanation. */
mod_timer(_object->our_timer, jiffies + interval); /* This doesn't
race, because. */
... while with freestanding comments it's:
/* We explain something here: */
res = check_stuff(our_object);
if (res)
return -EINVAL;
/* Another explanation: */
interval = nontrivial_calculation();
/* This doesn't race with init_our_stuff(), because: */
mod_timer(_object->our_timer, jiffies + interval);
This comment placement style has several advantages:
- Comments precede actual code - while in tail comments it's exactly
the wrong way around.
- We don't create over-long lines nor artificially short tail comments
just because we were trying to stay within the col80 limit with the
"this doesn't race" comment. The full-line comment was able to
explain that the race is with init_our_stuff().
- Freestanding oneliner comments are much better aligned as well: note
how ever comment starts at the exact same column, making it very easy
to read (or not to read) these comments.
- In short: predictable visual parsing rules and proper semantic
ordering of information is good, random joining of typographical
elements just to create marginally more compact source code is bad.
- Just *look* at the tail comments example: it's a visually diffuse,
jumble of statements and misaligned comments with good structure.
Do you want me to send a delta patch, or an edited document?
Thanks,
Ingo
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Wed, Nov 07, 2018 at 06:44:07PM +0100, Thomas Gleixner wrote:
> On Wed, 7 Nov 2018, Thomas Gleixner wrote:
>
> > Add a document to the subsystem/maintainer handbook section, which explains
> > what the tip tree is, how it operates and what rules and expectations it
> > has.
>
> Peter asked me to add a section about locking comments. I added it and
> forgot to refresh the patch before sending. Delta patch below.
>
> Thanks,
>
> tglx
> ---
> --- a/Documentation/process/maintainer-tip.rst
> +++ b/Documentation/process/maintainer-tip.rst
> @@ -578,6 +578,29 @@ Sentences in comments start with a upper
>usage of descriptive function names often replaces these tiny comments.
>Apply common sense as always.
>
> +
> +Documenting locking requirements
> +
> + Documenting locking requirements is a good thing, but comments are not
> + necessarily the best choice. Instead of writing::
> +
> + /* Caller must hold foo->lock */
> + void func(struct foo *foo)
> + {
> + ...
> + }
> +
> + Please use::
> +
> + void func(struct foo *foo)
> + {
> + lockdep_assert_held(>lock);
> + ...
> + }
> +
> + The latter enables run time debugging when lockdep is enabled which
> + verifies that all callers hold the lock. Comments can't do that.
In PROVE_LOCKING kernels, lockdep_assert_held() emits a warning
if the caller doesn't hold the lock. Comments can't do that.
Thanx, Paul
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Wed, Nov 07, 2018 at 06:44:07PM +0100, Thomas Gleixner wrote:
> On Wed, 7 Nov 2018, Thomas Gleixner wrote:
>
> > Add a document to the subsystem/maintainer handbook section, which explains
> > what the tip tree is, how it operates and what rules and expectations it
> > has.
>
> Peter asked me to add a section about locking comments. I added it and
> forgot to refresh the patch before sending. Delta patch below.
>
> Thanks,
>
> tglx
> ---
> --- a/Documentation/process/maintainer-tip.rst
> +++ b/Documentation/process/maintainer-tip.rst
> @@ -578,6 +578,29 @@ Sentences in comments start with a upper
>usage of descriptive function names often replaces these tiny comments.
>Apply common sense as always.
>
> +
> +Documenting locking requirements
> +
> + Documenting locking requirements is a good thing, but comments are not
> + necessarily the best choice. Instead of writing::
> +
> + /* Caller must hold foo->lock */
> + void func(struct foo *foo)
> + {
> + ...
> + }
> +
> + Please use::
> +
> + void func(struct foo *foo)
> + {
> + lockdep_assert_held(>lock);
> + ...
> + }
> +
> + The latter enables run time debugging when lockdep is enabled which
> + verifies that all callers hold the lock. Comments can't do that.
In PROVE_LOCKING kernels, lockdep_assert_held() emits a warning
if the caller doesn't hold the lock. Comments can't do that.
Thanx, Paul
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Wed, Nov 07, 2018 at 06:10:12PM +0100, Thomas Gleixner wrote: > Add a document to the subsystem/maintainer handbook section, which explains > what the tip tree is, how it operates and what rules and expectations it > has. > > Signed-off-by: Thomas Gleixner A few more suggestions below, again to new text. I have, admittedly uncharacteristically, trimmed the patch. ;-) Reviewed-by: Paul E. McKenney > --- > Documentation/process/maintainer-handbooks.rst |2 > Documentation/process/maintainer-tip.rst | 796 > + > 2 files changed, 798 insertions(+) > > --- a/Documentation/process/maintainer-handbooks.rst > +++ b/Documentation/process/maintainer-handbooks.rst > @@ -12,3 +12,5 @@ which is supplementary to the general de > .. toctree:: > :numbered: > :maxdepth: 2 > + > + maintainer-tip > --- /dev/null > +++ b/Documentation/process/maintainer-tip.rst > @@ -0,0 +1,796 @@ > +The tip tree handbook > += > + > +What is the tip tree? > +- > + > +The tip tree is a collection of several subsystems and areas of > +development. The tip tree is both a direct development tree and a > +aggregation tree for several sub-maintainer trees. The tip tree gitweb URL > +is: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git > + > +The tip tree contains the following subsystems: > + > + - **x86 architecture** > + > + The x86 architecture development takes place in the tip tree except > + for the x86 KVM and XEN specific parts which are maintained in the > + corresponding subsystems and routed directly to mainline from > + there. It's still good practice to Cc the x86 maintainers on > + x86-specific KVM and XEN patches. > + > + Some x86 subsystems have their own maintainers in addition to the > + overall x86 maintainers. Please Cc the overall x86 maintainers on > + patches touching files in arch/x86 even when they are not called out > + by the MAINTAINER file. > + > + Note, that ``x...@kernel.org`` is not a mailing list. It is merely a > + mail alias which distributes mails to the x86 top-level maintainer > + team. Please always Cc the Linux Kernel mailing list (LKML) > + ``linux-kernel@vger.kernel.org``, otherwise your mail ends up only in > + the private inboxes of the maintainers. > + > + - **Scheduler** > + > + - **Locking and atomics** > + > + - **Generic interrupt subsystem and interrupt chip drivers**: > + > + - interrupt core development happens in the irq/core branch > + > + - interrupt chip driver development happens also in the irq/core > + branch, but the patches are mostly applied in a separate maintainer - interrupt chip driver development also happens in the irq/core branch, but the patches are usually applied in a separate maintainer > + tree and then aggregated into irq/core > + > + - **Time, timers, timekeeping, NOHZ and related chip drivers**: > + > + - timekeeping, clocksource core, NTP and alarmtimer development > + happens in the timers/core branch, but patches are mostly applied in happens in the timers/core branch, but patches are usually applied in > + a separate maintainer tree and then aggregated into timers/core > + > + - clocksource/event driver development happens in the timers/core > + branch, but patches are mostly applied in a separate maintainer tree > + and then aggregated into timers/core > + > + - **Performance counters core, architecture support and tooling**: > + > + - perf core and architecture support development happens in the > + perf/core branch > + > + - perf tooling development happens in the perf tools maintainer > + tree and is aggregated into the tip tree. > + > + - **CPU hotplug core** > + > + - **RAS core** > + > + - **EFI core** > + > + EFI development in the efi git tree. The collected patches are > + aggregated in the tip efi/core branch. > + > + - **RCU** > + > + RCU development happens in the linux-rcu tree. The resulting changes > + are aggregated into the tip core/rcu branch. > + > + - **Various core code components**: > + > + - debugobjects > + > + - objtool > + > + - random bits and pieces [ . . . ] > +Backtraces in changelogs > + > + > +Backtraces can be useful to document the call chain which led to a > +problem. Though not all back traces are really valuable because the call > +chain is unique and obvious, e.g. in early boot code. Just copying the full > +dmesg output is adding a lot of distracting information like timestamps, > +module lists, register and stack dumps etc. Backtraces help document the call chain leading to a problem. However, not all back traces are helpful, for example. early boot call chains are unique and obvious. Furthermore, copying the full dmesg output adds distracting information like
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Wed, Nov 07, 2018 at 06:10:12PM +0100, Thomas Gleixner wrote: > Add a document to the subsystem/maintainer handbook section, which explains > what the tip tree is, how it operates and what rules and expectations it > has. > > Signed-off-by: Thomas Gleixner A few more suggestions below, again to new text. I have, admittedly uncharacteristically, trimmed the patch. ;-) Reviewed-by: Paul E. McKenney > --- > Documentation/process/maintainer-handbooks.rst |2 > Documentation/process/maintainer-tip.rst | 796 > + > 2 files changed, 798 insertions(+) > > --- a/Documentation/process/maintainer-handbooks.rst > +++ b/Documentation/process/maintainer-handbooks.rst > @@ -12,3 +12,5 @@ which is supplementary to the general de > .. toctree:: > :numbered: > :maxdepth: 2 > + > + maintainer-tip > --- /dev/null > +++ b/Documentation/process/maintainer-tip.rst > @@ -0,0 +1,796 @@ > +The tip tree handbook > += > + > +What is the tip tree? > +- > + > +The tip tree is a collection of several subsystems and areas of > +development. The tip tree is both a direct development tree and a > +aggregation tree for several sub-maintainer trees. The tip tree gitweb URL > +is: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git > + > +The tip tree contains the following subsystems: > + > + - **x86 architecture** > + > + The x86 architecture development takes place in the tip tree except > + for the x86 KVM and XEN specific parts which are maintained in the > + corresponding subsystems and routed directly to mainline from > + there. It's still good practice to Cc the x86 maintainers on > + x86-specific KVM and XEN patches. > + > + Some x86 subsystems have their own maintainers in addition to the > + overall x86 maintainers. Please Cc the overall x86 maintainers on > + patches touching files in arch/x86 even when they are not called out > + by the MAINTAINER file. > + > + Note, that ``x...@kernel.org`` is not a mailing list. It is merely a > + mail alias which distributes mails to the x86 top-level maintainer > + team. Please always Cc the Linux Kernel mailing list (LKML) > + ``linux-kernel@vger.kernel.org``, otherwise your mail ends up only in > + the private inboxes of the maintainers. > + > + - **Scheduler** > + > + - **Locking and atomics** > + > + - **Generic interrupt subsystem and interrupt chip drivers**: > + > + - interrupt core development happens in the irq/core branch > + > + - interrupt chip driver development happens also in the irq/core > + branch, but the patches are mostly applied in a separate maintainer - interrupt chip driver development also happens in the irq/core branch, but the patches are usually applied in a separate maintainer > + tree and then aggregated into irq/core > + > + - **Time, timers, timekeeping, NOHZ and related chip drivers**: > + > + - timekeeping, clocksource core, NTP and alarmtimer development > + happens in the timers/core branch, but patches are mostly applied in happens in the timers/core branch, but patches are usually applied in > + a separate maintainer tree and then aggregated into timers/core > + > + - clocksource/event driver development happens in the timers/core > + branch, but patches are mostly applied in a separate maintainer tree > + and then aggregated into timers/core > + > + - **Performance counters core, architecture support and tooling**: > + > + - perf core and architecture support development happens in the > + perf/core branch > + > + - perf tooling development happens in the perf tools maintainer > + tree and is aggregated into the tip tree. > + > + - **CPU hotplug core** > + > + - **RAS core** > + > + - **EFI core** > + > + EFI development in the efi git tree. The collected patches are > + aggregated in the tip efi/core branch. > + > + - **RCU** > + > + RCU development happens in the linux-rcu tree. The resulting changes > + are aggregated into the tip core/rcu branch. > + > + - **Various core code components**: > + > + - debugobjects > + > + - objtool > + > + - random bits and pieces [ . . . ] > +Backtraces in changelogs > + > + > +Backtraces can be useful to document the call chain which led to a > +problem. Though not all back traces are really valuable because the call > +chain is unique and obvious, e.g. in early boot code. Just copying the full > +dmesg output is adding a lot of distracting information like timestamps, > +module lists, register and stack dumps etc. Backtraces help document the call chain leading to a problem. However, not all back traces are helpful, for example. early boot call chains are unique and obvious. Furthermore, copying the full dmesg output adds distracting information like
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Wed, 7 Nov 2018, Thomas Gleixner wrote:
> Add a document to the subsystem/maintainer handbook section, which explains
> what the tip tree is, how it operates and what rules and expectations it
> has.
Peter asked me to add a section about locking comments. I added it and
forgot to refresh the patch before sending. Delta patch below.
Thanks,
tglx
---
--- a/Documentation/process/maintainer-tip.rst
+++ b/Documentation/process/maintainer-tip.rst
@@ -578,6 +578,29 @@ Sentences in comments start with a upper
usage of descriptive function names often replaces these tiny comments.
Apply common sense as always.
+
+Documenting locking requirements
+
+ Documenting locking requirements is a good thing, but comments are not
+ necessarily the best choice. Instead of writing::
+
+ /* Caller must hold foo->lock */
+ void func(struct foo *foo)
+ {
+ ...
+ }
+
+ Please use::
+
+ void func(struct foo *foo)
+ {
+ lockdep_assert_held(>lock);
+ ...
+ }
+
+ The latter enables run time debugging when lockdep is enabled which
+ verifies that all callers hold the lock. Comments can't do that.
+
Bracket rules
^
Re: [patch 2/2] Documentation/process: Add tip tree handbook
On Wed, 7 Nov 2018, Thomas Gleixner wrote:
> Add a document to the subsystem/maintainer handbook section, which explains
> what the tip tree is, how it operates and what rules and expectations it
> has.
Peter asked me to add a section about locking comments. I added it and
forgot to refresh the patch before sending. Delta patch below.
Thanks,
tglx
---
--- a/Documentation/process/maintainer-tip.rst
+++ b/Documentation/process/maintainer-tip.rst
@@ -578,6 +578,29 @@ Sentences in comments start with a upper
usage of descriptive function names often replaces these tiny comments.
Apply common sense as always.
+
+Documenting locking requirements
+
+ Documenting locking requirements is a good thing, but comments are not
+ necessarily the best choice. Instead of writing::
+
+ /* Caller must hold foo->lock */
+ void func(struct foo *foo)
+ {
+ ...
+ }
+
+ Please use::
+
+ void func(struct foo *foo)
+ {
+ lockdep_assert_held(>lock);
+ ...
+ }
+
+ The latter enables run time debugging when lockdep is enabled which
+ verifies that all callers hold the lock. Comments can't do that.
+
Bracket rules
^
