diff mbox series

[bpf-next,v2,1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

Message ID eafd46de2ff1bfc6103ec466d4fba0861ce416a6.1706717857.git.vmalik@redhat.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series tools/resolve_btfids: fix cross-compilation to non-host endianness | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / veristat
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 1 this patch: 0
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 85 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Viktor Malik Jan. 31, 2024, 4:24 p.m. UTC
Instead of using magic offsets to access BTF ID set data, leverage types
from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
layout of the data. Thanks to this change, set sorting should also
continue working if the layout changes.

This requires to sync the definition of 'struct btf_id_set8' from
include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
the rest of the file at the moment, b/c that would require to also sync
multiple dependent headers and we don't need any other defs from
btf_ids.h.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
 tools/include/linux/btf_ids.h   |  9 +++++++++
 2 files changed, 31 insertions(+), 8 deletions(-)

Comments

Daniel Borkmann Feb. 1, 2024, 3:51 p.m. UTC | #1
On 1/31/24 5:24 PM, Viktor Malik wrote:
> Instead of using magic offsets to access BTF ID set data, leverage types
> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
> layout of the data. Thanks to this change, set sorting should also
> continue working if the layout changes.
> 
> This requires to sync the definition of 'struct btf_id_set8' from
> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
> the rest of the file at the moment, b/c that would require to also sync
> multiple dependent headers and we don't need any other defs from
> btf_ids.h.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>   tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>   tools/include/linux/btf_ids.h   |  9 +++++++++
>   2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 27a23196d58e..7badf1557e5c 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -70,6 +70,7 @@
>   #include <sys/stat.h>
>   #include <fcntl.h>
>   #include <errno.h>
> +#include <linux/btf_ids.h>
>   #include <linux/rbtree.h>
>   #include <linux/zalloc.h>
>   #include <linux/err.h>
> @@ -78,7 +79,7 @@
>   #include <subcmd/parse-options.h>
>   
>   #define BTF_IDS_SECTION	".BTF_ids"
> -#define BTF_ID		"__BTF_ID__"
> +#define BTF_ID_PREFIX	"__BTF_ID__"
>   
>   #define BTF_STRUCT	"struct"
>   #define BTF_UNION	"union"
> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>   
>   static bool is_btf_id(const char *name)
>   {
> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>   }
>   
>   static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>   		 * __BTF_ID__TYPE__vfs_truncate__0
>   		 * prefix =  ^
>   		 */
> -		prefix = name + sizeof(BTF_ID) - 1;
> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>   
>   		/* struct */
>   		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>   	while (next) {
>   		unsigned long addr, idx;
>   		struct btf_id *id;
> -		int *base;
> -		int cnt;
> +		void *base;
> +		int cnt, size;
>   
>   		id   = rb_entry(next, struct btf_id, rb_node);
>   		addr = id->addr[0];
> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>   		}
>   
>   		idx = idx / sizeof(int);
> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> -		cnt = ptr[idx];
> +		if (id->is_set) {
> +			struct btf_id_set *set;
> +
> +			set = (struct btf_id_set *)&ptr[idx];
> +			base = set->ids;
> +			cnt = set->cnt;
> +			size = sizeof(set->ids[0]);
> +		} else {
> +			struct btf_id_set8 *set8;
> +
> +			set8 = (struct btf_id_set8 *)&ptr[idx];
> +			base = set8->pairs;
> +			cnt = set8->cnt;
> +			size = sizeof(set8->pairs[0]);
> +		}
>   
>   		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>   			 (idx + 1) * sizeof(int), cnt, id->name);
>   
> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> +		qsort(base, cnt, size, cmp_id);

Looks good to me, one small remark: perhaps it would also make sense to have an assert
on the id location, such that we have a build error in case the id would not be the
first in the struct / pairs array anymore given then cmp_id would look at wrong data
for the latter given the plain int cast from there.

