On Tue, Jan 26, 2021 at 1:49 AM Vlastimil Babka <vbabka@xxxxxxx> wrote: > > Maybe instead of replacing one magic value of 'pfn' with another > (cc->migrate_pfn with high_pfn) I would just add a "bool found" and use it > appropriately. Would make the function less subtle perhaps. I agree. Using that bool variable will look good. > > While reviewing I found more potentially questionable parts, if you're interested: > > - if we go through "if (get_pageblock_skip(freepage))... continue;" we are > increasing nr_scanned++; but not checking the limit. Yes, you've got a point. I think nr_scanned checking should be first. > > - the "if (list_is_last(freelist, &freepage->lru)) break;" part seems > unneccesary? if we are on the last page and just "continue;" the > list_for_each_entry() iteration should stop anyway. > Yes It seems unnecessary. only if the skipped block was reordered, it would be required. Then I will reflect your opinion and send a new version.
