> 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.
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 == 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.
We preferred to perform checks as per the arguments order specified in the
function definition so that all the parameter could
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 = 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?
This relates to high level hierarchy specification objects which doesn't need
to be allocated on specific
numa node as it is not used once the hierarchy is committed. All these objects
gets eventually translated into
TM implementation (librte_sched) specific objects. These objects are allocated
using rte_zmalloc and needs
lots of memory (approx. 2M mbufs for single instance of TM hierarchy ports) on
specific numa node.
> > + 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?
The subport has 4 shared shapers for each of the pipe traffic classes. Will add
comment.
> > + 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?
will add macro. Thanks.
> > +
> > +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?
Ok.
> Also the same concern of the naming as patch 3.
Ok. Will bring clarity in the names.
Thanks for the comments and time.
