From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Peter Zijlstra <peterz@infradead.org>,
Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>, Will Deacon <will@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-mm@kvack.org
Subject: Re: [PATCH 3/3] powerpc/8xx: Provide ptep_get() with 16k pages
Date: Wed, 17 Jun 2020 16:45:19 +0200 [thread overview]
Message-ID: <0bb024ce-11aa-80dc-c7d8-d5acc5329f25@csgroup.eu> (raw)
In-Reply-To: <20200617143826.GJ2531@hirez.programming.kicks-ass.net>
Le 17/06/2020 à 16:38, Peter Zijlstra a écrit :
> On Thu, Jun 18, 2020 at 12:21:22AM +1000, Michael Ellerman wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>>> On Mon, Jun 15, 2020 at 12:57:59PM +0000, Christophe Leroy wrote:
>
>>>> +#if defined(CONFIG_PPC_8xx) && defined(CONFIG_PPC_16K_PAGES)
>>>> +#define __HAVE_ARCH_PTEP_GET
>>>> +static inline pte_t ptep_get(pte_t *ptep)
>>>> +{
>>>> + pte_t pte = {READ_ONCE(ptep->pte), 0, 0, 0};
>>>> +
>>>> + return pte;
>>>> +}
>>>> +#endif
>>>
>>> Would it make sense to have a comment with this magic? The casual reader
>>> might wonder WTH just happened when he stumbles on this :-)
>>
>> I tried writing a helpful comment but it's too late for my brain to form
>> sensible sentences.
>>
>> Christophe can you send a follow-up with a comment explaining it? In
>> particular the zero entries stand out, it's kind of subtle that those
>> entries are only populated with the right value when we write to the
>> page table.
>
> static inline pte_t ptep_get(pte_t *ptep)
> {
> unsigned long val = READ_ONCE(ptep->pte);
> /* 16K pages have 4 identical value 4K entries */
> pte_t pte = {val, val, val, val);
> return pte;
> }
>
> Maybe something like that?
>
This should work as well. Indeed nobody cares about what's in the other
three. They are only there to ensure that ptep++ increases the ptep
pointer by 16 bytes. Only the HW require 4 identical values, that's
taken care of in set_pte_at() and pte_update().
So we should use the most efficient. Thinking once more, maybe what you
propose is the most efficient as there is no need to load another
register with value 0 in order to write it in the stack.
Christophe
WARNING: multiple messages have this Message-ID (diff)
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Peter Zijlstra <peterz@infradead.org>,
Michael Ellerman <mpe@ellerman.id.au>
Cc: Will Deacon <will@kernel.org>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Paul Mackerras <paulus@samba.org>,
Andrew Morton <akpm@linux-foundation.org>,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 3/3] powerpc/8xx: Provide ptep_get() with 16k pages
Date: Wed, 17 Jun 2020 16:45:19 +0200 [thread overview]
Message-ID: <0bb024ce-11aa-80dc-c7d8-d5acc5329f25@csgroup.eu> (raw)
In-Reply-To: <20200617143826.GJ2531@hirez.programming.kicks-ass.net>
Le 17/06/2020 à 16:38, Peter Zijlstra a écrit :
> On Thu, Jun 18, 2020 at 12:21:22AM +1000, Michael Ellerman wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>>> On Mon, Jun 15, 2020 at 12:57:59PM +0000, Christophe Leroy wrote:
>
>>>> +#if defined(CONFIG_PPC_8xx) && defined(CONFIG_PPC_16K_PAGES)
>>>> +#define __HAVE_ARCH_PTEP_GET
>>>> +static inline pte_t ptep_get(pte_t *ptep)
>>>> +{
>>>> + pte_t pte = {READ_ONCE(ptep->pte), 0, 0, 0};
>>>> +
>>>> + return pte;
>>>> +}
>>>> +#endif
>>>
>>> Would it make sense to have a comment with this magic? The casual reader
>>> might wonder WTH just happened when he stumbles on this :-)
>>
>> I tried writing a helpful comment but it's too late for my brain to form
>> sensible sentences.
>>
>> Christophe can you send a follow-up with a comment explaining it? In
>> particular the zero entries stand out, it's kind of subtle that those
>> entries are only populated with the right value when we write to the
>> page table.
>
> static inline pte_t ptep_get(pte_t *ptep)
> {
> unsigned long val = READ_ONCE(ptep->pte);
> /* 16K pages have 4 identical value 4K entries */
> pte_t pte = {val, val, val, val);
> return pte;
> }
>
> Maybe something like that?
>
This should work as well. Indeed nobody cares about what's in the other
three. They are only there to ensure that ptep++ increases the ptep
pointer by 16 bytes. Only the HW require 4 identical values, that's
taken care of in set_pte_at() and pte_update().
So we should use the most efficient. Thinking once more, maybe what you
propose is the most efficient as there is no need to load another
register with value 0 in order to write it in the stack.
Christophe
next prev parent reply other threads:[~2020-06-17 14:45 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-15 12:57 [PATCH 0/3] Fix build failure with v5.8-rc1 Christophe Leroy
2020-06-15 12:57 ` Christophe Leroy
2020-06-15 12:57 ` [PATCH 1/3] mm/gup: Use huge_ptep_get() in gup_hugepte() Christophe Leroy
2020-06-15 12:57 ` Christophe Leroy
2020-06-17 14:14 ` Michael Ellerman
2020-06-17 14:14 ` Michael Ellerman
2020-06-15 12:57 ` [PATCH 2/3] mm: Allow arches to provide ptep_get() Christophe Leroy
2020-06-15 12:57 ` Christophe Leroy
2020-06-15 12:57 ` [PATCH 3/3] powerpc/8xx: Provide ptep_get() with 16k pages Christophe Leroy
2020-06-15 12:57 ` Christophe Leroy
2020-06-15 13:22 ` Peter Zijlstra
2020-06-15 13:22 ` Peter Zijlstra
2020-06-17 14:21 ` Michael Ellerman
2020-06-17 14:21 ` Michael Ellerman
2020-06-17 14:38 ` Peter Zijlstra
2020-06-17 14:38 ` Peter Zijlstra
2020-06-17 14:45 ` Christophe Leroy [this message]
2020-06-17 14:45 ` Christophe Leroy
2020-06-18 1:00 ` Michael Ellerman
2020-06-18 1:00 ` Michael Ellerman
2020-06-18 14:19 ` Christophe Leroy
2020-06-18 14:19 ` Christophe Leroy
2020-06-18 0:58 ` Michael Ellerman
2020-06-18 0:58 ` Michael Ellerman
2020-06-18 14:21 ` Christophe Leroy
2020-06-18 14:21 ` Christophe Leroy
2020-06-17 10:57 ` [PATCH 0/3] Fix build failure with v5.8-rc1 Will Deacon
2020-06-17 10:57 ` Will Deacon
2020-06-17 14:13 ` Michael Ellerman
2020-06-17 14:13 ` Michael Ellerman
2020-06-18 12:37 ` Michael Ellerman
2020-06-18 12:37 ` Michael Ellerman
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=0bb024ce-11aa-80dc-c7d8-d5acc5329f25@csgroup.eu \
--to=christophe.leroy@csgroup.eu \
--cc=akpm@linux-foundation.org \
--cc=benh@kernel.crashing.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=will@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.