diff mbox series

[v5,5/9] app/testpmd: add clock_gettime_monotonic

Message ID 1618595864-27839-6-git-send-email-jizh@linux.microsoft.com (mailing list archive)
State Superseded
Delegated to: Ferruh Yigit
Headers show
Series app/testpmd: enable testpmd on Windows | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Zhou April 16, 2021, 5:57 p.m. UTC
Add clock_gettime_monotonic for testpmd on Windows

Signed-off-by: Jie Zhou <jizh@microsoft.com>
Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
---
 app/test-pmd/config.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

Comments

Tal Shnaiderman April 18, 2021, 5:20 p.m. UTC | #1
> Subject: [dpdk-dev] [PATCH v5 5/9] app/testpmd: add
> clock_gettime_monotonic
> 
> External email: Use caution opening links or attachments
> 
> 
> Add clock_gettime_monotonic for testpmd on Windows
> 
> Signed-off-by: Jie Zhou <jizh@microsoft.com>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> ---
>  app/test-pmd/config.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> ef0b9784d..a5f8fec5b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -63,6 +63,12 @@
> 
>  #define NS_PER_SEC 1E9
> 
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#define _clock_gettime_monotonic(cur_time)
> +clock_gettime_monotonic(&cur_time)
> +#else
> +#define _clock_gettime_monotonic(cur_time)
> clock_gettime(CLOCK_TYPE_ID,
> +&cur_time) #endif
> +

I think this change should be in EAL:
rte_get_time_monotonic(&cur_time)

Windows implementation in windows\eal_timer.c
Unix implementation in unix\eal_unix_timer.c

