On Thu, May 04, 2023 at 11:07:22AM +0200, Michal Hocko wrote: > No. I am mostly concerned about the _maintenance_ overhead. For the > bare tracking (without profiling and thus stack traces) only those > allocations that are directly inlined into the consumer are really > of any use. That increases the code impact of the tracing because any > relevant allocation location has to go through the micro surgery. > > e.g. is it really interesting to know that there is a likely memory > leak in seq_file proper doing and allocation? No as it is the specific > implementation using seq_file that is leaking most likely. There are > other examples like that See? So this is a rather strange usage of "maintenance overhead" :) But it's something we thought of. If we had to plumb around a _RET_IP_ parameter, or a codetag pointer, it would be a hassle annotating the correct callsite. Instead, alloc_hooks() wraps a memory allocation function and stashes a pointer to a codetag in task_struct for use by the core slub/buddy allocator code. That means that in your example, to move tracking to a given seq_file function, we just: - hook the seq_file function with alloc_hooks - change the seq_file function to call non-hooked memory allocation functions. > It would have been more convincing if you had some numbers at hands. > E.g. this is a typical workload we are dealing with. With the compile > time tags we are able to learn this with that much of cost. With a dynamic > tracing we are able to learn this much with that cost. See? As small as > possible is a rather vague term that different people will have a very > different idea about. Engineers don't prototype and benchmark everything as a matter of course, we're expected to have the rough equivealent of a CS education and an understanding of big O notation, cache architecture, etc. The slub fast path is _really_ fast - double word non locked cmpxchg. That's what we're trying to compete with. Adding a big globally accessible hash table is going to tank performance compared to that. I believe the numbers we already posted speak for themselves. We're considerably faster than memcg, fast enough to run in production. I'm not going to be switching to a design that significantly regresses performance, sorry :) > TBH I am much more concerned about the maintenance burden on the MM side > than the actual code tagging itslef which is much more self contained. I > haven't seen other potential applications of the same infrastructure and > maybe the code impact would be much smaller than in the MM proper. Our > allocator API is really hairy and convoluted. You keep saying "maintenance burden", but this is a criticism that can be directed at _any_ patchset that adds new code; it's generally understood that that is the accepted cost for new functionality. If you have specific concerns where you think we did something that makes the code harder to maintain, _please point them out in the appropriate patch_. I don't think you'll find too much - the instrumentation in the allocators simply generalizes what memcg was already doing, and the hooks themselves are a bit boilerplaty but hardly the sort of thing people will be tripping over later. TL;DR - put up or shut up :)
