Messages in this thread |  | | Date | Thu, 9 Apr 2020 13:22:26 +0200 | From | Michal Suchánek <> | Subject | Re: [PATCH] powerpcs: perf: consolidate perf_callchain_user_64 and perf_callchain_user_32 |
| |
On Tue, Apr 07, 2020 at 07:21:06AM +0200, Christophe Leroy wrote: > > > Le 06/04/2020 à 23:00, Michal Suchanek a écrit : > > perf_callchain_user_64 and perf_callchain_user_32 are nearly identical. > > Consolidate into one function with thin wrappers. > > > > Suggested-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > arch/powerpc/perf/callchain.h | 24 +++++++++++++++++++++++- > > arch/powerpc/perf/callchain_32.c | 21 ++------------------- > > arch/powerpc/perf/callchain_64.c | 14 ++++---------- > > 3 files changed, 29 insertions(+), 30 deletions(-) > > > > diff --git a/arch/powerpc/perf/callchain.h b/arch/powerpc/perf/callchain.h > > index 7a2cb9e1181a..7540bb71cb60 100644 > > --- a/arch/powerpc/perf/callchain.h > > +++ b/arch/powerpc/perf/callchain.h > > @@ -2,7 +2,7 @@ > > #ifndef _POWERPC_PERF_CALLCHAIN_H > > #define _POWERPC_PERF_CALLCHAIN_H > > -int read_user_stack_slow(void __user *ptr, void *buf, int nb); > > +int read_user_stack_slow(const void __user *ptr, void *buf, int nb); > > Does the constification of ptr has to be in this patch ? It was in the original patch. The code is touched anyway. > Wouldn't it be better to have it as a separate patch ? Don't care much either way. Can resend it as separate patches. > > > void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry, > > struct pt_regs *regs); > > void perf_callchain_user_32(struct perf_callchain_entry_ctx *entry, > > @@ -16,4 +16,26 @@ static inline bool invalid_user_sp(unsigned long sp) > > return (!sp || (sp & mask) || (sp > top)); > > } > > +/* > > + * On 32-bit we just access the address and let hash_page create a > > + * HPTE if necessary, so there is no need to fall back to reading > > + * the page tables. Since this is called at interrupt level, > > + * do_page_fault() won't treat a DSI as a page fault. > > + */ > > +static inline int __read_user_stack(const void __user *ptr, void *ret, > > + size_t size) > > +{ > > + int rc; > > + > > + if ((unsigned long)ptr > TASK_SIZE - size || > > + ((unsigned long)ptr & (size - 1))) > > + return -EFAULT; > > + rc = probe_user_read(ret, ptr, size); > > + > > + if (rc && IS_ENABLED(CONFIG_PPC64)) > > gcc is probably smart enough to deal with it efficiently, but it would > be more correct to test rc after checking CONFIG_PPC64. IS_ENABLED(CONFIG_PPC64) is constant so that part of the check should be compiled out in any case.
Thanks
Michal
|  |