LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v9 01/24] mm: Introduce CONFIG_SPECULATIVE_PAGE_FAULT
From: Laurent Dufour @ 2018-03-28 11:15 UTC (permalink / raw)
  To: David Rientjes
  Cc: Thomas Gleixner, 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.1803280310380.68839@chino.kir.corp.google.com>



On 28/03/2018 12:16, David Rientjes wrote:
> On Wed, 28 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 ?
>>
> 
> Putting this in mm/Kconfig is definitely the right way to go about it 
> instead of any generic option in arch/*.
> 
> My question, though, was making this configurable by the user:
> 
> config SPECULATIVE_PAGE_FAULT
> 	bool "Speculative page faults"
> 	depends on X86_64 || PPC
> 	default y
> 	help
> 	  ..
> 
> It's a question about whether we want this always enabled on x86_64 and 
> power or whether the user should be able to disable it (right now they 
> can't).  With a large feature like this, you may want to offer something 
> simple (disable CONFIG_SPECULATIVE_PAGE_FAULT) if someone runs into 
> regressions.

I agree, but I think it would be important to get the per architecture
enablement to avoid complex check here. For instance in the case of powerPC
this is only supported for PPC_BOOK3S_64.

To avoid exposing such per architecture define here, what do you think about
having supporting architectures setting ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT
and the SPECULATIVE_PAGE_FAULT depends on this, like this:

In mm/Kconfig:
config SPECULATIVE_PAGE_FAULT
 	bool "Speculative page faults"
 	depends on ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT && SMP
 	default y
 	help
		...

In arch/powerpc/Kconfig:
config PPC
	...
	select ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT	if PPC_BOOK3S_64

In arch/x86/Kconfig:
config X86_64
	...
	select ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT

^ permalink raw reply

* Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync()
From: Yury Norov @ 2018-03-28 10:58 UTC (permalink / raw)
  To: Will Deacon
  Cc: Paul E. McKenney, Chris Metcalf, Christopher Lameter,
	Russell King - ARM Linux, Mark Rutland, Steven Rostedt,
	Mathieu Desnoyers, Catalin Marinas, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman, linux-arm-kernel, linuxppc-dev,
	kvm-ppc, linux-mm, linux-kernel
In-Reply-To: <20180327102116.GA2464@arm.com>

On Tue, Mar 27, 2018 at 11:21:17AM +0100, Will Deacon wrote:
> On Sun, Mar 25, 2018 at 08:50:04PM +0300, Yury Norov wrote:
> > kick_all_cpus_sync() forces all CPUs to sync caches by sending broadcast IPI.
> > If CPU is in extended quiescent state (idle task or nohz_full userspace), this
> > work may be done at the exit of this state. Delaying synchronization helps to
> > save power if CPU is in idle state and decrease latency for real-time tasks.
> > 
> > This patch introduces kick_active_cpus_sync() and uses it in mm/slab and arm64
> > code to delay syncronization.
> > 
> > For task isolation (https://lkml.org/lkml/2017/11/3/589), IPI to the CPU running
> > isolated task would be fatal, as it breaks isolation. The approach with delaying
> > of synchronization work helps to maintain isolated state.
> > 
> > I've tested it with test from task isolation series on ThunderX2 for more than
> > 10 hours (10k giga-ticks) without breaking isolation.
> > 
> > Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> > ---
> >  arch/arm64/kernel/insn.c |  2 +-
> >  include/linux/smp.h      |  2 ++
> >  kernel/smp.c             | 24 ++++++++++++++++++++++++
> >  mm/slab.c                |  2 +-
> >  4 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> > index 2718a77da165..9d7c492e920e 100644
> > --- a/arch/arm64/kernel/insn.c
> > +++ b/arch/arm64/kernel/insn.c
> > @@ -291,7 +291,7 @@ int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)
> >  			 * synchronization.
> >  			 */
> >  			ret = aarch64_insn_patch_text_nosync(addrs[0], insns[0]);
> > -			kick_all_cpus_sync();
> > +			kick_active_cpus_sync();
> >  			return ret;
> >  		}
> >  	}
> 
> I think this means that runtime modifications to the kernel text might not
> be picked up by CPUs coming out of idle. Shouldn't we add an ISB on that
> path to avoid executing stale instructions?

Thanks, Will, for the hint. I'll do that.

Yury

^ permalink raw reply

* Re: [PATCH v9 06/24] mm: make pte_unmap_same compatible with SPF
From: Laurent Dufour @ 2018-03-28 10:43 UTC (permalink / raw)
  To: David Rientjes
  Cc: paulmck, peterz, Andrew Morton, 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.1803280318440.69353@chino.kir.corp.google.com>



On 28/03/2018 12:20, David Rientjes wrote:
> On Wed, 28 Mar 2018, Laurent Dufour wrote:
> 
>>>> @@ -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 ?
>>
> 
> The initialization of the ret variable.
> 
> @@ -2913,7 +2921,8 @@ int do_swap_page(struct vm_fault *vmf)
>  	int exclusive = 0;
>  	int ret = 0;
> 
> -	if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte))
> +	ret = pte_unmap_same(vmf);
> +	if (ret)
>  		goto out;
> 
>  	entry = pte_to_swp_entry(vmf->orig_pte);
> 
> "ret" is immediately set to the return value of pte_unmap_same(), so there 
> is no need to initialize it to 0.

Sorry, I missed that. I'll remove this initialization.

Thanks,
Laurent.

^ permalink raw reply

* Re: Aw: Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 10:20 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Will Deacon, Linus Torvalds, Alexander Duyck, 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: <trinity-2ee224b5-cbac-415e-9f8c-305e802b5369-1522232002354@3c-app-gmx-bs61>

On Wed, 2018-03-28 at 12:13 +0200, Lino Sanfilippo wrote:
> Hi,
> 
> 
> > 
> > Yeah so that other trick I'm talking about is also used for timing
> > accuracy.
> > 
> > For example, let's say I have a device with a reset bit and the spec
> > says the reset bit needs to be set for at least 10us.
> > 
> > This is wrong:
> > 
> > 	writel(1, RESET_REG);
> > 	usleep(10);
> > 	writel(0, RESET_REG);
> > 
> > Because of write posting, the first write might arrive to the device
> > right before the second one.
> > 
> 
> Does not write posting only concern PCI? This seems to be a different topic. Furthermore
> write posting should not include write reordering...

Nobody's talking about re-ordering and no, write posting is rather
common practice on a whole lot of different busses, not just PCI(e).

Cheers,
Ben.

^ permalink raw reply

* Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP
From: Matt Evans @ 2018-03-28 10:36 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <877epw3fu2.fsf@concordia.ellerman.id.au>

Howdy Michael,

> On 28 Mar 2018, at 06:54, Michael Ellerman <mpe@ellerman.id.au> wrote:
>=20
> Matt Evans <matt@ozlabs.org> writes:
>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>=20
> Hi Matt!
>=20
> It seems we might not be regularly testing this code :}

I know, rite? ;-)

> How did you hit/find the bug? And do you have a test case by any =
chance?
>=20
> I found the test code at the bottom of:
>  https://lwn.net/Articles/114587/
>=20
> But it didn't immediately work.

I'm using this feature as part of a debug harness I wrote to log a =
program=E2=80=99s control flow (to create a =E2=80=9Cknown good=E2=80=9D =
pattern to compare a PPC interpreter against).  So at least the feature =
has /one/ user.  ;-)

The symptoms of the bug are that if you use single-stepping you get a =
sequence of SIGTRAPs representing each instruction completion (good), =
but if you use branch tracing the process just dies with SIGTRAP (looks =
like it=E2=80=99s never caught by the signal handler).  What=E2=80=99s =
really happening is that there /is/ a signal delivered to the handler, =
but (because branch tracing is left on) that then causes a second debug =
exception from the handler itself, i.e. whilst SIGTRAP=E2=80=99s masked.

OK, let me have a dig to reduce my program to something very basic and =
I=E2=80=99ll post something =E2=80=94 sorry, I should=E2=80=99ve got a =
PoC ready before.  (I did start out inspired by that post you linked to, =
but IIRC I don=E2=80=99t think it worked out of the box for me either.)


Cheers,


Matt

^ permalink raw reply

* Re: [PATCH v9 04/24] mm: Prepare for FAULT_FLAG_SPECULATIVE
From: Laurent Dufour @ 2018-03-28 10: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.1803251426120.80485@chino.kir.corp.google.com>

On 25/03/2018 23:50, David Rientjes wrote:
> On Tue, 13 Mar 2018, Laurent Dufour wrote:
> 
>> From: Peter Zijlstra <peterz@infradead.org>
>>
>> When speculating faults (without holding mmap_sem) we need to validate
>> that the vma against which we loaded pages is still valid when we're
>> ready to install the new PTE.
>>
>> Therefore, replace the pte_offset_map_lock() calls that (re)take the
>> PTL with pte_map_lock() which can fail in case we find the VMA changed
>> since we started the fault.
>>
> 
> Based on how its used, I would have suspected this to be named 
> pte_map_trylock().
> 
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>
>> [Port to 4.12 kernel]
>> [Remove the comment about the fault_env structure which has been
>>  implemented as the vm_fault structure in the kernel]
>> [move pte_map_lock()'s definition upper in the file]
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>>  include/linux/mm.h |  1 +
>>  mm/memory.c        | 56 ++++++++++++++++++++++++++++++++++++++----------------
>>  2 files changed, 41 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 4d02524a7998..2f3e98edc94a 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -300,6 +300,7 @@ extern pgprot_t protection_map[16];
>>  #define FAULT_FLAG_USER		0x40	/* The fault originated in userspace */
>>  #define FAULT_FLAG_REMOTE	0x80	/* faulting for non current tsk/mm */
>>  #define FAULT_FLAG_INSTRUCTION  0x100	/* The fault was during an instruction fetch */
>> +#define FAULT_FLAG_SPECULATIVE	0x200	/* Speculative fault, not holding mmap_sem */
>>  
>>  #define FAULT_FLAG_TRACE \
>>  	{ FAULT_FLAG_WRITE,		"WRITE" }, \
> 
> I think FAULT_FLAG_SPECULATIVE should be introduced in the patch that 
> actually uses it.

I think you're right, I'll move down this define in the series.

>> diff --git a/mm/memory.c b/mm/memory.c
>> index e0ae4999c824..8ac241b9f370 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_map_lock(struct vm_fault *vmf)
> 
> inline?

Agreed.

>> +{
>> +	vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd,
>> +				       vmf->address, &vmf->ptl);
>> +	return true;
>> +}
>> +
>>  /*
>>   * handle_pte_fault chooses page fault handler according to an entry which was
>>   * read non-atomically.  Before making any commitment, on those architectures
>> @@ -2477,6 +2484,7 @@ static int wp_page_copy(struct vm_fault *vmf)
>>  	const unsigned long mmun_start = vmf->address & PAGE_MASK;
>>  	const unsigned long mmun_end = mmun_start + PAGE_SIZE;
>>  	struct mem_cgroup *memcg;
>> +	int ret = VM_FAULT_OOM;
>>  
>>  	if (unlikely(anon_vma_prepare(vma)))
>>  		goto oom;
>> @@ -2504,7 +2512,11 @@ static int wp_page_copy(struct vm_fault *vmf)
>>  	/*
>>  	 * Re-check the pte - we dropped the lock
>>  	 */
>> -	vmf->pte = pte_offset_map_lock(mm, vmf->pmd, vmf->address, &vmf->ptl);
>> +	if (!pte_map_lock(vmf)) {
>> +		mem_cgroup_cancel_charge(new_page, memcg, false);
>> +		ret = VM_FAULT_RETRY;
>> +		goto oom_free_new;
>> +	}
> 
> Ugh, but we aren't oom here, so maybe rename oom_free_new so that it makes 
> sense for return values other than VM_FAULT_OOM?

You're right, now this label name is not correct, I'll rename it to
"out_free_new" and rename also the label "oom" to "out" since it is generic too
now.

>>  	if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
>>  		if (old_page) {
>>  			if (!PageAnon(old_page)) {
>> @@ -2596,7 +2608,7 @@ static int wp_page_copy(struct vm_fault *vmf)
>>  oom:
>>  	if (old_page)
>>  		put_page(old_page);
>> -	return VM_FAULT_OOM;
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -2617,8 +2629,8 @@ static int wp_page_copy(struct vm_fault *vmf)
>>  int finish_mkwrite_fault(struct vm_fault *vmf)
>>  {
>>  	WARN_ON_ONCE(!(vmf->vma->vm_flags & VM_SHARED));
>> -	vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd, vmf->address,
>> -				       &vmf->ptl);
>> +	if (!pte_map_lock(vmf))
>> +		return VM_FAULT_RETRY;
>>  	/*
>>  	 * We might have raced with another page fault while we released the
>>  	 * pte_offset_map_lock.
>> @@ -2736,8 +2748,11 @@ static int do_wp_page(struct vm_fault *vmf)
>>  			get_page(vmf->page);
>>  			pte_unmap_unlock(vmf->pte, vmf->ptl);
>>  			lock_page(vmf->page);
>> -			vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> -					vmf->address, &vmf->ptl);
>> +			if (!pte_map_lock(vmf)) {
>> +				unlock_page(vmf->page);
>> +				put_page(vmf->page);
>> +				return VM_FAULT_RETRY;
>> +			}
>>  			if (!pte_same(*vmf->pte, vmf->orig_pte)) {
>>  				unlock_page(vmf->page);
>>  				pte_unmap_unlock(vmf->pte, vmf->ptl);
>> @@ -2947,8 +2962,10 @@ int do_swap_page(struct vm_fault *vmf)
>>  			 * Back out if somebody else faulted in this pte
>>  			 * while we released the pte lock.
>>  			 */
> 
> Comment needs updating, pte_same() isn't the only reason to bail out here.

I'll update it to :
			/*
			 * Back out if the VMA has changed in our back during
			 * a speculative page fault or if somebody else
			 * faulted in this pte while we released the pte lock.
			 */

> 
>> -			vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> -					vmf->address, &vmf->ptl);
>> +			if (!pte_map_lock(vmf)) {
>> +				delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
>> +				return VM_FAULT_RETRY;
>> +			}
>>  			if (likely(pte_same(*vmf->pte, vmf->orig_pte)))
>>  				ret = VM_FAULT_OOM;
>>  			delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
> 
> Not crucial, but it would be nice if this could do goto out instead, 
> otherwise this is the first mid function return.

ok will do.

> 
>> @@ -3003,8 +3020,11 @@ int do_swap_page(struct vm_fault *vmf)
>>  	/*
>>  	 * Back out if somebody else already faulted in this pte.
>>  	 */
> 
> Same as above.

Ok changing as above.

> 
>> -	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>> -			&vmf->ptl);
>> +	if (!pte_map_lock(vmf)) {
>> +		ret = VM_FAULT_RETRY;
>> +		mem_cgroup_cancel_charge(page, memcg, false);
>> +		goto out_page;
>> +	}
>>  	if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte)))
>>  		goto out_nomap;
>>  
> 
> mem_cgroup_try_charge() is done before grabbing pte_offset_map_lock(), why 
> does the out_nomap exit path do mem_cgroup_cancel_charge(); 
> pte_unmap_unlock()?  If the pte lock can be droppde first, there's no need 
> to embed the mem_cgroup_cancel_charge() here.

I think we can safely invert the call to mem_cgroup_cancel_charge() and to
pte_unmap_unlock(), and then introduce a new label and jump in if
pte_map_lock() failed.
Something like this:

@@ -3001,10 +3020,13 @@ int do_swap_page(struct vm_fault *vmf)
        }

        /*
-        * Back out if somebody else already faulted in this pte.
+        * Back out if the VMA has changed in our back during a speculative
+        * page fault or if somebody else already faulted in this pte.
         */
-       vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
-                       &vmf->ptl);
+       if (!pte_map_lock(vmf)) {
+               ret = VM_FAULT_RETRY;
+               goto out_cancel_cgroup;
+       }
        if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte)))
                goto out_nomap;

@@ -3082,8 +3104,9 @@ int do_swap_page(struct vm_fault *vmf)
 out:
        return ret;
 out_nomap:
-       mem_cgroup_cancel_charge(page, memcg, false);
        pte_unmap_unlock(vmf->pte, vmf->ptl);
+out_cancel_cgroup:
+       mem_cgroup_cancel_charge(page, memcg, false);
 out_page:
        unlock_page(page);
 out_release:



>> @@ -3133,8 +3153,8 @@ static int do_anonymous_page(struct vm_fault *vmf)
>>  			!mm_forbids_zeropage(vma->vm_mm)) {
>>  		entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address),
>>  						vma->vm_page_prot));
>> -		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> -				vmf->address, &vmf->ptl);
>> +		if (!pte_map_lock(vmf))
>> +			return VM_FAULT_RETRY;
>>  		if (!pte_none(*vmf->pte))
>>  			goto unlock;
>>  		ret = check_stable_address_space(vma->vm_mm);
>> @@ -3169,8 +3189,11 @@ static int do_anonymous_page(struct vm_fault *vmf)
>>  	if (vma->vm_flags & VM_WRITE)
>>  		entry = pte_mkwrite(pte_mkdirty(entry));
>>  
>> -	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>> -			&vmf->ptl);
>> +	if (!pte_map_lock(vmf)) {
>> +		mem_cgroup_cancel_charge(page, memcg, false);
>> +		put_page(page);
>> +		return VM_FAULT_RETRY;
>> +	}
>>  	if (!pte_none(*vmf->pte))
>>  		goto release;
>>  
> 
> This is more spaghetti, can the exit path be fixed up so we order things 
> consistently for all gotos?

I do agree, this was due to inverted calls to mem_cgroup_cancel_charge() and
pte_unmap_unlock().

This will become:
@@ -3170,14 +3193,16 @@ static int do_anonymous_page(struct vm_fault *vmf)
        if (vma->vm_flags & VM_WRITE)
                entry = pte_mkwrite(pte_mkdirty(entry));

-       vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
-                       &vmf->ptl);
-       if (!pte_none(*vmf->pte))
+       if (!pte_map_lock(vmf)) {
+               ret = VM_FAULT_RETRY;
                goto release;
+       }
+       if (!pte_none(*vmf->pte))
+               goto unlock_and_release;

        ret = check_stable_address_space(vma->vm_mm);
        if (ret)
-               goto release;
+               goto unlock_and_release;

        /* Deliver the page fault to userland, check inside PT lock */
        if (userfaultfd_missing(vma)) {
@@ -3199,10 +3224,12 @@ static int do_anonymous_page(struct vm_fault *vmf)
 unlock:
        pte_unmap_unlock(vmf->pte, vmf->ptl);
        return ret;
+unlock_and_release:
+       pte_unmap_unlock(vmf->pte, vmf->ptl);
 release:
        mem_cgroup_cancel_charge(page, memcg, false);
        put_page(page);
-       goto unlock;
+       return ret;
 oom_free_page:
        put_page(page);
 oom:

Thanks,
Laurent.

> 
>> @@ -3294,8 +3317,9 @@ static int pte_alloc_one_map(struct vm_fault *vmf)
>>  	 * pte_none() under vmf->ptl protection when we return to
>>  	 * alloc_set_pte().
>>  	 */
>> -	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
>> -			&vmf->ptl);
>> +	if (!pte_map_lock(vmf))
>> +		return VM_FAULT_RETRY;
>> +
>>  	return 0;
>>  }
>>  
> 

