lkml.org 
[lkml]   [2017]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker.
From
Date
On 02/10/2017 04:02 PM, Dmitry Vyukov wrote:
> On Fri, Feb 10, 2017 at 1:15 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
>>> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
>>>> This is the right thing to do per se, but I am concerned that now
>>>> people will just suffers from slow boot (it can take literally
>>>> minutes) and will not realize the root cause nor that it's fixable
>>>> (e.g. with rodata=n) and will probably just blame KASAN for slowness.
>>>>
>>>> Could we default this rodata check to n under KASAN? Or at least print
>>>> some explanatory warning message before doing marking rodata (it
>>>> should be printed right before "hang", so if you stare at it for a
>>>> minute during each boot you realize that it may be related)? Or
>>>> something along these lines. FWIW in my builds I just always disable
>>>> the check.
>>>
>>> That certainly makes sense and we emit such warnings in other places
>>> already (lockdep, trace_printk ...)
>>>
>>
>> Agreed, but perhaps it would be better to make this code faster for KASAN=y?
>> The main problem here is that we have many pgd entries containing kasan_zero_pud values
>> and ptdump walker checks kasan_zero_pud many times.
>> Instead, we could check it only once and skip further kasan_zero_pud's.
>>
>> I can't say I like this hack very much, but it wins me almost 20 seconds of boot time.
>> Any objections?
>
>
> Now I remember that we already discussed it in this thread:
> https://lkml.org/lkml/2016/11/8/775
>
> Andrey, you proposed:
>
> "I didn't look at any code, but we probably could can remember last
> visited pgd and skip next pgd if it's the same as previous."
>
> Do you still think it's a good idea?

Ah, indeed. It will do roughly the same but with less of code churn, see bellow.

> Walking the same pgd multiple times does not make sense (right?). And
> it could probably speedup non-kasan builds to some degree in some
> contexts. And the code will be free of additional ifdefs.
>

We could make it without ifdefs but this would be useless for KASAN=n
as page table entries normally unique. So I'm thinking to add #ifdef
at least for documentation purposes.



diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index 8aa6bea..1599a5c 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -373,6 +373,11 @@ static inline bool is_hypervisor_range(int idx)
#endif
}

+static bool pgd_already_checked(pgd_t *prev_pgd, pgd_t *pgd, bool checkwx)
+{
+ return checkwx && prev_pgd && (pgd_val(*prev_pgd) == pgd_val(*pgd));
+}
+
static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
bool checkwx)
{
@@ -381,6 +386,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
#else
pgd_t *start = swapper_pg_dir;
#endif
+ pgd_t *prev_pgd = NULL;
pgprotval_t prot;
int i;
struct pg_state st = {};
@@ -396,7 +402,8 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,

for (i = 0; i < PTRS_PER_PGD; i++) {
st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
- if (!pgd_none(*start) && !is_hypervisor_range(i)) {
+ if (!pgd_none(*start) && !is_hypervisor_range(i) &&
+ !pgd_already_checked(prev_pgd, start, checkwx)) {
if (pgd_large(*start) || !pgd_present(*start)) {
prot = pgd_flags(*start);
note_page(m, &st, __pgprot(prot), 1);
@@ -408,6 +415,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
note_page(m, &st, __pgprot(0), 1);

cond_resched();
+ prev_pgd = start;
start++;
}
\
 
 \ /
  Last update: 2017-02-10 14:58    [W:0.045 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site