RE: [PATCH v6 04/15] graph: add get/set graph worker model APIs

["Yan, Zhirun" <zhirun.yan@intel.com>]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, May 24, 2023 2:09 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com;
> ndabilpuram@marvell.com; stephen@networkplumber.org;
> pbhagavatula@marvell.com; Liang, Cunming <cunming.liang@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>
> Subject: Re: [PATCH v6 04/15] graph: add get/set graph worker model APIs
> 
> On Tue, May 9, 2023 at 11:34 AM Zhirun Yan <zhirun.yan@intel.com> wrote:
> >
> > Add new get/set APIs to configure graph worker model which is used to
> > determine which model will be chosen.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> > ---
> > diff --git a/lib/graph/rte_graph_worker.c
> > b/lib/graph/rte_graph_worker.c new file mode 100644 index
> > 0000000000..cabc101262
> > --- /dev/null
> > +++ b/lib/graph/rte_graph_worker.c
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Intel Corporation  */
> > +
> > +#include "rte_graph_worker_common.h"
> > +
> > +RTE_DEFINE_PER_LCORE(enum rte_graph_worker_model, worker_model) =
> > +RTE_GRAPH_MODEL_DEFAULT;
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > +notice
> > + * Set the graph worker model
> 
> Just declaring this top of the header file enough to avoid duplicating in every
> functions as all functions in header is experimental. See lib/graph/rte_graph.h
> 
Got it, I will do in next version.

> 
> > + *
> > + * @note This function does not perform any locking, and is only safe to call
> > + *    before graph running.
> > + *
> > + * @param name
> > + *   Name of the graph worker model.
> > + *
> > + * @return
> > + *   0 on success, -1 otherwise.
> > + */
> > +int
> > +rte_graph_worker_model_set(enum rte_graph_worker_model model) {
> > +       if (model >= RTE_GRAPH_MODEL_LIST_END)
> > +               goto fail;
> > +
> > +       RTE_PER_LCORE(worker_model) = model;
> 
> Application needs to set this per core . Right?

Yes. Each worker needs to know its model.

> Are we anticipating a case where one core runs one model and another core
> runs with another model?
> If not OR it is not practically possible, then,  To make application programmer
> life easy, We could loop through all lore and set on all of them instead of
> application setting on each one separately.
> 

For current rtc and dispatch models, it is not necessary.
To some extent that models are mutually exclusive.

For this case:
    Core 1: A->B->C (RTC)

    Core 2: A' (DISPATCH)
    Core 3: B' (DISPATCH)
    Core 4: C' (DISPATCH)

It may change the graph topo,  or need some prerequisites like RSS before input node A.

BTW, if there are some requirements with more models in future, we could add some attributions for graph, lcore, node.
Like taint/affinity for node and model. Then we could allow a node to appeal/repel a set of models. 

I will change to put the model into struct rte_graph as you suggested in patch 12  for this release.

> 
> > +       return 0;
> > +
> > +fail:
> > +       RTE_PER_LCORE(worker_model) = RTE_GRAPH_MODEL_DEFAULT;
> > +       return -1;
> > +}
> > +
> 
> > +/** Graph worker models */
> > +enum rte_graph_worker_model {
> > +       RTE_GRAPH_MODEL_DEFAULT,
> 
> Add Doxygen comment
> > +       RTE_GRAPH_MODEL_RTC = RTE_GRAPH_MODEL_DEFAULT,
> 
> 
> Add Doxygen comment to explain what this mode does.
> 
> 
> > +       RTE_GRAPH_MODEL_MCORE_DISPATCH,
> 
> Add Doxygen comment to explain what this mode does.
> 
Ok, I will add Doxygen comments for these models.

> > +       RTE_GRAPH_MODEL_LIST_END
> 
> This can break the ABI if we add one in middle. Please remove this.
> See lib/crytodev for
> how to handle with _END symbols.

Yes, I will remove this.