Benjamin Herrenschmidt <b...@kernel.crashing.org> writes:
> On Mon, 2017-08-28 at 11:55 +0530, Aneesh Kumar K.V wrote:
>> We need to add memory barrier so that the page table walk doesn't happen
>> before the cpumask is set and made visible to the other cpus. We need
>> to use a sync here instead of lwsync because lwsync is not sufficient for
>> store/load ordering.
>>
>> We also need to add an if (mm) check so that we do the right thing when
>> called
>> with a kernel context. For kernel context, we have mm = NULL. W.r.t kernel
>> address we can skip setting the mm cpumask.
>>
>> Fixes: 0f4bc0932e ("powerpc/mm/cxl: Add the fault handling cpu to mm
>> cpumask")
>> Cc: Andrew Donnellan <andrew.donnel...@au1.ibm.com>
>> Reported-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
>> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
>> ---
>> drivers/misc/cxl/fault.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
>> index ab507e4ed69b..caed2a523bee 100644
>> --- a/drivers/misc/cxl/fault.c
>> +++ b/drivers/misc/cxl/fault.c
>> @@ -141,9 +141,19 @@ int cxl_handle_mm_fault(struct mm_struct *mm, u64
>> dsisr, u64 dar)
>> /*
>> * Add the fault handling cpu to task mm cpumask so that we
>> * can do a safe lockless page table walk when inserting the
>> - * hash page table entry.
>> + * hash page table entry. This function get called with a
>> + * valid mm for all user space applications. Hence using
>> + * if (mm) check is sufficient here.
>> */
>> - cpumask_set_cpu(smp_processor_id(), mm_cpumask(mm));
>> + if (mm) {
>> + cpumask_set_cpu(smp_processor_id(), mm_cpumask(mm));
>
> First test if it's already set as this should be quite common and the
> cost of setting is a full atomic.
>
Something like below ?
diff --git a/drivers/misc/cxl/fault.c b/drivers/misc/cxl/fault.c
index caed2a523bee..ccf8568262e4 100644
--- a/drivers/misc/cxl/fault.c
+++ b/drivers/misc/cxl/fault.c
@@ -146,13 +146,13 @@ int cxl_handle_mm_fault(struct mm_struct *mm, u64 dsisr,
u64 dar)
* if (mm) check is sufficient here.
*/
if (mm) {
- cpumask_set_cpu(smp_processor_id(), mm_cpumask(mm));
- /*
- * We need to make sure we walk the table only after
- * we update the cpumask. The other side of the barrier is
- * explained * in serialize_against_pte_lookup()
- */
- smp_mb();
+ if (!cpumask_test_and_set_cpu(smp_processor_id(),
mm_cpumask(mm)))
+ /*
+ * We need to make sure we walk the table only after
+ * we update the cpumask. The other side of the barrier
+ * is explained in serialize_against_pte_lookup()
+ */
+ smp_mb();
}
if ((result = copro_handle_mm_fault(mm, dar, dsisr, &flt))) {
pr_devel("copro_handle_mm_fault failed: %#x\n", result);
