Re: [dpdk-dev] [PATCH v3 01/29] graph: define the public API for graph support

[Jerin Jacob <jerinjacobk@gmail.com>]

On Mon, Apr 6, 2020 at 6:06 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>
> Hello Jerin

Hello Andrzej,

>
> I've started reviewing this and will go patch-by-patch so some of the
> comments might sound silly and/or unnecessary.

Thanks for the comments.



> > +
> > +/**
> > + * @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).

Will fix it v5.

>
> > + *
> > + * 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.
> > + *
> > +
> > +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?

Considering typedef will not be pointers in Linux coding standard, I
have chosen shorter
name. considering eid, gid is cryptic and Since you think, rte_node_id
better, I will change to
that.


>
> > +
> > +/** 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?

Since RTE_GRAPH_BURST_SIZE used in fastpath, I thought of avoiding any operation
in the fastpath, even though the compiler most probably optimizes it.
The RTE_CACHE_LINE_SIZE also
taken a similar path.



>
> > +#endif
> > +
> > +/* Forward declaration */
> > +struct rte_node;  /**< Node data */
> > +struct rte_graph; /**< Graph data */
>
> 'data'?  Maybe 'object' or something like that.

Yes. I will change to data.

>
> [...]
> > +/**
> > + * @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?

Yes. This node name can be filled in registration time when NODE ID is
not known.


> > +/**
> > + * @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.

In order to look API synergy, I have returned   rte_graph_t everywhere.
But you are right. I will change "int" and pass the ID.


>
> [...]
> > +/**
> > + * @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?

Will change to int.

>
> [...]
> > +/**
> > + * @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?

Graph node ID is expressed in rte_graph_t.
Yes RTE_GRAPH_ID_IVALID can not be returned, I will remove it.

>
> [...]
> > +/**
> > + * @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?

Yes. I will change to rte_node_id_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".

No strong opinion on this. I will take your suggestion.

>
> [...]
> > +/**
> > + * @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.

It is next-node. I will update the doc.

>  Why edge-id typedef on return and why EDGE_ID_INVALID returned.  Would
> int/-EINVAL (for wrong 'id') be better?

Edge node ID is expressed in rte_edge_id_t. SO, I think, it fine to return
rte_edge_id_t. "This would avoid any compassion issue as well."


>
> > +
> > +/**
> > + * @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?

Next node.

>
> > +
> > +/**
> > + * @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.

Copy the names too.

>
> > +/**
> > + * @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.

Yes. I will change to max possible in the description.


>
> [...]
>
> With regards
> Andrzej Ostruszka