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

[system] Collect glibc tuning decisions #2812

Merged
merged 1 commit into from Jan 5, 2022

Conversation

pmoravec
Copy link
Contributor

Resolves: #2812

Signed-off-by: Pavel Moravec pmoravec@redhat.com


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname email@example.com?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-2812
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@pmoravec
Copy link
Contributor Author

I am not sure if we should stick to the hardcoded paths or not. ld.so is shipped exactly under /usr/bin on Red Hat distros, where this path is quite primary choice to look at among PATHs in sos (https://github.com/sosreport/sos/blob/main/sos/policies/distros/redhat.py#L75-L77). Original request in https://bugzilla.redhat.com/show_bug.cgi?id=2032913#c2 isn't much conclusive if the path does matter or not - but I feel it is improbable one to have a system with ld.so binary different than the /usr/bin/ld.so.

I havent found a different distro shipping this fairly new binary, yet. Maybe @BryanQuigley could comment wrt Debian/Ubuntu?

Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

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

Since this is in a canonical location, I think dropping the absolute path is the right move here. Let Policy handle PATH like we do for typical collections.

@@ -33,5 +33,11 @@ def setup(self):
"/proc/sys/net/ipv6/neigh/*/base_reachable_time"
])

# collect glibc tuning decisions
self.add_cmd_output([
"/usr/bin/ld.so --help",
Copy link
Member

Choose a reason for hiding this comment

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

Collecting --help output seems strange. Is there a particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same question to the original reporter, quoting his answer:

--help output contains some diagnostic output, too, perhaps in a more digestable form. It shows the search paths being used.

# collect glibc tuning decisions
self.add_cmd_output([
"/usr/bin/ld.so --help",
"/usr/bin/ld.so --list-diagnostics"
Copy link
Member

Choose a reason for hiding this comment

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

Should we also be collecting ld.so --list-tunables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing like that has been asked for.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably clarify - the commit and comment both reference tuning, but doesn't collect the only thing in the help output that references tuning:

  --list-tunables       list all tunables with minimum and maximum values

Choose a reason for hiding this comment

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

I wouldn't mind adding that, too, but the minimums/maximums currently aren't dynamic, so they won't confer additional system-specific information.

Cc @siddhesh

Copy link
Member

Choose a reason for hiding this comment

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

mins/maxes might not, but surely the actual value would?

# ld.so --list-tunables
glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10)
glibc.elision.skip_lock_after_retries: 3 (min: 0, max: 2147483647)
glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0xffffffffffffffff)

Copy link

Choose a reason for hiding this comment

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

I wouldn't mind adding that, too, but the minimums/maximums currently aren't dynamic, so they won't confer additional system-specific information.

mins/maxes might not, but surely the actual value would?

The x86-specific tunables (x86_data_cache_size for example) will depend on the target CPU since it gets the default value from _SC_LEVEL1_DCACHE_SIZE, so it could return different values on different machines. glibc.malloc.trim_threshold though will just return 0 if not set since the default is dynamic, i.e. it starts at 128K and the malloc implementation changes it at runtime. A single value is misleading since it implies that it is the single default value. Many tunables however should have a constant default value.

So in summary, "it depends".

Choose a reason for hiding this comment

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

Okay, then let's include it, too.

@BryanQuigley
Copy link
Member

I don't see that binary packaged yet in debian. Regardless I don't see the reason to include the /usr/bin/...

I also don't see any reason that Debian/Ubuntu would pick a different path for it in 2022..

pmoravec added a commit to pmoravec/sos that referenced this pull request Jan 3, 2022
Resolves: sosreport#2812

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
Resolves: sosreport#2812

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec
Copy link
Contributor Author

pmoravec commented Jan 5, 2022

Thanks for the feedback (and hi @siddhesh ;-) ), ld.so --list-tunables was added.

@TurboTurtle TurboTurtle merged commit ddf9eaa into sosreport:main Jan 5, 2022
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.

None yet

5 participants