[dpdk-dev] [PATCH] eal: roundup tsc frequency when estimating it

[dpdk-dev] [PATCH] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh]

When estimating tsc frequency using sleep/gettime round it up to the
nearest multiple of 10Mhz for more accuracy.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 lib/librte_eal/common/eal_common_timer.c   |  4 ++--
 lib/librte_eal/common/include/rte_common.h | 10 ++++++++++
 lib/librte_eal/linuxapp/eal/eal_timer.c    |  2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index dcf26bfea..1358bbed0 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -69,7 +69,7 @@ estimate_tsc_freq(void)
 	/* assume that the sleep(1) will sleep for 1 second */
 	uint64_t start = rte_rdtsc();
 	sleep(1);
-	return rte_rdtsc() - start;
+	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
 }
 
 void
@@ -83,7 +83,7 @@ set_tsc_freq(void)
 	if (!freq)
 		freq = estimate_tsc_freq();
 
-	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
+	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
 	eal_tsc_resolution_hz = freq;
 }
 
diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 66cdf60b2..e374b16b1 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -248,6 +248,16 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define RTE_ALIGN_MUL_FLOOR(v, mul) \
 	((v / ((typeof(v))(mul))) * (typeof(v))(mul))
 
+/**
+ * Macro to align a value to the nearest multiple of given value.
+ */
+#define RTE_ALIGN_MUL_NEAR(v, mul)				\
+	({							\
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul);	\
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul);	\
+		(ceil - v) > (v - floor) ? floor: ceil;		\
+	})
+
 /**
  * Checks if a pointer is aligned to a given power-of-two value
  *
diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index bc8f05199..864d6ef29 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -248,7 +248,7 @@ get_tsc_freq(void)
 
 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
-		return tsc_hz;
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
 	}
 #endif
 	return 0;
-- 
2.19.2

Re: [dpdk-dev] [PATCH] eal: roundup tsc frequency when estimating it

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 29 Nov 2018 14:02:03 +0530
> From: "Bhagavatula, Pavan" <Pavan.Bhagavatula@cavium.com>
> To: "Jacob,  Jerin" <Jerin.JacobKollanukkaran@cavium.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Bhagavatula, Pavan"
>  <Pavan.Bhagavatula@cavium.com>
> Subject: [dpdk-dev] [PATCH] eal: roundup tsc frequency when estimating it
> 
> When estimating tsc frequency using sleep/gettime round it up to the
> nearest multiple of 10Mhz for more accuracy.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> ---
>  lib/librte_eal/common/eal_common_timer.c   |  4 ++--
>  lib/librte_eal/common/include/rte_common.h | 10 ++++++++++
>  lib/librte_eal/linuxapp/eal/eal_timer.c    |  2 +-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
> index dcf26bfea..1358bbed0 100644
> --- a/lib/librte_eal/common/eal_common_timer.c
> +++ b/lib/librte_eal/common/eal_common_timer.c
> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
>  	/* assume that the sleep(1) will sleep for 1 second */
>  	uint64_t start = rte_rdtsc();
>  	sleep(1);
> -	return rte_rdtsc() - start;
> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
>  }
>  
>  void
> @@ -83,7 +83,7 @@ set_tsc_freq(void)
>  	if (!freq)
>  		freq = estimate_tsc_freq();
>  
> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
>  	eal_tsc_resolution_hz = freq;
>  }
>  
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index 66cdf60b2..e374b16b1 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -248,6 +248,16 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  #define RTE_ALIGN_MUL_FLOOR(v, mul) \
>  	((v / ((typeof(v))(mul))) * (typeof(v))(mul))
>  
> +/**
> + * Macro to align a value to the nearest multiple of given value.
> + */
> +#define RTE_ALIGN_MUL_NEAR(v, mul)				\
> +	({							\
> +		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul);	\
> +		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul);	\
> +		(ceil - v) > (v - floor) ? floor: ceil;		\
> +	})
> +

Probably it is better to have two patches.
First patch to add new API along with unit testcase.
Second patch to roundup tsc frequency when estimating it.


>  /**
>   * Checks if a pointer is aligned to a given power-of-two value
>   *
> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
> index bc8f05199..864d6ef29 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> @@ -248,7 +248,7 @@ get_tsc_freq(void)
>  
>  		double secs = (double)ns/NS_PER_SEC;
>  		tsc_hz = (uint64_t)((end - start)/secs);
> -		return tsc_hz;
> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
>  	}
>  #endif
>  	return 0;
> -- 
> 2.19.2
> 

Re: [dpdk-dev] [PATCH] eal: roundup tsc frequency when estimating it

[Stephen Hemminger]

On Thu, 29 Nov 2018 08:32:03 +0000
Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> wrote:

> When estimating tsc frequency using sleep/gettime round it up to the
> nearest multiple of 10Mhz for more accuracy.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

Rounding reduces accuracy.

Why is this code being used?  Shouldn't get_tsc_freq_arch return a
correct value?

How well does the rdmsr() logic work in VM?
It looks like Hyper-V has special MSR's for TSC frequency determination.

Re: [dpdk-dev] [PATCH] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh]

Hi Stephen,

On Thu, Nov 29, 2018 at 01:21:52PM -0800, Stephen Hemminger wrote:
> On Thu, 29 Nov 2018 08:32:03 +0000
> Pavan Nikhilesh <pbhagavatula@caviumnetworks.com> wrote:
>
> > When estimating tsc frequency using sleep/gettime round it up to the
> > nearest multiple of 10Mhz for more accuracy.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
>
> Rounding reduces accuracy.
>
> Why is this code being used?  Shouldn't get_tsc_freq_arch return a
> correct value?

This patch doesn't modify get_tsc_freq_arch(), it basically gives a more
accurate freq reading when we rely on sleep(1) i.e. only when
get_tsc_freq_arch() returns 0.

example:

static uint64_t
estimate_tsc_freq(void)
{
        RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
                " - clock timings may be less accurate.\n");
        /* assume that the sleep(1) will sleep for 1 second */
        uint64_t start = rte_rdtsc();
        sleep(1);
        return rte_rdtsc() - start;
}

This will not give the accurate cyc/sec in most cases, rounding it to 10Mhz wil
do the job.

In case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU, get_tsc_freq_arch()
will return 0 as there is no instruction to determine the clk of PMU.

>
> How well does the rdmsr() logic work in VM?
> It looks like Hyper-V has special MSR's for TSC frequency determination.

Maybe bruce can give a more accurate answer to this as it is x86 specific.

Thanks,
Pavan.

[dpdk-dev] [PATCH v2 1/2] eal: add macro to align value to the nearest multiple

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add macro to align value to the nearest multiple of the given value,
resultant value might be greater than or less than the first parameter
whichever difference is the lowest.
Update unit test to include the new macro.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 v2 Changes:
 - Spilt patch and add unit test for the new macro.

 app/test/test_common.c                     |  4 ++++
 lib/librte_eal/common/include/rte_common.h | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 94d367471..2b856f8ba 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -199,6 +199,10 @@ test_align(void)
 			val = RTE_ALIGN_MUL_FLOOR(i, p);
 			if (val % p != 0 || val > i)
 				FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
+			val = RTE_ALIGN_MUL_NEAR(i, p);
+			if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
+				& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
+				FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
 		}
 	}

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 7178ba1e9..bcf8afd39 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -248,6 +248,18 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define RTE_ALIGN_MUL_FLOOR(v, mul) \
 	((v / ((typeof(v))(mul))) * (typeof(v))(mul))

+/**
+ * Macro to align value to the nearest multiple of the given value.
+ * The resultant value might be greater than or less than the first parameter
+ * whichever difference is the lowest.
+ */
+#define RTE_ALIGN_MUL_NEAR(v, mul)				\
+	({							\
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul);	\
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul);	\
+		(ceil - v) > (v - floor) ? floor : ceil;	\
+	})
+
 /**
  * Checks if a pointer is aligned to a given power-of-two value
  *
--
2.21.0

[dpdk-dev] [PATCH v2 1/2] eal: add macro to align value to the nearest multiple

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add macro to align value to the nearest multiple of the given value,
resultant value might be greater than or less than the first parameter
whichever difference is the lowest.
Update unit test to include the new macro.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 v2 Changes:
 - Spilt patch and add unit test for the new macro.

 app/test/test_common.c                     |  4 ++++
 lib/librte_eal/common/include/rte_common.h | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 94d367471..2b856f8ba 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -199,6 +199,10 @@ test_align(void)
 			val = RTE_ALIGN_MUL_FLOOR(i, p);
 			if (val % p != 0 || val > i)
 				FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
+			val = RTE_ALIGN_MUL_NEAR(i, p);
+			if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
+				& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
+				FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
 		}
 	}

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 7178ba1e9..bcf8afd39 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -248,6 +248,18 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define RTE_ALIGN_MUL_FLOOR(v, mul) \
 	((v / ((typeof(v))(mul))) * (typeof(v))(mul))

+/**
+ * Macro to align value to the nearest multiple of the given value.
+ * The resultant value might be greater than or less than the first parameter
+ * whichever difference is the lowest.
+ */
+#define RTE_ALIGN_MUL_NEAR(v, mul)				\
+	({							\
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul);	\
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul);	\
+		(ceil - v) > (v - floor) ? floor : ceil;	\
+	})
+
 /**
  * Checks if a pointer is aligned to a given power-of-two value
  *
--
2.21.0

[dpdk-dev] [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

When estimating tsc frequency using sleep/gettime round it up to the
nearest multiple of 10Mhz for more accuracy.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
 get_tsc_freq_arch() will return 0 as there is no instruction to determine
 the clk of PMU and eal falls back to sleep(1).

 lib/librte_eal/common/eal_common_timer.c | 4 ++--
 lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index dcf26bfea..1358bbed0 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -69,7 +69,7 @@ estimate_tsc_freq(void)
 	/* assume that the sleep(1) will sleep for 1 second */
 	uint64_t start = rte_rdtsc();
 	sleep(1);
