From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id C2CE6A0487
for <public@inbox.dpdk.org>; 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 <dev@dpdk.org>; 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" <jasvinder.singh@intel.com>
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>, "dev@dpdk.org"
<dev@dpdk.org>
CC: "Tovar, AbrahamX" <abrahamx.tovar@intel.com>, "Krakowiak, LukaszX"
<lukaszx.krakowiak@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Tuesday, July 2, 2019 12:25 AM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; dev@dpdk.org
> Cc: Tovar, AbrahamX <abrahamx.tovar@intel.com>; Krakowiak, LukaszX
> <lukaszx.krakowiak@intel.com>
> 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 <cristian.dumitrescu@intel.com>; Tovar,
> > AbrahamX <abrahamx.tovar@intel.com>; Krakowiak, LukaszX
> > <lukaszx.krakowiak@intel.com>
> > 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 <jasvinder.singh@intel.com>
> > Signed-off-by: Abraham Tovar <abrahamx.tovar@intel.com>
> > Signed-off-by: Lukasz Krakowiak <lukaszx.krakowiak@intel.com>
> > ---
> > 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