| Message ID | 20210112181041.356734-6-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow parallel page faults with TDP MMU | expand |
On Tue, Jan 12, 2021, Ben Gardon wrote: > There are two problems with the way the TDP MMU yields in long running > functions. 1.) Given certain conditions, the function may not yield > reliably / frequently enough. 2.) In some functions the TDP iter risks > not making forward progress if two threads livelock yielding to > one another. > > Case 1 is possible if for example, a paging structure was very large > but had few, if any writable entries. wrprot_gfn_range could traverse many > entries before finding a writable entry and yielding. > > Case 2 is possible if two threads were trying to execute wrprot_gfn_range. > Each could write protect an entry and then yield. This would reset the > tdp_iter's walk over the paging structure and the loop would end up > repeating the same entry over and over, preventing either thread from > making forward progress. > > Fix these issues by moving the yield to the beginning of the loop, > before other checks and only yielding if the loop has made forward > progress since the last yield. I think it'd be best to split this into two patches, e.g. ensure forward progress and then yield more agressively. They are two separate bugs, and I don't think that ensuring forward progress would exacerbate case #1. I'm not worried about breaking things so much as getting more helpful shortlogs; "Fix yielding in TDP MMU" doesn't provide any insight into what exactly was broken. E.g. something like: KVM: x86/mmu: Ensure forward progress when yielding in TDP MMU iter KVM: x86/mmu: Yield in TDU MMU iter even if no real work was done > Fixes: a6a0b05da9f3 ("kvm: x86/mmu: Support dirty logging for the TDP MMU") > Reviewed-by: Peter Feiner <pfeiner@google.com> > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 83 +++++++++++++++++++++++++++++++------- > 1 file changed, 69 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index b2784514ca2d..1987da0da66e 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -470,9 +470,23 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, > gfn_t start, gfn_t end, bool can_yield) > { > struct tdp_iter iter; > + gfn_t last_goal_gfn = start; > bool flush_needed = false; > > tdp_root_for_each_pte(iter, root, start, end) { > + /* Ensure forward progress has been made before yielding. */ > + if (can_yield && iter.goal_gfn != last_goal_gfn && Make last_goal_gfn a property of the iterator, that way all this logic can be shoved into tdp_mmu_iter_flush_cond_resched(), and the comments about ensuring forward progress and effectively invalidating/resetting the iterator (the comment below) can be a function comment, as opposed to being copied everywhere. E.g. there can be a big scary warning in the function comment stating that the caller must restart its loop if the helper yielded. Tangentially related, the name goal_gfn is quite confusing. "goal" and "end" are synonyms, but "goal" is often initialized with "start", and it's not used to terminate the walk. Maybe next_gfn instead? And maybe yielded_gfn, since last_next_gfn is pretty horrendous. > + tdp_mmu_iter_flush_cond_resched(kvm, &iter)) { This isn't quite correct, as tdp_mmu_iter_flush_cond_resched() will do an expensive remote TLB flush on every yield, even if no flush is needed. The cleanest solution is likely to drop tdp_mmu_iter_flush_cond_resched() and instead add a @flush param to tdp_mmu_iter_cond_resched(). If it's tagged __always_inline, then the callers that unconditionally pass true/false will optimize out the conditional code. At that point, I think it would also make sense to fold tdp_iter_refresh_walk() into tdp_mmu_iter_cond_resched(), because really we shouldn't be mucking with the guts of the iter except for the yield case. > + last_goal_gfn = iter.goal_gfn; Another argument for both renaming goal_gfn and moving last_*_gfn into the iter: it's not at all obvious that updating the last gfn _after_ tdp_iter_refresh_walk() is indeed correct. You can also avoid a local variable by doing max(iter->next_gfn, iter->gfn) when calling tdp_iter_refresh_walk(). IMO, that's also a bit easier to understand than an open-coded equivalent. E.g. putting it all together, with yielded_gfn set by tdp_iter_start(): static __always_inline bool tdp_mmu_iter_cond_resched(struct kvm *kvm, struct tdp_iter *iter, bool flush) { /* Ensure forward progress has been made since the last yield. */ if (iter->next_gfn == iter->yielded_gfn) return false; if (need_resched() || spin_needbreak(&kvm->mmu_lock)) { if (flush) kvm_flush_remote_tlbs(kvm); cond_resched_lock(&kvm->mmu_lock); /* * Restart the walk over the paging structure from the root, * starting from the highest gfn the iterator had previously * reached. The entire paging structure, except the root, may * have been completely torn down and rebuilt while we yielded. */ tdp_iter_start(iter, iter->pt_path[iter->root_level - 1], iter->root_level, iter->min_level, max(iter->next_gfn, iter->gfn)); return true; } return false; } > + flush_needed = false; > + /* > + * Yielding caused the paging structure walk to be > + * reset so skip to the next iteration to continue the > + * walk from the root. > + */ > + continue; > + } > + > if (!is_shadow_present_pte(iter.old_spte)) > continue; >
On Wed, Jan 20, 2021 at 11:28 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Jan 12, 2021, Ben Gardon wrote: > > There are two problems with the way the TDP MMU yields in long running > > functions. 1.) Given certain conditions, the function may not yield > > reliably / frequently enough. 2.) In some functions the TDP iter risks > > not making forward progress if two threads livelock yielding to > > one another. > > > > Case 1 is possible if for example, a paging structure was very large > > but had few, if any writable entries. wrprot_gfn_range could traverse many > > entries before finding a writable entry and yielding. > > > > Case 2 is possible if two threads were trying to execute wrprot_gfn_range. > > Each could write protect an entry and then yield. This would reset the > > tdp_iter's walk over the paging structure and the loop would end up > > repeating the same entry over and over, preventing either thread from > > making forward progress. > > > > Fix these issues by moving the yield to the beginning of the loop, > > before other checks and only yielding if the loop has made forward > > progress since the last yield. > > I think it'd be best to split this into two patches, e.g. ensure forward > progress and then yield more agressively. They are two separate bugs, and I > don't think that ensuring forward progress would exacerbate case #1. I'm not > worried about breaking things so much as getting more helpful shortlogs; "Fix > yielding in TDP MMU" doesn't provide any insight into what exactly was broken. > E.g. something like: > > KVM: x86/mmu: Ensure forward progress when yielding in TDP MMU iter > KVM: x86/mmu: Yield in TDU MMU iter even if no real work was done > > > Fixes: a6a0b05da9f3 ("kvm: x86/mmu: Support dirty logging for the TDP MMU") > > Reviewed-by: Peter Feiner <pfeiner@google.com> > > > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > arch/x86/kvm/mmu/tdp_mmu.c | 83 +++++++++++++++++++++++++++++++------- > > 1 file changed, 69 insertions(+), 14 deletions(-) > > > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > > index b2784514ca2d..1987da0da66e 100644 > > --- a/arch/x86/kvm/mmu/tdp_mmu.c > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > > @@ -470,9 +470,23 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, > > gfn_t start, gfn_t end, bool can_yield) > > { > > struct tdp_iter iter; > > + gfn_t last_goal_gfn = start; > > bool flush_needed = false; > > > > tdp_root_for_each_pte(iter, root, start, end) { > > + /* Ensure forward progress has been made before yielding. */ > > + if (can_yield && iter.goal_gfn != last_goal_gfn && > > Make last_goal_gfn a property of the iterator, that way all this logic can be > shoved into tdp_mmu_iter_flush_cond_resched(), and the comments about ensuring > forward progress and effectively invalidating/resetting the iterator (the > comment below) can be a function comment, as opposed to being copied everywhere. > E.g. there can be a big scary warning in the function comment stating that the > caller must restart its loop if the helper yielded. > > Tangentially related, the name goal_gfn is quite confusing. "goal" and "end" > are synonyms, but "goal" is often initialized with "start", and it's not used to > terminate the walk. Maybe next_gfn instead? And maybe yielded_gfn, since > last_next_gfn is pretty horrendous. All these are excellent suggestions and definitely make the code cleaner. I'll definitely adopt yielded_gfn. While I agree goal_gfn is a little odd, I think next_gfn could be more misleading because the goal_gfn is really more of a target than the next step. It might take 4 or 5 steps to actually reach a last-level entry mapping that gfn. target_last_level_gfn or next_last_level_gfn would probably be the most accurate option. > > > + tdp_mmu_iter_flush_cond_resched(kvm, &iter)) { > > This isn't quite correct, as tdp_mmu_iter_flush_cond_resched() will do an > expensive remote TLB flush on every yield, even if no flush is needed. The > cleanest solution is likely to drop tdp_mmu_iter_flush_cond_resched() and > instead add a @flush param to tdp_mmu_iter_cond_resched(). If it's tagged > __always_inline, then the callers that unconditionally pass true/false will > optimize out the conditional code. > > At that point, I think it would also make sense to fold tdp_iter_refresh_walk() > into tdp_mmu_iter_cond_resched(), because really we shouldn't be mucking with > the guts of the iter except for the yield case. > > > + last_goal_gfn = iter.goal_gfn; > > Another argument for both renaming goal_gfn and moving last_*_gfn into the iter: > it's not at all obvious that updating the last gfn _after_ tdp_iter_refresh_walk() > is indeed correct. > > You can also avoid a local variable by doing max(iter->next_gfn, iter->gfn) when > calling tdp_iter_refresh_walk(). IMO, that's also a bit easier to understand > than an open-coded equivalent. > > E.g. putting it all together, with yielded_gfn set by tdp_iter_start(): > > static __always_inline bool tdp_mmu_iter_cond_resched(struct kvm *kvm, > struct tdp_iter *iter, > bool flush) > { > /* Ensure forward progress has been made since the last yield. */ > if (iter->next_gfn == iter->yielded_gfn) > return false; > > if (need_resched() || spin_needbreak(&kvm->mmu_lock)) { > if (flush) > kvm_flush_remote_tlbs(kvm); > cond_resched_lock(&kvm->mmu_lock); > > /* > * Restart the walk over the paging structure from the root, > * starting from the highest gfn the iterator had previously > * reached. The entire paging structure, except the root, may > * have been completely torn down and rebuilt while we yielded. > */ > tdp_iter_start(iter, iter->pt_path[iter->root_level - 1], > iter->root_level, iter->min_level, > max(iter->next_gfn, iter->gfn)); > return true; > } > > return false; > } > > > + flush_needed = false; > > + /* > > + * Yielding caused the paging structure walk to be > > + * reset so skip to the next iteration to continue the > > + * walk from the root. > > + */ > > + continue; > > + } > > + > > if (!is_shadow_present_pte(iter.old_spte)) > > continue; > >
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index b2784514ca2d..1987da0da66e 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -470,9 +470,23 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, gfn_t start, gfn_t end, bool can_yield) { struct tdp_iter iter; + gfn_t last_goal_gfn = start; bool flush_needed = false; tdp_root_for_each_pte(iter, root, start, end) { + /* Ensure forward progress has been made before yielding. */ + if (can_yield && iter.goal_gfn != last_goal_gfn && + tdp_mmu_iter_flush_cond_resched(kvm, &iter)) { + last_goal_gfn = iter.goal_gfn; + flush_needed = false; + /* + * Yielding caused the paging structure walk to be + * reset so skip to the next iteration to continue the + * walk from the root. + */ + continue; + } + if (!is_shadow_present_pte(iter.old_spte)) continue; @@ -487,12 +501,7 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, continue; tdp_mmu_set_spte(kvm, &iter, 0); - - if (can_yield) - flush_needed = !tdp_mmu_iter_flush_cond_resched(kvm, - &iter); - else - flush_needed = true; + flush_needed = true; } return flush_needed; } @@ -850,12 +859,25 @@ static bool wrprot_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, { struct tdp_iter iter; u64 new_spte; + gfn_t last_goal_gfn = start; bool spte_set = false; BUG_ON(min_level > KVM_MAX_HUGEPAGE_LEVEL); for_each_tdp_pte_min_level(iter, root->spt, root->role.level, min_level, start, end) { + /* Ensure forward progress has been made before yielding. */ + if (iter.goal_gfn != last_goal_gfn && + tdp_mmu_iter_cond_resched(kvm, &iter)) { + last_goal_gfn = iter.goal_gfn; + /* + * Yielding caused the paging structure walk to be + * reset so skip to the next iteration to continue the + * walk from the root. + */ + continue; + } + if (!is_shadow_present_pte(iter.old_spte) || !is_last_spte(iter.old_spte, iter.level)) continue; @@ -864,8 +886,6 @@ static bool wrprot_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, tdp_mmu_set_spte_no_dirty_log(kvm, &iter, new_spte); spte_set = true; - - tdp_mmu_iter_cond_resched(kvm, &iter); } return spte_set; } @@ -906,9 +926,22 @@ static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, { struct tdp_iter iter; u64 new_spte; + gfn_t last_goal_gfn = start; bool spte_set = false; tdp_root_for_each_leaf_pte(iter, root, start, end) { + /* Ensure forward progress has been made before yielding. */ + if (iter.goal_gfn != last_goal_gfn && + tdp_mmu_iter_cond_resched(kvm, &iter)) { + last_goal_gfn = iter.goal_gfn; + /* + * Yielding caused the paging structure walk to be + * reset so skip to the next iteration to continue the + * walk from the root. + */ + continue; + } + if (spte_ad_need_write_protect(iter.old_spte)) { if (is_writable_pte(iter.old_spte)) new_spte = iter.old_spte & ~PT_WRITABLE_MASK; @@ -923,8 +956,6 @@ static bool clear_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, tdp_mmu_set_spte_no_dirty_log(kvm, &iter, new_spte); spte_set = true; - - tdp_mmu_iter_cond_resched(kvm, &iter); } return spte_set; } @@ -1029,9 +1060,22 @@ static bool set_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, { struct tdp_iter iter; u64 new_spte; + gfn_t last_goal_gfn = start; bool spte_set = false; tdp_root_for_each_pte(iter, root, start, end) { + /* Ensure forward progress has been made before yielding. */ + if (iter.goal_gfn != last_goal_gfn && + tdp_mmu_iter_cond_resched(kvm, &iter)) { + last_goal_gfn = iter.goal_gfn; + /* + * Yielding caused the paging structure walk to be + * reset so skip to the next iteration to continue the + * walk from the root. + */ + continue; + } + if (!is_shadow_present_pte(iter.old_spte)) continue; @@ -1039,8 +1083,6 @@ static bool set_dirty_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, tdp_mmu_set_spte(kvm, &iter, new_spte); spte_set = true; - - tdp_mmu_iter_cond_resched(kvm, &iter); } return spte_set; @@ -1078,9 +1120,23 @@ static void zap_collapsible_spte_range(struct kvm *kvm, { struct tdp_iter iter; kvm_pfn_t pfn; + gfn_t last_goal_gfn = start; bool spte_set = false; tdp_root_for_each_pte(iter, root, start, end) { + /* Ensure forward progress has been made before yielding. */ + if (iter.goal_gfn != last_goal_gfn && + tdp_mmu_iter_flush_cond_resched(kvm, &iter)) { + last_goal_gfn = iter.goal_gfn; + spte_set = false; + /* + * Yielding caused the paging structure walk to be + * reset so skip to the next iteration to continue the + * walk from the root. + */ + continue; + } + if (!is_shadow_present_pte(iter.old_spte) || is_last_spte(iter.old_spte, iter.level)) continue; @@ -1091,8 +1147,7 @@ static void zap_collapsible_spte_range(struct kvm *kvm, continue; tdp_mmu_set_spte(kvm, &iter, 0); - - spte_set = !tdp_mmu_iter_flush_cond_resched(kvm, &iter); + spte_set = true; } if (spte_set)
There are two problems with the way the TDP MMU yields in long running functions. 1.) Given certain conditions, the function may not yield reliably / frequently enough. 2.) In some functions the TDP iter risks not making forward progress if two threads livelock yielding to one another. Case 1 is possible if for example, a paging structure was very large but had few, if any writable entries. wrprot_gfn_range could traverse many entries before finding a writable entry and yielding. Case 2 is possible if two threads were trying to execute wrprot_gfn_range. Each could write protect an entry and then yield. This would reset the tdp_iter's walk over the paging structure and the loop would end up repeating the same entry over and over, preventing either thread from making forward progress. Fix these issues by moving the yield to the beginning of the loop, before other checks and only yielding if the loop has made forward progress since the last yield. Fixes: a6a0b05da9f3 ("kvm: x86/mmu: Support dirty logging for the TDP MMU") Reviewed-by: Peter Feiner <pfeiner@google.com> Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 83 +++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 14 deletions(-)