^ permalink raw reply

* Re: [PATCH v9 06/24] mm: make pte_unmap_same compatible with SPF
From: David Rientjes @ 2018-03-28 10:20 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, peterz, Andrew Morton, 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: <fd9eedf4-b885-d8f5-2daa-4cc450e72427@linux.vnet.ibm.com>

On Wed, 28 Mar 2018, Laurent Dufour wrote:

> >> @@ -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 ?
> 

The initialization of the ret variable.

@@ -2913,7 +2921,8 @@ int do_swap_page(struct vm_fault *vmf)
 	int exclusive = 0;
 	int ret = 0;
 
-	if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte))
+	ret = pte_unmap_same(vmf);
+	if (ret)
 		goto out;
 
 	entry = pte_to_swp_entry(vmf->orig_pte);

"ret" is immediately set to the return value of pte_unmap_same(), so there 
is no need to initialize it to 0.

^ permalink raw reply

* Aw: Re: RFC on writel and writel_relaxed
From: Lino Sanfilippo @ 2018-03-28 10:13 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Will Deacon, Linus Torvalds, Alexander Duyck, 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: <1522230988.21446.7.camel@kernel.crashing.org>

Hi,


> 
> Yeah so that other trick I'm talking about is also used for timing
> accuracy.
> 
> For example, let's say I have a device with a reset bit and the spec
> says the reset bit needs to be set for at least 10us.
> 
> This is wrong:
> 
> 	writel(1, RESET_REG);
> 	usleep(10);
> 	writel(0, RESET_REG);
> 
> Because of write posting, the first write might arrive to the device
> right before the second one.
> 

Does not write posting only concern PCI? This seems to be a different topic. Furthermore
write posting should not include write reordering...

Regards,
Lino

^ permalink raw reply

* Re: [PATCH v9 01/24] mm: Introduce CONFIG_SPECULATIVE_PAGE_FAULT
From: David Rientjes @ 2018-03-28 10:16 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: Thomas Gleixner, 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: <32c80b6a-28c6-bf63-ed7b-6a042ae18e8f@linux.vnet.ibm.com>

On Wed, 28 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 ?
> 

Putting this in mm/Kconfig is definitely the right way to go about it 
instead of any generic option in arch/*.

My question, though, was making this configurable by the user:

config SPECULATIVE_PAGE_FAULT
	bool "Speculative page faults"
	depends on X86_64 || PPC
	default y
	help
	  ..

It's a question about whether we want this always enabled on x86_64 and 
power or whether the user should be able to disable it (right now they 
can't).  With a large feature like this, you may want to offer something 
simple (disable CONFIG_SPECULATIVE_PAGE_FAULT) if someone runs into 
regressions.

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-28 10:13 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Arnd Bergmann, 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, Peter Zijlstra,
	Ingo Molnar, Jonathan Corbet
In-Reply-To: <1522231287.21446.9.camel@kernel.crashing.org>

On Wed, Mar 28, 2018 at 09:01:27PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2018-03-28 at 11:55 +0200, Arnd Bergmann wrote:
> > > powerpc and ARM can't quite make them synchronous I think, but at least
> > > they should have the same semantics as writel.
> > 
> > One thing that ARM does IIRC is that it only guarantees to order writel() within
> > one device, and the memory mapped PCI I/O space window almost certainly
> > counts as a separate device to the CPU.
> 
> That sounds bogus.

To elaborate, if you do the following on arm:

	writel(DEVICE_FOO);
	writel(DEVICE_BAR);

we generally cannot guarantee in which order those accesses will hit the
devices even if we add every barrier under the sun. You'd need something
in between, specific to DEVICE_FOO (probably a read-back) to really push
the first write out. This doesn't sound like it would be that uncommon to
me.

On the other hand:

	writel(DEVICE_FOO);
	writel(DEVICE_FOO);

is obviously ordered and also things like:

	writel(DEVICE_FOO_IN_PCI_MEM_SPACE);
	writel(DEVICE_BAR_IN_SAME_PCI_MEM_SPACE);

are ordered up to the PCI host bridge, because that's really the "device"
here.

Will

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 10:01 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: 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, Peter Zijlstra,
	Ingo Molnar, Jonathan Corbet
In-Reply-To: <CAK8P3a1E54K2pxw-9J5NazF2xNFNvOY1JsGr5C727zAxaz0K5g@mail.gmail.com>

On Wed, 2018-03-28 at 11:55 +0200, Arnd Bergmann wrote:
> > powerpc and ARM can't quite make them synchronous I think, but at least
> > they should have the same semantics as writel.
> 
> One thing that ARM does IIRC is that it only guarantees to order writel() within
> one device, and the memory mapped PCI I/O space window almost certainly
> counts as a separate device to the CPU.

That sounds bogus.

> In the absence of an enforced global synchronization during an I/O port
> access, that means writel() and outb() can be reordered before they arrive
> at a device in theory. Again, this rarely matters in practice, but I think it
> makes sense to document the less strict behavior here, given that we have
> common hardware that can't provide x86 compatible semantics.

Can't you put some kind of super heavy handed barrier in inX/outX ?
These things are never going to be performance sensitive anyway...

Cheers,
Ben.

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28  9:56 UTC (permalink / raw)
  To: Will Deacon, David Laight
  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: <20180328090907.GC28871@arm.com>

On Wed, 2018-03-28 at 10:09 +0100, Will Deacon wrote:
> On Wed, Mar 28, 2018 at 09:00:01AM +0000, David Laight wrote:
> > 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 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.
> > 
> > A long time ago there was a document from Intel that said that inb/outb weren'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.
> 
> Thank you, David! I'll write another patch fixing this up and hopefully
> we'll soon have one making writeX/readX much clearer.

Thanks for doing the grunt work Will ! :-)

Cheers,
Ben.

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28  9:56 UTC (permalink / raw)
  To: Will Deacon
  Cc: Linus Torvalds, Alexander Duyck, 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: <20180328090710.GB28871@arm.com>

On Wed, 2018-03-28 at 10:07 +0100, Will Deacon wrote:
> 
> For arm/arm64 we guarantee ordering for (1) but not for (2) -- you'd need to
> add an mb() to make it work.
> 
> Do both of these work on power? 

Yes. There's even another quirk, see further down ;-)

> If so, I guess I can make readl even more
> expensive :/ Feels a bit like the tail wagging the dog, though.

Maybe, but then readl is always horribly slow anyway so you may not
necessarily be losing that much.

> Another thing I just realised is that we restrict the barriers we use in
> readl/writel on arm64 so that they don't necessary apply to both loads and
> stores. To be specific:
> 
>    writel is ordered against prior writes to memory, but not reads

That could be tricky... You may end up with something that reads before
triggering a DMA and ends up with the post-DMA value ... ugh.

>    readl is ordered against subsequent reads of memory, but not writes (but
>    note that in example (1) above, the control dependency ensures that).
> 
> If necessary, I could move the barrier in our readl implementation to be
> before the read, then play the control-dependency + instruction-sync (ISB)
> trick that you do on power.

Yeah so that other trick I'm talking about is also used for timing
accuracy.

For example, let's say I have a device with a reset bit and the spec
says the reset bit needs to be set for at least 10us.

This is wrong:

	writel(1, RESET_REG);
	usleep(10);
	writel(0, RESET_REG);

Because of write posting, the first write might arrive to the device
right before the second one.

The typical "fix" is to turn that into:

	writel(1, RESET_REG);
	readl(RESET_REG); /* Flush posted writes */
	usleep(10);
	writel(0, RESET_REG);

