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 0C875A0487 for ; Wed, 3 Jul 2019 15:41:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4655C44C3; Wed, 3 Jul 2019 15:40:55 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id A4F6F25D9 for ; Wed, 3 Jul 2019 15:40:49 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jul 2019 06:40:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,446,1557212400"; d="scan'208";a="164348906" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by fmsmga008.fm.intel.com with ESMTP; 03 Jul 2019 06:40:47 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.46]) by irsmsx105.ger.corp.intel.com ([169.254.7.184]) with mapi id 14.03.0439.000; Wed, 3 Jul 2019 14:40:47 +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: AQHVK2sxb7xvKII31kWLiN8L8eC5L6a2bZLAgAFegQCAASWYAA== Date: Wed, 3 Jul 2019 13:40:46 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891268E8B998A@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> <3EB4FA525960D640B5BDFFD6A3D891268E8B8D8E@IRSMSX108.ger.corp.intel.com> <54CBAA185211B4429112C315DA58FF6D3FD6CBDA@IRSMSX103.ger.corp.intel.com> In-Reply-To: <54CBAA185211B4429112C315DA58FF6D3FD6CBDA@IRSMSX103.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.2.0.6 dlp-reaction: no-action x-originating-ip: [163.33.239.182] 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, July 2, 2019 10:05 PM > To: Dumitrescu, Cristian ; 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: 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 > > > > > > > > > -----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)); > > > } > > > > > > > This function contains a critical bug: this patchset proposes that the = number > of > > pipes per subport is configurable independently for each subport; in ot= her > > words, each subport can be configured with a different number of pipes. > > Therefore, the above logic is broken, as it assumes all subports have t= he > same > > number of pipes. There is no longer possible to compute port- > > >max_subport_pipes_log2. Correct? >=20 > Yes, you are right. I didn't realize this issue. >=20 > > > > We might need to rethink the design solution for the per-subport > independent > > configuration. >=20 > One option to get around this is by computing start_queue_offset for eac= h > subport and store that in rte_sched_subport data structure during subport > configuration. >=20 Yes, it can be done, this is probably the only way to get it done, but it w= ould severely impact the performance, as in order to determine the subport = ID you'd have to iterate through the list of subports to search where this = queue ID fits. > During run time, when writing pkt metadata; add that offset to calculate > queue 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 mbu= f- > >hash.sched I am afraid we cannot write subport ID into mbuf->sched, as subport ID is n= ot generic, it is only specific to librte_sched feature set. This will poll= ute the generic mbuf->sched with librte_sched implementation details. >=20 > During packet read, offset value is retrieved from subport_id (from reser= ved > field). By subtracting offset from the qindex, > pipe, tc and queue id can determined from the remaining value. >=20 > This will allow contiguous value of the queue id at the port level. >=20 > > We also need to make sure we test this library with multiple subports p= er > port, > > with each subport having different number of pipes. Need to do the basi= c > uni > > test proposed earlier to trace the packet through the scheduler hierarc= hy > up to > > the packet queue. >=20 > Yes, will add unit test for this case. > > > > > 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); > > > } > > > > > > > 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); > > > > > > - *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); > > > } > > > > > > > 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