On Wed, Oct 7, 2020 at 10:21 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: > > On 07/10/20 18:30, Ben Gardon wrote: > >> I'm starting to wonder if another iterator like > >> for_each_tdp_leaf_pte_root would be clearer, since this idiom repeats > >> itself quite often. The tdp_iter_next_leaf function would be easily > >> implemented as > >> > >> while (likely(iter->valid) && > >> (!is_shadow_present_pte(iter.old_spte) || > >> is_last_spte(iter.old_spte, iter.level)) > >> tdp_iter_next(iter); > > Do you see a substantial efficiency difference between adding a > > tdp_iter_next_leaf and building on for_each_tdp_pte_using_root with > > something like: > > > > #define for_each_tdp_leaf_pte_using_root(_iter, _root, _start, _end) \ > > for_each_tdp_pte_using_root(_iter, _root, _start, _end) \ > > if (!is_shadow_present_pte(_iter.old_spte) || \ > > !is_last_spte(_iter.old_spte, _iter.level)) \ > > continue; \ > > else > > > > I agree that putting those checks in a wrapper makes the code more concise. > > > > No, that would be just another way to write the same thing. That said, > making the iteration API more complicated also has disadvantages because > if get a Cartesian explosion of changes. I wouldn't be too worried about that. The only things I ever found worth making an iterator case for were: Every SPTE Every present SPTE Every present leaf SPTE And really there aren't many cases that use the middle one. > > Regarding the naming, I'm leaning towards > > tdp_root_for_each_pte > tdp_vcpu_for_each_pte > > which is shorter than the version with "using" and still clarifies that > "root" and "vcpu" are the thing that the iteration works on. That sounds good to me. I agree it's similarly clear. > > Paolo >
