Re: [PATCH v6 14/17] powerpc: Add support for setting SPRN_TIDR

[Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>]

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