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

[Andrei Vagin]

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 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 ;-)

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