-	return rte_rdtsc() - start;
+	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
 }

 void
@@ -83,7 +83,7 @@ set_tsc_freq(void)
 	if (!freq)
 		freq = estimate_tsc_freq();

-	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
+	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
 	eal_tsc_resolution_hz = freq;
 }

diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index bc8f05199..864d6ef29 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -248,7 +248,7 @@ get_tsc_freq(void)

 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
-		return tsc_hz;
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
 	}
 #endif
 	return 0;
--
2.21.0

[dpdk-dev] [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

When estimating tsc frequency using sleep/gettime round it up to the
nearest multiple of 10Mhz for more accuracy.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
 get_tsc_freq_arch() will return 0 as there is no instruction to determine
 the clk of PMU and eal falls back to sleep(1).

 lib/librte_eal/common/eal_common_timer.c | 4 ++--
 lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index dcf26bfea..1358bbed0 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -69,7 +69,7 @@ estimate_tsc_freq(void)
 	/* assume that the sleep(1) will sleep for 1 second */
 	uint64_t start = rte_rdtsc();
 	sleep(1);
-	return rte_rdtsc() - start;
+	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
 }

 void
@@ -83,7 +83,7 @@ set_tsc_freq(void)
 	if (!freq)
 		freq = estimate_tsc_freq();

-	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
+	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
 	eal_tsc_resolution_hz = freq;
 }

diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index bc8f05199..864d6ef29 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -248,7 +248,7 @@ get_tsc_freq(void)

 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
-		return tsc_hz;
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
 	}
 #endif
 	return 0;
--
2.21.0

Re: [dpdk-dev] [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Wiles, Keith]

> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> When estimating tsc frequency using sleep/gettime round it up to the
> nearest multiple of 10Mhz for more accuracy.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> get_tsc_freq_arch() will return 0 as there is no instruction to determine
> the clk of PMU and eal falls back to sleep(1).
> 
> lib/librte_eal/common/eal_common_timer.c | 4 ++--
> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
> index dcf26bfea..1358bbed0 100644
> --- a/lib/librte_eal/common/eal_common_timer.c
> +++ b/lib/librte_eal/common/eal_common_timer.c
> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> 	/* assume that the sleep(1) will sleep for 1 second */
> 	uint64_t start = rte_rdtsc();
> 	sleep(1);
> -	return rte_rdtsc() - start;
> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> }
> 
> void
> @@ -83,7 +83,7 @@ set_tsc_freq(void)
> 	if (!freq)
> 		freq = estimate_tsc_freq();
> 
> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
> 	eal_tsc_resolution_hz = freq;
> }
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
> index bc8f05199..864d6ef29 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> @@ -248,7 +248,7 @@ get_tsc_freq(void)
> 
> 		double secs = (double)ns/NS_PER_SEC;
> 		tsc_hz = (uint64_t)((end - start)/secs);
> -		return tsc_hz;
> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);

Maybe I missed an email about this, but why would I want the TSC hz rounded here? I do not mind the macro just the fact that we are changing TSC hz value. If the TSC value is wrong then we need to fix the value, but I do not see it being wrong here.
> 	}
> #endif
> 	return 0;
> --
> 2.21.0
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Wiles, Keith]

> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> When estimating tsc frequency using sleep/gettime round it up to the
> nearest multiple of 10Mhz for more accuracy.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> get_tsc_freq_arch() will return 0 as there is no instruction to determine
> the clk of PMU and eal falls back to sleep(1).
> 
> lib/librte_eal/common/eal_common_timer.c | 4 ++--
> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
> index dcf26bfea..1358bbed0 100644
> --- a/lib/librte_eal/common/eal_common_timer.c
> +++ b/lib/librte_eal/common/eal_common_timer.c
> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> 	/* assume that the sleep(1) will sleep for 1 second */
> 	uint64_t start = rte_rdtsc();
> 	sleep(1);
> -	return rte_rdtsc() - start;
> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> }
> 
> void
> @@ -83,7 +83,7 @@ set_tsc_freq(void)
> 	if (!freq)
> 		freq = estimate_tsc_freq();
> 
> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
> 	eal_tsc_resolution_hz = freq;
> }
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
> index bc8f05199..864d6ef29 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> @@ -248,7 +248,7 @@ get_tsc_freq(void)
> 
> 		double secs = (double)ns/NS_PER_SEC;
> 		tsc_hz = (uint64_t)((end - start)/secs);
> -		return tsc_hz;
> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);

Maybe I missed an email about this, but why would I want the TSC hz rounded here? I do not mind the macro just the fact that we are changing TSC hz value. If the TSC value is wrong then we need to fix the value, but I do not see it being wrong here.
> 	}
> #endif
> 	return 0;
> --
> 2.21.0
> 

Regards,
Keith

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > 
> > When estimating tsc frequency using sleep/gettime round it up to
> > the
> > nearest multiple of 10Mhz for more accuracy.
> > 
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> > Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> > get_tsc_freq_arch() will return 0 as there is no instruction to
> > determine
> > the clk of PMU and eal falls back to sleep(1).
> > 
> > lib/librte_eal/common/eal_common_timer.c | 4 ++--
> > lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/eal_common_timer.c
> > b/lib/librte_eal/common/eal_common_timer.c
> > index dcf26bfea..1358bbed0 100644
> > --- a/lib/librte_eal/common/eal_common_timer.c
> > +++ b/lib/librte_eal/common/eal_common_timer.c
> > @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> > 	/* assume that the sleep(1) will sleep for 1 second */
> > 	uint64_t start = rte_rdtsc();
> > 	sleep(1);
> > -	return rte_rdtsc() - start;
> > +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> > }
> > 
> > void
> > @@ -83,7 +83,7 @@ set_tsc_freq(void)
> > 	if (!freq)
> > 		freq = estimate_tsc_freq();
> > 
> > -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq
> > / 1000);
> > +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
> > 	eal_tsc_resolution_hz = freq;

I missed this log will remove it in the next version.

> > }
> > 
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > index bc8f05199..864d6ef29 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > @@ -248,7 +248,7 @@ get_tsc_freq(void)
> > 
> > 		double secs = (double)ns/NS_PER_SEC;
> > 		tsc_hz = (uint64_t)((end - start)/secs);
> > -		return tsc_hz;
> > +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
> 
> Maybe I missed an email about this, but why would I want the TSC hz
> rounded here? I do not mind the macro just the fact that we are
> changing TSC hz value. If the TSC value is wrong then we need to fix
> the value, but I do not see it being wrong here.

Since in this function nanosleep might not be cycle accurate we need to
round it up.

Please note that estimation only applies when  get_tsc_freq_arch()
fails. i.e there is no CPU instruction that specifies the cyc/sec.

As I mentioned in the patch notes
"Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
get_tsc_freq_arch() will return 0 as there is no instruction to
determine the clock of PMU and eal falls back to sleep(1)/nanosleep." 

> > 	}
> > #endif
> > 	return 0;
> > --
> > 2.21.0
> > 
> 
> Regards,
> Keith
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > 
> > When estimating tsc frequency using sleep/gettime round it up to
> > the
> > nearest multiple of 10Mhz for more accuracy.
> > 
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> > Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> > get_tsc_freq_arch() will return 0 as there is no instruction to
> > determine
> > the clk of PMU and eal falls back to sleep(1).
> > 
> > lib/librte_eal/common/eal_common_timer.c | 4 ++--
> > lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/eal_common_timer.c
> > b/lib/librte_eal/common/eal_common_timer.c
> > index dcf26bfea..1358bbed0 100644
> > --- a/lib/librte_eal/common/eal_common_timer.c
> > +++ b/lib/librte_eal/common/eal_common_timer.c
> > @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> > 	/* assume that the sleep(1) will sleep for 1 second */
> > 	uint64_t start = rte_rdtsc();
> > 	sleep(1);
> > -	return rte_rdtsc() - start;
> > +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> > }
> > 
> > void
> > @@ -83,7 +83,7 @@ set_tsc_freq(void)
> > 	if (!freq)
> > 		freq = estimate_tsc_freq();
> > 
> > -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq
> > / 1000);
> > +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
> > 	eal_tsc_resolution_hz = freq;

I missed this log will remove it in the next version.

> > }
> > 
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > index bc8f05199..864d6ef29 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > @@ -248,7 +248,7 @@ get_tsc_freq(void)
> > 
> > 		double secs = (double)ns/NS_PER_SEC;
> > 		tsc_hz = (uint64_t)((end - start)/secs);
> > -		return tsc_hz;
> > +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
> 
> Maybe I missed an email about this, but why would I want the TSC hz
> rounded here? I do not mind the macro just the fact that we are
> changing TSC hz value. If the TSC value is wrong then we need to fix
> the value, but I do not see it being wrong here.

