Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3a96ec27-9175-e161-32ef-5cb40f4e1ea2@meta.com>
Date:   Thu, 27 Apr 2023 07:43:48 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.0
Subject: Re: [PATCH bpf-next v4] bpftool: Dump map id instead of value for
 map_of_maps types
Content-Language: en-US
To:     Xueming Feng <kuro@kuroa.me>,
        Quentin Monnet <quentin@isovalent.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230427120313.43574-1-kuro@kuroa.me>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <20230427120313.43574-1-kuro@kuroa.me>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZUV0aUdPZXdmR25tNnFVd1BsV2tCTVk3a1NYZG82OWgvYm4yR1JUc0lOUHFJ?=
 =?utf-8?B?Vi9kNnVoMXlWSWFDQkFvWjVJWTlrTGdXeStyMUd1Qm9kUGNnTXNqYm84dTR0?=
 =?utf-8?B?UWk0QzJIRjc0VjJIRS9xUGtBTlpIc2ZpdnhDanpFb2l0OTM4NlBpQ0VHd2Iv?=
 =?utf-8?B?M0V4TE9iWi9nVVZldHdNS1ZrNGhPVzJYOWR0eEp1VWpFcTlNZFprSmdTa2or?=
 =?utf-8?B?ZkRwaVR1NVI1Qml1VWlWSWFSa2Nkc2lyUHlPaVExUGp6dGtHTkFKNFduOERS?=
 =?utf-8?B?MTM1V0RDdVloUEJDWGp1MGZacUpYcU15TjIvcHF4L0tOeE56azFlVmVQMjA1?=
 =?utf-8?B?dU1ncnA3WDJ1T3Y5R3M3N0Q5c2xzNG91MnMvSFAxaW1wZXNzazU1ZjZweU5k?=
 =?utf-8?B?dmN6TjJEaUFVdCtiVmV5RnMvM3M4SDBpR3ZZNllmWnAyclBwU0J3aXJCZVIr?=
 =?utf-8?B?VnpLTGhZVG1wbzNzNFJXQkNad1hSVHo0Zy84eUUvaTdKUHZxbitwaXFJZ1Vl?=
 =?utf-8?B?SXJvL0NEUjcwb1BoWlMzN1FNTko5Q2lvWUNJcEV5dmQwUmVOYVpyc2s2bVhr?=
 =?utf-8?B?V2JYMHZvQXByNHpmdm8xOEZmaXNnbkIyWFpyTnNqeitielI1bGFVcXJmSTc3?=
 =?utf-8?B?bnp5ZG1uSWZVNXJzckpZbTREMU0vaHRPdVlwOW5RN28yUlNqMi84OStSakht?=
 =?utf-8?B?VjRvRVcxblc4RTVscXVjNGJGTWVZK1piM2ZpeVpibXliSlRPczVtaktENmpP?=
 =?utf-8?B?OU1pbjVMRWFqdi9zTUpWb3dkblJ4MlVpMlk0STZBVkdwT1dKdWZaekM2a3Iv?=
 =?utf-8?B?eGZSSHN5b3E1YmNQUXZybTM0QmFJay83MUw4TExIZ1ZzTzlEUldkUE11eFM3?=
 =?utf-8?B?SzhualdCaTJxWDhvL1FMenRML0lqaW1ZdjZNays4L0NQOU53WnNobTZmZ2NX?=
 =?utf-8?B?eFlSQkNRMmY0SVBqMVkvTVlBQXovSkQ0N25OaWJDelkzNjlBdUVBbUFlcUtY?=
 =?utf-8?B?RktSNmc5UUZwdWFaWTJ3ekgrR2Y3MG9CMmZWRVpIVWI1algzLzdQdHlmTXNZ?=
 =?utf-8?B?ME51bE1WWExPWXloUFpwajIxNHVoQUx1NWh2eVRUZTRXQVRkcnFnVTFwMjVH?=
 =?utf-8?B?UmNFVExZd0J5NDdGR1ZEL1ZxOXdTOWgzaklDZmhIMWVWVzM2NnNBSllqUytN?=
 =?utf-8?B?T0c0SGpGU2ZuOHRHSnZRNjlGdzlqTmVxYXphOS93alpzK1BwYWxVZWV1aThT?=
 =?utf-8?B?TlpOOUc3VVZKQ2JOalpPVWxNNVBhVHp0RE90WisvVVpQZkNKa1RCNEVHRmQ4?=
 =?utf-8?B?cld3eHJnbjNDT2VlS2RWNnVGRGpKZi85V1IxVlplUkpCbXg5bFczWXVWMTJY?=
 =?utf-8?B?dFVMTjJDdmhFL1gxMTJnT2FlZU9FVHFXZDA1UWJTTzZwVVNUK1VoT2dJcEN4?=
 =?utf-8?B?bHZQcnFsL3ViUmZ1US9oVHZTQ2YzcERuY05HTS9ndWorWlNPVVBEbldLOTQw?=
 =?utf-8?B?V2QwQkJMRFdaS1oxVzZZZ3NIblB1OWRlRndhUnl5RmN0Vmx2OWdKVlhzUHJ5?=
 =?utf-8?B?dGF3Um9pekliQkNBUmFkcm9YSTRUTzlDNG15L2dSTWYyRUQ2WWpHRGNsUGUw?=
 =?utf-8?B?b2RMcGdqOXBPTjQ4WVp5TGJxa1FtMm5HY0hLQjRSNkM2YzE3bVNXbXhkNnJ1?=
 =?utf-8?B?bldWc1ZubEhvN0JhVmw3RUxZTlkvclpvdnlqSVNZd0JDWmFFOWhHV3dzV21G?=
 =?utf-8?B?OGljMWhDaTJHNUs0SmFJazVhSEtDc2kvRkNnc3AzQitMMHZZSFJrZWFXUW9m?=
 =?utf-8?B?UHZRMkxhUC9CbVpqc2h1bEk2b3ZkMnAzRi9sRjV3dUErMmtSbSszdjhxTVRl?=
 =?utf-8?B?WU5DT0JHQWRQMXpiclZSVjk4N25VUmpkM3ZiR1VhZzY1S0lBVGQ5WnpCaGs4?=
 =?utf-8?B?UVYzWHNYYWVwOUtPQ2pFOVlrUThHSkJIVkxaclBBbDEvRzJydU9JUVRGeDY2?=
 =?utf-8?B?Q1FHdUdVY0pVZWNTcHI0TzBRVFpaVEx2dnpBNnJGVVpKUG5oY1N1S3gyWGFi?=
 =?utf-8?B?VDFSWjBkeTBaYW0xZFlmSUU1MDQ4ZVI3MGtBWUVQYkRaUTh3eFBUaUp0VEFJ?=
 =?utf-8?B?WWlIRmRnaXpzSEVBbVZHRGJjQ0syK08za3loS2NrUzg5RVNISUFaQnRWWHlG?=
 =?utf-8?B?WVE9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e82e41d6-7caa-4b4b-9585-08db472dd1f1
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2023 14:43:52.6326
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Zrs0btlT1E7TiY9iUR19L0I1hPsOdmVwYDnOIrlQrosU9Mm321i8WJ5At3uIkR1B
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB4613
X-Proofpoint-GUID: OPfyUbi5NSWUWCThse9NNBHUNwXVjQ4V
X-Proofpoint-ORIG-GUID: OPfyUbi5NSWUWCThse9NNBHUNwXVjQ4V
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-27_07,2023-04-27_01,2023-02-09_01
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/27/23 5:03 AM, Xueming Feng wrote:
> When using `bpftool map dump` with map_of_maps, it is usually
> more convenient to show the inner map id instead of raw value.
> 
> We are changing the plain print behavior to show inner_map_id
> instead of hex value, this would help with quick look up of
> inner map with `bpftool map dump id <inner_map_id>`.
> To avoid disrupting scripted behavior, we will add a new
> `inner_map_id` field to json output instead of replacing value.
> 
> plain print:
> ```
> $ 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
> ```
> 
> json print:
> ```
> $ bpftool -p map dump id 567
> 
> Without Patch:
> [{
>          "key": ["0xc0","0x00","0x02","0x05","0x27","0x16","0x06","0x00"
>          ],
>          "value": ["0x38","0x02","0x00","0x00"
>          ]
>      }
> ]
> 
> With Patch:
> [{
>          "key": ["0xc0","0x00","0x02","0x05","0x27","0x16","0x06","0x00"
>          ],
>          "value": ["0x38","0x02","0x00","0x00"
>          ],
>          "inner_map_id": 568
>      }
> ]
> ```
> 
> Signed-off-by: Xueming Feng <kuro@kuroa.me>

You can carry my Ack from v3 since there is no big changes.
Anyway, Ack again.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
> 
> Changes in v4:
>   - Remove unnecessary parenthesis ('{}').
> 
> Changes in v3:
>   - In plain print, use printf() directly since inner map id is always a 32bit int.
>   - Remove unused print_uint() function.
>   - Rename `id` to `inner_map_id` in plain print output for clearness.
>   - Add a new `inner_map_id` field to json output.
>   - Add example output to commit message.
> 
> 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.
> 
>   tools/bpf/bpftool/map.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index aaeb8939e137..ae9e822aa3fe 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -139,6 +139,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>   		print_hex_data_json(key, info->key_size);
>   		jsonw_name(json_wtr, "value");
>   		print_hex_data_json(value, info->value_size);
> +		if (map_is_map_of_maps(info->type))
> +			jsonw_uint_field(json_wtr, "inner_map_id",
> +					 *(unsigned int *)value);
>   		if (btf) {
>   			struct btf_dumper d = {
>   				.btf = btf,
> @@ -259,8 +262,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("inner_map_id:%c", break_names ? '\n' : ' ');
> +				printf("%u ", *(unsigned int *)value);
> +			} else {
> +				printf("value:%c", break_names ? '\n' : ' ');
> +				fprint_hex(stdout, value, info->value_size, " ");
> +			}
>   		}
>   
>   		printf("\n");