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

[Adrian Reber]

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 at 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 at 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