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

[Andrei Vagin]

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 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 a least one UFFD feature.
> > > 
> > > Signed-off-by: Adrian Reber <areber at 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