| Message ID | 20210311231658.1243953-5-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix RCU warnings in TDP MMU | expand |
On Thu, Mar 11, 2021, Ben Gardon wrote: > In tdp_mmu_iter_cond_resched there is a call to tdp_iter_start which > causes the iterator to continue its walk over the paging structure from > the root. This is needed after a yield as paging structure could have > been freed in the interim. > > The tdp_iter_start call is not very clear and something of a hack. It > requires exposing tdp_iter fields not used elsewhere in tdp_mmu.c and > the effect is not obvious from the function name. Factor a more aptly > named function out of tdp_iter_start and call it from > tdp_mmu_iter_cond_resched and tdp_iter_start. What about calling it tdp_iter_restart()? Or tdp_iter_resume()? Or something like tdp_iter_restart_at_next() if we want it to give a hint that the next_last thing is where it restarts. I think I like tdp_iter_restart() the best. It'd be easy enough to add a function comment clarifying from where it restarts, and IMO the resulting code in tdp_mmu_iter_cond_resched() is the most intutive, e.g. it makes it very clear that the walk is being restarted in some capacity after yielding.
On 12/03/21 17:35, Sean Christopherson wrote: > What about calling it tdp_iter_restart()? Or tdp_iter_resume()? Or something > like tdp_iter_restart_at_next() if we want it to give a hint that the next_last > thing is where it restarts. > > I think I like tdp_iter_restart() the best. It'd be easy enough to add a > function comment clarifying from where it restarts, and IMO the resulting code > in tdp_mmu_iter_cond_resched() is the most intutive, e.g. it makes it very clear > that the walk is being restarted in some capacity after yielding. I agree with tdp_iter_restart(), or tdp_iter_restart_from_root() too. Paolo
diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c index 8e2c053533b6..bbf53b98cc65 100644 --- a/arch/x86/kvm/mmu/tdp_iter.c +++ b/arch/x86/kvm/mmu/tdp_iter.c @@ -20,6 +20,21 @@ static gfn_t round_gfn_for_level(gfn_t gfn, int level) return gfn & -KVM_PAGES_PER_HPAGE(level); } +/* + * Return the TDP iterator to the root PT and allow it to continue its + * traversal over the paging structure from there. + */ +void tdp_iter_return_to_root(struct tdp_iter *iter) +{ + iter->yielded_gfn = iter->next_last_level_gfn; + iter->level = iter->root_level; + + iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level); + tdp_iter_refresh_sptep(iter); + + iter->valid = true; +} + /* * Sets a TDP iterator to walk a pre-order traversal of the paging structure * rooted at root_pt, starting with the walk to translate next_last_level_gfn. @@ -31,16 +46,11 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, WARN_ON(root_level > PT64_ROOT_MAX_LEVEL); iter->next_last_level_gfn = next_last_level_gfn; - iter->yielded_gfn = iter->next_last_level_gfn; iter->root_level = root_level; iter->min_level = min_level; - iter->level = root_level; - iter->pt_path[iter->level - 1] = (tdp_ptep_t)root_pt; + iter->pt_path[iter->root_level - 1] = (tdp_ptep_t)root_pt; - iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level); - tdp_iter_refresh_sptep(iter); - - iter->valid = true; + tdp_iter_return_to_root(iter); } /* diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h index 5a47c57810ab..2ecc48e78526 100644 --- a/arch/x86/kvm/mmu/tdp_iter.h +++ b/arch/x86/kvm/mmu/tdp_iter.h @@ -63,5 +63,6 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, int min_level, gfn_t next_last_level_gfn); void tdp_iter_next(struct tdp_iter *iter); u64 *tdp_iter_root_pt(struct tdp_iter *iter); +void tdp_iter_return_to_root(struct tdp_iter *iter); #endif /* __KVM_X86_MMU_TDP_ITER_H */ diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index a8fdccf4fd06..941e9d11c7ed 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -653,9 +653,7 @@ static inline bool tdp_mmu_iter_cond_resched(struct kvm *kvm, WARN_ON(iter->gfn > iter->next_last_level_gfn); - tdp_iter_start(iter, tdp_iter_root_pt(iter), - iter->root_level, iter->min_level, - iter->next_last_level_gfn); + tdp_iter_return_to_root(iter); return true; }
In tdp_mmu_iter_cond_resched there is a call to tdp_iter_start which causes the iterator to continue its walk over the paging structure from the root. This is needed after a yield as paging structure could have been freed in the interim. The tdp_iter_start call is not very clear and something of a hack. It requires exposing tdp_iter fields not used elsewhere in tdp_mmu.c and the effect is not obvious from the function name. Factor a more aptly named function out of tdp_iter_start and call it from tdp_mmu_iter_cond_resched and tdp_iter_start. No functional change intended. Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_iter.c | 24 +++++++++++++++++------- arch/x86/kvm/mmu/tdp_iter.h | 1 + arch/x86/kvm/mmu/tdp_mmu.c | 4 +--- 3 files changed, 19 insertions(+), 10 deletions(-)