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

pdu: Allow PDU to Tagged Stream to write partial PDUs #7028

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

Conversation

argilo
Copy link
Member

@argilo argilo commented Dec 23, 2023

Description

The tagged_stream_block class is a wrapper to help make Tagged Stream blocks. Unfortunately, it causes problems for the scheduler by refusing to produce any output unless the input and output buffers are long enough to contain an entire PDU:

if (d_n_input_items_reqd[i] > ninput_items[i]) {
return 0;
}

int min_output_size = calculate_output_stream_length(d_n_input_items_reqd);
if (noutput_items < min_output_size) {
set_min_noutput_items(min_output_size);
return 0;
}

If the scheduler observes that a block refuses to produce output and all of its upstream blocks are finished (i.e. won't produce any more output), then it concludes that the block is finished and terminates its worker thread.

For the PDU to Tagged Stream block in particular, this is problematic because the scheduler can terminate it while it still has PDUs waiting in its input queue. This causes test failures in the qa_fecapi_* tests, as noted in #6628. Failures can readily be reproduced by increasing rep to 7 in this test:

Prior to #323, the PDU to Tagged Stream block did not have this problematic behaviour; if it had PDUs in its input queue, it would always produce output. If there wasn't enough room to write out a full PDU, it would store the rest for later. Here I've rewritten the block to behave as it did before.

Related Issue

Fixes #6628.
Reverts #323.

Which blocks/areas does this affect?

  • PDU to Tagged Stream

Testing Done

I verified that all tests that use the PDU to Tagged Stream block still pass, and that the qa_fecapi_repetition test passes even if rep or frame_size are increased.

Checklist

@argilo argilo added pdu PDU-related (i.e. about packetized data in messages) Bug Fix Backport-3.10 Should be backported to 3.10 labels Dec 23, 2023
Signed-off-by: Clayton Smith <argilo@gmail.com>
@argilo
Copy link
Member Author

argilo commented Dec 23, 2023

the qa_fecapi_repetition test passes even if rep or frame_size are increased

Note: Increasing them too much can still cause trouble for another block (Tagged Stream to PDU):

block_executor :error: sched: <block tagged_stream_to_pdu (9)> is requesting more input data than we can provide.

This block also inherits the problematic behaviours of tagged_stream_block.

@argilo
Copy link
Member Author

argilo commented Dec 23, 2023

I suppose a more comprehensive solution (which would fix all Tagged Stream blocks) might be to instead implement input & output PDU buffering in the tagged_stream_block class. There would be a bit more risk of performance issues in making a change that affects all Tagged Stream blocks. Let me know what you think.

@argilo
Copy link
Member Author

argilo commented Dec 29, 2023

I'll put this into draft while I explore the other option (adding buffering to tagged_stream_block) which I suspect may also solve the problem of Tagged Stream blocks being incompatible with custom buffers.

@argilo argilo marked this pull request as draft December 29, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport-3.10 Should be backported to 3.10 Bug Fix pdu PDU-related (i.e. about packetized data in messages)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDU to Tagged Stream confuses the scheduler by failing to produce output
1 participant