| Message ID | 1455613198-5113-3-git-send-email-khandual@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 16 Feb 2016 14:29:32 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> 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 <khandual@linux.vnet.ibm.com> > --- > 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 <khandual@linux.vnet.ibm.com> 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 <khandual@linux.vnet.ibm.com> >> --- >> 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 <khandual@linux.vnet.ibm.com> wrote: > On 03/02/2016 05:45 AM, Cyril Bur wrote: > > On Tue, 16 Feb 2016 14:29:32 +0530 > > Anshuman Khandual <khandual@linux.vnet.ibm.com> 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 <khandual@linux.vnet.ibm.com> > >> --- > >> 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 >
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(). + */ +} +#endif + struct task_struct *__switch_to(struct task_struct *prev, struct task_struct *new) {
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. Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com> --- arch/powerpc/include/asm/switch_to.h | 8 ++++++++ arch/powerpc/kernel/process.c | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+)