Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Mar 2022 03:22:51 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Wei Yang <richard.weiyang@gmail.com>
Cc:     "Huang, Ying" <ying.huang@intel.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        mgorman@techsingularity.net
Subject: Re: [PATCH 2/2] mm/vmscan: make sure wakeup_kswapd with managed zone
Message-ID: <YkJte/L4jZv47FOO@casper.infradead.org>
References: <20220327024101.10378-1-richard.weiyang@gmail.com>
 <20220327024101.10378-2-richard.weiyang@gmail.com>
 <8735j2opd9.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <20220329004146.2xdswvrm2qu7f47x@master>
 <875ynxh9lg.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <20220329015230.hneciyfxoxtvfytl@master>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220329015230.hneciyfxoxtvfytl@master>
Precedence: bulk

On Tue, Mar 29, 2022 at 01:52:30AM +0000, Wei Yang wrote:
> @@ -1985,14 +1985,13 @@ static bool migrate_balanced_pgdat(struct pglist_data *pgdat,
>  			continue;
>  
>  		/* Avoid waking kswapd by allocating pages_to_migrate pages. */
> -		if (!zone_watermark_ok(zone, 0,
> +		if (zone_watermark_ok(zone, 0,
>  				       high_wmark_pages(zone) +
>  				       nr_migrate_pages,
>  				       ZONE_MOVABLE, 0))

Someone's done the silly thing of lining up all of these with spaces,
so either all these lines also need to be shrunk by one space, or you
need to break that convention and just go to a reasonable number of
tabs.  I'd do it like this:

		if (zone_watermark_ok(zone, 0,
				high_wmark_pages(zone) + nr_migrate_pages,
				ZONE_MOVABLE, 0))

but not everybody would.

> @@ -2040,16 +2040,11 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  		return 0;
>  
>  	/* Avoid migrating to a node that is nearly full */
> -	if (!migrate_balanced_pgdat(pgdat, nr_pages)) {
> -		int z;
> -
> +	if ((zone = migrate_balanced_pgdat(pgdat, nr_pages))) {

Linus had a rant about this style recently.  He much prefers:

	zone = migrate_balanced_pgdat(pgdat, nr_pages);
	if (zone) {

(the exception is for while loops:

	while ((zone = migrate_balanced_pgdat(pgdat, nr_pages)) != NULL)

where he wants to see the comparison against NULL instead of the awkard
double-bracket)