*However* the issue here, at least on power, is that the CPU can issue
that readl but doesn't necessarily wait for it to complete (ie, the
data to return), before proceeding to the usleep. Now a usleep contains
a bunch of loads and stores and is probably fine, but a udelay which
just loops on the timebase may not be.

Thus we may still violate the timing requirement.

What we did inside readl, with the twi;isync sequence (which basically
means, trap on return value with "trap never" as a condition, followed
by isync that ensures all excpetion conditions are resolved), is force
the CPU to "consume" the data from the read before moving on.

This effectively makes readl fully synchronous (we would probably avoid
that if we were to implement a readl_relaxed).

Cheers,
Ben.

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-28  9:55 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: 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, Peter Zijlstra,
	Ingo Molnar, Jonathan Corbet
In-Reply-To: <1522230616.21446.1.camel@kernel.crashing.org>

On Wed, Mar 28, 2018 at 11:50 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2018-03-28 at 09:53 +0100, Will Deacon wrote:
>> 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?.
>
> I doubt it ... the Intel ancestry here would make me think they are
> completely ordered there too.
>
> powerpc and ARM can't quite make them synchronous I think, but at least
> they should have the same semantics as writel.

One thing that ARM does IIRC is that it only guarantees to order writel() within
one device, and the memory mapped PCI I/O space window almost certainly
counts as a separate device to the CPU.

In the absence of an enforced global synchronization during an I/O port
access, that means writel() and outb() can be reordered before they arrive
at a device in theory. Again, this rarely matters in practice, but I think it
makes sense to document the less strict behavior here, given that we have
common hardware that can't provide x86 compatible semantics.

       Arnd

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28  9:50 UTC (permalink / raw)
  To: Will Deacon
  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: <20180328085338.GA28871@arm.com>

On Wed, 2018-03-28 at 09:53 +0100, Will Deacon wrote:
> 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?.

I doubt it ... the Intel ancestry here would make me think they are
completely ordered there too.

powerpc and ARM can't quite make them synchronous I think, but at least
they should have the same semantics as writel.

Cheers,
Ben.

^ permalink raw reply

* [PATCH 5/5] powerpc: move a stray NMI IPI case under NMI_IPI ifdef
From: Nicholas Piggin @ 2018-03-28  9:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180328092819.27015-1-npiggin@gmail.com>

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/smp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 17c6abed3def..6185b7245e9e 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -193,7 +193,9 @@ const char *smp_ipi_name[] = {
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	[PPC_MSG_TICK_BROADCAST] = "ipi tick-broadcast",
 #endif
+#ifdef CONFIG_NMI_IPI
 	[PPC_MSG_NMI_IPI] = "nmi ipi",
+#endif
 };
 
 /* optional function to request ipi, for controllers with >= 4 ipis */
-- 
2.16.1

^ permalink raw reply related

