diff mbox series

[bpf-next,v2,08/13] bpftool: Add support for qp-trie map

Message ID 20220924133620.4147153-9-houtao@huaweicloud.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Add support for qp-trie with dynptr key | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 3 maintainers not CCed: quentin@isovalent.com song@kernel.org martin.lau@linux.dev
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
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: 0 this patch: 0
netdev/checkpatch warning WARNING: Prefer 'fallthrough;' over fallthrough comment
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 fail Logs for test_maps on s390x with gcc

Commit Message

Hou Tao Sept. 24, 2022, 1:36 p.m. UTC
From: Hou Tao <houtao1@huawei.com>

Support lookup/update/delete/iterate/dump operations for qp-trie in
bpftool. Mainly add two functions: one function to parse dynptr key and
another one to dump dynptr key. The input format of dynptr key is:
"key [hex] size BYTES" and the output format of dynptr key is:
"size BYTES".

The following is the output when using bpftool to manipulate
qp-trie:

  $ bpftool map pin id 724953 /sys/fs/bpf/qp
  $ bpftool map show pinned /sys/fs/bpf/qp
  724953: qp_trie  name qp_trie  flags 0x1
          key 16B  value 4B  max_entries 2  memlock 65536B  map_extra 8
          btf_id 779
          pids test_qp_trie.bi(109167)
  $ bpftool map dump pinned /sys/fs/bpf/qp
  [{
          "key": {
              "size": 4,
              "data": ["0x0","0x0","0x0","0x0"
              ]
          },
          "value": 0
      },{
          "key": {
              "size": 4,
              "data": ["0x0","0x0","0x0","0x1"
              ]
          },
          "value": 2
      }
  ]
  $ bpftool map lookup pinned /sys/fs/bpf/qp key 4 0 0 0 1
  {
      "key": {
          "size": 4,
          "data": ["0x0","0x0","0x0","0x1"
          ]
      },
      "value": 2
  }
  $ bpftool map update pinned /sys/fs/bpf/qp key 4 0 0 0 1 value 2 0 0 0
  $ bpftool map getnext pinned /sys/fs/bpf/qp
  key: None
  next key:
  00 00 00 00
  $ bpftool map getnext pinned /sys/fs/bpf/qp key 4 0 0 0 0
  key:
  00 00 00 00
  next key:
  00 00 00 01
  $ bpftool map getnext pinned /sys/fs/bpf/qp key 4 0 0 0 1
  Error: can't get next key: No such file or directory
  $ bpftool map delete pinned /sys/fs/bpf/qp key 4 0 0 0 1
  $ bpftool map dump pinned /sys/fs/bpf/qp
  [{
          "key": {
              "size": 4,
              "data": ["0x0","0x0","0x0","0x0"
              ]
          },
          "value": 0
      }
  ]

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../bpf/bpftool/Documentation/bpftool-map.rst |   4 +-
 tools/bpf/bpftool/btf_dumper.c                |  33 ++++
 tools/bpf/bpftool/map.c                       | 149 +++++++++++++++---
 3 files changed, 164 insertions(+), 22 deletions(-)

Comments

Quentin Monnet Sept. 27, 2022, 11:24 a.m. UTC | #1
Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
<houtao@huaweicloud.com>
> From: Hou Tao <houtao1@huawei.com>
> 
> Support lookup/update/delete/iterate/dump operations for qp-trie in
> bpftool. Mainly add two functions: one function to parse dynptr key and
> another one to dump dynptr key. The input format of dynptr key is:
> "key [hex] size BYTES" and the output format of dynptr key is:
> "size BYTES".
> 
> The following is the output when using bpftool to manipulate
> qp-trie:
> 
>   $ bpftool map pin id 724953 /sys/fs/bpf/qp
>   $ bpftool map show pinned /sys/fs/bpf/qp
>   724953: qp_trie  name qp_trie  flags 0x1
>           key 16B  value 4B  max_entries 2  memlock 65536B  map_extra 8
>           btf_id 779
>           pids test_qp_trie.bi(109167)
>   $ bpftool map dump pinned /sys/fs/bpf/qp
>   [{
>           "key": {
>               "size": 4,
>               "data": ["0x0","0x0","0x0","0x0"
>               ]
>           },
>           "value": 0
>       },{
>           "key": {
>               "size": 4,
>               "data": ["0x0","0x0","0x0","0x1"
>               ]
>           },
>           "value": 2
>       }
>   ]
>   $ bpftool map lookup pinned /sys/fs/bpf/qp key 4 0 0 0 1
>   {
>       "key": {
>           "size": 4,
>           "data": ["0x0","0x0","0x0","0x1"
>           ]
>       },
>       "value": 2
>   }

