Hi
On 11/27/2023 10:47 AM, Hou Tao wrote:
> Hi Alexei,
>
> On 11/27/2023 9:49 AM, Alexei Starovoitov wrote:
>> On Fri, Nov 24, 2023 at 3:29 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>>
>>> When removing the inner map from the outer map, the inner map will be
>>> freed after one RCU grace period and one RCU tasks trace grace
>>> period, so it is certain that the bpf program, which may access the
>>> inner map, has exited before the inner map is freed.
>>>
>>> However there is unnecessary to wait for any RCU grace period, one RCU
>>> grace period or one RCU tasks trace grace period if the outer map is
>>> only accessed by userspace, sleepable program or non-sleepable program.
>>> So recording the sleepable attributes of the owned bpf programs when
>>> adding the outer map into env->used_maps, copying the recorded
>>> attributes to inner map atomically when removing inner map from the
>>> outer map and using the recorded attributes in the inner map to decide
>>> which, and how many, RCU grace periods are needed when freeing the
>>> inner map.
>>>
>>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
SNIP
>> I still think sleepable_refcnt and refcnt is more than enough to
>> optimize patch 3.
> 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);
> }
>
> 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);
>
Just find out the above code-snippet misses the corresponding
atomic_dec() for sleepable_refcnt. Could we replace sleepable_refcnt by
a bool (e.g, access_in_sleepable), so the check can be simplified further ?
> And are you OK with using call_rcu()/call_rcu_tasks_trace() instead of
> synchronize_rcu()/synchronize_rcu_mult() ?
>
> .
