From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 789AFC433EF for ; Fri, 7 Jan 2022 10:39:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237327AbiAGKjW (ORCPT ); Fri, 7 Jan 2022 05:39:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48520 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237776AbiAGKjV (ORCPT ); Fri, 7 Jan 2022 05:39:21 -0500 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 13C1CC061245 for ; Fri, 7 Jan 2022 02:39:21 -0800 (PST) Received: by mail-ed1-x52a.google.com with SMTP id j6so20365148edw.12 for ; Fri, 07 Jan 2022 02:39:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :references:from:in-reply-to:content-transfer-encoding; bh=2ck2vvqActQ5RxFN1gKv3/V/5HwlqV89doIXzmNoCrU=; b=g8YXfzx+1GeWXW/lfuW/aWm4sckF1wEjLk6p1HGY0LxJ5KFtxgh/M8oDDv+PE+MpZn zCF+ajr3/5et2R6YdysBpaltUg3+GFVuQkHxfPydd800ILQjSHBYeSCasyMvphJm1HcR hVnrrx2XAZAjQQPAQaY6GnQLuPYzE9nzgifS6Ut/v4qIe80Z6LeSPj51853SlkDFmPid 7PCF6kduuh2Ly83wIgcFuy4h1hNE7X8gUOrX4IhoVAlBjgF4xZfL+DtcEDzacPh2Umts 8Ba/9x4ol5ZNjHg76jjYKmItOfIFxeeGXoFIwCuWfsV08KO6aYtxHPtosdSoU9I7AEcq 2oYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=2ck2vvqActQ5RxFN1gKv3/V/5HwlqV89doIXzmNoCrU=; b=TwDZX6EPxjKRKADMFNU6O9VcETdljxFW1sypITSc4TcBjvaiLtv+J67SsMfdSSnJ+L GGt+r3Oh54gvPRPiXLc4f3HVhwSeXOPqUdqoridiDqxqFKRcN05TUh5dDU/TMmQ9KJ+W vxhCy1bNXpyaPG57WdXe1ut0ThH++nd+Eyc0WXy1Q1R9pyh1ywdlXQ1AZddenCUMrEkX iirPAT20LZhwo2f20Peu8GAO/xY16xzqSqIirGe1AI0tnqZpxvErXb6FcvterEaaysFu /lE1dKO17KpMGES2gIJk9i3RTQVg5EXGFtYRS5p2AXGJQBIt77SsVL9ZucUSrbysOWOT aeJw== X-Gm-Message-State: AOAM532HOxIdF+k6zD9vEgvLAN3DdkaMXrkDVUKwRcMTKKCvjo6at3h1 oDiAGd7ETIOpcmnzD7CgQDNkHgJY248= X-Google-Smtp-Source: ABdhPJxKGcGlpp1Fbx6bQ7Mdp8URLSN5HziqUDLrRpy5qxhkb1sib75IGtZjxW/+tY+zRZXEy9MmqQ== X-Received: by 2002:a50:ef0a:: with SMTP id m10mr22045288eds.236.1641551959558; Fri, 07 Jan 2022 02:39:19 -0800 (PST) Received: from [192.168.1.9] ([95.87.219.163]) by smtp.gmail.com with ESMTPSA id kv5sm14307ejc.113.2022.01.07.02.39.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Jan 2022 02:39:19 -0800 (PST) Message-ID: <7d8dc4c6-cd5c-df38-24e5-62813421c87c@gmail.com> Date: Fri, 7 Jan 2022 12:39:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 Subject: Re: [PATCH 1/2] kernel-shark: Move common APIs and definitions out to avoid duplication Content-Language: en-US To: Hongzhan Chen , linux-trace-devel@vger.kernel.org References: <20220107021846.893-1-hongzhan.chen@intel.com> From: Yordan Karadzhov In-Reply-To: <20220107021846.893-1-hongzhan.chen@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-trace-devel@vger.kernel.org Hi Hongzhan, This patch needs few final touches before going upstream. See my comments below. On 7.01.22 г. 4:18 ч., Hongzhan Chen wrote: > To avoid code duplication, move some common APIs and definitions > out from plugin SchedEvent to share with other plugins. > > Signed-off-by: Hongzhan Chen > > diff --git a/src/KsPlugins.hpp b/src/KsPlugins.hpp > index d41d094..4b29fa6 100644 > --- a/src/KsPlugins.hpp > +++ b/src/KsPlugins.hpp > @@ -13,6 +13,7 @@ > #define _KS_PLUGINS_H > > // C++ > +#include > #include > > // KernelShark > @@ -101,4 +102,66 @@ void eventFieldIntervalPlot(KsCppArgV *argvCpp, > KsPlot::Color col, > float size); > > +/** > + * This class represents the graphical element visualizing the latency between > + * two events such as sched_waking and sched_switch events for common Linux > + * kernel or cobalt_switch_contexts for Xenomai. > + */ > +class LatencyBox : public KsPlot::Rectangle > +{ > + /** On double click do. */ > + void _doubleClick() const override {} > + > +public: > + /** The trace record data that corresponds to this LatencyBox. */ > + std::vector _data; > + > + /** > + * @brief Distance between the click and the shape. Used to decide if > + * the double click action must be executed. > + * > + * @param x: X coordinate of the click. > + * @param y: Y coordinate of the click. > + * > + * @returns If the click is inside the box, the distance is zero. > + * Otherwise infinity. > + */ > + double distance(int x, int y) const override Move the implementation to src/KsPlugins.cpp > + { > + if (x < pointX(0) || x > pointX(2)) > + return std::numeric_limits::max(); > + > + if (y < pointY(0) || y > pointY(1)) > + return std::numeric_limits::max(); > + > + return 0; > + } > +}; > + > +template KsPlot::PlotObject * > +makeLatencyBox(std::vector graph, > + std::vector bins, > + std::vector data, > + KsPlot::Color col, float size) > +{ > + LatencyBox *rec = new T; > + rec->_data = data; > + > + KsPlot::Point p0 = graph[0]->bin(bins[0])._base; > + KsPlot::Point p1 = graph[0]->bin(bins[1])._base; > + int height = graph[0]->height() * .3; > + > + rec->setFill(false); > + rec->setPoint(0, p0.x() - 1, p0.y() - height); > + rec->setPoint(1, p0.x() - 1, p0.y() - 1); > + > + rec->setPoint(3, p1.x() - 1, p1.y() - height); > + rec->setPoint(2, p1.x() - 1, p1.y() - 1); > + > + rec->_size = size; > + rec->_color = col; > + > + return rec; > +} > + > #endif > diff --git a/src/plugins/SchedEvents.cpp b/src/plugins/SchedEvents.cpp > index b73e45f..573e4e3 100644 > --- a/src/plugins/SchedEvents.cpp > +++ b/src/plugins/SchedEvents.cpp > @@ -11,9 +11,6 @@ > * preempted by another task. > */ > > -// C++ > -#include > - > // KernelShark > #include "libkshark.h" > #include "libkshark-plugin.h" > @@ -24,7 +21,7 @@ > > using namespace KsPlot; > > -static KsMainWindow *ks_ptr; > +static KsMainWindow *ks_schedevent_ptr; This name is a bit confusing. It reads as pointer to a sched event while it is a pointer to the KS GUI. What about ks4sched_ptr? > > /** > * @brief Provide the plugin with a pointer to the KsMainWindow object (the GUI > @@ -32,7 +29,7 @@ static KsMainWindow *ks_ptr; > */ > __hidden void *plugin_set_gui_ptr(void *gui_ptr) > { > - ks_ptr = static_cast(gui_ptr); > + ks_schedevent_ptr = static_cast(gui_ptr); > return nullptr; > } > > @@ -40,64 +37,15 @@ __hidden void *plugin_set_gui_ptr(void *gui_ptr) > * This class represents the graphical element visualizing the latency between > * sched_waking and sched_switch events. Add here one sentence explaining that this child class is defined to re-implement the handler for double-click. > */ > -class LatencyBox : public Rectangle > +class SchedLatencyBox : public LatencyBox > { > /** On double click do. */ > void _doubleClick() const override > { > - ks_ptr->markEntry(_data[1]->entry, DualMarkerState::B); > - ks_ptr->markEntry(_data[0]->entry, DualMarkerState::A); > + ks_schedevent_ptr->markEntry(_data[1]->entry, DualMarkerState::B); > + ks_schedevent_ptr->markEntry(_data[0]->entry, DualMarkerState::A); > } > > -public: > - /** The trace record data that corresponds to this LatencyBox. */ > - std::vector _data; > - > - /** > - * @brief Distance between the click and the shape. Used to decide if > - * the double click action must be executed. > - * > - * @param x: X coordinate of the click. > - * @param y: Y coordinate of the click. > - * > - * @returns If the click is inside the box, the distance is zero. > - * Otherwise infinity. > - */ > - double distance(int x, int y) const override > - { > - if (x < pointX(0) || x > pointX(2)) > - return std::numeric_limits::max(); > - > - if (y < pointY(0) || y > pointY(1)) > - return std::numeric_limits::max(); > - > - return 0; > - } > -}; > - > -static PlotObject *makeShape(std::vector graph, > - std::vector bins, > - std::vector data, > - Color col, float size) > -{ > - LatencyBox *rec = new LatencyBox; > - rec->_data = data; > - > - Point p0 = graph[0]->bin(bins[0])._base; > - Point p1 = graph[0]->bin(bins[1])._base; > - int height = graph[0]->height() * .3; > - > - rec->setFill(false); > - rec->setPoint(0, p0.x() - 1, p0.y() - height); > - rec->setPoint(1, p0.x() - 1, p0.y() - 1); > - > - rec->setPoint(3, p1.x() - 1, p1.y() - height); > - rec->setPoint(2, p1.x() - 1, p1.y() - 1); > - > - rec->_size = size; > - rec->_color = col; > - > - return rec; > }; > > /* > @@ -191,14 +139,14 @@ __hidden void plugin_draw(kshark_cpp_argv *argv_c, > eventFieldIntervalPlot(argvCpp, > plugin_ctx->sw_data, checkFieldSW, > plugin_ctx->ss_data, checkEntryPid, > - makeShape, > + makeLatencyBox, > {0, 255, 0}, // Green > -1); // Default size > > eventFieldIntervalPlot(argvCpp, > plugin_ctx->ss_data, checkFieldSS, > plugin_ctx->ss_data, checkEntryPid, > - makeShape, > + makeLatencyBox, > {255, 0, 0}, // Red > -1); // Default size > } > diff --git a/src/plugins/common_sched.h b/src/plugins/common_sched.h > new file mode 100644 > index 0000000..1d564c0 > --- /dev/null > +++ b/src/plugins/common_sched.h > @@ -0,0 +1,52 @@ > +/* SPDX-License-Identifier: LGPL-2.1 */ > + > +/* > + * Copyright (C) 2021 Intel Inc, Hongzhan Chen Since this is a 'copy-paste' code I think you should keep the original copyright statement on top and add the new one (2021, Intel, Hongzhan) on the next line. > + */ > + > +/** > + * @file common_sched.h > + * @brief Plugin for common sched. Perhaps you mean 'Common definitions for sched plugins?' > + */ > + > +#ifndef _KS_PLUGIN_COMMON_SCHED_H > +#define _KS_PLUGIN_COMMON_SCHED_H > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +typedef unsigned long long tep_num_field_t; > + > +/** The type of the data field stored in the kshark_data_container object. */ > +typedef int64_t ks_num_field_t; > + > +#define PREV_STATE_SHIFT ((int) ((sizeof(ks_num_field_t) - 1) * 8)) > + > +#define PREV_STATE_MASK (((ks_num_field_t) 1 << 8) - 1) > + > +#define PID_MASK (((ks_num_field_t) 1 << PREV_STATE_SHIFT) - 1) > + > +static inline void plugin_sched_set_pid(ks_num_field_t *field, > + tep_num_field_t pid) > +{ > + *field &= ~PID_MASK; > + *field = pid & PID_MASK; Hmm, this looks like a bug in the schedevent plugin. This line should be *field |= pid & PID_MASK; > +} > + > +/** > + * @brief Retrieve the PID value from the data field stored in the > + * kshark_data_container object. > + * > + * @param field: Input location for the data field. > + */ > +static inline int plugin_sched_get_pid(ks_num_field_t field) > +{ > + return field & PID_MASK; > +} > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif > diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c > index 198ed49..3bb9bc2 100644 > --- a/src/plugins/sched_events.c > +++ b/src/plugins/sched_events.c > @@ -22,36 +22,6 @@ > > /** Plugin context instance. */ > > -//! @cond Doxygen_Suppress > - > -typedef unsigned long long tep_num_field_t; > - > -#define PREV_STATE_SHIFT ((int) ((sizeof(ks_num_field_t) - 1) * 8)) > - > -#define PREV_STATE_MASK (((ks_num_field_t) 1 << 8) - 1) > - > -#define PID_MASK (((ks_num_field_t) 1 << PREV_STATE_SHIFT) - 1) > - > -//! @endcond > - > -static void plugin_sched_set_pid(ks_num_field_t *field, > - tep_num_field_t pid) > -{ > - *field &= ~PID_MASK; > - *field = pid & PID_MASK; > -} > - > -/** > - * @brief Retrieve the PID value from the data field stored in the > - * kshark_data_container object. > - * > - * @param field: Input location for the data field. > - */ > -__hidden int plugin_sched_get_pid(ks_num_field_t field) > -{ > - return field & PID_MASK; > -} > - > /* Use the most significant byte to store the value of "prev_state". */ > static void plugin_sched_set_prev_state(ks_num_field_t *field, > tep_num_field_t prev_state) > diff --git a/src/plugins/sched_events.h b/src/plugins/sched_events.h > index 2c540fd..1032075 100644 > --- a/src/plugins/sched_events.h > +++ b/src/plugins/sched_events.h > @@ -9,12 +9,13 @@ > * @brief Plugin for Sched events. > */ > > -#ifndef _KS_PLUGIN_SHED_H > -#define _KS_PLUGIN_SHED_H > +#ifndef _KS_PLUGIN_SCHED_H > +#define _KS_PLUGIN_SCHED_H Good catch, please make this a separate patch. Thanks! Yordan > > // KernelShark > #include "libkshark.h" > #include "libkshark-plugin.h" > +#include "plugins/common_sched.h" > > #ifdef __cplusplus > extern "C" { > @@ -55,10 +56,6 @@ struct plugin_sched_context { > > KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_sched_context) > > -/** The type of the data field stored in the kshark_data_container object. */ > -typedef int64_t ks_num_field_t; > - > -int plugin_sched_get_pid(ks_num_field_t field); > > int plugin_sched_get_prev_state(ks_num_field_t field); > >