Messages in this thread | | | Subject | Re: [PATCH v2 3/7] powerpc: use task_pid_nr() for TID allocation | From | Andrew Donnellan <> | Date | Fri, 20 Apr 2018 18:43:55 +1000 |
| |
[+ Sukadev, Christophe]
On 18/04/18 11:08, Alastair D'Silva wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > The current implementation of TID allocation, using a global IDR, may > result in an errant process starving the system of available TIDs. > Instead, use task_pid_nr(), as mentioned by the original author. The > scenario described which prevented it's use is not applicable, as > set_thread_tidr can only be called after the task struct has been > populated. > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
So it's too late in the evening for me to completely get my head around what's going on here enough to give my Reviewed-by:, but my current thinking is:
- In the first version of the patch to add TIDR support (https://patchwork.ozlabs.org/patch/799494/), it was originally proposed to call assign_thread_id() (as it was then called) from copy_thread()
- The comment block documents the reason why we can't use task_pid_nr() but assumes that we're trying to assign a TIDR from within copy_thread()
- The final patch that was accepted (https://patchwork.ozlabs.org/patch/835552/, ec233ede4c8654894610ea54f4dae7adc954ac62) instead sets the TIDR to 0 from copy_thread(), so the original reasoning regarding not using task_pid_nr() within copy_thread() is no longer applicable.
Sukadev: does this sound right?
Andrew
> --- > arch/powerpc/include/asm/switch_to.h | 1 - > arch/powerpc/kernel/process.c | 97 +----------------------------------- > 2 files changed, 1 insertion(+), 97 deletions(-) > > diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h > index be8c9fa23983..5b03d8a82409 100644 > --- a/arch/powerpc/include/asm/switch_to.h > +++ b/arch/powerpc/include/asm/switch_to.h > @@ -94,6 +94,5 @@ static inline void clear_task_ebb(struct task_struct *t) > extern int set_thread_uses_vas(void); > > extern int 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 3b00da47699b..87f047fd2762 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -1496,103 +1496,12 @@ int set_thread_uses_vas(void) > } > > #ifdef CONFIG_PPC64 > -static DEFINE_SPINLOCK(vas_thread_id_lock); > -static DEFINE_IDA(vas_thread_ida); > - > -/* > - * We need to assign a unique thread id to each thread in a process. > - * > - * This thread id, referred to as TIDR, and separate from the Linux's tgid, > - * is intended to be used to direct an ASB_Notify from the hardware to the > - * thread, when a suitable event occurs in the system. > - * > - * One such event is a "paste" instruction in the context of Fast Thread > - * Wakeup (aka Core-to-core wake up in the Virtual Accelerator Switchboard > - * (VAS) in POWER9. > - * > - * To get a unique TIDR per process we could simply reuse task_pid_nr() but > - * the problem is that task_pid_nr() is not yet available copy_thread() is > - * called. Fixing that would require changing more intrusive arch-neutral > - * code in code path in copy_process()?. > - * > - * Further, to assign unique TIDRs 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 TIDRs 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 << 16) - 1) > -static int assign_thread_tidr(void) > -{ > - int index; > - int err; > - unsigned long flags; > - > -again: > - if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL)) > - return -ENOMEM; > - > - spin_lock_irqsave(&vas_thread_id_lock, flags); > - err = ida_get_new_above(&vas_thread_ida, 1, &index); > - spin_unlock_irqrestore(&vas_thread_id_lock, flags); > - > - if (err == -EAGAIN) > - goto again; > - else if (err) > - return err; > - > - if (index > MAX_THREAD_CONTEXT) { > - spin_lock_irqsave(&vas_thread_id_lock, flags); > - ida_remove(&vas_thread_ida, index); > - spin_unlock_irqrestore(&vas_thread_id_lock, flags); > - return -ENOMEM; > - } > - > - return index; > -} > - > -static void free_thread_tidr(int id) > -{ > - unsigned long flags; > - > - spin_lock_irqsave(&vas_thread_id_lock, flags); > - ida_remove(&vas_thread_ida, id); > - spin_unlock_irqrestore(&vas_thread_id_lock, flags); > -} > - > -/* > - * Clear any TIDR value assigned to this thread. > - */ > -void clear_thread_tidr(struct task_struct *t) > -{ > - if (!t->thread.tidr) > - return; > - > - if (!cpu_has_feature(CPU_FTR_P9_TIDR)) { > - WARN_ON_ONCE(1); > - return; > - } > - > - mtspr(SPRN_TIDR, 0); > - free_thread_tidr(t->thread.tidr); > - t->thread.tidr = 0; > -} > - > -void arch_release_task_struct(struct task_struct *t) > -{ > - clear_thread_tidr(t); > -} > - > /* > * Assign a unique TIDR (thread id) for task @t and set it in the thread > * structure. For now, we only support setting TIDR for 'current' task. > */ > int set_thread_tidr(struct task_struct *t) > { > - int rc; > - > if (!cpu_has_feature(CPU_FTR_P9_TIDR)) > return -EINVAL; > > @@ -1602,11 +1511,7 @@ int set_thread_tidr(struct task_struct *t) > if (t->thread.tidr) > return 0; > > - rc = assign_thread_tidr(); > - if (rc < 0) > - return rc; > - > - t->thread.tidr = rc; > + t->thread.tidr = (u16)task_pid_nr(t); > mtspr(SPRN_TIDR, t->thread.tidr); > > return 0; >
-- Andrew Donnellan OzLabs, ADL Canberra andrew.donnellan@au1.ibm.com IBM Australia Limited
| |