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] [day] [month] [year] [list]

Message-ID: <37d3d131-5443-291d-ee0d-c62cd136fc8a@fb.com>
Date:   Tue, 12 May 2020 07:22:12 -0700
From:   Yonghong Song <yhs@...com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
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/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@...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)?

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