| Message ID | 20210630003406.4013668-1-paulburton@google.com |
|---|---|
| State | Accepted |
| Commit | b81b3e959adb107cd5b36c7dc5ba1364bbd31eb2 |
| Headers | show |
| Series | [1/2] tracing: Simplify & fix saved_tgids logic | expand |
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. -- Steve
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 >
On Wed, 30 Jun 2021 19:11:45 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 30 Jun 2021 18:34:11 -0400 > Joel Fernandes <joelaf@google.com> wrote: > > > > Anyway, I'll wait to hear what Joel says on this. If he thinks this is > > > worth 16M of memory when enabled, I may take it. > > > > I am not a huge fan of the 16M, in Android we enable this feature on > > all devices. Low end Android devices traced in the field are sometimes > > 1 GB and the added memory pressure may be unwelcome. Very least, maybe > > make it optional only for folks who increase pid_max? > > Yeah, can we just set it to the size of pid_max, at whatever it is set > to? Can you send a V2 with this update and address Joel's comments to patch 1, and get it to me today? I plan on sending Linus a pull request tomorrow, which means I have to kick off my tests tonight for what I want to send. -- Steve
Hi Joel, On Wed, Jun 30, 2021 at 06:29:55PM -0400, Joel Fernandes wrote: > 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). That condition cannot happen, because both next() & show() are called to iterate through the content of the seq_file & by definition their v argument is non-NULL (else seq_file would have finished iterating already). That argument came from either start() or an earlier call to next(), which would only have returned a non-NULL pointer into tgid_map if tgid_map is non-NULL. I've added comments to this effect in v2, though the second patch in v2 does wind up effectively adding back the check in next() anyway in order to reuse some code. I was tempted to just add the redundant checks anyway (pick your battles and all) but for show() in particular it wound up making things seem non-sensical to me ("display the value describing this non-NULL pointer into tgid_map only if tgid_map is not NULL?"). Thanks, Paul > 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 > >
On Thu, Jul 1, 2021 at 1:32 PM Paul Burton <paulburton@google.com> wrote: > > Hi Joel, > > On Wed, Jun 30, 2021 at 06:29:55PM -0400, Joel Fernandes wrote: > > 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). > > That condition cannot happen, because both next() & show() are called to > iterate through the content of the seq_file & by definition their v > argument is non-NULL (else seq_file would have finished iterating > already). That argument came from either start() or an earlier call to > next(), which would only have returned a non-NULL pointer into tgid_map > if tgid_map is non-NULL. Hmm, You do have a point. Alright then. You could add my Reviewed-by tag for this patch to subsequent postings. thanks, -Joel
On Thu, Jul 1, 2021 at 1:24 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. > > 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> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
On Thu, 1 Jul 2021 10:31:59 -0700 Paul Burton <paulburton@google.com> wrote: > I was tempted to just add the redundant checks anyway (pick your battles > and all) but for show() in particular it wound up making things seem > non-sensical to me ("display the value describing this non-NULL pointer > into tgid_map only if tgid_map is not NULL?"). I agree with your assessment, and will actually take your first patch, as I don't think the comment is that helpful, not to mention, we don't use '//' comments in the kernel, so that would have to be changed. But for cases like this, I usually have something like: if (WARN_ON_ONCE(!tgid_map)) return -1; Because the logic is what makes tgid_map not being NULL, but as experience has taught me, the logic can sometimes be mistaken, at least as time goes by. And things that are protected by logic, deserve a WARN*() when it doesn't go as planned. We can always add that later, if needed. -- Steve
On Thu, Jul 1, 2021 at 2:07 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 1 Jul 2021 10:31:59 -0700 > Paul Burton <paulburton@google.com> wrote: > > > I was tempted to just add the redundant checks anyway (pick your battles > > and all) but for show() in particular it wound up making things seem > > non-sensical to me ("display the value describing this non-NULL pointer > > into tgid_map only if tgid_map is not NULL?"). > > I agree with your assessment, and will actually take your first patch, > as I don't think the comment is that helpful, not to mention, we don't > use '//' comments in the kernel, so that would have to be changed. > > But for cases like this, I usually have something like: > > > if (WARN_ON_ONCE(!tgid_map)) > return -1; > > Because the logic is what makes tgid_map not being NULL, but as > experience has taught me, the logic can sometimes be mistaken, at least > as time goes by. And things that are protected by logic, deserve a > WARN*() when it doesn't go as planned. > > We can always add that later, if needed. Agreed, was thinking similar/same. thanks, -Joel
On Thu, 1 Jul 2021 10:24:07 -0700 Paul Burton <paulburton@google.com> wrote: > +static int *trace_find_tgid_ptr(int pid) > +{ > + // Pairs with the smp_store_release in set_tracer_flag() to ensure that > + // if we observe a non-NULL tgid_map then we also observe the correct > + // tgid_map_max. BTW, it's against the Linux kernel coding style to use // for comments. I can take this patch, but I need to change this to: /* * Pairs with the smp_store_release in set_tracer_flag() to ensure that * if we observe a non-NULL tgid_map then we also observe the correct * tgid_map_max. */ Same with the other comments. Please follow coding style that can be found in: Documentation/process/coding-style.rst And see section 8 on Commenting. Thanks, -- Steve > + int *map = smp_load_acquire(&tgid_map); > + > + if (unlikely(!map || pid > tgid_map_max)) > + return NULL; > + > + return &map[pid]; > +} > +
On Thu, Jul 01, 2021 at 02:07:54PM -0400, Steven Rostedt wrote: > On Thu, 1 Jul 2021 10:31:59 -0700 > Paul Burton <paulburton@google.com> wrote: > > > I was tempted to just add the redundant checks anyway (pick your battles > > and all) but for show() in particular it wound up making things seem > > non-sensical to me ("display the value describing this non-NULL pointer > > into tgid_map only if tgid_map is not NULL?"). > > I agree with your assessment, and will actually take your first patch, > as I don't think the comment is that helpful, Thanks - agreed, the comment doesn't add much. > not to mention, we don't > use '//' comments in the kernel, so that would have to be changed. D'oh! Apparently a year away from the kernel melted my internal style checker. Interestingly though, checkpatch didn't complain about this as I would have expected... Thanks, Paul
Hi Steven, On Thu, Jul 01, 2021 at 02:12:21PM -0400, Steven Rostedt wrote: > On Thu, 1 Jul 2021 10:24:07 -0700 > Paul Burton <paulburton@google.com> wrote: > > > +static int *trace_find_tgid_ptr(int pid) > > +{ > > + // Pairs with the smp_store_release in set_tracer_flag() to ensure that > > + // if we observe a non-NULL tgid_map then we also observe the correct > > + // tgid_map_max. > > BTW, it's against the Linux kernel coding style to use // for comments. > > I can take this patch, but I need to change this to: > > /* > * Pairs with the smp_store_release in set_tracer_flag() to ensure that > * if we observe a non-NULL tgid_map then we also observe the correct > * tgid_map_max. > */ > > Same with the other comments. Please follow coding style that can be > found in: > > Documentation/process/coding-style.rst > > And see section 8 on Commenting. Yeah, sorry about that - I should know better having been a maintainer in a former life... Just to confirm - are you happy to fix those up when applying or should I send a v3? Thanks, Paul
[ Added Joe Perches ] On Thu, 1 Jul 2021 11:12:54 -0700 Paul Burton <paulburton@google.com> wrote: > > not to mention, we don't > > use '//' comments in the kernel, so that would have to be changed. > > D'oh! Apparently a year away from the kernel melted my internal style > checker. Interestingly though, checkpatch didn't complain about this as > I would have expected... Joe, should the above be added to checkpatch? I do understand that there are a few cases it's acceptable. Like for SPDX headers. -- Steve
On Thu, 1 Jul 2021 11:15:37 -0700 Paul Burton <paulburton@google.com> wrote: > Yeah, sorry about that - I should know better having been a maintainer > in a former life... No problem. > > Just to confirm - are you happy to fix those up when applying or should > I send a v3? I made the conversion and I'm going to start my testing now. Joel, I never saw a reviewed-by from you for this patch. Thanks! -- Steve
On 2021-07-01 11:26, Steven Rostedt wrote: > [ Added Joe Perches ] > > On Thu, 1 Jul 2021 11:12:54 -0700 > Paul Burton <paulburton@google.com> wrote: > >> > not to mention, we don't >> > use '//' comments in the kernel, so that would have to be changed. >> >> D'oh! Apparently a year away from the kernel melted my internal style >> checker. Interestingly though, checkpatch didn't complain about this >> as >> I would have expected... > > Joe, should the above be added to checkpatch? > > I do understand that there are a few cases it's acceptable. Like for > SPDX headers. C99 comments are allowed since about 5 years ago. And if you really want there's a checkpatch option to disable them. https://lore.kernel.org/patchwork/patch/1031132/
On Thu, 01 Jul 2021 12:35:29 -0700
Joe Perches <joe@perches.com> wrote:
> C99 comments are allowed since about 5 years ago.
Really, I thought Linus hated them. Personally, I find them rather ugly
myself. The only user of them I see in the kernel/ directory appears to
be for RCU. But Paul's on the C/C++ committee, so perhaps he favors them.
The net/ directory doesn't have any, except perhaps to comment out code
(which I sometimes use it for that too).
The block/, arch/x86/ directories don't have them either.
I wouldn't go and change checkpatch, but I still rather avoid them,
especially for multi line comments.
/*
* When it comes to multi line comments I prefer using something
* that denotes a start and an end to the comment, as it makes it
* look like a nice clip of information.
*/
Instead of:
// When it comes to multi line comments I prefer using something
// that denotes a start and an end to the comment, as it makes it
// look like a nice clip of information.
Which just looks like noise. But hey, maybe that's just me because I
find "*" as a sign of information and '//' something to ignore. ;-)
-- Steve
On 2021-07-01 12:51, Steven Rostedt wrote: > On Thu, 01 Jul 2021 12:35:29 -0700 > Joe Perches <joe@perches.com> wrote: > >> C99 comments are allowed since about 5 years ago. > > Really, I thought Linus hated them. Personally, I find them rather ugly > myself. The only user of them I see in the kernel/ directory appears to > be for RCU. But Paul's on the C/C++ committee, so perhaps he favors > them. > > The net/ directory doesn't have any, except perhaps to comment out code > (which I sometimes use it for that too). > > The block/, arch/x86/ directories don't have them either. > > I wouldn't go and change checkpatch, but I still rather avoid them, > especially for multi line comments. > > /* > * When it comes to multi line comments I prefer using something > * that denotes a start and an end to the comment, as it makes it > * look like a nice clip of information. > */ > > Instead of: > > // When it comes to multi line comments I prefer using something > // that denotes a start and an end to the comment, as it makes it > // look like a nice clip of information. > > Which just looks like noise. But hey, maybe that's just me because I > find "*" as a sign of information and '//' something to ignore. ;-) May I suggest using something other than an amber vt220?
On Thu, Jul 1, 2021 at 5:07 PM Joe Perches <joe@perches.com> wrote: > > On 2021-07-01 12:51, Steven Rostedt wrote: > > On Thu, 01 Jul 2021 12:35:29 -0700 > > Joe Perches <joe@perches.com> wrote: > > > >> C99 comments are allowed since about 5 years ago. > > > > Really, I thought Linus hated them. Personally, I find them rather ugly > > myself. The only user of them I see in the kernel/ directory appears to > > be for RCU. But Paul's on the C/C++ committee, so perhaps he favors > > them. > > > > The net/ directory doesn't have any, except perhaps to comment out code > > (which I sometimes use it for that too). > > > > The block/, arch/x86/ directories don't have them either. > > > > I wouldn't go and change checkpatch, but I still rather avoid them, > > especially for multi line comments. > > > > /* > > * When it comes to multi line comments I prefer using something > > * that denotes a start and an end to the comment, as it makes it > > * look like a nice clip of information. > > */ > > > > Instead of: > > > > // When it comes to multi line comments I prefer using something > > // that denotes a start and an end to the comment, as it makes it > > // look like a nice clip of information. > > > > Which just looks like noise. But hey, maybe that's just me because I > > find "*" as a sign of information and '//' something to ignore. ;-) > > May I suggest using something other than an amber vt220? Steve - mostly comments are to be ignored and the code is the ultimate source of truth ;-), so // is fine :-D That said, don't discard the amber vt220 I recently sent you just because Joe says so ;-) <:o) - Joel
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; - } + 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; }
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> --- kernel/trace/trace.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-) base-commit: 62fb9874f5da54fdb243003b386128037319b219