Messages in this thread | | | From | "Huang\, Ying" <> | Subject | Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED() | Date | Wed, 18 Jul 2018 11:25:56 +0800 |
| |
Dave Hansen <dave.hansen@linux.intel.com> writes:
>> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot) >> unsigned long offset, i; >> unsigned char *map; >> >> + if (!IS_ENABLED(CONFIG_THP_SWAP)) { >> + VM_WARN_ON_ONCE(1); >> + return 0; >> + } > > I see you seized the opportunity to keep this code gloriously > unencumbered by pesky comments. This seems like a time when you might > have slipped up and been temped to add a comment or two. Guess not. :) > > Seriously, though, does it hurt us to add a comment or two to say > something like: > > /* > * Should not even be attempting cluster allocations when > * huge page swap is disabled. Warn and fail the allocation. > */ > if (!IS_ENABLED(CONFIG_THP_SWAP)) { > VM_WARN_ON_ONCE(1); > return 0; > }
I totally agree with you that we should add more comments for THP swap to improve the code readability. As for this specific case, VM_WARN_ON_ONCE() here is just to capture some programming error during development. Do we really need comments here?
I will try to add more comments for other places in code regardless this one.
Best Regards, Huang, Ying
| |