Re: [PATCH 2/2] x86/paging: tidy paging_mfn_is_dirty()

[Jan Beulich <jbeulich@xxxxxxxx>]

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