x86/mm: fix cpu stuck issue in __change_page_attr_set_clr

This issue can be easily triggered by free_initmem() functuion on x86_64 cpu.

If cpu supports "pdpe1gb", kernel will try to use 1G large page. In worst case, it needs to check every 4K page in 1G range in try_preserve_large_page function. If __change_page_attr_set_clr needs to change lots of 4K pages, cpu will be stuck for long time.

arch/x86/mm/pageattr.c +1 -1

 		return 1;
+	}
 
-	spin_lock(&pgd_lock);
 	/*
 	 * Check for races, another CPU might have split this page
 	 * up already:

@@ -627,13 +631,25 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 
 	new_prot = static_protections(req_prot, address, pfn);
 

arch/x86/mm/pageattr.c +2 -2

+		goto out_unlock;
+	}
 	/*
 	 * We need to check the full range, whether
 	 * static_protection() requires a different pgprot for one of
 	 * the pages in the range we try to preserve:
 	 */
-	addr = address & pmask;
-	pfn = old_pfn;
 	for (i = 0; i < (psize >> PAGE_SHIFT); i++, addr += PAGE_SIZE, pfn++) {
 		pgprot_t chk_prot = static_protections(req_prot, addr, pfn);
 

Bin,

On Thu, 28 Jun 2018, Bin Yang wrote:

thanks for submitting this.

Quoted 3 lines

This issue can be easily triggered by free_initmem() functuion
on

    x86_64 cpu.

Please do not use the output of git show for submitting a patch. Use git
format-patch(1).

Quoted 7 lines

If cpu supports "pdpe1gb", kernel will try to use 1G large
page.
In worst case, it needs to check every 4K page in 1G range in try_preserve_large_page function. If __change_page_attr_set_clr needs to change lots of 4K pages, cpu will be stuck for long
time.

Unfortunately you missed to describe what the actual interface function is
which is called from a driver or whatever, including the parameters.

arch/x86/mm/pageattr.c static int · line 556

Quoted 7 lines

arch/x86/mm/pageattr.c +2 -0

@@ -552,16 +552,20 @@ static int
 try_preserve_large_page(pte_t *kpte, unsigned long address,
 			struct cpa_data *cpa)
 {
+	static unsigned long address_cache;
+	static unsigned long do_split_cache = 1;

What are the life time and protection rules of these static variables and
why are they static in the first place.

Quoted 8 lines

arch/x86/mm/pageattr.c +0 -0

 	unsigned long nextpage_addr, numpages, pmask, psize, addr,
 pfn, old_pfn;
 	pte_t new_pte, old_pte, *tmp;
 	pgprot_t old_prot, new_prot, req_prot;
 	int i, do_split = 1;
 	enum pg_level level;
 

arch/x86/mm/pageattr.c static int · line 565

Quoted 4 lines

arch/x86/mm/pageattr.c +3 -1

-	if (cpa->force_split)
+	spin_lock(&pgd_lock);
+	if (cpa->force_split) {
+		do_split_cache = 1;

Returns with pgd_lock held which will immediately lockup the system on the
next spin_lock(&pgd_lock) operation.

Also what's the point of storing the already available information of cpa->force_split in yet another variable? This enforces taking the lock on
every invocation for no reason.

Quoted 13 lines

arch/x86/mm/pageattr.c +1 -1

 		return 1;
+	}
 
-	spin_lock(&pgd_lock);
 	/*
 	 * Check for races, another CPU might have split this page

arch/x86/mm/pageattr.c +0 -0

 	 * up already:

@@ -627,13 +631,25 @@ try_preserve_large_page(pte_t *kpte, unsigned
 long address,
 
 	new_prot = static_protections(req_prot, address, pfn);
 

Quoted 13 lines

arch/x86/mm/pageattr.c +4 -0

+	addr = address & pmask;
+	pfn = old_pfn;
+	/*
+	 * If an address in same range had been checked just now,

re-use the

(quoted diff) +1 -0

+	 * cache value without full range check. In the worst

case, it needs to

(quoted diff) +1 -0

+	 * check every 4K page in 1G range, which causes cpu stuck

for long

arch/x86/mm/pageattr.c +4 -0

+	 * time.
+	 */
+	if (!do_split_cache &&
+	    address_cache >= addr && address_cache < nextpage_addr

Quoted 1 line

&&

On the first call, address_cache contains 0. On any subsequent call

address_caches contains the address of the previous invocation of try_preserve_large_page().

But addr = address & pmask! So you are comparing apples and oranges. Not
sure how that is supposed to work correctly.

arch/x86/mm/pageattr.c try_preserve_large_page(pte_t *kpte, unsigned long address, · line 645

Quoted 3 lines

arch/x86/mm/pageattr.c +2 -0

+	    pgprot_val(new_prot) == pgprot_val(old_prot)) {
+		do_split = do_split_cache;

This is an very obscure way to write:

   	        do_split = 0;

because do_split_cache must be 0 otherwise it cannot reach that code path.

Quoted 16 lines

arch/x86/mm/pageattr.c +2 -0

+		goto out_unlock;
+	}
 	/*
 	 * We need to check the full range, whether
 	 * static_protection() requires a different pgprot for one

of

arch/x86/mm/pageattr.c +0 -2

 	 * the pages in the range we try to preserve:
 	 */
-	addr = address & pmask;
-	pfn = old_pfn;
 	for (i = 0; i < (psize >> PAGE_SHIFT); i++, addr +=

PAGE_SIZE, pfn++) {
pgprot_t chk_prot = static_protections(req_prot,
addr, pfn);

arch/x86/mm/pageattr.c try_preserve_large_page(pte_t *kpte, unsigned · line 692

Quoted 7 lines

arch/x86/mm/pageattr.c +2 -0

@@ -670,6 +686,8 @@ try_preserve_large_page(pte_t *kpte, unsigned
 long address,
 	}
 
 out_unlock:
+	address_cache = address;
+	do_split_cache = do_split;
 	spin_unlock(&pgd_lock);

So here you store the 'cache' values and while this code suggests that it
protects the 'cache' via pgd_lock (due to lack of comments), the protection
is actually cpa_lock.

But, that cache information stays around when cpa_lock is dropped, i.e. when the current (partial) operation has been done and this information stays stale for the next user. That does not make sense.

I'm still puzzled how that can ever happen at all and which problem you are
trying to solve.

The first invocation of try_to_preserve_large_page() will either preserve
the 1GB page and consume the number of 4K pages which fit into it, or it
splits it into 2M chunks and tries again. The retry will either preserve
the 2M chunk and and consume the number of 4K pages which fit into it, or
it splits it into 4K pages. Once split into 4K, subsequent invocations will
return before reaching the magic cache checks.

Can you please describe the call chain, the driver facing function used and
the parameters which are handed in?

Thanks,

tglx

thanks for reviewing my patch. I will update a new patch version based on your feedback soon

Below is the new commit comment I will use in new patch version soon

Quoted 2 lines

Please do not use the output of git show for submitting a patch. Use git format-patch(1). >

I use "git send-email -1" to submit patch for review. Should I run "git format-patch" first and send the patch as email?

===========
If cpu supports "pdpe1gb", kernel will try its best to use 1G big page. When changing a 4K page attr inside 1G page range, __change_page_attr() will either consume this 4K page into the 1G page, or it splits 1G page into 2M pages and tries again. The retry will either consume the 4K page into a 2MB page, or it splits 2MB page into 4K pages. try_preserve_large_page() is called by __change_page_attr() to decide

it by checking all 4K pages inside the big page one by one.

This issue is discovered during kernel boot time optimization. Sometimes, free_initmem() returns after about 600ms on my x86_64 board with 4GB memory.

Since it is a common issue of x86_64, it can be reproduced by qemu too. We can add some logs in try_preserve_large_page() function to measure the loop count and elapsed time. Please make sure the host CPU has "pdpe1gb" flag and run below qemu command to reproduce it:

qemu-system-x86_64 -enable-kvm -cpu host -serial mon:stdio -m 4G -nographic -kernel bzImage -initrd ramdisk.img -append "console=ttyS0"

Since default x86_64 kernel enables CONFIG_RANDOMIZE_BASE, it needs to

try many times to let init memory be randomized in a 1G page range.

This patch try to cache the last address which had been checked just

now. If the next address is in same big page with same attr, the cache will be used without full range check.

Quoted 10 lines

arch/x86/mm/pageattr.c static int · line 556

Quoted 6 lines

arch/x86/mm/pageattr.c +2 -0

@@ -552,16 +552,20 @@ static int
 try_preserve_large_page(pte_t *kpte, unsigned long address,
 			struct cpa_data *cpa)
 {
+	static unsigned long address_cache;
+	static unsigned long do_split_cache = 1;

What are the life time and protection rules of these static variables and
why are they static in the first place.

they will be protected by pgd_lock. They only cache previous "do_split" result and will be refreshed every time.

Quoted 7 lines

Quoted 7 lines

arch/x86/mm/pageattr.c +0 -0

 	unsigned long nextpage_addr, numpages, pmask, psize, addr,
 pfn, old_pfn;
 	pte_t new_pte, old_pte, *tmp;
 	pgprot_t old_prot, new_prot, req_prot;
 	int i, do_split = 1;
 	enum pg_level level;
 

Quoted 8 lines

arch/x86/mm/pageattr.c static int · line 565

Quoted 4 lines

arch/x86/mm/pageattr.c +3 -1

-	if (cpa->force_split)
+	spin_lock(&pgd_lock);
+	if (cpa->force_split) {
+		do_split_cache = 1;

Returns with pgd_lock held which will immediately lockup the system on the
next spin_lock(&pgd_lock) operation.

I am so sorry to make such stupid mistake. force_split was not hit on my board :(

Quoted 1 line

Quoted 4 lines

Also what's the point of storing the already available information of cpa->force_split in yet another variable? This enforces taking the lock on
every invocation for no reason.

As you know, do_split is initialized as 1. If do_split_cache == 1, the cache value will not be used. If force_split == 1, cache value should be expired. Since do_split_cache is protected by this lock, it needs to task this lock here.

Quoted 6 lines

Quoted 6 lines

(quoted diff) +2 -0

+	/*
+	 * If an address in same range had been checked just now,

re-use the

(quoted diff) +1 -0

+	 * cache value without full range check. In the worst

case, it needs to

(quoted diff) +1 -0

+	 * check every 4K page in 1G range, which causes cpu stuck

Quoted 8 lines

Quoted 6 lines

for long

arch/x86/mm/pageattr.c +4 -0

+	 * time.
+	 */
+	if (!do_split_cache &&
+	    address_cache >= addr && address_cache < nextpage_addr

&&

On the first call, address_cache contains 0. On any subsequent call

on the first call, do_split_cache is 1. if do_split_cache == 1, address_cache will not be used.

Quoted 15 lines

arch/x86/mm/pageattr.c try_preserve_large_page(pte_t *kpte, unsigned · line 692

Quoted 8 lines

arch/x86/mm/pageattr.c +2 -0

@@ -670,6 +686,8 @@ try_preserve_large_page(pte_t *kpte, unsigned
 long address,
 	}
 
 out_unlock:
+	address_cache = address;
+	do_split_cache = do_split;
 	spin_unlock(&pgd_lock);

So here you store the 'cache' values and while this code suggests that it
protects the 'cache' via pgd_lock (due to lack of comments), the protection
is actually cpa_lock.

Quoted 3 lines

But, that cache information stays around when cpa_lock is dropped, i.e. when the current (partial) operation has been done and this information stays stale for the next user. That does not make sense.

__change_page_attr is the only function to change page attr and try_preserve_large_page will be called every time for big page check. If a big page had been splitted, it will not be merged again. So it is safe to cache previous result in try_preserve_large_page() function.

Quoted 3 lines · Bin

thanks for reviewing my patch. I will update a new patch version based
on your feedback soon

Take your time.

Quoted 10 lines · Thomas Gleixner

Below is the new commit comment I will use in new patch version soon

Quoted 3 lines

Please do not use the output of git show for submitting a patch. Use
git format-patch(1). >

I use "git send-email -1" to submit patch for review. Should I run "git
format-patch" first and send the patch as email?

git send-email is fine, but it should not result in the changelog being
indented by a bunch of spaces. That's why I assumed that you used git show
because that's exacty the format. I'm not using it, so I can't give you
advise there.

Quoted 11 lines

===========
If cpu supports "pdpe1gb", kernel will try its best to use 1G big page.
When changing a 4K page attr inside 1G page range, __change_page_attr()
will either consume this 4K page into the 1G page, or it splits 1G page
into 2M pages and tries again. The retry will either consume the 4K page into a 2MB page, or it splits 2MB page into 4K pages. try_preserve_large_page() is called by __change_page_attr() to decide

I know what calls try_preserve_large_page(), but you still fail to explain
the full call chain including parameters which I asked for.

Quoted 1 line

it by checking all 4K pages inside the big page one by one.

After your change this will still happen. You just shortcut the inner workings, but you are still not explaining why the shortcut is necessary in
the first place.

The try_preserve_large_page() logic should not need any of this unless
there is a subtle implementation bug. If that's the case, then the bug
needs to be isolated and fixed and not papered over by magic short cuts.

Quoted 12 lines

This issue is discovered during kernel boot time optimization. Sometimes, free_initmem() returns after about 600ms on my x86_64 board
with 4GB memory.

Since it is a common issue of x86_64, it can be reproduced by qemu too.
We can add some logs in try_preserve_large_page() function to measure
the loop count and elapsed time. Please make sure the host CPU has "pdpe1gb" flag and run below qemu command to reproduce it:

Quoted 6 lines

qemu-system-x86_64 -enable-kvm -cpu host -serial mon:stdio -m 4G -nographic -kernel bzImage -initrd ramdisk.img -append "console=ttyS0"

Since default x86_64 kernel enables CONFIG_RANDOMIZE_BASE, it needs to

Huch? What as this to do with randomize base?

Quoted 1 line

try many times to let init memory be randomized in a 1G page range.

And no, I'm not interested in random qemu commands and adding logs into
something. You already did the investigation, but you fail to provide the
information. And I'm not asking for random timing logs, I ask about a proper explanation why this happens even if it's supposed not to happen.

Quoted 2 lines

This patch try to cache the last address which had been checked just

See Documentation/process/submitting-patches.rst and search for 'This patch' ....

Quoted 15 lines

now. If the next address is in same big page with same attr, the cache
will be used without full range check.

Quoted 11 lines

arch/x86/mm/pageattr.c static int · line 556

Quoted 6 lines

arch/x86/mm/pageattr.c +2 -0

@@ -552,16 +552,20 @@ static int
 try_preserve_large_page(pte_t *kpte, unsigned long address,
 			struct cpa_data *cpa)
 {
+	static unsigned long address_cache;
+	static unsigned long do_split_cache = 1;

What are the life time and protection rules of these static variables
and
why are they static in the first place.

Quoted 3 lines

they will be protected by pgd_lock. They only cache previous "do_split"
result and will be refreshed every time.

So why is there no comment explaining this? And I'm still not convinced
about pgd_lock being the real protection. pgd_lock protects against concurrent page table manipulations, but it does not protect against concurrent calls of the change_page_attr logic at all. That's what cpa_lock
does.

Quoted 29 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/mm/pageattr.c +0 -0

 	unsigned long nextpage_addr, numpages, pmask, psize,
 addr,
 pfn, old_pfn;
 	pte_t new_pte, old_pte, *tmp;
 	pgprot_t old_prot, new_prot, req_prot;
 	int i, do_split = 1;
 	enum pg_level level;
 

Quoted 9 lines

arch/x86/mm/pageattr.c static int · line 565

Quoted 4 lines

arch/x86/mm/pageattr.c +3 -1

-	if (cpa->force_split)
+	spin_lock(&pgd_lock);
+	if (cpa->force_split) {
+		do_split_cache = 1;

Returns with pgd_lock held which will immediately lockup the system
on the
next spin_lock(&pgd_lock) operation.

I am so sorry to make such stupid mistake. force_split was not hit on
my board :(

Quoted 7 lines

Also what's the point of storing the already available information of
cpa->force_split in yet another variable? This enforces taking the
lock on
every invocation for no reason.

Quoted 7 lines

As you know, do_split is initialized as 1. If do_split_cache == 1, the
cache value will not be used. If force_split == 1, cache value should
be expired. Since do_split_cache is protected by this lock, it needs to
task this lock here.

No. It can be done w/o the lock and without touching the cache variable. cpa->force_split does not need any of it.

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +2 -0

+	/*
+	 * If an address in same range had been checked just
 now,

re-use the

(quoted diff) +1 -0

+	 * cache value without full range check. In the worst

case, it needs to

(quoted diff) +1 -0

+	 * check every 4K page in 1G range, which causes cpu

stuck
for long

arch/x86/mm/pageattr.c +1 -0

+	 * time.

Quoted 8 lines

Quoted 5 lines

arch/x86/mm/pageattr.c +3 -0

+	 */
+	if (!do_split_cache &&
+	    address_cache >= addr && address_cache <

nextpage_addr
&&

On the first call, address_cache contains 0. On any subsequent call

Quoted 2 lines

on the first call, do_split_cache is 1. if do_split_cache == 1, address_cache will not be used.

That's really not obvious and the whole code flow is obfuscated.

Quoted 22 lines

Quoted 16 lines

arch/x86/mm/pageattr.c try_preserve_large_page(pte_t *kpte, unsigned long address, · line 691

Quoted 9 lines

(quoted diff) +0 -0

@@ -670,6 +686,8 @@ try_preserve_large_page(pte_t *kpte,

unsigned
long address,

arch/x86/mm/pageattr.c +2 -0

 	}
 
 out_unlock:
+	address_cache = address;
+	do_split_cache = do_split;
 	spin_unlock(&pgd_lock);

So here you store the 'cache' values and while this code suggests that it
protects the 'cache' via pgd_lock (due to lack of comments), the protection
is actually cpa_lock.

Quoted 5 lines

But, that cache information stays around when cpa_lock is dropped,
i.e. when the current (partial) operation has been done and this information stays stale for the next user. That does not make sense.

Quoted 7 lines

__change_page_attr is the only function to change page attr and try_preserve_large_page will be called every time for big page check.
If a big page had been splitted, it will not be merged again. So it is
safe to cache previous result in try_preserve_large_page() function.

Come on. __change_page_attr() has a single call site:
__change_page_attr_set_clr() which itself is called from a ton of places.
And once cpa_lock is dropped in the loop, the 'cache' thing is not longer
protected and and stale.

Unless it's coherentely explained, this looks more like works by chance
than by design.

Thanks,

tglx

You are completely right. After considering, I think my patch is like a workaround but not real fix. I am trying to re-write a new patch without cache implementation.

Quoted 4 lines · Bin

You are completely right. After considering, I think my patch is like a
workaround but not real fix. I am trying to re-write a new patch without cache implementation.

Care to explain the actual problem coherently _before_ writing yet another
patch?

Thanks,

tglx

e820 table:
=================

[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009fbff]
usable
[    0.000000] BIOS-e820: [mem 0x000000000009fc00-0x000000000009ffff]
reserved
[    0.000000] BIOS-e820: [mem 0x00000000000f0000-0x00000000000fffff]
reserved
[    0.000000] BIOS-e820: [mem 0x0000000000100000-0x00000000bffdffff]
usable
[    0.000000] BIOS-e820: [mem 0x00000000bffe0000-0x00000000bfffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x00000000feffc000-0x00000000feffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x00000000fffc0000-0x00000000ffffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000013fffffff]

usable

call chain:
======================

...
=> free_init_pages(what="initrd" or "unused kernel", begin=ffff9b26b....000, end=ffff9b26c....000); begin and end addresses are random. The begin/end value above is just for reference.

=> set_memory_rw()
=> change_page_attr_set()
=> change_page_attr_set_clr()
=> __change_page_attr_set_clr(); cpa->numpages is 512 on my board if what=="unused kernel"
=> __change_page_attr()

=> try_preserve_large_page(); address=ffff9b26bfacf000, pfn=80000, level=3; and the check loop count is 262144, exit loop after 861 usecs on my board

the actual problem
===================
sometimes, free_init_pages returns after hundreds of secounds. The major impact is kernel boot time.

This is what my new patch tries to improve.

On 04/07/2018, 10:02 PM, "Thomas Gleixner" <[email protected]> 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 for the misunderstanding. I mean I will improve the check loop in patch v2. I just submitted patch v2. Thanks in advance for your kind review.

Please do not top post.

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.

Quoted 37 lines · Bin

e820 table:
=================

[    0.000000] BIOS-e820: [mem 0x0000000000000000-
0x000000000009fbff]
usable
[    0.000000] BIOS-e820: [mem 0x000000000009fc00-
0x000000000009ffff]
reserved
[    0.000000] BIOS-e820: [mem 0x00000000000f0000-
0x00000000000fffff]
reserved
[    0.000000] BIOS-e820: [mem 0x0000000000100000-
0x00000000bffdffff]
usable
[    0.000000] BIOS-e820: [mem 0x00000000bffe0000-
0x00000000bfffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x00000000feffc000-
0x00000000feffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x00000000fffc0000-
0x00000000ffffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x0000000100000000-

0x000000013fffffff]
usable

call chain:
======================

...
=> free_init_pages(what="initrd" or "unused kernel", begin=ffff9b26b....000, end=ffff9b26c....000); begin and end addresses
are random. The begin/end value above is just for reference.

Quoted 11 lines

=> set_memory_rw()
=> change_page_attr_set()
=> change_page_attr_set_clr()
=> __change_page_attr_set_clr(); cpa->numpages is 512 on my board if
what=="unused kernel"
=> __change_page_attr()
=> try_preserve_large_page(); address=ffff9b26bfacf000, pfn=80000, level=3; and the check loop count is 262144, exit loop after 861 usecs
on my board

You are talking about the static_protections() check, right?

Quoted 4 lines

the actual problem
===================
sometimes, free_init_pages returns after hundreds of secounds. The major impact is kernel boot time.

That's the symptom you are observing. The problem is in the try_to_preserve_large_page() logic.

The address range from the example above is:

   0xffff9b26b0000000 - 0xffff9b26c0000000

i.e. 256 MB.

So the first call with address 0xffff9b26b0000000 will try to preserve the
1GB page, but it's obvious that if pgrot changes that this has to split the
1GB page.

The current code is stupid in that regard simply because it does the static_protection() check loop _before_ checking:

1) Whether the new and the old pgprot are the same

2) Whether the address range which needs to be changed is aligned
to and
covers the full large mapping

So it does the static_protections() loop before #1 and #2 and checks the
full 1GB page wise, which makes it loop 262144 times.

With your magic 'cache' logic this will still loop exactly 262144 times at
least on the first invocation because there is no valid information in that
'cache'. So I really have no idea how your patch makes any difference unless it is breaking stuff left and right in very subtle ways.

If there is a second invocation with the same pgprot on that very same
range, then I can see it somehow having that effect by chance, but not by
design.

But this is all voodoo programming and there is a very obvious and simple
solution for this:

The check for pgprot_val(new_prot) == pgprot_val(old_port) can
definitely
be done _before_ the check loop. The check loop is pointless in
that
case, really. If there is a mismatch anywhere then it existed
already and
instead of yelling loudly the checking would just discover it,
enforce
the split and that would in the worst case preserve the old wrong
mapping
for those pages.

What we want there is a debug mechanism which catches such cases,
but is
not effective on production kernels and runs once or twice during
boot.

The range check whether the address is aligned to the large page
and
covers the full large page (1G or 2M) is also obvious to do
_before_ that
check, because if the requested range does not fit and has a
different
pgprot_val() then it will decide to split after the check anyway.

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.

Untested patch just moving the quick checks to the obvious right place
below.

Thanks,

tglx

8<-----------------

arch/x86/mm/pageattr.c +0 -2

@@ -628,47 +628,61 @@ try_preserve_large_page(pte_t *kpte, uns
 	new_prot = static_protections(req_prot, address, pfn);
 
 	/*
-	 * We need to check the full range, whether
-	 * static_protection() requires a different pgprot for one

of

arch/x86/mm/pageattr.c +1 -1

-	 * the pages in the range we try to preserve:
+	 * If there are no changes, return. cpa->numpages has been

updated

arch/x86/mm/pageattr.c +3 -0

+	 * above.
+	 *
+	 * There is no need to do any of the checks below because

the

arch/x86/mm/pageattr.c +1 -0

+	 * existing pgprot of the large mapping has to be correct.

If it's

arch/x86/mm/pageattr.c +1 -0

+	 * not then this is a bug in some other code and needs to be

fixed

arch/x86/mm/pageattr.c +1 -0

+	 * there and not silently papered over by the

static_protections()

arch/x86/mm/pageattr.c +1 -3

+	 * magic and then 'fixed' up in the wrong way.
 	 */
-	addr = address & pmask;
-	pfn = old_pfn;
-	for (i = 0; i < (psize >> PAGE_SHIFT); i++, addr +=

PAGE_SIZE, pfn++) {

arch/x86/mm/pageattr.c +0 -1

-		pgprot_t chk_prot = static_protections(req_prot,

addr, pfn);

arch/x86/mm/pageattr.c +3 -4

-
-		if (pgprot_val(chk_prot) != pgprot_val(new_prot))
-			goto out_unlock;
+	if (pgprot_val(new_prot) == pgprot_val(old_prot)) {
+		do_split = 0;
+		goto out_unlock;
 	}
 
 	/*
-	 * If there are no changes, return. maxpages has been

updated

arch/x86/mm/pageattr.c +1 -1

-	 * above:
+	 * If the requested address range is not aligned to the

start of

arch/x86/mm/pageattr.c +1 -0

+	 * the large page or does not cover the full range, split it

up.

arch/x86/mm/pageattr.c +1 -0

+	 * No matter what the static_protections() check below does,

it

arch/x86/mm/pageattr.c +1 -0

+	 * would anyway result in a split after doing all the check

work

arch/x86/mm/pageattr.c +5 -9

+	 * for nothing.
 	 */
-	if (pgprot_val(new_prot) == pgprot_val(old_prot)) {
-		do_split = 0;
+	addr = address & pmask;
+	numpages = psize >> PAGE_SHIFT;
+	if (address != addr || cpa->numpages != numpages)
 		goto out_unlock;
-	}
 
 	/*
-	 * We need to change the attributes. Check, whether we can
-	 * change the large page in one go. We request a split, when
-	 * the address is not aligned and the number of pages is
-	 * smaller than the number of pages in the large page. Note
-	 * that we limited the number of possible pages already to
-	 * the number of pages in the large page.
+	 * Check the full range, whether static_protection()

requires a

arch/x86/mm/pageattr.c +1 -0

+	 * different pgprot for one of the pages in the existing

large

arch/x86/mm/pageattr.c +1 -0

+	 * 2) Add a check for catching a case where the existing

mapping

arch/x86/mm/pageattr.c +2 -0

+	 *    is wrong already.
+	 * 3) Instead of looping over the whole thing, find the

overlapping

arch/x86/mm/pageattr.c +1 -0

+	 *    ranges and check them explicitely instead of looping

over a

arch/x86/mm/pageattr.c +1 -0

+	 *    full 1G mapping in 4K steps (256k iterations) for

figuring

arch/x86/mm/pageattr.c +1 -1

+	 *    that out.
 	 */
-	if (address == (address & pmask) && cpa->numpages == (psize

Quoted 1 line

Quoted 1 line

PAGE_SHIFT)) {

arch/x86/mm/pageattr.c +3 -8

-		/*
-		 * The address is aligned and the number of pages
-		 * covers the full page.
-		 */
-		new_pte = pfn_pte(old_pfn, new_prot);
-		__set_pmd_pte(kpte, address, new_pte);
-		cpa->flags |= CPA_FLUSHTLB;
-		do_split = 0;
+	pfn = old_pfn;
+	for (i = 0; i < numpages; i++, addr += PAGE_SIZE, pfn++) {
+		pgprot_t chk_prot = static_protections(req_prot,

addr, pfn);

arch/x86/mm/pageattr.c +9 -0

+
+		if (pgprot_val(chk_prot) != pgprot_val(new_prot))
+			goto out_unlock;
 	}
 
+	/* All checks passed. Just change the large mapping entry */
+	new_pte = pfn_pte(old_pfn, new_prot);
+	__set_pmd_pte(kpte, address, new_pte);
+	cpa->flags |= CPA_FLUSHTLB;
+	do_split = 0;
+
 out_unlock:
 	spin_unlock(&pgd_lock);

arch/x86/mm/pageattr.c try_preserve_large_page(pte_t *kpte, uns · line 664

Quoted 3 lines

arch/x86/mm/pageattr.c +3 -0

+	 * FIXME:
+	 * 1) Make this yell loudly if something tries to set a full
+	 *    large page to something which is not allowed.

I am trying to work out a patch based on your sample code and comments.
I do not understand here why it needs to yell loudly if set a full large page to something which is not allowed. It can split the large page to 4K-pages finally. And static_protection() will also be called for 4K-page change. Why not just add warning if 4K-page change is not allowed?