libctr/fs2: Initialize MaxUsage field with memory.peak value

[haircommander]

as well as rework the function to reduce duplication, and update the test for it

[haircommander]

I think this line should be questioned. I think it'd be better if we continued to collect if there was an error and returned whatever we found at the end

[kolyshkin]

Not quite. This is for case of name == "swap" (which is implicit here), meaning we're trying to read memory.swap.* values -- and they all are either present or not, so we bail out early when getting ENOENT from open("memory.swap.current").

OTOH if *.peak counter is optional, the logic should be changed. Is it?

While at it, we can always return memoryData not cgroups.MemoryData.

(BTW I took a look at CONFIG_MEMCG_SWAP and it was dropped from Linux v6.1, but there's still CONFIG_SWAP)

[kolyshkin]

Not quite. This is for case of name == "swap" (which is implicit here), meaning we're trying to read memory.swap.* values -- and they all are either present or not, so we bail out early when getting ENOENT from open("memory.swap.current").

Meaning, I'd drop the second commit in this PR (and maybe add a better comment explaining why we're bailing out early).

[haircommander]

*peak is added in kernel 5.14--so not yet in any el variants (it's being backported to centos 9 now) which is why I think the second commit is the correct behavior

[kolyshkin]

For peak, yes. So it should be like this:

• if memory.swap.current is not available, return empty set and no error (that's what the current code does);
• if *.peak is not available, return what we have collected so far.

[kolyshkin]

So, maybe something like this

if err != nil {
   if os.IsNotExist(err) {
      // Swap accounting is optional, so return early if absent.
      if name == "swap" {
         err = nil
      }
      // Peak usage is only available since kernel v5.14.
      if strings.HasSuffix(v.name, "peak") {
         err = nil
      }
     return memoryData, err
   }
}

(plus add a comment above saying the order is important)

[kolyshkin]

Or, for better readability,

-      if name == "swap" { 
+      if name == "swap" { // memory.swap.current not found.

or even

-      if name == "swap" { 
+      if f.name == "memory.swap.current" {

[lifubang]

Total memory max usage != memory.peak + memory.swap.peak
#4010 (comment)

[haircommander]

I didn't notice #4010! convo is more developed over there, so I can close this in favor of that