Re: [dpdk-dev] [PATCH v2 1/6] eventdev: introduce event driven programming model

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]

Hi Jerin,

Re v2 rte_event struct, there seems to be some changes in the struct layout and field sizes. I've investigated them, and would like to propose some changes to balance the byte-alignment and accessing of the fields.

These changes target only the first 64 bits of the rte_event struct. I've left the current v2 code for reference, please find my proposed changes below.

> +struct rte_event {
> +	/** WORD0 */
> +	RTE_STD_C11
> +	union {
> +		uint64_t event;
> +		/** Event attributes for dequeue or enqueue operation */
> +		struct {
> +			uint64_t flow_id:20;
> +			/**< Targeted flow identifier for the enqueue and
> +			 * dequeue operation.
> +			 * The value must be in the range of
> +			 * [0, nb_event_queue_flows - 1] which
> +			 * previously supplied to rte_event_dev_configure().
> +			 */
> +			uint64_t sub_event_type:8;
> +			/**< Sub-event types based on the event source.
> +			 * @see RTE_EVENT_TYPE_CPU
> +			 */
> +			uint64_t event_type:4;
> +			/**< Event type to classify the event source.
> +			 * @see RTE_EVENT_TYPE_ETHDEV, (RTE_EVENT_TYPE_*)
> +			 */
> +			uint64_t sched_type:2;
> +			/**< Scheduler synchronization type (RTE_SCHED_TYPE_*)
> +			 * associated with flow id on a given event queue
> +			 * for the enqueue and dequeue operation.
> +			 */
> +			uint64_t queue_id:8;
> +			/**< Targeted event queue identifier for the enqueue or
> +			 * dequeue operation.
> +			 * The value must be in the range of
> +			 * [0, nb_event_queues - 1] which previously supplied to
> +			 * rte_event_dev_configure().
> +			 */
> +			uint64_t priority:8;
> +			/**< Event priority relative to other events in the
> +			 * event queue. The requested priority should in the
> +			 * range of  [RTE_EVENT_DEV_PRIORITY_HIGHEST,
> +			 * RTE_EVENT_DEV_PRIORITY_LOWEST].
> +			 * The implementation shall normalize the requested
> +			 * priority to supported priority value.
> +			 * Valid when the device has
> +			 * RTE_EVENT_DEV_CAP_FLAG_EVENT_QOS capability.
> +			 */
> +			uint64_t op:2;
> +			/**< The type of event enqueue operation - new/forward/
> +			 * etc.This field is not preserved across an instance
> +			 * and is undefined on dequeue.
> +			 *  @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> +			 */
> +			uint64_t impl_opaque:12;
> +			/**< Implementation specific opaque value.
> +			 * An implementation may use this field to hold
> +			 * implementation specific value to share between
> +			 * dequeue and enqueue operation.
> +			 * The application should not modify this field.
> +			 */
> +		};
> +	};

struct rte_event {
	/** WORD0 */
	RTE_STD_C11
	union {
		uint64_t event;
		struct {
			uint32_t flow_id: 24;
			uint32_t impl_opaque : 8; /* not defined on deq */

			uint8_t queue_id;
			uint8_t priority;

			uint8_t operation  : 4; /* new fwd drop */
			uint8_t sched_type : 4;

			uint8_t event_type : 4;
			uint8_t sub_event_type : 4;
		};
	};
	/** word 1 */
<snip>


The changes made are as follows:
* Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits)
* Add impl_opaque to the remaining 8 bits of those 32 bits (previous size was 12 bits)

* QueueID and Priority remain 8 bit integers - but now accessible as 8 bit ints.

* Operation and sched_type *increased* to 4 bits each (from previous value of 2) to allow future expansion without ABI changes

* Event type remains constant at 4 bits
* sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of situations where 16 values for application specified identifiers of each event-type is genuinely not enough?

In my opinion this structure layout is more balanced, and will perform better due to less loads that will need masking to access the required value.


Feedback and improvements welcomed, -Harry