[dpdk-dev,v3,01/17] eventdev: fix API docs and test for timeout ticks

Message ID 1487343252-16092-2-git-send-email-harry.van.haaren@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Van Haaren, Harry Feb. 17, 2017, 2:53 p.m. UTC
  This commit improves the documentation of the api return
values for rte_event_dequeue_timeout_ticks(), and allows
-ENOTSUP to be returned by devices which do not support
timeouts.

The unit test is modified to accept -ENOTSUP as a pass,
as the device doesn't implement the timeout_ticks function.

Fixes: 4c9a26e419a7 ("app/test: unit test case for eventdev APIs")

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 app/test/test_eventdev.c           | 4 +++-
 lib/librte_eventdev/rte_eventdev.h | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)
  

Comments

John McNamara Feb. 20, 2017, 3:22 p.m. UTC | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Harry van Haaren
> Sent: Friday, February 17, 2017 2:54 PM
> To: dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: [dpdk-dev] [PATCH v3 01/17] eventdev: fix API docs and test for
> timeout ticks
> 
> This commit improves the documentation of the api return values for
> rte_event_dequeue_timeout_ticks(), and allows -ENOTSUP to be returned by
> devices which do not support timeouts.
> 
> The unit test is modified to accept -ENOTSUP as a pass, as the device
> doesn't implement the timeout_ticks function.
> 
> Fixes: 4c9a26e419a7 ("app/test: unit test case for eventdev APIs")
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>


Acked-by: John McNamara <john.mcnamara@intel.com>
  
Jerin Jacob March 6, 2017, 10:33 a.m. UTC | #2
On Fri, Feb 17, 2017 at 02:53:56PM +0000, Harry van Haaren wrote:
> This commit improves the documentation of the api return
> values for rte_event_dequeue_timeout_ticks(), and allows
> -ENOTSUP to be returned by devices which do not support
> timeouts.
> 
> The unit test is modified to accept -ENOTSUP as a pass,
> as the device doesn't implement the timeout_ticks function.
> 
> Fixes: 4c9a26e419a7 ("app/test: unit test case for eventdev APIs")
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  app/test/test_eventdev.c           | 4 +++-
>  lib/librte_eventdev/rte_eventdev.h | 6 ++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/test_eventdev.c b/app/test/test_eventdev.c
> index 042a446..756bc32 100644
> --- a/app/test/test_eventdev.c
> +++ b/app/test/test_eventdev.c
> @@ -519,7 +519,9 @@ test_eventdev_timeout_ticks(void)
>  	uint64_t timeout_ticks;
>  
>  	ret = rte_event_dequeue_timeout_ticks(TEST_DEV_ID, 100, &timeout_ticks);
> -	TEST_ASSERT_SUCCESS(ret, "Fail to get timeout_ticks");
> +	/* -ENOTSUP is a valid return if timeout is not supported by device  */
> +	if (ret != -ENOTSUP)
> +		TEST_ASSERT_SUCCESS(ret, "Fail to get timeout_ticks");

Header file change looks good. IMO, In the test case, We can introduce
TEST_UNSUPPORTED in addition to TEST_SUCCESS and TEST_FAILED to reflect
the actual status. I guess it will useful for future tests as well.

If you agree with that then you can post the header file change as separate
patch. For header file change,
Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

>  
>  	return TEST_SUCCESS;
>  }
> diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
> index b619160..b0c7f9c 100644
> --- a/lib/librte_eventdev/rte_eventdev.h
> +++ b/lib/librte_eventdev/rte_eventdev.h
> @@ -1158,7 +1158,9 @@ rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id,
>   *
>   * @return
>   *  - 0 on success.
> - *  - <0 on failure.
> + *  - -ENOTSUP if the device doesn't support timeouts.
> + *  - -EINVAL if *dev_id* is invalid or *timeout_ticks* is a null pointer.
> + *  - other values < 0 on failure.
>   *
>   * @see rte_event_dequeue_burst(), RTE_EVENT_DEV_CFG_PER_DEQUEUE_TIMEOUT
>   * @see rte_event_dev_configure()
> @@ -1166,7 +1168,7 @@ rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id,
>   */
>  int
>  rte_event_dequeue_timeout_ticks(uint8_t dev_id, uint64_t ns,
> -					uint64_t *timeout_ticks);
> +				uint64_t *timeout_ticks);
>  
>  /**
>   * Dequeue a burst of events objects or an event object from the event port
> -- 
> 2.7.4
>
  
Van Haaren, Harry March 10, 2017, 3:24 p.m. UTC | #3
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Monday, March 6, 2017 10:34 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Subject: Re: [PATCH v3 01/17] eventdev: fix API docs and test for timeout ticks

<snip>

> >  	ret = rte_event_dequeue_timeout_ticks(TEST_DEV_ID, 100, &timeout_ticks);
> > -	TEST_ASSERT_SUCCESS(ret, "Fail to get timeout_ticks");
> > +	/* -ENOTSUP is a valid return if timeout is not supported by device  */
> > +	if (ret != -ENOTSUP)
> > +		TEST_ASSERT_SUCCESS(ret, "Fail to get timeout_ticks");
> 
> Header file change looks good. IMO, In the test case, We can introduce
> TEST_UNSUPPORTED in addition to TEST_SUCCESS and TEST_FAILED to reflect
> the actual status. I guess it will useful for future tests as well.

Adding TEST_UNSUPPORTED requires a larger changeset than software eventdev should
make to the test infrastructure; my preference is to use the error check solution
as above for v4 patchset.

Rework of testing infrastructure should be done on mainline dpdk if deemed required,
and enable on Eventdev branch after a rebase.
  

Patch

diff --git a/app/test/test_eventdev.c b/app/test/test_eventdev.c
index 042a446..756bc32 100644
--- a/app/test/test_eventdev.c
+++ b/app/test/test_eventdev.c
@@ -519,7 +519,9 @@  test_eventdev_timeout_ticks(void)
 	uint64_t timeout_ticks;
 
 	ret = rte_event_dequeue_timeout_ticks(TEST_DEV_ID, 100, &timeout_ticks);
-	TEST_ASSERT_SUCCESS(ret, "Fail to get timeout_ticks");
+	/* -ENOTSUP is a valid return if timeout is not supported by device  */
+	if (ret != -ENOTSUP)
+		TEST_ASSERT_SUCCESS(ret, "Fail to get timeout_ticks");
 
 	return TEST_SUCCESS;
 }
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index b619160..b0c7f9c 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -1158,7 +1158,9 @@  rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id,
  *
  * @return
  *  - 0 on success.
- *  - <0 on failure.
+ *  - -ENOTSUP if the device doesn't support timeouts.
+ *  - -EINVAL if *dev_id* is invalid or *timeout_ticks* is a null pointer.
+ *  - other values < 0 on failure.
  *
  * @see rte_event_dequeue_burst(), RTE_EVENT_DEV_CFG_PER_DEQUEUE_TIMEOUT
  * @see rte_event_dev_configure()
@@ -1166,7 +1168,7 @@  rte_event_enqueue_burst(uint8_t dev_id, uint8_t port_id,
  */
 int
 rte_event_dequeue_timeout_ticks(uint8_t dev_id, uint64_t ns,
-					uint64_t *timeout_ticks);
+				uint64_t *timeout_ticks);
 
 /**
  * Dequeue a burst of events objects or an event object from the event port