On Wed, May 20, 2015 at 11:12:10AM +0300, Adrian Hunter wrote:
> On 20/05/15 09:34, Namhyung Kim wrote:
> > When dso__data_read_offset/addr() is called without prior
> > dso__data_fd() (or other functions which call it internally), it
> > failed to open dso in data_file_size() since its binary type was not
> > identified.
> >
> > However calling dso__data_fd() in dso__data_read_offset() will hurt
> > performance as it grabs a global lock everytime. So factor out the
> > loop on the binary type in dso__data_fd(), and call it from both.
> >
> > Cc: Adrian Hunter <[email protected]>
> > Signed-off-by: Namhyung Kim <[email protected]>
>
> Look good. A few points below.
Thank you.
>
> > ---
> > tools/perf/util/dso.c | 44 ++++++++++++++++++++++++--------------------
> > 1 file changed, 24 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 1b96c8d18435..a3984beca723 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -440,15 +440,7 @@ void dso__data_close(struct dso *dso)
> > pthread_mutex_unlock(&dso__data_open_lock);
> > }
> >
> > -/**
> > - * dso__data_fd - Get dso's data file descriptor
> > - * @dso: dso object
> > - * @machine: machine object
> > - *
> > - * External interface to find dso's file, open it and
> > - * returns file descriptor.
> > - */
> > -int dso__data_fd(struct dso *dso, struct machine *machine)
> > +static void try_to_open_dso(struct dso *dso, struct machine *machine)
>
> This could have 'nolock' in the name e.g. get_fd_nolock
Hmm.. it's always hard for me to choose a good name. But generally I
don't like a name ends with nolock or something like it. And, in
perf, adding '__' prefix is used more than adding nolock suffix.
Do you dislike the name 'try_to_open_dso'? I chose that because it
tries to open dso with a path prefix based on for each binary type.
Also it's called from other than dso__data_fd().
>
> > {
> > enum dso_binary_type binary_type_data[] = {
> > DSO_BINARY_TYPE__BUILD_ID_CACHE,
> > @@ -457,14 +449,6 @@ int dso__data_fd(struct dso *dso, struct machine
> > *machine)
> > };
> > int i = 0;
> >
> > - if (dso->data.status == DSO_DATA_STATUS_ERROR)
> > - return -1;
>
> Please retain this check. It is needed to prevent repeatedly
> trying to open files that aren't there.
I just move it out of the function, so it'll be checked before
entering this function without lock.
>
> > -
> > - pthread_mutex_lock(&dso__data_open_lock);
> > -
> > - if (dso->data.fd >= 0)
> > - goto out;
>
> I would retain this check too. The caller shouldn't really have to do it.
OK.
>
> > -
> > if (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND) {
> > dso->data.fd = open_dso(dso, machine);
> > goto out;
> > @@ -475,14 +459,34 @@ int dso__data_fd(struct dso *dso, struct machine
> > *machine)
> >
> > dso->data.fd = open_dso(dso, machine);
> > if (dso->data.fd >= 0)
> > - goto out;
> > + break;
> >
> > } while (dso->binary_type != DSO_BINARY_TYPE__NOT_FOUND);
> > +
> > out:
> > if (dso->data.fd >= 0)
> > dso->data.status = DSO_DATA_STATUS_OK;
> > else
> > dso->data.status = DSO_DATA_STATUS_ERROR;
> > +}
> > +
> > +/**
> > + * dso__data_fd - Get dso's data file descriptor
> > + * @dso: dso object
> > + * @machine: machine object
> > + *
> > + * External interface to find dso's file, open it and
> > + * returns file descriptor.
> > + */
> > +int dso__data_fd(struct dso *dso, struct machine *machine)
> > +{
> > + if (dso->data.status == DSO_DATA_STATUS_ERROR)
> > + return -1;
> > +
> > + pthread_mutex_lock(&dso__data_open_lock);
> > +
> > + if (dso->data.fd < 0)
> > + try_to_open_dso(dso, machine);
>
> Having the 'dso->data.fd < 0' check inside try_to_open_dso()
> saves a line here.
OK
>
> >
> > pthread_mutex_unlock(&dso__data_open_lock);
> > return dso->data.fd;
> > @@ -709,10 +713,10 @@ static int data_file_size(struct dso *dso, struct
> > machine *machine)
> > * file (dso) due to open file limit (RLIMIT_NOFILE).
> > */
> > if (dso->data.fd < 0) {
> > - dso->data.fd = open_dso(dso, machine);
> > + try_to_open_dso(dso, machine);
> > +
>
> Having the 'dso->data.fd < 0' check inside try_to_open_dso()
> saves a couple of lines here.
OK
>
> Really should change dso_cache__read() too.
Right, will add.
Thanks,
Namhyung
>
> > if (dso->data.fd < 0) {
> > ret = -errno;
> > - dso->data.status = DSO_DATA_STATUS_ERROR;
> > goto out;
> > }
> > }
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
