On Mon, Oct 24, 2022 at 6:36 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 10/24/22 5:48 PM, Yosry Ahmed wrote:
> > On Mon, Oct 24, 2022 at 5:32 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> >>
> >> On 10/24/22 5:21 PM, Yosry Ahmed wrote:
> >>> On Mon, Oct 24, 2022 at 2:15 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> >>>>
> >>>> On 10/23/22 11:05 AM, Yonghong Song wrote:
> >>>>> +void bpf_cgrp_storage_free(struct cgroup *cgroup)
> >>>>> +{
> >>>>> + struct bpf_local_storage *local_storage;
> >>>>> + struct bpf_local_storage_elem *selem;
> >>>>> + bool free_cgroup_storage = false;
> >>>>> + struct hlist_node *n;
> >>>>> + unsigned long flags;
> >>>>> +
> >>>>> + rcu_read_lock();
> >>>>> + local_storage = rcu_dereference(cgroup->bpf_cgrp_storage);
> >>>>> + if (!local_storage) {
> >>>>> + rcu_read_unlock();
> >>>>> + return;
> >>>>> + }
> >>>>> +
> >>>>> + /* Neither the bpf_prog nor the bpf_map's syscall
> >>>>> + * could be modifying the local_storage->list now.
> >>>>> + * Thus, no elem can be added to or deleted from the
> >>>>> + * local_storage->list by the bpf_prog or by the bpf_map's syscall.
> >>>>> + *
> >>>>> + * It is racing with __bpf_local_storage_map_free() alone
> >>>>> + * when unlinking elem from the local_storage->list and
> >>>>> + * the map's bucket->list.
> >>>>> + */
> >>>>> + bpf_cgrp_storage_lock();
> >>>>> + raw_spin_lock_irqsave(&local_storage->lock, flags);
> >>>>> + hlist_for_each_entry_safe(selem, n, &local_storage->list, snode) {
> >>>>> + bpf_selem_unlink_map(selem);
> >>>>> + /* If local_storage list has only one element, the
> >>>>> + * bpf_selem_unlink_storage_nolock() will return true.
> >>>>> + * Otherwise, it will return false. The current loop iteration
> >>>>> + * intends to remove all local storage. So the last iteration
> >>>>> + * of the loop will set the free_cgroup_storage to true.
> >>>>> + */
> >>>>> + free_cgroup_storage =
> >>>>> + bpf_selem_unlink_storage_nolock(local_storage, selem, false, false);
> >>>>> + }
> >>>>> + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> >>>>> + bpf_cgrp_storage_unlock();
> >>>>> + rcu_read_unlock();
> >>>>> +
> >>>>> + if (free_cgroup_storage)
> >>>>> + kfree_rcu(local_storage, rcu);
> >>>>> +}
> >>>>
> >>>> [ ... ]
> >>>>
> >>>>> +/* *gfp_flags* is a hidden argument provided by the verifier */
> >>>>> +BPF_CALL_5(bpf_cgrp_storage_get, struct bpf_map *, map, struct cgroup *, cgroup,
> >>>>> + void *, value, u64, flags, gfp_t, gfp_flags)
> >>>>> +{
> >>>>> + struct bpf_local_storage_data *sdata;
> >>>>> +
> >>>>> + WARN_ON_ONCE(!bpf_rcu_lock_held());
> >>>>> + if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
> >>>>> + return (unsigned long)NULL;
> >>>>> +
> >>>>> + if (!cgroup)
> >>>>> + return (unsigned long)NULL;
> >>>>> +
> >>>>> + if (!bpf_cgrp_storage_trylock())
> >>>>> + return (unsigned long)NULL;
> >>>>> +
> >>>>> + sdata = cgroup_storage_lookup(cgroup, map, true);
> >>>>> + if (sdata)
> >>>>> + goto unlock;
> >>>>> +
> >>>>> + /* only allocate new storage, when the cgroup is refcounted */
> >>>>> + if (!percpu_ref_is_dying(&cgroup->self.refcnt) &&
> >>>>> + (flags & BPF_LOCAL_STORAGE_GET_F_CREATE))
> >>>>> + sdata = bpf_local_storage_update(cgroup, (struct bpf_local_storage_map *)map,
> >>>>> + value, BPF_NOEXIST, gfp_flags);
> >>>>> +
> >>>>> +unlock:
> >>>>> + bpf_cgrp_storage_unlock();
> >>>>> + return IS_ERR_OR_NULL(sdata) ? (unsigned long)NULL : (unsigned long)sdata->data;
> >>>>> +}
> >>>>
> >>>> [ ... ]
> >>>>
> >>>>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> >>>>> index 764bdd5fd8d1..32145d066a09 100644
> >>>>> --- a/kernel/cgroup/cgroup.c
> >>>>> +++ b/kernel/cgroup/cgroup.c
> >>>>> @@ -5227,6 +5227,10 @@ static void css_free_rwork_fn(struct work_struct *work)
> >>>>> struct cgroup_subsys *ss = css->ss;
> >>>>> struct cgroup *cgrp = css->cgroup;
> >>>>>
> >>>>> +#ifdef CONFIG_BPF_SYSCALL
> >>>>> + bpf_cgrp_storage_free(cgrp);
> >>>>> +#endif
> >>>>
> >>>>
> >>>> After revisiting comment 4bfc0bb2c60e, some of the commit message came to my mind:
> >>>>
> >>>> " ...... it blocks a possibility to implement
> >>>> the memcg-based memory accounting for bpf objects, because a circular
> >>>> reference dependency will occur. Charged memory pages are pinning the
> >>>> corresponding memory cgroup, and if the memory cgroup is pinning
> >>>> the attached bpf program, nothing will be ever released."
> >>>>
> >>>> Considering the bpf_map_kzalloc() is used in bpf_local_storage_map.c and it can
> >>>> charge the memcg, I wonder if the cgrp_local_storage will have similar refcnt
> >>>> loop issue here.
> >>>>
> >>>> If here is the right place to free the cgrp_local_storage() and enough to break
> >>>> this refcnt loop, it will be useful to add some explanation and its
> >>>> consideration in the commit message.
> >>>>
> >>>
> >>> I think a similar refcount loop issue can happen here as well. IIUC,
> >>> this function will only be run when the css is released after all
> >>> references are dropped. So if memcg charging is enabled the cgroup
> >>> will never be removed. This one might be trickier to handle though..
> >>
> >> How about removing all storage from cgrp->bpf_cgrp_storage in
> >> cgroup_destroy_locked()?
> >
> > The problem here is that you lose information for cgroups that went
> > offline but still exist in the kernel (i.e offline cgroups). The
> > commit log 4bfc0bb2c60e mentions that such cgroups can have live
> > sockets attached, so this might be a problem?
>
> Keeping the cgrp_storage around is useful for the cgroup-bpf prog that will be
> called upon some sk events (eg ingress/egress). The cgrp_storage cleanup could
> be done in cgroup_bpf_release_fn() also such that it will wait till all sk is done.
>
> > From a memory perspective, offline memcgs can still undergo memory operations like
> > reclaim. If we are using BPF to collect cgroup statistics for memory
> > reclaim, we can't do so for offline memcgs, which is not the end of
> > the world, but the cgroup storages become slightly less powerful. We
> > might also lose some data that we have already stored for such offline
> > memcgs. Also BPF programs now need to handle the case where they have
> > a valid cgroup pointer but they cannot retrieve a cgroup storage for
> > it because it went offline.
>
> iiuc, the use case is to be able to use the cgrp_storage at some earlier stage
> of the cgroup destruction.
Yes, exactly. The cgroup gets "offlined" when the user removes the
directory, but is not actually freed until all references are dropped.
An offline memcg can still undergo some operations.
> A noob question, I wonder if there is a cgroup that
> it will never go away, the root cgrp? Then the cgrp_storage cleanup could be
> more selective and avoid cleaning up the cgrp storage charged to the root cgroup.
Yes, root cgrp doesn't go away, but I am not sure I understand how
this fixes the problem.
In all cases, Roman confirmed my theory. BPF maps charges are now
charged through objcg, not directly to memcgs, which means that when
the cgroup is offline, those charges are moved to the parent. IIUC,
this means there should not be a refcount loop. When the cgroup
directory is removed and the cgroup is offlined, the memory charges of
the cgrp_storage will move to the parent. The cgrp_storage will remain
accessible until all refs to it are dropped and it is actually freed
by css_free_rwork_fn(), at that point the cgrp_storage will be freed.
I just realized, I *think* the call to bpf_cgrp_storage_free(cgrp) is
actually misplaced within css_free_rwork_fn(). Reading the code, it
seems like css_free_rwork_fn() can be called in two cases:
(a) We are freeing a css (cgroup_subsys_state), but not the cgroup
itself (e.g. we are disabling a subsystem controller on a cgroup).
(b) We are freeing the cgroup itself.
I think we want to free the cgrp_storage only in (b), which would
correspond to the else statement (but before the nested if).
Basically, something like this *I think*:
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 2319946715e0..f1e6058089f5 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5349,6 +5349,7 @@ static void css_free_rwork_fn(struct work_struct *work)
atomic_dec(&cgrp->root->nr_cgrps);
cgroup1_pidlist_destroy_all(cgrp);
cancel_work_sync(&cgrp->release_agent_work);
+ bpf_cgrp_storage_free(cgrp);
if (cgroup_parent(cgrp)) {
/*
Tejun would know better, so please correct me if I am wrong.
(FWIW I think it would be nicer if we have an empty stub for
bpf_cgrp_storage_free() when !CONFIG_BPF_SYSCALL instead of the
#ifdef, similar to cgroup_bpf_offline()).
>
> > We ideally want to be able to charge the memory to the cgroup without
> > holding a ref to it, which is against the cgroup memory charging
> > model.
>
