Hi Jasvinder,
> -----Original Message----- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Jasvinder Singh > Sent: Monday, September 18, 2017 5:10 PM > To: dev@dpdk.org > Cc: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Yigit, Ferruh > <ferruh.yi...@intel.com>; tho...@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 <cristian.dumitre...@intel.com> > Signed-off-by: Jasvinder Singh <jasvinder.si...@intel.com> > --- > 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 = dev->data->dev_private; > + struct tm_node_list *nl = &p->soft.tm.h.nodes; > + struct tm_node *ns; > + uint32_t subport_id; > + > + subport_id = 0; > + TAILQ_FOREACH(ns, nl, node) { > + if (ns->level != TM_NODE_LEVEL_SUBPORT) > + continue; > + > + if (ns->node_id == 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 == 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 = 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 == 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 = dev->data->dev_private; > + struct tm_shaper_profile_list *spl = &p->soft.tm.h.shaper_profiles; > + struct tm_shaper_profile *sp; > + int status; > + > + /* Check input params */ > + status = shaper_profile_check(dev, shaper_profile_id, profile, error); > + if (status) > + return status; > + > + /* Memory allocation */ > + sp = calloc(1, sizeof(struct tm_shaper_profile)); Just curious, why not use rte_zmalloc? > + if (sp == NULL) > + return -rte_tm_error_set(error, > + ENOMEM, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(ENOMEM)); > + > + /* Fill in */ > + sp->shaper_profile_id = 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 = dev->data->dev_private; > + struct tm_node_list *nl = &p->soft.tm.h.nodes; > + struct tm_node *n; > + > + TAILQ_FOREACH(n, nl, node) { > + if ((n->level != TM_NODE_LEVEL_TC) || > + (n->params.n_shared_shapers == 0) || > + (n->params.shared_shaper_id[0] != ss- > >shared_shaper_id)) According to node_add_check_tc, only one shared shaper supported, right? Better 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 = dev->data->dev_private; > + struct tm_hierarchy *h = &p->soft.tm.h; > + struct tm_node_list *nl = &h->nodes; > + struct tm_node *nt, *nq; > + > + memset(pp, 0, sizeof(*pp)); > + > + /* Pipe */ > + pp->tb_rate = np->shaper_profile->params.peak.rate; > + pp->tb_size = np->shaper_profile->params.peak.size; > + > + /* Traffic Class (TC) */ > + pp->tc_period = 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 = dev->data->dev_private; > + struct tm_params *t = &p->soft.tm.params; > + > + if (t->n_pipe_profiles < RTE_SCHED_PIPE_PROFILES_PER_PORT) { > + *pipe_profile_id = t->n_pipe_profiles; > + return 1; Returning true or false is easier to understand? Also the same concern of the naming as patch 3.
