| Message ID | 1502233622-9330-15-git-send-email-sukadev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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)) {
Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> 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
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.
Benjamin Herrenschmidt [benh@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 != &init_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.
Michael Ellerman [mpe@ellerman.id.au] wrote: > Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> 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
On Mon, 2017-08-14 at 21:16 +1000, Michael Ellerman wrote: > Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> 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
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.
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(&vas_thread_ida, GFP_KERNEL)) + return -ENOMEM; + + spin_lock(&vas_thread_id_lock); + err = ida_get_new_above(&vas_thread_ida, 1, &index); + 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 } /* @@ -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)) {
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(+)