Skip to content

msg: insert PriorityDispatchers in sorted position#58631

Merged
yuriw merged 1 commit intoceph:mainfrom
cbodley:wip-66336
Aug 1, 2024
Merged

msg: insert PriorityDispatchers in sorted position#58631
yuriw merged 1 commit intoceph:mainfrom
cbodley:wip-66336

Conversation

@cbodley
Copy link
Contributor

@cbodley cbodley commented Jul 16, 2024

avoid calling stable_sort() after every insertion by inserting directly into the sorted position. use lower_bound() to insert at the head and upper_bound() to insert at the tail

this generally only happens during startup so isn't a performance problem, but std::stable_sort() was triggering strange valgrind warnings for "Mismatched free() / delete / delete []" when it allocates a temporary buffer

Fixes: https://tracker.ceph.com/issues/66336

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

avoid calling stable_sort() after every insertion by inserting directly
into the sorted position. use lower_bound() to insert at the head and
upper_bound() to insert at the tail

this generally only happens during startup so isn't a performance
problem, but std::stable_sort() was triggering strange valgrind warnings
for "Mismatched free() / delete / delete []" when it allocates a
temporary buffer

Fixes: https://tracker.ceph.com/issues/66336

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@github-actions github-actions bot added the core label Jul 16, 2024
@cbodley cbodley marked this pull request as ready for review July 16, 2024 22:18
@cbodley cbodley requested a review from a team as a code owner July 16, 2024 22:18
@cbodley cbodley requested a review from a team July 16, 2024 22:24
@cbodley
Copy link
Contributor Author

cbodley commented Jul 17, 2024

jenkins test make check

@cbodley
Copy link
Contributor Author

cbodley commented Jul 17, 2024

this resolves the valgrind MismatchedFree issues in the rgw suite: https://pulpito.ceph.com/cbodley-2024-07-17_02:14:05-rgw-wip-66336-distro-default-smithi/

@vshankar
Copy link
Contributor

this resolves the valgrind MismatchedFree issues in the rgw suite: https://pulpito.ceph.com/cbodley-2024-07-17_02:14:05-rgw-wip-66336-distro-default-smithi/

I'll run this through fs suite and report. We have been seeing similar valgrind warnings.

@vshankar
Copy link
Contributor

This PR is under test in https://tracker.ceph.com/issues/67020.

@cbodley
Copy link
Contributor Author

cbodley commented Jul 23, 2024

jenkins test make check

Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

Much better, thanks Casey!

bool first = dispatchers.empty();
dispatchers.push_back(PriorityDispatcher{priority, d});
std::stable_sort(dispatchers.begin(), dispatchers.end());
const PriorityDispatcher entry{priority, d};
Copy link
Member

Choose a reason for hiding this comment

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

has the valgrind issue spooked you into using const here to avoid a move? 😆

@batrick
Copy link
Member

batrick commented Jul 23, 2024

jenkins test make check

@batrick
Copy link
Member

batrick commented Jul 23, 2024

jenkins test make check arm64

@batrick
Copy link
Member

batrick commented Jul 23, 2024

(Adding this to a future QA run in case it isn't merged by then. Don't wait for me.)

Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

LGTM

@batrick
Copy link
Member

batrick commented Jul 25, 2024

jenkins test make check arm64

@batrick batrick added the messenger Issues involving one of the Ceph messenger implementations label Jul 25, 2024
@batrick
Copy link
Member

batrick commented Jul 26, 2024

jenkins test make check arm64

@batrick
Copy link
Member

batrick commented Jul 26, 2024

This PR is under test in https://tracker.ceph.com/issues/67214.

@NitzanMordhai
Copy link
Contributor

@yuriw yuriw merged commit 60cd2bc into ceph:main Aug 1, 2024
@cbodley
Copy link
Contributor Author

cbodley commented Aug 1, 2024

i don't think cephfs had finished testing this yet. hopefully it doesn't break anything there?

@vshankar
Copy link
Contributor

vshankar commented Aug 5, 2024

i don't think cephfs had finished testing this yet. hopefully it doesn't break anything there?

Noticed nothing unusual in my run, so merging this was all good 👍

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.

6 participants