[PATCH RFC] Interface to set SPRN_TIDR

[Philippe Bergheaud]

On 31/08/2017 20:06, Sukadev Bhattiprolu wrote:
> felix [felix at linux.vnet.ibm.com] wrote:
>> On 31/08/2017 01:32, Sukadev Bhattiprolu wrote:
>>> Michael Neuling [mikey at 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 <sukadev at 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)
Yes.
>
>>>>> +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?
Understood.
>
>>>>> +	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.
The extension to any thread can be implemented in a separate patch. 
'current' will do for now.
>
>>>>> +	t->thread.tidr = assign_thread_tidr();
>>>>> +	mtspr(SPRN_TIDR, t->thread.tidr);
>>>>> +}
>>>>> +