Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 6 Apr 2022 18:26:18 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Mingwei Zhang <mizhang@google.com>
Cc:     Peter Gonda <pgonda@google.com>, kvm <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] KVM: SEV: Add cond_resched() to loop in
 sev_clflush_pages()
Message-ID: <Yk3bSmQTspjZHUZf@google.com>
References: <20220330164306.2376085-1-pgonda@google.com>
 <CAL715W+S-SJwXBhYO=_T-9uAPLt6cQ-Hn+_+ehefAh6+kQ_zOA@mail.gmail.com>
 <YkYdlfYM/FWlMqMg@google.com>
 <CAL715WLhy7EkJCyO7vzak3O8iw8GDRHkPF8aRtDedPXO1vx_Qw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAL715WLhy7EkJCyO7vzak3O8iw8GDRHkPF8aRtDedPXO1vx_Qw@mail.gmail.com>
Precedence: bulk

On Wed, Apr 06, 2022, Mingwei Zhang wrote:
> Hi Sean,
> 
> > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > index 75fa6dd268f0..c2fe89ecdb2d 100644
> > > > --- a/arch/x86/kvm/svm/sev.c
> > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > @@ -465,6 +465,7 @@ static void sev_clflush_pages(struct page *pages[], unsigned long npages)
> > > >                 page_virtual = kmap_atomic(pages[i]);
> > > >                 clflush_cache_range(page_virtual, PAGE_SIZE);
> > > >                 kunmap_atomic(page_virtual);
> > > > +               cond_resched();
> > >
> > > If you add cond_resched() here, the frequency (once per 4K) might be
> > > too high. You may want to do it once per X pages, where X could be
> > > something like 1G/4K?
> >
> > No, every iteration is perfectly ok.  The "cond"itional part means that this will
> > reschedule if and only if it actually needs to be rescheduled, e.g. if the task's
> > timeslice as expired.  The check for a needed reschedule is cheap, using
> > cond_resched() in tight-ish loops is ok and intended, e.g. KVM does a reched
> > check prior to enterring the guest.
> 
> Double check on the code again. I think the point is not about flag
> checking. Obviously branch prediction could really help. The point I
> think is the 'call' to cond_resched(). Depending on the kernel
> configuration, cond_resched() may not always be inlined, at least this
> is my understanding so far? So if that is true, then it still might
> not always be the best to call cond_resched() that often.

Eh, compared to the cost of 64 back-to-back CLFLUSHOPTs, the cost of __cond_resched()
is peanuts.  Even accounting for the rcu_all_qs() work, it's still dwarfed by the
cost of flushing data from the cache.  E.g. based on Agner Fog's wonderful uop
latencies[*], the actual flush time for a single page is going to be upwards of
10k cycles, whereas __cond_resched() is going to well under 100 cycles in the happy
case of no work.  Even if those throughput numbers are off by an order of magnitude,
e.g. CLFLUSHOPT can complete in 15 cycles, that's still ~1k cycles.

Peter, don't we also theoretically need cond_resched() in the loops in
sev_launch_update_data()?  AFAICT, there's no articifical restriction on the size
of the payload, i.e. the kernel is effectively relying on userspace to not update
large swaths of memory.

[*] https://www.agner.org/optimize/instruction_tables.pdf