Messages in this thread | | | From | Nick Desaulniers <> | Date | Fri, 4 Jun 2021 10:28:45 -0700 | Subject | Re: [PATCH v3 1/1] pgo: Fix allocate_node() v2 |
| |
On Fri, Jun 4, 2021 at 10:03 AM Jarmo Tiitto <jarmo.tiitto@gmail.com> wrote: > > When clang instrumentation eventually calls allocate_node() > the struct llvm_prf_data *p argument tells us from what section > we should reserve the vnode: It either points into vmlinux's > core __llvm_prf_data section or some loaded module's > __llvm_prf_data section. > > But since we don't have access to corresponding > __llvm_prf_vnds section(s) for any module, the function > should return just NULL and ignore any profiling attempts > from modules for now. > > Signed-off-by: Jarmo Tiitto <jarmo.tiitto@gmail.com> > --- > Based on Kees and others feedback here is v3 patch > that clarifies why the current checks in allocate_node() > are flawed. I did fair amount of KGDB time on it. > > The commit is based on kees/for-next/clang/features tree, > hopefully this is ok. Should I have based it on linux-next > instead? > > I grep -R'd where the memory_contains() can be found and it is only > found in #include <asm-generic/sections.h> > > I cross my fingers and await if this is my first accepted patch. :-) > --- > kernel/pgo/instrument.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/kernel/pgo/instrument.c b/kernel/pgo/instrument.c > index 0e07ee1b17d9..c69b4f7ebaad 100644 > --- a/kernel/pgo/instrument.c > +++ b/kernel/pgo/instrument.c > @@ -18,6 +18,7 @@ > > #define pr_fmt(fmt) "pgo: " fmt > > +#include <asm-generic/sections.h>
^ What about Nathan's feedback on this being just `<asm/sections.h>`?
> #include <linux/bitops.h> > #include <linux/kernel.h> > #include <linux/export.h> > @@ -55,17 +56,21 @@ void prf_unlock(unsigned long flags) > static struct llvm_prf_value_node *allocate_node(struct llvm_prf_data *p, > u32 index, u64 value) > { > - if (&__llvm_prf_vnds_start[current_node + 1] >= __llvm_prf_vnds_end) > - return NULL; /* Out of nodes */ > - > - current_node++; > - > - /* Make sure the node is entirely within the section */ > - if (&__llvm_prf_vnds_start[current_node] >= __llvm_prf_vnds_end || > - &__llvm_prf_vnds_start[current_node + 1] > __llvm_prf_vnds_end) > + const int max_vnds = prf_vnds_count(); > + > + /* > + * Check that p is within vmlinux __llvm_prf_data section. > + * If not, don't allocate since we can't handle modules yet. > + */ > + if (!memory_contains(__llvm_prf_data_start, > + __llvm_prf_data_end, p, sizeof(*p))) > return NULL; > > - return &__llvm_prf_vnds_start[current_node]; > + if (WARN_ON_ONCE(current_node >= max_vnds)) > + return NULL; /* Out of nodes */ > + > + /* reserve vnode for vmlinux */ > + return &__llvm_prf_vnds_start[current_node++]; > } > > /* > > base-commit: 5d0cda65918279ada060417c5fecb7e86ccb3def > -- > 2.31.1 >
-- Thanks, ~Nick Desaulniers
| |