From: Liang Li <liliang324@gmail.com>
To: Alexander Duyck <alexander.h.duyck@linux.intel.com>,
liliang.opensource@gmail.com, liliang324@gmail.com
Cc: Mel Gorman <mgorman@techsingularity.net>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Andrea Arcangeli <aarcange@redhat.com>,
Dan Williams <dan.j.williams@intel.com>,
Dave Hansen <dave.hansen@intel.com>,
David Hildenbrand <david@redhat.com>,
Michal Hocko <mhocko@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [RFC PATCH 1/4] mm: reduce the impaction of page reporing worker
Date: Thu, 16 Apr 2020 09:12:16 +0800 [thread overview]
Message-ID: <CA+2MQi-u-Kd=iqoYt+Mg0Ts-wZogo3K5fSMWHGdnfCrYvt=2Yw@mail.gmail.com> (raw)
In-Reply-To: <c0bd9b1b-a0e7-5c88-ab99-e867ef25c935@linux.intel.com>
On Mon, Apr 13, 2020 at 10:59 PM Alexander Duyck
<alexander.h.duyck@linux.intel.com> wrote:
>
> On 4/12/2020 2:08 AM, liliangleo wrote:
> > When scaning the free list, 'page_reporting_cycle' may hold the
> > zone->lock for a long time when there are no reported page in the
> > free list. Setting PAGE_REPORTING_MIN_ORDER to a lower oder will
> > make this issue worse.
> >
> > Two ways were used to reduce the impact:
> > 1. Release zone lock periodicly
> > 2. Yield cpu voluntarily if needed.
> >
> > Signed-off-by: liliangleo <liliangleo@didiglobal.com>
>
> One of the reasons why I had limited this to no lower than pageblock
> order was in order to keep number of pages we would have to walk in each
> list on the smaller side.
>
> Also the lock ends up being released every time we report a batch of
> pages. It might make more sense to look at calling cond_resched after a
> batch as been submitted rather than try to introduce a new loop around
> page_reporting_cycle.
>
Hi Alexander,
My original intention is to prevent 'page_reporting_cycle' hold the
zone lock for too long
when scanning free list but there is very few pages need to report.
For PG_zero use case, it's better for users to decide the page order,
if the order is set to
a low order, the impaction will be much more serious.
Just call cond_resched after batch submission is not enough, that's
the reason why I add
cond_resched.
> > static struct page_reporting_dev_info __rcu *pr_dev_info __read_mostly;
> >
> > enum {
> > @@ -115,7 +118,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
> > unsigned int page_len = PAGE_SIZE << order;
> > struct page *page, *next;
> > long budget;
> > - int err = 0;
> > + int err = 0, scan_cnt = 0;
> >
> > /*
> > * Perform early check, if free area is empty there is
> > @@ -145,8 +148,14 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
> > /* loop through free list adding unreported pages to sg list */
> > list_for_each_entry_safe(page, next, list, lru) {
> > /* We are going to skip over the reported pages. */
> > - if (PageReported(page))
> > + if (PageReported(page)) {
> > + if (++scan_cnt >= MAX_SCAN_NUM) {
> > + err = scan_cnt;
> > + break;
> > + }
> > continue;
> > + }
> > +
> >
> > /*
> > * If we fully consumed our budget then update our
>
> Why add yet another loopvariable, why not just move our budget test to
> before the PageReported check and then increase the value?
>
The code can be refined, I just don't want to break the budget stuff.
Thanks for your feedback your work.
Liang
> > @@ -219,6 +228,26 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone,
> > return err;
> > }
> >
> > +static int
> > +reporting_order_type(struct page_reporting_dev_info *prdev, struct zone *zone,
> > + unsigned int order, unsigned int mt,
> > + struct scatterlist *sgl, unsigned int *offset)
> > +{
> > + int ret = 0;
> > + unsigned long total = 0;
> > +
> > + might_sleep();
> > + do {
> > + cond_resched();
> > + ret = page_reporting_cycle(prdev, zone, order, mt,
> > + sgl, offset);
> > + if (ret > 0)
> > + total += ret;
> > + } while (ret > 0 && total < zone->free_area[order].nr_free);
> > +
> > + return ret;
> > +}
> > +
>
> The idea behind page reporting is it is supposed to happen while the
> system is idle. As such we don't need to be in a hurry. I would get rid
> of the loop and just let the natural placing take over so that we are
> only processing something like 1/8 of the nr_free with each pass.
prev parent reply other threads:[~2020-04-16 1:12 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-12 9:08 [RFC PATCH 1/4] mm: reduce the impaction of page reporing worker liliangleo
2020-04-13 14:59 ` Alexander Duyck
2020-04-16 1:12 ` Liang Li [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+2MQi-u-Kd=iqoYt+Mg0Ts-wZogo3K5fSMWHGdnfCrYvt=2Yw@mail.gmail.com' \
--to=liliang324@gmail.com \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=alex.williamson@redhat.com \
--cc=alexander.h.duyck@linux.intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=david@redhat.com \
--cc=liliang.opensource@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).