| Message ID | cover.1676382188.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Change the return value for page isolation functions | expand |
On 14.02.23 14:59, Baolin Wang wrote: > Now the page isolation functions did not return a boolean to indicate > success or not, instead it will return a negative error when failed > to isolate a page. So below code used in most places seem a boolean > success/failure thing, which can confuse people whether the isolation > is successful. > > if (folio_isolate_lru(folio)) > continue; > > Moreover the page isolation functions only return 0 or -EBUSY, and > most users did not care about the negative error except for few users, > thus we can convert all page isolation functions to return a boolean > value, which can remove the confusion to make code more clear. > > No functional changes intended in this patch series. > > Changes from v1: > - Convert all isolation functions to return bool. Acked-by: David Hildenbrand <david@redhat.com> Although it's controversial if if (!ret) ret = -EBUSY; else ret = 0; is really appealing to the readers eye :) ret = ret ? 0 : -EBUSY; It's still confusing. would be better as ret = isolated ? 0 : -EBUSY; IOW, not reusing the "int ret" variable.
On 2/15/2023 1:52 AM, David Hildenbrand wrote: > On 14.02.23 14:59, Baolin Wang wrote: >> Now the page isolation functions did not return a boolean to indicate >> success or not, instead it will return a negative error when failed >> to isolate a page. So below code used in most places seem a boolean >> success/failure thing, which can confuse people whether the isolation >> is successful. >> >> if (folio_isolate_lru(folio)) >> continue; >> >> Moreover the page isolation functions only return 0 or -EBUSY, and >> most users did not care about the negative error except for few users, >> thus we can convert all page isolation functions to return a boolean >> value, which can remove the confusion to make code more clear. >> >> No functional changes intended in this patch series. >> >> Changes from v1: >> - Convert all isolation functions to return bool. > > Acked-by: David Hildenbrand <david@redhat.com> Thanks. > > Although it's controversial if > > if (!ret) > ret = -EBUSY; > else > ret = 0; > > is really appealing to the readers eye :) > > ret = ret ? 0 : -EBUSY; > > It's still confusing. > > would be better as > > ret = isolated ? 0 : -EBUSY; > > IOW, not reusing the "int ret" variable. Yes, pretty clear. Will do in next version.
Now the page isolation functions did not return a boolean to indicate success or not, instead it will return a negative error when failed to isolate a page. So below code used in most places seem a boolean success/failure thing, which can confuse people whether the isolation is successful. if (folio_isolate_lru(folio)) continue; Moreover the page isolation functions only return 0 or -EBUSY, and most users did not care about the negative error except for few users, thus we can convert all page isolation functions to return a boolean value, which can remove the confusion to make code more clear. No functional changes intended in this patch series. Changes from v1: - Convert all isolation functions to return bool. Baolin Wang (4): mm: change to return bool for folio_isolate_lru() mm: change to return bool for isolate_lru_page() mm: hugetlb: change to return bool for isolate_hugetlb() mm: change to return bool for isolate_movable_page() include/linux/hugetlb.h | 6 +++--- include/linux/migrate.h | 6 +++--- mm/compaction.c | 2 +- mm/damon/paddr.c | 2 +- mm/folio-compat.c | 4 ++-- mm/gup.c | 2 +- mm/hugetlb.c | 12 ++++++++---- mm/internal.h | 4 ++-- mm/khugepaged.c | 4 ++-- mm/madvise.c | 4 ++-- mm/memcontrol.c | 4 ++-- mm/memory-failure.c | 10 +++++----- mm/memory_hotplug.c | 2 +- mm/mempolicy.c | 4 ++-- mm/migrate.c | 17 ++++++++++------- mm/migrate_device.c | 2 +- mm/vmscan.c | 10 +++++----- 17 files changed, 51 insertions(+), 44 deletions(-)