Re: [PATCH v3 1/4] x86/clear_page: extend clear_page*() for multi-page clearing

[Ankur Arora <ankur.a.arora@oracle.com>]

Ingo Molnar <mingo@kernel.org> writes:

> * Ankur Arora <ankur.a.arora@oracle.com> wrote:
>
>> clear_page*() variants now take a page-aligned length parameter and
>> clears the whole region.
>
> Please read your changelogs and fix typos. ;-)

Ack!

>> +void clear_pages_orig(void *page, unsigned int length);
>> +void clear_pages_rep(void *page, unsigned int length);
>> +void clear_pages_erms(void *page, unsigned int length);
>
> What unit is 'length' in? If it's bytes, why is this interface
> artificially limiting itself to ~4GB? On x86-64 there's very little (if

I was in two minds about the unit. Given that the largest page size is
1GB, decided to go with 32bit. But, as you say below, there's no reason
to limit the x86-64 interface for MM reasons.  Will fix.

> any) performance difference between a 32-bit and a 64-bit length
> iterations.
>
> Even if we end up only exposing a 32-bit length API to the generic MM
> layer, there's no reason to limit the x86-64 assembly code in such a
> fashion.
>
>>  static inline void clear_page(void *page)
>>  {
>> +	unsigned int length = PAGE_SIZE;
>>  	/*
>> -	 * Clean up KMSAN metadata for the page being cleared. The assembly call
>> +	 * Clean up KMSAN metadata for the pages being cleared. The assembly call
>>  	 * below clobbers @page, so we perform unpoisoning before it.
>
>>  	 */
>> -	kmsan_unpoison_memory(page, PAGE_SIZE);
>> -	alternative_call_2(clear_page_orig,
>> -			   clear_page_rep, X86_FEATURE_REP_GOOD,
>> -			   clear_page_erms, X86_FEATURE_ERMS,
>> +	kmsan_unpoison_memory(page, length);
>> +
>> +	alternative_call_2(clear_pages_orig,
>> +			   clear_pages_rep, X86_FEATURE_REP_GOOD,
>> +			   clear_pages_erms, X86_FEATURE_ERMS,
>>  			   "=D" (page),
>> -			   "D" (page),
>> +			   ASM_INPUT("D" (page), "S" (length)),
>>  			   "cc", "memory", "rax", "rcx");
>>  }
>>
>> diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
>> index a508e4a8c66a..bce516263b69 100644
>> --- a/arch/x86/lib/clear_page_64.S
>> +++ b/arch/x86/lib/clear_page_64.S
>> @@ -13,20 +13,35 @@
>>   */
>>
>>  /*
>> - * Zero a page.
>> - * %rdi	- page
>> + * Zero kernel page aligned region.
>> + *
>> + * Input:
>> + * %rdi	- destination
>> + * %esi	- length
>> + *
>> + * Clobbers: %rax, %rcx
>>   */
>> -SYM_TYPED_FUNC_START(clear_page_rep)
>> -	movl $4096/8,%ecx
>> +SYM_TYPED_FUNC_START(clear_pages_rep)
>> +	movl %esi, %ecx
>>  	xorl %eax,%eax
>> +	shrl $3,%ecx
>>  	rep stosq
>>  	RET
>> -SYM_FUNC_END(clear_page_rep)
>> -EXPORT_SYMBOL_GPL(clear_page_rep)
>> +SYM_FUNC_END(clear_pages_rep)
>> +EXPORT_SYMBOL_GPL(clear_pages_rep)
>>
>> -SYM_TYPED_FUNC_START(clear_page_orig)
>> +/*
>> + * Original page zeroing loop.
>> + * Input:
>> + * %rdi	- destination
>> + * %esi	- length
>> + *
>> + * Clobbers: %rax, %rcx, %rflags
>> + */
>> +SYM_TYPED_FUNC_START(clear_pages_orig)
>> +	movl   %esi, %ecx
>>  	xorl   %eax,%eax
>> -	movl   $4096/64,%ecx
>> +	shrl   $6,%ecx
>
> So if the natural input parameter is RCX, why is this function using
> RSI as the input 'length' parameter? Causes unnecessary register
> shuffling.

True. Will fix via ALTERNATIVE as Peter describes in the other mail.
That should get rid of some of the repetition in clear_pages_rep and
clear_pages_erms() as well.

Though I guess clear_pages_orig() will need to stick around just with
a different parameter passing convention.

>> +/*
>> + * Zero kernel page aligned region.
>> + *
>> + * Input:
>> + * %rdi	- destination
>> + * %esi	- length
>> + *
>> + * Clobbers: %rax, %rcx
>> + */
>> +SYM_TYPED_FUNC_START(clear_pages_erms)
>> +	movl %esi, %ecx
>>  	xorl %eax,%eax
>>  	rep stosb
>>  	RET
>
> Same observation: unnecessary register shuffling.
>
> Also, please rename this (now-) terribly named interface:
>
>> +void clear_pages_orig(void *page, unsigned int length);
>> +void clear_pages_rep(void *page, unsigned int length);
>> +void clear_pages_erms(void *page, unsigned int length);
>
> Because the 'pages' is now a bit misleading, and why is the starting
> address called a 'page'?

> So a more sensible namespace would be to follow memset nomenclature:
>
> 	void memzero_page_aligned_*(void *addr, unsigned long len);
>
> ... and note the intentional abbreviation to 'len'.

Ack.

> Also, since most of these changes are to x86 architecture code, this is
> a new interface only used by x86, and the MM glue is minimal, I'd like
> to merge this series via the x86 tree, if the glue gets acks from MM
> folks.

Works for me!


Thanks for the review.

--
ankur