Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-29 Thread Jiri Olsa 
On Thu, May 29, 2014 at 09:02:36AM +0900, Namhyung Kim wrote:
> On Tue, 27 May 2014 09:37:38 +0200, Jiri Olsa wrote:
> > On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
> >> Hi Jiri,
> >> 
> >> On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
> >> 
> >> [SNIP]
> >> > +static void data_close(void)
> >> > +{
> >> > +bool cache_fd = may_cache_fd();
> >> > +
> >> > +if (!cache_fd)
> >> > +close_first_dso();
> >> > +}
> >> 
> >> Why do you do this at close()?  As long as there's no attempt to open a
> >> new file, we can keep existing fd, no?
> >
> > so the way it works now is:
> >
> >  - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
> >  - if we try to open dso and it fails, because we are out of
> >file descriptors, we close dso objects and try to reopen
> >(check do_open function)
> >  - when we close the dso object we check if number of opened
> >dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
> >the dso opened, if not we close first dso in the list
> >
> > util/dso.h tries to describe that
> 
> Yes, I know.  But my question is why do this at close()?  Isn't it
> sufficient to check the file limit at open() and close previous one if
> necessary?

hm, it's still the same operation to be done either in open
or in close.. but we would not need dso__data_close then..
ok, I'll make the change ;-)

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-29 Thread Jiri Olsa 
On Thu, May 29, 2014 at 09:02:36AM +0900, Namhyung Kim wrote:
 On Tue, 27 May 2014 09:37:38 +0200, Jiri Olsa wrote:
  On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
  Hi Jiri,
  
  On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
  
  [SNIP]
   +static void data_close(void)
   +{
   +bool cache_fd = may_cache_fd();
   +
   +if (!cache_fd)
   +close_first_dso();
   +}
  
  Why do you do this at close()?  As long as there's no attempt to open a
  new file, we can keep existing fd, no?
 
  so the way it works now is:
 
   - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
   - if we try to open dso and it fails, because we are out of
 file descriptors, we close dso objects and try to reopen
 (check do_open function)
   - when we close the dso object we check if number of opened
 dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
 the dso opened, if not we close first dso in the list
 
  util/dso.h tries to describe that
 
 Yes, I know.  But my question is why do this at close()?  Isn't it
 sufficient to check the file limit at open() and close previous one if
 necessary?

hm, it's still the same operation to be done either in open
or in close.. but we would not need dso__data_close then..
ok, I'll make the change ;-)

thanks,
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-28 Thread Namhyung Kim 
On Tue, 27 May 2014 09:37:38 +0200, Jiri Olsa wrote:
> On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
>> Hi Jiri,
>> 
>> On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
>> 
>> [SNIP]
>> > +static void data_close(void)
>> > +{
>> > +  bool cache_fd = may_cache_fd();
>> > +
>> > +  if (!cache_fd)
>> > +  close_first_dso();
>> > +}
>> 
>> Why do you do this at close()?  As long as there's no attempt to open a
>> new file, we can keep existing fd, no?
>
> so the way it works now is:
>
>  - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
>  - if we try to open dso and it fails, because we are out of
>file descriptors, we close dso objects and try to reopen
>(check do_open function)
>  - when we close the dso object we check if number of opened
>dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
>the dso opened, if not we close first dso in the list
>
> util/dso.h tries to describe that

Yes, I know.  But my question is why do this at close()?  Isn't it
sufficient to check the file limit at open() and close previous one if
necessary?

>
>> 
>> > +
>> > +void dso__data_close(struct dso *dso)
>> > +{
>> > +  if (dso->data.fd >= 0)
>> > +  data_close();
>> > +}
>> 
>> Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
>> than the given dso.  And this dso__data_close() is not paired with any
>> _open() also these close calls make me confusing which one to use. ;-p
>
> thats due to the caching.. as explained above
>
> About the pairing.. originally the interface was only dso__data_fd
> that opened and returned fd, which the caller needed to close.
>
> I added dso__data_close so we could keep track of file descriptors.
>
> I could add dso__data_open I guess, but it is dso__data_fd which is
> needed for elf interface anyway.

I'd rather suggest dropping the open/close idiom for this case since
it's confusing.  What about get/put or get_fd/put_fd?

Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-28 Thread Namhyung Kim 
On Tue, 27 May 2014 09:37:38 +0200, Jiri Olsa wrote:
 On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
 Hi Jiri,
 
 On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
 
 [SNIP]
  +static void data_close(void)
  +{
  +  bool cache_fd = may_cache_fd();
  +
  +  if (!cache_fd)
  +  close_first_dso();
  +}
 
 Why do you do this at close()?  As long as there's no attempt to open a
 new file, we can keep existing fd, no?

 so the way it works now is:

  - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
  - if we try to open dso and it fails, because we are out of
file descriptors, we close dso objects and try to reopen
(check do_open function)
  - when we close the dso object we check if number of opened
dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
the dso opened, if not we close first dso in the list

 util/dso.h tries to describe that

Yes, I know.  But my question is why do this at close()?  Isn't it
sufficient to check the file limit at open() and close previous one if
necessary?


 
  +
  +void dso__data_close(struct dso *dso)
  +{
  +  if (dso-data.fd = 0)
  +  data_close();
  +}
 
 Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
 than the given dso.  And this dso__data_close() is not paired with any
 _open() also these close calls make me confusing which one to use. ;-p

 thats due to the caching.. as explained above

 About the pairing.. originally the interface was only dso__data_fd
 that opened and returned fd, which the caller needed to close.

 I added dso__data_close so we could keep track of file descriptors.

 I could add dso__data_open I guess, but it is dso__data_fd which is
 needed for elf interface anyway.

I'd rather suggest dropping the open/close idiom for this case since
it's confusing.  What about get/put or get_fd/put_fd?

Thanks,
Namhyung
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-27 Thread Jiri Olsa 
On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
> 
> [SNIP]
> > +static void data_close(void)
> > +{
> > +   bool cache_fd = may_cache_fd();
> > +
> > +   if (!cache_fd)
> > +   close_first_dso();
> > +}
> 
> Why do you do this at close()?  As long as there's no attempt to open a
> new file, we can keep existing fd, no?

so the way it works now is:

 - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
 - if we try to open dso and it fails, because we are out of
   file descriptors, we close dso objects and try to reopen
   (check do_open function)
 - when we close the dso object we check if number of opened
   dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
   the dso opened, if not we close first dso in the list

util/dso.h tries to describe that

> 
> > +
> > +void dso__data_close(struct dso *dso)
> > +{
> > +   if (dso->data.fd >= 0)
> > +   data_close();
> > +}
> 
> Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
> than the given dso.  And this dso__data_close() is not paired with any
> _open() also these close calls make me confusing which one to use. ;-p

thats due to the caching.. as explained above

About the pairing.. originally the interface was only dso__data_fd
that opened and returned fd, which the caller needed to close.

I added dso__data_close so we could keep track of file descriptors.

I could add dso__data_open I guess, but it is dso__data_fd which is
needed for elf interface anyway.

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-27 Thread Jiri Olsa 
On Tue, May 27, 2014 at 10:05:28AM +0900, Namhyung Kim wrote:
 Hi Jiri,
 
 On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:
 
 [SNIP]
  +static void data_close(void)
  +{
  +   bool cache_fd = may_cache_fd();
  +
  +   if (!cache_fd)
  +   close_first_dso();
  +}
 
 Why do you do this at close()?  As long as there's no attempt to open a
 new file, we can keep existing fd, no?

so the way it works now is:

 - we keep up to the 'RLIMIT_NOFILE / 2' of open dso objects
 - if we try to open dso and it fails, because we are out of
   file descriptors, we close dso objects and try to reopen
   (check do_open function)
 - when we close the dso object we check if number of opened
   dso objects is below 'RLIMIT_NOFILE / 2'.. if it is, we keep
   the dso opened, if not we close first dso in the list

util/dso.h tries to describe that

 
  +
  +void dso__data_close(struct dso *dso)
  +{
  +   if (dso-data.fd = 0)
  +   data_close();
  +}
 
 Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
 than the given dso.  And this dso__data_close() is not paired with any
 _open() also these close calls make me confusing which one to use. ;-p

thats due to the caching.. as explained above

About the pairing.. originally the interface was only dso__data_fd
that opened and returned fd, which the caller needed to close.

I added dso__data_close so we could keep track of file descriptors.

I could add dso__data_open I guess, but it is dso__data_fd which is
needed for elf interface anyway.

thanks,
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-26 Thread Namhyung Kim 
Hi Jiri,

On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:

[SNIP]
> +static void data_close(void)
> +{
> + bool cache_fd = may_cache_fd();
> +
> + if (!cache_fd)
> + close_first_dso();
> +}

Why do you do this at close()?  As long as there's no attempt to open a
new file, we can keep existing fd, no?

> +
> +void dso__data_close(struct dso *dso)
> +{
> + if (dso->data.fd >= 0)
> + data_close();
> +}

Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
than the given dso.  And this dso__data_close() is not paired with any
_open() also these close calls make me confusing which one to use. ;-p


Thanks
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-26 Thread Namhyung Kim 
Hi Jiri,

On Thu, 15 May 2014 19:23:27 +0200, Jiri Olsa wrote:

[SNIP]
 +static void data_close(void)
 +{
 + bool cache_fd = may_cache_fd();
 +
 + if (!cache_fd)
 + close_first_dso();
 +}

Why do you do this at close()?  As long as there's no attempt to open a
new file, we can keep existing fd, no?

 +
 +void dso__data_close(struct dso *dso)
 +{
 + if (dso-data.fd = 0)
 + data_close();
 +}

Hmm.. it's confusing dso__data_close(dso) closes an other dso rather
than the given dso.  And this dso__data_close() is not paired with any
_open() also these close calls make me confusing which one to use. ;-p


Thanks
Namhyung
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-15 Thread Jiri Olsa 
Changing the dso__data_close to keep the dso data file
descriptor open if possible.

We keep dsos data file descriptors open until their
count reaches the half of the current fd open limit
(RLIMIT_NOFILE). In this case we close file descriptor
of the first opened dso object.

We've got speed up in dso__data_fd function.

Output from report over 10 GB data with DWARF unwind stacks:
(TODO fix perf diff)

  current code:
   0.16% perf  perf   [.] dso__data_fd
  change:
   no record of dso__data_fd

And we got some noticable overall speed up:
(perf perf stat -e '{cycles,instructions}:u' ...)

  current code:
   291,315,329,878  cycles ( +-  0.22% )
   391,763,485,304  instructions   ( +-  0.03% )

 180.742249687 seconds time elapsed( +-  0.64% )

  change:
   228,948,088,958  cycles ( +-  0.20% )
   324,936,504,336  instructions   ( +-  0.02% )

 133.789677792 seconds time elapsed( +-  0.76% )

Cc: Arnaldo Carvalho de Melo 
Cc: Corey Ashford 
Cc: David Ahern 
Cc: Frederic Weisbecker 
Cc: Ingo Molnar 
Cc: Jean Pihet 
Cc: Namhyung Kim 
Cc: Paul Mackerras 
Cc: Peter Zijlstra 
Signed-off-by: Jiri Olsa 
---
 tools/perf/util/dso.c | 62 ---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 76e5c13..316e087 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1,4 +1,6 @@
 #include 
+#include 
+#include 
 #include "symbol.h"
 #include "dso.h"
 #include "machine.h"
@@ -204,11 +206,60 @@ static void close_dso(struct dso *dso)
close_data_fd(dso);
 }
 
-void dso__data_close(struct dso *dso)
+static void close_first_dso(void)
 {
+   struct dso *dso;
+
+   dso = list_first_entry(__data_open, struct dso, data.open_entry);
close_dso(dso);
 }
 
