* Re: [v3 0/9] parallelized "struct page" zeroing
From: Michal Hocko @ 2017-05-11 8:05 UTC (permalink / raw)
To: David Miller
Cc: pasha.tatashin, linux-kernel, sparclinux, linux-mm, linuxppc-dev,
linux-s390, borntraeger, heiko.carstens
In-Reply-To: <20170510.111943.1940354761418085760.davem@davemloft.net>
On Wed 10-05-17 11:19:43, David S. Miller wrote:
> From: Michal Hocko <mhocko@kernel.org>
> Date: Wed, 10 May 2017 16:57:26 +0200
>
> > Have you measured that? I do not think it would be super hard to
> > measure. I would be quite surprised if this added much if anything at
> > all as the whole struct page should be in the cache line already. We do
> > set reference count and other struct members. Almost nobody should be
> > looking at our page at this time and stealing the cache line. On the
> > other hand a large memcpy will basically wipe everything away from the
> > cpu cache. Or am I missing something?
>
> I guess it might be clearer if you understand what the block
> initializing stores do on sparc64. There are no memory accesses at
> all.
>
> The cpu just zeros out the cache line, that's it.
>
> No L3 cache line is allocated. So this "wipe everything" behavior
> will not happen in the L3.
OK, good to know. My undestanding of sparc64 is close to zero.
Anyway, do you agree that doing the struct page initialization along
with other writes to it shouldn't add a measurable overhead comparing
to pre-zeroing of larger block of struct pages? We already have an
exclusive cache line and doing one 64B write along with few other stores
should be basically the same.
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply
* Re: [PATCH v2] mm: vmscan: scan until it founds eligible pages
From: Michal Hocko @ 2017-05-11 7:20 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-kernel, linux-mm, Johannes Weiner,
Mel Gorman, kernel-team
In-Reply-To: <1494457232-27401-1-git-send-email-minchan@kernel.org>
On Thu 11-05-17 08:00:32, Minchan Kim wrote:
> Although there are a ton of free swap and anonymous LRU page
> in elgible zones, OOM happened.
>
> balloon invoked oom-killer: gfp_mask=0x17080c0(GFP_KERNEL_ACCOUNT|__GFP_ZERO|__GFP_NOTRACK), nodemask=(null), order=0, oom_score_adj=0
> CPU: 7 PID: 1138 Comm: balloon Not tainted 4.11.0-rc6-mm1-zram-00289-ge228d67e9677-dirty #17
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
> dump_stack+0x65/0x87
> dump_header.isra.19+0x8f/0x20f
> ? preempt_count_add+0x9e/0xb0
> ? _raw_spin_unlock_irqrestore+0x24/0x40
> oom_kill_process+0x21d/0x3f0
> ? has_capability_noaudit+0x17/0x20
> out_of_memory+0xd8/0x390
> __alloc_pages_slowpath+0xbc1/0xc50
> ? anon_vma_interval_tree_insert+0x84/0x90
> __alloc_pages_nodemask+0x1a5/0x1c0
> pte_alloc_one+0x20/0x50
> __pte_alloc+0x1e/0x110
> __handle_mm_fault+0x919/0x960
> handle_mm_fault+0x77/0x120
> __do_page_fault+0x27a/0x550
> trace_do_page_fault+0x43/0x150
> do_async_page_fault+0x2c/0x90
> async_page_fault+0x28/0x30
> RIP: 0033:0x7fc4636bacb8
> RSP: 002b:00007fff97c9c4c0 EFLAGS: 00010202
> RAX: 00007fc3e818d000 RBX: 00007fc4639f8760 RCX: 00007fc46372e9ca
> RDX: 0000000000101002 RSI: 0000000000101000 RDI: 0000000000000000
> RBP: 0000000000100010 R08: 00000000ffffffff R09: 0000000000000000
> R10: 0000000000000022 R11: 00000000000a3901 R12: 00007fc3e818d010
> R13: 0000000000101000 R14: 00007fc4639f87b8 R15: 00007fc4639f87b8
> Mem-Info:
> active_anon:424716 inactive_anon:65314 isolated_anon:0
> active_file:52 inactive_file:46 isolated_file:0
> unevictable:0 dirty:27 writeback:0 unstable:0
> slab_reclaimable:3967 slab_unreclaimable:4125
> mapped:133 shmem:43 pagetables:1674 bounce:0
> free:4637 free_pcp:225 free_cma:0
> Node 0 active_anon:1698864kB inactive_anon:261256kB active_file:208kB inactive_file:184kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:532kB dirty:108kB writeback:0kB shmem:172kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no
> DMA free:7316kB min:32kB low:44kB high:56kB active_anon:8064kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB slab_reclaimable:464kB slab_unreclaimable:40kB kernel_stack:0kB pagetables:24kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
> lowmem_reserve[]: 0 992 992 1952
> DMA32 free:9088kB min:2048kB low:3064kB high:4080kB active_anon:952176kB inactive_anon:0kB active_file:36kB inactive_file:0kB unevictable:0kB writepending:88kB present:1032192kB managed:1019388kB mlocked:0kB slab_reclaimable:13532kB slab_unreclaimable:16460kB kernel_stack:3552kB pagetables:6672kB bounce:0kB free_pcp:56kB local_pcp:24kB free_cma:0kB
> lowmem_reserve[]: 0 0 0 959
> Movable free:3644kB min:1980kB low:2960kB high:3940kB active_anon:738560kB inactive_anon:261340kB active_file:188kB inactive_file:640kB unevictable:0kB writepending:20kB present:1048444kB managed:1010816kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:832kB local_pcp:60kB free_cma:0kB
> lowmem_reserve[]: 0 0 0 0
> DMA: 1*4kB (E) 0*8kB 18*16kB (E) 10*32kB (E) 10*64kB (E) 9*128kB (ME) 8*256kB (E) 2*512kB (E) 2*1024kB (E) 0*2048kB 0*4096kB = 7524kB
> DMA32: 417*4kB (UMEH) 181*8kB (UMEH) 68*16kB (UMEH) 48*32kB (UMEH) 14*64kB (MH) 3*128kB (M) 1*256kB (H) 1*512kB (M) 2*1024kB (M) 0*2048kB 0*4096kB = 9836kB
> Movable: 1*4kB (M) 1*8kB (M) 1*16kB (M) 1*32kB (M) 0*64kB 1*128kB (M) 2*256kB (M) 4*512kB (M) 1*1024kB (M) 0*2048kB 0*4096kB = 3772kB
> 378 total pagecache pages
> 17 pages in swap cache
> Swap cache stats: add 17325, delete 17302, find 0/27
> Free swap = 978940kB
> Total swap = 1048572kB
> 524157 pages RAM
> 0 pages HighMem/MovableOnly
> 12629 pages reserved
> 0 pages cma reserved
> 0 pages hwpoisoned
> [ pid ] uid tgid total_vm rss nr_ptes nr_pmds swapents oom_score_adj name
> [ 433] 0 433 4904 5 14 3 82 0 upstart-udev-br
> [ 438] 0 438 12371 5 27 3 191 -1000 systemd-udevd
>
> With investigation, skipping page of isolate_lru_pages makes reclaim
> void because it returns zero nr_taken easily so LRU shrinking is
> effectively nothing and just increases priority aggressively.
> Finally, OOM happens.
>
> The problem is that get_scan_count determines nr_to_scan with
> eligible zones so although priority drops to zero, it couldn't
> reclaim any pages if the LRU contains mostly ineligible pages.
>
> get_scan_count:
>
> size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
> size = size >> sc->priority;
>
> Assumes sc->priority is 0 and LRU list is as follows.
>
> N-N-N-N-H-H-H-H-H-H-H-H-H-H-H-H-H-H-H-H
>
> (Ie, small eligible pages are in the head of LRU but others are
> almost ineligible pages)
>
> In that case, size becomes 4 so VM want to scan 4 pages but 4 pages
> from tail of the LRU are not eligible pages.
> If get_scan_count counts skipped pages, it doesn't reclaim any pages
> remained after scanning 4 pages so it ends up OOM happening.
>
> This patch makes isolate_lru_pages try to scan pages until it
> encounters eligible zones's pages.
>
Fixes: 3db65812d688 ("Revert "mm, vmscan: account for skipped pages as a partial scan"")
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> * from v1
> * put more words in description and code
> * drop unncessary pages_skipped list flushing
>
> mm/vmscan.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 5ebf468c5429..e051bf4a1144 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1449,7 +1449,7 @@ static __always_inline void update_lru_sizes(struct lruvec *lruvec,
> *
> * Appropriate locks must be held before calling this function.
> *
> - * @nr_to_scan: The number of pages to look through on the list.
> + * @nr_to_scan: The number of eligible pages to look through on the list.
> * @lruvec: The LRU vector to pull pages from.
> * @dst: The temp list to put pages on to.
> * @nr_scanned: The number of pages that were scanned.
> @@ -1469,11 +1469,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> unsigned long nr_zone_taken[MAX_NR_ZONES] = { 0 };
> unsigned long nr_skipped[MAX_NR_ZONES] = { 0, };
> unsigned long skipped = 0;
> - unsigned long scan, nr_pages;
> + unsigned long scan, total_scan, nr_pages;
> LIST_HEAD(pages_skipped);
>
> - for (scan = 0; scan < nr_to_scan && nr_taken < nr_to_scan &&
> - !list_empty(src); scan++) {
> + for (total_scan = scan = 0; scan < nr_to_scan &&
> + nr_taken < nr_to_scan &&
> + !list_empty(src);
> + total_scan++) {
> struct page *page;
>
> page = lru_to_page(src);
> @@ -1487,6 +1489,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> continue;
> }
>
> + /*
> + * Do not count skipped pages because it makes the function to
> + * return with none isolated pages if the LRU mostly contains
> + * ineligible pages so that VM cannot reclaim any pages and
> + * trigger premature OOM.
> + */
> + scan++;
> switch (__isolate_lru_page(page, mode)) {
> case 0:
> nr_pages = hpage_nr_pages(page);
> @@ -1524,9 +1533,9 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> skipped += nr_skipped[zid];
> }
> }
> - *nr_scanned = scan;
> + *nr_scanned = total_scan;
> trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan,
> - scan, skipped, nr_taken, mode, lru);
> + total_scan, skipped, nr_taken, mode, lru);
> update_lru_sizes(lruvec, lru, nr_zone_taken);
> return nr_taken;
> }
> --
> 2.7.4
>
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply
* Re: [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm
From: Ingo Molnar @ 2017-05-11 7:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Thomas Gleixner, X86 ML, linux-kernel@vger.kernel.org,
Borislav Petkov, Linus Torvalds, Andrew Morton, Mel Gorman,
linux-mm@kvack.org, Rik van Riel, Dave Hansen, Nadav Amit,
Michal Hocko, Arjan van de Ven
In-Reply-To: <CALCETrV-c8n92v040HVw=6OdnNrLvN7ZAcAJ45Xs4wx-7H5r=g@mail.gmail.com>
* Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, May 10, 2017 at 1:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >> On Wed, 10 May 2017, Ingo Molnar wrote:
> >> >
> >> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >
> >> > > On Sun, 7 May 2017, Andy Lutomirski wrote:
> >> > > > /* context.lock is held for us, so we don't need any locking. */
> >> > > > static void flush_ldt(void *current_mm)
> >> > > > {
> >> > > > + struct mm_struct *mm = current_mm;
> >> > > > mm_context_t *pc;
> >> > > >
> >> > > > - if (current->active_mm != current_mm)
> >> > > > + if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> >> > >
> >> > > While functional correct, this really should compare against 'mm'.
> >> > >
> >> > > > return;
> >> > > >
> >> > > > - pc = ¤t->active_mm->context;
> >> > > > + pc = &mm->context;
> >> >
> >> > So this appears to be the function:
> >> >
> >> > static void flush_ldt(void *current_mm)
> >> > {
> >> > struct mm_struct *mm = current_mm;
> >> > mm_context_t *pc;
> >> >
> >> > if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> >> > return;
> >> >
> >> > pc = &mm->context;
> >> > set_ldt(pc->ldt->entries, pc->ldt->size);
> >> > }
> >> >
> >> > why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?
> >>
> >> Because you cannot dereference a void pointer, i.e. &mm->context ....
> >
> > Indeed, doh! The naming totally confused me. The way I'd write it is the canonical
> > form for such callbacks:
> >
> > static void flush_ldt(void *data)
> > {
> > struct mm_struct *mm = data;
> >
> > ... which beyond unconfusing me would probably also have prevented any accidental
> > use of the 'current_mm' callback argument.
> >
> >
>
> void *data and void *info both seem fairly common in the kernel.
Yes, the most common variants are:
triton:~/tip> git grep -E 'void.*\(.*void \*.*' | grep -vE ',|\*\*|;' | cut -d\( -f2- | cut -d\) -f1 | sort | uniq -c | sort -n | tail -10
38 void *args
38 void *p
39 void *ptr
42 void *foo
46 void *context
55 void *addr
69 void *priv
95 void *info
235 void *arg
292 void *data
> How about my personal favorite for non-kernel work, though: void *mm_void? It
> documents what the parameter means and avoids the confusion.
Dunno, and at the risk of painting that shed bright red it reads a bit weird to
me: void pointers are fine and are often primary parameters - the _real_ quality
here is not that it's void, but that's it's an opaque value passed in from a
common callback. Note that sometimes opaque data is 'unsigned long' (such as in
the case of timers), so it's really not the 'void' that matters.
In that sense 'data', 'arg' or 'info' seem the most readable names, as they
clearly express the type opaqueness.
My personal favorite is double underscores prefix, i.e. 'void *__mm', which would
clearly signal that this is something special. But this does not appear to have
been picked up overly widely:
triton:~/tip> git grep -E 'void.*\(.*void \*.*' | grep -vE ',|\*\*|;' | cut -d\( -f2- | cut -d\) -f1 | sort | uniq -c | sort -n | grep __
1 void *__data
1 void *__info
2 void *__dev
2 void *__tdata
2 void *__tve
3 void *__lock
3 void * __user *
3 volatile void *__p
4 void *__map
... but either of these variants is fine to me.
Thanks,
Ingo
--
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>
^ permalink raw reply
* Re: [PATCH -mm -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Minchan Kim @ 2017-05-11 4:31 UTC (permalink / raw)
To: Huang, Ying
Cc: Johannes Weiner, Andrew Morton, linux-mm, linux-kernel,
Andrea Arcangeli, Ebru Akagunduz, Michal Hocko, Tejun Heo,
Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <87h90sb4jq.fsf@yhuang-dev.intel.com>
On Thu, May 11, 2017 at 08:50:01AM +0800, Huang, Ying wrote:
< snip >
> >> > @@ -1125,8 +1125,28 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >> > !PageSwapCache(page)) {
> >> > if (!(sc->gfp_mask & __GFP_IO))
> >> > goto keep_locked;
> >> > - if (!add_to_swap(page, page_list))
> >> > +swap_retry:
> >> > + /*
> >> > + * Retry after split if we fail to allocate
> >> > + * swap space of a THP.
> >> > + */
> >> > + if (!add_to_swap(page)) {
> >> > + if (!PageTransHuge(page) ||
> >> > + split_huge_page_to_list(page, page_list))
> >> > + goto activate_locked;
> >> > + goto swap_retry;
> >> > + }
> >>
> >> This is definitely better.
> >
> > Thanks.
> >
> >>
> >> However, I think it'd be cleaner without the label here:
> >>
> >> if (!add_to_swap(page)) {
> >> if (!PageTransHuge(page))
> >> goto activate_locked;
> >> /* Split THP and swap individual base pages */
> >> if (split_huge_page_to_list(page, page_list))
> >> goto activate_locked;
> >> if (!add_to_swap(page))
> >> goto activate_locked;
> >
> > Yes.
> >
> >> }
> >>
> >> > + /*
> >> > + * Got swap space successfully. But unfortunately,
> >> > + * we don't support a THP page writeout so split it.
> >> > + */
> >> > + if (PageTransHuge(page) &&
> >> > + split_huge_page_to_list(page, page_list)) {
> >> > + delete_from_swap_cache(page);
> >> > goto activate_locked;
> >> > + }
> >>
> >> Pulling this out of add_to_swap() is an improvement for sure. Add an
> >> XXX: before that "we don't support THP writes" comment for good
> >> measure :)
> >
> > Sure.
> >
> > It could be a separate patch which makes add_to_swap clean via
> > removing page_list argument but I hope Huang take/fold it when he
> > resend it because it would be more important with THP swap.
>
> Sure. I will take this patch as one patch of the THP swap series.
> Because the first patch of the THP swap series is a little big, I don't
> think it is a good idea to fold this patch into it. Could you update
> the patch according to Johannes' comments and resend it?
Okay, I will resend this clean-up patch against on yours patch
after finishing this discussion.
Thanks.
--
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>
^ permalink raw reply
* Re: [PATCH v7 0/7] Introduce ZONE_CMA
From: Joonsoo Kim @ 2017-05-11 2:12 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Rik van Riel, Johannes Weiner, mgorman,
Laura Abbott, Minchan Kim, Marek Szyprowski, Michal Nazarewicz,
Aneesh Kumar K . V, Vlastimil Babka, Russell King, Will Deacon,
linux-mm, linux-kernel, kernel-team
In-Reply-To: <20170502133229.GK14593@dhcp22.suse.cz>
Sorry for the late response. I was on a vacation.
On Tue, May 02, 2017 at 03:32:29PM +0200, Michal Hocko wrote:
> On Tue 02-05-17 13:01:32, Joonsoo Kim wrote:
> > On Thu, Apr 27, 2017 at 05:06:36PM +0200, Michal Hocko wrote:
> [...]
> > > I see this point and I agree that using a specific zone might be a
> > > _nicer_ solution in the end but you have to consider another aspects as
> > > well. The main one I am worried about is a long term maintainability.
> > > We are really out of page flags and consuming one for a rather specific
> > > usecase is not good. Look at ZONE_DMA. I am pretty sure that almost
> > > no sane HW needs 16MB zone anymore, yet we have hard time to get rid
> > > of it and so we have that memory laying around unused all the time
> > > and blocking one page flag bit. CMA falls into a similar category
> > > AFAIU. I wouldn't be all that surprised if a future HW will not need CMA
> > > allocations in few years, yet we will have to fight to get rid of it
> > > like we do with ZONE_DMA. And not only that. We will also have to fight
> > > finding page flags for other more general usecases in the meantime.
> >
> > This maintenance problem is inherent. This problem exists even if we
> > uses MIGRATETYPE approach. We cannot remove many hooks for CMA if a
> > future HW will not need CMA allocation in few years. The only
> > difference is that one takes single zone bit only for CMA user and the
> > other approach takes many hooks that we need to take care about it all
> > the time.
>
> And I consider this a big difference. Because while hooks are not nice
> they will affect CMA users (in a sense of bugs/performance etc.). While
> an additional bit consumed will affect potential future and more generic
> features.
Because these hooks are so tricky and are spread on many places,
bugs about these hooks can be made by *non-CMA* user and they hurt
*CMA* user. These hooks could also delay non-CMA user's development speed
since there are many hooks about CMA and understanding how CMA is managed
is rather difficult. I think that this is a big maintenance overhead
not only for CMA user but also for non-CMA user. So, I think that it
can justify additional bit consumed.
>
> [...]
> > > I believe that the overhead in the hot path is not such a big deal. We
> > > have means to make it 0 when CMA is not used by jumplabels. I assume
> > > that the vast majority of systems will not use CMA. Those systems which
> > > use CMA should be able to cope with some slight overhead IMHO.
> >
> > Please don't underestimate number of CMA user. Most of android device
> > uses CMA. So, there would be more devices using CMA than the server
> > not using CMA. They also have a right to experience the best performance.
>
> This is not a fair comparison, though. Android development model is much
> more faster and tend to not care about future maintainability at all. I
> do not know about any android device that would run on a clean vanilla
> kernel because vendors simply do not care enough (or have time) to put
> the code into a proper shape to upstream it. I understand that this
> model might work quite well for rapidly changing and moving mobile or
> IoT segment but it is not the greatest fit to motivate the core kernel
> subsystem development. We are not in the drivers space!
>
> [...]
> > > This looks like a nice clean up. Those ifdefs are ugly as hell. One
> > > could argue that some of that could be cleaned up by simply adding some
> > > helpers (with a jump label to reduce the overhead), though. But is this
> > > really strong enough reason to bring the whole zone in? I am not really
> > > convinced to be honest.
> >
> > Please don't underestimate the benefit of this patchset.
> > I have said that we need *more* hooks to fix all the problems.
> >
> > And, please think that this code removal is not only code removal but
> > also concept removal. With this removing, we don't need to consider
> > ALLOC_CMA for alloc_flags when calling zone_watermark_ok(). There are
> > many bugs on it and it still remains. We don't need to consider
> > pageblock migratetype when handling pageblock migratetype. We don't
> > need to take a great care about calculating the number of CMA
> > freepages.
>
> And all this can be isolated to CMA specific hooks with mostly minimum
> impact to most users. I hear you saying that zone approach is more natural
> and I would agree if we wouldn't have to care about the number of zones.
I attach a solution about one more bit in page flags although I don't
agree with your opinion that additional bit is no-go approach. Just
note that we have already used three bits for zone encoding in some
configuration due to ZONE_DEVICE.
>
> > > [...]
> > >
> > > > > Please do _not_ take this as a NAK from me. At least not at this time. I
> > > > > am still trying to understand all the consequences but my intuition
> > > > > tells me that building on top of highmem like approach will turn out to
> > > > > be problematic in future (as we have already seen with the highmem and
> > > > > movable zones) so this needs a very prudent consideration.
> > > >
> > > > I can understand that you are prudent to this issue. However, it takes more
> > > > than two years and many people already expressed that ZONE approach is the
> > > > way to go.
> > >
> > > I can see a single Acked-by and one Reviewed-by. It would be much more
> > > convincing to see much larger support. Do not take me wrong I am not
> > > trying to undermine the feedback so far but we should be clear about one
> > > thing. CMA is mostly motivated by the industry which tries to overcome
> > > HW limitations which can change in future very easily. I would rather
> > > see good enough solution for something like that than a nicer solution
> > > which is pushing additional burden on more general usecases.
> >
> > First of all, current MIGRATETYPE approach isn't good enough to me.
> > They caused too many problems and there are many remanining problems.
> > It will causes maintenance issue for a long time.
> >
> > And, although good enough solution can be better than nicer solution
> > in some cases, it looks like current situation isn't that case.
> > There is a single reason, saving page flag bit, to support good enough
> > solution.
> >
> > I'd like to ask reversly. Is this a enough reason to make CMA user to
> > suffer from bugs?
>
> No, but I haven't heard any single argument that those bugs are
> impossible to fix with the current approach. They might be harder to fix
> but if I can chose between harder for CMA and harder for other more
> generic HW independent features I will go for the first one. And do not
> take me wrong, I have nothing against CMA as such. It solves a real life
> problem. I just believe it doesn't deserve to consume a new bit in page
> flags because that is just too scarce resource.
As I mentioned above, I think that maintenance overhead due to CMA
deserves to consume a new bit in page flags. It also provide us
extendability to introduce more zones in the future.
Anyway, this value-judgement is subjective so I guess that we
cannot agree with each other. To solve your concern,
I make following solution. Please let me know your opinion about this.
This patch can be applied on top of my ZONE_CMA series.
Thanks.
------------------->8----------------------
^ permalink raw reply
* Re: [PATCH -mm -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Minchan Kim @ 2017-05-11 1:22 UTC (permalink / raw)
To: Johannes Weiner
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel,
Andrea Arcangeli, Ebru Akagunduz, Michal Hocko, Tejun Heo,
Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <20170510232556.GA26521@bbox>
On Thu, May 11, 2017 at 08:25:56AM +0900, Minchan Kim wrote:
> On Wed, May 10, 2017 at 09:56:54AM -0400, Johannes Weiner wrote:
> > Hi Michan,
> >
> > On Tue, May 02, 2017 at 08:53:32AM +0900, Minchan Kim wrote:
> > > @@ -1144,7 +1144,7 @@ void swap_free(swp_entry_t entry)
> > > /*
> > > * Called after dropping swapcache to decrease refcnt to swap entries.
> > > */
> > > -void swapcache_free(swp_entry_t entry)
> > > +void __swapcache_free(swp_entry_t entry)
> > > {
> > > struct swap_info_struct *p;
> > >
> > > @@ -1156,7 +1156,7 @@ void swapcache_free(swp_entry_t entry)
> > > }
> > >
> > > #ifdef CONFIG_THP_SWAP
> > > -void swapcache_free_cluster(swp_entry_t entry)
> > > +void __swapcache_free_cluster(swp_entry_t entry)
> > > {
> > > unsigned long offset = swp_offset(entry);
> > > unsigned long idx = offset / SWAPFILE_CLUSTER;
> > > @@ -1182,6 +1182,14 @@ void swapcache_free_cluster(swp_entry_t entry)
> > > }
> > > #endif /* CONFIG_THP_SWAP */
> > >
> > > +void swapcache_free(struct page *page, swp_entry_t entry)
> > > +{
> > > + if (!PageTransHuge(page))
> > > + __swapcache_free(entry);
> > > + else
> > > + __swapcache_free_cluster(entry);
> > > +}
> >
> > I don't think this is cleaner :/
Let's see a example add_to_swap. Without it, it looks like that.
int add_to_swap(struct page *page)
{
entry = get_swap_page(page);
..
..
fail:
if (PageTransHuge(page))
swapcache_free_cluster(entry);
else
swapcache_free(entry);
}
It doesn't looks good to me because get_swap_page hides
where entry allocation is from cluster or slot but when
we free the entry allocated, we should be aware of the
internal and call right function. :(
Do you think it's better still?
--
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>
^ permalink raw reply
* Re: [PATCH -mm -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Huang, Ying @ 2017-05-11 0:50 UTC (permalink / raw)
To: Minchan Kim
Cc: Johannes Weiner, Huang, Ying, Andrew Morton, linux-mm,
linux-kernel, Andrea Arcangeli, Ebru Akagunduz, Michal Hocko,
Tejun Heo, Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <20170510232556.GA26521@bbox>
Minchan Kim <minchan@kernel.org> writes:
> On Wed, May 10, 2017 at 09:56:54AM -0400, Johannes Weiner wrote:
>> Hi Michan,
>>
>> On Tue, May 02, 2017 at 08:53:32AM +0900, Minchan Kim wrote:
>> > @@ -1144,7 +1144,7 @@ void swap_free(swp_entry_t entry)
>> > /*
>> > * Called after dropping swapcache to decrease refcnt to swap entries.
>> > */
>> > -void swapcache_free(swp_entry_t entry)
>> > +void __swapcache_free(swp_entry_t entry)
>> > {
>> > struct swap_info_struct *p;
>> >
>> > @@ -1156,7 +1156,7 @@ void swapcache_free(swp_entry_t entry)
>> > }
>> >
>> > #ifdef CONFIG_THP_SWAP
>> > -void swapcache_free_cluster(swp_entry_t entry)
>> > +void __swapcache_free_cluster(swp_entry_t entry)
>> > {
>> > unsigned long offset = swp_offset(entry);
>> > unsigned long idx = offset / SWAPFILE_CLUSTER;
>> > @@ -1182,6 +1182,14 @@ void swapcache_free_cluster(swp_entry_t entry)
>> > }
>> > #endif /* CONFIG_THP_SWAP */
>> >
>> > +void swapcache_free(struct page *page, swp_entry_t entry)
>> > +{
>> > + if (!PageTransHuge(page))
>> > + __swapcache_free(entry);
>> > + else
>> > + __swapcache_free_cluster(entry);
>> > +}
>>
>> I don't think this is cleaner :/
>>
>> On your second patch:
>>
>> > @@ -1125,8 +1125,28 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>> > !PageSwapCache(page)) {
>> > if (!(sc->gfp_mask & __GFP_IO))
>> > goto keep_locked;
>> > - if (!add_to_swap(page, page_list))
>> > +swap_retry:
>> > + /*
>> > + * Retry after split if we fail to allocate
>> > + * swap space of a THP.
>> > + */
>> > + if (!add_to_swap(page)) {
>> > + if (!PageTransHuge(page) ||
>> > + split_huge_page_to_list(page, page_list))
>> > + goto activate_locked;
>> > + goto swap_retry;
>> > + }
>>
>> This is definitely better.
>
> Thanks.
>
>>
>> However, I think it'd be cleaner without the label here:
>>
>> if (!add_to_swap(page)) {
>> if (!PageTransHuge(page))
>> goto activate_locked;
>> /* Split THP and swap individual base pages */
>> if (split_huge_page_to_list(page, page_list))
>> goto activate_locked;
>> if (!add_to_swap(page))
>> goto activate_locked;
>
> Yes.
>
>> }
>>
>> > + /*
>> > + * Got swap space successfully. But unfortunately,
>> > + * we don't support a THP page writeout so split it.
>> > + */
>> > + if (PageTransHuge(page) &&
>> > + split_huge_page_to_list(page, page_list)) {
>> > + delete_from_swap_cache(page);
>> > goto activate_locked;
>> > + }
>>
>> Pulling this out of add_to_swap() is an improvement for sure. Add an
>> XXX: before that "we don't support THP writes" comment for good
>> measure :)
>
> Sure.
>
> It could be a separate patch which makes add_to_swap clean via
> removing page_list argument but I hope Huang take/fold it when he
> resend it because it would be more important with THP swap.
Sure. I will take this patch as one patch of the THP swap series.
Because the first patch of the THP swap series is a little big, I don't
think it is a good idea to fold this patch into it. Could you update
the patch according to Johannes' comments and resend it?
Best Regards,
Huang, Ying
--
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>
^ permalink raw reply
* Re: [PATCH -mm -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Minchan Kim @ 2017-05-10 23:25 UTC (permalink / raw)
To: Johannes Weiner
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel,
Andrea Arcangeli, Ebru Akagunduz, Michal Hocko, Tejun Heo,
Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <20170510135654.GD17121@cmpxchg.org>
On Wed, May 10, 2017 at 09:56:54AM -0400, Johannes Weiner wrote:
> Hi Michan,
>
> On Tue, May 02, 2017 at 08:53:32AM +0900, Minchan Kim wrote:
> > @@ -1144,7 +1144,7 @@ void swap_free(swp_entry_t entry)
> > /*
> > * Called after dropping swapcache to decrease refcnt to swap entries.
> > */
> > -void swapcache_free(swp_entry_t entry)
> > +void __swapcache_free(swp_entry_t entry)
> > {
> > struct swap_info_struct *p;
> >
> > @@ -1156,7 +1156,7 @@ void swapcache_free(swp_entry_t entry)
> > }
> >
> > #ifdef CONFIG_THP_SWAP
> > -void swapcache_free_cluster(swp_entry_t entry)
> > +void __swapcache_free_cluster(swp_entry_t entry)
> > {
> > unsigned long offset = swp_offset(entry);
> > unsigned long idx = offset / SWAPFILE_CLUSTER;
> > @@ -1182,6 +1182,14 @@ void swapcache_free_cluster(swp_entry_t entry)
> > }
> > #endif /* CONFIG_THP_SWAP */
> >
> > +void swapcache_free(struct page *page, swp_entry_t entry)
> > +{
> > + if (!PageTransHuge(page))
> > + __swapcache_free(entry);
> > + else
> > + __swapcache_free_cluster(entry);
> > +}
>
> I don't think this is cleaner :/
>
> On your second patch:
>
> > @@ -1125,8 +1125,28 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > !PageSwapCache(page)) {
> > if (!(sc->gfp_mask & __GFP_IO))
> > goto keep_locked;
> > - if (!add_to_swap(page, page_list))
> > +swap_retry:
> > + /*
> > + * Retry after split if we fail to allocate
> > + * swap space of a THP.
> > + */
> > + if (!add_to_swap(page)) {
> > + if (!PageTransHuge(page) ||
> > + split_huge_page_to_list(page, page_list))
> > + goto activate_locked;
> > + goto swap_retry;
> > + }
>
> This is definitely better.
Thanks.
>
> However, I think it'd be cleaner without the label here:
>
> if (!add_to_swap(page)) {
> if (!PageTransHuge(page))
> goto activate_locked;
> /* Split THP and swap individual base pages */
> if (split_huge_page_to_list(page, page_list))
> goto activate_locked;
> if (!add_to_swap(page))
> goto activate_locked;
Yes.
> }
>
> > + /*
> > + * Got swap space successfully. But unfortunately,
> > + * we don't support a THP page writeout so split it.
> > + */
> > + if (PageTransHuge(page) &&
> > + split_huge_page_to_list(page, page_list)) {
> > + delete_from_swap_cache(page);
> > goto activate_locked;
> > + }
>
> Pulling this out of add_to_swap() is an improvement for sure. Add an
> XXX: before that "we don't support THP writes" comment for good
> measure :)
Sure.
It could be a separate patch which makes add_to_swap clean via
removing page_list argument but I hope Huang take/fold it when he
resend it because it would be more important with THP swap.
Thanks.
--
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>
^ permalink raw reply
* Re: [RFC 0/4] RFC - Coherent Device Memory (Not for inclusion)
From: Balbir Singh @ 2017-05-10 23:04 UTC (permalink / raw)
To: Michal Hocko, Benjamin Herrenschmidt
Cc: linux-mm, akpm, khandual, aneesh.kumar, paulmck, srikar, haren,
jglisse, mgorman, arbab, vbabka, cl
In-Reply-To: <20170509113638.GJ6481@dhcp22.suse.cz>
On Tue, 2017-05-09 at 13:36 +0200, Michal Hocko wrote:
> On Fri 05-05-17 17:57:02, Benjamin Herrenschmidt wrote:
> > On Fri, 2017-05-05 at 16:52 +0200, Michal Hocko wrote:
> > >
> > > This sounds pretty much like a HW specific details which is not the
> > > right criterion to design general CDM around.
> >
> > Which is why I don't see what's the problem with simply making this
> > a hot-plugged NUMA node, since it's basically what it is with a
> > "different" kind of CPU, possibly covered with a CMA, which provides
> > both some isolation and the ability to do large physical allocations
> > for applications who chose to use the legacy programming interfaces and
> > manually control the memory.
> >
> > Then, the "issues" with things like reclaim, autonuma can be handled
> > with policy tunables. Possibly node attributes.
> >
> > It seems to me that such a model fits well in the picture where we are
> > heading not just with GPUs, but with OpenCAPI based memory, CCIX or
> > other similar technologies that can provide memory possibly with co-
> > located acceleration devices.
> >
> > It also mostly already just work.
>
> But this is not what the CDM as proposed here is about AFAIU.
The main reason for the patches was to address "issues" with things like
reclaim, autonuma isolation, etc and the constraint of not willing to
make allocator changes.
Do we see node attributes as something we need generically? Is there
consensus that we need this or do we see all new algorithms working
across all of N_MEMORY all the time?
> It is
> argued this is not a _normal_ cpuless node and it neads tweak here and
> there. And that is my main objection about. I do not mind if the memory
> is presented as a hotplugable cpuless memory node. I just do not want it
> to be any more special than cpuless nodes are already.
The downsides being code complexity/run time overhead? Like Ben stated
there are several devices that will also have coherent memory, do you see all of
them abstracted as HMM-CDM?
>
> > > So let me repeat the fundamental question. Is the only difference from
> > > cpuless nodes the fact that the node should be invisible to processes
> > > unless they specify an explicit node mask?
> >
> > It would be *preferable* that it is.
> >
> > It's not necessarily an absolute requirement as long as what lands
> > there can be kicked out. However the system would potentially be
> > performing poorly if too much unrelated stuff lands on the GPU memory
> > as it has a much higher latency.
>
> This is a general concern for many cpuless NUMA node systems. You have
> to pay for the suboptimal performance when accessing that memory. And
> you have means to cope with that.
>
How do we evolve the NUMA subsystem to deal with additional requirements?
Do we not enhance NUMA and move to ZONE_DEVICE?
> > Due to the nature of GPUs (and possibly other such accelerators but not
> > necessarily all of them), that memory is also more likely to fail. GPUs
> > crash often. However that isn't necessarily true of OpenCAPI devices or
> > CCIX.
> >
> > This is the kind of attributes of the memory (quality ?) that can be
> > provided by the driver that is putting it online. We can then
> > orthogonally decide how we chose (or not) to take those into account,
> > either in the default mm algorithms or from explicit policy mechanisms
> > set from userspace, but the latter is often awkward and never done
> > right.
>
> The first adds maintain costs all over the place and just looking at
> what become of memory policies and cpusets makes me cry. I definitely do
> not want more special casing on top (and just to make it clear a special
> N_MEMORY_$FOO falls into the same category).
>
And I thought it was cleaner design, yes we have been special casing some
of the N_COHERENT_MEMORY bits in mm/mempolicy.c.
> [...]
> > > Moreover cpusets already support exclusive numa nodes AFAIR.
> >
> > Which implies that the user would have to do epxlciit cpuset
> > manipulations for the system to work right ? Most user wouldn't and the
> > rsult is that most user would have badly working systems. That's almost
> > always what happens when we chose to bounce *all* policy decision to
> > the user without the kernel attempting to have some kind of semi-sane
> > default.
>
> I would argue that this is the case for cpuless numa nodes already.
> Users should better know what they are doing when using such a
> specialized HW. And that includes a specialized configuration.
>
Like Ben said intimate knowledge of using specialized hardware
is an unfair assumption. It sounds like the decision then is that
we do HMM-CDM or live with cpuless nodes without enhancements?
Balbir
--
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>
^ permalink raw reply
* [PATCH v2] mm: vmscan: scan until it founds eligible pages
From: Minchan Kim @ 2017-05-10 23:00 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-mm, Johannes Weiner, Mel Gorman, Michal Hocko,
kernel-team, Minchan Kim
Although there are a ton of free swap and anonymous LRU page
in elgible zones, OOM happened.
balloon invoked oom-killer: gfp_mask=0x17080c0(GFP_KERNEL_ACCOUNT|__GFP_ZERO|__GFP_NOTRACK), nodemask=(null), order=0, oom_score_adj=0
CPU: 7 PID: 1138 Comm: balloon Not tainted 4.11.0-rc6-mm1-zram-00289-ge228d67e9677-dirty #17
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
Call Trace:
dump_stack+0x65/0x87
dump_header.isra.19+0x8f/0x20f
? preempt_count_add+0x9e/0xb0
? _raw_spin_unlock_irqrestore+0x24/0x40
oom_kill_process+0x21d/0x3f0
? has_capability_noaudit+0x17/0x20
out_of_memory+0xd8/0x390
__alloc_pages_slowpath+0xbc1/0xc50
? anon_vma_interval_tree_insert+0x84/0x90
__alloc_pages_nodemask+0x1a5/0x1c0
pte_alloc_one+0x20/0x50
__pte_alloc+0x1e/0x110
__handle_mm_fault+0x919/0x960
handle_mm_fault+0x77/0x120
__do_page_fault+0x27a/0x550
trace_do_page_fault+0x43/0x150
do_async_page_fault+0x2c/0x90
async_page_fault+0x28/0x30
RIP: 0033:0x7fc4636bacb8
RSP: 002b:00007fff97c9c4c0 EFLAGS: 00010202
RAX: 00007fc3e818d000 RBX: 00007fc4639f8760 RCX: 00007fc46372e9ca
RDX: 0000000000101002 RSI: 0000000000101000 RDI: 0000000000000000
RBP: 0000000000100010 R08: 00000000ffffffff R09: 0000000000000000
R10: 0000000000000022 R11: 00000000000a3901 R12: 00007fc3e818d010
R13: 0000000000101000 R14: 00007fc4639f87b8 R15: 00007fc4639f87b8
Mem-Info:
active_anon:424716 inactive_anon:65314 isolated_anon:0
active_file:52 inactive_file:46 isolated_file:0
unevictable:0 dirty:27 writeback:0 unstable:0
slab_reclaimable:3967 slab_unreclaimable:4125
mapped:133 shmem:43 pagetables:1674 bounce:0
free:4637 free_pcp:225 free_cma:0
Node 0 active_anon:1698864kB inactive_anon:261256kB active_file:208kB inactive_file:184kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:532kB dirty:108kB writeback:0kB shmem:172kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no
DMA free:7316kB min:32kB low:44kB high:56kB active_anon:8064kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB slab_reclaimable:464kB slab_unreclaimable:40kB kernel_stack:0kB pagetables:24kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
lowmem_reserve[]: 0 992 992 1952
DMA32 free:9088kB min:2048kB low:3064kB high:4080kB active_anon:952176kB inactive_anon:0kB active_file:36kB inactive_file:0kB unevictable:0kB writepending:88kB present:1032192kB managed:1019388kB mlocked:0kB slab_reclaimable:13532kB slab_unreclaimable:16460kB kernel_stack:3552kB pagetables:6672kB bounce:0kB free_pcp:56kB local_pcp:24kB free_cma:0kB
lowmem_reserve[]: 0 0 0 959
Movable free:3644kB min:1980kB low:2960kB high:3940kB active_anon:738560kB inactive_anon:261340kB active_file:188kB inactive_file:640kB unevictable:0kB writepending:20kB present:1048444kB managed:1010816kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:832kB local_pcp:60kB free_cma:0kB
lowmem_reserve[]: 0 0 0 0
DMA: 1*4kB (E) 0*8kB 18*16kB (E) 10*32kB (E) 10*64kB (E) 9*128kB (ME) 8*256kB (E) 2*512kB (E) 2*1024kB (E) 0*2048kB 0*4096kB = 7524kB
DMA32: 417*4kB (UMEH) 181*8kB (UMEH) 68*16kB (UMEH) 48*32kB (UMEH) 14*64kB (MH) 3*128kB (M) 1*256kB (H) 1*512kB (M) 2*1024kB (M) 0*2048kB 0*4096kB = 9836kB
Movable: 1*4kB (M) 1*8kB (M) 1*16kB (M) 1*32kB (M) 0*64kB 1*128kB (M) 2*256kB (M) 4*512kB (M) 1*1024kB (M) 0*2048kB 0*4096kB = 3772kB
378 total pagecache pages
17 pages in swap cache
Swap cache stats: add 17325, delete 17302, find 0/27
Free swap = 978940kB
Total swap = 1048572kB
524157 pages RAM
0 pages HighMem/MovableOnly
12629 pages reserved
0 pages cma reserved
0 pages hwpoisoned
[ pid ] uid tgid total_vm rss nr_ptes nr_pmds swapents oom_score_adj name
[ 433] 0 433 4904 5 14 3 82 0 upstart-udev-br
[ 438] 0 438 12371 5 27 3 191 -1000 systemd-udevd
With investigation, skipping page of isolate_lru_pages makes reclaim
void because it returns zero nr_taken easily so LRU shrinking is
effectively nothing and just increases priority aggressively.
Finally, OOM happens.
The problem is that get_scan_count determines nr_to_scan with
eligible zones so although priority drops to zero, it couldn't
reclaim any pages if the LRU contains mostly ineligible pages.
get_scan_count:
size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
size = size >> sc->priority;
Assumes sc->priority is 0 and LRU list is as follows.
N-N-N-N-H-H-H-H-H-H-H-H-H-H-H-H-H-H-H-H
(Ie, small eligible pages are in the head of LRU but others are
almost ineligible pages)
In that case, size becomes 4 so VM want to scan 4 pages but 4 pages
from tail of the LRU are not eligible pages.
If get_scan_count counts skipped pages, it doesn't reclaim any pages
remained after scanning 4 pages so it ends up OOM happening.
This patch makes isolate_lru_pages try to scan pages until it
encounters eligible zones's pages.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
* from v1
* put more words in description and code
* drop unncessary pages_skipped list flushing
mm/vmscan.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 5ebf468c5429..e051bf4a1144 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1449,7 +1449,7 @@ static __always_inline void update_lru_sizes(struct lruvec *lruvec,
*
* Appropriate locks must be held before calling this function.
*
- * @nr_to_scan: The number of pages to look through on the list.
+ * @nr_to_scan: The number of eligible pages to look through on the list.
* @lruvec: The LRU vector to pull pages from.
* @dst: The temp list to put pages on to.
* @nr_scanned: The number of pages that were scanned.
@@ -1469,11 +1469,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
unsigned long nr_zone_taken[MAX_NR_ZONES] = { 0 };
unsigned long nr_skipped[MAX_NR_ZONES] = { 0, };
unsigned long skipped = 0;
- unsigned long scan, nr_pages;
+ unsigned long scan, total_scan, nr_pages;
LIST_HEAD(pages_skipped);
- for (scan = 0; scan < nr_to_scan && nr_taken < nr_to_scan &&
- !list_empty(src); scan++) {
+ for (total_scan = scan = 0; scan < nr_to_scan &&
+ nr_taken < nr_to_scan &&
+ !list_empty(src);
+ total_scan++) {
struct page *page;
page = lru_to_page(src);
@@ -1487,6 +1489,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
continue;
}
+ /*
+ * Do not count skipped pages because it makes the function to
+ * return with none isolated pages if the LRU mostly contains
+ * ineligible pages so that VM cannot reclaim any pages and
+ * trigger premature OOM.
+ */
+ scan++;
switch (__isolate_lru_page(page, mode)) {
case 0:
nr_pages = hpage_nr_pages(page);
@@ -1524,9 +1533,9 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
skipped += nr_skipped[zid];
}
}
- *nr_scanned = scan;
+ *nr_scanned = total_scan;
trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan,
- scan, skipped, nr_taken, mode, lru);
+ total_scan, skipped, nr_taken, mode, lru);
update_lru_sizes(lruvec, lru, nr_zone_taken);
return nr_taken;
}
--
2.7.4
--
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>
^ permalink raw reply related
* Re: [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm
From: Andy Lutomirski @ 2017-05-10 22:42 UTC (permalink / raw)
To: Ingo Molnar
Cc: Thomas Gleixner, Andy Lutomirski, X86 ML,
linux-kernel@vger.kernel.org, Borislav Petkov, Linus Torvalds,
Andrew Morton, Mel Gorman, linux-mm@kvack.org, Rik van Riel,
Dave Hansen, Nadav Amit, Michal Hocko, Arjan van de Ven
In-Reply-To: <20170510082425.5ks5okbjne7xgjtv@gmail.com>
On Wed, May 10, 2017 at 1:24 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> On Wed, 10 May 2017, Ingo Molnar wrote:
>> >
>> > * Thomas Gleixner <tglx@linutronix.de> wrote:
>> >
>> > > On Sun, 7 May 2017, Andy Lutomirski wrote:
>> > > > /* context.lock is held for us, so we don't need any locking. */
>> > > > static void flush_ldt(void *current_mm)
>> > > > {
>> > > > + struct mm_struct *mm = current_mm;
>> > > > mm_context_t *pc;
>> > > >
>> > > > - if (current->active_mm != current_mm)
>> > > > + if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
>> > >
>> > > While functional correct, this really should compare against 'mm'.
>> > >
>> > > > return;
>> > > >
>> > > > - pc = ¤t->active_mm->context;
>> > > > + pc = &mm->context;
>> >
>> > So this appears to be the function:
>> >
>> > static void flush_ldt(void *current_mm)
>> > {
>> > struct mm_struct *mm = current_mm;
>> > mm_context_t *pc;
>> >
>> > if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
>> > return;
>> >
>> > pc = &mm->context;
>> > set_ldt(pc->ldt->entries, pc->ldt->size);
>> > }
>> >
>> > why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?
>>
>> Because you cannot dereference a void pointer, i.e. &mm->context ....
>
> Indeed, doh! The naming totally confused me. The way I'd write it is the canonical
> form for such callbacks:
>
> static void flush_ldt(void *data)
> {
> struct mm_struct *mm = data;
>
> ... which beyond unconfusing me would probably also have prevented any accidental
> use of the 'current_mm' callback argument.
>
>
void *data and void *info both seem fairly common in the kernel. How
about my personal favorite for non-kernel work, though: void *mm_void?
It documents what the parameter means and avoids the confusion.
--Andy
--
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>
^ permalink raw reply
* [patch] mm, thp: copying user pages must schedule on collapse
From: David Rientjes @ 2017-05-10 21:27 UTC (permalink / raw)
To: Andrew Morton
Cc: Kirill A. Shutemov, Johannes Weiner, Vlastimil Babka, Mel Gorman,
linux-kernel, linux-mm
We have encountered need_resched warnings in __collapse_huge_page_copy()
while doing {clear,copy}_user_highpage() over HPAGE_PMD_NR source pages.
mm->mmap_sem is held for write, but the iteration is well bounded.
Reschedule as needed.
Signed-off-by: David Rientjes <rientjes@google.com>
---
mm/khugepaged.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -612,7 +612,8 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
spinlock_t *ptl)
{
pte_t *_pte;
- for (_pte = pte; _pte < pte+HPAGE_PMD_NR; _pte++) {
+ for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
+ _pte++, page++, address += PAGE_SIZE) {
pte_t pteval = *_pte;
struct page *src_page;
@@ -651,9 +652,7 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
spin_unlock(ptl);
free_page_and_swap_cache(src_page);
}
-
- address += PAGE_SIZE;
- page++;
+ cond_resched();
}
}
--
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>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: Matthew Wilcox @ 2017-05-10 21:11 UTC (permalink / raw)
To: David Miller
Cc: mhocko, pasha.tatashin, linux-kernel, sparclinux, linux-mm,
linuxppc-dev, linux-s390, borntraeger, heiko.carstens
In-Reply-To: <20170510.140026.1367439672848112283.davem@davemloft.net>
On Wed, May 10, 2017 at 02:00:26PM -0400, David Miller wrote:
> From: Matthew Wilcox <willy@infradead.org>
> Date: Wed, 10 May 2017 10:17:03 -0700
> > On Wed, May 10, 2017 at 11:19:43AM -0400, David Miller wrote:
> >> I guess it might be clearer if you understand what the block
> >> initializing stores do on sparc64. There are no memory accesses at
> >> all.
> >>
> >> The cpu just zeros out the cache line, that's it.
> >>
> >> No L3 cache line is allocated. So this "wipe everything" behavior
> >> will not happen in the L3.
> >
> > There's either something wrong with your explanation or my reading
> > skills :-)
> >
> > "There are no memory accesses"
> > "No L3 cache line is allocated"
> >
> > You can have one or the other ... either the CPU sends a cacheline-sized
> > write of zeroes to memory without allocating an L3 cache line (maybe
> > using the store buffer?), or the CPU allocates an L3 cache line and sets
> > its contents to zeroes, probably putting it in the last way of the set
> > so it's the first thing to be evicted if not touched.
>
> There is no conflict in what I said.
>
> Only an L2 cache line is allocated and cleared. L3 is left alone.
I thought SPARC had inclusive caches. So allocating an L2 cacheline
would necessitate allocating an L3 cacheline. Or is this an exception
to the normal order of things?
--
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>
^ permalink raw reply
* Re: [PATCH] mark protection_map as __ro_after_init
From: Kees Cook @ 2017-05-10 18:20 UTC (permalink / raw)
To: Daniel Micay
Cc: Andrew Morton, Linux-MM, kernel-hardening@lists.openwall.com,
LKML
In-Reply-To: <20170510174441.26163-1-danielmicay@gmail.com>
On Wed, May 10, 2017 at 10:44 AM, Daniel Micay <danielmicay@gmail.com> wrote:
> The protection map is only modified by per-arch init code so it can be
> protected from writes after the init code runs.
>
> This change was extracted from PaX where it's part of KERNEXEC.
>
> Signed-off-by: Daniel Micay <danielmicay@gmail.com>
Thanks!
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> mm/mmap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f82741e199c0..3bd5ecd20d4d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -94,7 +94,7 @@ static void unmap_region(struct mm_struct *mm,
> * w: (no) no
> * x: (yes) yes
> */
> -pgprot_t protection_map[16] = {
> +pgprot_t protection_map[16] __ro_after_init = {
> __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111,
> __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111
> };
> --
> 2.12.2
>
--
Kees Cook
Pixel Security
--
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>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: David Miller @ 2017-05-10 18:00 UTC (permalink / raw)
To: willy
Cc: mhocko, pasha.tatashin, linux-kernel, sparclinux, linux-mm,
linuxppc-dev, linux-s390, borntraeger, heiko.carstens
In-Reply-To: <20170510171703.GC1590@bombadil.infradead.org>
From: Matthew Wilcox <willy@infradead.org>
Date: Wed, 10 May 2017 10:17:03 -0700
> On Wed, May 10, 2017 at 11:19:43AM -0400, David Miller wrote:
>> From: Michal Hocko <mhocko@kernel.org>
>> Date: Wed, 10 May 2017 16:57:26 +0200
>>
>> > Have you measured that? I do not think it would be super hard to
>> > measure. I would be quite surprised if this added much if anything at
>> > all as the whole struct page should be in the cache line already. We do
>> > set reference count and other struct members. Almost nobody should be
>> > looking at our page at this time and stealing the cache line. On the
>> > other hand a large memcpy will basically wipe everything away from the
>> > cpu cache. Or am I missing something?
>>
>> I guess it might be clearer if you understand what the block
>> initializing stores do on sparc64. There are no memory accesses at
>> all.
>>
>> The cpu just zeros out the cache line, that's it.
>>
>> No L3 cache line is allocated. So this "wipe everything" behavior
>> will not happen in the L3.
>
> There's either something wrong with your explanation or my reading
> skills :-)
>
> "There are no memory accesses"
> "No L3 cache line is allocated"
>
> You can have one or the other ... either the CPU sends a cacheline-sized
> write of zeroes to memory without allocating an L3 cache line (maybe
> using the store buffer?), or the CPU allocates an L3 cache line and sets
> its contents to zeroes, probably putting it in the last way of the set
> so it's the first thing to be evicted if not touched.
There is no conflict in what I said.
Only an L2 cache line is allocated and cleared. L3 is left alone.
--
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>
^ permalink raw reply
* [PATCH] mark protection_map as __ro_after_init
From: Daniel Micay @ 2017-05-10 17:44 UTC (permalink / raw)
To: Andrew Morton, linux-mm; +Cc: Kees Cook, kernel-hardening, Daniel Micay
The protection map is only modified by per-arch init code so it can be
protected from writes after the init code runs.
This change was extracted from PaX where it's part of KERNEXEC.
Signed-off-by: Daniel Micay <danielmicay@gmail.com>
---
mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index f82741e199c0..3bd5ecd20d4d 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -94,7 +94,7 @@ static void unmap_region(struct mm_struct *mm,
* w: (no) no
* x: (yes) yes
*/
-pgprot_t protection_map[16] = {
+pgprot_t protection_map[16] __ro_after_init = {
__P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111,
__S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111
};
--
2.12.2
--
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>
^ permalink raw reply related
* Re: [PATCH 0/4 v4] mm,dax: Fix data corruption due to mmap inconsistency
From: Ross Zwisler @ 2017-05-10 17:27 UTC (permalink / raw)
To: Jan Kara
Cc: Andrew Morton, Ross Zwisler, Dan Williams, linux-ext4,
linux-fsdevel, linux-nvdimm, linux-mm
In-Reply-To: <20170510085419.27601-1-jack@suse.cz>
On Wed, May 10, 2017 at 10:54:15AM +0200, Jan Kara wrote:
> Hello,
>
> this series fixes data corruption that can happen for DAX mounts when
> page faults race with write(2) and as a result page tables get out of sync
> with block mappings in the filesystem and thus data seen through mmap is
> different from data seen through read(2).
>
> The series passes testing with t_mmap_stale test program from Ross and also
> other mmap related tests on DAX filesystem.
>
> Andrew, can you please merge these patches? Thanks!
>
> Changes since v3:
> * Rebased on top of current Linus' tree due to non-trivial conflicts with
> added tracepoint
Cool, the merge update looks correct to me.
--
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>
^ permalink raw reply
* [PATCH 5/4] dax: Fix PMD data corruption when fault races with write
From: Ross Zwisler @ 2017-05-10 17:27 UTC (permalink / raw)
To: Andrew Morton, Jan Kara
Cc: Ross Zwisler, Dan Williams, linux-ext4, linux-fsdevel,
linux-nvdimm, linux-mm, stable
In-Reply-To: <20170510085419.27601-5-jack@suse.cz>
This is based on a patch from Jan Kara that fixed the equivalent race in
the DAX PTE fault path.
Currently DAX PMD read fault can race with write(2) in the following way:
CPU1 - write(2) CPU2 - read fault
dax_iomap_pmd_fault()
->iomap_begin() - sees hole
dax_iomap_rw()
iomap_apply()
->iomap_begin - allocates blocks
dax_iomap_actor()
invalidate_inode_pages2_range()
- there's nothing to invalidate
grab_mapping_entry()
- we add huge zero page to the radix tree
and map it to page tables
The result is that hole page is mapped into page tables (and thus zeros
are seen in mmap) while file has data written in that place.
Fix the problem by locking exception entry before mapping blocks for the
fault. That way we are sure invalidate_inode_pages2_range() call for
racing write will either block on entry lock waiting for the fault to
finish (and unmap stale page tables after that) or read fault will see
already allocated blocks by write(2).
Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: 9f141d6ef6258a3a37a045842d9ba7e68f368956
CC: stable@vger.kernel.org
---
Jan, I just realized that we need an equivalent fix in the PMD path. Let's
keep this with the rest of your series so they get applied together,
applied to stable together, etc.
This applies cleanly to the current linux/master (56868a460b83) + the four
patches from Jan's series. I've run it through xfstests and some targeted
testing for the PMD path.
---
fs/dax.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 32f020c..93ae872 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1388,6 +1388,16 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
goto fallback;
/*
+ * grab_mapping_entry() will make sure we get a 2M empty entry, a DAX
+ * PMD or a HZP entry. If it can't (because a 4k page is already in
+ * the tree, for instance), it will return -EEXIST and we just fall
+ * back to 4k entries.
+ */
+ entry = grab_mapping_entry(mapping, pgoff, RADIX_DAX_PMD);
+ if (IS_ERR(entry))
+ goto fallback;
+
+ /*
* Note that we don't use iomap_apply here. We aren't doing I/O, only
* setting up a mapping, so really we're using iomap_begin() as a way
* to look up our filesystem block.
@@ -1395,21 +1405,11 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
pos = (loff_t)pgoff << PAGE_SHIFT;
error = ops->iomap_begin(inode, pos, PMD_SIZE, iomap_flags, &iomap);
if (error)
- goto fallback;
+ goto unlock_entry;
if (iomap.offset + iomap.length < pos + PMD_SIZE)
goto finish_iomap;
- /*
- * grab_mapping_entry() will make sure we get a 2M empty entry, a DAX
- * PMD or a HZP entry. If it can't (because a 4k page is already in
- * the tree, for instance), it will return -EEXIST and we just fall
- * back to 4k entries.
- */
- entry = grab_mapping_entry(mapping, pgoff, RADIX_DAX_PMD);
- if (IS_ERR(entry))
- goto finish_iomap;
-
switch (iomap.type) {
case IOMAP_MAPPED:
result = dax_pmd_insert_mapping(vmf, &iomap, pos, &entry);
@@ -1417,7 +1417,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
case IOMAP_UNWRITTEN:
case IOMAP_HOLE:
if (WARN_ON_ONCE(write))
- goto unlock_entry;
+ break;
result = dax_pmd_load_hole(vmf, &iomap, &entry);
break;
default:
@@ -1425,8 +1425,6 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
break;
}
- unlock_entry:
- put_locked_mapping_entry(mapping, pgoff, entry);
finish_iomap:
if (ops->iomap_end) {
int copied = PMD_SIZE;
@@ -1442,6 +1440,8 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf,
ops->iomap_end(inode, pos, PMD_SIZE, copied, iomap_flags,
&iomap);
}
+ unlock_entry:
+ put_locked_mapping_entry(mapping, pgoff, entry);
fallback:
if (result == VM_FAULT_FALLBACK) {
split_huge_pmd(vma, vmf->pmd, vmf->address);
--
2.9.3
--
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>
^ permalink raw reply related
* Re: [v3 0/9] parallelized "struct page" zeroing
From: Matthew Wilcox @ 2017-05-10 17:17 UTC (permalink / raw)
To: David Miller
Cc: mhocko, pasha.tatashin, linux-kernel, sparclinux, linux-mm,
linuxppc-dev, linux-s390, borntraeger, heiko.carstens
In-Reply-To: <20170510.111943.1940354761418085760.davem@davemloft.net>
On Wed, May 10, 2017 at 11:19:43AM -0400, David Miller wrote:
> From: Michal Hocko <mhocko@kernel.org>
> Date: Wed, 10 May 2017 16:57:26 +0200
>
> > Have you measured that? I do not think it would be super hard to
> > measure. I would be quite surprised if this added much if anything at
> > all as the whole struct page should be in the cache line already. We do
> > set reference count and other struct members. Almost nobody should be
> > looking at our page at this time and stealing the cache line. On the
> > other hand a large memcpy will basically wipe everything away from the
> > cpu cache. Or am I missing something?
>
> I guess it might be clearer if you understand what the block
> initializing stores do on sparc64. There are no memory accesses at
> all.
>
> The cpu just zeros out the cache line, that's it.
>
> No L3 cache line is allocated. So this "wipe everything" behavior
> will not happen in the L3.
There's either something wrong with your explanation or my reading
skills :-)
"There are no memory accesses"
"No L3 cache line is allocated"
You can have one or the other ... either the CPU sends a cacheline-sized
write of zeroes to memory without allocating an L3 cache line (maybe
using the store buffer?), or the CPU allocates an L3 cache line and sets
its contents to zeroes, probably putting it in the last way of the set
so it's the first thing to be evicted if not touched.
Or there's some magic in the memory bus protocol where the CPU gets to
tell the DRAM "hey, clear these cache lines". Although that's also a
memory access of sorts ...
--
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>
^ permalink raw reply
* Re: [PATCH v4 23/27] gfs2: clean up some filemap_* calls
From: Bob Peterson @ 2017-05-10 16:18 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, bo li liu
In-Reply-To: <20170509154930.29524-24-jlayton@redhat.com>
----- Original Message -----
| In some places, it's trying to reset the mapping error after calling
| filemap_fdatawait. That's no longer required. Also, turn several
| filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
| That will at least return writeback errors that occur during the write
| phase.
|
| Signed-off-by: Jeff Layton <jlayton@redhat.com>
| ---
Hi Jeff,
This version looks better. ACK.
Regards,
Bob Peterson
Red Hat File Systems
--
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>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Dave Hansen @ 2017-05-10 15:45 UTC (permalink / raw)
To: Igor Stoppa, Michal Hocko, Laura Abbott
Cc: linux-mm, linux-kernel, kernel-hardening@lists.openwall.com
In-Reply-To: <1a8cc1f4-0b72-34ea-43ad-5ece22a8d5cf@huawei.com>
On 05/10/2017 08:19 AM, Igor Stoppa wrote:
> So I'd like to play a little what-if scenario:
> what if I was to support exclusively virtual memory and convert to it
> everything that might need sealing?
Because of the issues related to fracturing large pages, you might have
had to go this route eventually anyway. Changing the kernel linear map
isn't nice.
FWIW, you could test this scheme by just converting all the users to
vmalloc() and seeing what breaks. They'd all end up rounding up all
their allocations to PAGE_SIZE, but that'd be fine for testing.
Could you point out 5 or 10 places in the kernel that you want to convert?
--
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>
^ permalink raw reply
* [Patch v3] mm/vmscan: fix unsequenced modification and access warning
From: Nick Desaulniers @ 2017-05-10 15:40 UTC (permalink / raw)
Cc: akpm, hannes, mgorman, mhocko, vbabka, minchan, linux-mm,
linux-kernel, Nick Desaulniers
In-Reply-To: <20170510092505.GH31466@dhcp22.suse.cz>
Clang and its -Wunsequenced emits a warning
mm/vmscan.c:2961:25: error: unsequenced modification and access to
'gfp_mask' [-Wunsequenced]
.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
^
While it is not clear to me whether the initialization code violates the
specification (6.7.8 par 19 (ISO/IEC 9899) looks like it disagrees) the
code is quite confusing and worth cleaning up anyway. Fix this by
reusing sc.gfp_mask rather than the updated input gfp_mask parameter.
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
Changes in v3:
- changed commit message
- added previous ack
Will file a bug with llvm later today
mm/vmscan.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4e7ed65842af..d32c42d17935 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
unsigned long nr_reclaimed;
struct scan_control sc = {
.nr_to_reclaim = SWAP_CLUSTER_MAX,
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.reclaim_idx = gfp_zone(gfp_mask),
.order = order,
.nodemask = nodemask,
@@ -2973,12 +2973,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
* 1 is returned so that the page allocator does not OOM kill at this
* point.
*/
- if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
+ if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
return 1;
trace_mm_vmscan_direct_reclaim_begin(order,
sc.may_writepage,
- gfp_mask,
+ sc.gfp_mask,
sc.reclaim_idx);
nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
@@ -3763,16 +3763,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
const unsigned long nr_pages = 1 << order;
struct task_struct *p = current;
struct reclaim_state reclaim_state;
- int classzone_idx = gfp_zone(gfp_mask);
struct scan_control sc = {
.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.order = order,
.priority = NODE_RECLAIM_PRIORITY,
.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
.may_swap = 1,
- .reclaim_idx = classzone_idx,
+ .reclaim_idx = gfp_zone(gfp_mask),
};
cond_resched();
@@ -3782,7 +3781,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
* and RECLAIM_UNMAP.
*/
p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
- lockdep_set_current_reclaim_state(gfp_mask);
+ lockdep_set_current_reclaim_state(sc.gfp_mask);
reclaim_state.reclaimed_slab = 0;
p->reclaim_state = &reclaim_state;
--
2.11.0
--
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>
^ permalink raw reply related
* Re: [patch 3/3] MM: allow per-cpu vmstat_worker configuration
From: Rik van Riel @ 2017-05-10 15:34 UTC (permalink / raw)
To: Marcelo Tosatti, linux-kernel, linux-mm; +Cc: Luiz Capitulino, Linux RT Users
In-Reply-To: <20170503184039.901336380@redhat.com>
On Wed, 2017-05-03 at 15:40 -0300, Marcelo Tosatti wrote:
> Following the reasoning on the last patch in the series,
> this patch allows configuration of the per-CPU vmstat worker:
> it allows the user to disable the per-CPU vmstat worker.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Is there ever a case where you would want to configure
this separately from the vmstat_threshold parameter?
What use cases are you trying to address?
--
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>
^ permalink raw reply
* Re: Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks
From: Vlastimil Babka @ 2017-05-10 15:24 UTC (permalink / raw)
To: Michal Hocko, Igor Stoppa, Andrew Morton; +Cc: namhyung, linux-mm, linux-kernel
In-Reply-To: <20170427133523.GG4706@dhcp22.suse.cz>
On 04/27/2017 03:35 PM, Michal Hocko wrote:
> On Thu 27-04-17 15:16:47, Igor Stoppa wrote:
>> On 26/04/17 18:29, Igor Stoppa wrote:
>>
>>> On 26/04/17 17:47, Michal Hocko wrote:
>>
>> [...]
>>
>>>> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
>>>> here so I suspect you have based your change on the Linus tree.
>>
>>> I used your tree from kernel.org
>>
>> I found it, I was using master, instead of auto-latest (is it correct?)
>
> yes
>
>> But now I see something that I do not understand (apologies if I'm
>> asking something obvious).
>>
>> First there is:
>>
>> [...]
>> #define ___GFP_WRITE 0x800000u
>> #define ___GFP_KSWAPD_RECLAIM 0x1000000u
>> #ifdef CONFIG_LOCKDEP
>> #define ___GFP_NOLOCKDEP 0x4000000u
>> #else
>> #define ___GFP_NOLOCKDEP 0
>> #endif
>>
>> Then:
>>
>> /* Room for N __GFP_FOO bits */
>> #define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))
>>
>>
>>
>> Shouldn't it be either:
>> ___GFP_NOLOCKDEP 0x2000000u
>
> Yes it should. At the time when this patch was written this value was
> used. Later I've removed __GFP_OTHER by 41b6167e8f74 ("mm: get rid of
> __GFP_OTHER_NODE") and forgot to refresh this one. Thanks for noticing
> this.
>
> Andrew, could you fold the following in please?
> ---
> From 8dc9c917af215f659bb990fa48ae7b4753027c19 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 27 Apr 2017 15:28:10 +0200
> Subject: [PATCH] lockdep-allow-to-disable-reclaim-lockup-detection-fix
>
> Igor Stoppa has noticed that __GFP_NOLOCKDEP can use a lower bit. At the
> time lockdep-allow-to-disable-reclaim-lockup-detection was written we
> still had __GFP_OTHER_NODE but I have removed it in 41b6167e8f74 ("mm:
> get rid of __GFP_OTHER_NODE") and forgot to lower the bit value.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Ping, I have noticed (at least in the mmotm-2017-05-08-16-30 git tag)
there's still 0x4000000u ?
> ---
> include/linux/gfp.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 2b1a44f5bdb6..a89d37e8b387 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -41,7 +41,7 @@ struct vm_area_struct;
> #define ___GFP_WRITE 0x800000u
> #define ___GFP_KSWAPD_RECLAIM 0x1000000u
> #ifdef CONFIG_LOCKDEP
> -#define ___GFP_NOLOCKDEP 0x4000000u
> +#define ___GFP_NOLOCKDEP 0x2000000u
> #else
> #define ___GFP_NOLOCKDEP 0
> #endif
>
--
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>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Igor Stoppa @ 2017-05-10 15:19 UTC (permalink / raw)
To: Michal Hocko, Dave Hansen, Laura Abbott
Cc: linux-mm, linux-kernel, kernel-hardening@lists.openwall.com
In-Reply-To: <20170510114319.GK31466@dhcp22.suse.cz>
On 10/05/17 14:43, Michal Hocko wrote:
> On Wed 10-05-17 11:57:42, Igor Stoppa wrote:
>> On 10/05/17 11:05, Michal Hocko wrote:
> [...]
>>> To me it seems that this being an initialization mostly thingy a simple
>>> allocator which manages a pool of pages (one set of sealed and one for
>>> allocations)
>>
>> Shouldn't also the set of pages used for keeping track of the others be
>> sealed? Once one is ro, also the other should not change.
>
> Heh, that really depends how much consistency and robustness you want to
> achieve. It is really hard to defend against targeted attacks against
> the allocator metadata when a code is running in the kernel.
Taking the trouble to implement the sealing, then anything that doesn't
have a justification for staying R/W is fair game for sealing, IMHO.
>>> and which only appends new objects as they fit to unsealed
>>> pages would be sufficient for starter.
>>
>> Any "free" that might happen during the initialization transient, would
>> actually result in an untracked gap, right?
>
> yes. And once the whole page is free it would get unsealed and returned
> to the (page) allocator.
Which means that there must be some way to track the freeing.
I intentionally omitted it, because I wasn't sure it would still be
compatible with the idea of a simple linear allocator.
> This approach would inevitably lead to internal
> fragmentation but reducing that would require a pool which is shared for
> objects with the common life cycle which is quite hard with requirements
> you have (you would have to convey the allocation context to all users
> somehow).
What if the users were unaware of most of the context and would only use
some flag, say GFP_SEAL?
Shouldn't the allocator be the only one aware of the context?
Context being the actual set of pages used.
Other idea: for each logical group of objects having same lifecycle,
define a pool, then do linear allocation within the pool for the
respective logical group.
Still some way would be needed to track the utilization of each page,
but it would ensure that when a logical group is discarded, all its
related pages are freed.
>> What about the size of the pool of pages?
>
> I wouldn't see that as a big deal. New pages would be allocated as
> needed.
ok
[...]
>> - modules: unloading and reloading modules will eventually lead to
>> permanently lost pages, in increasing number.
>
> Each module should free all objects that were allocated on its behalf
> and that should result in pages being freed as well
Only if the objects are enforced to be contiguous and the start is at
the beginning of a page, which seems to go in the direction of having a
memory pool for each module.
>> Loading/unloading repeatedly the same module is probably not so common,
>> with a major exception being USB, where almost anything can show up.
>> And disappear.
>> This seems like a major showstopper for the linear allocator you propose.
>
> I am not sure I understand. If such a module kept allocations behind it
> would be a memory leak no matter what.
What I had in mind is that, with a global linear allocator _without_
support for returning "freed" pages, there would be a memory consumption
progressively increasing.
But even if the module frees correctly its allocations and they are
tracked correctly, it's still possible that some page doesn't get
returned, unless the module had started using data from the beginning of
a brand new page and nothing else but that module used it.
So it really looks like we are discussing a per-module (linear) allocator.
Probably that's what you meant all the time and I just realized it now ...
>> My reasoning in pursuing the kmalloc approach was that it is already
>> equipped with mechanisms for dealing with these sort of cases, where
>> memory can be fragmented.
>
> Yeah, but kmalloc is optimized for a completely different usecase. You
> can reuse same pages again and again while you clearly cannot do the
> same once you seal a page and make it read only.
No, but during the allocation transient, I could.
Cons: less protection for what is already in the page.
Pros: tighter packing.
> Well unless you want to
> open time windows when the page stops being RO or use a different
> mapping for the allocator.
Yes, I was proposing to temporarily make the specific page RW.
> But try to consider how many features of the slab allocator you are
> actually going to need wrt. to tweaks it would have to implement to
> support this new use case. Maybe duplicating general purpose caches and
> creating specialized explicitly is a viable path. I haven't tried
> it.
>
>> I also wouldn't risk introducing bugs with my homebrew allocator ...
>>
>> The initial thought was that there could be a master toggle to
>> seal/unseal all the memory affected.
>>
>> But you were not too excited, iirc :-D
>
> yes, If there are different users a pool (kmem_cache like) would be more
> natural.
>
>> Alternatively, kmalloc could be enhanced to unseal only the pages it
>> wants to modify.
>
> You would have to stop the world to prevent from an accidental overwrite
> during that time. Which makes the whole thing quite dubious IMHO.
>
>> I don't think much can be done for data that is placed together, in the
>> same page with something that needs to be altered.
>> But what is outside of that page could still enjoy the protection from
>> the seal.
Recap
-----
The latest proposal would be (I can create a new version of the RFC if
preferred):
Have a per-module linear memory allocator, using on on-demand new pages,
with some way to track:
* pages used in each pool (ex: a next ptr in each page)
* free space at the end of the page
the allocation would be aligned accordingly to arch requirements
implemented, for example, with a counter
Pages are sealed as soon as they fill up and a next one is allocated.
Or they are explicitly sealed by their respective module.
In the typical case the freeing would happen on the entire pool, for
example when the module is unloaded.
It might be even nicer to have a master teardown call, for the whole pool.
There is still the problem of how to deal with large physical pages and
kmalloc.
Having a per-module pool of pages is likely to generate even more waste,
if the pages are particularly large.
So I'd like to play a little what-if scenario:
what if I was to support exclusively virtual memory and convert to it
everything that might need sealing?
I cannot find any reason why this could not be done, even if the
original code uses kmalloc.
Extension
---------
What I discussed so far is about things that are not expected to change.
At most they would be freed, as units.
However, if some other data exhibits quasi-or-characteristics, it could
be protected as well.
With the understanding that there would be holes in the memory
allocation and a linear allocator probably would not be enough anymore.
This could be achieved by keeping a bitmaps of machine aligned words.
Ex: a 4k page with 8bytes words would need 64 bytes.
--
igor
--
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>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox