On Mon, Apr 13, 2020 at 10:59 PM Alexander Duyck
<alexander.h.duyck@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>
> 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.
