lkml.org 
[lkml]   [2022]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH bpf-next v1 0/5] bpf: rstat: cgroup hierarchical stats
    On Fri, Jun 3, 2022 at 9:22 AM Michal Koutný <mkoutny@suse.com> wrote:
    >
    > Hello Yosry et al.
    >
    > This is an interesting piece of work, I'll add some questions and
    > comments.
    >
    > On Fri, May 20, 2022 at 01:21:28AM +0000, Yosry Ahmed <yosryahmed@google.com> wrote:
    > > This patch series allows for using bpf to collect hierarchical cgroup
    > > stats efficiently by integrating with the rstat framework. The rstat
    > > framework provides an efficient way to collect cgroup stats and
    > > propagate them through the cgroup hierarchy.
    >
    > About the efficiency. Do you have any numbers or examples?
    > IIUC the idea is to utilize the cgroup's rstat subgraph of full tree
    > when flushing.
    > I was looking at your selftest example and the measuring hooks call
    > cgroup_rstat_updated() and they also allocate an entry bpf_map[cg_id].
    > The flush callback then looks up the cg_id for cgroups in the rstat
    > subgraph.
    > (I'm not familiar with bpf_map implementation or performance but I
    > imagine, you're potentially one step away from erasing bpf_map[cg_id] in
    > the flush callback.)
    > It seems to me that you're building a parallel structure (inside
    > bpf_map(s)) with similar purpose to the rstat subgraph.
    >
    > So I wonder whether there remains any benefit of coupling this with
    > rstat?

    Hi Michal,

    Thanks for taking a look at this!

    The bpf_map[cg_id] is not a similar structure to the rstat flush
    subgraph. This is where the stats are stored. These are long running
    numbers for (virtually) all cgroups on the system, they do not get
    allocated every time we call cgroup_rstat_updated(), only the first
    time. They are actually not erased at all in the whole selftest
    (except when the map is deleted at the end). In a production
    environment, we might have "setup" and "destroy" bpf programs that run
    when cgroups are created/destroyed, and allocate/delete these map
    entries then, to avoid the overhead in the first stat update/flush if
    necessary.

    The only reason I didn't do this in the demo selftest is because it
    was complex/long enough as-is, and for the purposes of showcasing and
    testing it seemed enough to allocate entries on demand on the first
    stat update. I can add a comment about this in the selftest if you
    think it's not obvious.

    In short, think of these bpf maps as equivalents to "struct
    memcg_vmstats" and "struct memcg_vmstats_percpu" in the memory
    controller. They are just containers to store the stats in, they do
    not have any subgraph structure and they have no use beyond storing
    percpu and total stats.

    I run small microbenchmarks that are not worth posting, they compared
    the latency of bpf stats collection vs. in-kernel code that adds stats
    to struct memcg_vmstats[_percpu] and flushes them accordingly, the
    difference was marginal. If the map lookups are deemed expensive and a
    bottleneck in the future, I have some ideas about improving that. We
    can rewrite the cgroup storage map to use the generic bpf local
    storage code, and have it be accessible from all programs by a cgroup
    key (like task_storage for e.g.) rather than only programs attached to
    that cgroup. However, this discussion is a tangent here.

    >
    >
    > Also, I'd expect the custom-processed data are useful in the
    > structured form (within bpf_maps) but then there's the cgroup iter thing
    > that takes available data and "flattens" them into text files.
    > I see this was discussed in subthreads already so it's not necessary to
    > return to it. IIUC you somehow intend to provide the custom info via the
    > text files. If that's true, I'd include that in the next cover message
    > (the purpose of the iterator).

    The main reason for this is to provide data in a similar fashion to
    cgroupfs, in text file per-cgroup. I will include this clearly in the
    next cover message. You can always not use the cgroup_iter and access
    the data directly from bpf maps.

    >
    >
    > > * The second patch adds cgroup_rstat_updated() and cgorup_rstat_flush()
    > > kfuncs, to allow bpf stat collectors and readers to communicate with rstat.
    >
    > kfunc means that it can be just called from any BPF program?
    > (I'm thinking of an unprivileged user who issues cgroup_rstat_updated()
    > deep down in the hierarchy repeatedly just to "spam" the rstat subgraph
    > (which slows down flushers above). Arguably, this can be done already
    > e.g. by causing certain MM events, so I'd like to just clarify if this
    > can be a new source of such arbitrary updates.)

    AFAIK loading bpf programs requires a privileged user, so someone has
    to approve such a program. Am I missing something?

    >
    > > * The third patch is actually v2 of a previously submitted patch [1]
    > > by Hao Luo. We agreed that it fits better as a part of this series. It
    > > introduces cgroup_iter programs that can dump stats for cgroups to
    > > userspace.
    > > v1 - > v2:
    > > - Getting the cgroup's reference at the time at attaching, instead of
    > > at the time when iterating. (Yonghong) (context [1])
    >
    > I noticed you take the reference to cgroup, that's fine.
    > But the demo program also accesses via RCU pointers
    > (memory_subsys_enabled():cgroup->subsys).
    > Again, my BPF ignorance here, does the iterator framework somehow take
    > care of RCU locks?

    bpf_iter_run_prog() is used to run bpf iterator programs, and it grabs
    rcu read lock before doing so. So AFAICT we are good on that front.

    Thanks a lot for this great discussion!

    >
    >
    > Thanks,
    > Michal

    \
     
     \ /
      Last update: 2022-06-03 21:48    [W:4.950 / U:0.272 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site