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 803212B9C for ; Mon, 25 Sep 2017 09:14:37 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Sep 2017 00:14:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,435,1500966000"; d="scan'208";a="1175470408" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 25 Sep 2017 00:14:29 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 25 Sep 2017 00:14:29 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 25 Sep 2017 00:14:28 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Mon, 25 Sep 2017 15:14:26 +0800 From: "Lu, Wenzhuo" To: "Singh, Jasvinder" , "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: AQHTMFyJX46E6T6/MkuvP5B7Y0frSaLFHgng Date: Mon, 25 Sep 2017 07:14:25 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC09093B6BD299@shsmsx102.ccr.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> In-Reply-To: <20170918091015.82824-5-jasvinder.singh@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] 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: Mon, 25 Sep 2017 07:14:38 -0000 Hi Jasvinder, > -----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 >=20 > Implement ethdev TM hierarchy related APIs in SoftNIC PMD. >=20 > 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(-) > + > +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. > +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. > + > +/* 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, error)= ; > + if (status) > + return status; > + > + /* Memory allocation */ > + sp =3D calloc(1, sizeof(struct tm_shaper_profile)); Just curious, why not use rte_zmalloc? > + 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; > +} > + > + > +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? Be= tter adding some comments here? > + continue; > + > + return n; > + } > + > + return NULL; > +} > + > +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? > + > +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? Also the same concern of the naming as patch 3.