Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1363657 > unrolled thread

Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem Hugh Dickins <hughd@google.com> - 2016-03-23 21:10 +0100
    Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-03-24 10:20 +0100
      Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem Hugh Dickins <hughd@google.com> - 2016-03-24 20:10 +0100
        Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-03-25 16:10 +0100
          Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem Hugh Dickins <hughd@google.com> - 2016-03-26 01:10 +0100
            Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-03-28 20:10 +0200
              Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem Hugh Dickins <hughd@google.com> - 2016-03-28 20:50 +0200
    Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-03-28 14:30 +0200

#1363657 — Re: [PATCHv4 00/25] THP-enabled tmpfs/shmem

On Sat, 12 Mar 2016, Kirill A. Shutemov wrote:

> Here is an updated version of huge pages support implementation in
> tmpfs/shmem.
> 
> All known issues has been fixed. I'll continue with validation.
> I will also send follow up patch with documentation update.
> 
> Hugh, I would be glad to hear your opinion on this patchset.

At long last I've managed to spend some time getting into it.

My opinion is simple: generally I liked your patches,
but I'm disappointed by where they leave tmpfs functionally,
so wouldn't want the patchset to go in as is.

As soon as I ran it up and tried to copy a tree in there for testing,
my (huge) tmpfs filled up with all these 2MB files: which is just the
same as when you started out two or three years ago on ramfs.

As I've said on several occasions, I am not interested in emulating
the limitations of hugetlbfs inside tmpfs: there might one day be a
case for such a project, but it's transparent hugepages that we want to
have now - blocksize 4kB, but in 2MB extents when possible (on x86_64).

I accept that supporting small files correctly is not the #1
requirement for a "huge" tmpfs; and if it were impossible or
unreasonable to ask for, I'd give up on it.  But since we (Google)
have been using a successful implementation for a year now, I see
no reason to give up on it at all: it allows for much wider and
easier adoption (and testing) of huge tmpfs - without "but"s.

The small files thing formed my first impression.  My second
impression was similar, when I tried mmap(NULL, size_of_RAM,
PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0) and
cycled around the arena touching all the pages (which of
course has to push a little into swap): that soon OOMed.

But there I think you probably just have some minor bug to be fixed:
I spent a little while trying to debug it, but then decided I'd
better get back to writing to you.  I didn't really understand what
I was seeing, but when I hacked some stats into shrink_page_list(),
converting !is_page_cache_freeable(page) to page_cache_references(page)
to return the difference instead of the bool, a large proportion of
huge tmpfs pages seemed to have count 1 too high to be freeable at
that point (and one huge tmpfs page had a count of 3477).

Hmm, I just went to repeat that test, to see if MemFree goes down and
down on each run, but got a VM_BUG_ON_PAGE(page_ref_count(page) == 0)
in put_page_testzero() in find_get_entries() during shmem_evict_inode().
So, something not quite right with the refcounting when under pressure.

My third impression was much better, when I just did a straight
cp /dev/zero /tmpfs_twice_size_of_RAM: that went smoothly,
pushed into swap just as it should, nice.

> 
> The main difference with Hugh's approach[1] is that I continue with
> compound pages, where Hugh invents new way couple pages: team pages.
> I believe THP refcounting rework made team pages unnecessary: compound
> page are flexible enough to serve needs of page cache.

I have disagreed with you about the suitability of compound pages;
but at some point on Sunday night, after reading your patchset,
found myself looking at it differently, and now agreeing with you.

They are not flexible enough yet, but I believe you have done a large
part of the work (by diverting all those tail ops to the head), and it
just needs a little more.  I say "a little", but it might not be easy.

What I realize now, is that you should be able to do almost everything
I did with team pages, instead with your compound pages.  Just abandon
the idea that the whole compound page has to be initialized in one go:
initialize (charge to memcg, add to page cache, fill, SetPageUptodate)
each subpage as it is touched.  (But of course, the whole extent has
to be to be initialized before it can be pmd-mapped.)

And the convenient thing about a compound page is that you have plenty
of space to hold your metadata: where I have been squeezing everything
into page->private (which makes it difficult then to extend the same
design to miscellaneous filesystems), you're free to use (certain)
fields of 511 more struct pages.

I said above that you should be able to do almost everything: I think
that "almost" involves dropping a few things that I have, that we can
well do without.  With team pages, it is possible for each member to
be charged to a different memcg; but I don't have anyone calling out
for that behaviour, it's more an awkward possibility that we have to
be on guard against in a few places, and I'd be quite happy just to
stop it (charging instead to whichever memcg first touched the extent).

And it's been nice to entertain the idea of the individual team tails,
travelling separately up and down their own lrus, subject to their own
pagesize access patterns even while the whole is pmd-mapped.  But in
practice, we continue to throw them on the unevictable list once they
get in the way, with no attempt to bring them back when accessed
individually; so I think it's a nice idea that we can live without.

And a compound page implementation would avoid the overhead I have,
of putting all those tails on lru in the first place.

Another thing I did, that I think you can safely fail to emulate:
I was careful to allow sparse population, and not insist that the
head page be instantiated.  That might turn out to be more of a
problem for compound pages, and has been an awkward edge case for me.
Although in principle there are sparse files which would be doubled
in charge by insisting that the head be instantiated, I doubt they
will hurt anyone in practice: perhaps best treated as a bug to be
revisited if it actually comes up as an issue later.

(Thinking about that last paragraph later, I'm not so sure.
Forcing the instantiation of the head is okay when first allocating,
but when trying to recover a fragmented extent into a hugepage later,
it might prove to be seriously limiting to require that the head be
migrated into place before anything else is done.)

> 
> Many ideas and some patches were stolen from Hugh's patchset. Having this
> patchset around was very helpful.

