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

[Andrei Vagin]

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.

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;

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.

Why do we need to create a new process to call these functions?

>  out:
>  	resp.features = &feat;
>  	resp.type = msg->type;
> 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
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu