Message-ID: <1502694131.9844.7.camel@neuling.org>
Subject: Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR
From: Michael Neuling <mikey@neuling.org>
To: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
        Michael Ellerman <mpe@ellerman.id.au>
Cc: stewart@linux.vnet.ibm.com, linuxppc-dev@ozlabs.org,
        linux-kernel@vger.kernel.org, apopple@au1.ibm.com, oohall@gmail.com
Date: Mon, 14 Aug 2017 17:02:11 +1000
In-Reply-To: <1502233622-9330-15-git-send-email-sukadev@linux.vnet.ibm.com>
References: <1502233622-9330-1-git-send-email-sukadev@linux.vnet.ibm.com>
         <1502233622-9330-15-git-send-email-sukadev@linux.vnet.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Mime-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4519
Lines: 164

On Tue, 2017-08-08 at 16:06 -0700, Sukadev Bhattiprolu wrote:
> We need the SPRN_TIDR to bet set for use with fast thread-wakeup
> (core-to-core wakeup).  Each thread in a process needs to have a
> unique id within the process but as explained below, for now, we
> assign globally unique thread ids to all threads in the system.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/processor.h |  4 ++
>  arch/powerpc/kernel/process.c        | 74
> ++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/processor.h
> b/arch/powerpc/include/asm/processor.h
> index fab7ff8..bf6ba63 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -232,6 +232,10 @@ struct debug_reg {
>  struct thread_struct {
>  	unsigned long	ksp;		/* Kernel stack pointer */
>  
> +#ifdef CONFIG_PPC_VAS

I'm tempted to have this always, or a new feature CONFIG_PPC_TID that's PPC_VAS
depends on.

> +	unsigned long	tidr;

> +#endif
> +
>  #ifdef CONFIG_PPC64
>  	unsigned long	ksp_vsid;
>  #endif
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 9f3e2c9..6123859 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1213,6 +1213,16 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
>  		hard_irq_disable();
>  	}
>  
> +#ifdef CONFIG_PPC_VAS
> +	mtspr(SPRN_TIDR, new->thread.tidr);

how much does this hurt our context_switch benchmark in
tools/testing/selftests/powerpc/benchmarks/context_switch.c ?

Also you need an CPU_FTR_ARCH_300 test here (and elsewhere)

> +#endif
> +	/*
> +	 * We can't take a PMU exception inside _switch() since there is a
> +	 * window where the kernel stack SLB and the kernel stack are out
> +	 * of sync. Hard disable here.
> +	 */
> +	hard_irq_disable();
> +

What is this?

>  	/*
>  	 * Call restore_sprs() before calling _switch(). If we move it after
>  	 * _switch() then we miss out on calling it for new tasks. The reason
> @@ -1449,9 +1459,70 @@ void flush_thread(void)
>  #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  }
>  
> +#ifdef CONFIG_PPC_VAS
> +static DEFINE_SPINLOCK(vas_thread_id_lock);
> +static DEFINE_IDA(vas_thread_ida);

This IDA be per process, not global.

> +
> +/*
> + * We need to assign an unique thread id to each thread in a process. This
> + * thread id is intended to be used with the Fast Thread-wakeup (aka Core-
> + * to-core wakeup) mechanism being implemented on top of Virtual Accelerator
> + * Switchboard (VAS).
> + *
> + * To get a unique thread-id per process we could simply use task_pid_nr()
> + * but the problem is that task_pid_nr() is not yet available for the thread
> + * when copy_thread() is called. Fixing that would require changing more
> + * intrusive arch-neutral code in code path in copy_process()?.
> + *
> + * Further, to assign unique thread ids within each process, we need an
> + * atomic field (or an IDR) in task_struct, which again intrudes into the
> + * arch-neutral code.

Really?

> + * So try to assign globally unique thraed ids for now.

Yuck!

> + */
> +static int assign_thread_id(void)
> +{
> +	int index;
> +	int err;
> +
> +again:
> +	if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	spin_lock(&vas_thread_id_lock);
> +	err = ida_get_new_above(&vas_thread_ida, 1, &index);

We can't use 0 or 1?

> +	spin_unlock(&vas_thread_id_lock);
> +
> +	if (err == -EAGAIN)
> +		goto again;
> +	else if (err)
> +		return err;
> +
> +	if (index > MAX_USER_CONTEXT) {
> +		spin_lock(&vas_thread_id_lock);
> +		ida_remove(&vas_thread_ida, index);
> +		spin_unlock(&vas_thread_id_lock);
> +		return -ENOMEM;
> +	}
> +
> +	return index;
> +}
> +
> +static void free_thread_id(int id)
> +{
> +	spin_lock(&vas_thread_id_lock);
> +	ida_remove(&vas_thread_ida, id);
> +	spin_unlock(&vas_thread_id_lock);
> +}
> +#endif /* CONFIG_PPC_VAS */
> +
> +
>  void
>  release_thread(struct task_struct *t)
>  {
> +#ifdef CONFIG_PPC_VAS
> +	free_thread_id(t->thread.tidr);
> +#endif

Can you restructure this to avoid the #ifdef ugliness

>  }
>  
>  /*
> @@ -1587,6 +1658,9 @@ int copy_thread(unsigned long clone_flags, unsigned long
> usp,
>  #endif
>  
>  	setup_ksp_vsid(p, sp);
> +#ifdef CONFIG_PPC_VAS
> +	p->thread.tidr = assign_thread_id();
> +#endif

Same here... 

>  
>  #ifdef CONFIG_PPC64 
>  	if (cpu_has_feature(CPU_FTR_DSCR)) {