[3/6] criu: pagemap: replace 'zero' and 'lazy' booleans with 'flags'

Submitted by Mike Rapoport on Sept. 8, 2016, 7:39 a.m.

Details

Message ID 1473320352-28669-4-git-send-email-rppt@linux.vnet.ibm.com
State Rejected
Series "criu: pagemap: improve accuracy of lazy pages"
Headers show

Commit Message

Mike Rapoport Sept. 8, 2016, 7:39 a.m.
Having three booleans in pagemap entry clues for usage of good old flags.
Replace 'zero' and 'lazy' booleans with flags and use flags for internal
tracking of in_parent value. Eventually, in_parent may be deprecated.

Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
---
 criu/cr-dedup.c        |  2 +-
 criu/include/pagemap.h | 21 +++++++++++++++++++++
 criu/page-xfer.c       | 12 +++++-------
 criu/pagemap.c         | 20 ++++++++++++++------
 criu/uffd.c            |  2 +-
 images/pagemap.proto   |  3 +--
 6 files changed, 43 insertions(+), 17 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-dedup.c b/criu/cr-dedup.c
index cc44152..745b17b 100644
--- a/criu/cr-dedup.c
+++ b/criu/cr-dedup.c
@@ -88,7 +88,7 @@  static int cr_dedup_one_pagemap(int id, int flags)
 			goto exit;
 
 		pr_debug("dedup iovec base=%p, len=%zu\n", iov.iov_base, iov.iov_len);
