Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Benjamin Herrenschmidt]

On Mon, 2017-08-14 at 13:03 -0700, Sukadev Bhattiprolu wrote:
> As Ben pointed out, we are going to be have limit the number of TIDs (to
> be within the size limits), so we won't be able to use task_pid_nr()? But
> if we assign the TIDs in the RX_WIN_OPEN ioctl, then only the FTW processes
> will need the TIDR value.

But you'll have to assign it for all present and future threads of that
process which is somewhat hard to do without races.

> Can we then assign new, globally-unique TID values for now and have the ioctl
> fail with -EAGAIN if all TIDs are in use? We can extend to per-process TID
> values, later?

Why would you want to do that ?

Ben.

Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Benjamin Herrenschmidt]

On Mon, 2017-08-14 at 21:16 +1000, Michael Ellerman wrote:
> Sukadev Bhattiprolu  writes:
> 
> > 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.
> 
> Each thread in a process already has a unique id, ie. its pid (in the
> init PID namespace), accessible in the kernel as task_pid_nr(task).
> 
> So if that's all we need, we don't need a new allocator, and we don't
> need to store it in the thread_struct.

We need an allocator, I think, due to size restriction on the HW TID.

> Also 99.99% of processes aren't going to care about the TIDR, so we
> should avoid setting it in the common case. ie. it should start out zero
> and only be initialised in the FTW code, or a helper that it calls.


> > 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);
> > +#endif
> 
> That should be in restore_sprs().
> 
> It should also check that the TIDR is initialised, and only switch it
> when necessary.
> 
> > +   /*
> > +* 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();
> 
> We removed that in June in:
> 
>  e4c0fc5f72bc ("powerpc/64s: Leave interrupts hard enabled in context switch 
> for radix")
> 
> You've obviously picked it up somewhere along the line during a rebase,
> please be more careful!
> 
> cheers

Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Sukadev Bhattiprolu]

Michael Ellerman [m...@ellerman.id.au] wrote:
> Sukadev Bhattiprolu  writes:
> 
> > 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.
> 
> Each thread in a process already has a unique id, ie. its pid (in the
> init PID namespace), accessible in the kernel as task_pid_nr(task).
> 
> So if that's all we need, we don't need a new allocator, and we don't
> need to store it in the thread_struct.
> 
> Also 99.99% of processes aren't going to care about the TIDR, so we
> should avoid setting it in the common case. ie. it should start out zero
> and only be initialised in the FTW code, or a helper that it calls.

Good point. So, should we just set when the RX_WIN_OPEN ioctl is called
rather than at the time of clone()?

_switch_to() (restore_sprs() could check for non-zero and save/restore
the value.

As Ben pointed out, we are going to be have limit the number of TIDs (to
be within the size limits), so we won't be able to use task_pid_nr()? But
if we assign the TIDs in the RX_WIN_OPEN ioctl, then only the FTW processes
will need the TIDR value.

Can we then assign new, globally-unique TID values for now and have the ioctl
fail with -EAGAIN if all TIDs are in use? We can extend to per-process TID
values, later?

> 
> > 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);
> > +#endif
> 
> That should be in restore_sprs().

ok.
> 
> It should also check that the TIDR is initialised, and only switch it
> when necessary.
> 
> > +   /*
> > +* 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();
> 
> We removed that in June in:
> 
>  e4c0fc5f72bc ("powerpc/64s: Leave interrupts hard enabled in context switch 
> for radix")
> 
> You've obviously picked it up somewhere along the line during a rebase,
> please be more careful!

Yeah, That was stupid. I picked it up on a recent rebase. Will be careful.

> 
> cheers

Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Sukadev Bhattiprolu]

Benjamin Herrenschmidt [b...@au1.ibm.com] wrote:
> On Mon, 2017-08-14 at 17:02 +1000, Michael Neuling wrote:
> > > +/*
> > > + * 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!

I know :-) copy_process() has:

retval = copy_thread_tls(clone_flags, stack_start, stack_size, p, tls);
if (retval)
goto bad_fork_cleanup_io;

if (pid != _struct_pid) {
pid = alloc_pid(p->nsproxy->pid_ns_for_children);
if (IS_ERR(pid)) {


so copy_thread() is called before a pid_nr is assigned to the task.

But see also response to Michael Ellerman.

> 
> Also CAPI has size limits for the TIDR afaik

Ok.

> 
> Ben.

Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Benjamin Herrenschmidt]

On Mon, 2017-08-14 at 17:02 +1000, Michael Neuling wrote:
> > +/*
> > + * 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!

Also CAPI has size limits for the TIDR afaik

Ben.

Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Michael Ellerman]

Sukadev Bhattiprolu  writes:

> 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.

Each thread in a process already has a unique id, ie. its pid (in the
init PID namespace), accessible in the kernel as task_pid_nr(task).

So if that's all we need, we don't need a new allocator, and we don't
need to store it in the thread_struct.

Also 99.99% of processes aren't going to care about the TIDR, so we
should avoid setting it in the common case. ie. it should start out zero
and only be initialised in the FTW code, or a helper that it calls.

> 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);
> +#endif

That should be in restore_sprs().

It should also check that the TIDR is initialised, and only switch it
when necessary.

> + /*
> +  * 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();

We removed that in June in:

 e4c0fc5f72bc ("powerpc/64s: Leave interrupts hard enabled in context switch 
for radix")

You've obviously picked it up somewhere along the line during a rebase,
please be more careful!

cheers

Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Michael Neuling]

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 
> ---
>  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(_thread_ida, GFP_KERNEL))
> + return -ENOMEM;
> +
> + spin_lock(_thread_id_lock);
> + err = ida_get_new_above(_thread_ida, 1, );

We can't use 0 or 1?

> + spin_unlock(_thread_id_lock);
> +
> + if (err == -EAGAIN)
> + goto again;
> + else if (err)
> + return err;
> +
> + if (index > MAX_USER_CONTEXT) {
> + spin_lock(_thread_id_lock);
> + ida_remove(_thread_ida, index);
> + spin_unlock(_thread_id_lock);
> + return -ENOMEM;
> + }
> +
> + return index;
> +}
> +
> +static void free_thread_id(int id)
> +{
> + spin_lock(_thread_id_lock);
> + ida_remove(_thread_ida, id);
> + spin_unlock(_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)) {

[PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Sukadev Bhattiprolu]

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 
---
 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
+   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);
+#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();
+
/*
 * 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);
+
+/*
+ * 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.
+ *
+ * So try to assign globally unique thraed ids for now.
+ */
+static int assign_thread_id(void)
+{
+   int index;
+   int err;
+
+again:
+   if (!ida_pre_get(_thread_ida, GFP_KERNEL))
+   return -ENOMEM;
+
+   spin_lock(_thread_id_lock);
+   err = ida_get_new_above(_thread_ida, 1, );
+   spin_unlock(_thread_id_lock);
+
+   if (err == -EAGAIN)
+   goto again;
+   else if (err)
+   return err;
+
+   if (index > MAX_USER_CONTEXT) {
+   spin_lock(_thread_id_lock);
+   ida_remove(_thread_ida, index);
+   spin_unlock(_thread_id_lock);
+   return -ENOMEM;
+   }
+
+   return index;
+}
+
+static void free_thread_id(int id)
+{
+   spin_lock(_thread_id_lock);
+   ida_remove(_thread_ida, id);
+   spin_unlock(_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
 }
 
 /*
@@ -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
 
 #ifdef CONFIG_PPC64 
if (cpu_has_feature(CPU_FTR_DSCR)) {
-- 
2.7.4