LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Toshi Kani <toshi.kani@hp.com> To: Ingo Molnar <mingo@kernel.org> Cc: akpm@linux-foundation.org, hpa@zytor.com, tglx@linutronix.de, mingo@redhat.com, arnd@arndb.de, linux-mm@kvack.org, x86@kernel.org, linux-kernel@vger.kernel.org, dave.hansen@intel.com, Elliott@hp.com, pebolle@tiscali.nl Subject: Re: [PATCH 3/3] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping Date: Wed, 11 Mar 2015 10:52:08 -0600 [thread overview] Message-ID: <1426092728.17007.380.camel@misato.fc.hp.com> (raw) In-Reply-To: <20150311070216.GD29788@gmail.com> On Wed, 2015-03-11 at 08:02 +0100, Ingo Molnar wrote: > * Toshi Kani <toshi.kani@hp.com> wrote: > > > This patch adds an additional argument, *uniform, to > > s/*uniform/'uniform' Done. > > mtrr_type_lookup(), which returns 1 when a given range is > > either fully covered by a single MTRR entry or not covered > > at all. > > s/or not covered/or is not covered Done. > > pud_set_huge() and pmd_set_huge() are changed to check the > > new uniform flag to see if it is safe to create a huge page > > s/uniform/'uniform' Done. > > mapping to the range. This allows them to create a huge page > > mapping to a range covered by a single MTRR entry of any > > memory type. It also detects an unoptimal request properly. > > s/unoptimal/non-optimal Done. > or nonoptimal > > Also, some description in the changelog about what a 'non-optimal' > request is would be most userful. > > > They continue to check with the WB type since the WB type has > > no effect even if a request spans to multiple MTRR entries. > > s/spans to/spans Done. > > -static inline u8 mtrr_type_lookup(u64 addr, u64 end) > > +static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform) > > { > > /* > > * Return no-MTRRs: > > */ > > + *uniform = 1; > > return 0xff; > > } > > #define mtrr_save_fixed_ranges(arg) do {} while (0) > > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c > > index cdb955f..aef238c 100644 > > --- a/arch/x86/kernel/cpu/mtrr/generic.c > > +++ b/arch/x86/kernel/cpu/mtrr/generic.c > > @@ -108,14 +108,19 @@ static int check_type_overlap(u8 *prev, u8 *curr) > > * *repeat == 1 implies [start:end] spanned across MTRR range and type returned > > * corresponds only to [start:*partial_end]. > > * Caller has to lookup again for [*partial_end:end]. > > + * *uniform == 1 The requested range is either fully covered by a single MTRR > > + * entry or not covered at all. > > */ > > So I think a better approach would be to count the number of separate > MTRR caching types a range is covered by, instead of this hard to > quality 'uniform' flag. > > I.e. a 'nr_mtrr_types' count. > > If for example a range partially intersects with an MTRR, then that > count would be 2: the MTRR, and the outside (default cache policy) > type. > > ( Note that with this approach is not only easy to understand and easy > to review, but could also be refined in the future, to count the > number of _incompatible_ caching types present within a range. ) I agree that using a count is more flexible. However, there are some issues below. - MTRRs have both fixed and variable ranges. The first 1MB is covered with 11 fixed-range registers with different sizes of granularity, 512KB, 128KB, and 32KB. __mtrr_type_lookup() checks the memory type of the range at 'start', but does not check if a requested range spans multiple memory types. This first 1MB can be handled as 'uniform = 0' since processors do not create a huge page map in this 1MB range. However, setting a correct value to 'nr_mtrr_types' requires a major overhaul in this code. - mtrr_type_lookup() returns without walking through all MTRR entries when check_type_overlap() returns 1, i.e. the overlap made the resulted memory type UC. In this case, the code cannot set a correct value to 'nr_mtrr_type'. Since MTRRs are legacy, esp. the fixed range, there is not much benefit from enhancing the functionality of mtrr_type_lookup() unless there is an issue with the current platforms. For this patch, we only need to know whether the mapping count is 1 or >1. So, I think using 'uniform' makes sense for simplicity. > > -static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat) > > +static u8 __mtrr_type_lookup(u64 start, u64 end, > > + u64 *partial_end, int *repeat, u8 *uniform) > > { > > int i; > > u64 base, mask; > > u8 prev_match, curr_match; > > > > *repeat = 0; > > + *uniform = 1; > > + > > if (!mtrr_state_set) > > return 0xFF; > > > > @@ -128,6 +133,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat) > > /* Look in fixed ranges. Just return the type as per start */ > > if (mtrr_state.have_fixed && (start < 0x100000)) { > > int idx; > > + *uniform = 0; > > So this function scares me, because the code is clearly crap: > > if (mtrr_state.have_fixed && (start < 0x100000)) { > ... > } else if (start < 0x1000000) { > ... > > How can that 'else if' branch ever not be true? This 'else if' is always true. So, it can be simply 'else' without any condition. > Did it perhaps want to be the other way around: > > if (mtrr_state.have_fixed && (start < 0x1000000)) { > ... > } else if (start < 0x100000) { > ... > > or did it simply mess up the condition? I think it was just paranoid to test the same condition twice... > > > > if (start < 0x80000) { > > idx = 0; > > @@ -195,6 +201,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat) > > > > end = *partial_end - 1; /* end is inclusive */ > > *repeat = 1; > > + *uniform = 0; > > } > > > > if (!start_state) > > @@ -206,6 +213,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat) > > continue; > > } > > > > + *uniform = 0; > > if (check_type_overlap(&prev_match, &curr_match)) > > return curr_match; > > } > > > > @@ -222,17 +230,21 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat) > > } > > > > /* > > - * Returns the effective MTRR type for the region > > + * Returns the effective MTRR type for the region. *uniform is set to 1 > > + * when a given range is either fully covered by a single MTRR entry or > > + * not covered at all. > > + * > > * Error return: > > * 0xFF - when MTRR is not enabled > > */ > > -u8 mtrr_type_lookup(u64 start, u64 end) > > +u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform) > > { > > - u8 type, prev_type; > > + u8 type, prev_type, is_uniform, dummy; > > int repeat; > > u64 partial_end; > > > > - type = __mtrr_type_lookup(start, end, &partial_end, &repeat); > > + type = __mtrr_type_lookup(start, end, > > + &partial_end, &repeat, &is_uniform); > > > > /* > > * Common path is with repeat = 0. > > @@ -242,12 +254,18 @@ u8 mtrr_type_lookup(u64 start, u64 end) > > while (repeat) { > > prev_type = type; > > start = partial_end; > > - type = __mtrr_type_lookup(start, end, &partial_end, &repeat); > > + is_uniform = 0; > > > > - if (check_type_overlap(&prev_type, &type)) > > + type = __mtrr_type_lookup(start, end, > > + &partial_end, &repeat, &dummy); > > + > > + if (check_type_overlap(&prev_type, &type)) { > > + *uniform = 0; > > return type; > > + } > > } > > > > + *uniform = is_uniform; > > return type; > > So the MTRR code is from hell, it would be nice to first clean up the > whole code and the MTRR data structures before extending it with more > complexity ... Good idea. I will clean up the code (no functional change) before making this change. Thanks, -Toshi
next prev parent reply other threads:[~2015-03-11 16:52 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-03-10 20:23 [PATCH 0/3] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping Toshi Kani 2015-03-10 20:23 ` [PATCH 1/3] mm, x86: Document return values of mapping funcs Toshi Kani 2015-03-11 6:30 ` Ingo Molnar 2015-03-11 15:25 ` Toshi Kani 2015-03-10 20:23 ` [PATCH 2/3] mtrr, x86: Fix MTRR lookup to handle inclusive entry Toshi Kani 2015-03-11 6:32 ` Ingo Molnar 2015-03-11 15:27 ` Toshi Kani 2015-03-10 20:23 ` [PATCH 3/3] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping Toshi Kani 2015-03-11 7:02 ` Ingo Molnar 2015-03-11 16:52 ` Toshi Kani [this message] 2015-03-12 11:03 ` Ingo Molnar 2015-03-12 13:58 ` Toshi Kani
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1426092728.17007.380.camel@misato.fc.hp.com \ --to=toshi.kani@hp.com \ --cc=Elliott@hp.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=dave.hansen@intel.com \ --cc=hpa@zytor.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mingo@kernel.org \ --cc=mingo@redhat.com \ --cc=pebolle@tiscali.nl \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).