linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
To: David Laight <David.Laight@ACULAB.COM>,
	'Pavel Tatashin' <pasha.tatashin@gmail.com>,
	Michal Hocko <mhocko@kernel.org>
Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"pavel.tatashin@microsoft.com" <pavel.tatashin@microsoft.com>,
	"dave.jiang@intel.com" <dave.jiang@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"willy@infradead.org" <willy@infradead.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"yi.z.zhang@linux.intel.com" <yi.z.zhang@linux.intel.com>,
	"khalid.aziz@oracle.com" <khalid.aziz@oracle.com>,
	"rppt@linux.vnet.ibm.com" <rppt@linux.vnet.ibm.com>,
	"vbabka@suse.cz" <vbabka@suse.cz>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>,
	"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	"ldufour@linux.vnet.ibm.com" <ldufour@linux.vnet.ibm.com>,
	"mgorman@techsingularity.net" <mgorman@techsingularity.net>,
	"mingo@kernel.org" <mingo@kernel.org>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>
Subject: Re: [mm PATCH v3 1/6] mm: Use mm_zero_struct_page from SPARC on all 64b architectures
Date: Wed, 17 Oct 2018 09:31:12 -0700	[thread overview]
Message-ID: <e7cd1e6f-cc04-31ea-3322-5c8e25a6e58a@linux.intel.com> (raw)
In-Reply-To: <7d313318f1234a1eb45b608bd853c17c@AcuMS.aculab.com>

