[PATCH] i386: fix vmalloc_sync_all() for Xen

[PATCH] i386: fix vmalloc_sync_all() for Xen

[Jan Beulich]

Since the fourth PDPT entry cannot be shared under Xen,
vmalloc_sync_all() must iterate over pmd-s rather than pgd-s here.
Luckily, the code isn't used for native PAE (SHARED_KERNEL_PMD is 1)
and the change is benign to non-PAE.

Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/mm/fault.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

--- linux-2.6.26-rc6/arch/x86/mm/fault.c	2008-06-18 09:56:16.000000000 +0200
+++ 2.6.26-rc6-i386-xen-vmalloc_sync_all/arch/x86/mm/fault.c	2008-06-06 08:51:52.000000000 +0200
@@ -921,32 +921,43 @@ void vmalloc_sync_all(void)
 	 * start are only improving performance (without affecting correctness
 	 * if undone).
 	 */
-	static DECLARE_BITMAP(insync, PTRS_PER_PGD);
+#define sync_index(a) ((a) >> PMD_SHIFT)
+	static DECLARE_BITMAP(insync, PTRS_PER_PGD*PTRS_PER_PMD);
 	static unsigned long start = TASK_SIZE;
 	unsigned long address;
 
 	if (SHARED_KERNEL_PMD)
 		return;
 
-	BUILD_BUG_ON(TASK_SIZE & ~PGDIR_MASK);
-	for (address = start; address >= TASK_SIZE; address += PGDIR_SIZE) {
-		if (!test_bit(pgd_index(address), insync)) {
+	BUILD_BUG_ON(TASK_SIZE & ~PMD_MASK);
+	for (address = start; address >= TASK_SIZE; address += PMD_SIZE) {
+		if (!test_bit(sync_index(address), insync)) {
 			unsigned long flags;
 			struct page *page;
 
 			spin_lock_irqsave(&pgd_lock, flags);
+			if (unlikely(list_empty(&pgd_list))) {
+				spin_unlock_irqrestore(&pgd_lock, flags);
+				return;
+			}
 			list_for_each_entry(page, &pgd_list, lru) {
 				if (!vmalloc_sync_one(page_address(page),
-						      address))
+						      address)) {
+					BUG_ON(list_first_entry(&pgd_list,
+								struct page,
+								lru) != page);
+					page = NULL;
 					break;
+				}
 			}
 			spin_unlock_irqrestore(&pgd_lock, flags);
-			if (!page)
-				set_bit(pgd_index(address), insync);
+			if (page)
+				set_bit(sync_index(address), insync);
 		}
-		if (address == start && test_bit(pgd_index(address), insync))
-			start = address + PGDIR_SIZE;
+		if (address == start && test_bit(sync_index(address), insync))
+			start = address + PMD_SIZE;
 	}
+#undef sync_index
 #else /* CONFIG_X86_64 */
 	/*
 	 * Note that races in the updates of insync and start aren't

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> Since the fourth PDPT entry cannot be shared under Xen,
> vmalloc_sync_all() must iterate over pmd-s rather than pgd-s here.
> Luckily, the code isn't used for native PAE (SHARED_KERNEL_PMD is 1)
> and the change is benign to non-PAE.
>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
> ---
>  arch/x86/mm/fault.c |   29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> --- linux-2.6.26-rc6/arch/x86/mm/fault.c	2008-06-18 09:56:16.000000000 +0200
> +++ 2.6.26-rc6-i386-xen-vmalloc_sync_all/arch/x86/mm/fault.c	2008-06-06 08:51:52.000000000 +0200
> @@ -921,32 +921,43 @@ void vmalloc_sync_all(void)
>  	 * start are only improving performance (without affecting correctness
>  	 * if undone).
>  	 */
> -	static DECLARE_BITMAP(insync, PTRS_PER_PGD);
> +#define sync_index(a) ((a) >> PMD_SHIFT)
> +	static DECLARE_BITMAP(insync, PTRS_PER_PGD*PTRS_PER_PMD);
>   

Given that the usermode PGDs will never need syncing, I think it would 
be better to use KERNEL_PGD_PTRS, and define

#define sync_index(a) (((a) >> PMD_SHIFT) - KERNEL_PGD_BOUNDARY)

for a massive 192 byte saving in bss.

