On Mon, Aug 29, 2022 at 5:45 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > On Tue, 30 Aug 2022 at 02:26, Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Mon, Aug 29, 2022 at 5:20 PM Kumar Kartikeya Dwivedi > > <memxor@xxxxxxxxx> wrote: > > > > > > On Tue, 30 Aug 2022 at 01:45, Alexei Starovoitov > > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > > > On Mon, Aug 29, 2022 at 4:18 PM Delyan Kratunov <delyank@xxxxxx> wrote: > > > > > > > > > > > > > > > > > It is not very precise, but until those maps are gone it delays > > > > > > release of the allocator (we can empty all percpu caches to save > > > > > > memory once bpf_map pinning the allocator is gone, because allocations > > > > > > are not going to be served). But it allows unit_free to be relatively > > > > > > less costly as long as those 'candidate' maps are around. > > > > > > > > > > Yes, we considered this but it's much easier to get to pathological behaviors, by > > > > > just loading and unloading programs that can access an allocator in a loop. The > > > > > freelists being empty help but it's still quite easy to hold a lot of memory for > > > > > nothing. > > > > > > > > > > The pointer walk was proposed to prune most such pathological cases while still being > > > > > conservative enough to be easy to implement. Only races with the pointer walk can > > > > > extend the lifetime unnecessarily. > > > > > > > > I'm getting lost in this thread. > > > > > > > > Here is my understanding so far: > > > > We don't free kernel kptrs from map in release_uref, > > > > but we should for local kptrs, since such objs are > > > > not much different from timers. > > > > So release_uref will xchg all such kptrs and free them > > > > into the allocator without touching allocator's refcnt. > > > > So there is no concurrency issue that Kumar was concerned about. > > > > > > Haven't really thought through whether this will fix the concurrent > > > kptr swap problem, but then with this I think you need: > > > - New helper bpf_local_kptr_xchg(map, map_value, kptr) > > > > no. why? > > current bpf_kptr_xchg(void *map_value, void *ptr) should work. > > The verifier knows map ptr from map_value. > > > > > - Associating map_uid of map, map_value > > > - Always doing atomic_inc_not_zero(map->usercnt) for each call to > > > local_kptr_xchg > > > 1 and 2 because of inner_maps, 3 because of release_uref. > > > But maybe not a deal breaker? > > > > No run-time refcnts. > > How is future kptr_xchg prevented for the map after its usercnt drops to 0? > If we don't check it at runtime we can xchg in non-NULL kptr after > release_uref callback. > For timer you are taking timer spinlock and reading map->usercnt in > timer_set_callback. Sorry I confused myself and others with release_uref. I meant map_poke_untrack-like call. When we drop refs from used maps in __bpf_free_used_maps we walk all elements. Similar idea here. When prog is unloaded it cleans up all objects it allocated and stored into maps before dropping refcnt-s in prog->used_allocators.
