Re: [PATCH 2/3] mm/page_alloc: Avoid changing pcp->high decaying when adjusting CONFIG_PCP_BATCH_SCALE_MAX

["Huang, Ying" <ying.huang@intel.com>]

Yafang Shao <laoar.shao@gmail.com> 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".

>
> Suggested-by: "Huang, Ying" <ying.huang@intel.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  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.

> +		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.

>  		spin_unlock(&pcp->lock);
> +		count += batch;
>  		todo++;
> +		cond_resched();
>  	}
>  
>  	return todo;

--
Best Regards,
Huang, Ying