The bpftool patch looks good, thanks! I have one comment on the syntax
for the keys, I don't find it intuitive to have the size as the first
BYTE. It makes it awkward to understand what the command does if we read
it in the wild without knowing the map type. I can see two alternatives,
either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
parse_bytes() to make it able to parse as much as it can then count the
bytes, when we don't know in advance how many we get.

Thanks,
Quentin
Hou Tao Sept. 28, 2022, 4:14 a.m. UTC | #2
Hi,

On 9/27/2022 7:24 PM, Quentin Monnet wrote:
> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
> <houtao@huaweicloud.com>
>> From: Hou Tao <houtao1@huawei.com>
>>
>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>> bpftool. Mainly add two functions: one function to parse dynptr key and
>> another one to dump dynptr key. The input format of dynptr key is:
>> "key [hex] size BYTES" and the output format of dynptr key is:
>> "size BYTES".
>>
>> The following is the output when using bpftool to manipulate
>> qp-trie:
>>
>>   $ bpftool map pin id 724953 /sys/fs/bpf/qp
>>   $ bpftool map show pinned /sys/fs/bpf/qp
>>   724953: qp_trie  name qp_trie  flags 0x1
>>           key 16B  value 4B  max_entries 2  memlock 65536B  map_extra 8
>>           btf_id 779
>>           pids test_qp_trie.bi(109167)
>>   $ bpftool map dump pinned /sys/fs/bpf/qp
>>   [{
>>           "key": {
>>               "size": 4,
>>               "data": ["0x0","0x0","0x0","0x0"
>>               ]
>>           },
>>           "value": 0
>>       },{
>>           "key": {
>>               "size": 4,
>>               "data": ["0x0","0x0","0x0","0x1"
>>               ]
>>           },
>>           "value": 2
>>       }
>>   ]
>>   $ bpftool map lookup pinned /sys/fs/bpf/qp key 4 0 0 0 1
>>   {
>>       "key": {
>>           "size": 4,
>>           "data": ["0x0","0x0","0x0","0x1"
>>           ]
>>       },
>>       "value": 2
>>   }
> The bpftool patch looks good, thanks! I have one comment on the syntax
> for the keys, I don't find it intuitive to have the size as the first
> BYTE. It makes it awkward to understand what the command does if we read
> it in the wild without knowing the map type. I can see two alternatives,
> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
> parse_bytes() to make it able to parse as much as it can then count the
> bytes, when we don't know in advance how many we get.
The suggestion is reasonable, but there is also reason for the current choice (
I should written it down in commit message). For dynptr-typed key, these two
proposed suggestions will work. But for key with embedded dynptrs as show below,
both explict key_size keyword and implicit key_size in BYTEs can not express the
key correctly.

struct map_key {
unsigned int cookie;
struct bpf_dynptr name;
struct bpf_dynptr addr;
unsigned int flags;
};

I also had thought about adding another key word "dynptr_key" (or "dyn_key") to
support dynptr-typed key or key with embedded dynptr, and the format will still
be: "dynptr_key size [BYTES]". But at least we can tell it is different with
"key" which is fixed size. What do you think ?
>
> Thanks,
> Quentin
>
> .
Quentin Monnet Sept. 28, 2022, 8:40 a.m. UTC | #3
Wed Sep 28 2022 05:14:45 GMT+0100 (British Summer Time) ~ Hou Tao
<houtao@huaweicloud.com>
> Hi,
> 
> On 9/27/2022 7:24 PM, Quentin Monnet wrote:
>> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
>> <houtao@huaweicloud.com>
>>> From: Hou Tao <houtao1@huawei.com>
>>>
>>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>>> bpftool. Mainly add two functions: one function to parse dynptr key and
>>> another one to dump dynptr key. The input format of dynptr key is:
>>> "key [hex] size BYTES" and the output format of dynptr key is:
>>> "size BYTES".
>>>
>>> The following is the output when using bpftool to manipulate
>>> qp-trie:
>>>
>>>   $ bpftool map pin id 724953 /sys/fs/bpf/qp
>>>   $ bpftool map show pinned /sys/fs/bpf/qp
>>>   724953: qp_trie  name qp_trie  flags 0x1
>>>           key 16B  value 4B  max_entries 2  memlock 65536B  map_extra 8
>>>           btf_id 779
>>>           pids test_qp_trie.bi(109167)
>>>   $ bpftool map dump pinned /sys/fs/bpf/qp
>>>   [{
>>>           "key": {
>>>               "size": 4,
>>>               "data": ["0x0","0x0","0x0","0x0"
>>>               ]
>>>           },
>>>           "value": 0
>>>       },{
>>>           "key": {
>>>               "size": 4,
>>>               "data": ["0x0","0x0","0x0","0x1"
>>>               ]
>>>           },
>>>           "value": 2
>>>       }
>>>   ]
>>>   $ bpftool map lookup pinned /sys/fs/bpf/qp key 4 0 0 0 1
>>>   {
>>>       "key": {
>>>           "size": 4,
>>>           "data": ["0x0","0x0","0x0","0x1"
>>>           ]
>>>       },
>>>       "value": 2
>>>   }
>> The bpftool patch looks good, thanks! I have one comment on the syntax
>> for the keys, I don't find it intuitive to have the size as the first
>> BYTE. It makes it awkward to understand what the command does if we read
>> it in the wild without knowing the map type. I can see two alternatives,
>> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
>> parse_bytes() to make it able to parse as much as it can then count the
>> bytes, when we don't know in advance how many we get.
> The suggestion is reasonable, but there is also reason for the current choice (
> I should written it down in commit message). For dynptr-typed key, these two
> proposed suggestions will work. But for key with embedded dynptrs as show below,
> both explict key_size keyword and implicit key_size in BYTEs can not express the
> key correctly.
> 
> struct map_key {
> unsigned int cookie;
> struct bpf_dynptr name;
> struct bpf_dynptr addr;
> unsigned int flags;
> };

I'm not sure I follow. I don't understand the difference for dealing
internally with the key between "key_size N key BYTES" and "key N BYTES"
(or for parsing then counting). Please could you give an example telling
how you would you express the key from the structure above, with the
syntax you proposed?

> I also had thought about adding another key word "dynptr_key" (or "dyn_key") to
> support dynptr-typed key or key with embedded dynptr, and the format will still
> be: "dynptr_key size [BYTES]". But at least we can tell it is different with
> "key" which is fixed size. What do you think ?
If the other suggestions do not work, then yes, using a dedicated
keyword (Just "dynkey"? We can detail in the docs) sounds better to me.
Hou Tao Sept. 28, 2022, 9:05 a.m. UTC | #4
Hi,

On 9/28/2022 4:40 PM, Quentin Monnet wrote:
> Wed Sep 28 2022 05:14:45 GMT+0100 (British Summer Time) ~ Hou Tao
> <houtao@huaweicloud.com>
>> Hi,
>>
>> On 9/27/2022 7:24 PM, Quentin Monnet wrote:
>>> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
>>> <houtao@huaweicloud.com>
>>>> From: Hou Tao <houtao1@huawei.com>
>>>>
>>>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>>>> bpftool. Mainly add two functions: one function to parse dynptr key and
>>>> another one to dump dynptr key. The input format of dynptr key is:
>>>> "key [hex] size BYTES" and the output format of dynptr key is:
>>>> "size BYTES".
SNIP
>>> The bpftool patch looks good, thanks! I have one comment on the syntax
>>> for the keys, I don't find it intuitive to have the size as the first
>>> BYTE. It makes it awkward to understand what the command does if we read
>>> it in the wild without knowing the map type. I can see two alternatives,
>>> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
>>> parse_bytes() to make it able to parse as much as it can then count the
>>> bytes, when we don't know in advance how many we get.
>> The suggestion is reasonable, but there is also reason for the current choice (
>> I should written it down in commit message). For dynptr-typed key, these two
>> proposed suggestions will work. But for key with embedded dynptrs as show below,
>> both explict key_size keyword and implicit key_size in BYTEs can not express the
>> key correctly.
>>
>> struct map_key {
>> unsigned int cookie;
>> struct bpf_dynptr name;
>> struct bpf_dynptr addr;
>> unsigned int flags;
>> };
> I'm not sure I follow. I don't understand the difference for dealing
> internally with the key between "key_size N key BYTES" and "key N BYTES"
> (or for parsing then counting). Please could you give an example telling
> how you would you express the key from the structure above, with the
> syntax you proposed?
In my understand, if using "key_size N key BYTES" to represent map_key, it can
not tell the exact size of "name" and "addr" and it only can tell the total size
of name and addr. If using "key BYTES" to do that, it has the similar problem.
But if using "key size BYTES" format, map_key can be expressed as follows:

key c c c c [name_size] n n n [addr_size] a a  f f f f
>
>> I also had thought about adding another key word "dynptr_key" (or "dyn_key") to
>> support dynptr-typed key or key with embedded dynptr, and the format will still
>> be: "dynptr_key size [BYTES]". But at least we can tell it is different with
>> "key" which is fixed size. What do you think ?
> If the other suggestions do not work, then yes, using a dedicated
> keyword (Just "dynkey"? We can detail in the docs) sounds better to me.
Quentin Monnet Sept. 28, 2022, 9:23 a.m. UTC | #5
Wed Sep 28 2022 10:05:55 GMT+0100 (British Summer Time) ~ Hou Tao
<houtao@huaweicloud.com>
> Hi,
> 
> On 9/28/2022 4:40 PM, Quentin Monnet wrote:
>> Wed Sep 28 2022 05:14:45 GMT+0100 (British Summer Time) ~ Hou Tao
>> <houtao@huaweicloud.com>
>>> Hi,
>>>
>>> On 9/27/2022 7:24 PM, Quentin Monnet wrote:
>>>> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
>>>> <houtao@huaweicloud.com>
>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>
>>>>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>>>>> bpftool. Mainly add two functions: one function to parse dynptr key and
>>>>> another one to dump dynptr key. The input format of dynptr key is:
>>>>> "key [hex] size BYTES" and the output format of dynptr key is:
>>>>> "size BYTES".
> SNIP
>>>> The bpftool patch looks good, thanks! I have one comment on the syntax
>>>> for the keys, I don't find it intuitive to have the size as the first
>>>> BYTE. It makes it awkward to understand what the command does if we read
>>>> it in the wild without knowing the map type. I can see two alternatives,
>>>> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
>>>> parse_bytes() to make it able to parse as much as it can then count the
>>>> bytes, when we don't know in advance how many we get.
>>> The suggestion is reasonable, but there is also reason for the current choice (
>>> I should written it down in commit message). For dynptr-typed key, these two
>>> proposed suggestions will work. But for key with embedded dynptrs as show below,
>>> both explict key_size keyword and implicit key_size in BYTEs can not express the
>>> key correctly.
>>>
>>> struct map_key {
>>> unsigned int cookie;
>>> struct bpf_dynptr name;
>>> struct bpf_dynptr addr;
>>> unsigned int flags;
>>> };
>> I'm not sure I follow. I don't understand the difference for dealing
>> internally with the key between "key_size N key BYTES" and "key N BYTES"
>> (or for parsing then counting). Please could you give an example telling
>> how you would you express the key from the structure above, with the
>> syntax you proposed?
> In my understand, if using "key_size N key BYTES" to represent map_key, it can
> not tell the exact size of "name" and "addr" and it only can tell the total size
> of name and addr. If using "key BYTES" to do that, it has the similar problem.
> But if using "key size BYTES" format, map_key can be expressed as follows:
> 
> key c c c c [name_size] n n n [addr_size] a a  f f f f

OK thanks I get it now, you can have multiple sizes within the key, one
for each field. Yes, let's use a new keyword in that case please. Can
you also provide more details in the man page, and ideally add a new
example to the list?

Thanks,
Quentin
Hou Tao Sept. 28, 2022, 10:54 a.m. UTC | #6
Hi,

On 9/28/2022 5:23 PM, Quentin Monnet wrote:
> Wed Sep 28 2022 10:05:55 GMT+0100 (British Summer Time) ~ Hou Tao
> <houtao@huaweicloud.com>
>> Hi,
>>
>> On 9/28/2022 4:40 PM, Quentin Monnet wrote:
>>> Wed Sep 28 2022 05:14:45 GMT+0100 (British Summer Time) ~ Hou Tao
>>> <houtao@huaweicloud.com>
>>>> Hi,
>>>>
>>>> On 9/27/2022 7:24 PM, Quentin Monnet wrote:
>>>>> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
>>>>> <houtao@huaweicloud.com>
>>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>>
>>>>>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>>>>>> bpftool. Mainly add two functions: one function to parse dynptr key and
>>>>>> another one to dump dynptr key. The input format of dynptr key is:
>>>>>> "key [hex] size BYTES" and the output format of dynptr key is:
>>>>>> "size BYTES".
>> SNIP
>>>>> The bpftool patch looks good, thanks! I have one comment on the syntax
>>>>> for the keys, I don't find it intuitive to have the size as the first
>>>>> BYTE. It makes it awkward to understand what the command does if we read
>>>>> it in the wild without knowing the map type. I can see two alternatives,
>>>>> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
>>>>> parse_bytes() to make it able to parse as much as it can then count the
>>>>> bytes, when we don't know in advance how many we get.
>>>> The suggestion is reasonable, but there is also reason for the current choice (
>>>> I should written it down in commit message). For dynptr-typed key, these two
>>>> proposed suggestions will work. But for key with embedded dynptrs as show below,
>>>> both explict key_size keyword and implicit key_size in BYTEs can not express the
>>>> key correctly.
>>>>
>>>> struct map_key {
>>>> unsigned int cookie;
>>>> struct bpf_dynptr name;
>>>> struct bpf_dynptr addr;
>>>> unsigned int flags;
>>>> };
>>> I'm not sure I follow. I don't understand the difference for dealing
>>> internally with the key between "key_size N key BYTES" and "key N BYTES"
>>> (or for parsing then counting). Please could you give an example telling
>>> how you would you express the key from the structure above, with the
>>> syntax you proposed?
>> In my understand, if using "key_size N key BYTES" to represent map_key, it can
>> not tell the exact size of "name" and "addr" and it only can tell the total size
>> of name and addr. If using "key BYTES" to do that, it has the similar problem.
>> But if using "key size BYTES" format, map_key can be expressed as follows:
>>
>> key c c c c [name_size] n n n [addr_size] a a  f f f f
> OK thanks I get it now, you can have multiple sizes within the key, one
> for each field. Yes, let's use a new keyword in that case please. Can
> you also provide more details in the man page, and ideally add a new
> example to the list?
Forget to mention that the map key with embedded dynptr is not supported yet and
now only support using dynptr as the map key. So will add a new keyword "dynkey"
in v3 to support operations on qp-trie.
>
> Thanks,
> Quentin
>
> .
Quentin Monnet Sept. 28, 2022, 11:49 a.m. UTC | #7
Wed Sep 28 2022 11:54:39 GMT+0100 (British Summer Time) ~ Hou Tao
<houtao@huaweicloud.com>
> Hi,
> 
> On 9/28/2022 5:23 PM, Quentin Monnet wrote:
>> Wed Sep 28 2022 10:05:55 GMT+0100 (British Summer Time) ~ Hou Tao
>> <houtao@huaweicloud.com>
>>> Hi,
>>>
>>> On 9/28/2022 4:40 PM, Quentin Monnet wrote:
>>>> Wed Sep 28 2022 05:14:45 GMT+0100 (British Summer Time) ~ Hou Tao
>>>> <houtao@huaweicloud.com>
>>>>> Hi,
>>>>>
>>>>> On 9/27/2022 7:24 PM, Quentin Monnet wrote:
>>>>>> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
>>>>>> <houtao@huaweicloud.com>
>>>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>>>
>>>>>>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>>>>>>> bpftool. Mainly add two functions: one function to parse dynptr key and
>>>>>>> another one to dump dynptr key. The input format of dynptr key is:
>>>>>>> "key [hex] size BYTES" and the output format of dynptr key is:
>>>>>>> "size BYTES".
>>> SNIP
>>>>>> The bpftool patch looks good, thanks! I have one comment on the syntax
>>>>>> for the keys, I don't find it intuitive to have the size as the first
>>>>>> BYTE. It makes it awkward to understand what the command does if we read
>>>>>> it in the wild without knowing the map type. I can see two alternatives,
>>>>>> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
>>>>>> parse_bytes() to make it able to parse as much as it can then count the
>>>>>> bytes, when we don't know in advance how many we get.
>>>>> The suggestion is reasonable, but there is also reason for the current choice (
>>>>> I should written it down in commit message). For dynptr-typed key, these two
>>>>> proposed suggestions will work. But for key with embedded dynptrs as show below,
>>>>> both explict key_size keyword and implicit key_size in BYTEs can not express the
>>>>> key correctly.
>>>>>
>>>>> struct map_key {
>>>>> unsigned int cookie;
>>>>> struct bpf_dynptr name;
>>>>> struct bpf_dynptr addr;
>>>>> unsigned int flags;
>>>>> };
>>>> I'm not sure I follow. I don't understand the difference for dealing
>>>> internally with the key between "key_size N key BYTES" and "key N BYTES"
>>>> (or for parsing then counting). Please could you give an example telling
>>>> how you would you express the key from the structure above, with the
>>>> syntax you proposed?
>>> In my understand, if using "key_size N key BYTES" to represent map_key, it can
>>> not tell the exact size of "name" and "addr" and it only can tell the total size
>>> of name and addr. If using "key BYTES" to do that, it has the similar problem.
>>> But if using "key size BYTES" format, map_key can be expressed as follows:
>>>
>>> key c c c c [name_size] n n n [addr_size] a a  f f f f
>> OK thanks I get it now, you can have multiple sizes within the key, one
>> for each field. Yes, let's use a new keyword in that case please. Can
>> you also provide more details in the man page, and ideally add a new
>> example to the list?
> Forget to mention that the map key with embedded dynptr is not supported yet and
> now only support using dynptr as the map key. So will add a new keyword "dynkey"
> in v3 to support operations on qp-trie.

Sounds good thank you, let's do that and ideally mention it in the
commit log for context.
diff mbox series

Patch

diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 7f3b67a8b48f..020df5481fd6 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -45,7 +45,7 @@  MAP COMMANDS
 |	**bpftool** **map help**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* | **name** *MAP_NAME* }
-|	*DATA* := { [**hex**] *BYTES* }
+|	*DATA* := { [**hex**] *BYTES* | [**hex**] *size* *BYTES* }
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
 |	*VALUE* := { *DATA* | *MAP* | *PROG* }
 |	*UPDATE_FLAGS* := { **any** | **exist** | **noexist** }
@@ -55,7 +55,7 @@  MAP COMMANDS
 |		| **devmap** | **devmap_hash** | **sockmap** | **cpumap** | **xskmap** | **sockhash**
 |		| **cgroup_storage** | **reuseport_sockarray** | **percpu_cgroup_storage**
 |		| **queue** | **stack** | **sk_storage** | **struct_ops** | **ringbuf** | **inode_storage**
-|		| **task_storage** | **bloom_filter** | **user_ringbuf** }
+|		| **task_storage** | **bloom_filter** | **user_ringbuf** | **qp_trie** }
 
 DESCRIPTION
 ===========
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
index 19924b6ce796..817868961963 100644
--- a/tools/bpf/bpftool/btf_dumper.c
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -462,6 +462,30 @@  static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset,
 	return 0;
 }
 
+static int btf_dumper_dynptr_user(const struct btf_dumper *d,
+				  const struct bpf_dynptr_user *dynptr)
+{
+	unsigned int i, size;
+	unsigned char *data;
+
+	data = bpf_dynptr_user_get_data(dynptr);
+	size = bpf_dynptr_user_get_size(dynptr);
+
+	jsonw_start_object(d->jw);
+
+	jsonw_name(d->jw, "size");
+	jsonw_printf(d->jw, "%u", size);
+	jsonw_name(d->jw, "data");
+	jsonw_start_array(d->jw);
+	for (i = 0; i < size; i++)
+		jsonw_printf(d->jw, "\"0x%hhx\"", data[i]);
+	jsonw_end_array(d->jw);
+
+	jsonw_end_object(d->jw);
+
+	return 0;
+}
+
 static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id,
 			     const void *data)
 {
@@ -552,6 +576,12 @@  static int btf_dumper_datasec(const struct btf_dumper *d, __u32 type_id,
 	return ret;
 }
 
+static bool btf_is_dynptr(const struct btf *btf, const struct btf_type *t)
+{
+	return t->size == sizeof(struct bpf_dynptr) &&
+	       !strcmp(btf__name_by_offset(btf, t->name_off), "bpf_dynptr");
+}
+
 static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id,
 			      __u8 bit_offset, const void *data)
 {
@@ -562,6 +592,9 @@  static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id,
 		return btf_dumper_int(t, bit_offset, data, d->jw,
 				     d->is_plain_text);
 	case BTF_KIND_STRUCT:
+		if (btf_is_dynptr(d->btf, t))
+			return btf_dumper_dynptr_user(d, data);
+		/* fallthrough */
 	case BTF_KIND_UNION:
 		return btf_dumper_struct(d, type_id, data);
 	case BTF_KIND_ARRAY:
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 9a6ca9f31133..92c175518293 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -43,6 +43,17 @@  static bool map_is_map_of_progs(__u32 type)
 	return type == BPF_MAP_TYPE_PROG_ARRAY;
 }
 
+static bool map_is_dynptr_key(__u32 type)
+{
+	return type == BPF_MAP_TYPE_QP_TRIE;
+}
+
+static bool map_use_map_extra(__u32 type)
+{
+	return type == BPF_MAP_TYPE_BLOOM_FILTER ||
+	       type == BPF_MAP_TYPE_QP_TRIE;
+}
+
 static int map_type_from_str(const char *type)
 {
 	const char *map_type_str;
@@ -130,14 +141,44 @@  static json_writer_t *get_btf_writer(void)
 	return jw;
 }
 
-static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
+static void print_key_by_hex_data_json(struct bpf_map_info *info, void *key)
+{
+	unsigned int data_size;
+	unsigned char *data;
+
+	if (map_is_dynptr_key(info->type)) {
+		data = bpf_dynptr_user_get_data(key);
+		data_size = bpf_dynptr_user_get_size(key);
+	} else {
+		data = key;
+		data_size = info->key_size;
+	}
+	print_hex_data_json(data, data_size);
+}
+
+static void fprint_key_in_hex(struct bpf_map_info *info, void *key)
+{
+	unsigned int data_size;
+	unsigned char *data;
+
+	if (map_is_dynptr_key(info->type)) {
+		data = bpf_dynptr_user_get_data(key);
+		data_size = bpf_dynptr_user_get_size(key);
+	} else {
+		data = key;
+		data_size = info->key_size;
+	}
+	fprint_hex(stdout, data, data_size, " ");
+}
+
+static void print_entry_json(struct bpf_map_info *info, void *key,
 			     unsigned char *value, struct btf *btf)
 {
 	jsonw_start_object(json_wtr);
 
 	if (!map_is_per_cpu(info->type)) {
 		jsonw_name(json_wtr, "key");
-		print_hex_data_json(key, info->key_size);
+		print_key_by_hex_data_json(info, key);
 		jsonw_name(json_wtr, "value");
 		print_hex_data_json(value, info->value_size);
 		if (btf) {
@@ -242,19 +283,23 @@  print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno)
 	}
 }
 
