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

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

[jozmarti]

From: Jozef Martiniak <jozmarti@cisco.com>

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 <jozmarti@cisco.com>
---
 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

Re: [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@dpdk.org \
	--in-reply-to 1469016644-6521-1-git-send-email-jozmarti@cisco.com

More comments below.

2016-07-20 14:10, jozmarti@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));

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

[Thomas Monjalon]

Hi,

I think this feature should enter in the release 16.11.
We just need to make sure it is implemented with the right API.
Do you have any comment about managing several builtin handlers?


2016-09-13 22:04, 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@dpdk.org \
> 	--in-reply-to 1469016644-6521-1-git-send-email-jozmarti@cisco.com
> 
> More comments below.
> 
> 2016-07-20 14:10, jozmarti@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));
> 

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

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

Hello Thomas,

> First a general comment: please check your patch with scripts/checkpatches.sh.
> Done. checkpath.pl is taken from latest linux kernel...

> What will happen if we need to provide more builtin handlers?
> I still think that rte_delay_us_block can be exported and initialized
The idea was to make this function simple&&fast as possible. If user needs
more builtin handlers - he can implement this functionality within his delay 
function.

Btw there is also another patch with the same functionality (uses weak symbols) 
which I would prefer:
https://git.fd.io/cgit/vpp/tree/dpdk/dpdk-16.04_patches/0005-Allow-applications-to-override-rte_delay_us.patch
Do you have any idea how should rte_delay_us_callback_register look like for 
multiple handlers ?

Other comments are accepted.

Best regards,
Jozef

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Wednesday, September 21, 2016 3:13 PM
To: Jozef Martiniak -X (jozmarti - PANTHEON TECHNOLOGIES at Cisco) <jozmarti@cisco.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] rte_delay_us can be replaced with user function

Hi,

I think this feature should enter in the release 16.11.
We just need to make sure it is implemented with the right API.
Do you have any comment about managing several builtin handlers?


2016-09-13 22:04, 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@dpdk.org \
> 	--in-reply-to 1469016644-6521-1-git-send-email-jozmarti@cisco.com
> 
> More comments below.
> 
> 2016-07-20 14:10, jozmarti@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));
> 

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

[Thomas Monjalon]

2016-09-22 08:37, Jozef Martiniak -X:
> Hello Thomas,
> 
> > First a general comment: please check your patch with scripts/checkpatches.sh.
> Done. checkpath.pl is taken from latest linux kernel...

I can see these warnings:
UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
LEADING_SPACE: please, no spaces at the start of a line

> > What will happen if we need to provide more builtin handlers?
> > I still think that rte_delay_us_block can be exported and initialized
> The idea was to make this function simple&&fast as possible. If user needs
> more builtin handlers - he can implement this functionality within his delay 
> function.
> 
> Btw there is also another patch with the same functionality (uses weak symbols) 
> which I would prefer:
> https://git.fd.io/cgit/vpp/tree/dpdk/dpdk-16.04_patches/0005-Allow-applications-to-override-rte_delay_us.patch

I don't like the VPP patch because it is hacky and weak symbols should
be avoided for static linking.

> Do you have any idea how should rte_delay_us_callback_register look like for 
> multiple handlers ?

Yes, just as simple as an assignment:
void rte_delay_us_callback_register(void (*func)(unsigned))
{
    rte_delay_us = func;
}

We just need to export rte_delay_us_block and call
rte_delay_us_callback_register(rte_delay_us_block)
at EAL initialization or in a constructor.


> Other comments are accepted.
> 
> Best regards,
> Jozef
> 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> Sent: Wednesday, September 21, 2016 3:13 PM
> To: Jozef Martiniak -X (jozmarti - PANTHEON TECHNOLOGIES at Cisco) <jozmarti@cisco.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] rte_delay_us can be replaced with user function
> 
> Hi,
> 
> I think this feature should enter in the release 16.11.
> We just need to make sure it is implemented with the right API.
> Do you have any comment about managing several builtin handlers?
> 
> 
> 2016-09-13 22:04, 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@dpdk.org \
> > 	--in-reply-to 1469016644-6521-1-git-send-email-jozmarti@cisco.com
> > 
> > More comments below.
> > 
> > 2016-07-20 14:10, jozmarti@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 v3] rte_delay_us can be replaced with user function

[jozmarti]

From: Jozef Martiniak <jozmarti@cisco.com>

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==rte_delay_us_block build-in blocking delay function is
restored.

