Re: [PATCH bpf-next 5/5] selftests/bpf: fix bench runner SIGSEGV

[Yonghong Song <yonghong.song@linux.dev>]

On 1/31/24 9:17 AM, Andrii Nakryiko wrote:
> On Tue, Jan 30, 2024 at 11:41 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 1/30/24 11:36 AM, Andrii Nakryiko wrote:
>>> Some benchmarks don't anticipate "consumer" and/or "producer" sides. Add
>> For this, you mean some future potential benchmarks, right?
> No, existing ones as well. Like trig-tp and other "trigger"
> benchmarks. I ran into this when I was trying to set consumers to 0
> explicitly, which wasn't allowed due to <= check. Then I fixed the
> check, and I ran into SIGSEGV. So I decided to fix that up.

Some description like this in the commit message will be good.

>
>>> NULL checks in corresponding places and warn about inappropriate
>>> consumer/producer count argument values.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>>
>>> ---
>>>    tools/testing/selftests/bpf/bench.c | 10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
>>> index 73ce11b0547d..36962fc305eb 100644
>>> --- a/tools/testing/selftests/bpf/bench.c
>>> +++ b/tools/testing/selftests/bpf/bench.c
>>> @@ -330,7 +330,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>>>                break;
>>>        case 'c':
>>>                env.consumer_cnt = strtol(arg, NULL, 10);
>>> -             if (env.consumer_cnt <= 0) {
>>> +             if (env.consumer_cnt < 0) {
>>>                        fprintf(stderr, "Invalid consumer count: %s\n", arg);
>>>                        argp_usage(state);
>>>                }
>>> @@ -607,6 +607,10 @@ static void setup_benchmark(void)
>>>                bench->setup();
>>>
>>>        for (i = 0; i < env.consumer_cnt; i++) {
>>> +             if (!bench->consumer_thread) {
>>> +                     fprintf(stderr, "benchmark doesn't have consumers!\n");
>>> +                     exit(1);
>>> +             }
>>>                err = pthread_create(&state.consumers[i], NULL,
>>>                                     bench->consumer_thread, (void *)(long)i);
>>>                if (err) {
>>> @@ -626,6 +630,10 @@ static void setup_benchmark(void)
>>>                env.prod_cpus.next_cpu = env.cons_cpus.next_cpu;
>>>
>>>        for (i = 0; i < env.producer_cnt; i++) {
>>> +             if (!bench->producer_thread) {
>>> +                     fprintf(stderr, "benchmark doesn't have producers!\n");
>>> +                     exit(1);
>>> +             }
>>>                err = pthread_create(&state.producers[i], NULL,
>>>                                     bench->producer_thread, (void *)(long)i);
>>>                if (err) {