On Fri, Feb 24, 2023 at 12:01 PM Yan, Zhirun <zhirun....@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjac...@gmail.com>
> > Sent: Monday, February 20, 2023 9:51 PM
> > To: Yan, Zhirun <zhirun....@intel.com>
> > Cc: dev@dpdk.org; jer...@marvell.com; kirankum...@marvell.com;
> > ndabilpu...@marvell.com; Liang, Cunming <cunming.li...@intel.com>; Wang,
> > Haiyue <haiyue.w...@intel.com>
> > Subject: Re: [PATCH v1 04/13] graph: add get/set graph worker model APIs
> >
> > On Thu, Nov 17, 2022 at 10:40 AM Zhirun Yan <zhirun....@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.w...@intel.com>
> > > Signed-off-by: Cunming Liang <cunming.li...@intel.com>
> > > Signed-off-by: Zhirun Yan <zhirun....@intel.com>
> > > ---
> > > lib/graph/rte_graph_worker.h | 51 +++++++++++++++++++++++++++++
> > > lib/graph/rte_graph_worker_common.h | 13 ++++++++
> > > lib/graph/version.map | 3 ++
> > > 3 files changed, 67 insertions(+)
> > >
> > > diff --git a/lib/graph/rte_graph_worker.h
> > > b/lib/graph/rte_graph_worker.h index 54d1390786..a0ea0df153 100644
> > > --- a/lib/graph/rte_graph_worker.h
> > > +++ b/lib/graph/rte_graph_worker.h
> > > @@ -1,5 +1,56 @@
> > > #include "rte_graph_model_rtc.h"
> > >
> > > +static enum rte_graph_worker_model worker_model =
> > > +RTE_GRAPH_MODEL_DEFAULT;
> >
> > This will break the multiprocess.
>
> Thanks. I will use TLS for per-thread local storage.
If it needs to be used from secondary process, then it needs to be from memzone.
>
> >
> > > +
> > > +/** Graph worker models */
> > > +enum rte_graph_worker_model {
> > > +#define WORKER_MODEL_DEFAULT "default"
> >
> > Why need strings?
> > Also, every symbol in a public header file should start with RTE_ to avoid
> > namespace conflict.
>
> It was used to config the model in app. I can put the string into example.
OK
>
> >
> > > + RTE_GRAPH_MODEL_DEFAULT = 0,
> > > +#define WORKER_MODEL_RTC "rtc"
> > > + RTE_GRAPH_MODEL_RTC,
> >
> > Why not RTE_GRAPH_MODEL_RTC = RTE_GRAPH_MODEL_DEFAULT in enum
> > itself.
> Yes, will do in next version.
>
> >
> > > +#define WORKER_MODEL_GENERIC "generic"
> >
> > Generic is a very overloaded term. Use pipeline here i.e
> > RTE_GRAPH_MODEL_PIPELINE
>
> Actually, it's not a purely pipeline mode. I prefer to change to hybrid.
Hybrid is very overloaded term, and it will be confusing (considering
there will be new models in future).
Please pick a word that really express the model working.
> >
> >
> > > + RTE_GRAPH_MODEL_GENERIC,
> > > + RTE_GRAPH_MODEL_MAX,
> >
> > No need for MAX, it will break the ABI for future. See other subsystem such
> > as
> > cryptodev.
>
> Thanks, I will change it.
> >
> > > +};
> >
> > >
