On Wed, Jul 10, 2024 at 9:53 AM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
>
> Yafang Shao <laoar.shao@xxxxxxxxx> writes:
>
> > When adjusting the CONFIG_PCP_BATCH_SCALE_MAX configuration from its
> > default value of 5 to a lower value, such as 0, it's important to ensure
> > that the pcp->high decaying is not inadvertently slowed down. Similarly,
> > when increasing CONFIG_PCP_BATCH_SCALE_MAX to a larger value, like 6, we
> > must avoid inadvertently increasing the number of pages freed in
> > free_pcppages_bulk() as a result of this change.
> >
> > So below improvements are made:
> > - hardcode the default value of 5 to avoiding modifying the pcp->high
> > - refactore free_pcppages_bulk() into multiple steps, with each step
> > processing a fixed batch size of pages
>
> This is confusing. You don't change free_pcppages_bulk() itself. I
> guess what you mean is "change free_pcppages_bulk() calling into
> multiple steps".
will change it.
>
> >
> > Suggested-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > ---
> > mm/page_alloc.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 8e2f4e1ab4f2..2b76754a48e0 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2247,7 +2247,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
> > int decay_pcp_high(struct zone *zone, struct per_cpu_pages *pcp)
> > {
> > int high_min, to_drain, batch;
> > - int todo = 0;
> > + int todo = 0, count = 0;
> >
> > high_min = READ_ONCE(pcp->high_min);
> > batch = READ_ONCE(pcp->batch);
> > @@ -2257,18 +2257,25 @@ int decay_pcp_high(struct zone *zone, struct per_cpu_pages *pcp)
> > * control latency. This caps pcp->high decrement too.
> > */
> > if (pcp->high > high_min) {
> > - pcp->high = max3(pcp->count - (batch << CONFIG_PCP_BATCH_SCALE_MAX),
> > + /* When tuning the pcp batch scale value, we want to ensure that
> > + * the pcp->high decay rate is not slowed down. Therefore, we
> > + * hard-code the historical default scale value of 5 here to
> > + * prevent any unintended effects.
> > + */
>
> This is good description for history. But, in the result code, it's
> not easy for people to connect the code with pcp batch scale directly.
> How about something as follows,
>
> We will decay 1/8 pcp->high each time in general, so that the idle PCP
> pages can be returned to buddy system timely. To control the max
> latency of decay, we also constrain the number pages freed each time.
Thanks for your suggestion.
will change it.
>
> > + pcp->high = max3(pcp->count - (batch << 5),
> > pcp->high - (pcp->high >> 3), high_min);
> > if (pcp->high > high_min)
> > todo++;
> > }
> >
> > to_drain = pcp->count - pcp->high;
> > - if (to_drain > 0) {
> > + while (count < to_drain) {
> > spin_lock(&pcp->lock);
> > - free_pcppages_bulk(zone, to_drain, pcp, 0);
> > + free_pcppages_bulk(zone, batch, pcp, 0);
>
> "to_drain - count" may < batch.
Nice catch. will fix it.
>
> > spin_unlock(&pcp->lock);
> > + count += batch;
> > todo++;
> > + cond_resched();
> > }
> >
> > return todo;
>
> --
> Best Regards,
> Huang, Ying
--
Regards
Yafang
