On Thu, 30 Nov 2017 04:10:34 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Nov 29, 2017 at 4:11 PM, <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > kmemleak_scan() will scan struct page for each node and it can be really
> > large and resulting in a soft lockup.
> ..
> > + if (!(pfn % (MAX_SCAN_SIZE / sizeof(*page))))
> > + cond_resched();
>
> I applied this already, but looking at it, doesn't the above cause a
> completely pointless divide to get the remainder?
>
> It seems like it's a pretty damn arbitrary scheduling point too.
>
> Why isn't it just something like
>
> if (!(pfn & 63))
> cond_resched();
>
> which is also entirely arbitrary, but at least generates ok code?
>
> I guess that depending on config options etc, 'sizeof(struct page)'
> could be just right to hide this all, but it still just seems silly
> and pointless to use some arbitrary and possibly expensive modulus
> operation.
>
Yup. In fact it's probably more efficient to just do a bare
cond_resched() on each pass through the loop.
I did this:
From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Subject: mm/kmemleak.c: make cond_resched() rate-limiting more efficient
bde5f6bc68db5 ("kmemleak: add scheduling point to kmemleak_scan()") tries
to rate-limit the frequency of cond_resched() calls, but does it in a way
which might incur an expensive division operation in the inner loop.
Simplify this.
Fixes: bde5f6bc68db5 ("kmemleak: add scheduling point to kmemleak_scan()")
Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Yisheng Xie <xieyisheng1@xxxxxxxxxx>
Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
mm/kmemleak.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN mm/kmemleak.c~a mm/kmemleak.c
--- a/mm/kmemleak.c~a
+++ a/mm/kmemleak.c
@@ -1523,7 +1523,7 @@ static void kmemleak_scan(void)
if (page_count(page) == 0)
continue;
scan_block(page, page + 1, NULL);
- if (!(pfn % (MAX_SCAN_SIZE / sizeof(*page))))
+ if (!(pfn & 63))
cond_resched();
}
}
_
--
To unsubscribe from this list: send the line "unsubscribe mm-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