On 10/17/2018 8:40 AM, David Laight wrote:
> From: Pavel Tatashin
>> Sent: 17 October 2018 16:12
>> On 10/17/18 11:07 AM, Alexander Duyck wrote:
>>> On 10/17/2018 1:47 AM, Michal Hocko wrote:
>>>> On Mon 15-10-18 13:26:56, Alexander Duyck wrote:
>>>>> This change makes it so that we use the same approach that was
>>>>> already in
>>>>> use on Sparc on all the archtectures that support a 64b long.
>>>>>
>>>>> This is mostly motivated by the fact that 8 to 10 store/move
>>>>> instructions
>>>>> are likely always going to be faster than having to call into a function
>>>>> that is not specialized for handling page init.
>>>>>
>>>>> An added advantage to doing it this way is that the compiler can get
>>>>> away
>>>>> with combining writes in the __init_single_page call. As a result the
>>>>> memset call will be reduced to only about 4 write operations, or at
>>>>> least
>>>>> that is what I am seeing with GCC 6.2 as the flags, LRU poitners, and
>>>>> count/mapcount seem to be cancelling out at least 4 of the 8
>>>>> assignments on
>>>>> my system.
>>>>>
>>>>> One change I had to make to the function was to reduce the minimum page
>>>>> size to 56 to support some powerpc64 configurations.
>>>>
>>>> This really begs for numbers. I do not mind the change itself with some
>>>> minor comments below.
>>>>
>>>> [...]
>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>> index bb0de406f8e7..ec6e57a0c14e 100644
>>>>> --- a/include/linux/mm.h
>>>>> +++ b/include/linux/mm.h
>>>>> @@ -102,8 +102,42 @@ static inline void set_max_mapnr(unsigned long
>>>>> limit) { }
>>>>>     * zeroing by defining this macro in <asm/pgtable.h>.
>>>>>     */
>>>>>    #ifndef mm_zero_struct_page
>>>>
>>>> Do we still need this ifdef? I guess we can wait for an arch which
>>>> doesn't like this change and then add the override. I would rather go
>>>> simple if possible.
>>>
>>> We probably don't, but as soon as I remove it somebody will probably
>>> complain somewhere. I guess I could drop it for now and see if anybody
>>> screams. Adding it back should be pretty straight forward since it would
>>> only be 2 lines.
>>>
>>>>> +#if BITS_PER_LONG == 64
>>>>> +/* This function must be updated when the size of struct page grows
>>>>> above 80
>>>>> + * or reduces below 64. The idea that compiler optimizes out switch()
>>>>> + * statement, and only leaves move/store instructions
>>>>> + */
>>>>> +#define    mm_zero_struct_page(pp) __mm_zero_struct_page(pp)
>>>>> +static inline void __mm_zero_struct_page(struct page *page)
>>>>> +{
>>>>> +    unsigned long *_pp = (void *)page;
>>>>> +
>>>>> +     /* Check that struct page is either 56, 64, 72, or 80 bytes */
>>>>> +    BUILD_BUG_ON(sizeof(struct page) & 7);
>>>>> +    BUILD_BUG_ON(sizeof(struct page) < 56);
>>>>> +    BUILD_BUG_ON(sizeof(struct page) > 80);
>>>>> +
>>>>> +    switch (sizeof(struct page)) {
>>>>> +    case 80:
>>>>> +        _pp[9] = 0;    /* fallthrough */
>>>>> +    case 72:
>>>>> +        _pp[8] = 0;    /* fallthrough */
>>>>> +    default:
>>>>> +        _pp[7] = 0;    /* fallthrough */
>>>>> +    case 56:
>>>>> +        _pp[6] = 0;
>>>>> +        _pp[5] = 0;
>>>>> +        _pp[4] = 0;
>>>>> +        _pp[3] = 0;
>>>>> +        _pp[2] = 0;
>>>>> +        _pp[1] = 0;
>>>>> +        _pp[0] = 0;
>>>>> +    }
>>>>
>>>> This just hit my eyes. I have to confess I have never seen default: to
>>>> be not the last one in the switch. Can we have case 64 instead or does
>>>> gcc
>>>> complain? I would be surprised with the set of BUILD_BUG_ONs.
>>
>> It was me, C does not really care where default is placed, I was trying
>> to keep stores sequential for better cache locality, but "case 64"
>> should be OK, and even better for this purpose.
> 
> You'd need to put memory barriers between them to force sequential stores.
> I'm also surprised that gcc doesn't inline the memset().
> 
> 	David

We don't need them to be sequential. The general idea is we have have to 
fill a given amount of space with 0s. After that we have some calls that 
are initialing the memory that doesn't have to be zero. Ideally the 
compiler is smart enough to realize that since we don't have barriers 
and we are performing assignments after the assignment of zero it can 
just combine the two writes into one and drop the zero assignment.

- Alex

  reply	other threads:[~2018-10-17 16:31 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-15 20:26 [mm PATCH v3 0/6] Deferred page init improvements Alexander Duyck
2018-10-15 20:26 ` [mm PATCH v3 1/6] mm: Use mm_zero_struct_page from SPARC on all 64b architectures Alexander Duyck
2018-10-16 19:01   ` Pavel Tatashin
2018-10-17  7:30     ` Mike Rapoport
2018-10-17 14:52       ` Alexander Duyck
2018-10-17  8:47   ` Michal Hocko
2018-10-17 15:07     ` Alexander Duyck
2018-10-17 15:12       ` Pavel Tatashin
2018-10-17 15:40         ` David Laight
2018-10-17 16:31           ` Alexander Duyck [this message]
2018-10-17 17:08             ` Pavel Tatashin
2018-10-17 16:34       ` Michal Hocko
2018-10-15 20:27 ` [mm PATCH v3 2/6] mm: Drop meminit_pfn_in_nid as it is redundant Alexander Duyck
2018-10-16 20:33   ` Pavel Tatashin
2018-10-16 20:49     ` Alexander Duyck
2018-10-16 21:06       ` Pavel Tatashin
2018-10-17  9:04   ` Michal Hocko
2018-10-15 20:27 ` [mm PATCH v3 3/6] mm: Use memblock/zone specific iterator for handling deferred page init Alexander Duyck
2018-10-17  9:11   ` Michal Hocko
2018-10-17 15:17     ` Alexander Duyck
2018-10-17 16:42   ` Mike Rapoport
2018-10-15 20:27 ` [mm PATCH v3 4/6] mm: Move hot-plug specific memory init into separate functions and optimize Alexander Duyck
2018-10-17  9:18   ` Michal Hocko
2018-10-17 15:26     ` Alexander Duyck
2018-10-24 12:36       ` Michal Hocko
2018-10-24 15:08         ` Alexander Duyck
2018-10-24 15:27           ` Michal Hocko
2018-10-24 17:35             ` Alexander Duyck
2018-10-25 12:41               ` Michal Hocko
2018-10-15 20:27 ` [mm PATCH v3 5/6] mm: Use common iterator for deferred_init_pages and deferred_free_pages Alexander Duyck
2018-10-15 20:27 ` [mm PATCH v3 6/6] mm: Add reserved flag setting to set_page_links Alexander Duyck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e7cd1e6f-cc04-31ea-3322-5c8e25a6e58a@linux.intel.com \
    --to=alexander.h.duyck@linux.intel.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=akpm@linux-foundation.org \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=davem@davemloft.net \
    --cc=khalid.aziz@oracle.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=ldufour@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@kernel.org \
    --cc=mingo@kernel.org \
    --cc=pasha.tatashin@gmail.com \
    --cc=pavel.tatashin@microsoft.com \
    --cc=rppt@linux.vnet.ibm.com \
    --cc=sparclinux@vger.kernel.org \
    --cc=vbabka@suse.cz \
    --cc=willy@infradead.org \
    --cc=yi.z.zhang@linux.intel.com \
    --subject='Re: [mm PATCH v3 1/6] mm: Use mm_zero_struct_page from SPARC on all 64b architectures' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).