Re: [v3 PATCH 3/5] mm: hwpoison: refactor refcount check handling

[Peter Xu <peterx@redhat.com>]

On Thu, Sep 30, 2021 at 02:53:09PM -0700, Yang Shi wrote:
> +/*
> + * Return true if page is still referenced by others, otherwise return
> + * false.
> + *
> + * The dec is true when one extra refcount is expected.
> + */
> +static bool has_extra_refcount(struct page_state *ps, struct page *p,
> +			       bool dec)

Nit: would it be nicer to keep using things like "extra_pins", so we pass in 1
for swapcache dirty case and 0 for the rest?  Then it'll also match with most
of the similar cases in e.g. huge_memory.c (please try grep "extra_pins" there).

> +{
> +	int count = page_count(p) - 1;
> +
> +	if (dec)
> +		count -= 1;
> +
> +	if (count > 0) {
> +		pr_err("Memory failure: %#lx: %s still referenced by %d users\n",
> +		       page_to_pfn(p), action_page_types[ps->type], count);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * Error hit kernel page.
>   * Do nothing, try to be lucky and not touch this instead. For a few cases we
>   * could be more sophisticated.
>   */
> -static int me_kernel(struct page *p, unsigned long pfn)
> +static int me_kernel(struct page_state *ps, struct page *p)

Not sure whether it's intended, but some of the action() hooks do not call the
refcount check now while in the past they'll all do.  Just to double check
they're expected, like this one and me_unknown().

>  {
>  	unlock_page(p);
>  	return MF_IGNORED;
> @@ -820,9 +852,9 @@ static int me_kernel(struct page *p, unsigned long pfn)
>  /*
>   * Page in unknown state. Do nothing.
>   */
> -static int me_unknown(struct page *p, unsigned long pfn)
> +static int me_unknown(struct page_state *ps, struct page *p)
>  {
> -	pr_err("Memory failure: %#lx: Unknown page state\n", pfn);
> +	pr_err("Memory failure: %#lx: Unknown page state\n", page_to_pfn(p));
>  	unlock_page(p);
>  	return MF_FAILED;
>  }

Thanks,

-- 
Peter Xu