Skip to content
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

Jump to bottom

nvme/plugins: Add performance stats v2 #2176

Closed
wants to merge 2 commits into from
Closed

nvme/plugins: Add performance stats v2 #2176

wants to merge 2 commits into from

Conversation

jinhuahuang2021
Copy link
Contributor

Add performance stats v2

@jinhuahuang2021 jinhuahuang2021 force-pushed the feature/nvme-performance-stats-v2-1812 branch 16 times, most recently from 6a3deef to 69c67d5 Compare December 27, 2023 12:20
@jinhuahuang2021
nvme/plugins: Add performance stats v2
f543444 
Add performance stats v2

Signed-off-by: jinhua.huang <jinhua.huang@memblaze.com>
@jinhuahuang2021 jinhuahuang2021 force-pushed the feature/nvme-performance-stats-v2-1812 branch from 69c67d5 to f543444 Compare December 27, 2023 12:22
igaw
igaw requested changes Jan 18, 2024
View reviewed changes
Copy link
Collaborator

@igaw igaw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

some nitpicking:

  • split the white space changes from the new code
  • pay more attention on proper alignment
  • use the new cleanup helpers for struct nvme_dev allocations
  • change commit prefix to plugins/memblaze: ....

Rest looks okay.

plugins/memblaze/memblaze-nvme.c
@@ -107,7 +107,8 @@ static __u64 raw_2_u64(const __u8 *buf, size_t len)
return le64_to_cpu(val);
}

static void get_memblaze_new_smart_info(struct nvme_p4_smart_log *smart, int index, __u8 *nm_val, __u8 *raw_val)
static void get_memblaze_new_smart_info(struct nvme_p4_smart_log *smart, int index, __u8 *nm_val,
__u8 *raw_val)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if you could split out white space changes into a separate patch. Mixing janitor updates and new code is making the review harder that it needs to be.

Anyway, when shortening overlong lines, please align the arguments or use two spaces as indent, e.g.

static void get_memblaze_new_smart_info(struct nvme_p4_smart_log *smart, int index, __u8 *nm_val,
					__u8 *raw_val)

or

static void get_memblaze_new_smart_info(struct nvme_p4_smart_log *smart, int index, __u8 *nm_val,
		__u8 *raw_val)

plugins/memblaze/memblaze-nvme.c
@@ -144,35 +145,42 @@ static void show_memblaze_smart_log_new(struct nvme_memblaze_smart_log *s,
"min: ", *(__u16 *)raw, ", max: ", *(__u16 *)(raw+2), ", avg: ", *(__u16 *)(raw+4));

get_memblaze_new_smart_info(smart, RAISIN_SI_VD_E2E_DECTECTION_COUNT, nm, raw);
printf("%-31s: %3d%% %"PRIu64"\n", "end_to_end_error_detection_count", *nm, int48_to_long(raw));
printf("%-31s: %3d%% %"PRIu64"\n", "end_to_end_error_detection_count", *nm,
int48_to_long(raw));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the int48_to_long argument could align with the opening '('.

Obviously, this is also true for all cases below.

plugins/memblaze/memblaze-nvme.c

get_memblaze_new_smart_info(smart, RAISIN_SI_VD_THERMAL_THROTTLE_STATUS, nm, raw);
printf("%-32s: %3d%% %u%%%s%"PRIu64"\n", "thermal_throttle_status", *nm,
*raw, ", cnt: ", int48_to_long(raw+1));
*raw, ", cnt: ", int48_to_long(raw+1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is not needed as the line is already properly align.

plugins/memblaze/memblaze-nvme.c
static int mb_get_additional_smart_log(int argc, char **argv, struct command *cmd, struct plugin *plugin)
static int mb_get_additional_smart_log(
int argc, char **argv,
struct command *cmd, struct plugin *plugin)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indent these two lines with double tap, so the arguments do not align with the single tab indented variable declearations.

plugins/memblaze/memblaze-nvme.c
{
struct nvme_memblaze_smart_log smart_log;
char *desc =
"Get Memblaze vendor specific additional smart log (optionally, for the specified namespace), and show it.";
char *desc = "Get Memblaze additional smart log, and show it.";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overlong strings are okay to be one line. This is an exception but if the shortened help text is also fine. Either way you like it.

plugins/memblaze/memblaze-nvme.c
@@ -667,7 +688,7 @@ struct log_page_high_latency {
static int find_deadbeef(char *buf)
{
if (((*(buf + 0) & 0xff) == 0xef) && ((*(buf + 1) & 0xff) == 0xbe) &&
((*(buf + 2) & 0xff) == 0xad) && ((*(buf + 3) & 0xff) == 0xde))
((*(buf + 2) & 0xff) == 0xad) && ((*(buf + 3) & 0xff) == 0xde))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed

plugins/memblaze/memblaze-nvme.c

// Close device

dev_close(dev);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use the cleanup helper and then you could just remove the dev_close.

        _cleanup_nvme_dev_ struct nvme_dev *dev = NULL;

plugins/memblaze/memblaze-nvme.c
struct command *cmd,
struct plugin *plugin)
{
// Get the configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid non meaningful comments. It's obvious from the code what is happening. This clutters just the code. Some for the rest of the comments in this function.

plugins/memblaze/memblaze-nvme.c

static int mb_get_latency_stats(int argc, char **argv,
struct command *cmd,
struct plugin *plugin)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indention is off.

plugins/memblaze/memblaze-nvme.c
"%s.%03d", str_time_s, time_ms);
printf("%-24s", str_time_ms);

//
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this empty comment

@jinhuahuang2021
Copy link
Contributor Author

The PR need more change, I will update it together.

@igaw
Copy link
Collaborator

igaw commented Mar 7, 2024

Do you still work on this?

@jinhuahuang2021
Copy link
Contributor Author

Yes, we are :)

@igaw
Copy link
Collaborator

igaw commented Mar 8, 2024

Okay, not sure what the update from yesterday means. FWIW, there shouldn't be any merges in the PR. Just rebase your changes to the latest upstream version and update the commit from there. Hope this helps.

@jinhuahuang2021
Copy link
Contributor Author

This PR contains various modifications, which makes it confusing.
I close this PR and split it to small PRs.

1 similar comment
@jinhuahuang2021
Copy link
Contributor Author

This PR contains various modifications, which makes it confusing.
I close this PR and split it to small PRs.

@jinhuahuang2021 jinhuahuang2021 deleted the feature/nvme-performance-stats-v2-1812 branch March 14, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igaw igaw igaw requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jinhuahuang2021 @igaw