* RE: RFC on writel and writel_relaxed
From: David Laight @ 2018-03-28 9:00 UTC (permalink / raw)
To: 'Will Deacon', Benjamin Herrenschmidt
Cc: Sinan Kaya, Arnd Bergmann, Jason Gunthorpe, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <20180328085338.GA28871@arm.com>
From: Will Deacon
> Sent: 28 March 2018 09:54
...
> > > I don't think so. My reading of memory-barriers.txt says that writeX =
might
> > > expand to outX, and outX is not ordered with respect to other types o=
f
> > > memory.
> >
> > Ugh ?
> >
> > My understanding of HW at least is the exact opposite. outX is *more*
> > ordered if anything, than any other accessors. IO space is completely
> > synchronous, non posted and ordered afaik.
>=20
> I'm just going by memory-barriers.txt:
>=20
>=20
> (*) inX(), outX():
>=20
> [...]
>=20
> They are guaranteed to be fully ordered with respect to each other.
>=20
> They are not guaranteed to be fully ordered with respect to other ty=
pes of
> memory and I/O operation.
A long time ago there was a document from Intel that said that inb/outb wer=
en't
necessarily synchronised wrt memory accesses.
(Might be P-pro era).
However no processors actually behaved that way and more recent docs
say that inb/outb are fully ordered.
David
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-28 8:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Sinan Kaya, Arnd Bergmann, Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney, Peter Zijlstra,
Ingo Molnar, Jonathan Corbet
In-Reply-To: <1522186185.7364.59.camel@kernel.crashing.org>
On Wed, Mar 28, 2018 at 08:29:45AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2018-03-27 at 15:36 +0100, Will Deacon wrote:
> > > Can we say the same thing for iowrite32() and iowrite32be(). I also see wmb()
> > > in front of these.
> >
> > I don't think so. My reading of memory-barriers.txt says that writeX might
> > expand to outX, and outX is not ordered with respect to other types of
> > memory.
>
> Ugh ?
>
> My understanding of HW at least is the exact opposite. outX is *more*
> ordered if anything, than any other accessors. IO space is completely
> synchronous, non posted and ordered afaik.
I'm just going by memory-barriers.txt:
(*) inX(), outX():
[...]
They are guaranteed to be fully ordered with respect to each other.
They are not guaranteed to be fully ordered with respect to other types of
memory and I/O operation.
For arm/arm64 these end up behaving exactly the same as readX/writeX, but
I'm nervous about changing the documentation without understanding why it's
like it is currently. Maybe another ia64 thing?.
Will
^ permalink raw reply
* Re: [PATCH v9 06/24] mm: make pte_unmap_same compatible with SPF
From: Laurent Dufour @ 2018-03-28 8:27 UTC (permalink / raw)
To: David Rientjes
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <alpine.DEB.2.20.1803271417510.31115@chino.kir.corp.google.com>
On 27/03/2018 23:18, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 2f3e98edc94a..b6432a261e63 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1199,6 +1199,7 @@ static inline void clear_page_pfmemalloc(struct page *page)
>> #define VM_FAULT_NEEDDSYNC 0x2000 /* ->fault did not modify page tables
>> * and needs fsync() to complete (for
>> * synchronous page faults in DAX) */
>> +#define VM_FAULT_PTNOTSAME 0x4000 /* Page table entries have changed */
>>
>> #define VM_FAULT_ERROR (VM_FAULT_OOM | VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | \
>> VM_FAULT_HWPOISON | VM_FAULT_HWPOISON_LARGE | \
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 21b1212a0892..4bc7b0bdcb40 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -2309,21 +2309,29 @@ static bool pte_map_lock(struct vm_fault *vmf)
>> * parts, do_swap_page must check under lock before unmapping the pte and
>> * proceeding (but do_wp_page is only called after already making such a check;
>> * and do_anonymous_page can safely check later on).
>> + *
>> + * pte_unmap_same() returns:
>> + * 0 if the PTE are the same
>> + * VM_FAULT_PTNOTSAME if the PTE are different
>> + * VM_FAULT_RETRY if the VMA has changed in our back during
>> + * a speculative page fault handling.
>> */
>> -static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd,
>> - pte_t *page_table, pte_t orig_pte)
>> +static inline int pte_unmap_same(struct vm_fault *vmf)
>> {
>> - int same = 1;
>> + int ret = 0;
>> +
>> #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
>> if (sizeof(pte_t) > sizeof(unsigned long)) {
>> - spinlock_t *ptl = pte_lockptr(mm, pmd);
>> - spin_lock(ptl);
>> - same = pte_same(*page_table, orig_pte);
>> - spin_unlock(ptl);
>> + if (pte_spinlock(vmf)) {
>> + if (!pte_same(*vmf->pte, vmf->orig_pte))
>> + ret = VM_FAULT_PTNOTSAME;
>> + spin_unlock(vmf->ptl);
>> + } else
>> + ret = VM_FAULT_RETRY;
>> }
>> #endif
>> - pte_unmap(page_table);
>> - return same;
>> + pte_unmap(vmf->pte);
>> + return ret;
>> }
>>
>> static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va, struct vm_area_struct *vma)
>> @@ -2913,7 +2921,8 @@ int do_swap_page(struct vm_fault *vmf)
>> int exclusive = 0;
>> int ret = 0;
>
> Initialization is now unneeded.
I'm sorry, what "initialization" are you talking about here ?
>
> Otherwise:
>
> Acked-by: David Rientjes <rientjes@google.com>
Thanks,
Laurent.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 7:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Torvalds, Alexander Duyck, Will Deacon, Sinan Kaya,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CAK8P3a1QTjCtJMAv1RSN5ReiOShZ_ygkCXY484OJUvn6r+1mbg@mail.gmail.com>
On Wed, 2018-03-28 at 09:11 +0200, Arnd Bergmann wrote:
> On Wed, Mar 28, 2018 at 8:56 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Wed, 2018-03-28 at 06:53 +0000, Linus Torvalds wrote:
> > > On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > > That's why in/out were *so* slow, and why nobody uses them any more
> > > (well, the address size limitations and the lack of any remapping of
> > > the address obviously also are a reason).
> >
> > All true indeed, though a lot of other archs never quite made them
> > fully synchronous, which was another can of worms ... oh well.
>
> Many architectures have no way of providing PCI compliant semantics
> for outb, as their instruction set and/or bus interconnect lacks a
> method of waiting for completion of an outb.
Yup, that includes powerpc. Note that since POWER8 we don't even
genetate IO space anymore :-)
> In practice, it doesn't seem to matter for any of the devices one would
> encounter these days: very few use I/O space, and those that do don't
> actually rely on the strict ordering. Some architectures (in particular
> s390, but I remember seeing the same thing elsewhere) explicitly
> disallow I/O space access on PCI because of this. On ARM, the typical
> PCI implementations have other problems that are worse than this
> one, so most drivers are fine with the almost-working semantics.
/me cries...
Ben.
^ permalink raw reply
* Re: [PATCH v9 05/24] mm: Introduce pte_spinlock for FAULT_FLAG_SPECULATIVE
From: Laurent Dufour @ 2018-03-28 8:15 UTC (permalink / raw)
To: David Rientjes
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <alpine.DEB.2.20.1803251446180.80485@chino.kir.corp.google.com>
On 25/03/2018 23:50, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
>
>> When handling page fault without holding the mmap_sem the fetch of the
>> pte lock pointer and the locking will have to be done while ensuring
>> that the VMA is not touched in our back.
>>
>> So move the fetch and locking operations in a dedicated function.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>> mm/memory.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 8ac241b9f370..21b1212a0892 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -2288,6 +2288,13 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>> }
>> EXPORT_SYMBOL_GPL(apply_to_page_range);
>>
>> +static bool pte_spinlock(struct vm_fault *vmf)
>
> inline?
You're right.
Indeed this was done in the patch 18 : "mm: Provide speculative fault
infrastructure", but this has to be done there too, I'll fix that.
>
>> +{
>> + vmf->ptl = pte_lockptr(vmf->vma->vm_mm, vmf->pmd);
>> + spin_lock(vmf->ptl);
>> + return true;
>> +}
>> +
>> static bool pte_map_lock(struct vm_fault *vmf)
>> {
>> vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd,
>
> Shouldn't pte_unmap_same() take struct vm_fault * and use the new
> pte_spinlock()?
done in the next patch, but you already acked it..
^ permalink raw reply
* Re: [PATCH v9 01/24] mm: Introduce CONFIG_SPECULATIVE_PAGE_FAULT
From: Laurent Dufour @ 2018-03-28 7:49 UTC (permalink / raw)
To: David Rientjes, Thomas Gleixner
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, linux-kernel,
linux-mm, haren, khandual, npiggin, bsingharora, Tim Chen,
linuxppc-dev, x86
In-Reply-To: <alpine.DEB.2.20.1803251442090.80485@chino.kir.corp.google.com>
Hi David,
Thanks a lot for your deep review on this series.
On 25/03/2018 23:50, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
>
>> This configuration variable will be used to build the code needed to
>> handle speculative page fault.
>>
>> By default it is turned off, and activated depending on architecture
>> support.
>>
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>> mm/Kconfig | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index abefa573bcd8..07c566c88faf 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>> @@ -759,3 +759,6 @@ config GUP_BENCHMARK
>> performance of get_user_pages_fast().
>>
>> See tools/testing/selftests/vm/gup_benchmark.c
>> +
>> +config SPECULATIVE_PAGE_FAULT
>> + bool
>
> Should this be configurable even if the arch supports it?
Actually, this is not configurable unless by manually editing the .config file.
I made it this way on the Thomas's request :
https://lkml.org/lkml/2018/1/15/969
That sounds to be the smarter way to achieve that, isn't it ?
Laurent.
^ permalink raw reply
* Re: [PATCH 14/19] powerpc/altivec: Add missing prototypes for altivec
From: Mathieu Malaterre @ 2018-03-28 7:26 UTC (permalink / raw)
To: LEROY Christophe; +Cc: kvm-ppc, LKML, Paul Mackerras, linuxppc-dev
In-Reply-To: <20180327193323.Horde.S-Hqc40p7JjEsZPPEa3YNg1@messagerie.si.c-s.fr>
On Tue, Mar 27, 2018 at 7:33 PM, LEROY Christophe
<christophe.leroy@c-s.fr> wrote:
> LEROY Christophe <christophe.leroy@c-s.fr> a =C3=A9crit :
>
>
>> Mathieu Malaterre <malat@debian.org> a =C3=A9crit :
>>
>>> Christophe,
>>>
>>> On Sat, Mar 24, 2018 at 9:10 PM, LEROY Christophe
>>> <christophe.leroy@c-s.fr> wrote:
>>>>
>>>> Mathieu Malaterre <malat@debian.org> a =C3=A9crit :
>>>>
>>>>
>>>>> On Fri, Mar 23, 2018 at 1:19 PM, christophe leroy
>>>>> <christophe.leroy@c-s.fr> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Le 22/03/2018 =C3=A0 21:20, Mathieu Malaterre a =C3=A9crit :
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Some functions prototypes were missing for the non-altivec code. Ad=
d
>>>>>>> the
>>>>>>> missing prototypes directly in xor_vmx, fix warnings treated as
>>>>>>> errors
>>>>>>> with
>>>>>>> W=3D1:
>>>>>>>
>>>>>>> arch/powerpc/lib/xor_vmx_glue.c:18:6: error: no previous prototyp=
e
>>>>>>> for
>>>>>>> =E2=80=98xor_altivec_2=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>>>> arch/powerpc/lib/xor_vmx_glue.c:29:6: error: no previous prototyp=
e
>>>>>>> for
>>>>>>> =E2=80=98xor_altivec_3=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>>>> arch/powerpc/lib/xor_vmx_glue.c:40:6: error: no previous prototyp=
e
>>>>>>> for
>>>>>>> =E2=80=98xor_altivec_4=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>>>> arch/powerpc/lib/xor_vmx_glue.c:52:6: error: no previous prototyp=
e
>>>>>>> for
>>>>>>> =E2=80=98xor_altivec_5=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>>>>
>>>>>>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
>>>>>>> ---
>>>>>>> arch/powerpc/lib/xor_vmx.h | 14 ++++++++++++++
>>>>>>> 1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/lib/xor_vmx.h b/arch/powerpc/lib/xor_vmx.=
h
>>>>>>> index 5c2b0839b179..2173e3c84151 100644
>>>>>>> --- a/arch/powerpc/lib/xor_vmx.h
>>>>>>> +++ b/arch/powerpc/lib/xor_vmx.h
>>>>>>> @@ -19,3 +19,17 @@ void __xor_altivec_4(unsigned long bytes, unsign=
ed
>>>>>>> long
>>>>>>> *v1_in,
>>>>>>> void __xor_altivec_5(unsigned long bytes, unsigned long *v1_in,
>>>>>>> unsigned long *v2_in, unsigned long
>>>>>>> *v3_in,
>>>>>>> unsigned long *v4_in, unsigned long
>>>>>>> *v5_in);
>>>>>>> +
>>>>>>> +void xor_altivec_2(unsigned long bytes, unsigned long *v1_in,
>>>>>>> + unsigned long *v2_in);
>>>>>>> +
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Only used in one place, should be static instead of adding it in a .=
h
>>>>>>
>>>>>> Same for the other ones.
>>>>>
>>>>>
>>>>>
>>>>> $ git grep xor_altivec_2
>>>>> [...]
>>>>> arch/powerpc/lib/xor_vmx_glue.c:EXPORT_SYMBOL(xor_altivec_2);
>>>>>
>>>>> Are you sure I can change this function to static ?
>>>>
>>>>
>>>>
>>>> Yes you are right. But in fact those fonctions are already defined in
>>>> asm/xor. h
>>>> So you just need to add the missing #include
>>>
>>>
>>> I originally tried it, but this leads to:
>>>
>>> CC arch/powerpc/lib/xor_vmx_glue.o
>>> In file included from arch/powerpc/lib/xor_vmx_glue.c:16:0:
>>> ./arch/powerpc/include/asm/xor.h:39:15: error: variable
>>> =E2=80=98xor_block_altivec=E2=80=99 has initializer but incomplete type
>>> static struct xor_block_template xor_block_altivec =3D {
>>> ^~~~~~~~~~~~~~~~~~
>>> ./arch/powerpc/include/asm/xor.h:40:2: error: unknown field =E2=80=98na=
me=E2=80=99
>>> specified in initializer
>>> .name =3D "altivec",
>>> ^
>>> [...]
>>>
>>> The file <asm/xor.h> (powerpc) is pretty much expected to be included
>>> after <include/linux/raid/xor.h>.
>>>
>>> I did not want to tweak <asm/xor.h> to test for #ifdef _XOR_H just befo=
re
>>>
>>> #ifdef _XOR_H
>>> static struct xor_block_template xor_block_altivec =3D {
>>> [...]
>>>
>>> since this seems like a hack to me.
>>>
>>> Is this ok to test for #ifdef _XOR_H in <arch/powerpc/include/asm/xor.h=
>
>>> ?
>>
>>
>> What about including linux/raid/xor.h in asm/xor.h ?
This leads to:
CALL ../arch/powerpc/kernel/systbl_chk.sh
In file included from ../arch/powerpc/include/asm/xor.h:57:0,
from ../arch/powerpc/lib/xor_vmx_glue.c:17:
../include/asm-generic/xor.h:688:34: error: =E2=80=98xor_block_32regs=E2=80=
=99 defined
but not used [-Werror=3Dunused-variable]
static struct xor_block_template xor_block_32regs =3D {
^~~~~~~~~~~~~~~~
../include/asm-generic/xor.h:680:34: error: =E2=80=98xor_block_8regs=E2=80=
=99 defined
but not used [-Werror=3Dunused-variable]
static struct xor_block_template xor_block_8regs =3D {
^~~~~~~~~~~~~~~
In file included from ../arch/powerpc/lib/xor_vmx_glue.c:17:0:
../arch/powerpc/include/asm/xor.h:39:34: error: =E2=80=98xor_block_altivec=
=E2=80=99
defined but not used [-Werror=3Dunused-variable]
static struct xor_block_template xor_block_altivec =3D {
^~~~~~~~~~~~~~~~~
CALL ../arch/powerpc/kernel/prom_init_check.sh
>
> Or better: including linux/raid/xor.h then asm/xor.h in xor_vmx_glue.c ?
>
> Christophe
>
>>
>> Christophe
>>>
>>>
>>>> Christophe
>>>>
>>>>
>>>>>
>>>>>> Christophe
>>>>>>
>>>>>>
>>>>>>> +void xor_altivec_3(unsigned long bytes, unsigned long *v1_in,
>>>>>>> + unsigned long *v2_in, unsigned long
>>>>>>> *v3_in);
>>>>>>> +
>>>>>>> +void xor_altivec_4(unsigned long bytes, unsigned long *v1_in,
>>>>>>> + unsigned long *v2_in, unsigned long
>>>>>>> *v3_in,
>>>>>>> + unsigned long *v4_in);
>>>>>>> +
>>>>>>> +void xor_altivec_5(unsigned long bytes, unsigned long *v1_in,
>>>>>>> + unsigned long *v2_in, unsigned long
>>>>>>> *v3_in,
>>>>>>> + unsigned long *v4_in, unsigned long
>>>>>>> *v5_in);
>>>>>>>
>>>>>>
>>>>>> ---
>>>>>> L'absence de virus dans ce courrier =C3=A9lectronique a =C3=A9t=C3=
=A9 v=C3=A9rifi=C3=A9e par le
>>>>>> logiciel antivirus Avast.
>>>>>> https://www.avast.com/antivirus
>>>>>>
>>>>
>>>>
>
>
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-28 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Linus Torvalds, Alexander Duyck, Will Deacon, Sinan Kaya,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522220165.7364.110.camel@kernel.crashing.org>
On Wed, Mar 28, 2018 at 8:56 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2018-03-28 at 06:53 +0000, Linus Torvalds wrote:
>> On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>> That's why in/out were *so* slow, and why nobody uses them any more
>> (well, the address size limitations and the lack of any remapping of
>> the address obviously also are a reason).
>
> All true indeed, though a lot of other archs never quite made them
> fully synchronous, which was another can of worms ... oh well.
Many architectures have no way of providing PCI compliant semantics
for outb, as their instruction set and/or bus interconnect lacks a
method of waiting for completion of an outb.
In practice, it doesn't seem to matter for any of the devices one would
encounter these days: very few use I/O space, and those that do don't
actually rely on the strict ordering. Some architectures (in particular
s390, but I remember seeing the same thing elsewhere) explicitly
disallow I/O space access on PCI because of this. On ARM, the typical
PCI implementations have other problems that are worse than this
one, so most drivers are fine with the almost-working semantics.
Arnd
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 6:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFyaZ=WEXFg4KZ0yCWaR=8Dm_FPK-xcA0EYi3fEzm+KZkA@mail.gmail.com>
On Wed, 2018-03-28 at 06:53 +0000, Linus Torvalds wrote:
>
>
> On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crash
> ing.org> wrote:
> > >
> > > Of course, you'd have to be pretty odd to want to start a DMA
> > with a
> > > read anyway - partly exactly because it's bad for performance
> > since
> > > reads will be synchronous and not buffered like a write).
> >
> > I have bad memories of old adaptec controllers ...
>
> *Old* adaptec controllers were likely to use the in/out instructions
> for status and command data.
>
> Those are actually even more ordered than UC reads and writes: the
> in/out instructions are not just fully ordered, but are fully
> *synchronous* on x86.
>
> So not just doing accesses in order, but actually waiting for
> everything to drain before they start executing, but they also wait
> for the operation itself to complete (ie "out" will not just queue
> the write, it will then wait for the queue to empty and the write
> data to hit the line).
>
> That's why in/out were *so* slow, and why nobody uses them any more
> (well, the address size limitations and the lack of any remapping of
> the address obviously also are a reason).
All true indeed, though a lot of other archs never quite made them
fully synchronous, which was another can of worms ... oh well.
As for Adaptec, you might be right, I do remember having cases of old
stuff triggering DMA on reads, it might have been "Mac" variants of
Adaptec using MMIO or something...
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 6:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522219376.7364.109.camel@kernel.crashing.org>
[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]
On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crashing.org>
wrote:
> >
> > Of course, you'd have to be pretty odd to want to start a DMA with a
> > read anyway - partly exactly because it's bad for performance since
> > reads will be synchronous and not buffered like a write).
>
> I have bad memories of old adaptec controllers ...
>
*Old* adaptec controllers were likely to use the in/out instructions for
status and command data.
Those are actually even more ordered than UC reads and writes: the in/out
instructions are not just fully ordered, but are fully *synchronous* on
x86.
So not just doing accesses in order, but actually waiting for everything to
drain before they start executing, but they also wait for the operation
itself to complete (ie "out" will not just queue the write, it will then
wait for the queue to empty and the write data to hit the line).
That's why in/out were *so* slow, and why nobody uses them any more (well,
the address size limitations and the lack of any remapping of the address
obviously also are a reason).
Linus
>
[-- Attachment #2: Type: text/html, Size: 1798 bytes --]
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 6:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFw24R3YAujir59GdG+sBBQOA9WyXySEN3AcEYK99ZHuwQ@mail.gmail.com>
On Tue, 2018-03-27 at 20:26 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 6:33 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > This is why, I want (with your agreement) to define clearly and once
> > and for all, that the Linux semantics of writel are that it is ordered
> > with previous writes to coherent memory (*)
>
> Honestly, I think those are the sane semantics. In fact, make it
> "ordered with previous writes" full stop, since it's not only ordered
> wrt previous writes to memory, but also previous writel's.
Of course. It was somewhat a given that it's ordered vs. any previous
MMIO actually, but it doesn't hurt to spell it out once more.
> > Also, can I assume the above ordering with writel() equally applies to
> > readl() or not ?
> >
> > IE:
> > dma_buf->foo = 1;
> > readl(STUPID_DEVICE_DMA_KICK_ON_READ);
>
> If that KICK_ON_READ is UC, then that's definitely the case. And
> honestly, status registers like that really should always be UC.
>
> But if somebody sets the area WC (which is crazy), then I think it
> might be at least debatable. x86 semantics does allow reads to be done
> before previous writes (or, put another way, writes to be buffered -
> the buffers are ordered so writes don't get re-ordered, but reads can
> happen during the buffering).
Right, for now I worry about UC semantics. Once we have nailed that, we
can look at WC, which is a lot more tricky as archs differs more
widely, but one thing at a time.
> But UC accesses are always done entirely ordered, and honestly, any
> status register that starts a DMA would not make sense any other way.
>
> Of course, you'd have to be pretty odd to want to start a DMA with a
> read anyway - partly exactly because it's bad for performance since
> reads will be synchronous and not buffered like a write).
I have bad memories of old adaptec controllers ...
That said, I think the above might not be right on ARM if we want to
make it the rule, Will, what do you reckon ?
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 6:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522211620.7364.94.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 6:33 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> This is why, I want (with your agreement) to define clearly and once
> and for all, that the Linux semantics of writel are that it is ordered
> with previous writes to coherent memory (*)
Honestly, I think those are the sane semantics. In fact, make it
"ordered with previous writes" full stop, since it's not only ordered
wrt previous writes to memory, but also previous writel's.
> Also, can I assume the above ordering with writel() equally applies to
> readl() or not ?
>
> IE:
> dma_buf->foo = 1;
> readl(STUPID_DEVICE_DMA_KICK_ON_READ);
If that KICK_ON_READ is UC, then that's definitely the case. And
honestly, status registers like that really should always be UC.
But if somebody sets the area WC (which is crazy), then I think it
might be at least debatable. x86 semantics does allow reads to be done
before previous writes (or, put another way, writes to be buffered -
the buffers are ordered so writes don't get re-ordered, but reads can
happen during the buffering).
But UC accesses are always done entirely ordered, and honestly, any
status register that starts a DMA would not make sense any other way.
Of course, you'd have to be pretty odd to want to start a DMA with a
read anyway - partly exactly because it's bad for performance since
reads will be synchronous and not buffered like a write).
Linus
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 6:14 UTC (permalink / raw)
To: Sinan Kaya
Cc: Benjamin Herrenschmidt, Alexander Duyck, Will Deacon,
Arnd Bergmann, Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <bf77be28-051d-79c8-9b65-948c8a254d6d@codeaurora.org>
On Tue, Mar 27, 2018 at 5:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>
> Basically changing it to
>
> dma_buffer->foo = 1; /* WB */
> wmb()
> writel_relaxed(KICK, DMA_KICK_REGISTER); /* UC */
> mmiowb()
Why?
Why not just remove the wmb(), and keep the barrier in the writel()?
The above code makes no sense, and just looks stupid to me. It also
generates pointlessly bad code on x86, so it's bad there too.
Linus
^ permalink raw reply
* Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP
From: Michael Ellerman @ 2018-03-28 5:54 UTC (permalink / raw)
To: Matt Evans, linuxppc-dev
In-Reply-To: <74EBF461-0D94-4C2D-ABA7-0B9BE010F173@ozlabs.org>
Matt Evans <matt@ozlabs.org> writes:
> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
> user context when single_step_exception() prepares the SIGTRAP
> delivery. The resulting branch-trap-within-the-SIGTRAP-handler
> isn't healthy.
>
> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
> to clear_single_step() which only clears MSR_SE.
>
> This patch adds a new helper, clear_br_trace(), which clears the
> debug trap before invoking the signal handler. This helper is a
> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>
> Signed-off-by: Matt Evans <matt@ozlabs.org>
Hi Matt!
It seems we might not be regularly testing this code :}
How did you hit/find the bug? And do you have a test case by any chance?
I found the test code at the bottom of:
https://lwn.net/Articles/114587/
But it didn't immediately work.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Fix sparse data type warnings in pci-ioda.c
From: Michael Ellerman @ 2018-03-28 4:53 UTC (permalink / raw)
To: Paul Mackerras, linuxppc-dev; +Cc: Alexey Kardashevskiy
In-Reply-To: <20180328004442.GA29865@fergus.ozlabs.ibm.com>
Paul Mackerras <paulus@ozlabs.org> writes:
> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
> The value passed to __raw_rm_writeq() and __raw_writeq() should be "u64"
> and "unsigned long". This fixes warning reported by sparse:
>
> gwshan@gwshan:~/sandbox/l$ make C=2 CF=-D__CHECK_ENDIAN__ \
> arch/powerpc/platforms/powernv/pci-ioda.o
> arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
> warning: incorrect type in argument 1 (different base types)
> arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
> expected unsigned long long [unsigned] [usertype] val
> arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
> got restricted __be64 [usertype] <noident>
> arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
> warning: incorrect type in argument 1 (different base types)
> arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
> expected unsigned long [unsigned] v
> arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
> got restricted __be64 [usertype] <noident>
>
> This also fixes another warning reported by sparse:
>
> gwshan@gwshan:~/sandbox/l$ make C=2 CF=-D__CHECK_ENDIAN__ \
> arch/powerpc/platforms/powernv/pci-ioda.o
> :
> arch/powerpc/platforms/powernv/pci-ioda.c:2647:45: \
> warning: cast to restricted __be64
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index a6c92c7..71de087 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1976,9 +1976,11 @@ static void pnv_pci_p7ioc_tce_invalidate(struct iommu_table *tbl,
> mb(); /* Ensure above stores are visible */
> while (start <= end) {
> if (rm)
> - __raw_rm_writeq(cpu_to_be64(start), invalidate);
> + __raw_rm_writeq((__force u64)cpu_to_be64(start),
> + invalidate);
> else
> - __raw_writeq(cpu_to_be64(start), invalidate);
> + __raw_writeq((__force unsigned long)cpu_to_be64(start),
> + invalidate);
I didn't merge this when it was originally sent because sprinkling
__force casts everywhere is not a good strategy for producing
maintainable code IMHO.
There's a writeq_be() which does the byte swap, I think we should
probably just mirror that for these __raw versions.
I'll do a patch.
cheers
^ permalink raw reply
* Re: [RFC PATCH v2 0/2] Randomization of address chosen by mmap.
From: Rob Landley @ 2018-03-28 4:50 UTC (permalink / raw)
To: Matthew Wilcox, Rich Felker
Cc: Ilya Smith, rth, ink, mattst88, vgupta, linux, tony.luck,
fenghua.yu, jhogan, ralf, jejb, deller, benh, paulus, mpe,
schwidefsky, heiko.carstens, ysato, davem, tglx, mingo, hpa, x86,
nyc, viro, arnd, gregkh, deepa.kernel, mhocko, hughd, kstewart,
pombredanne, akpm, steve.capper, punit.agrawal, paul.burton,
aneesh.kumar, npiggin, keescook, bhsharma, riel, nitin.m.gupta,
kirill.shutemov, dan.j.williams, jack, ross.zwisler, jglisse,
aarcange, oleg, linux-alpha, linux-kernel, linux-snps-arc,
linux-arm-kernel, linux-ia64, linux-metag, linux-mips,
linux-parisc, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-mm
In-Reply-To: <20180323190618.GA23763@bombadil.infradead.org>
On 03/23/2018 02:06 PM, Matthew Wilcox wrote:
> On Fri, Mar 23, 2018 at 02:00:24PM -0400, Rich Felker wrote:
>> On Fri, Mar 23, 2018 at 05:48:06AM -0700, Matthew Wilcox wrote:
>>> On Thu, Mar 22, 2018 at 07:36:36PM +0300, Ilya Smith wrote:
>>>> Current implementation doesn't randomize address returned by mmap.
>>>> All the entropy ends with choosing mmap_base_addr at the process
>>>> creation. After that mmap build very predictable layout of address
>>>> space. It allows to bypass ASLR in many cases. This patch make
>>>> randomization of address on any mmap call.
>>>
>>> Why should this be done in the kernel rather than libc? libc is perfectly
>>> capable of specifying random numbers in the first argument of mmap.
>>
>> Generally libc does not have a view of the current vm maps, and thus
>> in passing "random numbers", they would have to be uniform across the
>> whole vm space and thus non-uniform once the kernel rounds up to avoid
>> existing mappings.
>
> I'm aware that you're the musl author, but glibc somehow manages to
> provide etext, edata and end, demonstrating that it does know where at
> least some of the memory map lies.
You can parse /proc/self/maps, but it's really expensive and disgusting.
Rob
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 4:41 UTC (permalink / raw)
To: Sinan Kaya, Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Arnd Bergmann, Jason Gunthorpe,
David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <bf77be28-051d-79c8-9b65-948c8a254d6d@codeaurora.org>
On Tue, 2018-03-27 at 23:24 -0400, Sinan Kaya wrote:
> On 3/27/2018 10:51 PM, Linus Torvalds wrote:
> > > The discussion at hand is about
> > >
> > > dma_buffer->foo = 1; /* WB */
> > > writel(KICK, DMA_KICK_REGISTER); /* UC */
> >
> > Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> > even if that writel() might be of type WC, because that only delays
> > writes, it doesn't move them earlier.
>
> Now that we clarified x86 myth, Is this guaranteed on all architectures?
If not we need to fix it. It's guaranteed on the "main" ones (arm,
arm64, powerpc, i386, x86_64). We might need to check with other arch
maintainers for the rest.
We really want Linux to provide well defined "sane" semantics for the
basic writel accessors.
Note: We still have open questions about how readl() relates to
surrounding memory accesses. It looks like ARM and powerpc do different
things here.
> We keep getting IA64 exception example. Maybe, this is also corrected since
> then.
I would think ia64 fixed it back when it was all discussed. I was under
the impression all ia64 had "special" was the writel vs. spin_unlock
which requires mmiowb, but maybe that was never completely fixed ?
> Jose Abreu says "I don't know about x86 but arc architecture doesn't
> have a wmb() in the writel() function (in some configs)".
Well, it probably should then.
> As long as we have these exceptions, these wmb() in common drivers is not
> going anywhere and relaxed-arches will continue paying performance penalty.
Well, let's fix them or leave them broken, at this point, it doesn't
matter. We can give all arch maintainers a wakeup call and start making
drivers work based on the documented assumptions.
> I see 15% performance loss on ARM64 servers using Intel i40e network
> drivers and an XL710 adapter due to CPU keeping itself busy doing barriers
> most of the time rather than real work because of sequences like this all over
> the place.
>
> dma_buffer->foo = 1; /* WB */
> wmb()
> writel(KICK, DMA_KICK_REGISTER); /* UC */
>
> I posted several patches last week to remove duplicate barriers on ARM while
> trying to make the code friendly with other architectures.
>
> Basically changing it to
>
> dma_buffer->foo = 1; /* WB */
> wmb()
> writel_relaxed(KICK, DMA_KICK_REGISTER); /* UC */
> mmiowb()
>
> This is a small step in the performance direction until we remove all exceptions.
>
> https://www.spinics.net/lists/netdev/msg491842.html
> https://www.spinics.net/lists/linux-rdma/msg62434.html
> https://www.spinics.net/lists/arm-kernel/msg642336.html
>
> Discussion started to move around the need for relaxed API on PPC and then
> why wmb() question came up.
I'm working on the problem of relaxed APIs for powerpc, but we can keep
that orthogonal. As is, today, a wmb() + writel() and a wmb() +
writel_relaxed() on powerpc are identical. So changing them will not
break us.
But I don't see the point of doing that transformation if we can just
get the straying archs fixed. It's not like any of them has a
significant market presence these days anyway.
Cheers,
Ben.
> Sinan
>
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 4:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFyTeuXPmC5-1JGh2ow-rf5wMA6Vcv0aeS_q0_EnptSK9Q@mail.gmail.com>
On Tue, 2018-03-27 at 16:51 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 3:03 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > The discussion at hand is about
> >
> > dma_buffer->foo = 1; /* WB */
> > writel(KICK, DMA_KICK_REGISTER); /* UC */
>
> Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> even if that writel() might be of type WC, because that only delays
> writes, it doesn't move them earlier.
Ok so this is our answer ...
... snip ... (thanks for the background info !)
> Oh, the above UC case is absoutely guaranteed.
Good.
Then....
> The only issue really is that 99.9% of all testing gets done on x86
> unless you look at specific SoC drivers.
>
> On ARM, for example, there is likely little reason to care about x86
> memory ordering, because there is almost zero driver overlap between
> x86 and ARM.
>
> *Historically*, the reason for following the x86 IO ordering was
> simply that a lot of architectures used the drivers that were
> developed on x86. The alpha and powerpc workstations were *designed*
> with the x86 IO bus (PCI, then PCIe) and to work with the devices that
> came with it.
>
> ARM? PCIe is almost irrelevant. For ARM servers, if they ever take
> off, sure. But 99.99% of ARM is about their own SoC's, and so "x86
> test coverage" is simply not an issue.
>
> How much of an issue is it for Power? Maybe you decide it's not a big deal.
>
> Then all the above is almost irrelevant.
So the overlap may not be that NIL in practice :-) But even then that
doesn't matter as ARM has been happily implementing the same semantic
you describe above for years, as do we powerpc.
This is why, I want (with your agreement) to define clearly and once
and for all, that the Linux semantics of writel are that it is ordered
with previous writes to coherent memory (*)
This is already what ARM and powerpc provide, from what you say, what
x86 provides, I don't see any reason to keep that badly documented and
have drivers randomly growing useless wmb()'s because they don't think
it works on x86 without them !
Once that's sorted, let's tackle the problem of mmiowb vs. spin_unlock
and the problem of writel_relaxed semantics but as separate issues :-)
Also, can I assume the above ordering with writel() equally applies to
readl() or not ?
IE:
dma_buf->foo = 1;
readl(STUPID_DEVICE_DMA_KICK_ON_READ);
Also works on x86 ? (It does on power, maybe not on ARM).
Cheers,
Ben.
(*) From an Linux API perspective, all of this is only valid if the
memory was allocated by dma_alloc_coherent(). Anything obtained by
dma_map_something() might have been bounced bufferred or might require
extra cache flushes on some architectures, and thus needs
dma_sync_for_{cpu,device} calls.
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Sinan Kaya @ 2018-03-28 3:24 UTC (permalink / raw)
To: Linus Torvalds, Benjamin Herrenschmidt
Cc: Alexander Duyck, Will Deacon, Arnd Bergmann, Jason Gunthorpe,
David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFyTeuXPmC5-1JGh2ow-rf5wMA6Vcv0aeS_q0_EnptSK9Q@mail.gmail.com>
On 3/27/2018 10:51 PM, Linus Torvalds wrote:
>> The discussion at hand is about
>>
>> dma_buffer->foo = 1; /* WB */
>> writel(KICK, DMA_KICK_REGISTER); /* UC */
> Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> even if that writel() might be of type WC, because that only delays
> writes, it doesn't move them earlier.
Now that we clarified x86 myth, Is this guaranteed on all architectures?
We keep getting IA64 exception example. Maybe, this is also corrected since
then.
Jose Abreu says "I don't know about x86 but arc architecture doesn't
have a wmb() in the writel() function (in some configs)".
As long as we have these exceptions, these wmb() in common drivers is not
going anywhere and relaxed-arches will continue paying performance penalty.
I see 15% performance loss on ARM64 servers using Intel i40e network
drivers and an XL710 adapter due to CPU keeping itself busy doing barriers
most of the time rather than real work because of sequences like this all over
the place.
dma_buffer->foo = 1; /* WB */
wmb()
writel(KICK, DMA_KICK_REGISTER); /* UC */
I posted several patches last week to remove duplicate barriers on ARM while
trying to make the code friendly with other architectures.
Basically changing it to
dma_buffer->foo = 1; /* WB */
wmb()
writel_relaxed(KICK, DMA_KICK_REGISTER); /* UC */
mmiowb()
This is a small step in the performance direction until we remove all exceptions.
https://www.spinics.net/lists/netdev/msg491842.html
https://www.spinics.net/lists/linux-rdma/msg62434.html
https://www.spinics.net/lists/arm-kernel/msg642336.html
Discussion started to move around the need for relaxed API on PPC and then
why wmb() question came up.
Sinan
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] powerpc/mm: Fix printing of process table address
From: Balbir Singh @ 2018-03-28 3:09 UTC (permalink / raw)
To: linuxppc-dev
New changes to %p cause the information printed at boot
about radix-mmu partition and process table opaque, if
we really want to protect those values, we should make
dmesg more secure using CONFIG_SECURITY_DMESG_RESTRICT
Before the patches:
radix-mmu: Partition table (ptrval)
radix-mmu: Process table (ptrval) and radix root for kernel: (ptrval)
After the patches
radix-mmu: Partition table c0000000ffff0000
radix-mmu: Process table c0000000fe000000 and radix root for kernel: c0000000015a0000
Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
arch/powerpc/mm/pgtable-radix.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 2e10a964e290..3f676e89c3a6 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -329,7 +329,8 @@ static void __init radix_init_pgtable(void)
* physical address here.
*/
register_process_table(__pa(process_tb), 0, PRTB_SIZE_SHIFT - 12);
- pr_info("Process table %p and radix root for kernel: %p\n", process_tb, init_mm.pgd);
+ pr_info("Process table %px and radix root for kernel: %px\n",
+ process_tb, init_mm.pgd);
asm volatile("ptesync" : : : "memory");
asm volatile(PPC_TLBIE_5(%0,%1,2,1,1) : :
"r" (TLBIEL_INVAL_SET_LPID), "r" (0));
@@ -363,7 +364,7 @@ static void __init radix_init_partition_table(void)
mmu_partition_table_set_entry(0, dw0, 0);
pr_info("Initializing Radix MMU\n");
- pr_info("Partition table %p\n", partition_tb);
+ pr_info("Partition table %px\n", partition_tb);
}
void __init radix_init_native(void)
--
2.13.6
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 2:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522198981.7364.81.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 3:03 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> The discussion at hand is about
>
> dma_buffer->foo = 1; /* WB */
> writel(KICK, DMA_KICK_REGISTER); /* UC */
Yes. That certainly is ordered on x86. In fact, afaik it's ordered
even if that writel() might be of type WC, because that only delays
writes, it doesn't move them earlier.
Whether people *do* that or not, I don't know. But I wouldn't be
surprised if they do.
So if it's a DMA buffer, it's "cached". And even cached accesses are
ordered wrt MMIO.
Basically, to get unordered writes on x86, you need to either use
explicitly nontemporal stores, or have a writecombining region with
back-to-back writes that actually combine.
And nobody really does that nontemporal store thing any more because
the hardware that cared pretty much doesn't exist any more. It was too
much pain. People use DMA and maybe an UC store for starting the DMA
(or possibly a WC buffer that gets multiple stores in ascending order
as a stream of commands).
Things like UC will force everything to be entirely ordered, but even
without UC, loads won't pass loads, and stores won't pass stores.
> Now it appears that this wasn't fully understood back then, and some
> people are now saying that x86 might not even provide that semantic
> always.
Oh, the above UC case is absoutely guaranteed.
And I think even if it's WC, the write to kick off the DMA is ordered
wrt the cached write.
On x86, I think you need barriers only if you do things like
- do two non-temporal stores and require them to be ordered: put a
sfence or mfence in between them.
- do two WC stores, and make sure they do not combine: put a sfence
or mfence between them.
- do a store, and a subsequent from a different address, and neither
of them is UC: put a mfence between them. But note that this is
literally just "load after store". A "store after load" doesn't need
one.
I think that's pretty much it.
For example, the "lfence" instruction is almost entirely pointless on
x86 - it was designed back in the time when people *thought* they
might re-order loads. But loads don't get re-ordered. At least Intel
seems to document that only non-temporal *stores* can get re-ordered
wrt each other.
End result: lfence is a historical oddity that can now be used to
guarantee that a previous load has finished, and that in turn meant
that it is now used in the Spectre mitigations. But it basically has
no real memory ordering meaning since nothing passes an earlier load
anyway, it's more of a pipeline thing.
But in the end, one question is just "how much do drivers actually
_rely_ on the x86 strong ordering?"
We so support "smp_wmb()" even though x86 has strong enough ordering
that just a barrier suffices. Somebody might just say "screw the x86
memory ordering, we're relaxed, and we'll fix up the drivers we care
about".
The only issue really is that 99.9% of all testing gets done on x86
unless you look at specific SoC drivers.
On ARM, for example, there is likely little reason to care about x86
memory ordering, because there is almost zero driver overlap between
x86 and ARM.
*Historically*, the reason for following the x86 IO ordering was
simply that a lot of architectures used the drivers that were
developed on x86. The alpha and powerpc workstations were *designed*
with the x86 IO bus (PCI, then PCIe) and to work with the devices that
came with it.
ARM? PCIe is almost irrelevant. For ARM servers, if they ever take
off, sure. But 99.99% of ARM is about their own SoC's, and so "x86
test coverage" is simply not an issue.
How much of an issue is it for Power? Maybe you decide it's not a big deal.
Then all the above is almost irrelevant.
Linus
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 1:21 UTC (permalink / raw)
To: Will Deacon, Sinan Kaya
Cc: Arnd Bergmann, Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org, Alexander Duyck, torvalds
In-Reply-To: <20180327151029.GB17494@arm.com>
On Tue, 2018-03-27 at 16:10 +0100, Will Deacon wrote:
> To clarify: are you saying that on x86 you need a wmb() prior to a writel
> if you want that writel to be ordered after prior writes to memory? Is this
> specific to WC memory or some other non-standard attribute?
>
> The only reason we have wmb() inside writel() on arm, arm64 and power is for
> parity with x86 because Linus (CC'd) wanted architectures to order I/O vs
> memory by default so that it was easier to write portable drivers. The
> performance impact of that implicit barrier is non-trivial, but we want the
> driver portability and I went as far as adding generic _relaxed versions for
> the cases where ordering isn't required. You seem to be suggesting that none
> of this is necessary and drivers would already run into problems on x86 if
> they didn't use wmb() explicitly in conjunction with writel, which I find
> hard to believe and is in direct contradiction with the current Linux I/O
> memory model (modulo the broken example in the dma_*mb section of
> memory-barriers.txt).
Another clarification while we are at it ....
All of this only applies to concurrent access by the CPU and the device
to memory allocate with dma_alloc_coherent().
For memory "mapped" into the DMA domain via dma_map_* then an extra
dma_sync_for_* is needed.
In most useful server cases etc... these latter are NOPs, but
architecture without full DMA cache coherency or using swiotlb,
dma_map_* might maintain bounce buffers or play additional cache
flushing tricks.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Fix sparse data type warnings in pci-ioda.c
From: Alexey Kardashevskiy @ 2018-03-28 1:21 UTC (permalink / raw)
To: Paul Mackerras, linuxppc-dev
In-Reply-To: <20180328004442.GA29865@fergus.ozlabs.ibm.com>
On 28/3/18 11:44 am, Paul Mackerras wrote:
> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
> The value passed to __raw_rm_writeq() and __raw_writeq() should be "u64"
> and "unsigned long". This fixes warning reported by sparse:
>
> gwshan@gwshan:~/sandbox/l$ make C=2 CF=-D__CHECK_ENDIAN__ \
> arch/powerpc/platforms/powernv/pci-ioda.o
> arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
> warning: incorrect type in argument 1 (different base types)
> arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
> expected unsigned long long [unsigned] [usertype] val
> arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
> got restricted __be64 [usertype] <noident>
> arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
> warning: incorrect type in argument 1 (different base types)
> arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
> expected unsigned long [unsigned] v
> arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
> got restricted __be64 [usertype] <noident>
>
> This also fixes another warning reported by sparse:
>
> gwshan@gwshan:~/sandbox/l$ make C=2 CF=-D__CHECK_ENDIAN__ \
> arch/powerpc/platforms/powernv/pci-ioda.o
> :
> arch/powerpc/platforms/powernv/pci-ioda.c:2647:45: \
> warning: cast to restricted __be64
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index a6c92c7..71de087 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1976,9 +1976,11 @@ static void pnv_pci_p7ioc_tce_invalidate(struct iommu_table *tbl,
> mb(); /* Ensure above stores are visible */
> while (start <= end) {
> if (rm)
> - __raw_rm_writeq(cpu_to_be64(start), invalidate);
> + __raw_rm_writeq((__force u64)cpu_to_be64(start),
> + invalidate);
> else
> - __raw_writeq(cpu_to_be64(start), invalidate);
> + __raw_writeq((__force unsigned long)cpu_to_be64(start),
> + invalidate);
> start += inc;
> }
>
> @@ -2055,9 +2057,10 @@ static void pnv_pci_phb3_tce_invalidate_entire(struct pnv_phb *phb, bool rm)
>
> mb(); /* Ensure previous TCE table stores are visible */
> if (rm)
> - __raw_rm_writeq(cpu_to_be64(val), invalidate);
> + __raw_rm_writeq((__force u64)cpu_to_be64(val), invalidate);
> else
> - __raw_writeq(cpu_to_be64(val), invalidate);
> + __raw_writeq((__force unsigned long)cpu_to_be64(val),
> + invalidate);
> }
>
> static inline void pnv_pci_phb3_tce_invalidate_pe(struct pnv_ioda_pe *pe)
> @@ -2067,7 +2070,7 @@ static inline void pnv_pci_phb3_tce_invalidate_pe(struct pnv_ioda_pe *pe)
> unsigned long val = PHB3_TCE_KILL_INVAL_PE | (pe->pe_number & 0xFF);
>
> mb(); /* Ensure above stores are visible */
> - __raw_writeq(cpu_to_be64(val), invalidate);
> + __raw_writeq((__force unsigned long)cpu_to_be64(val), invalidate);
> }
>
> static void pnv_pci_phb3_tce_invalidate(struct pnv_ioda_pe *pe, bool rm,
> @@ -2090,9 +2093,11 @@ static void pnv_pci_phb3_tce_invalidate(struct pnv_ioda_pe *pe, bool rm,
>
> while (start <= end) {
> if (rm)
> - __raw_rm_writeq(cpu_to_be64(start), invalidate);
> + __raw_rm_writeq((__force u64)cpu_to_be64(start),
> + invalidate);
> else
> - __raw_writeq(cpu_to_be64(start), invalidate);
> + __raw_writeq((__force unsigned long)cpu_to_be64(start),
> + invalidate);
> start += inc;
> }
> }
> @@ -2864,7 +2869,8 @@ static void pnv_pci_ioda2_table_do_free_pages(__be64 *addr,
> u64 *tmp = (u64 *) addr_ul;
>
> for (i = 0; i < size; ++i) {
> - unsigned long hpa = be64_to_cpu(tmp[i]);
> + unsigned long hpa =
> + be64_to_cpu((__force __be64)(tmp[i]));
>
> if (!(hpa & (TCE_PCI_READ | TCE_PCI_WRITE)))
> continue;
>
--
Alexey
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 1:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFw2R4pirO0m+jtj4F8AF2-tVi3guEaWOA-EBqNyFmh29w@mail.gmail.com>
On Tue, 2018-03-27 at 14:39 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 11:33 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > Well, we need to clarify that once and for all, because as I wrote
> > earlier, it was decreed by Linus more than a decade ago that writel
> > would be fully ordered by itself vs. previous memory stores (at least
> > on UC memory).
>
> Yes.
>
> So "writel()" needs to be ordered with respect to other writel() uses
> on the same thread. Anything else *will* break drivers. Obviously, the
> drivers may then do magic to say "do write combining etc", but that
> magic will be architecture-specific.
>
> The other issue is that "writel()" needs to be ordered wrt other CPU's
> doing "writel()" if those writel's are in a spinlocked region.
.../...
The discussion at hand is about
dma_buffer->foo = 1; /* WB */
writel(KICK, DMA_KICK_REGISTER); /* UC */
(The WC case is something else, let's not mix things up just yet)
IE, a store to normal WB cache memory followed by a writel to a device
which will then DMA from that buffer.
Back in the days, we did require on powerpc a wmb() between these, but
you made the point that x86 didn't and driver writers would never get
that right.
We decided to go conservative, added the necessary barrier inside
writel, so did ARM and it became the norm that writel is also fully
ordered vs. previous stores to memory *by the same CPU* of course (or
protected by the same spinlock).
Now it appears that this wasn't fully understood back then, and some
people are now saying that x86 might not even provide that semantic
always.
So a number (fairly large) of drivers have been adding wmb() in those
case, while others haven't, and it's a mess.
The mess is compounded by the fact that if writel is now defined to
*not* provide that ordering guarantee, then writel_relaxed() is
pointless since all it is defined to relax is precisely the above
ordering guarantee.
So I want to get to the bottom of this once and for all so we can have
well defined and documented semantics and stop having drivers do random
things that may or may not work on some or all architectures (including
x86 !).
Quite note about the spinlock case... In fact this is the only case you
did allow back then to be relaxed. In theory a writel followed by a
spin_unlock requires an mmiowb (which is the only point of that barrier
in fact). This was done because an arch (I think ia64) had a hard time
getting MMIOs from multiple CPUs get in order vs. a lock and required
an expensive access to the PCI host bridge to do so.
Back then, on powerpc, we chose not to allow that relaxing and instead
added code to our writel to set a per-cpu flag which would cause the
next spin_unlock to use a stronger barrier than usual.
We do need to clarify this as well, but let's start with the most basic
one first, there is enough confusion already.
Cheers,
Ben.
^ permalink raw reply
* [PATCH] powerpc/powernv: Fix sparse data type warnings in pci-ioda.c
From: Paul Mackerras @ 2018-03-28 0:44 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy
From: Gavin Shan <gwshan@linux.vnet.ibm.com>
The value passed to __raw_rm_writeq() and __raw_writeq() should be "u64"
and "unsigned long". This fixes warning reported by sparse:
gwshan@gwshan:~/sandbox/l$ make C=2 CF=-D__CHECK_ENDIAN__ \
arch/powerpc/platforms/powernv/pci-ioda.o
arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
warning: incorrect type in argument 1 (different base types)
arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
expected unsigned long long [unsigned] [usertype] val
arch/powerpc/platforms/powernv/pci-ioda.c:1794:41: \
got restricted __be64 [usertype] <noident>
arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
warning: incorrect type in argument 1 (different base types)
arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
expected unsigned long [unsigned] v
arch/powerpc/platforms/powernv/pci-ioda.c:1796:38: \
got restricted __be64 [usertype] <noident>
This also fixes another warning reported by sparse:
gwshan@gwshan:~/sandbox/l$ make C=2 CF=-D__CHECK_ENDIAN__ \
arch/powerpc/platforms/powernv/pci-ioda.o
:
arch/powerpc/platforms/powernv/pci-ioda.c:2647:45: \
warning: cast to restricted __be64
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index a6c92c7..71de087 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1976,9 +1976,11 @@ static void pnv_pci_p7ioc_tce_invalidate(struct iommu_table *tbl,
mb(); /* Ensure above stores are visible */
while (start <= end) {
if (rm)
- __raw_rm_writeq(cpu_to_be64(start), invalidate);
+ __raw_rm_writeq((__force u64)cpu_to_be64(start),
+ invalidate);
else
- __raw_writeq(cpu_to_be64(start), invalidate);
+ __raw_writeq((__force unsigned long)cpu_to_be64(start),
+ invalidate);
start += inc;
}
@@ -2055,9 +2057,10 @@ static void pnv_pci_phb3_tce_invalidate_entire(struct pnv_phb *phb, bool rm)
mb(); /* Ensure previous TCE table stores are visible */
if (rm)
- __raw_rm_writeq(cpu_to_be64(val), invalidate);
+ __raw_rm_writeq((__force u64)cpu_to_be64(val), invalidate);
else
- __raw_writeq(cpu_to_be64(val), invalidate);
+ __raw_writeq((__force unsigned long)cpu_to_be64(val),
+ invalidate);
}
static inline void pnv_pci_phb3_tce_invalidate_pe(struct pnv_ioda_pe *pe)
@@ -2067,7 +2070,7 @@ static inline void pnv_pci_phb3_tce_invalidate_pe(struct pnv_ioda_pe *pe)
unsigned long val = PHB3_TCE_KILL_INVAL_PE | (pe->pe_number & 0xFF);
mb(); /* Ensure above stores are visible */
- __raw_writeq(cpu_to_be64(val), invalidate);
+ __raw_writeq((__force unsigned long)cpu_to_be64(val), invalidate);
}
static void pnv_pci_phb3_tce_invalidate(struct pnv_ioda_pe *pe, bool rm,
@@ -2090,9 +2093,11 @@ static void pnv_pci_phb3_tce_invalidate(struct pnv_ioda_pe *pe, bool rm,
while (start <= end) {
if (rm)
- __raw_rm_writeq(cpu_to_be64(start), invalidate);
+ __raw_rm_writeq((__force u64)cpu_to_be64(start),
+ invalidate);
else
- __raw_writeq(cpu_to_be64(start), invalidate);
+ __raw_writeq((__force unsigned long)cpu_to_be64(start),
+ invalidate);
start += inc;
}
}
@@ -2864,7 +2869,8 @@ static void pnv_pci_ioda2_table_do_free_pages(__be64 *addr,
u64 *tmp = (u64 *) addr_ul;
for (i = 0; i < size; ++i) {
- unsigned long hpa = be64_to_cpu(tmp[i]);
+ unsigned long hpa =
+ be64_to_cpu((__force __be64)(tmp[i]));
if (!(hpa & (TCE_PCI_READ | TCE_PCI_WRITE)))
continue;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox