On 01.12.2021 17:06, Andrew Cooper wrote:
> On 01/12/2021 10:35, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/mm-locks.h
>> +++ b/xen/arch/x86/mm/mm-locks.h
>> @@ -40,7 +40,7 @@ static inline void mm_lock_init(mm_lock_
>> l->unlock_level = 0;
>> }
>>
>> -static inline int mm_locked_by_me(mm_lock_t *l)
>> +static inline int mm_locked_by_me(const mm_lock_t *l)
>
> bool too?
Oh, indeed. Will do.
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -351,14 +351,14 @@ void paging_mark_dirty(struct domain *d,
>> paging_mark_pfn_dirty(d, pfn);
>> }
>>
>> -
>> +#ifdef CONFIG_SHADOW_PAGING
>> /* Is this guest page dirty? */
>> -int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
>> +bool paging_mfn_is_dirty(const struct domain *d, mfn_t gmfn)
>> {
>> pfn_t pfn;
>> mfn_t mfn, *l4, *l3, *l2;
>> unsigned long *l1;
>> - int rv;
>> + bool dirty;
>>
>> ASSERT(paging_locked_by_me(d));
>> ASSERT(paging_mode_log_dirty(d));
>> @@ -367,36 +367,37 @@ int paging_mfn_is_dirty(struct domain *d
>> pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
>
> There's nothing inherently paging.c related about this function.
> Thoughts on moving the implementation across, rather than #ifdef-ing it?
I wouldn't strictly object to a move, but doing so would disconnect this
function from paging_mark_dirty() and other log-dirty code. Please
clarify whether you've explicitly considered this aspect when making the
suggestion (I did when deciding to use #ifdef-s).
Also, to make the changes reasonable to spot, that would be a separate
patch then.
> If not, can we at least correct gmfn to mfn here and in the prototype?
Hmm, that would incur further changes, which I'd prefer to avoid at this
point: The function already has a local variable named "mfn" (as can be
seen in context above).
I also don't see how this request is related to the question of moving
the function.
> Alternatively, do we want to start putting things like this in a real
> library so we can have the toolchain figure this out automatically?
I wouldn't want to go this far with a function like this one. To me it
doesn't feel like code which is actually
>> /* Invalid pages can't be dirty. */
>> if ( unlikely(!VALID_M2P(pfn_x(pfn))) )
>> - return 0;
>> + return false;
>>
>> mfn = d->arch.paging.log_dirty.top;
>> if ( !mfn_valid(mfn) )
>
> These don't need to be mfn_valid(). They can be checks against
> MFN_INVALID instead, because the logdirty bitmap is a Xen internal
> structure.
>
> In response to your comment in the previous patch, this would
> substantially reduce the overhead of paging_mark_pfn_dirty() and
> paging_mfn_is_dirty().
May I suggest that the conversion from mfn_valid() be a separate
topic and (set of) change(s)? I'd be happy to add a further patch
here to at least deal with its unnecessary uses on log_dirty.top
and alike. Of course such a change won't alter the "moderately
expensive" comment in the earlier patch - it'll be less expensive
then, but still not cheap. Even less so as soon as
map_domain_page() loses its shortcut in release builds (when the
direct map has disappeared).
Jan
