[bpf-next,4/5] samples/bpf: Refine printing symbol for sampleip

Message ID 1524101646-6544-5-git-send-email-leo.yan@linaro.org
State New
Headers show
Series
  • samples/bpf: Minor fixes and cleanup
Related show

Commit Message

Leo Yan April 19, 2018, 1:34 a.m.
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(-)

-- 
1.9.1

Comments

Alexei Starovoitov April 19, 2018, 4:47 a.m. | #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 <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!
Leo Yan April 19, 2018, 5:12 a.m. | #2
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!

>
Alexei Starovoitov April 19, 2018, 5:21 a.m. | #3
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!
Leo Yan April 19, 2018, 5:33 a.m. | #4
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!

>

Patch

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) {