On Tue, 16 Feb 2016 14:29:32 +0530
Anshuman Khandual <[email protected]> wrote:
> This patch creates a function flush_tmregs_to_thread which
> will then be used by subsequent patches in this series. The
> function checks for self tracing ptrace interface attempts
> while in the TM context and logs appropriate warning message.
>
Hi Anshuman,
You'll have to bare with me, my ptrace knowledge is non existent so you might
have to walk me though some aspects.
I have been playing with FPU/VMX and VSX saving so I thought I'd take a look.
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> arch/powerpc/include/asm/switch_to.h | 8 ++++++++
> arch/powerpc/kernel/process.c | 20 ++++++++++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
> index 5b268b6..7b297bf 100644
> --- a/arch/powerpc/include/asm/switch_to.h
> +++ b/arch/powerpc/include/asm/switch_to.h
> @@ -70,6 +70,14 @@ static inline void disable_kernel_spe(void)
> }
> #endif
>
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +extern void flush_tmregs_to_thread(struct task_struct *);
> +#else
> +static inline void flush_tmregs_to_thread(struct task_struct *t)
> +{
> +}
> +#endif
> +
> static inline void clear_task_ebb(struct task_struct *t)
> {
> #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index dccc87e..2c4fa7f 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -918,6 +918,26 @@ static inline void restore_sprs(struct thread_struct *old_thread,
> #endif
> }
>
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +void flush_tmregs_to_thread(struct task_struct *tsk)
> +{
> + /*
> + * Process self tracing is not yet supported through
> + * ptrace interface. Ptrace generic code should have
> + * prevented this from happening in the first place.
> + * Warn once here with the message, if some how it
> + * is attempted.
> + */
> + WARN_ONCE(tsk == current,
> + "Not expecting ptrace on self: TM regs may be incorrect\n");
> +
> + /*
> + * If task is not current, it should have been flushed
> + * already to it's thread_struct during __switch_to().
> + */
I totally agree except this highlights something that I notice in subsequent
patches, and existing code. All the *_{get,set}() functions call
flush_*_to_thread() when, as per your comment (and my understanding of task
switching) there really shouldn't be a need to do that. My only thought is that
this could be a relic of uniprocessor days when it would have been necessary but
Anton recently stripped that out. Are you able to shed some light here?
The reason I ask is that if the flush_*_to_thread() calls ARE actually
important then I worry that this function is inadequate...
> +}
> +#endif
> +
> struct task_struct *__switch_to(struct task_struct *prev,
> struct task_struct *new)
> {
On 03/02/2016 05:45 AM, Cyril Bur wrote:
> On Tue, 16 Feb 2016 14:29:32 +0530
> Anshuman Khandual <[email protected]> wrote:
>
>> This patch creates a function flush_tmregs_to_thread which
>> will then be used by subsequent patches in this series. The
>> function checks for self tracing ptrace interface attempts
>> while in the TM context and logs appropriate warning message.
>>
>
> Hi Anshuman,
>
> You'll have to bare with me, my ptrace knowledge is non existent so you might
> have to walk me though some aspects.
>
> I have been playing with FPU/VMX and VSX saving so I thought I'd take a look.
Sure.
>
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> arch/powerpc/include/asm/switch_to.h | 8 ++++++++
>> arch/powerpc/kernel/process.c | 20 ++++++++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
>> index 5b268b6..7b297bf 100644
>> --- a/arch/powerpc/include/asm/switch_to.h
>> +++ b/arch/powerpc/include/asm/switch_to.h
>> @@ -70,6 +70,14 @@ static inline void disable_kernel_spe(void)
>> }
>> #endif
>>
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +extern void flush_tmregs_to_thread(struct task_struct *);
>> +#else
>> +static inline void flush_tmregs_to_thread(struct task_struct *t)
>> +{
>> +}
>> +#endif
>> +
>> static inline void clear_task_ebb(struct task_struct *t)
>> {
>> #ifdef CONFIG_PPC_BOOK3S_64
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index dccc87e..2c4fa7f 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -918,6 +918,26 @@ static inline void restore_sprs(struct thread_struct *old_thread,
>> #endif
>> }
>>
>
>
>
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +void flush_tmregs_to_thread(struct task_struct *tsk)
>> +{
>> + /*
>> + * Process self tracing is not yet supported through
>> + * ptrace interface. Ptrace generic code should have
>> + * prevented this from happening in the first place.
>> + * Warn once here with the message, if some how it
>> + * is attempted.
>> + */
>> + WARN_ONCE(tsk == current,
>> + "Not expecting ptrace on self: TM regs may be incorrect\n");
>> +
>> + /*
>> + * If task is not current, it should have been flushed
>> + * already to it's thread_struct during __switch_to().
>> + */
>
> I totally agree except this highlights something that I notice in subsequent
> patches, and existing code. All the *_{get,set}() functions call
> flush_*_to_thread() when, as per your comment (and my understanding of task
> switching) there really shouldn't be a need to do that. My only thought is that
> this could be a relic of uniprocessor days when it would have been necessary but
> Anton recently stripped that out. Are you able to shed some light here?
Its been sometime I had looked into this aspect of the series. I remember
Michael Neuling and myself discussed about this and settled on a single
WARN_ON here as nothing else was required to be done in the function. It
may be possible that all the flush_*_to_thread() functions used else where
are because of uniprocessor concerns. I dont understand completely our
context save/restore paths including the lazy ones. I believed that these
flush_*_to_thread() routines just made sure task struct has the latest
values of the thread context in case of some complicated save/restore
paths might not have done the complete save at that point in time.
If you think that all these flush_*_to_thread() functions used through
out POWER ptrace need review to see whether they are required or not
anymore I would suggest we should do it as a separate patch after this
series and I am willing to work with you on that.
>
> The reason I ask is that if the flush_*_to_thread() calls ARE actually
> important then I worry that this function is inadequate...
I guess we went through that and finally settled on WARN_ON once but dont
remember the exact context now. Will look into all previous discussions
on this and get back.
On Wed, 02 Mar 2016 09:59:06 +0530
Anshuman Khandual <[email protected]> wrote:
> On 03/02/2016 05:45 AM, Cyril Bur wrote:
> > On Tue, 16 Feb 2016 14:29:32 +0530
> > Anshuman Khandual <[email protected]> wrote:
> >
> >> This patch creates a function flush_tmregs_to_thread which
> >> will then be used by subsequent patches in this series. The
> >> function checks for self tracing ptrace interface attempts
> >> while in the TM context and logs appropriate warning message.
> >>
> >
> > Hi Anshuman,
> >
> > You'll have to bare with me, my ptrace knowledge is non existent so you might
> > have to walk me though some aspects.
> >
> > I have been playing with FPU/VMX and VSX saving so I thought I'd take a look.
>
> Sure.
>
>
> >
> >> Signed-off-by: Anshuman Khandual <[email protected]>
> >> ---
> >> arch/powerpc/include/asm/switch_to.h | 8 ++++++++
> >> arch/powerpc/kernel/process.c | 20 ++++++++++++++++++++
> >> 2 files changed, 28 insertions(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
> >> index 5b268b6..7b297bf 100644
> >> --- a/arch/powerpc/include/asm/switch_to.h
> >> +++ b/arch/powerpc/include/asm/switch_to.h
> >> @@ -70,6 +70,14 @@ static inline void disable_kernel_spe(void)
> >> }
> >> #endif
> >>
> >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >> +extern void flush_tmregs_to_thread(struct task_struct *);
> >> +#else
> >> +static inline void flush_tmregs_to_thread(struct task_struct *t)
> >> +{
> >> +}
> >> +#endif
> >> +
> >> static inline void clear_task_ebb(struct task_struct *t)
> >> {
> >> #ifdef CONFIG_PPC_BOOK3S_64
> >> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> >> index dccc87e..2c4fa7f 100644
> >> --- a/arch/powerpc/kernel/process.c
> >> +++ b/arch/powerpc/kernel/process.c
> >> @@ -918,6 +918,26 @@ static inline void restore_sprs(struct thread_struct *old_thread,
> >> #endif
> >> }
> >>
> >
> >
> >
> >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >> +void flush_tmregs_to_thread(struct task_struct *tsk)
> >> +{
> >> + /*
> >> + * Process self tracing is not yet supported through
> >> + * ptrace interface. Ptrace generic code should have
> >> + * prevented this from happening in the first place.
> >> + * Warn once here with the message, if some how it
> >> + * is attempted.
> >> + */
> >> + WARN_ONCE(tsk == current,
> >> + "Not expecting ptrace on self: TM regs may be incorrect\n");
> >> +
> >> + /*
> >> + * If task is not current, it should have been flushed
> >> + * already to it's thread_struct during __switch_to().
> >> + */
> >
> > I totally agree except this highlights something that I notice in subsequent
> > patches, and existing code. All the *_{get,set}() functions call
> > flush_*_to_thread() when, as per your comment (and my understanding of task
> > switching) there really shouldn't be a need to do that. My only thought is that
> > this could be a relic of uniprocessor days when it would have been necessary but
> > Anton recently stripped that out. Are you able to shed some light here?
>
> Its been sometime I had looked into this aspect of the series. I remember
> Michael Neuling and myself discussed about this and settled on a single
> WARN_ON here as nothing else was required to be done in the function. It
> may be possible that all the flush_*_to_thread() functions used else where
> are because of uniprocessor concerns. I dont understand completely our
> context save/restore paths including the lazy ones. I believed that these
> flush_*_to_thread() routines just made sure task struct has the latest
> values of the thread context in case of some complicated save/restore
> paths might not have done the complete save at that point in time.
>
Well as you note in the comment though, it should be done since we've gone
through __switch_to()...
> If you think that all these flush_*_to_thread() functions used through
> out POWER ptrace need review to see whether they are required or not
> anymore I would suggest we should do it as a separate patch after this
> series and I am willing to work with you on that.
I THINK your patches are correct and we're just performing needless
flush_*_to_thread() calls now in which case its fine and we can review laster,
my concern is that I've been wrong before so having flush_tmregs_to_thread() do
nothing worries me. I wonder if Mr Neuling or Mr Ellerman have anything to say
on the subject...
>
> >
> > The reason I ask is that if the flush_*_to_thread() calls ARE actually
> > important then I worry that this function is inadequate...
>
> I guess we went through that and finally settled on WARN_ON once but dont
> remember the exact context now. Will look into all previous discussions
> on this and get back.
Thanks,
Cyril
>