Hi Alexei,
On 6/20/2023 4:35 AM, Alexei Starovoitov wrote:
> 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?
OK.
>
>> +{
>> + while (true) {
>> + (void)syscall(__NR_getpgid);
>> + /* Notify for start */
> the comment is confusing too.
> Maybe /* Notify map_deleter that map_updates are done */ ?
Will update.
>
>> + pthread_barrier_wait(notify);
>> + /* Wait for completion */
> and /* Wait for deletions to complete */ ?
Yes. Will update.
>
>> + 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.
Will update.
>
>> + 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?
MAX_ENTRIES / 64 = 128, so when then number of producers is greater than
128, it will be hit. But I think we can remove it and adjust max_entries
accordingly when the number of producers is greater than 128.
>
>> +
>> + 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...
If these threads update or delete the non-overlapped part of the hash
map, then there will be no fail. But when the number of producer is
greater than the number of online CPU, there will be failure, but this
use case is not expected benchmark use case.
>
>> + 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?
Will fix.
>
>> +
>> + 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.
from is also used below to decide the bpf program should do update or
deletion.
>
>> + 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.
OK. Will do that in next version.
>
>> +
>> + __sync_fetch_and_add(&op_cnt, 1);
>> + return 0;
>> +}
>> --
>> 2.29.2
>>
