Re: [dpdk-dev] [PATCH v2 7/7] rte_sched: rearrange data structures

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Thursday, February 5, 2015 6:14 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger
> Subject: [dpdk-dev] [PATCH v2 7/7] rte_sched: rearrange data structures
> 
> From: Stephen Hemminger <shemming@brocade.com>
> 
> Rearrange internal data structures to eliminate holes.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_sched/rte_sched.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 3023457..7de1395 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -198,6 +198,8 @@ struct rte_sched_grinder {
>  	enum grinder_state state;
>  	uint32_t productive;
>  	uint32_t pindex;
> +	uint32_t qpos;
> +
>  	struct rte_sched_subport *subport;
>  	struct rte_sched_pipe *pipe;
>  	struct rte_sched_pipe_profile *pipe_params;
> @@ -212,11 +214,10 @@ struct rte_sched_grinder {
>  	uint32_t tc_index;
>  	struct rte_sched_queue
> *queue[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	struct rte_mbuf **qbase[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> +	struct rte_mbuf *pkt;
>  	uint32_t qindex[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> -	uint16_t qsize;
>  	uint32_t qmask;
> -	uint32_t qpos;
> -	struct rte_mbuf *pkt;
> +	uint16_t qsize;
> 
>  	/* WRR */
>  	uint16_t wrr_tokens[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
> @@ -234,9 +235,7 @@ struct rte_sched_port {
>  	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	uint32_t n_pipe_profiles;
>  	uint32_t pipe_tc3_rate_max;
> -#ifdef RTE_SCHED_RED
> -	struct rte_red_config
> red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][e_RTE_METER_COLO
> RS];
> -#endif
> +	uint32_t busy_grinders;
> 
>  	/* Timing */
>  	uint64_t time_cpu_cycles;     /* Current CPU time measured in CPU
> cyles */
> @@ -247,16 +246,15 @@ struct rte_sched_port {
>  	/* Scheduling loop detection */
>  	uint32_t pipe_loop;
>  	uint32_t pipe_exhaustion;
> +	uint32_t n_pkts_out;
> 
>  	/* Bitmap */
>  	struct rte_bitmap *bmp;
> +	struct rte_mbuf **pkts_out;
>  	uint32_t grinder_base_bmp_pos[RTE_SCHED_PORT_N_GRINDERS]
> __rte_aligned_16;
> 
>  	/* Grinders */
>  	struct rte_sched_grinder grinder[RTE_SCHED_PORT_N_GRINDERS];
> -	uint32_t busy_grinders;
> -	struct rte_mbuf **pkts_out;
> -	uint32_t n_pkts_out;
> 
>  	/* Queue base calculation */
>  	uint32_t qsize_add[RTE_SCHED_QUEUES_PER_PIPE];
> @@ -270,6 +268,9 @@ struct rte_sched_port {
>  	struct rte_sched_pipe_profile *pipe_profiles;
>  	uint8_t *bmp_array;
>  	struct rte_mbuf **queue_array;
> +#ifdef RTE_SCHED_RED
> +	struct rte_red_config
> red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][e_RTE_METER_COLO
> RS];
> +#endif
>  	uint8_t memory[0] __rte_cache_aligned;
>  } __rte_cache_aligned;
> 
> --
> 2.1.4

The fields of the grinder structure (and other structures, as well) are organized into groups (based on the low level functionality they're used for) for readability purpose.

I agree this sometimes requires the compiler to use padding. I don't think we getting into using an additional cache line due to padding.

Did you see any performance improvement due to this change? I don't think there is any, and I would favour readability over performance in this case?

Thanks,
Cristian

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.