[1/5] mm: compaction: Remove redundant VM_BUG_ON() in compact_zone()

  The compaction_suitable() will never return values other than COMPACT_SUCCESS,
COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
the redundant VM_BUG_ON() validation for the return values of compaction_suitable().

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 3 ---
 1 file changed, 3 deletions(-)
  

On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> the redundant VM_BUG_ON() validation for the return values of compaction_suitable().

I don't understand why we'd remove this check.

> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/compaction.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 62a61de44658..5e6f5e35748d 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2313,9 +2313,6 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>  	if (ret == COMPACT_SUCCESS || ret == COMPACT_SKIPPED)
>  		return ret;
>  
> -	/* huh, compaction_suitable is returning something unexpected */
> -	VM_BUG_ON(ret != COMPACT_CONTINUE);
> -
>  	/*
>  	 * Clear pageblock skip if there were failures recently and compaction
>  	 * is about to be retried after being deferred.
> -- 
> 2.27.0
> 
>
  

On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> > The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> > COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> > and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> > the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
> 
> I don't understand why we'd remove this check.

Well, just from code inspection it serves no purpose.

Such an assertion might be useful during early code development, but I
think we can consider compaction_suitable() to adequately debugged by
now?
  

On Tue, Jan 10, 2023 at 03:25:32PM -0800, Andrew Morton wrote:
> On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> > > The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> > > COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> > > and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> > > the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
> > 
> > I don't understand why we'd remove this check.
> 
> Well, just from code inspection it serves no purpose.
> 
> Such an assertion might be useful during early code development, but I
> think we can consider compaction_suitable() to adequately debugged by
> now?

What if compaction_suitable() is modified to return another value?
This seems like a relatively innocuous check.
  

On 1/11/2023 7:37 AM, Matthew Wilcox wrote:
> On Tue, Jan 10, 2023 at 03:25:32PM -0800, Andrew Morton wrote:
>> On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>>
>>> On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
>>>> The compaction_suitable() will never return values other than COMPACT_SUCCESS,
>>>> COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
>>>> and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
>>>> the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
>>>
>>> I don't understand why we'd remove this check.
>>
>> Well, just from code inspection it serves no purpose.
>>
>> Such an assertion might be useful during early code development, but I
>> think we can consider compaction_suitable() to adequately debugged by
>> now?
> 
> What if compaction_suitable() is modified to return another value?

Then this will be an expected value which should be handled by caller, 
and IMO we can not make such assumption for future to keep this 
unhelpful check.
  

On Wed, 11 Jan 2023 14:40:20 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> 
> 
> On 1/11/2023 7:37 AM, Matthew Wilcox wrote:
> > On Tue, Jan 10, 2023 at 03:25:32PM -0800, Andrew Morton wrote:
> >> On Tue, 10 Jan 2023 13:37:57 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> >>
> >>> On Tue, Jan 10, 2023 at 09:36:18PM +0800, Baolin Wang wrote:
> >>>> The compaction_suitable() will never return values other than COMPACT_SUCCESS,
> >>>> COMPACT_SKIPPED and COMPACT_CONTINUE, so after validation of COMPACT_SUCCESS
> >>>> and COMPACT_SKIPPED, we will never hit other unexpected case. Thus remove
> >>>> the redundant VM_BUG_ON() validation for the return values of compaction_suitable().
> >>>
> >>> I don't understand why we'd remove this check.
> >>
> >> Well, just from code inspection it serves no purpose.
> >>
> >> Such an assertion might be useful during early code development, but I
> >> think we can consider compaction_suitable() to adequately debugged by
> >> now?
> > 
> > What if compaction_suitable() is modified to return another value?
> 
> Then this will be an expected value which should be handled by caller, 
> and IMO we can not make such assumption for future to keep this 
> unhelpful check.

One way of looking at this: if the assertion wasn't there and someone
sent a patch which added it, would we merge the patch?

"[patch] add check for compaction_suitable() return value"

"why"

"it might be wrong"

"it isn't"

"but we might make it wrong later"

"the same can be said of every function in the kernel"


And if we wouldn't merge a hypothetical patch which adds some code, we
shouldn't retain that code, no?