Re: [PATCH bpf-next 0/5] bpf: BPF specific memory allocator.

[Yafang Shao <laoar.shao@gmail.com>]

On Wed, Jul 13, 2022 at 10:39 AM Roman Gushchin
<roman.gushchin@linux.dev> wrote:
>
> On Tue, Jul 12, 2022 at 11:52:11AM +0200, Michal Hocko wrote:
> > On Tue 12-07-22 16:39:48, Yafang Shao wrote:
> > > On Tue, Jul 12, 2022 at 3:40 PM Michal Hocko <mhocko@suse.com> wrote:
> > [...]
> > > > > Roman already sent reparenting fix:
> > > > > https://patchwork.kernel.org/project/netdevbpf/patch/20220711162827.184743-1-roman.gushchin@linux.dev/
> > > >
> > > > Reparenting is nice but not a silver bullet. Consider a shallow
> > > > hierarchy where the charging happens in the first level under the root
> > > > memcg. Reparenting to the root is just pushing everything under the
> > > > system resources category.
> > > >
> > >
> > > Agreed. That's why I don't like reparenting.
> > > Reparenting just reparent the charged pages and then redirect the new
> > > charge, but can't reparents the 'limit' of the original memcg.
> > > So it is a risk if the original memcg is still being charged. We have
> > > to forbid the destruction of the original memcg.
>
> I agree, I also don't like reparenting for !kmem case. For kmem (and *maybe*
> bpf maps is an exception), I don't think there is a better choice.
>
> > yes, I was toying with an idea like that. I guess we really want a
> > measure to keep cgroups around if they are bound to a resource which is
> > sticky itself. I am not sure how many other resources like BPF (aka
> > module like) we already do charge for memcg but considering the
> > potential memory consumption just reparenting will not help in general
> > case I am afraid.
>
> Well, then we have to make these objects a first-class citizens in cgroup API,
> like processes. E.g. introduce cgroup.bpf.maps, cgroup.mounts.tmpfs etc.
> I easily can see some value here, but it's a big API change.
>
> With the current approach when a bpf map pins a memory cgroup of the creator
> process (which I think is completely transparent for most bpf users), I don't
> think preventing the deletion of a such cgroup is possible. It will break too
> many things.
>
> But honestly I don't see why userspace can't handle it. If there is a cgroup which
> contains shared bpf maps, why would it delete it? It's a weird use case, I don't
> think we have to optimize for it. Also, we do a ton of optimizations for live
> cgroups (e.g. css refcounting being percpu) which are not working for a deleted
> cgroup. So noone really should expect any properties from dying cgroups.
>

I think we have discussed why the user can't handle it easily.
Actually It's NOT a weird use case if you are a k8s user.  (Of course
it may seem weird to the systemd user, but unfortunately systemd
doesn't rule the whole world. )
I have told you that it is not reasonable to refuse a containerized
process to pin bpf programs, but if you are not familiar with k8s, it
is not easy to explain clearly why it is a trouble for deployment.
But I can try to explain to you from a *systemd user's* perspective.

                   bpf-memcg                       (must be persistent)
                  /                \
  bpf-foo-memcg       bpf-bar-memcg   (must be persistent, and limit here)
-------------------------------------------------------
           /                              \
    bpf-foo pod              bpf-bar pod    (being created and
destroyed, but not limited)

I assume the above hierarchy is what you expect.
But you know, in the k8s environment, everything is pod-based, that
means if we use the above hierarchy in the k8s environment, the k8s's
limiting, monitoring, debugging must be changed consequently.  That
means it may be a fullstack change in k8s, a great refactor.

So below hierarchy is a reasonable solution,
                                          bpf-memcg
                                                |
  bpf-foo pod                    bpf-foo-memcg     (limited)
       /          \                                /
(charge)     (not-charged)      (charged)
proc-foo                     bpf-foo

And then keep the bpf-memgs persistent.

-- 
Regards
Yafang