From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A87D842C3A; Tue, 6 Jun 2023 07:48:37 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 41C1440A84; Tue, 6 Jun 2023 07:48:37 +0200 (CEST) Received: from mail-ua1-f42.google.com (mail-ua1-f42.google.com [209.85.222.42]) by mails.dpdk.org (Postfix) with ESMTP id 13043406B7 for ; Tue, 6 Jun 2023 07:48:36 +0200 (CEST) Received: by mail-ua1-f42.google.com with SMTP id a1e0cc1a2514c-78a230a687aso660984241.2 for ; Mon, 05 Jun 2023 22:48:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686030515; x=1688622515; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ew5NSQTqDMquBD+imDwLfLM0UzzqAGn0bQPHwH/ek6s=; b=e0MvVicAInm7kdGBnofx2wrEtr9HX2SI896Hvqmgtu4EoQ+KSuhAZXlUCiCR2jgDIr 5dVLD+t7WteZ3rMdGa+vUTC/OCYsJPd6cUHTtWx5SZpPvWhYWDMPVr+GedUqKhdf98qQ 7SG6Pxi6pzR18VHe5ILf32ntLMZw3PBzAZfZnTlnO995RCHzb4TTtqg48ymnG/rfLmjS K7qCiJoG46hH/SCO9X0RVYLSHIy9Sl2C9jRfqBaJRFUJReihnV3d5UudMExyxTt/VeYX BSHRaw0hEoowXg80AXLhICVllyDm+tdDx0N3j/fyf1/Edq2kuBLp+W2nuBrAjJJKWl6+ QAcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686030515; x=1688622515; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ew5NSQTqDMquBD+imDwLfLM0UzzqAGn0bQPHwH/ek6s=; b=kzyIOBfZ5HGBe3V78PBbXB6FqTyJdD0BLpuJfhFl/wXe1U8DuEhvknz4nb6t350Osk R5h40mSHWcemq7ZaNClPsO2p7FB8nK9ZS5a+XtogAvW2S+vVeRpSDRM/gh+ChgRZtERL QBvCf4K6PJr5oHu6Jv08t8I/zdktWeNln6UQcCxxXM7Uffzkvm9r1WxTnTbT7O8/9uSa p4CpSA7pjbnh6iKTtVNBhp6m69iglg+pyUVgcYonUGfDOXDP6SxTtdByfVSStw+kHEwc B7wWQTIgADmgEc2+lHLNcP0Z+Kvp8q+ieiwOksDU8sUs6pGbOcsW5HXn/yq9HKye8xfa esCg== X-Gm-Message-State: AC+VfDwZYHwtN1BCWG06MVCfS7LW1oWTX+S70cmJB1dEzgopkeI6EJDC x2T+OyQu3qt/DCXqwDoOfKpR6SLVB6ZAc33GHaQ= X-Google-Smtp-Source: ACHHUZ5dYjxEns0tLGO/0/4NajCcBjbE7FgXbRILPrYXIHM16ezwwh8os9u+uLkT1yBBM5Z0bX70M6EMrEsLmgWCQ/c= X-Received: by 2002:a05:6102:134d:b0:43b:33e1:751d with SMTP id j13-20020a056102134d00b0043b33e1751dmr693538vsl.5.1686030515082; Mon, 05 Jun 2023 22:48:35 -0700 (PDT) MIME-Version: 1.0 References: <20230509060347.1237884-1-zhirun.yan@intel.com> <20230605111923.3772260-1-zhirun.yan@intel.com> <20230605111923.3772260-5-zhirun.yan@intel.com> In-Reply-To: From: Jerin Jacob Date: Tue, 6 Jun 2023 11:18:08 +0530 Message-ID: Subject: Re: [PATCH v7 04/17] graph: add get/set graph worker model APIs To: "Yan, Zhirun" Cc: "dev@dpdk.org" , "jerinj@marvell.com" , "kirankumark@marvell.com" , "ndabilpuram@marvell.com" , "stephen@networkplumber.org" , "pbhagavatula@marvell.com" , "Liang, Cunming" , "Wang, Haiyue" , "mattias.ronnblom" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Jun 6, 2023 at 10:00=E2=80=AFAM Yan, Zhirun = wrote: > > > > > -----Original Message----- > > From: Jerin Jacob > > Sent: Monday, June 5, 2023 8:38 PM > > To: Yan, Zhirun > > Cc: dev@dpdk.org; jerinj@marvell.com; kirankumark@marvell.com; > > ndabilpuram@marvell.com; stephen@networkplumber.org; > > pbhagavatula@marvell.com; Liang, Cunming ; Wan= g, > > Haiyue ; mattias.ronnblom > > > > Subject: Re: [PATCH v7 04/17] graph: add get/set graph worker model API= s > > > > On Mon, Jun 5, 2023 at 4:56=E2=80=AFPM Zhirun Yan 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 > > > Signed-off-by: Cunming Liang > > > Signed-off-by: Zhirun Yan > > > --- > > > lib/graph/meson.build | 1 + > > > lib/graph/rte_graph_worker.c | 70 +++++++++++++++++++++++++++= ++ > > > lib/graph/rte_graph_worker_common.h | 19 ++++++++ > > > lib/graph/version.map | 3 ++ > > > 4 files changed, 93 insertions(+) > > > create mode 100644 lib/graph/rte_graph_worker.c > > > > > > diff --git a/lib/graph/meson.build b/lib/graph/meson.build index > > > 3526d1b5d4..9fab8243da 100644 > > > --- a/lib/graph/meson.build > > > +++ b/lib/graph/meson.build > > > @@ -15,6 +15,7 @@ sources =3D files( > > > 'graph_stats.c', > > > 'graph_populate.c', > > > 'graph_pcap.c', > > > + 'rte_graph_worker.c', > > > ) > > > headers =3D files('rte_graph.h', 'rte_graph_worker.h') > > > > > > diff --git a/lib/graph/rte_graph_worker.c > > > b/lib/graph/rte_graph_worker.c new file mode 100644 index > > > 0000000000..fc188e7cfa > > > --- /dev/null > > > +++ b/lib/graph/rte_graph_worker.c > > > @@ -0,0 +1,70 @@ > > > +/* SPDX-License-Identifier: BSD-3-Clause > > > + * Copyright(C) 2023 Intel Corporation */ > > > + > > > +/** > > > + * @file graph_worker.c > > > + * > > > + * @warning > > > + * @b EXPERIMENTAL: > > > + * All functions in this file may be changed or removed without prio= r notice. > > > + * > > > + * These API enable to set/get graph walking model. > > > + * > > > + */ > > > + > > > +#include "rte_graph_worker_common.h" > > > +#include "graph_private.h" > > > + > > > +/** > > > + * @note This function does not perform any locking, and is only saf= e to call > > > + * before graph running. It will set all graphs the same model. > > > + * > > > + * @param name > > > + * Name of the graph worker model. > > > + * > > > + * @return > > > + * 0 on success, -1 otherwise. > > < 0 otherwise > > > > Doxygen comment is not required .c file. > > > Yes, I will move the declaration into rte_graph_worker.h > > > > > > + */ > > > +int > > > +rte_graph_worker_model_set(enum rte_graph_worker_model model) { > > > + struct graph_head *graph_head =3D graph_list_head_get(); > > > + struct graph *graph; > > > + int ret =3D 0; > > > + > > > + if (model =3D=3D RTE_GRAPH_MODEL_DEFAULT || model =3D=3D > > RTE_GRAPH_MODEL_RTC || > > > + model =3D=3D RTE_GRAPH_MODEL_MCORE_DISPATCH) > > > + STAILQ_FOREACH(graph, graph_head, next) > > > + graph->graph->model =3D model; > > > + else { > > > + STAILQ_FOREACH(graph, graph_head, next) > > > + graph->graph->model =3D RTE_GRAPH_MODEL_DEFAU= LT; > > > + ret =3D -1; > > > > Why returning -1 here? > > Also, why "else" case needed as RTE_GRAPH_MODEL_RTC =3D=3D > > RTE_GRAPH_MODEL_DEFAULT > > Actually, the "else" offers a way to recover if this func called with mod= el > > RTE_GRAPH_MODEL_MCORE_DISPATCH, or another not supported value. Then the > app could have ability to run in default RTC model or user app could rese= t the model. > > > > > Why not > > > > struct graph_head *graph_head =3D graph_list_head_get(); struct graph *= graph; > > > > [1] > > if (model > RTE_GRAPH_MODEL_MCORE_DISPATCH) > > return -EINVAL; > > > > STAILQ_FOREACH(graph, graph_head, next) > > graph->graph->model =3D model; > > > > > > For [1], Please add internal helper function graph_model_is_valid() to = use > > everywhere as needed. > > > > I will add graph_model_is_valid(). > > And change it to > > If (!graph_model_is_valid()) { > model =3D RTE_GRAPH_MODEL_DEFAULT; Since it returning from below, Do we need to update model? > return -EINVAL; > } > STAILQ_FOREACH(graph, graph_head, next) > graph->graph->model =3D model; graph->model =3D model. Right? > > return 0; > > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/** > > > + * Get the graph worker model > > > + * > > > + * @note All graph will use the same model and this function will ge= t > > > +model from the first one > > > + * > > > + * @param name > > > + * Name of the graph worker model. > > > + * > > > + * @return > > > + * Graph worker model on success. > > > + */ > > > +inline > > > +enum rte_graph_worker_model > > > +rte_graph_worker_model_get(void) > > > +{ > > > + struct graph_head *graph_head =3D graph_list_head_get(); > > > + struct graph *graph; > > > + > > > + graph =3D STAILQ_FIRST(graph_head); > > > > This can be used in fastpath, So lets pass graph object and make inline= function > > to return graph->model > > > > Some functions don't have the graph object like graph_stats_*. No param c= ould make > this API easy to call for current impl. And I think this func is mainly f= or configuration, But, you are using this in fastpath here. https://patches.dpdk.org/project/dpdk/patch/20230605111923.3772260-14-zhiru= n.yan@intel.com/ > should not in fastpath. If different models coexisted, then the graph obj= ect must be passed. I think, all fastpath cases graph->model can tell the model. graph_stats_* API etc still has rte_graph object so it should be possible to reuse there.