libsubprocess: support flow control on writes via credits #6353

chu11 · 2024-10-08T05:35:07Z

Per flux-framework/rfc#427 and discussion in #4572.

Some minor tweaks to the add-credit RPC. I think the design in the RFC assumed multiple channels would return credits at the same time, but I think that is impractical, so it's just credits for each channel.

Haven't tested that this works by adding support in flux-exec yet. Just thought I'd post this initial work. May split off the cleanup into another PR.

garlick · 2024-10-08T14:50:06Z

I think the design in the RFC assumed multiple channels would return credits at the same time, but I think that is impractical, so it's just credits for each channel.

In that PR I proposed that an add-credit response payload would be

{
  "type":"add-credit",
  "channels":{
    "stdin":42
  }
}

while I think you are using

{
  "type":"add-credit",
  "channel":{
    "stream":"stdin",
    "bytes":42
  }
}

The former is fewer bytes on the wire and leaves the door open to combine messages for multiple streams, should that be useful. What issue did you run into?

BTW the main use case I was thinking of for combining messages was at initialization (server sends {"channel":BUFSIZE} for all input buffers).

chu11 · 2024-10-08T15:18:22Z

BTW the main use case I was thinking of for combining messages was at initialization (server sends {"channel":BUFSIZE} for all input buffers).

Lemme see if I can refactor it that way.

chu11 · 2024-10-08T22:07:13Z

BTW the main use case I was thinking of for combining messages was at initialization (server sends {"channel":BUFSIZE} for all input buffers).

Ahhh tried to refactor and realized why it initially didn't work out. The on_credit() callback is

typedef void (*flux_subprocess_credit_f) (flux_subprocess_t *p,
                                          const char *stream,
                                          int bytes);

i.e. only one stream and "bytes" is reported at a time. For remote subprocesses I just report the initial credits that the local subprocess reports.

Could have a special case flag where local subprocess doesn't report initial credits and remote subprocess could "collate" them in one RPC instead?

garlick · 2024-10-08T22:36:45Z

It's a bit unfortunate if we have to wait for the reactor loop to tell us the initial buffer size before sending it to the remote user since we should already know what it is and that adds latency.

Still, is there a reason not to support multiple streams in the protocol even if this server doesn't use it yet? There are other subprocess servers (sdexec).

chu11 · 2024-10-09T00:01:40Z

It's a bit unfortunate if we have to wait for the reactor loop to tell us the initial buffer size before sending it to the remote user since we should already know what it is and that adds latency.

Perhaps some special casing should be done here. Let me think about it.

Still, is there a reason not to support multiple streams in the protocol even if this server doesn't use it yet? There are other subprocess servers (sdexec).

Fair enough.

chu11 · 2024-10-09T20:59:24Z

added tweaks to flux-exec that appear to handle the reproducer from #4572, (cat /tmp/achu/100mfile | flux exec -r 0 sed -n 'w /tmp/achu/foo.out') hooray. Gotta work on the edges of all this though (including comments from above).

garlick · 2024-10-09T22:15:20Z

Ooh nice, does it work for an even smaller buffer like 4K?

chu11 · 2024-10-09T22:51:07Z

Ooh nice, does it work for an even smaller buffer like 4K?

Yup, seems to work!

chu11 · 2024-10-11T23:08:40Z

re-pushed, fixing up a lot of things based on comments above. Most notably, the remote subprocess sends out a single "initial credits" RPC back to the client for all channels. Could use some tests that have more than just the single stdin channel. That's a TODO. But I think I can split out an obvious commit from here.

garlick · 2024-10-15T15:09:24Z

Is this ready for another look? I wasn't sure if you were still working on it.

chu11 · 2024-10-15T18:27:40Z

Is this ready for another look? I wasn't sure if you were still working on it.

Good timing! I was cleaning it up a bit and think it's ready for a review. Removed WIP.

Edit: Side note, some updates to RFC coming too.

chu11 · 2024-10-15T21:28:09Z

re-pushed, changing REXEC_CREDIT to REXEC_WRITE_CREDIT per flux-framework/rfc#427 updates

garlick

I think I'd better get your feedback on my one comment so far before I proceed, as addressing it would change things a bit.

src/common/libsubprocess/fbuf.c

src/common/libsubprocess/local.c

chu11 · 2024-10-17T20:33:20Z

re-pushed, re-working the PR per discussion above, all "space" changes are now via the fbuf notify callback

garlick

This is looking good!

Here's a batch of comments. I just reviewed code not tests.

Still hate that flag :-)

src/common/libsubprocess/fbuf.c

src/common/libsubprocess/command.h

src/common/libsubprocess/client.c

src/common/libsubprocess/remote.c

src/common/libsubprocess/server.c

src/common/libsubprocess/subprocess.h

garlick

Nice - couple comments inline.
The big one is why is the initial on_credit callback still special cased with its own watcher?

src/common/libsubprocess/fbuf.c

src/common/libsubprocess/local.c

chu11 · 2024-10-23T22:46:13Z

re-pushed per comments / discussion above.

Moving the "initial credits" to the fbuf watcher wasn't as ugly as I thought it would be. The main trickiness is that we have to "special case" the first "initial credits" callback at both the fbuf_watcher level and the local subprocess level. That's really it.

garlick

Looking great! Here are some comments just reviewing the code, not the tests. All my comments are fairly superficial. Great work here.

I'll try the flux-exec changes on top of this on my test cluster and see how it goes moving some serious data and watching the message traces.

src/common/libsubprocess/ev_fbuf_write.c

src/common/libsubprocess/fbuf_watcher.h

src/common/libsubprocess/remote.c

src/common/libsubprocess/subprocess.c

src/common/libsubprocess/subprocess.h

src/common/libsubprocess/subprocess_private.h

src/common/libsubprocess/local.c

src/common/libsubprocess/client.c

Problem: A comment for fbuf_write_watcher_create() indicates that a FLUX_POLLOUT event can occur "fd closed". This comment is lacking and can be ambiguous. Update the comment for clarity.

Problem: libsubprocess iostress tests only support "API" or "direct" writes in tests. Currently a simple boolean is used in the tests to determine which write to use. But in the near future other options may be available. Refactor the iostress tests to use an enum rather than boolean to indicate how subprocess writes should happen.

Problem: In libsubprocess iostress tests, stdin input for tests is generated in a prep watcher. However, in future tests the input will be needed in other callbacks. Generate the stdin input earlier during test launch time. It will be available for all future callbacks.

Problem: an implementation of credit-based flow control will require the underlying buffer implementation to provide notifications for any change in free buffer space, not just full/empty transitions. Call the notifier callback on any change to the amount of free buffer space. The only current users of the notifiers are the ev_fbuf_read and ev_fbuf_write classes, which need not change. Although their notifiers may be called more frequently, they simply start watchers if the buffer is non-empty or non-full (respectively), so the extra calls are merely fast no-ops. Update fbuf unit test.

Problem: subprocess channel writers have no way to know how much remote buffer space is available, and writing too much may overflow the remote buffer and cause data loss. Support a new on_credit callback that will inform the user that space is available in the write buffer. In order to support this, the fbuf write watcher callback behavior has been altered. It now also issues a callback when there is a change in the amount of buffer space available. This includes an initial callback indicating all of the buffer space is available. Adjust unit tests accordingly. Fixes flux-framework#6291

Problem: There is no coverage for the new libsubprocess on_credit callback. Add unit tests in libsubprocess/test/stdio.c and libsubprocess/test/iostress.c.

chu11 · 2024-10-24T21:09:14Z

re-pushed, fixing up from comments above

garlick

I think this is working as advertised! Awesome.

