Re: [dpdk-dev] [PATCH v2] eventdev: remove default queue overriding

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

On Fri, Mar 10, 2017 at 03:19:15PM +0000, Harry van Haaren wrote:
> PMDs that only do a specific type of scheduling cannot provide
> CFG_ALL_TYPES, so the Eventdev infrastructure should not demand
> that every PMD supports CFG_ALL_TYPES.
> 
> By not overriding the default configuration of the queue as
> suggested by the PMD, the eventdev_common unit tests can pass
> on all PMDs, regardless of their capabilities.
> 
> RTE_EVENT_QUEUE_CFG_DEFAULT is no longer used by the eventdev layer
> it can be removed now. Applications should use CFG_ALL_TYPES
> if they require enqueue of all types a queue, or specify which
> type of queue they require.
> 
> The CFG_DEFAULT value is changed to CFG_ALL_TYPES in event/skeleton,
> to not break the compile.
> 
> A capability flag is added that indicates if the underlying PMD
> supports creating queues of ALL_TYPES.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

I think It is reasonable to have this capability if SW PMD can not
support it for performance reasons. The only downside is, In application
side there will be changes in the fast path.I think, The reasonable
trade-off between performance and portability to keep packet processing
functions as common and keep the pipeline advancement logic as different
main loop based on capability.

Two reasons why CFG_ALL_TYPES important for HW
- Event queue is the precious resource and it is very limited and it
  consumes power and internal resources like SRAM
- The use case like flow based event pipelining will not have constraint
  on which event queues it enqueues to

I think We can add this capability flag now and once we have performance
and latency test cases for eventdev then we can check is there any scope
for improvement in SW PMD.With that note,

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

> 
> ---
> 
> v2:
> - added capability flag to indicate if PMD supports ALL_TYPES
> 
> ---
>  drivers/event/skeleton/skeleton_eventdev.c |  2 +-
>  lib/librte_eventdev/rte_eventdev.c         |  1 -
>  lib/librte_eventdev/rte_eventdev.h         | 13 +++++++------
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/event/skeleton/skeleton_eventdev.c b/drivers/event/skeleton/skeleton_eventdev.c
> index dee0faf..308e28e 100644
> --- a/drivers/event/skeleton/skeleton_eventdev.c
> +++ b/drivers/event/skeleton/skeleton_eventdev.c
> @@ -196,7 +196,7 @@ skeleton_eventdev_queue_def_conf(struct rte_eventdev *dev, uint8_t queue_id,
>  
>  	queue_conf->nb_atomic_flows = (1ULL << 20);
>  	queue_conf->nb_atomic_order_sequences = (1ULL << 20);
> -	queue_conf->event_queue_cfg = RTE_EVENT_QUEUE_CFG_DEFAULT;
> +	queue_conf->event_queue_cfg = RTE_EVENT_QUEUE_CFG_ALL_TYPES;
>  	queue_conf->priority = RTE_EVENT_DEV_PRIORITY_NORMAL;
>  }
>  
> diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
> index 68bfc3b..c32a776 100644
> --- a/lib/librte_eventdev/rte_eventdev.c
> +++ b/lib/librte_eventdev/rte_eventdev.c
> @@ -593,7 +593,6 @@ rte_event_queue_setup(uint8_t dev_id, uint8_t queue_id,
>  		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_def_conf,
>  					-ENOTSUP);
>  		(*dev->dev_ops->queue_def_conf)(dev, queue_id, &def_conf);
> -		def_conf.event_queue_cfg = RTE_EVENT_QUEUE_CFG_DEFAULT;
>  		queue_conf = &def_conf;
>  	}
>  
> diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
> index 7073987..4c73a82 100644
> --- a/lib/librte_eventdev/rte_eventdev.h
> +++ b/lib/librte_eventdev/rte_eventdev.h
> @@ -271,6 +271,13 @@ struct rte_mbuf; /* we just use mbuf pointers; no need to include rte_mbuf.h */
>   *
>   * @see rte_event_schedule(), rte_event_dequeue_burst()
>   */
> +#define RTE_EVENT_DEV_CAP_QUEUE_ALL_TYPES     (1ULL << 3)
> +/**< Event device is capable of enqueuing events of any type to any queue.
> + * If this capability is not set, the queue only supports events of the
> + *  *RTE_EVENT_QUEUE_CFG_* type that it was created with.
> + *
> + * @see RTE_EVENT_QUEUE_CFG_* values
> + */
>  
>  /* Event device priority levels */
>  #define RTE_EVENT_DEV_PRIORITY_HIGHEST   0
> @@ -471,12 +478,6 @@ rte_event_dev_configure(uint8_t dev_id,
>  /* Event queue specific APIs */
>  
>  /* Event queue configuration bitmap flags */
> -#define RTE_EVENT_QUEUE_CFG_DEFAULT            (0)
> -/**< Default value of *event_queue_cfg* when rte_event_queue_setup() invoked
> - * with queue_conf == NULL
> - *
> - * @see rte_event_queue_setup()
> - */
>  #define RTE_EVENT_QUEUE_CFG_TYPE_MASK          (3ULL << 0)
>  /**< Mask for event queue schedule type configuration request */
>  #define RTE_EVENT_QUEUE_CFG_ALL_TYPES          (0ULL << 0)
> -- 
> 2.7.4
>