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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- 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 --]