From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E14E3A0487 for ; Tue, 2 Jul 2019 01:25:35 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0B9E41B998; Tue, 2 Jul 2019 01:25:35 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 98F4C1B996 for ; Tue, 2 Jul 2019 01:25:33 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Jul 2019 16:25:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,441,1557212400"; d="scan'208";a="362000836" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by fmsmga005.fm.intel.com with ESMTP; 01 Jul 2019 16:25:31 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.46]) by IRSMSX109.ger.corp.intel.com ([169.254.13.220]) with mapi id 14.03.0439.000; Tue, 2 Jul 2019 00:25:30 +0100 From: "Dumitrescu, Cristian" To: "Singh, Jasvinder" , "dev@dpdk.org" CC: "Tovar, AbrahamX" , "Krakowiak, LukaszX" Thread-Topic: [PATCH v2 09/28] sched: update pkt read and write API Thread-Index: AQHVK2sxb7xvKII31kWLiN8L8eC5L6a2bZLA Date: Mon, 1 Jul 2019 23:25:29 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891268E8B8D8E@IRSMSX108.ger.corp.intel.com> References: <20190528120553.2992-2-lukaszx.krakowiak@intel.com> <20190625153217.24301-1-jasvinder.singh@intel.com> <20190625153217.24301-10-jasvinder.singh@intel.com> In-Reply-To: <20190625153217.24301-10-jasvinder.singh@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTg3YmNlZDktZWM4ZC00MmFjLTkwM2ItZDcwYjhmYTQ2ZjFlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoid2xsVHVUYzNYSE1ZWEd0SmU3blBNaTY2Yjgzc3dqcnp0eDd0aUE1WjdEZGFUNjJUNlF6N0l1SDVlanNqWDhmNCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 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 v2 09/28] sched: update pkt read and write API 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Singh, Jasvinder > Sent: Tuesday, June 25, 2019 4:32 PM > To: dev@dpdk.org > Cc: Dumitrescu, Cristian ; Tovar, AbrahamX > ; Krakowiak, LukaszX > > Subject: [PATCH v2 09/28] sched: update pkt read and write API >=20 > Update run time packet read and write api implementation > to allow configuration flexiblity for pipe traffic classes > and queues, and subport level configuration of the pipe > parameters. >=20 > Signed-off-by: Jasvinder Singh > Signed-off-by: Abraham Tovar > Signed-off-by: Lukasz Krakowiak > --- > lib/librte_sched/rte_sched.c | 32 +++++++++++++++++--------------- > lib/librte_sched/rte_sched.h | 8 ++++---- > 2 files changed, 21 insertions(+), 19 deletions(-) >=20 > diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c > index 1999bbfa3..cd82fd918 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -1433,17 +1433,15 @@ rte_sched_port_pipe_profile_add(struct > rte_sched_port *port, >=20 > static inline uint32_t > rte_sched_port_qindex(struct rte_sched_port *port, > + struct rte_sched_subport *s, > uint32_t subport, > uint32_t pipe, > - uint32_t traffic_class, > uint32_t queue) > { > return ((subport & (port->n_subports_per_port - 1)) << > - (port->n_pipes_per_subport_log2 + 4)) | > - ((pipe & (port->n_pipes_per_subport - 1)) << 4) | > - ((traffic_class & > - (RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE - 1)) << > 2) | > - (queue & > (RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS - 1)); > + (port->max_subport_pipes_log2 + 4)) | > + ((pipe & (s->n_subport_pipes - 1)) << 4) | > + (queue & (RTE_SCHED_QUEUES_PER_PIPE - 1)); > } >=20 This function contains a critical bug: this patchset proposes that the numb= er of pipes per subport is configurable independently for each subport; in = other words, each subport can be configured with a different number of pipe= s. Therefore, the above logic is broken, as it assumes all subports have th= e same number of pipes. There is no longer possible to compute port->max_su= bport_pipes_log2. Correct? We might need to rethink the design solution for the per-subport independen= t configuration. We also need to make sure we test this library with multiple subports per p= ort, with each subport having different number of pipes. Need to do the bas= ic uni test proposed earlier to trace the packet through the scheduler hier= archy up to the packet queue. > void > @@ -1453,9 +1451,9 @@ rte_sched_port_pkt_write(struct rte_sched_port > *port, > uint32_t traffic_class, > uint32_t queue, enum rte_color color) > { > - uint32_t queue_id =3D rte_sched_port_qindex(port, subport, pipe, > - traffic_class, queue); > - rte_mbuf_sched_set(pkt, queue_id, traffic_class, (uint8_t)color); > + struct rte_sched_subport *s =3D port->subports[subport]; > + uint32_t qindex =3D rte_sched_port_qindex(port, s, subport, pipe, > queue); > + rte_mbuf_sched_set(pkt, qindex, traffic_class, (uint8_t)color); > } >=20 Same comment here. > void > @@ -1464,13 +1462,17 @@ rte_sched_port_pkt_read_tree_path(struct > rte_sched_port *port, > uint32_t *subport, uint32_t *pipe, > uint32_t *traffic_class, uint32_t *queue) > { > - uint32_t queue_id =3D rte_mbuf_sched_queue_get(pkt); > + struct rte_sched_subport *s; > + uint32_t qindex =3D rte_mbuf_sched_queue_get(pkt); > + uint32_t tc_id =3D rte_mbuf_sched_traffic_class_get(pkt); > + > + *subport =3D (qindex >> (port->max_subport_pipes_log2 + 4)) & > + (port->n_subports_per_port - 1); >=20 > - *subport =3D queue_id >> (port->n_pipes_per_subport_log2 + 4); > - *pipe =3D (queue_id >> 4) & (port->n_pipes_per_subport - 1); > - *traffic_class =3D (queue_id >> 2) & > - (RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE - > 1); > - *queue =3D queue_id & (RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS - > 1); > + s =3D port->subports[*subport]; > + *pipe =3D (qindex >> 4) & (s->n_subport_pipes - 1); > + *traffic_class =3D tc_id; > + *queue =3D qindex & (RTE_SCHED_QUEUES_PER_PIPE - 1); > } >=20 Same comment here. > enum rte_color > diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h > index 121e1f669..6a6ea84aa 100644 > --- a/lib/librte_sched/rte_sched.h > +++ b/lib/librte_sched/rte_sched.h > @@ -421,9 +421,9 @@ rte_sched_queue_read_stats(struct rte_sched_port > *port, > * @param pipe > * Pipe ID within subport > * @param traffic_class > - * Traffic class ID within pipe (0 .. 3) > + * Traffic class ID within pipe (0 .. 8) > * @param queue > - * Queue ID within pipe traffic class (0 .. 3) > + * Queue ID within pipe traffic class (0 .. 15) > * @param color > * Packet color set > */ > @@ -448,9 +448,9 @@ rte_sched_port_pkt_write(struct rte_sched_port > *port, > * @param pipe > * Pipe ID within subport > * @param traffic_class > - * Traffic class ID within pipe (0 .. 3) > + * Traffic class ID within pipe (0 .. 8) > * @param queue > - * Queue ID within pipe traffic class (0 .. 3) > + * Queue ID within pipe traffic class (0 .. 15) > * > */ > void > -- > 2.21.0