lkml.org 
[lkml]   [2020]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4 1/2] tools api: add a lightweight buffered reading api
    On Tue, Apr 14, 2020 at 7:20 PM Namhyung Kim <namhyung@kernel.org> wrote:
    >
    > On Tue, Apr 14, 2020 at 9:48 AM Ian Rogers <irogers@google.com> wrote:
    > >
    > > On Mon, Apr 13, 2020 at 5:16 PM Namhyung Kim <namhyung@kernel.org> wrote:
    > > >
    > > > On Tue, Apr 14, 2020 at 1:22 AM Ian Rogers <irogers@google.com> wrote:
    > > > >
    > > > > On Mon, Apr 13, 2020 at 12:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
    > > > > >
    > > > > > Hi Ian,
    > > > > >
    > > > > > On Sat, Apr 11, 2020 at 3:42 PM Ian Rogers <irogers@google.com> wrote:
    > > > > > >
    > > > > > > The synthesize benchmark shows the majority of execution time going to
    > > > > > > fgets and sscanf, necessary to parse /proc/pid/maps. Add a new buffered
    > > > > > > reading library that will be used to replace these calls in a follow-up
    > > > > > > CL. Add tests for the library to perf test.
    > > > > > >
    > > > > > > v4 adds the test file missed in v3.
    > > > > > >
    > > > > > > Signed-off-by: Ian Rogers <irogers@google.com>
    > > > > > > ---
    > > > > > > +/* Read a hexadecimal value with no 0x prefix into the out argument hex. If the
    > > > > > > + * first character isn't hexadecimal returns -2, io->eof returns -1, otherwise
    > > > > > > + * returns the character after the hexadecimal value which may be -1 for eof.
    > > > > >
    > > > > > I'm not sure returning -1 is good when it actually reads something and
    > > > > > meets EOF.
    > > > > > Although it would have a valid value, users might consider it an error IMHO.
    > > > > > Why not returning 0 instead? (I'm ok with -1 for the later use of the API).
    > > > >
    > > > > Thanks for the feedback! In the code for /proc/pid/maps this is a
    > > > > hypothetical, but I think having the API right is important. I didn't
    > > > > go with 0 as you mention 0 'could' encode a character, for example,
    > > > > 7fffabcd\0 wouldn't be distinguishable from 7fffabcd<EOF>.
    > > >
    > > > Practically I don't think it matters in this case as long as we can
    > > > distinguish them in the next call (if the user wants to do).
    > > > What users want to do (I think) is whether the returned value
    > > > (in *hex) is ok to use or not. By returning -1 on EOF, it might
    > > > be confusing for users..
    > >
    > > In the /proc/pid/maps case the code for reading an address like
    > > "00400000-00452000 " the code is:
    > >
    > > if (io__get_hex(io, start) != '-')
    > > return false;
    > > if (io__get_hex(io, end) != ' ')
    > > return false;
    > >
    > > If io__get_hex doesn't return the next character it becomes:
    > >
    > > if (io__get_hex(io, start))
    > > return false;
    > > if (io__get_char(io) != '-')
    > > return false;
    > > if (io__get_hex(io, end))
    > > return false;
    > > if (io__get_char(io) != ' ')
    > > return false;
    > >
    > > Which is twice as verbose and requires that io have a rewind operation
    > > to go backward when io__get_hex and io__get_dec have gone 1 character
    > > too far.
    >
    > Yeah, I'm not against returning the next character - it's good.
    > The only concern was whether it should return -1 or 0 when
    > it meets EOF after parsing some digits.
    >
    > But I think we can go with this version as there's no such case
    > when parsing /proc/pid/maps.
    >
    > >
    > > > > The updated
    > > > > code distinguishes the cases as 0 meaning character \0, -1 meaning EOF
    > > > > and -2 meaning bad encoding. Your worry is that a hex number that's
    > > > > next to EOF will get a result of -1 showing the EOF came next. and
    > > > > code that does 'if ( .. < 0)' would trigger. While clunky, it'd be
    > > > > possible in those cases to change the code to 'if ( .. < -1)'.
    > > >
    > > > Yes, but it's not conventional IMHO.
    > > >
    > > >
    > > > > So my thoughts are:
    > > > > 1) being able to tell apart the 3 cases could be important - this is
    > > > > all hypothetical;
    > > > > 2) keeping EOF and error as negative numbers has a degree of consistency;
    > > > > 3) using -1 for EOF comes from get_char, it'd be nice to have one
    > > > > value mean EOF.
    > > > > Perhaps the issue is the name of the function? It isn't a standard API
    > > > > to return the next character, but it simplified things for me as I
    > > > > didn't need to add a 'rewind' operation. The function names could be
    > > > > something like io__get_hex_then_char and io__get_dec_then_char, EOF
    > > > > for the 'then_char' part would be more consistent. I'd tried to keep
    > > > > the names short and have a load bearing comment, which isn't ideal but
    > > > > generally I believe the style is that function names are kept short.
    > > > > Let me know what you think.
    > > >
    > > > I'm ok with the function name and understand your concerns.
    > > > And I don't want to insist it strongly but just sharing my thoughts.
    > > >
    > > > Thanks
    > > > Namhyung
    > >
    > > Thanks, feedback appreciated! It is useful to discuss and it is
    > > straightforward to change the API but I'm in two minds as to whether
    > > it would be better.
    > >
    > > I'd still like to land this and the next patch, as getting rid of
    > > fgets/sscanf saves 50us from event synthesis. Breaking out the io part
    > > of that change wasn't done so much with a view to replacing stdio, but
    > > just something minimal that serves the /proc/pid/maps case.
    >
    > The performance gain looks nice! Thanks for working on this.

    Thanks for the feedback! I think we can keep an eye on the API, the
    idea to change to 0 isn't an unreasonable one. I have some other
    multithreaded synthesis improving patches to send from Stephane, and
    I've updated the benchmark to measure these. I'll re-send these
    patches with the new single and multi-threaded benchmark data.

    Thanks,
    Ian

    > Thanks
    > Namhyung

    \
     
     \ /
      Last update: 2020-04-15 04:27    [W:2.601 / U:0.848 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site