Hi Linus,
please pull the RAS side of things for 5.18.
But before you do, a git question if I may:
So the branch I'm sending you is tip/ras/core and I fast-forwarded it to
v5.17-rc4 before adding new stuff ontop.
However, I needed to have prerequisite work from another tip branch:
tip/locking/core which was fast-forwarded to v5.17-rc1 before it got
that prerequisite work added ontop.
So I merged tip/locking/core into tip/ras/core - see merge commit below
- and added the RAS stuff ontop.
However, when creating the diffstat for the pull request, it would
add additional files to it from tip/locking/core even if all the
tip/locking/core changes are already in your master branch:
$ git log master..tip/locking/core
$
After poking at it a bit more, I found a hint as to what it might be
complaining about:
$ git diff --stat master...ras_core_for_v5.18_rc1
warning: master...ras_core_for_v5.18_rc1: multiple merge bases, using 754e0b0e35608ed5206d6a67a791563c631cec07
and:
$ git merge-base -a master ras_core_for_v5.18_rc1
754e0b0e35608ed5206d6a67a791563c631cec07 - that's rc4
b008893b08dcc8c30d756db05c229a1491bcb992 - that's the top-commit of tip/locking/core at the time the merge commit was created.
So it looks like the diffstat for the pull request is created by using
the -rc4 as the merge base:
$ git diff --stat v5.17-rc4..ras_core_for_v5.18_rc1
...
while the correct diffstat should be from the merge-commit onwards:
$ git diff --stat c0f6799de2a0..ras_core_for_v5.18_rc1
...
So, long story short, what am I doing wrong?
Thx a lot!
---
The following changes since commit 754e0b0e35608ed5206d6a67a791563c631cec07:
Linux 5.17-rc4 (2022-02-13 12:13:30 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/ras_core_for_v5.18_rc1
for you to fetch changes up to 7f1b8e0d6360178e3527d4f14e6921c254a86035:
x86/mce: Remove the tolerance level control (2022-02-23 11:09:25 +0100)
----------------------------------------------------------------
- More noinstr fixes
- Add an erratum workaround for Intel CPUs which, in certain
circumstances, end up consuming an unrelated uncorrectable memory error
when using fast string copy insns
- Remove the MCE tolerance level control as it is not really needed or
used anymore
----------------------------------------------------------------
Borislav Petkov (3):
Merge tip:locking/core into tip:ras/core
x86/mce: Use arch atomic and bit helpers
x86/mce: Remove the tolerance level control
Jue Wang (1):
x86/mce: Work around an erratum on fast string copy instructions
Auto-merging MAINTAINERS
Auto-merging arch/x86/kernel/cpu/mce/core.c
Merge made by the 'ort' strategy.
Documentation/ABI/removed/sysfs-mce | 37 ++++++++++++++++
Documentation/ABI/testing/sysfs-mce | 32 --------------
Documentation/vm/hwpoison.rst | 2 -
Documentation/x86/x86_64/boot-options.rst | 9 +---
arch/x86/kernel/cpu/mce/core.c | 175 ++++++++++++++++++++++++++++++++++++++++++++-------------------------------
arch/x86/kernel/cpu/mce/internal.h | 31 +++++++++++---
arch/x86/kernel/cpu/mce/severity.c | 23 +++++-----
7 files changed, 177 insertions(+), 132 deletions(-)
create mode 100644 Documentation/ABI/removed/sysfs-mce
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg
[ I've written this kind of reply multiple times before and I
_thought_ we had something in the docs about this, but I can't find
them, so here goes again ]
On Wed, Mar 23, 2022 at 10:29 AM Borislav Petkov <[email protected]> wrote:
>
> But before you do, a git question if I may:
> [.. details removed for brevity..]
> However, when creating the diffstat for the pull request, it would
> add additional files to it from tip/locking/core even if all the
> tip/locking/core changes are already in your master branch:
So what you are describing is a very fundamental thing - your branch
has multiple separate starting points, since you had different
branches that you merged into your tree..
Sometimes having multiple branches doesn't actually cause that,
because the different branches may all have the same base starting
point.
Git calls these things "merge bases", because those starting points is
what you have to take into account when merging, they are the "base"
for actually resolving the differences that come in through multiple
branches.
And git handles that perfectly fine when merging by doing all the
appropriate magic. And "git log" has no problem with it either - you
can list all the commits that are in your head but are *not* in some
arbitrary number of merge bases just fine).
But when you do a "git diff", things are different (and "git
request-pull" basically just does a diff to show what the thing was
about).
A "diff" is fundamentally something you do on two end-points. You have
a beginning, you have an end, and you ask "what changed between these
two end-points".
But that fundamentally means that when you have multiple different
merge bases, and you ask "what changed since the beginning and the
current state", your question is fundamentally ambiguous. There is not
a "the beginning". There are *multiple* beginnings.
So what git will do it to pick _one_ beginning, and just use that.
And that means that yes, the diff will show the changes since that
beginning, but since the end result depends on the _other_ beginning
too, it will show the changes that came from that other beginning as
well.
Sometimes those changes end up being empty, because the "first
beginning" might already have had all of that. So sometimes you might
not even notice that what "git diff" gave you was ambiguous.
So "git request-pull" does both a log (for the shortlog of commits)
and a diff (for the diffstat), and the log should always be correct,
but the diffstat will have this ambiguity problem if you have multiple
merge bases.
> After poking at it a bit more, I found a hint as to what it might be
> complaining about:
>
> $ git diff --stat master...ras_core_for_v5.18_rc1
> warning: master...ras_core_for_v5.18_rc1: multiple merge bases, using 754e0b0e35608ed5206d6a67a791563c631cec07
Yeah, so using that "..." format will warn about how there are
multiple possible merge bases, and point out how it just picked one at
random.
> So it looks like the diffstat for the pull request is created by using
> the -rc4 as the merge base:
>
> $ git diff --stat v5.17-rc4..ras_core_for_v5.18_rc1
> ...
>
> while the correct diffstat should be from the merge-commit onwards:
No. There is no such thing as a *correct* merge base. You had two, and
git picked one of them. In the general case there just isn't a correct
answer.
Now, it turns out that you shouldn't have done a merge at all. I'm not
sure why that commit c0f6799de2a0 ("Merge tip:locking/core into
tip:ras/core") even exists, because it could have been done as a
fast-forward. Did you use "--no-ff" to explicitly not do that?
So *because* you have that pointless merge that could just have been a
fast-forward, you think that "hey, if it had just picked the other
merge base it would all have been fine". But in a normal merge
situation, the two merge bases would both have had some work that
wasn't in the other side, so that's just because you did something
odd.
> So, long story short, what am I doing wrong?
So in the general case, you aren't doing anything wrong: if you merge
multiple real branches, it's just that "git diff" cannot find a single
unique point to use as the base, and you'll get some odd random diff.
But if you are a developer who merges multiple real branches, you
obviously know how to merge things, and one way to sort it out is to
basically do a test-merge just for yourself:
# WWLS? ("What would Linus See?")
git branch -b test-merge linus
git merge my-branch
git diff -C --stat --summary ORIG_HEAD..
.. save that away ..
# go back to your real work, and remove that test-merge
git checkout <normal-branch>
git branch -D test-merge
will generate a diffstat of what a merge (which fundamentally knows
how to resolve multiple merge bases) would generate.
(Obviously you can just do the above in a completely separate git tree
too, if you don't like doing those temporary branches that might mess
up your working tree).
The other alternative is to just send me the bogus diffstat - I'm
sadly quite used to it, since a number of people just do "git
request-pull", see that it's odd, don't understand why, and just let
me sort it out.
Now the good news is that people who are afraid of merges and the
above kind of complexity will never actually see this situation. You
can't get multiple merge bases if you don't do any merges yourself.
So this kind of git complexity only happens to people who are supposed
to be able to handle it. You clearly figured out what was going on,
you didn't perhaps just realize the full story.
Linus
On Fri, Mar 25, 2022 at 01:01:15PM -0700, Linus Torvalds wrote:
> [ I've written this kind of reply multiple times before and I
> _thought_ we had something in the docs about this, but I can't find
> them, so here goes again ]
Thanks!
I had a faint notion that I had read you telling people that their
diffstat was bogus but I couldn't find anything relevant for the short
time I was searching.
How about I start a maintainers-specific documentation in
Documentation/process/ - we already have maintainer handbooks there -
and put that there?
I'm sure it'll come up again and it'll be easier to point to it next
time...
> On Wed, Mar 23, 2022 at 10:29 AM Borislav Petkov <[email protected]> wrote:
<snip detailed explanation - thanks for taking the time!>
> But that fundamentally means that when you have multiple different
> merge bases, and you ask "what changed since the beginning and the
> current state", your question is fundamentally ambiguous. There is not
> a "the beginning". There are *multiple* beginnings.
Aaaha, there it is. I suspected it was something fundamental...
> So what git will do it to pick _one_ beginning, and just use that.
>
> And that means that yes, the diff will show the changes since that
> beginning, but since the end result depends on the _other_ beginning
> too, it will show the changes that came from that other beginning as
> well.
Right.
...
> No. There is no such thing as a *correct* merge base. You had two, and
> git picked one of them. In the general case there just isn't a correct
> answer.
>
> Now, it turns out that you shouldn't have done a merge at all. I'm not
> sure why that commit c0f6799de2a0 ("Merge tip:locking/core into
> tip:ras/core") even exists, because it could have been done as a
> fast-forward. Did you use "--no-ff" to explicitly not do that?
Well, let me recreate the situation:
$ git checkout -b ras/core v5.17-rc4
Switched to a new branch 'ras/core'
$ git merge tip/locking/core
Auto-merging MAINTAINERS
Merge made by the 'ort' strategy.
MAINTAINERS | 1 +
arch/x86/include/asm/cpumask.h | 10 ++++++++++
arch/x86/include/asm/ptrace.h | 2 +-
include/asm-generic/bitops/instrumented-atomic.h | 12 ++++++------
include/asm-generic/bitops/instrumented-non-atomic.h | 16 ++++++++--------
include/linux/atomic/atomic-arch-fallback.h | 38 +++++++++++++++++++++++++++++++++-----
include/linux/cpumask.h | 18 +++++++++---------
include/linux/jump_label.h | 13 ++++---------
include/linux/local_lock_internal.h | 6 +++---
init/Kconfig | 1 +
kernel/locking/lockdep.c | 43 +++++++++++++++++++++++++------------------
kernel/locking/lockdep_internals.h | 6 ++++--
kernel/locking/lockdep_proc.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
kernel/locking/percpu-rwsem.c | 5 +++--
kernel/locking/rwsem.c | 2 +-
scripts/atomic/fallbacks/read_acquire | 11 ++++++++++-
scripts/atomic/fallbacks/set_release | 7 ++++++-
17 files changed, 168 insertions(+), 74 deletions(-)
so that tip/locking/core branch is based on v5.17-rc1 and has locking,
etc stuff which I needed in ras/core. Thus the merge. And git does a
merge commit.
If I try to make it do a --ff, it still does a merge commit:
$ git merge --ff tip/locking/core
Auto-merging MAINTAINERS
Merge made by the 'ort' strategy.
MAINTAINERS | 1 +
arch/x86/include/asm/cpumask.h | 10 ++++++++++
arch/x86/include/asm/ptrace.h | 2 +-
include/asm-generic/bitops/instrumented-atomic.h | 12 ++++++------
include/asm-generic/bitops/instrumented-non-atomic.h | 16 ++++++++--------
include/linux/atomic/atomic-arch-fallback.h | 38 +++++++++++++++++++++++++++++++++-----
include/linux/cpumask.h | 18 +++++++++---------
include/linux/jump_label.h | 13 ++++---------
include/linux/local_lock_internal.h | 6 +++---
init/Kconfig | 1 +
kernel/locking/lockdep.c | 43 +++++++++++++++++++++++++------------------
kernel/locking/lockdep_internals.h | 6 ++++--
kernel/locking/lockdep_proc.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
kernel/locking/percpu-rwsem.c | 5 +++--
kernel/locking/rwsem.c | 2 +-
scripts/atomic/fallbacks/read_acquire | 11 ++++++++++-
scripts/atomic/fallbacks/set_release | 7 ++++++-
17 files changed, 168 insertions(+), 74 deletions(-)
so I don't see how to do a fast-forward thing here.
> So *because* you have that pointless merge that could just have been a
> fast-forward, you think that "hey, if it had just picked the other
> merge base it would all have been fine". But in a normal merge
> situation, the two merge bases would both have had some work that
> wasn't in the other side, so that's just because you did something
> odd.
Right.
I guess my strategy for the future should be: either make sure branches
have a common merge base or generate a "fake" diffstat.
> So in the general case, you aren't doing anything wrong: if you merge
> multiple real branches, it's just that "git diff" cannot find a single
> unique point to use as the base, and you'll get some odd random diff.
Right.
> But if you are a developer who merges multiple real branches, you
> obviously know how to merge things, and one way to sort it out is to
> basically do a test-merge just for yourself:
>
> # WWLS? ("What would Linus See?")
> git branch -b test-merge linus
> git merge my-branch
> git diff -C --stat --summary ORIG_HEAD..
> .. save that away ..
>
> # go back to your real work, and remove that test-merge
> git checkout <normal-branch>
> git branch -D test-merge
>
> will generate a diffstat of what a merge (which fundamentally knows
> how to resolve multiple merge bases) would generate.
Yes, as a matter of fact I did that before sending you this email and
the diffstat it issued when doing the "git merge my-branch" into your
tree was the one I was expecting. I guess yeah, that's the way I should
be creating the diffstat when I have this situation in the future. Thx!
> The other alternative is to just send me the bogus diffstat - I'm
> sadly quite used to it, since a number of people just do "git
> request-pull", see that it's odd, don't understand why, and just let
> me sort it out.
Yeah, unlikely. I wanted to know what is going on so you got this email
with a question instead. :-)
> Now the good news is that people who are afraid of merges and the
> above kind of complexity will never actually see this situation. You
> can't get multiple merge bases if you don't do any merges yourself.
>
> So this kind of git complexity only happens to people who are supposed
> to be able to handle it. You clearly figured out what was going on,
> you didn't perhaps just realize the full story.
Thanks for taking the time and explaining - it was very helpful!
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg
On Fri, Mar 25, 2022 at 1:40 PM Borislav Petkov <[email protected]> wrote:
>
> If I try to make it do a --ff, it still does a merge commit:
Oh, they indeed aren't fast-forwards of each other, they just looked
superficially that way to me because when I did my
gitk ORIG_HEAD..
after merging, the fact that I had already merged everything else in
both of those branches.
So never mind. It wasn't a pointless fast-forward merge, it's just
that neither of those branches had anything new in them as far as I
was concerned any more.
Linus
The pull request you sent on Wed, 23 Mar 2022 18:29:38 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/ras_core_for_v5.18_rc1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/636f64db07f33a18630248b4c57e182cd315b0da
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
Borislav Petkov <[email protected]> writes:
> On Fri, Mar 25, 2022 at 01:01:15PM -0700, Linus Torvalds wrote:
>> [ I've written this kind of reply multiple times before and I
>> _thought_ we had something in the docs about this, but I can't find
>> them, so here goes again ]
>
> Thanks!
>
> I had a faint notion that I had read you telling people that their
> diffstat was bogus but I couldn't find anything relevant for the short
> time I was searching.
>
> How about I start a maintainers-specific documentation in
> Documentation/process/ - we already have maintainer handbooks there -
> and put that there?
Maybe something that looks like:
https://lore.kernel.org/lkml/[email protected]/
:)
Thanks,
jon
On Mon, Mar 28, 2022 at 08:21:51AM -0600, Jonathan Corbet wrote:
> Maybe something that looks like:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> :)
I was just about to scratch something together but it is a good thing
I got distracted by other stuff so that you'll document it a lot more
eloquently than me! :-)
A typo:
"So what is to be done? The best response when confronted with this
situation is to indeed to a merge,"
should be
"... to indeed *do* a merge, ..."
Other than that, very much
Acked-by: Borislav Petkov <[email protected]>
Thanks for doing that!
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg