Addition of LevelZero metric support to the LevelZero device pool and IO Group #2897

lhlawson · 2023-03-15T17:58:17Z

Relates to As a user of Intel Hardware I would like to be able to access LevelZero Metrics #2148 feature quest from github issues
Provides Intel hardware support analogous to the NVIDIA support added in Addition of DCGMDevicePool for NVIDIA Profiling Metrics #1917 and Added DCGMIOGroup for NVIDIA Profiling Metrics #1914

The addition of ZET metric polling capability to the LevelZero IO Group, Device Pool, and API shim.

General Metric Streamer flowchart implemented follows the LevelZero approach shown here

Details of change:

Updated LevelZero.cpp to set ZET environment variables and follow the general time based metric query approach outlined in the LevelZero Tools API.
Added querying for the 'ComputeBasics' group of Intel Hardware. An additional metric group may be queried but is not supported at this time.

Additional Tasks:

Define Default LevelZero Tools report/event rate based on workload performance
Add Test for signal and control of LevelZero Tools.
Later PR: Consider adding LEVELZERO::METRICS:XVE_*:AGGREGATION. Currently all signals are averaged over the number of reports received, but this could be modified to be AVG, LAST, MIN, MAX, etc
Test dynamic tracing with geopm controller (read_signal and batch read have been tested, but not with the controller)
Add multi-tile support
Re-implement metric_destroy/teardown mechanism
Remove geopmread functionality until an appropriate tear-down handling with read_batch and read_signal can occur.
Later PR: Add 'check and fail' if the metric group is changed out from under the user

dannosliwcd · 2023-03-22T20:29:40Z

service/src/LevelZero.cpp

