[v2] cr-service: add lazy-pages RPC feature check

Submitted by Adrian Reber on Feb. 23, 2017, 10:17 a.m.

Details

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

Commit Message

Adrian Reber Feb. 23, 2017, 10:17 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 non-zero.

The RPC response is now transmitted from the forked process instead of
encoding all the results into the return code. The parent RPC process
now only sends an RPC message in the case of a failure.

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

Patch hide | download patch | download mbox

diff --git a/criu/cr-service.c b/criu/cr-service.c
index 169d637..d1007e7 100644
--- a/criu/cr-service.c
+++ b/criu/cr-service.c
@@ -823,33 +823,21 @@  static int handle_feature_check(int sk, CriuReq * msg)
 {
 	CriuResp resp = CRIU_RESP__INIT;
 	CriuFeatures feat = CRIU_FEATURES__INIT;
-	bool success = false;
 	int pid, status;
 
 	/* 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;
 	}
 
-	/*
-	 * From this point on the function will always
-	 * 'succeed'. If the requested features are supported
-	 * can be seen if the requested optional parameters are
-	 * set in the message 'criu_features'.
-	 */
-	success = true;
-
 	pid = fork();
 	if (pid < 0) {
 		pr_perror("Can't fork");
@@ -857,27 +845,72 @@  static int handle_feature_check(int sk, CriuReq * msg)
 	}
 
 	if (pid == 0) {
-		int ret = 1;
+		int ret;
 
 		setproctitle("feature-check --rpc");
 
-		kerndat_get_dirty_track();
+		if ((msg->features->has_mem_track == 1) &&
+		    (msg->features->mem_track == true)) {
+
+			feat.mem_track = true;
+			ret = kerndat_get_dirty_track();
+
+			if (ret)
+				feat.mem_track = false;
+
+			if (!kdat.has_dirty_track)
+				feat.mem_track = false;
+		}
+
+		if ((msg->features->has_lazy_pages == 1) &&
+		    (msg->features->lazy_pages == true)) {
+			ret = kerndat_uffd(true);
+
+			/*
+			 * Not checking for specific UFFD features yet.
+			 * If no error is returned it is probably
+			 * enough for basic UFFD functionality. This can
+			 * be extended in the future for a more detailed
+			 * UFFD feature check.
+			 */
+			if (ret)
+				feat.lazy_pages = false;
+			else
+				feat.lazy_pages = true;
+		}
 
-		if (kdat.has_dirty_track)
-			ret = 0;
+		resp.features = &feat;
+		resp.type = msg->type;
+		/* The feature check is working, actual results are in resp.features */
+		resp.success = true;
+
+		/*
+		 * If this point is reached the information about the features
+		 * is transmitted from the forked CRIU process (here).
+		 * If an error occured earlier, the feature check response will be
+		 * be send from the parent process.
+		 */
+		ret = send_criu_msg(sk, &resp);
 
 		exit(ret);
 	}
 
 	wait(&status);
-	if (!WIFEXITED(status) || WEXITSTATUS(status))
-		goto out;
+	if (WIFEXITED(status) && !WEXITSTATUS(status))
+		/*
+		 * The child process exited was able to send the answer.
+		 * Nothing more to do here.
+		 */
+		return 0;
 
-	feat.mem_track = true;
+	/*
+	 * The child process was not able to send an answer. Tell
+	 * the RPC client that something did not work as expected.
+	 */
 out:
 	resp.features = &feat;
 	resp.type = msg->type;
-	resp.success = success;
+	resp.success = false;
 
 	return send_criu_msg(sk, &resp);
 }
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

Andrey Vagin Feb. 27, 2017, 6:21 p.m.
On Thu, Feb 23, 2017 at 11:17:42AM +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 non-zero.
> 
> The RPC response is now transmitted from the forked process instead of
> encoding all the results into the return code. The parent RPC process
> now only sends an RPC message in the case of a failure.
> 
> Signed-off-by: Adrian Reber <areber@redhat.com>
> ---
>  criu/cr-service.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------
>  images/rpc.proto  |  1 +
>  2 files changed, 59 insertions(+), 25 deletions(-)
> 
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index 169d637..d1007e7 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -823,33 +823,21 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  {
>  	CriuResp resp = CRIU_RESP__INIT;
>  	CriuFeatures feat = CRIU_FEATURES__INIT;
> -	bool success = false;
>  	int pid, status;
>  
>  	/* 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;
>  	}
>  
> -	/*
> -	 * From this point on the function will always
> -	 * 'succeed'. If the requested features are supported
> -	 * can be seen if the requested optional parameters are
> -	 * set in the message 'criu_features'.
> -	 */
> -	success = true;
> -
>  	pid = fork();
>  	if (pid < 0) {
>  		pr_perror("Can't fork");
> @@ -857,27 +845,72 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  	}
>  
>  	if (pid == 0) {
> -		int ret = 1;
> +		int ret;
>  
>  		setproctitle("feature-check --rpc");
>  
> -		kerndat_get_dirty_track();
> +		if ((msg->features->has_mem_track == 1) &&
> +		    (msg->features->mem_track == true)) {
> +
> +			feat.mem_track = true;
> +			ret = kerndat_get_dirty_track();
> +
> +			if (ret)
> +				feat.mem_track = false;
> +
> +			if (!kdat.has_dirty_track)
> +				feat.mem_track = false;
> +		}
> +
> +		if ((msg->features->has_lazy_pages == 1) &&
> +		    (msg->features->lazy_pages == true)) {
> +			ret = kerndat_uffd(true);

offtopic: I like an idea when kerndat_* functions fill kdat and return
errors when they meet an unexpected error.

			if (kerndat_uffd())
				goto err;

			if (kdat.laze_pages)
				feat.lazy_pages = true;

> +
> +			/*
> +			 * Not checking for specific UFFD features yet.
> +			 * If no error is returned it is probably
> +			 * enough for basic UFFD functionality. This can
> +			 * be extended in the future for a more detailed
> +			 * UFFD feature check.
> +			 */
> +			if (ret)
> +				feat.lazy_pages = false;
> +			else
> +				feat.lazy_pages = true;
> +		}
>  
> -		if (kdat.has_dirty_track)
> -			ret = 0;
> +		resp.features = &feat;
> +		resp.type = msg->type;
> +		/* The feature check is working, actual results are in resp.features */
> +		resp.success = true;
> +
> +		/*
> +		 * If this point is reached the information about the features
> +		 * is transmitted from the forked CRIU process (here).
> +		 * If an error occured earlier, the feature check response will be
> +		 * be send from the parent process.
> +		 */
> +		ret = send_criu_msg(sk, &resp);
>  
>  		exit(ret);
>  	}
>  
>  	wait(&status);

You don't check an exit code from wait here, so status may be
uninitialized. I recomend to use waitpid if pid is known.

> -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> -		goto out;
> +	if (WIFEXITED(status) && !WEXITSTATUS(status))
> +		/*
> +		 * The child process exited was able to send the answer.
> +		 * Nothing more to do here.
> +		 */
> +		return 0;
>  
> -	feat.mem_track = true;
> +	/*
> +	 * The child process was not able to send an answer. Tell
> +	 * the RPC client that something did not work as expected.
> +	 */
>  out:
>  	resp.features = &feat;

I think we should not set feat here, should we?

>  	resp.type = msg->type;
> -	resp.success = success;
> +	resp.success = false;
>  
>  	return send_criu_msg(sk, &resp);
>  }
> 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
>
Adrian Reber March 8, 2017, 3:32 p.m.
On Mon, Feb 27, 2017 at 10:21:04AM -0800, Andrei Vagin wrote:
> On Thu, Feb 23, 2017 at 11:17:42AM +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 non-zero.
> > 
> > The RPC response is now transmitted from the forked process instead of
> > encoding all the results into the return code. The parent RPC process
> > now only sends an RPC message in the case of a failure.
> > 
> > Signed-off-by: Adrian Reber <areber@redhat.com>
> > ---
> >  criu/cr-service.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------
> >  images/rpc.proto  |  1 +
> >  2 files changed, 59 insertions(+), 25 deletions(-)
> > 
> > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > index 169d637..d1007e7 100644
> > --- a/criu/cr-service.c
> > +++ b/criu/cr-service.c
> > @@ -823,33 +823,21 @@ static int handle_feature_check(int sk, CriuReq * msg)
> >  {
> >  	CriuResp resp = CRIU_RESP__INIT;
> >  	CriuFeatures feat = CRIU_FEATURES__INIT;
> > -	bool success = false;
> >  	int pid, status;
> >  
> >  	/* 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;
> >  	}
> >  
> > -	/*
> > -	 * From this point on the function will always
> > -	 * 'succeed'. If the requested features are supported
> > -	 * can be seen if the requested optional parameters are
> > -	 * set in the message 'criu_features'.
> > -	 */
> > -	success = true;
> > -
> >  	pid = fork();
> >  	if (pid < 0) {
> >  		pr_perror("Can't fork");
> > @@ -857,27 +845,72 @@ static int handle_feature_check(int sk, CriuReq * msg)
> >  	}
> >  
> >  	if (pid == 0) {
> > -		int ret = 1;
> > +		int ret;
> >  
> >  		setproctitle("feature-check --rpc");
> >  
> > -		kerndat_get_dirty_track();
> > +		if ((msg->features->has_mem_track == 1) &&
> > +		    (msg->features->mem_track == true)) {
> > +
> > +			feat.mem_track = true;
> > +			ret = kerndat_get_dirty_track();
> > +
> > +			if (ret)
> > +				feat.mem_track = false;
> > +
> > +			if (!kdat.has_dirty_track)
> > +				feat.mem_track = false;
> > +		}
> > +
> > +		if ((msg->features->has_lazy_pages == 1) &&
> > +		    (msg->features->lazy_pages == true)) {
> > +			ret = kerndat_uffd(true);
> 
> offtopic: I like an idea when kerndat_* functions fill kdat and return
> errors when they meet an unexpected error.

Yes, that is also what I thought working with it.

> 			if (kerndat_uffd())
> 				goto err;
> 
> 			if (kdat.laze_pages)
> 				feat.lazy_pages = true;
> 
> > +
> > +			/*
> > +			 * Not checking for specific UFFD features yet.
> > +			 * If no error is returned it is probably
> > +			 * enough for basic UFFD functionality. This can
> > +			 * be extended in the future for a more detailed
> > +			 * UFFD feature check.
> > +			 */
> > +			if (ret)
> > +				feat.lazy_pages = false;
> > +			else
> > +				feat.lazy_pages = true;
> > +		}
> >  
> > -		if (kdat.has_dirty_track)
> > -			ret = 0;
> > +		resp.features = &feat;
> > +		resp.type = msg->type;
> > +		/* The feature check is working, actual results are in resp.features */
> > +		resp.success = true;
> > +
> > +		/*
> > +		 * If this point is reached the information about the features
> > +		 * is transmitted from the forked CRIU process (here).
> > +		 * If an error occured earlier, the feature check response will be
> > +		 * be send from the parent process.
> > +		 */
> > +		ret = send_criu_msg(sk, &resp);
> >  
> >  		exit(ret);
> >  	}
> >  
> >  	wait(&status);
> 
> You don't check an exit code from wait here, so status may be
> uninitialized. I recomend to use waitpid if pid is known.

Will send a new version of the patch. Pavel was easier as criu-dev
maintainer ;-)

> > -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > -		goto out;
> > +	if (WIFEXITED(status) && !WEXITSTATUS(status))
> > +		/*
> > +		 * The child process exited was able to send the answer.
> > +		 * Nothing more to do here.
> > +		 */
> > +		return 0;
> >  
> > -	feat.mem_track = true;
> > +	/*
> > +	 * The child process was not able to send an answer. Tell
> > +	 * the RPC client that something did not work as expected.
> > +	 */
> >  out:
> >  	resp.features = &feat;
> 
> I think we should not set feat here, should we?

Not sure. I removed it. Users should not read it anyway in an error
case. But not setting it at all is probably the right thing to do.

> >  	resp.type = msg->type;
> > -	resp.success = success;
> > +	resp.success = false;
> >  
> >  	return send_criu_msg(sk, &resp);
> >  }
> > 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
> > 

		Adrian
Andrey Vagin March 13, 2017, 5:13 a.m.
On Wed, Mar 08, 2017 at 04:32:14PM +0100, Adrian Reber wrote:
> On Mon, Feb 27, 2017 at 10:21:04AM -0800, Andrei Vagin wrote:
> > On Thu, Feb 23, 2017 at 11:17:42AM +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 non-zero.
> > > 
> > > The RPC response is now transmitted from the forked process instead of
> > > encoding all the results into the return code. The parent RPC process
> > > now only sends an RPC message in the case of a failure.
> > > 
> > > Signed-off-by: Adrian Reber <areber@redhat.com>
> > > ---
> > >  criu/cr-service.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------
> > >  images/rpc.proto  |  1 +
> > >  2 files changed, 59 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > > index 169d637..d1007e7 100644
> > > --- a/criu/cr-service.c
> > > +++ b/criu/cr-service.c
> > > @@ -823,33 +823,21 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > >  {
> > >  	CriuResp resp = CRIU_RESP__INIT;
> > >  	CriuFeatures feat = CRIU_FEATURES__INIT;
> > > -	bool success = false;
> > >  	int pid, status;
> > >  
> > >  	/* 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;
> > >  	}
> > >  
> > > -	/*
> > > -	 * From this point on the function will always
> > > -	 * 'succeed'. If the requested features are supported
> > > -	 * can be seen if the requested optional parameters are
> > > -	 * set in the message 'criu_features'.
> > > -	 */
> > > -	success = true;
> > > -
> > >  	pid = fork();
> > >  	if (pid < 0) {
> > >  		pr_perror("Can't fork");
> > > @@ -857,27 +845,72 @@ static int handle_feature_check(int sk, CriuReq * msg)
> > >  	}
> > >  
> > >  	if (pid == 0) {
> > > -		int ret = 1;
> > > +		int ret;
> > >  
> > >  		setproctitle("feature-check --rpc");
> > >  
> > > -		kerndat_get_dirty_track();
> > > +		if ((msg->features->has_mem_track == 1) &&
> > > +		    (msg->features->mem_track == true)) {
> > > +
> > > +			feat.mem_track = true;
> > > +			ret = kerndat_get_dirty_track();
> > > +
> > > +			if (ret)
> > > +				feat.mem_track = false;
> > > +
> > > +			if (!kdat.has_dirty_track)
> > > +				feat.mem_track = false;
> > > +		}
> > > +
> > > +		if ((msg->features->has_lazy_pages == 1) &&
> > > +		    (msg->features->lazy_pages == true)) {
> > > +			ret = kerndat_uffd(true);
> > 
> > offtopic: I like an idea when kerndat_* functions fill kdat and return
> > errors when they meet an unexpected error.
> 
> Yes, that is also what I thought working with it.
> 
> > 			if (kerndat_uffd())
> > 				goto err;
> > 
> > 			if (kdat.laze_pages)
> > 				feat.lazy_pages = true;
> > 
> > > +
> > > +			/*
> > > +			 * Not checking for specific UFFD features yet.
> > > +			 * If no error is returned it is probably
> > > +			 * enough for basic UFFD functionality. This can
> > > +			 * be extended in the future for a more detailed
> > > +			 * UFFD feature check.
> > > +			 */
> > > +			if (ret)
> > > +				feat.lazy_pages = false;
> > > +			else
> > > +				feat.lazy_pages = true;
> > > +		}
> > >  
> > > -		if (kdat.has_dirty_track)
> > > -			ret = 0;
> > > +		resp.features = &feat;
> > > +		resp.type = msg->type;
> > > +		/* The feature check is working, actual results are in resp.features */
> > > +		resp.success = true;
> > > +
> > > +		/*
> > > +		 * If this point is reached the information about the features
> > > +		 * is transmitted from the forked CRIU process (here).
> > > +		 * If an error occured earlier, the feature check response will be
> > > +		 * be send from the parent process.
> > > +		 */
> > > +		ret = send_criu_msg(sk, &resp);
> > >  
> > >  		exit(ret);
> > >  	}
> > >  
> > >  	wait(&status);
> > 
> > You don't check an exit code from wait here, so status may be
> > uninitialized. I recomend to use waitpid if pid is known.
> 
> Will send a new version of the patch. Pavel was easier as criu-dev
> maintainer ;-)

I'm sorry. I will try to be not so boring next time ;)

> 
> > > -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > > -		goto out;
> > > +	if (WIFEXITED(status) && !WEXITSTATUS(status))
> > > +		/*
> > > +		 * The child process exited was able to send the answer.
> > > +		 * Nothing more to do here.
> > > +		 */
> > > +		return 0;
> > >  
> > > -	feat.mem_track = true;
> > > +	/*
> > > +	 * The child process was not able to send an answer. Tell
> > > +	 * the RPC client that something did not work as expected.
> > > +	 */
> > >  out:
> > >  	resp.features = &feat;
> > 
> > I think we should not set feat here, should we?
> 
> Not sure. I removed it. Users should not read it anyway in an error
> case. But not setting it at all is probably the right thing to do.
> 
> > >  	resp.type = msg->type;
> > > -	resp.success = success;
> > > +	resp.success = false;
> > >  
> > >  	return send_criu_msg(sk, &resp);
> > >  }
> > > 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
> > > 
> 
> 		Adrian