On Mon, May 22, 2023 at 11:08:02AM +0800, zhaoyang.huang wrote:
> +static bool skip_cma(struct page *page, struct scan_control *sc)
> +{
> + if (!current_is_kswapd() && gfp_migratetype(sc->gfp_mask) != MIGRATE_MOVABLE
> + && get_pageblock_migratetype(page) == MIGRATE_CMA)
> + return true;
Putting the 'return' at the same level of indentation as the second half
of the conditional is wrong. It confuses the reader. Also, the &&
needs to go at the end of the line not the beginning (read the
codingstyle documentation!) Also there's no good reason to use such
long lines. ie do this instead:
if (!current_is_kswapd() &&
gfp_migratetype(sc->gfp_mask) != MIGRATE_MOVABLE &&
get_pageblock_migratetype(page) == MIGRATE_CMA)
return true;
if you prefer, this style of indent is also acceptable:
if (!current_is_kswapd() &&
gfp_migratetype(sc->gfp_mask) != MIGRATE_MOVABLE &&
get_pageblock_migratetype(page) == MIGRATE_CMA)
return true;
> @@ -2225,10 +2242,12 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
> unsigned long nr_skipped[MAX_NR_ZONES] = { 0, };
> unsigned long skipped = 0;
> unsigned long scan, total_scan, nr_pages;
> + struct page *page;
No, don't do this.
> LIST_HEAD(folios_skipped);
>
> total_scan = 0;
> scan = 0;
> +
Don't add this completely unrelated whitespace change either.
> while (scan < nr_to_scan && !list_empty(src)) {
> struct list_head *move_to = src;
> struct folio *folio;
> @@ -2239,12 +2258,14 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
> nr_pages = folio_nr_pages(folio);
> total_scan += nr_pages;
>
> - if (folio_zonenum(folio) > sc->reclaim_idx) {
> + page = &folio->page;
> +
> + if (folio_zonenum(folio) > sc->reclaim_idx
> + || skip_cma(page, sc)) {
Again, this is not where the || goes.
And skip_cma() should take a folio, not a page. It's unreasonable
to ask you to convert get_pageblock_migratetype(),
get_pfnblock_flags_mask(), __get_pfnblock_flags_mask(), etc to
use a folio (... although someone looking for a project could do that
...). Instead, you should do the folio->page conversion inside
skip_cma().
> nr_skipped[folio_zonenum(folio)] += nr_pages;
> move_to = &folios_skipped;
> goto move;
> }
> -
No unnecessary whitespace changes.