Since in this function nanosleep might not be cycle accurate we need to
round it up.

Please note that estimation only applies when  get_tsc_freq_arch()
fails. i.e there is no CPU instruction that specifies the cyc/sec.

As I mentioned in the patch notes
"Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
get_tsc_freq_arch() will return 0 as there is no instruction to
determine the clock of PMU and eal falls back to sleep(1)/nanosleep." 

> > 	}
> > #endif
> > 	return 0;
> > --
> > 2.21.0
> > 
> 
> Regards,
> Keith
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Wiles, Keith]

> On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
>>> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
>>> pbhagavatula@marvell.com> wrote:
>>> 
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> 
>>> When estimating tsc frequency using sleep/gettime round it up to
>>> the
>>> nearest multiple of 10Mhz for more accuracy.

How does rounding up the TSC value become more accurate, If the value is 1 cycles more then it should be then your macro would round down and if it is 1 cycle greater than 1E7 it would round up.
>>> 
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> ---
>>> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>> determine
>>> the clk of PMU and eal falls back to sleep(1).
>>> 
>>> lib/librte_eal/common/eal_common_timer.c | 4 ++--
>>> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
>>> 2 files changed, 3 insertions(+), 3 deletions(-)

It appears you did not use the head of the master as linuxapp is now just linux and freebsdapp is freebsd. You need to rebase to the head of master and send a new version.
>>> 
>>> diff --git a/lib/librte_eal/common/eal_common_timer.c
>>> b/lib/librte_eal/common/eal_common_timer.c
>>> index dcf26bfea..1358bbed0 100644
>>> --- a/lib/librte_eal/common/eal_common_timer.c
>>> +++ b/lib/librte_eal/common/eal_common_timer.c
>>> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
>>> 	/* assume that the sleep(1) will sleep for 1 second */
>>> 	uint64_t start = rte_rdtsc();
>>> 	sleep(1);
>>> -	return rte_rdtsc() - start;
>>> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);

The 1E7 is a magic number convert this to a meaningful define.
>>> }
>>> 
>>> void
>>> @@ -83,7 +83,7 @@ set_tsc_freq(void)
>>> 	if (!freq)
>>> 		freq = estimate_tsc_freq();
>>> 
>>> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq
>>> / 1000);
>>> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
>>> 	eal_tsc_resolution_hz = freq;
> 
> I missed this log will remove it in the next version.
> 
>>> }
>>> 
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> index bc8f05199..864d6ef29 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> @@ -248,7 +248,7 @@ get_tsc_freq(void)
>>> 
>>> 		double secs = (double)ns/NS_PER_SEC;
>>> 		tsc_hz = (uint64_t)((end - start)/secs);
>>> -		return tsc_hz;
>>> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
>> 
>> Maybe I missed an email about this, but why would I want the TSC hz
>> rounded here? I do not mind the macro just the fact that we are
>> changing TSC hz value. If the TSC value is wrong then we need to fix
>> the value, but I do not see it being wrong here.
> 
> Since in this function nanosleep might not be cycle accurate we need to
> round it up.
> 
> Please note that estimation only applies when  get_tsc_freq_arch()
> fails. i.e there is no CPU instruction that specifies the cyc/sec.
> 
> As I mentioned in the patch notes
> "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> get_tsc_freq_arch() will return 0 as there is no instruction to
> determine the clock of PMU and eal falls back to sleep(1)/nanosleep.” 

OK, I looked at the changes and the code for setting the TSC again. I would have not handled the setting of TSC in the way it was done, but that is not your problem. I agree the changes do look ok, the only problem I have is the new macro will roundup or down depending on the value. In your statement you are wanting to roundup the values.

If the sleep/nanosleep is less than a second for some reason, then your macro would round it down is that what we wanted? I guess my point is you are assuming the TSC calculation will always be less than a second (with sleep) and the macro would round up depending on the value calculated using the sleep/nanosleep.

I was playing with these MUL macros and I am not sure they do what we expect in the case of the multiple value is much closer to the value passed.

If we have a v = 10001 and multiple to 1000 we have the following:

RTE_ALIGN_MUL_CEIL(10001, 1000)
	(10001 + (1000 - 1)) / (1000 * 1000)
	(10001 + 999)        / 1000000
	20000                / 1000000
Result: 0

RTE_ALIGN_MUL_FLOOR(10001, 1000)
	(10001 / (1000 * 1000))
	(10001 / 1000000)
Result: 0

Unless I am wrong here the value v must be over a 1,000,000 to make these macros work or the value v to be greater than (mul * mul) in all cases or zero is the result. It may work for the TSC values as we are using a small mul value compared to the TSC value. If DPDK was ported to a slower machine it could be also zero.

I think we need to fix the macros and rethink how TSC is set here.

> 
>>> 	}
>>> #endif
>>> 	return 0;
>>> --
>>> 2.21.0
>>> 
>> 
>> Regards,
>> Keith

Regards,
Keith

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Wiles, Keith]

> On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
>>> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
>>> pbhagavatula@marvell.com> wrote:
>>> 
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> 
>>> When estimating tsc frequency using sleep/gettime round it up to
>>> the
>>> nearest multiple of 10Mhz for more accuracy.

How does rounding up the TSC value become more accurate, If the value is 1 cycles more then it should be then your macro would round down and if it is 1 cycle greater than 1E7 it would round up.
>>> 
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> ---
>>> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>> determine
>>> the clk of PMU and eal falls back to sleep(1).
>>> 
>>> lib/librte_eal/common/eal_common_timer.c | 4 ++--
>>> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
>>> 2 files changed, 3 insertions(+), 3 deletions(-)

It appears you did not use the head of the master as linuxapp is now just linux and freebsdapp is freebsd. You need to rebase to the head of master and send a new version.
>>> 
>>> diff --git a/lib/librte_eal/common/eal_common_timer.c
>>> b/lib/librte_eal/common/eal_common_timer.c
>>> index dcf26bfea..1358bbed0 100644
>>> --- a/lib/librte_eal/common/eal_common_timer.c
>>> +++ b/lib/librte_eal/common/eal_common_timer.c
>>> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
>>> 	/* assume that the sleep(1) will sleep for 1 second */
>>> 	uint64_t start = rte_rdtsc();
>>> 	sleep(1);
>>> -	return rte_rdtsc() - start;
>>> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);

The 1E7 is a magic number convert this to a meaningful define.
>>> }
>>> 
>>> void
>>> @@ -83,7 +83,7 @@ set_tsc_freq(void)
>>> 	if (!freq)
>>> 		freq = estimate_tsc_freq();
>>> 
>>> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq
>>> / 1000);
>>> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
>>> 	eal_tsc_resolution_hz = freq;
> 
> I missed this log will remove it in the next version.
> 
>>> }
>>> 
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> index bc8f05199..864d6ef29 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> @@ -248,7 +248,7 @@ get_tsc_freq(void)
>>> 
>>> 		double secs = (double)ns/NS_PER_SEC;
>>> 		tsc_hz = (uint64_t)((end - start)/secs);
>>> -		return tsc_hz;
>>> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
>> 
>> Maybe I missed an email about this, but why would I want the TSC hz
>> rounded here? I do not mind the macro just the fact that we are
>> changing TSC hz value. If the TSC value is wrong then we need to fix
>> the value, but I do not see it being wrong here.
> 
> Since in this function nanosleep might not be cycle accurate we need to
> round it up.
> 
> Please note that estimation only applies when  get_tsc_freq_arch()
> fails. i.e there is no CPU instruction that specifies the cyc/sec.
> 
> As I mentioned in the patch notes
> "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> get_tsc_freq_arch() will return 0 as there is no instruction to
> determine the clock of PMU and eal falls back to sleep(1)/nanosleep.” 

OK, I looked at the changes and the code for setting the TSC again. I would have not handled the setting of TSC in the way it was done, but that is not your problem. I agree the changes do look ok, the only problem I have is the new macro will roundup or down depending on the value. In your statement you are wanting to roundup the values.

If the sleep/nanosleep is less than a second for some reason, then your macro would round it down is that what we wanted? I guess my point is you are assuming the TSC calculation will always be less than a second (with sleep) and the macro would round up depending on the value calculated using the sleep/nanosleep.

I was playing with these MUL macros and I am not sure they do what we expect in the case of the multiple value is much closer to the value passed.

If we have a v = 10001 and multiple to 1000 we have the following:

RTE_ALIGN_MUL_CEIL(10001, 1000)
	(10001 + (1000 - 1)) / (1000 * 1000)
	(10001 + 999)        / 1000000
	20000                / 1000000
Result: 0

RTE_ALIGN_MUL_FLOOR(10001, 1000)
	(10001 / (1000 * 1000))
	(10001 / 1000000)
Result: 0

Unless I am wrong here the value v must be over a 1,000,000 to make these macros work or the value v to be greater than (mul * mul) in all cases or zero is the result. It may work for the TSC values as we are using a small mul value compared to the TSC value. If DPDK was ported to a slower machine it could be also zero.

I think we need to fix the macros and rethink how TSC is set here.

> 
>>> 	}
>>> #endif
>>> 	return 0;
>>> --
>>> 2.21.0
>>> 
>> 
>> Regards,
>> Keith

Regards,
Keith

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
> > > > On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
> > > > pbhagavatula@marvell.com> wrote:
> > > > 
> > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > 
> > > > When estimating tsc frequency using sleep/gettime round it up
> > > > to
> > > > the
> > > > nearest multiple of 10Mhz for more accuracy.
> 
> How does rounding up the TSC value become more accurate, If the value
> is 1 cycles more then it should be then your macro would round down
> and if it is 1 cycle greater than 1E7 it would round up.

Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 

Before roundup : 1400000979
After roundup : 1400000000
EAL: TSC frequency is ~1400000000 Hz


Before roundup : 1399999060
After roundup : 1400000000
EAL: TSC frequency is ~1400000000 Hz

> > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > ---
> > > > Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> > > > get_tsc_freq_arch() will return 0 as there is no instruction to
> > > > determine
> > > > the clk of PMU and eal falls back to sleep(1).
> > > > 
> > > > lib/librte_eal/common/eal_common_timer.c | 4 ++--
> > > > lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> 
> It appears you did not use the head of the master as linuxapp is now
> just linux and freebsdapp is freebsd. You need to rebase to the head
> of master and send a new version.
> > > > diff --git a/lib/librte_eal/common/eal_common_timer.c
> > > > b/lib/librte_eal/common/eal_common_timer.c
> > > > index dcf26bfea..1358bbed0 100644
> > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> > > > 	/* assume that the sleep(1) will sleep for 1 second */
> > > > 	uint64_t start = rte_rdtsc();
> > > > 	sleep(1);
> > > > -	return rte_rdtsc() - start;
> > > > +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> 
> The 1E7 is a magic number convert this to a meaningful define.

1E7 ~ 10Mhz will convert to a macro.

> > > > }
> > > > 
> > > > void
> > > > @@ -83,7 +83,7 @@ set_tsc_freq(void)
> > > > 	if (!freq)
> > > > 		freq = estimate_tsc_freq();
> > > > 
> > > > -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 "
> > > > KHz\n", freq
> > > > / 1000);
> > > > +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 "
> > > > Hz\n", freq);
> > > > 	eal_tsc_resolution_hz = freq;
> > 
> > I missed this log will remove it in the next version.
> > 
> > > > }
> > > > 
> > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > index bc8f05199..864d6ef29 100644
> > > > --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > @@ -248,7 +248,7 @@ get_tsc_freq(void)
> > > > 
> > > > 		double secs = (double)ns/NS_PER_SEC;
> > > > 		tsc_hz = (uint64_t)((end - start)/secs);
> > > > -		return tsc_hz;
> > > > +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
> > > 
> > > Maybe I missed an email about this, but why would I want the TSC
> > > hz
> > > rounded here? I do not mind the macro just the fact that we are
> > > changing TSC hz value. If the TSC value is wrong then we need to
> > > fix
> > > the value, but I do not see it being wrong here.
> > 
> > Since in this function nanosleep might not be cycle accurate we
> > need to
> > round it up.
> > 
> > Please note that estimation only applies when  get_tsc_freq_arch()
> > fails. i.e there is no CPU instruction that specifies the cyc/sec.
> > 
> > As I mentioned in the patch notes
> > "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> > get_tsc_freq_arch() will return 0 as there is no instruction to
> > determine the clock of PMU and eal falls back to
> > sleep(1)/nanosleep.” 
> 
> OK, I looked at the changes and the code for setting the TSC again. I
> would have not handled the setting of TSC in the way it was done, but
> that is not your problem. I agree the changes do look ok, the only
> problem I have is the new macro will roundup or down depending on the
> value. In your statement you are wanting to roundup the values.
> 
> If the sleep/nanosleep is less than a second for some reason, then
> your macro would round it down is that what we wanted? I guess my
> point is you are assuming the TSC calculation will always be less
> than a second (with sleep) and the macro would round up depending on
> the value calculated using the sleep/nanosleep.
> 
> I was playing with these MUL macros and I am not sure they do what we
> expect in the case of the multiple value is much closer to the value
> passed.
> 
> If we have a v = 10001 and multiple to 1000 we have the following:
> 
> RTE_ALIGN_MUL_CEIL(10001, 1000)
> 	(10001 + (1000 - 1)) / (1000 * 1000)
((10001 + (1000 - 1)) / 1000) * 1000
> 	(10001 + 999)        / 1000000
> 	20000                / 1000000
> Result: 0

((10001 + (1000 - 1) / 1000) * 1000
((10001 + 999) / 1000) * 1000
(11000/1000) * 1000
11 * 1000 

Result : 11000

> 
> RTE_ALIGN_MUL_FLOOR(10001, 1000)
> 	(10001 / (1000 * 1000))
(10001 / 1000) * 1000
> 	(10001 / 1000000)
> Result: 0
10.001 * 1000

Result : 1000

> 
> Unless I am wrong here the value v must be over a 1,000,000 to make
> these macros work or the value v to be greater than (mul * mul) in
> all cases or zero is the result. It may work for the TSC values as we
> are using a small mul value compared to the TSC value. If DPDK was
> ported to a slower machine it could be also zero.

Unless we have machines that run at freq < 10Mhz this scheme will
always work.
If we have such machines lets hope that they have a CPU instruction
that tells us the cyc/sec.

> 
> I think we need to fix the macros and rethink how TSC is set here.
> 
> > > > 	}
> > > > #endif
> > > > 	return 0;
> > > > --
> > > > 2.21.0
> > > > 
> > > 
> > > Regards,
> > > Keith
> 
> Regards,
> Keith
> 

Regards,
Pavan.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
> > > > On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
> > > > pbhagavatula@marvell.com> wrote:
> > > > 
> > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > 
> > > > When estimating tsc frequency using sleep/gettime round it up
> > > > to
> > > > the
> > > > nearest multiple of 10Mhz for more accuracy.
> 
> How does rounding up the TSC value become more accurate, If the value
> is 1 cycles more then it should be then your macro would round down
> and if it is 1 cycle greater than 1E7 it would round up.

Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 

Before roundup : 1400000979
After roundup : 1400000000
EAL: TSC frequency is ~1400000000 Hz


Before roundup : 1399999060
After roundup : 1400000000
EAL: TSC frequency is ~1400000000 Hz

> > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > ---
> > > > Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> > > > get_tsc_freq_arch() will return 0 as there is no instruction to
> > > > determine
> > > > the clk of PMU and eal falls back to sleep(1).
> > > > 
> > > > lib/librte_eal/common/eal_common_timer.c | 4 ++--
> > > > lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> 
> It appears you did not use the head of the master as linuxapp is now
> just linux and freebsdapp is freebsd. You need to rebase to the head
> of master and send a new version.
> > > > diff --git a/lib/librte_eal/common/eal_common_timer.c
> > > > b/lib/librte_eal/common/eal_common_timer.c
> > > > index dcf26bfea..1358bbed0 100644
> > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> > > > 	/* assume that the sleep(1) will sleep for 1 second */
> > > > 	uint64_t start = rte_rdtsc();
> > > > 	sleep(1);
> > > > -	return rte_rdtsc() - start;
> > > > +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> 
> The 1E7 is a magic number convert this to a meaningful define.

1E7 ~ 10Mhz will convert to a macro.

> > > > }
> > > > 
> > > > void
> > > > @@ -83,7 +83,7 @@ set_tsc_freq(void)
> > > > 	if (!freq)
> > > > 		freq = estimate_tsc_freq();
> > > > 
> > > > -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 "
> > > > KHz\n", freq
> > > > / 1000);
> > > > +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 "
> > > > Hz\n", freq);
> > > > 	eal_tsc_resolution_hz = freq;
> > 
> > I missed this log will remove it in the next version.
> > 
> > > > }
> > > > 
> > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > index bc8f05199..864d6ef29 100644
> > > > --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > @@ -248,7 +248,7 @@ get_tsc_freq(void)
> > > > 
> > > > 		double secs = (double)ns/NS_PER_SEC;
> > > > 		tsc_hz = (uint64_t)((end - start)/secs);
> > > > -		return tsc_hz;
> > > > +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
> > > 
> > > Maybe I missed an email about this, but why would I want the TSC
> > > hz
> > > rounded here? I do not mind the macro just the fact that we are
> > > changing TSC hz value. If the TSC value is wrong then we need to
> > > fix
> > > the value, but I do not see it being wrong here.
> > 
> > Since in this function nanosleep might not be cycle accurate we
> > need to
> > round it up.
> > 
> > Please note that estimation only applies when  get_tsc_freq_arch()
> > fails. i.e there is no CPU instruction that specifies the cyc/sec.
> > 
> > As I mentioned in the patch notes
> > "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> > get_tsc_freq_arch() will return 0 as there is no instruction to
> > determine the clock of PMU and eal falls back to
> > sleep(1)/nanosleep.” 
> 
> OK, I looked at the changes and the code for setting the TSC again. I
> would have not handled the setting of TSC in the way it was done, but
> that is not your problem. I agree the changes do look ok, the only
> problem I have is the new macro will roundup or down depending on the
> value. In your statement you are wanting to roundup the values.
> 
> If the sleep/nanosleep is less than a second for some reason, then
> your macro would round it down is that what we wanted? I guess my
> point is you are assuming the TSC calculation will always be less
> than a second (with sleep) and the macro would round up depending on
> the value calculated using the sleep/nanosleep.
> 
> I was playing with these MUL macros and I am not sure they do what we
> expect in the case of the multiple value is much closer to the value
> passed.
> 
> If we have a v = 10001 and multiple to 1000 we have the following:
> 
> RTE_ALIGN_MUL_CEIL(10001, 1000)
> 	(10001 + (1000 - 1)) / (1000 * 1000)
((10001 + (1000 - 1)) / 1000) * 1000
> 	(10001 + 999)        / 1000000
> 	20000                / 1000000
> Result: 0

((10001 + (1000 - 1) / 1000) * 1000
((10001 + 999) / 1000) * 1000
(11000/1000) * 1000
11 * 1000 

Result : 11000

> 
> RTE_ALIGN_MUL_FLOOR(10001, 1000)
> 	(10001 / (1000 * 1000))
(10001 / 1000) * 1000
> 	(10001 / 1000000)
> Result: 0
10.001 * 1000

Result : 1000

> 
> Unless I am wrong here the value v must be over a 1,000,000 to make
> these macros work or the value v to be greater than (mul * mul) in
> all cases or zero is the result. It may work for the TSC values as we
> are using a small mul value compared to the TSC value. If DPDK was
> ported to a slower machine it could be also zero.

Unless we have machines that run at freq < 10Mhz this scheme will
always work.
If we have such machines lets hope that they have a CPU instruction
that tells us the cyc/sec.

> 
> I think we need to fix the macros and rethink how TSC is set here.
> 
> > > > 	}
> > > > #endif
> > > > 	return 0;
> > > > --
> > > > 2.21.0
> > > > 
> > > 
> > > Regards,
> > > Keith
> 
> Regards,
> Keith
> 

Regards,
Pavan.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Wiles, Keith]

> On Mar 16, 2019, at 12:56 PM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
>>> On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <
>>> pbhagavatula@marvell.com> wrote:
>>> 
>>> On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
>>>>> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
>>>>> pbhagavatula@marvell.com> wrote:
>>>>> 
>>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>> 
>>>>> When estimating tsc frequency using sleep/gettime round it up
>>>>> to
>>>>> the
>>>>> nearest multiple of 10Mhz for more accuracy.
>> 
>> How does rounding up the TSC value become more accurate, If the value
>> is 1 cycles more then it should be then your macro would round down
>> and if it is 1 cycle greater than 1E7 it would round up.
> 
> Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 
> 
> Before roundup : 1400000979
> After roundup : 1400000000
> EAL: TSC frequency is ~1400000000 Hz
> 
> 
> Before roundup : 1399999060
> After roundup : 1400000000
> EAL: TSC frequency is ~1400000000 Hz
> 
>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>> ---
>>>>> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>>>> determine
>>>>> the clk of PMU and eal falls back to sleep(1).
>>>>> 
>>>>> lib/librte_eal/common/eal_common_timer.c | 4 ++--
>>>>> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
>>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> It appears you did not use the head of the master as linuxapp is now
>> just linux and freebsdapp is freebsd. You need to rebase to the head
>> of master and send a new version.
>>>>> diff --git a/lib/librte_eal/common/eal_common_timer.c
>>>>> b/lib/librte_eal/common/eal_common_timer.c
>>>>> index dcf26bfea..1358bbed0 100644
>>>>> --- a/lib/librte_eal/common/eal_common_timer.c
>>>>> +++ b/lib/librte_eal/common/eal_common_timer.c
>>>>> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
>>>>> 	/* assume that the sleep(1) will sleep for 1 second */
>>>>> 	uint64_t start = rte_rdtsc();
>>>>> 	sleep(1);
>>>>> -	return rte_rdtsc() - start;
>>>>> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
>> 
>> The 1E7 is a magic number convert this to a meaningful define.
> 
> 1E7 ~ 10Mhz will convert to a macro.
> 
>>>>> }
>>>>> 
>>>>> void
>>>>> @@ -83,7 +83,7 @@ set_tsc_freq(void)
>>>>> 	if (!freq)
>>>>> 		freq = estimate_tsc_freq();
>>>>> 
>>>>> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 "
>>>>> KHz\n", freq
>>>>> / 1000);
>>>>> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 "
>>>>> Hz\n", freq);
>>>>> 	eal_tsc_resolution_hz = freq;
>>> 
>>> I missed this log will remove it in the next version.
>>> 
>>>>> }
>>>>> 
>>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> index bc8f05199..864d6ef29 100644
>>>>> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> @@ -248,7 +248,7 @@ get_tsc_freq(void)
>>>>> 
>>>>> 		double secs = (double)ns/NS_PER_SEC;
>>>>> 		tsc_hz = (uint64_t)((end - start)/secs);
>>>>> -		return tsc_hz;
>>>>> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
>>>> 
>>>> Maybe I missed an email about this, but why would I want the TSC
>>>> hz
>>>> rounded here? I do not mind the macro just the fact that we are
>>>> changing TSC hz value. If the TSC value is wrong then we need to
>>>> fix
>>>> the value, but I do not see it being wrong here.
>>> 
>>> Since in this function nanosleep might not be cycle accurate we
>>> need to
>>> round it up.
>>> 
>>> Please note that estimation only applies when  get_tsc_freq_arch()
>>> fails. i.e there is no CPU instruction that specifies the cyc/sec.
>>> 
>>> As I mentioned in the patch notes
>>> "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>> determine the clock of PMU and eal falls back to
>>> sleep(1)/nanosleep.” 
>> 
>> OK, I looked at the changes and the code for setting the TSC again. I
>> would have not handled the setting of TSC in the way it was done, but
>> that is not your problem. I agree the changes do look ok, the only
>> problem I have is the new macro will roundup or down depending on the
>> value. In your statement you are wanting to roundup the values.
>> 
>> If the sleep/nanosleep is less than a second for some reason, then
>> your macro would round it down is that what we wanted? I guess my
>> point is you are assuming the TSC calculation will always be less
>> than a second (with sleep) and the macro would round up depending on
>> the value calculated using the sleep/nanosleep.
>> 
>> I was playing with these MUL macros and I am not sure they do what we
>> expect in the case of the multiple value is much closer to the value
>> passed.
>> 
>> If we have a v = 10001 and multiple to 1000 we have the following:
>> 
>> RTE_ALIGN_MUL_CEIL(10001, 1000)
>> 	(10001 + (1000 - 1)) / (1000 * 1000)
> ((10001 + (1000 - 1)) / 1000) * 1000
>> 	(10001 + 999)        / 1000000
>> 	20000                / 1000000
>> Result: 0
> 
> ((10001 + (1000 - 1) / 1000) * 1000
> ((10001 + 999) / 1000) * 1000
> (11000/1000) * 1000
> 11 * 1000 
> 
> Result : 11000
> 
>> 
>> RTE_ALIGN_MUL_FLOOR(10001, 1000)
>> 	(10001 / (1000 * 1000))
> (10001 / 1000) * 1000
>> 	(10001 / 1000000)
>> Result: 0
> 10.001 * 1000
> 
> Result : 1000

Ooops, too many parans and missed it.

Then we can get a new version and that should be OK.

I will add my $0.02 then:

Reviewed-by: Keith Wiles<keith.wiles>

> 
>> 
>> Unless I am wrong here the value v must be over a 1,000,000 to make
>> these macros work or the value v to be greater than (mul * mul) in
>> all cases or zero is the result. It may work for the TSC values as we
>> are using a small mul value compared to the TSC value. If DPDK was
>> ported to a slower machine it could be also zero.
> 
> Unless we have machines that run at freq < 10Mhz this scheme will
> always work.
> If we have such machines lets hope that they have a CPU instruction
> that tells us the cyc/sec.
> 
>> 
>> I think we need to fix the macros and rethink how TSC is set here.
>> 
>>>>> 	}
>>>>> #endif
>>>>> 	return 0;
>>>>> --
>>>>> 2.21.0
>>>>> 
>>>> 
>>>> Regards,
>>>> Keith
>> 
>> Regards,
>> Keith
>> 
> 
> Regards,
> Pavan.

Regards,
Keith

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Wiles, Keith]

> On Mar 16, 2019, at 12:56 PM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
>>> On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <
>>> pbhagavatula@marvell.com> wrote:
>>> 
>>> On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
>>>>> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
>>>>> pbhagavatula@marvell.com> wrote:
>>>>> 
>>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>> 
>>>>> When estimating tsc frequency using sleep/gettime round it up
>>>>> to
>>>>> the
>>>>> nearest multiple of 10Mhz for more accuracy.
>> 
>> How does rounding up the TSC value become more accurate, If the value
>> is 1 cycles more then it should be then your macro would round down
>> and if it is 1 cycle greater than 1E7 it would round up.
> 
> Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 
> 
> Before roundup : 1400000979
> After roundup : 1400000000
> EAL: TSC frequency is ~1400000000 Hz
> 
> 
> Before roundup : 1399999060
> After roundup : 1400000000
> EAL: TSC frequency is ~1400000000 Hz
> 
>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>> ---
>>>>> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>>>> determine
>>>>> the clk of PMU and eal falls back to sleep(1).
>>>>> 
>>>>> lib/librte_eal/common/eal_common_timer.c | 4 ++--
>>>>> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
>>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> It appears you did not use the head of the master as linuxapp is now
>> just linux and freebsdapp is freebsd. You need to rebase to the head
>> of master and send a new version.
>>>>> diff --git a/lib/librte_eal/common/eal_common_timer.c
>>>>> b/lib/librte_eal/common/eal_common_timer.c
>>>>> index dcf26bfea..1358bbed0 100644
>>>>> --- a/lib/librte_eal/common/eal_common_timer.c
>>>>> +++ b/lib/librte_eal/common/eal_common_timer.c
>>>>> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
>>>>> 	/* assume that the sleep(1) will sleep for 1 second */
>>>>> 	uint64_t start = rte_rdtsc();
>>>>> 	sleep(1);
>>>>> -	return rte_rdtsc() - start;
>>>>> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
>> 
>> The 1E7 is a magic number convert this to a meaningful define.
> 
> 1E7 ~ 10Mhz will convert to a macro.
> 
>>>>> }
>>>>> 
>>>>> void
>>>>> @@ -83,7 +83,7 @@ set_tsc_freq(void)
>>>>> 	if (!freq)
>>>>> 		freq = estimate_tsc_freq();
>>>>> 
>>>>> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 "
>>>>> KHz\n", freq
>>>>> / 1000);
>>>>> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 "
>>>>> Hz\n", freq);
>>>>> 	eal_tsc_resolution_hz = freq;
>>> 
>>> I missed this log will remove it in the next version.
>>> 
>>>>> }
>>>>> 
>>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> index bc8f05199..864d6ef29 100644
>>>>> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>>>> @@ -248,7 +248,7 @@ get_tsc_freq(void)
>>>>> 
>>>>> 		double secs = (double)ns/NS_PER_SEC;
>>>>> 		tsc_hz = (uint64_t)((end - start)/secs);
>>>>> -		return tsc_hz;
>>>>> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
>>>> 
>>>> Maybe I missed an email about this, but why would I want the TSC
>>>> hz
>>>> rounded here? I do not mind the macro just the fact that we are
>>>> changing TSC hz value. If the TSC value is wrong then we need to
>>>> fix
>>>> the value, but I do not see it being wrong here.
>>> 
>>> Since in this function nanosleep might not be cycle accurate we
>>> need to
>>> round it up.
>>> 
>>> Please note that estimation only applies when  get_tsc_freq_arch()
>>> fails. i.e there is no CPU instruction that specifies the cyc/sec.
>>> 
>>> As I mentioned in the patch notes
>>> "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>> determine the clock of PMU and eal falls back to
>>> sleep(1)/nanosleep.” 
>> 
>> OK, I looked at the changes and the code for setting the TSC again. I
>> would have not handled the setting of TSC in the way it was done, but
>> that is not your problem. I agree the changes do look ok, the only
>> problem I have is the new macro will roundup or down depending on the
>> value. In your statement you are wanting to roundup the values.
>> 
>> If the sleep/nanosleep is less than a second for some reason, then
>> your macro would round it down is that what we wanted? I guess my
>> point is you are assuming the TSC calculation will always be less
>> than a second (with sleep) and the macro would round up depending on
>> the value calculated using the sleep/nanosleep.
>> 
>> I was playing with these MUL macros and I am not sure they do what we
>> expect in the case of the multiple value is much closer to the value
>> passed.
>> 
>> If we have a v = 10001 and multiple to 1000 we have the following:
>> 
>> RTE_ALIGN_MUL_CEIL(10001, 1000)
>> 	(10001 + (1000 - 1)) / (1000 * 1000)
> ((10001 + (1000 - 1)) / 1000) * 1000
>> 	(10001 + 999)        / 1000000
>> 	20000                / 1000000
>> Result: 0
> 
> ((10001 + (1000 - 1) / 1000) * 1000
> ((10001 + 999) / 1000) * 1000
> (11000/1000) * 1000
> 11 * 1000 
> 
> Result : 11000
> 
>> 
>> RTE_ALIGN_MUL_FLOOR(10001, 1000)
>> 	(10001 / (1000 * 1000))
> (10001 / 1000) * 1000
>> 	(10001 / 1000000)
>> Result: 0
> 10.001 * 1000
> 
> Result : 1000

Ooops, too many parans and missed it.

Then we can get a new version and that should be OK.

I will add my $0.02 then:

Reviewed-by: Keith Wiles<keith.wiles>

> 
>> 
>> Unless I am wrong here the value v must be over a 1,000,000 to make
>> these macros work or the value v to be greater than (mul * mul) in
>> all cases or zero is the result. It may work for the TSC values as we
>> are using a small mul value compared to the TSC value. If DPDK was
>> ported to a slower machine it could be also zero.
> 
> Unless we have machines that run at freq < 10Mhz this scheme will
> always work.
> If we have such machines lets hope that they have a CPU instruction
> that tells us the cyc/sec.
> 
>> 
>> I think we need to fix the macros and rethink how TSC is set here.
>> 
>>>>> 	}
>>>>> #endif
>>>>> 	return 0;
>>>>> --
>>>>> 2.21.0
>>>>> 
>>>> 
>>>> Regards,
>>>> Keith
>> 
>> Regards,
>> Keith
>> 
> 
> Regards,
> Pavan.

Regards,
Keith

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

On Sat, 2019-03-16 at 18:22 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 12:56 PM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
> > > > On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <
> > > > pbhagavatula@marvell.com> wrote:
> > > > 
> > > > On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
> > > > > > On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
> > > > > > pbhagavatula@marvell.com> wrote:
> > > > > > 
> > > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > 
> > > > > > When estimating tsc frequency using sleep/gettime round it
> > > > > > up
> > > > > > to
> > > > > > the
> > > > > > nearest multiple of 10Mhz for more accuracy.
> > > 
> > > How does rounding up the TSC value become more accurate, If the
> > > value
> > > is 1 cycles more then it should be then your macro would round
> > > down
> > > and if it is 1 cycle greater than 1E7 it would round up.
> > 
> > Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 
> > 
> > Before roundup : 1400000979
> > After roundup : 1400000000
> > EAL: TSC frequency is ~1400000000 Hz
> > 
> > 
> > Before roundup : 1399999060
> > After roundup : 1400000000
> > EAL: TSC frequency is ~1400000000 Hz
> > 
> > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > ---
> > > > > > Useful in case of ARM64 if we enable
> > > > > > RTE_ARM_EAL_RDTSC_USE_PMU,
> > > > > > get_tsc_freq_arch() will return 0 as there is no
> > > > > > instruction to
> > > > > > determine
> > > > > > the clk of PMU and eal falls back to sleep(1).
> > > > > > 
> > > > > > lib/librte_eal/common/eal_common_timer.c | 4 ++--
> > > > > > lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> > > > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > It appears you did not use the head of the master as linuxapp is
> > > now
> > > just linux and freebsdapp is freebsd. You need to rebase to the
> > > head
> > > of master and send a new version.
> > > > > > diff --git a/lib/librte_eal/common/eal_common_timer.c
> > > > > > b/lib/librte_eal/common/eal_common_timer.c
> > > > > > index dcf26bfea..1358bbed0 100644
> > > > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > > > @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> > > > > > 	/* assume that the sleep(1) will sleep for 1 second */
> > > > > > 	uint64_t start = rte_rdtsc();
> > > > > > 	sleep(1);
> > > > > > -	return rte_rdtsc() - start;
> > > > > > +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> > > 
> > > The 1E7 is a magic number convert this to a meaningful define.
> > 
> > 1E7 ~ 10Mhz will convert to a macro.
> > 
> > > > > > }
> > > > > > 
> > > > > > void
> > > > > > @@ -83,7 +83,7 @@ set_tsc_freq(void)
> > > > > > 	if (!freq)
> > > > > > 		freq = estimate_tsc_freq();
> > > > > > 
> > > > > > -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 "
> > > > > > KHz\n", freq
> > > > > > / 1000);
> > > > > > +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 "
> > > > > > Hz\n", freq);
> > > > > > 	eal_tsc_resolution_hz = freq;
> > > > 
> > > > I missed this log will remove it in the next version.
> > > > 
> > > > > > }
> > > > > > 
> > > > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > index bc8f05199..864d6ef29 100644
> > > > > > --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > @@ -248,7 +248,7 @@ get_tsc_freq(void)
> > > > > > 
> > > > > > 		double secs = (double)ns/NS_PER_SEC;
> > > > > > 		tsc_hz = (uint64_t)((end - start)/secs);
> > > > > > -		return tsc_hz;
> > > > > > +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
> > > > > 
> > > > > Maybe I missed an email about this, but why would I want the
> > > > > TSC
> > > > > hz
> > > > > rounded here? I do not mind the macro just the fact that we
> > > > > are
> > > > > changing TSC hz value. If the TSC value is wrong then we need
> > > > > to
> > > > > fix
> > > > > the value, but I do not see it being wrong here.
> > > > 
> > > > Since in this function nanosleep might not be cycle accurate we
> > > > need to
> > > > round it up.
> > > > 
> > > > Please note that estimation only applies
> > > > when  get_tsc_freq_arch()
> > > > fails. i.e there is no CPU instruction that specifies the
> > > > cyc/sec.
> > > > 
> > > > As I mentioned in the patch notes
> > > > "Useful in case of ARM64 if we enable
> > > > RTE_ARM_EAL_RDTSC_USE_PMU,
> > > > get_tsc_freq_arch() will return 0 as there is no instruction to
> > > > determine the clock of PMU and eal falls back to
> > > > sleep(1)/nanosleep.” 
> > > 
> > > OK, I looked at the changes and the code for setting the TSC
> > > again. I
> > > would have not handled the setting of TSC in the way it was done,
> > > but
> > > that is not your problem. I agree the changes do look ok, the
> > > only
> > > problem I have is the new macro will roundup or down depending on
> > > the
> > > value. In your statement you are wanting to roundup the values.
> > > 
> > > If the sleep/nanosleep is less than a second for some reason,
> > > then
> > > your macro would round it down is that what we wanted? I guess my
> > > point is you are assuming the TSC calculation will always be less
> > > than a second (with sleep) and the macro would round up depending
> > > on
> > > the value calculated using the sleep/nanosleep.
> > > 
> > > I was playing with these MUL macros and I am not sure they do
> > > what we
> > > expect in the case of the multiple value is much closer to the
> > > value
> > > passed.
> > > 
> > > If we have a v = 10001 and multiple to 1000 we have the
> > > following:
> > > 
> > > RTE_ALIGN_MUL_CEIL(10001, 1000)
> > > 	(10001 + (1000 - 1)) / (1000 * 1000)
> > ((10001 + (1000 - 1)) / 1000) * 1000
> > > 	(10001 + 999)        / 1000000
> > > 	20000                / 1000000
> > > Result: 0
> > 
> > ((10001 + (1000 - 1) / 1000) * 1000
> > ((10001 + 999) / 1000) * 1000
> > (11000/1000) * 1000
> > 11 * 1000 
> > 
> > Result : 11000
> > 
> > > RTE_ALIGN_MUL_FLOOR(10001, 1000)
> > > 	(10001 / (1000 * 1000))
> > (10001 / 1000) * 1000
> > > 	(10001 / 1000000)
> > > Result: 0
> > 10.001 * 1000
> > 
> > Result : 10000
> 
> Ooops, too many parans and missed it.
> 
> Then we can get a new version and that should be OK.

Yup, thanks for reviewing :-).

> 
> I will add my $0.02 then:
> 
> Reviewed-by: Keith Wiles<keith.wiles>
> 
> > > Unless I am wrong here the value v must be over a 1,000,000 to
> > > make
> > > these macros work or the value v to be greater than (mul * mul)
> > > in
> > > all cases or zero is the result. It may work for the TSC values
> > > as we
> > > are using a small mul value compared to the TSC value. If DPDK
> > > was
> > > ported to a slower machine it could be also zero.
> > 
> > Unless we have machines that run at freq < 10Mhz this scheme will
> > always work.
> > If we have such machines lets hope that they have a CPU instruction
> > that tells us the cyc/sec.
> > 
> > > I think we need to fix the macros and rethink how TSC is set
> > > here.
> > > 
> > > > > > 	}
> > > > > > #endif
> > > > > > 	return 0;
> > > > > > --
> > > > > > 2.21.0
> > > > > > 
> > > > > 
> > > > > Regards,
> > > > > Keith
> > > 
> > > Regards,
> > > Keith
> > > 
> > 
> > Regards,
> > Pavan.
> 
> Regards,
> Keith
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

[Pavan Nikhilesh Bhagavatula]

On Sat, 2019-03-16 at 18:22 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 12:56 PM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
> > > > On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <
> > > > pbhagavatula@marvell.com> wrote:
> > > > 
> > > > On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
> > > > > > On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
> > > > > > pbhagavatula@marvell.com> wrote:
> > > > > > 
> > > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > 
> > > > > > When estimating tsc frequency using sleep/gettime round it
> > > > > > up
> > > > > > to
> > > > > > the
> > > > > > nearest multiple of 10Mhz for more accuracy.
> > > 
> > > How does rounding up the TSC value become more accurate, If the
> > > value
> > > is 1 cycles more then it should be then your macro would round
> > > down
> > > and if it is 1 cycle greater than 1E7 it would round up.
> > 
> > Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 
> > 
> > Before roundup : 1400000979
> > After roundup : 1400000000
> > EAL: TSC frequency is ~1400000000 Hz
> > 
> > 
> > Before roundup : 1399999060
> > After roundup : 1400000000
> > EAL: TSC frequency is ~1400000000 Hz
> > 
> > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > ---
> > > > > > Useful in case of ARM64 if we enable
> > > > > > RTE_ARM_EAL_RDTSC_USE_PMU,
> > > > > > get_tsc_freq_arch() will return 0 as there is no
> > > > > > instruction to
> > > > > > determine
> > > > > > the clk of PMU and eal falls back to sleep(1).
> > > > > > 
> > > > > > lib/librte_eal/common/eal_common_timer.c | 4 ++--
> > > > > > lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
> > > > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > It appears you did not use the head of the master as linuxapp is
> > > now
> > > just linux and freebsdapp is freebsd. You need to rebase to the
> > > head
> > > of master and send a new version.
> > > > > > diff --git a/lib/librte_eal/common/eal_common_timer.c
> > > > > > b/lib/librte_eal/common/eal_common_timer.c
> > > > > > index dcf26bfea..1358bbed0 100644
> > > > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > > > @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
> > > > > > 	/* assume that the sleep(1) will sleep for 1 second */
> > > > > > 	uint64_t start = rte_rdtsc();
> > > > > > 	sleep(1);
> > > > > > -	return rte_rdtsc() - start;
> > > > > > +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);
> > > 
> > > The 1E7 is a magic number convert this to a meaningful define.
> > 
> > 1E7 ~ 10Mhz will convert to a macro.
> > 
> > > > > > }
> > > > > > 
> > > > > > void
> > > > > > @@ -83,7 +83,7 @@ set_tsc_freq(void)
> > > > > > 	if (!freq)
> > > > > > 		freq = estimate_tsc_freq();
> > > > > > 
> > > > > > -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 "
> > > > > > KHz\n", freq
> > > > > > / 1000);
> > > > > > +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 "
> > > > > > Hz\n", freq);
> > > > > > 	eal_tsc_resolution_hz = freq;
> > > > 
> > > > I missed this log will remove it in the next version.
> > > > 
> > > > > > }
> > > > > > 
> > > > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > index bc8f05199..864d6ef29 100644
> > > > > > --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
> > > > > > @@ -248,7 +248,7 @@ get_tsc_freq(void)
> > > > > > 
> > > > > > 		double secs = (double)ns/NS_PER_SEC;
> > > > > > 		tsc_hz = (uint64_t)((end - start)/secs);
> > > > > > -		return tsc_hz;
> > > > > > +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
> > > > > 
> > > > > Maybe I missed an email about this, but why would I want the
> > > > > TSC
> > > > > hz
> > > > > rounded here? I do not mind the macro just the fact that we
> > > > > are
> > > > > changing TSC hz value. If the TSC value is wrong then we need
> > > > > to
> > > > > fix
> > > > > the value, but I do not see it being wrong here.
> > > > 
> > > > Since in this function nanosleep might not be cycle accurate we
> > > > need to
> > > > round it up.
> > > > 
> > > > Please note that estimation only applies
> > > > when  get_tsc_freq_arch()
> > > > fails. i.e there is no CPU instruction that specifies the
> > > > cyc/sec.
> > > > 
> > > > As I mentioned in the patch notes
> > > > "Useful in case of ARM64 if we enable
> > > > RTE_ARM_EAL_RDTSC_USE_PMU,
> > > > get_tsc_freq_arch() will return 0 as there is no instruction to
> > > > determine the clock of PMU and eal falls back to
> > > > sleep(1)/nanosleep.” 
> > > 
> > > OK, I looked at the changes and the code for setting the TSC
> > > again. I
> > > would have not handled the setting of TSC in the way it was done,
> > > but
> > > that is not your problem. I agree the changes do look ok, the
> > > only
> > > problem I have is the new macro will roundup or down depending on
> > > the
> > > value. In your statement you are wanting to roundup the values.
> > > 
> > > If the sleep/nanosleep is less than a second for some reason,
> > > then
> > > your macro would round it down is that what we wanted? I guess my
> > > point is you are assuming the TSC calculation will always be less
> > > than a second (with sleep) and the macro would round up depending
> > > on
> > > the value calculated using the sleep/nanosleep.
> > > 
> > > I was playing with these MUL macros and I am not sure they do
> > > what we
> > > expect in the case of the multiple value is much closer to the
> > > value
> > > passed.
> > > 
> > > If we have a v = 10001 and multiple to 1000 we have the
> > > following:
> > > 
> > > RTE_ALIGN_MUL_CEIL(10001, 1000)
> > > 	(10001 + (1000 - 1)) / (1000 * 1000)
> > ((10001 + (1000 - 1)) / 1000) * 1000
> > > 	(10001 + 999)        / 1000000
> > > 	20000                / 1000000
> > > Result: 0
> > 
> > ((10001 + (1000 - 1) / 1000) * 1000
> > ((10001 + 999) / 1000) * 1000
> > (11000/1000) * 1000
> > 11 * 1000 
> > 
> > Result : 11000
> > 
> > > RTE_ALIGN_MUL_FLOOR(10001, 1000)
> > > 	(10001 / (1000 * 1000))
> > (10001 / 1000) * 1000
> > > 	(10001 / 1000000)
> > > Result: 0
> > 10.001 * 1000
> > 
> > Result : 10000
> 
> Ooops, too many parans and missed it.
> 
> Then we can get a new version and that should be OK.

Yup, thanks for reviewing :-).

> 
> I will add my $0.02 then:
> 
> Reviewed-by: Keith Wiles<keith.wiles>
> 
> > > Unless I am wrong here the value v must be over a 1,000,000 to
> > > make
> > > these macros work or the value v to be greater than (mul * mul)
> > > in
> > > all cases or zero is the result. It may work for the TSC values
> > > as we
> > > are using a small mul value compared to the TSC value. If DPDK
> > > was
> > > ported to a slower machine it could be also zero.
> > 
> > Unless we have machines that run at freq < 10Mhz this scheme will
> > always work.
> > If we have such machines lets hope that they have a CPU instruction
> > that tells us the cyc/sec.
> > 
> > > I think we need to fix the macros and rethink how TSC is set
> > > here.
> > > 
> > > > > > 	}
> > > > > > #endif
> > > > > > 	return 0;
> > > > > > --
> > > > > > 2.21.0
> > > > > > 
> > > > > 
> > > > > Regards,
> > > > > Keith
> > > 
> > > Regards,
> > > Keith
> > > 
> > 
> > Regards,
> > Pavan.
> 
> Regards,
> Keith
> 

[dpdk-dev] [PATCH v3 1/2] eal: add macro to align value to the nearest multiple

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add macro to align value to the nearest multiple of the given value,
resultant value might be greater than or less than the first parameter
whichever difference is the lowest.
Update unit test to include the new macro.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 v2 Changes:
 - Spilt patch and add unit test for the new macro.
 v3 Changes:
 - Rebase to ToT.
 - Use macro instead of 1E7.

 app/test/test_common.c                     |  4 ++++
 lib/librte_eal/common/include/rte_common.h | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 94d367471..2b856f8ba 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -199,6 +199,10 @@ test_align(void)
 			val = RTE_ALIGN_MUL_FLOOR(i, p);
 			if (val % p != 0 || val > i)
 				FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
+			val = RTE_ALIGN_MUL_NEAR(i, p);
+			if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
+				& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
+				FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
 		}
 	}

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 7178ba1e9..bcf8afd39 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -248,6 +248,18 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define RTE_ALIGN_MUL_FLOOR(v, mul) \
 	((v / ((typeof(v))(mul))) * (typeof(v))(mul))

+/**
+ * Macro to align value to the nearest multiple of the given value.
+ * The resultant value might be greater than or less than the first parameter
+ * whichever difference is the lowest.
+ */
+#define RTE_ALIGN_MUL_NEAR(v, mul)				\
+	({							\
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul);	\
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul);	\
+		(ceil - v) > (v - floor) ? floor : ceil;	\
+	})
+
 /**
  * Checks if a pointer is aligned to a given power-of-two value
  *
--
2.21.0

[dpdk-dev] [PATCH v3 1/2] eal: add macro to align value to the nearest multiple

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add macro to align value to the nearest multiple of the given value,
resultant value might be greater than or less than the first parameter
whichever difference is the lowest.
Update unit test to include the new macro.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 v2 Changes:
 - Spilt patch and add unit test for the new macro.
 v3 Changes:
 - Rebase to ToT.
 - Use macro instead of 1E7.

 app/test/test_common.c                     |  4 ++++
 lib/librte_eal/common/include/rte_common.h | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 94d367471..2b856f8ba 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -199,6 +199,10 @@ test_align(void)
 			val = RTE_ALIGN_MUL_FLOOR(i, p);
 			if (val % p != 0 || val > i)
 				FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
+			val = RTE_ALIGN_MUL_NEAR(i, p);
+			if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
+				& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
+				FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
 		}
 	}

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 7178ba1e9..bcf8afd39 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -248,6 +248,18 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define RTE_ALIGN_MUL_FLOOR(v, mul) \
 	((v / ((typeof(v))(mul))) * (typeof(v))(mul))

+/**
+ * Macro to align value to the nearest multiple of the given value.
+ * The resultant value might be greater than or less than the first parameter
+ * whichever difference is the lowest.
+ */
+#define RTE_ALIGN_MUL_NEAR(v, mul)				\
+	({							\
+		typeof(v) ceil = RTE_ALIGN_MUL_CEIL(v, mul);	\
+		typeof(v) floor = RTE_ALIGN_MUL_FLOOR(v, mul);	\
+		(ceil - v) > (v - floor) ? floor : ceil;	\
+	})
+
 /**
  * Checks if a pointer is aligned to a given power-of-two value
  *
--
2.21.0

[dpdk-dev] [PATCH v3 2/2] eal: roundup tsc frequency when estimating

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

When estimating tsc frequency using sleep/gettime round it up to the
nearest multiple of 10Mhz for more accuracy.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Reviewed-by: Keith Wiles <keith.wiles@intel.com>
---
 Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
 get_tsc_freq_arch() will return 0 as there is no instruction to determine
 the clk of PMU and eal falls back to sleep(1).

 lib/librte_eal/common/eal_common_timer.c | 4 +++-
 lib/librte_eal/linux/eal/eal_timer.c     | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index dcf26bfea..68d67e684 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -64,12 +64,14 @@ rte_get_tsc_hz(void)
 static uint64_t
 estimate_tsc_freq(void)
 {
+#define CYC_PER_10MHZ 1E7
 	RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
 		" - clock timings may be less accurate.\n");
 	/* assume that the sleep(1) will sleep for 1 second */
 	uint64_t start = rte_rdtsc();
 	sleep(1);
-	return rte_rdtsc() - start;
+	/* Round up to 10Mhz. 1E7 ~ 10Mhz */
+	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
 }

 void
diff --git a/lib/librte_eal/linux/eal/eal_timer.c b/lib/librte_eal/linux/eal/eal_timer.c
index bc8f05199..76ec17034 100644
--- a/lib/librte_eal/linux/eal/eal_timer.c
+++ b/lib/librte_eal/linux/eal/eal_timer.c
@@ -232,6 +232,7 @@ get_tsc_freq(void)
 {
 #ifdef CLOCK_MONOTONIC_RAW
 #define NS_PER_SEC 1E9
+#define CYC_PER_10MHZ 1E7

 	struct timespec sleeptime = {.tv_nsec = NS_PER_SEC / 10 }; /* 1/10 second */

@@ -248,7 +249,8 @@ get_tsc_freq(void)

 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
-		return tsc_hz;
+		/* Round up to 10Mhz. 1E7 ~ 10Mhz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_10MHZ);
 	}
 #endif
 	return 0;
--
2.21.0

[dpdk-dev] [PATCH v3 2/2] eal: roundup tsc frequency when estimating

[Pavan Nikhilesh Bhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

When estimating tsc frequency using sleep/gettime round it up to the
nearest multiple of 10Mhz for more accuracy.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Reviewed-by: Keith Wiles <keith.wiles@intel.com>
---
 Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
 get_tsc_freq_arch() will return 0 as there is no instruction to determine
 the clk of PMU and eal falls back to sleep(1).

 lib/librte_eal/common/eal_common_timer.c | 4 +++-
 lib/librte_eal/linux/eal/eal_timer.c     | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index dcf26bfea..68d67e684 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -64,12 +64,14 @@ rte_get_tsc_hz(void)
 static uint64_t
 estimate_tsc_freq(void)
 {
+#define CYC_PER_10MHZ 1E7
 	RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
 		" - clock timings may be less accurate.\n");
 	/* assume that the sleep(1) will sleep for 1 second */
 	uint64_t start = rte_rdtsc();
 	sleep(1);
-	return rte_rdtsc() - start;
+	/* Round up to 10Mhz. 1E7 ~ 10Mhz */
+	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
 }

 void
diff --git a/lib/librte_eal/linux/eal/eal_timer.c b/lib/librte_eal/linux/eal/eal_timer.c
index bc8f05199..76ec17034 100644
--- a/lib/librte_eal/linux/eal/eal_timer.c
+++ b/lib/librte_eal/linux/eal/eal_timer.c
@@ -232,6 +232,7 @@ get_tsc_freq(void)
 {
 #ifdef CLOCK_MONOTONIC_RAW
 #define NS_PER_SEC 1E9
+#define CYC_PER_10MHZ 1E7

 	struct timespec sleeptime = {.tv_nsec = NS_PER_SEC / 10 }; /* 1/10 second */

@@ -248,7 +249,8 @@ get_tsc_freq(void)

 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
-		return tsc_hz;
+		/* Round up to 10Mhz. 1E7 ~ 10Mhz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_10MHZ);
 	}
 #endif
 	return 0;
--
2.21.0

Re: [dpdk-dev] [PATCH v3 2/2] eal: roundup tsc frequency when estimating

[Thomas Monjalon]

16/03/2019 20:01, Pavan Nikhilesh Bhagavatula:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> When estimating tsc frequency using sleep/gettime round it up to the
> nearest multiple of 10Mhz for more accuracy.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Reviewed-by: Keith Wiles <keith.wiles@intel.com>

Series applied, thanks

Re: [dpdk-dev] [PATCH v3 2/2] eal: roundup tsc frequency when estimating

[Thomas Monjalon]

16/03/2019 20:01, Pavan Nikhilesh Bhagavatula:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> When estimating tsc frequency using sleep/gettime round it up to the
> nearest multiple of 10Mhz for more accuracy.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Reviewed-by: Keith Wiles <keith.wiles@intel.com>

Series applied, thanks