Re: [PATCH bpf-next v6 5/5] selftests/bpf: Add benchmark for bpf memory allocator

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, Jun 13, 2023 at 04:09:21PM +0800, Hou Tao wrote:
> +
> +static void htab_mem_notify_wait_producer(pthread_barrier_t *notify)

notify_wait and wait_notify names are confusing.
The first one is doing map_update and 2nd is map_delete, right?
Just call them such?

> +{
> +	while (true) {
> +		(void)syscall(__NR_getpgid);
> +		/* Notify for start */

the comment is confusing too.
Maybe /* Notify map_deleter that map_updates are done */ ?

> +		pthread_barrier_wait(notify);
> +		/* Wait for completion */

and /* Wait for deletions to complete */ ?

> +		pthread_barrier_wait(notify);
> +	}
> +}
> +
> +static void htab_mem_wait_notify_producer(pthread_barrier_t *notify)
> +{
> +	while (true) {
> +		/* Wait for start */
> +		pthread_barrier_wait(notify);
> +		(void)syscall(__NR_getpgid);
> +		/* Notify for completion */

similar.

> +		pthread_barrier_wait(notify);
> +	}
> +}


> +static int write_htab(unsigned int i, struct update_ctx *ctx, unsigned int flags)
> +{
> +	if (ctx->from >= MAX_ENTRIES)
> +		return 1;

It can never be hit, right?
Remove it then?

> +
> +	bpf_map_update_elem(&htab, &ctx->from, zeroed_value, flags);

please add error check.
I think update/delete notification is correct, but it could be silently broken.
update(BPF_NOEXIST) could be returning error in one thread and
map_delete_elem could be failing too...

> +	ctx->from += ctx->step;
> +
> +	return 0;
> +}
> +
> +static int overwrite_htab(unsigned int i, struct update_ctx *ctx)
> +{
> +	return write_htab(i, ctx, 0);
> +}
> +
> +static int newwrite_htab(unsigned int i, struct update_ctx *ctx)
> +{
> +	return write_htab(i, ctx, BPF_NOEXIST);
> +}
> +
> +static int del_htab(unsigned int i, struct update_ctx *ctx)
> +{
> +	if (ctx->from >= MAX_ENTRIES)
> +		return 1;

delete?

> +
> +	bpf_map_delete_elem(&htab, &ctx->from);

and here.

> +	ctx->from += ctx->step;
> +
> +	return 0;
> +}
> +
> +SEC("?tp/syscalls/sys_enter_getpgid")
> +int overwrite(void *ctx)
> +{
> +	struct update_ctx update;
> +
> +	update.from = bpf_get_smp_processor_id();
> +	update.step = nr_thread;
> +	bpf_loop(64, overwrite_htab, &update, 0);
> +	__sync_fetch_and_add(&op_cnt, 1);
> +	return 0;
> +}
> +
> +SEC("?tp/syscalls/sys_enter_getpgid")
> +int batch_add_batch_del(void *ctx)
> +{
> +	struct update_ctx update;
> +
> +	update.from = bpf_get_smp_processor_id();
> +	update.step = nr_thread;
> +	bpf_loop(64, overwrite_htab, &update, 0);
> +
> +	update.from = bpf_get_smp_processor_id();
> +	bpf_loop(64, del_htab, &update, 0);
> +
> +	__sync_fetch_and_add(&op_cnt, 2);
> +	return 0;
> +}
> +
> +SEC("?tp/syscalls/sys_enter_getpgid")
> +int add_del_on_diff_cpu(void *ctx)
> +{
> +	struct update_ctx update;
> +	unsigned int from;
> +
> +	from = bpf_get_smp_processor_id();
> +	update.from = from / 2;

why extra 'from' variable? Just combine above two lines.

> +	update.step = nr_thread / 2;
> +
> +	if (from & 1)
> +		bpf_loop(64, newwrite_htab, &update, 0);
> +	else
> +		bpf_loop(64, del_htab, &update, 0);

I think it's cleaner to split this into two bpf programs.
Do update(NOEXIST) in one triggered by sys_enter_getpgid
and do delete_elem() in another triggered by a different syscall.

> +
> +	__sync_fetch_and_add(&op_cnt, 1);
> +	return 0;
> +}
> -- 
> 2.29.2
>