[v6] graph: mcore: optimize graph search

Message ID 20241216014353.2036-1-chcchc88@163.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series [v6] graph: mcore: optimize graph search |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build fail github build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing warning Testing issues
ci/iol-abi-testing warning Testing issues

Commit Message

Huichao Cai Dec. 16, 2024, 1:43 a.m. UTC
In the function __rte_graph_mcore_dispatch_sched_node_enqueue,
use a slower loop to search for the graph, modify the search logic
to record the result of the first search, and use this record for
subsequent searches to improve search speed.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 devtools/libabigail.abignore               |  5 +++++
 doc/guides/rel_notes/release_25_03.rst     |  2 ++
 lib/graph/rte_graph_model_mcore_dispatch.c | 11 +++++++----
 lib/graph/rte_graph_worker_common.h        |  1 +
 4 files changed, 15 insertions(+), 4 deletions(-)
  

Comments

David Marchand Dec. 16, 2024, 2:49 p.m. UTC | #1
Salut Dodji,

On Mon, Dec 16, 2024 at 2:44 AM Huichao Cai <chcchc88@163.com> wrote:
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 21b8cd6113..a92ee29512 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -33,3 +33,8 @@
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ; Temporary exceptions till next major ABI version ;
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> +[suppress_type]
> +       name = rte_node
> +       has_size_change = no
> +       has_data_member_inserted_between =
> +{offset_of(total_sched_fail), offset_of(xstat_off)}

Here is a suppression rule I suggested but does not have the intended effect.

For the context:

Before the change (that you can find below with the next hunk), we
made sure to zero the whole rte_node object at runtime in the library
allocator.
And the offset of the field next to 'dispatch' is fixed with an
explicit alignas() statement.

        /** Fast schedule area for mcore dispatch model. */
        union {
                alignas(RTE_CACHE_LINE_MIN_SIZE) struct {
                        unsigned int lcore_id;  /**< Node running
lcore. */
                        uint64_t total_sched_objs; /**< Number of
objects scheduled. */
                        uint64_t total_sched_fail; /**< Number of
scheduled failure. */
                } dispatch;
        };

        /** Fast path area cache line 1. */
        alignas(RTE_CACHE_LINE_MIN_SIZE)
        rte_graph_off_t xstat_off; /**< Offset to xstat counters. */

If you want the whole definition, you can have a look at:
https://git.dpdk.org/dpdk/tree/lib/graph/rte_graph_worker_common.h#n87

[...]

> diff --git a/lib/graph/rte_graph_worker_common.h b/lib/graph/rte_graph_worker_common.h
> index d3ec88519d..aef0f65673 100644
> --- a/lib/graph/rte_graph_worker_common.h
> +++ b/lib/graph/rte_graph_worker_common.h
> @@ -110,6 +110,7 @@ struct __rte_cache_aligned rte_node {
>                         unsigned int lcore_id;  /**< Node running lcore. */
>                         uint64_t total_sched_objs; /**< Number of objects scheduled. */
>                         uint64_t total_sched_fail; /**< Number of scheduled failure. */
> +                       struct rte_graph *graph;  /**< Graph corresponding to lcore_id. */
>                 } dispatch;
>         };

Now, the patch adds a new field in the struct {} dispatch field.

Here is what abidiff reports:

$ abidiff --version
abidiff: 2.6.0

$ abidiff --suppr
/home/dmarchan/git/pub/dpdk.org/main/devtools/libabigail.abignore
--no-added-syms --headers-dir1
/home/dmarchan/abi/v24.11/build-gcc-shared/usr/local/include
--headers-dir2 /home/dmarchan/builds/main/build-gcc-shared/install/usr/local/include
/home/dmarchan/abi/v24.11/build-gcc-shared/usr/local/lib64/librte_graph.so.25.0
/home/dmarchan/builds/main/build-gcc-shared/install/usr/local/lib64/librte_graph.so.25.1
Functions changes summary: 0 Removed, 1 Changed (9 filtered out), 0
Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C] 'function bool
__rte_graph_mcore_dispatch_sched_node_enqueue(rte_node*,
rte_graph_rq_head*)' at rte_graph_model_mcore_dispatch.c:117:1 has
some indirect sub-type changes:
    parameter 1 of type 'rte_node*' has sub-type changes:
      in pointed to type 'struct rte_node' at rte_graph_worker_common.h:92:1:
        type size hasn't changed
        1 data member changes (2 filtered):
          anonymous data member at offset 1536 (in bits) changed from:
            union {struct {unsigned int lcore_id; uint64_t
total_sched_objs; uint64_t total_sched_fail;} dispatch;}
          to:
            union {struct {unsigned int lcore_id; uint64_t
total_sched_objs; uint64_t total_sched_fail; rte_graph* graph;}
dispatch;}


What would be the best way to suppress this warning?
I tried the following which seems to work, but I prefer to ask for your advice.

