From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1070EA0577; Mon, 6 Apr 2020 17:00:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DE09F2B86; Mon, 6 Apr 2020 17:00:17 +0200 (CEST) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by dpdk.org (Postfix) with ESMTP id 6ED062B83 for ; Mon, 6 Apr 2020 17:00:16 +0200 (CEST) Received: by mail-io1-f66.google.com with SMTP id i3so16023954ioo.13 for ; Mon, 06 Apr 2020 08:00:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2UJTropxBCk83GPx+2XYfIPFWXdRsl7c5Ar1oCV4g7M=; b=NTNhm7POAHOsKJQUcZYata5y5lsF2O9ay2QsRiYBS9595Yd97TFOoeIPc30pUkUzaJ pxvUeV8UGzscVtGHMth/7EVN4oTog0i4hpYYlmJu5xuvTuou6osz6ClJbCMj0avQZusr 66BG0decI5k99VSvDUfgq4MsHLPL1A9Kfhkty29o2yYpsd8Z7PLNrnh/TyiFapO+3n6o eOIGj8ytrdJyLViMPi3W/V32GMYjk8Ns5QLPK47u2rwl7NiycJzLlhoW9jTDHUHKaYyS 2QxQoVrZnE/5XJ5OFMk6dX0T7uLiLh4b7NltO3V4W4ymWGHnkMsJFHdGd7XUUd8hMge7 gc8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2UJTropxBCk83GPx+2XYfIPFWXdRsl7c5Ar1oCV4g7M=; b=l4xIdGK4lW6CXmtMSdkfPv5fKiNqtKHlZ4AyCsKgxUGucd4wFI7+o1JFBz6AoFm3pO G+pylAISShxmsa5WpKXcgwMSLN98Xo7NVPTN0fkQmHalKI55UxcQqyWMRJapKIt1QYtF X+4lCUEY96Fw/2gjUqJ4fiOfhPex/TgOikl2K2WKB2NKoPs40qVGey+EUGylSeCkQDA/ Lre0yRQWGhAys8o1nq4rIpBKIKQMMprh30BAQUeFjYjUvsC3AWGafJNz6KdNMyfj15Ei 87lWqMGx61iVd42uMBLSJCKfaBdqqBZDW0YOOCi4/mNWzv/OrJ3CQ1JKRyqFHldYb26d /a7g== X-Gm-Message-State: AGi0PubYgOGR6FGoSjqqKZ/kZy6QYOMx4A9bkGUkKY/8RmRKfNDpmSZ/ UyPT92W96rJFPu3clwAhQig1gRhSvfQYaGt7Hb/Cxt1L0YlQbQ== X-Google-Smtp-Source: APiQypJ1s0kUki89Sfn0z7Ka1/x4PDFfyOUWihxIuXmSYgWJqqaL9QBuOFUVomF3jdWwAxJbdTdAi+aq1CcbndRMQmk= X-Received: by 2002:a5d:87c6:: with SMTP id q6mr19657975ios.163.1586185215483; Mon, 06 Apr 2020 08:00:15 -0700 (PDT) MIME-Version: 1.0 References: <20200326165644.866053-1-jerinj@marvell.com> <20200331192945.2466880-1-jerinj@marvell.com> <20200331192945.2466880-2-jerinj@marvell.com> <75092d70-d1c1-832e-047f-d24d82654e4c@semihalf.com> In-Reply-To: <75092d70-d1c1-832e-047f-d24d82654e4c@semihalf.com> From: Jerin Jacob Date: Mon, 6 Apr 2020 20:29:56 +0530 Message-ID: To: Andrzej Ostruszka Cc: dpdk-dev Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3 01/29] graph: define the public API for graph support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Apr 6, 2020 at 6:06 PM Andrzej Ostruszka 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