Re: [PATCH bpf-next 09/13] tools/bpftool: add bpftool support for bpf map element iterator

From: Yonghong Song <yhs@fb.com>
To: Quentin Monnet <quentin@isovalent.com>, <bpf@vger.kernel.org>,
	<netdev@vger.kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>, <kernel-team@fb.com>,
	Martin KaFai Lau <kafai@fb.com>
Subject: Re: [PATCH bpf-next 09/13] tools/bpftool: add bpftool support for bpf map element iterator
Date: Thu, 16 Jul 2020 10:42:13 -0700	[thread overview]
Message-ID: <c70ebb0a-538c-a84f-f606-1d08af426fde@fb.com> (raw)
In-Reply-To: <9f865c02-291c-8622-b601-f4613356a469@isovalent.com>

On 7/16/20 9:39 AM, Quentin Monnet wrote:
> 2020-07-13 09:17 UTC-0700 ~ Yonghong Song <yhs@fb.com>
>> The optional parameter "map MAP" can be added to "bpftool iter"
>> command to create a bpf iterator for map elements. For example,
>>    bpftool iter pin ./prog.o /sys/fs/bpf/p1 map id 333
>>
>> For map element bpf iterator "map MAP" parameter is required.
>> Otherwise, bpf link creation will return an error.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   .../bpftool/Documentation/bpftool-iter.rst    | 16 ++++++++--
>>   tools/bpf/bpftool/iter.c                      | 32 ++++++++++++++++---
>>   2 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> index 8dce698eab79..53ee4fb188b4 100644
>> --- a/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> +++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> @@ -17,14 +17,15 @@ SYNOPSIS
>>   ITER COMMANDS
>>   ===================
>>   
>> -|	**bpftool** **iter pin** *OBJ* *PATH*
>> +|	**bpftool** **iter pin** *OBJ* *PATH* [**map** *MAP*]
>>   |	**bpftool** **iter help**
>>   |
>>   |	*OBJ* := /a/file/of/bpf_iter_target.o
>> +|       *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> 
> Please don't change the indentation style (other lines have a tab).

Will fix.

> 
>>   
>>   DESCRIPTION
>>   ===========
>> -	**bpftool iter pin** *OBJ* *PATH*
>> +	**bpftool iter pin** *OBJ* *PATH* [**map** *MAP*]
>>   		  A bpf iterator combines a kernel iterating of
>>   		  particular kernel data (e.g., tasks, bpf_maps, etc.)
>>   		  and a bpf program called for each kernel data object
>> @@ -37,6 +38,10 @@ DESCRIPTION
>>   		  character ('.'), which is reserved for future extensions
>>   		  of *bpffs*.
>>   
>> +                  Map element bpf iterator requires an additional parameter
>> +                  *MAP* so bpf program can iterate over map elements for
>> +                  that map.
>> +
> 
> Same note on indentation.
> 
> Could you please also explain in a few words what the "Map element bpf
> iterator" is? Reusing part of your cover letter (see below) could do,
> it's just so that users not familiar with the concept can get an idea of
> what it does.

Will do.

> 
> ---
> User can have a bpf program in kernel to run with each map element,
> do checking, filtering, aggregation, etc. without copying data
> to user space.
> ---
> 
>>   		  User can then *cat PATH* to see the bpf iterator output.
>>   
>>   	**bpftool iter help**
> 
> [...]
> 
>> @@ -62,13 +83,16 @@ static int do_pin(int argc, char **argv)
>>   	bpf_link__destroy(link);
>>   close_obj:
>>   	bpf_object__close(obj);
>> +close_map_fd:
>> +	if (map_fd >= 0)
>> +		close(map_fd);
>>   	return err;
>>   }
>>   
>>   static int do_help(int argc, char **argv)
>>   {
>>   	fprintf(stderr,
>> -		"Usage: %1$s %2$s pin OBJ PATH\n"
>> +		"Usage: %1$s %2$s pin OBJ PATH [map MAP]\n"
> 
> You probably want to add HELP_SPEC_MAP (as in map.c) to tell the user
> what MAP should be.

Good suggestion.

> 
> Could you please also update the bash completion?

This is always my hardest part! In this case it is
   bpftool iter pin <filedir> <filedir> [map MAP]

Any particular existing bpftool implementation I can imitate?

> 
> Thanks,
> Quentin
> 