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

Set timestamping socket options in individual socket implementations instead of ntcs::Compat in order to detect failure #115

Merged
merged 5 commits into from
Feb 3, 2024

Conversation

mattrm456
Copy link
Contributor

The current implementation sets timestamping options in ntcs::Compat::configure immediately after the socket file descriptor is created. On some Linux platforms that do not support our requested timestamping configuration, setting this socket option fails with an error other than "not supported". The current implementation fails ntcs::Compat::configure in this case which causes ntcr::StreamSocket to fail to open.

This PR moves the location where we set the timestamping options from ntcs::Compat::configure to ntcr::StreamSocket::privateOpen and ntcr::DatagramSocket::privateOpen. In this location we now detect that setting the timestamping option failed, specifically, and we proceed with timestamping disabled.

…instead of ntcs::Compat in order to detect failure

error = streamSocket->setOption(option);
if (error) {
BSLS_LOG_DEBUG("Failed to set socket option: "
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use NTCI_LOG_DEBUG everywhere instead of BSLS_LOG_DEBUG?
And maybe warning would suit better for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but I'm keeping the DEBUG severity level. WARN is too loud for a requested-but-unavailable feature, not required for operation.

static bool supportsTimestamps();
/// process supports asynchronous socket notifications via some mechanism
/// such as the Linux error queue.
static bool supportsNotifications();
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds sort of strange when it is at nts level. Concept of notifications was introduced on Reactor level, which is NTC.

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 have removed ntscfg::Platform::supportsTimestamps in favor of a new function, ntsu::SocketOptionUtil::supportsTimestamps(socket). We need low-level support for platform capabilities, because even if the high-level interface is in NTC the low-level mechanics are requested from NTS.


// Timestamp reporting.
e_SOF_TIMESTAMPING_SOFTWARE = (1 << 4),
e_SOF_TIMESTAMPING_SYS_HARDWARE = (1 << 5),
Copy link
Contributor

Choose a reason for hiding this comment

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

SOF_TIMESTAMPING_SYS_HARDWARE flag is deprecated afaik, and I suggest not to use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


{ ntsu::TimestampUtil::e_SOF_TIMESTAMPING_TX_HARDWARE, 0, 0, 0 },
{ ntsu::TimestampUtil::e_SOF_TIMESTAMPING_TX_SOFTWARE, 0, 0, 0 },
{ ntsu::TimestampUtil::e_SOF_TIMESTAMPING_TX_SCHED, 3, 17, 0 },
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you wanted to make a cut at TX timestamping usage being supported only starting from 4.18. Then it's not clear why you want to have kernel 3.17 mentioned here.

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'm not sure we need this facility; currently its preprocessed out. The idea was to detect the run-time kernel version and clear out the bits for unsupported flags. My testing shows that this isn't needed. I'm a bit paranoid about what we may find in the future, though, so I'd like to leave this system in even though it is preprocessed out of the code. In the meantime, I've stated the minimum necessary kernel version for all timestamping features is 4.18 (RHEL 8).

If we find this system is necessary in the future, we'll go an exactly specify when each of the these flags were introduced. If we don't find this system necessary after some time has passed let's just remove it.


e_SOF_TIMESTAMPING_TX_GENERATION =
e_SOF_TIMESTAMPING_TX_HARDWARE |
Copy link
Contributor

@smtrfnv smtrfnv Jan 30, 2024

Choose a reason for hiding this comment

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

We've never exercised tx harware timestamps. And I am inclined to think that it will lead to logical errors regarding how tx timestamp notifications are processed. I think that if HW TX timestamps are supported, then the way we request them we will receive both SF and HW TX timestamps in diferent messages. Probably they all will have the same type (like TimestampUtil::e_SCM_TSTAMP_SND). And the we are cursed in ntcr::StreamSocket

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'm trying to find some machine where I can test hardware TX timestamping. I'd like to leave this as-is until we verify that our correlation implementation needs enhancement to accomodate hardware TX timestamps.

…im support for all timestamping options begins with Linux 4.18
ntsa::Error privateZeroCopyEngage(
const bsl::shared_ptr<DatagramSocket>& self,
bsl::size_t threshold);

// Process the completion of one or more zero-copy tranmsissions described
Copy link
Contributor

Choose a reason for hiding this comment

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

tranmsissions -> transmissions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ntsa::Error privateZeroCopyEngage(
const bsl::shared_ptr<StreamSocket>& self,
bsl::size_t threshold);

// Process the completion of one or more zero-copy tranmsissions described
Copy link
Contributor

Choose a reason for hiding this comment

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

tranmsissions -> transmissions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@smtrfnv smtrfnv left a comment

Choose a reason for hiding this comment

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

Ok to merge

@mattrm456 mattrm456 merged commit 5a04483 into bloomberg:main Feb 3, 2024
1 check passed
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.

2 participants