Received-SPF: pass (google.com: domain of linux-kernel+bounces-108458-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 20 Mar 2024 06:49:05 +0100
From: Oscar Salvador <osalvador@suse.de>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, Michal Hocko <mhocko@suse.com>,
	Vlastimil Babka <vbabka@suse.cz>, Marco Elver <elver@google.com>,
	Andrey Konovalov <andreyknvl@gmail.com>,
	Alexander Potapenko <glider@google.com>,
	syzbot+41bbfdb8d41003d12c0f@syzkaller.appspotmail.com
Subject: Re: [PATCH v2 1/2] mm,page_owner: Fix refcount imbalance
Message-ID: <Zfp40dsYSlCouvJW@localhost.localdomain>
References: <20240319183212.17156-1-osalvador@suse.de>
 <20240319183212.17156-2-osalvador@suse.de>
 <20240319162430.3e76504d06f12edc107029a4@linux-foundation.org>
 <d82aedb5-9406-4808-96b4-1d9ef63485a3@I-love.SAKURA.ne.jp>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d82aedb5-9406-4808-96b4-1d9ef63485a3@I-love.SAKURA.ne.jp>

On Wed, Mar 20, 2024 at 01:40:05PM +0900, Tetsuo Handa wrote:
> Hmm, I guess that this is not an expected user of refcount API.
> If it is correct behavior that refcount becomes 0 here, you need to explain like
> 
> -		refcount_sub_and_test(nr_base_pages, &stack_record->count);
> +		if (refcount_sub_and_test(nr_base_pages, &stack_record->count)) {
> +			// Explain why nothing to do here, and explain where/how
> +			// refcount again becomes positive value using refcount_set().
> +		}
> 
> or replace refcount_t with atomic_t where it is legal to make refcount positive
> without using atomic_set().

No, it is not expected for the refcount to become 0.
I do know why, but I lost a chunk in the middle of a rebase.
This should have the follwing on top:

 diff --git a/mm/page_owner.c b/mm/page_owner.c
 index 2613805cb665..e477a71d6adc 100644
 --- a/mm/page_owner.c
 +++ b/mm/page_owner.c
 @@ -222,8 +222,11 @@ static void dec_stack_record_count(depot_stack_handle_t handle,
  {
         struct stack_record *stack_record = __stack_depot_get_stack_record(handle);
  
 -       if (stack_record)
 -               refcount_sub_and_test(nr_base_pages, &stack_record->count);
 +       if (!stack_record)
 +               return;
 +
 +       if (refcount_sub_and_test(nr_base_pages, &stack_record->count))
 +               WARN(1, "%s refcount went to 0 for %u handle\n", __func__, handle);
  }


-- 
Oscar Salvador
SUSE Labs