|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms
On 11/13/18 7:57 PM, George Dunlap wrote:
> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part. While
>> touching the code, I've switched global_logdirty from bool_t
>> to bool.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>
> I've convinced myself that this patch is probably correct now, and as a
> result I've had a chance to look a bit at the resulting code. Which
> means, unfortunately, that I'm going to be a bit annoying and ask more
> questions that I didn't ask last time.
Thanks for the review, and please ask away. :)
>> ---
>> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>
>> ---
>> Changes since V4:
>> - Always call p2m_free_logdirty() in p2m_free_one() (previously
>> the call was gated on hap_enabled(p2m->domain) && cpu_has_vmx).
>> ---
>> xen/arch/x86/mm/p2m.c | 74
>> ++++++++++++++++++++++++++++++++++++-----------
>> xen/include/asm-x86/p2m.h | 2 +-
>> 2 files changed, 58 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 42b9ef4..69536c1 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -59,6 +59,28 @@ static void p2m_nestedp2m_init(struct p2m_domain *p2m)
>> #endif
>> }
>>
>> +static int p2m_init_logdirty(struct p2m_domain *p2m)
>> +{
>> + if ( p2m->logdirty_ranges )
>> + return 0;
>> +
>> + p2m->logdirty_ranges = rangeset_new(p2m->domain, "log-dirty",
>> + RANGESETF_prettyprint_hex);
>> + if ( !p2m->logdirty_ranges )
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> +static void p2m_free_logdirty(struct p2m_domain *p2m)
>> +{
>> + if ( !p2m->logdirty_ranges )
>> + return;
>> +
>> + rangeset_destroy(p2m->logdirty_ranges);
>> + p2m->logdirty_ranges = NULL;
>> +}
>> +
>> /* Init the datastructures for later use by the p2m code */
>> static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
>> {
>> @@ -107,6 +129,7 @@ free_p2m:
>>
>> static void p2m_free_one(struct p2m_domain *p2m)
>> {
>> + p2m_free_logdirty(p2m);
>> if ( hap_enabled(p2m->domain) && cpu_has_vmx )
>> ept_p2m_uninit(p2m);
>> free_cpumask_var(p2m->dirty_cpumask);
>> @@ -116,19 +139,19 @@ static void p2m_free_one(struct p2m_domain *p2m)
>> static int p2m_init_hostp2m(struct domain *d)
>> {
>> struct p2m_domain *p2m = p2m_init_one(d);
>> + int rc;
>>
>> - if ( p2m )
>> - {
>> - p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>> - RANGESETF_prettyprint_hex);
>> - if ( p2m->logdirty_ranges )
>> - {
>> - d->arch.p2m = p2m;
>> - return 0;
>> - }
>> + if ( !p2m )
>> + return -ENOMEM;
>> +
>> + rc = p2m_init_logdirty(p2m);
>> +
>> + if ( !rc )
>> + d->arch.p2m = p2m;
>> + else
>> p2m_free_one(p2m);
>> - }
>> - return -ENOMEM;
>> +
>> + return rc;
>> }
>>
>> static void p2m_teardown_hostp2m(struct domain *d)
>> @@ -138,7 +161,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
>>
>> if ( p2m )
>> {
>> - rangeset_destroy(p2m->logdirty_ranges);
>> p2m_free_one(p2m);
>> d->arch.p2m = NULL;
>> }
>> @@ -2279,6 +2301,18 @@ void p2m_flush_altp2m(struct domain *d)
>> altp2m_list_unlock(d);
>> }
>
> I think everything above here could usefully be in its own patch; it
> would make it easier to verify that there were no functional changes in
> the refactoring.
Right, I'll split this patch then.
>> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
>> +{
>> + struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
>> + int rc = p2m_init_logdirty(p2m);
>> +
>> + if ( rc )
>> + return rc;
>> +
>> + /* The following is really just a rangeset copy. */
>> + return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
>> +}
>> +
>> int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>> {
>> int rc = -EINVAL;
>> @@ -2290,8 +2324,9 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned
>> int idx)
>>
>> if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>> {
>> - p2m_init_altp2m_ept(d, idx);
>> - rc = 0;
>> + rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
>> + if ( !rc )
>> + p2m_init_altp2m_ept(d, idx);
>> }
>>
>> altp2m_list_unlock(d);
>> @@ -2310,9 +2345,13 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t
>> *idx)
>> if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>> continue;
>>
>> - p2m_init_altp2m_ept(d, i);
>> - *idx = i;
>> - rc = 0;
>> + rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[i]);
>> +
>> + if ( !rc )
>> + {
>> + p2m_init_altp2m_ept(d, i);
>> + *idx = i;
>> + }
>
> It looks like there's a 1-1 correspondence between
> p2m_init_altp2m_logdirty() succeeding and calling p2m_inti_altp2m_ept().
> Would it make sense to combine them into the same function, maybe named
> "p2m_activate_altp2m()" or something (to distinguish it from
> p2m_init_altp2m())?
Of course, that'll cut back on some redundancy. Always a good thing.
>> @@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d,
>> unsigned int idx)
>> {
>> p2m_flush_table(d->arch.altp2m_p2m[idx]);
>> /* Uninit and reinit ept to force TLB shootdown */
>> + p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>> ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>> ept_p2m_init(d->arch.altp2m_p2m[idx]);
>> d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>
> (In case I forget: Also, this is called without holding the appropriate
> p2m lock. )
Could you please provide more details? I have assumed that at the point
of calling a function called p2m_destroy_altp2m_by_id() it should be
safe to tear the altp2m down without further precaution.
I think you're saying that I should p2m_lock(d->arch.altp2m_p2m[idx])
just for the duration of the p2m_free_logdirty() call?
> I'm a bit suspicious of long strings of these sorts of functions in the
> middle of another function. It turns out that there are three copies of
> this sequence of function calls (p2m_flush_table -> ept_p2m_uninit ->
> ept_p2m_init):
>
> * Here (p2m_destroy_altp2m_id), when the user asks for the alt2m index
> to be destroyed
>
> * In p2m_flush_altp2m(), which is called when altp2m is disabled for a
> domain
>
> * In p2m_reset_altp2m(), which is called when an entry in the hostp2m is
> set to INVALID_MFN.
>
> Presumably in p2m_reset_altp2m() we don't want to call
> p2m_free_logdirty(), as the altp2m is still active and we want to keep
> the logdirty ranges around. But in p2m_flush_altp2m(), I'm pretty sure
> we do want to discard them: when altp2m is enabled again,
> p2m_init_logdirty() will return early, leaving the old rangesets in
> place; if the hostp2m rangesets have changed between the time altp2m was
> disabled and enabled again, the rangeset_merge() may have incorrect results.
I'll call p2m_free_logdirty() in p2m_flush_altp2m() as well.
> At the moment we essentially have two "init" states:
> * After domain creation; altp2m structures allocated, but no rangesets, & c
> * After being enabled for the first time: rangesets mirroring hostp2m,
> p2m_init_altp2m_ept() initialization done
>
> Is there any particular reason we allocate the p2m structures on domain
> creation, but not logdirty range structures? It seems like allocating
> altp2m structures on-demand, rather than at domain creation time, might
> make a lot of the reasoning here simpler.
I assume that this question is not addressed to me, since I'm not able
to answer it - I can only assume that having less heap used has been
preferred.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |