Skip to content

crimson/net: set TCP_NODELAY according to ms_tcp_nodelay#52592

Merged
Matan-B merged 1 commit intoceph:mainfrom
xxhdx1985126:wip-62098
Jul 25, 2023
Merged

crimson/net: set TCP_NODELAY according to ms_tcp_nodelay#52592
Matan-B merged 1 commit intoceph:mainfrom
xxhdx1985126:wip-62098

Conversation

@xxhdx1985126
Copy link
Member

@xxhdx1985126 xxhdx1985126 commented Jul 23, 2023 •
edited
Loading

Fixes: https://tracker.ceph.com/issues/62098
Signed-off-by: Xuehan Xu xuxuehan@qianxin.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
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

@xxhdx1985126 xxhdx1985126 requested a review from cyx1231st July 23, 2023 09:35
@xxhdx1985126 xxhdx1985126 requested a review from a team as a code owner July 23, 2023 09:35
@tchaikov
Copy link
Contributor

tchaikov commented Jul 23, 2023 •
edited
Loading

@xxhdx1985126 Hi Xuehan, thank you for rootcausing the latency issue. But, IMHO, disabling the Nagel’s algorithm is not a pure win. We have shorter latency in expense of sending more smaller segments. Have you tried to reduce tcp_delack_min?

Also, the commit message could use some more explanation on the reasoning. Performance issue is a complicated thing.

@xxhdx1985126
Copy link
Member Author

xxhdx1985126 commented Jul 23, 2023

@xxhdx1985126 Hi Xuehan, thank you for rootcausing the latency issue. But, IMHO, disabling the Nagel’s algorithm is not a pure win. We have shorter latency in expense of sending more smaller segments. Have you tried to reduce tcp_delack_min?

Also, the commit message could use some more explanation on the reasoning. Performance issue is a complicated thing.

Will try:-)

@xxhdx1985126
Copy link
Member Author

xxhdx1985126 commented Jul 24, 2023 •
edited
Loading

@xxhdx1985126 Hi Xuehan, thank you for rootcausing the latency issue. But, IMHO, disabling the Nagel’s algorithm is not a pure win. We have shorter latency in expense of sending more smaller segments. Have you tried to reduce tcp_delack_min?

@tchaikov Hi, kefu, it seems that the kernel doesn't have the "TCP_DELACK_MIN" feature enabled by default when compiling.

Also, the commit message could use some more explanation on the reasoning. Performance issue is a complicated thing.

I've added explanation in the commit message, please take a look again:-)

@xxhdx1985126
crimson/net: set TCP_NODELAY according to ms_tcp_nodelay
e21cfa7 
There are cases in which we need low latency of an individual op, like
updating RBD image's object map which would block other ops for a
object not created yet. We need to make sure the rbd-object-map-update
like ops have as low latency as possible

Fixes: https://tracker.ceph.com/issues/62098
Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
cyx1231st
cyx1231st approved these changes Jul 24, 2023
View reviewed changes
Copy link
Member

@cyx1231st cyx1231st left a comment

Choose a reason for hiding this comment

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

Looks another possible option is to set TCP_QUICKACK without setting TCP_NODELAY. This will keep small packets buffering.

LGTM to integrate the option.

@xxhdx1985126
Copy link
Member Author

xxhdx1985126 commented Jul 24, 2023

Looks another possible option is to set TCP_QUICKACK without setting TCP_NODELAY. This will keep small packets buffering.

LGTM to integrate the option.

Just did a test, it seems that TCP_QUICKACK didn't help, trying to see why.

@athanatos athanatos self-requested a review July 24, 2023 05:32
@tchaikov
Copy link
Contributor

tchaikov commented Jul 24, 2023 •
edited
Loading

@xxhdx1985126 Hi Xuehan, thank you for rootcausing the latency issue. But, IMHO, disabling the Nagel’s algorithm is not a pure win. We have shorter latency in expense of sending more smaller segments. Have you tried to reduce tcp_delack_min?

@tchaikov Hi, kefu, it seems that the kernel doesn't have the "TCP_DELACK_MIN" feature enabled by default when compiling.

thank you. seems it is only available on some linux branches, and the change which makes TCP_DELACK_MIN a knob has not been included by any of linux upstream branches. i spotted it on https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux_for_real_time/7/html/tuning_guide/reducing_the_tcp_delayed_ack_timeout . guess RHEL for RT applied this patch.

Also, the commit message could use some more explanation on the reasoning. Performance issue is a complicated thing.

I've added explanation in the commit message, please take a look again:-)

thanks. could you note in the commit message that this change mirrors the default setting in the async messenger ? see

ceph/src/msg/async/Stack.h

Line 43 in 8c4d93c

bool nodelay = true;

EDIT, we just respect the setting in this change. we are not changing the default setting at all -- it is true by default already.

@cyx1231st
Copy link
Member

cyx1231st commented Jul 25, 2023

@athanatos @Matan-B This looks to be a minor change, can we merge it directly, or batch with other ready crimson PRs for convenience (is there a label for this case) ?

@Matan-B
Copy link
Contributor

Matan-B commented Jul 25, 2023

@athanatos @Matan-B This looks to be a minor change, can we merge it directly, or batch with other ready crimson PRs for convenience (is there a label for this case) ?

I already scheduled a test run with this PR. Generally it looks good to me. I spotted the one failure which to me appears unrelated (AFAIK, we already experienced this issue). See https://tracker.ceph.com/issues/62162
@cyx1231st, if you agree this is unrelated as well, I'm ok with merging.

https://pulpito.ceph.com/matan-2023-07-25_10:55:34-crimson-rados-wip-matanb-crimson-only-wip-62098-distro-crimson-smithi/

btw, (not a reason to block this PR) but can the change be re-written to socket.set_nodelay(local_conf()->ms_tcp_nodelay);?

@cyx1231st
Copy link
Member

cyx1231st commented Jul 25, 2023

See https://tracker.ceph.com/issues/62162

Agree, looks unrelated.

@Matan-B Matan-B merged commit 86b36a1 into ceph:main Jul 25, 2023
@Matan-B Matan-B mentioned this pull request Oct 17, 2023
Merged
@xxhdx1985126 xxhdx1985126 deleted the wip-62098 branch April 23, 2024 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaikov tchaikov tchaikov approved these changes

@athanatos athanatos athanatos approved these changes

@cyx1231st cyx1231st cyx1231st approved these changes

Assignees

No one assigned

Labels

crimson

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xxhdx1985126 @tchaikov @cyx1231st @Matan-B @athanatos