Re: [PATCH] kswapd should only wait on IO if there is IO
On Thu, 27 Sep 2007 15:59:07 -0700 Andrew Morton <[EMAIL PROTECTED]> wrote: > And lost the changelog ;) Good point. The current kswapd (and try_to_free_pages) code has an oddity where the code will wait on IO, even if there is no IO in flight. This problem is notable especially when the system scans through many unfreeable pages, causing unnecessary stalls in the VM. Additionally, tasks without __GFP_FS or __GFP_IO in the direct reclaim path will sleep if a significant number of pages are encountered that should be written out. This gives kswapd a chance to write out those pages, while the direct reclaim task sleeps. Signed-off-by: Rik van Riel <[EMAIL PROTECTED]> diff -up linux-2.6.22/mm/vmscan.c.wait linux-2.6.22/mm/vmscan.c --- linux-2.6.22/mm/vmscan.c.wait 2007-09-27 18:45:57.0 -0400 +++ linux-2.6.22/mm/vmscan.c2007-09-27 18:48:43.0 -0400 @@ -68,6 +68,13 @@ struct scan_control { int all_unreclaimable; int order; + + /* +* Pages that have (or should have) IO pending. If we run into +* a lot of these, we're better off waiting a little for IO to +* finish rather than scanning more pages in the VM. +*/ + int nr_io_pages; }; #define lru_to_page(_head) (list_entry((_head)->prev, struct page, lru)) @@ -489,8 +496,10 @@ static unsigned long shrink_page_list(st */ if (sync_writeback == PAGEOUT_IO_SYNC && may_enter_fs) wait_on_page_writeback(page); - else + else { + sc->nr_io_pages++; goto keep_locked; + } } referenced = page_referenced(page, 1); @@ -529,8 +538,10 @@ static unsigned long shrink_page_list(st if (PageDirty(page)) { if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced) goto keep_locked; - if (!may_enter_fs) + if (!may_enter_fs) { + sc->nr_io_pages++; goto keep_locked; + } if (!sc->may_writepage) goto keep_locked; @@ -541,8 +552,10 @@ static unsigned long shrink_page_list(st case PAGE_ACTIVATE: goto activate_locked; case PAGE_SUCCESS: - if (PageWriteback(page) || PageDirty(page)) + if (PageWriteback(page) || PageDirty(page)) { + sc->nr_io_pages++; goto keep; + } /* * A synchronous write - probably a ramdisk. Go * ahead and try to reclaim the page. @@ -1201,6 +1214,7 @@ unsigned long try_to_free_pages(struct z for (priority = DEF_PRIORITY; priority >= 0; priority--) { sc.nr_scanned = 0; + sc.nr_io_pages = 0; if (!priority) disable_swap_token(); nr_reclaimed += shrink_zones(priority, zones, &sc); @@ -1229,7 +1243,8 @@ unsigned long try_to_free_pages(struct z } /* Take a nap, wait for some writeback to complete */ - if (sc.nr_scanned && priority < DEF_PRIORITY - 2) + if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && + sc.nr_io_pages > sc.swap_cluster_max) congestion_wait(WRITE, HZ/10); } /* top priority shrink_caches still had more to do? don't OOM, then */ @@ -1315,6 +1330,7 @@ loop_again: if (!priority) disable_swap_token(); + sc.nr_io_pages = 0; all_zones_ok = 1; /* @@ -1398,7 +1414,8 @@ loop_again: * OK, kswapd is getting into trouble. Take a nap, then take * another pass across the zones. */ - if (total_scanned && priority < DEF_PRIORITY - 2) + if (total_scanned && priority < DEF_PRIORITY - 2 && + sc.nr_io_pages > sc.swap_cluster_max) congestion_wait(WRITE, HZ/10); /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kswapd should only wait on IO if there is IO
On Thu, 27 Sep 2007 18:50:27 -0400 Rik van Riel <[EMAIL PROTECTED]> wrote: > On Thu, 27 Sep 2007 15:21:21 -0700 > Andrew Morton <[EMAIL PROTECTED]> wrote: > > > > Nope, sc.nr_io_pages will also be incremented when the code runs into > > > pages that are already PageWriteback. > > > > yup, I didn't think of that. Hopefully someone else will be in there > > working on that zone too. If this caller yields and defers to kswapd > > then that's very likely. Except we just took away the ability to do that.. > > if (PageDirty(page)) { > if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && > referenced) > goto keep_locked; > if (!may_enter_fs) > goto keep_locked; > > I think we can fix that problem by adding a sc->nr_io_pages++ > between the last if and the goto keep_locked in shrink_page_list. > > That way !GFP_IO or !GFP_FS tasks will cause themselves to sleep > if there are pages that need to be written out, even if those > pages are not in flight to disk yet. yeah, that's prudent I guess. > I have also added the comment you wanted. And lost the changelog ;) > - if (sc.nr_scanned && priority < DEF_PRIORITY - 2) > + if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && > + sc.nr_io_pages > sc.swap_cluster_max) I do think this design decision needs a bit of explanation too. > congestion_wait(WRITE, HZ/10); > } > /* top priority shrink_caches still had more to do? don't OOM, then */ > @@ -1315,6 +1330,7 @@ loop_again: > if (!priority) > disable_swap_token(); > > + sc.nr_io_pages = 0; > all_zones_ok = 1; > > /* > @@ -1398,7 +1414,8 @@ loop_again: >* OK, kswapd is getting into trouble. Take a nap, then take >* another pass across the zones. >*/ > - if (total_scanned && priority < DEF_PRIORITY - 2) > + if (total_scanned && priority < DEF_PRIORITY - 2 && As did that one. Ho hum :( Maybe it's in the git history somewhere. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kswapd should only wait on IO if there is IO
On Thu, 27 Sep 2007 15:21:21 -0700 Andrew Morton <[EMAIL PROTECTED]> wrote: > > Nope, sc.nr_io_pages will also be incremented when the code runs into > > pages that are already PageWriteback. > > yup, I didn't think of that. Hopefully someone else will be in there > working on that zone too. If this caller yields and defers to kswapd > then that's very likely. Except we just took away the ability to do that.. if (PageDirty(page)) { if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced) goto keep_locked; if (!may_enter_fs) goto keep_locked; I think we can fix that problem by adding a sc->nr_io_pages++ between the last if and the goto keep_locked in shrink_page_list. That way !GFP_IO or !GFP_FS tasks will cause themselves to sleep if there are pages that need to be written out, even if those pages are not in flight to disk yet. I have also added the comment you wanted. Signed-off-by: Rik van Riel <[EMAIL PROTECTED]> diff -up linux-2.6.23-rc7/mm/vmscan.c.wait linux-2.6.22/mm/vmscan.c --- linux-2.6.23-rc7/mm/vmscan.c.wait 2007-09-27 18:45:57.0 -0400 +++ linux-2.6.23-rc7/mm/vmscan.c2007-09-27 18:48:43.0 -0400 @@ -68,6 +68,13 @@ struct scan_control { int all_unreclaimable; int order; + + /* +* Pages that have (or should have) IO pending. If we run into +* a lot of these, we're better off waiting a little for IO to +* finish rather than scanning more pages in the VM. +*/ + int nr_io_pages; }; #define lru_to_page(_head) (list_entry((_head)->prev, struct page, lru)) @@ -489,8 +496,10 @@ static unsigned long shrink_page_list(st */ if (sync_writeback == PAGEOUT_IO_SYNC && may_enter_fs) wait_on_page_writeback(page); - else + else { + sc->nr_io_pages++; goto keep_locked; + } } referenced = page_referenced(page, 1); @@ -529,8 +538,10 @@ static unsigned long shrink_page_list(st if (PageDirty(page)) { if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced) goto keep_locked; - if (!may_enter_fs) + if (!may_enter_fs) { + sc->nr_io_pages++; goto keep_locked; + } if (!sc->may_writepage) goto keep_locked; @@ -541,8 +552,10 @@ static unsigned long shrink_page_list(st case PAGE_ACTIVATE: goto activate_locked; case PAGE_SUCCESS: - if (PageWriteback(page) || PageDirty(page)) + if (PageWriteback(page) || PageDirty(page)) { + sc->nr_io_pages++; goto keep; + } /* * A synchronous write - probably a ramdisk. Go * ahead and try to reclaim the page. @@ -1201,6 +1214,7 @@ unsigned long try_to_free_pages(struct z for (priority = DEF_PRIORITY; priority >= 0; priority--) { sc.nr_scanned = 0; + sc.nr_io_pages = 0; if (!priority) disable_swap_token(); nr_reclaimed += shrink_zones(priority, zones, &sc); @@ -1229,7 +1243,8 @@ unsigned long try_to_free_pages(struct z } /* Take a nap, wait for some writeback to complete */ - if (sc.nr_scanned && priority < DEF_PRIORITY - 2) + if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && + sc.nr_io_pages > sc.swap_cluster_max) congestion_wait(WRITE, HZ/10); } /* top priority shrink_caches still had more to do? don't OOM, then */ @@ -1315,6 +1330,7 @@ loop_again: if (!priority) disable_swap_token(); + sc.nr_io_pages = 0; all_zones_ok = 1; /* @@ -1398,7 +1414,8 @@ loop_again: * OK, kswapd is getting into trouble. Take a nap, then take * another pass across the zones. */ - if (total_scanned && priority < DEF_PRIORITY - 2) + if (total_scanned && priority < DEF_PRIORITY - 2 && + sc.nr_io_pages > sc.swap_cluster_max) congestion_wait(WRITE, HZ/10); /* - To unsubscribe from this list: se
Re: [PATCH] kswapd should only wait on IO if there is IO
On Thu, 27 Sep 2007 18:13:25 -0400 Rik van Riel <[EMAIL PROTECTED]> wrote: > On Thu, 27 Sep 2007 14:47:02 -0700 > Andrew Morton <[EMAIL PROTECTED]> wrote: > > > On Thu, 27 Sep 2007 17:08:16 -0400 > > Rik van Riel <[EMAIL PROTECTED]> wrote: > > > > > The current kswapd (and try_to_free_pages) code has an oddity where the > > > code will wait on IO, even if there is no IO in flight. This problem is > > > notable especially when the system scans through many unfreeable pages, > > > causing unnecessary stalls in the VM. > > > > > > > What effect did this change have? > > Kswapd was no longer sitting in "D" state as often and pages got > freed more promptly. The test was done on a RHEL kernel with > this change though - I guess I should redo it with a current upstream > kernel. OK. Yes, it should help quite a bit in the common cases. > > > > > > /* Take a nap, wait for some writeback to complete */ > > > - if (sc.nr_scanned && priority < DEF_PRIORITY - 2) > > > + if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && > > > + sc.nr_io_pages > sc.swap_cluster_max) > > > > The comparison with swap_cluster_max is unobvious, and merits a > > comment. What is the thinking here? > > If the number of pages undergoing IO is really small, waiting > for them may be a waste of time. > > Maybe my thinking is wrong, not sure... The thinking sounds good to me, but I'm looking for weirdo side-effects in corner cases. And I'm trying to work out what actual design we want to have behind these various magic numbers and thresholds. > > Also, we now have this: > > > > if (total_scanned > sc.swap_cluster_max + > > sc.swap_cluster_max / 2) { > > wakeup_pdflush(laptop_mode ? 0 : total_scanned); > > sc.may_writepage = 1; > > } > > > > /* Take a nap, wait for some writeback to complete */ > > if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && > > sc.nr_io_pages > sc.swap_cluster_max) > > congestion_wait(WRITE, HZ/10); > > > > > > So in the case where total_scanned has not yet reached > > swap_cluster_max, this process isn't initiating writeout and it isn't > > sleeping, either. Nor is it incrementing nr_io_pages. > > Actually, nr_io_pages is also incremented when we run into pages that > are already PageWriteback - even if we did not start the IO ourselves. OK, that'll help a lot in this scenario. > > In the range (swap_cluster_max < nr_io_pages < 1.5*swap_cluster_max) this > > process still isn't incrementing nr_io_pages, but it _is_ running > > congestion_wait(). > > It is incrementing sc.nr_io_pages and will wait on IO to complete if > the amount of pages in flight to disk that it scanned over is larger > than the number of pages that it is trying to free. > > > Once nr_io_pages exceeds 1.5*swap_cluster_max, this process is both > > initiating IO and is throttling on writeback completion events. > > > > This all seems a bit weird and arbitrary - what is the reason for > > throttling-but-not-writing in that 1.0->1.5 window? > > Good question. Note that the throttling-but-not-writing window in > the current code is 0.0->1.5, so this patch does reduce the throttling > window compared to the current code. > > What is the reason that the current code does IO throttling even if > there is no IO at all in flight? Buggered if I know ;) It may have the accidental effect that it opens a window in which some may_enter_fs-capable process can get scheduled and do some writeout, perhaps. > > If there _is_ a reason and it's all been carefully thought out and > > designed, then can we please capture a description of that design in the > > changelog or in the code? > > I'll add a description for the sc.nr_io_pages > sc.swap_cluster_max > test. OK, thanks. Perhaps a few words tacked onto the nr_io_pages definition site would be the place to capture this. > > Also, I wonder about what this change will do to the dynamic behaviour of > > GFP_NOFS direct-reclaimers. Previously they would throttle if they > > encounter dirty pages which they can't write out. Hopefully someone else > > (kswapd or a __GFP_FS direct-reclaimer) will write some of those pages > > and this caller will be woken when that writeout completes and will go off > > and scoop them off the tail of the LRU. > > > > But after this change, such a GFP_NOFS caller will, afacit, burn its way > > through potentially the entire inactive list and will then declare oom. > > Nope, sc.nr_io_pages will also be incremented when the code runs into > pages that are already PageWriteback. yup, I didn't think of that. Hopefully someone else will be in there working on that zone too. If this caller yields and defers to kswapd then that's very likely. Except we just took away the ability to do that.. - To unsubscribe from this lis
Re: [PATCH] kswapd should only wait on IO if there is IO
On Thu, 27 Sep 2007 14:47:02 -0700 Andrew Morton <[EMAIL PROTECTED]> wrote: > On Thu, 27 Sep 2007 17:08:16 -0400 > Rik van Riel <[EMAIL PROTECTED]> wrote: > > > The current kswapd (and try_to_free_pages) code has an oddity where the > > code will wait on IO, even if there is no IO in flight. This problem is > > notable especially when the system scans through many unfreeable pages, > > causing unnecessary stalls in the VM. > > > > What effect did this change have? Kswapd was no longer sitting in "D" state as often and pages got freed more promptly. The test was done on a RHEL kernel with this change though - I guess I should redo it with a current upstream kernel. > > diff -up linux-2.6.22.x86_64/mm/vmscan.c.wait > > linux-2.6.22.x86_64/mm/vmscan.c > > --- linux-2.6.22.x86_64/mm/vmscan.c.wait2007-09-25 11:33:30.0 > > -0400 > > +++ linux-2.6.22.x86_64/mm/vmscan.c 2007-09-25 21:27:08.0 -0400 > > @@ -68,6 +68,8 @@ struct scan_control { > > int all_unreclaimable; > > > > int order; > > + > > + int nr_io_pages; > > }; > > > > #define lru_to_page(_head) (list_entry((_head)->prev, struct page, lru)) > > @@ -489,8 +491,10 @@ static unsigned long shrink_page_list(st > > */ > > if (sync_writeback == PAGEOUT_IO_SYNC && may_enter_fs) > > wait_on_page_writeback(page); > > - else > > + else { > > + sc->nr_io_pages++; > > goto keep_locked; > > + } > > } > > > > referenced = page_referenced(page, 1); > > @@ -541,8 +545,10 @@ static unsigned long shrink_page_list(st > > case PAGE_ACTIVATE: > > goto activate_locked; > > case PAGE_SUCCESS: > > - if (PageWriteback(page) || PageDirty(page)) > > + if (PageWriteback(page) || PageDirty(page)) { > > + sc->nr_io_pages++; > > goto keep; > > + } > > /* > > * A synchronous write - probably a ramdisk. Go > > * ahead and try to reclaim the page. > > @@ -1201,6 +1207,7 @@ unsigned long try_to_free_pages(struct z > > > > for (priority = DEF_PRIORITY; priority >= 0; priority--) { > > sc.nr_scanned = 0; > > + sc.nr_io_pages = 0; > > if (!priority) > > disable_swap_token(); > > nr_reclaimed += shrink_zones(priority, zones, &sc); > > @@ -1229,7 +1236,8 @@ unsigned long try_to_free_pages(struct z > > } > > > > /* Take a nap, wait for some writeback to complete */ > > - if (sc.nr_scanned && priority < DEF_PRIORITY - 2) > > + if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && > > + sc.nr_io_pages > sc.swap_cluster_max) > > The comparison with swap_cluster_max is unobvious, and merits a > comment. What is the thinking here? If the number of pages undergoing IO is really small, waiting for them may be a waste of time. Maybe my thinking is wrong, not sure... > Also, we now have this: > > if (total_scanned > sc.swap_cluster_max + > sc.swap_cluster_max / 2) { > wakeup_pdflush(laptop_mode ? 0 : total_scanned); > sc.may_writepage = 1; > } > > /* Take a nap, wait for some writeback to complete */ > if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && > sc.nr_io_pages > sc.swap_cluster_max) > congestion_wait(WRITE, HZ/10); > > > So in the case where total_scanned has not yet reached > swap_cluster_max, this process isn't initiating writeout and it isn't > sleeping, either. Nor is it incrementing nr_io_pages. Actually, nr_io_pages is also incremented when we run into pages that are already PageWriteback - even if we did not start the IO ourselves. > In the range (swap_cluster_max < nr_io_pages < 1.5*swap_cluster_max) this > process still isn't incrementing nr_io_pages, but it _is_ running > congestion_wait(). It is incrementing sc.nr_io_pages and will wait on IO to complete if the amount of pages in flight to disk that it scanned over is larger than the number of pages that it is trying to free. > Once nr_io_pages exceeds 1.5*swap_cluster_max, this process is both > initiating IO and is throttling on writeback completion events. > > This all seems a bit weird and arbitrary - what is the reason for > throttling-but-not-writing in that 1.0->1.5 window? Good question. Note that the throttling-but-not-writing window in the current code is 0.0->1.5, so this patch does reduce the throttling window compared to the c
Re: [PATCH] kswapd should only wait on IO if there is IO
On Thu, 27 Sep 2007 17:08:16 -0400 Rik van Riel <[EMAIL PROTECTED]> wrote: > The current kswapd (and try_to_free_pages) code has an oddity where the > code will wait on IO, even if there is no IO in flight. This problem is > notable especially when the system scans through many unfreeable pages, > causing unnecessary stalls in the VM. > What effect did this change have? > > diff -up linux-2.6.22.x86_64/mm/vmscan.c.wait linux-2.6.22.x86_64/mm/vmscan.c > --- linux-2.6.22.x86_64/mm/vmscan.c.wait 2007-09-25 11:33:30.0 > -0400 > +++ linux-2.6.22.x86_64/mm/vmscan.c 2007-09-25 21:27:08.0 -0400 > @@ -68,6 +68,8 @@ struct scan_control { > int all_unreclaimable; > > int order; > + > + int nr_io_pages; > }; > > #define lru_to_page(_head) (list_entry((_head)->prev, struct page, lru)) > @@ -489,8 +491,10 @@ static unsigned long shrink_page_list(st >*/ > if (sync_writeback == PAGEOUT_IO_SYNC && may_enter_fs) > wait_on_page_writeback(page); > - else > + else { > + sc->nr_io_pages++; > goto keep_locked; > + } > } > > referenced = page_referenced(page, 1); > @@ -541,8 +545,10 @@ static unsigned long shrink_page_list(st > case PAGE_ACTIVATE: > goto activate_locked; > case PAGE_SUCCESS: > - if (PageWriteback(page) || PageDirty(page)) > + if (PageWriteback(page) || PageDirty(page)) { > + sc->nr_io_pages++; > goto keep; > + } > /* >* A synchronous write - probably a ramdisk. Go >* ahead and try to reclaim the page. > @@ -1201,6 +1207,7 @@ unsigned long try_to_free_pages(struct z > > for (priority = DEF_PRIORITY; priority >= 0; priority--) { > sc.nr_scanned = 0; > + sc.nr_io_pages = 0; > if (!priority) > disable_swap_token(); > nr_reclaimed += shrink_zones(priority, zones, &sc); > @@ -1229,7 +1236,8 @@ unsigned long try_to_free_pages(struct z > } > > /* Take a nap, wait for some writeback to complete */ > - if (sc.nr_scanned && priority < DEF_PRIORITY - 2) > + if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && > + sc.nr_io_pages > sc.swap_cluster_max) The comparison with swap_cluster_max is unobvious, and merits a comment. What is the thinking here? Also, we now have this: if (total_scanned > sc.swap_cluster_max + sc.swap_cluster_max / 2) { wakeup_pdflush(laptop_mode ? 0 : total_scanned); sc.may_writepage = 1; } /* Take a nap, wait for some writeback to complete */ if (sc.nr_scanned && priority < DEF_PRIORITY - 2 && sc.nr_io_pages > sc.swap_cluster_max) congestion_wait(WRITE, HZ/10); So in the case where total_scanned has not yet reached swap_cluster_max, this process isn't initiating writeout and it isn't sleeping, either. Nor is it incrementing nr_io_pages. In the range (swap_cluster_max < nr_io_pages < 1.5*swap_cluster_max) this process still isn't incrementing nr_io_pages, but it _is_ running congestion_wait(). Once nr_io_pages exceeds 1.5*swap_cluster_max, this process is both initiating IO and is throttling on writeback completion events. This all seems a bit weird and arbitrary - what is the reason for throttling-but-not-writing in that 1.0->1.5 window? If there _is_ a reason and it's all been carefully thought out and designed, then can we please capture a description of that design in the changelog or in the code? Also, I wonder about what this change will do to the dynamic behaviour of GFP_NOFS direct-reclaimers. Previously they would throttle if they encounter dirty pages which they can't write out. Hopefully someone else (kswapd or a __GFP_FS direct-reclaimer) will write some of those pages and this caller will be woken when that writeout completes and will go off and scoop them off the tail of the LRU. But after this change, such a GFP_NOFS caller will, afacit, burn its way through potentially the entire inactive list and will then declare oom. Non-preemtible uniprocessor kernels would be most at risk from this. > congestion_wait(WRITE, HZ/10); > } > /* top priority shrink_caches still had more to do? don't OOM, then */ > @@ -1315,6 +1323,7 @@ loop_again: > if (!priority) >