+static rlim_t get_fd_limit(void)
+{
+   struct rlimit l;
+   rlim_t limit = 0;
+
+   /* Allow half of the current open fd limit. */
+   if (getrlimit(RLIMIT_NOFILE, ) == 0) {
+   if (l.rlim_cur == RLIM_INFINITY)
+   limit = l.rlim_cur;
+   else
+   limit = l.rlim_cur / 2;
+   } else {
+   pr_err("failed to get fd limit\n");
+   limit = 1;
+   }
+
+   return limit;
+}
+
+static bool may_cache_fd(void)
+{
+   static rlim_t limit;
+
+   if (!limit)
+   limit = get_fd_limit();
+
+   if (limit == RLIM_INFINITY)
+   return true;
+
+   return limit > (rlim_t) dso__data_open_cnt;
+}
+
+static void data_close(void)
+{
+   bool cache_fd = may_cache_fd();
+
+   if (!cache_fd)
+   close_first_dso();
+}
+
+void dso__data_close(struct dso *dso)
+{
+   if (dso->data.fd >= 0)
+   data_close();
+}
+
 int dso__data_fd(struct dso *dso, struct machine *machine)
 {
enum dso_binary_type binary_type_data[] = {
@@ -318,6 +369,7 @@ static ssize_t
 dso_cache__read(struct dso *dso, struct machine *machine,
 u64 offset, u8 *data, ssize_t size)
 {
+   bool new_fd = dso->data.fd == -1;
struct dso_cache *cache;
ssize_t ret;
int fd;
@@ -356,7 +408,10 @@ dso_cache__read(struct dso *dso, struct machine *machine,
if (ret <= 0)
free(cache);
 
-   dso__data_close(dso);
+   /* Check the cache only if we just added new fd. */
+   if (new_fd)
+   data_close();
+
return ret;
 }
 
@@ -563,7 +618,8 @@ void dso__delete(struct dso *dso)
dso->long_name_allocated = false;
}
 
-   dso__data_close(dso);
+   if (dso->data.fd != -1)
+   close_dso(dso);
dso_cache__free(>data.cache);
dso__free_a2l(dso);
zfree(>symsrc_filename);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 06/14] perf tools: Cache dso data file descriptor

2014-05-15 Thread Jiri Olsa 
Changing the dso__data_close to keep the dso data file
descriptor open if possible.

We keep dsos data file descriptors open until their
count reaches the half of the current fd open limit
(RLIMIT_NOFILE). In this case we close file descriptor
of the first opened dso object.

We've got speed up in dso__data_fd function.

Output from report over 10 GB data with DWARF unwind stacks:
(TODO fix perf diff)

  current code:
   0.16% perf  perf   [.] dso__data_fd
  change:
   no record of dso__data_fd

And we got some noticable overall speed up:
(perf perf stat -e '{cycles,instructions}:u' ...)

  current code:
   291,315,329,878  cycles ( +-  0.22% )
   391,763,485,304  instructions   ( +-  0.03% )

 180.742249687 seconds time elapsed( +-  0.64% )

  change:
   228,948,088,958  cycles ( +-  0.20% )
   324,936,504,336  instructions   ( +-  0.02% )

 133.789677792 seconds time elapsed( +-  0.76% )

Cc: Arnaldo Carvalho de Melo a...@kernel.org
Cc: Corey Ashford cjash...@linux.vnet.ibm.com
Cc: David Ahern dsah...@gmail.com
Cc: Frederic Weisbecker fweis...@gmail.com
Cc: Ingo Molnar mi...@kernel.org
Cc: Jean Pihet jean.pi...@linaro.org
Cc: Namhyung Kim namhy...@kernel.org
Cc: Paul Mackerras pau...@samba.org
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Signed-off-by: Jiri Olsa jo...@kernel.org
---
 tools/perf/util/dso.c | 62 ---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 76e5c13..316e087 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1,4 +1,6 @@
 #include asm/bug.h
+#include sys/time.h
+#include sys/resource.h
 #include symbol.h
 #include dso.h
 #include machine.h
@@ -204,11 +206,60 @@ static void close_dso(struct dso *dso)
close_data_fd(dso);
 }
 
-void dso__data_close(struct dso *dso)
+static void close_first_dso(void)
 {
+   struct dso *dso;
+
+   dso = list_first_entry(dso__data_open, struct dso, data.open_entry);
close_dso(dso);
 }
 
+static rlim_t get_fd_limit(void)
+{
+   struct rlimit l;
+   rlim_t limit = 0;
+
+   /* Allow half of the current open fd limit. */
+   if (getrlimit(RLIMIT_NOFILE, l) == 0) {
+   if (l.rlim_cur == RLIM_INFINITY)
+   limit = l.rlim_cur;
+   else
+   limit = l.rlim_cur / 2;
+   } else {
+   pr_err(failed to get fd limit\n);
+   limit = 1;
+   }
+
+   return limit;
+}
+
+static bool may_cache_fd(void)
+{
+   static rlim_t limit;
+
+   if (!limit)
+   limit = get_fd_limit();
+
+   if (limit == RLIM_INFINITY)
+   return true;
+
+   return limit  (rlim_t) dso__data_open_cnt;
+}
+
+static void data_close(void)
+{
+   bool cache_fd = may_cache_fd();
+
+   if (!cache_fd)
+   close_first_dso();
+}
+
+void dso__data_close(struct dso *dso)
+{
+   if (dso-data.fd = 0)
+   data_close();
+}
+
 int dso__data_fd(struct dso *dso, struct machine *machine)
 {
enum dso_binary_type binary_type_data[] = {
@@ -318,6 +369,7 @@ static ssize_t
 dso_cache__read(struct dso *dso, struct machine *machine,
 u64 offset, u8 *data, ssize_t size)
 {
+   bool new_fd = dso-data.fd == -1;
struct dso_cache *cache;
ssize_t ret;
int fd;
@@ -356,7 +408,10 @@ dso_cache__read(struct dso *dso, struct machine *machine,
if (ret = 0)
free(cache);
 
-   dso__data_close(dso);
+   /* Check the cache only if we just added new fd. */
+   if (new_fd)
+   data_close();
+
return ret;
 }
 
@@ -563,7 +618,8 @@ void dso__delete(struct dso *dso)
dso-long_name_allocated = false;
}
 
-   dso__data_close(dso);
+   if (dso-data.fd != -1)
+   close_dso(dso);
dso_cache__free(dso-data.cache);
dso__free_a2l(dso);
zfree(dso-symsrc_filename);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/