[PATCH bpf-next] samples/bpf: xdp_redirect_cpu: set MAX_CPUS according to NR_CPUS

[PATCH bpf-next] samples/bpf: xdp_redirect_cpu: set MAX_CPUS according to NR_CPUS

[Lorenzo Bianconi]

xdp_redirect_cpu is currently failing in bpf_prog_load_xattr()
allocating cpu_map map if CONFIG_NR_CPUS is less than 64 since
cpu_map_alloc() requires max_entries to be less than NR_CPUS.
Set cpu_map max_entries according to NR_CPUS in xdp_redirect_cpu_kern.c
and get currently running cpus in xdp_redirect_cpu_user.c

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 samples/bpf/xdp_redirect_cpu_kern.c |  2 +-
 samples/bpf/xdp_redirect_cpu_user.c | 29 ++++++++++++++++-------------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/samples/bpf/xdp_redirect_cpu_kern.c b/samples/bpf/xdp_redirect_cpu_kern.c
index 313a8fe6d125..2baf8db1f7e7 100644
--- a/samples/bpf/xdp_redirect_cpu_kern.c
+++ b/samples/bpf/xdp_redirect_cpu_kern.c
@@ -15,7 +15,7 @@
 #include <bpf/bpf_helpers.h>
 #include "hash_func01.h"
 
-#define MAX_CPUS 64 /* WARNING - sync with _user.c */
+#define MAX_CPUS NR_CPUS
 
 /* Special map type that can XDP_REDIRECT frames to another CPU */
 struct {
diff --git a/samples/bpf/xdp_redirect_cpu_user.c b/samples/bpf/xdp_redirect_cpu_user.c
index 15bdf047a222..100e72cb4cf5 100644
--- a/samples/bpf/xdp_redirect_cpu_user.c
+++ b/samples/bpf/xdp_redirect_cpu_user.c
@@ -13,6 +13,7 @@ static const char *__doc__ =
 #include <unistd.h>
 #include <locale.h>
 #include <sys/resource.h>
+#include <sys/sysinfo.h>
 #include <getopt.h>
 #include <net/if.h>
 #include <time.h>
@@ -24,8 +25,6 @@ static const char *__doc__ =
 #include <arpa/inet.h>
 #include <linux/if_link.h>
 
-#define MAX_CPUS 64 /* WARNING - sync with _kern.c */
-
 /* How many xdp_progs are defined in _kern.c */
 #define MAX_PROG 6
 
@@ -40,6 +39,7 @@ static char *ifname;
 static __u32 prog_id;
 
 static __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
+static int n_cpus;
 static int cpu_map_fd;
 static int rx_cnt_map_fd;
 static int redirect_err_cnt_map_fd;
@@ -170,7 +170,7 @@ struct stats_record {
 	struct record redir_err;
 	struct record kthread;
 	struct record exception;
-	struct record enq[MAX_CPUS];
+	struct record enq[];
 };
 
 static bool map_collect_percpu(int fd, __u32 key, struct record *rec)
@@ -225,10 +225,11 @@ static struct datarec *alloc_record_per_cpu(void)
 static struct stats_record *alloc_stats_record(void)
 {
 	struct stats_record *rec;
-	int i;
+	int i, size;
 
-	rec = malloc(sizeof(*rec));
-	memset(rec, 0, sizeof(*rec));
+	size = sizeof(*rec) + n_cpus * sizeof(struct record);
+	rec = malloc(size);
+	memset(rec, 0, size);
 	if (!rec) {
 		fprintf(stderr, "Mem alloc error\n");
 		exit(EXIT_FAIL_MEM);
@@ -237,7 +238,7 @@ static struct stats_record *alloc_stats_record(void)
 	rec->redir_err.cpu = alloc_record_per_cpu();
 	rec->kthread.cpu   = alloc_record_per_cpu();
 	rec->exception.cpu = alloc_record_per_cpu();
-	for (i = 0; i < MAX_CPUS; i++)
+	for (i = 0; i < n_cpus; i++)
 		rec->enq[i].cpu = alloc_record_per_cpu();
 
 	return rec;
@@ -247,7 +248,7 @@ static void free_stats_record(struct stats_record *r)
 {
 	int i;
 
-	for (i = 0; i < MAX_CPUS; i++)
+	for (i = 0; i < n_cpus; i++)
 		free(r->enq[i].cpu);
 	free(r->exception.cpu);
 	free(r->kthread.cpu);
@@ -350,7 +351,7 @@ static void stats_print(struct stats_record *stats_rec,
 	}
 
 	/* cpumap enqueue stats */
-	for (to_cpu = 0; to_cpu < MAX_CPUS; to_cpu++) {
+	for (to_cpu = 0; to_cpu < n_cpus; to_cpu++) {
 		char *fmt = "%-15s %3d:%-3d %'-14.0f %'-11.0f %'-10.2f %s\n";
 		char *fm2 = "%-15s %3s:%-3d %'-14.0f %'-11.0f %'-10.2f %s\n";
 		char *errstr = "";
@@ -475,7 +476,7 @@ static void stats_collect(struct stats_record *rec)
 	map_collect_percpu(fd, 1, &rec->redir_err);
 
 	fd = cpumap_enqueue_cnt_map_fd;
-	for (i = 0; i < MAX_CPUS; i++)
+	for (i = 0; i < n_cpus; i++)
 		map_collect_percpu(fd, i, &rec->enq[i]);
 
 	fd = cpumap_kthread_cnt_map_fd;
@@ -549,10 +550,10 @@ static int create_cpu_entry(__u32 cpu, __u32 queue_size,
  */
 static void mark_cpus_unavailable(void)
 {
-	__u32 invalid_cpu = MAX_CPUS;
+	__u32 invalid_cpu = n_cpus;
 	int ret, i;
 
-	for (i = 0; i < MAX_CPUS; i++) {
+	for (i = 0; i < n_cpus; i++) {
 		ret = bpf_map_update_elem(cpus_available_map_fd, &i,
 					  &invalid_cpu, 0);
 		if (ret) {
@@ -688,6 +689,8 @@ int main(int argc, char **argv)
 	int prog_fd;
 	__u32 qsize;
 
+	n_cpus = get_nprocs();
+
 	/* Notice: choosing he queue size is very important with the
 	 * ixgbe driver, because it's driver page recycling trick is
 	 * dependend on pages being returned quickly.  The number of
@@ -757,7 +760,7 @@ int main(int argc, char **argv)
 		case 'c':
 			/* Add multiple CPUs */
 			add_cpu = strtoul(optarg, NULL, 0);
-			if (add_cpu >= MAX_CPUS) {
+			if (add_cpu >= n_cpus) {
 				fprintf(stderr,
 				"--cpu nr too large for cpumap err(%d):%s\n",
 					errno, strerror(errno));
-- 
2.26.2

Re: [PATCH bpf-next] samples/bpf: xdp_redirect_cpu: set MAX_CPUS according to NR_CPUS

[Yonghong Song]

On 5/11/20 1:24 PM, Lorenzo Bianconi wrote:
> xdp_redirect_cpu is currently failing in bpf_prog_load_xattr()
> allocating cpu_map map if CONFIG_NR_CPUS is less than 64 since
> cpu_map_alloc() requires max_entries to be less than NR_CPUS.
> Set cpu_map max_entries according to NR_CPUS in xdp_redirect_cpu_kern.c
> and get currently running cpus in xdp_redirect_cpu_user.c
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   samples/bpf/xdp_redirect_cpu_kern.c |  2 +-
>   samples/bpf/xdp_redirect_cpu_user.c | 29 ++++++++++++++++-------------
>   2 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/samples/bpf/xdp_redirect_cpu_kern.c b/samples/bpf/xdp_redirect_cpu_kern.c
> index 313a8fe6d125..2baf8db1f7e7 100644
> --- a/samples/bpf/xdp_redirect_cpu_kern.c
> +++ b/samples/bpf/xdp_redirect_cpu_kern.c
> @@ -15,7 +15,7 @@
>   #include <bpf/bpf_helpers.h>
>   #include "hash_func01.h"
>   
> -#define MAX_CPUS 64 /* WARNING - sync with _user.c */
> +#define MAX_CPUS NR_CPUS
>   
>   /* Special map type that can XDP_REDIRECT frames to another CPU */
>   struct {
> diff --git a/samples/bpf/xdp_redirect_cpu_user.c b/samples/bpf/xdp_redirect_cpu_user.c
> index 15bdf047a222..100e72cb4cf5 100644
> --- a/samples/bpf/xdp_redirect_cpu_user.c
> +++ b/samples/bpf/xdp_redirect_cpu_user.c
> @@ -13,6 +13,7 @@ static const char *__doc__ =
>   #include <unistd.h>
>   #include <locale.h>
>   #include <sys/resource.h>
> +#include <sys/sysinfo.h>
>   #include <getopt.h>
>   #include <net/if.h>
>   #include <time.h>
> @@ -24,8 +25,6 @@ static const char *__doc__ =
>   #include <arpa/inet.h>
>   #include <linux/if_link.h>
>   
> -#define MAX_CPUS 64 /* WARNING - sync with _kern.c */
> -
>   /* How many xdp_progs are defined in _kern.c */
>   #define MAX_PROG 6
>   
> @@ -40,6 +39,7 @@ static char *ifname;
>   static __u32 prog_id;
>   
>   static __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
> +static int n_cpus;
>   static int cpu_map_fd;
>   static int rx_cnt_map_fd;
>   static int redirect_err_cnt_map_fd;
> @@ -170,7 +170,7 @@ struct stats_record {
>   	struct record redir_err;
>   	struct record kthread;
>   	struct record exception;
> -	struct record enq[MAX_CPUS];
> +	struct record enq[];
>   };
>   
>   static bool map_collect_percpu(int fd, __u32 key, struct record *rec)
> @@ -225,10 +225,11 @@ static struct datarec *alloc_record_per_cpu(void)
>   static struct stats_record *alloc_stats_record(void)
>   {
>   	struct stats_record *rec;
> -	int i;
> +	int i, size;
>   
> -	rec = malloc(sizeof(*rec));
> -	memset(rec, 0, sizeof(*rec));
> +	size = sizeof(*rec) + n_cpus * sizeof(struct record);
> +	rec = malloc(size);
> +	memset(rec, 0, size);
>   	if (!rec) {
>   		fprintf(stderr, "Mem alloc error\n");
>   		exit(EXIT_FAIL_MEM);
> @@ -237,7 +238,7 @@ static struct stats_record *alloc_stats_record(void)
>   	rec->redir_err.cpu = alloc_record_per_cpu();
>   	rec->kthread.cpu   = alloc_record_per_cpu();
>   	rec->exception.cpu = alloc_record_per_cpu();
> -	for (i = 0; i < MAX_CPUS; i++)
> +	for (i = 0; i < n_cpus; i++)
>   		rec->enq[i].cpu = alloc_record_per_cpu();
>   
>   	return rec;
> @@ -247,7 +248,7 @@ static void free_stats_record(struct stats_record *r)
>   {
>   	int i;
>   
> -	for (i = 0; i < MAX_CPUS; i++)
> +	for (i = 0; i < n_cpus; i++)
>   		free(r->enq[i].cpu);
>   	free(r->exception.cpu);
>   	free(r->kthread.cpu);
> @@ -350,7 +351,7 @@ static void stats_print(struct stats_record *stats_rec,
>   	}
>   
>   	/* cpumap enqueue stats */
> -	for (to_cpu = 0; to_cpu < MAX_CPUS; to_cpu++) {
> +	for (to_cpu = 0; to_cpu < n_cpus; to_cpu++) {
>   		char *fmt = "%-15s %3d:%-3d %'-14.0f %'-11.0f %'-10.2f %s\n";
>   		char *fm2 = "%-15s %3s:%-3d %'-14.0f %'-11.0f %'-10.2f %s\n";
>   		char *errstr = "";
> @@ -475,7 +476,7 @@ static void stats_collect(struct stats_record *rec)
>   	map_collect_percpu(fd, 1, &rec->redir_err);
>   
>   	fd = cpumap_enqueue_cnt_map_fd;
> -	for (i = 0; i < MAX_CPUS; i++)
> +	for (i = 0; i < n_cpus; i++)
>   		map_collect_percpu(fd, i, &rec->enq[i]);
>   
>   	fd = cpumap_kthread_cnt_map_fd;
> @@ -549,10 +550,10 @@ static int create_cpu_entry(__u32 cpu, __u32 queue_size,
>    */
>   static void mark_cpus_unavailable(void)
>   {
> -	__u32 invalid_cpu = MAX_CPUS;
> +	__u32 invalid_cpu = n_cpus;
>   	int ret, i;
>   
> -	for (i = 0; i < MAX_CPUS; i++) {
> +	for (i = 0; i < n_cpus; i++) {
>   		ret = bpf_map_update_elem(cpus_available_map_fd, &i,
>   					  &invalid_cpu, 0);
>   		if (ret) {
> @@ -688,6 +689,8 @@ int main(int argc, char **argv)
>   	int prog_fd;
>   	__u32 qsize;
>   
> +	n_cpus = get_nprocs();

get_nprocs() gets the number of available cpus, not including offline 
cpus. But gaps may exist in cpus, e.g., get_nprocs() returns 4, and cpus 
are 0-1,4-5. map updates will miss cpus 4-5. And in this situation,
map_update will fail on offline cpus.

This sample test does not need to deal with complication of
cpu offlining, I think. Maybe we can get
	n_cpus = get_nprocs();
	n_cpus_conf = get_nprocs_conf();
	if (n_cpus != n_cpus_conf) {
		/* message that some cpus are offline and not supported. */
		return error
	}

> +
>   	/* Notice: choosing he queue size is very important with the
>   	 * ixgbe driver, because it's driver page recycling trick is
>   	 * dependend on pages being returned quickly.  The number of
> @@ -757,7 +760,7 @@ int main(int argc, char **argv)
>   		case 'c':
>   			/* Add multiple CPUs */
>   			add_cpu = strtoul(optarg, NULL, 0);
> -			if (add_cpu >= MAX_CPUS) {
> +			if (add_cpu >= n_cpus) {
>   				fprintf(stderr,
>   				"--cpu nr too large for cpumap err(%d):%s\n",
>   					errno, strerror(errno));
> 

Re: [PATCH bpf-next] samples/bpf: xdp_redirect_cpu: set MAX_CPUS according to NR_CPUS

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2480 bytes --]

> 
> 
> On 5/11/20 1:24 PM, Lorenzo Bianconi wrote:
> > xdp_redirect_cpu is currently failing in bpf_prog_load_xattr()
> > allocating cpu_map map if CONFIG_NR_CPUS is less than 64 since
> > cpu_map_alloc() requires max_entries to be less than NR_CPUS.
> > Set cpu_map max_entries according to NR_CPUS in xdp_redirect_cpu_kern.c
> > and get currently running cpus in xdp_redirect_cpu_user.c
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >   samples/bpf/xdp_redirect_cpu_kern.c |  2 +-
> >   samples/bpf/xdp_redirect_cpu_user.c | 29 ++++++++++++++++-------------
> >   2 files changed, 17 insertions(+), 14 deletions(-)
> > 

[...]

> >   static void mark_cpus_unavailable(void)
> >   {
> > -	__u32 invalid_cpu = MAX_CPUS;
> > +	__u32 invalid_cpu = n_cpus;
> >   	int ret, i;
> > -	for (i = 0; i < MAX_CPUS; i++) {
> > +	for (i = 0; i < n_cpus; i++) {
> >   		ret = bpf_map_update_elem(cpus_available_map_fd, &i,
> >   					  &invalid_cpu, 0);
> >   		if (ret) {
> > @@ -688,6 +689,8 @@ int main(int argc, char **argv)
> >   	int prog_fd;
> >   	__u32 qsize;
> > +	n_cpus = get_nprocs();
> 
> get_nprocs() gets the number of available cpus, not including offline cpus.
> But gaps may exist in cpus, e.g., get_nprocs() returns 4, and cpus are
> 0-1,4-5. map updates will miss cpus 4-5. And in this situation,
> map_update will fail on offline cpus.
> 
> This sample test does not need to deal with complication of
> cpu offlining, I think. Maybe we can get
> 	n_cpus = get_nprocs();
> 	n_cpus_conf = get_nprocs_conf();
> 	if (n_cpus != n_cpus_conf) {
> 		/* message that some cpus are offline and not supported. */
> 		return error
> 	}
> 

Hi Yonghong,

thanks for pointing this out. Why not just use:

n_cpus = get_nprocs_conf()

and let the user pick the right cpu id with 'c' option (since it is mandatory)?

Regards,
Lorenzo

> > +
> >   	/* Notice: choosing he queue size is very important with the
> >   	 * ixgbe driver, because it's driver page recycling trick is
> >   	 * dependend on pages being returned quickly.  The number of
> > @@ -757,7 +760,7 @@ int main(int argc, char **argv)
> >   		case 'c':
> >   			/* Add multiple CPUs */
> >   			add_cpu = strtoul(optarg, NULL, 0);
> > -			if (add_cpu >= MAX_CPUS) {
> > +			if (add_cpu >= n_cpus) {
> >   				fprintf(stderr,
> >   				"--cpu nr too large for cpumap err(%d):%s\n",
> >   					errno, strerror(errno));
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next] samples/bpf: xdp_redirect_cpu: set MAX_CPUS according to NR_CPUS

[Yonghong Song]

On 5/12/20 3:51 AM, Lorenzo Bianconi wrote:
>>
>>
>> On 5/11/20 1:24 PM, Lorenzo Bianconi wrote:
>>> xdp_redirect_cpu is currently failing in bpf_prog_load_xattr()
>>> allocating cpu_map map if CONFIG_NR_CPUS is less than 64 since
>>> cpu_map_alloc() requires max_entries to be less than NR_CPUS.
>>> Set cpu_map max_entries according to NR_CPUS in xdp_redirect_cpu_kern.c
>>> and get currently running cpus in xdp_redirect_cpu_user.c
>>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>> ---
>>>    samples/bpf/xdp_redirect_cpu_kern.c |  2 +-
>>>    samples/bpf/xdp_redirect_cpu_user.c | 29 ++++++++++++++++-------------
>>>    2 files changed, 17 insertions(+), 14 deletions(-)
>>>
> 
> [...]
> 
>>>    static void mark_cpus_unavailable(void)
>>>    {
>>> -	__u32 invalid_cpu = MAX_CPUS;
>>> +	__u32 invalid_cpu = n_cpus;
>>>    	int ret, i;
>>> -	for (i = 0; i < MAX_CPUS; i++) {
>>> +	for (i = 0; i < n_cpus; i++) {
>>>    		ret = bpf_map_update_elem(cpus_available_map_fd, &i,
>>>    					  &invalid_cpu, 0);
>>>    		if (ret) {
>>> @@ -688,6 +689,8 @@ int main(int argc, char **argv)
>>>    	int prog_fd;
>>>    	__u32 qsize;
>>> +	n_cpus = get_nprocs();
>>
>> get_nprocs() gets the number of available cpus, not including offline cpus.
>> But gaps may exist in cpus, e.g., get_nprocs() returns 4, and cpus are
>> 0-1,4-5. map updates will miss cpus 4-5. And in this situation,
>> map_update will fail on offline cpus.
>>
>> This sample test does not need to deal with complication of
>> cpu offlining, I think. Maybe we can get
>> 	n_cpus = get_nprocs();
>> 	n_cpus_conf = get_nprocs_conf();
>> 	if (n_cpus != n_cpus_conf) {
>> 		/* message that some cpus are offline and not supported. */
>> 		return error
>> 	}
>>
> 
> Hi Yonghong,
> 
> thanks for pointing this out. Why not just use:
> 
> n_cpus = get_nprocs_conf()
> 
> and let the user pick the right cpu id with 'c' option (since it is mandatory)?

Not aware of 'c' option. Yes, get_nprocs_conf() directly works too.

> 
> Regards,
> Lorenzo
> 
>>> +
>>>    	/* Notice: choosing he queue size is very important with the
>>>    	 * ixgbe driver, because it's driver page recycling trick is
>>>    	 * dependend on pages being returned quickly.  The number of
>>> @@ -757,7 +760,7 @@ int main(int argc, char **argv)
>>>    		case 'c':
>>>    			/* Add multiple CPUs */
>>>    			add_cpu = strtoul(optarg, NULL, 0);
>>> -			if (add_cpu >= MAX_CPUS) {
>>> +			if (add_cpu >= n_cpus) {
>>>    				fprintf(stderr,
>>>    				"--cpu nr too large for cpumap err(%d):%s\n",
>>>    					errno, strerror(errno));
>>>