Yordan Karadzhov (VMware) (7): kernel-shark: Preserve markers when appending data kernel-shark: Preserve open graphs when appending data kernel-shark: Clear before loading new session kernel-shark: Better handling of plugins when appending data file kernel-shark: Do draw the combo point of the mark kernel-shark: Check if "trace_seq" was destroyed before using it kernel-shark: Quiet the warning printing from libtraceevent src/KsDualMarker.hpp | 6 ++++++ src/KsGLWidget.cpp | 40 ++++++++++++++++++++++++++-------------- src/KsGLWidget.hpp | 4 +++- src/KsMainWindow.cpp | 31 ++++++++++++++++++++++++++----- src/KsPlotTools.cpp | 3 +++ src/KsSession.cpp | 1 + src/KsTraceGraph.cpp | 10 +++++++--- src/KsTraceGraph.hpp | 2 +- src/KsUtils.cpp | 4 ++-- src/libkshark-tepdata.c | 16 +++++++++++++++- 10 files changed, 90 insertions(+), 27 deletions(-) -- 2.27.0
When an entry is selected in the KernelShark GUI (using marker A and marker B) we only keep track the index of this entry inside the array of entries loaded at the moment of selecting. However, then a data file is appended, the new entries are merged to this array and the array is sorted. As a result the index of the marker can/will point to completely different entry. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/KsDualMarker.hpp | 6 ++++++ src/KsMainWindow.cpp | 24 ++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/KsDualMarker.hpp b/src/KsDualMarker.hpp index 0dcaf93..39c0ce2 100644 --- a/src/KsDualMarker.hpp +++ b/src/KsDualMarker.hpp @@ -154,6 +154,12 @@ public: void updateLabels(); + /** Get the index inside the data array marker A points to. */ + ssize_t markerAPos() {return markerA()._isSet ? markerA()._pos : -1;} + + /** Get the index inside the data array marker B points to. */ + ssize_t markerBPos() {return markerB()._isSet ? markerB()._pos : -1;} + signals: /** * This signal is emitted when the Table View has to switch the color diff --git a/src/KsMainWindow.cpp b/src/KsMainWindow.cpp index d0a434a..fa893ce 100644 --- a/src/KsMainWindow.cpp +++ b/src/KsMainWindow.cpp @@ -571,8 +571,15 @@ void KsMainWindow::markEntry(ssize_t row, DualMarkerState st) /** Select given kshark_entry with a given maker. */ void KsMainWindow::markEntry(const kshark_entry *e, DualMarkerState st) { - ssize_t row = kshark_find_entry_by_time(e->ts, _data.rows(), - 0, _data.size() - 1); + ssize_t row; + + if (!e) { + _mState.getMarker(st).reset(); + return; + } + + row = kshark_find_entry_by_time(e->ts, _data.rows(), + 0, _data.size() - 1); markEntry(row, st); } @@ -1341,7 +1348,20 @@ void KsMainWindow::loadDataFile(const QString& fileName) /** Append trace data for file. */ void KsMainWindow::appendDataFile(const QString& fileName) { + kshark_entry *eMarkA(nullptr), *eMarkB(nullptr); + int rowA = _mState.markerAPos(); + int rowB = _mState.markerBPos(); + + if (rowA >= 0) + eMarkA = _data.rows()[rowA]; + + if (rowB >= 0) + eMarkB = _data.rows()[rowB]; + _load(fileName, true); + + markEntry(eMarkA, DualMarkerState::A); + markEntry(eMarkB, DualMarkerState::B); } void KsMainWindow::_error(const QString &mesg, -- 2.27.0
If the user has already made some selection of graphs to be displayed by the GUI, those particular graphs are likely to be important for him, so it is better to keep this configuration after the data file is appended. The opposite can be annoying for the user. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/KsGLWidget.cpp | 40 ++++++++++++++++++++++++++-------------- src/KsGLWidget.hpp | 4 +++- src/KsMainWindow.cpp | 7 ++++--- src/KsTraceGraph.cpp | 10 +++++++--- src/KsTraceGraph.hpp | 2 +- src/KsUtils.cpp | 1 + 6 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/KsGLWidget.cpp b/src/KsGLWidget.cpp index 9f8e43b..8aab7b3 100644 --- a/src/KsGLWidget.cpp +++ b/src/KsGLWidget.cpp @@ -409,25 +409,12 @@ void KsGLWidget::keyReleaseEvent(QKeyEvent *event) */ #define KS_MAX_START_PLOTS 16 -/** - * @brief Load and show trace data. - * - * @param data: Input location for the KsDataStore object. - * KsDataStore::loadDataFile() must be called first. - */ -void KsGLWidget::loadData(KsDataStore *data) +void KsGLWidget::_defaultPlots(kshark_context *kshark_ctx) { - kshark_context *kshark_ctx(nullptr); QVector<int> streamIds, plotVec; uint64_t tMin, tMax; int nCPUs, nBins; - if (!kshark_instance(&kshark_ctx) || !kshark_ctx->n_streams) - return; - - loadColors(); - - _data = data; _model.reset(); _streamPlots.clear(); @@ -463,6 +450,31 @@ void KsGLWidget::loadData(KsDataStore *data) tMin = _data->rows()[0]->ts; tMax = _data->rows()[_data->size() - 1]->ts; ksmodel_set_bining(_model.histo(), nBins, tMin, tMax); +} + +/** + * @brief Load and show trace data. + * + * @param data: Input location for the KsDataStore object. + * KsDataStore::loadDataFile() must be called first. + * @param resetPlots: If true, all existing graphs are closed + * and a default configuration of graphs is displayed + * (all CPU plots). If false, the current set of graphs + * is preserved. + */ +void KsGLWidget::loadData(KsDataStore *data, bool resetPlots) +{ + kshark_context *kshark_ctx(nullptr); + + if (!kshark_instance(&kshark_ctx) || !kshark_ctx->n_streams) + return; + + loadColors(); + + _data = data; + if (resetPlots) + _defaultPlots(kshark_ctx); + _model.fill(_data); } diff --git a/src/KsGLWidget.hpp b/src/KsGLWidget.hpp index 629ae37..6a72a35 100644 --- a/src/KsGLWidget.hpp +++ b/src/KsGLWidget.hpp @@ -101,7 +101,7 @@ public: void keyReleaseEvent(QKeyEvent *event); - void loadData(KsDataStore *data); + void loadData(KsDataStore *data, bool resetPlots); void loadColors(); @@ -326,6 +326,8 @@ private: int _getLastCPU(struct kshark_trace_histo *histo, int bin, int sd, int pid); + void _defaultPlots(kshark_context *kshark_ctx); + void _deselect(); int _bin0Offset() {return _labelSize + 2 * _hMargin;} diff --git a/src/KsMainWindow.cpp b/src/KsMainWindow.cpp index fa893ce..1cbec55 100644 --- a/src/KsMainWindow.cpp +++ b/src/KsMainWindow.cpp @@ -1283,7 +1283,8 @@ void KsMainWindow::_load(const QString& fileName, bool append) QApplication::processEvents(); _view.reset(); - _graph.reset(); + if (!append) + _graph.reset(); auto lamLoadJob = [&, this] () { QVector<kshark_dpi *> v; @@ -1333,7 +1334,7 @@ void KsMainWindow::_load(const QString& fileName, bool append) _view.loadData(&_data); pb.setValue(175); - _graph.loadData(&_data); + _graph.loadData(&_data, !append); pb.setValue(195); } @@ -1454,7 +1455,7 @@ void KsMainWindow::loadSession(const QString &fileName) _view.loadData(&_data); pb.setValue(155); - _graph.loadData(&_data); + _graph.loadData(&_data, true); _filterSyncCBoxUpdate(kshark_ctx); pb.setValue(175); diff --git a/src/KsTraceGraph.cpp b/src/KsTraceGraph.cpp index 1e976df..65e5a79 100644 --- a/src/KsTraceGraph.cpp +++ b/src/KsTraceGraph.cpp @@ -148,12 +148,16 @@ KsTraceGraph::KsTraceGraph(QWidget *parent) * @brief Load and show trace data. * * @param data: Input location for the KsDataStore object. - * KsDataStore::loadDataFile() must be called first. + * KsDataStore::loadDataFile() must be called first. + * @param resetPlots: If true, all existing graphs are closed + * and a default configuration of graphs is displayed + * (all CPU plots). If false, the current set of graphs + * is preserved. */ -void KsTraceGraph::loadData(KsDataStore *data) +void KsTraceGraph::loadData(KsDataStore *data, bool resetPlots) { _data = data; - _glWindow.loadData(data); + _glWindow.loadData(data, resetPlots); updateGeom(); } diff --git a/src/KsTraceGraph.hpp b/src/KsTraceGraph.hpp index 6e83f21..b1132e0 100644 --- a/src/KsTraceGraph.hpp +++ b/src/KsTraceGraph.hpp @@ -45,7 +45,7 @@ class KsTraceGraph : public KsWidgetsLib::KsDataWidget public: explicit KsTraceGraph(QWidget *parent = nullptr); - void loadData(KsDataStore *data); + void loadData(KsDataStore *data, bool resetPlots); void setMarkerSM(KsDualMarkerSM *m); diff --git a/src/KsUtils.cpp b/src/KsUtils.cpp index ec53267..757f49c 100644 --- a/src/KsUtils.cpp +++ b/src/KsUtils.cpp @@ -785,6 +785,7 @@ int KsDataStore::appendDataFile(const QString &file, int64_t offset) _rows = mergedRows; registerCPUCollections(); + emit updateWidgets(this); return sd; } -- 2.27.0
Make sure that all already loaded Data streams are properly closed, before loading a new configuration from session description file. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/KsSession.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/KsSession.cpp b/src/KsSession.cpp index 1e19a5f..b9edb3a 100644 --- a/src/KsSession.cpp +++ b/src/KsSession.cpp @@ -128,6 +128,7 @@ void KsSession::loadDataStreams(kshark_context *kshark_ctx, data->unregisterCPUCollections(); + kshark_close_all(kshark_ctx); dataSize = kshark_import_all_dstreams(kshark_ctx, _config, data->rows_r()); -- 2.27.0
It may sound like a good idea to reinitialize all plugins for all existing Data streams after a new stream is appended, but it isn't. Such reset will re-initialize all Data containers open by the plugins, hence it requires to do a complete reload of all previously loaded data. Currently we reset the plugins but do not reload the data, which is a bug. Because, reloading the data can be slow on large data sets, I prefer to fix the bug, by eliminating the reload of the plugins. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/KsUtils.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/KsUtils.cpp b/src/KsUtils.cpp index 757f49c..3db8951 100644 --- a/src/KsUtils.cpp +++ b/src/KsUtils.cpp @@ -677,8 +677,7 @@ int KsDataStore::_openDataFile(kshark_context *kshark_ctx, if (kshark_is_tep(kshark_ctx->stream[sd])) { kshark_tep_init_all_buffers(kshark_ctx, sd); - for (int i = 0; i < kshark_ctx->n_streams; ++i) - kshark_tep_handle_plugins(kshark_ctx, i); + kshark_tep_handle_plugins(kshark_ctx, sd); } return sd; -- 2.27.0
The lines of the code that initialize and plot the combo point are missing in the original commit. Fixing: fc14e40 (kernel-shark: Add combo point to Mark) Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/KsPlotTools.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp index abef5f8..1d63a9b 100644 --- a/src/KsPlotTools.cpp +++ b/src/KsPlotTools.cpp @@ -680,6 +680,8 @@ Mark::Mark() _cpu._size = 5.5f; _task._color = Color(0, 255, 0); _task._size = 5.5f; + _combo._color = Color(100, 150, 255); + _combo._size = 5.5f; } void Mark::_draw(const Color &col, float size) const @@ -691,6 +693,7 @@ void Mark::_draw(const Color &col, float size) const _cpu.draw(); _task.draw(); + _combo.draw(); } /** -- 2.27.0
When closing a "tep" data stream we destroy the "trace_seq" object. However, trace_seq_destroy() sets the buffer to "TRACE_SEQ_POISON" which is different from NULL. It is unfortunate that TRACE_SEQ_POISON is an internal definition of libtraceevent, so we have to redefine it here, but this can be fixed in the future. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/libkshark-tepdata.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c index bc5babb..4a84141 100644 --- a/src/libkshark-tepdata.c +++ b/src/libkshark-tepdata.c @@ -29,11 +29,17 @@ #include "libkshark-plugin.h" #include "libkshark-tepdata.h" +/** + * The TEP_SEQ_POISON is to catch the use of + * a trace_seq structure after it was destroyed. + */ +#define TEP_SEQ_POISON ((void *)0xdeadbeef) + static __thread struct trace_seq seq; static bool init_thread_seq(void) { - if (!seq.buffer) + if (!seq.buffer || seq.buffer == TEP_SEQ_POISON) trace_seq_init(&seq); return seq.buffer != NULL; -- 2.27.0
All those warning messages are not really relevant for the users of the GUI. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/libkshark-tepdata.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c index 4a84141..323383f 100644 --- a/src/libkshark-tepdata.c +++ b/src/libkshark-tepdata.c @@ -45,6 +45,14 @@ static bool init_thread_seq(void) return seq.buffer != NULL; } +//! @cond Doxygen_Suppress + +void tep_warning(const char *fmt, ...) {} + +void pr_stat(const char *fmt, ...) {} + +//! @endcond + /** Structure for handling all unique attributes of the FTRACE data. */ struct tepdata_handle { /** Page event used to parse the page. */ -- 2.27.0
On Fri, 14 May 2021 15:18:25 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > When closing a "tep" data stream we destroy the "trace_seq" object. > However, trace_seq_destroy() sets the buffer to "TRACE_SEQ_POISON" > which is different from NULL. > > It is unfortunate that TRACE_SEQ_POISON is an internal definition > of libtraceevent, so we have to redefine it here, but this can be > fixed in the future. It's not unfortunate. It can change in the future without breaking API. Redefining it here is not robust, and if trace-seq decides to do something different with that poison value, this will break, and it can't be blamed on API (using internal knowledge to implement code is not protected by being backward compatible). The correct solution is to NULL the buffer after calling destroy. if (seq.buffer) { trace_seq_destroy(&seq); seq.buffer = NULL; } Just like you would do with a pointer you free but may use NULL as a value you are checking. -- Steve > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/libkshark-tepdata.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c > index bc5babb..4a84141 100644 > --- a/src/libkshark-tepdata.c > +++ b/src/libkshark-tepdata.c > @@ -29,11 +29,17 @@ > #include "libkshark-plugin.h" > #include "libkshark-tepdata.h" > > +/** > + * The TEP_SEQ_POISON is to catch the use of > + * a trace_seq structure after it was destroyed. > + */ > +#define TEP_SEQ_POISON ((void *)0xdeadbeef) > + > static __thread struct trace_seq seq; > > static bool init_thread_seq(void) > { > - if (!seq.buffer) > + if (!seq.buffer || seq.buffer == TEP_SEQ_POISON) > trace_seq_init(&seq); > > return seq.buffer != NULL;
On Fri, 14 May 2021 15:18:26 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> All those warning messages are not really relevant for the users
> of the GUI.
>
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
> src/libkshark-tepdata.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
> index 4a84141..323383f 100644
> --- a/src/libkshark-tepdata.c
> +++ b/src/libkshark-tepdata.c
> @@ -45,6 +45,14 @@ static bool init_thread_seq(void)
> return seq.buffer != NULL;
> }
>
> +//! @cond Doxygen_Suppress
> +
> +void tep_warning(const char *fmt, ...) {}
> +
> +void pr_stat(const char *fmt, ...) {}
> +
> +//! @endcond
> +
> /** Structure for handling all unique attributes of the FTRACE data. */
> struct tepdata_handle {
> /** Page event used to parse the page. */
We released libtraceevent 1.3 that has the new logging by Tzvetomir. You
can set it to be quiet without the need to redefine these functions (which
have also been labeled as deprecated).
-- Steve
On 14.05.21 г. 16:31, Steven Rostedt wrote: > On Fri, 14 May 2021 15:18:25 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> When closing a "tep" data stream we destroy the "trace_seq" object. >> However, trace_seq_destroy() sets the buffer to "TRACE_SEQ_POISON" >> which is different from NULL. >> >> It is unfortunate that TRACE_SEQ_POISON is an internal definition >> of libtraceevent, so we have to redefine it here, but this can be >> fixed in the future. > > It's not unfortunate. It can change in the future without breaking API. > > Redefining it here is not robust, and if trace-seq decides to do something > different with that poison value, this will break, and it can't be blamed > on API (using internal knowledge to implement code is not protected by > being backward compatible). > > The correct solution is to NULL the buffer after calling destroy. > > if (seq.buffer) { > trace_seq_destroy(&seq); > seq.buffer = NULL; > } This was the first fix I did when I found the problem, but I don't like it because it looks like a hack the the user of the library is doing to trick the internal logic of the library. Why not just moving the definition of TRACE_SEQ_POISON to the header or adding book trace_seq_is_destroyed() After this we can remove the clone from here. Thanks! Yordan > > Just like you would do with a pointer you free but may use NULL as a value > you are checking. > > -- Steve > > >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> src/libkshark-tepdata.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c >> index bc5babb..4a84141 100644 >> --- a/src/libkshark-tepdata.c >> +++ b/src/libkshark-tepdata.c >> @@ -29,11 +29,17 @@ >> #include "libkshark-plugin.h" >> #include "libkshark-tepdata.h" >> >> +/** >> + * The TEP_SEQ_POISON is to catch the use of >> + * a trace_seq structure after it was destroyed. >> + */ >> +#define TEP_SEQ_POISON ((void *)0xdeadbeef) >> + >> static __thread struct trace_seq seq; >> >> static bool init_thread_seq(void) >> { >> - if (!seq.buffer) >> + if (!seq.buffer || seq.buffer == TEP_SEQ_POISON) >> trace_seq_init(&seq); >> >> return seq.buffer != NULL; >
On Fri, 14 May 2021 16:45:51 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > On 14.05.21 г. 16:31, Steven Rostedt wrote: > > On Fri, 14 May 2021 15:18:25 +0300 > > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > > > >> When closing a "tep" data stream we destroy the "trace_seq" object. > >> However, trace_seq_destroy() sets the buffer to "TRACE_SEQ_POISON" > >> which is different from NULL. > >> > >> It is unfortunate that TRACE_SEQ_POISON is an internal definition > >> of libtraceevent, so we have to redefine it here, but this can be > >> fixed in the future. > > > > It's not unfortunate. It can change in the future without breaking API. > > > > Redefining it here is not robust, and if trace-seq decides to do something > > different with that poison value, this will break, and it can't be blamed > > on API (using internal knowledge to implement code is not protected by > > being backward compatible). > > > > The correct solution is to NULL the buffer after calling destroy. > > > > if (seq.buffer) { > > trace_seq_destroy(&seq); > > seq.buffer = NULL; > > } > > This was the first fix I did when I found the problem, but I don't like > it because it looks like a hack the the user of the library is doing to > trick the internal logic of the library. It's not a hack. seq.buffer is exposed via the API (it's in the header) and is allowed to be used (we use it all the time). The trace_seq_destroy() function is only to clean up everything that trace_seq_init() had done, and the seq is no longer valid, and the user is free to do whatever they want with it afterward. Like set seq.buffer to NULL. This is a perfectly valid use case. > > Why not just moving the definition of TRACE_SEQ_POISON to the header or > adding Because TRACE_SEQ_POISON is an internal API that I never want to expose, because I may even change it in the future. After destroy is called, the trace_seq code is done. If you want to use the trace_seq again, you need to call init. Technically, if you want to do it prim and proper (but I don't actually recommend this), you need to have another variable that keeps track of the seq if it was allocated or not. That's not the responsibility of the trace_seq API to do so. All the trace_seq API cares about is the time trace_seq_init() is called till trace_seq_destroy() is called. Before or after that, the seq is of no use to it. You would need to have: bool seq_is_init; if (!seq_is_init) { trace_seq_init(&seq); seq_is_init = true; } and later if (seq_is_init) { trace_seq_destroy(&seq); seq_is_init = false; } that's if you don't want to use seq.buffer == NULL to do that for you. -- Steve
On 14.05.21 г. 16:57, Steven Rostedt wrote:
> The trace_seq_destroy() function is only to clean up everything that
> trace_seq_init() had done, and the seq is no longer valid, and the user is
> free to do whatever they want with it afterward. Like set seq.buffer to
> NULL.
>
> This is a perfectly valid use case.
OK, thanks!
Y.
On Fri, 14 May 2021 15:18:19 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > Yordan Karadzhov (VMware) (7): > kernel-shark: Preserve markers when appending data > kernel-shark: Preserve open graphs when appending data > kernel-shark: Clear before loading new session > kernel-shark: Better handling of plugins when appending data file > kernel-shark: Do draw the combo point of the mark > kernel-shark: Check if "trace_seq" was destroyed before using it Besides the above patch, the rest look good. Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > kernel-shark: Quiet the warning printing from libtraceevent > > src/KsDualMarker.hpp | 6 ++++++ > src/KsGLWidget.cpp | 40 ++++++++++++++++++++++++++-------------- > src/KsGLWidget.hpp | 4 +++- > src/KsMainWindow.cpp | 31 ++++++++++++++++++++++++++----- > src/KsPlotTools.cpp | 3 +++ > src/KsSession.cpp | 1 + > src/KsTraceGraph.cpp | 10 +++++++--- > src/KsTraceGraph.hpp | 2 +- > src/KsUtils.cpp | 4 ++-- > src/libkshark-tepdata.c | 16 +++++++++++++++- > 10 files changed, 90 insertions(+), 27 deletions(-) >
On Fri, 14 May 2021 15:18:19 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> Yordan Karadzhov (VMware) (7):
> kernel-shark: Preserve markers when appending data
> kernel-shark: Preserve open graphs when appending data
> kernel-shark: Clear before loading new session
> kernel-shark: Better handling of plugins when appending data file
> kernel-shark: Do draw the combo point of the mark
> kernel-shark: Check if "trace_seq" was destroyed before using it
> kernel-shark: Quiet the warning printing from libtraceevent
>
>
After applying and testing out this patch set, I realized that the appended
file does not have any of its CPUs plotted after it is loaded.
-- Steve
On 14.05.21 г. 21:01, Steven Rostedt wrote: > On Fri, 14 May 2021 15:18:19 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> Yordan Karadzhov (VMware) (7): >> kernel-shark: Preserve markers when appending data >> kernel-shark: Preserve open graphs when appending data >> kernel-shark: Clear before loading new session >> kernel-shark: Better handling of plugins when appending data file >> kernel-shark: Do draw the combo point of the mark >> kernel-shark: Check if "trace_seq" was destroyed before using it >> kernel-shark: Quiet the warning printing from libtraceevent >> >> > > After applying and testing out this patch set, I realized that the appended > file does not have any of its CPUs plotted after it is loaded. Yes, this is how it works right now. No new plots are added when you append trace file. Do you think it will be to show all CPU plots from the new trace file? Thanks! Yordan > > -- Steve >
On Mon, 17 May 2021 13:22:34 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:
> On 14.05.21 г. 21:01, Steven Rostedt wrote:
> > On Fri, 14 May 2021 15:18:19 +0300
> > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> >
> >> Yordan Karadzhov (VMware) (7):
> >> kernel-shark: Preserve markers when appending data
> >> kernel-shark: Preserve open graphs when appending data
> >> kernel-shark: Clear before loading new session
> >> kernel-shark: Better handling of plugins when appending data file
> >> kernel-shark: Do draw the combo point of the mark
> >> kernel-shark: Check if "trace_seq" was destroyed before using it
> >> kernel-shark: Quiet the warning printing from libtraceevent
> >>
> >>
> >
> > After applying and testing out this patch set, I realized that the appended
> > file does not have any of its CPUs plotted after it is loaded.
>
> Yes, this is how it works right now. No new plots are added when you
> append trace file.
>
> Do you think it will be to show all CPU plots from the new trace file?
>
Yeah, I think it is better to see something than nothing.
I don't think there's an issue with showing it. Although, if you could
automate the kvm combo to be selected by default on appending a file, then
you wouldn't need to show the cpu plots of the guest.
-- Steve
On 17.05.21 г. 15:54, Steven Rostedt wrote: > On Mon, 17 May 2021 13:22:34 +0300 > Yordan Karadzhov <y.karadz@gmail.com> wrote: > >> On 14.05.21 г. 21:01, Steven Rostedt wrote: >>> On Fri, 14 May 2021 15:18:19 +0300 >>> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: >>> >>>> Yordan Karadzhov (VMware) (7): >>>> kernel-shark: Preserve markers when appending data >>>> kernel-shark: Preserve open graphs when appending data >>>> kernel-shark: Clear before loading new session >>>> kernel-shark: Better handling of plugins when appending data file >>>> kernel-shark: Do draw the combo point of the mark >>>> kernel-shark: Check if "trace_seq" was destroyed before using it >>>> kernel-shark: Quiet the warning printing from libtraceevent >>>> >>>> >>> >>> After applying and testing out this patch set, I realized that the appended >>> file does not have any of its CPUs plotted after it is loaded. >> >> Yes, this is how it works right now. No new plots are added when you >> append trace file. >> >> Do you think it will be to show all CPU plots from the new trace file? >> > > Yeah, I think it is better to see something than nothing. OK, I will send v2 that is doing this. It will be a trivial change. > > I don't think there's an issue with showing it. Although, if you could > automate the kvm combo to be selected by default on appending a file, then > you wouldn't need to show the cpu plots of the guest. This can be done as well but it may be a bit tricky. We should probably also provide a command line option for selecting combo plots. But let's release v2.0 now and keep this feature for v2.1, together with the other features you requested on Bugzilla. Thanks! Yordan > > -- Steve >