[PATCH v3] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal

[PATCH v3] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal

[Nathan Lynch]

The drmem lmb list can have hundreds of thousands of entries, and
unfortunately lookups take the form of linear searches. As long as
this is the case, traversals have the potential to monopolize the CPU
and provoke lockup reports, workqueue stalls, and the like unless
they explicitly yield.

Rather than placing cond_resched() calls within various
for_each_drmem_lmb() loop blocks in the code, put it in the iteration
expression of the loop macro itself so users can't omit it.

Introduce a drmem_lmb_next() iteration helper function which calls
cond_resched() at a regular interval during array traversal. Each
iteration of the loop in DLPAR code paths can involve around ten RTAS
calls which can each take up to 250us, so this ensures the check is
performed at worst every few milliseconds.

Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---

Notes:
    Changes since v2:
    * Make drmem_lmb_next() more general.
    * Adjust reschedule interval for better code generation.
    * Add commentary to drmem_lmb_next() to explain the cond_resched()
      call.
    * Remove bounds assertions.
    
    Changes since v1:
    * Add bounds assertions in drmem_lmb_next().
    * Call cond_resched() in the iterator on only every 20th element
      instead of on every iteration, to reduce overhead in tight loops.

 arch/powerpc/include/asm/drmem.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
index 17ccc6474ab6..6fb928605ed1 100644
--- a/arch/powerpc/include/asm/drmem.h
+++ b/arch/powerpc/include/asm/drmem.h
@@ -8,6 +8,8 @@
 #ifndef _ASM_POWERPC_LMB_H
 #define _ASM_POWERPC_LMB_H
 
+#include <linux/sched.h>
+
 struct drmem_lmb {
 	u64     base_addr;
 	u32     drc_index;
@@ -26,8 +28,22 @@ struct drmem_lmb_info {
 
 extern struct drmem_lmb_info *drmem_info;
 
+static inline struct drmem_lmb *drmem_lmb_next(struct drmem_lmb *lmb,
+					       const struct drmem_lmb *start)
+{
+	/*
+	 * DLPAR code paths can take several milliseconds per element
+	 * when interacting with firmware. Ensure that we don't
+	 * unfairly monopolize the CPU.
+	 */
+	if (((++lmb - start) % 16) == 0)
+		cond_resched();
+
+	return lmb;
+}
+
 #define for_each_drmem_lmb_in_range(lmb, start, end)		\
-	for ((lmb) = (start); (lmb) < (end); (lmb)++)
+	for ((lmb) = (start); (lmb) < (end); lmb = drmem_lmb_next(lmb, start))
 
 #define for_each_drmem_lmb(lmb)					\
 	for_each_drmem_lmb_in_range((lmb),			\
-- 
2.25.4

Re: [PATCH v3] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal

[Christophe Leroy]

Le 13/08/2020 à 17:11, Nathan Lynch a écrit :
> The drmem lmb list can have hundreds of thousands of entries, and
> unfortunately lookups take the form of linear searches. As long as
> this is the case, traversals have the potential to monopolize the CPU
> and provoke lockup reports, workqueue stalls, and the like unless
> they explicitly yield.
> 
> Rather than placing cond_resched() calls within various
> for_each_drmem_lmb() loop blocks in the code, put it in the iteration
> expression of the loop macro itself so users can't omit it.
> 
> Introduce a drmem_lmb_next() iteration helper function which calls
> cond_resched() at a regular interval during array traversal. Each
> iteration of the loop in DLPAR code paths can involve around ten RTAS
> calls which can each take up to 250us, so this ensures the check is
> performed at worst every few milliseconds.
> 
> Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Looks a lot better to me than v2.

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
> 
> Notes:
>      Changes since v2:
>      * Make drmem_lmb_next() more general.
>      * Adjust reschedule interval for better code generation.
>      * Add commentary to drmem_lmb_next() to explain the cond_resched()
>        call.
>      * Remove bounds assertions.
>      
>      Changes since v1:
>      * Add bounds assertions in drmem_lmb_next().
>      * Call cond_resched() in the iterator on only every 20th element
>        instead of on every iteration, to reduce overhead in tight loops.
> 
>   arch/powerpc/include/asm/drmem.h | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index 17ccc6474ab6..6fb928605ed1 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -8,6 +8,8 @@
>   #ifndef _ASM_POWERPC_LMB_H
>   #define _ASM_POWERPC_LMB_H
>   
> +#include <linux/sched.h>
> +
>   struct drmem_lmb {
>   	u64     base_addr;
>   	u32     drc_index;
> @@ -26,8 +28,22 @@ struct drmem_lmb_info {
>   
>   extern struct drmem_lmb_info *drmem_info;
>   
> +static inline struct drmem_lmb *drmem_lmb_next(struct drmem_lmb *lmb,
> +					       const struct drmem_lmb *start)
> +{
> +	/*
> +	 * DLPAR code paths can take several milliseconds per element
> +	 * when interacting with firmware. Ensure that we don't
> +	 * unfairly monopolize the CPU.
> +	 */
> +	if (((++lmb - start) % 16) == 0)
> +		cond_resched();
> +
> +	return lmb;
> +}
> +
>   #define for_each_drmem_lmb_in_range(lmb, start, end)		\
> -	for ((lmb) = (start); (lmb) < (end); (lmb)++)
> +	for ((lmb) = (start); (lmb) < (end); lmb = drmem_lmb_next(lmb, start))
>   
>   #define for_each_drmem_lmb(lmb)					\
>   	for_each_drmem_lmb_in_range((lmb),			\
> 

Re: [PATCH v3] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal

[Michael Ellerman]

On Thu, 13 Aug 2020 10:11:31 -0500, Nathan Lynch wrote:
> The drmem lmb list can have hundreds of thousands of entries, and
> unfortunately lookups take the form of linear searches. As long as
> this is the case, traversals have the potential to monopolize the CPU
> and provoke lockup reports, workqueue stalls, and the like unless
> they explicitly yield.
> 
> Rather than placing cond_resched() calls within various
> for_each_drmem_lmb() loop blocks in the code, put it in the iteration
> expression of the loop macro itself so users can't omit it.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal
      https://git.kernel.org/powerpc/c/9d6792ffe140240ae54c881cc4183f9acc24b4df

cheers

Re: [PATCH v3] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal

[Michael Ellerman]

On Thu, 13 Aug 2020 10:11:31 -0500, Nathan Lynch wrote:
> The drmem lmb list can have hundreds of thousands of entries, and
> unfortunately lookups take the form of linear searches. As long as
> this is the case, traversals have the potential to monopolize the CPU
> and provoke lockup reports, workqueue stalls, and the like unless
> they explicitly yield.
> 
> Rather than placing cond_resched() calls within various
> for_each_drmem_lmb() loop blocks in the code, put it in the iteration
> expression of the loop macro itself so users can't omit it.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal
      https://git.kernel.org/powerpc/c/9d6792ffe140240ae54c881cc4183f9acc24b4df

cheers