Hello Jerin
I've started reviewing this and will go patch-by-patch so some of the
comments might sound silly and/or unnecessary.
On 3/31/20 9:29 PM, jer...@marvell.com wrote:
> From: Jerin Jacob <jer...@marvell.com>
>
> Graph architecture abstracts the data processing functions as
> "node" and "link" them together to create a complex "graph" to enable
> reusable/modular data processing functions.
>
> These APIs enables graph framework operations such as create, lookup,
> dump and destroy on graph and node operations such as clone,
> edge update, and edge shrink, etc. The API also allows creating the
> stats cluster to monitor per graph and per node stats.
>
> This patch defines the public API for graph support.
> This patch also adds support for the build infrastructure and
> update the MAINTAINERS file for the graph subsystem.
>
> Signed-off-by: Jerin Jacob <jer...@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankum...@marvell.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com>
> Signed-off-by: Nithin Dabilpuram <ndabilpu...@marvell.com>
> ---
[...]
> diff --git a/lib/librte_graph/rte_graph.h b/lib/librte_graph/rte_graph.h
> new file mode 100644
> index 000000000..4bcf0a6e5
> --- /dev/null
> +++ b/lib/librte_graph/rte_graph.h
> @@ -0,0 +1,786 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#ifndef _RTE_GRAPH_H_
> +#define _RTE_GRAPH_H_
> +
> +/**
> + * @file rte_graph.h
> + *
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
I think this @warning doc at global level is enough - no need to repeat
it below (just keeping __rte_experimental should be fine).
> + *
> + * Graph architecture abstracts the data processing functions as
> + * "node" and "link" them together to create a complex "graph" to enable
> + * reusable/modular data processing functions.
> + *
> + * This API enables graph framework operations such as create, lookup,
> + * dump and destroy on graph and node operations such as clone,
> + * edge update, and edge shrink, etc. The API also allows to create the stats
> + * cluster to monitor per graph and per node stats.
> + *
> + */
> +
> +#include <stdbool.h>
> +#include <stdio.h>
> +
> +#include <rte_common.h>
> +#include <rte_compat.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define RTE_GRAPH_NAMESIZE 64 /**< Max length of graph name. */
> +#define RTE_NODE_NAMESIZE 64 /**< Max length of node name. */
> +#define RTE_GRAPH_OFF_INVALID UINT32_MAX /**< Invalid graph offset. */
> +#define RTE_NODE_ID_INVALID UINT32_MAX /**< Invalid node id. */
> +#define RTE_EDGE_ID_INVALID UINT16_MAX /**< Invalid edge id. */
> +#define RTE_GRAPH_ID_INVALID UINT16_MAX /**< Invalid graph id. */
> +#define RTE_GRAPH_FENCE 0xdeadbeef12345678ULL /**< Graph fence data. */
> +
> +typedef uint32_t rte_graph_off_t; /**< Graph offset type. */
> +typedef uint32_t rte_node_t; /**< Node id type. */
> +typedef uint16_t rte_edge_t; /**< Edge id type. */
> +typedef uint16_t rte_graph_t; /**< Graph id type. */
I would use 'id' somewhere in the name of these typedefs - e.g. seeing
rte_node_t in the code (without knowing what it is) I'd be guessing this
is a pointer to 'struct rte_node'.
So maybe 'rte_node_id' or if we stick with _t convention and
rte_node_id_t is too long then maybe simple rte_nid_t/rte_eid_t/rte_gid_t?
> +
> +/** Burst size in terms of log2 */
> +#if RTE_GRAPH_BURST_SIZE == 1
> +#define RTE_GRAPH_BURST_SIZE_LOG2 0 /**< Object burst size of 1. */
> +#elif RTE_GRAPH_BURST_SIZE == 2
> +#define RTE_GRAPH_BURST_SIZE_LOG2 1 /**< Object burst size of 2. */
> +#elif RTE_GRAPH_BURST_SIZE == 4
> +#define RTE_GRAPH_BURST_SIZE_LOG2 2 /**< Object burst size of 4. */
> +#elif RTE_GRAPH_BURST_SIZE == 8
> +#define RTE_GRAPH_BURST_SIZE_LOG2 3 /**< Object burst size of 8. */
> +#elif RTE_GRAPH_BURST_SIZE == 16
> +#define RTE_GRAPH_BURST_SIZE_LOG2 4 /**< Object burst size of 16. */
> +#elif RTE_GRAPH_BURST_SIZE == 32
> +#define RTE_GRAPH_BURST_SIZE_LOG2 5 /**< Object burst size of 32. */
> +#elif RTE_GRAPH_BURST_SIZE == 64
> +#define RTE_GRAPH_BURST_SIZE_LOG2 6 /**< Object burst size of 64. */
> +#elif RTE_GRAPH_BURST_SIZE == 128
> +#define RTE_GRAPH_BURST_SIZE_LOG2 7 /**< Object burst size of 128. */
> +#elif RTE_GRAPH_BURST_SIZE == 256
> +#define RTE_GRAPH_BURST_SIZE_LOG2 8 /**< Object burst size of 256. */
> +#else
> +#error "Unsupported burst size"
If other sizes are not supported then maybe it would be better to have
in options RTE_GRAPH_BURST_SIZE_LOG2 and define BURST_SIZE in terms of
this LOG2?
> +#endif
> +
> +/* Forward declaration */
> +struct rte_node; /**< Node data */
> +struct rte_graph; /**< Graph data */
'data'? Maybe 'object' or something like that.
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Structure defines the node registration parameters.
> + *
> + * @see __rte_node_register(), RTE_NODE_REGISTER()
> + */
> +struct rte_node_register {
> + char name[RTE_NODE_NAMESIZE]; /**< Name of the node. */
> + uint64_t flags; /**< Node configuration flag. */
> +#define RTE_NODE_SOURCE_F (1ULL << 0) /**< Node type is source. */
> + rte_node_process_t process; /**< Node process function. */
> + rte_node_init_t init; /**< Node init function. */
> + rte_node_fini_t fini; /**< Node fini function. */
> + rte_node_t id; /**< Node Identifier. */
> + rte_node_t parent_id; /**< Identifier of parent node. */
> + rte_edge_t nb_edges; /**< Number of edges from this node. */
> + const char *next_nodes[]; /**< Names of next nodes. */
Not nodes ids? It seems that basic handle for graph/node is its name -
not an id or pointer. Is it so? If so could you shed some light why it
is better/more convenient? Late binding of nodes during graph creation?
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Create Graph.
> + *
> + * Create memory reel, detect loops and find isolated nodes.
> + *
> + * @param name
> + * Unique name for this graph.
> + * @param prm
> + * Graph parameter, includes node names and count to be included
> + * in this graph.
> + *
> + * @return
> + * Unique graph id on success, RTE_GRAPH_ID_INVALID otherwise.
> + */
> +__rte_experimental
> +rte_graph_t rte_graph_create(const char *name, struct rte_graph_param *prm);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Destroy Graph.
> + *
> + * Free Graph memory reel.
> + *
> + * @param name
> + * Name of the graph to destroy.
> + *
> + * @return
> + * 0 on success, error otherwise.
> + */
> +__rte_experimental
> +rte_graph_t rte_graph_destroy(const char *name);
Why rte_graph_t on return? I have no experience with this but would
expect to have rte_graph_t (id) to be the handle via which graph is kept
(this is what rte_graph_create() returns) so would expect rte_graph_t to
be the input arg here.
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Export the graph as graph viz dot file
> + *
> + * @param name
> + * Name of the graph to export.
> + * @param f
> + * File pointer to export the graph.
> + *
> + * @return
> + * 0 on success, error otherwise.
> + */
> +__rte_experimental
> +rte_graph_t rte_graph_export(const char *name, FILE *f);
Why rte_graph_t on return?
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get maximum number of graph available.
> + *
> + * @return
> + * Maximum graph count on success, RTE_GRAPH_ID_INVALID otherwise.
> + */
> +__rte_experimental
> +rte_graph_t rte_graph_max_count(void);
Why rte_graph_t on return? And why RTE_GRAPH_ID_IVALID can be returned?
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get node object with in graph from id.
> + *
> + * @param graph_id
> + * Graph id to get node pointer from.
> + * @param node_id
> + * Node id to get node pointer.
> + *
> + * @return
> + * Node pointer on success, NULL otherwise.
> + */
> +__rte_experimental
> +struct rte_node *rte_graph_node_get(rte_graph_t graph_id, uint32_t node_id);
Maybe rte_node_t (or its changed name if you accept earlier comment)
instead of uint32_t?
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Register new packet processing node. Nodes can be registered
> + * dynamically via this call or statically via the RTE_NODE_REGISTER
> + * macro.
> + *
> + * @param node
> + * Valid node pointer with name, process function and next_nodes.
> + *
> + * @return
> + * Valid node id on success, RTE_NODE_ID_INVALID otherwise.
> + *
> + * @see RTE_NODE_REGISTER()
> + */
> +__rte_experimental
> +rte_node_t __rte_node_register(const struct rte_node_register *node);
> +
> +/**
> + * Register a static node.
> + *
> + * The static node is registered through the constructor scheme, thereby, it
> can
> + * be used in a multi-process scenario.
> + *
> + * @param node
> + * Valid node pointer with name, process function, and next_nodes.
> + */
> +#define RTE_NODE_REGISTER(node)
> \
> + RTE_INIT(rte_node_register_##node) \
> + { \
> + node.parent_id = RTE_NODE_ID_INVALID; \
> + node.id = __rte_node_register(&node); \
> + }
I would position rte_node_register struct definition near these so they
are grouped by "topic".
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get the number of edges for a node from node id.
> + *
> + * @param id
> + * Valid node id.
> + *
> + * @return
> + * Valid edge count on success, RTE_EDGE_ID_INVALID otherwise.
> + */
> +__rte_experimental
> +rte_edge_t rte_node_edge_count(rte_node_t id);
I would clarify here what edge is? Incoming nodes, next-nodes or both.
Why edge-id typedef on return and why EDGE_ID_INVALID returned. Would
int/-EINVAL (for wrong 'id') be better?
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Update the edges for a node from node id.
> + *
> + * @param id
> + * Valid node id.
> + * @param from
> + * Index to update the edges from. RTE_EDGE_ID_INVALID is valid,
> + * in that case, it will be added to the end of the list.
> + * @param next_nodes
> + * Name of the edges to update.
> + * @param nb_edges
> + * Number of edges to update.
> + *
> + * @return
> + * Valid edge count on success, 0 otherwise.
> + */
> +__rte_experimental
> +rte_edge_t rte_node_edge_update(rte_node_t id, rte_edge_t from,
> + const char **next_nodes, uint16_t nb_edges);
So from this I infer that edge is either incoming or outgoing - right?
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Shrink the edges to a given size.
> + *
> + * @param id
> + * Valid node id.
> + * @param size
> + * New size to shrink the edges.
> + *
> + * @return
> + * New size on success, RTE_EDGE_ID_INVALID otherwise.
> + */
> +__rte_experimental
> +rte_edge_t rte_node_edge_shrink(rte_node_t id, rte_edge_t size);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get the edge names from a given node.
> + *
> + * @param id
> + * Valid node id.
> + * @param[out] next_nodes
> + * Buffer to copy the edge names. The NULL value is allowed in that case,
> + * the function returns the size of the array that needs to be allocated.
> + *
> + * @return
> + * When next_nodes == NULL, it returns the size of the array else
> + * number of item copied.
> + */
> +__rte_experimental
> +rte_node_t rte_node_edge_get(rte_node_t id, char *next_nodes[]);
I guess this doesn't copy names just stores pointers to names.
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get maximum nodes created so far.
> + *
> + * @return
> + * Maximum nodes count on success, 0 otherwise.
> + */
> +__rte_experimental
> +rte_node_t rte_node_max_count(void);
If this is "created so far" then why call it 'max'? I guess this is max
possible so I would update description.
[...]
With regards
Andrzej Ostruszka
