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

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

On Wed, Dec 07, 2016 at 10:57:13AM +0000, Van Haaren, Harry wrote:
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> 
> Hi Jerin,

Hi Harry,

> 
> 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.

OK. Looks like balanced byte-alignment makes more sense on IA.We will go with that then.
Few comments below,


> 
> 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:
> * Add impl_opaque to the remaining 8 bits of those 32 bits (previous size was 12 bits)
OK
> 
> * QueueID and Priority remain 8 bit integers - but now accessible as 8 bit ints.
OK
> 
> * Operation and sched_type *increased* to 4 bits each (from previous value of 2) to allow future expansion without ABI changes

Anyway it will break ABI if we add new operation. I would propose to keep 4bit
reserved and add it when required.

> 
> * Event type remains constant at 4 bits

OK

> * Restore flow_id to 24 bits of a 32 bit int (previous size was 20 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?
One packet will not go beyond 16 stages but an application may have more stages and
each packet may go mutually exclusive stages. For example,

packet 0: stagex_0 ->stagex_1
packet 1: stagey_0 ->stagey_1

In that sense, IMO, more than 16 is required.(AFIAK, VPP has any much larger limit on number of stages)

> 
> 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.
OK. Considering more balanced layout and above points. I propose following scheme(based on your input)

	union {
		uint64_t event;
		struct {
			uint32_t flow_id: 20;
			uint32_t sub_event_type : 8;
			uint32_t event_type : 4;

			uint8_t rsvd: 4; /* for future additions */
			uint8_t operation  : 2; /* new fwd drop */
			uint8_t sched_type : 2;

			uint8_t queue_id;
			uint8_t priority;
			uint8_t impl_opaque;
		};
	};

Feedback and improvements welcomed,