Re: [PATCH] perf tool: Don't discard prev in backward mode

Re: [PATCH] perf tool: Don't discard prev in backward mode

[Wangnan (F)]

Hi Kan, Please see if this patch works for you.


On 2017/10/14 7:16, Wang Nan wrote:
> Perf record can switch output. The new output should only store the
> data after switching. However, in overwrite backward mode, the new
> output still have the data from old output. That also brings extra overhead.
>
> At the end of mmap_read, the position of processed ring buffer is
> saved in md->prev. Next mmap_read should be end in md->prev if it is
> not overwriten. That avoids to process duplicate data.
> However, the md->prev is discarded. So next mmap_read has to process
> whole valid ring buffer, which probably include the old processed
> data.
>
> Avoid calling backward_rb_find_range() when md->prev is still
> available.
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Liang Kan <kan.liang@intel.com>
> ---
>   tools/perf/util/mmap.c | 33 +++++++++++++++------------------
>   1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index 9fe5f9c..df1de55 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -287,18 +287,6 @@ static int backward_rb_find_range(void *buf, int mask, u64 head, u64 *start, u64
>   	return -1;
>   }
>   
> -static int rb_find_range(void *data, int mask, u64 head, u64 old,
> -			 u64 *start, u64 *end, bool backward)
> -{
> -	if (!backward) {
> -		*start = old;
> -		*end = head;
> -		return 0;
> -	}
> -
> -	return backward_rb_find_range(data, mask, head, start, end);
> -}
> -
>   int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool backward,
>   		    void *to, int push(void *to, void *buf, size_t size))
>   {
> @@ -310,19 +298,28 @@ int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool backward,
>   	void *buf;
>   	int rc = 0;
>   
> -	if (rb_find_range(data, md->mask, head, old, &start, &end, backward))
> -		return -1;
> +	start = backward ? head : old;
> +	end = backward ? old : head;
>   
>   	if (start == end)
>   		return 0;
>   
>   	size = end - start;
>   	if (size > (unsigned long)(md->mask) + 1) {
> -		WARN_ONCE(1, "failed to keep up with mmap data. (warn only once)\n");
> +		if (!backward) {
> +			WARN_ONCE(1, "failed to keep up with mmap data. (warn only once)\n");
>   
> -		md->prev = head;
> -		perf_mmap__consume(md, overwrite || backward);
> -		return 0;
> +			md->prev = head;
> +			perf_mmap__consume(md, overwrite || backward);
> +			return 0;
> +		}
> +
> +		/*
> +		 * Backward ring buffer is full. We still have a chance to read
> +		 * most of data from it.
> +		 */
> +		if (backward_rb_find_range(data, md->mask, head, &start, &end))
> +			return -1;
>   	}
>   
>   	if ((start & md->mask) + size != (end & md->mask)) {

RE: [PATCH] perf tool: Don't discard prev in backward mode

[Liang, Kan]

> Perf record can switch output. The new output should only store the data
> after switching. However, in overwrite backward mode, the new output still
> have the data from old output. That also brings extra overhead.
> 
> At the end of mmap_read, the position of processed ring buffer is saved in
> md->prev. Next mmap_read should be end in md->prev if it is not overwriten.
> That avoids to process duplicate data.
> However, the md->prev is discarded. So next mmap_read has to process
> whole valid ring buffer, which probably include the old processed data.
> 
> Avoid calling backward_rb_find_range() when md->prev is still available.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Liang Kan <kan.liang@intel.com>
> ---

The patch looks good to me.

Tested-by: Kan Liang <kan.liang@intel.com>


>  tools/perf/util/mmap.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c index
> 9fe5f9c..df1de55 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -287,18 +287,6 @@ static int backward_rb_find_range(void *buf, int
> mask, u64 head, u64 *start, u64
>  	return -1;
>  }
> 
> -static int rb_find_range(void *data, int mask, u64 head, u64 old,
> -			 u64 *start, u64 *end, bool backward)
> -{
> -	if (!backward) {
> -		*start = old;
> -		*end = head;
> -		return 0;
> -	}
> -
> -	return backward_rb_find_range(data, mask, head, start, end);
> -}
> -
>  int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool
> backward,
>  		    void *to, int push(void *to, void *buf, size_t size))  { @@ -
> 310,19 +298,28 @@ int perf_mmap__push(struct perf_mmap *md, bool
> overwrite, bool backward,
>  	void *buf;
>  	int rc = 0;
> 
> -	if (rb_find_range(data, md->mask, head, old, &start, &end,
> backward))
> -		return -1;
> +	start = backward ? head : old;
> +	end = backward ? old : head;
> 
>  	if (start == end)
>  		return 0;
> 
>  	size = end - start;
>  	if (size > (unsigned long)(md->mask) + 1) {
> -		WARN_ONCE(1, "failed to keep up with mmap data. (warn
> only once)\n");
> +		if (!backward) {
> +			WARN_ONCE(1, "failed to keep up with mmap data.
> (warn only
> +once)\n");
> 
> -		md->prev = head;
> -		perf_mmap__consume(md, overwrite || backward);
> -		return 0;
> +			md->prev = head;
> +			perf_mmap__consume(md, overwrite || backward);
> +			return 0;
> +		}
> +
> +		/*
> +		 * Backward ring buffer is full. We still have a chance to read
> +		 * most of data from it.
> +		 */
> +		if (backward_rb_find_range(data, md->mask, head, &start,
> &end))
> +			return -1;
>  	}
> 
>  	if ((start & md->mask) + size != (end & md->mask)) {
> --
> 2.9.3

[PATCH] perf tool: Don't discard prev in backward mode

[Wang Nan]

Perf record can switch output. The new output should only store the
data after switching. However, in overwrite backward mode, the new
output still have the data from old output. That also brings extra overhead.

At the end of mmap_read, the position of processed ring buffer is
saved in md->prev. Next mmap_read should be end in md->prev if it is
not overwriten. That avoids to process duplicate data.
However, the md->prev is discarded. So next mmap_read has to process
whole valid ring buffer, which probably include the old processed
data.

Avoid calling backward_rb_find_range() when md->prev is still
available.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Liang Kan <kan.liang@intel.com>
---
 tools/perf/util/mmap.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
index 9fe5f9c..df1de55 100644
--- a/tools/perf/util/mmap.c
+++ b/tools/perf/util/mmap.c
@@ -287,18 +287,6 @@ static int backward_rb_find_range(void *buf, int mask, u64 head, u64 *start, u64
 	return -1;
 }
 
-static int rb_find_range(void *data, int mask, u64 head, u64 old,
-			 u64 *start, u64 *end, bool backward)
-{
-	if (!backward) {
-		*start = old;
-		*end = head;
-		return 0;
-	}
-
-	return backward_rb_find_range(data, mask, head, start, end);
-}
-
 int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool backward,
 		    void *to, int push(void *to, void *buf, size_t size))
 {
@@ -310,19 +298,28 @@ int perf_mmap__push(struct perf_mmap *md, bool overwrite, bool backward,
 	void *buf;
 	int rc = 0;
 
-	if (rb_find_range(data, md->mask, head, old, &start, &end, backward))
-		return -1;
+	start = backward ? head : old;
+	end = backward ? old : head;
 
 	if (start == end)
 		return 0;
 
 	size = end - start;
 	if (size > (unsigned long)(md->mask) + 1) {
-		WARN_ONCE(1, "failed to keep up with mmap data. (warn only once)\n");
+		if (!backward) {
+			WARN_ONCE(1, "failed to keep up with mmap data. (warn only once)\n");
 
-		md->prev = head;
-		perf_mmap__consume(md, overwrite || backward);
-		return 0;
+			md->prev = head;
+			perf_mmap__consume(md, overwrite || backward);
+			return 0;
+		}
+
+		/*
+		 * Backward ring buffer is full. We still have a chance to read
+		 * most of data from it.
+		 */
+		if (backward_rb_find_range(data, md->mask, head, &start, &end))
+			return -1;
 	}
 
 	if ((start & md->mask) + size != (end & md->mask)) {
-- 
2.9.3