[suppress_type]
    name = rte_node
    has_data_member_at = offset_of(total_sched_fail)


Thanks.
  
David Marchand Dec. 17, 2024, 9:04 a.m. UTC | #2
On Mon, Dec 16, 2024 at 3:49 PM David Marchand
<david.marchand@redhat.com> wrote:
> $ abidiff --suppr
> /home/dmarchan/git/pub/dpdk.org/main/devtools/libabigail.abignore
> --no-added-syms --headers-dir1
> /home/dmarchan/abi/v24.11/build-gcc-shared/usr/local/include
> --headers-dir2 /home/dmarchan/builds/main/build-gcc-shared/install/usr/local/include
> /home/dmarchan/abi/v24.11/build-gcc-shared/usr/local/lib64/librte_graph.so.25.0
> /home/dmarchan/builds/main/build-gcc-shared/install/usr/local/lib64/librte_graph.so.25.1
> Functions changes summary: 0 Removed, 1 Changed (9 filtered out), 0
> Added functions
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>
> 1 function with some indirect sub-type change:
>
>   [C] 'function bool
> __rte_graph_mcore_dispatch_sched_node_enqueue(rte_node*,
> rte_graph_rq_head*)' at rte_graph_model_mcore_dispatch.c:117:1 has
> some indirect sub-type changes:
>     parameter 1 of type 'rte_node*' has sub-type changes:
>       in pointed to type 'struct rte_node' at rte_graph_worker_common.h:92:1:
>         type size hasn't changed
>         1 data member changes (2 filtered):
>           anonymous data member at offset 1536 (in bits) changed from:
>             union {struct {unsigned int lcore_id; uint64_t
> total_sched_objs; uint64_t total_sched_fail;} dispatch;}
>           to:
>             union {struct {unsigned int lcore_id; uint64_t
> total_sched_objs; uint64_t total_sched_fail; rte_graph* graph;}
> dispatch;}
>
>
> What would be the best way to suppress this warning?
> I tried the following which seems to work, but I prefer to ask for your advice.
>
> [suppress_type]
>     name = rte_node
>     has_data_member_at = offset_of(total_sched_fail)

Gah.. I meant has_data_member_inserted_at.
But then testing with has_data_member_inserted_at, the warning is not
suppressed either...

Any help appreciated.
  
Huichao Cai Dec. 20, 2024, 2:05 a.m. UTC | #3
Hi David, does "has_data_member_inserted_between = {offset_of(total_sched_fail), offset_of(xstat_off)}" need to be on the same line?
  
Huichao Cai Jan. 20, 2025, 2:36 p.m. UTC | #4
Hi, Thomas
I tested(See below) this patch locally and it can suppress this error(github build: failed).
Is the difference in CI results from patchwork due to different versions of abidiff?
My abidiff version:
[root@localhost dpdk.chc1]# abidiff --version
abidiff: 1.6.0
There is currently no version 2.6.0 abidiff available in my local environment...

The following is the testing process and results:

==========When not adding the [suppress_type] field==========
[root@localhost dpdk.chc1]# abidiff --suppr ./devtools/libabigail.abignore --no-added-syms --headers-dir1 /tmp/v24.11/build-gcc-shared/usr/local/include --headers-dir2 ./build-gcc-shared/install/usr/local/include /tmp/v24.11/build-gcc-shared/usr/local/lib64/librte_graph.so.25.0 ./build-gcc-shared/install/usr/local/lib64/librte_graph.so.25.1
Functions changes summary: 0 Removed, 1 Changed (5 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C]'function rte_node_t __rte_node_register(const rte_node_register*)' at node.c:58:1 has some indirect sub-type changes:
    parameter 1 of type 'const rte_node_register*' has sub-type changes:
      in pointed to type 'const rte_node_register':
        in unqualified underlying type 'struct rte_node_register' at rte_graph.h:482:1:
          type size hasn't changed
          1 data member changes (2 filtered):
           type of 'rte_node_fini_t rte_node_register::fini' changed:
             underlying type 'void (const rte_graph*, rte_node*)*' changed:
               in pointed to type 'function type void (const rte_graph*, rte_node*)':
                 parameter 2 of type 'rte_node*' has sub-type changes:
                   in pointed to type 'struct rte_node' at rte_graph_worker_common.h:92:1:
                     type size hasn't changed
                     no data member change (1 filtered);
                     1 data member change:
                      anonymous data member at offset 1536 (in bits) changed from:
                        union {struct {unsigned int lcore_id; uint64_t total_sched_objs; uint64_t total_sched_fail;} dispatch;}
                      to:
                        union {struct {unsigned int lcore_id; uint64_t total_sched_objs; uint64_t total_sched_fail; rte_graph* graph;} dispatch;}

==========When adding the [suppress_type] field==========
root@localhost devtools]# cat libabigail.abignore 
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Core suppression rules: DO NOT TOUCH ;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

[suppress_function]
        symbol_version = EXPERIMENTAL
[suppress_variable]
        symbol_version = EXPERIMENTAL

[suppress_function]
        symbol_version = INTERNAL
[suppress_variable]
        symbol_version = INTERNAL

; Ignore generated PMD information strings
[suppress_variable]
        name_regexp = _pmd_info$

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Special rules to skip libraries ;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;
; This is not a libabigail rule (see check-abi.sh).
; This is used for driver removal and other special cases like mlx glue libs.
;
; SKIP_LIBRARY=librte_common_mlx5_glue
; SKIP_LIBRARY=librte_net_mlx4_glue

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Experimental APIs exceptions ;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Temporary exceptions till next major ABI version ;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
[suppress_type]
       name = rte_node
       has_size_change = no
       has_data_member_inserted_between =
{offset_of(total_sched_fail), offset_of(xstat_off)}

[root@localhost dpdk.chc1]# abidiff --suppr ./devtools/libabigail.abignore --no-added-syms --headers-dir1 /tmp/v24.11/build-gcc-shared/usr/local/include --headers-dir2 ./build-gcc-shared/install/usr/local/include /tmp/v24.11/build-gcc-shared/usr/local/lib64/librte_graph.so.25.0 ./build-gcc-shared/install/usr/local/lib64/librte_graph.so.25.1
Functions changes summary: 0 Removed, 0 Changed (6 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
  
Thomas Monjalon Jan. 20, 2025, 2:55 p.m. UTC | #5
20/01/2025 15:36, Huichao Cai:
> Hi, Thomas
> I tested(See below) this patch locally and it can suppress this error(github build: failed).
> Is the difference in CI results from patchwork due to different versions of abidiff?
> My abidiff version:
> [root@localhost dpdk.chc1]# abidiff --version
> abidiff: 1.6.0

I believe it is a very old version.

> There is currently no version 2.6.0 abidiff available in my local environment...

Is there any way you can upgrade it manually?
  
Huichao Cai Jan. 20, 2025, 3:23 p.m. UTC | #6
I'll try updating it, thank you.
  

Patch

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 21b8cd6113..a92ee29512 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -33,3 +33,8 @@ 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till next major ABI version ;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+[suppress_type]
+       name = rte_node
+       has_size_change = no
+       has_data_member_inserted_between =
+{offset_of(total_sched_fail), offset_of(xstat_off)}
diff --git a/doc/guides/rel_notes/release_25_03.rst b/doc/guides/rel_notes/release_25_03.rst
index 426dfcd982..55ffe8170d 100644
--- a/doc/guides/rel_notes/release_25_03.rst
+++ b/doc/guides/rel_notes/release_25_03.rst
@@ -102,6 +102,8 @@  ABI Changes
 
 * No ABI change that would break compatibility with 24.11.
 
+* graph: Added ``graph`` field to the ``dispatch`` structure in the ``rte_node`` structure.
+
 
 Known Issues
 ------------
diff --git a/lib/graph/rte_graph_model_mcore_dispatch.c b/lib/graph/rte_graph_model_mcore_dispatch.c
index a590fc9497..a81d338227 100644
--- a/lib/graph/rte_graph_model_mcore_dispatch.c
+++ b/lib/graph/rte_graph_model_mcore_dispatch.c
@@ -118,11 +118,14 @@  __rte_graph_mcore_dispatch_sched_node_enqueue(struct rte_node *node,
 					      struct rte_graph_rq_head *rq)
 {
 	const unsigned int lcore_id = node->dispatch.lcore_id;
-	struct rte_graph *graph;
+	struct rte_graph *graph = node->dispatch.graph;
 
-	SLIST_FOREACH(graph, rq, next)
-		if (graph->dispatch.lcore_id == lcore_id)
-			break;
+	if (unlikely((!graph) || (graph->dispatch.lcore_id != lcore_id))) {
+		SLIST_FOREACH(graph, rq, next)
+			if (graph->dispatch.lcore_id == lcore_id)
+				break;
+		node->dispatch.graph = graph;
+	}
 
 	return graph != NULL ? __graph_sched_node_enqueue(node, graph) : false;
 }
diff --git a/lib/graph/rte_graph_worker_common.h b/lib/graph/rte_graph_worker_common.h
index d3ec88519d..aef0f65673 100644
--- a/lib/graph/rte_graph_worker_common.h
+++ b/lib/graph/rte_graph_worker_common.h
@@ -110,6 +110,7 @@  struct __rte_cache_aligned rte_node {
 			unsigned int lcore_id;  /**< Node running lcore. */
 			uint64_t total_sched_objs; /**< Number of objects scheduled. */
 			uint64_t total_sched_fail; /**< Number of scheduled failure. */
+			struct rte_graph *graph;  /**< Graph corresponding to lcore_id. */
 		} dispatch;
 	};