Signed-off-by: Jozef Martiniak <jozmarti@cisco.com>
---
 app/test/test_cycles.c                             | 45 ++++++++++++++++++++++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map      |  8 ++++
 lib/librte_eal/common/eal_common_timer.c           | 17 +++++++-
 lib/librte_eal/common/include/generic/rte_cycles.h | 24 +++++++++++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map    |  8 ++++
 5 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index f6c043a..f189797 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 unsigned int pattern;
+static void my_rte_delay_us(unsigned int 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(rte_delay_us_block);
+
+	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..a06acc1 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -162,3 +162,11 @@ DPDK_16.07 {
 	rte_thread_setname;
 
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_delay_us_block;
+	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..7265617 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;
 
+/* Pointer to user delay function */
+void (*rte_delay_us)(unsigned int) = NULL;
+
 void
-rte_delay_us(unsigned us)
+rte_delay_us_block(unsigned int us)
 {
 	const uint64_t start = rte_get_timer_cycles();
 	const uint64_t ticks = (uint64_t)us * rte_get_timer_hz() / 1E6;
@@ -84,3 +87,15 @@ 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 int))
+{
+	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(rte_delay_us_block);
+}
diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h b/lib/librte_eal/common/include/generic/rte_cycles.h
index 8cc21f2..4df5833 100644
--- a/lib/librte_eal/common/include/generic/rte_cycles.h
+++ b/lib/librte_eal/common/include/generic/rte_cycles.h
@@ -180,15 +180,17 @@ rte_get_timer_hz(void)
 	default: rte_panic("Invalid timer source specified\n");
 	}
 }
-
 /**
+ *
  * Wait at least us microseconds.
+ * This function can be replaced with user-defined function.
+ * @see rte_delay_us_callback_register
  *
  * @param us
  *   The number of microseconds to wait.
  */
 void
-rte_delay_us(unsigned us);
+(*rte_delay_us)(unsigned int us);
 
 /**
  * Wait at least ms milliseconds.
@@ -202,4 +204,22 @@ rte_delay_ms(unsigned ms)
 	rte_delay_us(ms * 1000);
 }
 
+/**
+ * Blocking delay function.
+ *
+ * @param us
+ *   Number of microseconds to wait.
+ */
+void rte_delay_us_block(unsigned int us);
+/**
+ * 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 int));
+
+
+
 #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..db216cc 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -166,3 +166,11 @@ DPDK_16.07 {
 	rte_thread_setname;
 
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_delay_us_block;
+	rte_delay_us_callback_register;
+
+} DPDK_16.07;
-- 
2.1.4

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

[Thomas Monjalon]

2016-09-23 08:39, jozmarti@cisco.com:
> --- a/lib/librte_eal/common/include/generic/rte_cycles.h
> +++ b/lib/librte_eal/common/include/generic/rte_cycles.h
> @@ -180,15 +180,17 @@ rte_get_timer_hz(void)
>  	default: rte_panic("Invalid timer source specified\n");
>  	}
>  }
> -
>  /**
> + *
>   * Wait at least us microseconds.
> + * This function can be replaced with user-defined function.
> + * @see rte_delay_us_callback_register
>   *
>   * @param us
>   *   The number of microseconds to wait.
>   */
>  void
> -rte_delay_us(unsigned us);
> +(*rte_delay_us)(unsigned int us);
>  
>  /**
>   * Wait at least ms milliseconds.
> @@ -202,4 +204,22 @@ rte_delay_ms(unsigned ms)
>  	rte_delay_us(ms * 1000);
>  }
>  
> +/**
> + * Blocking delay function.
> + *
> + * @param us
> + *   Number of microseconds to wait.
> + */
> +void rte_delay_us_block(unsigned int us);
> +/**
> + * Replace rte_delay_us with user defined function.
> + *
> + * @param userfunc
> + *   User function which replaces rte_delay_us. NULL restores
> + *   buildin block delay function.
> + */

The comment about NULL value is not valid anymore.

> +void rte_delay_us_callback_register(void(*userfunc)(unsigned int));
> +
> +
> +
>  #endif /* _RTE_CYCLES_H_ */

There are extra or missing blank lines in this chunk.
Except these and the above comment, it looks good to me.

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

[jozmarti]

From: Jozef Martiniak <jozmarti@cisco.com>

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==rte_delay_us_block build-in blocking delay function is
restored.

Signed-off-by: Jozef Martiniak <jozmarti@cisco.com>
---
 app/test/test_cycles.c                             | 45 ++++++++++++++++++++++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map      |  8 ++++
 lib/librte_eal/common/eal_common_timer.c           | 17 +++++++-
 lib/librte_eal/common/include/generic/rte_cycles.h | 22 ++++++++++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map    |  8 ++++
 5 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/app/test/test_cycles.c b/app/test/test_cycles.c
index f6c043a..f189797 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 unsigned int pattern;
+static void my_rte_delay_us(unsigned int 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(rte_delay_us_block);
+
+	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..a06acc1 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -162,3 +162,11 @@ DPDK_16.07 {
 	rte_thread_setname;
 
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_delay_us_block;
+	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..7265617 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;
 
+/* Pointer to user delay function */
+void (*rte_delay_us)(unsigned int) = NULL;
+
 void
-rte_delay_us(unsigned us)
+rte_delay_us_block(unsigned int us)
 {
 	const uint64_t start = rte_get_timer_cycles();
 	const uint64_t ticks = (uint64_t)us * rte_get_timer_hz() / 1E6;
@@ -84,3 +87,15 @@ 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 int))
+{
+	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(rte_delay_us_block);
+}
diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h b/lib/librte_eal/common/include/generic/rte_cycles.h
index 8cc21f2..96a2da9 100644
--- a/lib/librte_eal/common/include/generic/rte_cycles.h
+++ b/lib/librte_eal/common/include/generic/rte_cycles.h
@@ -180,15 +180,16 @@ rte_get_timer_hz(void)
 	default: rte_panic("Invalid timer source specified\n");
 	}
 }
-
 /**
  * Wait at least us microseconds.
+ * This function can be replaced with user-defined function.
+ * @see rte_delay_us_callback_register
  *
  * @param us
  *   The number of microseconds to wait.
  */
 void
-rte_delay_us(unsigned us);
+(*rte_delay_us)(unsigned int us);
 
 /**
  * Wait at least ms milliseconds.
@@ -202,4 +203,21 @@ rte_delay_ms(unsigned ms)
 	rte_delay_us(ms * 1000);
 }
 
+/**
+ * Blocking delay function.
+ *
+ * @param us
+ *   Number of microseconds to wait.
+ */
+void rte_delay_us_block(unsigned int us);
+
+/**
+ * Replace rte_delay_us with user defined function.
+ *
+ * @param userfunc
+ *   User function which replaces rte_delay_us. rte_delay_us_block restores
+ *   buildin block delay function.
+ */
+void rte_delay_us_callback_register(void(*userfunc)(unsigned int));
+
 #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..db216cc 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -166,3 +166,11 @@ DPDK_16.07 {
 	rte_thread_setname;
 
 } DPDK_16.04;
+
+DPDK_16.11 {
+	global:
+
+	rte_delay_us_block;
+	rte_delay_us_callback_register;
+
+} DPDK_16.07;
-- 
2.1.4

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

[Thomas Monjalon]

2016-09-26 10:35, jozmarti@cisco.com:
> From: Jozef Martiniak <jozmarti@cisco.com>
> 
> 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==rte_delay_us_block build-in blocking delay function is
> restored.
> 
> Signed-off-by: Jozef Martiniak <jozmarti@cisco.com>

Applied, thanks.

Just 2 details (below) were fixed when applying.

> --- a/lib/librte_eal/common/include/generic/rte_cycles.h
> +++ b/lib/librte_eal/common/include/generic/rte_cycles.h
> @@ -180,15 +180,16 @@ rte_get_timer_hz(void)
>  	default: rte_panic("Invalid timer source specified\n");
>  	}
>  }
> -

This blank line should remain.

>  /**
>   * Wait at least us microseconds.
> + * This function can be replaced with user-defined function.
> + * @see rte_delay_us_callback_register
[...]
> + * @param userfunc
> + *   User function which replaces rte_delay_us. rte_delay_us_block restores
> + *   buildin block delay function.

buildin -> builtin

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

[jozmarti]

From: Jozef Martiniak <jozmarti@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.
Test user_delay_us is included.

Signed-off-by: Jozef Martiniak <jozmarti@cisco.com>
---
 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

Re: [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@cisco.com wrote:
> 
> From: Jozef Martiniak <jozmarti@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.
> Test user_delay_us is included.
> 
> Signed-off-by: Jozef Martiniak <jozmarti@cisco.com>
> ---
> 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
> 

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

[Thomas Monjalon]

Hi,

2016-07-19 14:42, jozmarti@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)?

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

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

Hi,

On Tue, 2016-07-19 at 15:21 +0200, Thomas Monjalon wrote:
Hi,
> 
> 2016-07-19 14:42, jozmarti@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.