Hi,
On 1/7/2025 6:24 AM, Alexei Starovoitov wrote:
> On Mon, Jan 6, 2025 at 12:07 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>
>> Disabling migration before calling ops->map_free() to simplify the
>> freeing of map values or special fields allocated from bpf memory
>> allocator.
>>
>> After disabling migration in bpf_map_free(), there is no need for
>> additional migration_{disable|enable} pairs in the ->map_free()
>> callbacks. Remove these redundant invocations.
>>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>> ---
>> kernel/bpf/arraymap.c | 2 --
>> kernel/bpf/bpf_local_storage.c | 2 --
>> kernel/bpf/hashtab.c | 2 --
>> kernel/bpf/range_tree.c | 2 --
>> kernel/bpf/syscall.c | 8 +++++++-
>> 5 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index 451737493b17..eb28c0f219ee 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -455,7 +455,6 @@ static void array_map_free(struct bpf_map *map)
>> struct bpf_array *array = container_of(map, struct bpf_array, map);
>> int i;
>>
>> - migrate_disable();
>> if (!IS_ERR_OR_NULL(map->record)) {
>> if (array->map.map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
>> for (i = 0; i < array->map.max_entries; i++) {
>> @@ -472,7 +471,6 @@ static void array_map_free(struct bpf_map *map)
>> bpf_obj_free_fields(map->record, array_map_elem_ptr(array, i));
>> }
>> }
>> - migrate_enable();
>>
>> if (array->map.map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
>> bpf_array_free_percpu(array);
>> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
>> index b649cf736438..12cf6382175e 100644
>> --- a/kernel/bpf/bpf_local_storage.c
>> +++ b/kernel/bpf/bpf_local_storage.c
>> @@ -905,13 +905,11 @@ void bpf_local_storage_map_free(struct bpf_map *map,
>> while ((selem = hlist_entry_safe(
>> rcu_dereference_raw(hlist_first_rcu(&b->list)),
>> struct bpf_local_storage_elem, map_node))) {
>> - migrate_disable();
>> if (busy_counter)
>> this_cpu_inc(*busy_counter);
>> bpf_selem_unlink(selem, true);
>> if (busy_counter)
>> this_cpu_dec(*busy_counter);
>> - migrate_enable();
>> cond_resched_rcu();
>> }
>> rcu_read_unlock();
>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>> index 8bf1ad326e02..6051f8a39fec 100644
>> --- a/kernel/bpf/hashtab.c
>> +++ b/kernel/bpf/hashtab.c
>> @@ -1570,14 +1570,12 @@ static void htab_map_free(struct bpf_map *map)
>> * underneath and is responsible for waiting for callbacks to finish
>> * during bpf_mem_alloc_destroy().
>> */
>> - migrate_disable();
>> if (!htab_is_prealloc(htab)) {
>> delete_all_elements(htab);
>> } else {
>> htab_free_prealloced_fields(htab);
>> prealloc_destroy(htab);
>> }
>> - migrate_enable();
>>
>> bpf_map_free_elem_count(map);
>> free_percpu(htab->extra_elems);
>> diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
>> index 5bdf9aadca3a..37b80a23ae1a 100644
>> --- a/kernel/bpf/range_tree.c
>> +++ b/kernel/bpf/range_tree.c
>> @@ -259,9 +259,7 @@ void range_tree_destroy(struct range_tree *rt)
>>
>> while ((rn = range_it_iter_first(rt, 0, -1U))) {
>> range_it_remove(rn, rt);
>> - migrate_disable();
>> bpf_mem_free(&bpf_global_ma, rn);
>> - migrate_enable();
>> }
>> }
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 0503ce1916b6..e7a41abe4809 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -835,8 +835,14 @@ static void bpf_map_free(struct bpf_map *map)
>> struct btf_record *rec = map->record;
>> struct btf *btf = map->btf;
>>
>> - /* implementation dependent freeing */
>> + /* implementation dependent freeing. Disabling migration to simplify
>> + * the free of values or special fields allocated from bpf memory
>> + * allocator.
>> + */
>> + migrate_disable();
>> map->ops->map_free(map);
>> + migrate_enable();
>> +
> I was about to comment on patches 10-13 that it's
> better to do it in bpf_map_free(), but then I got to this patch.
> All makes sense, but the patch breakdown is too fine grain.
> Patches 10-13 introduce migrate pairs only to be deleted
> in patch 15. Please squash them into one patch.
OK. However I need to argue for the fine grained break down. The
original though is that if disabling migration for ->map_free callback
for all maps introduces some problems, we could revert the patch #15
separately instead of reverting the squashed patch and moving the
migrate_{disable|enable}() pair to maps which are OK with that change
again. What do you think ?
>
> Also you mention in the cover letter:
>
>> Considering the bpf-next CI is broken
> What is this about?
Er, I said it wrong. It is my local bpf-next setup. A few days ago, when
I tried to verify the patch by using bpf_next/for-next treee, the
running of test_maps and test_progs failed. Will check today that
whether it is OK.
>
> The cant_migrate() additions throughout looks
> a bit out of place. All that code doesn't care about migrations.
> Only bpf_ma code does. Let's add it there instead?
> The stack trace will tell us the caller anyway,
> so no information lost.
OK. However bpf_ma is not the only one which needs disabled migration.
The reason that bpf_ma needs migrate_disable() is the use of
this_cpu_ptr(). However, there are many places in bpf which use
this_cpu_ptr() (e.g., bpf_for_each_array_elem) and this_cpu_{in|del}
pair (e.g., bpf_cgrp_storage_lock). I will check the cant_migrate which
can be removed in v2.
>
> Overall it looks great.
Thanks for these suggestions.
>
> pw-bot: cr
> .
