Interface to set SPRN_TIDR

RFC PATCH In review Sukadev Bhattiprolu 3 participants 6 messages Aug 30, 2017 – Sep 1, 2017 PowerPC

Share & tools

Sukadev Bhattiprolu

author

2017-08-30 02:39 UTC #0 lore raw

RFC PATCH 1/1 Interface to set SPRN_TIDR

We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-to-core wakeup) in VAS. Each user thread that has a receive window setup and expects to be notified when a sender issues a paste needs to have a unique SPRN_TIDR value.

The SPRN_TIDR value only needs to unique within the process but for now we use a globally unique thread id as described below.

Signed-off-by Sukadev Bhattiprolu <[email protected]>

---
Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is assigned to only threads that need it; move assignment to restore_sprs(). Drop lint from rebase;

 arch/powerpc/include/asm/processor.h |  4 ++
 arch/powerpc/include/asm/switch_to.h |  3 ++
 arch/powerpc/kernel/process.c        | 97 ++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)

arch/powerpc/include/asm/processor.h +4 -0

index fab7ff8..bf6ba63 100644
@@ -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

arch/powerpc/include/asm/switch_to.h +3 -0

index 17c8380..4962455 100644
@@ -91,4 +91,7 @@ static inline void clear_task_ebb(struct task_struct *t)
 #endif
 }
 
+extern void set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
 #endif /* _ASM_POWERPC_SWITCH_TO_H */

arch/powerpc/kernel/process.c +97 -0

124 lines not on this page · raw message

Signature · 1 line

2.7.4

Michael Neuling

2017-08-30 07:08 UTC #1 lore raw

Suka,

Please CC Christophe who as an alternative way of doing this. We ned to get agreement across all users of TIDR/AS_notify...

His patch is here:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-August/161582.html

Mikey

Quoted 9 lines · Sukadev Bhattiprolu

We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-to-core wakeup) in VAS. Each user thread that has a receive window setup and expects to be notified when a sender issues a paste needs to have a unique SPRN_TIDR value.
=20
The SPRN_TIDR value only needs to unique within the process but for now we use a globally unique thread id as described below. =20
Signed-off-by Sukadev Bhattiprolu <[email protected]>

Quoted 10 lines

---
Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is =C2=A0=C2=A0assigned to only threads that need it; move assignment to =C2=A0=C2=A0restore_sprs(). Drop lint from rebase;
=20
=20
=C2=A0arch/powerpc/include/asm/processor.h |=C2=A0=C2=A04 ++ =C2=A0arch/powerpc/include/asm/switch_to.h |=C2=A0=C2=A03 ++ =C2=A0arch/powerpc/kernel/process.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=

=C2=A0=C2=A0| 97