* [PATCH 4/5] powerpc: move timer broadcast code under GENERIC_CLOCKEVENTS_BROADCAST ifdef
From: Nicholas Piggin @ 2018-03-28  9:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180328092819.27015-1-npiggin@gmail.com>

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/smp.c  | 8 ++++++++
 arch/powerpc/kernel/time.c | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index de5ac63b4ad6..17c6abed3def 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -155,11 +155,13 @@ static irqreturn_t reschedule_action(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 static irqreturn_t tick_broadcast_ipi_action(int irq, void *data)
 {
 	timer_broadcast_interrupt();
 	return IRQ_HANDLED;
 }
+#endif
 
 #ifdef CONFIG_NMI_IPI
 static irqreturn_t nmi_ipi_action(int irq, void *data)
@@ -172,7 +174,9 @@ static irqreturn_t nmi_ipi_action(int irq, void *data)
 static irq_handler_t smp_ipi_action[] = {
 	[PPC_MSG_CALL_FUNCTION] =  call_function_action,
 	[PPC_MSG_RESCHEDULE] = reschedule_action,
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	[PPC_MSG_TICK_BROADCAST] = tick_broadcast_ipi_action,
+#endif
 #ifdef CONFIG_NMI_IPI
 	[PPC_MSG_NMI_IPI] = nmi_ipi_action,
 #endif
@@ -186,7 +190,9 @@ static irq_handler_t smp_ipi_action[] = {
 const char *smp_ipi_name[] = {
 	[PPC_MSG_CALL_FUNCTION] =  "ipi call function",
 	[PPC_MSG_RESCHEDULE] = "ipi reschedule",
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	[PPC_MSG_TICK_BROADCAST] = "ipi tick-broadcast",
+#endif
 	[PPC_MSG_NMI_IPI] = "nmi ipi",
 };
 
@@ -277,8 +283,10 @@ irqreturn_t smp_ipi_demux_relaxed(void)
 			generic_smp_call_function_interrupt();
 		if (all & IPI_MESSAGE(PPC_MSG_RESCHEDULE))
 			scheduler_ipi();
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 		if (all & IPI_MESSAGE(PPC_MSG_TICK_BROADCAST))
 			timer_broadcast_interrupt();
+#endif
 #ifdef CONFIG_NMI_IPI
 		if (all & IPI_MESSAGE(PPC_MSG_NMI_IPI))
 			nmi_ipi_action(0, NULL);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 199f330e8f01..4c02b6725844 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -604,6 +604,7 @@ void timer_interrupt(struct pt_regs *regs)
 }
 EXPORT_SYMBOL(timer_interrupt);
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 void timer_broadcast_interrupt(void)
 {
 	u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
@@ -615,6 +616,7 @@ void timer_broadcast_interrupt(void)
 	__this_cpu_inc(irq_stat.timer_irqs_event);
 	trace_timer_interrupt_exit(regs);
 }
+#endif
 
 /*
  * Hypervisor decrementer interrupts shouldn't occur but are sometimes
-- 
2.16.1

^ permalink raw reply related

* [PATCH 3/5] powerpc: clockevents broadcast receiver use tick_receive_broadcast
From: Nicholas Piggin @ 2018-03-28  9:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin, Srivatsa S . Bhat, Preeti U Murthy
In-Reply-To: <20180328092819.27015-1-npiggin@gmail.com>

The broadcast tick recipient can call tick_receive_broadcast rather
than re-running the full timer interrupt.

It does not not have to check for the next event time, because the
sender already determined the timer has expired. It does not have to
test irq_work_pending, because that's a direct decrementer interrupt
and does not go through the clock events subsystem.

This results in no code size change, but both the decrementer and
broadcast path lengths are reduced.

Cc: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/hw_irq.h |  1 +
 arch/powerpc/include/asm/time.h   |  1 -
 arch/powerpc/kernel/smp.c         |  4 +-
 arch/powerpc/kernel/time.c        | 84 ++++++++++++++++++---------------------
 4 files changed, 42 insertions(+), 48 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index 855e17d158b1..dcdb95a13c47 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -55,6 +55,7 @@ extern void replay_system_reset(void);
 extern void __replay_interrupt(unsigned int vector);
 
 extern void timer_interrupt(struct pt_regs *);
+extern void timer_broadcast_interrupt(void);
 extern void performance_monitor_exception(struct pt_regs *regs);
 extern void WatchdogException(struct pt_regs *regs);
 extern void unknown_exception(struct pt_regs *regs);
diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index c965c79765c4..69b89f941252 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -28,7 +28,6 @@ extern struct clock_event_device decrementer_clockevent;
 
 struct rtc_time;
 extern void to_tm(int tim, struct rtc_time * tm);
-extern void tick_broadcast_ipi_handler(void);
 
 extern void generic_calibrate_decr(void);
 extern void hdec_interrupt(struct pt_regs *regs);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index bbe7634b3a43..de5ac63b4ad6 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -157,7 +157,7 @@ static irqreturn_t reschedule_action(int irq, void *data)
 
 static irqreturn_t tick_broadcast_ipi_action(int irq, void *data)
 {
-	tick_broadcast_ipi_handler();
+	timer_broadcast_interrupt();
 	return IRQ_HANDLED;
 }
 
@@ -278,7 +278,7 @@ irqreturn_t smp_ipi_demux_relaxed(void)
 		if (all & IPI_MESSAGE(PPC_MSG_RESCHEDULE))
 			scheduler_ipi();
 		if (all & IPI_MESSAGE(PPC_MSG_TICK_BROADCAST))
-			tick_broadcast_ipi_handler();
+			timer_broadcast_interrupt();
 #ifdef CONFIG_NMI_IPI
 		if (all & IPI_MESSAGE(PPC_MSG_NMI_IPI))
 			nmi_ipi_action(0, NULL);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 3e531ab4e836..199f330e8f01 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -535,47 +535,16 @@ void arch_irq_work_raise(void)
 
 #endif /* CONFIG_IRQ_WORK */
 
-static void __timer_interrupt(void)
-{
-	struct pt_regs *regs = get_irq_regs();
-	u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
-	struct clock_event_device *evt = this_cpu_ptr(&decrementers);
-	u64 now;
-
-	trace_timer_interrupt_entry(regs);
-
-	if (test_irq_work_pending()) {
-		clear_irq_work_pending();
-		irq_work_run();
-	}
-
-	now = get_tb_or_rtc();
-	if (now >= *next_tb) {
-		*next_tb = ~(u64)0;
-		if (evt->event_handler)
-			evt->event_handler(evt);
-		__this_cpu_inc(irq_stat.timer_irqs_event);
-	} else {
-		now = *next_tb - now;
-		if (now <= decrementer_max)
-			set_dec(now);
-		/* We may have raced with new irq work */
-		if (test_irq_work_pending())
-			set_dec(1);
-		__this_cpu_inc(irq_stat.timer_irqs_others);
-	}
-
-	trace_timer_interrupt_exit(regs);
-}
-
 /*
  * timer_interrupt - gets called when the decrementer overflows,
  * with interrupts disabled.
  */
-void timer_interrupt(struct pt_regs * regs)
+void timer_interrupt(struct pt_regs *regs)
 {
-	struct pt_regs *old_regs;
+	struct clock_event_device *evt = this_cpu_ptr(&decrementers);
 	u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
+	struct pt_regs *old_regs;
+	u64 now;
 
 	/* Ensure a positive value is written to the decrementer, or else
 	 * some CPUs will continue to take decrementer exceptions.
@@ -606,13 +575,47 @@ void timer_interrupt(struct pt_regs * regs)
 
 	old_regs = set_irq_regs(regs);
 	irq_enter();
+	trace_timer_interrupt_entry(regs);
+
+	if (test_irq_work_pending()) {
+		clear_irq_work_pending();
+		irq_work_run();
+	}
+
+	now = get_tb_or_rtc();
+	if (now >= *next_tb) {
+		*next_tb = ~(u64)0;
+		if (evt->event_handler)
+			evt->event_handler(evt);
+		__this_cpu_inc(irq_stat.timer_irqs_event);
+	} else {
+		now = *next_tb - now;
+		if (now <= decrementer_max)
+			set_dec(now);
+		/* We may have raced with new irq work */
+		if (test_irq_work_pending())
+			set_dec(1);
+		__this_cpu_inc(irq_stat.timer_irqs_others);
+	}
 
-	__timer_interrupt();
+	trace_timer_interrupt_exit(regs);
 	irq_exit();
 	set_irq_regs(old_regs);
 }
 EXPORT_SYMBOL(timer_interrupt);
 
+void timer_broadcast_interrupt(void)
+{
+	u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
+	struct pt_regs *regs = get_irq_regs();
+
+	trace_timer_interrupt_entry(regs);
+	*next_tb = ~(u64)0;
+	tick_receive_broadcast();
+	__this_cpu_inc(irq_stat.timer_irqs_event);
+	trace_timer_interrupt_exit(regs);
+}
+
 /*
  * Hypervisor decrementer interrupts shouldn't occur but are sometimes
  * left pending on exit from a KVM guest.  We don't need to do anything
@@ -960,15 +963,6 @@ static int decrementer_shutdown(struct clock_event_device *dev)
 	return 0;
 }
 
-/* Interrupt handler for the timer broadcast IPI */
-void tick_broadcast_ipi_handler(void)
-{
-	u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
-
-	*next_tb = get_tb_or_rtc();
-	__timer_interrupt();
-}
-
 static void register_decrementer_clockevent(int cpu)
 {
 	struct clock_event_device *dec = &per_cpu(decrementers, cpu);
-- 
2.16.1

^ permalink raw reply related

* [PATCH 2/5] powerpc/pseries: lparcfg calculate PURR on demand
From: Nicholas Piggin @ 2018-03-28  9:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180328092819.27015-1-npiggin@gmail.com>

For SPLPAR, lparcfg provides a sum of PURR registers for all CPUs.
Currently this is done by reading PURR in context switch and timer
interrupt, and storing that into a per-CPU variable. These are
summed to provide the value.

This does not work with all timer schemes (e.g., NO_HZ_FULL), and it
is sub-optimal for performance because it reads the PURR register
on every context switch, although that's been difficult to distinguish
from noise in the contxt_switch microbenchmark.

This patch implements the sum by calling a function on each CPU, to
read and add PURR values of each CPU.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/time.h          |  8 --------
 arch/powerpc/kernel/process.c            | 14 --------------
 arch/powerpc/kernel/time.c               |  8 --------
 arch/powerpc/platforms/pseries/lparcfg.c | 18 ++++++++++--------
 4 files changed, 10 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index db546c034905..c965c79765c4 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -196,14 +196,6 @@ extern u64 mulhdu(u64, u64);
 extern void div128_by_32(u64 dividend_high, u64 dividend_low,
 			 unsigned divisor, struct div_result *dr);
 
-/* Used to store Processor Utilization register (purr) values */
-
-struct cpu_usage {
-        u64 current_tb;  /* Holds the current purr register values */
-};
-
-DECLARE_PER_CPU(struct cpu_usage, cpu_usage_array);
-
 extern void secondary_cpu_time_init(void);
 extern void __init time_init(void);
 
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 9626b991da52..48cb033ecd33 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -827,10 +827,6 @@ void set_breakpoint(struct arch_hw_breakpoint *brk)
 	preempt_enable();
 }
 
-#ifdef CONFIG_PPC64
-DEFINE_PER_CPU(struct cpu_usage, cpu_usage_array);
-#endif
-
 static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
 			      struct arch_hw_breakpoint *b)
 {
@@ -1163,16 +1159,6 @@ struct task_struct *__switch_to(struct task_struct *prev,
 
 	WARN_ON(!irqs_disabled());
 
-#ifdef CONFIG_PPC64
-	/*
-	 * Collect processor utilization data per process
-	 */
-	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
-		struct cpu_usage *cu = this_cpu_ptr(&cpu_usage_array);
-		cu->current_tb = mfspr(SPRN_PURR);
-	}
-#endif /* CONFIG_PPC64 */
-
 #ifdef CONFIG_PPC_BOOK3S_64
 	batch = this_cpu_ptr(&ppc64_tlb_batch);
 	if (batch->active) {
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index f7d96a68ecaa..3e531ab4e836 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -565,14 +565,6 @@ static void __timer_interrupt(void)
 		__this_cpu_inc(irq_stat.timer_irqs_others);
 	}
 
-#ifdef CONFIG_PPC64
-	/* collect purr register values often, for accurate calculations */
-	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
-		struct cpu_usage *cu = this_cpu_ptr(&cpu_usage_array);
-		cu->current_tb = mfspr(SPRN_PURR);
-	}
-#endif
-
 	trace_timer_interrupt_exit(regs);
 }
 
diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
index c508c938dc71..7c872dc01bdb 100644
--- a/arch/powerpc/platforms/pseries/lparcfg.c
+++ b/arch/powerpc/platforms/pseries/lparcfg.c
@@ -52,18 +52,20 @@
  * Track sum of all purrs across all processors. This is used to further
  * calculate usage values by different applications
  */
+static void cpu_get_purr(void *arg)
+{
+	atomic64_t *sum = arg;
+
+	atomic64_add(mfspr(SPRN_PURR), sum);
+}
+
 static unsigned long get_purr(void)
 {
-	unsigned long sum_purr = 0;
-	int cpu;
+	atomic64_t purr = ATOMIC64_INIT(0);
 
-	for_each_possible_cpu(cpu) {
-		struct cpu_usage *cu;
+	on_each_cpu(cpu_get_purr, &purr, 1);
 
-		cu = &per_cpu(cpu_usage_array, cpu);
-		sum_purr += cu->current_tb;
-	}
-	return sum_purr;
+	return atomic64_read(&purr);
 }
 
 /*
-- 
2.16.1

^ permalink raw reply related

* [PATCH 1/5] powerpc/64: remove start_tb and accum_tb from thread_struct
From: Nicholas Piggin @ 2018-03-28  9:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180328092819.27015-1-npiggin@gmail.com>

These fields are only written to.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/processor.h | 4 ----
 arch/powerpc/kernel/process.c        | 6 +-----
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 01299cdc9806..67eedcc6d803 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -251,10 +251,6 @@ struct thread_struct {
 	struct thread_fp_state	*fp_save_area;
 	int		fpexc_mode;	/* floating-point exception mode */
 	unsigned int	align_ctl;	/* alignment handling control */
-#ifdef CONFIG_PPC64
-	unsigned long	start_tb;	/* Start purr when proc switched in */
-	unsigned long	accum_tb;	/* Total accumulated purr for process */
-#endif
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	struct perf_event *ptrace_bps[HBP_NUM];
 	/*
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index ec4f363ebb89..9626b991da52 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1169,11 +1169,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
 	 */
 	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
 		struct cpu_usage *cu = this_cpu_ptr(&cpu_usage_array);
-		long unsigned start_tb, current_tb;
-		start_tb = old_thread->start_tb;
-		cu->current_tb = current_tb = mfspr(SPRN_PURR);
-		old_thread->accum_tb += (current_tb - start_tb);
-		new_thread->start_tb = current_tb;
+		cu->current_tb = mfspr(SPRN_PURR);
 	}
 #endif /* CONFIG_PPC64 */
 
