Hi Jasvinder,
> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Friday, November 23, 2018 4:54 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Pattan, Reshma
> <reshma.pat...@intel.com>
> Subject: [PATCH] mbuf: implement generic format for sched field
>
> This patch implements the changes proposed in the deprecation
> notes [1][2].
>
> The opaque mbuf->hash.sched field is updated to support generic
> definition in line with the ethdev TM and MTR APIs. The new generic
> format contains: queue ID, traffic class, color.
>
> In addtion, following API functions of the sched library have
> been modified with an additional parameter of type struct
> rte_sched_port to accomodate the changes made to mbuf sched field.
> (i) rte_sched_port_pkt_write()
> (ii) rte_sched_port_pkt_read()
>
> The other libraries, sample applications and tests which use mbuf
> sched field have been updated as well.
>
> [1] http://mails.dpdk.org/archives/dev/2018-February/090651.html
> [2] https://mails.dpdk.org/archives/dev/2018-November/119051.html
>
> Signed-off-by: Jasvinder Singh <jasvinder.si...@intel.com>
> Signed-off-by: Reshma Pattan <reshma.pat...@intel.com>
> ---
> examples/qos_sched/app_thread.c | 6 +-
> examples/qos_sched/main.c | 1 +
> .../rte_event_eth_tx_adapter.h | 4 +-
> lib/librte_mbuf/Makefile | 2 +-
> lib/librte_mbuf/rte_mbuf.h | 10 +--
> lib/librte_pipeline/rte_table_action.c | 60 ++++++++-----
> lib/librte_sched/Makefile | 2 +-
> lib/librte_sched/rte_sched.c | 89 +++++++------------
> lib/librte_sched/rte_sched.h | 10 ++-
> test/test/test_sched.c | 8 +-
> 10 files changed, 92 insertions(+), 100 deletions(-)
>
<snip>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 3dbc6695e..98428bd21 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -575,12 +575,10 @@ struct rte_mbuf {
> */
> } fdir; /**< Filter identifier if FDIR enabled */
> struct {
> - uint32_t lo;
> - uint32_t hi;
> - /**< The event eth Tx adapter uses this field
> - * to store Tx queue id.
> - * @see
> rte_event_eth_tx_adapter_txq_set()
> - */
> + uint32_t queue_id; /**< Queue ID. */
> + uint8_t traffic_class; /**< Traffic class ID.
> */
We should add comment here that traffic class 0 is the highest priority traffic
class.
> + uint8_t color; /**< Color. */
We should create a new file rte_color.h in a common place
(librte_eal/common/include) to consolidate the color definition, which is
currently replicated in too many places, such as: rte_meter.h, rte_mtr.h,
rte_tm.h.
We should include the rte_color.h file here (and in the above header files)
We should also document the link between this field and the color enum type
(@see ...).
> + uint16_t reserved; /**< Reserved. */
> } sched; /**< Hierarchical scheduler */
> /**< User defined tags. See
> rte_distributor_process() */
> uint32_t usr;
We should also add trivial inline functions to read/write these mbuf fields as
part of this header file. We want to discourage people from accessing these
fields directly.
Besides the functions to read/write each field individually, we should also
have a function to read all the sched fields in one operation, as well as
another one to write all the sched fields in one operation.
> diff --git a/lib/librte_pipeline/rte_table_action.c
> b/lib/librte_pipeline/rte_table_action.c
> index 7c7c8dd82..99f2d779b 100644
> --- a/lib/librte_pipeline/rte_table_action.c
> +++ b/lib/librte_pipeline/rte_table_action.c
> @@ -108,12 +108,12 @@ mtr_cfg_check(struct rte_table_action_mtr_config
> *mtr)
> }
>
> #define MBUF_SCHED_QUEUE_TC_COLOR(queue, tc, color) \
> - ((uint16_t)((((uint64_t)(queue)) & 0x3) | \
> - ((((uint64_t)(tc)) & 0x3) << 2) | \
> - ((((uint64_t)(color)) & 0x3) << 4)))
> + ((uint64_t)((((uint64_t)(queue)) & 0xffffffff) | \
> + ((((uint64_t)(tc)) & 0xff) << 32) | \
> + ((((uint64_t)(color)) & 0xff) << 40)))
>
> #define MBUF_SCHED_COLOR(sched, color) \
> - (((sched) & (~0x30LLU)) | ((color) << 4))
> + ((uint64_t)((sched) & (~0xff000000LLU)) | (((uint64_t)(color)) << 40))
>
Given the read/write mbuf->sched field functions, the above two macros are no
longer needed.
> struct mtr_trtcm_data {
> struct rte_meter_trtcm trtcm;
> @@ -176,7 +176,7 @@ mtr_data_size(struct rte_table_action_mtr_config
> *mtr)
> struct dscp_table_entry_data {
> enum rte_meter_color color;
> uint16_t tc;
> - uint16_t queue_tc_color;
> + uint32_t queue;
> };
>
> struct dscp_table_data {
> @@ -368,7 +368,6 @@ tm_cfg_check(struct rte_table_action_tm_config
> *tm)
> }
>
> struct tm_data {
> - uint16_t queue_tc_color;
> uint16_t subport;
> uint32_t pipe;
> } __attribute__((__packed__));
> @@ -397,26 +396,40 @@ tm_apply(struct tm_data *data,
> return status;
>
> /* Apply */
> - data->queue_tc_color = 0;
> data->subport = (uint16_t) p->subport_id;
> data->pipe = p->pipe_id;
>
> return 0;
> }
>
> +static uint32_t
> +tm_sched_qindex(struct tm_data *data,
> + struct dscp_table_entry_data *dscp,
> + struct rte_table_action_tm_config *cfg) {
> +
> + uint32_t result;
> +
> + result = data->subport * cfg->n_pipes_per_subport + data->pipe;
Since n_subports_per_pipe and n_pipes_per_subport are enforced to be power of
2, we should replace multiplication/division with shift left/right. We probably
need to store log2 correspondents in the action context.
> + result = result * RTE_TABLE_ACTION_TC_MAX + dscp->tc;
> + result = result * RTE_TABLE_ACTION_TC_QUEUE_MAX + dscp-
> >queue;
> +
> + return result;
> +}
> +
> static __rte_always_inline void
> pkt_work_tm(struct rte_mbuf *mbuf,
> struct tm_data *data,
> struct dscp_table_data *dscp_table,
> - uint32_t dscp)
> + uint32_t dscp,
> + struct rte_table_action_tm_config *cfg)
> {
> struct dscp_table_entry_data *dscp_entry = &dscp_table-
> >entry[dscp];
> - struct tm_data *sched_ptr = (struct tm_data *) &mbuf->hash.sched;
> - struct tm_data sched;
> + uint64_t *sched_ptr = (uint64_t *) &mbuf->hash.sched;
> + uint32_t queue = tm_sched_qindex(data, dscp_entry, cfg);
>
> - sched = *data;
> - sched.queue_tc_color = dscp_entry->queue_tc_color;
> - *sched_ptr = sched;
> + *sched_ptr = MBUF_SCHED_QUEUE_TC_COLOR(queue,
> + dscp_entry->tc,
> + dscp_entry->color);
> }
>
> /**
> @@ -2580,17 +2593,13 @@ rte_table_action_dscp_table_update(struct
> rte_table_action *action,
> &action->dscp_table.entry[i];
> struct rte_table_action_dscp_table_entry *entry =
> &table->entry[i];
> - uint16_t queue_tc_color =
> - MBUF_SCHED_QUEUE_TC_COLOR(entry-
> >tc_queue_id,
> - entry->tc_id,
> - entry->color);
>
> if ((dscp_mask & (1LLU << i)) == 0)
> continue;
>
> data->color = entry->color;
> data->tc = entry->tc_id;
> - data->queue_tc_color = queue_tc_color;
> + data->queue = entry->tc_queue_id;
> }
>
> return 0;
> @@ -2882,7 +2891,8 @@ pkt_work(struct rte_mbuf *mbuf,
> pkt_work_tm(mbuf,
> data,
> &action->dscp_table,
> - dscp);
> + dscp,
> + &cfg->tm);
> }
>
> if (cfg->action_mask & (1LLU << RTE_TABLE_ACTION_DECAP)) {
> @@ -3108,22 +3118,26 @@ pkt4_work(struct rte_mbuf **mbufs,
> pkt_work_tm(mbuf0,
> data0,
> &action->dscp_table,
> - dscp0);
> + dscp0,
> + &cfg->tm);
>
> pkt_work_tm(mbuf1,
> data1,
> &action->dscp_table,
> - dscp1);
> + dscp1,
> + &cfg->tm);
>
> pkt_work_tm(mbuf2,
> data2,
> &action->dscp_table,
> - dscp2);
> + dscp2,
> + &cfg->tm);
>
> pkt_work_tm(mbuf3,
> data3,
> &action->dscp_table,
> - dscp3);
> + dscp3,
> + &cfg->tm);
> }
>
> if (cfg->action_mask & (1LLU << RTE_TABLE_ACTION_DECAP)) {
> diff --git a/lib/librte_sched/Makefile b/lib/librte_sched/Makefile
> index 46c53ed71..644fd9d15 100644
> --- a/lib/librte_sched/Makefile
> +++ b/lib/librte_sched/Makefile
> @@ -18,7 +18,7 @@ LDLIBS += -lrte_timer
>
> EXPORT_MAP := rte_sched_version.map
>
> -LIBABIVER := 1
> +LIBABIVER := 2
>
> #
> # all source are stored in SRCS-y
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 587d5e602..7bf4d6400 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -128,22 +128,6 @@ enum grinder_state {
> e_GRINDER_READ_MBUF
> };
>
> -/*
> - * Path through the scheduler hierarchy used by the scheduler enqueue
> - * operation to identify the destination queue for the current
> - * packet. Stored in the field pkt.hash.sched of struct rte_mbuf of
> - * each packet, typically written by the classification stage and read
> - * by scheduler enqueue.
> - */
> -struct rte_sched_port_hierarchy {
> - uint16_t queue:2; /**< Queue ID (0 .. 3) */
> - uint16_t traffic_class:2; /**< Traffic class ID (0 .. 3)*/
> - uint32_t color:2; /**< Color */
> - uint16_t unused:10;
> - uint16_t subport; /**< Subport ID */
> - uint32_t pipe; /**< Pipe ID */
> -};
> -
> struct rte_sched_grinder {
> /* Pipe cache */
> uint16_t pcache_qmask[RTE_SCHED_GRINDER_PCACHE_SIZE];
> @@ -241,16 +225,12 @@ enum rte_sched_port_array {
> e_RTE_SCHED_PORT_ARRAY_TOTAL,
> };
>
> -#ifdef RTE_SCHED_COLLECT_STATS
> -
> static inline uint32_t
> rte_sched_port_queues_per_subport(struct rte_sched_port *port)
> {
> return RTE_SCHED_QUEUES_PER_PIPE * port-
> >n_pipes_per_subport;
> }
>
> -#endif
> -
> static inline uint32_t
> rte_sched_port_queues_per_port(struct rte_sched_port *port)
> {
> @@ -1006,44 +986,50 @@ rte_sched_port_pipe_profile_add(struct
> rte_sched_port *port,
> return 0;
> }
>
> +static inline uint32_t
> +rte_sched_port_qindex(struct rte_sched_port *port,
> + uint32_t subport,
> + uint32_t pipe,
> + uint32_t traffic_class,
> + uint32_t queue)
> +{
> + uint32_t result;
> +
> + result = subport * port->n_pipes_per_subport + pipe;
Since n_subports_per_pipe and n_pipes_per_subport are enforced to be power of
2, we should replace multiplication/division with shift left/right.
> + result = result * RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE +
> traffic_class;
> + result = result * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + queue;
> +
> + return result;
> +}
> +
> void
> -rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> +rte_sched_port_pkt_write(struct rte_sched_port *port,
> + struct rte_mbuf *pkt,
> uint32_t subport, uint32_t pipe, uint32_t
> traffic_class,
> uint32_t queue, enum rte_meter_color color)
> {
> - struct rte_sched_port_hierarchy *sched
> - = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
> -
> - RTE_BUILD_BUG_ON(sizeof(*sched) > sizeof(pkt->hash.sched));
> -
> - sched->color = (uint32_t) color;
> - sched->subport = subport;
> - sched->pipe = pipe;
> - sched->traffic_class = traffic_class;
> - sched->queue = queue;
> + pkt->hash.sched.traffic_class = traffic_class;
> + pkt->hash.sched.queue_id = rte_sched_port_qindex(port, subport,
> pipe,
> + traffic_class, queue);
> + pkt->hash.sched.color = (uint8_t) color;
> }
>
> void
> -rte_sched_port_pkt_read_tree_path(const struct rte_mbuf *pkt,
> +rte_sched_port_pkt_read_tree_path(struct rte_sched_port *port,
> + const struct rte_mbuf *pkt,
> uint32_t *subport, uint32_t *pipe,
> uint32_t *traffic_class, uint32_t *queue)
> {
> - const struct rte_sched_port_hierarchy *sched
> - = (const struct rte_sched_port_hierarchy *) &pkt-
> >hash.sched;
> -
> - *subport = sched->subport;
> - *pipe = sched->pipe;
> - *traffic_class = sched->traffic_class;
> - *queue = sched->queue;
> + *subport = pkt->hash.sched.queue_id /
> rte_sched_port_queues_per_subport(port);
> + *pipe = pkt->hash.sched.queue_id /
> RTE_SCHED_QUEUES_PER_PIPE;
Since n_subports_per_pipe and n_pipes_per_subport are enforced to be power of
2, we should replace multiplication/division with shift left/right.
> + *traffic_class = pkt->hash.sched.traffic_class;
> + *queue = pkt->hash.sched.queue_id %
> RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
Since RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS is enforced to be a power of 2, please
replace modulo with bitwise AND of (RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS - 1).
> }
>
> enum rte_meter_color
> rte_sched_port_pkt_read_color(const struct rte_mbuf *pkt)
> {
> - const struct rte_sched_port_hierarchy *sched
> - = (const struct rte_sched_port_hierarchy *) &pkt-
> >hash.sched;
> -
> - return (enum rte_meter_color) sched->color;
> + return (enum rte_meter_color) pkt->hash.sched.color;
> }
Should use the mbuf->sched.color read function to be added in rte_mbuf.h.
>
> int
> @@ -1100,18 +1086,6 @@ rte_sched_queue_read_stats(struct
> rte_sched_port *port,
> return 0;
> }
>
> -static inline uint32_t
> -rte_sched_port_qindex(struct rte_sched_port *port, uint32_t subport,
> uint32_t pipe, uint32_t traffic_class, uint32_t queue)
> -{
> - uint32_t result;
> -
> - result = subport * port->n_pipes_per_subport + pipe;
> - result = result * RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE +
> traffic_class;
> - result = result * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + queue;
> -
> - return result;
> -
Since n_subports_per_pipe and n_pipes_per_subport are enforced to be power of
2, we should replace multiplication/division with shift left/right.
}
> -
> #ifdef RTE_SCHED_DEBUG
>
> static inline int
> @@ -1272,11 +1246,8 @@ rte_sched_port_enqueue_qptrs_prefetch0(struct
> rte_sched_port *port,
> #ifdef RTE_SCHED_COLLECT_STATS
> struct rte_sched_queue_extra *qe;
> #endif
> - uint32_t subport, pipe, traffic_class, queue, qindex;
> -
> - rte_sched_port_pkt_read_tree_path(pkt, &subport, &pipe,
> &traffic_class, &queue);
> + uint32_t qindex = pkt->hash.sched.queue_id;
Let's use the read function for this mbuf field.
>
> - qindex = rte_sched_port_qindex(port, subport, pipe, traffic_class,
> queue);
> q = port->queue + qindex;
> rte_prefetch0(q);
> #ifdef RTE_SCHED_COLLECT_STATS
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index 84fa896de..4d9f869eb 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -355,6 +355,8 @@ rte_sched_queue_read_stats(struct rte_sched_port
> *port,
> * Scheduler hierarchy path write to packet descriptor. Typically
> * called by the packet classification stage.
> *
> + * @param port
> + * Handle to port scheduler instance
> * @param pkt
> * Packet descriptor handle
> * @param subport
> @@ -369,7 +371,8 @@ rte_sched_queue_read_stats(struct rte_sched_port
> *port,
> * Packet color set
> */
> void
> -rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> +rte_sched_port_pkt_write(struct rte_sched_port *port,
> + struct rte_mbuf *pkt,
> uint32_t subport, uint32_t pipe, uint32_t
> traffic_class,
> uint32_t queue, enum rte_meter_color color);
>
> @@ -379,6 +382,8 @@ rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> * enqueue operation. The subport, pipe, traffic class and queue
> * parameters need to be pre-allocated by the caller.
> *
> + * @param port
> + * Handle to port scheduler instance
> * @param pkt
> * Packet descriptor handle
> * @param subport
> @@ -392,7 +397,8 @@ rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> *
> */
> void
> -rte_sched_port_pkt_read_tree_path(const struct rte_mbuf *pkt,
> +rte_sched_port_pkt_read_tree_path(struct rte_sched_port *port,
> + const struct rte_mbuf *pkt,
> uint32_t *subport, uint32_t *pipe,
> uint32_t *traffic_class, uint32_t *queue);
>
<snip>
Regards,
Cristian
