Received-SPF: pass (google.com: domain of linux-kernel+bounces-60042-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 9 Feb 2024 22:39:04 +0100
From: Oscar Salvador <osalvador@suse.de>
To: Marco Elver <elver@google.com>
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>,
	Andrey Konovalov <andreyknvl@gmail.com>,
	Alexander Potapenko <glider@google.com>
Subject: Re: [PATCH v7 2/4] mm,page_owner: Implement the tracking of the
 stacks count
Message-ID: <ZcabeMCHCkl3Bf6q@localhost.localdomain>
References: <20240208234539.19113-1-osalvador@suse.de>
 <20240208234539.19113-3-osalvador@suse.de>
 <CANpmjNOoYC93dt5hNmWsC2N8-7GuSp2L6Lb7mNOKxTGhreceUg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANpmjNOoYC93dt5hNmWsC2N8-7GuSp2L6Lb7mNOKxTGhreceUg@mail.gmail.com>

On Fri, Feb 09, 2024 at 08:45:00AM +0100, Marco Elver wrote:
> > +/**
> > + * stack_depo_get_stack - Get a pointer to a stack struct
> 
> Typo: "depo" -> depot
> 
> I would also write "stack_record struct", because "stack struct" does not exist.

Fixed.

> > + * @handle: Stack depot handle
> > + *
> > + * Return: Returns a pointer to a stack struct
> > + */
> > +struct stack_record *stack_depot_get_stack(depot_stack_handle_t handle);
> 
> I don't know what other usecases there are for this, but I'd want to
> make make sure we give users a big hint to avoid unnecessary uses of
> this function.
> 
> Perhaps we also want to mark it as somewhat internal, e.g. by
> prefixing it with __. So I'd call it __stack_depot_get_stack_record().

Yes, I went with __stack_depot_get_stack_record(), and I updated its doc
in stackdepot.h, mentioning that is only for internal purposes.

> > +static void inc_stack_record_count(depot_stack_handle_t handle)
> > +{
> > +       struct stack_record *stack = stack_depot_get_stack(handle);
> > +
> > +       if (stack)
> > +               refcount_inc(&stack->count);
> > +}
> 
> In the latest stackdepot version in -next, the count is initialized to
> REFCOUNT_SATURATED to warn if a non-refcounted entry is suddenly used
> as a refcounted one. In your case this is intentional and there is no
> risk that the entry will be evicted, so that's ok. But you need to set
> the refcount to 1 somewhere here on the initial stack_depot_save().

Well, I went with something like:

 static void inc_stack_record_count(depot_stack_handle_t handle)
 {
         struct stack_record *stack = __stack_depot_get_stack_record(handle);
 
         if (stack) {
                 /*
                  * New stack_records 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,PUT} API, let us
                  * set a refcount of 1 ourselves.
                  */
                 if (refcount_read(&stack->count) == REFCOUNT_SATURATED)
                         refcount_set(&stack->count, 1);
                 refcount_inc(&stack->count);
         }
 }


-- 
Oscar Salvador
SUSE Labs