>  static char *flowtype_to_str(uint16_t flow_type);
> 
>  static const struct {
> @@ -170,6 +176,27 @@ print_ethaddr(const char *name, struct
> rte_ether_addr *eth_addr)
>         printf("%s%s", name, buf);
>  }
> 
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +static int
> +clock_gettime_monotonic(struct timespec *tp) {
> +       LARGE_INTEGER pf, pc;
> +       LONGLONG nsec;
> +
> +       if (QueryPerformanceFrequency(&pf) == 0)
> +               return -1;
> +
> +       if (QueryPerformanceCounter(&pc) == 0)
> +               return -1;
> +
> +       nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
> +       tp->tv_sec = nsec / NS_PER_SEC;
> +       tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
> +
> +       return 0;
> +}
> +#endif
> +
>  void
>  nic_stats_display(portid_t port_id)
>  {
> @@ -186,6 +213,8 @@ nic_stats_display(portid_t port_id)
> 
>         static const char *nic_stats_border = "########################";
> 
> +       int ret;
> +
>         if (port_id_is_invalid(port_id, ENABLED_WARN)) {
>                 print_valid_ports();
>                 return;
> @@ -202,7 +231,9 @@ nic_stats_display(portid_t port_id)
>                "%-"PRIu64"\n", stats.opackets, stats.oerrors, stats.obytes);
> 
>         diff_ns = 0;
> -       if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> +
> +       ret = _clock_gettime_monotonic(cur_time);
> +       if (ret == 0) {
>                 uint64_t ns;
> 
>                 ns = cur_time.tv_sec * NS_PER_SEC;
> --
> 2.30.0.vfs.0.2
Jie Zhou April 19, 2021, 6:04 p.m. UTC | #2
On Sun, Apr 18, 2021 at 05:20:42PM +0000, Tal Shnaiderman wrote:
> > Subject: [dpdk-dev] [PATCH v5 5/9] app/testpmd: add
> > clock_gettime_monotonic
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Add clock_gettime_monotonic for testpmd on Windows
> > 
> > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > ---
> >  app/test-pmd/config.c | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > ef0b9784d..a5f8fec5b 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -63,6 +63,12 @@
> > 
> >  #define NS_PER_SEC 1E9
> > 
> > +#ifdef RTE_EXEC_ENV_WINDOWS
> > +#define _clock_gettime_monotonic(cur_time)
> > +clock_gettime_monotonic(&cur_time)
> > +#else
> > +#define _clock_gettime_monotonic(cur_time)
> > clock_gettime(CLOCK_TYPE_ID,
> > +&cur_time) #endif
> > +
> 
> I think this change should be in EAL:
> rte_get_time_monotonic(&cur_time)
> 
> Windows implementation in windows\eal_timer.c
> Unix implementation in unix\eal_unix_timer.c

Originally and internally, the function was added into eal. But then restricted the functionality just inside testpmd to avoid currently seems unnecessary version change, per a discussion in community meeting several weeks back. If we believe eal support of clock_gettime for windows will benefit other drivers/apps now instead of future when real need comes up, I can move it back into eal. DmitryK and Tyler, any conern or inputs here?

> 
> >  static char *flowtype_to_str(uint16_t flow_type);
> > 
> >  static const struct {
> > @@ -170,6 +176,27 @@ print_ethaddr(const char *name, struct
> > rte_ether_addr *eth_addr)
> >         printf("%s%s", name, buf);
> >  }
> > 
> > +#ifdef RTE_EXEC_ENV_WINDOWS
> > +static int
> > +clock_gettime_monotonic(struct timespec *tp) {
> > +       LARGE_INTEGER pf, pc;
> > +       LONGLONG nsec;
> > +
> > +       if (QueryPerformanceFrequency(&pf) == 0)
> > +               return -1;
> > +
> > +       if (QueryPerformanceCounter(&pc) == 0)
> > +               return -1;
> > +
> > +       nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
> > +       tp->tv_sec = nsec / NS_PER_SEC;
> > +       tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> >  void
> >  nic_stats_display(portid_t port_id)
> >  {
> > @@ -186,6 +213,8 @@ nic_stats_display(portid_t port_id)
> > 
> >         static const char *nic_stats_border = "########################";
> > 
> > +       int ret;
> > +
> >         if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> >                 print_valid_ports();
> >                 return;
> > @@ -202,7 +231,9 @@ nic_stats_display(portid_t port_id)
> >                "%-"PRIu64"\n", stats.opackets, stats.oerrors, stats.obytes);
> > 
> >         diff_ns = 0;
> > -       if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> > +
> > +       ret = _clock_gettime_monotonic(cur_time);
> > +       if (ret == 0) {
> >                 uint64_t ns;
> > 
> >                 ns = cur_time.tv_sec * NS_PER_SEC;
> > --
> > 2.30.0.vfs.0.2
Thomas Monjalon April 19, 2021, 6:13 p.m. UTC | #3
19/04/2021 20:04, Jie Zhou:
> On Sun, Apr 18, 2021 at 05:20:42PM +0000, Tal Shnaiderman wrote:
> > > --- a/app/test-pmd/config.c
> > > +++ b/app/test-pmd/config.c
> > > @@ -63,6 +63,12 @@
> > > 
> > >  #define NS_PER_SEC 1E9
> > > 
> > > +#ifdef RTE_EXEC_ENV_WINDOWS
> > > +#define _clock_gettime_monotonic(cur_time)
> > > +clock_gettime_monotonic(&cur_time)
> > > +#else
> > > +#define _clock_gettime_monotonic(cur_time)
> > > clock_gettime(CLOCK_TYPE_ID,
> > > +&cur_time) #endif
> > > +
> > 
> > I think this change should be in EAL:
> > rte_get_time_monotonic(&cur_time)
> > 
> > Windows implementation in windows\eal_timer.c
> > Unix implementation in unix\eal_unix_timer.c
> 
> Originally and internally, the function was added into eal. But then restricted the functionality just inside testpmd to avoid currently seems unnecessary version change, per a discussion in community meeting several weeks back. If we believe eal support of clock_gettime for windows will benefit other drivers/apps now instead of future when real need comes up, I can move it back into eal. DmitryK and Tyler, any conern or inputs here?

My point of view:
A test application is also testing the API availability.
Here it shows something is missing in EAL.
Instead of workarounding in the test application,
it should direct you to fixing EAL.

I don't know who decided to take this shortcut,
but it should be considered as an exception,
and accepted only if improving EAL is really difficult.
Tyler Retzlaff April 19, 2021, 6:34 p.m. UTC | #4
-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Monday, April 19, 2021 11:14 AM

\eal_timer.c Unix implementation in 
> > unix\eal_unix_timer.c
> 
> Originally and internally, the function was added into eal. But then restricted the functionality just inside testpmd to avoid currently seems unnecessary version change, per a discussion in community meeting several weeks back. If we believe eal support of clock_gettime for windows will benefit other drivers/apps now instead of future when real need comes up, I can move it back into eal. DmitryK and Tyler, any conern or inputs here?

My point of view:
A test application is also testing the API availability.
Here it shows something is missing in EAL.
Instead of workarounding in the test application, it should direct you to fixing EAL.

I think we have discussed to some degree in other threads but the more POSIX interfaces that get integrated into eal with an 'rte_' namespace pasted on to the front of them causes the scale of making DPDK portable grows.  If individual applications need portable/cross platform APIs like they should look to other packages tailored for the job instead of trying to put everything into DPDK.  Threads is an example of where this has gone wrong, I don't think doing more of it is going to be beneficial.

Shouldn't EAL be in the business of being DPDK and do it well instead of an all encompassing cross-platform application development kit?
Thomas Monjalon April 19, 2021, 7:41 p.m. UTC | #5
19/04/2021 20:34, Tyler Retzlaff:
> > > Originally and internally, the function was added into eal. But then
> > > restricted the functionality just inside testpmd to avoid currently
> > > seems unnecessary version change, per a discussion in community meeting
> > > several weeks back. If we believe eal support of clock_gettime for
> > > windows will benefit other drivers/apps now instead of future when real
> > > need comes up, I can move it back into eal. DmitryK and Tyler, any
> > > conern or inputs here?
> > 
> > My point of view:
> > A test application is also testing the API availability.
> > Here it shows something is missing in EAL.
> > Instead of workarounding in the test application, it should direct you to
> > fixing EAL.
> 
> I think we have discussed to some degree in other threads but the more POSIX interfaces that get integrated into eal with an 'rte_' namespace pasted on to the front of them causes the scale of making DPDK portable grows.  If individual applications need portable/cross platform APIs like they should look to other packages tailored for the job instead of trying to put everything into DPDK.  Threads is an example of where this has gone wrong, I don't think doing more of it is going to be beneficial.
> 
> Shouldn't EAL be in the business of being DPDK and do it well instead of an all encompassing cross-platform application development kit?

Yes good point.
Dmitry Kozlyuk April 28, 2021, 8:45 a.m. UTC | #6
2021-04-19 21:41 (UTC+0200), Thomas Monjalon:
> 19/04/2021 20:34, Tyler Retzlaff:
> > > > Originally and internally, the function was added into eal. But then
> > > > restricted the functionality just inside testpmd to avoid currently
> > > > seems unnecessary version change, per a discussion in community meeting
> > > > several weeks back. If we believe eal support of clock_gettime for
> > > > windows will benefit other drivers/apps now instead of future when real
> > > > need comes up, I can move it back into eal. DmitryK and Tyler, any
> > > > conern or inputs here?  
> > > 
> > > My point of view:
> > > A test application is also testing the API availability.
> > > Here it shows something is missing in EAL.
> > > Instead of workarounding in the test application, it should direct you to
> > > fixing EAL.  
> > 
> > I think we have discussed to some degree in other threads but the more POSIX interfaces that get integrated into eal with an 'rte_' namespace pasted on to the front of them causes the scale of making DPDK portable grows.  If individual applications need portable/cross platform APIs like they should look to other packages tailored for the job instead of trying to put everything into DPDK.  Threads is an example of where this has gone wrong, I don't think doing more of it is going to be beneficial.
> > 
> > Shouldn't EAL be in the business of being DPDK and do it well instead of an all encompassing cross-platform application development kit?  
> 
> Yes good point.

While Tyler's point is valid in general, monotonic time is something required
in many PMDs for timeouts. App networking code often deals with timeouts, too.

There's already a patch adding clock_gettime():

http://patchwork.dpdk.org/project/dpdk/patch/1619597563-56716-1-git-send-email-humin29@huawei.com/

Luckily EAL only needs this in multiprocess part, disabled on Windows;
but PMDs do require it in portable code. Even Unices would benefit a little
from not having #ifdef CLOCK_MONOTONIC_RAW in several files.

I'm for moving this to EAL.

P.S. Not all gettimeofday() are subject to replacement with new API: for
example, in PCAP we (arguably) need a realtime stamp in packets.
Jie Zhou April 29, 2021, 7:52 p.m. UTC | #7
On Wed, Apr 28, 2021 at 11:45:40AM +0300, Dmitry Kozlyuk wrote:
> 2021-04-19 21:41 (UTC+0200), Thomas Monjalon:
> > 19/04/2021 20:34, Tyler Retzlaff:
> > > > > Originally and internally, the function was added into eal. But then
> > > > > restricted the functionality just inside testpmd to avoid currently
> > > > > seems unnecessary version change, per a discussion in community meeting
> > > > > several weeks back. If we believe eal support of clock_gettime for
> > > > > windows will benefit other drivers/apps now instead of future when real
> > > > > need comes up, I can move it back into eal. DmitryK and Tyler, any
> > > > > conern or inputs here?  
> > > > 
> > > > My point of view:
> > > > A test application is also testing the API availability.
> > > > Here it shows something is missing in EAL.
> > > > Instead of workarounding in the test application, it should direct you to
> > > > fixing EAL.  
> > > 
> > > I think we have discussed to some degree in other threads but the more POSIX interfaces that get integrated into eal with an 'rte_' namespace pasted on to the front of them causes the scale of making DPDK portable grows.  If individual applications need portable/cross platform APIs like they should look to other packages tailored for the job instead of trying to put everything into DPDK.  Threads is an example of where this has gone wrong, I don't think doing more of it is going to be beneficial.
> > > 
> > > Shouldn't EAL be in the business of being DPDK and do it well instead of an all encompassing cross-platform application development kit?  
> > 
> > Yes good point.
> 
> While Tyler's point is valid in general, monotonic time is something required
> in many PMDs for timeouts. App networking code often deals with timeouts, too.
> 
> There's already a patch adding clock_gettime():
> 
> http://patchwork.dpdk.org/project/dpdk/patch/1619597563-56716-1-git-send-email-humin29@huawei.com/
> 
> Luckily EAL only needs this in multiprocess part, disabled on Windows;
> but PMDs do require it in portable code. Even Unices would benefit a little
> from not having #ifdef CLOCK_MONOTONIC_RAW in several files.
> 
> I'm for moving this to EAL.
> 
> P.S. Not all gettimeofday() are subject to replacement with new API: for
> example, in PCAP we (arguably) need a realtime stamp in packets.

I will move this into EAL in V9. Thanks.
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ef0b9784d..a5f8fec5b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -63,6 +63,12 @@ 
 
 #define NS_PER_SEC 1E9
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define _clock_gettime_monotonic(cur_time) clock_gettime_monotonic(&cur_time)
+#else
+#define _clock_gettime_monotonic(cur_time) clock_gettime(CLOCK_TYPE_ID, &cur_time)
+#endif
+
 static char *flowtype_to_str(uint16_t flow_type);
 
 static const struct {
@@ -170,6 +176,27 @@  print_ethaddr(const char *name, struct rte_ether_addr *eth_addr)
 	printf("%s%s", name, buf);
 }
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+static int
+clock_gettime_monotonic(struct timespec *tp)
+{
+	LARGE_INTEGER pf, pc;
+	LONGLONG nsec;
+
+	if (QueryPerformanceFrequency(&pf) == 0)
+		return -1;
+
+	if (QueryPerformanceCounter(&pc) == 0)
+		return -1;
+
+	nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
+	tp->tv_sec = nsec / NS_PER_SEC;
+	tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
+
+	return 0;
+}
+#endif
+
 void
 nic_stats_display(portid_t port_id)
 {
@@ -186,6 +213,8 @@  nic_stats_display(portid_t port_id)
 
 	static const char *nic_stats_border = "########################";
 
+	int ret;
+
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		print_valid_ports();
 		return;
@@ -202,7 +231,9 @@  nic_stats_display(portid_t port_id)
 	       "%-"PRIu64"\n", stats.opackets, stats.oerrors, stats.obytes);
 
 	diff_ns = 0;
-	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
+
+	ret = _clock_gettime_monotonic(cur_time);
+	if (ret == 0) {
 		uint64_t ns;
 
 		ns = cur_time.tv_sec * NS_PER_SEC;