cr-service: add lazy-pages RPC feature check

Submitted by Adrian Reber on Feb. 16, 2017, 11 a.m.

Details

Message ID 20170216110026.6590-1-adrian@lisas.de
State New
Series "cr-service: add lazy-pages RPC feature check"
Headers show

Commit Message

Adrian Reber Feb. 16, 2017, 11 a.m.
From: Adrian Reber <areber@redhat.com>

Extend the RPC feature check functionality to also test for lazy-pages
support. This does not check for certain UFFD features (yet). Right now
it only checks if kerndat_uffd() returns a least one UFFD feature.

Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/cr-service.c | 39 ++++++++++++++++++++++++++-------------
 images/rpc.proto  |  1 +
 2 files changed, 27 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-service.c b/criu/cr-service.c
index ff5a7a8..3e15e5f 100644
--- a/criu/cr-service.c
+++ b/criu/cr-service.c
@@ -829,15 +829,12 @@  static int handle_feature_check(int sk, CriuReq * msg)
 	/* enable setting of an optional message */
 	feat.has_mem_track = 1;
 	feat.mem_track = false;
+	feat.has_lazy_pages = 1;
+	feat.lazy_pages = false;
 
-	/*
-	 * Check if the requested feature check can be answered.
-	 *
-	 * This function is right now hard-coded to memory
-	 * tracking detection and needs other/better logic to
-	 * handle multiple feature checks.
-	 */
-	if (msg->features->has_mem_track != 1) {
+	/* Check if the requested feature check can be answered. */
+	if ((msg->features->has_mem_track != 1) ||
+	    (msg->features->has_lazy_pages != 1)) {
 		pr_warn("Feature checking for unknown feature.\n");
 		goto out;
 	}
@@ -850,6 +847,9 @@  static int handle_feature_check(int sk, CriuReq * msg)
 	 */
 	success = true;
 
+#define HAS_MEM_TRACK (1<<0)
+#define HAS_LAZY_PAGES (1<<1)
+
 	pid = fork();
 	if (pid < 0) {
 		pr_perror("Can't fork");
@@ -857,23 +857,36 @@  static int handle_feature_check(int sk, CriuReq * msg)
 	}
 
 	if (pid == 0) {
-		int ret = 1;
+		int rc = -1;
+		int ret = 0;
 
 		setproctitle("feature-check --rpc");
 
-		kerndat_get_dirty_track();
+		if ((msg->features->has_mem_track == 1) &&
+		    (msg->features->mem_track == true))
+			kerndat_get_dirty_track();
 
 		if (kdat.has_dirty_track)
-			ret = 0;
+			ret |= HAS_MEM_TRACK;
+
+		if ((msg->features->has_lazy_pages == 1) &&
+		    (msg->features->lazy_pages == true))
+			rc = kerndat_uffd(true);
+
+		if ((rc != -1) && kdat.uffd_features)
+			ret |= HAS_LAZY_PAGES;
 
 		exit(ret);
 	}
 
 	wait(&status);
-	if (!WIFEXITED(status) || WEXITSTATUS(status))
+	if (!WIFEXITED(status))
 		goto out;
 
-	feat.mem_track = true;
+	if (WEXITSTATUS(status) == (HAS_MEM_TRACK & 0xff))
+		feat.mem_track = true;
+	if (WEXITSTATUS(status) == (HAS_LAZY_PAGES & 0xff))
+		feat.lazy_pages = true;
 out:
 	resp.features = &feat;
 	resp.type = msg->type;
diff --git a/images/rpc.proto b/images/rpc.proto
index f924b73..f894ae1 100644
--- a/images/rpc.proto
+++ b/images/rpc.proto
@@ -148,6 +148,7 @@  enum criu_req_type {
  */
 message criu_features {
 	optional bool			mem_track	= 1;
+	optional bool			lazy_pages	= 2;
 }
 
 /*

Comments

Adrian Reber Feb. 16, 2017, 12:40 p.m.
On Thu, Feb 16, 2017 at 12:12:08PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: cr-service: add lazy-pages RPC feature check
> URL   : https://patchwork.criu.org/series/1252/
> State : failure
> 
> == Logs ==
> 
> For more details see: https://travis-ci.org/criupatchwork/criu/builds/202207001

ERROR: Failed to create usr/libexec/gcc/x86_64-alpine-linux-musl/5.3.0/cc1: No space left on device

This does not seem to be patch related.

		Adrian
Andrey Vagin Feb. 17, 2017, 1:36 a.m.
On Thu, Feb 16, 2017 at 12:00:26PM +0100, Adrian Reber wrote:
> From: Adrian Reber <areber@redhat.com>
> 
> Extend the RPC feature check functionality to also test for lazy-pages
> support. This does not check for certain UFFD features (yet). Right now
> it only checks if kerndat_uffd() returns a least one UFFD feature.
> 
> Signed-off-by: Adrian Reber <areber@redhat.com>
> ---
>  criu/cr-service.c | 39 ++++++++++++++++++++++++++-------------
>  images/rpc.proto  |  1 +
>  2 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index ff5a7a8..3e15e5f 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -829,15 +829,12 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  	/* enable setting of an optional message */
>  	feat.has_mem_track = 1;
>  	feat.mem_track = false;
> +	feat.has_lazy_pages = 1;
> +	feat.lazy_pages = false;
>  
> -	/*
> -	 * Check if the requested feature check can be answered.
> -	 *
> -	 * This function is right now hard-coded to memory
> -	 * tracking detection and needs other/better logic to
> -	 * handle multiple feature checks.
> -	 */
> -	if (msg->features->has_mem_track != 1) {
> +	/* Check if the requested feature check can be answered. */
> +	if ((msg->features->has_mem_track != 1) ||
> +	    (msg->features->has_lazy_pages != 1)) {
>  		pr_warn("Feature checking for unknown feature.\n");
>  		goto out;
>  	}
> @@ -850,6 +847,9 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  	 */
>  	success = true;
>  
> +#define HAS_MEM_TRACK (1<<0)
> +#define HAS_LAZY_PAGES (1<<1)
> +
>  	pid = fork();
>  	if (pid < 0) {
>  		pr_perror("Can't fork");
> @@ -857,23 +857,36 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  	}
>  
>  	if (pid == 0) {
> -		int ret = 1;
> +		int rc = -1;
> +		int ret = 0;
>  
>  		setproctitle("feature-check --rpc");
>  
> -		kerndat_get_dirty_track();
> +		if ((msg->features->has_mem_track == 1) &&
> +		    (msg->features->mem_track == true))
> +			kerndat_get_dirty_track();
>  
>  		if (kdat.has_dirty_track)
> -			ret = 0;
> +			ret |= HAS_MEM_TRACK;
> +
> +		if ((msg->features->has_lazy_pages == 1) &&
> +		    (msg->features->lazy_pages == true))
> +			rc = kerndat_uffd(true);
> +
> +		if ((rc != -1) && kdat.uffd_features)
> +			ret |= HAS_LAZY_PAGES;
>  
>  		exit(ret);
>  	}
>  
>  	wait(&status);
> -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> +	if (!WIFEXITED(status))
>  		goto out;
>  
> -	feat.mem_track = true;
> +	if (WEXITSTATUS(status) == (HAS_MEM_TRACK & 0xff))
> +		feat.mem_track = true;
> +	if (WEXITSTATUS(status) == (HAS_LAZY_PAGES & 0xff))
> +		feat.lazy_pages = true;

I think something wrong here ^^^^. This code can not set feat.mem_track
and feat.lazy_pages together.

maybe you mean something like this:
status = WEXITSTATUS(status);

if (status & HAS_MEM_TRACK)
	feat.mem_track = true;
if (status & HAS_LAZY_PAGES)
	feat.lazy_pages = true;

I don't like a game with a process exit code. We don't check exit codes
of kerndat_uffd and kerndat_get_dirty_track, but I think we should.

Why do we need to create a new process to call these functions?

>  out:
>  	resp.features = &feat;
>  	resp.type = msg->type;
> diff --git a/images/rpc.proto b/images/rpc.proto
> index f924b73..f894ae1 100644
> --- a/images/rpc.proto
> +++ b/images/rpc.proto
> @@ -148,6 +148,7 @@ enum criu_req_type {
>   */
>  message criu_features {
>  	optional bool			mem_track	= 1;
> +	optional bool			lazy_pages	= 2;
>  }
>  
>  /*
> -- 
> 2.9.3
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Adrian Reber Feb. 17, 2017, 6:39 a.m.
On Thu, Feb 16, 2017 at 05:36:05PM -0800, Andrei Vagin wrote:
> On Thu, Feb 16, 2017 at 12:00:26PM +0100, Adrian Reber wrote:
> > From: Adrian Reber <areber@redhat.com>
> > 
> > Extend the RPC feature check functionality to also test for lazy-pages
> > support. This does not check for certain UFFD features (yet). Right now
> > it only checks if kerndat_uffd() returns a least one UFFD feature.
> > 
> > Signed-off-by: Adrian Reber <areber@redhat.com>
> > ---
> >  criu/cr-service.c | 39 ++++++++++++++++++++++++++-------------
> >  images/rpc.proto  |  1 +
> >  2 files changed, 27 insertions(+), 13 deletions(-)
> > 
> > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > index ff5a7a8..3e15e5f 100644
> > --- a/criu/cr-service.c
> > +++ b/criu/cr-service.c
> > @@ -829,15 +829,12 @@ static int handle_feature_check(int sk, CriuReq * msg)
> >  	/* enable setting of an optional message */
> >  	feat.has_mem_track = 1;
> >  	feat.mem_track = false;
> > +	feat.has_lazy_pages = 1;
> > +	feat.lazy_pages = false;
> >  
> > -	/*
> > -	 * Check if the requested feature check can be answered.
> > -	 *
> > -	 * This function is right now hard-coded to memory
> > -	 * tracking detection and needs other/better logic to
> > -	 * handle multiple feature checks.
> > -	 */
> > -	if (msg->features->has_mem_track != 1) {
> > +	/* Check if the requested feature check can be answered. */
> > +	if ((msg->features->has_mem_track != 1) ||
> > +	    (msg->features->has_lazy_pages != 1)) {
> >  		pr_warn("Feature checking for unknown feature.\n");
> >  		goto out;
> >  	}
> > @@ -850,6 +847,9 @@ static int handle_feature_check(int sk, CriuReq * msg)
> >  	 */
> >  	success = true;
> >  
> > +#define HAS_MEM_TRACK (1<<0)
> > +#define HAS_LAZY_PAGES (1<<1)
> > +
> >  	pid = fork();
> >  	if (pid < 0) {
> >  		pr_perror("Can't fork");
> > @@ -857,23 +857,36 @@ static int handle_feature_check(int sk, CriuReq * msg)
> >  	}
> >  
> >  	if (pid == 0) {
> > -		int ret = 1;
> > +		int rc = -1;
> > +		int ret = 0;
> >  
> >  		setproctitle("feature-check --rpc");
> >  
> > -		kerndat_get_dirty_track();
> > +		if ((msg->features->has_mem_track == 1) &&
> > +		    (msg->features->mem_track == true))
> > +			kerndat_get_dirty_track();
> >  
> >  		if (kdat.has_dirty_track)
> > -			ret = 0;
> > +			ret |= HAS_MEM_TRACK;
> > +
> > +		if ((msg->features->has_lazy_pages == 1) &&
> > +		    (msg->features->lazy_pages == true))
> > +			rc = kerndat_uffd(true);
> > +
> > +		if ((rc != -1) && kdat.uffd_features)
> > +			ret |= HAS_LAZY_PAGES;
> >  
> >  		exit(ret);
> >  	}
> >  
> >  	wait(&status);
> > -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > +	if (!WIFEXITED(status))
> >  		goto out;
> >  
> > -	feat.mem_track = true;
> > +	if (WEXITSTATUS(status) == (HAS_MEM_TRACK & 0xff))
> > +		feat.mem_track = true;
> > +	if (WEXITSTATUS(status) == (HAS_LAZY_PAGES & 0xff))
> > +		feat.lazy_pages = true;
> 
> I think something wrong here ^^^^. This code can not set feat.mem_track
> and feat.lazy_pages together.

Hmm, I am testing from runc and it seems to work:

New kernel, new CRIU:

DEBU[0000] RPC response from CRIU: FEATURE_CHECK
DEBU[0000] Feature check says: mem_track:true lazy_pages:true

Older kernel, new CRIU:

DEBU[0000] RPC response from CRIU: FEATURE_CHECK
DEBU[0000] Feature check says: mem_track:false lazy_pages:false

New kernel, old CRIU:

DEBU[0000] RPC response from CRIU: FEATURE_CHECK
DEBU[0000] Feature check says: mem_track:true

So it kind of works. I have not tested a system without UFFD but with
dirty tracking.

> maybe you mean something like this:
> status = WEXITSTATUS(status);
> 
> if (status & HAS_MEM_TRACK)
> 	feat.mem_track = true;
> if (status & HAS_LAZY_PAGES)
> 	feat.lazy_pages = true;

Good idea, indeed.

> I don't like a game with a process exit code. We don't check exit codes
> of kerndat_uffd and kerndat_get_dirty_track, but I think we should.

Will add the return code checking.

> Why do we need to create a new process to call these functions?

That is a good question. I was just going with what already existed. I
also was not happy with the return code magic but that is what the
CPU_FEATURE_CHECK also does and my original version of the FEATURE_CHECK.

Pavel, do you know why we fork an additional CRIU process instead of
just doing the feature check directly?

		Adrian
Andrey Vagin Feb. 17, 2017, 6:33 p.m.
On Fri, Feb 17, 2017 at 07:39:17AM +0100, Adrian Reber wrote:
> On Thu, Feb 16, 2017 at 05:36:05PM -0800, Andrei Vagin wrote:
> > On Thu, Feb 16, 2017 at 12:00:26PM +0100, Adrian Reber wrote:
> > > From: Adrian Reber <areber@redhat.com>
> > > 
> > > Extend the RPC feature check functionality to also test for lazy-pages
> > > support. This does not check for certain UFFD features (yet). Right now
> > > it only checks if kerndat_uffd() returns a least one UFFD feature.
> > > 
> > > Signed-off-by: Adrian Reber <areber@redhat.com>
> > > ---
> > >  criu/cr-service.c | 39 ++++++++++++++++++++++++++-------------
> > >  images/rpc.proto  |  1 +
> > >  2 files changed, 27 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > > index ff5a7a8..3e15e5f 100644
> > > --- a/criu/cr-service.c
> > > +++ b/criu/cr-service.c
> > > @@ -829,15 +829,12 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > >  	/* enable setting of an optional message */
> > >  	feat.has_mem_track = 1;
> > >  	feat.mem_track = false;
> > > +	feat.has_lazy_pages = 1;
> > > +	feat.lazy_pages = false;
> > >  
> > > -	/*
> > > -	 * Check if the requested feature check can be answered.
> > > -	 *
> > > -	 * This function is right now hard-coded to memory
> > > -	 * tracking detection and needs other/better logic to
> > > -	 * handle multiple feature checks.
> > > -	 */
> > > -	if (msg->features->has_mem_track != 1) {
> > > +	/* Check if the requested feature check can be answered. */
> > > +	if ((msg->features->has_mem_track != 1) ||
> > > +	    (msg->features->has_lazy_pages != 1)) {
> > >  		pr_warn("Feature checking for unknown feature.\n");
> > >  		goto out;
> > >  	}
> > > @@ -850,6 +847,9 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > >  	 */
> > >  	success = true;
> > >  
> > > +#define HAS_MEM_TRACK (1<<0)
> > > +#define HAS_LAZY_PAGES (1<<1)
> > > +
> > >  	pid = fork();
> > >  	if (pid < 0) {
> > >  		pr_perror("Can't fork");
> > > @@ -857,23 +857,36 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > >  	}
> > >  
> > >  	if (pid == 0) {
> > > -		int ret = 1;
> > > +		int rc = -1;
> > > +		int ret = 0;
> > >  
> > >  		setproctitle("feature-check --rpc");
> > >  
> > > -		kerndat_get_dirty_track();
> > > +		if ((msg->features->has_mem_track == 1) &&
> > > +		    (msg->features->mem_track == true))
> > > +			kerndat_get_dirty_track();
> > >  
> > >  		if (kdat.has_dirty_track)
> > > -			ret = 0;
> > > +			ret |= HAS_MEM_TRACK;
> > > +
> > > +		if ((msg->features->has_lazy_pages == 1) &&
> > > +		    (msg->features->lazy_pages == true))
> > > +			rc = kerndat_uffd(true);
> > > +
> > > +		if ((rc != -1) && kdat.uffd_features)
> > > +			ret |= HAS_LAZY_PAGES;
> > >  
> > >  		exit(ret);
> > >  	}
> > >  
> > >  	wait(&status);
> > > -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > > +	if (!WIFEXITED(status))
> > >  		goto out;
> > >  
> > > -	feat.mem_track = true;
> > > +	if (WEXITSTATUS(status) == (HAS_MEM_TRACK & 0xff))
> > > +		feat.mem_track = true;
> > > +	if (WEXITSTATUS(status) == (HAS_LAZY_PAGES & 0xff))
> > > +		feat.lazy_pages = true;
> > 
> > I think something wrong here ^^^^. This code can not set feat.mem_track
> > and feat.lazy_pages together.
> 
> Hmm, I am testing from runc and it seems to work:
> 
> New kernel, new CRIU:
> 
> DEBU[0000] RPC response from CRIU: FEATURE_CHECK
> DEBU[0000] Feature check says: mem_track:true lazy_pages:true
> 
> Older kernel, new CRIU:
> 
> DEBU[0000] RPC response from CRIU: FEATURE_CHECK
> DEBU[0000] Feature check says: mem_track:false lazy_pages:false
> 
> New kernel, old CRIU:
> 
> DEBU[0000] RPC response from CRIU: FEATURE_CHECK
> DEBU[0000] Feature check says: mem_track:true
> 
> So it kind of works. I have not tested a system without UFFD but with
> dirty tracking.

Hmmm. It works if you check features separately, doesn't it?

because if WEXITSTATUS(status) is equal to (HAS_MEM_TRACK & 0xff))
it can't be equal to (HAS_LAZY_PAGES & 0xff) at the same moment.

> > > +         feat.mem_track = true;
> 
> > maybe you mean something like this:
> > status = WEXITSTATUS(status);
> > 
> > if (status & HAS_MEM_TRACK)
> > 	feat.mem_track = true;
> > if (status & HAS_LAZY_PAGES)
> > 	feat.lazy_pages = true;
> 
> Good idea, indeed.
> 
> > I don't like a game with a process exit code. We don't check exit codes
> > of kerndat_uffd and kerndat_get_dirty_track, but I think we should.
> 
> Will add the return code checking.
> 
> > Why do we need to create a new process to call these functions?
> 
> That is a good question. I was just going with what already existed. I
> also was not happy with the return code magic but that is what the
> CPU_FEATURE_CHECK also does and my original version of the FEATURE_CHECK.
> 
> Pavel, do you know why we fork an additional CRIU process instead of
> just doing the feature check directly?

> 
> 		Adrian
Adrian Reber Feb. 17, 2017, 9:31 p.m.
On Fri, Feb 17, 2017 at 10:33:04AM -0800, Andrei Vagin wrote:
> On Fri, Feb 17, 2017 at 07:39:17AM +0100, Adrian Reber wrote:
> > On Thu, Feb 16, 2017 at 05:36:05PM -0800, Andrei Vagin wrote:
> > > On Thu, Feb 16, 2017 at 12:00:26PM +0100, Adrian Reber wrote:
> > > > From: Adrian Reber <areber@redhat.com>
> > > > 
> > > > Extend the RPC feature check functionality to also test for lazy-pages
> > > > support. This does not check for certain UFFD features (yet). Right now
> > > > it only checks if kerndat_uffd() returns a least one UFFD feature.
> > > > 
> > > > Signed-off-by: Adrian Reber <areber@redhat.com>
> > > > ---
> > > >  criu/cr-service.c | 39 ++++++++++++++++++++++++++-------------
> > > >  images/rpc.proto  |  1 +
> > > >  2 files changed, 27 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > > > index ff5a7a8..3e15e5f 100644
> > > > --- a/criu/cr-service.c
> > > > +++ b/criu/cr-service.c
> > > > @@ -829,15 +829,12 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > > >  	/* enable setting of an optional message */
> > > >  	feat.has_mem_track = 1;
> > > >  	feat.mem_track = false;
> > > > +	feat.has_lazy_pages = 1;
> > > > +	feat.lazy_pages = false;
> > > >  
> > > > -	/*
> > > > -	 * Check if the requested feature check can be answered.
> > > > -	 *
> > > > -	 * This function is right now hard-coded to memory
> > > > -	 * tracking detection and needs other/better logic to
> > > > -	 * handle multiple feature checks.
> > > > -	 */
> > > > -	if (msg->features->has_mem_track != 1) {
> > > > +	/* Check if the requested feature check can be answered. */
> > > > +	if ((msg->features->has_mem_track != 1) ||
> > > > +	    (msg->features->has_lazy_pages != 1)) {
> > > >  		pr_warn("Feature checking for unknown feature.\n");
> > > >  		goto out;
> > > >  	}
> > > > @@ -850,6 +847,9 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > > >  	 */
> > > >  	success = true;
> > > >  
> > > > +#define HAS_MEM_TRACK (1<<0)
> > > > +#define HAS_LAZY_PAGES (1<<1)
> > > > +
> > > >  	pid = fork();
> > > >  	if (pid < 0) {
> > > >  		pr_perror("Can't fork");
> > > > @@ -857,23 +857,36 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > > >  	}
> > > >  
> > > >  	if (pid == 0) {
> > > > -		int ret = 1;
> > > > +		int rc = -1;
> > > > +		int ret = 0;
> > > >  
> > > >  		setproctitle("feature-check --rpc");
> > > >  
> > > > -		kerndat_get_dirty_track();
> > > > +		if ((msg->features->has_mem_track == 1) &&
> > > > +		    (msg->features->mem_track == true))
> > > > +			kerndat_get_dirty_track();
> > > >  
> > > >  		if (kdat.has_dirty_track)
> > > > -			ret = 0;
> > > > +			ret |= HAS_MEM_TRACK;
> > > > +
> > > > +		if ((msg->features->has_lazy_pages == 1) &&
> > > > +		    (msg->features->lazy_pages == true))
> > > > +			rc = kerndat_uffd(true);
> > > > +
> > > > +		if ((rc != -1) && kdat.uffd_features)
> > > > +			ret |= HAS_LAZY_PAGES;
> > > >  
> > > >  		exit(ret);
> > > >  	}
> > > >  
> > > >  	wait(&status);
> > > > -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > > > +	if (!WIFEXITED(status))
> > > >  		goto out;
> > > >  
> > > > -	feat.mem_track = true;
> > > > +	if (WEXITSTATUS(status) == (HAS_MEM_TRACK & 0xff))
> > > > +		feat.mem_track = true;
> > > > +	if (WEXITSTATUS(status) == (HAS_LAZY_PAGES & 0xff))
> > > > +		feat.lazy_pages = true;
> > > 
> > > I think something wrong here ^^^^. This code can not set feat.mem_track
> > > and feat.lazy_pages together.
> > 
> > Hmm, I am testing from runc and it seems to work:
> > 
> > New kernel, new CRIU:
> > 
> > DEBU[0000] RPC response from CRIU: FEATURE_CHECK
> > DEBU[0000] Feature check says: mem_track:true lazy_pages:true
> > 
> > Older kernel, new CRIU:
> > 
> > DEBU[0000] RPC response from CRIU: FEATURE_CHECK
> > DEBU[0000] Feature check says: mem_track:false lazy_pages:false
> > 
> > New kernel, old CRIU:
> > 
> > DEBU[0000] RPC response from CRIU: FEATURE_CHECK
> > DEBU[0000] Feature check says: mem_track:true
> > 
> > So it kind of works. I have not tested a system without UFFD but with
> > dirty tracking.
> 
> Hmmm. It works if you check features separately, doesn't it?
> 
> because if WEXITSTATUS(status) is equal to (HAS_MEM_TRACK & 0xff))
> it can't be equal to (HAS_LAZY_PAGES & 0xff) at the same moment.

Yes, you are right. That is of course wrong. I wait for some insights
why the code fork()-s for feature check and maybe I could then remove
the whole return code magic. I will send a new patch once I understand
why it is the way it is.

		Adrian
Pavel Emelyanov Feb. 20, 2017, 9:50 a.m.
On 02/17/2017 09:39 AM, Adrian Reber wrote:

> Pavel, do you know why we fork an additional CRIU process instead of
> just doing the feature check directly?

We did this because a) cr-service can be a long-running service if keep-open
option is set and b) checking features is implemented as sub-routines of
criu check command and none of them was verified to clean after itself. So
not to leak any resources on RPC check request the exact routines are called
in a subprocess.

-- Pavel
Andrey Vagin Feb. 20, 2017, 7:08 p.m.
On Mon, Feb 20, 2017 at 12:50:48PM +0300, Pavel Emelyanov wrote:
> On 02/17/2017 09:39 AM, Adrian Reber wrote:
> 
> > Pavel, do you know why we fork an additional CRIU process instead of
> > just doing the feature check directly?
> 
> We did this because a) cr-service can be a long-running service if keep-open
> option is set and b) checking features is implemented as sub-routines of
> criu check command and none of them was verified to clean after itself. So
> not to leak any resources on RPC check request the exact routines are called
> in a subprocess.

Adrian, I think we can send a response from this process.

> 
> -- Pavel
>