Hi,
On 11/27/2023 11:20 AM, Alexei Starovoitov wrote:
> On Sun, Nov 26, 2023 at 6:47 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>>
>> Before posting v4, do the following code-snippets match your suggestions ?
>>
>> resolve_pseudo_ldimm64()
>> if (env->prog->aux->sleepable)
>> /* The max number of program is INT_MAX -
>> 1, so atomic_t will be enough */
>> atomic_inc(&map->sleepable_refcnt);
>>
>> bpf_map_fd_put_ptr()
>> if (deferred) {
>> if (atomic_read(&map->sleepable_refcnt))
>> WRITE_ONCE(map->free_after_tt_rcu_gp, true);
>> else
>> WRITE_ONCE(map->free_after_rcu_gp, true);
>> }
> Yes. That was an idea and I hope you see that in this form it's
> much easier to understand, right?
Yes.
>
> and regarding your other question:
>
>> Could we replace sleepable_refcnt by
>> a bool (e.g, access_in_sleepable), so the check can be simplified further ?
> I think you're trying to optimize too soon.
> How would that bool access_in_sleepable work?
> Something needs to set it and the last sleepable to unset it.
> You might need a refcnt to do that.
Yes, a premature optimization. I only consider the case that one bpf map
will be only accessed by one bpf program and bpf program will always
exit after the inner map is deleted from outer map (so sleepable_refcnt
is still greater than zero when calling bpf_map_fd_put_ptr()). Will drop
the idea.
>
>> bpf_map_put()
>> if (READ_ONCE(map->free_after_tt_rcu_gp))
>> call_rcu_tasks_trace(&map->rcu,
>> bpf_map_free_mult_rcu_gp);
>> else if (READ_ONCE(map->free_after_rcu_gp))
>> call_rcu(&map->rcu, bpf_map_free_rcu_gp);
>> else
>> bpf_map_free_in_work(map);
>>
>> And are you OK with using call_rcu()/call_rcu_tasks_trace() instead of
>> synchronize_rcu()/synchronize_rcu_mult() ?
> Of course. Not sure what you meant by that.
> bpf_map_put() cannot call sync_rcu.
> Are you asking about doing queue_work first and then sync_rcu* inside?
> I think it will not scale compared to call_rcu approach.
> .
Yes, I mean the deferment implementation in v2 which calls sync_rcu()*
in kworker. Will still use call_rcu()* in v4. Thanks for all these
suggestions.
