| Message ID | 20111206152955.GC31720@arm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 06, 2011 at 03:29:55PM +0000, Catalin Marinas wrote: > I updated the series with an additional signed-off-by to your patch. The > code is unchanged. Could you please pull it again? I will for this time only, but note - last time you said: > If there are no further comments, could you please pull the ARM LPAE > branch below? They should be merged after Will's idmap patches (no real > conflict, just correctly functioning setup_mm_for_reboot). Now, the thing is, merging it after Will's patches won't solve that in any shape or form - it really does not matter what order I do the merges in, the fact of the matter is that there is no ordering or dependence between your patches and Will's, so there's no guarantee that your code will see a properly functioning setup_mm_for_reboot. If you actually depend on something in some other tree, you need to have it merged into your tree _before_ the objects which depend on it. So, what the above comment admits to me is that "what I'm asking you to pull is known to be broken if a git-bisect hits one of these commits, but I don't care." As I said, I will merge it this time around, but next time I won't.
On Tue, Dec 06, 2011 at 08:34:16PM +0000, Russell King - ARM Linux wrote: > On Tue, Dec 06, 2011 at 03:29:55PM +0000, Catalin Marinas wrote: > > I updated the series with an additional signed-off-by to your patch. The > > code is unchanged. Could you please pull it again? > > I will for this time only, but note - last time you said: > > > If there are no further comments, could you please pull the ARM LPAE > > branch below? They should be merged after Will's idmap patches (no real > > conflict, just correctly functioning setup_mm_for_reboot). > > Now, the thing is, merging it after Will's patches won't solve that in > any shape or form - it really does not matter what order I do the merges > in, the fact of the matter is that there is no ordering or dependence > between your patches and Will's, so there's no guarantee that your code > will see a properly functioning setup_mm_for_reboot. > > If you actually depend on something in some other tree, you need to have > it merged into your tree _before_ the objects which depend on it. > > So, what the above comment admits to me is that "what I'm asking you to > pull is known to be broken if a git-bisect hits one of these commits, > but I don't care." > > As I said, I will merge it this time around, but next time I won't. Well, I might do if you told me that your changes conflict with Will's idmap changes in arch/arm/mm/idmap.c: ++<<<<<<< HEAD +extern char __idmap_text_start[], __idmap_text_end[]; ++======= + #ifdef CONFIG_SMP + static void idmap_del_pmd(pud_t *pud, unsigned long addr, unsigned long end) + { + pmd_t *pmd; + + if (pud_none_or_clear_bad(pud)) + return; + pmd = pmd_offset(pud, addr); + pmd_clear(pmd); + } ++>>>>>>> cf9a53fb7cf8dd2fd2b5d7bb07434bd2ebeb5a8f I'm assuming that idmap_del_pmd() just gets deleted? Alternatively, if you wish to fix the dependencies of your commits and resubmit a pull request, let me know.
On 6 December 2011 20:34, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Dec 06, 2011 at 03:29:55PM +0000, Catalin Marinas wrote: >> I updated the series with an additional signed-off-by to your patch. The >> code is unchanged. Could you please pull it again? > > I will for this time only, but note - last time you said: > >> If there are no further comments, could you please pull the ARM LPAE >> branch below? They should be merged after Will's idmap patches (no real >> conflict, just correctly functioning setup_mm_for_reboot). > > Now, the thing is, merging it after Will's patches won't solve that in > any shape or form - it really does not matter what order I do the merges > in, the fact of the matter is that there is no ordering or dependence > between your patches and Will's, so there's no guarantee that your code > will see a properly functioning setup_mm_for_reboot. This situation is not unique to my LPAE patches (git bisect doesn't always go well merges). There are many other situations where one relies on some stuff to get into mainline (or maintainer tree) before merging. Just have a look at the kernel logs. But if you want, there other solutions: 1. I can rebase the LPAE branch on top of your devel-stable _after_ you merged Will's patches. Then I send another pull request. 2. Only merge the LPAE patches without the one adding the Kconfig option. Later apply the Kconfig patch. > If you actually depend on something in some other tree, you need to have > it merged into your tree _before_ the objects which depend on it. Not easy in this instance. AFAIK Will's code relied on some reset patches from you, so Will just used your devel-stable branch. > As I said, I will merge it this time around, but next time I won't. As I also said above, there are solutions. What it is really needed is _better_ collaboration during merges and _discussing_ the best approach rather than threatening that there won't be other pulls. I'm open to rebase my branch against other commits, just say it. I doubt this is the only time where merge issues occured.
On 6 December 2011 20:45, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Dec 06, 2011 at 08:34:16PM +0000, Russell King - ARM Linux wrote: >> On Tue, Dec 06, 2011 at 03:29:55PM +0000, Catalin Marinas wrote: >> > I updated the series with an additional signed-off-by to your patch. The >> > code is unchanged. Could you please pull it again? >> >> I will for this time only, but note - last time you said: >> >> > If there are no further comments, could you please pull the ARM LPAE >> > branch below? They should be merged after Will's idmap patches (no real >> > conflict, just correctly functioning setup_mm_for_reboot). >> >> Now, the thing is, merging it after Will's patches won't solve that in >> any shape or form - it really does not matter what order I do the merges >> in, the fact of the matter is that there is no ordering or dependence >> between your patches and Will's, so there's no guarantee that your code >> will see a properly functioning setup_mm_for_reboot. >> >> If you actually depend on something in some other tree, you need to have >> it merged into your tree _before_ the objects which depend on it. >> >> So, what the above comment admits to me is that "what I'm asking you to >> pull is known to be broken if a git-bisect hits one of these commits, >> but I don't care." >> >> As I said, I will merge it this time around, but next time I won't. > > Well, I might do if you told me that your changes conflict with Will's > idmap changes in arch/arm/mm/idmap.c: > > ++<<<<<<< HEAD > +extern char __idmap_text_start[], __idmap_text_end[]; > ++======= > + #ifdef CONFIG_SMP > + static void idmap_del_pmd(pud_t *pud, unsigned long addr, unsigned long end) > + { > + pmd_t *pmd; > + > + if (pud_none_or_clear_bad(pud)) > + return; > + pmd = pmd_offset(pud, addr); > + pmd_clear(pmd); > + } > ++>>>>>>> cf9a53fb7cf8dd2fd2b5d7bb07434bd2ebeb5a8f > > I'm assuming that idmap_del_pmd() just gets deleted? > > Alternatively, if you wish to fix the dependencies of your commits and > resubmit a pull request, let me know. Indeed, idmap_del_pmd() is deleted. But a better approach if you already merged Will's idmap branch is for me to rebase LPAE against your devel-stable. I'll solve the conflicts and test the branch before sending another pull request. If you prefer this, is there some commit I can rebase LPAE onto?
On Tue, Dec 06, 2011 at 10:24:28PM +0000, Catalin Marinas wrote: > On 6 December 2011 20:34, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Tue, Dec 06, 2011 at 03:29:55PM +0000, Catalin Marinas wrote: > >> I updated the series with an additional signed-off-by to your patch. The > >> code is unchanged. Could you please pull it again? > > > > I will for this time only, but note - last time you said: > > > >> If there are no further comments, could you please pull the ARM LPAE > >> branch below? They should be merged after Will's idmap patches (no real > >> conflict, just correctly functioning setup_mm_for_reboot). > > > > Now, the thing is, merging it after Will's patches won't solve that in > > any shape or form - it really does not matter what order I do the merges > > in, the fact of the matter is that there is no ordering or dependence > > between your patches and Will's, so there's no guarantee that your code > > will see a properly functioning setup_mm_for_reboot. > > This situation is not unique to my LPAE patches (git bisect doesn't > always go well merges). We should _strive_ to make git bisect work where possible. It's merge windows where the most breakage happens, and if they're not bisectable then that kills a valuable debugging tool. With more and more thousands of commits going in to each merge window, it's becoming increasingly important that it works. > But if you want, there other solutions: > > 1. I can rebase the LPAE branch on top of your devel-stable _after_ > you merged Will's patches. Then I send another pull request. > > 2. Only merge the LPAE patches without the one adding the Kconfig > option. Later apply the Kconfig patch. > > > If you actually depend on something in some other tree, you need to have > > it merged into your tree _before_ the objects which depend on it. > > Not easy in this instance. AFAIK Will's code relied on some reset > patches from you, so Will just used your devel-stable branch. I'll grant you that it hasn't been easy, because Will has had to rebase his git tree at my request, because of the breakage of Samsung and OMAP with Marc's irqchip-consolidation patches. Nevertheless, we're at -rc3, which means there is no reason to rush this. There is no problem with you waiting until Will's idmap patches have been merged (they have) and then moving on from there. > > As I said, I will merge it this time around, but next time I won't. > > As I also said above, there are solutions. What it is really needed is > _better_ collaboration during merges and _discussing_ the best > approach rather than threatening that there won't be other pulls. And what about the email from Philippe on me rejecting your first pull request? There is no need to try to exert commercial pressures when the problems are legal and technical.
On Tue, Dec 06, 2011 at 11:16:30PM +0000, Russell King - ARM Linux wrote: > On Tue, Dec 06, 2011 at 10:24:28PM +0000, Catalin Marinas wrote: > > On 6 December 2011 20:34, Russell King - ARM Linux > > <linux@arm.linux.org.uk> wrote: > > > As I said, I will merge it this time around, but next time I won't. > > > > As I also said above, there are solutions. What it is really needed is > > _better_ collaboration during merges and _discussing_ the best > > approach rather than threatening that there won't be other pulls. > > And what about the email from Philippe on me rejecting your first pull > request? There is no need to try to exert commercial pressures when the > problems are legal and technical. I'm not discussing Philippe's email publicly (as it was in private). >From my perspective, the tone of your email suggested more of a good opportunity not to merge LPAE (and in subsequent emails doing ARM a great favour by only accepting it this time) rather than a more constructive 'fix this before merging'. Since you mentioned commercial pressures, yes, there are many, but I'm not sure you are aware of all of them (many companies haven't publicly announced their plans, though they have very aggressive targets). I am asked on a weekly basis when features like LPAE (or other bug-fixes - TLBs, ASIDs) get into mainline. I can't really answer as it does not depend on me and I don't have any visibility on when it would happen. Long delays is not a problem, it's predictability that matters. As a temporary (can be 1-2 years) workaround, I have to recommend them to use one of my development trees (e.g. linux-arm-arch). On the positive side, more and more companies rely on the mainline kernel and many of them don't even trust a set of patches unless it's in the mainline kernel. The Linux community complained in the past that companies forked the kernel and used old versions but now we should really help those that want to stay up to date with the latest mainline.
On Wed, Dec 07, 2011 at 10:59:13AM +0000, Catalin Marinas wrote: > On Tue, Dec 06, 2011 at 11:16:30PM +0000, Russell King - ARM Linux wrote: > > On Tue, Dec 06, 2011 at 10:24:28PM +0000, Catalin Marinas wrote: > > > On 6 December 2011 20:34, Russell King - ARM Linux > > > <linux@arm.linux.org.uk> wrote: > > > > As I said, I will merge it this time around, but next time I won't. > > > > > > As I also said above, there are solutions. What it is really needed is > > > _better_ collaboration during merges and _discussing_ the best > > > approach rather than threatening that there won't be other pulls. > > > > And what about the email from Philippe on me rejecting your first pull > > request? There is no need to try to exert commercial pressures when the > > problems are legal and technical. > > I'm not discussing Philippe's email publicly (as it was in private). > > >From my perspective, the tone of your email suggested more of a good > opportunity not to merge LPAE (and in subsequent emails doing ARM a > great favour by only accepting it this time) rather than a more > constructive 'fix this before merging'. For fuck sake Catalin - which bit of "I MERGED IT AND THEN I HAD TO THROW IT OUT" do you not understand? I said precisely what needed to be said. You fucked up because you didn't follow the DCO. Plain and simple. And I had to throw it out of an already published devel-stable branch which had been published for 9+ hours. Of course I'd be annoyed by that - that mistake means I *HAD* to break the promise I made to the rest of the community that I would never rewind the devel-stable branch, ever. For someone who has been working on the kernel for a great number of years, you show quite a lack of understanding of community process when you slip up like that, and _then_ have the audacity to say that it's a 'minor issue' (your words not mine.) > Since you mentioned commercial pressures, yes, there are many, but I'm > not sure you are aware of all of them (many companies haven't publicly > announced their plans, though they have very aggressive targets). I am > asked on a weekly basis when features like LPAE (or other bug-fixes - > TLBs, ASIDs) get into mainline. I can't really answer as it does not > depend on me and I don't have any visibility on when it would happen. > Long delays is not a problem, it's predictability that matters. As a > temporary (can be 1-2 years) workaround, I have to recommend them to use > one of my development trees (e.g. linux-arm-arch). None of that should _ever_ be used as any kind of motivation to get something into mainline if it's not ready to be there. Clearly, from the issues raised with your tree thus far it wasn't actually ready. Ready does not mean "it builds for me". Ready means it builds, it's tested, it's got proper commit messages, attributations and sign-offs. If it's not at that stage, then quite simply it is not ready. There's no two ways about that. And as I've already explained to you several times now, since sign-offs are seen by many to be a legal statement, incorrect sign-offs are not a minor problem. Had it been correct, I would not have had to throw it out of my tree. So please, get it through your head that _had_ the sign-offs been correct we wouldn't be having this discussion and your tree wouldn't have been thrown out - and it would've actually been merged _before_ Will's idmap changes.
Hi Russell, I updated the series with an additional signed-off-by to your patch. The code is unchanged. Could you please pull it again? Thanks. The following changes since commit 5611cc4572e889b62a7b4c72a413536bf6a9c416: Linux 3.2-rc4 (2011-12-01 14:56:01 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/linux.git for-next Catalin Marinas (12): ARM: pgtable: Fix compiler warning in ioremap.c introduced by nopud ARM: LPAE: Move page table maintenance macros to pgtable-2level.h ARM: LPAE: Move the FSR definitions to separate files ARM: LPAE: Factor out classic-MMU specific code into proc-v7-2level.S ARM: LPAE: Introduce the 3-level page table format definitions ARM: LPAE: Page table maintenance for the 3-level format ARM: LPAE: MMU setup for the 3-level page table format ARM: LPAE: Invalidate the TLB before freeing the PMD ARM: LPAE: Add fault handling support ARM: LPAE: Add context switching support ARM: LPAE: Add identity mapping support for the 3-level page table format ARM: LPAE: Add the Kconfig entries Russell King (1): ARM: pgtable: switch to use pgtable-nopud.h Will Deacon (2): ARM: LPAE: add ISBs around MMU enabling code ARM: LPAE: mark memory banks with start > ULONG_MAX as highmem arch/arm/Kconfig | 2 +- arch/arm/boot/compressed/head.S | 1 + arch/arm/include/asm/assembler.h | 11 ++ arch/arm/include/asm/page.h | 4 + arch/arm/include/asm/pgalloc.h | 26 ++++- arch/arm/include/asm/pgtable-2level.h | 41 ++++++ arch/arm/include/asm/pgtable-3level-hwdef.h | 77 ++++++++++++ arch/arm/include/asm/pgtable-3level-types.h | 70 +++++++++++ arch/arm/include/asm/pgtable-3level.h | 155 +++++++++++++++++++++++ arch/arm/include/asm/pgtable-hwdef.h | 4 + arch/arm/include/asm/pgtable.h | 43 +------ arch/arm/include/asm/proc-fns.h | 21 +++ arch/arm/include/asm/system.h | 8 ++ arch/arm/include/asm/tlb.h | 12 ++- arch/arm/kernel/head.S | 47 +++++++- arch/arm/kernel/hw_breakpoint.c | 8 +- arch/arm/kernel/sleep.S | 2 + arch/arm/mm/Kconfig | 17 +++ arch/arm/mm/alignment.c | 2 +- arch/arm/mm/context.c | 19 +++- arch/arm/mm/fault.c | 111 +++-------------- arch/arm/mm/fault.h | 27 ++++- arch/arm/mm/fsr-2level.c | 78 ++++++++++++ arch/arm/mm/fsr-3level.c | 68 ++++++++++ arch/arm/mm/idmap.c | 34 +++++- arch/arm/mm/ioremap.c | 39 ++++--- arch/arm/mm/mmu.c | 46 +++++++- arch/arm/mm/pgd.c | 51 +++++++- arch/arm/mm/proc-macros.S | 5 +- arch/arm/mm/proc-v7-2level.S | 171 ++++++++++++++++++++++++++ arch/arm/mm/proc-v7-3level.S | 150 +++++++++++++++++++++++ arch/arm/mm/proc-v7.S | 177 +++------------------------ 32 files changed, 1204 insertions(+), 323 deletions(-) create mode 100644 arch/arm/include/asm/pgtable-3level-hwdef.h create mode 100644 arch/arm/include/asm/pgtable-3level-types.h create mode 100644 arch/arm/include/asm/pgtable-3level.h create mode 100644 arch/arm/mm/fsr-2level.c create mode 100644 arch/arm/mm/fsr-3level.c create mode 100644 arch/arm/mm/proc-v7-2level.S create mode 100644 arch/arm/mm/proc-v7-3level.S