>  	static unsigned long start = TASK_SIZE;
>  	unsigned long address;
>  
>  	if (SHARED_KERNEL_PMD)
>  		return;
>  
> -	BUILD_BUG_ON(TASK_SIZE & ~PGDIR_MASK);
> -	for (address = start; address >= TASK_SIZE; address += PGDIR_SIZE) {
> -		if (!test_bit(pgd_index(address), insync)) {
> +	BUILD_BUG_ON(TASK_SIZE & ~PMD_MASK);
> +	for (address = start; address >= TASK_SIZE; address += PMD_SIZE) {
>   

Would it be better - especially for the Xen case - to only iterate from 
TASK_SIZE to FIXADDR_TOP rather than wrapping around?  What will 
vmalloc_sync_one do on Xen mappings?

> +		if (!test_bit(sync_index(address), insync)) {
>   
It's probably worth reversing this test and removing a layer of indentation.
>  			unsigned long flags;
>  			struct page *page;
>  
>  			spin_lock_irqsave(&pgd_lock, flags);
> +			if (unlikely(list_empty(&pgd_list))) {
> +				spin_unlock_irqrestore(&pgd_lock, flags);
> +				return;
> +			}
>   

This seems a bit warty.  If the list is empty, then won't the 
list_for_each_entry() just fall through?  Presumably this only applies 
to boot, since pgd_list won't be empty on a running system with usermode 
processes.  Is there a correctness issue here, or is it just a 
micro-optimisation?

>  			list_for_each_entry(page, &pgd_list, lru) {
>  				if (!vmalloc_sync_one(page_address(page),
> -						      address))
> +						      address)) {
> +					BUG_ON(list_first_entry(&pgd_list,
> +								struct page,
> +								lru) != page);
>   

What condition is this testing for?

> +					page = NULL;
>  					break;
> +				}
>  			}
>  			spin_unlock_irqrestore(&pgd_lock, flags);
> -			if (!page)
> -				set_bit(pgd_index(address), insync);
> +			if (page)
> +				set_bit(sync_index(address), insync);
>  		}
> -		if (address == start && test_bit(pgd_index(address), insync))
> -			start = address + PGDIR_SIZE;
> +		if (address == start && test_bit(sync_index(address), insync))
> +			start = address + PMD_SIZE;
>  	}
> +#undef sync_index
>  #else /* CONFIG_X86_64 */
>  	/*
>  	 * Note that races in the updates of insync and start aren't
>   

Any chance of unifying this with the very similar-looking loop below it?

(I have to admit I don't understand why 64-bit needs to worry about 
syncing stuff.  Doesn't it have enough pgds to go around?  Is it because 
it wants to put modules within the same 2G chunk as the kernel?)

    J

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jan Beulich]

>Given that the usermode PGDs will never need syncing, I think it would 
>be better to use KERNEL_PGD_PTRS, and define
>
>#define sync_index(a) (((a) >> PMD_SHIFT) - KERNEL_PGD_BOUNDARY)
>
>for a massive 192 byte saving in bss.

I was considering that, too, but didn't do so for simplicity's sake. If I'll
have to re-spin the patch, I may as well do it.

>> +	for (address = start; address >= TASK_SIZE; address += PMD_SIZE) {
>>   
>
>Would it be better - especially for the Xen case - to only iterate from 
>TASK_SIZE to FIXADDR_TOP rather than wrapping around?  What will 
>vmalloc_sync_one do on Xen mappings?

Could be done, but since there will never be any out-of-sync Xen entries,
it doesn't hurt doing the full pass. I agree it would possibly be more
correct,though.

>> +		if (!test_bit(sync_index(address), insync)) {
>>   
>It's probably worth reversing this test and removing a layer of indentation.

How? There's a second if() following this one, so we can't just 'continue;'
here.

>>  			spin_lock_irqsave(&pgd_lock, flags);
>> +			if (unlikely(list_empty(&pgd_list))) {
>> +				spin_unlock_irqrestore(&pgd_lock, flags);
>> +				return;
>> +			}
>>   
>
>This seems a bit warty.  If the list is empty, then won't the 
>list_for_each_entry() just fall through?  Presumably this only applies 
>to boot, since pgd_list won't be empty on a running system with usermode 
>processes.  Is there a correctness issue here, or is it just a 
>micro-optimisation?

No, it isn't. Note the setting to NULL of page, which after the loop gets
tested for. list_for_each_entry() would never yield a NULL page, even
if the list is empty. And

>>  			list_for_each_entry(page, &pgd_list, lru) {
>>  				if (!vmalloc_sync_one(page_address(page),
>> -						      address))
>> +						      address)) {
>> +					BUG_ON(list_first_entry(&pgd_list,
>> +								struct page,
>> +								lru) != page);
>>   
>
>What condition is this testing for?

This is a replacement of the BUG_ON() that an earlier patch from you
removed: Failure of vmalloc_sync_one() must happen on the first
entry or never, and this is what is being checked for here.

>>  #else /* CONFIG_X86_64 */
>>  	/*
>>  	 * Note that races in the updates of insync and start aren't
>>   
>
>Any chance of unifying this with the very similar-looking loop below it?

Not at the same time I would say. As a subsequent thing, it might be
possible, though there are differences that make it desirable to keep
it distinct in my opinion.

>(I have to admit I don't understand why 64-bit needs to worry about 
>syncing stuff.  Doesn't it have enough pgds to go around?  Is it because 
>it wants to put modules within the same 2G chunk as the kernel?)

No, just like on 32-bit it's because modules loaded may access
vmalloc()-ed memory from notifiers that are called in contexts (NMI)
where taking even simple (propagation) page faults cannot be
tolerated (since the final IRET would result in finishing the NMI
handling from a CPU (or hypervisor) perspective.

Jan

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> >Given that the usermode PGDs will never need syncing, I think it would 
> >be better to use KERNEL_PGD_PTRS, and define
> >
> >#define sync_index(a) (((a) >> PMD_SHIFT) - KERNEL_PGD_BOUNDARY)
> >
> >for a massive 192 byte saving in bss.
> 
> I was considering that, too, but didn't do so for simplicity's sake. 
> If I'll have to re-spin the patch, I may as well do it.

you could do it as a second add-on patch as well - or as a respun patch. 
Jeremy, is it now Acked-by you?

	Ingo

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
>> Given that the usermode PGDs will never need syncing, I think it would 
>> be better to use KERNEL_PGD_PTRS, and define
>>
>> #define sync_index(a) (((a) >> PMD_SHIFT) - KERNEL_PGD_BOUNDARY)
>>
>> for a massive 192 byte saving in bss.
>>     
>
> I was considering that, too, but didn't do so for simplicity's sake. If I'll
> have to re-spin the patch, I may as well do it.
>
>   
>>> +	for (address = start; address >= TASK_SIZE; address += PMD_SIZE) {
>>>   
>>>       
>> Would it be better - especially for the Xen case - to only iterate from 
>> TASK_SIZE to FIXADDR_TOP rather than wrapping around?  What will 
>> vmalloc_sync_one do on Xen mappings?
>>     
>
> Could be done, but since there will never be any out-of-sync Xen entries,
> it doesn't hurt doing the full pass. I agree it would possibly be more
> correct,though.
>
>   
>>> +		if (!test_bit(sync_index(address), insync)) {
>>>   
>>>       
>> It's probably worth reversing this test and removing a layer of indentation.
>>     
>
> How? There's a second if() following this one, so we can't just 'continue;'
> here.
>   

That second if() block seems completely redundant:

		if (address == start && test_bit(pgd_index(address), insync))
			start = address + PGDIR_SIZE;

All it does it update "start", but start isn't used anywhere else in the 
loop.

>>>  			spin_lock_irqsave(&pgd_lock, flags);
>>> +			if (unlikely(list_empty(&pgd_list))) {
>>> +				spin_unlock_irqrestore(&pgd_lock, flags);
>>> +				return;
>>> +			}
>>>   
>>>       
>> This seems a bit warty.  If the list is empty, then won't the 
>> list_for_each_entry() just fall through?  Presumably this only applies 
>> to boot, since pgd_list won't be empty on a running system with usermode 
>> processes.  Is there a correctness issue here, or is it just a 
>> micro-optimisation?
>>     
>
> No, it isn't. Note the setting to NULL of page, which after the loop gets
> tested for. list_for_each_entry() would never yield a NULL page, even
> if the list is empty.

Does that matter?  If pgd_list is empty, then it's in sync by 
definition.  Why does it need special-casing?


>  And
>
>   
>>>  			list_for_each_entry(page, &pgd_list, lru) {
>>>  				if (!vmalloc_sync_one(page_address(page),
>>> -						      address))
>>> +						      address)) {
>>> +					BUG_ON(list_first_entry(&pgd_list,
>>> +								struct page,
>>> +								lru) != page);
>>>   
>>>       
>> What condition is this testing for?
>>     
>
> This is a replacement of the BUG_ON() that an earlier patch from you
> removed: Failure of vmalloc_sync_one() must happen on the first
> entry or never, and this is what is being checked for here.
>   

Could you add a comment?

> No, just like on 32-bit it's because modules loaded may access
> vmalloc()-ed memory from notifiers that are called in contexts (NMI)
> where taking even simple (propagation) page faults cannot be
> tolerated (since the final IRET would result in finishing the NMI
> handling from a CPU (or hypervisor) perspective.
>   

Well, 32-bit PAE avoids any syncing by having all pagetables share the 
same pmd containing the vmalloc mappings (ignoring the complications Xen 
adds here).  Couldn't 64-bit do the same thing at the pud level 
(preallocate as many puds needed to fill out the vmalloc area size).

Uh, I guess that's not practical with 64TB of vmalloc address space 
reserved...

    J

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jeremy Fitzhardinge]

Ingo Molnar wrote:
> you could do it as a second add-on patch as well - or as a respun patch. 
> Jeremy, is it now Acked-by you?

It seems sound but I think it needs another spin.

BTW, have you picked up the patches I sent out a couple of days ago?

    J

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jan Beulich]

>>>> +		if (!test_bit(sync_index(address), insync)) {
>>>>   
>>>>       
>>> It's probably worth reversing this test and removing a layer of indentation.
>>>     
>>
>> How? There's a second if() following this one, so we can't just 'continue;'
>> here.
>>   
>
>That second if() block seems completely redundant:
>
>		if (address == start && test_bit(pgd_index(address), insync))
>			start = address + PGDIR_SIZE;
>
>All it does it update "start", but start isn't used anywhere else in the 
>loop.

Since start is a static variable, it must be updated this way. The intention
here is to shorten the loop in later runs - since kernel page table entries
never go away, this is possible. Possibly just using the insync array would
be sufficient, but when I first coded this I wanted to avoid as much
overhead as was possible.

>>>>  			spin_lock_irqsave(&pgd_lock, flags);
>>>> +			if (unlikely(list_empty(&pgd_list))) {
>>>> +				spin_unlock_irqrestore(&pgd_lock, flags);
>>>> +				return;
>>>> +			}
>>>>   
>>>>       
>>> This seems a bit warty.  If the list is empty, then won't the 
>>> list_for_each_entry() just fall through?  Presumably this only applies 
>>> to boot, since pgd_list won't be empty on a running system with usermode 
>>> processes.  Is there a correctness issue here, or is it just a 
>>> micro-optimisation?
>>>     
>>
>> No, it isn't. Note the setting to NULL of page, which after the loop gets
>> tested for. list_for_each_entry() would never yield a NULL page, even
>> if the list is empty.
>
>Does that matter?  If pgd_list is empty, then it's in sync by 
>definition.  Why does it need special-casing?

Yes, certainly. But it would result in all insync bits set, which would be
wrong - only non-empty page directory entries can be in sync.

>>>>  			list_for_each_entry(page, &pgd_list, lru) {
>>>>  				if (!vmalloc_sync_one(page_address(page),
>>>> -						      address))
>>>> +						      address)) {
>>>> +					BUG_ON(list_first_entry(&pgd_list,
>>>> +								struct page,
>>>> +								lru) != page);
>>>>   
>>>>       
>>> What condition is this testing for?
>>>     
>>
>> This is a replacement of the BUG_ON() that an earlier patch from you
>> removed: Failure of vmalloc_sync_one() must happen on the first
>> entry or never, and this is what is being checked for here.
>>   
>
>Could you add a comment?

Sure, though there was none originally, and the intention seemed
quite clear to me.

Jan

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> Since start is a static variable, it must be updated this way. The intention
> here is to shorten the loop in later runs - since kernel page table entries
> never go away, this is possible. Possibly just using the insync array would
> be sufficient, but when I first coded this I wanted to avoid as much
> overhead as was possible.
>   

Yes, I see.  How often does this get called?  alloc_vm_area() and 
register_notify_die().  alloc_vm_area is only called by the grant-table 
code, and register_notify_die() is boot-time init.  Is this worth 
optimising at all?


>>>>>  			spin_lock_irqsave(&pgd_lock, flags);
>>>>> +			if (unlikely(list_empty(&pgd_list))) {
>>>>> +				spin_unlock_irqrestore(&pgd_lock, flags);
>>>>> +				return;
>>>>> +			}
>>>>>   
>>>>>       
>>>>>           
>>>> This seems a bit warty.  If the list is empty, then won't the 
>>>> list_for_each_entry() just fall through?  Presumably this only applies 
>>>> to boot, since pgd_list won't be empty on a running system with usermode 
>>>> processes.  Is there a correctness issue here, or is it just a 
>>>> micro-optimisation?
>>>>     
>>>>         
>>> No, it isn't. Note the setting to NULL of page, which after the loop gets
>>> tested for. list_for_each_entry() would never yield a NULL page, even
>>> if the list is empty.
>>>       
>> Does that matter?  If pgd_list is empty, then it's in sync by 
>> definition.  Why does it need special-casing?
>>     
>
> Yes, certainly. But it would result in all insync bits set, which would be
> wrong - only non-empty page directory entries can be in sync.
>   

I think it would be better to separately test whether the vmalloc 
mapping is present in the init_mm and skip the syncing loop in that 
case, rather than this somewhat convoluted logic to overload the test in 
vmalloc_sync_one.

>>>>>  			list_for_each_entry(page, &pgd_list, lru) {
>>>>>  				if (!vmalloc_sync_one(page_address(page),
>>>>> -						      address))
>>>>> +						      address)) {
>>>>> +					BUG_ON(list_first_entry(&pgd_list,
>>>>> +								struct page,
>>>>> +								lru) != page);
>>>>>   
>>>>>       
>>>>>           
>>>> What condition is this testing for?
>>>>     
>>>>         
>>> This is a replacement of the BUG_ON() that an earlier patch from you
>>> removed: Failure of vmalloc_sync_one() must happen on the first
>>> entry or never, and this is what is being checked for here.
>>>   
>>>       
>> Could you add a comment?
>>     
>
> Sure, though there was none originally, and the intention seemed
> quite clear to me.

Well, looks to me like vmalloc_sync_one can only return NULL iff the 
vmalloc mapping is absent in init_mm, so that's going to be invariant 
with respect to any other pgd you pass in.  So I don't think the BUG_ON 
will ever fire, and it's unclear what actual logical property it's 
testing for.

I think all this can be cleaned up quite a bit, but this patch is an 
improvement over what's currently there.

Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

    J

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jan Beulich]

>I think it would be better to separately test whether the vmalloc 
>mapping is present in the init_mm and skip the syncing loop in that 
>case, rather than this somewhat convoluted logic to overload the test in 
>vmalloc_sync_one.

That's what the x86-64 code does. When I wrote this originally, I tried
to keep the pre-existing logic as much as possible, so I split out
vmalloc_sync_one() by mostly moving existing code. I certainly agree
that this has room for cleaning up (and then possibly including unification
with x86-64).

>>>> This is a replacement of the BUG_ON() that an earlier patch from you
>>>> removed: Failure of vmalloc_sync_one() must happen on the first
>>>> entry or never, and this is what is being checked for here.
>>>>   
>>>>       
>>> Could you add a comment?
>>>     
>>
>> Sure, though there was none originally, and the intention seemed
>> quite clear to me.
>
>Well, looks to me like vmalloc_sync_one can only return NULL iff the 
>vmalloc mapping is absent in init_mm, so that's going to be invariant 

Correct.

>with respect to any other pgd you pass in.  So I don't think the BUG_ON 
>will ever fire, and it's unclear what actual logical property it's 
>testing for.

My point of adding the BUG_ON() is that in vmalloc_sync_all() it is not
clear that vmalloc_sync_one() can fail only due to init_mm's page table
not being appropriately populated. So yes, this BUG_ON() is not
expected to ever fire - but isn't that a property of all BUG_ON()'s?

Jan

Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
>> I think it would be better to separately test whether the vmalloc 
>> mapping is present in the init_mm and skip the syncing loop in that 
>> case, rather than this somewhat convoluted logic to overload the test in 
>> vmalloc_sync_one.
>>     
>
> That's what the x86-64 code does. When I wrote this originally, I tried
> to keep the pre-existing logic as much as possible, so I split out
> vmalloc_sync_one() by mostly moving existing code. I certainly agree
> that this has room for cleaning up (and then possibly including unification
> with x86-64).
>   

Yep.  I think the first step should be to simplify the code to remove 
all the insync/start optimisations and just implement it in a very 
straightforward way.  I can't see any way this code could be even 
slightly performance critical.

>> with respect to any other pgd you pass in.  So I don't think the BUG_ON 
>> will ever fire, and it's unclear what actual logical property it's 
>> testing for.
>>     
>
> My point of adding the BUG_ON() is that in vmalloc_sync_all() it is not
> clear that vmalloc_sync_one() can fail only due to init_mm's page table
> not being appropriately populated. So yes, this BUG_ON() is not
> expected to ever fire - but isn't that a property of all BUG_ON()'s?

Uh, I guess that's one way of putting it.  The other is that it tests 
for an obscure condition that's indirectly related to something that 
might indicate a bug if the code were written differently.  A good 
BUG_ON/assert acts as documentation because it tells the reader about 
the expected logical state at that point, but it needs to be expressed 
in terms which are directly relevant to the algorithm in question.

    J