People are probably wondering why I didn't repost it, and whether
it's vapourware.  No, it's worked out very well, better than we were
expecting; and several of the TODOs that I felt needed attention,
have not yet caused anyone trouble - though wider adoption is sure
to meet new usecases and new bugs.  I didn't repost it because, as I
said at the time, I felt it wasn't ready for real use until recovery
of huge pages after fragmentation and swapping (what khugepaged does
for anon THP) was added.  After that went in (and then quite a period
of fixing the bugs, I admit!), other priorities intervened until now.
Your patchset also lacks recovery at present, so equally that's a
reason why I don't consider yours ready for merging.

I'll repost mine, including recovery and a few minor additions, against
v4.6-rc2.  I still haven't got around to doing any NUMA work on it, but
it does appear to be useful to many people even without that.  I'd have
preferred to spend longer rebasing it, it may turn out to be a little
raw: most of the rebases from one release to another have been easy,
but to v4.5 was messier - not particularly due to your THP refcounting
work going in, just various different conflicts that I feel I've not
properly digested yet.  But v4.6-rc2 looks about as early and as late
as I can do it, with LSF/MM coming up later in April; and should be a
good base for people to try it out on.  (I'll work on preparing it in
advance, then I guess a last-minute edit of PAGE_CACHEs before posting.)

I now believe that the right way forward is to get that team page
implementation in as soon as we can, so users have the functionality;
and then (if you're still interested) you work on converting it over
from teams to compound pages, both as an optimization, and to satisfy
your understandable preference for a single way of managing huge pages.

(I would say "How does that sound to you?" but I'd prefer you to think
it over, maybe wait to see what the new team patchset looks like.  Though
aside from the addition of recovery, it is very much like the one against
v3.19 from February last year: no changes to the design at all, which
turned out to fit recovery well; but quite a few bug fixes folded back
in.  Or you may prefer to wait to discuss the way forward at LSF/MM.)

> 
> I will continue with code validation. I would expect mlock require some
> more attention.
> 
> Please, review and test the code.
> 
> Git tree:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/kas/linux.git hugetmpfs/v4
> 
> Rebased onto linux-next:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/kas/linux.git hugetmpfs/v4-next

It wasn't obvious to me what tree your posted patchset was based on,
so I made a patch out of the top your hugetmpfs/v4-next branch, and
applied that on top of Andrew's 4.5-rc7-mm1: which appeared to be
fine, just a trivial clash with mmotm's
"do_shared_fault(): check that mmap_sem is held".

[...]

Some random thoughts on your patches FWIW:

01/25 mm: do not pass mm_struct into handle_mm_fault
02/25 mm: introduce fault_env

These two inflate your patchset considerably, with seemingly unrelated
changes: the first with arch files otherwise untouched, the second with
lots of change to memory.c that makes it harder to compare our patchsets.

I think it would be better to leave the handle_mm_fault(mm) removal to
another patchset, and I'm ambivalent about the struct fault_env changes:
I prefer to avoid such argument blocks (and think I saw Linus once say
he detested my zap_details), which do make it harder to see what the
real arguments are.  I wonder what your 03/25 would look like without
the fault_env, perhaps I'd agree it's too cumbersome that way (and
perhaps I should look back at earlier versions of your patchset).

But I do understand your desire to get some cleanups out of the way
before basing the real work on the new structure: I'm going to have
to reconsider the initial patches of my series in the light of this
criticism of yours, not sure yet if I'll try to cut them out or not.

03/25 mm: postpone page table allocation until we have page to map

I've not stopped to consider whether your prealloc_pte work (which
perhaps was the trigger for fault_env) meets the case or not, but
want to mention that we recently discovered a drawback to my delaying
the pagetable allocation: I'm allocating the pagetable while holding
the lock on the page from the filesystem (any filesystem, though the
change was to suit tmpfs), which presented a deadlock to the OOM killer.
The base kernel involved was not a recent one, and there have been
rewrites in the OOMing area since, so I'm not sure whether it would
be a deadlock today: but I am sure that it would be better to do the
allocation while not holding the page lock.  We have a fix in place,
but I don't think it's quite the right fix, so I want to revisit it:
unless I get a gift of time from somewhere, I'll post my series with
the ordering here just as I had it before.

You remark on yours: "This way we can postpone allocation even in
faultaround case without moving do_fault_around() after __do_fault()."
Was there something wrong or inefficient with my moving do_fault_around()
after __do_fault()?  I thought it was fine; but I did also think when
I made the change, that it would be fixing a bug in VM_FAULT_MAJOR
acccounting too; yet when I went to confirm that, couldn't find evidence
for such a bug before; but I moved on before getting to the bottom of it. 

04/25 rmap: support file thp
05/25 mm: introduce do_set_pmd()
06/25 mm, rmap: account file thp pages

A firm NAK on this one, for a simple reason: we have enough times
regretted mixing up the swap-backed Shmem into File stats; so although
you intend to support huge file pages by the same mechanism later,
the ones you support in this patchset are Shmem: ShmemPmdMapped is
the name I was using.

07/25 thp, vmstats: add counters for huge file pages
08/25 thp: support file pages in zap_huge_pmd()

I see there's an ongoing discussion with Aneesh about whether you
need to deposit/withdraw pagetables.  I blindly assumed that I would
have to participate in that mechanism, so my series should be safe
in that regard, though not as nice as yours.  (But I did get the
sequence needed for PowerPC wrong somewhere in my original posting.)

09/25 thp: handle file pages in split_huge_pmd()

That's neat, just unmapping the pmd for the shared file/shmem case,
I didn't think to do it that way.  Or did I have some other reason
for converting the pmd to ptes?  I think it is incorrect not to
provide ptes when splitting when it's an mlock()ed area; but
you've said yourself that mlock probably needs more work.

10/25 thp: handle file COW faults

We made the same decision (and Matthew for DAX too IIRC): don't
bother to COW hugely, it's more code, and often not beneficial.

11/25 thp: handle file pages in mremap()

Right, I missed that from my v3.19 rebase, but added it later
when I woke up to your v3.15 mod: I prefer my version of this patch,
which adds take_rmap_locks(vma) and drop_rmap_locks(vma) helpers.

