Skip to content

Allow specifiying data areas 1-4 when generating telemetry logs #2755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dsfw-opensource wants to merge 1 commit into from
Closed

Allow specifiying data areas 1-4 when generating telemetry logs #2755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/ocp/ocp-nvme.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1465,8 +1465,8 @@ static int ocp_telemetry_log(int argc, char **argv, struct command *cmd, struct
if (!opt.data_area) {
nvme_show_result("Missing data-area. Using default data area 1.\n");
opt.data_area = DATA_AREA_1;//Default data area 1
} else if (opt.data_area != 1 && opt.data_area != 2) {
nvme_show_result("Invalid data-area specified. Please specify 1 or 2.\n");
} else if ((opt.data_area < DATA_AREA_1) || (opt.data_area > DATA_AREA_4)) {
nvme_show_result("Invalid data-area specified. Please specify 1-4.\n");
goto out;
}

Expand Down
35 changes: 18 additions & 17 deletions plugins/ocp/ocp-telemetry-decode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,9 +980,10 @@

int status = 0;
unsigned int event_fifo_number = fifo_num + 1;
char *description = (char *)malloc((40 + 1) * sizeof(char));
const size_t desc_size = sizeof(char) * (40 + 1);
char *description = (char *)malloc(desc_size);

memset(description, 0, sizeof(40));
memset(description, 0, desc_size);

status =
parse_ocp_telemetry_string_log(event_fifo_number, 0, 0, EVENT_STRING, description);
Expand Down Expand Up @@ -1202,9 +1203,9 @@
(event_fifo[fifo_no].event_fifo_size * SIZE_OF_DWORD);
__u8 *pfifo_start = NULL;

if (event_fifo[fifo_no].event_fifo_da == 1)
if (event_fifo[fifo_no].event_fifo_da == DATA_AREA_1)
pfifo_start = pda1_header_offset + fifo_offset;
else if (event_fifo[fifo_no].event_fifo_da == 2)
else if (event_fifo[fifo_no].event_fifo_da == DATA_AREA_2)
pfifo_start = pda2_offset + fifo_offset;
else {
nvme_show_error("Unsupported Data Area:[%d]", poffsets->data_area);
Expand All @@ -1222,7 +1223,7 @@
}

