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 6D79EA0577; Mon, 6 Apr 2020 14:36:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AA32B1BED9; Mon, 6 Apr 2020 14:36:50 +0200 (CEST) Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) by dpdk.org (Postfix) with ESMTP id 58DD32BE9 for ; Mon, 6 Apr 2020 14:36:49 +0200 (CEST) Received: by mail-lj1-f196.google.com with SMTP id p10so14517190ljn.1 for ; Mon, 06 Apr 2020 05:36:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=from:subject:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=PKCHwB2k0fbJrZZ1dYzdPRx02oZUq2R8LXl/ySzCsFU=; b=aoecVAcl7DhpTxYLIg2+t4Bdii5gqFdbzm2jib46gJuaL9/n7w2zGkjYtaBDNYRO6p cJlMwykxyPlAZCSOYXTIeON/s6B5QgV/c9KnPhfnH/EBPxQbSMHMUO6rMBdiTTr1SOEr NE//vrYsmFcxoG+c95kEtPOGbmsDOeP7JqsFOu6sJ1P7NU1/bBqjT8yehX9jBX2ZYVoI i3tmnqR0DlW2qpZdNHcq2JCI7rGXmheyQvmwJLXIDdnsqdyE0to8capiMkX63Rd3Eieo amdyj4Prg+ZRF1MB5bBcDLGP/3SJ6oO5PElmZHWao7kfen8Fc6OnAy9Km07qGVK8Ou5s rOoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=PKCHwB2k0fbJrZZ1dYzdPRx02oZUq2R8LXl/ySzCsFU=; b=TtRZwhBdBWM4jt0JnM2wfTEJDaE4bAmSfQKX0TnD19K2Rgtb7ZZHT7MdJAaLMMnSP7 JF5cSS5Nnv1WhIdrukxeMUF5c5dbJcO2wLLuYf40/v1Hx2GbN+lV10yiRAM/BU0BLpN1 hr6VV2WtV9eUjrjqU0j1dj5SWK9ME5gpfOlcNgvHfR1ie830dOG5ib2Lmx0g+BKZKSR0 RyIFr6WJRPC13iGLHALhngTSQ1k5pxmnRzkZ7df5Yfi+CVGByYl4Wpv55nSmL0VDeE1k 1td1YKu3IGO2R9mFVSXSX9rsFUuiw6azfvcB0ZICD/Qh1GdDfXtipfmdCrRKeEhZc3b5 gvKg== X-Gm-Message-State: AGi0PuZNRnx3VdIaC+F6HdMkO1zLxngDTs7u00/N9mPrHu3k3h/uYljo IgD5xazpzElIKsDvQdYRmcvPDdJEtsY= X-Google-Smtp-Source: APiQypIbRnuTEnP7LwRCL2K6jkTHTiAgmopVQv2b0J3oRU2Ys1poJ0/d4sTNz68bpwqH7qw7QVdm5A== X-Received: by 2002:a2e:9cce:: with SMTP id g14mr11797475ljj.161.1586176608154; Mon, 06 Apr 2020 05:36:48 -0700 (PDT) Received: from [192.168.8.100] (user-5-173-33-152.play-internet.pl. [5.173.33.152]) by smtp.gmail.com with ESMTPSA id p14sm9909004ljn.48.2020.04.06.05.36.47 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Apr 2020 05:36:47 -0700 (PDT) From: Andrzej Ostruszka To: dev@dpdk.org References: <20200326165644.866053-1-jerinj@marvell.com> <20200331192945.2466880-1-jerinj@marvell.com> <20200331192945.2466880-2-jerinj@marvell.com> Message-ID: <75092d70-d1c1-832e-047f-d24d82654e4c@semihalf.com> Date: Mon, 6 Apr 2020 14:36:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200331192945.2466880-2-jerinj@marvell.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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" 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, jerinj@marvell.com wrote: > From: Jerin Jacob > > 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 > Signed-off-by: Kiran Kumar K > Signed-off-by: Pavan Nikhilesh > Signed-off-by: Nithin Dabilpuram > --- [...] > 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 > +#include > + > +#include > +#include > + > +#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