Received-SPF: pass (google.com: domain of linux-kernel+bounces-60048-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240208234539.19113-1-osalvador@suse.de> <20240208234539.19113-3-osalvador@suse.de>
 <CANpmjNOoYC93dt5hNmWsC2N8-7GuSp2L6Lb7mNOKxTGhreceUg@mail.gmail.com>
 <ZcabeMCHCkl3Bf6q@localhost.localdomain> <CANpmjNPM4aKMWAmxWEMQ-Antq0jDFauOya2XHHX5RhnONucgrg@mail.gmail.com>
In-Reply-To: <CANpmjNPM4aKMWAmxWEMQ-Antq0jDFauOya2XHHX5RhnONucgrg@mail.gmail.com>
From: Marco Elver <elver@google.com>
Date: Fri, 9 Feb 2024 22:44:23 +0100
Message-ID: <CANpmjNP4H+FL-Jwq6Bpf1W0Rz2ZSn-ZquWf7tz4n57r1Jq6m_g@mail.gmail.com>
Subject: Re: [PATCH v7 2/4] mm,page_owner: Implement the tracking of the
 stacks count
To: Oscar Salvador <osalvador@suse.de>
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>
Content-Type: text/plain; charset="UTF-8"

On Fri, 9 Feb 2024 at 22:42, Marco Elver <elver@google.com> wrote:
>
> On Fri, 9 Feb 2024 at 22:37, Oscar Salvador <osalvador@suse.de> wrote:
> >
> > 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
>
> There is no FLAG_PUT, only stack_depot_put(). Saying you do not use
> the refcount to free any entries should hopefully make it clear that
> even if the refcount saturates and you wrap around to 1, nothing
> catastrophic will happen.
>
> >                   * set a refcount of 1 ourselves.
> >                   */
> >                  if (refcount_read(&stack->count) == REFCOUNT_SATURATED)
> >                          refcount_set(&stack->count, 1);

Do you need to inc the first allocation? Should there be an "else"
here instead of always doing refcount_inc()?

> >                  refcount_inc(&stack->count);
> >          }
> >  }
>
> That looks reasonable.