| Message ID | 1524101646-6544-5-git-send-email-leo.yan@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | samples/bpf: Minor fixes and cleanup | expand |
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 <leo.yan@linaro.org> > --- > 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 <leo.yan@linaro.org> > > --- > > 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 <leo.yan@linaro.org> > > > --- > > > 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 <leo.yan@linaro.org> > > > > --- > > > > 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! >
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) {
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 <leo.yan@linaro.org> --- samples/bpf/sampleip_user.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)