Re: [Toybox] [PATCH] xputs: Do flush
On 5/20/24 08:32, Yi-Yo Chiang wrote: > Thanks! Adding TOYFLAG_NOBUF worked. > > I feel the same way about "manual flushing of the output buffer is a terrible > interface". I asked myself the question "Why am I manually flushing so much? > There must be a better way..." multiple times when I wrote the other patch > that > does s/xprintf/dprintf/, s/xputs/xputsn/ It's an annoying design quandry. > > Your other patch changes a bunch of xprintf() to dprintf() which is even > _more_ > > fun because dprintf() writes directly to the file descriptor (bypassing > the > > buffer in the libc global FILE * instance "stdio"), which means in the > absence > > of manual flushing the dprintf() data will go out first and then the > stdio > data > > will go out sometime later, in the wrong order. Mixing the two output > formats > > tends to invert the order that way, unless you disable output buffering. > > Which is the reason I replaced those all with the "flush" functions (xputsn) > or > direct fd-write functions (dprintf), so that their order won't shuffle. > Anyway the problem is moot now that we have TOYFLAG_NOBUF. Eh, not moot. Shifted. Currently there's one command using TOYFLAG_NOBUF, and a lot of recent buffering fixes: ea119151ccc5 59b041d14aec afeed2d46a9a a57e42a386b0 ca6bde9e1c43 I should probably audit all the commands and figure out which buffering type to use for each. (grep currently finds manual fflush() in hexedit, login, watch, and ps). But not today... > > But that hasn't been popular, and it's a pain to implement in userspace > > (because > > we don't have access to mulitple cheap timers like the kernel does, we need > > to > > take a signal and there's both a limited number of signals). > > 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... Within toybox I could probably come up with something, true. Although fflush() locking is still a bit problematic if I'm not depending on thread infrastructure. (Either I don't use FILE * and do it myself, or I require libc to be thread aware.) Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] xputs: Do flush
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
Re: [Toybox] [PATCH] xputs: Do flush
Thanks! Adding TOYFLAG_NOBUF worked.
I feel the same way about "manual flushing of the output buffer is a
terrible interface". I asked myself the question "Why am I manually
flushing so much? There must be a better way..." multiple times when I
wrote the other patch that does s/xprintf/dprintf/, s/xputs/xputsn/
On Mon, May 20, 2024 at 8:36 PM enh wrote:
> On Sun, May 19, 2024 at 10:56 PM Rob Landley wrote:
> >
> > On 5/18/24 21:53, Yi-Yo Chiang wrote:
> > > What I wanted to address with this patch are:
> > >
> > > 1. Fix this line of
> > > xputs()
> https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> > > The prompt text is not flushed immediately, so it is not shown to the
> user until
> > > the escape char is entered (which defeats the purpose of the prompt,
> that is to
> >
> > I agree you've identified two problems (unflushed prompt, comment not
> matching
> > code) that both need to be fixed. I'm just unhappy with the solutions,
> and am
> > concerned about a larger design problem.
> >
> > I implemented TOYFLAG_NOBUF and applied it to this command. The result
> compiles
> > but I'm not near serial hardware at the moment, does it fix the problem
> for you?
> >
> > Trying to fix it via micromanagement (adding more flushing and switching
> some
> > but not all output contexts in the same command between FILE * and file
> > descriptor) makes my head hurt...
> >
> > 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 :-) )
>
> > > tell the user what the escape char is) and stdout is flushed by
> handle_esc.
> > > To fix this we either make xputs() flush automatically, or we just add
> a single
> > > line of manual flush() after xputs() in microcom.c.
> > > Either is fine with me.
> >
> > When I searched for the first xputs in microcom I got:
> >
> > xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
> > if (read(0, , 1)<1 || input == CTRL('D') || input == 'q') {
> >
> > Which is a separate function (the n version is no newline, it does not
> add the
> > newline the way libc puts() traditionally does), with its own flushing
> > semantics: xputsn() doesn't call xputs(), and neither calls or is called
> by
> > xprintf(). "Some functions flush, some functions don't" is a bit of a
> design
> > sharp edge.
>
> (yeah, exactly.)
>
> > The larger problem is manual flushing of the output buffer is a terrible
> > interface, and leads to missing error checking without which a command
> won't
> > reliably exit when its output terminal closes because the whole SIGPIPE
> thing
> > was its own can of worms that even bionic used to manually mess with.
> Which is
> > why I originally made toybox not ever do that (systemic fix) but I got
> > complaints about performance.
> >
> > Your other patch changes a bunch of xprintf() to dprintf() which is even
> _more_
> > fun because dprintf() writes directly to the file descriptor (bypassing
> the
> > buffer in the libc global FILE * instance "stdio"), which means in the
> absence
> > of manual flushing the dprintf() data will go out first and then the
> stdio data
> > will go out sometime later, in the wrong order. Mixing the two output
> formats
> > tends to invert the order that way, unless you disable output buffering.
>
Which is the reason I replaced those all with the "flush" functions
(xputsn) or direct fd-write functions (dprintf), so that their order won't
shuffle.
Anyway the problem is moot now that we have TOYFLAG_NOBUF.
> >
> > 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
> >
Re: [Toybox] [PATCH] xputs: Do flush
On Sun, May 19, 2024 at 10:56 PM Rob Landley wrote:
>
> On 5/18/24 21:53, Yi-Yo Chiang wrote:
> > What I wanted to address with this patch are:
> >
> > 1. Fix this line of
> > xputs()
> > https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> > The prompt text is not flushed immediately, so it is not shown to the user
> > until
> > the escape char is entered (which defeats the purpose of the prompt, that
> > is to
>
> I agree you've identified two problems (unflushed prompt, comment not matching
> code) that both need to be fixed. I'm just unhappy with the solutions, and am
> concerned about a larger design problem.
>
> I implemented TOYFLAG_NOBUF and applied it to this command. The result
> compiles
> but I'm not near serial hardware at the moment, does it fix the problem for
> you?
>
> Trying to fix it via micromanagement (adding more flushing and switching some
> but not all output contexts in the same command between FILE * and file
> descriptor) makes my head hurt...
>
> 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 :-) )
> > tell the user what the escape char is) and stdout is flushed by handle_esc.
> > To fix this we either make xputs() flush automatically, or we just add a
> > single
> > line of manual flush() after xputs() in microcom.c.
> > Either is fine with me.
>
> When I searched for the first xputs in microcom I got:
>
> xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
> if (read(0, , 1)<1 || input == CTRL('D') || input == 'q') {
>
> Which is a separate function (the n version is no newline, it does not add the
> newline the way libc puts() traditionally does), with its own flushing
> semantics: xputsn() doesn't call xputs(), and neither calls or is called by
> xprintf(). "Some functions flush, some functions don't" is a bit of a design
> sharp edge.
(yeah, exactly.)
> The larger problem is manual flushing of the output buffer is a terrible
> interface, and leads to missing error checking without which a command won't
> reliably exit when its output terminal closes because the whole SIGPIPE thing
> was its own can of worms that even bionic used to manually mess with. Which is
> why I originally made toybox not ever do that (systemic fix) but I got
> complaints about performance.
>
> Your other patch changes a bunch of xprintf() to dprintf() which is even
> _more_
> fun because dprintf() writes directly to the file descriptor (bypassing the
> buffer in the libc global FILE * instance "stdio"), which means in the absence
> of manual flushing the dprintf() data will go out first and then the stdio
> data
> will go out sometime later, in the wrong order. Mixing the two output formats
> tends to invert the order that way, unless you disable output buffering.
>
> 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?)
> And OTHER implementations can't consistently get this right, which is why
> whether:
>
> for i in {1..100}; do echo -n .; sleep .1; done | less
>
> Produces any output before 10 seconds have elapsed is potluck, and varies from
> release to release of the same distro.
>
> Oh, and the gnu/crazies just came up with a fourth category of write as a
> gnu/extension: flush after NUL byte.
Re: [Toybox] [PATCH] xputs: Do flush
On 5/18/24 21:53, Yi-Yo Chiang wrote:
> What I wanted to address with this patch are:
>
> 1. Fix this line of
> xputs() https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> The prompt text is not flushed immediately, so it is not shown to the user
> until
> the escape char is entered (which defeats the purpose of the prompt, that is
> to
I agree you've identified two problems (unflushed prompt, comment not matching
code) that both need to be fixed. I'm just unhappy with the solutions, and am
concerned about a larger design problem.
I implemented TOYFLAG_NOBUF and applied it to this command. The result compiles
but I'm not near serial hardware at the moment, does it fix the problem for you?
Trying to fix it via micromanagement (adding more flushing and switching some
but not all output contexts in the same command between FILE * and file
descriptor) makes my head hurt...
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...
> tell the user what the escape char is) and stdout is flushed by handle_esc.
> To fix this we either make xputs() flush automatically, or we just add a
> single
> line of manual flush() after xputs() in microcom.c.
> Either is fine with me.
When I searched for the first xputs in microcom I got:
xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
if (read(0, , 1)<1 || input == CTRL('D') || input == 'q') {
Which is a separate function (the n version is no newline, it does not add the
newline the way libc puts() traditionally does), with its own flushing
semantics: xputsn() doesn't call xputs(), and neither calls or is called by
xprintf(). "Some functions flush, some functions don't" is a bit of a design
sharp edge.
The larger problem is manual flushing of the output buffer is a terrible
interface, and leads to missing error checking without which a command won't
reliably exit when its output terminal closes because the whole SIGPIPE thing
was its own can of worms that even bionic used to manually mess with. Which is
why I originally made toybox not ever do that (systemic fix) but I got
complaints about performance.
Your other patch changes a bunch of xprintf() to dprintf() which is even _more_
fun because dprintf() writes directly to the file descriptor (bypassing the
buffer in the libc global FILE * instance "stdio"), which means in the absence
of manual flushing the dprintf() data will go out first and then the stdio data
will go out sometime later, in the wrong order. Mixing the two output formats
tends to invert the order that way, unless you disable output buffering.
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.
And OTHER implementations can't consistently get this right, which is why
whether:
for i in {1..100}; do echo -n .; sleep .1; done | less
Produces any output before 10 seconds have elapsed is potluck, and varies from
release to release of the same distro.
Oh, and the gnu/crazies just came up with a fourth category of write as a
gnu/extension: flush after NUL byte.
https://lists.gnu.org/archive/html/coreutils/2024-03/msg00016.html
It's very gnu to fix "this is too complicated to be reliable" by adding MORE
complication. Note how the problem WE hit here was 1) we didn't ask for LINEBUF
mode, 2) \r doesn't count as a line for buffer flushing purposes anyway, 3) the
new feature making it trigger on NUL instead _still_ wouldn't make \r count as a
line for buffer flushing purposes.
My suggestion for a "proper fix" to the problem _category_ of small writes being
too expensive was to have either libc or the kernel use nagle's algorithm for
writes from userspace, like it does for network connections. (There was a fix to
this category of issue decades ago, it just never got applied HERE.)
But that hasn't been popular, and it's a pain to implement in
Re: [Toybox] [PATCH] xputs: Do flush
What I wanted to address with this patch are: 1. Fix this line of xputs() https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113 The prompt text is not flushed immediately, so it is not shown to the user until the escape char is entered (which defeats the purpose of the prompt, that is to tell the user what the escape char is) and stdout is flushed by handle_esc. To fix this we either make xputs() flush automatically, or we just add a single line of manual flush() after xputs() in microcom.c. Either is fine with me. 2. The comment string before xputs() is misleading and I wanted to fix that. I was misled by the comment line, and incorrectly thought xputs() was flushing. I only discovered otherwise after iterations of trial-and-error. To fix this is to either make xputs() flush automatically, or just update the comment string to clarify that xputs() does _not_ flush. Again either path is fine with me. I just merged the fixes of the two problems into one because they share the former solution. Plus xputsl() and xputsn() are also flushing, so I thought / believed xputs() was mistakenly made non-flushing. Since the intention _is_ to keep xputs() non-flushing, how about we just update the comment of xputs() (2), and add the line of flush in microcom.c (1)? On Sun, May 19, 2024 at 8:41 AM Rob Landley wrote: > On 5/16/24 06:46, Yi-Yo Chiang via Toybox wrote: > > The comment string claims xputs() to write, flush and check error. > > However the 'flush' operation is actually missing due to 3e0e8c6 > > changing the default buffering mode from 'line' to 'block'. > > That's sort of an Elliott question? > > Originally, xprintf() and friends all flushed (which is necessary to detect > output errors and xexit() if so), but Elliott complained that was too > slow, so > the flushes got removed, and then we changed the default stdout buffering > type, > and... > > Alas, it was a whole multi-year thing. Elliott has volunteered to put > manual > flushes everywhere it's a problem. I've seriously thought about going > exclusively to file descriptor output (dprintf() is in posix now) and > leaving > FILE * for input only. > > Personally, I honestly believe the _proper_ fix is to upgrade the kernel > to use > vdso to implement nagle's algorithm on file descriptor 1: > > https://landley.net/notes-2024.html#28-04-2024 > > But I'm not holding my breath. > > Rob > > P.S. I should post some subset of > https://landley.net/bin/mkroot/latest/linux-patches/ to linux-kernel > again. So > they can be ignored again. > ___ > Toybox mailing list > Toybox@lists.landley.net > http://lists.landley.net/listinfo.cgi/toybox-landley.net > ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] xputs: Do flush
On 5/16/24 06:46, Yi-Yo Chiang via Toybox wrote: > The comment string claims xputs() to write, flush and check error. > However the 'flush' operation is actually missing due to 3e0e8c6 > changing the default buffering mode from 'line' to 'block'. That's sort of an Elliott question? Originally, xprintf() and friends all flushed (which is necessary to detect output errors and xexit() if so), but Elliott complained that was too slow, so the flushes got removed, and then we changed the default stdout buffering type, and... Alas, it was a whole multi-year thing. Elliott has volunteered to put manual flushes everywhere it's a problem. I've seriously thought about going exclusively to file descriptor output (dprintf() is in posix now) and leaving FILE * for input only. Personally, I honestly believe the _proper_ fix is to upgrade the kernel to use vdso to implement nagle's algorithm on file descriptor 1: https://landley.net/notes-2024.html#28-04-2024 But I'm not holding my breath. Rob P.S. I should post some subset of https://landley.net/bin/mkroot/latest/linux-patches/ to linux-kernel again. So they can be ignored again. ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