-		if (!pr.pe->in_parent) {
+		if (!pagemap_in_parent(pr.pe)) {
 			ret = dedup_one_iovec(prp, &iov);
 			if (ret)
 				goto exit;
diff --git a/criu/include/pagemap.h b/criu/include/pagemap.h
index f45b2a9..f4a9285 100644
--- a/criu/include/pagemap.h
+++ b/criu/include/pagemap.h
@@ -92,4 +92,25 @@  extern void pagemap2iovec(PagemapEntry *pe, struct iovec *iov);
 extern void iovec2pagemap(struct iovec *iov, PagemapEntry *pe);
 
 extern int dedup_one_iovec(struct page_read *pr, struct iovec *iov);
+
+/* Pagemap flags */
+#define PE_PARENT	(1 << 0)	/* pages are in parent snapshot */
+#define PE_ZERO		(1 << 1)	/* pages can be lazily restored */
+#define PE_LAZY		(1 << 2)	/* pages are mapped to zero pfn */
+
+static inline bool pagemap_in_parent(PagemapEntry *pe)
+{
+	return !!(pe->flags & PE_PARENT);
+}
+
+static inline bool pagemap_zero(PagemapEntry *pe)
+{
+	return !!(pe->flags & PE_ZERO);
+}
+
+static inline bool pagemap_lazy(PagemapEntry *pe)
+{
+	return !!(pe->flags & PE_LAZY);
+}
+
 #endif /* __CR_PAGE_READ_H__ */
diff --git a/criu/page-xfer.c b/criu/page-xfer.c
index cd07dee..a02ca3b 100644
--- a/criu/page-xfer.c
+++ b/criu/page-xfer.c
@@ -159,6 +159,7 @@  static int write_pagemap_loc(struct page_xfer *xfer,
 	PagemapEntry pe = PAGEMAP_ENTRY__INIT;
 
 	iovec2pagemap(iov, &pe);
+	pe.has_flags = true;
 	if (opts.auto_dedup && xfer->parent != NULL) {
 		ret = dedup_one_iovec(xfer->parent, iov);
 		if (ret == -1) {
@@ -234,6 +235,7 @@  static int write_hole_loc(struct page_xfer *xfer, struct iovec *iov, int type)
 	PagemapEntry pe = PAGEMAP_ENTRY__INIT;
 
 	iovec2pagemap(iov, &pe);
+	pe.has_flags = true;
 
 	switch (type) {
 	case PS_IOV_HOLE:
@@ -247,17 +249,13 @@  static int write_hole_loc(struct page_xfer *xfer, struct iovec *iov, int type)
 				return -1;
 			}
 		}
-
-		pe.has_in_parent = true;
-		pe.in_parent = true;
+		pe.flags |= PE_PARENT;
 		break;
 	case PS_IOV_ZERO:
-		pe.has_zero = true;
-		pe.zero = true;
+		pe.flags |= PE_ZERO;
 		break;
 	case PS_IOV_LAZY:
-		pe.has_lazy = true;
-		pe.lazy = true;
+		pe.flags |= PE_LAZY;
 		break;
 	default:
 		return -1;
diff --git a/criu/pagemap.c b/criu/pagemap.c
index 47c6788..aaef5a4 100644
--- a/criu/pagemap.c
+++ b/criu/pagemap.c
@@ -95,7 +95,7 @@  int dedup_one_iovec(struct page_read *pr, struct iovec *iov)
 			return -1;
 		pagemap2iovec(pr->pe, &piov);
 		piov_end = (unsigned long)piov.iov_base + piov.iov_len;
-		if (!pr->pe->in_parent) {
+		if (!pagemap_in_parent(pr->pe)) {
 			ret = punch_hole(pr, pr->pi_off, min(piov_end, iov_end) - off, false);
 			if (ret == -1)
 				return ret;
@@ -138,11 +138,11 @@  static int get_pagemap(struct page_read *pr, struct iovec *iov)
 	for (;;) {
 		if (!advance(pr))
 			return 0;
-		if (!pr->pe->zero)
+		if (!pagemap_zero(pr->pe))
 			break;
 	}
 
-	if (pr->pe->in_parent && !pr->parent) {
+	if (pagemap_in_parent(pr->pe) && !pr->parent) {
 		pr_err("No parent for snapshot pagemap\n");
 		return -1;
 	}
@@ -158,7 +158,7 @@  static void skip_pagemap_pages(struct page_read *pr, unsigned long len)
 		return;
 
 	pr_debug("\tpr%u Skip %lu bytes from page-dump\n", pr->id, len);
-	if (!pr->pe->in_parent && !pr->pe->zero && !pr->pe->lazy)
+	if (!pagemap_in_parent(pr->pe) && !pagemap_zero(pr->pe) && !pagemap_lazy(pr->pe))
 		pr->pi_off += len;
 	pr->cvaddr += len;
 }
@@ -204,7 +204,7 @@  static int read_pagemap_page(struct page_read *pr, unsigned long vaddr, int nr,
 	pr_info("pr%u Read %lx %u pages\n", pr->id, vaddr, nr);
 	pagemap_bound_check(pr->pe, vaddr, nr);
 
-	if (pr->pe->in_parent) {
+	if (pagemap_in_parent(pr->pe)) {
 		struct page_read *ppr = pr->parent;
 
 		/*
@@ -246,7 +246,7 @@  static int read_pagemap_page(struct page_read *pr, unsigned long vaddr, int nr,
 			vaddr += p_nr * PAGE_SIZE;
 			buf += p_nr * PAGE_SIZE;
 		} while (nr);
-	} else if (pr->pe->zero) {
+	} else if (pagemap_zero(pr->pe)) {
 		/* zero mappings should be skipped by get_pagemap */
 		BUG();
 	} else {
@@ -358,6 +358,12 @@  err_cl:
 	return -1;
 }
 
+static void init_compat_pagemap_entry(PagemapEntry *pe)
+{
+	if (pe->has_in_parent && pe->in_parent)
+		pe->flags |= PE_PARENT;
+}
+
 /*
  * The pagemap entry size is at least 8 bytes for small mappings with
  * low address and may get to 18 bytes or even more for large mappings
@@ -393,6 +399,8 @@  static int init_pagemaps(struct page_read *pr)
 		if (ret == 0)
 			break;
 
+		init_compat_pagemap_entry(pr->pmes[pr->nr_pmes]);
+
 		pr->nr_pmes++;
 		if (pr->nr_pmes >= nr_pmes) {
 			nr_pmes += nr_realloc;
diff --git a/criu/uffd.c b/criu/uffd.c
index ebb0e8e..81dc7ae 100644
--- a/criu/uffd.c
+++ b/criu/uffd.c
@@ -372,7 +372,7 @@  static int get_page(struct lazy_pages_info *lpi, unsigned long addr, void *dest)
 	if (ret <= 0)
 		return ret;
 
-	if (lpi->pr.pe->zero)
+	if (pagemap_zero(lpi->pr.pe))
 		return 0;
 
 	ret = lpi->pr.read_pages(&lpi->pr, addr, 1, buf);
diff --git a/images/pagemap.proto b/images/pagemap.proto
index 8b0092f..d5b3433 100644
--- a/images/pagemap.proto
+++ b/images/pagemap.proto
@@ -10,6 +10,5 @@  message pagemap_entry {
 	required uint64 vaddr		= 1 [(criu).hex = true];
 	required uint32 nr_pages	= 2;
 	optional bool	in_parent	= 3;
-	optional bool	zero		= 4;
-	optional bool	lazy		= 5;
+	optional uint32	flags		= 4 [(criu).hex = true];
 }

Comments

Dmitry Safonov Sept. 8, 2016, 12:33 p.m.
2016-09-08 10:39 GMT+03:00 Mike Rapoport <rppt@linux.vnet.ibm.com>:
> --- a/images/pagemap.proto
> +++ b/images/pagemap.proto
> @@ -10,6 +10,5 @@ message pagemap_entry {
>         required uint64 vaddr           = 1 [(criu).hex = true];
>         required uint32 nr_pages        = 2;
>         optional bool   in_parent       = 3;
> -       optional bool   zero            = 4;
> -       optional bool   lazy            = 5;
> +       optional uint32 flags           = 4 [(criu).hex = true];
>  }

Not sure that protobuf will correctly understand this change.
Maybe comment out zero and lazy fields, preserving their tag
number? And introduce flags with tag #6.
Also it's possible to preserve zero&lazy with names like
DEPRICATED_{zero,lazy} instead commenting them out.

Otherwise, point me to protobuf guide/manual if I'm wrong here.
Mike Rapoport Sept. 8, 2016, 1:13 p.m.
On Thu, Sep 08, 2016 at 03:33:58PM +0300, Dmitry Safonov wrote:
> 2016-09-08 10:39 GMT+03:00 Mike Rapoport <rppt@linux.vnet.ibm.com>:
> > --- a/images/pagemap.proto
> > +++ b/images/pagemap.proto
> > @@ -10,6 +10,5 @@ message pagemap_entry {
> >         required uint64 vaddr           = 1 [(criu).hex = true];
> >         required uint32 nr_pages        = 2;
> >         optional bool   in_parent       = 3;
> > -       optional bool   zero            = 4;
> > -       optional bool   lazy            = 5;
> > +       optional uint32 flags           = 4 [(criu).hex = true];
> >  }
> 
> Not sure that protobuf will correctly understand this change.
> Maybe comment out zero and lazy fields, preserving their tag
> number? And introduce flags with tag #6.
> Also it's possible to preserve zero&lazy with names like
> DEPRICATED_{zero,lazy} instead commenting them out.

I believe that since neither 'zero' nor 'lazy' never have been in master,
it's not a problem to break compatibility here and just replace 'zero' and
'lazy' with flags at this point in time.
 
> Otherwise, point me to protobuf guide/manual if I'm wrong here.
> 
> -- 
>              Dmitry
>

--
Sincerely yours,
Mike.
Dmitry Safonov Sept. 8, 2016, 1:48 p.m.
2016-09-08 16:13 GMT+03:00 Mike Rapoport <rppt@linux.vnet.ibm.com>:
> On Thu, Sep 08, 2016 at 03:33:58PM +0300, Dmitry Safonov wrote:
>> 2016-09-08 10:39 GMT+03:00 Mike Rapoport <rppt@linux.vnet.ibm.com>:
>> > --- a/images/pagemap.proto
>> > +++ b/images/pagemap.proto
>> > @@ -10,6 +10,5 @@ message pagemap_entry {
>> >         required uint64 vaddr           = 1 [(criu).hex = true];
>> >         required uint32 nr_pages        = 2;
>> >         optional bool   in_parent       = 3;
>> > -       optional bool   zero            = 4;
>> > -       optional bool   lazy            = 5;
>> > +       optional uint32 flags           = 4 [(criu).hex = true];
>> >  }
>>
>> Not sure that protobuf will correctly understand this change.
>> Maybe comment out zero and lazy fields, preserving their tag
>> number? And introduce flags with tag #6.
>> Also it's possible to preserve zero&lazy with names like
>> DEPRICATED_{zero,lazy} instead commenting them out.
>
> I believe that since neither 'zero' nor 'lazy' never have been in master,
> it's not a problem to break compatibility here and just replace 'zero' and
> 'lazy' with flags at this point in time.

Ok, right, thanks.
Pavel Emelianov Sept. 13, 2016, 2:59 p.m.
1-3 applied
Mike Rapoport Sept. 13, 2016, 3:07 p.m.
On Tue, Sep 13, 2016 at 05:59:07PM +0300, Pavel Emelyanov wrote:
> 1-3 applied
> 
what about 4?