> On Oct. 31, 2013, 10:18 p.m., Ian Downes wrote: > > src/slave/cgroups_isolator.cpp, line 782 > > <https://reviews.apache.org/r/15015/diff/1/?file=372971#file372971line782> > > > > Why don't we correct the naming while we're at it: > > > > total_memory <- memory.usage_in_bytes, this is the primary memory > > statistic > > > > # and then the breakdown: > > total_rss <- total_rss from memory.stat > > total_cached <- total_cache from memory.stat > > total_mapped_file <- total_mapped_file from memory.stat > > > > We probably don't need to include the total_ prefix? > > > > An executor's memory limit then matches with total_memory and, only if > > necessary, one can look at the breakdown. > > Eric Biederman wrote: > I don't follow. > > We absolutely need the total_ prefix when reading the fields from the > kernel. And it is the kernel's memory.stat file where the naming is > deceptive I can't fix that with a mesos patch. > > I did do what I can do with a mesos patch which is name the fields in > ResourceStatics and in the http interface in a non-deceptive way. > > We absolutely do not even want to consider passing the kernel names > straight through as those names are wrong. Especially the borked total_rss > field name as that is totally wrong. > > The current mapping implemented by this patch is: > > mem_rss_bytes <- memory.usage_in_bytes > mem_anon_bytes <- memory.stat:total_rss > mem_file_bytes <- memory.stat:total_cache > mem_mapped_file_bytes <- memory.stat:total_mapped_file > > I would include mem_mapped_anon_bytes but by definition that is exactly > the same as mem_anon_bytes. > > I can see renaming mem_rss_bytes something like mem_usage_bytes, however > that is going to break existing users of mesos, and breaking existing users > is a bad idea. And resident set size and usage synonyms so I don't see any > pressing need for a rename there. > > >
Sorry, I was meaning we shouldn't expose them as total_* but you're not doing that so please disregard. With the patch's renaming we'd have cgroups: mem_rss_bytes is not rss but the total process: mem_rss_bytes is rss and not the total So running the same executor under different isolators would report (potentially) very different numbers? - Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15015/#review27967 ----------------------------------------------------------- On Oct. 29, 2013, 6:53 a.m., Eric Biederman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15015/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2013, 6:53 a.m.) > > > Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. > > > Bugs: Mesos-758 > https://issues.apache.org/jira/browse/Mesos-758 > > > Repository: mesos-git > > > Description > ------- > > This reports a basic break down of memory usage so jobs whose page > cache expands to their configured hard limit will have information > that allows them to size their jobs. > > This does not include all fields from memory.stat and not even all > total_ fields from memory.stat but rather the fields I have reviewed > and seen a need for, and have a reasonable expectation that the fields > will report useful information and work as designed. > > Quite a few of the fields appear only useful for kernel level debugging. > > In particular the fields: > memory.stat:total_cache which reports the amount of file backed > memory in bytes > memory.stat:total_rss which reports the amount of anonymous memory > in bytes > memory.stat:total_mapped_file which reports the amount of mapped file > memory in bytes > > Reading memory.stat is not quite atomic and neither is computing the > values. As even in the simplest cases the values are sums of per cpu > counters of the various statistics. However for the most part the numbers > are a reasonable > snapshot of the state of the cgroup at a particular instant. > > > Diffs > ----- > > include/mesos/mesos.proto fe1d82b > src/slave/cgroups_isolator.cpp fc36f1f > src/slave/monitor.cpp 9cb6256 > src/tests/monitor_tests.cpp 3d3f8af > > Diff: https://reviews.apache.org/r/15015/diff/ > > > Testing > ------- > > make and make check pass > The code is a trivial extension of what already exists. > > > Thanks, > > Eric Biederman > >
