[dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

[Thomas Monjalon]

Hi,

Sorry for late review.
This patch was in a summer hole :/

First a general comment: please check your patch with
scripts/checkpatches.sh.
In order to ease tracking of this patch, please increment the version
when sending a new one in the same thread:
git send-email -1 -v3 --annotate --to dev at dpdk.org \
--in-reply-to 1469016644-6521-1-git-send-email-jozmarti at cisco.com

More comments below.

2016-07-20 14:10, jozmarti at cisco.com:
> +void rte_delay_us_callback_register(void (*userfunc)(unsigned))
> +{
> +if (userfunc == NULL)
> +rte_delay_us = rte_delay_us_block;

Here you are creating an exception for rte_delay_us_block which is
mapped as a NULL handler.
What will happen if we need to provide more builtin handlers?
I still think that rte_delay_us_block can be exported and initialized
as the default handler. Other opinions are obviously welcome.

> +else
> +rte_delay_us = userfunc;
> +}
> +
> +static void __attribute__((constructor))
> +rte_timer_init(void)
> +{
> +/* set rte_delay_us_block as a delay function */
> +rte_delay_us_callback_register(NULL);
> +}
> diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h 
> b/lib/librte_eal/common/include/generic/rte_cycles.h
> index 8cc21f2..7a45b58 100644
> --- a/lib/librte_eal/common/include/generic/rte_cycles.h
> +++ b/lib/librte_eal/common/include/generic/rte_cycles.h
> @@ -182,13 +182,16 @@ rte_get_timer_hz(void)
>  }
>  
>  /**
> + *

useless newline

>   * Wait at least us microseconds.
> + * This function can be replaced with user-defined function using
> + * rte_delay_us_callback_register

I think you can use @see to point to rte_delay_us_callback_register.

>   *
>   * @param us
>   *   The number of microseconds to wait.
>   */
>  void
> -rte_delay_us(unsigned us);
> +(*rte_delay_us)(unsigned us);
>  
>  /**
>   * Wait at least ms milliseconds.
> @@ -202,4 +205,14 @@ rte_delay_ms(unsigned ms)
>   rte_delay_us(ms * 1000);
>  }
>  
> +/**
> + * Replace rte_delay_us with user defined function.
> + *
> + * @param userfunc
> + *   User function which replaces rte_delay_us. NULL restores
> + *   buildin block delay function.

buildin -> builtin ?

> + */
> +void rte_delay_us_callback_register(void(*userfunc)(unsigned));

[dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

[jozma...@cisco.com]

From: Jozef Martiniak 

When running single-core, some drivers tend to call rte_delay_us for a
long time, and that is causing packet drops.
To avoid this, rte_delay_us can be replaced with user-defined delay
function with:

void rte_delay_us_callback_register(void(*userfunc)(unsigned));

When userfunc==NULL, build-in blocking delay function is restored.

Signed-off-by: Jozef Martiniak 
---
 app/test/test_cycles.c | 45 ++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map  |  7 
 lib/librte_eal/common/eal_common_timer.c   | 22 ++-
 lib/librte_eal/common/include/generic/rte_cycles.h | 15 +++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map|  7 
 5 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index f6c043a..7415ac8 100644
--- a/app/test/test_cycles.c
+++ b/app/test/test_cycles.c
@@ -90,3 +90,48 @@ test_cycles(void)
 }

 REGISTER_TEST_COMMAND(cycles_autotest, test_cycles);
+
+/*
+ * rte_delay_us_callback test
+ *
+ * - check if callback is correctly registered/unregistered
+ *
+ */
+
+static int pattern;
+static void my_rte_delay_us(unsigned us)
+{
+pattern += us;
+}
+
+static int
+test_user_delay_us(void)
+{
+pattern = 0;
+
+rte_delay_us(2);
+if (pattern != 0)
+return -1;
+
+/* register custom delay function */
+rte_delay_us_callback_register(my_rte_delay_us);
+
+rte_delay_us(2);
+if (pattern != 2)
+return -1;
+
+rte_delay_us(3);
+if (pattern != 5)
+return -1;
+
+/* restore original delay function */
+rte_delay_us_callback_register(NULL);
+
+rte_delay_us(3);
+if (pattern != 5)
+return -1;
+
+return 0;
+}
+
+REGISTER_TEST_COMMAND(user_delay_us, test_user_delay_us);
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map 
b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index a335e04..2f83ed0 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -162,3 +162,10 @@ DPDK_16.07 {
rte_thread_setname;

 } DPDK_16.04;
+
+DPDK_16.11 {
+   global:
+
+   rte_delay_us_callback_register;
+
+} DPDK_16.07;
diff --git a/lib/librte_eal/common/eal_common_timer.c 
b/lib/librte_eal/common/eal_common_timer.c
index c4227cd..ddfa9d1 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -47,8 +47,11 @@
 /* The frequency of the RDTSC timer resolution */
 static uint64_t eal_tsc_resolution_hz;

-void
-rte_delay_us(unsigned us)
+/* Pointer to user delay function */
+void (*rte_delay_us)(unsigned) = NULL;
+
+static void
+rte_delay_us_block(unsigned us)
 {
const uint64_t start = rte_get_timer_cycles();
const uint64_t ticks = (uint64_t)us * rte_get_timer_hz() / 1E6;
@@ -84,3 +87,18 @@ set_tsc_freq(void)
RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
eal_tsc_resolution_hz = freq;
 }
+
+void rte_delay_us_callback_register(void (*userfunc)(unsigned))
+{
+if (userfunc == NULL)
+rte_delay_us = rte_delay_us_block;
+else
+rte_delay_us = userfunc;
+}
+
+static void __attribute__((constructor))
+rte_timer_init(void)
+{
+/* set rte_delay_us_block as a delay function */
+rte_delay_us_callback_register(NULL);
+}
diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h 
b/lib/librte_eal/common/include/generic/rte_cycles.h
index 8cc21f2..7a45b58 100644
--- a/lib/librte_eal/common/include/generic/rte_cycles.h
+++ b/lib/librte_eal/common/include/generic/rte_cycles.h
@@ -182,13 +182,16 @@ rte_get_timer_hz(void)
 }

 /**
+ *
  * Wait at least us microseconds.
+ * This function can be replaced with user-defined function using
+ * rte_delay_us_callback_register
  *
  * @param us
  *   The number of microseconds to wait.
  */
 void
-rte_delay_us(unsigned us);
+(*rte_delay_us)(unsigned us);

 /**
  * Wait at least ms milliseconds.
@@ -202,4 +205,14 @@ rte_delay_ms(unsigned ms)
rte_delay_us(ms * 1000);
 }

+/**
+ * Replace rte_delay_us with user defined function.
+ *
+ * @param userfunc
+ *   User function which replaces rte_delay_us. NULL restores
+ *   buildin block delay function.
+ */
+void rte_delay_us_callback_register(void(*userfunc)(unsigned));
+
+
 #endif /* _RTE_CYCLES_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map 
b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index db8c984..4ba30ed 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -166,3 +166,10 @@ DPDK_16.07 {
rte_thread_setname;

 } DPDK_16.04;
+
+DPDK_16.11 {
+   global:
+
+   rte_delay_us_callback_register;
+
+} DPDK_16.07;
-- 
2.1.4

[dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

[Thomas Monjalon]

Hi,

2016-07-19 14:42, jozmarti at cisco.com:
> when running single-core, some drivers tend to call rte_delay_us for a
> long time, and that is causing packet drops.
> Attached patch introduces 2 new functions:
> 
> void rte_delay_us_callback_register(void(*userfunc)(unsigned));
> void rte_delay_us_callback_unregister(void);
> 
> First one replaces rte_delay_us with userfunc and second one restores
> original rte_delay_us.

I think we could avoid the function unregister by exporting the
default implementation (let's say rte_delay_us_block).


> +REGISTER_TEST_COMMAND(user_delay_us, test_user_delay_us);

Thanks for providing an unit test.


> --- a/lib/librte_eal/common/eal_common_timer.c
> +++ b/lib/librte_eal/common/eal_common_timer.c
>  void
>  rte_delay_us(unsigned us)
>  {
> + if (unlikely(rte_delay_us_override != NULL))
> + {
> + rte_delay_us_override(us);
> + return;
> + }

Why not always call the registered callback and initialize it
to the default implementation (maybe using a constructor)?

[dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

[jozma...@cisco.com]

From: Jozef Martiniak 

when running single-core, some drivers tend to call rte_delay_us for a
long time, and that is causing packet drops.
Attached patch introduces 2 new functions:

void rte_delay_us_callback_register(void(*userfunc)(unsigned));
void rte_delay_us_callback_unregister(void);

First one replaces rte_delay_us with userfunc and second one restores
original rte_delay_us.
Test user_delay_us is included.

Signed-off-by: Jozef Martiniak 
---
 app/test/test_cycles.c | 39 ++
 lib/librte_eal/common/eal_common_timer.c   | 19 +++
 lib/librte_eal/common/include/generic/rte_cycles.h | 13 
 3 files changed, 71 insertions(+)

diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index f6c043a..2b44a53 100644
--- a/app/test/test_cycles.c
+++ b/app/test/test_cycles.c
@@ -90,3 +90,42 @@ test_cycles(void)
 }

 REGISTER_TEST_COMMAND(cycles_autotest, test_cycles);
+
+/*
+ * rte_delay_us_callback test
+ *
+ * - check if callback is correctly registered/unregistered
+ *
+ */
+
+static int pattern;
+static void my_rte_delay_us(unsigned us)
+{
+pattern += us;
+}
+
+static int
+test_user_delay_us(void)
+{
+pattern = 0;
+
+rte_delay_us_callback_register(my_rte_delay_us);
+
+rte_delay_us(2);
+if (pattern != 2)
+return -1;
+
+rte_delay_us(3);
+if (pattern != 5)
+return -1;
+
+rte_delay_us_callback_unregister();
+
+rte_delay_us(3);
+if (pattern != 5)
+return -1;
+
+return 0;
+}
+
+REGISTER_TEST_COMMAND(user_delay_us, test_user_delay_us);
diff --git a/lib/librte_eal/common/eal_common_timer.c 
b/lib/librte_eal/common/eal_common_timer.c
index c4227cd..a982562 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -47,9 +47,18 @@
 /* The frequency of the RDTSC timer resolution */
 static uint64_t eal_tsc_resolution_hz;

+/* User function which replaces rte_delay_us function */
+static void (*rte_delay_us_override)(unsigned) = NULL;
+
 void
 rte_delay_us(unsigned us)
 {
+   if (unlikely(rte_delay_us_override != NULL))
+   {
+   rte_delay_us_override(us);
+   return;
+   }
+
const uint64_t start = rte_get_timer_cycles();
const uint64_t ticks = (uint64_t)us * rte_get_timer_hz() / 1E6;
while ((rte_get_timer_cycles() - start) < ticks)
@@ -84,3 +93,13 @@ set_tsc_freq(void)
RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
eal_tsc_resolution_hz = freq;
 }
+
+void rte_delay_us_callback_register(void (*userfunc)(unsigned))
+{
+rte_delay_us_override = userfunc;
+}
+
+void rte_delay_us_callback_unregister(void)
+{
+rte_delay_us_override = NULL;
+}
diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h 
b/lib/librte_eal/common/include/generic/rte_cycles.h
index 8cc21f2..274f798 100644
--- a/lib/librte_eal/common/include/generic/rte_cycles.h
+++ b/lib/librte_eal/common/include/generic/rte_cycles.h
@@ -202,4 +202,17 @@ rte_delay_ms(unsigned ms)
rte_delay_us(ms * 1000);
 }

+/**
+ * Replace rte_delay_us with user defined function.
+ *
+ * @param userfunc
+ *   User function which replaces rte_delay_us.
+ */
+void rte_delay_us_callback_register(void(*userfunc)(unsigned));
+
+/**
+ * Unregister user callback function. Restores original rte_delay_us.
+ */
+void rte_delay_us_callback_unregister(void);
+
 #endif /* _RTE_CYCLES_H_ */
-- 
2.1.4

[dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

[Jozef Martiniak -X (jozmarti - PANTHEON TECHNOLOGIES@Cisco)]

Hi,

On Tue, 2016-07-19 at 15:21 +0200, Thomas Monjalon wrote:
Hi,
> 
> 2016-07-19 14:42, jozmarti at cisco.com:
> > when running single-core, some drivers tend to call rte_delay_us for a
> > long time, and that is causing packet drops.
> > Attached patch introduces 2 new functions:
> > 
> > void rte_delay_us_callback_register(void(*userfunc)(unsigned));
> > void rte_delay_us_callback_unregister(void);
> > 
> > First one replaces rte_delay_us with userfunc and second one restores
> > original rte_delay_us.
> 
> I think we could avoid the function unregister by exporting the
> default implementation (let's say rte_delay_us_block).
> 

I think register/unregister is the standard way how to handle callbacks. 
Unregister func is now "empty" but can be extended in the future.

> > +REGISTER_TEST_COMMAND(user_delay_us, test_user_delay_us);
> 
> Thanks for providing an unit test.
> 
> 
> > --- a/lib/librte_eal/common/eal_common_timer.c
> > +++ b/lib/librte_eal/common/eal_common_timer.c
> >  void
> >  rte_delay_us(unsigned us)
> >  {
> > +   if (unlikely(rte_delay_us_override != NULL))
> > +   {
> > +   rte_delay_us_override(us);
> > +   return;
> > +   }
> 
> Why not always call the registered callback and initialize it
> to the default implementation (maybe using a constructor)?
> 
I wanted to touch as few things as possible with this patch.

[dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

[Wiles, Keith]

Hi Jozef,

I have two quick comments inline.
> On Jul 19, 2016, at 7:42 AM, jozmarti at cisco.com wrote:
> 
> From: Jozef Martiniak 
> 
> when running single-core, some drivers tend to call rte_delay_us for a
> long time, and that is causing packet drops.
> Attached patch introduces 2 new functions:
> 
> void rte_delay_us_callback_register(void(*userfunc)(unsigned));
> void rte_delay_us_callback_unregister(void);
> 
> First one replaces rte_delay_us with userfunc and second one restores
> original rte_delay_us.
> Test user_delay_us is included.
> 
> Signed-off-by: Jozef Martiniak 
> ---
> app/test/test_cycles.c | 39 ++
> lib/librte_eal/common/eal_common_timer.c   | 19 +++
> lib/librte_eal/common/include/generic/rte_cycles.h | 13 
> 3 files changed, 71 insertions(+)
> 
> diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
> index f6c043a..2b44a53 100644
> --- a/app/test/test_cycles.c
> +++ b/app/test/test_cycles.c
> @@ -90,3 +90,42 @@ test_cycles(void)
> }
> 
> REGISTER_TEST_COMMAND(cycles_autotest, test_cycles);
> +
> +/*
> + * rte_delay_us_callback test
> + *
> + * - check if callback is correctly registered/unregistered
> + *
> + */
> +
> +static int pattern;
> +static void my_rte_delay_us(unsigned us)
> +{
> +pattern += us;
> +}
> +
> +static int
> +test_user_delay_us(void)
> +{
> +pattern = 0;
> +
> +rte_delay_us_callback_register(my_rte_delay_us);
> +
> +rte_delay_us(2);
> +if (pattern != 2)
> +return -1;
> +
> +rte_delay_us(3);
> +if (pattern != 5)
> +return -1;
> +
> +rte_delay_us_callback_unregister();
> +
> +rte_delay_us(3);
> +if (pattern != 5)
> +return -1;
> +
> +return 0;
> +}
> +
> +REGISTER_TEST_COMMAND(user_delay_us, test_user_delay_us);
> diff --git a/lib/librte_eal/common/eal_common_timer.c 
> b/lib/librte_eal/common/eal_common_timer.c
> index c4227cd..a982562 100644
> --- a/lib/librte_eal/common/eal_common_timer.c
> +++ b/lib/librte_eal/common/eal_common_timer.c
> @@ -47,9 +47,18 @@
> /* The frequency of the RDTSC timer resolution */
> static uint64_t eal_tsc_resolution_hz;
> 
> +/* User function which replaces rte_delay_us function */
> +static void (*rte_delay_us_override)(unsigned) = NULL;
> +
> void
> rte_delay_us(unsigned us)
> {
> + if (unlikely(rte_delay_us_override != NULL))
> + {
> + rte_delay_us_override(us);
> + return;
> + }
> +
>   const uint64_t start = rte_get_timer_cycles();
>   const uint64_t ticks = (uint64_t)us * rte_get_timer_hz() / 1E6;
>   while ((rte_get_timer_cycles() - start) < ticks)
> @@ -84,3 +93,13 @@ set_tsc_freq(void)
>   RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq / 1000);
>   eal_tsc_resolution_hz = freq;
> }
> +
> +void rte_delay_us_callback_register(void (*userfunc)(unsigned))
> +{
> +rte_delay_us_override = userfunc;
> +}
> +
> +void rte_delay_us_callback_unregister(void)
> +{
> +rte_delay_us_override = NULL;
> +}

I guess I would have used the rte_delay_us_callback_register(NULL) to 
unregister, but this is fine.

> diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h 
> b/lib/librte_eal/common/include/generic/rte_cycles.h
> index 8cc21f2..274f798 100644
> --- a/lib/librte_eal/common/include/generic/rte_cycles.h
> +++ b/lib/librte_eal/common/include/generic/rte_cycles.h
> @@ -202,4 +202,17 @@ rte_delay_ms(unsigned ms)
>   rte_delay_us(ms * 1000);
> }
> 
> +/**
> + * Replace rte_delay_us with user defined function.
> + *
> + * @param userfunc
> + *   User function which replaces rte_delay_us.
> + */
> +void rte_delay_us_callback_register(void(*userfunc)(unsigned));
> +
> +/**
> + * Unregister user callback function. Restores original rte_delay_us.
> + */
> +void rte_delay_us_callback_unregister(void);

Just a note we need to add these two new APIs to the map file for ABI checking.

Other then these two comments I would give this one a +1 unless someone else 
has some comments.

> +
> #endif /* _RTE_CYCLES_H_ */
> -- 
> 2.1.4
>