procfs uglyness caused by "cat"

procfs uglyness caused by "cat"

[Herbert Rosmanith]

hello list,

when doing "cat /proc/uptime" or similar files, the
routine which prepares the buffer gets called twice.

this is because "cat" reads two times from the file,
as a system-call-trace shows:

    # strace -f cat /proc/uptime
    ...
    read(3, "5241.09 5082.74\n", 1024)      = 16
    ...
    read(3, "", 1024)                       = 0
    ...

this leads to uptime_read_proc() being called two times,
the second time with parameter off=16, but since this
is ignored, the work is done twice: clock_...gettime(),
cputime_to_timespec(), sprintf(), proc_calc_metrics()
and so on.

insert a printk-statement if you don't beleive this.

btw, there's no wrong information produced because of this,
because *page points to the same location both calls.

a simple way to get rid of this:

static int uptime_read_proc(char *page, char **start, off_t off,
                                 int count, int *eof, void *data)
{
        struct timespec uptime;
        struct timespec idle;
        int len;
        cputime_t idletime;

+	if (off)
+		return 0;

        cputime_add(init_task.utime, init_task.stime);
        do_posix_clock_monotonic_gettime(&uptime);
        cputime_to_timespec(idletime, &idle);
        len = sprintf(page,"%lu.%02lu %lu.%02lu\n",
...
and so on.

this affects possibly all /proc files which ignore the offset parameter
and are evaluated(?) with "cat".

regards,
h.rosmanith

Re: procfs uglyness caused by "cat"

[Robert Hancock]

Herbert Rosmanith wrote:
> a simple way to get rid of this:
> 
> static int uptime_read_proc(char *page, char **start, off_t off,
>                                  int count, int *eof, void *data)
> {
>         struct timespec uptime;
>         struct timespec idle;
>         int len;
>         cputime_t idletime;
> 
> +	if (off)
> +		return 0;

Except that this is wrong - if you try to advance the offset a bit from 
the start of the file and read something, you'll get nothing. This is 
inconsistent with normal file behavior.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: procfs uglyness caused by "cat"

[Paul Rolland]

> > static int uptime_read_proc(char *page, char **start, off_t off,
> >                                  int count, int *eof, void *data)
> > {
> >         struct timespec uptime;
> >         struct timespec idle;
> >         int len;
> >         cputime_t idletime;
> > 
> > +	if (off)
> > +		return 0;
> 
> Except that this is wrong - if you try to advance the offset 
> a bit from 
> the start of the file and read something, you'll get nothing. This is 
> inconsistent with normal file behavior.

Right... What's weird is : what do we get if a process decides to read
this using a 1 byte buffer, asking for 1 char at a time ?
And what we'll be the result if you read 1 char every 1 second ?

#include <stdio.h>

int main(int argc, char * argv[])
{
  FILE * f;
  char lChar;

  f = fopen("/proc/uptime", "r");
  if (f == NULL) {
    exit(0);
  } /* endif */

  while (!feof(f)) {
    fread(&lChar, 1, 1, f);
    fprintf(stdout, "%c", lChar); fflush(stdout);
    sleep(1);
  } /* endwhile */

  close(f);

  exit(0);
}

is funny enough...

2.2.x :
58 [15:30] rol@www-dev:/tmp> cat /proc/uptime ; ./test
13849305.25 13555633.92
13849312.38 13555635.64

2.4.31 :
bash-2.05# cat /proc/uptime ; ./test
100711.77 100366.30
100711.77 100366.30

Paul

Re: procfs uglyness caused by "cat"

[Paul Jackson]

Robert Hancock explained what's wrong with your proposal of:
+	if (off)
+		return 0;

For an alternative that seems to work better, see the processing
behind the /proc/<pid>/cpuset file, by grep'ing for seq_file in
kernel/cpuset.c.

Essentially, it composes the result string on the open, and then
lets user code read and seek over that string at will.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: procfs uglyness caused by "cat"

[Paul Jackson]

Paul Rolland wrote:
> is funny enough...

You used the stdio routine fread - which buffers in user space.  It
does a single read, and then feeds you the characters as you ask for
them, out of its stdio buffer.

Try the following program, which doesn't buffer:

main()
{
    char c;
    int fd = open("/proc/uptime", 0);
    while (read(fd, &c, 1) == 1) {
        write(1, &c, 1);
        sleep(1);
    }
}

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: procfs uglyness caused by "cat"

[Paul Rolland]

Hello,

> main()
> {
>     char c;
>     int fd = open("/proc/uptime", 0);
>     while (read(fd, &c, 1) == 1) {
>         write(1, &c, 1);
>         sleep(1);
>     }
> }

Yes, much better, same result for 2.2.x and 2.4.31 :
bash-2.05# cat /proc/uptime; ./test2  
112049.81 111665.11
112054.89 111670.29

The result is more "correct", but I guess this makes it worse for
Herbert, as uptime_read_proc() is then called for each character...

Paul

Re: procfs uglyness caused by "cat"

[Herbert Rosmanith]

> > static int uptime_read_proc(char *page, char **start, off_t off,
> >                                  int count, int *eof, void *data)
> > {
> >         struct timespec uptime;
> >         struct timespec idle;
> >         int len;
> >         cputime_t idletime;
> > 
> > +	if (off)
> > +		return 0;
> 
> Except that this is wrong - if you try to advance the offset a bit from 
> the start of the file and read something, you'll get nothing. This is 
> inconsistent with normal file behavior.

so what - "uptime_read_proc" ignores the offset parameter anyway.
you get wrong results right now, too, even without the two lines
I've added.

if you write "Except that this is wrong" you should have in mind that
"this is wrong" currently, too.

just "try to advance the offset a bit from the start of the file and
read something", like "dd if=/proc/uptime bs=1 count=1 seek=1".
do you expect to get right results?

regards,
h.rosmanith

Re: procfs uglyness caused by "cat"

[Herbert Rosmanith]

> > > static int uptime_read_proc(char *page, char **start, off_t off,
> > >                                  int count, int *eof, void *data)
> > > {
> > >         struct timespec uptime;
> > >         struct timespec idle;
> > >         int len;
> > >         cputime_t idletime;
> > > 
> > > +	if (off)
> > > +		return 0;
> > 
> > Except that this is wrong - if you try to advance the offset a bit from 
> > the start of the file and read something, you'll get nothing. This is 
> > inconsistent with normal file behavior.
> 
> so what - "uptime_read_proc" ignores the offset parameter anyway.
> you get wrong results right now, too, even without the two lines
> I've added.
> 
> if you write "Except that this is wrong" you should have in mind that
> "this is wrong" currently, too.
> 
> just "try to advance the offset a bit from the start of the file and
> read something", like "dd if=/proc/uptime bs=1 count=1 seek=1".
> do you expect to get right results?

argh, my mistake, sorry. I see. I tried reading a character at a time
and with the two lines, /proc/uptime will return only a single character.

so even though currently the update-routine is called more than
needed, my patch is even more wrong.

but this means that e.g.:

    int main() {
    int fd;
    char ch;
            fd=open("/proc/uptime",0);
            for(;;) {
                    if (read(fd,&ch,1)!=1)
                            break;
                    write(1,&ch,1);
            }
    }

will call the buffer-formatting routines 16 times on the whole
buffer, just to return 1 single character, and each call will
return a different result. eeks...

by the way: why does "dd if=/proc/uptime bs=1 seek=1" hang?
e.g.:

    root@afp ~ # strace -f dd if=/proc/uptime bs=1 count=1 seek=1
    open("/proc/uptime", O_RDONLY|O_LARGEFILE) = 0
    ...
    _llseek(1, 1, 0xbfaa8464, SEEK_CUR)     = -1 ESPIPE (Illegal seek)
    read(1, <unfinished ...>

processing stops here.


regards,
h.rosmanith


regards,
h.rosmanith