@@ -19,11 +19,13 @@
 static void __attribute__((constructor)) geopm_levelzero_init(void)
 {
    setenv("ZES_ENABLE_SYSMAN", "1", 1);
+//    setenv("ZET_ENABLE_METRICS", "1", 1);


Suggested change

// setenv("ZET_ENABLE_METRICS", "1", 1);

Re-enabled ZET by default.

dannosliwcd · 2023-03-22T20:32:53Z

service/src/LevelZero.cpp

+        for (int subdevice_idx = 0;
+         subdevice_idx < m_devices.at(device_idx).m_num_subdevice;
+         ++subdevice_idx) {
+            //assume false


Seems to just be truth rather than assumption at this point, since it isn't cached or initialized yet

dannosliwcd · 2023-03-22T20:41:32Z

service/src/LevelZero.cpp

+
+                std::vector<zet_metric_group_handle_t> metric_group_handle(num_metric_group);
+
+                //TODO: save the relevant per GPU pointers


Which pointers?

TODO removed, this was a hold over from when the metric group handle, context, etc were not being stored in the tracking struct.

dannosliwcd · 2023-03-22T20:43:09Z

service/src/LevelZero.cpp

+                                __LINE__);
+
+                //sampling period in nanoseconds
+                m_devices.at(device_idx).metric_sampling_period = 2000000;


Optionally rename to make sure the units are known everywhere this is used. E.g., sampling_period_ns

dannosliwcd · 2023-03-22T20:54:56Z

service/src/LevelZeroImp.hpp

@@ -155,6 +188,10 @@ namespace geopm

            std::vector<ze_driver_handle_t> m_levelzero_driver;
            std::vector<m_device_info_s> m_devices;
+
+            void metric_group_cache(unsigned int l0_device_idx);


Need a comment explaining what this is caching and/or what data or functions are invalid until this is cached. My read-through understanding is:

Caches to several parts of m_devices. I guess this function needs to be called after initialization and before attempting to request any metric data.

Caches the names of metrics in the ComputeBasic L0 metric group.

Maybe rename this to indicate that it is hard-coded to computebasic. Or parameterize the computebasic part.

Or docs for the other functions might cover that (e.g., they say they'll throw if not yet cached)

Comment added

dannosliwcd · 2023-03-22T21:02:33Z

service/src/LevelZero.cpp

+        ze_event_pool_handle_t event_pool_handle = nullptr;
+        ze_event_pool_desc_t event_pool_desc = {ZE_STRUCTURE_TYPE_EVENT_POOL_DESC, nullptr, 0, 1};
+
+        ze_result = zeEventPoolCreate(context, &event_pool_desc, 1,


Knowledge check for myself. Docs say 1 here is the number of devices. Is this 1 because we're initializing one GPU at a time?

From reading further down I guess it also means we're sampling one device at a time, since this is the event that read attempts block on while waiting for metric data

We're targeting 1 event in each event pool, and 1 sub-device per event.

dannosliwcd · 2023-03-22T21:04:53Z

service/src/LevelZero.cpp

+        zet_metric_streamer_desc_t metric_streamer_desc = {
+            ZET_STRUCTURE_TYPE_METRIC_STREAMER_DESC,
+            nullptr,
+            32768,


Why this number of reports per notification?

Updated to 4 reports (1ms per report target)

service/src/LevelZero.cpp

dannosliwcd · 2023-03-22T21:21:41Z

service/src/LevelZero.cpp

+                            m_devices.at(l0_device_idx).subdevice.metric_streamer.at(l0_domain_idx));
+            }
+            else {
+                metric_read(l0_device_idx, l0_domain_idx); //TODO: possible infinite loop


Should this have a timeout mechanism? E.g., change it to a loop around line 858 and trigger an error if too many attempts or too much time has passed without the event

Might want to add a non-zero timeout to the last arg in zeEventHostSynchronize if going that route

The recursive call is removed in the most recent version. In cases where the event is not ready the old data will be provided to the user.
We may want to add a 'collected' timestamp so the user can identify if this happens.

dannosliwcd · 2023-03-22T21:28:58Z

service/src/LevelZero.cpp

+            if (ze_host_result != ZE_RESULT_NOT_READY) {
+                metric_calc(l0_device_idx, l0_domain_idx,
+                            m_devices.at(l0_device_idx).subdevice.metric_streamer.at(l0_domain_idx));
+            }
+            else {


Should probably be something like if == NOT_READY then retry else if SUCCESS then calc() else error

Retry logic removed in most recent version.

service/src/LevelZero.cpp

service/src/LevelZero.hpp

avilcheslopez · 2023-03-22T19:12:12Z

service/src/LevelZero.cpp

+         subdevice_idx < m_devices.at(device_idx).m_num_subdevice;
+         ++subdevice_idx) {


nit: please fix indentation

service/src/LevelZeroImp.hpp

service/src/LevelZero.cpp

avilcheslopez · 2023-03-22T21:37:28Z

service/src/LevelZero.cpp

+                    for (unsigned int report_idx = 0; report_idx < num_reports; report_idx++)
+                    {
+                        //This is the actual gathering of the data
+                        zet_typed_value_t data = metric_values.at(report_idx*num_metric + metric_idx);


Preferably, it would be better if a data structure was used that more naturally represented the data being stored. If it's not convenient for whatever reason, please add a comment explaining this formula and how metric_values is structured such that this formula correctly indexes into the data item of interest.

I'm a bit nervous that report_idx depends on num_reports which is num_metric_values / num_metric. Does this formula take into account what happens when num_metric_values is not divisible by num_metric? If needed, please add checks and/or assertions to ensure any preconditions needed for this formula to work.

avilcheslopez · 2023-03-22T21:40:07Z

service/src/LevelZero.cpp

+            m_devices.at(l0_device_idx).subdevice.m_metric_data.at(l0_domain_idx).at("XVE_ACTIVE") = {};
+            m_devices.at(l0_device_idx).subdevice.m_metric_data.at(l0_domain_idx).at("XVE_STALL") = {};


Consider putting these strings (the metric names) in symbolic constants, as they're used in multiple places (including the IOGroup).

service/src/LevelZeroImp.hpp

bgeltz · 2023-04-03T16:53:42Z

service/src/LevelZero.cpp


 #include "LevelZeroImp.hpp"

 static void __attribute__((constructor)) geopm_levelzero_init(void)
 {
    setenv("ZES_ENABLE_SYSMAN", "1", 1);
+    setenv("ZET_ENABLE_METRICS", "1", 1);


Should this be set conditionally somehow?

If it's too late to set this when push_signal() is called with metrics based signals/controls, then I think this has to be done at compile time with a #define.

We're setting this at all times, but having the actual usage of ZET (and context running on the GPU) be conditional on the user doing a push signal of a ZET metric and the first read of those metrics.

bgeltz · 2023-04-03T16:53:58Z

service/src/LevelZero.hpp

-    const LevelZero &levelzero();
+    LevelZero &levelzero();


Why was this required?

This change was required for the lazy init approach being used to update some level zero metric containers. After more recent code changes it might not be required.

service/src/LevelZero.cpp

dannosliwcd · 2023-04-13T15:33:57Z

service/src/LevelZero.cpp


        zet_metric_streamer_desc_t metric_streamer_desc = {
            ZET_STRUCTURE_TYPE_METRIC_STREAMER_DESC,
            nullptr,
-            32768,
-            m_devices.at(l0_device_idx).metric_sampling_period};
+            4, // number of reports to notify on.


Same question as before: why 4 reports at 1 ms per sample? Is that something to do with our typical 5ms agent loops?

service/src/LevelZero.cpp

avilcheslopez

I'm still seeing multiple "TODO" comments, which makes it seem like this is still in draft form. Which of those TODOs need to be addressed as part of this PR?

avilcheslopez · 2023-04-24T15:58:04Z

service/src/LevelZero.cpp

+        // Allocate memory for future reads
+        size_t data_size = 0;
+        ze_result = zetMetricStreamerReadData(m_devices.at(l0_device_idx).subdevice.metric_streamer.at(l0_domain_idx),
+                                              UINT32_MAX, &data_size, nullptr); //TODO: this value should match the report_count_req in metric_read


nit: alignment

avilcheslopez · 2023-04-24T17:01:56Z

service/src/LevelZero.cpp

+             subdevice_idx < m_devices.at(gpu_idx).num_subdevice;
+             ++subdevice_idx) {


nit: fix indentation

avilcheslopez · 2023-04-24T20:58:58Z

service/src/LevelZero.cpp

+
+            std::string metric_name (metric_properties.name);
+
+            //TODO: check timing impact of only processing metrics supported by IOGroup


TODO for this PR?

avilcheslopez · 2023-04-24T20:59:18Z

service/src/LevelZero.cpp

+                            ": LevelZero Metric property acquisition failed",
+                            __LINE__);
+
+            std::string metric_name (metric_properties.name);


nit:

Suggested change

std::string metric_name (metric_properties.name);

std::string metric_name(metric_properties.name);

avilcheslopez · 2023-04-24T20:59:29Z

service/src/LevelZero.cpp

+                metric_name == "XVE_STALL") {
+                for (unsigned int report_idx = 0; report_idx < num_reports; report_idx++) {
+                    // Each report contains num_metric entries of data.  We need to access
+                    // this metric (metric_idx) from each report ( report_idx * num_metric)


nit:

Suggested change

// this metric (metric_idx) from each report ( report_idx * num_metric)

// this metric (metric_idx) from each report (report_idx * num_metric)

avilcheslopez · 2023-04-24T21:00:21Z

service/src/LevelZero.cpp

+
+        for (unsigned int metric_idx = 0; metric_idx < num_metric; metric_idx++)
+        {
+            //TODO: It is possible that simply parsing all the metrics is


TODO for this PR?

avilcheslopez · 2023-04-25T14:36:05Z

service/src/LevelZero.cpp

+                        ": LevelZero Event Create failed",
+                        __LINE__);
+
+        // TODO: nothing guarantees CHIP 0 was called first


avilcheslopez · 2023-04-25T14:38:28Z

service/src/LevelZero.cpp

+        zet_metric_streamer_desc_t metric_streamer_desc = {
+            ZET_STRUCTURE_TYPE_METRIC_STREAMER_DESC,
+            nullptr,
+            4, // number of reports to notify on.  Targeting 4 reports, 1 per millisecond as that will, generally, 


This would be better to place in a symbolic constant, to make it easier to update when needed (as I know you've had to test different values), and make the code easier to read.

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Co-authored-by: Brad Geltz <brgeltz@gmail.com>

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

…read of METRIC signal Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Signed-off-by: Alejandro Vilches <alejandro.vilches@intel.com>

lhlawson mentioned this pull request Mar 15, 2023

Addition of LevelZero metric support to the LevelZero device pool and IO Group #2149

Closed

5 tasks

lhlawson force-pushed the public-lhlawson-zet-support branch from 754421c to a0aa7e1 Compare March 16, 2023 17:08

lhlawson requested review from bgeltz and avilcheslopez March 16, 2023 17:13

lhlawson force-pushed the public-lhlawson-zet-support branch from a0aa7e1 to 3327b5d Compare March 16, 2023 17:14

lhlawson requested a review from dannosliwcd March 16, 2023 17:31

dannosliwcd reviewed Mar 22, 2023

View reviewed changes

avilcheslopez reviewed Mar 22, 2023

View reviewed changes

service/src/LevelZeroImp.hpp Outdated Show resolved Hide resolved

lhlawson force-pushed the public-lhlawson-zet-support branch from 9de489f to 8ead8a9 Compare March 27, 2023 23:12

lhlawson marked this pull request as ready for review March 29, 2023 01:20

bgeltz requested changes Apr 3, 2023

View reviewed changes

lhlawson commented Apr 3, 2023

View reviewed changes

service/src/LevelZero.cpp Outdated Show resolved Hide resolved

dannosliwcd reviewed Apr 13, 2023

View reviewed changes

lhlawson commented Apr 13, 2023

View reviewed changes

service/src/LevelZero.cpp Outdated Show resolved Hide resolved

dannosliwcd previously approved these changes Apr 13, 2023

View reviewed changes

avilcheslopez reviewed Apr 25, 2023

View reviewed changes

cmcantalupo force-pushed the dev branch 2 times, most recently from 2cd441d to 0859ce3 Compare October 19, 2023 00:41

cmcantalupo force-pushed the dev branch from 54d8906 to 3a36fcd Compare October 26, 2023 02:54

avilcheslopez dismissed dannosliwcd’s stale review via 62bf92c December 5, 2023 23:10

avilcheslopez force-pushed the public-lhlawson-zet-support branch 2 times, most recently from b73a7af to 8f287c2 Compare December 8, 2023 18:07

avilcheslopez force-pushed the public-lhlawson-zet-support branch from 8f287c2 to 94169ed Compare January 31, 2024 21:55

lhlawson added 6 commits February 7, 2024 13:54

First Pass LevelZero Tools & Metrics Support using metriccalcexp calls

5bb9dcf

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Moved to single tile API, with GPU_CHIP support

3a3940a

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Removed timing code

95c6fcf

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Cleanup, fixed debug message

240f3a3

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Fix-up from debug with Brad, disable all but 1 GPU

b524c27

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Addressed Review Feedback

93d5448

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

lhlawson and others added 18 commits February 7, 2024 13:54

Addressed Review Comments: function names, spacing, DEBUG asserts

c41cd3e

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

fixup: missed change of metric_group_cache to metric_group_init

fa2ffb7

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

updated L0 mocks with ZET related functions

a2df517

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Addressed Review Feedback: remove m_ from inner members of structs

1b6da07

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Legacy tests passing

7ee3abc

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Update read batch test fix num_report scaling factor issue

14fda8b

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Added restriction on ZET read_signal, updated read_batch test

3913950

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Added ZET tests for L0 dev pool, updated dev pool to use reduce

44bf8fe

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Apply suggestions from code review

50c2603

Co-authored-by: Brad Geltz <brgeltz@gmail.com>

Add comment on for loop break

dc99b4b

Fixed typo

5cf658f

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Made initial data sizing for metric stream a one time operation

27124a4

Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

removed dumping of extra data in buffer, removed error on L0 IOGroup …

39f6d40

…read of METRIC signal Signed-off-by: lhlawson <lowren.h.lawson@intel.com>

Update comment on number of report choice.

7c461e0

Fix zet branch build and unit test failures

56a3fa9

Signed-off-by: Alejandro Vilches <alejandro.vilches@intel.com>

Fix typos in LevelZero.cpp

051ee12

Signed-off-by: Alejandro Vilches <alejandro.vilches@intel.com>

Don't throw when reading level zero METRIC signals

d437996

Signed-off-by: Alejandro Vilches <alejandro.vilches@intel.com>

Fix minor issues after rebase

bc6da09

Signed-off-by: Alejandro Vilches <alejandro.vilches@intel.com>

avilcheslopez force-pushed the public-lhlawson-zet-support branch from 90c85df to bc6da09 Compare February 7, 2024 21:54

bgeltz mentioned this pull request Mar 14, 2024

Review and merge L0 Tools & Metrics PR #3372

Open

2 tasks

avilcheslopez mentioned this pull request Mar 14, 2024

LevelZeroIOGroup not gathering LevelZero metrics #3374

Open


		std::vector<zet_metric_group_handle_t> metric_group_handle(num_metric_group);

		//TODO: save the relevant per GPU pointers

		subdevice_idx < m_devices.at(device_idx).m_num_subdevice;
		++subdevice_idx) {

		m_devices.at(l0_device_idx).subdevice.m_metric_data.at(l0_domain_idx).at("XVE_ACTIVE") = {};
		m_devices.at(l0_device_idx).subdevice.m_metric_data.at(l0_domain_idx).at("XVE_STALL") = {};

		subdevice_idx < m_devices.at(gpu_idx).num_subdevice;
		++subdevice_idx) {


		std::string metric_name (metric_properties.name);

		//TODO: check timing impact of only processing metrics supported by IOGroup

	std::string metric_name (metric_properties.name);
	std::string metric_name(metric_properties.name);

	// this metric (metric_idx) from each report ( report_idx * num_metric)
	// this metric (metric_idx) from each report (report_idx * num_metric)

Addition of LevelZero metric support to the LevelZero device pool and IO Group #2897

Are you sure you want to change the base?

Addition of LevelZero metric support to the LevelZero device pool and IO Group #2897

Conversation

lhlawson commented Mar 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avilcheslopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhlawson commented Mar 15, 2023 •

edited