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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 May 2020 12:51:09 +0200
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Yonghong Song <yhs@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
        davem@...emloft.net, brouer@...hat.com, daniel@...earbox.net,
        lorenzo.bianconi@...hat.com
Subject: Re: [PATCH bpf-next] samples/bpf: xdp_redirect_cpu: set MAX_CPUS
 according to NR_CPUS

> 
> 
> 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@...nel.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));
> > 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)