* [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
@ 2023-04-24 9:09 Xueming Feng
2023-04-24 16:44 ` Kui-Feng Lee
2023-04-25 1:07 ` Yonghong Song
0 siblings, 2 replies; 11+ messages in thread
From: Xueming Feng @ 2023-04-24 9:09 UTC (permalink / raw
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel, Xueming Feng
When using `bpftool map dump` in plain format, it is usually
more convenient to show the inner map id instead of raw value.
Changing this behavior would help with quick debugging with
`bpftool`, without disrupting scripted behavior. Since user
could dump the inner map with id, and need to convert value.
Signed-off-by: Xueming Feng <kuro@kuroa.me>
---
Changes in v2:
- Fix commit message grammar.
- Change `print_uint` to only print to stdout, make `arg` const, and rename
`n` to `arg_size`.
- Make `print_uint` able to take any size of argument up to `unsigned long`,
and print it as unsigned decimal.
Thanks for the review and suggestions! I have changed my patch accordingly.
There is a possibility that `arg_size` is larger than `unsigned long`,
but previous review suggested that it should be up to the caller function to
set `arg_size` correctly. So I didn't add check for that, should I?
tools/bpf/bpftool/main.c | 15 +++++++++++++++
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/map.c | 9 +++++++--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 08d0ac543c67..810c0dc10ecb 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
return 0;
}
+void print_uint(const void *arg, unsigned int arg_size)
+{
+ const unsigned char *data = arg;
+ unsigned long val = 0ul;
+
+ #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+ memcpy(&val, data, arg_size);
+ #else
+ memcpy((unsigned char *)&val + sizeof(val) - arg_size,
+ data, arg_size);
+ #endif
+
+ fprintf(stdout, "%lu", val);
+}
+
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
{
unsigned char *data = arg;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 0ef373cef4c7..0de671423431 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
bool is_prefix(const char *pfx, const char *str);
int detect_common_prefix(const char *arg, ...);
+void print_uint(const void *arg, unsigned int arg_size);
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
void usage(void) __noreturn;
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index aaeb8939e137..f5be4c0564cf 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
}
if (info->value_size) {
- printf("value:%c", break_names ? '\n' : ' ');
- fprint_hex(stdout, value, info->value_size, " ");
+ if (map_is_map_of_maps(info->type)) {
+ printf("id:%c", break_names ? '\n' : ' ');
+ print_uint(value, info->value_size);
+ } else {
+ printf("value:%c", break_names ? '\n' : ' ');
+ fprint_hex(stdout, value, info->value_size, " ");
+ }
}
printf("\n");
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-24 9:09 [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types Xueming Feng
@ 2023-04-24 16:44 ` Kui-Feng Lee
2023-04-25 3:58 ` Xueming Feng
2023-04-25 1:07 ` Yonghong Song
1 sibling, 1 reply; 11+ messages in thread
From: Kui-Feng Lee @ 2023-04-24 16:44 UTC (permalink / raw
To: Xueming Feng, Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel
On 4/24/23 02:09, Xueming Feng wrote:
> When using `bpftool map dump` in plain format, it is usually
> more convenient to show the inner map id instead of raw value.
> Changing this behavior would help with quick debugging with
> `bpftool`, without disrupting scripted behavior. Since user
> could dump the inner map with id, and need to convert value.
>
> Signed-off-by: Xueming Feng <kuro@kuroa.me>
> ---
> Changes in v2:
> - Fix commit message grammar.
> - Change `print_uint` to only print to stdout, make `arg` const, and rename
> `n` to `arg_size`.
> - Make `print_uint` able to take any size of argument up to `unsigned long`,
> and print it as unsigned decimal.
>
> Thanks for the review and suggestions! I have changed my patch accordingly.
> There is a possibility that `arg_size` is larger than `unsigned long`,
> but previous review suggested that it should be up to the caller function to
> set `arg_size` correctly. So I didn't add check for that, should I?
>
> tools/bpf/bpftool/main.c | 15 +++++++++++++++
> tools/bpf/bpftool/main.h | 1 +
> tools/bpf/bpftool/map.c | 9 +++++++--
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 08d0ac543c67..810c0dc10ecb 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
> return 0;
> }
>
> +void print_uint(const void *arg, unsigned int arg_size)
> +{
> + const unsigned char *data = arg;
> + unsigned long val = 0ul;
> +
> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> + memcpy(&val, data, arg_size);
> + #else
> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
> + data, arg_size);
> + #endif
Is it possible that arg_size is bigger than sizeof(val)?
> +
> + fprintf(stdout, "%lu", val);
> +}
> +
> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
> {
> unsigned char *data = arg;
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0ef373cef4c7..0de671423431 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>
> bool is_prefix(const char *pfx, const char *str);
> int detect_common_prefix(const char *arg, ...);
> +void print_uint(const void *arg, unsigned int arg_size);
> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
> void usage(void) __noreturn;
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index aaeb8939e137..f5be4c0564cf 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
> }
>
> if (info->value_size) {
> - printf("value:%c", break_names ? '\n' : ' ');
> - fprint_hex(stdout, value, info->value_size, " ");
> + if (map_is_map_of_maps(info->type)) {
> + printf("id:%c", break_names ? '\n' : ' ');
> + print_uint(value, info->value_size);
> + } else {
> + printf("value:%c", break_names ? '\n' : ' ');
> + fprint_hex(stdout, value, info->value_size, " ");
> + }
> }
>
> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-24 9:09 [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types Xueming Feng
2023-04-24 16:44 ` Kui-Feng Lee
@ 2023-04-25 1:07 ` Yonghong Song
2023-04-25 4:10 ` Xueming Feng
1 sibling, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2023-04-25 1:07 UTC (permalink / raw
To: Xueming Feng, Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel
On 4/24/23 2:09 AM, Xueming Feng wrote:
> When using `bpftool map dump` in plain format, it is usually
> more convenient to show the inner map id instead of raw value.
> Changing this behavior would help with quick debugging with
> `bpftool`, without disrupting scripted behavior. Since user
> could dump the inner map with id, and need to convert value.
>
> Signed-off-by: Xueming Feng <kuro@kuroa.me>
> ---
> Changes in v2:
> - Fix commit message grammar.
> - Change `print_uint` to only print to stdout, make `arg` const, and rename
> `n` to `arg_size`.
> - Make `print_uint` able to take any size of argument up to `unsigned long`,
> and print it as unsigned decimal.
>
> Thanks for the review and suggestions! I have changed my patch accordingly.
> There is a possibility that `arg_size` is larger than `unsigned long`,
> but previous review suggested that it should be up to the caller function to
> set `arg_size` correctly. So I didn't add check for that, should I?
>
> tools/bpf/bpftool/main.c | 15 +++++++++++++++
> tools/bpf/bpftool/main.h | 1 +
> tools/bpf/bpftool/map.c | 9 +++++++--
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 08d0ac543c67..810c0dc10ecb 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
> return 0;
> }
>
> +void print_uint(const void *arg, unsigned int arg_size)
> +{
> + const unsigned char *data = arg;
> + unsigned long val = 0ul;
> +
> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> + memcpy(&val, data, arg_size);
> + #else
> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
> + data, arg_size);
> + #endif
> +
> + fprintf(stdout, "%lu", val);
> +}
> +
> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
> {
> unsigned char *data = arg;
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0ef373cef4c7..0de671423431 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>
> bool is_prefix(const char *pfx, const char *str);
> int detect_common_prefix(const char *arg, ...);
> +void print_uint(const void *arg, unsigned int arg_size);
> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
> void usage(void) __noreturn;
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index aaeb8939e137..f5be4c0564cf 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
> }
>
> if (info->value_size) {
> - printf("value:%c", break_names ? '\n' : ' ');
> - fprint_hex(stdout, value, info->value_size, " ");
> + if (map_is_map_of_maps(info->type)) {
> + printf("id:%c", break_names ? '\n' : ' ');
> + print_uint(value, info->value_size);
For all map_in_map types, the inner map value size is 32bit int which
represents a fd (for map creation) and a id (for map info), e.g., in
show_prog_maps() in prog.c. So maybe we can simplify the code as below:
printf("id: %u", *(unsigned int *)value);
> + } else {
> + printf("value:%c", break_names ? '\n' : ' ');
> + fprint_hex(stdout, value, info->value_size, " ");
> + }
> }
>
> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-24 16:44 ` Kui-Feng Lee
@ 2023-04-25 3:58 ` Xueming Feng
2023-04-25 5:19 ` Kui-Feng Lee
0 siblings, 1 reply; 11+ messages in thread
From: Xueming Feng @ 2023-04-25 3:58 UTC (permalink / raw
To: sinquersw
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
kuro, linux-kernel, martin.lau, quentin, sdf, song, yhs
> On 4/24/23 02:09, Xueming Feng wrote:
>> When using `bpftool map dump` in plain format, it is usually
>> more convenient to show the inner map id instead of raw value.
>> Changing this behavior would help with quick debugging with
>> `bpftool`, without disrupting scripted behavior. Since user
>> could dump the inner map with id, and need to convert value.
>>
>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>> ---
>> Changes in v2:
>> - Fix commit message grammar.
>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>> `n` to `arg_size`.
>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>> and print it as unsigned decimal.
>>
>> Thanks for the review and suggestions! I have changed my patch accordingly.
>> There is a possibility that `arg_size` is larger than `unsigned long`,
>> but previous review suggested that it should be up to the caller function to
>> set `arg_size` correctly. So I didn't add check for that, should I?
>>
>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>> tools/bpf/bpftool/main.h | 1 +
>> tools/bpf/bpftool/map.c | 9 +++++++--
>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>> index 08d0ac543c67..810c0dc10ecb 100644
>> --- a/tools/bpf/bpftool/main.c
>> +++ b/tools/bpf/bpftool/main.c
>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>> return 0;
>> }
>>
>> +void print_uint(const void *arg, unsigned int arg_size)
>> +{
>> + const unsigned char *data = arg;
>> + unsigned long val = 0ul;
>> +
>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> + memcpy(&val, data, arg_size);
>> + #else
>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>> + data, arg_size);
>> + #endif
On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote:
> Is it possible that arg_size is bigger than sizeof(val)?
Yes it is possible, I had the thought of adding a check. But as I mentioned
before the diff section, previous review
https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that
I should leave it to the caller function to behave. If I were to add a check,
what action do you recommend if the check fails? Print a '-1', do nothing,
or just use the first sizeof(val) bytes?
>> +
>> + fprintf(stdout, "%lu", val);
>> +}
>> +
>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>> {
>> unsigned char *data = arg;
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 0ef373cef4c7..0de671423431 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>
>> bool is_prefix(const char *pfx, const char *str);
>> int detect_common_prefix(const char *arg, ...);
>> +void print_uint(const void *arg, unsigned int arg_size);
>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>> void usage(void) __noreturn;
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index aaeb8939e137..f5be4c0564cf 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>> }
>>
>> if (info->value_size) {
>> - printf("value:%c", break_names ? '\n' : ' ');
>> - fprint_hex(stdout, value, info->value_size, " ");
>> + if (map_is_map_of_maps(info->type)) {
>> + printf("id:%c", break_names ? '\n' : ' ');
>> + print_uint(value, info->value_size);
>> + } else {
>> + printf("value:%c", break_names ? '\n' : ' ');
>> + fprint_hex(stdout, value, info->value_size, " ");
>> + }
>> }
>>
>> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 1:07 ` Yonghong Song
@ 2023-04-25 4:10 ` Xueming Feng
2023-04-25 5:58 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Xueming Feng @ 2023-04-25 4:10 UTC (permalink / raw
To: yhs
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
kuro, linux-kernel, martin.lau, quentin, sdf, song, yhs
> On 4/24/23 2:09 AM, Xueming Feng wrote:
>> When using `bpftool map dump` in plain format, it is usually
>> more convenient to show the inner map id instead of raw value.
>> Changing this behavior would help with quick debugging with
>> `bpftool`, without disrupting scripted behavior. Since user
>> could dump the inner map with id, and need to convert value.
>>
>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>> ---
>> Changes in v2:
>> - Fix commit message grammar.
>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>> `n` to `arg_size`.
>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>> and print it as unsigned decimal.
>>
>> Thanks for the review and suggestions! I have changed my patch accordingly.
>> There is a possibility that `arg_size` is larger than `unsigned long`,
>> but previous review suggested that it should be up to the caller function to
>> set `arg_size` correctly. So I didn't add check for that, should I?
>>
>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>> tools/bpf/bpftool/main.h | 1 +
>> tools/bpf/bpftool/map.c | 9 +++++++--
>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>> index 08d0ac543c67..810c0dc10ecb 100644
>> --- a/tools/bpf/bpftool/main.c
>> +++ b/tools/bpf/bpftool/main.c
>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>> return 0;
>> }
>>
>> +void print_uint(const void *arg, unsigned int arg_size)
>> +{
>> + const unsigned char *data = arg;
>> + unsigned long val = 0ul;
>> +
>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> + memcpy(&val, data, arg_size);
>> + #else
>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>> + data, arg_size);
>> + #endif
>> +
>> + fprintf(stdout, "%lu", val);
>> +}
>> +
>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>> {
>> unsigned char *data = arg;
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 0ef373cef4c7..0de671423431 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>
>> bool is_prefix(const char *pfx, const char *str);
>> int detect_common_prefix(const char *arg, ...);
>> +void print_uint(const void *arg, unsigned int arg_size);
>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>> void usage(void) __noreturn;
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index aaeb8939e137..f5be4c0564cf 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>> }
>>
>> if (info->value_size) {
>> - printf("value:%c", break_names ? '\n' : ' ');
>> - fprint_hex(stdout, value, info->value_size, " ");
>> + if (map_is_map_of_maps(info->type)) {
>> + printf("id:%c", break_names ? '\n' : ' ');
>1> + print_uint(value, info->value_size);
On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote:
> For all map_in_map types, the inner map value size is 32bit int which
> represents a fd (for map creation) and a id (for map info), e.g., in
> show_prog_maps() in prog.c. So maybe we can simplify the code as below:
> printf("id: %u", *(unsigned int *)value);
That is true, maybe the "id" could also be changed to "map_id" to follow the
convention. Do you think that `print_uint` could be useful in the future?
If that is the case, should I keep using it here as an example usage, and to
avoid dead code? Or should I just remove it?
>> + } else {
>> + printf("value:%c", break_names ? '\n' : ' ');
>> + fprint_hex(stdout, value, info->value_size, " ");
>> + }
>> }
>>
>> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 3:58 ` Xueming Feng
@ 2023-04-25 5:19 ` Kui-Feng Lee
2023-04-25 6:09 ` Xueming Feng
0 siblings, 1 reply; 11+ messages in thread
From: Kui-Feng Lee @ 2023-04-25 5:19 UTC (permalink / raw
To: Xueming Feng
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
linux-kernel, martin.lau, quentin, sdf, song, yhs
On 4/24/23 20:58, Xueming Feng wrote:
>> On 4/24/23 02:09, Xueming Feng wrote:
>>> When using `bpftool map dump` in plain format, it is usually
>>> more convenient to show the inner map id instead of raw value.
>>> Changing this behavior would help with quick debugging with
>>> `bpftool`, without disrupting scripted behavior. Since user
>>> could dump the inner map with id, and need to convert value.
>>>
>>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>>> ---
>>> Changes in v2:
>>> - Fix commit message grammar.
>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>> `n` to `arg_size`.
>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>> and print it as unsigned decimal.
>>>
>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>> but previous review suggested that it should be up to the caller function to
>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>
>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>> tools/bpf/bpftool/main.h | 1 +
>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>> index 08d0ac543c67..810c0dc10ecb 100644
>>> --- a/tools/bpf/bpftool/main.c
>>> +++ b/tools/bpf/bpftool/main.c
>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>> return 0;
>>> }
>>>
>>> +void print_uint(const void *arg, unsigned int arg_size)
>>> +{
>>> + const unsigned char *data = arg;
>>> + unsigned long val = 0ul;
>>> +
>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>> + memcpy(&val, data, arg_size);
>>> + #else
>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>> + data, arg_size);
>>> + #endif
>
> On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote:
>> Is it possible that arg_size is bigger than sizeof(val)?
>
> Yes it is possible, I had the thought of adding a check. But as I mentioned
> before the diff section, previous review
> https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that
> I should leave it to the caller function to behave. If I were to add a check,
> what action do you recommend if the check fails? Print a '-1', do nothing,
> or just use the first sizeof(val) bytes?
In the previous patch, it may have integer overflow, but it is never
buffer underrun. This version uses memcpy and may cause buffer underrun
if arg_size is bigger than sizeof(val). I would say that at least
prevent buffer underrun from happening.
>
>>> +
>>> + fprintf(stdout, "%lu", val);
>>> +}
>>> +
>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>> {
>>> unsigned char *data = arg;
>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>> index 0ef373cef4c7..0de671423431 100644
>>> --- a/tools/bpf/bpftool/main.h
>>> +++ b/tools/bpf/bpftool/main.h
>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>
>>> bool is_prefix(const char *pfx, const char *str);
>>> int detect_common_prefix(const char *arg, ...);
>>> +void print_uint(const void *arg, unsigned int arg_size);
>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>> void usage(void) __noreturn;
>>>
>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>> index aaeb8939e137..f5be4c0564cf 100644
>>> --- a/tools/bpf/bpftool/map.c
>>> +++ b/tools/bpf/bpftool/map.c
>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>> }
>>>
>>> if (info->value_size) {
>>> - printf("value:%c", break_names ? '\n' : ' ');
>>> - fprint_hex(stdout, value, info->value_size, " ");
>>> + if (map_is_map_of_maps(info->type)) {
>>> + printf("id:%c", break_names ? '\n' : ' ');
>>> + print_uint(value, info->value_size);
>>> + } else {
>>> + printf("value:%c", break_names ? '\n' : ' ');
>>> + fprint_hex(stdout, value, info->value_size, " ");
>>> + }
>>> }
>>>
>>> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 4:10 ` Xueming Feng
@ 2023-04-25 5:58 ` Yonghong Song
2023-04-25 6:37 ` Xueming Feng
0 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2023-04-25 5:58 UTC (permalink / raw
To: Xueming Feng
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
linux-kernel, martin.lau, quentin, sdf, song, yhs
On 4/24/23 9:10 PM, Xueming Feng wrote:
>> On 4/24/23 2:09 AM, Xueming Feng wrote:
>>> When using `bpftool map dump` in plain format, it is usually
>>> more convenient to show the inner map id instead of raw value.
>>> Changing this behavior would help with quick debugging with
>>> `bpftool`, without disrupting scripted behavior. Since user
>>> could dump the inner map with id, and need to convert value.
>>>
>>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>>> ---
>>> Changes in v2:
>>> - Fix commit message grammar.
>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>> `n` to `arg_size`.
>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>> and print it as unsigned decimal.
>>>
>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>> but previous review suggested that it should be up to the caller function to
>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>
>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>> tools/bpf/bpftool/main.h | 1 +
>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>> index 08d0ac543c67..810c0dc10ecb 100644
>>> --- a/tools/bpf/bpftool/main.c
>>> +++ b/tools/bpf/bpftool/main.c
>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>> return 0;
>>> }
>>>
>>> +void print_uint(const void *arg, unsigned int arg_size)
>>> +{
>>> + const unsigned char *data = arg;
>>> + unsigned long val = 0ul;
>>> +
>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>> + memcpy(&val, data, arg_size);
>>> + #else
>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>> + data, arg_size);
>>> + #endif
>>> +
>>> + fprintf(stdout, "%lu", val);
>>> +}
>>> +
>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>> {
>>> unsigned char *data = arg;
>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>> index 0ef373cef4c7..0de671423431 100644
>>> --- a/tools/bpf/bpftool/main.h
>>> +++ b/tools/bpf/bpftool/main.h
>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>
>>> bool is_prefix(const char *pfx, const char *str);
>>> int detect_common_prefix(const char *arg, ...);
>>> +void print_uint(const void *arg, unsigned int arg_size);
>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>> void usage(void) __noreturn;
>>>
>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>> index aaeb8939e137..f5be4c0564cf 100644
>>> --- a/tools/bpf/bpftool/map.c
>>> +++ b/tools/bpf/bpftool/map.c
>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>> }
>>>
>>> if (info->value_size) {
>>> - printf("value:%c", break_names ? '\n' : ' ');
>>> - fprint_hex(stdout, value, info->value_size, " ");
>>> + if (map_is_map_of_maps(info->type)) {
>>> + printf("id:%c", break_names ? '\n' : ' ');
>> 1> + print_uint(value, info->value_size);
>
> On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote:
>> For all map_in_map types, the inner map value size is 32bit int which
>> represents a fd (for map creation) and a id (for map info), e.g., in
>> show_prog_maps() in prog.c. So maybe we can simplify the code as below:
>> printf("id: %u", *(unsigned int *)value);
>
> That is true, maybe the "id" could also be changed to "map_id" to follow the
> convention. Do you think that `print_uint` could be useful in the future?
> If that is the case, should I keep using it here as an example usage, and to
> avoid dead code? Or should I just remove it?
Maybe, "inner_map_id" is a better choice. For array of maps, some array
element value could be 0, implying "inner_map_id 0", but I think it is
okay, people should know a real inner_map_id (or any map_id) should
never be 0.
Function "print_uint" is not needed any more. Please remove it.
Please add the command line to dump map values triggering the above
change, also the actual dumps with and without this patch.
>
>>> + } else {
>>> + printf("value:%c", break_names ? '\n' : ' ');
>>> + fprint_hex(stdout, value, info->value_size, " ");
>>> + }
>>> }
>>>
>>> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 5:19 ` Kui-Feng Lee
@ 2023-04-25 6:09 ` Xueming Feng
0 siblings, 0 replies; 11+ messages in thread
From: Xueming Feng @ 2023-04-25 6:09 UTC (permalink / raw
To: sinquersw
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
kuro, linux-kernel, martin.lau, quentin, sdf, song, yhs
>On 4/24/23 20:58, Xueming Feng wrote:
>>> On 4/24/23 02:09, Xueming Feng wrote:
>>>> When using `bpftool map dump` in plain format, it is usually
>>>> more convenient to show the inner map id instead of raw value.
>>>> Changing this behavior would help with quick debugging with
>>>> `bpftool`, without disrupting scripted behavior. Since user
>>>> could dump the inner map with id, and need to convert value.
>>>>
>>>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>>>> ---
>>>> Changes in v2:
>>>> - Fix commit message grammar.
>>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>>> `n` to `arg_size`.
>>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>>> and print it as unsigned decimal.
>>>>
>>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>>> but previous review suggested that it should be up to the caller function to
>>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>>
>>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>>> tools/bpf/bpftool/main.h | 1 +
>>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>>> index 08d0ac543c67..810c0dc10ecb 100644
>>>> --- a/tools/bpf/bpftool/main.c
>>>> +++ b/tools/bpf/bpftool/main.c
>>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>>> return 0;
>>>> }
>>>>
>>>> +void print_uint(const void *arg, unsigned int arg_size)
>>>> +{
>>>> + const unsigned char *data = arg;
>>>> + unsigned long val = 0ul;
>>>> +
>>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> + memcpy(&val, data, arg_size);
>>>> + #else
>>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>>> + data, arg_size);
>>>> + #endif
>>
>> On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote:
>>> Is it possible that arg_size is bigger than sizeof(val)?
>>
>> Yes it is possible, I had the thought of adding a check. But as I mentioned
>> before the diff section, previous review
>> https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that
>> I should leave it to the caller function to behave. If I were to add a check,
>> what action do you recommend if the check fails? Print a '-1', do nothing,
>> or just use the first sizeof(val) bytes?
On Mon, 24 Apr 2023 22:19:52 -0700, Kui-Feng Lee wrote:
> In the previous patch, it may have integer overflow, but it is never
> buffer underrun. This version uses memcpy and may cause buffer underrun
> if arg_size is bigger than sizeof(val). I would say that at least
> prevent buffer underrun from happening.
Thanks for the advice! This function is pending remove in the next version of
this patch. But I will remember this for future patches.
>>> +
>>> + fprintf(stdout, "%lu", val);
>>> +}
>>> +
>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>> {
>>> unsigned char *data = arg;
>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>> index 0ef373cef4c7..0de671423431 100644
>>> --- a/tools/bpf/bpftool/main.h
>>> +++ b/tools/bpf/bpftool/main.h
>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>
>>> bool is_prefix(const char *pfx, const char *str);
>>> int detect_common_prefix(const char *arg, ...);
>>> +void print_uint(const void *arg, unsigned int arg_size);
>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>> void usage(void) __noreturn;
>>>
>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>> index aaeb8939e137..f5be4c0564cf 100644
>>> --- a/tools/bpf/bpftool/map.c
>>> +++ b/tools/bpf/bpftool/map.c
>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>> }
>>>
>>> if (info->value_size) {
>>> - printf("value:%c", break_names ? '\n' : ' ');
>>> - fprint_hex(stdout, value, info->value_size, " ");
>>> + if (map_is_map_of_maps(info->type)) {
>>> + printf("id:%c", break_names ? '\n' : ' ');
>>> + print_uint(value, info->value_size);
>>> + } else {
>>> + printf("value:%c", break_names ? '\n' : ' ');
>>> + fprint_hex(stdout, value, info->value_size, " ");
>>> + }
>>> }
>>>
>>> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 5:58 ` Yonghong Song
@ 2023-04-25 6:37 ` Xueming Feng
2023-04-25 8:57 ` Quentin Monnet
0 siblings, 1 reply; 11+ messages in thread
From: Xueming Feng @ 2023-04-25 6:37 UTC (permalink / raw
To: yhs
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
kuro, linux-kernel, martin.lau, quentin, sdf, song, yhs
>On 4/24/23 9:10 PM, Xueming Feng wrote:
>>> On 4/24/23 2:09 AM, Xueming Feng wrote:
>>>> When using `bpftool map dump` in plain format, it is usually
>>>> more convenient to show the inner map id instead of raw value.
>>>> Changing this behavior would help with quick debugging with
>>>> `bpftool`, without disrupting scripted behavior. Since user
>>>> could dump the inner map with id, and need to convert value.
>>>>
>>>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>>>> ---
>>>> Changes in v2:
>>>> - Fix commit message grammar.
>>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>>> `n` to `arg_size`.
>>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>>> and print it as unsigned decimal.
>>>>
>>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>>> but previous review suggested that it should be up to the caller function to
>>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>>
>>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>>> tools/bpf/bpftool/main.h | 1 +
>>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>>> index 08d0ac543c67..810c0dc10ecb 100644
>>>> --- a/tools/bpf/bpftool/main.c
>>>> +++ b/tools/bpf/bpftool/main.c
>>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>>> return 0;
>>>> }
>>>>
>>>> +void print_uint(const void *arg, unsigned int arg_size)
>>>> +{
>>>> + const unsigned char *data = arg;
>>>> + unsigned long val = 0ul;
>>>> +
>>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> + memcpy(&val, data, arg_size);
>>>> + #else
>>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>>> + data, arg_size);
>>>> + #endif
>>>> +
>>>> + fprintf(stdout, "%lu", val);
>>>> +}
>>>> +
>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>>> {
>>>> unsigned char *data = arg;
>>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>>> index 0ef373cef4c7..0de671423431 100644
>>>> --- a/tools/bpf/bpftool/main.h
>>>> +++ b/tools/bpf/bpftool/main.h
>>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>>
>>>> bool is_prefix(const char *pfx, const char *str);
>>>> int detect_common_prefix(const char *arg, ...);
>>>> +void print_uint(const void *arg, unsigned int arg_size);
>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>>> void usage(void) __noreturn;
>>>>
>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>> index aaeb8939e137..f5be4c0564cf 100644
>>>> --- a/tools/bpf/bpftool/map.c
>>>> +++ b/tools/bpf/bpftool/map.c
>>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>>> }
>>>>
>>>> if (info->value_size) {
>>>> - printf("value:%c", break_names ? '\n' : ' ');
>>>> - fprint_hex(stdout, value, info->value_size, " ");
>>>> + if (map_is_map_of_maps(info->type)) {
>>>> + printf("id:%c", break_names ? '\n' : ' ');
>>> 1> + print_uint(value, info->value_size);
>>
>> On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote:
>>> For all map_in_map types, the inner map value size is 32bit int which
>>> represents a fd (for map creation) and a id (for map info), e.g., in
>>> show_prog_maps() in prog.c. So maybe we can simplify the code as below:
>>> printf("id: %u", *(unsigned int *)value);
>>
>> That is true, maybe the "id" could also be changed to "map_id" to follow the
>> convention. Do you think that `print_uint` could be useful in the future?
>> If that is the case, should I keep using it here as an example usage, and to
>> avoid dead code? Or should I just remove it?
On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote:
> Maybe, "inner_map_id" is a better choice. For array of maps, some array
> element value could be 0, implying "inner_map_id 0", but I think it is
> okay, people should know a real inner_map_id (or any map_id) should
> never be 0.
>
> Function "print_uint" is not needed any more. Please remove it.
Will reflect this in v3.
>
> Please add the command line to dump map values triggering the above
> change, also the actual dumps with and without this patch.
$ bpftool map dump id 138
Without patch:
```
key:
fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
27 16 06 00
value:
8b 00 00 00
Found 1 element
```
With patch:
```
key:
fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
27 16 06 00
inner_map_id:
139
Found 1 element
```
>>
>>>> + } else {
>>>> + printf("value:%c", break_names ? '\n' : ' ');
>>>> + fprint_hex(stdout, value, info->value_size, " ");
>>>> + }
>>>> }
>>>>
>>>> printf("\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 6:37 ` Xueming Feng
@ 2023-04-25 8:57 ` Quentin Monnet
2023-04-25 9:52 ` Xueming Feng
0 siblings, 1 reply; 11+ messages in thread
From: Quentin Monnet @ 2023-04-25 8:57 UTC (permalink / raw
To: Xueming Feng, yhs
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
linux-kernel, martin.lau, sdf, song, yhs
2023-04-25 14:37 UTC+0800 ~ Xueming Feng <kuro@kuroa.me>
>> On 4/24/23 9:10 PM, Xueming Feng wrote:
>>>> On 4/24/23 2:09 AM, Xueming Feng wrote:
>>>>> When using `bpftool map dump` in plain format, it is usually
>>>>> more convenient to show the inner map id instead of raw value.
>>>>> Changing this behavior would help with quick debugging with
>>>>> `bpftool`, without disrupting scripted behavior. Since user
>>>>> could dump the inner map with id, and need to convert value.
>>>>>
>>>>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Fix commit message grammar.
>>>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>>>> `n` to `arg_size`.
>>>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>>>> and print it as unsigned decimal.
>>>>>
>>>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>>>> but previous review suggested that it should be up to the caller function to
>>>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>>>
>>>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>>>> tools/bpf/bpftool/main.h | 1 +
>>>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>>>> index 08d0ac543c67..810c0dc10ecb 100644
>>>>> --- a/tools/bpf/bpftool/main.c
>>>>> +++ b/tools/bpf/bpftool/main.c
>>>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +void print_uint(const void *arg, unsigned int arg_size)
>>>>> +{
>>>>> + const unsigned char *data = arg;
>>>>> + unsigned long val = 0ul;
>>>>> +
>>>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>> + memcpy(&val, data, arg_size);
>>>>> + #else
>>>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>>>> + data, arg_size);
>>>>> + #endif
>>>>> +
>>>>> + fprintf(stdout, "%lu", val);
>>>>> +}
>>>>> +
>>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>>>> {
>>>>> unsigned char *data = arg;
>>>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>>>> index 0ef373cef4c7..0de671423431 100644
>>>>> --- a/tools/bpf/bpftool/main.h
>>>>> +++ b/tools/bpf/bpftool/main.h
>>>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>>>
>>>>> bool is_prefix(const char *pfx, const char *str);
>>>>> int detect_common_prefix(const char *arg, ...);
>>>>> +void print_uint(const void *arg, unsigned int arg_size);
>>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>>>> void usage(void) __noreturn;
>>>>>
>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>>> index aaeb8939e137..f5be4c0564cf 100644
>>>>> --- a/tools/bpf/bpftool/map.c
>>>>> +++ b/tools/bpf/bpftool/map.c
>>>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>>>> }
>>>>>
>>>>> if (info->value_size) {
>>>>> - printf("value:%c", break_names ? '\n' : ' ');
>>>>> - fprint_hex(stdout, value, info->value_size, " ");
>>>>> + if (map_is_map_of_maps(info->type)) {
>>>>> + printf("id:%c", break_names ? '\n' : ' ');
>>>> 1> + print_uint(value, info->value_size);
>>>
>>> On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote:
>>>> For all map_in_map types, the inner map value size is 32bit int which
>>>> represents a fd (for map creation) and a id (for map info), e.g., in
>>>> show_prog_maps() in prog.c. So maybe we can simplify the code as below:
>>>> printf("id: %u", *(unsigned int *)value);
>>>
>>> That is true, maybe the "id" could also be changed to "map_id" to follow the
>>> convention. Do you think that `print_uint` could be useful in the future?
>>> If that is the case, should I keep using it here as an example usage, and to
>>> avoid dead code? Or should I just remove it?
This makes me think we could also have something similar for prog_array
maps (but not necessarily as part of your patchset).
>
> On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote:
>> Maybe, "inner_map_id" is a better choice. For array of maps, some array
>> element value could be 0, implying "inner_map_id 0", but I think it is
>> okay, people should know a real inner_map_id (or any map_id) should
>> never be 0.
>>
>> Function "print_uint" is not needed any more. Please remove it.
>
> Will reflect this in v3.
>
>>
>> Please add the command line to dump map values triggering the above
>> change, also the actual dumps with and without this patch.
>
> $ bpftool map dump id 138
> Without patch:
> ```
> key:
> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
> 27 16 06 00
> value:
> 8b 00 00 00
> Found 1 element
> ```
> With patch:
> ```
> key:
> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
> 27 16 06 00
> inner_map_id:
> 139
> Found 1 element
> ```
Thanks! Please add those sample outputs to the commit description for
v3. Can you please also add an example with JSON ("-p")?
Quentin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
2023-04-25 8:57 ` Quentin Monnet
@ 2023-04-25 9:52 ` Xueming Feng
0 siblings, 0 replies; 11+ messages in thread
From: Xueming Feng @ 2023-04-25 9:52 UTC (permalink / raw
To: quentin
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa, kpsingh,
kuro, linux-kernel, martin.lau, sdf, song, yhs, yhs
>2023-04-25 14:37 UTC+0800 ~ Xueming Feng <kuro@kuroa.me>
>>> On 4/24/23 9:10 PM, Xueming Feng wrote:
>>>>> On 4/24/23 2:09 AM, Xueming Feng wrote:
>>>>>> When using `bpftool map dump` in plain format, it is usually
>>>>>> more convenient to show the inner map id instead of raw value.
>>>>>> Changing this behavior would help with quick debugging with
>>>>>> `bpftool`, without disrupting scripted behavior. Since user
>>>>>> could dump the inner map with id, and need to convert value.
>>>>>>
>>>>>> Signed-off-by: Xueming Feng <kuro@kuroa.me>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - Fix commit message grammar.
>>>>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>>>>> `n` to `arg_size`.
>>>>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>>>>> and print it as unsigned decimal.
>>>>>>
>>>>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>>>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>>>>> but previous review suggested that it should be up to the caller function to
>>>>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>>>>
>>>>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>>>>> tools/bpf/bpftool/main.h | 1 +
>>>>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>>>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>>>>> index 08d0ac543c67..810c0dc10ecb 100644
>>>>>> --- a/tools/bpf/bpftool/main.c
>>>>>> +++ b/tools/bpf/bpftool/main.c
>>>>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +void print_uint(const void *arg, unsigned int arg_size)
>>>>>> +{
>>>>>> + const unsigned char *data = arg;
>>>>>> + unsigned long val = 0ul;
>>>>>> +
>>>>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>>> + memcpy(&val, data, arg_size);
>>>>>> + #else
>>>>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>>>>> + data, arg_size);
>>>>>> + #endif
>>>>>> +
>>>>>> + fprintf(stdout, "%lu", val);
>>>>>> +}
>>>>>> +
>>>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>>>>> {
>>>>>> unsigned char *data = arg;
>>>>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>>>>> index 0ef373cef4c7..0de671423431 100644
>>>>>> --- a/tools/bpf/bpftool/main.h
>>>>>> +++ b/tools/bpf/bpftool/main.h
>>>>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>>>>
>>>>>> bool is_prefix(const char *pfx, const char *str);
>>>>>> int detect_common_prefix(const char *arg, ...);
>>>>>> +void print_uint(const void *arg, unsigned int arg_size);
>>>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>>>>> void usage(void) __noreturn;
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>>>> index aaeb8939e137..f5be4c0564cf 100644
>>>>>> --- a/tools/bpf/bpftool/map.c
>>>>>> +++ b/tools/bpf/bpftool/map.c
>>>>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>>>>> }
>>>>>>
>>>>>> if (info->value_size) {
>>>>>> - printf("value:%c", break_names ? '\n' : ' ');
>>>>>> - fprint_hex(stdout, value, info->value_size, " ");
>>>>>> + if (map_is_map_of_maps(info->type)) {
>>>>>> + printf("id:%c", break_names ? '\n' : ' ');
>>>>> 1> + print_uint(value, info->value_size);
>>>>
>>>> On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote:
>>>>> For all map_in_map types, the inner map value size is 32bit int which
>>>>> represents a fd (for map creation) and a id (for map info), e.g., in
>>>>> show_prog_maps() in prog.c. So maybe we can simplify the code as below:
>>>>> printf("id: %u", *(unsigned int *)value);
>>>>
>>>> That is true, maybe the "id" could also be changed to "map_id" to follow the
>>>> convention. Do you think that `print_uint` could be useful in the future?
>>>> If that is the case, should I keep using it here as an example usage, and to
>>>> avoid dead code? Or should I just remove it?
On Tue, 25 Apr 2023 09:57:17 +0100, Quentin Monnet wrote:
> This makes me think we could also have something similar for prog_array
> maps (but not necessarily as part of your patchset).
Good idea, will take a look at that after finishing v3 patch, and possibly
make a separate patch for it. (This is my first time contributing, better keep
it simple).
>> On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote:
>>> Maybe, "inner_map_id" is a better choice. For array of maps, some array
>>> element value could be 0, implying "inner_map_id 0", but I think it is
>>> okay, people should know a real inner_map_id (or any map_id) should
>>> never be 0.
>>>
>>> Function "print_uint" is not needed any more. Please remove it.
>>
>> Will reflect this in v3.
>>
>>>
>>> Please add the command line to dump map values triggering the above
>>> change, also the actual dumps with and without this patch.
>>
>> $ bpftool map dump id 138
>> Without patch:
>> ```
>> key:
>> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
>> 27 16 06 00
>> value:
>> 8b 00 00 00
>> Found 1 element
>> ```
>> With patch:
>> ```
>> key:
>> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
>> 27 16 06 00
>> inner_map_id:
>> 139
>> Found 1 element
>> ```
> Thanks! Please add those sample outputs to the commit description for
> v3. Can you please also add an example with JSON ("-p")?
Sure, will add them in v3! About json output, they are currently not touched
to avoid breaking scripts. I will add inner_map_id as a new field in v3 like the
following patch.
https://lore.kernel.org/bpf/20230419003651.988865-1-kuifeng@meta.com/
> Quentin
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-04-25 9:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-24 9:09 [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types Xueming Feng
2023-04-24 16:44 ` Kui-Feng Lee
2023-04-25 3:58 ` Xueming Feng
2023-04-25 5:19 ` Kui-Feng Lee
2023-04-25 6:09 ` Xueming Feng
2023-04-25 1:07 ` Yonghong Song
2023-04-25 4:10 ` Xueming Feng
2023-04-25 5:58 ` Yonghong Song
2023-04-25 6:37 ` Xueming Feng
2023-04-25 8:57 ` Quentin Monnet
2023-04-25 9:52 ` Xueming Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.