Re: [PATCH v7 1/7] eventtimer: add event timer adapter API

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Thu, 8 Mar 2018 15:54:00 -0600
> From: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> To: pbhagavatula@caviumnetworks.com
> CC: dev@dpdk.org, jerin.jacob@caviumnetworks.com, nipun.gupta@nxp.com,
>  hemant.agrawal@nxp.com
> Subject: [PATCH v7 1/7] eventtimer: add event timer adapter API
> X-Mailer: git-send-email 1.7.10

IMO, you can add some git commit description here.

> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  lib/librte_eventdev/Makefile                  |   1 +
>  lib/librte_eventdev/rte_event_timer_adapter.h | 645 ++++++++++++++++++++++++++
>  lib/librte_eventdev/rte_eventdev.h            |  41 +-
>  3 files changed, 653 insertions(+), 34 deletions(-)
>  create mode 100644 lib/librte_eventdev/rte_event_timer_adapter.h
> 
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * Timer adapter configuration structure
> + */
> +struct rte_event_timer_adapter_conf {
> +	uint8_t event_dev_id;
> +	/**< Event device identifier */
> +	uint16_t timer_adapter_id;
> +	/**< Event timer adapter identifier */
> +	uint32_t socket_id;
> +	/**< Identifer of socket from which to allocate memory for adapter */

s/Identifer/Identifier

> +	enum rte_event_timer_adapter_clk_src clk_src;
> +	/**< Clock source for timer adapter */
> +	uint64_t timer_tick_ns;
> +	/**< Timer adapter resolution in ns */
> +	uint64_t max_tmo_ns;
> +	/**< Maximum timer timeout(expiry) in ns */
> +	uint64_t nb_timers;
> +	/**< Total number of timers per adapter */
> +	uint64_t flags;
> +	/**< Timer adapter config flags (RTE_EVENT_TIMER_ADAPTER_F_*) */
> +};
> +/**
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set an event timer's initial state and initialize the event it carries.
> + *
> + * @param evtim
> + *   A pointer to an event timer structure.
> + */
> +void __rte_experimental
> +rte_event_timer_init(struct rte_event_timer *evtim);