>   		next = rb_next(next);
>   	}
> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> index 2f882d5cb30f..72535f00572f 100644
> --- a/tools/include/linux/btf_ids.h
> +++ b/tools/include/linux/btf_ids.h
> @@ -8,6 +8,15 @@ struct btf_id_set {
>   	u32 ids[];
>   };
>   
> +struct btf_id_set8 {
> +	u32 cnt;
> +	u32 flags;
> +	struct {
> +		u32 id;
> +		u32 flags;
> +	} pairs[];
> +};
> +
>   #ifdef CONFIG_DEBUG_INFO_BTF
>   
>   #include <linux/compiler.h> /* for __PASTE */
>
Viktor Malik Feb. 2, 2024, 9:55 a.m. UTC | #2
On 2/1/24 16:51, Daniel Borkmann wrote:
> On 1/31/24 5:24 PM, Viktor Malik wrote:
>> Instead of using magic offsets to access BTF ID set data, leverage types
>> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
>> layout of the data. Thanks to this change, set sorting should also
>> continue working if the layout changes.
>>
>> This requires to sync the definition of 'struct btf_id_set8' from
>> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
>> the rest of the file at the moment, b/c that would require to also sync
>> multiple dependent headers and we don't need any other defs from
>> btf_ids.h.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>   tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>>   tools/include/linux/btf_ids.h   |  9 +++++++++
>>   2 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 27a23196d58e..7badf1557e5c 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -70,6 +70,7 @@
>>   #include <sys/stat.h>
>>   #include <fcntl.h>
>>   #include <errno.h>
>> +#include <linux/btf_ids.h>
>>   #include <linux/rbtree.h>
>>   #include <linux/zalloc.h>
>>   #include <linux/err.h>
>> @@ -78,7 +79,7 @@
>>   #include <subcmd/parse-options.h>
>>   
>>   #define BTF_IDS_SECTION	".BTF_ids"
>> -#define BTF_ID		"__BTF_ID__"
>> +#define BTF_ID_PREFIX	"__BTF_ID__"
>>   
>>   #define BTF_STRUCT	"struct"
>>   #define BTF_UNION	"union"
>> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>>   
>>   static bool is_btf_id(const char *name)
>>   {
>> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
>> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>>   }
>>   
>>   static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
>> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>>   		 * __BTF_ID__TYPE__vfs_truncate__0
>>   		 * prefix =  ^
>>   		 */
>> -		prefix = name + sizeof(BTF_ID) - 1;
>> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>>   
>>   		/* struct */
>>   		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
>> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>>   	while (next) {
>>   		unsigned long addr, idx;
>>   		struct btf_id *id;
>> -		int *base;
>> -		int cnt;
>> +		void *base;
>> +		int cnt, size;
>>   
>>   		id   = rb_entry(next, struct btf_id, rb_node);
>>   		addr = id->addr[0];
>> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>>   		}
>>   
>>   		idx = idx / sizeof(int);
>> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
>> -		cnt = ptr[idx];
>> +		if (id->is_set) {
>> +			struct btf_id_set *set;
>> +
>> +			set = (struct btf_id_set *)&ptr[idx];
>> +			base = set->ids;
>> +			cnt = set->cnt;
>> +			size = sizeof(set->ids[0]);
>> +		} else {
>> +			struct btf_id_set8 *set8;
>> +
>> +			set8 = (struct btf_id_set8 *)&ptr[idx];
>> +			base = set8->pairs;
>> +			cnt = set8->cnt;
>> +			size = sizeof(set8->pairs[0]);
>> +		}
>>   
>>   		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>>   			 (idx + 1) * sizeof(int), cnt, id->name);
>>   
>> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>> +		qsort(base, cnt, size, cmp_id);
> 
> Looks good to me, one small remark: perhaps it would also make sense to have an assert
> on the id location, such that we have a build error in case the id would not be the
> first in the struct / pairs array anymore given then cmp_id would look at wrong data
> for the latter given the plain int cast from there.

That's a good idea, I'll add a BUILD_BUG_ON check for set8.

Thanks!

> 
>>   		next = rb_next(next);
>>   	}
>> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
>> index 2f882d5cb30f..72535f00572f 100644
>> --- a/tools/include/linux/btf_ids.h
>> +++ b/tools/include/linux/btf_ids.h
>> @@ -8,6 +8,15 @@ struct btf_id_set {
>>   	u32 ids[];
>>   };
>>   
>> +struct btf_id_set8 {
>> +	u32 cnt;
>> +	u32 flags;
>> +	struct {
>> +		u32 id;
>> +		u32 flags;
>> +	} pairs[];
>> +};
>> +
>>   #ifdef CONFIG_DEBUG_INFO_BTF
>>   
>>   #include <linux/compiler.h> /* for __PASTE */
>>
>
Jiri Olsa Feb. 2, 2024, 1:06 p.m. UTC | #3
On Wed, Jan 31, 2024 at 05:24:08PM +0100, Viktor Malik wrote:
> Instead of using magic offsets to access BTF ID set data, leverage types
> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
> layout of the data. Thanks to this change, set sorting should also
> continue working if the layout changes.
> 
> This requires to sync the definition of 'struct btf_id_set8' from
> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
> the rest of the file at the moment, b/c that would require to also sync
> multiple dependent headers and we don't need any other defs from
> btf_ids.h.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>  tools/include/linux/btf_ids.h   |  9 +++++++++
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 27a23196d58e..7badf1557e5c 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -70,6 +70,7 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <errno.h>
> +#include <linux/btf_ids.h>
>  #include <linux/rbtree.h>
>  #include <linux/zalloc.h>
>  #include <linux/err.h>
> @@ -78,7 +79,7 @@
>  #include <subcmd/parse-options.h>
>  
>  #define BTF_IDS_SECTION	".BTF_ids"
> -#define BTF_ID		"__BTF_ID__"
> +#define BTF_ID_PREFIX	"__BTF_ID__"

nit does not look necessary to me

>  
>  #define BTF_STRUCT	"struct"
>  #define BTF_UNION	"union"
> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>  
>  static bool is_btf_id(const char *name)
>  {
> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>  }
>  
>  static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>  		 * __BTF_ID__TYPE__vfs_truncate__0
>  		 * prefix =  ^
>  		 */
> -		prefix = name + sizeof(BTF_ID) - 1;
> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>  
>  		/* struct */
>  		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>  	while (next) {
>  		unsigned long addr, idx;
>  		struct btf_id *id;
> -		int *base;
> -		int cnt;
> +		void *base;
> +		int cnt, size;
>  
>  		id   = rb_entry(next, struct btf_id, rb_node);
>  		addr = id->addr[0];
> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>  		}
>  
>  		idx = idx / sizeof(int);
> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> -		cnt = ptr[idx];
> +		if (id->is_set) {
> +			struct btf_id_set *set;
> +
> +			set = (struct btf_id_set *)&ptr[idx];
> +			base = set->ids;
> +			cnt = set->cnt;
> +			size = sizeof(set->ids[0]);
> +		} else {
> +			struct btf_id_set8 *set8;
> +
> +			set8 = (struct btf_id_set8 *)&ptr[idx];
> +			base = set8->pairs;
> +			cnt = set8->cnt;
> +			size = sizeof(set8->pairs[0]);
> +		}
>  
>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>  			 (idx + 1) * sizeof(int), cnt, id->name);
>  
> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> +		qsort(base, cnt, size, cmp_id);

maybe we could call qsort on top of each set type, seems simpler:

 	while (next) {
+		struct btf_id_set8 *set8;
+		struct btf_id_set *set;
 		unsigned long addr, idx;
 		struct btf_id *id;
-		int *base;
-		int cnt;
 
 		id   = rb_entry(next, struct btf_id, rb_node);
 		addr = id->addr[0];
@@ -671,13 +672,16 @@ static int sets_patch(struct object *obj)
 		}
 
 		idx = idx / sizeof(int);
-		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
-		cnt = ptr[idx];
+		if (id->is_set) {
+			set = (struct btf_id_set *)&ptr[idx];
+			qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
+		} else {
+			set8 = (struct btf_id_set8 *)&ptr[idx];
+			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
+		}
 
 		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
-			 (idx + 1) * sizeof(int), cnt, id->name);
-
-		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
+			 (idx + 1) * sizeof(int), id->is_set ? set->cnt : set8->cnt, id->name);
 
 		next = rb_next(next);
 	}


jirka
Viktor Malik Feb. 5, 2024, 5:42 a.m. UTC | #4
On 2/2/24 14:06, Jiri Olsa wrote:
> On Wed, Jan 31, 2024 at 05:24:08PM +0100, Viktor Malik wrote:
>> Instead of using magic offsets to access BTF ID set data, leverage types
>> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
>> layout of the data. Thanks to this change, set sorting should also
>> continue working if the layout changes.
>>
>> This requires to sync the definition of 'struct btf_id_set8' from
>> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
>> the rest of the file at the moment, b/c that would require to also sync
>> multiple dependent headers and we don't need any other defs from
>> btf_ids.h.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>  tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>>  tools/include/linux/btf_ids.h   |  9 +++++++++
>>  2 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 27a23196d58e..7badf1557e5c 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -70,6 +70,7 @@
>>  #include <sys/stat.h>
>>  #include <fcntl.h>
>>  #include <errno.h>
>> +#include <linux/btf_ids.h>
>>  #include <linux/rbtree.h>
>>  #include <linux/zalloc.h>
>>  #include <linux/err.h>
>> @@ -78,7 +79,7 @@
>>  #include <subcmd/parse-options.h>
>>  
>>  #define BTF_IDS_SECTION	".BTF_ids"
>> -#define BTF_ID		"__BTF_ID__"
>> +#define BTF_ID_PREFIX	"__BTF_ID__"
> 
> nit does not look necessary to me

There's a conflicting definition of BTF_ID in btf_ids.h, this change is
to prevent a warning after we include the header.

> 
>>  
>>  #define BTF_STRUCT	"struct"
>>  #define BTF_UNION	"union"
>> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>>  
>>  static bool is_btf_id(const char *name)
>>  {
>> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
>> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>>  }
>>  
>>  static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
>> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>>  		 * __BTF_ID__TYPE__vfs_truncate__0
>>  		 * prefix =  ^
>>  		 */
>> -		prefix = name + sizeof(BTF_ID) - 1;
>> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>>  
>>  		/* struct */
>>  		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
>> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>>  	while (next) {
>>  		unsigned long addr, idx;
>>  		struct btf_id *id;
>> -		int *base;
>> -		int cnt;
>> +		void *base;
>> +		int cnt, size;
>>  
>>  		id   = rb_entry(next, struct btf_id, rb_node);
>>  		addr = id->addr[0];
>> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>>  		}
>>  
>>  		idx = idx / sizeof(int);
>> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
>> -		cnt = ptr[idx];
>> +		if (id->is_set) {
>> +			struct btf_id_set *set;
>> +
>> +			set = (struct btf_id_set *)&ptr[idx];
>> +			base = set->ids;
>> +			cnt = set->cnt;
>> +			size = sizeof(set->ids[0]);
>> +		} else {
>> +			struct btf_id_set8 *set8;
>> +
>> +			set8 = (struct btf_id_set8 *)&ptr[idx];
>> +			base = set8->pairs;
>> +			cnt = set8->cnt;
>> +			size = sizeof(set8->pairs[0]);
>> +		}
>>  
>>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>>  			 (idx + 1) * sizeof(int), cnt, id->name);
>>  
>> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>> +		qsort(base, cnt, size, cmp_id);
> 
> maybe we could call qsort on top of each set type, seems simpler:

That looks much cleaner, I'll use that, thanks!

V.

> 
>  	while (next) {
> +		struct btf_id_set8 *set8;
> +		struct btf_id_set *set;
>  		unsigned long addr, idx;
>  		struct btf_id *id;
> -		int *base;
> -		int cnt;
>  
>  		id   = rb_entry(next, struct btf_id, rb_node);
>  		addr = id->addr[0];
> @@ -671,13 +672,16 @@ static int sets_patch(struct object *obj)
>  		}
>  
>  		idx = idx / sizeof(int);
> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> -		cnt = ptr[idx];
> +		if (id->is_set) {
> +			set = (struct btf_id_set *)&ptr[idx];
> +			qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
> +		} else {
> +			set8 = (struct btf_id_set8 *)&ptr[idx];
> +			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
> +		}
>  
>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
> -			 (idx + 1) * sizeof(int), cnt, id->name);
> -
> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> +			 (idx + 1) * sizeof(int), id->is_set ? set->cnt : set8->cnt, id->name);
>  
>  		next = rb_next(next);
>  	}
> 
> 
> jirka
>
diff mbox series

Patch

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index 27a23196d58e..7badf1557e5c 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -70,6 +70,7 @@ 
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <linux/btf_ids.h>
 #include <linux/rbtree.h>
 #include <linux/zalloc.h>
 #include <linux/err.h>
@@ -78,7 +79,7 @@ 
 #include <subcmd/parse-options.h>
 
 #define BTF_IDS_SECTION	".BTF_ids"
-#define BTF_ID		"__BTF_ID__"
+#define BTF_ID_PREFIX	"__BTF_ID__"
 
 #define BTF_STRUCT	"struct"
 #define BTF_UNION	"union"
@@ -161,7 +162,7 @@  static int eprintf(int level, int var, const char *fmt, ...)
 
 static bool is_btf_id(const char *name)
 {
-	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
+	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
 }
 
 static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
@@ -441,7 +442,7 @@  static int symbols_collect(struct object *obj)
 		 * __BTF_ID__TYPE__vfs_truncate__0
 		 * prefix =  ^
 		 */
-		prefix = name + sizeof(BTF_ID) - 1;
+		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
 
 		/* struct */
 		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
@@ -656,8 +657,8 @@  static int sets_patch(struct object *obj)
 	while (next) {
 		unsigned long addr, idx;
 		struct btf_id *id;
-		int *base;
-		int cnt;
+		void *base;
+		int cnt, size;
 
 		id   = rb_entry(next, struct btf_id, rb_node);
 		addr = id->addr[0];
@@ -671,13 +672,26 @@  static int sets_patch(struct object *obj)
 		}
 
 		idx = idx / sizeof(int);
-		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
-		cnt = ptr[idx];
+		if (id->is_set) {
+			struct btf_id_set *set;
+
+			set = (struct btf_id_set *)&ptr[idx];
+			base = set->ids;
+			cnt = set->cnt;
+			size = sizeof(set->ids[0]);
+		} else {
+			struct btf_id_set8 *set8;
+
+			set8 = (struct btf_id_set8 *)&ptr[idx];
+			base = set8->pairs;
+			cnt = set8->cnt;
+			size = sizeof(set8->pairs[0]);
+		}
 
 		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
 			 (idx + 1) * sizeof(int), cnt, id->name);
 
-		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
+		qsort(base, cnt, size, cmp_id);
 
 		next = rb_next(next);
 	}
diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 2f882d5cb30f..72535f00572f 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -8,6 +8,15 @@  struct btf_id_set {
 	u32 ids[];
 };
 
+struct btf_id_set8 {
+	u32 cnt;
+	u32 flags;
+	struct {
+		u32 id;
+		u32 flags;
+	} pairs[];
+};
+
 #ifdef CONFIG_DEBUG_INFO_BTF
 
 #include <linux/compiler.h> /* for __PASTE */