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

Re: [PATCH 1/2] x86/shadow: defer/avoid paging_mfn_is_dirty() invocation


  • To: Andrew Cooper <amc96@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 2 Dec 2021 09:15:54 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=sqRU5mbCOaYExaC9tD0BuvMXBCE+HbW5KwP4sSJvw0I=; b=XdG6RGWBWNGZ6aTGFVPJgUcR9pqR8eXqHAt+jcwVpplQPyvLwNjatntnxtLWOZvbaqfe/EHR9fJDg+IHgBJ4mO7pHaZ9byDHzSiuIJlbLtGIz7u2VJ2ASAeRdE+0zjcptPnNN3Gmhp1R+yWGyDrgPKWrxjxIWOFhc+YYH/U46FQ/eITwYQH0weUQFcKdvc5KKPDeW5RSg9a1EIRJX9B74it5WUxa86KgqSkxkspuHddG/sL32abt+eSQ8zxRSXmvOOkmcaQQKxGMOlhGRoPpMW4wOCAoP33fxwJ3Nd76zcGAA+IDDcnXUDBVicnDmTDqYJ5lMljPlcHz3e+7Ak2OqA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Lr3qhjiPMBFKkjxaJ2jiUTOXdVIyML7aIlbDCitcF0gwQbtkb91e7ZmmAI6lOdDZohWlm89uBuBFY6e+ibMjvZa5pPQgcUnNAxkmFz1hfd8CJS3GhuoS4wFLIgZxH9cUoHLaUaDCTaPFO353eok1hLrSV69qkHuaxCEFVY0Zkqr1rm6evIpxY+zfhCuKfSe2ZkUSleS/eAe1z/6uC18CJBWz6KfhXU7L3hhtFNIP1FIX1vblDcL0+07+5xJIiLbH1LWnZcchGpiAP89w3Tf1Dav6rnX1Fjms+684k1R3U2yowGOznMGCvNRpKvBScl0qES8kx0uRLDLgTNKatVg/mA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 02 Dec 2021 08:16:15 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 01.12.2021 19:33, Andrew Cooper wrote:
> On 01/12/2021 10:35, Jan Beulich wrote:
>> paging_mfn_is_dirty() is moderately expensive, so avoid its use unless
>> its result might actually change anything. This means moving the
>> surrounding if() down below all other checks that can result in clearing
>> _PAGE_RW from sflags, in order to then check whether _PAGE_RW is
>> actually still set there before calling the function.
>>
>> While moving the block of code, fold two if()s and make a few style
>> adjustments.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> TBD: Perhaps the dirty-VRAM piece also wants moving down a little, such
>>      that all three "level == 1" conditionals can be folded?
> 
> TBH, lots of the layout looks dubious, but I'm not sure how worthwhile
> micro-optimising it is.  This particular rearrangement is surely
> unmeasurable.

The point of the rearrangement suggestion was source tidying far more
than micro-optimizing.

> Fixing the (mis)use of mfn_valid() in the logdirty infrastructure will
> gain a far larger improvement, because that's dropping a fair number of
> lfence's from multiple paths, but it's still the case that anything here
> is rare-to-non-existent in production workloads, and vastly dominated by
> the VMExit cost even when migrating shadow VMs.

Quite possible, but entirely orthogonal to this change.

>> @@ -661,6 +644,25 @@ _sh_propagate(struct vcpu *v,
>>                    ) )
>>          sflags &= ~_PAGE_RW;
>>  
>> +    /*
>> +     * shadow_mode_log_dirty support
>> +     *
>> +     * Only allow the guest write access to a page a) on a demand fault,
>> +     * or b) if the page is already marked as dirty.
>> +     *
>> +     * (We handle log-dirty entirely inside the shadow code, without using 
>> the
>> +     * p2m_ram_logdirty p2m type: only HAP uses that.)
>> +     */
>> +    if ( level == 1 && unlikely(shadow_mode_log_dirty(d)) &&
>> +         mfn_valid(target_mfn) )
>> +    {
>> +        if ( ft & FETCH_TYPE_WRITE )
>> +            paging_mark_dirty(d, target_mfn);
>> +        else if ( (sflags & _PAGE_RW) &&
>> +                  !paging_mfn_is_dirty(d, target_mfn) )
>> +            sflags &= ~_PAGE_RW;
> 
> This is almost crying out for a logdirty_test_and_maybe_set() helper,
> because the decent of the logdirty trie is common between the two, but
> as this would be the only user, probably not worth it.

I'm struggling to see how this would improve the code construct without
altering the behavior. But then you say anyway that introducing one
probably isn't worth it as long as there's just one use site (and there
aren't ever two decents of the trie, so there's not really anything to
be saved performance wise).

> However, the more I look at the logdirty logic, the more it is clear
> that all the mfn_valid() tests should change to MFN_INVALID, and the
> result will be far more efficient.

As said - an orthogonal change; nothing to be folded into here imo.

Jan




 


Rackspace

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