arch/powerpc/include/asm/switch_to.h static inline void clear_task_ebb(struct task_struct *t=

Quoted 25 lines

++++++++++++++++++++++++++++++++++++
=C2=A03 files changed, 104 insertions(+)
=20
arch/powerpc/include/asm/processor.h +0 -0
index fab7ff8..bf6ba63 100644
@@ -232,6 +232,10 @@ struct debug_reg {
 =C2=A0struct thread_struct {
=C2=A0 unsigned long ksp; /* Kernel stack pointer */
=20
arch/powerpc/include/asm/processor.h +4 -0
+#ifdef CONFIG_PPC_VAS
+	unsigned long	tidr;
+#endif
+
=C2=A0#ifdef CONFIG_PPC64
=C2=A0 unsigned long ksp_vsid;
=C2=A0#endif
arch/powerpc/include/asm/switch_to.h +0 -0
index 17c8380..4962455 100644
@@ -91,4 +91,7 @@ static inline void clear_task_ebb(struct task_struct *t=

)

arch/powerpc/kernel/process.=

Quoted 8 lines

=C2=A0#endif
=C2=A0}
=20
arch/powerpc/include/asm/switch_to.h +3 -0
+extern void set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
=C2=A0#endif /* _ASM_POWERPC_SWITCH_TO_H */
arch/powerpc/kernel/process.= renamed from arch/powerpc/kernel/process.c +0 -0

arch/powerpc/kernel/process.c static inline void restore_sprs(struct thread_stru=

Quoted 4 lines

index 1f0fd36..13abb22 100644
arch/powerpc/kernel/process.c +0 -0
@@ -1132,6 +1132,10 @@ static inline void restore_sprs(struct thread_stru=

Quoted 15 lines

arch/powerpc/kernel/process.c +0 -0
 *old_thread,
 =C2=A0			mtspr(SPRN_TAR, new_thread->tar);
 =C2=A0	}
=C2=A0#endif
arch/powerpc/kernel/process.c +4 -0
+#ifdef CONFIG_PPC_VAS
+	if (old_thread->tidr !=3D new_thread->tidr)
+		mtspr(SPRN_TIDR, new_thread->tidr);
+#endif
=C2=A0}
=20
=C2=A0#ifdef CONFIG_PPC_BOOK3S_64
arch/powerpc/kernel/process.c +0 -0
@@ -1446,9 +1450,97 @@ void flush_thread(void)
=C2=A0#endif /* CONFIG_HAVE_HW_BREAKPOINT */
=C2=A0}
=20

arch/powerpc/kernel/process.c void flush_thread(void)

Quoted 6 lines

arch/powerpc/kernel/process.c +6 -0

+#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. Th=

Quoted 1 line

+ * thread id is intended to be used with the Fast Thread-wakeup (aka Cor=

Quoted 1 line

+ * to-core wakeup) mechanism being implemented on top of Virtual Acceler=

ator

Quoted 3 lines

+ * Switchboard (VAS).
+ *
+ * To get a unique thread-id per process we could simply use task_pid_nr=

()

Quoted 1 line

+ * but the problem is that task_pid_nr() is not yet available for the th=

read

Quoted 5 lines

+ * 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 th=

Quoted 6 lines

+ * arch-neutral code.
+ *
+ * So try to assign globally unique thraed ids for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ *=C2=A0 =C2=A0For now, only threads that expect to be notified by the V=

Quoted 1 line

+ *=C2=A0 =C2=A0hardware need a TIDR value and we assign values > 0 for t=

hose.

arch/powerpc/kernel/process.c int arch_dup_task_struct(struct task_struct *dst, s=

Quoted 71 lines

+ */

arch/powerpc/kernel/process.c +60 -0

+#define MAX_THREAD_CONTEXT	((1 << 15) - 2)
+static int assign_thread_tidr(void)
+{
+	int index;
+	int err;
+
+again:
+	if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL))
+		return -ENOMEM;
+
+	spin_lock(&vas_thread_id_lock);
+	err =3D ida_get_new_above(&vas_thread_ida, 1, &index);
+	spin_unlock(&vas_thread_id_lock);
+
+	if (err =3D=3D -EAGAIN)
+		goto again;
+	else if (err)
+		return err;
+
+	if (index > MAX_THREAD_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_tidr(int id)
+{
+	spin_lock(&vas_thread_id_lock);
+	ida_remove(&vas_thread_ida, id);
+	spin_unlock(&vas_thread_id_lock);
+}
+
+void clear_thread_tidr(struct task_struct *t)
+{
+	if (t->thread.tidr) {
+		free_thread_tidr(t->thread.tidr);
+		t->thread.tidr =3D 0;
+		mtspr(SPRN_TIDR, 0);
+	}
+}
+
+/*
+ * Assign an unique thread id for this thread and set it in the
+ * thread structure. For now, we need this interface only for
+ * the current task.
+ */
+void set_thread_tidr(struct task_struct *t)
+{
+	WARN_ON(t !=3D current);
+	t->thread.tidr =3D assign_thread_tidr();
+	mtspr(SPRN_TIDR, t->thread.tidr);
+}
+
+#endif /* CONFIG_PPC_VAS */
+
+

=C2=A0void
=C2=A0release_thread(struct task_struct *t)
=C2=A0{

arch/powerpc/kernel/process.c +3 -0

+#ifdef CONFIG_PPC_VAS
+	clear_thread_tidr(t);
+#endif

=C2=A0}
=20
=C2=A0/*

arch/powerpc/kernel/process.c +0 -0

@@ -1474,6 +1566,8 @@ int arch_dup_task_struct(struct task_struct *dst, s=

truct

arch/powerpc/kernel/process.c int copy_thread(unsigned long clone_flags, unsigned=

Quoted 10 lines

arch/powerpc/kernel/process.c +0 -0
 task_struct *src)
=20
=C2=A0 clear_task_ebb(dst);
=20
arch/powerpc/kernel/process.c +2 -0
+	dst->thread.tidr =3D 0;
+
=C2=A0 return 0;
=C2=A0}
=20
arch/powerpc/kernel/process.c +0 -0
@@ -1584,6 +1678,9 @@ int copy_thread(unsigned long clone_flags, unsigned=

long

Quoted 10 lines

arch/powerpc/kernel/process.c +0 -0
 usp,
=C2=A0#endif
=20
=C2=A0 setup_ksp_vsid(p, sp);
arch/powerpc/kernel/process.c +3 -0
+#ifdef CONFIG_PPC_VAS
+	p->thread.tidr =3D 0;
+#endif
=20
=C2=A0#ifdef CONFIG_PPC64=C2=A0
=C2=A0 if (cpu_has_feature(CPU_FTR_DSCR)) {

Sukadev Bhattiprolu

author

2017-08-30 23:32 UTC #2 lore raw

Quoted 3 lines · Michael Neuling [[email protected]] wrote:

Suka,
=20
Please CC Christophe who as an alternative way of doing this. We ned to g=

Quoted 1 line

agreement across all users of TIDR/AS_notify...

Mikey,

Thanks. There is overlap between the two patches. I will send a patch on top of Christophe's for the interfaces to assign/clear the TIDR value and clear the thread->tidr during arch_dup_task_struct(). I will also drop the CONFIG_VAS check since its not only for VAS.

Christophe, can you let me know of any other comments on this patch?

Suka

Quoted 43 lines

=20
His patch is here:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-August/161582.html
=20
Mikey
=20
Quoted 36 lines · Sukadev Bhattiprolu
We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-to-core wakeup) in VAS. Each user thread that has a receive window setup and expects to be notified when a sender issues a paste needs to have a unique SPRN_TIDR value.
=20
The SPRN_TIDR value only needs to unique within the process but for now we use a globally unique thread id as described below. =20
Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is =A0=A0assigned to only threads that need it; move assignment to =A0=A0restore_sprs(). Drop lint from rebase;
=20
=20
=A0arch/powerpc/include/asm/processor.h |=A0=A04 ++ =A0arch/powerpc/include/asm/switch_to.h |=A0=A03 ++ =A0arch/powerpc/kernel/process.c=A0=A0=A0=A0=A0=A0=A0=A0| 97
++++++++++++++++++++++++++++++++++++
=A03 files changed, 104 insertions(+)
=20
arch/powerpc/include/asm/processor.h +0 -0
index fab7ff8..bf6ba63 100644
@@ -232,6 +232,10 @@ struct debug_reg {
 =A0struct thread_struct {
=A0 unsigned long ksp; /* Kernel stack pointer */
=20
arch/powerpc/include/asm/processor.h +4 -0
+#ifdef CONFIG_PPC_VAS
+	unsigned long	tidr;
+#endif
+
=A0#ifdef CONFIG_PPC64

Quoted 8 lines

arch/powerpc/include/asm/switch_to.h static inline void clear_task_ebb(struct task_struct =
Quoted 8 lines
=A0 unsigned long ksp_vsid;
=A0#endif
arch/powerpc/include/asm/switch_to.h +0 -0
index 17c8380..4962455 100644
@@ -91,4 +91,7 @@ static inline void clear_task_ebb(struct task_struct =

*t)

Quoted 8 lines

arch/powerpc/kernel/proces=
Quoted 8 lines
=A0#endif
=A0}
=20
arch/powerpc/include/asm/switch_to.h +3 -0
+extern void set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
=A0#endif /* _ASM_POWERPC_SWITCH_TO_H */
arch/powerpc/kernel/proces= renamed from arch/powerpc/kernel/process.c +0 -0

s.c

Quoted 4 lines

arch/powerpc/kernel/process.c static inline void restore_sprs(struct thread_st=
Quoted 4 lines
index 1f0fd36..13abb22 100644
arch/powerpc/kernel/process.c +0 -0
@@ -1132,6 +1132,10 @@ static inline void restore_sprs(struct thread_st=

ruct

Quoted 13 lines

Quoted 13 lines
arch/powerpc/kernel/process.c +0 -0
 *old_thread,
 =A0			mtspr(SPRN_TAR, new_thread->tar);
 =A0	}
=A0#endif
arch/powerpc/kernel/process.c +4 -0
+#ifdef CONFIG_PPC_VAS
+	if (old_thread->tidr !=3D new_thread->tidr)
+		mtspr(SPRN_TIDR, new_thread->tidr);
+#endif
=A0}
=20
=A0#ifdef CONFIG_PPC_BOOK3S_64
arch/powerpc/kernel/process.c +0 -0
@@ -1446,9 +1450,97 @@ void flush_thread(void)
=A0#endif /* CONFIG_HAVE_HW_BREAKPOINT */

Quoted 8 lines

arch/powerpc/kernel/process.c void flush_thread(void)
Quoted 8 lines
=A0}
=20
arch/powerpc/kernel/process.c +6 -0
+#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

Quoted 1 line

Quoted 1 line
+ * thread id is intended to be used with the Fast Thread-wakeup (aka C=

ore-

Quoted 1 line

Quoted 1 line
+ * to-core wakeup) mechanism being implemented on top of Virtual Accel=

erator

Quoted 3 lines

Quoted 3 lines
+ * Switchboard (VAS).
+ *
+ * To get a unique thread-id per process we could simply use task_pid_=

nr()

Quoted 1 line

Quoted 1 line
+ * but the problem is that task_pid_nr() is not yet available for the =

thread

Quoted 1 line

Quoted 1 line
+ * when copy_thread() is called. Fixing that would require changing mo=

Quoted 4 lines

Quoted 4 lines
+ * 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

Quoted 70 lines

+ * arch-neutral code.
+ *
+ * So try to assign globally unique thraed ids for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ *=A0 =A0For now, only threads that expect to be notified by the VAS
+ *=A0 =A0hardware need a TIDR value and we assign values > 0 for those.
+ */

arch/powerpc/kernel/process.c +60 -0

+#define MAX_THREAD_CONTEXT	((1 << 15) - 2)
+static int assign_thread_tidr(void)
+{
+	int index;
+	int err;
+
+again:
+	if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL))
+		return -ENOMEM;
+
+	spin_lock(&vas_thread_id_lock);
+	err =3D ida_get_new_above(&vas_thread_ida, 1, &index);
+	spin_unlock(&vas_thread_id_lock);
+
+	if (err =3D=3D -EAGAIN)
+		goto again;
+	else if (err)
+		return err;
+
+	if (index > MAX_THREAD_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_tidr(int id)
+{
+	spin_lock(&vas_thread_id_lock);
+	ida_remove(&vas_thread_ida, id);
+	spin_unlock(&vas_thread_id_lock);
+}
+
+void clear_thread_tidr(struct task_struct *t)
+{
+	if (t->thread.tidr) {
+		free_thread_tidr(t->thread.tidr);
+		t->thread.tidr =3D 0;
+		mtspr(SPRN_TIDR, 0);
+	}
+}
+
+/*
+ * Assign an unique thread id for this thread and set it in the
+ * thread structure. For now, we need this interface only for
+ * the current task.
+ */
+void set_thread_tidr(struct task_struct *t)
+{
+	WARN_ON(t !=3D current);
+	t->thread.tidr =3D assign_thread_tidr();
+	mtspr(SPRN_TIDR, t->thread.tidr);
+}
+
+#endif /* CONFIG_PPC_VAS */
+
+

=A0void
=A0release_thread(struct task_struct *t)

Quoted 8 lines

arch/powerpc/kernel/process.c int arch_dup_task_struct(struct task_struct *dst,=
Quoted 8 lines
=A0{
arch/powerpc/kernel/process.c +3 -0
+#ifdef CONFIG_PPC_VAS
+	clear_thread_tidr(t);
+#endif
=A0}
=20
=A0/*
arch/powerpc/kernel/process.c +0 -0
@@ -1474,6 +1566,8 @@ int arch_dup_task_struct(struct task_struct *dst,=

struct

Quoted 10 lines

arch/powerpc/kernel/process.c int copy_thread(unsigned long clone_flags, unsign=
Quoted 10 lines
arch/powerpc/kernel/process.c +0 -0
 task_struct *src)
=20
=A0 clear_task_ebb(dst);
=20
arch/powerpc/kernel/process.c +2 -0
+	dst->thread.tidr =3D 0;
+
=A0 return 0;
=A0}
=20
arch/powerpc/kernel/process.c +0 -0
@@ -1584,6 +1678,9 @@ int copy_thread(unsigned long clone_flags, unsign=

ed long

Quoted 10 lines

Quoted 10 lines
arch/powerpc/kernel/process.c +0 -0
 usp,
=A0#endif
=20
=A0 setup_ksp_vsid(p, sp);
arch/powerpc/kernel/process.c +3 -0
+#ifdef CONFIG_PPC_VAS
+	p->thread.tidr =3D 0;
+#endif
=20
=A0#ifdef CONFIG_PPC64=A0
=A0 if (cpu_has_feature(CPU_FTR_DSCR)) {

felix

2017-08-31 15:15 UTC #3 lore raw

Quoted 7 lines · Sukadev Bhattiprolu

Quoted 4 lines · Michael Neuling [[email protected]] wrote:
Suka,
Please CC Christophe who as an alternative way of doing this. We ned to get agreement across all users of TIDR/AS_notify...
Mikey,

Quoted 8 lines

Thanks. There is overlap between the two patches. I will send a patch on top of Christophe's for the interfaces to assign/clear the TIDR value and clear the thread->tidr during arch_dup_task_struct(). I will also drop the CONFIG_VAS check since its not only for VAS.
Christophe, can you let me know of any other comments on this patch?
Suka

Suka,

I am seconding Christophe on this matter. I think that your patch now fulfills the CAPI use case requirements, with one exception: CAPI does not restrict assigning a thread id to the current task. Please find a few minor questions below.

Philippe

Quoted 95 lines

Quoted 95 lines
His patch is here:
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.ozlabs.org_pipermail_linuxppc-2Ddev_2017-2DAugust_161582.html&d=DwIFAw&c=jf_iaSHvJObTbx-siA1ZOg&r=KC0fX9VGJYXlSiH9qN2ZONDbh8vUCZFX8GUhF3rHAvg&m=XQenBfWewOBjWopgf1Fh2UAVGnlzq766MNuzx7jYfuA&s=07WOVTh9f_4IBZfCJes4lvc7LWenBlqVfAXIXxL2QH4&e=
Mikey
Quoted 89 lines · Sukadev Bhattiprolu
We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-to-core wakeup) in VAS. Each user thread that has a receive window setup and expects to be notified when a sender issues a paste needs to have a unique SPRN_TIDR value.
The SPRN_TIDR value only needs to unique within the process but for now we use a globally unique thread id as described below.
Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is assigned to only threads that need it; move assignment to restore_sprs(). Drop lint from rebase;
  arch/powerpc/include/asm/processor.h |  4 ++
  arch/powerpc/include/asm/switch_to.h |  3 ++
  arch/powerpc/kernel/process.c        | 97
++++++++++++++++++++++++++++++++++++
  3 files changed, 104 insertions(+)
arch/powerpc/include/asm/processor.h +4 -0
index fab7ff8..bf6ba63 100644
@@ -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
arch/powerpc/include/asm/switch_to.h +3 -0
index 17c8380..4962455 100644
@@ -91,4 +91,7 @@ static inline void clear_task_ebb(struct task_struct *t)
  #endif
  }
 
+extern void set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
  #endif /* _ASM_POWERPC_SWITCH_TO_H */
arch/powerpc/kernel/process.c +4 -0
index 1f0fd36..13abb22 100644
@@ -1132,6 +1132,10 @@ static inline void restore_sprs(struct thread_struct
 *old_thread,
  			mtspr(SPRN_TAR, new_thread->tar);
  	}
  #endif
+#ifdef CONFIG_PPC_VAS
+	if (old_thread->tidr != new_thread->tidr)
+		mtspr(SPRN_TIDR, new_thread->tidr);
+#endif
  }
 
  #ifdef CONFIG_PPC_BOOK3S_64
arch/powerpc/kernel/process.c +18 -0
@@ -1446,9 +1450,97 @@ 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.

Quoted 8 lines

arch/powerpc/kernel/process.c void flush_thread(void) · line 1478

Quoted 8 lines

arch/powerpc/kernel/process.c +8 -0

+ *
+ * So try to assign globally unique thraed ids for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ * 	 For now, only threads that expect to be notified by the VAS
+ * 	 hardware need a TIDR value and we assign values > 0 for those.
+ */
+#define MAX_THREAD_CONTEXT	((1 << 15) - 2)

Why are you excluding ((1 << 15) - 1)?

Quoted 11 lines

Quoted 11 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1489
Quoted 11 lines
arch/powerpc/kernel/process.c +11 -0
+static int assign_thread_tidr(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);

Why are you excluding 1?

Quoted 33 lines

arch/powerpc/kernel/process.c +33 -0

+	spin_unlock(&vas_thread_id_lock);
+
+	if (err == -EAGAIN)
+		goto again;
+	else if (err)
+		return err;
+
+	if (index > MAX_THREAD_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_tidr(int id)
+{
+	spin_lock(&vas_thread_id_lock);
+	ida_remove(&vas_thread_ida, id);
+	spin_unlock(&vas_thread_id_lock);
+}
+
+void clear_thread_tidr(struct task_struct *t)
+{
+	if (t->thread.tidr) {
+		free_thread_tidr(t->thread.tidr);
+		t->thread.tidr = 0;
+		mtspr(SPRN_TIDR, 0);
+	}
+}
+

Quoted 8 lines

Quoted 8 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1530
Quoted 8 lines
arch/powerpc/kernel/process.c +8 -0
+/*
+ * Assign an unique thread id for this thread and set it in the
+ * thread structure. For now, we need this interface only for
+ * the current task.
+ */
+void set_thread_tidr(struct task_struct *t)
+{
+	WARN_ON(t != current);

CAPI does not have this restriction. It should be possible to assign a thread id to an arbitrary task.

Quoted 37 lines

arch/powerpc/kernel/process.c +15 -0

+	t->thread.tidr = assign_thread_tidr();
+	mtspr(SPRN_TIDR, t->thread.tidr);
+}
+
+#endif /* CONFIG_PPC_VAS */
+
+
  void
  release_thread(struct task_struct *t)
  {
+#ifdef CONFIG_PPC_VAS
+	clear_thread_tidr(t);
+#endif
  }
 
  /*

@@ -1474,6 +1566,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct
 task_struct *src)
 
  	clear_task_ebb(dst);
 
+	dst->thread.tidr = 0;
+
  	return 0;
  }

@@ -1584,6 +1678,9 @@ int copy_thread(unsigned long clone_flags, unsigned long
 usp,
  #endif
 
  	setup_ksp_vsid(p, sp);
+#ifdef CONFIG_PPC_VAS
+	p->thread.tidr = 0;
+#endif
 
  #ifdef CONFIG_PPC64

  	if (cpu_has_feature(CPU_FTR_DSCR)) {

Sukadev Bhattiprolu

author

2017-08-31 18:06 UTC #4 lore raw

Quoted 121 lines · felix [[email protected]] wrote:

Quoted 7 lines · Sukadev Bhattiprolu
Quoted 4 lines · Michael Neuling [[email protected]] wrote:
Suka,
Please CC Christophe who as an alternative way of doing this. We ned to get agreement across all users of TIDR/AS_notify...
Mikey,
Quoted 8 lines
Thanks. There is overlap between the two patches. I will send a patch on top of Christophe's for the interfaces to assign/clear the TIDR value and clear the thread->tidr during arch_dup_task_struct(). I will also drop the CONFIG_VAS check since its not only for VAS.
Christophe, can you let me know of any other comments on this patch?
Suka
Suka,
I am seconding Christophe on this matter. I think that your patch now fulfills the CAPI use case requirements, with one exception: CAPI does not restrict assigning a thread id to the current task. Please find a few minor questions below.
Philippe
Quoted 96 lines
Quoted 96 lines
His patch is here:
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.ozlabs.org_pipermail_linuxppc-2Ddev_2017-2DAugust_161582.html&d=DwIFAw&c=jf_iaSHvJObTbx-siA1ZOg&r=KC0fX9VGJYXlSiH9qN2ZONDbh8vUCZFX8GUhF3rHAvg&m=XQenBfWewOBjWopgf1Fh2UAVGnlzq766MNuzx7jYfuA&s=07WOVTh9f_4IBZfCJes4lvc7LWenBlqVfAXIXxL2QH4&e=
Mikey
Quoted 90 lines · Sukadev Bhattiprolu
We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-to-core wakeup) in VAS. Each user thread that has a receive window setup and expects to be notified when a sender issues a paste needs to have a unique SPRN_TIDR value.
The SPRN_TIDR value only needs to unique within the process but for now we use a globally unique thread id as described below.
Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is assigned to only threads that need it; move assignment to restore_sprs(). Drop lint from rebase;
  arch/powerpc/include/asm/processor.h |  4 ++
  arch/powerpc/include/asm/switch_to.h |  3 ++
  arch/powerpc/kernel/process.c        | 97
++++++++++++++++++++++++++++++++++++
  3 files changed, 104 insertions(+)
arch/powerpc/include/asm/processor.h +4 -0
index fab7ff8..bf6ba63 100644
@@ -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
arch/powerpc/include/asm/switch_to.h +3 -0
index 17c8380..4962455 100644
@@ -91,4 +91,7 @@ static inline void clear_task_ebb(struct task_struct *t)
  #endif
  }
 
+extern void set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
  #endif /* _ASM_POWERPC_SWITCH_TO_H */
arch/powerpc/kernel/process.c +4 -0
index 1f0fd36..13abb22 100644
@@ -1132,6 +1132,10 @@ static inline void restore_sprs(struct thread_struct
 *old_thread,
  			mtspr(SPRN_TAR, new_thread->tar);
  	}
  #endif
+#ifdef CONFIG_PPC_VAS
+	if (old_thread->tidr != new_thread->tidr)
+		mtspr(SPRN_TIDR, new_thread->tidr);
+#endif
  }
 
  #ifdef CONFIG_PPC_BOOK3S_64
arch/powerpc/kernel/process.c +19 -0
@@ -1446,9 +1450,97 @@ 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.
+ *

Quoted 8 lines

Quoted 7 lines
Quoted 7 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1478
Quoted 7 lines
arch/powerpc/kernel/process.c +7 -0
+ * So try to assign globally unique thraed ids for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ * 	 For now, only threads that expect to be notified by the VAS
+ * 	 hardware need a TIDR value and we assign values > 0 for those.
+ */
+#define MAX_THREAD_CONTEXT	((1 << 15) - 2)
Why are you excluding ((1 << 15) - 1)?

You are right. I don't need to exclude that. Also, TIDR is a 16-bit (0:15 in VAS's Local Notify TID) value right? So, will change to

	#define MAX_THREAD_CONTEXT	((1 << 16) - 1)

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/powerpc/kernel/process.c +4 -0
+static int assign_thread_tidr(void)
+{
+	int index;
+	int err;

Quoted 8 lines

Quoted 7 lines
Quoted 7 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1489
Quoted 7 lines
arch/powerpc/kernel/process.c +7 -0
+
+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);
Why are you excluding 1?

I think the "_above" in the name is misleading. The function header for ida_get_new_above() says "above or equal" so 1 is not excluded?

Quoted 35 lines

arch/powerpc/kernel/process.c +35 -0

+	spin_unlock(&vas_thread_id_lock);
+
+	if (err == -EAGAIN)
+		goto again;
+	else if (err)
+		return err;
+
+	if (index > MAX_THREAD_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_tidr(int id)
+{
+	spin_lock(&vas_thread_id_lock);
+	ida_remove(&vas_thread_ida, id);
+	spin_unlock(&vas_thread_id_lock);
+}
+
+void clear_thread_tidr(struct task_struct *t)
+{
+	if (t->thread.tidr) {
+		free_thread_tidr(t->thread.tidr);
+		t->thread.tidr = 0;
+		mtspr(SPRN_TIDR, 0);
+	}
+}
+
+/*
+ * Assign an unique thread id for this thread and set it in the

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1530
Quoted 6 lines
arch/powerpc/kernel/process.c +6 -0
+ * thread structure. For now, we need this interface only for
+ * the current task.
+ */
+void set_thread_tidr(struct task_struct *t)
+{
+	WARN_ON(t != current);
CAPI does not have this restriction. It should be possible to assign a thread id to an arbitrary task.

I see. We (or caller) just have to figure out the locking for the task.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/powerpc/kernel/process.c +4 -0
+	t->thread.tidr = assign_thread_tidr();
+	mtspr(SPRN_TIDR, t->thread.tidr);
+}
+

Philippe Bergheaud

2017-09-01 09:18 UTC #5 lore raw

Quoted 130 lines · Sukadev Bhattiprolu

Quoted 121 lines · felix [[email protected]] wrote:
Quoted 7 lines · Sukadev Bhattiprolu
Quoted 4 lines · Michael Neuling [[email protected]] wrote:
Suka,
Please CC Christophe who as an alternative way of doing this. We ned to get agreement across all users of TIDR/AS_notify...
Mikey,
Quoted 8 lines
Thanks. There is overlap between the two patches. I will send a patch on top of Christophe's for the interfaces to assign/clear the TIDR value and clear the thread->tidr during arch_dup_task_struct(). I will also drop the CONFIG_VAS check since its not only for VAS.
Christophe, can you let me know of any other comments on this patch?
Suka
Suka,
I am seconding Christophe on this matter. I think that your patch now fulfills the CAPI use case requirements, with one exception: CAPI does not restrict assigning a thread id to the current task. Please find a few minor questions below.
Philippe
Quoted 96 lines
Quoted 96 lines
His patch is here:
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.ozlabs.org_pipermail_linuxppc-2Ddev_2017-2DAugust_161582.html&d=DwIFAw&c=jf_iaSHvJObTbx-siA1ZOg&r=KC0fX9VGJYXlSiH9qN2ZONDbh8vUCZFX8GUhF3rHAvg&m=XQenBfWewOBjWopgf1Fh2UAVGnlzq766MNuzx7jYfuA&s=07WOVTh9f_4IBZfCJes4lvc7LWenBlqVfAXIXxL2QH4&e=
Mikey
Quoted 90 lines · Sukadev Bhattiprolu
We need the SPRN_TIDR to be set for use with fast thread-wakeup (core-to-core wakeup) in VAS. Each user thread that has a receive window setup and expects to be notified when a sender issues a paste needs to have a unique SPRN_TIDR value.
The SPRN_TIDR value only needs to unique within the process but for now we use a globally unique thread id as described below.
Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
Changelog[v2]
- Michael Ellerman: Use an interface to assign TIDR so it is assigned to only threads that need it; move assignment to restore_sprs(). Drop lint from rebase;
   arch/powerpc/include/asm/processor.h |  4 ++
   arch/powerpc/include/asm/switch_to.h |  3 ++
   arch/powerpc/kernel/process.c        | 97
++++++++++++++++++++++++++++++++++++
   3 files changed, 104 insertions(+)
arch/powerpc/include/asm/processor.h +4 -0
index fab7ff8..bf6ba63 100644
@@ -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
arch/powerpc/include/asm/switch_to.h +3 -0
index 17c8380..4962455 100644
@@ -91,4 +91,7 @@ static inline void clear_task_ebb(struct task_struct *t)
   #endif
   }
 
+extern void set_thread_tidr(struct task_struct *t);
+extern void clear_thread_tidr(struct task_struct *t);
+
   #endif /* _ASM_POWERPC_SWITCH_TO_H */
arch/powerpc/kernel/process.c +4 -0
index 1f0fd36..13abb22 100644
@@ -1132,6 +1132,10 @@ static inline void restore_sprs(struct thread_struct
 *old_thread,
   			mtspr(SPRN_TAR, new_thread->tar);
   	}
   #endif
+#ifdef CONFIG_PPC_VAS
+	if (old_thread->tidr != new_thread->tidr)
+		mtspr(SPRN_TIDR, new_thread->tidr);
+#endif
   }
 
   #ifdef CONFIG_PPC_BOOK3S_64
arch/powerpc/kernel/process.c +19 -0
@@ -1446,9 +1450,97 @@ 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.
+ *
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1478
Quoted 7 lines
arch/powerpc/kernel/process.c +7 -0
+ * So try to assign globally unique thraed ids for now.
+ *
+ * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
+ * 	 For now, only threads that expect to be notified by the VAS
+ * 	 hardware need a TIDR value and we assign values > 0 for those.
+ */
+#define MAX_THREAD_CONTEXT	((1 << 15) - 2)
Why are you excluding ((1 << 15) - 1)?

