| Message ID | 20191017234348.wcbbo2njexn7ixpk@willie-the-truck |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] arm64: Fixes for -rc4 | expand |
On Thu, Oct 17, 2019 at 4:43 PM Will Deacon <will@kernel.org> wrote: > > Note that the workaround code ended up being based on -rc2, so I had a > bit of a faff trying to generate the right diffstat for this pull request > after merging that branch into our fixes branch based on -rc1. In the end > I had to emulate the pull locally because I couldn't figure out how to > drive request-pull correctly despite the shortlog being correct. I'd love > to know what I should've done instead. You did the right thing. When there are multiple merge bases, a regular "git diff" doesn't work since it's fundamentally about two end-points (well, it _can_ work almost by mistake, but doesn't work in the general case). So the only way to get a "proper" diff is to do a merge and then diff the result. That said, I also accept the output of "git diff" which will then have a lot of noise from all the _other_ work done between the two merge bases. I can figure out what happened, and do my own two-endpoint diff and see what happened, and still se that "yes, that's what the pull request meant, and that's why the diffstat is garbage". What you did is the "good quality" pull request, though. In general, people who aren't doing fancy things with git should never get to the "multiple merge bases" situation, and then the regular pull request logic works fine. And people likme you who are doing multiple branches and know what they are doing are also able to them handle the "uhhuh, I need to do a merge to get a good diffstat" situation. Linus
The pull request you sent on Fri, 18 Oct 2019 00:43:49 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git tags/arm64-fixes
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0e2adab6cf285c41e825b6c74a3aa61324d1132c
Thank you!
On Thu, Oct 17, 2019 at 05:06:54PM -0700, Linus Torvalds wrote: > On Thu, Oct 17, 2019 at 4:43 PM Will Deacon <will@kernel.org> wrote: > > > > Note that the workaround code ended up being based on -rc2, so I had a > > bit of a faff trying to generate the right diffstat for this pull request > > after merging that branch into our fixes branch based on -rc1. In the end > > I had to emulate the pull locally because I couldn't figure out how to > > drive request-pull correctly despite the shortlog being correct. I'd love > > to know what I should've done instead. > > You did the right thing. > > When there are multiple merge bases, a regular "git diff" doesn't work > since it's fundamentally about two end-points (well, it _can_ work > almost by mistake, but doesn't work in the general case). So the only > way to get a "proper" diff is to do a merge and then diff the result. > > That said, I also accept the output of "git diff" which will then have > a lot of noise from all the _other_ work done between the two merge > bases. I can figure out what happened, and do my own two-endpoint diff > and see what happened, and still se that "yes, that's what the pull > request meant, and that's why the diffstat is garbage". > > What you did is the "good quality" pull request, though. Thanks, that's helpful to know for next time. I guess I'm most surprised by the discrepancy between the shortlog and the diffstat, whereas I intuitively expected them to be generated in the same way. Will
On Fri, Oct 18, 2019 at 10:42 AM Will Deacon <will@kernel.org> wrote: > > Thanks, that's helpful to know for next time. I guess I'm most surprised by > the discrepancy between the shortlog and the diffstat, whereas I intuitively > expected them to be generated in the same way. So logs and diffs are fundamentally different. A log is an operation on a _set_ of commits (that's the whole point - you don't list the beginning and the end, you list all the commits in between), while a diff is fundamentally an operation on two end-points and shows the code difference between those two points. And the summary versions of those operations (shortlog and diffstat) are no different. So as a set operation, "shortlog" has no issues with multiple merge bases. Doing a shortlog is still just a set difference between your commits and the upstream commits, and the number of merge bases is irrelevant. "List all commits that I have, but upstream doesn't have" is a very straightforward and natural set operation. But as a "two endpoints" operation, diffstat has real problems any time you have more than two endpoints - when you have multiple merge bases, you fundamentally have more than two endpoints: you have all of the merge bases, and then you have your end result. What you doing the merge does is to turn the multiple merge bases into just one point: the thing you merged against now becomes the common merge point, and now you have a "two endpoints" for the diffstat: the thing you merged against, and your end result are now the two points that you can diff against. But the shortlog is always correct, because it just doesn't even care about that whole issue. Linus
On Fri, Oct 18, 2019 at 12:43:49AM +0100, Will Deacon wrote: > Hi Linus, > > Please pull these arm64 fixes for -rc4. The main thing here is a > long-awaited workaround for a CPU erratum on ThunderX2 which we have > developed in conjunction with engineers from Cavium/Marvell. At the moment, > the workaround is unconditionally enabled for affected CPUs at runtime > but we may add a command-line option to disable it in future if performance > numbers show up indicating a significant cost for real workloads. As the Cavium/Marvell engineer who was involved in this, I will note that I had suggested a patch providing a runtime override[1] while providing safe defaults. Marc's patchset adds a trap to hypervisor in the system call path when KPTI is enabled, and KPTI is generally enabled on stock VM images. So normal users will see some performance regression (e.g I see something in the range of 3-4% on guest kernel compile). As a policy, I don't agree with having errata workarounds that can be left to the discretion of the admin to be forced at compile time. Since most of these workarounds use run-time code patching with alternatives, there is no need to do this at all. But given that this is already merged and cc:ed to stable, I will see if I can come up with enough data to convince Will. JC [1] https://lore.kernel.org/linux-arm-kernel/20191011232031.GA29752@dc5-eodlnx05.marvell.com/T/
* Linus Torvalds <torvalds@linux-foundation.org> wrote: > What you doing the merge does is to turn the multiple merge bases into > just one point: the thing you merged against now becomes the common > merge point, and now you have a "two endpoints" for the diffstat: the > thing you merged against, and your end result are now the two points > that you can diff against. > > But the shortlog is always correct, because it just doesn't even care > about that whole issue. FWIW I regularly ran into this problem too and resolved it manually by 'emulating' your merge. (Once every 20-30 pull requests or so. Finally ended up scripting around request-pull altogether.) I think at least once I ran into that and sent you a 'slightly wrong' diffstat - and maybe there's also been a few cases where you noticed diffstats that didn't match your merge result, double checked it yourself and didn't complain about it because you knew that this is a "git request-pull" artifact? Most of the time I notice it like Will did because the diffstat is obviously weird and it's good to check pull requests a second (and a third :-) time as well, but it's possible to have relatively small distances between the merge bases where the diffstat doesn't look 'obviously' bogus and mistakes can slip through. Anyway, a small Git feature request: it would be super useful if "git request-pull" output was a bit more dependable and at least warned about this and didn't include what is, from the viewpoint of the person doing the merge, a bogus diffstat. (Generating the correct diffstat is probably beyond request-pull's abilities: it would require changing the working tree to actually perform the merge - while request-pull is a read-only operation right now. But detecting the condition and warning about it should be possible?) Thanks, Ingo
On Mon, Oct 21, 2019 at 2:47 AM Ingo Molnar <mingo@kernel.org> wrote: > > I think at least once I ran into that and sent you a 'slightly wrong' > diffstat - and maybe there's also been a few cases where you noticed > diffstats that didn't match your merge result, double checked it yourself > and didn't complain about it because you knew that this is a "git > request-pull" artifact? Right. If I see a diffstat that doesn't match, I just look at what a non-merged diffstat would have looked like, and if that matches I know what happened. There are other reasons why diffstats won't match, of course. Like me just having merged part of the same commits from another source (or multiple trees applying the same patch). So it's not _just_ due to multiple merge bases that the mis-match can happen. > Most of the time I notice it like Will did because the diffstat is > obviously weird and it's good to check pull requests a second (and a > third :-) time as well, but it's possible to have relatively small > distances between the merge bases where the diffstat doesn't look > 'obviously' bogus and mistakes can slip through. Yup. > Anyway, a small Git feature request: it would be super useful if "git > request-pull" output was a bit more dependable and at least warned about > this and didn't include what is, from the viewpoint of the person doing > the merge, a bogus diffstat. Well, warning for it would be fairly simple. Giving the "right" result isn't simple, though, since the merge might need manual fixup to succeed. The warning you can check yourself: just do git merge-base --all upstream mybranch and if you get more than one result, you know you are in the situation where a diff from the merge base might not work (it *might* work, but probably won't). You can play around with it yourself, of course. Look at the git-request-puill.sh script, it says something like this: merge_base=$(git merge-base $baserev $headrev) || die "fatal: No commits in common between $base and $head" and you could add something like all_merge_bases="$(git merge-base --all $baserev $headrev)" and then add a warning if "all_merge_bases" doesn't match "merge_base". Linus
Hello, I added the git list to Cc:. For the new readers: The context of this thread can be found at https://lwn.net/ml/linux-kernel/20191017234348.wcbbo2njexn7ixpk@willie-the-truck/ On Mon, Oct 21, 2019 at 08:46:58AM +0200, Ingo Molnar wrote: > Anyway, a small Git feature request: it would be super useful if "git > request-pull" output was a bit more dependable and at least warned about > this and didn't include what is, from the viewpoint of the person doing > the merge, a bogus diffstat. (Generating the correct diffstat is probably > beyond request-pull's abilities: it would require changing the working > tree to actually perform the merge - while request-pull is a read-only > operation right now. But detecting the condition and warning about it > should be possible?) I think Will's case is still an easy one compared with what could actually happen. The related history looks as follows: ,-. ,-. ,-. ,-. ,-. v5.4-rc1 --| |-...-| |-- v5.4-rc2 --| |-..-| |-..-| |-- v5.4-rc3 \ `-' `-' \ `-' /-' `-' \ ,-. ,-. \ ,-/ ,-. ,-. `--| |-...-| |--------------------|*|----| |-...-|H| `-' `-' \ `-' `-' /-' \ ,-. ,-. / `--| |-...-| |-----' `-' `-' Will asked Linus to merge the Commit marked 'H', the two merge bases are v5.4-rc2 and '*'. (FTR: * = 3e7c93bd04edfb0cae7dad1215544c9350254b8f H = 777d062e5bee0e3c0751cdcbce116a76ee2310ec , they can be found in git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git) The formally correct way to create the diffstat is to merge v5.4-rc2 and '*' (in general: all merge bases) and calculate the diff between this merge and the to-be-merged-commit. Compared to what Will did (i.e. merge Linus' HEAD and this branch and then diff @~ with @) doing it the way I described has the advantage(?) that commits that conflict with this merge request in Linus' tree since the merge bases are not in the way. In this case this can be done automatically: $ git read-tree --index-output=tralala v5.4-rc2 3e7c93bd04edfb0cae7dad1215544c9350254b8f $ GIT_INDEX=tralala git write-tree 6a2acfd1870d9da3c330ea9b648a7e858b5ee39f $ git diff --stat 6a2acfd1870d9da3c330ea9b648a7e858b5ee39f 777d062e5bee0e3c0751cdcbce116a76ee2310ec Documentation/arm64/silicon-errata.rst | 2 ++ arch/arm64/Kconfig | 17 ++++++++++++++ arch/arm64/include/asm/asm-uaccess.h | 7 +++--- arch/arm64/include/asm/cpucaps.h | 4 +++- arch/arm64/include/asm/memory.h | 10 ++++++-- arch/arm64/include/asm/pgtable.h | 3 --- arch/arm64/include/asm/sysreg.h | 2 +- arch/arm64/kernel/cpu_errata.c | 38 +++++++++++++++++++++++++++++++ arch/arm64/kernel/cpufeature.c | 15 ++++++++---- arch/arm64/kernel/entry.S | 8 ++++--- arch/arm64/kernel/hibernate.c | 9 +++++++- arch/arm64/kernel/process.c | 18 +++++++++++++++ arch/arm64/kvm/hyp/switch.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- arch/arm64/mm/fault.c | 6 ++++- include/linux/sched.h | 1 + 15 files changed, 186 insertions(+), 23 deletions(-) Would be great if git-request-pull learned to do that. Best regards Uwe
Hi Linus, Please pull these arm64 fixes for -rc4. The main thing here is a long-awaited workaround for a CPU erratum on ThunderX2 which we have developed in conjunction with engineers from Cavium/Marvell. At the moment, the workaround is unconditionally enabled for affected CPUs at runtime but we may add a command-line option to disable it in future if performance numbers show up indicating a significant cost for real workloads. The other fixes are summarised in the tag. Note that the workaround code ended up being based on -rc2, so I had a bit of a faff trying to generate the right diffstat for this pull request after merging that branch into our fixes branch based on -rc1. In the end I had to emulate the pull locally because I couldn't figure out how to drive request-pull correctly despite the shortlog being correct. I'd love to know what I should've done instead. Thanks, Will --->8 The following changes since commit 3e7c93bd04edfb0cae7dad1215544c9350254b8f: arm64: armv8_deprecated: Checking return value for memory allocation (2019-10-08 13:34:04 +0100) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git tags/arm64-fixes for you to fetch changes up to 777d062e5bee0e3c0751cdcbce116a76ee2310ec: Merge branch 'errata/tx2-219' into for-next/fixes (2019-10-17 13:42:42 -0700) ---------------------------------------------------------------- arm64 fixes for -rc4 - Work around Cavium/Marvell ThunderX2 erratum #219 - Fix regression in mlock() ABI caused by sign-extension of TTBR1 addresses - More fixes to the spurious kernel fault detection logic - Fix pathological preemption race when enabling some CPU features at boot - Drop broken kcore macros in favour of generic implementations - Fix userspace view of ID_AA64ZFR0_EL1 when SVE is disabled - Avoid NULL dereference on allocation failure during hibernation ---------------------------------------------------------------- Chris von Recklinghausen (1): arm64: Fix kcore macros after 52-bit virtual addressing fallout Julien Grall (1): arm64: cpufeature: Treat ID_AA64ZFR0_EL1 as RAZ when SVE is not enabled Julien Thierry (1): arm64: entry.S: Do not preempt from IRQ before all cpufeatures are enabled Marc Zyngier (4): arm64: KVM: Trap VM ops when ARM64_WORKAROUND_CAVIUM_TX2_219_TVM is set arm64: Enable workaround for Cavium TX2 erratum 219 when running SMT arm64: Avoid Cavium TX2 erratum 219 when switching TTBR arm64: Allow CAVIUM_TX2_ERRATUM_219 to be selected Mark Rutland (1): arm64: mm: fix inverted PAR_EL1.F check Pavel Tatashin (1): arm64: hibernate: check pgd table allocation Will Deacon (2): arm64: tags: Preserve tags for addresses translated via TTBR1 Merge branch 'errata/tx2-219' into for-next/fixes Yang Yingliang (1): arm64: sysreg: fix incorrect definition of SYS_PAR_EL1_F Documentation/arm64/silicon-errata.rst | 2 + arch/arm64/Kconfig | 17 +++++++++ arch/arm64/include/asm/asm-uaccess.h | 7 ++-- arch/arm64/include/asm/cpucaps.h | 4 +- arch/arm64/include/asm/memory.h | 10 ++++- arch/arm64/include/asm/pgtable.h | 3 -- arch/arm64/include/asm/sysreg.h | 2 +- arch/arm64/kernel/cpu_errata.c | 38 +++++++++++++++++++ arch/arm64/kernel/cpufeature.c | 15 +++++--- arch/arm64/kernel/entry.S | 8 ++-- arch/arm64/kernel/hibernate.c | 9 ++++- arch/arm64/kernel/process.c | 18 +++++++++ arch/arm64/kvm/hyp/switch.c | 69 +++++++++++++++++++++++++++++++++- arch/arm64/mm/fault.c | 6 ++- include/linux/sched.h | 1 + 15 files changed, 186 insertions(+), 23 deletions(-)