Messages in this thread | | | From | Nick Desaulniers <> | Date | Thu, 3 Jun 2021 13:50:17 -0700 | Subject | Re: [PATCH v2 1/1] pgo: Fix allocate_node() v2 |
| |
On Thu, Jun 3, 2021 at 6:41 AM Jarmo Tiitto <jarmo.tiitto@gmail.com> wrote: > > Based on Kees and others feedback here is v2 patch > that clarifies why the current checks in allocate_node() > are flawed. I did fair amount of KGDB time on it.
Kees can probably cut it when merging, but the above paragraph might be better "below the fold" below next time (doesn't necessitate a v3).
> > 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> > ---
^ ie. here. If you put text between the `---` and the diffstat, git just discards it when applying. It's a good way to hide commentary just meant for reviewers when sending a patch.
> kernel/pgo/instrument.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/kernel/pgo/instrument.c b/kernel/pgo/instrument.c > index 0e07ee1b17d9..afe9982b07a3 100644 > --- a/kernel/pgo/instrument.c > +++ b/kernel/pgo/instrument.c > @@ -23,6 +23,7 @@ > #include <linux/export.h> > #include <linux/spinlock.h> > #include <linux/types.h> > +#include <asm-generic/sections.h> > #include "pgo.h" > > /* > @@ -55,17 +56,19 @@ 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();
Sorry, where was prf_vnds_count() defined? I don't see it in linux-next, but I'm also not sure which tree has 5d0cda65918279ada060417c5fecb7e86ccb3def.
> + /* 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
| |