felix [fe...@linux.vnet.ibm.com] wrote: > On 31/08/2017 01:32, Sukadev Bhattiprolu wrote: > > Michael Neuling [mi...@neuling.org] 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, > > > > 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 > > > > 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 > > > > > > On Tue, 2017-08-29 at 19:38 -0700, Sukadev Bhattiprolu wrote: > > > > 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 <suka...@linux.vnet.ibm.com> > > > > --- > > > > 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(+) > > > > > > > > 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/include/asm/switch_to.h > > > > b/arch/powerpc/include/asm/switch_to.h > > > > index 17c8380..4962455 100644 > > > > --- a/arch/powerpc/include/asm/switch_to.h > > > > +++ b/arch/powerpc/include/asm/switch_to.h > > > > @@ -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 */ > > > > diff --git a/arch/powerpc/kernel/process.c > > > > b/arch/powerpc/kernel/process.c > > > > index 1f0fd36..13abb22 100644 > > > > --- a/arch/powerpc/kernel/process.c > > > > +++ b/arch/powerpc/kernel/process.c > > > > @@ -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 > > > > @@ -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. > > > > + * > > > > + * 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) > > > > +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? 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? > > > > + 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 > > > > + * 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. > > > > + t->thread.tidr = assign_thread_tidr(); > > > > + mtspr(SPRN_TIDR, t->thread.tidr); > > > > +} > > > > +