if (pevent_fifos_object != NULL && root != NULL) {
const char *data_area = (poffsets->data_area == 1 ? STR_DA_1_EVENT_FIFO_INFO :
const char *data_area = (poffsets->data_area == DATA_AREA_1 ? STR_DA_1_EVENT_FIFO_INFO :

Check failure on line 1226 in plugins/ocp/ocp-telemetry-decode.c

View workflow job for this annotation

GitHub Actions / checkpatch review 

WARNING: line length of 104 exceeds 100 columns
Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the data area is 3 or 4 then the data area 2 event fifo information used but is it expected as correct?

Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are checkpatch.pl 2 warnings as below.

tokunori@tokunori-desktop:~/nvme-cli-2$ ~/linux/scripts/checkpatch.pl -g HEAD
WARNING: 'specifiying' may be misspelled - perhaps 'specifying'?
#4:
Subject: [PATCH] Allow specifiying data areas 1-4 when generating telemetry
                       ^^^^^^^^^^^

WARNING: line length of 104 exceeds 100 columns
#68: FILE: plugins/ocp/ocp-telemetry-decode.c:1226:
+               const char *data_area = (poffsets->data_area == DATA_AREA_1 ? STR_DA_1_EVENT_FIFO_INFO :

total: 0 errors, 2 warnings, 120 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit ffa58cc6d956 ("Allow specifiying data areas 1-4 when generating telemetry logs") has style problems, please review.

NOTE: Ignored message types: EMAIL_SUBJECT FILE_PATH_CHANGES GERRIT_CHANGE_ID GIT_COMMIT_ID NOT_UNIFIED_DIFF

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the behavior makes sense. Is there a maintainer I can contact to confirm if this is the intended behavior? The specification is not very clear, but I based it on the original behavior where, if a user requested data area 2, they also received data areas 1 and 2 parsed into a human-readable format. Following that logic, I would expect data area 3 to include data areas 1 and 2 parsed into a human-readable format, with data areas >= 3 remaining in the raw binary log since it is vendor-specific.

Copy link
Copy Markdown
Collaborator

@igaw igaw Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base spec has the following on telemetry log:

image

I assume the ocp spec will follow the base spec, so no need to include the areas 1,2 if 3 is selected.

To clarify this you could contact the authors of the ocp spec though.

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will try contacting the authors.

STR_DA_2_EVENT_FIFO_INFO);

json_object_add_value_array(root, data_area, pevent_fifos_object);
Expand Down Expand Up @@ -1410,7 +1411,7 @@
__u8 *pstats_offset = NULL;
int parse_rc = 0;

if (poffsets->data_area == 1) {
if (poffsets->data_area == DATA_AREA_1) {
__u32 stats_da_1_start = *(__u32 *)(pda1_ocp_header_offset +
offsetof(struct nvme_ocp_header_in_da1, da1_statistic_start));
__u32 stats_da_1_size = *(__u32 *)(pda1_ocp_header_offset +
Expand All @@ -1422,7 +1423,7 @@

pstats_offset = pda1_ocp_header_offset + stats_da_1_start_dw;
statistics_size = stats_da_1_size_dw;
} else if (poffsets->data_area == 2) {
} else if (poffsets->data_area == DATA_AREA_2) {
__u32 stats_da_2_start = *(__u32 *)(pda1_ocp_header_offset +
offsetof(struct nvme_ocp_header_in_da1, da2_statistic_start));
__u32 stats_da_2_size = *(__u32 *)(pda1_ocp_header_offset +
Expand Down Expand Up @@ -1459,7 +1460,7 @@

if (root != NULL && pstats_array != NULL) {
const char *pdata_area =
(poffsets->data_area == 1 ? STR_DA_1_STATS : STR_DA_2_STATS);
(poffsets->data_area == DATA_AREA_1 ? STR_DA_1_STATS : STR_DA_2_STATS);
Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also correct to use the data area 2 statistics for the data area 3 or 4?

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data area 3 and 4 are vendor-specific. This is just to ensure data area 2 also gets parsed.


json_object_add_value_array(root, pdata_area, pstats_array);
}
Expand Down Expand Up @@ -1509,7 +1510,7 @@
//Set DA to 1 and get offsets
Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below seems better for the comment. Anyway a space needed after // I think.

- 			//Set DA to 1 and get offsets
+ 			/* Set DA to 1 and get offsets */

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will change it.

struct nvme_ocp_telemetry_offsets offsets = { 0 };

offsets.data_area = 1;// Default DA - DA1
offsets.data_area = DATA_AREA_1;// Default DA - DA1
Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below seems better for the comment.

- 			offsets.data_area = DATA_AREA_1;// Default DA - DA1
+ 			offsets.data_area = DATA_AREA_1; /* Default DA - DA1 */

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will change it.


struct nvme_ocp_telemetry_common_header *ptelemetry_common_header =
(struct nvme_ocp_telemetry_common_header *) ptelemetry_buffer;
Expand Down Expand Up @@ -1562,8 +1563,8 @@
}

//Set the DA to 2
if (options->data_area == 2) {
offsets.data_area = 2;
if (options->data_area >= DATA_AREA_2) {
offsets.data_area = DATA_AREA_2;
fprintf(fp, STR_LINE);
fprintf(fp, "%s\n", STR_DA_2_STATS);
fprintf(fp, STR_LINE);
Expand Down Expand Up @@ -1622,7 +1623,7 @@
//Set DA to 1 and get offsets
struct nvme_ocp_telemetry_offsets offsets = { 0 };

offsets.data_area = 1;
offsets.data_area = DATA_AREA_1;

struct nvme_ocp_telemetry_common_header *ptelemetry_common_header =
(struct nvme_ocp_telemetry_common_header *) ptelemetry_buffer;
Expand Down Expand Up @@ -1671,8 +1672,8 @@
}

//Set the DA to 2
if (options->data_area == 2) {
offsets.data_area = 2;
if (options->data_area >= DATA_AREA_2) {
offsets.data_area = DATA_AREA_2;
Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary to set DATA_AREA_3 or DATA_AREA_4 for the data area 3 and 4?

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data areas 3 and 4 are vendor specific.

printf(STR_LINE);
printf("%s\n", STR_DA_2_STATS);
printf(STR_LINE);
Expand Down Expand Up @@ -1730,7 +1731,7 @@
struct nvme_ocp_telemetry_offsets offsets = { 0 };

//Set DA to 1 and get offsets
offsets.data_area = 1;
offsets.data_area = DATA_AREA_1;
struct nvme_ocp_telemetry_common_header *ptelemetry_common_header =
(struct nvme_ocp_telemetry_common_header *) ptelemetry_buffer;

Expand Down Expand Up @@ -1776,9 +1777,9 @@
return -1;
}

if (options->data_area == 2) {
if (options->data_area >= DATA_AREA_2) {
//Set the DA to 2
offsets.data_area = 2;
offsets.data_area = DATA_AREA_2;
Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also really expected for the data area 3 and 4?

Copy link
Copy Markdown
Contributor

@ikegami-t ikegami-t Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway if for the option data area 3 or 4 specified it is expected to use the data area 2 internally then the description comment needed I think.

Copy link
Copy Markdown
Author

@dsfw-opensource dsfw-opensource Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will add a comment.

//Data Area 2 Statistics
status = parse_statistics(root, &offsets, NULL);
if (status != 0) {
Expand Down
Loading