All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup
@ 2018-04-19  1:34 Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment Leo Yan
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Leo Yan @ 2018-04-19  1:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel; +Cc: Leo Yan

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment
  2018-04-19  1:34 [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup Leo Yan
@ 2018-04-19  1:34 ` Leo Yan
  2018-04-20 12:10   ` Jesper Dangaard Brouer
  2018-04-19  1:34 ` [PATCH bpf-next 2/5] samples/bpf: Dynamically allocate structure 'syms' Leo Yan
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Leo Yan @ 2018-04-19  1:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel; +Cc: Leo Yan

Fix typo by replacing 'iif' with 'if'.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH bpf-next 2/5] samples/bpf: Dynamically allocate structure 'syms'
  2018-04-19  1:34 [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment Leo Yan
@ 2018-04-19  1:34 ` Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 3/5] samples/bpf: Use NULL for failed to find symbol Leo Yan
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2018-04-19  1:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel; +Cc: Leo Yan

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 <leo.yan@linaro.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH bpf-next 3/5] samples/bpf: Use NULL for failed to find symbol
  2018-04-19  1:34 [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 2/5] samples/bpf: Dynamically allocate structure 'syms' Leo Yan
@ 2018-04-19  1:34 ` Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip Leo Yan
  2018-04-19  1:34 ` [PATCH bpf-next 5/5] samples/bpf: Handle NULL pointer returned by ksym_search() Leo Yan
  4 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2018-04-19  1:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel; +Cc: Leo Yan

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 <leo.yan@linaro.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip
  2018-04-19  1:34 [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup Leo Yan
                   ` (2 preceding siblings ...)
  2018-04-19  1:34 ` [PATCH bpf-next 3/5] samples/bpf: Use NULL for failed to find symbol Leo Yan
@ 2018-04-19  1:34 ` Leo Yan
  2018-04-19  4:47   ` Alexei Starovoitov
  2018-04-19  1:34 ` [PATCH bpf-next 5/5] samples/bpf: Handle NULL pointer returned by ksym_search() Leo Yan
  4 siblings, 1 reply; 14+ messages in thread
From: Leo Yan @ 2018-04-19  1:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel; +Cc: Leo Yan

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);
+		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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH bpf-next 5/5] samples/bpf: Handle NULL pointer returned by ksym_search()
  2018-04-19  1:34 [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup Leo Yan
                   ` (3 preceding siblings ...)
  2018-04-19  1:34 ` [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip Leo Yan
@ 2018-04-19  1:34 ` Leo Yan
  4 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2018-04-19  1:34 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel; +Cc: Leo Yan

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 <leo.yan@linaro.org>
---
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip
  2018-04-19  1:34 ` [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip Leo Yan
@ 2018-04-19  4:47   ` Alexei Starovoitov
  2018-04-19  5:12     ` Leo Yan
  0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2018-04-19  4:47 UTC (permalink / raw)
  To: Leo Yan; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel

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!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip
  2018-04-19  4:47   ` Alexei Starovoitov
@ 2018-04-19  5:12     ` Leo Yan
  2018-04-19  5:21       ` Alexei Starovoitov
  0 siblings, 1 reply; 14+ messages in thread
From: Leo Yan @ 2018-04-19  5:12 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel

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!
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip
  2018-04-19  5:12     ` Leo Yan
@ 2018-04-19  5:21       ` Alexei Starovoitov
  2018-04-19  5:33         ` Leo Yan
  0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2018-04-19  5:21 UTC (permalink / raw)
  To: Leo Yan; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel

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!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip
  2018-04-19  5:21       ` Alexei Starovoitov
@ 2018-04-19  5:33         ` Leo Yan
  0 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2018-04-19  5:33 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel

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!
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment
  2018-04-19  1:34 ` [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment Leo Yan
@ 2018-04-20 12:10   ` Jesper Dangaard Brouer
  2018-04-20 13:21     ` Daniel Thompson
  0 siblings, 1 reply; 14+ messages in thread
From: Jesper Dangaard Brouer @ 2018-04-20 12:10 UTC (permalink / raw)
  To: Leo Yan; +Cc: brouer, Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel


On Thu, 19 Apr 2018 09:34:02 +0800 Leo Yan <leo.yan@linaro.org> wrote:

> Fix typo by replacing 'iif' with 'if'.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment
  2018-04-20 12:10   ` Jesper Dangaard Brouer
@ 2018-04-20 13:21     ` Daniel Thompson
  2018-04-20 13:52       ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Thompson @ 2018-04-20 13:21 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Leo Yan, Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel

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 <leo.yan@linaro.org> wrote:
> 
> > Fix typo by replacing 'iif' with 'if'.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment
  2018-04-20 13:21     ` Daniel Thompson
@ 2018-04-20 13:52       ` Jesper Dangaard Brouer
  2018-04-25 10:01         ` Leo Yan
  0 siblings, 1 reply; 14+ messages in thread
From: Jesper Dangaard Brouer @ 2018-04-20 13:52 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Leo Yan, Alexei Starovoitov, Daniel Borkmann, netdev,
	linux-kernel, brouer

On Fri, 20 Apr 2018 14:21:16 +0100
Daniel Thompson <daniel.thompson@linaro.org> 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 <leo.yan@linaro.org> wrote:
> >   
> > > Fix typo by replacing 'iif' with 'if'.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment
  2018-04-20 13:52       ` Jesper Dangaard Brouer
@ 2018-04-25 10:01         ` Leo Yan
  0 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2018-04-25 10:01 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Daniel Thompson, Alexei Starovoitov, Daniel Borkmann, netdev,
	linux-kernel

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 <daniel.thompson@linaro.org> 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 <leo.yan@linaro.org> wrote:
> > >   
> > > > Fix typo by replacing 'iif' with 'if'.
> > > > 
> > > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > ---
> > > >  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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-04-25 10:01 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19  1:34 [PATCH bpf-next 0/5] samples/bpf: Minor fixes and cleanup Leo Yan
2018-04-19  1:34 ` [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment Leo Yan
2018-04-20 12:10   ` Jesper Dangaard Brouer
2018-04-20 13:21     ` Daniel Thompson
2018-04-20 13:52       ` Jesper Dangaard Brouer
2018-04-25 10:01         ` Leo Yan
2018-04-19  1:34 ` [PATCH bpf-next 2/5] samples/bpf: Dynamically allocate structure 'syms' Leo Yan
2018-04-19  1:34 ` [PATCH bpf-next 3/5] samples/bpf: Use NULL for failed to find symbol Leo Yan
2018-04-19  1:34 ` [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip Leo Yan
2018-04-19  4:47   ` Alexei Starovoitov
2018-04-19  5:12     ` Leo Yan
2018-04-19  5:21       ` Alexei Starovoitov
2018-04-19  5:33         ` Leo Yan
2018-04-19  1:34 ` [PATCH bpf-next 5/5] samples/bpf: Handle NULL pointer returned by ksym_search() Leo Yan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.