Message-ID: <20150708105143.GA11055@gmail.com>
Date:	Wed, 8 Jul 2015 12:51:43 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Kees Cook <keescook@...omium.org>,
	"H. Peter Anvin" <hpa@...or.com>, Baoquan He <bhe@...hat.com>,
	linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 00/42] x86: updated patches for kaslr and setup_data etc
 for v4.3


* Yinghai Lu <yinghai@...nel.org> wrote:

> Those patches are rebased on v4.2-rc1 that I sent before but were rejected by 
> Ingo on changelog.

What does 'on changelog' mean?

So I rejected them because the patches had poor organization and poor 
explanations. I just re-checked this series and the organization is still poor, 
and it has similar problems which I pointed out 3 months ago - which need to be 
fixed. (Due to that I couldn't even begin to evaluate any deeper merits (or 
problems) of the changes.)

> Kees Cook said that he would like to give a try to make improvement on changelog 
> to get things moving.

So if that means that Kees is willing to write proper changelogs, then in the mail 
below I'm pointing out the most glaring problem with your patches, but there are 
other details missing as well - as usual I have to (sadly) say.

But if Kees sends truly new changelogs that meet the technical requirements below 
I'll have a look - but the simple Acked-by's from Kees in this thread won't 
suffice to fix the deficiencies.


Thanks,

	Ingo

=========================================>

In your changelogs you are typically only talking only about the
low level code and about low level symptoms, while in contrast to
that the primary question with _any_ change to the kernel is always:

   Why are we changing the kernel, what bad high level behavior can a
   user observe if the bug or problem is not fixed?

Your descriptions totally ignore the high level effects of the bug on 
the system and on users of the machine, and you fail to describe them 
properly. You totally concentrate on the low level: your descriptions 
are missing the forest from all the trees.

That makes 90% of your patch descriptions useless.

In fact, 90% of your patches had that deficiency and had it for the 
past 4 years, non-stop, and maintainers were complaining to you about 
that, non-stop as well. Do you think maintainers enjoy complaining 
about deficiencies? Do you wonder why maintainers were forced to 
simply stop looking at any complex series from yours after you refused 
to change?

> will refresh the patchset.

So let me try this again, one very last time.

That sentence demonstrates your problem: it's not a 'refresh' your 
patches need, but a 'hard reboot', a totally new viewpoint that 
concentrates on what matters: that zooms out of the small details of 
the patch!

For any serious change to the Linux kernel, analyzing small details is 
fine and required as well, AFTER the high level has been discussed 
properly:

  - What happened, what high level concern motivates you to change the 
    Linux kernel?

       And no, starting a changelog with:

          commit e6023367d779 ("x86, kaslr: Prevent .bss from 
          overlaping initrd") introduced one run_size for kaslr.

       is not 'high level' in any way, it talks about code in the 
       first sentence! Talking about code, not talking about high 
       level concerns is a BUG().

  - What was the previous (often bad) high level behavior of the
    kernel?

       And no, 'KASLR will not find a proper area' is NOT a high level
       description, it's a very low level description! Not discussing 
       high level behavior of the kernel in a changelog is a BUG().

  - What new high level behavior do we want to happen instead?

       And no, saying that 'KASLR should be passed init_size instead
       of run_size' is not a description of desired new high level
       behavior! Not discussing the desired high level behavior of the 
       kernel in a changelog is a BUG().

  - What was the high level design of the old code, was that design
    fine, should it be changed, and if yes, in what way?

       Note that we describe the high level design even if we don't
       change it, partly to give context to the reader, partly to 
       prove that you know what you are doing, to build trust in your 
       patch! Not discussing the old (and new) design of that area of 
       the kernel is a BUG().

and only then do we:

  - Describe the old implementation, and describe how the new
    implementation works in all that context.

       Here is where 99.9% of your existing changelogs fit in.
       Put differently: your changelogs are missing the first 3 
       essential components of a changelog.

       And note, mentioning 'run_size' in a low level description is 
       fine. Mentioning it in a high level description is a BUG(): 
       that is why Boris was trying to change your changelogs into 
       spoken English, to recover some of that missing high level 
       context. Maintainers like Boris should not be forced to do 
       that, you are supposed to offer this with any significant 
       patch, as a passport to prove that your changes to the code are 
       worth looking at.

And yes, we do it in that order. Take a look at a recent single line 
change Linus made in 53da3bc2ba9e48, attached to this mail.

Check how the changelog is structured: it discusses high level 
concepts first. It's a _ONE LINER FIX_ from Linus, yet it's spread 
over 8 paragraphs and 50 lines, because the change justified that kind 
of description.

And observe the moment when Linus, in his 8 paragraphs, 50 lines 
description starts talking about low level implementational details, 
when he mentions lines of code, function names, such as 
do_numa_page(), 'pte_write()' or 'pte_dirty()'.

He doesnt!

It's not needed for a one-liner most of the time: but the high level 
concepts around that code are very much necessary to convey.

Now contrast that with your changelogs: your changelogs are stock full 
of non-English phrases resembling code more than a fluid description, 
they are stock full of low level details, mentioning of function 
names, variables and fields with no high level context conveyed 
whatsoever.

Let me try to put it to you in a way that might come across: when I 
run maintainer code over your changelogs it runs into an instant BUG() 
most of the time, forcing me to drop your patches.

High level discussion of old behavior, new behavior, old design and 
new design isn't just some detail to be slapped on a change after the 
fact, this is a serious and required technological aspect of the Linux 
kernel, and 90% of your patches are buggy in that respect.

In fact, I noticed that both your descriptions and your followups to 
them are totally lacking the high level viewpoint!

Either you:

   - refuse to think in high level concepts when you are writing 
     patches, in which case we need to keep your patches away from
     the Linux kernel,

   - or you are unwilling to document such high level thinking 
     processes, in which case we need to keep your patches away from 
     the Linux kernel as well.

If your appoach to writing kernel patches does not change then I will 
be forced to take action, currently you are this -->.<-- close to 
being filtered out from my default inbox for your total refusal to 
change the technology of how you are writing patches.

Thanks,

	Ingo

[ Sample 'good' changelog from Linus: ]

======================>
>From 53da3bc2ba9e4899f32707b5cd7d18421b943687 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@...ux-foundation.org>
Date: Thu, 12 Mar 2015 08:45:46 -0700
Subject: [PATCH] mm: fix up numa read-only thread grouping logic

Dave Chinner reported that commit 4d9424669946 ("mm: convert
p[te|md]_mknonnuma and remaining page table manipulations") slowed down
his xfsrepair test enormously.  In particular, it was using more system
time due to extra TLB flushing.

The ultimate reason turns out to be how the change to use the regular
page table accessor functions broke the NUMA grouping logic.  The old
special mknuma/mknonnuma code accessed the page table present bit and
the magic NUMA bit directly, while the new code just changes the page
protections using PROT_NONE and the regular vma protections.

That sounds equivalent, and from a fault standpoint it really is, but a
subtle side effect is that the *other* protection bits of the page table
entries also change.  And the code to decide how to group the NUMA
entries together used the writable bit to decide whether a particular
page was likely to be shared read-only or not.

And with the change to make the NUMA handling use the regular permission
setting functions, that writable bit was basically always cleared for
private mappings due to COW.  So even if the page actually ends up being
written to in the end, the NUMA balancing would act as if it was always
shared RO.

This code is a heuristic anyway, so the fix - at least for now - is to
instead check whether the page is dirty rather than writable.  The bit
doesn't change with protection changes.

NOTE! This also adds a FIXME comment to revisit this issue,

Not only should we probably re-visit the whole "is this a shared
read-only page" heuristic (we might want to take the vma permissions
into account and base this more on those than the per-page ones, and
also look at whether the particular access that triggers it is a write
or not), but the whole COW issue shows that we should think about the
NUMA fault handling some more.

For example, maybe we should do the early-COW thing that a regular fault
does.  Or maybe we should accept that while using the same bits as
PROTNONE was a good thing (and got rid of the specual NUMA bit), we
might still want to just preseve the other protection bits across NUMA
faulting.

Those are bigger questions, left for later.  This just fixes up the
heuristic so that it at least approximates working again.  More analysis
and work needed.

Reported-by: Dave Chinner <david@...morbit.com>
Tested-by: Mel Gorman <mgorman@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Aneesh Kumar <aneesh.kumar@...ux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@...nel.org>,
Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
---
 mm/memory.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index 8068893697bb..411144f977b1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3072,8 +3072,13 @@ static int do_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	 * Avoid grouping on DSO/COW pages in specific and RO pages
 	 * in general, RO pages shouldn't hurt as much anyway since
 	 * they can be in shared cache state.
+	 *
+	 * FIXME! This checks "pmd_dirty()" as an approximation of
+	 * "is this a read-only page", since checking "pmd_write()"
+	 * is even more broken. We haven't actually turned this into
+	 * a writable page, so pmd_write() will always be false.
 	 */
-	if (!pte_write(pte))
+	if (!pte_dirty(pte))
 		flags |= TNF_NO_GROUP;
 
 	/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists