On Tue, Jul 12, 2022 at 3:40 PM Michal Hocko [off-list ref] wrote:

On Mon 11-07-22 21:39:14, Alexei Starovoitov wrote:

quoted

On Mon, Jul 11, 2022 at 02:15:07PM +0200, Michal Hocko wrote:

quoted

On Sun 10-07-22 07:32:13, Shakeel Butt wrote:

quoted

On Sat, Jul 09, 2022 at 10:26:23PM -0700, Alexei Starovoitov wrote:

quoted

On Fri, Jul 8, 2022 at 2:55 PM Shakeel Butt [off-list ref] wrote:

[...]

quoted

quoted

Most probably Michal's comment was on free objects sitting in the caches
(also pointed out by Yosry). Should we drain them on memory pressure /
OOM or should we ignore them as the amount of memory is not significant?

Are you suggesting to design a shrinker for 0.01% of the memory
consumed by bpf?

No, just claim that the memory sitting on such caches is insignificant.

yes, that is not really clear from the patch description. Earlier you
have said that the memory consumed might go into GBs. If that is a
memory that is actively used and not really reclaimable then bad luck.
There are other users like that in the kernel and this is not a new
problem. I think it would really help to add a counter to describe both
the overall memory claimed by the bpf allocator and actively used
portion of it. If you use our standard vmstat infrastructure then we can
easily show that information in the OOM report.

OOM report can potentially be extended with info about bpf consumed
memory, but it's not clear whether it will help OOM analysis.

If GBs of memory can be sitting there then it is surely an interesting
information to have when seeing OOM. One of the big shortcomings of the
OOM analysis is unaccounted memory.

quoted

bpftool map show
prints all map data already.
Some devs use bpf to inspect bpf maps for finer details in run-time.
drgn scripts pull that data from crash dumps.
There is no need for new counters.
The idea of bpf specific counters/limits was rejected by memcg folks.

I would argue that integration into vmstat is useful not only for oom
analysis but also for regular health check scripts watching /proc/vmstat
content. I do not think most of those generic tools are BPF aware. So
unless there is a good reason to not account this memory there then I
would vote for adding them. They are cheap and easy to integrate.

quoted

quoted

OK, thanks for the clarification. There is still one thing that is not
really clear to me. Without a proper ownership bound to any process why
is it desired/helpful to account the memory to a memcg?

The first step is to have a limit. memcg provides it.

I am sorry but this doesn't really explain it. Could you elaborate
please? Is the limit supposed to protect against adversaries? Or is it
just to prevent from accidental runaways? Is it purely for accounting
purposes?

quoted

quoted

We have discussed something similar in a different email thread and I
still didn't manage to find time to put all the parts together. But if
the initiator (or however you call the process which loads the program)
exits then this might be the last process in the specific cgroup and so
it can be offlined and mostly invisible to an admin.

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.

quoted

quoted

As you have explained there is nothing really actionable on this memory
by the OOM killer either. So does it actually buy us much to account?

It will be actionable. One step at a time.
In the other thread we've discussed an idea to make memcg selectable when
bpf objects are created. The user might create a special memcg and use it for
all things bpf. This might be the way to provide bpf specific accounting
and limits.

Do you have a reference for those discussions?

I think it is https://lore.kernel.org/bpf/CALOAHbCM=ZxwutQOPmJx2LKY3Pd_hs+8v8r4-ybwPbBNBuNjXA@mail.gmail.com/ (local)
.
Introducing independent memcg to manage pinned bpf programs and maps
and forbid the user to destroy them if bpf programs are not unpinned,
that's the best workaround so sar, per my analysis.

-- 
Regards
Yafang