[Toybox] [PATCH] xputs: Do flush

[Rob Landley]

On 5/20/24 07:36, enh wrote:
>> Adding flushing to xputs() is an Elliott question is because he's the one who
>> can presumably keep stdio buffer semantics in his head. They make zero sense to
>> me. I added a flush to xputsn() because in line buffering mode it was the
>> "dangerous" one that had no newline thus no flush, but then when we go to block
>> buffering mode xputs() needs a flush just like xputsn() would, and MAYBE it's
>> good to have the flush because in line buffer mode it would be a NOP? Except the
>> command selected block buffering mode precisely BECAUSE it didn't want to flush
>> each line, so why should xputs() do it when the command asked not to? And if
>> xputs() is doing it, why is xprintf() _not_ doing it? And if xprintf() _is_
>> doing it, then we're back to basically not buffering the output...
> 
> this to me was exactly why it should be "everything flushes" or
> "nothing flushes". not "some subset of calls for some subset of
> inputs", because no-one can keep all the special cases in their heads.
> and "everything flushes" is problematic from a performance
> perspective, so "nothing flushes" wins by default. (but, yes, when we
> have our own kernel, have a time-based component to buffering layer's
> flushing is an interesting idea :-) )

Eh, now Yi-Yo's pointed me back at timer_create() and reminded me of realtime
signals, it seems like the plumbing is there to make FILE * output use nagle.

The problem is a userspace wrapper trying to fflush() from signal context
assumes everything's written in an async-safe way (and of course that everyone
else will SA_RESTART when interrupted), which either means spray it down with
thread locking or use cmpxchg() within the flush() implementation, and either
way involves me trusting libc in a way I currently don't. (And I can't do it
"right" myself due to FILE * internals being opaque, I have to wrap an unknown
implementation...)

But it sounds quite feasible for _libc_ to do setvbuf(_IONAG) these days. :)

(Sigh, or threads with signalfd(). Grrr.)

>> I like systematic solutions that mean the problem never comes up again. Elliott
>> has been advocating the whack-a-mole "fix them as we find them" approach here
>> which I am not comfortable with. I've been leaning towards adding a
>> TOYFLAG_NOBUF so we can select a third buffering type, and go back to "do not
>> buffer output at all, flush after every ANSI write function" for at least some
>> commands I'd like to be able to reason about. Especially for commands like this
>> where output performance really doesn't seem like it should be an issue.
> 
> +1 --- an inherently interactive program like this seems reasonable to
> be unbuffered. (except for file transfer?)

Isn't file transfer sending 4k blocks?

Buffering gets really weird with this kind of program anyway: when you're
sending data across a serial port that's breaking it up into individually
transmitted bytes, and depending on what your 16550a-or-similar threshold is set
to the recipient's probably getting notified of each group of 8 bytes. (And yes,
the hardware uses SOMETHING LIKE NAGLE internally to enforce the input and
output notification thresholds.)

Then you layer ppp over it, which breaks your 4k into something like 1.5k
chunks, then does nagle on that trying to fill out the last packet, and that's
assuming there were no pipes in there which do their own re-collating on the
data. And then of course files, once there's filesystem involvement... but of
course the VFS is in there before that marshalling data into and out of page
cache...

The point of the output buffer is to deal with chunks of data "big enough" to
amortize the transaction overhead. Zerocopy of the data has always been somewhat
aspirational, and handing off buffers between page table contexts is often more
expensive than copying it. (Or not! It changes between hardware generations and
I didn't even PRETEND to be current on how the "mitigations for cache
speculation side channel attacks" differ between different kernel versions
running on different arm processors...

There comes a point where "locality within process good, launch largeish buffer
out into the operating system, wave bye-bye as it goes off into the machinery"
is the best I can do. Although the definition of largeish still has the dregs of
moore's law clinging to it with the recent 4k->256k push. (xmodem had 128 byte
packets, I suppose it's roughly the same jump...)

>> https://lists.gnu.org/archive/html/coreutils/2024-03/msg00016.html
> 
> (fwiw, i think that was just some internet rando asking for it, no?
> and they didn't actually implement it?)

Padraig's reply was "this does seem like useful functionality" and a pointer to
the libc people, and then there were over a dozen additional replies in the
thread, so I wouldn't call it a clear no...

> do you run on anything that doesn't have real-time signals? i was
> going to say that -- since toybox is a closed world -- you could just
> use SIGUSR2, but i see that dhcp is already using that! but if you can
> assume real-time signals, there are plenty of them...

Can I safely call fflush() from signal context? It's not listed in man 7
signal-safety...

>> I don't know if xputs() should have the flushing added or the comment removed,
>> that's a design question for Elliott.
> 
> comment removed makes sense to me?

Done.

> i like the x<foo>() => "it's <foo>, but exits on error" pattern. super
> consistent and easy to reason about, and easy to read when you know
> it, and easy to fix code that should/shouldn't error in either
> direction, by adding or removing the 'x'. (one of the biggest problems
> with the old xflush() was that it mostly _didn't_ flush! xferror() is
> a weird name, but at least it does what it says on the tin :-) )

Indeed. The original design was elegant, but slow. It is now less slow. I would
like to reclaim some variant of elegance about it, but it's a todo cleanup item
at this point. (Needs an analysis pass over all commands.)

I _kind_ of want to make unbuffered be the default and have a TOYFLAG_OUTBUF
request the 4k buffer so the ones with sharp edges are annotated. But that's for
a future cleanup pass that actually EXAMINES all the semantics, and performance,
and make sure no write() vs printf() order inversions can happen...

Rob

P.S. I get "stdout vs stderr" order inversions all the time in script
redirections, usually because I washed something through sed or tee or similar.
Buffering within commands is seldom as transparent as people seem to think it
is, especially when they didn't TEST it. But then most people aren't doing
horrors like this line from mkroot/testroot.sh:

    } | tee root/build/log/$Y-test.txt | { [ -z "$V" ] && cat >/dev/null || { [
"$V" -gt 1 ] && cat || grep '^=== '; } }