Also, since nothing really uses the flow control yet I don't think this could have much effect on critical stuff like stdin handling in the shell or PMI.

I say let's merge!

Thank you for all the iterations!

chu11 · 2024-10-24T22:53:39Z

sounds good, setting MWP

codecov · 2024-10-24T23:07:04Z

Codecov Report

Attention: Patch coverage is 85.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 83.50%. Comparing base (4b1631d) to head (02689dd).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/common/libsubprocess/local.c	77.41%	7 Missing ⚠️
src/common/libsubprocess/client.c	80.95%	4 Missing ⚠️
src/common/libsubprocess/server.c	60.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6353      +/-   ##
==========================================
- Coverage   83.52%   83.50%   -0.02%     
==========================================
  Files         525      525              
  Lines       87825    87900      +75     
==========================================
+ Hits        73353    73405      +52     
- Misses      14472    14495      +23

Files with missing lines	Coverage Δ
src/common/libsubprocess/ev_fbuf_write.c	`84.84% <100.00%> (+2.08%)`	⬆️
src/common/libsubprocess/fbuf.c	`96.87% <100.00%> (-0.10%)`	⬇️
src/common/libsubprocess/remote.c	`80.83% <100.00%> (+1.05%)`	⬆️
src/common/libsubprocess/subprocess.c	`89.07% <100.00%> (+0.01%)`	⬆️
src/common/libsubprocess/client.c	`80.12% <80.95%> (+0.12%)`	⬆️
src/common/libsubprocess/server.c	`78.47% <60.00%> (-0.52%)`	⬇️
src/common/libsubprocess/local.c	`82.83% <77.41%> (-0.83%)`	⬇️

... and 12 files with indirect coverage changes

chu11 force-pushed the issue4572_stdin_overflow branch from d29b711 to eb6e6e5 Compare October 9, 2024 20:55

chu11 mentioned this pull request Oct 11, 2024

libsubprocess: check bufsize is > 0 #6365

Merged

chu11 force-pushed the issue4572_stdin_overflow branch from eb6e6e5 to 4f7aa82 Compare October 11, 2024 23:07

chu11 mentioned this pull request Oct 11, 2024

libflux: support setting prep watcher priority #6367

Closed

chu11 force-pushed the issue4572_stdin_overflow branch from 4f7aa82 to 8825537 Compare October 15, 2024 06:48

chu11 mentioned this pull request Oct 15, 2024

flux-exec: use subprocess credits to avoid overflowing stdin buffers #6370

Merged

chu11 force-pushed the issue4572_stdin_overflow branch from 8825537 to 0bdbf8e Compare October 15, 2024 18:08

chu11 changed the title ~~WIP: libsubprocess: support flow control on writes via credits~~ libsubprocess: support flow control on writes via credits Oct 15, 2024

chu11 force-pushed the issue4572_stdin_overflow branch from 0bdbf8e to b4e4e26 Compare October 15, 2024 18:27

chu11 mentioned this pull request Oct 15, 2024

rfc42: add credit based flow control for channels flux-framework/rfc#427

Merged

chu11 force-pushed the issue4572_stdin_overflow branch from b4e4e26 to 938f560 Compare October 15, 2024 21:27

garlick reviewed Oct 15, 2024

View reviewed changes

src/common/libsubprocess/fbuf.c Outdated Show resolved Hide resolved

garlick reviewed Oct 15, 2024

View reviewed changes

src/common/libsubprocess/local.c Outdated Show resolved Hide resolved

chu11 force-pushed the issue4572_stdin_overflow branch from 938f560 to 8717bce Compare October 17, 2024 20:32

garlick reviewed Oct 21, 2024

View reviewed changes

chu11 force-pushed the issue4572_stdin_overflow branch from 8717bce to c6b593e Compare October 22, 2024 22:01

garlick reviewed Oct 22, 2024

View reviewed changes

src/common/libsubprocess/fbuf.c Show resolved Hide resolved

src/common/libsubprocess/local.c Outdated Show resolved Hide resolved

src/common/libsubprocess/local.c Outdated Show resolved Hide resolved

chu11 force-pushed the issue4572_stdin_overflow branch 2 times, most recently from 9fbd8d7 to ec06861 Compare October 23, 2024 22:42

garlick reviewed Oct 24, 2024

View reviewed changes

chu11 force-pushed the issue4572_stdin_overflow branch 2 times, most recently from f791d0e to 34c52df Compare October 24, 2024 20:48

chu11 and others added 6 commits October 24, 2024 14:08

libsubprocess: improve comment details

5bc867a

Problem: A comment for fbuf_write_watcher_create() indicates that a FLUX_POLLOUT event can occur "fd closed". This comment is lacking and can be ambiguous. Update the comment for clarity.

libsubprocess: cover subprocess on_credit

02689dd

Problem: There is no coverage for the new libsubprocess on_credit callback. Add unit tests in libsubprocess/test/stdio.c and libsubprocess/test/iostress.c.

chu11 force-pushed the issue4572_stdin_overflow branch from 34c52df to 02689dd Compare October 24, 2024 21:08

garlick approved these changes Oct 24, 2024

View reviewed changes

chu11 added the merge-when-passing label Oct 24, 2024

mergify bot merged commit 86db3b4 into flux-framework:master Oct 24, 2024
33 checks passed

chu11 deleted the issue4572_stdin_overflow branch October 24, 2024 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libsubprocess: support flow control on writes via credits #6353

libsubprocess: support flow control on writes via credits #6353

chu11 commented Oct 8, 2024

garlick commented Oct 8, 2024

chu11 commented Oct 8, 2024

chu11 commented Oct 8, 2024

garlick commented Oct 8, 2024

chu11 commented Oct 9, 2024

chu11 commented Oct 9, 2024 •

edited

Loading

garlick commented Oct 9, 2024

chu11 commented Oct 9, 2024

chu11 commented Oct 11, 2024

garlick commented Oct 15, 2024

chu11 commented Oct 15, 2024 •

edited

Loading

chu11 commented Oct 15, 2024

garlick left a comment

chu11 commented Oct 17, 2024 •

edited

Loading

garlick left a comment

garlick left a comment

chu11 commented Oct 23, 2024

garlick left a comment

chu11 commented Oct 24, 2024

garlick left a comment

chu11 commented Oct 24, 2024

codecov bot commented Oct 24, 2024

libsubprocess: support flow control on writes via credits #6353

libsubprocess: support flow control on writes via credits #6353

Conversation

chu11 commented Oct 8, 2024

garlick commented Oct 8, 2024

chu11 commented Oct 8, 2024

chu11 commented Oct 8, 2024

garlick commented Oct 8, 2024

chu11 commented Oct 9, 2024

chu11 commented Oct 9, 2024 • edited Loading

garlick commented Oct 9, 2024

chu11 commented Oct 9, 2024

chu11 commented Oct 11, 2024

garlick commented Oct 15, 2024

chu11 commented Oct 15, 2024 • edited Loading

chu11 commented Oct 15, 2024

garlick left a comment

Choose a reason for hiding this comment

chu11 commented Oct 17, 2024 • edited Loading

garlick left a comment

Choose a reason for hiding this comment

garlick left a comment

Choose a reason for hiding this comment

chu11 commented Oct 23, 2024

garlick left a comment

Choose a reason for hiding this comment

chu11 commented Oct 24, 2024

garlick left a comment

Choose a reason for hiding this comment

chu11 commented Oct 24, 2024

codecov bot commented Oct 24, 2024

Codecov Report

chu11 commented Oct 9, 2024 •

edited

Loading

chu11 commented Oct 15, 2024 •

edited

Loading

chu11 commented Oct 17, 2024 •

edited

Loading