[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v2 06/11] vmx: add help functions to support PML





On 04/17/2015 03:37 PM, Jan Beulich wrote:
On 17.04.15 at 09:23, <kai.huang@xxxxxxxxxxxxxxx> wrote:
On 04/17/2015 02:58 PM, Jan Beulich wrote:
On 17.04.15 at 08:51, <kai.huang@xxxxxxxxxxxxxxx> wrote:
On 04/17/2015 02:23 PM, Jan Beulich wrote:
On 17.04.15 at 05:10, <kai.huang@xxxxxxxxxxxxxxx> wrote:
On 04/16/2015 11:42 PM, Jan Beulich wrote:
On 15.04.15 at 09:03, <kai.huang@xxxxxxxxxxxxxxx> wrote:
+void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
+{
+    uint64_t *pml_buf;
+    unsigned long pml_idx;
+
+    ASSERT(vmx_vcpu_pml_enabled(v));
+
+    vmx_vmcs_enter(v);
+
+    __vmread(GUEST_PML_INDEX, &pml_idx);
Don't you require the vCPU to be non-running or current when you
get here? If so, perhaps add a respective ASSERT()?
Yes an ASSERT would be better.

v->pause_count will be increased if vcpu is kicked out by domain_pause
explicitly, but looks the same thing won't be done if vcpu is kicked out
by PML buffer full VMEXIT. So should the ASSERT be done like below?

ASSERT(atomic_read(&v->pause_count) || (v == current));
For one I'd reverse the two parts. And then I think pause count
being non-zero is not a sufficient condition - if a non-synchronous
pause was issued against the vCPU it may still be running. I'd
suggest !vcpu_runnable(v) && !v->is_running, possibly with the
pause count check instead of the runnable one if the only
permitted case where v != current requires the vCPU to be
paused.
The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:

       - When PML full VMEXIT happens
       - In paging_log_dirty_op & hap_track_dirty_vram, before reporting
dirty pages to userspace.
       - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when
log-dirty mode is disabled.

In the latter two cases, domain_pause is guaranteed to be called before
vmx_vcpu_flush_pml_buffer is called, therefore looks there's no
possibility of non-synchronous pause of the vcpu.

Or are you suggesting we should suppose this function can be called from
any caller, and meanwhile is able to act reasonably?
No. All I'm saying is in order to protect against eventual undue
future callers, it should assert that its preconditions are met. I.e.
if the vCPU is expected to be paused, check that the pause
count in non-zero _and_ that the pause actually took effect.
I see. I will do as you suggested:

ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));

And v != current should be the only case requires the vcpu to be paused.
But if you require (or at least expect) the vCPU to be paused, this
isn't what I suggested. Afaict

ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));

would then be the right check (and, while not be a full guarantee that
things wouldn't change behind your back, would at least increase
chances that the vCPU's runnable state won't change, as the vCPU
could have been non-runnable for reasons other than having been
paused).
You are right. Your last sentence convinced me. I didn't go that far. I will use atomic_read(&v->pause_count) instead of !vcpu_runnable(v).

Thanks,
-Kai

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.