Re: [PATCH v2 11/11] powerpc/mm/book3s64/pgtable: Uses counting method to skip serializing

From: John Hubbard <jhubbard@nvidia.com>
To: Leonardo Bras <leonardo@linux.ibm.com>,
	<linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Richard Fontana <rfontana@redhat.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	YueHaibing <yuehaibing@huawei.com>,
	Nicholas Piggin <npiggin@gmail.com>,
	Mike Rapoport <rppt@linux.ibm.com>,
	Keith Busch <keith.busch@intel.com>,
	Jason Gunthorpe <jgg@ziepe.ca>, Paul Mackerras <paulus@samba.org>,
	Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>,
	Allison Randal <allison@lohutok.net>,
	"Mahesh Salgaonkar" <mahesh@linux.vnet.ibm.com>,
	Ganesh Goudar <ganeshgr@linux.ibm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ira Weiny <ira.weiny@intel.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Dan Williams" <dan.j.williams@intel.com>
Subject: Re: [PATCH v2 11/11] powerpc/mm/book3s64/pgtable: Uses counting method to skip serializing
Date: Mon, 23 Sep 2019 12:58:32 -0700	[thread overview]
Message-ID: <1568b3ef-cec9-bf47-edaa-c775c2f544fb@nvidia.com> (raw)
In-Reply-To: <dc9fad3577551d34ead36c0f7340a573086c0cab.camel@linux.ibm.com>

On 9/23/19 12:40 PM, Leonardo Bras wrote:
> On Mon, 2019-09-23 at 11:14 -0700, John Hubbard wrote:
>> On 9/23/19 10:25 AM, Leonardo Bras wrote:
>> [...]
>> That part is all fine, but there are no run-time memory barriers in the 
>> atomic_inc() and atomic_dec() additions, which means that this is not
>> safe, because memory operations on CPU 1 can be reordered. It's safe
>> as shown *if* there are memory barriers to keep the order as shown:
>>
>> CPU 0                            CPU 1
>> ------                         --------------
>>                                atomic_inc(val) (no run-time memory barrier!)
>> pmd_clear(pte)
>> if (val)
>>     run_on_all_cpus(): IPI
>>                                local_irq_disable() (also not a mem barrier)
>>
>>                                READ(pte)
>>                                if(pte)
>>                                   walk page tables
>>
>>                                local_irq_enable() (still not a barrier)
>>                                atomic_dec(val)
>>
>> free(pte)
>>
>> thanks,
> 
> This is serialize:
> 
> void serialize_against_pte_lookup(struct mm_struct *mm)
> {
> 	smp_mb();
> 	if (running_lockless_pgtbl_walk(mm))
> 		smp_call_function_many(mm_cpumask(mm), do_nothing,
> NULL, 1);
> }
> 
> That would mean:
> 
> CPU 0                            CPU 1
> ------                         --------------
>                                atomic_inc(val) 
> pmd_clear(pte)
> smp_mb()
> if (val)
>     run_on_all_cpus(): IPI
>                                local_irq_disable() 
> 
>                                READ(pte)
>                                if(pte)
>                                   walk page tables
> 
>                                local_irq_enable() (still not a barrier)
>                                atomic_dec(val)
> 
> By https://www.kernel.org/doc/Documentation/memory-barriers.txt :
> 'If you need all the CPUs to see a given store at the same time, use
> smp_mb().'
> 
> Is it not enough? 

Nope. CPU 1 memory accesses could be re-ordered, as I said above:

CPU 0                            CPU 1
------                         --------------
                               READ(pte) (re-ordered at run time)
                               atomic_inc(val) (no run-time memory barrier!)

pmd_clear(pte)
if (val)
    run_on_all_cpus(): IPI
                               local_irq_disable() (also not a mem barrier)

                               if(pte)
                                  walk page tables
...

> Do you suggest adding 'smp_mb()' after atomic_{inc,dec} ?
> 

Yes (approximately: I'd have to look closer to see which barrier call is really
required). Unless there is something else that is providing the barrier, which
is why I called this a pre-existing question: it seems like the interrupt
interlock in the current gup_fast() might not have what it needs.

In other words, if your code needs a barrier, then the pre-existing gup_fast()
code probably does, too.

thanks,
-- 
John Hubbard
NVIDIA