Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <90518c5d-36ea-ec97-9f14-0687fdd6074f@fb.com>
Date:   Mon, 1 Nov 2021 15:21:20 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.2.1
Subject: Re: [PATCH -next v2] bpf/benchs: Fix return value check of
 bpf_program__attach()
Content-Language: en-US
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Yang Yingliang <yangyingliang@huawei.com>,
        Joanne Koong <joannekoong@fb.com>,
        Andrii Nakryiko <andrii@kernel.org>
CC:     LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>
References: <20211101124310.3947887-1-yangyingliang@huawei.com>
 <CAADnVQJS_2St=iaqHU+zasy_0A0bidJN=STnkHrNcSNL5vO1Dg@mail.gmail.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <CAADnVQJS_2St=iaqHU+zasy_0A0bidJN=STnkHrNcSNL5vO1Dg@mail.gmail.com>
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?VGVJR1ppOU9IbjY1ZUdzR1hnRXlpZVErU1RhRWlnbE9xYlBHTzVaUjNaWXFy?=
 =?utf-8?B?aGlyMVhFYnlWY290QVdYOFNuc3BrdWRTZXg5ZGNWUC9ySXY1SVI0dXVkSkVG?=
 =?utf-8?B?Y1lpV2RXRlJwakFIc2ZzVzRrRzdSMmN5RVkrNFlISHV2Y21QZnRiTE5WYzhl?=
 =?utf-8?B?UHJZdGF6YS9MZGYwdHBJbmxKZTJKOFRPakxNazlJNjg1RjQrWG05dDlmbml6?=
 =?utf-8?B?NXY4WjdrVXlCbGxrek1PR3NwT0VoUnQvbnJyRGpPeVJYbWtNRjIxaGtHSjJT?=
 =?utf-8?B?MzErclV3cUF4ZnZPdUUrWHhuZml2QXM3MjZZTldLMGRDUUZyNW54aUxsbmww?=
 =?utf-8?B?bEJzdURJWC9ueTBxZkZ3a1ZUQzcrQi91K2JwVmx1NEtZV2ptYzBIU2RPS0Nn?=
 =?utf-8?B?U1YvRm5qTEp2aGZzd2tIUUp5b0c2cGtpd1ZlRVRMUXc4MndHd0I1akZsK0I0?=
 =?utf-8?B?eDdhL2ZGa3h3SWhRZmpFUmpBSDIwdnJGeHRFdDdwVVozMEcxY09Da0IzSks5?=
 =?utf-8?B?d2FKNklvRkYza2hXYkN2ay94V2FQTUdYUkpsblRBbjdUMTUwc0N0VG10NURP?=
 =?utf-8?B?NW9XT0xjS3RDdGdCTDhSOUtEMEl5czdkaGV4SG9HLzNESzRKMjlNVlh1MW9s?=
 =?utf-8?B?ZkFYN1VOYWMvRGVqRnhNTWkxMHpyMzVDeEk5UVFRbnpIVG9mTWxDakZFbFMx?=
 =?utf-8?B?TndBZzIyVlFNM3JzUGVDQWpVWE5SSk15M3JHUUl2QVUrM2dwRGQ3UlkvdGtG?=
 =?utf-8?B?V0FVdzB2YjBIQ1M5STNWVGpqaU1ha0hlblZUVUtzSFNHb2tOQU9TN0FVUENF?=
 =?utf-8?B?aU5nSUZ6ckVqZmRNSmU3RVV4b2VySXgrUklndU44SkpYN0FOMmpxT3l4ZzBC?=
 =?utf-8?B?ZjBpdFF1a1J1dDRwN3hFK094bC9Kd3dDam9FYmRoWXpyRWJlanhxRlJYWWhw?=
 =?utf-8?B?QjArUE9jNkFJbGRkVVVtSkRXU3NRcTB4T3BlYThWa0Q5QVMwZ2JydStBRXJj?=
 =?utf-8?B?TjBjNVRnSEdob202Yk56ZU5ScDZGT28yWGJ3WUpVT2xkcjB4Y2VhMmc2V0Vp?=
 =?utf-8?B?NFNTY0lIeDBiQ1RBcHRZc3N3KzlLV1liNzJsa2p0NS8zelBhQUt4WmtOOEpD?=
 =?utf-8?B?cDFhOHdQOEkzcnhTVkZYdzJxM0xnakh6V0JxYU9yaE1kK09LcnVFdkFVc1U5?=
 =?utf-8?B?SmU0OHdKQURBeTJyL0F4dUd1N2VmUm5JbEVyeWVXTkFXTGN4R2pOTGIzdlB6?=
 =?utf-8?B?U2xnbVVQdmE3NTUrSkZXQ1YwYnVwSHJMd09wUEhFcXRnWW5EOUp6ZHBuUnJ5?=
 =?utf-8?B?VVB1VWRGb3JBdWJvSk01U3kvK2Y4b1VUbllYNG0zaFl0RWtsL1dCcnNTYVRv?=
 =?utf-8?B?aHlVZ0J5enY4Rmt6VnVSWmRLWUtHR055UHB0ZkNIQnc0UlB5WkgzYkxjVTc4?=
 =?utf-8?B?U3VPQmJjNlRwc1hzR3c3STh0REQ2L0xsRFdmU0NtTWxqR2k4M2lUU2JjR2Jq?=
 =?utf-8?B?R3ZWcVRwTTZjOWZPdHpUTnVmTmdmcXZaTnlhWFN3eEVVRng4UmJ1V2pGZXVH?=
 =?utf-8?B?dStpRUMyZW9jbk42YVpWQmFCTEdWb0x2TEQyWU9xc3FxQ2hhMllrY1JVUkJs?=
 =?utf-8?B?TXM1T0JDYWpRTSs3OE9yQnRpU1U4Qi9NUktXYU1XR08vMVB4OGc5VU41UWhx?=
 =?utf-8?B?OGNoTzRPaDdlVXgxWDJ3dE5kM2VOV3BWbnVnRDdEdVRDQ0dPaE1zdXlHNXRh?=
 =?utf-8?B?dldhQUJvak5JVzdjYkFOQmNGamVrUnJtOCtqVWE1ZURwbFc0ZktwQStNeGRJ?=
 =?utf-8?B?cm1pU2svMFhJcElWcXhMQ0FuUm1CNkRrL2wydVRXY2F3WEFwZXBndjYvMUUr?=
 =?utf-8?B?alg5WVVCTnJ1VGNOZDQ1dVE4bFBtVVZPcmpGaHZLTU50RjdZQ25wemZMaWpp?=
 =?utf-8?B?Yko0cDVSbWR3NlVSb2FoYXFsb0lRbDJFR0FDRmdYVnhCQkxaeHlTeks5ZUlh?=
 =?utf-8?B?MjF3WlJjZXpsWHZZZmx6TW1wSUN6djFScTcyaDBEWVB5VFI3L2s1M0NyOW5v?=
 =?utf-8?B?VksycGpyS1hyUm54M3A5dXlSWHBpM29XazBtT2dmaE9kRlJPbHpqUFlHWU51?=
 =?utf-8?Q?1QOF0y6jwlmv4P9htX4fA4HRd?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f0baa856-74e0-4b8e-9e93-08d99d85f0e3
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Nov 2021 22:21:24.8735
 (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: V749PTBXUI+XnkwQlrct0HHoOfnjXqAhaQw4XwMZyQ62RAQxrpdDvnfx/ljj6Np8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR15MB2367
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: tWLJggohAJOsvZKQK5quK_G4TLmp57eI
X-Proofpoint-ORIG-GUID: tWLJggohAJOsvZKQK5quK_G4TLmp57eI
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475
 definitions=2021-11-01_07,2021-11-01_01,2020-04-07_01
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 mlxscore=0 phishscore=0
 clxscore=1015 malwarescore=0 mlxlogscore=752 spamscore=0 impostorscore=0
 lowpriorityscore=0 adultscore=0 suspectscore=0 bulkscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2111010117
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 11/1/21 3:00 PM, Alexei Starovoitov wrote:
> On Mon, Nov 1, 2021 at 5:35 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
>>
>> If bpf_program__attach() fails, it never returns NULL,
>> we should use libbpf_get_error() to check the return value.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> Acked-by: Yonghong Song <yhs@fb.com>
>> ---
>> v2:
>>    don't use 'int err'
>> ---
>>   .../selftests/bpf/benchs/bench_bloom_filter_map.c      | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c
>> index 6eeeed2913e6..4afaa4adb327 100644
>> --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c
>> +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c
>> @@ -304,7 +304,7 @@ static void bloom_lookup_setup(void)
>>          populate_maps();
>>
>>          link = bpf_program__attach(ctx.skel->progs.bloom_lookup);
>> -       if (!link) {
>> +       if (libbpf_get_error(link)) {
> 
> Please use ASSERT_OK_PTR() instead.
> See how other tests are doing it.

I actually looked at this. ASSERT_OK_PTR() is defined in test_progs.h
and test_progs.h is ONLY included in files which eventually linked to
test_progs. That is why I didn't recommend to use ASSERT_OK_PTR().

Maybe it is okay to include test_progs.h in benchs/*.c. Or we may
want to refactor to a separate header file to contain these macros
which can be used for test_progs.h and other applications.

>