On Fri, Dec 24, 2021 at 03:29:15PM +0000, KP Singh wrote:
> Other maps like hashmaps are already available to sleepable programs.
> Sleepable BPF programs run under trace RCU. Allow task, sk and inode
> storage to be used from sleepable programs. This allows sleepable and
> non-sleepable programs to provide shareable annotations on kernel
> objects.
>
> Sleepable programs run in trace RCU where as non-sleepable programs run
> in a normal RCU critical section i.e. __bpf_prog_enter{_sleepable}
> and __bpf_prog_exit{_sleepable}) (rcu_read_lock or rcu_read_lock_trace).
>
> In order to make the local storage maps accessible to both sleepable
> and non-sleepable programs, one needs to call both
> call_rcu_tasks_trace and call_rcu to wait for both trace and classical
> RCU grace periods to expire before freeing memory.
>
> Paul's work on call_rcu_tasks_trace allows us to have per CPU queueing
> for call_rcu_tasks_trace. This behaviour can be achieved by setting
> rcupdate.rcu_task_enqueue_lim=<num_cpus> boot parameter.
>
> In light of these new performance changes and to keep the local storage
> code simple, avoid adding a new flag for sleepable maps / local storage
> to select the RCU synchronization (trace / classical).
>
> Also, update the dereferencing of the pointers to use
> rcu_derference_check (with either the trace or normal RCU locks held)
> with a common bpf_rcu_lock_held helper method.
>
> Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
Thanks for the patches. One minor comment which is not very related to
this set.
We can land this set first and then use a follow up.
Acked-by: Martin KaFai Lau <kafai@xxxxxx>
> @@ -306,7 +328,8 @@ int bpf_local_storage_alloc(void *owner,
> * bucket->list, first_selem can be freed immediately
> * (instead of kfree_rcu) because
> * bpf_local_storage_map_free() does a
> - * synchronize_rcu() before walking the bucket->list.
> + * synchronize_rcu_mult (waiting for both sleepable and
> + * normal programs) before walking the bucket->list.
> * Hence, no one is accessing selem from the
> * bucket->list under rcu_read_lock().
> */
This whole comment section is outdated and can be removed because
the bpf_free_used_maps(bpf_prog->aux) is now only called after
call_rcu_tasks_trace() or call_rcu().
Meaning bpf_local_storage_map_free() cannot be running in parallel
with bpf_local_storage_alloc() because the running bpf prog holds a
refcnt of the smap here.
