| Message ID | 20230809175340.3066-1-stevie.6strings@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | histograms: Fix memory leak, logic bugs, legibility | expand |
On Wed, 9 Aug 2023 13:53:33 -0400 Stevie Alvarez <stevie.6strings@gmail.com> wrote: > From: Stevie Alvarez (Google) <stevie.6strings@gmail.com> > > Changes in v4: > * Reorder enum traceeval_data_type numeric types for algorithmic access. > * Reorder union traceeval_data numeric fields for legibility. > * Remove parenthesis from function name in kerneldocs. > * Optimize logic in type_alloc(). > * Distinguish internal vs public interfaces within include statements. > * Rework copy_traceeval_data() logic for legibility. > * Rework comparator return types to fix query and insert bugs. > * update_entry() frees the entries old values to avoid a memory leak. > > Steven, I didn't get around to adding in the cstring field to > traceeval_data, as I wasn't able to completely grasp what you were > looking for. My apologies. Thanks Stevie! I'm going to pull these in as-is and build on top of them for the rest so that you can work on other things. I'll still do a review of these patches for your own understanding, but you don't need to send another series. As for the cstring. Without it, I get these warnings in the task-eval.c program: libtraceeval.git/samples/task-eval.c: In function ‘update_process’: libtraceeval.git/samples/task-eval.c:197:35: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 197 | .string = comm, | ^~~~ libtraceeval.git/samples/task-eval.c: In function ‘get_process_data’: libtraceeval.git/samples/task-eval.c:255:35: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 255 | .string = comm, | ^~~~ libtraceeval.git/samples/task-eval.c: In function ‘set_process_data’: libtraceeval.git/samples/task-eval.c:278:35: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 278 | .string = comm, as in update_process() (and other functions) we have: static void update_process(struct task_data *tdata, const char *comm, enum sched_state state, enum command cmd, unsigned long long ts) { const union traceeval_data keys[] = { { .string = comm, }, { .number = state, }, }; Because these queries are performed on const char *strings. That is, in order to do a traceeval_query() I need to populate the keys array. And if I'm using a const char *string to do the search, I need to use a const pointer to assign with. So, if we supply a "const char * cstring" to the union, and I use: const union traceeval_data keys[] = { { .cstring = comm, }, It makes the compiler happy! -- Steve > > --- > v3 discussion: https://lore.kernel.org/linux-trace-devel/20230808180156.GA1300@3xKetch/T/#t > > Stevie Alvarez (Google) (5): > histograms: Initial histograms interface > histograms: Add traceeval initialize and release > histograms: Add traceeval compare > histograms: Add traceeval query > histograms: Add traceeval insert > > Makefile | 2 +- > include/traceeval-hist.h | 147 +++++++ > include/traceeval-test.h | 16 + > src/Makefile | 1 + > src/histograms.c | 804 +++++++++++++++++++++++++++++++++++++++ > 5 files changed, 969 insertions(+), 1 deletion(-) > create mode 100644 include/traceeval-hist.h > create mode 100644 include/traceeval-test.h > create mode 100644 src/histograms.c >
On Wed, Aug 09, 2023 at 02:30:41PM -0400, Steven Rostedt wrote: > On Wed, 9 Aug 2023 13:53:33 -0400 > Stevie Alvarez <stevie.6strings@gmail.com> wrote: > > > From: Stevie Alvarez (Google) <stevie.6strings@gmail.com> > > > > Changes in v4: > > * Reorder enum traceeval_data_type numeric types for algorithmic access. > > * Reorder union traceeval_data numeric fields for legibility. > > * Remove parenthesis from function name in kerneldocs. > > * Optimize logic in type_alloc(). > > * Distinguish internal vs public interfaces within include statements. > > * Rework copy_traceeval_data() logic for legibility. > > * Rework comparator return types to fix query and insert bugs. > > * update_entry() frees the entries old values to avoid a memory leak. > > > > Steven, I didn't get around to adding in the cstring field to > > traceeval_data, as I wasn't able to completely grasp what you were > > looking for. My apologies. > > Thanks Stevie! > > I'm going to pull these in as-is and build on top of them for the rest so > that you can work on other things. I'll still do a review of these patches > for your own understanding, but you don't need to send another series. Thanks for these, Stevie. You've addressed all of my review comments, so Steven, feel free to add: Reviewed-by: Ross Zwisler <zwisler@google.com> when you apply the series.
On Wed, 9 Aug 2023 13:53:33 -0400 Stevie Alvarez <stevie.6strings@gmail.com> wrote: > From: Stevie Alvarez (Google) <stevie.6strings@gmail.com> > > Changes in v4: > * Reorder enum traceeval_data_type numeric types for algorithmic access. > * Reorder union traceeval_data numeric fields for legibility. > * Remove parenthesis from function name in kerneldocs. > * Optimize logic in type_alloc(). > * Distinguish internal vs public interfaces within include statements. > * Rework copy_traceeval_data() logic for legibility. > * Rework comparator return types to fix query and insert bugs. > * update_entry() frees the entries old values to avoid a memory leak. > > Steven, I didn't get around to adding in the cstring field to > traceeval_data, as I wasn't able to completely grasp what you were > looking for. My apologies. > Congratulations Stevie, Your patches are now committed: https://github.com/rostedt/libtraceeval/commits?author=Shiztev -- Steve
From: Stevie Alvarez (Google) <stevie.6strings@gmail.com> Changes in v4: * Reorder enum traceeval_data_type numeric types for algorithmic access. * Reorder union traceeval_data numeric fields for legibility. * Remove parenthesis from function name in kerneldocs. * Optimize logic in type_alloc(). * Distinguish internal vs public interfaces within include statements. * Rework copy_traceeval_data() logic for legibility. * Rework comparator return types to fix query and insert bugs. * update_entry() frees the entries old values to avoid a memory leak. Steven, I didn't get around to adding in the cstring field to traceeval_data, as I wasn't able to completely grasp what you were looking for. My apologies. --- v3 discussion: https://lore.kernel.org/linux-trace-devel/20230808180156.GA1300@3xKetch/T/#t Stevie Alvarez (Google) (5): histograms: Initial histograms interface histograms: Add traceeval initialize and release histograms: Add traceeval compare histograms: Add traceeval query histograms: Add traceeval insert Makefile | 2 +- include/traceeval-hist.h | 147 +++++++ include/traceeval-test.h | 16 + src/Makefile | 1 + src/histograms.c | 804 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 969 insertions(+), 1 deletion(-) create mode 100644 include/traceeval-hist.h create mode 100644 include/traceeval-test.h create mode 100644 src/histograms.c