Tyrel Datwyler <tyr...@linux.ibm.com> writes:
> On 8/11/20 6:20 PM, 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.
>>
>> Call cond_resched() on every 20th element. 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 <nath...@linux.ibm.com>
>> ---
>> arch/powerpc/include/asm/drmem.h | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> 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.
>>
>> diff --git a/arch/powerpc/include/asm/drmem.h
>> b/arch/powerpc/include/asm/drmem.h
>> index 17ccc6474ab6..583277e30dd2 100644
>> --- a/arch/powerpc/include/asm/drmem.h
>> +++ b/arch/powerpc/include/asm/drmem.h
>> @@ -8,6 +8,9 @@
>> #ifndef _ASM_POWERPC_LMB_H
>> #define _ASM_POWERPC_LMB_H
>>
>> +#include <linux/bug.h>
>> +#include <linux/sched.h>
>> +
>> struct drmem_lmb {
>> u64 base_addr;
>> u32 drc_index;
>> @@ -26,8 +29,21 @@ struct drmem_lmb_info {
>>
>> extern struct drmem_lmb_info *drmem_info;
>>
>> +static inline struct drmem_lmb *drmem_lmb_next(struct drmem_lmb *lmb)
>> +{
>> + const unsigned int resched_interval = 20;
>> +
>> + BUG_ON(lmb < drmem_info->lmbs);
>> + BUG_ON(lmb >= drmem_info->lmbs + drmem_info->n_lmbs);
>
> I think BUG_ON is a pretty big no-no these days unless there is no other
> option.
It's complicated, but yes we would like to avoid adding them if we can.
In a case like this there is no other option, *if* the check has to be
in drmem_lmb_next().
But I don't think we really need to check that there.
If for some reason this was called with an *lmb pointing outside of the
lmbs array it would confuse the cond_resched() logic, but it's not worth
crashing the box for that IMHO.
cheers