-- 
2.16.1

^ permalink raw reply related

* [PATCH 0/5] remove PURR read from context switch and timer
From: Nicholas Piggin @ 2018-03-28  9:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Nicholas Piggin

And some associated cleanups to the timer code as a result

Nicholas Piggin (5):
  powerpc/64: remove start_tb and accum_tb from thread_struct
  powerpc/pseries: lparcfg calculate PURR on demand
  powerpc: clockevents broadcast receiver use
    tick_receive_broadcast
  powerpc: move timer broadcast code under GENERIC_CLOCKEVENTS_BROADCAST
    ifdef
  powerpc: move a stray NMI IPI case under NMI_IPI ifdef

 arch/powerpc/include/asm/hw_irq.h        |  1 +
 arch/powerpc/include/asm/processor.h     |  4 --
 arch/powerpc/include/asm/time.h          |  9 ---
 arch/powerpc/kernel/process.c            | 18 ------
 arch/powerpc/kernel/smp.c                | 14 ++++-
 arch/powerpc/kernel/time.c               | 94 ++++++++++++++------------------
 arch/powerpc/platforms/pseries/lparcfg.c | 18 +++---
 7 files changed, 64 insertions(+), 94 deletions(-)

-- 
2.16.1

^ permalink raw reply

* Re: [PATCH v2 10/10] powerpc/64s: Wire up cpu_show_spectre_v2()
From: Diana Madalina Craciun @ 2018-03-28  9:15 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev@ozlabs.org; +Cc: mauricfo@linux.vnet.ibm.com
In-Reply-To: <20180327120153.31612-10-mpe@ellerman.id.au>

Why is the speculation barrier specific to Spectre v2? Can't the barrier=0A=
be used as a mitigation for Spectre v1 as well?=0A=
=0A=
Regards,=0A=
Diana=0A=
=0A=
On 3/27/2018 3:32 PM, Michael Ellerman wrote:=0A=
> Add a definition for cpu_show_spectre_v2() to override the generic=0A=
> version. This has several permuations, though in practice some may not=0A=
> occur we cater for any combination.=0A=
>=0A=
> The most verbose is:=0A=
>=0A=
>   Mitigation: Indirect branch serialisation (kernel only), Indirect=0A=
>   branch cache disabled, ori31 speculation barrier enabled=0A=
>=0A=
> We don't treat the ori31 speculation barrier as a mitigation on its=0A=
> own, because it has to be *used* by code in order to be a mitigation=0A=
> and we don't know if userspace is doing that. So if that's all we see=0A=
> we say:=0A=
>=0A=
>   Vulnerable, ori31 speculation barrier enabled=0A=
>=0A=
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>=0A=
> ---=0A=
>  arch/powerpc/kernel/security.c | 33 +++++++++++++++++++++++++++++++++=0A=
>  1 file changed, 33 insertions(+)=0A=
>=0A=
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/securit=
y.c=0A=
> index 0eace3cac818..2cee3dcd231b 100644=0A=
> --- a/arch/powerpc/kernel/security.c=0A=
> +++ b/arch/powerpc/kernel/security.c=0A=
> @@ -58,3 +58,36 @@ ssize_t cpu_show_spectre_v1(struct device *dev, struct=
 device_attribute *attr, c=0A=
>  =0A=
>  	return sprintf(buf, "Vulnerable\n");=0A=
>  }=0A=
> +=0A=
> +ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute =
*attr, char *buf)=0A=
> +{=0A=
> +	bool bcs, ccd, ori;=0A=
> +	struct seq_buf s;=0A=
> +=0A=
> +	seq_buf_init(&s, buf, PAGE_SIZE - 1);=0A=
> +=0A=
> +	bcs =3D security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);=0A=
> +	ccd =3D security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);=0A=
> +	ori =3D security_ftr_enabled(SEC_FTR_SPEC_BAR_ORI31);=0A=
> +=0A=
> +	if (bcs || ccd) {=0A=
> +		seq_buf_printf(&s, "Mitigation: ");=0A=
> +=0A=
> +		if (bcs)=0A=
> +			seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");=0A=
> +=0A=
> +		if (bcs && ccd)=0A=
> +			seq_buf_printf(&s, ", ");=0A=
> +=0A=
> +		if (ccd)=0A=
> +			seq_buf_printf(&s, "Indirect branch cache disabled");=0A=
> +	} else=0A=
> +		seq_buf_printf(&s, "Vulnerable");=0A=
> +=0A=
> +	if (ori)=0A=
> +		seq_buf_printf(&s, ", ori31 speculation barrier enabled");=0A=
> +=0A=
> +	seq_buf_printf(&s, "\n");=0A=
> +=0A=
> +	return s.len;=0A=
> +}=0A=
=0A=
=0A=

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-28  9:09 UTC (permalink / raw)
  To: David Laight
  Cc: Benjamin Herrenschmidt, 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: <5d3973edea93473197505f2e42cd3107@AcuMS.aculab.com>

On Wed, Mar 28, 2018 at 09:00:01AM +0000, David Laight wrote:
> 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 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.
> 
> A long time ago there was a document from Intel that said that inb/outb weren'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.

Thank you, David! I'll write another patch fixing this up and hopefully
we'll soon have one making writeX/readX much clearer.

Will

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Will Deacon @ 2018-03-28  9:07 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Linus Torvalds, Alexander Duyck, 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>

On Wed, Mar 28, 2018 at 05:42:56PM +1100, Benjamin Herrenschmidt wrote:
> 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.

Good. So I think this confirms our understanding so far.

> 
> > > 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 ?

So there are two cases to consider:

1.
	if (readl(DEVICE_DMA_STATUS) == DMA_DONE)
		mydata = *dma_bufp;



2.
	*dma_bufp = 42;
	readl(DEVICE_DMA_KICK_ON_READ);


For arm/arm64 we guarantee ordering for (1) but not for (2) -- you'd need to
add an mb() to make it work.

Do both of these work on power? If so, I guess I can make readl even more
expensive :/ Feels a bit like the tail wagging the dog, though.

Another thing I just realised is that we restrict the barriers we use in
readl/writel on arm64 so that they don't necessary apply to both loads and
stores. To be specific:

   writel is ordered against prior writes to memory, but not reads

   readl is ordered against subsequent reads of memory, but not writes (but
   note that in example (1) above, the control dependency ensures that).

If necessary, I could move the barrier in our readl implementation to be
before the read, then play the control-dependency + instruction-sync (ISB)
trick that you do on power.

Will

^ permalink raw reply

* 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


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox