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

[Adrian Reber]

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.

> 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