12/25 thp: skip file huge pmd on copy_huge_pmd()

Oh, that looks good, I'd forgotten about that optimization.
And I think in this case you don't have to worry about VM_LOCKED
or deposit/withdraw, it's just fine to skip over it.  I think I'll
post my one as it is, and let you scoff at it then.

13/25 thp: prepare change_huge_pmd() for file thp

You're very dedicated to inserting the vma_is_anonymous(),
where I just delete the BUG_ON.  Good for you.

14/25 thp: run vma_adjust_trans_huge() outside i_mmap_rwsem

Interesting.  I haven't spent long enough looking at this, just noted
it as something to come back to: I haven't decided whether this is a
good safe obvious change (always better to do something outside the
lock if it's safe), or not.  I don't have that issue of munlock()ing
when splitting, so hadn't noticed the peculiar placing of
vma_adjust_trans_huge() inside the one lock but outside the other.

15/25 thp: file pages support for split_huge_page()

I didn't go much beyond your very helpful commit comment on this one.
I'd been worried by how you would freeze for the split, but it sounds
right.  Very interested to see your comment about dropping pages beyond
i_size from the radix-tree: I think that's absolutely right, but never
occurred to me to do - not something that tests or users notice, but
needed for correctness.

16/25 thp, mlock: do not mlock PTE-mapped file huge pages

I think we both wish there were no mlock(), it gave me more trouble
than anything.  And it wouldn't be a surprise if what I have and
think works, actually turns out to be very broken.  So I'm going to
be careful about criticising your workarounds, they might be better.

17/25 vmscan: split file huge pages before paging them out

As mentioned at the beginning, seemed to work well for the straight
cp, but not when the huge pages were mapped.

18/25 page-flags: relax policy for PG_mappedtodisk and PG_reclaim
19/25 radix-tree: implement radix_tree_maybe_preload_order()
20/25 filemap: prepare find and delete operations for huge pages
21/25 truncate: handle file thp
22/25 shmem: prepare huge= mount option and sysfs knob
23/25 shmem: get_unmapped_area align huge page
24/25 shmem: add huge pages support
25/25 shmem, thp: respect MADV_{NO,}HUGEPAGE for file mappings

Not much to say on these at the moment, but clearly we'll need to
settle the user interfaces of 22/25 and 25/25 before anything reaches
Linus.  For simplicity, I've carried on with my distinct mount options
and knobs, but agree that some rapprochement with anon THP conventions
will probably be required before finalizing.

That's all for now - thanks,
Hugh

[toc] | [next] | [standalone]

#1364018

On Wed, Mar 23, 2016 at 01:09:05PM -0700, Hugh Dickins wrote:
> The small files thing formed my first impression.  My second
> impression was similar, when I tried mmap(NULL, size_of_RAM,
> PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0) and
> cycled around the arena touching all the pages (which of
> course has to push a little into swap): that soon OOMed.
> 
> But there I think you probably just have some minor bug to be fixed:
> I spent a little while trying to debug it, but then decided I'd
> better get back to writing to you.  I didn't really understand what
> I was seeing, but when I hacked some stats into shrink_page_list(),
> converting !is_page_cache_freeable(page) to page_cache_references(page)
> to return the difference instead of the bool, a large proportion of
> huge tmpfs pages seemed to have count 1 too high to be freeable at
> that point (and one huge tmpfs page had a count of 3477).

I'll reply to your other points later, but first I wanted to address this
obvious bug.

I cannot really explain page_count() == 3477, but otherwise:

The root cause is that try_to_unmap() doesn't handle PMD-mapped huge
pages, so we hit 'case SWAP_AGAIN' all the time.

The patch below effectively rewrites 17/25: now we split the huge page
before trying to unmap it.

split_huge_page() has its own check similar to is_page_cache_freeable(),
so we woundn't split pages we cannot free later on.

And split_huge_page() for file pages would unmap the page, so we wouldn't
need to go to try_to_unmap() after that.

The patch look rather simple, but I haven't done full validation cycle for
it. Regressions are unlikely, but possible.

At some point we would need to teach try_to_unmap() to handle huge pages.
It would be required for filesystems with backing storage. But I don't see
need for it to get huge tmpfs/shmem work.

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 9fa9e15594e9..86008f8f1f9b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -473,14 +473,12 @@ void drop_slab(void)
 
 static inline int is_page_cache_freeable(struct page *page)
 {
-	int radix_tree_pins = PageTransHuge(page) ? HPAGE_PMD_NR : 1;
-
 	/*
 	 * A freeable page cache page is referenced only by the caller
 	 * that isolated the page, the page cache radix tree and
 	 * optional buffer heads at page->private.
 	 */
-	return page_count(page) - page_has_private(page) == 1 + radix_tree_pins;
+	return page_count(page) - page_has_private(page) == 2;
 }
 
 static int may_write_to_inode(struct inode *inode, struct scan_control *sc)
@@ -550,6 +548,8 @@ static pageout_t pageout(struct page *page, struct address_space *mapping,
 	 * swap_backing_dev_info is bust: it doesn't reflect the
 	 * congestion state of the swapdevs.  Easy to fix, if needed.
 	 */
+	if (!is_page_cache_freeable(page))
+		return PAGE_KEEP;
 	if (!mapping) {
 		/*
 		 * Some data journaling orphaned pages can have
@@ -1055,8 +1055,14 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 
 			/* Adding to swap updated mapping */
 			mapping = page_mapping(page);
+		} else if (unlikely(PageTransHuge(page))) {
+			/* Split file THP */
+			if (split_huge_page_to_list(page, page_list))
+				goto keep_locked;
 		}
 
+		VM_BUG_ON_PAGE(PageTransHuge(page), page);
+
 		/*
 		 * The page is mapped into the page tables of one or more
 		 * processes. Try to unmap it here.
@@ -1112,15 +1118,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 			 * starts and then write it out here.
 			 */
 			try_to_unmap_flush_dirty();
-
-			if (!is_page_cache_freeable(page))
-				goto keep_locked;
-
-			if (unlikely(PageTransHuge(page))) {
-				if (split_huge_page_to_list(page, page_list))
-					goto keep_locked;
-			}
-
 			switch (pageout(page, mapping, sc)) {
 			case PAGE_KEEP:
 				goto keep_locked;
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1364406

On Thu, 24 Mar 2016, Kirill A. Shutemov wrote:
> On Wed, Mar 23, 2016 at 01:09:05PM -0700, Hugh Dickins wrote:
> > The small files thing formed my first impression.  My second
> > impression was similar, when I tried mmap(NULL, size_of_RAM,
> > PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0) and
> > cycled around the arena touching all the pages (which of
> > course has to push a little into swap): that soon OOMed.
> > 
> > But there I think you probably just have some minor bug to be fixed:
> > I spent a little while trying to debug it, but then decided I'd
> > better get back to writing to you.  I didn't really understand what
> > I was seeing, but when I hacked some stats into shrink_page_list(),
> > converting !is_page_cache_freeable(page) to page_cache_references(page)
> > to return the difference instead of the bool, a large proportion of
> > huge tmpfs pages seemed to have count 1 too high to be freeable at
> > that point (and one huge tmpfs page had a count of 3477).
> 
> I'll reply to your other points later, but first I wanted to address this
> obvious bug.

Thanks.  That works better, but is not yet right: memory isn't freed
as it should be, so when I exit then try to run a second time, the
mmap() just gets ENOMEM (with /proc/sys/vm/overcommit_memory 0):
MemFree is low.  No rush to fix, I've other stuff to do.

I don't get as far as that on the laptop, since the first run is OOM
killed while swapping; but I can't vouch for the OOM-kill-correctness
of the base tree I'm using, and this laptop has a history of OOMing
rather too easily if all's not right.

Hugh

[toc] | [prev] | [next] | [standalone]

#1364731

On Thu, Mar 24, 2016 at 12:08:55PM -0700, Hugh Dickins wrote:
> On Thu, 24 Mar 2016, Kirill A. Shutemov wrote:
> > On Wed, Mar 23, 2016 at 01:09:05PM -0700, Hugh Dickins wrote:
> > > The small files thing formed my first impression.  My second
> > > impression was similar, when I tried mmap(NULL, size_of_RAM,
> > > PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0) and
> > > cycled around the arena touching all the pages (which of
> > > course has to push a little into swap): that soon OOMed.
> > > 
> > > But there I think you probably just have some minor bug to be fixed:
> > > I spent a little while trying to debug it, but then decided I'd
> > > better get back to writing to you.  I didn't really understand what
> > > I was seeing, but when I hacked some stats into shrink_page_list(),
> > > converting !is_page_cache_freeable(page) to page_cache_references(page)
> > > to return the difference instead of the bool, a large proportion of
> > > huge tmpfs pages seemed to have count 1 too high to be freeable at
> > > that point (and one huge tmpfs page had a count of 3477).
> > 
> > I'll reply to your other points later, but first I wanted to address this
> > obvious bug.
> 
> Thanks.  That works better, but is not yet right: memory isn't freed
> as it should be, so when I exit then try to run a second time, the
> mmap() just gets ENOMEM (with /proc/sys/vm/overcommit_memory 0):
> MemFree is low.  No rush to fix, I've other stuff to do.
> 
> I don't get as far as that on the laptop, since the first run is OOM
> killed while swapping; but I can't vouch for the OOM-kill-correctness
> of the base tree I'm using, and this laptop has a history of OOMing
> rather too easily if all's not right.

Hm. I don't see the issue.

I tried to reproduce it in my VM with following script:

#!/bin/sh -efu

swapon -a

ram="$(grep MemTotal /proc/meminfo | sed 's,[^0-9\]\+,,; s, kB,k,')"

usemem -w -f /dev/zero "$ram"

swapoff -a
swapon -a

usemem -w -f /dev/zero "$ram"

cat /proc/meminfo
grep thp /proc/vmstat

-----

usemem is a tool from this archive:

http://www.spinics.net/lists/linux-mm/attachments/gtarazbJaHPaAT.gtar

It works fine even if would double size of mapping.

Do you have a reproducer?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1364883

On Fri, 25 Mar 2016, Kirill A. Shutemov wrote:
> On Thu, Mar 24, 2016 at 12:08:55PM -0700, Hugh Dickins wrote:
> > On Thu, 24 Mar 2016, Kirill A. Shutemov wrote:
> > > On Wed, Mar 23, 2016 at 01:09:05PM -0700, Hugh Dickins wrote:
> > > > The small files thing formed my first impression.  My second
> > > > impression was similar, when I tried mmap(NULL, size_of_RAM,
> > > > PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0) and
> > > > cycled around the arena touching all the pages (which of
> > > > course has to push a little into swap): that soon OOMed.
> > > > 
> > > > But there I think you probably just have some minor bug to be fixed:
> > > > I spent a little while trying to debug it, but then decided I'd
> > > > better get back to writing to you.  I didn't really understand what
> > > > I was seeing, but when I hacked some stats into shrink_page_list(),
> > > > converting !is_page_cache_freeable(page) to page_cache_references(page)
> > > > to return the difference instead of the bool, a large proportion of
> > > > huge tmpfs pages seemed to have count 1 too high to be freeable at
> > > > that point (and one huge tmpfs page had a count of 3477).
> > > 
> > > I'll reply to your other points later, but first I wanted to address this
> > > obvious bug.
> > 
> > Thanks.  That works better, but is not yet right: memory isn't freed
> > as it should be, so when I exit then try to run a second time, the
> > mmap() just gets ENOMEM (with /proc/sys/vm/overcommit_memory 0):
> > MemFree is low.  No rush to fix, I've other stuff to do.
> > 
> > I don't get as far as that on the laptop, since the first run is OOM
> > killed while swapping; but I can't vouch for the OOM-kill-correctness
> > of the base tree I'm using, and this laptop has a history of OOMing
> > rather too easily if all's not right.
> 
> Hm. I don't see the issue.
> 
> I tried to reproduce it in my VM with following script:
> 
> #!/bin/sh -efu
> 
> swapon -a
> 
> ram="$(grep MemTotal /proc/meminfo | sed 's,[^0-9\]\+,,; s, kB,k,')"
> 
> usemem -w -f /dev/zero "$ram"
> 
> swapoff -a
> swapon -a
> 
> usemem -w -f /dev/zero "$ram"
> 
> cat /proc/meminfo
> grep thp /proc/vmstat
> 
> -----
> 
> usemem is a tool from this archive:
> 
> http://www.spinics.net/lists/linux-mm/attachments/gtarazbJaHPaAT.gtar
> 
> It works fine even if would double size of mapping.
> 
> Do you have a reproducer?

Yes, my reproducer is simpler (just cycling twice around the arena,
touching each page in order); and I too did not see it running your
script using usemem above.  It looks as if that invocation isn't doing
enough work with swap: if I add a "-r 2" to those usemem lines, then
I get "usemem: mmap failed: Cannot allocate memory" on the second.

I also added a "sleep 2" before the second call to usemem: I'm not sure
of the current state of vmstat, but historically it's slow to gather
back from each cpu to global, and I think it used to leave some cpu
counts stranded indefinitely once upon a time.  In my own testing,
I have a /proc/sys/vm/stat_refresh to touch before checking meminfo
or vmstat - and I think the vm_enough_memory() check in mmap() may
need that same care, since it refers to NR_FREE_PAGES etc.

8GB is my ramsize, if that matters.

Hugh

[toc] | [prev] | [next] | [standalone]

#1365615

On Fri, Mar 25, 2016 at 05:00:50PM -0700, Hugh Dickins wrote:
> On Fri, 25 Mar 2016, Kirill A. Shutemov wrote:
> > On Thu, Mar 24, 2016 at 12:08:55PM -0700, Hugh Dickins wrote:
> > > On Thu, 24 Mar 2016, Kirill A. Shutemov wrote:
> > > > On Wed, Mar 23, 2016 at 01:09:05PM -0700, Hugh Dickins wrote:
> > > > > The small files thing formed my first impression.  My second
> > > > > impression was similar, when I tried mmap(NULL, size_of_RAM,
> > > > > PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0) and
> > > > > cycled around the arena touching all the pages (which of
> > > > > course has to push a little into swap): that soon OOMed.
> > > > > 
> > > > > But there I think you probably just have some minor bug to be fixed:
> > > > > I spent a little while trying to debug it, but then decided I'd
> > > > > better get back to writing to you.  I didn't really understand what
> > > > > I was seeing, but when I hacked some stats into shrink_page_list(),
> > > > > converting !is_page_cache_freeable(page) to page_cache_references(page)
> > > > > to return the difference instead of the bool, a large proportion of
> > > > > huge tmpfs pages seemed to have count 1 too high to be freeable at
> > > > > that point (and one huge tmpfs page had a count of 3477).
> > > > 
> > > > I'll reply to your other points later, but first I wanted to address this
> > > > obvious bug.
> > > 
> > > Thanks.  That works better, but is not yet right: memory isn't freed
> > > as it should be, so when I exit then try to run a second time, the
> > > mmap() just gets ENOMEM (with /proc/sys/vm/overcommit_memory 0):
> > > MemFree is low.  No rush to fix, I've other stuff to do.
> > > 
> > > I don't get as far as that on the laptop, since the first run is OOM
> > > killed while swapping; but I can't vouch for the OOM-kill-correctness
> > > of the base tree I'm using, and this laptop has a history of OOMing
> > > rather too easily if all's not right.
> > 
> > Hm. I don't see the issue.
> > 
> > I tried to reproduce it in my VM with following script:
> > 
> > #!/bin/sh -efu
> > 
> > swapon -a
> > 
> > ram="$(grep MemTotal /proc/meminfo | sed 's,[^0-9\]\+,,; s, kB,k,')"
> > 
> > usemem -w -f /dev/zero "$ram"
> > 
> > swapoff -a
> > swapon -a
> > 
> > usemem -w -f /dev/zero "$ram"
> > 
> > cat /proc/meminfo
> > grep thp /proc/vmstat
> > 
> > -----
> > 
> > usemem is a tool from this archive:
> > 
> > http://www.spinics.net/lists/linux-mm/attachments/gtarazbJaHPaAT.gtar
> > 
> > It works fine even if would double size of mapping.
> > 
> > Do you have a reproducer?
> 
> Yes, my reproducer is simpler (just cycling twice around the arena,
> touching each page in order); and I too did not see it running your
> script using usemem above.  It looks as if that invocation isn't doing
> enough work with swap: if I add a "-r 2" to those usemem lines, then
> I get "usemem: mmap failed: Cannot allocate memory" on the second.
> 
> I also added a "sleep 2" before the second call to usemem: I'm not sure
> of the current state of vmstat, but historically it's slow to gather
> back from each cpu to global, and I think it used to leave some cpu
> counts stranded indefinitely once upon a time.  In my own testing,
> I have a /proc/sys/vm/stat_refresh to touch before checking meminfo
> or vmstat - and I think the vm_enough_memory() check in mmap() may
> need that same care, since it refers to NR_FREE_PAGES etc.
> 
> 8GB is my ramsize, if that matters.

I think I found it. I have refcounting screwed up in faultaround.

This should fix the problem:

diff --git a/mm/filemap.c b/mm/filemap.c
index 94c097ec08e7..1325bb4568d1 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2292,19 +2292,18 @@ repeat:
 		if (fe->pte)
 			fe->pte += iter.index - last_pgoff;
 		last_pgoff = iter.index;
-		alloc_set_pte(fe, NULL, page);
+		if (alloc_set_pte(fe, NULL, page))
+			goto unlock;
 		unlock_page(page);
-		/* Huge page is mapped? No need to proceed. */
-		if (pmd_trans_huge(*fe->pmd))
-			break;
-		/* Failed to setup page table? */
-		VM_BUG_ON(!fe->pte);
 		goto next;
 unlock:
 		unlock_page(page);
 skip:
 		page_cache_release(page);
 next:
+		/* Huge page is mapped? No need to proceed. */
+		if (pmd_trans_huge(*fe->pmd))
+			break;
 		if (iter.index == end_pgoff)
 			break;
 	}
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1365632

On Mon, 28 Mar 2016, Kirill A. Shutemov wrote:
> 
> I think I found it. I have refcounting screwed up in faultaround.
> 
> This should fix the problem:

Yes, this fixes it for me - thanks.

Hugh

> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 94c097ec08e7..1325bb4568d1 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2292,19 +2292,18 @@ repeat:
>  		if (fe->pte)
>  			fe->pte += iter.index - last_pgoff;
>  		last_pgoff = iter.index;
> -		alloc_set_pte(fe, NULL, page);
> +		if (alloc_set_pte(fe, NULL, page))
> +			goto unlock;
>  		unlock_page(page);
> -		/* Huge page is mapped? No need to proceed. */
> -		if (pmd_trans_huge(*fe->pmd))
> -			break;
> -		/* Failed to setup page table? */
> -		VM_BUG_ON(!fe->pte);
>  		goto next;
>  unlock:
>  		unlock_page(page);
>  skip:
>  		page_cache_release(page);
>  next:
> +		/* Huge page is mapped? No need to proceed. */
> +		if (pmd_trans_huge(*fe->pmd))
> +			break;
>  		if (iter.index == end_pgoff)
>  			break;
>  	}
> -- 
>  Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1365465

On Wed, Mar 23, 2016 at 01:09:05PM -0700, Hugh Dickins wrote:
> On Sat, 12 Mar 2016, Kirill A. Shutemov wrote:
...
> As I've said on several occasions, I am not interested in emulating
> the limitations of hugetlbfs inside tmpfs: there might one day be a
> case for such a project, but it's transparent hugepages that we want to
> have now - blocksize 4kB, but in 2MB extents when possible (on x86_64).
> 
> I accept that supporting small files correctly is not the #1
> requirement for a "huge" tmpfs; and if it were impossible or
> unreasonable to ask for, I'd give up on it.  But since we (Google)
> have been using a successful implementation for a year now, I see
> no reason to give up on it at all: it allows for much wider and
> easier adoption (and testing) of huge tmpfs - without "but"s.

I think, once we get collapse work, huge=within_size would provide
reasonable support for fs with mixed-sized files.

Yes, we still would have overhead on sparsed or punch-holed files.
As for now, I don't see it being show-stopper. The hugepage allocation
policy can be overwritten with MADV/FADV_NOHUGEPAGE if an application
don't want to see huge pages in a particular case.

...

> Hmm, I just went to repeat that test, to see if MemFree goes down and
> down on each run, but got a VM_BUG_ON_PAGE(page_ref_count(page) == 0)
> in put_page_testzero() in find_get_entries() during shmem_evict_inode().
> So, something not quite right with the refcounting when under pressure.

I suspect some race with split_huge_page(), but don't see it so far.

> My third impression was much better, when I just did a straight
> cp /dev/zero /tmpfs_twice_size_of_RAM: that went smoothly,
> pushed into swap just as it should, nice.
> 
> > 
> > The main difference with Hugh's approach[1] is that I continue with
> > compound pages, where Hugh invents new way couple pages: team pages.
> > I believe THP refcounting rework made team pages unnecessary: compound
> > page are flexible enough to serve needs of page cache.
> 
> I have disagreed with you about the suitability of compound pages;
> but at some point on Sunday night, after reading your patchset,
> found myself looking at it differently, and now agreeing with you.
> 
> They are not flexible enough yet, but I believe you have done a large
> part of the work (by diverting all those tail ops to the head), and it
> just needs a little more.  I say "a little", but it might not be easy.
> 
> What I realize now, is that you should be able to do almost everything
> I did with team pages, instead with your compound pages.  Just abandon
> the idea that the whole compound page has to be initialized in one go:
> initialize (charge to memcg, add to page cache, fill, SetPageUptodate)
> each subpage as it is touched.  (But of course, the whole extent has
> to be to be initialized before it can be pmd-mapped.)

I'm not convinced, that doing such operations on per-4k is better.
All-at-once is simpler and has natural performance benefit from batching.

I think we can switch some operations to per-4k basis once upside will be
obvious. It's more on optimization side.

> And the convenient thing about a compound page is that you have plenty
> of space to hold your metadata: where I have been squeezing everything
> into page->private (which makes it difficult then to extend the same
> design to miscellaneous filesystems), you're free to use (certain)
> fields of 511 more struct pages.
> 
> I said above that you should be able to do almost everything: I think
> that "almost" involves dropping a few things that I have, that we can
> well do without.  With team pages, it is possible for each member to
> be charged to a different memcg; but I don't have anyone calling out
> for that behaviour, it's more an awkward possibility that we have to
> be on guard against in a few places, and I'd be quite happy just to
> stop it (charging instead to whichever memcg first touched the extent).
> 
> And it's been nice to entertain the idea of the individual team tails,
> travelling separately up and down their own lrus, subject to their own
> pagesize access patterns even while the whole is pmd-mapped.  But in
> practice, we continue to throw them on the unevictable list once they
> get in the way, with no attempt to bring them back when accessed
> individually; so I think it's a nice idea that we can live without.
> 
> And a compound page implementation would avoid the overhead I have,
> of putting all those tails on lru in the first place.

Exactly. LRU scanning is one of places where this kind of batching is
beneficial.

> Another thing I did, that I think you can safely fail to emulate:
> I was careful to allow sparse population, and not insist that the
> head page be instantiated.  That might turn out to be more of a
> problem for compound pages, and has been an awkward edge case for me.
> Although in principle there are sparse files which would be doubled
> in charge by insisting that the head be instantiated, I doubt they
> will hurt anyone in practice: perhaps best treated as a bug to be
> revisited if it actually comes up as an issue later.

Agreed.

> (Thinking about that last paragraph later, I'm not so sure.
> Forcing the instantiation of the head is okay when first allocating,
> but when trying to recover a fragmented extent into a hugepage later,
> it might prove to be seriously limiting to require that the head be
> migrated into place before anything else is done.)

Hm. I think you imply some implementation details of your recovering
process here, don't you?

> > 
> > Many ideas and some patches were stolen from Hugh's patchset. Having this
> > patchset around was very helpful.
> 
> People are probably wondering why I didn't repost it, and whether
> it's vapourware.  No, it's worked out very well, better than we were
> expecting; and several of the TODOs that I felt needed attention,
> have not yet caused anyone trouble - though wider adoption is sure
> to meet new usecases and new bugs.  I didn't repost it because, as I
> said at the time, I felt it wasn't ready for real use until recovery
> of huge pages after fragmentation and swapping (what khugepaged does
> for anon THP) was added.  After that went in (and then quite a period
> of fixing the bugs, I admit!), other priorities intervened until now.
> Your patchset also lacks recovery at present, so equally that's a
> reason why I don't consider yours ready for merging.

I hoped we will be able to address this later, after initial merge, but
since you insist...

I have started looking into collapsing. I have week or so catch you up on
this. Will see what I'll be able to achieve...

Not having your repair implementation public is a bummer -- I miss
opportunity to rip off your ideas. :-P

> I'll repost mine, including recovery and a few minor additions, against
> v4.6-rc2.  I still haven't got around to doing any NUMA work on it, but
> it does appear to be useful to many people even without that.  I'd have
> preferred to spend longer rebasing it, it may turn out to be a little
> raw: most of the rebases from one release to another have been easy,
> but to v4.5 was messier - not particularly due to your THP refcounting
> work going in, just various different conflicts that I feel I've not
> properly digested yet.  But v4.6-rc2 looks about as early and as late
> as I can do it, with LSF/MM coming up later in April; and should be a
> good base for people to try it out on.  (I'll work on preparing it in
> advance, then I guess a last-minute edit of PAGE_CACHEs before posting.)
> 
> I now believe that the right way forward is to get that team page
> implementation in as soon as we can, so users have the functionality;
> and then (if you're still interested) you work on converting it over
> from teams to compound pages, both as an optimization, and to satisfy
> your understandable preference for a single way of managing huge pages.

That might work. But major switch of implementation is always risky.
Minor inconsistency in behaviour after switch may be considered as
regression. So we need to be careful.

> (I would say "How does that sound to you?" but I'd prefer you to think
> it over, maybe wait to see what the new team patchset looks like.  Though
> aside from the addition of recovery, it is very much like the one against
> v3.19 from February last year: no changes to the design at all, which
> turned out to fit recovery well; but quite a few bug fixes folded back
> in.  Or you may prefer to wait to discuss the way forward at LSF/MM.)
> 
> > 
> > I will continue with code validation. I would expect mlock require some
> > more attention.
> > 
> > Please, review and test the code.
> > 
> > Git tree:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/kas/linux.git hugetmpfs/v4
> > 
> > Rebased onto linux-next:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/kas/linux.git hugetmpfs/v4-next
> 
> It wasn't obvious to me what tree your posted patchset was based on,
> so I made a patch out of the top your hugetmpfs/v4-next branch, and
> applied that on top of Andrew's 4.5-rc7-mm1: which appeared to be
> fine, just a trivial clash with mmotm's
> "do_shared_fault(): check that mmap_sem is held".

It was based on mhocko/mm.git since-4.4.

> 
> [...]
> 
> Some random thoughts on your patches FWIW:
> 
> 01/25 mm: do not pass mm_struct into handle_mm_fault
> 02/25 mm: introduce fault_env
> 
> These two inflate your patchset considerably, with seemingly unrelated
> changes: the first with arch files otherwise untouched, the second with
> lots of change to memory.c that makes it harder to compare our patchsets.
> 
> I think it would be better to leave the handle_mm_fault(mm) removal to
> another patchset, and I'm ambivalent about the struct fault_env changes:
> I prefer to avoid such argument blocks (and think I saw Linus once say
> he detested my zap_details), which do make it harder to see what the
> real arguments are.  I wonder what your 03/25 would look like without
> the fault_env, perhaps I'd agree it's too cumbersome that way (and
> perhaps I should look back at earlier versions of your patchset).
> 
> But I do understand your desire to get some cleanups out of the way
> before basing the real work on the new structure: I'm going to have
> to reconsider the initial patches of my series in the light of this
> criticism of yours, not sure yet if I'll try to cut them out or not.

On my side, I would prefer to keep them unless it's total no-go.
Cutting them out would be painful.

> 03/25 mm: postpone page table allocation until we have page to map
> 
> I've not stopped to consider whether your prealloc_pte work (which
> perhaps was the trigger for fault_env) meets the case or not, but
> want to mention that we recently discovered a drawback to my delaying
> the pagetable allocation: I'm allocating the pagetable while holding
> the lock on the page from the filesystem (any filesystem, though the
> change was to suit tmpfs), which presented a deadlock to the OOM killer.
> The base kernel involved was not a recent one, and there have been
> rewrites in the OOMing area since, so I'm not sure whether it would
> be a deadlock today: but I am sure that it would be better to do the
> allocation while not holding the page lock.  We have a fix in place,
> but I don't think it's quite the right fix, so I want to revisit it:
> unless I get a gift of time from somewhere, I'll post my series with
> the ordering here just as I had it before.

prealloc_pte was invented to avoid allocation under ptl in faultaround
case. But I think we can re-use it to pre-allocate page table before
->fault request.

I wounder why it's a deadlock for OOM. Shoudn't oom-killer use trylock and
bail out if it fails?

> You remark on yours: "This way we can postpone allocation even in
> faultaround case without moving do_fault_around() after __do_fault()."
> Was there something wrong or inefficient with my moving do_fault_around()
> after __do_fault()?

For hot page cache case we can avoid ->fault() call if do_fault_around()
managed to solve the fault. It also means we don't need to lock the page
again and take ptl.

> I thought it was fine; but I did also think when
> I made the change, that it would be fixing a bug in VM_FAULT_MAJOR
> acccounting too; yet when I went to confirm that, couldn't find evidence
> for such a bug before; but I moved on before getting to the bottom of it. 

Hm. do_fault_around() is not able to solve major page fault by design.
Or am I missing something?

> 04/25 rmap: support file thp
> 05/25 mm: introduce do_set_pmd()
> 06/25 mm, rmap: account file thp pages
> 
> A firm NAK on this one, for a simple reason: we have enough times
> regretted mixing up the swap-backed Shmem into File stats; so although
> you intend to support huge file pages by the same mechanism later,
> the ones you support in this patchset are Shmem: ShmemPmdMapped is
> the name I was using.

Okay. I'll rework that.
> 
> 07/25 thp, vmstats: add counters for huge file pages
> 08/25 thp: support file pages in zap_huge_pmd()
> 
> I see there's an ongoing discussion with Aneesh about whether you
> need to deposit/withdraw pagetables.  I blindly assumed that I would
> have to participate in that mechanism, so my series should be safe
> in that regard, though not as nice as yours.  (But I did get the
> sequence needed for PowerPC wrong somewhere in my original posting.)
> 
> 09/25 thp: handle file pages in split_huge_pmd()
> 
> That's neat, just unmapping the pmd for the shared file/shmem case,
> I didn't think to do it that way.  Or did I have some other reason
> for converting the pmd to ptes?  I think it is incorrect not to
> provide ptes when splitting when it's an mlock()ed area; but
> you've said yourself that mlock probably needs more work.

Since we don't deposit page tables for file mapping, there's no easy way
to provide ptes for mlocked() areas. DAX has the same problem too.

Special-cased deposit for VM_LOCKED VMAs is going to be ugly and prune to
leaks.

Always-deposit is to wasteful especially for very large mappings (consider
DAX).

/me hates mlock.

> 10/25 thp: handle file COW faults
> 
> We made the same decision (and Matthew for DAX too IIRC): don't
> bother to COW hugely, it's more code, and often not beneficial.

Actually, one small topic I wanted to discuss on LSF/MM is if we want to
do the same for anon-THP. New refcounting allows that.

Although, it probably would require some work on khugepaged side to allow
COW-break of PTE-mapped THP to collapse.

> 11/25 thp: handle file pages in mremap()
> 
> Right, I missed that from my v3.19 rebase, but added it later
> when I woke up to your v3.15 mod: I prefer my version of this patch,
> which adds take_rmap_locks(vma) and drop_rmap_locks(vma) helpers.

Okay, I'll check it out once it will be public.

> 12/25 thp: skip file huge pmd on copy_huge_pmd()
> 
> Oh, that looks good, I'd forgotten about that optimization.
> And I think in this case you don't have to worry about VM_LOCKED
> or deposit/withdraw, it's just fine to skip over it.  I think I'll
> post my one as it is, and let you scoff at it then.
> 
> 13/25 thp: prepare change_huge_pmd() for file thp
> 
> You're very dedicated to inserting the vma_is_anonymous(),
> where I just delete the BUG_ON.  Good for you.

Dave suggested this. :)

