Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210630003406.4013668-1-paulburton@google.com> <20210630083146.7514cb1f@oasis.local.home>
In-Reply-To: <20210630083146.7514cb1f@oasis.local.home>
From:   Joel Fernandes <joelaf@google.com>
Date:   Wed, 30 Jun 2021 12:43:24 -0400
Message-ID: <CAJWu+oofNcZtg+EAduKzp=yFe5+AYNogvr-V0q3NWyLA_QCRJw@mail.gmail.com>
Subject: Re: [PATCH 1/2] tracing: Simplify & fix saved_tgids logic
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Paul Burton <paulburton@google.com>, linux-kernel@vger.kernel.org,
        Ingo Molnar <mingo@redhat.com>, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 30, 2021 at 8:31 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 29 Jun 2021 17:34:05 -0700
> Paul Burton <paulburton@google.com> wrote:
>
> > The tgid_map array records a mapping from pid to tgid, where the index
> > of an entry within the array is the pid & the value stored at that index
> > is the tgid.
> >
> > The saved_tgids_next() function iterates over pointers into the tgid_map
> > array & dereferences the pointers which results in the tgid, but then it
> > passes that dereferenced value to trace_find_tgid() which treats it as a
> > pid & does a further lookup within the tgid_map array. It seems likely
> > that the intent here was to skip over entries in tgid_map for which the
> > recorded tgid is zero, but instead we end up skipping over entries for
> > which the thread group leader hasn't yet had its own tgid recorded in
> > tgid_map.
> >
> > A minimal fix would be to remove the call to trace_find_tgid, turning:
> >
> >   if (trace_find_tgid(*ptr))
> >
> > into:
> >
> >   if (*ptr)
> >
> > ..but it seems like this logic can be much simpler if we simply let
> > seq_read() iterate over the whole tgid_map array & filter out empty
> > entries by returning SEQ_SKIP from saved_tgids_show(). Here we take that
> > approach, removing the incorrect logic here entirely.
> >
> > Signed-off-by: Paul Burton <paulburton@google.com>
> > Fixes: d914ba37d714 ("tracing: Add support for recording tgid of tasks")
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Joel Fernandes <joelaf@google.com>
> > Cc: <stable@vger.kernel.org>
> > ---
>
> Joel,
>
> Can you review this please.

Sure thing Steve, will review it today.

thanks,
-Joel