From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 5649A276C for ; Thu, 28 Sep 2017 10:39:39 +0200 (CEST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Sep 2017 01:39:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,449,1500966000"; d="scan'208";a="154294740" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga005.jf.intel.com with ESMTP; 28 Sep 2017 01:39:36 -0700 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.49]) by IRSMSX107.ger.corp.intel.com ([169.254.10.65]) with mapi id 14.03.0319.002; Thu, 28 Sep 2017 09:39:35 +0100 From: "Singh, Jasvinder" To: "Lu, Wenzhuo" , "dev@dpdk.org" CC: "Dumitrescu, Cristian" , "Yigit, Ferruh" , "thomas@monjalon.net" Thread-Topic: [dpdk-dev] [PATCH v4 4/4] net/softnic: add TM hierarchy related ops Thread-Index: AQHTMFyJuhOmh124SU2OpD8SYun+1KLFKlmAgATZKQA= Date: Thu, 28 Sep 2017 08:39:34 +0000 Message-ID: <54CBAA185211B4429112C315DA58FF6D3325C667@IRSMSX103.ger.corp.intel.com> References: <20170811124929.118564-2-jasvinder.singh@intel.com> <20170918091015.82824-1-jasvinder.singh@intel.com> <20170918091015.82824-5-jasvinder.singh@intel.com> <6A0DE07E22DDAD4C9103DF62FEBC09093B6BD299@shsmsx102.ccr.corp.intel.com> In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093B6BD299@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmU1ZjllZDAtMTNmNC00Y2U5LWIzZWEtOTM2NjViOWNjZWI3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Im1iY2N4SEFGakRCdGRBTmhtaVhLb1BXZXp3QWQ1UmpMZ2I1b1VUbm53QU09In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 4/4] net/softnic: add TM hierarchy related ops 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, 28 Sep 2017 08:39:39 -0000 > Hi Jasvinder, >=20 >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jasvinder Singh > > Sent: Monday, September 18, 2017 5:10 PM > > To: dev@dpdk.org > > Cc: Dumitrescu, Cristian ; Yigit, > > Ferruh ; thomas@monjalon.net > > Subject: [dpdk-dev] [PATCH v4 4/4] net/softnic: add TM hierarchy > > related ops > > > > Implement ethdev TM hierarchy related APIs in SoftNIC PMD. > > > > Signed-off-by: Cristian Dumitrescu > > Signed-off-by: Jasvinder Singh > > --- > > drivers/net/softnic/rte_eth_softnic_internals.h | 41 + > > drivers/net/softnic/rte_eth_softnic_tm.c | 2776 > > ++++++++++++++++++++++- > > 2 files changed, 2813 insertions(+), 4 deletions(-) >=20 >=20 > > + > > +static uint32_t > > +tm_node_subport_id(struct rte_eth_dev *dev, struct tm_node > > *subport_node) > > +{ > > + struct pmd_internals *p =3D dev->data->dev_private; > > + struct tm_node_list *nl =3D &p->soft.tm.h.nodes; > > + struct tm_node *ns; > > + uint32_t subport_id; > > + > > + subport_id =3D 0; > > + TAILQ_FOREACH(ns, nl, node) { > > + if (ns->level !=3D TM_NODE_LEVEL_SUBPORT) > > + continue; > > + > > + if (ns->node_id =3D=3D subport_node->node_id) > > + return subport_id; > > + > > + subport_id++; > > + } > > + > > + return UINT32_MAX; > UINT32_MAX means invalid number, right? Better define a specific MACRO > for the invalid number in case you may not want to use 0xff.. or uint32. > The same suggestion for the below functions. Ok. > > +static int > > +shaper_profile_check(struct rte_eth_dev *dev, > > + uint32_t shaper_profile_id, > > + struct rte_tm_shaper_params *profile, > > + struct rte_tm_error *error) > > +{ > > + struct tm_shaper_profile *sp; > > + > > + /* Shaper profile ID must not be NONE. */ > > + if (shaper_profile_id =3D=3D RTE_TM_SHAPER_PROFILE_ID_NONE) > > + return -rte_tm_error_set(error, > > + EINVAL, > > + RTE_TM_ERROR_TYPE_SHAPER_PROFILE_ID, > > + NULL, > > + rte_strerror(EINVAL)); > > + > > + /* Shaper profile must not exist. */ > > + sp =3D tm_shaper_profile_search(dev, shaper_profile_id); > > + if (sp) > > + return -rte_tm_error_set(error, > > + EEXIST, > > + RTE_TM_ERROR_TYPE_SHAPER_PROFILE_ID, > > + NULL, > > + rte_strerror(EEXIST)); > > + > > + /* Profile must not be NULL. */ > > + if (profile =3D=3D NULL) > > + return -rte_tm_error_set(error, > > + EINVAL, > > + RTE_TM_ERROR_TYPE_SHAPER_PROFILE, > > + NULL, > > + rte_strerror(EINVAL)); > A slight suggestion. We can do the easiest check at first. We preferred to perform checks as per the arguments order specified in the = function definition so that all the parameter could=20 be scanned in a systematic manner instead of picking them randomly. > > + > > +/* Traffic manager shaper profile add */ static int > > +pmd_tm_shaper_profile_add(struct rte_eth_dev *dev, > > + uint32_t shaper_profile_id, > > + struct rte_tm_shaper_params *profile, > > + struct rte_tm_error *error) > > +{ > > + struct pmd_internals *p =3D dev->data->dev_private; > > + struct tm_shaper_profile_list *spl =3D &p->soft.tm.h.shaper_profiles; > > + struct tm_shaper_profile *sp; > > + int status; > > + > > + /* Check input params */ > > + status =3D shaper_profile_check(dev, shaper_profile_id, profile, erro= r); > > + if (status) > > + return status; > > + > > + /* Memory allocation */ > > + sp =3D calloc(1, sizeof(struct tm_shaper_profile)); > Just curious, why not use rte_zmalloc? This relates to high level hierarchy specification objects which doesn't ne= ed to be allocated on specific numa node as it is not used once the hierarchy is committed. All these obje= cts gets eventually translated into TM implementation (librte_sched) specific objects. These objects are alloc= ated using rte_zmalloc and needs lots of memory (approx. 2M mbufs for single instance of TM hierarchy ports)= on specific numa node. > > + if (sp =3D=3D NULL) > > + return -rte_tm_error_set(error, > > + ENOMEM, > > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > > + NULL, > > + rte_strerror(ENOMEM)); > > + > > + /* Fill in */ > > + sp->shaper_profile_id =3D shaper_profile_id; > > + memcpy(&sp->params, profile, sizeof(sp->params)); > > + > > + /* Add to list */ > > + TAILQ_INSERT_TAIL(spl, sp, node); > > + p->soft.tm.h.n_shaper_profiles++; > > + > > + return 0; > > +} > > + >=20 > > + > > +static struct tm_node * > > +tm_shared_shaper_get_tc(struct rte_eth_dev *dev, > > + struct tm_shared_shaper *ss) > > +{ > > + struct pmd_internals *p =3D dev->data->dev_private; > > + struct tm_node_list *nl =3D &p->soft.tm.h.nodes; > > + struct tm_node *n; > > + > > + TAILQ_FOREACH(n, nl, node) { > > + if ((n->level !=3D TM_NODE_LEVEL_TC) || > > + (n->params.n_shared_shapers =3D=3D 0) || > > + (n->params.shared_shaper_id[0] !=3D ss- > > >shared_shaper_id)) > According to node_add_check_tc, only one shared shaper supported, right? > Better adding some comments here? The subport has 4 shared shapers for each of the pipe traffic classes. Will= add comment. > > + continue; > > + > > + return n; > > + } > > + > > + return NULL; > > +} >=20 >=20 >=20 > > + > > +static void > > +pipe_profile_build(struct rte_eth_dev *dev, > > + struct tm_node *np, > > + struct rte_sched_pipe_params *pp) > > +{ > > + struct pmd_internals *p =3D dev->data->dev_private; > > + struct tm_hierarchy *h =3D &p->soft.tm.h; > > + struct tm_node_list *nl =3D &h->nodes; > > + struct tm_node *nt, *nq; > > + > > + memset(pp, 0, sizeof(*pp)); > > + > > + /* Pipe */ > > + pp->tb_rate =3D np->shaper_profile->params.peak.rate; > > + pp->tb_size =3D np->shaper_profile->params.peak.size; > > + > > + /* Traffic Class (TC) */ > > + pp->tc_period =3D 40; > 40 means? A MACRO is better? will add macro. Thanks. > > + > > +static int > > +pipe_profile_free_exists(struct rte_eth_dev *dev, > > + uint32_t *pipe_profile_id) > > +{ > > + struct pmd_internals *p =3D dev->data->dev_private; > > + struct tm_params *t =3D &p->soft.tm.params; > > + > > + if (t->n_pipe_profiles < RTE_SCHED_PIPE_PROFILES_PER_PORT) { > > + *pipe_profile_id =3D t->n_pipe_profiles; > > + return 1; > Returning true or false is easier to understand? Ok.=20 > Also the same concern of the naming as patch 3. Ok. Will bring clarity in the names. Thanks for the comments and time.