> 14/25 thp: run vma_adjust_trans_huge() outside i_mmap_rwsem
> 
> Interesting.  I haven't spent long enough looking at this, just noted
> it as something to come back to: I haven't decided whether this is a
> good safe obvious change (always better to do something outside the
> lock if it's safe), or not.  I don't have that issue of munlock()ing
> when splitting, so hadn't noticed the peculiar placing of
> vma_adjust_trans_huge() inside the one lock but outside the other.

We had the same issue back on my first attempt of huge page cache. Before
refcounting rework vma_adjust_trans_huge() implied split_huge_page() which
does rmap walk.

> 15/25 thp: file pages support for split_huge_page()
> 
> I didn't go much beyond your very helpful commit comment on this one.
> I'd been worried by how you would freeze for the split, but it sounds
> right.  Very interested to see your comment about dropping pages beyond
> i_size from the radix-tree: I think that's absolutely right, but never
> occurred to me to do - not something that tests or users notice, but
> needed for correctness.
> 
> 16/25 thp, mlock: do not mlock PTE-mapped file huge pages
> 
> I think we both wish there were no mlock(), it gave me more trouble
> than anything.  And it wouldn't be a surprise if what I have and
> think works, actually turns out to be very broken.  So I'm going to
> be careful about criticising your workarounds, they might be better.
> 
> 17/25 vmscan: split file huge pages before paging them out
> 
> As mentioned at the beginning, seemed to work well for the straight
> cp, but not when the huge pages were mapped.
> 
> 18/25 page-flags: relax policy for PG_mappedtodisk and PG_reclaim
> 19/25 radix-tree: implement radix_tree_maybe_preload_order()
> 20/25 filemap: prepare find and delete operations for huge pages
> 21/25 truncate: handle file thp
> 22/25 shmem: prepare huge= mount option and sysfs knob
> 23/25 shmem: get_unmapped_area align huge page
> 24/25 shmem: add huge pages support
> 25/25 shmem, thp: respect MADV_{NO,}HUGEPAGE for file mappings
> 
> Not much to say on these at the moment, but clearly we'll need to
> settle the user interfaces of 22/25 and 25/25 before anything reaches
> Linus.  For simplicity, I've carried on with my distinct mount options
> and knobs, but agree that some rapprochement with anon THP conventions
> will probably be required before finalizing.
> 
> That's all for now - thanks,
> Hugh
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web