Some thoughts after seeing a similar discussion on Slack.
I think that, on Linux, we should be really careful in using MADV_FREE.
First of all, MADV_FREE is rather new, so any use should really really check whether it's supported or not, i,.e,. whether madvise(MADV_FREE) returns EINVAL or such.

Even when supported, though, MADV_FREE has lazy semantic. It marks memory as free-able, but, opposite to MADV_WONTNEED, memory is not cleared up immediately. In Linux is moved into some inactive list and the physical pages are released in a deferred way in case of memory pressure.

I personally think we should never use it by default, and use only when we can prove that the free() hurts the fastpath. Rational: MADV_FREE is more likely to leave resident memory around.

c#1 SGTM, so long as the behavior is consistent in the codebase.  I would also recommend adding a presubmit to warn against using MADV_FREE.

Adding Partition Alloc owners.

#1: I think we *want* the lazy semantics of MADV_FREE, right? (At least in Partition Alloc?)

If we end up re-using 1 or more of the _FREE'd pages, it might cost us less than if we had _DONTNEEDed them. _DONTNEED is more likely to have to zero-fill-on-demand, whereas with _FREE we might get lucky and the page will still be around.

That is, if I am understanding http://man7.org/linux/man-pages/man2/madvise.2.html correctly. I could be wrong.

Regarding "leav[ing] resident memory around": you're concerned that the kernel will consider us to still be using the pages when we use _FREE, as opposed to _DONTNEED, for which is documented: "The resident set size (RSS) of the calling process will be immediately reduced however." (No such guarantee is documented for _FREE, AFAICT.)

Any thoughts on https://chromium-review.googlesource.com/c/chromium/src/+/754038 ? I'm amenable to simplifying it by just getting rid of MADV_FREE, but only if I understand the issues correctly.

I am similarly uncertain why we care to force the freeing of RSS immediately. The Kernel knows it can get it back so is this just an optimization for optics?

> The Kernel knows it can get it back so is this just an optimization for optics?
And predictability of measurements.

Not sure what is the advantage of using MADV_FREE.
https://lwn.net/Articles/591214/ seems to suggest the advantage is in terms of not taking the write-side map_sem lock held while madvsising. Not sure if this is something that makes a perf difference for us though.
I'd try moving back to MADV_DONTNEED and if the perf waterfall doesn't detect any perf regression just keep it that way.

Right now it seems a potential perf optimization vs a more real near-term memory retention.

#6: Predictability of measurements is a compelling reason, indeed. Are we seeing unpredictable measurements now, due to MADV_FREE? (How can I see this unpredictability?)

I propose to land https://chromium-review.googlesource.com/c/chromium/src/+/754038 just because it is textually more clear (and does avoid a redundant system call), and then testing performance of a single-purpose "get rid of _FREE" CL and seeing how it goes.

#7 sounds like a good strategy to me.

#7 in progress...

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be7b68a78100830d2cf97319ca2aee64cf4f3edb

commit be7b68a78100830d2cf97319ca2aee64cf4f3edb
Author: Chris Palmer <palmer@chromium.org>
Date: Tue Nov 07 22:50:24 2017

[Partition Alloc] Don't redundantly call madvise redundantly.

When

  #ifndef MADV_FREE
  #define MADV_FREE MADV_DONTNEED
  #endif

we would call madvise(..., MADV_FREE); and then, if that failed, call
madvise(..., MADV_DONTNEED). This would result in pointlessly calling
madvise(..., MADV_DONTNEED) and then trying again instead of CHECKing
immediately.

This is more of a readability refactor than a performance-relevant change,
obviously.

Thanks to ajwong for noticing this!

BUG=766882, 755284 

Change-Id: If8cbe14f38dfe2bd126bc24f79e670c45b29c8d5
Reviewed-on: https://chromium-review.googlesource.com/754038
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514635}
[modify] https://crrev.com/be7b68a78100830d2cf97319ca2aee64cf4f3edb/base/allocator/partition_allocator/page_allocator.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85ec7854664107ae9f5285b8febb97d5d1f5a0ae

commit 85ec7854664107ae9f5285b8febb97d5d1f5a0ae
Author: Chris Palmer <palmer@chromium.org>
Date: Thu Nov 09 04:06:10 2017

Revert "[Partition Alloc] Don't redundantly call madvise redundantly."

This reverts commit be7b68a78100830d2cf97319ca2aee64cf4f3edb.

Reason for revert: Lost the crucial CHECK(!ret) in a merge conflict, and re-adding it causes macOS to fail (EINVAL, invalid address). That requires further investigation.

Original change's description:
> [Partition Alloc] Don't redundantly call madvise redundantly.
> 
> When
> 
>   #ifndef MADV_FREE
>   #define MADV_FREE MADV_DONTNEED
>   #endif
> 
> we would call madvise(..., MADV_FREE); and then, if that failed, call
> madvise(..., MADV_DONTNEED). This would result in pointlessly calling
> madvise(..., MADV_DONTNEED) and then trying again instead of CHECKing
> immediately.
> 
> This is more of a readability refactor than a performance-relevant change,
> obviously.
> 
> Thanks to ajwong for noticing this!
> 
> BUG=766882, 755284 
> 
> Change-Id: If8cbe14f38dfe2bd126bc24f79e670c45b29c8d5
> Reviewed-on: https://chromium-review.googlesource.com/754038
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Commit-Queue: Chris Palmer <palmer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#514635}

TBR=ajwong@chromium.org,palmer@chromium.org,primiano@chromium.org,haraken@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 766882,  755284 
Change-Id: I2b71c2f6773c38109d17c151e5a81aa50bc46b10
Reviewed-on: https://chromium-review.googlesource.com/759058
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515091}
[modify] https://crrev.com/85ec7854664107ae9f5285b8febb97d5d1f5a0ae/base/allocator/partition_allocator/page_allocator.cc

I've posted https://chromium-review.googlesource.com/c/chromium/src/+/990584 as a fresh take on it. (There was a refactor that changed and moved a bunch of stuff, so this was easiest.)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/292df1662cdb1d04f210343aee0f9fc361fd16c6

commit 292df1662cdb1d04f210343aee0f9fc361fd16c6
Author: Chris Palmer <palmer@chromium.org>
Date: Tue Apr 03 01:54:52 2018

[Partition Alloc] Linux: Use MADV_DONTNEED instead of MADV_FREE[_RESUABLE].

Linux: We're not sure what the performance benefits of MADV_FREE are, and it makes
our measurements less predictable.

macOS: MADV_FREE_REUSABLE seems to have been failing, and the error was hidden by
the recovery case intended for Linux MADV_FREE. The result was an unnecessary,
failing call to `madvise`.

Bug:  755284 
Change-Id: Ic7aa3089ada131f6a1b5c40b5ddd353d34ed2c20
Reviewed-on: https://chromium-review.googlesource.com/990584
Commit-Queue: Chris Palmer <palmer@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547598}
[modify] https://crrev.com/292df1662cdb1d04f210343aee0f9fc361fd16c6/base/allocator/partition_allocator/page_allocator_internals_posix.h
[modify] https://crrev.com/292df1662cdb1d04f210343aee0f9fc361fd16c6/base/allocator/partition_allocator/partition_alloc_unittest.cc

Also ended up removing non-functional code on macOS, too, saving the cost of a wasted system call.