* [PATCH] mm: vmscan: count only dirty pages as congested
@ 2014-10-15 19:58 Jamie Liu
2014-10-15 20:05 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Jamie Liu @ 2014-10-15 19:58 UTC (permalink / raw)
To: Andrew Morton, Johannes Weiner, Mel Gorman
Cc: Greg Thelen, Hugh Dickins, linux-mm, linux-kernel, Jamie Liu
shrink_page_list() counts all pages with a mapping, including clean
pages, toward nr_congested if they're on a write-congested BDI.
shrink_inactive_list() then sets ZONE_CONGESTED if nr_dirty ==
nr_congested. Fix this apples-to-oranges comparison by only counting
pages for nr_congested if they count for nr_dirty.
Signed-off-by: Jamie Liu <jamieliu@google.com>
---
mm/vmscan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index dcb4707..ad9cd9f 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -875,7 +875,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
* end of the LRU a second time.
*/
mapping = page_mapping(page);
- if ((mapping && bdi_write_congested(mapping->backing_dev_info)) ||
+ if (((dirty || writeback) && mapping &&
+ bdi_write_congested(mapping->backing_dev_info)) ||
(writeback && PageReclaim(page)))
nr_congested++;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: vmscan: count only dirty pages as congested
2014-10-15 19:58 [PATCH] mm: vmscan: count only dirty pages as congested Jamie Liu
@ 2014-10-15 20:05 ` Andrew Morton
2014-10-15 23:07 ` Jamie Liu
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2014-10-15 20:05 UTC (permalink / raw)
To: Jamie Liu
Cc: Johannes Weiner, Mel Gorman, Greg Thelen, Hugh Dickins, linux-mm,
linux-kernel
On Wed, 15 Oct 2014 12:58:35 -0700 Jamie Liu <jamieliu@google.com> wrote:
> shrink_page_list() counts all pages with a mapping, including clean
> pages, toward nr_congested if they're on a write-congested BDI.
> shrink_inactive_list() then sets ZONE_CONGESTED if nr_dirty ==
> nr_congested. Fix this apples-to-oranges comparison by only counting
> pages for nr_congested if they count for nr_dirty.
>
> ...
>
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -875,7 +875,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> * end of the LRU a second time.
> */
> mapping = page_mapping(page);
> - if ((mapping && bdi_write_congested(mapping->backing_dev_info)) ||
> + if (((dirty || writeback) && mapping &&
> + bdi_write_congested(mapping->backing_dev_info)) ||
> (writeback && PageReclaim(page)))
> nr_congested++;
What are the observed runtime effects of this change?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: vmscan: count only dirty pages as congested
2014-10-15 20:05 ` Andrew Morton
@ 2014-10-15 23:07 ` Jamie Liu
2014-10-15 23:16 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Jamie Liu @ 2014-10-15 23:07 UTC (permalink / raw)
To: Andrew Morton
Cc: Johannes Weiner, Mel Gorman, Greg Thelen, Hugh Dickins, linux-mm,
linux-kernel
wait_iff_congested() only waits if ZONE_CONGESTED is set (and at least
one BDI is still congested). Modulo concurrent changes to BDI
congestion status:
After this change, the probability that a given shrink_inactive_list()
sets ZONE_CONGESTED increases monotonically with the fraction of dirty
pages on the LRU, to 100% if all dirty pages are backed by a
write-congested BDI. This is in line with what appears to intended,
judging by the comment:
/*
* Tag a zone as congested if all the dirty pages scanned were
* backed by a congested BDI and wait_iff_congested will stall.
*/
if (nr_dirty && nr_dirty == nr_congested)
set_bit(ZONE_CONGESTED, &zone->flags);
Before this change, the probability that a given
shrink_inactive_list() sets ZONE_CONGESTED varies erratically. Because
the ZONE_CONGESTED condition is nr_dirty && nr_dirty == nr_congested,
the probability peaks when the fraction of dirty pages is equal to the
fraction of file pages backed by congested BDIs. So under some
circumstances, an increase in the fraction of dirty pages or in the
fraction of congested pages can actually result in an *decreased*
probability that reclaim will stall for writeback congestion, and vice
versa; which is both counterintuitive and counterproductive.
On Wed, Oct 15, 2014 at 1:05 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 15 Oct 2014 12:58:35 -0700 Jamie Liu <jamieliu@google.com> wrote:
>
>> shrink_page_list() counts all pages with a mapping, including clean
>> pages, toward nr_congested if they're on a write-congested BDI.
>> shrink_inactive_list() then sets ZONE_CONGESTED if nr_dirty ==
>> nr_congested. Fix this apples-to-oranges comparison by only counting
>> pages for nr_congested if they count for nr_dirty.
>>
>> ...
>>
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -875,7 +875,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>> * end of the LRU a second time.
>> */
>> mapping = page_mapping(page);
>> - if ((mapping && bdi_write_congested(mapping->backing_dev_info)) ||
>> + if (((dirty || writeback) && mapping &&
>> + bdi_write_congested(mapping->backing_dev_info)) ||
>> (writeback && PageReclaim(page)))
>> nr_congested++;
>
> What are the observed runtime effects of this change?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: vmscan: count only dirty pages as congested
2014-10-15 23:07 ` Jamie Liu
@ 2014-10-15 23:16 ` Andrew Morton
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2014-10-15 23:16 UTC (permalink / raw)
To: Jamie Liu
Cc: Johannes Weiner, Mel Gorman, Greg Thelen, Hugh Dickins, linux-mm,
linux-kernel
On Wed, 15 Oct 2014 16:07:42 -0700 Jamie Liu <jamieliu@google.com> wrote:
> On Wed, Oct 15, 2014 at 1:05 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Wed, 15 Oct 2014 12:58:35 -0700 Jamie Liu <jamieliu@google.com> wrote:
> >
> >> shrink_page_list() counts all pages with a mapping, including clean
> >> pages, toward nr_congested if they're on a write-congested BDI.
> >> shrink_inactive_list() then sets ZONE_CONGESTED if nr_dirty ==
> >> nr_congested. Fix this apples-to-oranges comparison by only counting
> >> pages for nr_congested if they count for nr_dirty.
> >>
> >> ...
> >>
> >> --- a/mm/vmscan.c
> >> +++ b/mm/vmscan.c
> >> @@ -875,7 +875,8 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >> * end of the LRU a second time.
> >> */
> >> mapping = page_mapping(page);
> >> - if ((mapping && bdi_write_congested(mapping->backing_dev_info)) ||
> >> + if (((dirty || writeback) && mapping &&
> >> + bdi_write_congested(mapping->backing_dev_info)) ||
> >> (writeback && PageReclaim(page)))
> >> nr_congested++;
> >
> > What are the observed runtime effects of this change?
>
> wait_iff_congested() only waits if ZONE_CONGESTED is set (and at least
> one BDI is still congested). Modulo concurrent changes to BDI
> congestion status:
>
> After this change, the probability that a given shrink_inactive_list()
> sets ZONE_CONGESTED increases monotonically with the fraction of dirty
> pages on the LRU, to 100% if all dirty pages are backed by a
> write-congested BDI. This is in line with what appears to intended,
> judging by the comment:
>
> /*
> * Tag a zone as congested if all the dirty pages scanned were
> * backed by a congested BDI and wait_iff_congested will stall.
> */
> if (nr_dirty && nr_dirty == nr_congested)
> set_bit(ZONE_CONGESTED, &zone->flags);
>
> Before this change, the probability that a given
> shrink_inactive_list() sets ZONE_CONGESTED varies erratically. Because
> the ZONE_CONGESTED condition is nr_dirty && nr_dirty == nr_congested,
> the probability peaks when the fraction of dirty pages is equal to the
> fraction of file pages backed by congested BDIs. So under some
> circumstances, an increase in the fraction of dirty pages or in the
> fraction of congested pages can actually result in an *decreased*
> probability that reclaim will stall for writeback congestion, and vice
> versa; which is both counterintuitive and counterproductive.
(top-posting repaired. Please don't do that!)
OK, I buy all that. But has any runtime testing been performed to
confirm this and to quantify the effects?
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-15 23:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-15 19:58 [PATCH] mm: vmscan: count only dirty pages as congested Jamie Liu
2014-10-15 20:05 ` Andrew Morton
2014-10-15 23:07 ` Jamie Liu
2014-10-15 23:16 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).