Fix free memory calculation on v3.14+

[chrisrd]

Description

Fix free memory calculation on v3.14+

Motivation and Context

arc_free_memory() has been broken since an API change in Linux v3.14:

2016-07-28 599d0c95 mm, vmscan: move LRU lists to node

That moved some of global_page_state() into global_node_page_state(). The API change was particularly egregious as, instead of breaking the old code, it silently did the wrong thing and we continued using global_page_state() where we should have been using global_node_page_state(), thus indexing into the wrong array via NR_SLAB_RECLAIMABLE et al.

This was nearly fixed in ZoL by:

2017-08-24 2209e40 Linux 4.8+ compatibility fix for vm stats

...which put in place the config infrastructure to detect the API change and fix an instance of NR_FILE_PAGES in arc_evictable_memory(). Unfortunately the free memory calculation at that time was done by SPL freemem, and the API fix wasn't applied to the SPL.

As a result our free memory calculation remained broken until it was again nearly fixed by:

2017-09-16 787acae Linux 3.14 compat: IO acct, global_page_state, etc
2017-09-19 661907e Linux 4.14 compat: IO acct, global_page_state, etc (#6655)

These two patches copy SPL freemem to arc_free_memory() and do the global_node_page_state() split, almost correctly...

Unfortunately they assume NR_SLAB_RECLAIMABLE moved to global_node_page_state() along with NR_INACTIVE_FILE and NR_INACTIVE_ANON, but NR_SLAB_RECLAIMABLE actually stayed in global_page_state(), thus leaving our free memory calculation broken for 19 months on Linux v3.14+ - until now!

Additionally, Linux v4.14 (torvalds/linux@c41f012a) changes global_page_state() to global_zone_page_state(), so fix that as well.

Note:

This patch should be added to any forthcoming zfs-0.7.7 patch stack - is anyone going to look after that?

How Has This Been Tested?

A previous version of this patch (w/o 4.14 API change) has been running for a week on a heavily loaded 0.7.6 / v4.9.76 system w/ 1500-2000 mounted filesystems/snapshots, and has enormously improved the sanity of the system, including removing extreme loads associated with otherwise inexplicable zfs_prune storms, and occasional wild swings in ARC usage.

Time to let the buildbots at it...

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.
• Change has been approved by a ZFS on Linux member.

[behlendorf]

Looks good! Thank you for staying on top of this one and making sure it's really fixed.

[behlendorf]

nit: spaces accidentally used here

[chrisrd]

Huh! Copied from config/kernel-vm_node_stat.m4. I'll fix mine once buildbots have done their thing.

[behlendorf]

Feel free to fix both when you refresh this. At some point we should extend to style checker to cover the m4 files.

[chrisrd]

...speaking of buildbots, style check seemingly doesn't like the "unbalanced" braces due to multiple alternate closing lines:

        return (ptob(nr_free_pages() +
            ...
#ifdef ZFS_GLOBAL_ZONE_PAGE_STATE
            global_zone_page_state(NR_SLAB_RECLAIMABLE)));
#else
            global_page_state(NR_SLAB_RECLAIMABLE)));
#endif /* ZFS_GLOBAL_ZONE_PAGE_STATE */

Would this be the preferred style?

        return (ptob(nr_free_pages() +
            ...
#ifdef ZFS_GLOBAL_ZONE_PAGE_STATE
            global_zone_page_state(NR_SLAB_RECLAIMABLE)
#else
            global_page_state(NR_SLAB_RECLAIMABLE)
#endif /* ZFS_GLOBAL_ZONE_PAGE_STATE */
        ));

[chrisrd]

...nope, stylecheck doesn't like the closing parentheses on a line by themselves (which is a shame because I'm partial to that style). Which seems to leave us with the verbose but stylecheck clean:

#ifdef ZFS_GLOBAL_ZONE_PAGE_STATE
        return (ptob(nr_free_pages() +
            /* node stats */
            global_node_page_state(NR_INACTIVE_FILE) +
            global_node_page_state(NR_INACTIVE_ANON) +
            /* zone stats */
            global_zone_page_state(NR_SLAB_RECLAIMABLE)));
#else
#ifdef ZFS_GLOBAL_NODE_PAGE_STATE
        return (ptob(nr_free_pages() +
            /* node stats */
            global_node_page_state(NR_INACTIVE_FILE) +
            global_node_page_state(NR_INACTIVE_ANON) +
            /* zone stats */
            global_page_state(NR_SLAB_RECLAIMABLE)));
#else
        return (ptob(nr_free_pages() +
            global_page_state(NR_INACTIVE_FILE) +
            global_page_state(NR_INACTIVE_ANON) +
            global_page_state(NR_SLAB_RECLAIMABLE)));
#endif /* ZFS_GLOBAL_NODE_PAGE_STATE */
#endif /* ZFS_GLOBAL_ZONE_PAGE_STATE */

[behlendorf]

My editor really isn't happy with it either. I'd suggest we wrap the entire statement like this, it's a little more verbose but more readable.

diff --git a/module/zfs/arc.c b/module/zfs/arc.c
index 7aa221b..d830eb3 100644
--- a/module/zfs/arc.c
+++ b/module/zfs/arc.c
@@ -4700,10 +4700,17 @@ arc_free_memory(void)
        return (ptob(si.freeram - si.freehigh));
 #else
 #ifdef ZFS_GLOBAL_NODE_PAGE_STATE
+#ifdef ZFS_GLOBAL_ZONE_PAGE_STATE
        return (ptob(nr_free_pages() +
            global_node_page_state(NR_INACTIVE_FILE) +
            global_node_page_state(NR_INACTIVE_ANON) +
-           global_node_page_state(NR_SLAB_RECLAIMABLE)));
+           global_zone_page_state(NR_SLAB_RECLAIMABLE)));
+#else
+       return (ptob(nr_free_pages() +
+           global_node_page_state(NR_INACTIVE_FILE) +
+           global_node_page_state(NR_INACTIVE_ANON) +
+           global_page_state(NR_SLAB_RECLAIMABLE)));
+#endif /* ZFS_GLOBAL_ZONE_PAGE_STATE */
 #else
        return (ptob(nr_free_pages() +
            global_page_state(NR_INACTIVE_FILE) +

[chrisrd]

Looks good, cleaner than my version, I'll include it next push. I had the /* node stats */ and /* zone stats */ comments in there to further distinguish between the two types of calls - leave the comments in?

[behlendorf]

I'd suggest leaving them out, personally I think it's pretty clear without them. Even more so now that it's wrapped in ZFS_GLOBAL_ZONE_PAGE_STATE. However, elaborating a little more more in the block comment above the function would be good, or even in each respective case which is handled.

[chrisrd]

Like so?

/*
 * Return the amount of memory that is considered free.  In user space
 * which is primarily used for testing we pretend that free memory ranges
 * from 0-20% of all memory.
 *
 * Note: be very careful about global_node_page_state() vs 
 * global_zone_page_state() (or global_page_state() depending on kernel
 * version).  Both calls will silently accept the other call's arguments without
 * complaint, and you'll be indexing into the wrong underlying array, causing
 * unspecified results.
 */  

[behlendorf]

Looks good.

[gmelikov]

Great catch, thanks!

[dinatale2]

@chrisrd Thank you for the PR!

I was looking at enum zone_stat_item and enum node_stat_item in the Linux kernel for various tags. I'm noticing that at some point NR_SLAB_RECLAIMABLE moves from being part of enum zone_stat_item to being a part of enum node_stat_item.

In v4.12 (and prior), NR_SLAB_RECLAIMABLE is part of enum zone_stat_item as seen here.
In v4.13, NR_SLAB_RECLAIMABLE was moved to enum node_stat_item as seen here. The actual move occurred in commit torvalds/linux@385386c.
As of v4.16-rc1 NR_SLAB_RECLAIMABLE is still part of enum node_stat_item as seen here.

In v4.13 (and prior), the prototype for global_page_state is static inline unsigned long global_page_state(enum zone_stat_item item) as seen here.
In v4.14, the global_zone_page_state rename occurs and it has a prototype of static inline unsigned long global_zone_page_state(enum zone_stat_item item) as seen here.

In v4.12 and above, the prototype for global_node_page_state is static inline unsigned long global_node_page_state(enum node_stat_item item) as seen here.

Long story short, what I'm worried about (and this could be a lack of understanding on my part) is that for kernel version 4.13 and above, it appears that we should be using global_node_page_state(NR_SLAB_RECLAIMABLE) based on the enum definitions.

A reasonable solution might be to break this up into 3 cases and use the right function/enums for each. The 3 cases are:

1. v4.12 and older
2. v4.13
3. v4.14 and above

[behlendorf]

Please triple check this but I believe we should be instead checking for the change of NR_SLAB_RECLAIMABLE from a zone_stat_item to a node_stat_item. In fact we never need to use the global_zone_page_state interface since by the time it was added NR_SLAB_RECLAIMABLE was moved to the node.

#if defined(ZFS_GLOBAL_NODE_PAGE_STATE) && \
    defined(ZFS_NODE_STAT_NR_SLAB_RECLAIMABLE)
        /* 4.13+ API */
        return (ptob(nr_free_pages() +
            global_node_page_state(NR_INACTIVE_FILE) +
            global_node_page_state(NR_INACTIVE_ANON) +
            global_node_page_state(NR_SLAB_RECLAIMABLE)));
#elif defined(ZFS_GLOBAL_NODE_PAGE_STATE)
        /* 4.9 - 4.12 API */
        return (ptob(nr_free_pages() +
            global_node_page_state(NR_INACTIVE_FILE) +
            global_node_page_state(NR_INACTIVE_ANON) +
            global_page_state(NR_SLAB_RECLAIMABLE)));
#else
        /* x - 4.8 API */
        return (ptob(nr_free_pages() +
            global_page_state(NR_INACTIVE_FILE) +
            global_page_state(NR_INACTIVE_ANON) +
            global_page_state(NR_SLAB_RECLAIMABLE)));
#endif

[dinatale2]

@behlendorf That looks correct to me.

[chrisrd]

Oh wow, it's even worse than I imagined, even with my spelunking through the history. Looking at...

[chrisrd]

I agree something like ZFS_NODE_STAT_NR_SLAB_RECLAIMABLE is the correct way to deal with this issue, along with similar tests for the other page stats we use, currently NR_INACTIVE_FILE and NR_INACTIVE_ANON.

However I'm a bit stumped as to how to create config tests for these.

The basic issue is, we're needing to detect where we're using the wrong enum type in a function call. Unfortunately, in standard C, one enum type isn't distinguished from another. gcc adheres to this and doesn't have any option to warn/error on enum type mismatches; g++ does warn on this, as does clang, e.g.:

$ cat > foo.c <<END
enum node_stat_item { FOO };
enum zone_stat_item { NR_SLAB_RECLAIMABLE };

void global_node_page_state(enum node_stat_item e);
void global_zone_page_state(enum zone_stat_item e);

/* wrong enum type */
void foo() { global_node_page_state(NR_SLAB_RECLAIMABLE); }
END
$ for CC in gcc g++ clang; do
  echo -e "\n=== $CC ==="
  $CC -Wall -Wextra -Werror -c foo.c && echo "*** No error ***"
done

=== gcc ===
*** No error ***

=== g++ ===
foo.c: In function ‘void foo()’:
foo.c:8:56: error: cannot convert ‘zone_stat_item’ to ‘node_stat_item’ for argument ‘1’ to ‘void global_node_page_state(node_stat_item)’
 void foo() { global_node_page_state(NR_SLAB_RECLAIMABLE); }
                                                        ^

=== clang ===
foo.c:8:37: error: implicit conversion from enumeration type 'enum zone_stat_item' to different enumeration type 'enum node_stat_item'
      [-Werror,-Wenum-conversion]
void foo() { global_node_page_state(NR_SLAB_RECLAIMABLE); }
             ~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~
1 error generated.

gcc does have a -Wswitch-enum option which looked promising, but that doesn't distinguish enum types or labels, just the underlying values; it (and, somewhat surprisingly, g++ and clang) will happily compile this without error:

$ cat > foo2.c <<END
enum E { A, B };
enum X { C, D };

int foo(enum E e) {
  switch (e) {
  case C: return 1;
  case D: return 2;
  }
  return 0;
}
END
$ for CC in gcc g++ clang; do
  echo -e "\n=== $CC ==="
  $CC -Wall -Wextra -Werror -c foo2.c && echo "*** No error ***"
done


=== gcc ===
*** No error ***

=== g++ ===
*** No error ***

=== clang ===
*** No error ***

So at this point the only way I've found is to use g++ (or clang) to detect the enum type mismatch.

But can we reasonably require g++ (or clang) be available on any system where ZoL is built?

If so, is it possible to have configure use g++ (or clang) for a specific test (e.g. config/kernel-global-page-state.m4) so we have access to the enum mismatch detection for the purpose of the ZFS_NODE_STAT_NR_SLAB_RECLAIMABLE config test?

Or is there another way?

[chrisrd]

Incidentally, I found this useful to see if there were any other instances of these things moving around in recent times (there aren't ...yet):

$ {
  cd git/linux
  pp='^[+-][[:space:]]+(NR_INACTIVE_FILE|NR_INACTIVE_ANON|NR_SLAB_RECLAIMABLE)'
  git log --date=short --pretty='format:%h' include/linux/mmzone.h |
  while read c
  do
    git show $c | grep -Eq "${pp}" || continue
    git log --abbrev=8 --date=short -n 1 --pretty='format:%cd %h %s' "$c"
  done
}
2017-07-06 385386cf mm: vmstat: move slab statistics from zone to node counters
2016-07-28 599d0c95 mm, vmscan: move LRU lists to node
2008-10-20 4f98a2fe vmscan: split LRU lists into anon & file sets
2007-02-11 51ed4491 [PATCH] Reorder ZVCs according to cacheline
2006-09-26 0ff38490 [PATCH] zone_reclaim: dynamic slab reclaim
2006-09-26 972d1a7b [PATCH] ZVC: Support NR_SLAB_RECLAIMABLE / NR_SLAB_UNRECLAIMABLE

[dinatale2]

I was looking at the gcc warnings yesterday and ran into -Wenum-compare. Maybe you can do something like

enum node_stat_item n = NR_SLAB_RECLAIMABLE == (enum node_stat_item)0 ? NR_SLAB_RECLAIMABLE : NR_SLAB_RECLAIMABLE;

That would force a comparison between enum values and gcc would warn if there’s a bad comparison there?

[behlendorf]

@chrisrd @dinatale2 I don't see a good way to do this either, and we don't want to bring in any additional dependencies. How about opting for a more primitive approach. Instead of invoking gcc and keying off a compiler warning/error, we could just match the pattern in the header. Something like this.

awk '/enum node_stat_item/ {flag=1;next}  /\}/{flag=0} flag {print}' include/linux/mmzone.h |  grep NR_SLAB_RECLAIMABLE

We should check for NR_SLAB_RECLAIMABLE in both node_stat_item and zone_stat_item and fail if we can't find it in either. The good news is that these enums have been in include/linux/mmzone.h since at least 2.6.32.

[sempervictus]

This might explain a lot of lost hours of sleep with memory loaded scst systems inexplicably buying the farm when the kernel OOMs. Thank you!

[chrisrd]

New patch pushed, with full auto-configure for global page state API changes.

[chrisrd]

OK, I'm officially perplexed about the buildbots...

Thus far, with "7 failing, 2 pending, and 14 successful checks", all the (TEST) bots have failed, and all the (BUILD) bots are ok.

What's the difference between the two?

Some and maybe all of the (TEST) bots are correctly detecting the page stuff coming out of config/kernel-global_page_state.m4 per their config.log, e.g. Amazon 2 x86_64 Release (TEST) - config.log

#define ZFS_GLOBAL_NODE_PAGE_STATE 1
#define ZFS_ENUM_NODE_STAT_ITEM_NR_FILE_PAGES 1
#define ZFS_ENUM_NODE_STAT_ITEM_NR_INACTIVE_ANON 1
#define ZFS_ENUM_NODE_STAT_ITEM_NR_INACTIVE_FILE 1
#define ZFS_ENUM_NODE_STAT_ITEM_NR_SLAB_RECLAIMABLE 1

...but the make fails like:

In file included from /tmp/zfs-build-buildbot-DWI44zSR/BUILD/zfs-kmod-0.7.0/_kmod_build_4.9.77-41.59.amzn2.x86_64/../zfs-0.7.0/module/zfs/arc.c:300:0:
/tmp/zfs-build-buildbot-DWI44zSR/BUILD/zfs-kmod-0.7.0/_kmod_build_4.9.77-41.59.amzn2.x86_64/../zfs-0.7.0/include/linux/mm_compat_zfs.h:23:2: error: #error NR_FILE_PAGES not found in either node_stat_item or zone_stat_item
 #error NR_FILE_PAGES not found in either node_stat_item or zone_stat_item
  ^~~~~
/tmp/zfs-build-buildbot-DWI44zSR/BUILD/zfs-kmod-0.7.0/_kmod_build_4.9.77-41.59.amzn2.x86_64/../zfs-0.7.0/include/linux/mm_compat_zfs.h:31:2: error: #error NR_INACTIVE_ANON not found in either node_stat_item or zone_stat_item
 #error NR_INACTIVE_ANON not found in either node_stat_item or zone_stat_item
  ^~~~~
/tmp/zfs-build-buildbot-DWI44zSR/BUILD/zfs-kmod-0.7.0/_kmod_build_4.9.77-41.59.amzn2.x86_64/../zfs-0.7.0/include/linux/mm_compat_zfs.h:39:2: error: #error NR_INACTIVE_FILE not found in either node_stat_item or zone_stat_item
 #error NR_INACTIVE_FILE not found in either node_stat_item or zone_stat_item
  ^~~~~
/tmp/zfs-build-buildbot-DWI44zSR/BUILD/zfs-kmod-0.7.0/_kmod_build_4.9.77-41.59.amzn2.x86_64/../zfs-0.7.0/include/linux/mm_compat_zfs.h:47:2: error: #error NR_SLAB_RECLAIMABLE not found in either node_stat_item or zone_stat_item
 #error NR_SLAB_RECLAIMABLE not found in either node_stat_item or zone_stat_item
  ^~~~~

which comes out of include/linux/mm_compat_zfs.h and indicates the defines shown in the config.log aren't being seen - but they're obviously being seen by the (BUILD) bots.

Any ideas?

And Fedora 27 x86_64 (TEST) config.log is showing a whole bunch of:

WARNING: modpost: missing MODULE_LICENSE() in /var/lib/buildbot/slaves/zfs/Fedora_27_x86_64__TEST_/build/zfs/build/conftest.o

...no idea what that's about either.

[edillmann]

Same thing on my side, building using autogen.sh + configure + make works,
But make deb (stretch/4.14.19-vanilla) give the same traces

[edillmann]

configure:26815: checking whether enum node_stat_item contains NR_SLAB_RECLAIMABLE
configure:26818: scripts/enum-extract.pl node_stat_item /usr/src/kernel/linux-4.14.19/include/linux/mmzone.h | egrep -qx NR_SLAB_RECLAIMABLE
../zfs-0.7.0/configure: line 26819: scripts/enum-extract.pl: No such file or directory
configure:26821: $? = 1

This should give you a clue

[chrisrd]

Thanks - I'll look after those comments.

What do you think of current version vs the "pure autoconf" style for ZFS_AC_KERNEL_GLOBAL_ZONE_PAGE_STATE_SANITY?

current
pure autoconf

Worthwhile changing to the pure version?

[behlendorf]

Worthwhile changing to the pure version?

Yes, please do. I like the pure version much better and since you've already done the work let's include it when you refresh this.

[chrisrd]

Refreshed!

[behlendorf]

Looks good, thanks for tackling this bit of insanity!

[chrisrd]

Is anyone looking after a patch stack for any forthcoming zfs-0.7.7? This definitely needs to go in...

[behlendorf]

@chrisrd yes, and I've already adding this PR to the project.

[dinatale2]

Thanks for all the effort on this PR!

[behlendorf]

@chrisrd if you can rebase this one last time on master to resolve the new config/kernel.m4 conflict we can then get this merged.

[chrisrd]

Done!

[sempervictus]

Yeah, I pulled the 0.7.7 diff and the script IS there, but not executable - currently using scriptage in our build system to chmod+x it after the diff application.

[beren12]

So would this bug cause zfs_arc_max to be ignored and grow larger? I'm seeing this happen on my raspberry pi 3B+…