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

Fix some issues causing errors in TiKV on my machine #1

Closed
wants to merge 2 commits into from

Conversation

nrc
Copy link

@nrc nrc commented Jan 22, 2020

I'm running an AMD machine with Linux kernel version 5.3. This causes two issues:

  • The stat entries in /sys/block/ have 15 columns instead of 11. This is a backwards compatible change, so we can just ignore the extra four columns.
  • The layout of data returned by the cpuid instruction is completely different. The cache-size crate is based on the rust-cpuid crate, which does not yet support AMD. In order to distinguish this state I need to know the vendor_id of the CPU.

These are addressed in the first and second commits, respectively.

PTAL @lonng

@nrc nrc requested a review from lonng January 22, 2020 06:03
@GuillaumeGomez
Copy link

Interesting, sysinfo doesn't provide such information. Do you mind if I take your code? I won't put keep them as standalone functions but put them into the ProcessorExt trait instead.

Nick Cameron added 2 commits January 22, 2020 18:51
@siddontang
Copy link

can we add test for different kernel in CI?

@zhouqiang-cl @nrc

@nrc
Copy link
Author

nrc commented Jan 26, 2020

can we add test for different kernel in CI?

I think we can, but it would increase resource use by a large amount. We should probably be testing on CentOS 8 as well as 7 + most recent and most recent LTE Ubuntu. It think with those distros we'll cover all kernel versions that are important. However, if we run the full test suite, etc. that is a 4x increase in resources.

@@ -277,6 +277,19 @@ pub fn get_cpu_frequency() -> u64 {
.unwrap_or_default()
}

/// Returns the brand/vendor string for the first CPU (which should be the same for all CPUs).
pub fn get_vendor_id() -> String {

Choose a reason for hiding this comment

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

do we have a test to cover it in CI?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet.

Copy link
Member

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@GuillaumeGomez
Copy link

BTW, I released a new version of sysinfo with new functionalites (such as "get_vendor_id"). You might want to take a look.

@lonng
Copy link
Member

lonng commented Feb 12, 2020

@GuillaumeGomez Thanks, @nrc, it seems this PR does not necessary anymore.

@nrc nrc closed this Feb 13, 2020
LykxSassinator pushed a commit that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants