From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id D82C556A3 for ; Thu, 29 Nov 2018 11:42:45 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Nov 2018 02:42:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,294,1539673200"; d="scan'208";a="289679217" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga005.fm.intel.com with ESMTP; 29 Nov 2018 02:42:43 -0800 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.253]) by IRSMSX102.ger.corp.intel.com ([169.254.2.13]) with mapi id 14.03.0415.000; Thu, 29 Nov 2018 10:42:42 +0000 From: "Singh, Jasvinder" To: "Dumitrescu, Cristian" , "dev@dpdk.org" CC: "Pattan, Reshma" Thread-Topic: [PATCH] mbuf: implement generic format for sched field Thread-Index: AQHUhXxt1GJdiKEPhUqRKZE7fTjyMKVmjkuw Date: Thu, 29 Nov 2018 10:42:42 +0000 Message-ID: <54CBAA185211B4429112C315DA58FF6D33666245@IRSMSX103.ger.corp.intel.com> References: <20181123165423.134922-1-jasvinder.singh@intel.com> <3EB4FA525960D640B5BDFFD6A3D891268E800A9E@IRSMSX108.ger.corp.intel.com> In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891268E800A9E@IRSMSX108.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMWNjMTgwY2YtM2RkOC00ZDljLWE3MTctYzA2OTZiODdjYjc1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiS2hpaXlxd3d0QXF3QUxhREFxcXNkNkdhMWwwWnpRTWczR3h1NzlMa3RkZHFlKzhJM0p3Z1hEOFd5Z2p6cG5qZCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] mbuf: implement generic format for sched field X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Nov 2018 10:42:46 -0000 >=20 > > 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. */ >=20 > We should add comment here that traffic class 0 is the highest priority t= raffic > class. Will add the suggested comment. >=20 > > + uint8_t color; /**< Color. */ >=20 > 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. >=20 > We should include the rte_color.h file here (and in the above header file= s) >=20 > We should also document the link between this field and the color enum ty= pe > (@see ...). Replacing the existing color definition with the above suggested one in rte= _meter.h (librte_meter) would be ABI break. We can do it separately through different patch. >=20 > > + uint16_t reserved; /**< Reserved. */ > > } sched; /**< Hierarchical scheduler */ > > /**< User defined tags. See > > rte_distributor_process() */ > > uint32_t usr; >=20 > We should also add trivial inline functions to read/write these mbuf fiel= ds as > part of this header file. We want to discourage people from accessing the= se > fields directly. >=20 > Besides the functions to read/write each field individually, we should al= so > 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. Will create inline functions for reading and writing each field separately = and jointly. =20 >=20 > > 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)) > > >=20 > Given the read/write mbuf->sched field functions, the above two macros ar= e > no longer needed. Will remove this. > > 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 =3D 0; > > data->subport =3D (uint16_t) p->subport_id; > > data->pipe =3D 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 =3D data->subport * cfg->n_pipes_per_subport + data->pipe; >=20 > 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/rig= ht. We > probably need to store log2 correspondents in the action context. Thanks. Will store log2 component in action context and use them in run tim= e for shift operations. > > + result =3D result * RTE_TABLE_ACTION_TC_MAX + dscp->tc; > > + result =3D 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 =3D &dscp_table- > > >entry[dscp]; > > - struct tm_data *sched_ptr =3D (struct tm_data *) &mbuf->hash.sched; > > - struct tm_data sched; > > + uint64_t *sched_ptr =3D (uint64_t *) &mbuf->hash.sched; > > + uint32_t queue =3D tm_sched_qindex(data, dscp_entry, cfg); > > > > - sched =3D *data; > > - sched.queue_tc_color =3D dscp_entry->queue_tc_color; > > - *sched_ptr =3D sched; > > + *sched_ptr =3D 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 =3D > > &table->entry[i]; > > - uint16_t queue_tc_color =3D > > - MBUF_SCHED_QUEUE_TC_COLOR(entry- > > >tc_queue_id, > > - entry->tc_id, > > - entry->color); > > > > if ((dscp_mask & (1LLU << i)) =3D=3D 0) > > continue; > > > > data->color =3D entry->color; > > data->tc =3D entry->tc_id; > > - data->queue_tc_color =3D queue_tc_color; > > + data->queue =3D 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 +=3D -lrte_timer > > > > EXPORT_MAP :=3D rte_sched_version.map > > > > -LIBABIVER :=3D 1 > > +LIBABIVER :=3D 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 =3D subport * port->n_pipes_per_subport + pipe; >=20 > 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/rig= ht. Will replace with shift operation using log2 of n_subports_per_pipe and n_p= ipes_per_subport. > > + result =3D result * RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE + > > traffic_class; > > + result =3D 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 > > - =3D (struct rte_sched_port_hierarchy *) &pkt->hash.sched; > > - > > - RTE_BUILD_BUG_ON(sizeof(*sched) > sizeof(pkt->hash.sched)); > > - > > - sched->color =3D (uint32_t) color; > > - sched->subport =3D subport; > > - sched->pipe =3D pipe; > > - sched->traffic_class =3D traffic_class; > > - sched->queue =3D queue; > > + pkt->hash.sched.traffic_class =3D traffic_class; > > + pkt->hash.sched.queue_id =3D rte_sched_port_qindex(port, subport, > > pipe, > > + traffic_class, queue); > > + pkt->hash.sched.color =3D (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 > > - =3D (const struct rte_sched_port_hierarchy *) &pkt- > > >hash.sched; > > - > > - *subport =3D sched->subport; > > - *pipe =3D sched->pipe; > > - *traffic_class =3D sched->traffic_class; > > - *queue =3D sched->queue; > > + *subport =3D pkt->hash.sched.queue_id / > > rte_sched_port_queues_per_subport(port); > > + *pipe =3D pkt->hash.sched.queue_id / > > RTE_SCHED_QUEUES_PER_PIPE; >=20 > 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/rig= ht. Will do that. > > + *traffic_class =3D pkt->hash.sched.traffic_class; > > + *queue =3D pkt->hash.sched.queue_id % > > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS; >=20 > 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). >=20 > > } Will do that. > > enum rte_meter_color > > rte_sched_port_pkt_read_color(const struct rte_mbuf *pkt) { > > - const struct rte_sched_port_hierarchy *sched > > - =3D (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; > > } >=20 > Should use the mbuf->sched.color read function to be added in rte_mbuf.h. Yes. Will change this. > > 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 =3D subport * port->n_pipes_per_subport + pipe; > > - result =3D result * RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE + > > traffic_class; > > - result =3D result * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + queue; > > - > > - return result; > > - >=20 > 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/rig= ht. Will change this as well. > } > > - > > #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 =3D pkt->hash.sched.queue_id; >=20 > Let's use the read function for this mbuf field. Yes. > > > > - qindex =3D rte_sched_port_qindex(port, subport, pipe, traffic_class, > > queue); > > q =3D 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); > > >=20 > >=20 > Regards, > Cristian We will work on the suggested changes and send another version. Thank you, = Cristian. =20