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: <9c49d2a7-0729-6ed3-2e09-6d07f3eda04b@meta.com>
Date:   Wed, 26 Apr 2023 21:58:17 -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 v3] 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: <20230427041949.93752-1-kuro@kuroa.me>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <20230427041949.93752-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?UzhISlJJSVMzOE52VlpROFZRYmNGRlJzMEk1REFoa2g1SENiaTRLc01rbU9l?=
 =?utf-8?B?cDY3dEc4OGdaNWZkak1iTGY3WXlyYTY3V01mYlhVekdFOUx2bEsvY2h2ME9J?=
 =?utf-8?B?NDRPRHkzWHlQajQrdUhHZVIyVXdCRU04OHRwZzlRNnZYTWdGRy9NQTF1N3dL?=
 =?utf-8?B?eUtZbnliaGxERm8vTGRCN3BQUEh1emIvdDRWOHR3Q2VPeVRTS000MlRONEJr?=
 =?utf-8?B?WHY4Mjc5ci9wSStkZ1I5QzdkQ3I4c0k0NjBZVldTWVdQOU9UQW5zdWVUaTd1?=
 =?utf-8?B?RmUrZzV0UUpHaEFwT2N5WXczZGNFREwwUVlCMEd5bUh1K1dEMWxmaVlKeFhr?=
 =?utf-8?B?NWVZRVdyRkEvUWlxbFA3NGZBU0gzVi9jVnlpTDdSaGdvanFBMmIwYVBNNUFR?=
 =?utf-8?B?dkh4NWhsQWdCcnhBVVM1WUVXcHBIamtTOUJETWlCKzZpa01COXRBUDhnOUpl?=
 =?utf-8?B?QnB0SGErRFMwYlZHcFZ6OWw3eGVRbExQZ3luU3o4Uk82bjdDSDEvN0dRdEpi?=
 =?utf-8?B?MnRLN0RHdFBEWEtac0tvdjVWcDlQRUhleWJNWGhaVnFDWjVtOVNTMlRsaXNt?=
 =?utf-8?B?WlR4VHRrWTFvak1aSHMxWmIrOFV5N1pOUXJVS1RNZXpuNHRsU21nTWhIQXNW?=
 =?utf-8?B?akRzWTFFU3VoK3gzN3czQ0l3ZzFVOTQySVBpekxnVEF4RTJaSFhpL0FYZUxh?=
 =?utf-8?B?dXJjL2dNQkJ0WS9yaVhpaWlHT0tCNnRaa1J6eWFVdXJaSGhURytmUHlhRWc4?=
 =?utf-8?B?OGljckJQZEdqYjJad1g4SHlzQ3RNWkp3WkVZb1NEdTFqb3dQdzZJdlh3TmM4?=
 =?utf-8?B?UVFJTVZiblBZWDhuam1USDYxUk1iVTczaU1sS2YrZzcxZ1RoU3hDa1FQOVpG?=
 =?utf-8?B?RklOMHMrdzhwTnU0MEV0d2Y2NGcrWTdXaTZ2cFBNY295U0NEdnlmVEI2U3N6?=
 =?utf-8?B?Z0xGOVZTZER0OWEyeWVGVVplVTdaRnN2U08zN0l6RFlDa1paN3B2cjhWeDVG?=
 =?utf-8?B?TGdoTkxPWGxnTjdrazgzNVhDYlpoVjZuYmtlTFBKNk0vZmZmSHFPZHFyMlB6?=
 =?utf-8?B?YjdXcDBiMGdlNVYvSjZxbExSOGJjMm1xWXFkekpCejJoYnNseVdPclNnc2tU?=
 =?utf-8?B?ci9Hd3c2WEVaUGNjUWMxaU9keEhzR1g5ZTU4QnJyWTEzU0VqYStOQnRDK2Jq?=
 =?utf-8?B?Q2dKNi9DY3djSU9WSnZlNjZzbDczdGQvVms5UGVGQSs0UjErTUdWRVN4and2?=
 =?utf-8?B?U2h0ZzZYQzRoS2duT0s5ZndNMjkwckg0RFB1RVpaR2dhTm1LUlllRitsVmE2?=
 =?utf-8?B?Y3ZNUWpuYjlXVjdmK1IzL25UTDQvMWppcVJFSkpUcEpZdlhFMUNkUWZKM09Z?=
 =?utf-8?B?QXhrV1dNbnpBYW9YTGx6REQydkpPaTRDK2RGSkgzRkcrZXZGRjJ1dG43N3ZV?=
 =?utf-8?B?NDBJUGJVajNHVzhjWVl0TzQ5TDlacnpxUDQvRHh1ZlNQOWRRaTc3WDdCQlJL?=
 =?utf-8?B?TVR1eUdCTXZoWXNCanBpR1RGdFIyS1FTWkZsaWVlU0RXSEw5QUNQMXM4WndG?=
 =?utf-8?B?K0dDUW95bU9DcHJZU2QxQzhJbmg0d2R0czRUeDRMeDVqcVBrWWpPanNLdFJB?=
 =?utf-8?B?dWg0clZhODdBUmdERDlsNzJzMUVYb2IzVEFncXJjUUZKdE5pZDgzZjhIb3VS?=
 =?utf-8?B?d0hvaEI3OU5SOGI2c0lFMW1sWWl3M0FieXhYZDBpMmVMMEhEb1B1QTdoQmVZ?=
 =?utf-8?B?Qys1WTNpQnB5cTdqbzlnL3ppbEtiYU90ZE5vTjlScUoyaUdEUVNmaHljTlZ4?=
 =?utf-8?B?NUpJYTcyN3praHYwWTJHVWtqS2lGYkxndlc2ekRYQ0ozalpKcmxCMnorc0Ey?=
 =?utf-8?B?UmZacXdaMnpmaUJScXRFYm0xb3RRTE5RRGxNVUZDVll4Zmp2ZGIwZ2Q3bW5h?=
 =?utf-8?B?QUNBcjNnQkE0dVNPZDdHNXJkYWZGY0NzT3JwTHlSUTQwa2RHaEE3ei9QS0Fj?=
 =?utf-8?B?eWV1WWp6SW0wa0oycjlkR09Ya0hFUUxTWEZZVitWLzk0T3ZYbTQ1L0V5aC84?=
 =?utf-8?B?Vmc4K2lXbU9xQWtpRDdsSE5EVTRrcmVma3lWeHhTNWFpNG1qL1g3UGxVRHpp?=
 =?utf-8?B?ZzgwU2w3dU91Mnh6a1JCelZYVnNuQmJaS0NIenRRRDY2QlJDNTlEa3M1Q0kv?=
 =?utf-8?B?MEE9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 302efdcd-0fba-4a99-0422-08db46dc0637
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2023 04:58:21.6772
 (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: 8hKHmsp2tM4lHaQYJSQHzFbopsZHqzQ8qin8begXXUA+cIhKr+gLoRdanCjZk+Pz
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB5823
X-Proofpoint-ORIG-GUID: XLmZW8DGfN2eyWqPGcpk5nwaEgl9TQo-
X-Proofpoint-GUID: XLmZW8DGfN2eyWqPGcpk5nwaEgl9TQo-
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_03,2023-04-26_03,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/26/23 9:19 PM, 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>

Ack with a small nit below.

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

> ---
> 
> 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.
> 
> Sorry for taking this long, my compiling machine is acting weird. And thanks for
> the reviews and suggestions!
> 
>   tools/bpf/bpftool/map.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index aaeb8939e137..afcd8455db74 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -139,6 +139,10 @@ 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);
> +		}

There is only statement under 'if' condition. The above
parenthesis ('{}') is not needed.

>   		if (btf) {
>   			struct btf_dumper d = {
>   				.btf = btf,
> @@ -259,8 +263,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");