Quoted 4 lines

You are right. I don't need to exclude that. Also, TIDR is a 16-bit (0:15 in VAS's Local Notify TID) value right? So, will change to
	#define MAX_THREAD_CONTEXT	((1 << 16) - 1)

Yes.

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 12 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1489
Quoted 12 lines
arch/powerpc/kernel/process.c +11 -0
+static int assign_thread_tidr(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);
Why are you excluding 1?

Quoted 2 lines

I think the "_above" in the name is misleading. The function header for ida_get_new_above() says "above or equal" so 1 is not excluded?

Understood.

Quoted 44 lines

Quoted 36 lines

arch/powerpc/kernel/process.c +35 -0

+	spin_unlock(&vas_thread_id_lock);
+
+	if (err == -EAGAIN)
+		goto again;
+	else if (err)
+		return err;
+
+	if (index > MAX_THREAD_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_tidr(int id)
+{
+	spin_lock(&vas_thread_id_lock);
+	ida_remove(&vas_thread_ida, id);
+	spin_unlock(&vas_thread_id_lock);
+}
+
+void clear_thread_tidr(struct task_struct *t)
+{
+	if (t->thread.tidr) {
+		free_thread_tidr(t->thread.tidr);
+		t->thread.tidr = 0;
+		mtspr(SPRN_TIDR, 0);
+	}
+}
+
+/*
+ * Assign an unique thread id for this thread and set it in the

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
arch/powerpc/kernel/process.c void flush_thread(void) · line 1530
Quoted 6 lines
arch/powerpc/kernel/process.c +6 -0
+ * thread structure. For now, we need this interface only for
+ * the current task.
+ */
+void set_thread_tidr(struct task_struct *t)
+{
+	WARN_ON(t != current);
CAPI does not have this restriction. It should be possible to assign a thread id to an arbitrary task.

Quoted 1 line

I see. We (or caller) just have to figure out the locking for the task.

The extension to any thread can be implemented in a separate patch. 'current' will do for now.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
arch/powerpc/kernel/process.c +4 -0
+	t->thread.tidr = assign_thread_tidr();
+	mtspr(SPRN_TIDR, t->thread.tidr);
+}
+