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

[Adrian Reber]

On Fri, Feb 17, 2017 at 10:33:04AM -0800, Andrei Vagin wrote:
> 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.

Yes, you are right. That is of course wrong. I wait for some insights
why the code fork()-s for feature check and maybe I could then remove
the whole return code magic. I will send a new patch once I understand
why it is the way it is.

		Adrian