[Skiboot-stable] [Skiboot] [PATCH] opal-prd: Have a worker process handle page offlining

[Mahesh J Salgaonkar]

On 2020-09-16 10:41:50 Wed, Oliver O'Halloran wrote:
> The memory_error() hservice interface expects the memory_error() call to
> just accept the offline request and return without actually offlining the
> memory. Currently we will attempt to offline the marked pages before
> returning to HBRT which can result in an excessively long time spent in the
> memory_error() hservice call which blocks HBRT from processing other
> errors. Fix this by adding a worker process which performs the page
> offlining via the sysfs memory error interfaces.
> 
> Cc: skiboot-stable at lists.ozlabs.org
> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
> ---
>  external/opal-prd/opal-prd.c | 85 ++++++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 19 deletions(-)
> 
> diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c
> index 33ea5f5a8f6e..f2861611fe7a 100644
> --- a/external/opal-prd/opal-prd.c
> +++ b/external/opal-prd/opal-prd.c
> @@ -41,6 +41,7 @@
>  #include <sys/time.h>
>  #include <sys/socket.h>
>  #include <sys/un.h>
> +#include <sys/wait.h>
>  
>  #include <linux/ipmi.h>
>  #include <linux/limits.h>
> @@ -701,13 +702,40 @@ out:
>  	return rc;
>  }
>  
[...]
> @@ -727,26 +755,21 @@ int hservice_memory_error(uint64_t i_start_addr, uint64_t i_endAddr,
>  	pr_log(LOG_ERR, "MEM: Memory error: range %016lx-%016lx, type: %s",
>  			i_start_addr, i_endAddr, typestr);
>  
> +	/*
> +	 * HBRT expects the memory offlining process to happen in the background
> +	 * after the notification is delivered.
> +	 */
> +	pid = fork();
> +	if (pid > 0)
> +		exit(memory_error_worker(sysfsfile, typestr, i_start_addr, i_endAddr));
>  
> -	memfd = open(sysfsfile, O_WRONLY);
> -	if (memfd < 0) {
> -		pr_log(LOG_CRIT, "MEM: Failed to offline memory! "
> -				"Unable to open sysfs node %s: %m", sysfsfile);
> +	if (pid < 0) {
> +		perror("MEM: unable to fork worker to offline memory!\n");
>  		return -1;
>  	}
>  
> -	for (addr = i_start_addr; addr <= i_endAddr; addr += ctx->page_size) {
> -		n = snprintf(buf, ADDR_STRING_SZ, "0x%lx", addr);
> -		rc = write(memfd, buf, n);
> -		if (rc != n) {
> -			pr_log(LOG_CRIT, "MEM: Failed to offline memory! "
> -					"page addr: %016lx type: %d: %m",
> -				addr, i_errorType);
> -			ret = rc;
> -		}
> -	}
> -
> -	return ret;
> +	pr_log(LOG_INFO, "MEM: forked off %d to handle mem error\n", pid);
> +	return 0;
>  }
>  
>  uint64_t hservice_get_interface_capabilities(uint64_t set)
> @@ -2003,6 +2026,13 @@ out_send:
>  		free(send_msg);
>  }
>  
> +volatile bool worker_terminated;
> +
> +void signchild_handler(int sig)
> +{
> +	worker_terminated = true;
> +}

Should this handler be registered to catch SIGCHILD ? I don't
see it is being registered.

> +
>  static int run_attn_loop(struct opal_prd_ctx *ctx)
>  {
>  	struct pollfd pollfds[2];
> @@ -2049,6 +2079,23 @@ static int run_attn_loop(struct opal_prd_ctx *ctx)
>  		process_msgq(ctx);
>  
>  		rc = poll(pollfds, 2, -1);
> +
> +		if (worker_terminated) {
> +			pid_t pid;
> +
> +			worker_terminated = false;
> +			do {
> +				pid = waitpid(-1, NULL, WNOHANG);
> +				if (pid > 0) {
> +					pr_log(LOG_DEBUG, "reaped %d\n", pid);
> +				} else if (rc == -1 && errno != ECHILD) {

Shouldn't this be if (pid == -1 && ... ?

> +					pr_log(LOG_ERR, "error %m while reaping\n");
> +					break;
> +				}
> +			} while (pid > 0);
> +
> +			continue;
> +		}
>  		if (rc < 0) {
>  			pr_log(LOG_ERR, "FW: event poll failed: %m");
>  			exit(EXIT_FAILURE);

Rest looks good to me.

Thanks,
-Mahesh.

> -- 
> 2.26.2
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

-- 
Mahesh J Salgaonkar