lkml.org 
[lkml]   [2021]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/2] tracing: Simplify & fix saved_tgids logic
    On Tue, Jun 29, 2021 at 8:34 PM 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.

    Looks reasonable except for one nit:

    > 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>
    > ---
    > kernel/trace/trace.c | 38 +++++++++++++-------------------------
    > 1 file changed, 13 insertions(+), 25 deletions(-)
    >
    > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
    > index d23a09d3eb37b..9570667310bcc 100644
    > --- a/kernel/trace/trace.c
    > +++ b/kernel/trace/trace.c
    > @@ -5608,37 +5608,20 @@ static const struct file_operations tracing_readme_fops = {
    >
    > static void *saved_tgids_next(struct seq_file *m, void *v, loff_t *pos)
    > {
    > - int *ptr = v;
    > + int pid = ++(*pos);
    >
    > - if (*pos || m->count)
    > - ptr++;
    > -
    > - (*pos)++;
    > -
    > - for (; ptr <= &tgid_map[PID_MAX_DEFAULT]; ptr++) {
    > - if (trace_find_tgid(*ptr))
    > - return ptr;

    It would be great if you can add back the check for !tgid_map to both
    next() and show() as well, for added robustness (since the old code
    previously did it).

    With that change:
    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

    thanks,

    -Joel


    > - }
    > + if (pid > PID_MAX_DEFAULT)
    > + return NULL;
    >
    > - return NULL;
    > + return &tgid_map[pid];
    > }
    >
    > static void *saved_tgids_start(struct seq_file *m, loff_t *pos)
    > {
    > - void *v;
    > - loff_t l = 0;
    > -
    > - if (!tgid_map)
    > + if (!tgid_map || *pos > PID_MAX_DEFAULT)
    > return NULL;
    >
    > - v = &tgid_map[0];
    > - while (l <= *pos) {
    > - v = saved_tgids_next(m, v, &l);
    > - if (!v)
    > - return NULL;
    > - }
    > -
    > - return v;
    > + return &tgid_map[*pos];
    > }
    >
    > static void saved_tgids_stop(struct seq_file *m, void *v)
    > @@ -5647,9 +5630,14 @@ static void saved_tgids_stop(struct seq_file *m, void *v)
    >
    > static int saved_tgids_show(struct seq_file *m, void *v)
    > {
    > - int pid = (int *)v - tgid_map;
    > + int *entry = (int *)v;
    > + int pid = entry - tgid_map;
    > + int tgid = *entry;
    > +
    > + if (tgid == 0)
    > + return SEQ_SKIP;
    >
    > - seq_printf(m, "%d %d\n", pid, trace_find_tgid(pid));
    > + seq_printf(m, "%d %d\n", pid, tgid);
    > return 0;
    > }
    >
    >
    > base-commit: 62fb9874f5da54fdb243003b386128037319b219
    > --
    > 2.32.0.93.g670b81a890-goog
    >

    \
     
     \ /
      Last update: 2021-07-01 00:31    [W:3.815 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site