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. Or do you mean this case can never happen with your approach? > All possible allocators will be added to map->used_allocators > at prog load time and allocator's refcnt incremented. > At run-time bpf_kptr_xchg(map_value, ptr) will be happening > with an allocator A which was added to that map->used_allocators > already.
