Re: [PATCH] mm/page_alloc: remove prefetchw() on freeing page to buddy system

[David Hildenbrand <david@redhat.com>]

On 03.07.24 02:12, Wei Yang wrote:
> On Tue, Jul 02, 2024 at 08:57:57AM +0200, David Hildenbrand wrote:
>> On 02.07.24 04:09, Wei Yang wrote:
>>> The prefetchw() is introduced from an ancient patch[1].
>>>
>>> The change log says:
>>>
>>>       The basic idea is to free higher order pages instead of going
>>>       through every single one.  Also, some unnecessary atomic operations
>>>       are done away with and replaced with non-atomic equivalents, and
>>>       prefetching is done where it helps the most.  For a more in-depth
>>>       discusion of this patch, please see the linux-ia64 archives (topic
>>>       is "free bootmem feedback patch").
>>>
>>> So there are several changes improve the bootmem freeing, in which the
>>> most basic idea is freeing higher order pages. And as Matthew says,
>>> "Itanium CPUs of this era had no prefetchers."
>>>
>>> I did 10 round bootup tests before and after this change, the data
>>> doesn't prove prefetchw() help speeding up bootmem freeing. The sum of
>>> the 10 round bootmem freeing time after prefetchw() removal even 5.2%
>>> faster than before.
>>
>> I suspect this is noise, though.
>>
>>>
>>> [1]: https://lore.kernel.org/linux-ia64/40F46962.4090604@sgi.com/
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>>> CC: David Hildenbrand <david@redhat.com>
>>>
>>> ---
>>> The patch is based on mm-stable with David's change.
>>> ---
>>>    mm/page_alloc.c | 13 ++-----------
>>>    1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 116ee33fd1ce..c46aedfc9a12 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -1236,16 +1236,11 @@ void __meminit __free_pages_core(struct page *page, unsigned int order,
>>>    	 */
>>>    	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG) &&
>>>    	    unlikely(context == MEMINIT_HOTPLUG)) {
>>> -		prefetchw(p);
>>> -		for (loop = 0; loop < (nr_pages - 1); loop++, p++) {
>>> -			prefetchw(p + 1);
>>> +		for (loop = 0; loop < nr_pages; loop++, p++) {
>>>    			VM_WARN_ON_ONCE(PageReserved(p));
>>>    			__ClearPageOffline(p);
>>>    			set_page_count(p, 0);
>>>    		}
>>
>> Something like:
>>
>> for (;;) {
>> 	...
>> 	if (++loop >= nr_pages)
>> 		break;
>> 	p++;
>> }
>>
> 
> So you prefer to have another version with this format? Sth like this?

Whatever you prefer, just pointing it out that we can do slightly better :)

-- 
Cheers,

David / dhildenb