On 2020-06-22T09:20:03 -0700, Andrii Nakryiko wrote:
> On Mon, Jun 22, 2020 at 8:01 AM Matt Pallissard <matt@xxxxxxxxxxxxxx> wrote:
> > On 2020-06-21T08:44:28 -0700, Matt Pallissard wrote:
> > > On 2020-06-20T20:29:43 -0700, Andrii Nakryiko wrote:
> > > > On Sat, Jun 20, 2020 at 1:07 PM Matt Pallissard <matt@xxxxxxxxxxxxxx> wrote:
> > > > > On 2020-06-20T11:11:55 -0700, Yonghong Song wrote:
> > > > > > On 6/20/20 9:22 AM, Matt Pallissard wrote:
> > > > > > > New to bpf here.
> > > > > > >
> > > > > > > I'm trying to read values out of of mm_struct. I have code like this;
> > > > > > >
> > > > > > > unsigned long i[10] = {};
> > > > > > > struct task_struct *t;
> > > > > > > struct mm_rss_stat *rss;
> > > > > > >
> > > > > > > t = (struct task_struct *)bpf_get_current_task();
> > > > > > > BPF_CORE_READ_INTO(&rss, t, mm, rss_stat);
> > > > > > > BPF_CORE_READ_INTO(i, rss, count);
> > > > > > >
> > > > > > > However, all values in `i` appear to be 0 (i[MM_FILEPAGES], etc), as if no data gets copied. I'm about 100% confident that this is caused by a glaring oversight on my part.
> > > > > >
> > > > > > Maybe you want to check the return value of BPF_CORE_READ_INTO.
> > > > > > Underlying it is using bpf_probe_read and bpf_probe_read may fail e.g., due
> > > > > > to major fault.
> > > > >
> > > > > Doh, I should have known to check the return codes! Yes, it was failing. I knew I was overlooking something trivial.
> > > > >
> > > >
> > > > I wrote exactly such piece of code a while ago. Here's part of it for
> > > > reference, I think it will be helpful:
> > > >
> > > > struct task_struct *task = (struct task_struct *)bpf_get_current_task();
> > > > const struct mm_struct *mm = BPF_CORE_READ(task, mm);
> > > >
> > > > if (mm) {
> > > > u64 hiwater_rss = BPF_CORE_READ(mm, hiwater_rss);
> > > > u64 file_pages = BPF_CORE_READ(mm, rss_stat.count[MM_FILEPAGES].counter);
> > > > u64 anon_pages = BPF_CORE_READ(mm, rss_stat.count[MM_ANONPAGES].counter);
> > > > u64 shmem_pages = BPF_CORE_READ(mm,
> > > > rss_stat.count[MM_SHMEMPAGES].counter);
> > > > u64 active_rss = file_pages + anon_pages + shmem_pages;
> > > > /* ... */
> > >
> > > Thank you,
> > >
> > > After realizing that I was referencing the struct incorrectly, I wound up with a similar block of code. However, as I started testing it against /proc/pid/smaps[,_rollup] I noticed that my numbers didn't match up. Always smaller.
> > >
> > > I took a quick glance at fs/proc/task_mmu.c. I think I'll have to walk some sort of accounting structure.
> >
> >
> > I started to take a hard look at fs/proc/task_mmu.c. With all the locking, globals, and compile-time constants, I'm not sure that it's even possible to correctly walk `vm_area_struct` in bpf.
>
> Yes, you can't take all those locks from BPF. But reading atomic
> counters from BPF should be no problem. You might get a slightly out
> of sync readings, but whatever you are doing shouldn't expect to have
> 100% correct values anyways, because they might change so fast after
> you read them.
That was my initial thought. I didn't care to much about stale data, my only real concern was walking vm_area_struct and having memory freed. I wasn't sure if that could break the list underneath me. Although, that shouldn't be too difficult to get to the bottom of.
> > If anyone has suggestions for getting memory numbers from an entire process, not just a task/thread, I'd love to hear them. If not, I'll pursue this on my own.
>
> For this, you'd need to iterate across many tasks and aggregate their
> results based on tasks's tgid. Check iter/task programs in selftests
> (progs/bpf_iter_task.c, I think).
Sounds like a great starting point. Thanks again.
Matt Pallissard
