Received-SPF: pass (google.com: domain of linux-kernel+bounces-66744-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <9fc95f61-827f-40ee-a823-576cdcad7939@suse.cz>
Date: Thu, 15 Feb 2024 12:08:53 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 4/7] mm,page_owner: Implement the tracking of the
 stacks count
Content-Language: en-US
To: Oscar Salvador <osalvador@suse.de>,
 Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
 Michal Hocko <mhocko@suse.com>, Marco Elver <elver@google.com>,
 Andrey Konovalov <andreyknvl@gmail.com>,
 Alexander Potapenko <glider@google.com>
References: <20240214170157.17530-1-osalvador@suse.de>
 <20240214170157.17530-5-osalvador@suse.de>
From: Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <20240214170157.17530-5-osalvador@suse.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2/14/24 18:01, Oscar Salvador wrote:
> Implement {inc,dec}_stack_record_count() which increments or
> decrements on respective allocation and free operations, via
> __reset_page_owner() (free operation) and __set_page_owner() (alloc
> operation).
> Newly allocated stack_record structs will be added to the list stack_list
> via add_stack_record_to_list().
> Modifications on the list are protected via a spinlock with irqs
> disabled, since this code can also be reached from IRQ context.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> Reviewed-by: Marco Elver <elver@google.com>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Note:

> +static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask)
> +{
> +	struct stack_record *stack_record = __stack_depot_get_stack_record(handle);
> +
> +	if (!stack_record)
> +		return;
> +
> +	/*
> +	 * New stack_record's that do not use STACK_DEPOT_FLAG_GET start
> +	 * with REFCOUNT_SATURATED to catch spurious increments of their
> +	 * refcount.
> +	 * Since we do not use STACK_DEPOT_FLAG_GET API, let us
> +	 * set a refcount of 1 ourselves.
> +	 */
> +	if (refcount_read(&stack_record->count) == REFCOUNT_SATURATED) {
> +		int old = REFCOUNT_SATURATED;
> +
> +		if (atomic_try_cmpxchg_relaxed(&stack_record->count.refs, &old, 1))
> +			/* Add the new stack_record to our list */
> +			add_stack_record_to_list(stack_record, gfp_mask);
			
			Not returning here...

> +	}
> +	refcount_inc(&stack_record->count);

.. means we'll increase the count to 2 on the first store, so there's a
bias. Which would be consistent with the failure and dummy stacks that also
start with a refcount of 1. But then the stack count reporting should
decrement by 1 to prevent confusion? (in the following patch). Imagine
somebody debugging an allocation stack where there are not so many of them,
but the allocation is large, and being sidetracked by an off-by-one error.