-static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
+static void print_entry_plain(struct bpf_map_info *info, void *key,
 			      unsigned char *value)
 {
 	if (!map_is_per_cpu(info->type)) {
 		bool single_line, break_names;
+		unsigned int key_size;
 
-		break_names = info->key_size > 16 || info->value_size > 16;
-		single_line = info->key_size + info->value_size <= 24 &&
-			!break_names;
+		if (map_is_dynptr_key(info->type))
+			key_size = bpf_dynptr_user_get_size(key);
+		else
+			key_size = info->key_size;
+		break_names = key_size > 16 || info->value_size > 16;
+		single_line = key_size + info->value_size <= 24 && !break_names;
 
-		if (info->key_size) {
+		if (key_size) {
 			printf("key:%c", break_names ? '\n' : ' ');
-			fprint_hex(stdout, key, info->key_size, " ");
+			fprint_key_in_hex(info, key);
 
 			printf(single_line ? "  " : "\n");
 		}
@@ -316,6 +361,38 @@  static char **parse_bytes(char **argv, const char *name, unsigned char *val,
 	return argv + i;
 }
 
+/* The format of dynptr is "[hex] size BYTES" */
+static char **parse_dynptr(char **argv, const char *name,
+			   struct bpf_dynptr_user *dynptr)
+{
+	unsigned int base = 0, size;
+	char *endptr;
+
+	if (is_prefix(*argv, "hex")) {
+		base = 16;
+		argv++;
+	}
+	if (!*argv) {
+		p_err("no byte length");
+		return NULL;
+	}
+
+	size = strtoul(*argv, &endptr, base);
+	if (*endptr) {
+		p_err("error parsing byte length: %s", *argv);
+		return NULL;
+	}
+	if (!size || size > bpf_dynptr_user_get_size(dynptr)) {
+		p_err("invalid byte length %u (max length %u)",
+		      size, bpf_dynptr_user_get_size(dynptr));
+		return NULL;
+	}
+	bpf_dynptr_user_trim(dynptr, size);
+
+	return parse_bytes(argv + 1, name, bpf_dynptr_user_get_data(dynptr),
+			   size);
+}
+
 /* on per cpu maps we must copy the provided value on all value instances */
 static void fill_per_cpu_value(struct bpf_map_info *info, void *value)
 {
@@ -350,7 +427,10 @@  static int parse_elem(char **argv, struct bpf_map_info *info,
 			return -1;
 		}
 
-		argv = parse_bytes(argv + 1, "key", key, key_size);
+		if (map_is_dynptr_key(info->type))
+			argv = parse_dynptr(argv + 1, "key", key);
+		else
+			argv = parse_bytes(argv + 1, "key", key, key_size);
 		if (!argv)
 			return -1;
 
@@ -568,6 +648,9 @@  static int show_map_close_plain(int fd, struct bpf_map_info *info)
 		printf("  memlock %sB", memlock);
 	free(memlock);
 
+	if (map_use_map_extra(info->type))
+		printf("  map_extra %llu", info->map_extra);
+
 	if (info->type == BPF_MAP_TYPE_PROG_ARRAY) {
 		char *owner_prog_type = get_fdinfo(fd, "owner_prog_type");
 		char *owner_jited = get_fdinfo(fd, "owner_jited");
@@ -820,6 +903,18 @@  static void free_btf_vmlinux(void)
 		btf__free(btf_vmlinux);
 }
 
+static struct bpf_dynptr_user *bpf_dynptr_user_new(__u32 size)
+{
+	struct bpf_dynptr_user *dynptr;
+
+	dynptr = malloc(sizeof(*dynptr) + size);
+	if (!dynptr)
+		return NULL;
+
+	bpf_dynptr_user_init(&dynptr[1], size, dynptr);
+	return dynptr;
+}
+
 static int
 map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
 	 bool show_header)
@@ -829,7 +924,10 @@  map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
 	struct btf *btf = NULL;
 	int err;
 
-	key = malloc(info->key_size);
+	if (map_is_dynptr_key(info->type))
+		key = bpf_dynptr_user_new(info->map_extra);
+	else
+		key = malloc(info->key_size);
 	value = alloc_value(info);
 	if (!key || !value) {
 		p_err("mem alloc failed");
@@ -966,7 +1064,10 @@  static int alloc_key_value(struct bpf_map_info *info, void **key, void **value)
 	*value = NULL;
 
 	if (info->key_size) {
-		*key = malloc(info->key_size);
+		if (map_is_dynptr_key(info->type))
+			*key = bpf_dynptr_user_new(info->map_extra);
+		else
+			*key = malloc(info->key_size);
 		if (!*key) {
 			p_err("key mem alloc failed");
 			return -1;
@@ -1132,8 +1233,13 @@  static int do_getnext(int argc, char **argv)
 	if (fd < 0)
 		return -1;
 
-	key = malloc(info.key_size);
-	nextkey = malloc(info.key_size);
+	if (map_is_dynptr_key(info.type)) {
+		key = bpf_dynptr_user_new(info.map_extra);
+		nextkey = bpf_dynptr_user_new(info.map_extra);
+	} else {
+		key = malloc(info.key_size);
+		nextkey = malloc(info.key_size);
+	}
 	if (!key || !nextkey) {
 		p_err("mem alloc failed");
 		err = -1;
@@ -1160,23 +1266,23 @@  static int do_getnext(int argc, char **argv)
 		jsonw_start_object(json_wtr);
 		if (key) {
 			jsonw_name(json_wtr, "key");
-			print_hex_data_json(key, info.key_size);
+			print_key_by_hex_data_json(&info, key);
 		} else {
 			jsonw_null_field(json_wtr, "key");
 		}
 		jsonw_name(json_wtr, "next_key");
-		print_hex_data_json(nextkey, info.key_size);
+		print_key_by_hex_data_json(&info, nextkey);
 		jsonw_end_object(json_wtr);
 	} else {
 		if (key) {
 			printf("key:\n");
-			fprint_hex(stdout, key, info.key_size, " ");
+			fprint_key_in_hex(&info, key);
 			printf("\n");
 		} else {
 			printf("key: None\n");
 		}
 		printf("next key:\n");
-		fprint_hex(stdout, nextkey, info.key_size, " ");
+		fprint_key_in_hex(&info, nextkey);
 		printf("\n");
 	}
 
@@ -1203,7 +1309,10 @@  static int do_delete(int argc, char **argv)
 	if (fd < 0)
 		return -1;
 
-	key = malloc(info.key_size);
+	if (map_is_dynptr_key(info.type))
+		key = bpf_dynptr_user_new(info.map_extra);
+	else
+		key = malloc(info.key_size);
 	if (!key) {
 		p_err("mem alloc failed");
 		err = -1;
@@ -1449,7 +1558,7 @@  static int do_help(int argc, char **argv)
 		"       %1$s %2$s help\n"
 		"\n"
 		"       " HELP_SPEC_MAP "\n"
-		"       DATA := { [hex] BYTES }\n"
+		"       DATA := { [hex] BYTES | [hex] size BYTES }\n"
 		"       " HELP_SPEC_PROGRAM "\n"
 		"       VALUE := { DATA | MAP | PROG }\n"
 		"       UPDATE_FLAGS := { any | exist | noexist }\n"
@@ -1459,7 +1568,7 @@  static int do_help(int argc, char **argv)
 		"                 devmap | devmap_hash | sockmap | cpumap | xskmap | sockhash |\n"
 		"                 cgroup_storage | reuseport_sockarray | percpu_cgroup_storage |\n"
 		"                 queue | stack | sk_storage | struct_ops | ringbuf | inode_storage |\n"
-		"                 task_storage | bloom_filter | user_ringbuf }\n"
+		"                 task_storage | bloom_filter | user_ringbuf | qp_trie }\n"
 		"       " HELP_SPEC_OPTIONS " |\n"
 		"                    {-f|--bpffs} | {-n|--nomount} }\n"
 		"",