On Tue, Jan 3, 2023 at 11:32 AM Mingwei Zhang <mizhang@xxxxxxxxxx> wrote:
>
> On Wed, Dec 21, 2022 at 6:35 PM Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
> >
> > +static void mmu_free_sp_memory_cache(struct kvm_mmu_memory_cache *cache,
> > + spinlock_t *cache_lock)
> > +{
> > + int orig_nobjs;
> > +
> > + spin_lock(cache_lock);
> > + orig_nobjs = cache->nobjs;
> > + kvm_mmu_free_memory_cache(cache);
> > + if (orig_nobjs)
> > + percpu_counter_sub(&kvm_total_unused_mmu_pages, orig_nobjs);
> > +
> > + spin_unlock(cache_lock);
> > +}
>
> I think the mmu_cache allocation and deallocation may cause the usage
> of GFP_ATOMIC (as observed by other reviewers as well). Adding a new
> lock would definitely sound like a plan, but I think it might affect
> the performance. Alternatively, I am wondering if we could use a
> mmu_cache_sequence similar to mmu_notifier_seq to help avoid the
> concurrency?
>
Can you explain more about the performance impact? Each vcpu will have
its own mutex. So, only contention will be with the mmu_shrinker. This
shrinker will use mutex_try_lock() which will not block to wait for
the lock, it will just pass on to the next vcpu. While shrinker is
holding the lock, vcpu will be blocked in the page fault path but I
think it should not have a huge impact considering it will execute
rarely and for a small time.
> Similar to mmu_notifier_seq, mmu_cache_sequence should be protected by
> mmu write lock. In the page fault path, each vcpu has to collect a
> snapshot of mmu_cache_sequence before calling into
> mmu_topup_memory_caches() and check the value again when holding the
> mmu lock. If the value is different, that means the mmu_shrinker has
> removed the cache objects and because of that, the vcpu should retry.
>
Yeah, this can be one approach. I think it will come down to the
performance impact of using mutex which I don't think should be a
concern.
