Received-SPF: pass (google.com: domain of linux-kernel+bounces-108429-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d82aedb5-9406-4808-96b4-1d9ef63485a3@I-love.SAKURA.ne.jp>
Date: Wed, 20 Mar 2024 13:40:05 +0900
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/2] mm,page_owner: Fix refcount imbalance
Content-Language: en-US
To: Andrew Morton <akpm@linux-foundation.org>,
        Oscar Salvador <osalvador@suse.de>
Cc: 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
References: <20240319183212.17156-1-osalvador@suse.de>
 <20240319183212.17156-2-osalvador@suse.de>
 <20240319162430.3e76504d06f12edc107029a4@linux-foundation.org>
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <20240319162430.3e76504d06f12edc107029a4@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2024/03/20 8:24, Andrew Morton wrote:
> On Tue, 19 Mar 2024 19:32:11 +0100 Oscar Salvador <osalvador@suse.de> wrote:
>> -static void dec_stack_record_count(depot_stack_handle_t handle)
>> +static void dec_stack_record_count(depot_stack_handle_t handle,
>> +				   int nr_base_pages)
>>  {
>>  	struct stack_record *stack_record = __stack_depot_get_stack_record(handle);
>>  
>>  	if (stack_record)
>> -		refcount_dec(&stack_record->count);
>> +		refcount_sub_and_test(nr_base_pages, &stack_record->count);
>>  }
> 
> mm/page_owner.c: In function 'dec_stack_record_count':
> mm/page_owner.c:226:17: error: ignoring return value of 'refcount_sub_and_test' declared with attribute 'warn_unused_result' [-Werror=unused-result]
>   226 |                 refcount_sub_and_test(nr_base_pages, &stack_record->count);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 

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().