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

SILKIT-1608: disable nagle algorithm by default #181

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

SILKIT-1608: disable nagle algorithm by default #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion SilKit/source/config/ParticipantConfiguration.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ struct Middleware
int connectAttempts{1}; //!< Number of connection attempts to the registry a participant should perform.
int tcpReceiveBufferSize{-1};
int tcpSendBufferSize{-1};
bool tcpNoDelay{false}; //!< Disables Nagle's algorithm.
bool tcpNoDelay{true}; //!< Nagle's algorithm disabled by default.
Copy link
Member

Choose a reason for hiding this comment

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

I think changing the default value is a good idea. However, this will 'surprise' everyone who hasn't explicitly set this configuration value to false. We should carefully evaluate if this could be an issue. We should also think about how this should influence our version number - or the configuration SchemaVersion (my gut reaction is to increment the SchemaVersion, but treat this specifically as a patch for SIL Kit).

Copy link
Member

Choose a reason for hiding this comment

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

However, we have no prescedent for incrementing the SchemaVersion like this. And we also need some code to actually handle varying defaults (e.g.) in different SchemaVersions.

Copy link
Member

Choose a reason for hiding this comment

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

The main issue I see in changing the default value, is for users that have configurations where they only set TcpQuickAck: true but do not specify TcpNoDelay (because they relied on the old default).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, users who did not explicitly set the TCP middleware options may experience different behaviour.
However, we only change the default config value, not the code itself. Does that justify a version increment?
Maybe, a short remark in the release notes is already sufficient?

bool tcpQuickAck{false}; //!< Setting this Linux specific flag disables delayed TCP/IP acknowledgements.
bool enableDomainSockets{true}; //!< By default local domain socket is preferred to TCP/IP sockets.
std::vector<std::string>
Expand Down
4 changes: 2 additions & 2 deletions docs/configuration/middleware-configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ running on 'localhost' listening on port 8500. These values can be changed via t
Middleware:
RegistryUri: silkit://localhost:8500
ConnectAttempts: 1
TcpNoDelay: false
TcpNoDelay: true
TcpQuickAck: false
TcpSendBufferSize: 1024
TcpReceiveBufferSize: 1024
Expand All @@ -57,7 +57,7 @@ running on 'localhost' listening on port 8500. These values can be changed via t
By default, only a single connect is attempted.

* - TcpNoDelay
- Enable the TCP_NODELAY flag on TCP sockets. This disables Nagle's algorithm.
- Disable the TCP_NODELAY flag on TCP sockets. This enables Nagle's algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

The old formulation is correct! TcpNoDelay: true means to enable the TCP_NODELAY flag, which disables Nagle.

auto MakeAsioSocketOptionsFromConfiguration(const SilKit::Config::ParticipantConfiguration& participantConfiguration) -> SilKit::Core::AsioSocketOptions
{
    SilKit::Core::AsioSocketOptions socketOptions{};
    // ...
    socketOptions.tcp.noDelay = participantConfiguration.middleware.tcpNoDelay;
    // ...
}
if (socketOptions.tcp.noDelay)
{
    socket.set_option(asio::ip::tcp::no_delay{true}, errorCode);
    // ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes!


* - TcpQuickAck
- Enable the TCP_QUICKACK flag on TCP sockets (Linux only). Disables delayed
Expand Down
2 changes: 2 additions & 0 deletions docs/faq/faq.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ Finally, the hardware being used, the network infrastructure and the distributio

To get a first impression of the performance that can be expected of the |ProductName| in your use case, refer to the :ref:`Benchmark Demo<sec:benchmark-demo>` where you can specify a set of relevant parameters and get real performance measurements for your setup.

You can find further information on performance aspects in the :doc:`troubleshooting<../troubleshooting/performance>` section.

Can I control the order in which simulation participants execute their simulation steps, in case they occur at the same time?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
21 changes: 21 additions & 0 deletions docs/troubleshooting/performance.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.. include:: /substitutions.rst

Performance
=========

The following sections provide support for improving the performance of |ProductName|.

Configuration of TCP Stack (Kernel)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment on lines +8 to +9
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Configuration of TCP Stack (Kernel)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
TCP Socket Configuration
~~~~~~~~~~~~~~~~~~~~~~~~


The TCP stack of the kernel can be configured via the participant configuration file, more precisely the :doc:`middleware<../configuration/middleware-configuration>` options.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The TCP stack of the kernel can be configured via the participant configuration file, more precisely the :doc:`middleware<../configuration/middleware-configuration>` options.
The TCP sockets used by SIL Kit can be configured via the participant configuration file, more precisely the :doc:`middleware<../configuration/middleware-configuration>` options.

This configuration needs to be done carefully. In particular, Nagle's algorithm (``TcpNoDelay``) and delayed acknowledgements (``TcpQuickAck``) are known to interact badly.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This configuration needs to be done carefully. In particular, Nagle's algorithm (``TcpNoDelay``) and delayed acknowledgements (``TcpQuickAck``) are known to interact badly.
This configuration needs to be done carefully. In particular, using Nagle's algorithm (``TcpNoDelay: false``) and delayed acknowledgements (``TcpQuickAck: false``) are known to interact badly.

I would add which configuration values interact badly.

It's also only a 'bad' interaction, if latency is a major concern - which is the case for us, but, e.g., not for bulk transfers (downloads). Nagle and delayed ACKs exist to use bandwidth efficiently, i.e., to use the local buffers as much as possible and only transmit when enough data for 'full' packets is queued and enough space is available for reception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, adding the values is really helpful!

A short comment on the 'bad' interaction: It is not only a concern when it comes to latency. I observed problematic behaviour in the following situations:

  • Latency peaks (40ms): To be reproduced with LatencyDemo (with small message size)
  • Large throughput jitter (standard deviation decreases by one order of magnitude, if Nagle is disabled): Reproduce with BenchmarkDemo (100kB, 10 messages/sim. step)
  • Stuck communication: Reproduce with BenchmarkDemo (1 byte, 1 msg/sim. step)

Per default, Nagle's algorithm is disabled and delayed acknowledgements are enabled. Note that enabling both features may cause severe performance losses as well as non-deterministic behaviour.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Per default, Nagle's algorithm is disabled and delayed acknowledgements are enabled. Note that enabling both features may cause severe performance losses as well as non-deterministic behaviour.
Please note that version X.Y.Z and following will use the ``NO_DELAY`` flag (disable Nagle's algorithm) by default (``TcpNoDelay: true``).
Using ``TcpNoDelay: true`` and ``TcpQuickAck: true`` together, may cause severe performance losses as well as non-deterministic behaviour.

I would probably reword this to get away from using 'enabled' and 'disabled' so much - it at least confused me a fair bit 😹

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excellent hint! I was getting confused as well 😁

Latency peaks of 40 ms and a large throughput variance have been observed in this case so far.

Message Aggregation
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~


For the time-synchronized case, |ProductName| offers the possibility of aggregating messages prior to sending.
This feature can be enabled via the :doc:`EnableMessageAggregation<../configuration/experimental-configuration>` parameter in the participant configuration file.
We mention that the message aggregation feature has a significant impact on the throughput, in particular if several small messages (<1kB) are sent within one simulation step.
2 changes: 2 additions & 0 deletions docs/troubleshooting/troubleshooting.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@ Most issues can be identified based on log messages by the application.
lifecycle.rst
interoperability.rst
connection-guides.rst
performance.rst

..
.. include:: connectivity.rst
.. include:: lifecycle.rst
.. include:: interoperability.rst
.. include:: connection-guides.rst
.. include:: performance.rst
Loading