Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 5 Jul 2018 07:30:11 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Yang, Bin" <bin.yang@intel.com>
cc:     "mingo@kernel.org" <mingo@kernel.org>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCH] x86/mm: fix cpu stuck issue in
 __change_page_attr_set_clr
In-Reply-To: <E0B0C3E3-5AA5-4AEC-A11A-B55040A007C3@intel.com>
Message-ID: <alpine.DEB.2.21.1807050708320.1579@nanos.tec.linutronix.de>
References: <1530180340-18593-1-git-send-email-bin.yang@intel.com> <alpine.DEB.2.21.1807031442030.1601@nanos.tec.linutronix.de> <0131cecd5d0456c2a109f4b8bdbfe558389671dd.camel@intel.com> <alpine.DEB.2.21.1807040849520.1816@nanos.tec.linutronix.de>
 <3224aae1d09788aba687fd7bd9e088f233016fc8.camel@intel.com> <alpine.DEB.2.21.1807041119260.1816@nanos.tec.linutronix.de> <34a76fad5c43c11ce8ceb491537b453d8053bdb2.camel@intel.com> <alpine.DEB.2.21.1807041221110.1816@nanos.tec.linutronix.de>
 <E0B0C3E3-5AA5-4AEC-A11A-B55040A007C3@intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 5 Jul 2018, Yang, Bin wrote:

Please do not top post.

> This is what my new patch tries to improve.

> On 04/07/2018, 10:02 PM, "Thomas Gleixner" <tglx@linutronix.de> wrote:
> 
>       The check loop itself is stupid as well. Instead of looping in 4K steps
>       the thing can be rewritten to check for overlapping ranges and then check
>       explicitely for those. If there is no overlap in the first place the
>       whole loop thing can be avoided, but that's a pure optimization and needs
>       more thought than the straight forward and obvious solution to the
>       problem at hand.

Sorry, I don't see in which way your patch would improve the check loop.

It tries to avoid the checks for a consecutive invocation of CPA on the
same address range, but it does it in a convoluted way instead of doing the
obvious.

And in no way it does improve the situation when the check needs to be
done. Here is the relevant snippet:

+       if (!do_split_cache &&
+           address_cache >= addr && address_cache < nextpage_addr &&
+           pgprot_val(new_prot) == pgprot_val(old_prot)) {
+               do_split = do_split_cache;
+               goto out_unlock;
+       }

That only ever avoids the check loop when:

     1) the previous call cleared do_split_cache

     2) the address is within the range of the previous call

     3) the pgprot_vals match

while moving the pgprot_val check and the alignment check _before_ the loop
avoids even more pointless invocations of the loop and is obvious and
correct without voodoo logic.

The check loop is with both your and my variant absolutely the same and it
does not get any smarter or more performant when it's entered.

That is a totally different change which needs to do the following:

	if (overlaps_check_regions(address, numpages)) 
		check_overlapping_regions(address, numpages);

and that can be done smart without massive looping in 4K steps, but we
really want to analyze _first_ whether the checks are just silently fixing
up sloppy callers and whether they are needed at all.

Thanks,

	tglx