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 ECFEC42B8A; Wed, 24 May 2023 10:46:27 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C89C640ED8; Wed, 24 May 2023 10:46:27 +0200 (CEST) Received: from mail-vk1-f177.google.com (mail-vk1-f177.google.com [209.85.221.177]) by mails.dpdk.org (Postfix) with ESMTP id 8C5434067E for ; Wed, 24 May 2023 10:46:26 +0200 (CEST) Received: by mail-vk1-f177.google.com with SMTP id 71dfb90a1353d-45701a8a1b3so255110e0c.3 for ; Wed, 24 May 2023 01:46:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684917986; x=1687509986; 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=yZtqixR6qaKajvXfUtqA/rpb9kBN6bUX0UA6sL9cc+8=; b=k9w41UzdVjbWoeaCpV0wtvXXkR8QK2PW+xCyLCmC5fNKEJyj5I/j8FEq1UyCRRHEnd TkcvR31ltJBI6sOMg41mWu79ShUCwdpBFJeqp2Lw/D2H/CDqjt0TjJsxTFFHOz/VdWn2 hew7zvFS17mtI4hA1Q+pRExWkdOCCWdsFwYGQy3hgeTe1l+3YmDTyGFAv644DOm6MALM eVBecTU0ugC0WYbM6YxrsTCenmwugKTJuAlcxefZmFFrf+Q9k+PQ/BSkFdiEB5PmJT8M jLsISOvTKcDyG4ElxEzuTl7By4v8DRuypn7yPRFY6yBowVflHorBXCqK/s4psyPWUCgg PFjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684917986; x=1687509986; 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=yZtqixR6qaKajvXfUtqA/rpb9kBN6bUX0UA6sL9cc+8=; b=Nl+f9qswy725GG9RQoRe2ZhuAsC0FcpXDx8Lb9rHIL1VcVHq8YOmvQq6eXDDzuikBQ 8ndBDV0OUn+xECTyGGsX6xICarkGy5ulyBHYObpON9LWN46UEyy/V2CPGxbAaBwazsxf qzpDlXk0XyfjZbT2L3R3Vk0IWA8JVvU9Xvq0im/s2JsFTmIfzD0Z7h3AUGLwP5/cHM8A 27j74rMJI8VeTtNkfaamIBmD/8LLrZnpeUHUaT31/BxkLb7C7Mo1dK39BG5PEYl1Z+95 Zl8bStCCrsC99IYMjZXopPfiiml6T4vAninW0vgVWpR8BbBZsTnrjLlqkFVdsBVXhcw4 K7Aw== X-Gm-Message-State: AC+VfDx7T3USKVfoL0cpkPn78qqm65JwC8azkGMhJlJ4TijjcCoEC/Ek GUsoVbX1a1Woj7vFOYuZDbt6ZzTQUADcNcR/0Sw= X-Google-Smtp-Source: ACHHUZ53zuYt4tHtgQfpVl6PQsquIFj6JeHg0fyaO4aQHQD1atcG8E3IsjoYTSJq1Ph4RvpJhAyXZzXjz0kSV4QhJY4= X-Received: by 2002:a1f:da86:0:b0:436:3174:c8a0 with SMTP id r128-20020a1fda86000000b004363174c8a0mr5510395vkg.10.1684917985773; Wed, 24 May 2023 01:46:25 -0700 (PDT) MIME-Version: 1.0 References: <20230331040306.3143693-1-zhirun.yan@intel.com> <20230509060347.1237884-1-zhirun.yan@intel.com> <20230509060347.1237884-13-zhirun.yan@intel.com> In-Reply-To: <20230509060347.1237884-13-zhirun.yan@intel.com> From: Jerin Jacob Date: Wed, 24 May 2023 14:15:59 +0530 Message-ID: Subject: Re: [PATCH v6 12/15] graph: enable graph multicore dispatch scheduler model To: Zhirun Yan Cc: dev@dpdk.org, jerinj@marvell.com, kirankumark@marvell.com, ndabilpuram@marvell.com, stephen@networkplumber.org, pbhagavatula@marvell.com, cunming.liang@intel.com, haiyue.wang@intel.com, =?UTF-8?Q?Mattias_R=C3=B6nnblom?= 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, May 9, 2023 at 11:35=E2=80=AFAM Zhirun Yan w= rote: > > This patch enables to chose new scheduler model. > > Signed-off-by: Haiyue Wang > Signed-off-by: Cunming Liang > Signed-off-by: Zhirun Yan > rte_graph_walk(struct rte_graph *graph) > { > - rte_graph_walk_rtc(graph); > + int model =3D rte_graph_worker_model_get(); Any specific to reason to keep model value in LCORE variable , why not in struct rte_graph? It is not specific to this patch. But good to understand as storing in rte_graph* will avoid cache miss. > + > + if (model =3D=3D RTE_GRAPH_MODEL_DEFAULT || > + model =3D=3D RTE_GRAPH_MODEL_RTC) I think, there can be three ways to do this a) Store model in PER_LCORE or struct rte_graph and add runtime check b) Make it as function pointer for graph_walk mcore_dispatch model is reusing all rte_node_enqueue_* functions, so for NOW only graph walk is different. But if need to integrate other graph models like eventdev backend(similar problem trying to solve in https://patches.dpdk.org/project/dpdk/patch/20230522091628.96236-2-mattias.= ronnblom@ericsson.com/), I think, we need to change enqueue variants. Both (a) and (b) has little performance impact in "current situation with this patch" and if we need to add similar check and function pointer for overriding node_enqueue_ functions it will have major impact. In order to have NO performance impact and able to overide node_enqueue_ functions, I think, we can have the following scheme in application and library. In application #define RTE_GRAPH_MODEL_SELECT RTE_GRAPH_MODEL_RTC #include In library: #if RTE_GRAPH_MODEL_SELECT =3D=3D RTE_GRAPH_MODEL_RTC #define rte_graph_walk rte_graph_walk_rtc #else if RTE_GRAPH_MODEL_SELECT =3D=3D RTE_GRAPH_MODEL_MCORE_DISPATCH #define rte_graph_walk rte_graph_walk_mcore_dispatch It is kind of compile time, But application can use function templating by proving different values RTE_GRAPH_MODEL_SELECT to make runtime if given application needs to support all modes at runtime. As an example: app_my_graph_processing.h has application code for graph walk and node processing. app_worker_rtc.c ------------------------ #define RTE_GRAPH_MODEL_SELECT RTE_GRAPH_MODEL_RTC #include #include app_my_graph_processing.h void app_worker_rtc() { while (1) { rte_graph_walk() } } app_worker_mcore_dispatch.c ----------------------------------------- #define RTE_GRAPH_MODEL_SELECT RTE_GRAPH_MODEL_MCORE_DISPATCH #include #include app_my_graph_processing.h void app_worker_mcore_dispatch() { while (1) { rte_graph_walk() } } in main() ------------- if (command line arg provided worker as rtc) rte_eal_remote_launch(app_worker_rtc) else rte_eal_remote_launch(app_worker_mcore_dispatch) ----------------------------------------- With that note, ending review comment for this series. In general patches look good high level, following items need to be sorted in next version. Then I think, it is good to merge in this release. 1) Above points on fixing performance and supporting more graph model varia= nts 2) Need to add UT for ALL new APIs in app/test/test_graph.c 3) Make sure no performance regression with app/test/test_graph_perf.c with new changes 4) Addressing existing comments in this series. Thanks for great work.