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

Backports to Yangtze: jemalloc, PAM #5478

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Backports to Yangtze: jemalloc, PAM #5478

merged 2 commits into from
Feb 29, 2024

Conversation

lindig
Copy link
Contributor

@lindig lindig commented Feb 28, 2024

No description provided.

@edwintorok
CP-45703 jemalloc: avoid bottlenecks with C threads
056d215 
Backport of 490ea9a

Current default jemalloc settings:
```
╭──────────────────────────────────────┬───────────────────────────┬───────────────────────────╮
│name                                  │  minor-allocated          │  monotonic clock/op       │
├──────────────────────────────────────┼───────────────────────────┼───────────────────────────┤
│  concurrent semaphore + sleep fix:1  │            14.4615 mnw/run│          40.7085 ms/op/run│
│  concurrent semaphore + sleep fix:8  │            76.0000 mnw/run│          39.6229 ms/op/run│
╰──────────────────────────────────────┴───────────────────────────┴───────────────────────────╯
```

A flamegraph shows that 'sha512_crypt_r' spends >90% of time in jemalloc and not on hashing.

With 'tcache:true'
```
╭──────────────────────────────────────┬───────────────────────────┬───────────────────────────╮
│name                                  │  minor-allocated          │  monotonic clock/op       │
├──────────────────────────────────────┼───────────────────────────┼───────────────────────────┤
│  concurrent semaphore + sleep fix:1  │            10.1538 mnw/run│           1.6766 ms/op/run│
│  concurrent semaphore + sleep fix:8  │            67.3600 mnw/run│           0.4431 ms/op/run│
╰──────────────────────────────────────┴───────────────────────────┴───────────────────────────╯
```

Without jemalloc:
```
╭──────────────────────────────────────┬───────────────────────────┬───────────────────────────╮
│name                                  │  minor-allocated          │  monotonic clock/op       │
├──────────────────────────────────────┼───────────────────────────┼───────────────────────────┤
│  concurrent semaphore + sleep fix:1  │            10.0488 mnw/run│           1.5649 ms/op/run│
│  concurrent semaphore + sleep fix:8  │            67.1111 mnw/run│           0.3858 ms/op/run│
╰──────────────────────────────────────┴───────────────────────────┴───────────────────────────╯
```

Signed-off-by: Edwin Török <[email protected]>
Signed-off-by: Christian Lindig <[email protected]>
@edwintorok
CP-43755: Pam: avoid sleep(1) call when multithreaded
920f9fe 
Backport of 3b52b72

This enables PAM to be used in multithreaded mode (currently XAPI has a global lock around auth).

Using an off-cpu flamegraph I identified that concurrent PAM calls are slow due to a call to `sleep(1)`.
`pam_authenticate` calls `crypt_r` which calls `NSSLOW_Init` which on first use will try to initialize the just `dlopen`-ed library.
If it encounters a race condition it does a `sleep(1)`. This race condition can be quite reliably reproduced when performing a lot of PAM authentications from multiple threads in parallel.

GDB can also be used to confirm this by putting a breakpoint on `sleep`:
```
  #0  __sleep (seconds=seconds@entry=1) at ../sysdeps/unix/sysv/linux/sleep.c:42
  xapi-project#1  0x00007ffff1548e22 in freebl_RunLoaderOnce () at lowhash_vector.c:122
  xapi-project#2  0x00007ffff1548f31 in freebl_InitVector () at lowhash_vector.c:131
  xapi-project#3  NSSLOW_Init () at lowhash_vector.c:148
  xapi-project#4  0x00007ffff1b8f09a in __sha512_crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=0x7ffff31e17b8 "dIJbsXKc0",
  xapi-project#5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
  xapi-project#6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
  xapi-project#7  0x00007ffff1dc9139 in _unix_verify_password (pamh=pamh@entry=0x7fffd8002910, name=0x7fffd8002ab0 "pamtest-edvint", p=0x7fffd8005a60 "pamtest-edvint", ctrl=ctrl@entry=8389156) at support.c:777
  xapi-project#8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
  xapi-project#9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
  xapi-project#10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
  xapi-project#11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
  xapi-project#12 0x00000000005ae567 in XA_mh_authorize (username=username@entry=0x7fffd80028d0 "pamtest-edvint", password=password@entry=0x7fffd80028f0 "pamtest-edvint", error=error@entry=0x7ffff31e1be8) at xa_auth.c:83
  xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) at xa_auth_stubs.c:42
```

`pam_start` and `pam_end` doesn't help here, because on `pam_end` the library is `dlclose`-ed, so on next `pam_authenticate` it will have to go through the initialization code again.
(This initialization code would've belonged into `pam_start`, not `pam_authenticate`, but there are several layers here including a call to `crypt_r`).
Upstream has fixed this problem >5 years ago by switching to libxcrypt instead.

Signed-off-by: Edwin Török <[email protected]>
Signed-off-by: Christian Lindig <[email protected]>
@lindig lindig requested review from robhoes and edwintorok February 28, 2024 10:51
@edwintorok
Copy link
Contributor

edwintorok commented Feb 28, 2024 •
edited
Loading

I think for PAM there is a second set of changes to backport, this alone won't make a huge difference.

@edwintorok
Copy link
Contributor

edwintorok commented Feb 28, 2024 •
edited
Loading

https://github.com/xapi-project/xen-api/pull/5198/commits (CP-45705) has some commits that could be backported (except the datamodel changes, those must not be backported, but we can e.g. hardcode the thread count to 8)

edwintorok
edwintorok approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

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

The backport looks good though, the rest of the backport could be in a separate PR?

@lindig
Copy link
Contributor Author

lindig commented Feb 28, 2024

The reason I only picked the commit was a comment on CP-45705

@lindig lindig merged commit 8f355ea into xapi-project:1.249-lcm Feb 29, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwintorok edwintorok edwintorok approved these changes

@robhoes robhoes robhoes approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@lindig @edwintorok @robhoes