mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@aerifal.cx>
To: musl@lists.openwall.com
Subject: Re: procfs stdio writev problem
Date: Fri, 10 May 2013 23:49:47 -0400	[thread overview]
Message-ID: <20130511034947.GR20323@brightrain.aerifal.cx> (raw)
In-Reply-To: <alpine.LNX.2.10.1305052042240.18514@laas.mine.nu>

On Sun, May 05, 2013 at 08:49:50PM +0200, Jens wrote:
> >Looks to me at a quick glance like stdio needs something like (untested)
> >
> >--- ./src/stdio/__stdio_write.c~ 2012-12-01 22:56:34.156555480 +0000
> >+++ ./src/stdio/__stdio_write.c 2013-05-05 10:59:49.856504883 +0100
> >@@ -37,7 +37,7 @@
> > return iovcnt == 2 ? 0 : len-iov[0].iov_len;
> > }
> > rem -= cnt;
> >- if (cnt > iov[0].iov_len) {
> >+ if (cnt >= iov[0].iov_len) {
> > f->wpos = f->wbase = f->buf;
> > cnt -= iov[0].iov_len;
> > iov++; iovcnt--;
> >
> >In the case where the kernel exactly eats the iov you need to move
> >onto the next one rather than have a zero length write pointing just
> >after the existing one, as that could be an invalid address.
> 
> In this case its not the zero length that is the problem.
> The problem is that procfs treats each write (or apprently each part
> of the iov) as a separate operation.
> 
> So the first operation is "60" which is fine.
> The next one is "\n" which is invalid.
> So we get two operations instead of one.
> 
> The implementation in bash amounts to a printf("60") followed by
> putchar('\n');
> 
> The same thing in uclibc works as intended.
> 
> I guess I can patch bash, or use sysctl program.
> 
> AFAIK neither musl or procfs is doing anything wrong here, it just
> happens that a pure echo no longer works as it used to.

I think we can work around this (and possibly improve performance) by
modifying __overflow to store the new character into the buffer and
pass 0 length to __stdio_write. However, __stdio_write does not seem
prepared to deal with zero length specially, and although per the
standard it should not cause problems as long as the buffer is
non-empty, I don't trust Linux not to do stupid things with a zero iov
length on certain file types (like /proc).

So I think two changes are needed:

1. Modifying __overflow to submit the new char as part of the buffer.

2. Modifying __stdio_write to optimize the iov array, removing
   zero-length FILE-buffer or caller-buffer components. This will
   require some overhaul since the current code is ugly and using the
   iov index to make assumptions about whether it's operating on the
   FILE buffer or not.

It's this second change that I think should improve performance, since
Linux often behaves suboptimally on multiple iov components.

Rich


      parent reply	other threads:[~2013-05-11  3:49 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-05-05  9:16 Jens
2013-05-05 10:01 ` Justin Cormack
2013-05-05 18:49   ` Jens
2013-05-05 19:03     ` Rich Felker
2013-05-11  3:49     ` Rich Felker [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130511034947.GR20323@brightrain.aerifal.cx \
    --to=dalias@aerifal.cx \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).