| Message ID | 20210701095635.15648-13-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leftover from 2020 | expand |
On 01/07/2021 10:56, Olaf Hering wrote: > Introduce a helper which decides if a given pfn in the migration > stream is backed by memory. > > This specifically deals with type XEN_DOMCTL_PFINFO_XALLOC, which was > a synthetic toolstack-only type used in Xen 4.2 to 4.5. It indicated a > dirty page on the sending side for which no data will be send in the > initial iteration. > > No change in behavior intended. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > --- > tools/libs/saverestore/common.h | 17 +++++++++++++++++ > tools/libs/saverestore/restore.c | 5 ++--- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/tools/libs/saverestore/common.h b/tools/libs/saverestore/common.h > index 07c506360c..fa242e808d 100644 > --- a/tools/libs/saverestore/common.h > +++ b/tools/libs/saverestore/common.h > @@ -500,6 +500,23 @@ static inline bool sr_is_known_page_type(xen_pfn_t type) > return ret; > } > > +static inline bool page_type_to_populate(uint32_t type) > +{ > + bool ret; > + > + switch (type) > + { Same style comments as before. > + case XEN_DOMCTL_PFINFO_XTAB: > + case XEN_DOMCTL_PFINFO_BROKEN: > + ret = false; > + break; > + case XEN_DOMCTL_PFINFO_XALLOC: > + default: > + ret = true; > + break; I know you're replacing the logic as-was, but in hindsight, I'm not sure it was great to begin with. It defaults the unallocated types to being considered populated, which isn't a clever idea. Anyone adding a new page type is going to have to audit/edit each of these helpers. I think it would be better to write all the true cases explicitly. > + } > + return ret; > +} > #endif > /* > * Local variables: > diff --git a/tools/libs/saverestore/restore.c b/tools/libs/saverestore/restore.c > index 324b9050e2..477b7527a1 100644 > --- a/tools/libs/saverestore/restore.c > +++ b/tools/libs/saverestore/restore.c > @@ -152,9 +152,8 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, > > for ( i = 0; i < count; ++i ) > { > - if ( (!types || (types && > - (types[i] != XEN_DOMCTL_PFINFO_XTAB && > - types[i] != XEN_DOMCTL_PFINFO_BROKEN))) && > + if ( (!types || > + (types && page_type_to_populate(types[i]) == true)) && I'm surprised not to have seen a compiler or static analysis complaint about this. !A || (A && B) is redundant, and simplifies to !A || B. Clearly need to blame whichever numpty wrote this code to begin with. ~Andrew
Am Fri, 2 Jul 2021 20:43:13 +0100 schrieb Andrew Cooper <andrew.cooper3@citrix.com>: > Anyone adding a new page type is going to have to audit/edit each of > these helpers. I think it would be better to write all the true cases > explicitly. You mean the check if a page has data or needs to be populated should look like sr_is_known_page_type, where each known variant is listed? Olaf
On 05/07/2021 09:59, Olaf Hering wrote: > Am Fri, 2 Jul 2021 20:43:13 +0100 > schrieb Andrew Cooper <andrew.cooper3@citrix.com>: > >> Anyone adding a new page type is going to have to audit/edit each of >> these helpers. I think it would be better to write all the true cases >> explicitly. > You mean the check if a page has data or needs to be populated should look like sr_is_known_page_type, where each known variant is listed? Yes. I think that is a safer approach overall. ~Andrew
On 01/07/2021 10:56, Olaf Hering wrote: > Introduce a helper which decides if a given pfn in the migration > stream is backed by memory. > > This specifically deals with type XEN_DOMCTL_PFINFO_XALLOC, which was > a synthetic toolstack-only type used in Xen 4.2 to 4.5. It indicated a > dirty page on the sending side for which no data will be send in the > initial iteration. What do you mean "This specifically deals with" ? AFACT, the code before was correct. ~Andrew
On Mon, Jul 05, Andrew Cooper wrote:
> What do you mean "This specifically deals with" ?
This was a result from Jürgen pointing out that XEN_DOMCTL_PFINFO_XALLOC
is not handled. If all the type checking changes go into a single
commit, the commig message has to be reworded.
Olaf
On 05/07/2021 14:53, Olaf Hering wrote: > On Mon, Jul 05, Andrew Cooper wrote: > >> What do you mean "This specifically deals with" ? > This was a result from Jürgen pointing out that XEN_DOMCTL_PFINFO_XALLOC > is not handled. But it is... Before the patch, we only don't populate for XTAB or BROKEN. We populate for every other type, including the unknown/invalid ones. There is no change in behaviour (for the non-invalid cases) that I can see. ~Andrew
Am Mon, 5 Jul 2021 19:54:21 +0100
schrieb Andrew Cooper <andrew.cooper3@citrix.com>:
> But it is...
It was not handled in an earlier variant of the patch. Meanwhile the original behavior is restored with the current variant.
Olaf
diff --git a/tools/libs/saverestore/common.h b/tools/libs/saverestore/common.h index 07c506360c..fa242e808d 100644 --- a/tools/libs/saverestore/common.h +++ b/tools/libs/saverestore/common.h @@ -500,6 +500,23 @@ static inline bool sr_is_known_page_type(xen_pfn_t type) return ret; } +static inline bool page_type_to_populate(uint32_t type) +{ + bool ret; + + switch (type) + { + case XEN_DOMCTL_PFINFO_XTAB: + case XEN_DOMCTL_PFINFO_BROKEN: + ret = false; + break; + case XEN_DOMCTL_PFINFO_XALLOC: + default: + ret = true; + break; + } + return ret; +} #endif /* * Local variables: diff --git a/tools/libs/saverestore/restore.c b/tools/libs/saverestore/restore.c index 324b9050e2..477b7527a1 100644 --- a/tools/libs/saverestore/restore.c +++ b/tools/libs/saverestore/restore.c @@ -152,9 +152,8 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, for ( i = 0; i < count; ++i ) { - if ( (!types || (types && - (types[i] != XEN_DOMCTL_PFINFO_XTAB && - types[i] != XEN_DOMCTL_PFINFO_BROKEN))) && + if ( (!types || + (types && page_type_to_populate(types[i]) == true)) && !pfn_is_populated(ctx, original_pfns[i]) ) { rc = pfn_set_populated(ctx, original_pfns[i]);
Introduce a helper which decides if a given pfn in the migration stream is backed by memory. This specifically deals with type XEN_DOMCTL_PFINFO_XALLOC, which was a synthetic toolstack-only type used in Xen 4.2 to 4.5. It indicated a dirty page on the sending side for which no data will be send in the initial iteration. No change in behavior intended. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- tools/libs/saverestore/common.h | 17 +++++++++++++++++ tools/libs/saverestore/restore.c | 5 ++--- 2 files changed, 19 insertions(+), 3 deletions(-)