Re: [PATCH V2 08/13] powerpc/vas: Update CSB and notify process for fault CRBs

[Christoph Hellwig <hch@infradead.org>]

On Sun, Dec 08, 2019 at 07:33:37PM -0800, Haren Myneni wrote:
> +static void notify_process(pid_t pid, u64 fault_addr)
> +{
> +	int rc;
> +	struct kernel_siginfo info;
> +
> +	memset(&info, 0, sizeof(info));
> +
> +	info.si_signo = SIGSEGV;
> +	info.si_errno = EFAULT;
> +	info.si_code = SEGV_MAPERR;
> +	info.si_addr = (void *)fault_addr;
> +	/*
> +	 * process will be polling on csb.flags after request is sent to
> +	 * NX. So generally CSB update should not fail except when an
> +	 * application does not follow the process properly. So an error
> +	 * message will be displayed and leave it to user space whether
> +	 * to ignore or handle this signal.
> +	 */
> +	rcu_read_lock();
> +	rc = kill_pid_info(SIGSEGV, &info, find_vpid(pid));
> +	rcu_read_unlock();
> +
> +	pr_devel("%s(): pid %d kill_proc_info() rc %d\n", __func__, pid, rc);
> +}

I think you want to pass in the struct pid * here instead of looking
up again, given that..

> +	if (tsk) {
> +		if (tsk->flags & PF_EXITING)
> +			task_exit = 1;
> +		put_task_struct(tsk);
> +		pid = vas_window_pid(window);

We already have the struct pid in the window structure here.

> +	} else {
> +		pid = window->tgid;
> +
> +		rcu_read_lock();
> +		tsk = find_task_by_vpid(pid);
> +		if (!tsk) {

.. and could have easily stored on here.  Or at least only do the
look up once, given that already looks it up.

> +	/* Do not notify if the task is exiting. */
> +	if (!task_exit) {
> +		pr_err("Invalid CSB address 0x%p signalling pid(%d)\n",
> +				csb_addr, pid);
> +		notify_process(pid, (u64)csb_addr);
> +	}

I suspect inlining notify_process and just existing early for the
task_exit case also makes the code a bit easier to follow.