[PATCH] powerpc/mm/cxl: Add barrier when setting mm cpumask

Subsystems: char and misc drivers, the rest

STALE3282d

5 messages, 3 authors, 2017-08-28 · open the first message on its own page

[PATCH] powerpc/mm/cxl: Add barrier when setting mm cpumask

From: Aneesh Kumar K.V <hidden>
Date: 2017-08-28 06:26:15

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 <redacted>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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));
+		/*
+		 * 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);
 		return result;
-- 
2.13.5

Re: [PATCH] powerpc/mm/cxl: Add barrier when setting mm cpumask

From: Andrew Donnellan <hidden>
Date: 2017-08-28 06:30:19

On 28/08/17 16:25, 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 <redacted>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
Acked-by: Andrew Donnellan <redacted>
quoted hunk
---
  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));
+		/*
+		 * 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);
  		return result;
-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH] powerpc/mm/cxl: Add barrier when setting mm cpumask

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2017-08-28 06:40:24

On Mon, 2017-08-28 at 11:55 +0530, Aneesh Kumar K.V wrote:
quoted hunk
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 <redacted>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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.
+		/*
+		 * 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);
 		return result;

Re: [PATCH] powerpc/mm/cxl: Add barrier when setting mm cpumask

From: Aneesh Kumar K.V <hidden>
Date: 2017-08-28 07:54:08

Benjamin Herrenschmidt [off-list ref] writes:
On Mon, 2017-08-28 at 11:55 +0530, Aneesh Kumar K.V wrote:
quoted
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 <redacted>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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);

Re: [PATCH] powerpc/mm/cxl: Add barrier when setting mm cpumask

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2017-08-28 07:59:46

On Mon, 2017-08-28 at 13:23 +0530, Aneesh Kumar K.V wrote:
Benjamin Herrenschmidt [off-list ref] writes:
quoted
On Mon, 2017-08-28 at 11:55 +0530, Aneesh Kumar K.V wrote:
quoted
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 <redacted>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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 ?
No, don't use cpumask_test_and_set_cpu, that has extra barriers you
don't want.

Do an

	if (!cpumask_test(...)) {
		cpumask_set(...);
		smp_mb();
	}

Like we do in context switch.
quoted hunk
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);
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help