Re: [PATCH] mm, swap: use rbtree for swap_extent

[Aaron Lu <aaron.lu@linux.alibaba.com>]

On 2019/5/24 3:00, Andrew Morton wrote:
...

> On Thu, 23 May 2019 22:24:15 +0800 Aaron Lu <aaron.lu@linux.alibaba.com> wrote:
>> ...
>>
>> +static struct swap_extent *
>> +offset_to_swap_extent(struct swap_info_struct *sis, unsigned long offset)
>> +{
>> +	struct swap_extent *se;
>> +	struct rb_node *rb;
>> +
>> +	rb = sis->swap_extent_root.rb_node;
>> +	while (rb) {
>> +		se = rb_entry(rb, struct swap_extent, rb_node);
>> +		if (offset < se->start_page)
>> +			rb = rb->rb_left;
>> +		else if (offset >= se->start_page + se->nr_pages)
>> +			rb = rb->rb_right;
>> +		else
>> +			return se;
>> +	}
>> +	/* It *must* be present */
>> +	BUG_ON(1);
> 
> I'm surprised this doesn't generate a warning about the function

Ah right, I'm also surprised after you mentioned.
This BUG_ON(1) here is meant to serve the same purpose as the
original code in map_swap_entry():

static sector_t map_swap_entry(swp_entry_t entry, struct block_device **bdev)
{
	...

	offset = swp_offset(entry);
	start_se = sis->curr_swap_extent;
	se = start_se;

	for ( ; ; ) {
		if (se->start_page <= offset &&
				offset < (se->start_page + se->nr_pages)) {
			return se->start_block + (offset - se->start_page);
		}
		se = list_next_entry(se, list);
		sis->curr_swap_extent = se;
		BUG_ON(se == start_se);		/* It *must* be present */
	}
}

I just copied the pattern and changed the condition to 1 without
much thought.

> failing to return a value.  I guess the compiler figured out that
> BUG_ON(non-zero-constant) is equivalent to BUG(), which is noreturn.
> 
> Let's do this?

Yes, it doesn't make much sense to use BUG_ON when the condition
is 1...Thanks for the cleanup.

> 
> --- a/mm/swapfile.c~mm-swap-use-rbtree-for-swap_extent-fix
> +++ a/mm/swapfile.c
> @@ -218,7 +218,7 @@ offset_to_swap_extent(struct swap_info_s
>  			return se;
>  	}
>  	/* It *must* be present */
> -	BUG_ON(1);
> +	BUG();
>  }
>