Dmatest Behavior

Commit Message

Sinan Kaya May 19, 2016, 5:39 p.m. UTC

This is what I put together yesterday. I also want to show the amount of time spent
in DMA map and unmap calls and remove it from the runtime cost too. 

Comments welcome.

>> I have testers and customers opening a bug case to me saying that we are
>> not reaching to performance numbers advertised. I have to carefully explain
>> them that the test is not correct and they are resisting.
> 
> Ah, sorry about that.  If they're reading this thread, "Listen to Sinan!!!"
> 
>> I want to remove this ambiguity.
> 
> Sounds good to me.
> 


From ec9f4480fac1648e9f53112b143a535774e3795d Mon Sep 17 00:00:00 2001
From: Sinan Kaya <[e-mail]>
Date: Wed, 18 May 2016 16:57:15 -0400
Subject: [PATCH] dmatest: exclude compare and fill time during performance
 report

Change-Id: I3428a85c3a58f990b985fc10fe736dfb3e56b206
Signed-off-by: Sinan Kaya <[e-mail]>
---
 drivers/dma/dmatest.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Vinod Koul May 30, 2016, 5:11 a.m. UTC | #1

On Thu, May 19, 2016 at 01:39:58PM -0400, Sinan Kaya wrote:
 
> From ec9f4480fac1648e9f53112b143a535774e3795d Mon Sep 17 00:00:00 2001
> From: Sinan Kaya <[e-mail]>
> Date: Wed, 18 May 2016 16:57:15 -0400
> Subject: [PATCH] dmatest: exclude compare and fill time during performance
>  report
> 
> Change-Id: I3428a85c3a58f990b985fc10fe736dfb3e56b206

We dont want these here...

> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -416,7 +416,9 @@ static int dmatest_func(void *data)
>  	int			src_cnt;
>  	int			dst_cnt;
>  	int			i;
> -	ktime_t			ktime;
> +	ktime_t			ktime, start, diff;
> +	ktime_t			filltime = ktime_set(0, 0);
> +	ktime_t			comparetime = ktime_set(0, 0);
>  	s64			runtime = 0;
>  	unsigned long long	total_len = 0;
>  
> @@ -517,6 +519,7 @@ static int dmatest_func(void *data)
>  			src_off = 0;
>  			dst_off = 0;
>  		} else {
> +			start = ktime_get();
>  			src_off = dmatest_random() % (params->buf_size - len + 1);
>  			dst_off = dmatest_random() % (params->buf_size - len + 1);
>  
> @@ -527,6 +530,9 @@ static int dmatest_func(void *data)
>  					  params->buf_size);
>  			dmatest_init_dsts(thread->dsts, dst_off, len,
>  					  params->buf_size);
> +
> +			diff = ktime_sub(ktime_get(), start);

If you are very picky about perf, shouldn't we just track time here and then
later do subtraction to get diff

Sinan Kaya May 30, 2016, 8:58 p.m. UTC | #2

On 5/30/2016 1:11 AM, Vinod Koul wrote:
> On Thu, May 19, 2016 at 01:39:58PM -0400, Sinan Kaya wrote:
>  
>> From ec9f4480fac1648e9f53112b143a535774e3795d Mon Sep 17 00:00:00 2001
>> From: Sinan Kaya <[e-mail]>
>> Date: Wed, 18 May 2016 16:57:15 -0400
>> Subject: [PATCH] dmatest: exclude compare and fill time during performance
>>  report
>>
>> Change-Id: I3428a85c3a58f990b985fc10fe736dfb3e56b206
> 
> We dont want these here...
> 

Sure,

>> --- a/drivers/dma/dmatest.c
>> +++ b/drivers/dma/dmatest.c
>> @@ -416,7 +416,9 @@ static int dmatest_func(void *data)
>>  	int			src_cnt;
>>  	int			dst_cnt;
>>  	int			i;
>> -	ktime_t			ktime;
>> +	ktime_t			ktime, start, diff;
>> +	ktime_t			filltime = ktime_set(0, 0);
>> +	ktime_t			comparetime = ktime_set(0, 0);
>>  	s64			runtime = 0;
>>  	unsigned long long	total_len = 0;
>>  
>> @@ -517,6 +519,7 @@ static int dmatest_func(void *data)
>>  			src_off = 0;
>>  			dst_off = 0;
>>  		} else {
>> +			start = ktime_get();
>>  			src_off = dmatest_random() % (params->buf_size - len + 1);
>>  			dst_off = dmatest_random() % (params->buf_size - len + 1);
>>  
>> @@ -527,6 +530,9 @@ static int dmatest_func(void *data)
>>  					  params->buf_size);
>>  			dmatest_init_dsts(thread->dsts, dst_off, len,
>>  					  params->buf_size);
>> +
>> +			diff = ktime_sub(ktime_get(), start);
> 
> If you are very picky about perf, shouldn't we just track time here and then
> later do subtraction to get diff
> 

That's what I was originally doing. I realized that the loop gets called multiple
times before reaching to final point where the actual performance is measured. 

I'm trying to account for multiple fills and compares by using a stack local variable.
Then, I deduct the final result.

Patch

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index b8576fd..0b3e1f9 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -416,7 +416,9 @@  static int dmatest_func(void *data)
 	int			src_cnt;
 	int			dst_cnt;
 	int			i;
-	ktime_t			ktime;
+	ktime_t			ktime, start, diff;
+	ktime_t			filltime = ktime_set(0, 0);
+	ktime_t			comparetime = ktime_set(0, 0);
 	s64			runtime = 0;
 	unsigned long long	total_len = 0;
 
@@ -517,6 +519,7 @@  static int dmatest_func(void *data)
 			src_off = 0;
 			dst_off = 0;
 		} else {
+			start = ktime_get();
 			src_off = dmatest_random() % (params->buf_size - len + 1);
 			dst_off = dmatest_random() % (params->buf_size - len + 1);
 
@@ -527,6 +530,9 @@  static int dmatest_func(void *data)
 					  params->buf_size);
 			dmatest_init_dsts(thread->dsts, dst_off, len,
 					  params->buf_size);
+
+			diff = ktime_sub(ktime_get(), start);
+			filltime = ktime_add(filltime, diff);
 		}
 
 		um = dmaengine_get_unmap_data(dev->dev, src_cnt+dst_cnt,
@@ -657,6 +663,7 @@  static int dmatest_func(void *data)
 			continue;
 		}
 
+		start = ktime_get();
 		pr_debug("%s: verifying source buffer...\n", current->comm);
 		error_count = dmatest_verify(thread->srcs, 0, src_off,
 				0, PATTERN_SRC, true);
@@ -677,6 +684,9 @@  static int dmatest_func(void *data)
 				params->buf_size, dst_off + len,
 				PATTERN_DST, false);
 
+		diff = ktime_sub(ktime_get(), start);
+		comparetime = ktime_add(comparetime, diff);
+
 		if (error_count) {
 			result("data error", total_tests, src_off, dst_off,
 			       len, error_count);
@@ -686,7 +696,10 @@  static int dmatest_func(void *data)
 				       dst_off, len, 0);
 		}
 	}
-	runtime = ktime_us_delta(ktime_get(), ktime);
+	ktime = ktime_sub(ktime_get(), ktime);
+	ktime = ktime_sub(ktime, comparetime);
+	ktime = ktime_sub(ktime, filltime);
+	runtime = ktime_to_us(ktime);
 
 	ret = 0;
 err_dstbuf: