From: "Wiles, Keith" <keith.wiles@intel.com>
To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
Cc: "stephen@networkplumber.org" <stephen@networkplumber.org>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"dev@dpdk.org" <dev@dpdk.org>,
Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it
Date: Sat, 16 Mar 2019 17:18:27 +0000 [thread overview]
Message-ID: <96FB992B-82D4-4B5D-97B1-5E255DD0621F@intel.com> (raw)
Message-ID: <20190316171827.j8EpSFRWM44x1GYDwIPuaz7qcKIE-JYKp8I0Iv4aHEI@z> (raw)
In-Reply-To: <3e56d8dbbddfde0c48a26c6257af9c43a42f99dd.camel@marvell.com>
> 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
next prev parent reply other threads:[~2019-03-16 17:18 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-29 8:32 [dpdk-dev] [PATCH] " Pavan Nikhilesh
2018-11-29 9:08 ` Jerin Jacob
2018-11-29 21:21 ` Stephen Hemminger
2018-11-30 7:17 ` Pavan Nikhilesh
2019-03-16 7:03 ` [dpdk-dev] [PATCH v2 1/2] eal: add macro to align value to the nearest multiple Pavan Nikhilesh Bhagavatula
2019-03-16 7:03 ` Pavan Nikhilesh Bhagavatula
2019-03-16 7:03 ` [dpdk-dev] [PATCH v2 2/2] eal: roundup tsc frequency when estimating it Pavan Nikhilesh Bhagavatula
2019-03-16 7:03 ` Pavan Nikhilesh Bhagavatula
2019-03-16 14:42 ` Wiles, Keith
2019-03-16 14:42 ` Wiles, Keith
2019-03-16 15:06 ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2019-03-16 15:06 ` Pavan Nikhilesh Bhagavatula
2019-03-16 17:18 ` Wiles, Keith [this message]
2019-03-16 17:18 ` Wiles, Keith
2019-03-16 17:56 ` Pavan Nikhilesh Bhagavatula
2019-03-16 17:56 ` Pavan Nikhilesh Bhagavatula
2019-03-16 18:22 ` Wiles, Keith
2019-03-16 18:22 ` Wiles, Keith
2019-03-16 18:27 ` Pavan Nikhilesh Bhagavatula
2019-03-16 18:27 ` Pavan Nikhilesh Bhagavatula
2019-03-16 19:01 ` [dpdk-dev] [PATCH v3 1/2] eal: add macro to align value to the nearest multiple Pavan Nikhilesh Bhagavatula
2019-03-16 19:01 ` Pavan Nikhilesh Bhagavatula
2019-03-16 19:01 ` [dpdk-dev] [PATCH v3 2/2] eal: roundup tsc frequency when estimating Pavan Nikhilesh Bhagavatula
2019-03-16 19:01 ` Pavan Nikhilesh Bhagavatula
2019-03-27 22:43 ` Thomas Monjalon
2019-03-27 22:43 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=96FB992B-82D4-4B5D-97B1-5E255DD0621F@intel.com \
--to=keith.wiles@intel.com \
--cc=dev@dpdk.org \
--cc=jerinj@marvell.com \
--cc=pbhagavatula@marvell.com \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).