Since it can be used in fastpath, How about making it as "static inline" function?
Any it is just setting some variables.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Arm a burst of event timers with separate expiration timeout tick for each
> + * event timer.
> + *
> + * Before calling this function, the application allocates
> + * ``struct rte_event_timer`` objects from mempool or huge page backed
> + * application buffers of desired size. On successful allocation,
> + * application updates the `struct rte_event_timer`` attributes such as
> + * expiry event attributes, timeout ticks from now.
> + * This function submits the event timer arm requests to the event timer adapter
> + * and on expiry, the events will be injected to designated event queue.
> + *
> + * @param adapter
> + *   A pointer to an event timer adapter structure.
> + * @param evtims
> + *   Pointer to an array of objects of type *rte_event_timer* structure.
> + * @param nb_evtims
> + *   Number of event timers in the supplied array.
> + *
> + * @return
> + *   The number of successfully armed event timers. The return value can be less
> + *   than the value of the *nb_evtims* parameter. If the return value is less
> + *   than *nb_evtims*, the remaining event timers at the end of *evtims*
> + *   are not consumed, and the caller has to take care of them, and rte_errno
> + *   is set accordingly. Possible errno values include:
> + *   - EINVAL Invalid timer adapter, expiry event queue ID is invalid, or an
> + *   expiry event's sched type doesn't match the capabilities of the
> + *   destination event queue.
> + *   - EAGAIN Specified timer adapter is not running
> + *   - EALREADY A timer was encountered that was already armed
> + */
> +int __rte_experimental

To maintain the consistency across eventdev and other subsystems in DPDK.
We could return uint16_t for all the fast path functions. ie. exiting
"int" error return can be changed to 

rte_errno = -EINVAL;
return 0;

This would avoid some series typecasting issues as well for the _retry_
case.

> +rte_event_timer_arm_burst(const struct rte_event_timer_adapter *adapter,
> +		struct rte_event_timer **evtims,
> +		uint16_t nb_evtims);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Arm a burst of event timers with same expiration timeout tick.
> + *
> + * Provides the same functionality as ``rte_event_timer_arm_burst()``, except
> + * that application can use this API when all the event timers have the
> + * same timeout expiration tick. This specialized function can provide the
> + * additional hint to the adapter implementation and optimize if possible.
> + *
> + * @param adapter
> + *   A pointer to an event timer adapter structure.
> + * @param evtims
> + *   Points to an array of objects of type *rte_event_timer* structure.
> + * @param timeout_ticks
> + *   The number of ticks in which the timers should expire.
> + * @param nb_evtims
> + *   Number of event timers in the supplied array.
> + *
> + * @return
> + *   The number of successfully armed event timers. The return value can be less
> + *   than the value of the *nb_evtims* parameter. If the return value is less
> + *   than *nb_evtims*, the remaining event timers at the end of *evtims*
> + *   are not consumed, and the caller has to take care of them, and rte_errno
> + *   is set accordingly. Possible errno values include:
> + *   - EINVAL Invalid timer adapter, expiry event queue ID is invalid, or an
> + *   expiry event's sched type doesn't match the capabilities of the
> + *   destination event queue.
> + *   - EAGAIN Specified event timer adapter is not running
> + *   - EALREADY A timer was encountered that was already armed
> + */
> +int __rte_experimental

Same as above

> +rte_event_timer_arm_tmo_tick_burst(
> +		const struct rte_event_timer_adapter *adapter,
> +		struct rte_event_timer **evtims,
> +		const uint64_t timeout_ticks,
> +		const uint16_t nb_evtims);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Cancel a burst of event timers from being scheduled to the event device.
> + *
> + * @param adapter
> + *   A pointer to an event timer adapter structure.
> + * @param evtims
> + *   Points to an array of objects of type *rte_event_timer* structure
> + * @param nb_evtims
> + *   Number of event timer instances in the supplied array.
> + *
> + * @return
> + *   The number of successfully canceled event timers. The return value can be
> + *   less than the value of the *nb_evtims* parameter. If the return value is
> + *   less than *nb_evtims*, the remaining event timers at the end of *evtims*
> + *   are not consumed, and the caller has to take care of them, and rte_errno
> + *   is set accordingly. Possible errno values include:
> + *   - EINVAL Invalid timer adapter identifier
> + *   - EAGAIN Specified timer adapter is not running
> + *   - EALREADY  A timer was encountered that was already canceled
> + */
> +int __rte_experimental

Same as above

> +rte_event_timer_cancel_burst(const struct rte_event_timer_adapter *adapter,
> +		struct rte_event_timer **evtims,
> +		uint16_t nb_evtims);
> +
> +#endif /* __RTE_EVENT_TIMER_ADAPTER_H__ */
> diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
> index b21c271..f9ad71e 100644
> --- a/lib/librte_eventdev/rte_eventdev.h
> +++ b/lib/librte_eventdev/rte_eventdev.h
> @@ -1,35 +1,8 @@
> -/*
> - *   BSD LICENSE
> - *
> - *   Copyright 2016 Cavium, Inc.
> - *   Copyright 2016 Intel Corporation.
> - *   Copyright 2016 NXP.
> - *
> - *   Redistribution and use in source and binary forms, with or without
> - *   modification, are permitted provided that the following conditions
> - *   are met:
> - *
> - *     * Redistributions of source code must retain the above copyright
> - *       notice, this list of conditions and the following disclaimer.
> - *     * Redistributions in binary form must reproduce the above copyright
> - *       notice, this list of conditions and the following disclaimer in
> - *       the documentation and/or other materials provided with the
> - *       distribution.
> - *     * Neither the name of Cavium, Inc nor the names of its
> - *       contributors may be used to endorse or promote products derived
> - *       from this software without specific prior written permission.
> - *
> - *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> - *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> - *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> - *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> - *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> - *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> - *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> - *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> - *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> - *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016 Cavium, Inc.
> + * Copyright(c) 2016-2018 Intel Corporation.
> + * Copyright(c) 2016 NXP.

Please send existing license changes as separate patch as we need to
get ACK from all vendors.

> + * All rights reserved.
>   */
>  
>  #ifndef _RTE_EVENTDEV_H_
> @@ -923,8 +896,8 @@ rte_event_dev_close(uint8_t dev_id);
>  /**< The event generated from ethdev subsystem */
>  #define RTE_EVENT_TYPE_CRYPTODEV        0x1
>  /**< The event generated from crypodev subsystem */
> -#define RTE_EVENT_TYPE_TIMERDEV         0x2
> -/**< The event generated from timerdev subsystem */
> +#define RTE_EVENT_TYPE_TIMER		0x2
> +/**< The event generated from event timer adapter */
>  #define RTE_EVENT_TYPE_CPU              0x3
>  /**< The event generated from cpu for pipelining.
>   * Application may use *sub_event_type* to further classify the event
> -- 
> 2.6.4

Other than above minor changes, It looks really good to me.