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.

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

Implement Mhz to current, min and max. #1517

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexwbaule
Copy link

Implement of #1516

Please, is my first PR here, and i only have Linux to test it.

@shirou shirou added the v4 label Aug 29, 2023
@shirou
Copy link
Owner

shirou commented Aug 29, 2023

Thank you for your contribution. However, unfortunately, this PR breaks current compatibility. So we cannot accept while in current major version, v3. I have added v4 label, but v4 release date is not discussed yet.

@alexwbaule
Copy link
Author

Hi @shirou , can we merge this ?

@shirou
Copy link
Owner

shirou commented Oct 14, 2023

As I commented, this PR breaks current compatibility. So we cannot merge until v4 is released.

@kkartaltepe
Copy link

kkartaltepe commented Nov 19, 2024

It seems the current version is v4 now can this be considered again? I was also interested in this data.

--- Edit

And if it does get picked back up a fallback to scaling_cur_freq should be added to support amd-pstate's governor which only reports frequency from there https://docs.kernel.org/admin-guide/pm/amd-pstate.html#key-governors-support (I don't know which is more correct on other platforms/configurations but amd-pstate doesnt report cpuinfo_cur_freq at all).

@alexwbaule
Copy link
Author

@shirou , need something to merge ?

@renard
Copy link

renard commented Jan 20, 2025

@alexwbaule I +1 this feature because it eases the ability to figure out any glitches during a benchmark. However I'd recommend to keep a flat structure such as:

MhzCurrent float64 `json:"mhz_current"`
MhzMin float64 `json:"mhz_min"`
MhzMax float64 `json:"mhz_max"`

instead of using a MHz structure.

So far no other collected metric use any sub-structure. It would be better IMHO to keep it that way.

Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

It's a breaking change but why not

CacheSize int32 `json:"cacheSize"`
Flags []string `json:"flags"`
Microcode string `json:"microcode"`
}

type Mhz struct {

Choose a reason for hiding this comment

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

I would use MHz

And while you are changing the type and so the output format, I would change the Mhz variable name in the struct

But here,I'm only a random Gopher.

The maintainers of this project might have another opinion

Comment on lines +106 to 109
c.Mhz.Current = float64(u32)
c.Mhz.Min = 0
c.Mhz.Max = 0

Choose a reason for hiding this comment

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

At some point if it's for test purpose that you are resetting it

Comment on lines +106 to +108
c.Mhz.Current = float64(u32)
c.Mhz.Min = 0
c.Mhz.Max = 0

Choose a reason for hiding this comment

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

Please consider writing this

Suggested change
c.Mhz.Current = float64(u32)
c.Mhz.Min = 0
c.Mhz.Max = 0
c.Mhz = Mhz{Current: float64(u32)}

Abd everywhere you did the replacement

Comment on lines +144 to +146
c.Mhz.Current = fillMhz(ctx, "cur", c)
c.Mhz.Min = fillMhz(ctx, "min", c)
c.Mhz.Max = fillMhz(ctx, "max", c)

Choose a reason for hiding this comment

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

I would pass

Suggested change
c.Mhz.Current = fillMhz(ctx, "cur", c)
c.Mhz.Min = fillMhz(ctx, "min", c)
c.Mhz.Max = fillMhz(ctx, "max", c)
c.Mhz.Current = fillMhz(ctx, "cpuinfo_cur_freq", c)
c.Mhz.Min = fillMhz(ctx, "cpuinfo_min_freq", c)
c.Mhz.Max = fillMhz(ctx, "cpuinfo_max_freq", c)

And then I would use

Suggested change
c.Mhz.Current = fillMhz(ctx, "cur", c)
c.Mhz.Min = fillMhz(ctx, "min", c)
c.Mhz.Max = fillMhz(ctx, "max", c)
c.Mhz = Mhz{Current: fillMhz(ctx, "cpuinfo_cur_freq", c), Min: fillMhz(ctx, "cpuinfo_min_freq", c), Max: fillMhz(ctx, "cpuinfo_max_freq", c)}

var line float64
var mhz float64 = 0

if value == "min" || value == "max" || value == "cur" {

Choose a reason for hiding this comment

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

Add a switch here and return 0 if the value are not the one you expect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants