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 C2CE6A0487 for ; Tue, 2 Jul 2019 23:05:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 32D291BDE6; Tue, 2 Jul 2019 23:05:17 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id BF1F31BC07 for ; Tue, 2 Jul 2019 23:05:15 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Jul 2019 14:05:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,444,1557212400"; d="scan'208";a="362780850" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by fmsmga006.fm.intel.com with ESMTP; 02 Jul 2019 14:05:13 -0700 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.140]) by IRSMSX104.ger.corp.intel.com ([169.254.5.143]) with mapi id 14.03.0439.000; Tue, 2 Jul 2019 22:05:12 +0100 From: "Singh, Jasvinder" To: "Dumitrescu, Cristian" , "dev@dpdk.org" CC: "Tovar, AbrahamX" , "Krakowiak, LukaszX" Thread-Topic: [PATCH v2 09/28] sched: update pkt read and write API Thread-Index: AQHVMGRFzMhAmqr0EkyoI5nbD8lFdqa3iC6Q Date: Tue, 2 Jul 2019 21:05:12 +0000 Message-ID: <54CBAA185211B4429112C315DA58FF6D3FD6CBDA@IRSMSX103.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> <3EB4FA525960D640B5BDFFD6A3D891268E8B8D8E@IRSMSX108.ger.corp.intel.com> In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891268E8B8D8E@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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTg3YmNlZDktZWM4ZC00MmFjLTkwM2ItZDcwYjhmYTQ2ZjFlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoid2xsVHVUYzNYSE1ZWEd0SmU3blBNaTY2Yjgzc3dqcnp0eDd0aUE1WjdEZGFUNjJUNlF6N0l1SDVlanNqWDhmNCJ9 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 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: Dumitrescu, Cristian > Sent: Tuesday, July 2, 2019 12:25 AM > To: Singh, Jasvinder ; dev@dpdk.org > Cc: Tovar, AbrahamX ; Krakowiak, LukaszX > > Subject: RE: [PATCH v2 09/28] sched: update pkt read and write API >=20 >=20 >=20 > > -----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 > > > > 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. > > > > 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(-) > > > > 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, > > > > 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 nu= mber of > pipes per subport is configurable independently for each subport; in othe= r > words, each subport can be configured with a different number of pipes. > Therefore, the above logic is broken, as it assumes all subports have the= same > number of pipes. There is no longer possible to compute port- > >max_subport_pipes_log2. Correct? Yes, you are right. I didn't realize this issue. =20 >=20 > We might need to rethink the design solution for the per-subport independ= ent > configuration. One option to get around this is by computing start_queue_offset for each = subport and store that in rte_sched_subport data structure during subport c= onfiguration. During run time, when writing pkt metadata; add that offset to calculate q= ueue index ; subport->start_queue_offset+((pipe & (s->n_pipes_per_subport - 1)) << 4) |= (queue & (RTE_SCHED_QUEUES_PER_PIPE - 1)); At the same time, subport id can be written to reserved field of the mbuf-= >hash.sched=20 During packet read, offset value is retrieved from subport_id (from reserve= d field). By subtracting offset from the qindex,=20 pipe, tc and queue id can determined from the remaining value. This will allow contiguous value of the queue id at the port level. > We also need to make sure we test this library with multiple subports per= port, > with each subport having different number of pipes. Need to do the basic = uni > test proposed earlier to trace the packet through the scheduler hierarchy= up to > the packet queue. Yes, will add unit test for this case. >=20 > > 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. >=20 > > 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); > > > > - *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. >=20 > > 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