This patch series is minor fixes and cleanup for bpf load and samples
code. The first one patch is typo fixing; patch 0002 is refactor for
dynamically allocate memory for kernel symbol structures; the last
three patches are mainly related with refactor with function
ksym_search(), the main benefit of this refactor is program sampleip
can be used without architecture dependency.
The patch series has been tested on ARM64 Hikey960 boards.
Leo Yan (5):
samples/bpf: Fix typo in comment
samples/bpf: Dynamically allocate structure 'syms'
samples/bpf: Use NULL for failed to find symbol
samples/bpf: Refine printing symbol for sampleip
samples/bpf: Handle NULL pointer returned by ksym_search()
samples/bpf/bpf_load.c | 29 +++++++++++++++++++++++------
samples/bpf/offwaketime_user.c | 5 +++++
samples/bpf/sampleip_user.c | 8 +++-----
samples/bpf/spintest_user.c | 5 ++++-
samples/bpf/trace_event_user.c | 5 +++++
5 files changed, 40 insertions(+), 12 deletions(-)
--
1.9.1
The code defines macro 'PAGE_OFFSET' and uses it to decide if the
address is in kernel space or not. But different architecture has
different 'PAGE_OFFSET' so this program cannot be used for all
platforms.
This commit changes to check returned pointer from ksym_search() to
judge if the address falls into kernel space or not, and removes
macro 'PAGE_OFFSET' as it isn't used anymore. As result, this program
has no architecture dependency.
Signed-off-by: Leo Yan <[email protected]>
---
samples/bpf/sampleip_user.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index 4ed690b..0eea1b3 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c
@@ -26,7 +26,6 @@
#define DEFAULT_FREQ 99
#define DEFAULT_SECS 5
#define MAX_IPS 8192
-#define PAGE_OFFSET 0xffff880000000000
static int nr_cpus;
@@ -107,14 +106,13 @@ static void print_ip_map(int fd)
/* sort and print */
qsort(counts, max, sizeof(struct ipcount), count_cmp);
for (i = 0; i < max; i++) {
- if (counts[i].ip > PAGE_OFFSET) {
- sym = ksym_search(counts[i].ip);
+ sym = ksym_search(counts[i].ip);
+ if (sym)
printf("0x%-17llx %-32s %u\n", counts[i].ip, sym->name,
counts[i].count);
- } else {
+ else
printf("0x%-17llx %-32s %u\n", counts[i].ip, "(user)",
counts[i].count);
- }
}
if (max == MAX_IPS) {
--
1.9.1
Structure 'syms' is used to store kernel symbol info by reading proc fs
node '/proc/kallsyms', this structure is declared with 300000 entries
and static linked into bss section. For most case the kernel symbols
has less than 300000 entries, so it's safe to define so large array, but
the side effect is bss section is big introduced by this structure and
it isn't flexible.
To fix this, this patch dynamically allocates memory for structure
'syms' based on parsing '/proc/kallsyms' line number at the runtime,
which can save elf file required memory significantly.
Before:
text data bss dec hex filename
18841 1172 5199776 5219789 4fa5cd samples/bpf/sampleip
After:
text data bss dec hex filename
19101 1188 399792 420081 668f1 samples/bpf/sampleip
Signed-off-by: Leo Yan <[email protected]>
---
samples/bpf/bpf_load.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 28e4678..c2bf7ca 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -651,8 +651,7 @@ void read_trace_pipe(void)
}
}
-#define MAX_SYMS 300000
-static struct ksym syms[MAX_SYMS];
+static struct ksym *syms;
static int sym_cnt;
static int ksym_cmp(const void *p1, const void *p2)
@@ -678,12 +677,30 @@ int load_kallsyms(void)
break;
if (!addr)
continue;
+ sym_cnt++;
+ }
+
+ syms = calloc(sym_cnt, sizeof(*syms));
+ if (!syms) {
+ fclose(f);
+ return -ENOMEM;
+ }
+
+ rewind(f);
+ while (!feof(f)) {
+ if (!fgets(buf, sizeof(buf), f))
+ break;
+ if (sscanf(buf, "%p %c %s", &addr, &symbol, func) != 3)
+ break;
+ if (!addr)
+ continue;
syms[i].addr = (long) addr;
syms[i].name = strdup(func);
i++;
}
- sym_cnt = i;
qsort(syms, sym_cnt, sizeof(struct ksym), ksym_cmp);
+
+ fclose(f);
return 0;
}
--
1.9.1
This commit handles NULL pointer returned by ksym_search() to directly
print address hexadecimal value, the change is applied in 'trace_event',
'spintest' and 'offwaketime' programs.
Signed-off-by: Leo Yan <[email protected]>
---
samples/bpf/offwaketime_user.c | 5 +++++
samples/bpf/spintest_user.c | 5 ++++-
samples/bpf/trace_event_user.c | 5 +++++
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c
index 512f87a..fce2113 100644
--- a/samples/bpf/offwaketime_user.c
+++ b/samples/bpf/offwaketime_user.c
@@ -27,6 +27,11 @@ static void print_ksym(__u64 addr)
if (!addr)
return;
sym = ksym_search(addr);
+ if (!sym) {
+ printf("%llx;", addr);
+ return;
+ }
+
if (PRINT_RAW_ADDR)
printf("%s/%llx;", sym->name, addr);
else
diff --git a/samples/bpf/spintest_user.c b/samples/bpf/spintest_user.c
index 3d73621..3140803 100644
--- a/samples/bpf/spintest_user.c
+++ b/samples/bpf/spintest_user.c
@@ -36,7 +36,10 @@ int main(int ac, char **argv)
bpf_map_lookup_elem(map_fd[0], &next_key, &value);
assert(next_key == value);
sym = ksym_search(value);
- printf(" %s", sym->name);
+ if (!sym)
+ printf(" %lx", value);
+ else
+ printf(" %s", sym->name);
key = next_key;
}
if (key)
diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index 56f7a25..d2ab33e 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c
@@ -33,6 +33,11 @@ static void print_ksym(__u64 addr)
if (!addr)
return;
sym = ksym_search(addr);
+ if (!sym) {
+ printf("%llx;", addr);
+ return;
+ }
+
printf("%s;", sym->name);
if (!strcmp(sym->name, "sys_read"))
sys_read_seen = true;
--
1.9.1
Fix typo by replacing 'iif' with 'if'.
Signed-off-by: Leo Yan <[email protected]>
---
samples/bpf/bpf_load.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index bebe418..28e4678 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -393,7 +393,7 @@ static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
continue;
if (sym[nr_maps].st_shndx != maps_shndx)
continue;
- /* Only increment iif maps section */
+ /* Only increment if maps section */
nr_maps++;
}
--
1.9.1
Function ksym_search() is used to parse address and return the symbol
structure, when the address is out of range for kernel symbols it
returns the symbol structure of kernel '_stext' entry; this introduces
confusion and it misses the chance to intuitively tell the address is
out of range.
This commit changes to use NULL pointer for failed to find symbol, user
functions need to check the pointer is NULL and get to know the address
has no corresponding kernel symbol for it.
Signed-off-by: Leo Yan <[email protected]>
---
samples/bpf/bpf_load.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index c2bf7ca..0c0584f 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -726,7 +726,7 @@ struct ksym *ksym_search(long key)
/* valid ksym */
return &syms[start - 1];
- /* out of range. return _stext */
- return &syms[0];
+ /* out of range. return NULL */
+ return NULL;
}
--
1.9.1
On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> address is in kernel space or not. But different architecture has
> different 'PAGE_OFFSET' so this program cannot be used for all
> platforms.
>
> This commit changes to check returned pointer from ksym_search() to
> judge if the address falls into kernel space or not, and removes
> macro 'PAGE_OFFSET' as it isn't used anymore. As result, this program
> has no architecture dependency.
>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> samples/bpf/sampleip_user.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> index 4ed690b..0eea1b3 100644
> --- a/samples/bpf/sampleip_user.c
> +++ b/samples/bpf/sampleip_user.c
> @@ -26,7 +26,6 @@
> #define DEFAULT_FREQ 99
> #define DEFAULT_SECS 5
> #define MAX_IPS 8192
> -#define PAGE_OFFSET 0xffff880000000000
>
> static int nr_cpus;
>
> @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> /* sort and print */
> qsort(counts, max, sizeof(struct ipcount), count_cmp);
> for (i = 0; i < max; i++) {
> - if (counts[i].ip > PAGE_OFFSET) {
> - sym = ksym_search(counts[i].ip);
yes. it is x64 specific, since it's a sample code,
but simply removing it is not a fix.
It makes this sampleip code behaving incorrectly.
To do such 'cleanup of ksym' please refactor it in the true generic way,
so these ksym* helpers can work on all archs and put this new
functionality into selftests.
If you can convert these examples into proper self-checking tests
that run out of selftests that would be awesome.
Thanks!
On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > address is in kernel space or not. But different architecture has
> > different 'PAGE_OFFSET' so this program cannot be used for all
> > platforms.
> >
> > This commit changes to check returned pointer from ksym_search() to
> > judge if the address falls into kernel space or not, and removes
> > macro 'PAGE_OFFSET' as it isn't used anymore. As result, this program
> > has no architecture dependency.
> >
> > Signed-off-by: Leo Yan <[email protected]>
> > ---
> > samples/bpf/sampleip_user.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > index 4ed690b..0eea1b3 100644
> > --- a/samples/bpf/sampleip_user.c
> > +++ b/samples/bpf/sampleip_user.c
> > @@ -26,7 +26,6 @@
> > #define DEFAULT_FREQ 99
> > #define DEFAULT_SECS 5
> > #define MAX_IPS 8192
> > -#define PAGE_OFFSET 0xffff880000000000
> >
> > static int nr_cpus;
> >
> > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > /* sort and print */
> > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > for (i = 0; i < max; i++) {
> > - if (counts[i].ip > PAGE_OFFSET) {
> > - sym = ksym_search(counts[i].ip);
>
> yes. it is x64 specific, since it's a sample code,
> but simply removing it is not a fix.
> It makes this sampleip code behaving incorrectly.
> To do such 'cleanup of ksym' please refactor it in the true generic way,
> so these ksym* helpers can work on all archs and put this new
> functionality into selftests.
Just want to check, are you suggesting to create a standalone
testing for kallsyms (like a folder tools/testing/selftests/ksym) or
do you mean to place the generic code into folder
tools/testing/selftests/bpf/?
> If you can convert these examples into proper self-checking tests
> that run out of selftests that would be awesome.
Thank you for suggestions, Alexei.
> Thanks!
>
On Thu, Apr 19, 2018 at 01:12:49PM +0800, Leo Yan wrote:
> On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> > On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > > address is in kernel space or not. But different architecture has
> > > different 'PAGE_OFFSET' so this program cannot be used for all
> > > platforms.
> > >
> > > This commit changes to check returned pointer from ksym_search() to
> > > judge if the address falls into kernel space or not, and removes
> > > macro 'PAGE_OFFSET' as it isn't used anymore. As result, this program
> > > has no architecture dependency.
> > >
> > > Signed-off-by: Leo Yan <[email protected]>
> > > ---
> > > samples/bpf/sampleip_user.c | 8 +++-----
> > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > > index 4ed690b..0eea1b3 100644
> > > --- a/samples/bpf/sampleip_user.c
> > > +++ b/samples/bpf/sampleip_user.c
> > > @@ -26,7 +26,6 @@
> > > #define DEFAULT_FREQ 99
> > > #define DEFAULT_SECS 5
> > > #define MAX_IPS 8192
> > > -#define PAGE_OFFSET 0xffff880000000000
> > >
> > > static int nr_cpus;
> > >
> > > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > > /* sort and print */
> > > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > > for (i = 0; i < max; i++) {
> > > - if (counts[i].ip > PAGE_OFFSET) {
> > > - sym = ksym_search(counts[i].ip);
> >
> > yes. it is x64 specific, since it's a sample code,
> > but simply removing it is not a fix.
> > It makes this sampleip code behaving incorrectly.
> > To do such 'cleanup of ksym' please refactor it in the true generic way,
> > so these ksym* helpers can work on all archs and put this new
> > functionality into selftests.
>
> Just want to check, are you suggesting to create a standalone
> testing for kallsyms (like a folder tools/testing/selftests/ksym) or
> do you mean to place the generic code into folder
> tools/testing/selftests/bpf/?
I think the minimal first step is to cleanup ksym bits into something
that bpf selftests can use and keep it as new .c in
tools/testing/selftests/bpf/
Later if it becomes useful for other tests in selftests it can be moved.
Thanks!
On Wed, Apr 18, 2018 at 10:21:25PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 19, 2018 at 01:12:49PM +0800, Leo Yan wrote:
> > On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > > > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > > > address is in kernel space or not. But different architecture has
> > > > different 'PAGE_OFFSET' so this program cannot be used for all
> > > > platforms.
> > > >
> > > > This commit changes to check returned pointer from ksym_search() to
> > > > judge if the address falls into kernel space or not, and removes
> > > > macro 'PAGE_OFFSET' as it isn't used anymore. As result, this program
> > > > has no architecture dependency.
> > > >
> > > > Signed-off-by: Leo Yan <[email protected]>
> > > > ---
> > > > samples/bpf/sampleip_user.c | 8 +++-----
> > > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > > > index 4ed690b..0eea1b3 100644
> > > > --- a/samples/bpf/sampleip_user.c
> > > > +++ b/samples/bpf/sampleip_user.c
> > > > @@ -26,7 +26,6 @@
> > > > #define DEFAULT_FREQ 99
> > > > #define DEFAULT_SECS 5
> > > > #define MAX_IPS 8192
> > > > -#define PAGE_OFFSET 0xffff880000000000
> > > >
> > > > static int nr_cpus;
> > > >
> > > > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > > > /* sort and print */
> > > > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > > > for (i = 0; i < max; i++) {
> > > > - if (counts[i].ip > PAGE_OFFSET) {
> > > > - sym = ksym_search(counts[i].ip);
> > >
> > > yes. it is x64 specific, since it's a sample code,
> > > but simply removing it is not a fix.
> > > It makes this sampleip code behaving incorrectly.
> > > To do such 'cleanup of ksym' please refactor it in the true generic way,
> > > so these ksym* helpers can work on all archs and put this new
> > > functionality into selftests.
> >
> > Just want to check, are you suggesting to create a standalone
> > testing for kallsyms (like a folder tools/testing/selftests/ksym) or
> > do you mean to place the generic code into folder
> > tools/testing/selftests/bpf/?
>
> I think the minimal first step is to cleanup ksym bits into something
> that bpf selftests can use and keep it as new .c in
> tools/testing/selftests/bpf/
> Later if it becomes useful for other tests in selftests it can be moved.
Thanks for explanation, now it's clear for me :)
> Thanks!
>
On Thu, 19 Apr 2018 09:34:02 +0800 Leo Yan <[email protected]> wrote:
> Fix typo by replacing 'iif' with 'if'.
>
> Signed-off-by: Leo Yan <[email protected]>
> ---
> samples/bpf/bpf_load.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> index bebe418..28e4678 100644
> --- a/samples/bpf/bpf_load.c
> +++ b/samples/bpf/bpf_load.c
> @@ -393,7 +393,7 @@ static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
> continue;
> if (sym[nr_maps].st_shndx != maps_shndx)
> continue;
> - /* Only increment iif maps section */
> + /* Only increment if maps section */
> nr_maps++;
> }
This was actually not a typo from my side.
With 'iif' I mean 'if and only if' ... but it doesn't matter much.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
On Fri, Apr 20, 2018 at 02:10:04PM +0200, Jesper Dangaard Brouer wrote:
>
> On Thu, 19 Apr 2018 09:34:02 +0800 Leo Yan <[email protected]> wrote:
>
> > Fix typo by replacing 'iif' with 'if'.
> >
> > Signed-off-by: Leo Yan <[email protected]>
> > ---
> > samples/bpf/bpf_load.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> > index bebe418..28e4678 100644
> > --- a/samples/bpf/bpf_load.c
> > +++ b/samples/bpf/bpf_load.c
> > @@ -393,7 +393,7 @@ static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
> > continue;
> > if (sym[nr_maps].st_shndx != maps_shndx)
> > continue;
> > - /* Only increment iif maps section */
> > + /* Only increment if maps section */
> > nr_maps++;
> > }
>
> This was actually not a typo from my side.
>
> With 'iif' I mean 'if and only if' ... but it doesn't matter much.
I think 'if and only if' is more commonly abbreviated 'iff' isn't it?
Daniel.
On Fri, 20 Apr 2018 14:21:16 +0100
Daniel Thompson <[email protected]> wrote:
> On Fri, Apr 20, 2018 at 02:10:04PM +0200, Jesper Dangaard Brouer wrote:
> >
> > On Thu, 19 Apr 2018 09:34:02 +0800 Leo Yan <[email protected]> wrote:
> >
> > > Fix typo by replacing 'iif' with 'if'.
> > >
> > > Signed-off-by: Leo Yan <[email protected]>
> > > ---
> > > samples/bpf/bpf_load.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> > > index bebe418..28e4678 100644
> > > --- a/samples/bpf/bpf_load.c
> > > +++ b/samples/bpf/bpf_load.c
> > > @@ -393,7 +393,7 @@ static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
> > > continue;
> > > if (sym[nr_maps].st_shndx != maps_shndx)
> > > continue;
> > > - /* Only increment iif maps section */
> > > + /* Only increment if maps section */
> > > nr_maps++;
> > > }
> >
> > This was actually not a typo from my side.
> >
> > With 'iif' I mean 'if and only if' ... but it doesn't matter much.
>
> I think 'if and only if' is more commonly abbreviated 'iff' isn't it?
Ah, yes![1] -- then it *is* actually a typo! - LOL
I'm fine with changing this to "if" :-)
[1] https://en.wikipedia.org/wiki/If_and_only_if
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
On Fri, Apr 20, 2018 at 03:52:13PM +0200, Jesper Dangaard Brouer wrote:
> On Fri, 20 Apr 2018 14:21:16 +0100
> Daniel Thompson <[email protected]> wrote:
>
> > On Fri, Apr 20, 2018 at 02:10:04PM +0200, Jesper Dangaard Brouer wrote:
> > >
> > > On Thu, 19 Apr 2018 09:34:02 +0800 Leo Yan <[email protected]> wrote:
> > >
> > > > Fix typo by replacing 'iif' with 'if'.
> > > >
> > > > Signed-off-by: Leo Yan <[email protected]>
> > > > ---
> > > > samples/bpf/bpf_load.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> > > > index bebe418..28e4678 100644
> > > > --- a/samples/bpf/bpf_load.c
> > > > +++ b/samples/bpf/bpf_load.c
> > > > @@ -393,7 +393,7 @@ static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
> > > > continue;
> > > > if (sym[nr_maps].st_shndx != maps_shndx)
> > > > continue;
> > > > - /* Only increment iif maps section */
> > > > + /* Only increment if maps section */
> > > > nr_maps++;
> > > > }
> > >
> > > This was actually not a typo from my side.
> > >
> > > With 'iif' I mean 'if and only if' ... but it doesn't matter much.
> >
> > I think 'if and only if' is more commonly abbreviated 'iff' isn't it?
>
> Ah, yes![1] -- then it *is* actually a typo! - LOL
>
> I'm fine with changing this to "if" :-)
Thanks for the reviewing, Daniel & Jesper.
I also learn it from the discussion :)
> [1] https://en.wikipedia.org/wiki/If_and_only_if
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer