[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/paging: tidy paging_mfn_is_dirty()
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |