patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Kiran Kumar Kokkilagadda <kirankumark@marvell.com>
To: Gongming Chen <chengongming1900@outlook.com>,
	Jerin Jacob <jerinj@marvell.com>,
	Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>,
	"yanzhirun_163@163.com" <yanzhirun_163@163.com>,
	Robin Jarry <rjarry@redhat.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Gongming Chen <chengm11@chinatelecom.cn>,
	"stable@dpdk.org" <stable@dpdk.org>,
	Fan Yin <yinf1@chinatelecom.cn>
Subject: RE: [EXTERNAL] [PATCH v2] graph: fix does not return the unique id when create graph
Date: Fri, 10 May 2024 08:39:10 +0000	[thread overview]
Message-ID: <PH0PR18MB50717BEE1DF15249CE390878ACE72@PH0PR18MB5071.namprd18.prod.outlook.com> (raw)
In-Reply-To: <TYAP286MB06490547F3F405A998AA9248D8E72@TYAP286MB0649.JPNP286.PROD.OUTLOOK.COM>



> -----Original Message-----
> From: Gongming Chen <chengongming1900@outlook.com>
> Sent: Friday, May 10, 2024 12:26 PM
> To: Jerin Jacob <jerinj@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; yanzhirun_163@163.com
> Cc: dev@dpdk.org; Gongming Chen <chengm11@chinatelecom.cn>;
> stable@dpdk.org; Fan Yin <yinf1@chinatelecom.cn>
> Subject: [EXTERNAL] [PATCH v2] graph: fix does not return the unique id
> when create graph
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> From: Gongming Chen <chengm11@chinatelecom.cn>
> 
> When the order of graph destroy is not the reverse order of create, that is,
> when it is destroyed at will, the newly created graph id will be the same as
> the existing graph id, which is not the expected unique graph id. This graph
> id incorrectly corresponds to multiple graphs.
> 
> Fixes: a91fecc19c5c ("graph: implement create and destroy")
> Cc: stable@dpdk.org
> 
> Reported-by: Fan Yin <yinf1@chinatelecom.cn>
> Signed-off-by: Gongming Chen <chengm11@chinatelecom.cn>
> Signed-off-by: Fan Yin <yinf1@chinatelecom.cn>
> ---

There is a similar patch from robin in review.
https://patchwork.dpdk.org/project/dpdk/patch/20240326154936.1132201-1-rjarry@redhat.com/
I already Acked it. It is ready for merge.



>  lib/graph/graph.c | 163 ++++++++++++++++++++++++++--------------------
>  1 file changed, 94 insertions(+), 69 deletions(-)
> 
> diff --git a/lib/graph/graph.c b/lib/graph/graph.c index
> 26f0968a97..b8fecd9c6a 100644
> --- a/lib/graph/graph.c
> +++ b/lib/graph/graph.c
> @@ -19,9 +19,6 @@
> 
>  static struct graph_head graph_list = STAILQ_HEAD_INITIALIZER(graph_list);
>  static rte_spinlock_t graph_lock = RTE_SPINLOCK_INITIALIZER; -static
> rte_graph_t graph_id;
> -
> -#define GRAPH_ID_CHECK(id) ID_CHECK(id, graph_id)
> 
>  /* Private functions */
>  struct graph_head *
> @@ -217,6 +214,46 @@ graph_node_fini(struct graph *graph)
>  						       graph_node->node-
> >name));
>  }
> 
> +static struct graph *
> +graph_find_by_id(rte_graph_t id)
> +{
> +	struct graph *graph;
> +
> +	STAILQ_FOREACH(graph, &graph_list, next)
> +		if (graph->id == id)
> +			return graph;
> +	return NULL;
> +}
> +
> +static struct graph *
> +graph_find_by_name(const char *name)
> +{
> +	struct graph *graph;
> +
> +	STAILQ_FOREACH(graph, &graph_list, next)
> +		if (strncmp(graph->name, name, RTE_GRAPH_NAMESIZE) ==
> 0)
> +			return graph;
> +	return NULL;
> +}
> +
> +static rte_graph_t
> +graph_free_id_find(void)
> +{
> +	static rte_graph_t graph_id;
> +	if (graph_id == RTE_GRAPH_ID_INVALID)
> +		graph_id++;
> +
> +	rte_graph_t end_id = graph_id;
> +	do {
> +		if (!graph_find_by_id(graph_id))
> +			return graph_id++;
> +		if (++graph_id == RTE_GRAPH_ID_INVALID)
> +			graph_id++;
> +	} while (graph_id != end_id);
> +
> +	return RTE_GRAPH_ID_INVALID;
> +}
> +
>  static struct rte_graph *
>  graph_mem_fixup_node_ctx(struct rte_graph *graph)  { @@ -279,13 +316,12
> @@ rte_graph_model_mcore_dispatch_core_bind(rte_graph_t id, int lcore)  {
>  	struct graph *graph;
> 
> -	GRAPH_ID_CHECK(id);
>  	if (!rte_lcore_is_enabled(lcore))
>  		SET_ERR_JMP(ENOLINK, fail, "lcore %d not enabled", lcore);
> 
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (graph->id == id)
> -			break;
> +	graph = graph_find_by_id(id);
> +	if (!graph)
> +		goto fail;
> 
>  	if (graph->graph->model != RTE_GRAPH_MODEL_MCORE_DISPATCH)
>  		goto fail;
> @@ -309,15 +345,12 @@
> rte_graph_model_mcore_dispatch_core_unbind(rte_graph_t id)  {
>  	struct graph *graph;
> 
> -	GRAPH_ID_CHECK(id);
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (graph->id == id)
> -			break;
> +	graph = graph_find_by_id(id);
> +	if (!graph)
> +		return;
> 
>  	graph->lcore_id = RTE_MAX_LCORE;
>  	graph->graph->dispatch.lcore_id = RTE_MAX_LCORE;
> -
> -fail:
>  	return;
>  }
> 
> @@ -352,10 +385,8 @@ rte_graph_create(const char *name, struct
> rte_graph_param *prm)
>  		SET_ERR_JMP(EINVAL, fail, "Graph name should not be
> NULL");
> 
>  	/* Check for existence of duplicate graph */
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (strncmp(name, graph->name, RTE_GRAPH_NAMESIZE) ==
> 0)
> -			SET_ERR_JMP(EEXIST, fail, "Found duplicate graph
> %s",
> -				    name);
> +	if (graph_find_by_name(name))
> +		SET_ERR_JMP(EEXIST, fail, "Found duplicate graph %s",
> name);
> 
>  	/* Create graph object */
>  	graph = calloc(1, sizeof(*graph));
> @@ -403,10 +434,12 @@ rte_graph_create(const char *name, struct
> rte_graph_param *prm)
>  	graph_pcap_enable(prm->pcap_enable);
> 
>  	/* Initialize graph object */
> +	graph->id = graph_free_id_find();
> +	if (graph->id == RTE_GRAPH_ID_INVALID)
> +		goto graph_cleanup;
>  	graph->socket = prm->socket_id;
>  	graph->src_node_count = src_node_count;
>  	graph->node_count = graph_nodes_count(graph);
> -	graph->id = graph_id;
>  	graph->parent_id = RTE_GRAPH_ID_INVALID;
>  	graph->lcore_id = RTE_MAX_LCORE;
>  	graph->num_pkt_to_capture = prm->num_pkt_to_capture; @@ -
> 422,7 +455,6 @@ rte_graph_create(const char *name, struct
> rte_graph_param *prm)
>  		goto graph_mem_destroy;
> 
>  	/* All good, Lets add the graph to the list */
> -	graph_id++;
>  	STAILQ_INSERT_TAIL(&graph_list, graph, next);
> 
>  	graph_spinlock_unlock();
> @@ -467,7 +499,6 @@ rte_graph_destroy(rte_graph_t id)
>  			graph_cleanup(graph);
>  			STAILQ_REMOVE(&graph_list, graph, graph, next);
>  			free(graph);
> -			graph_id--;
>  			goto done;
>  		}
>  		graph = tmp;
> @@ -515,12 +546,14 @@ graph_clone(struct graph *parent_graph, const
> char *name, struct rte_graph_param
>  		goto graph_cleanup;
> 
>  	/* Initialize the graph object */
> +	graph->id = graph_free_id_find();
> +	if (graph->id == RTE_GRAPH_ID_INVALID)
> +		goto graph_cleanup;
>  	graph->src_node_count = parent_graph->src_node_count;
>  	graph->node_count = parent_graph->node_count;
>  	graph->parent_id = parent_graph->id;
>  	graph->lcore_id = parent_graph->lcore_id;
>  	graph->socket = parent_graph->socket;
> -	graph->id = graph_id;
> 
>  	/* Allocate the Graph fast path memory and populate the data */
>  	if (graph_fp_mem_create(graph))
> @@ -539,7 +572,6 @@ graph_clone(struct graph *parent_graph, const char
> *name, struct rte_graph_param
>  		goto graph_mem_destroy;
> 
>  	/* All good, Lets add the graph to the list */
> -	graph_id++;
>  	STAILQ_INSERT_TAIL(&graph_list, graph, next);
> 
>  	graph_spinlock_unlock();
> @@ -561,12 +593,10 @@ rte_graph_clone(rte_graph_t id, const char *name,
> struct rte_graph_param *prm)  {
>  	struct graph *graph;
> 
> -	GRAPH_ID_CHECK(id);
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (graph->id == id)
> -			return graph_clone(graph, name, prm);
> +	graph = graph_find_by_id(id);
> +	if (graph)
> +		return graph_clone(graph, name, prm);
> 
> -fail:
>  	return RTE_GRAPH_ID_INVALID;
>  }
> 
> @@ -575,9 +605,9 @@ rte_graph_from_name(const char *name)  {
>  	struct graph *graph;
> 
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (strncmp(graph->name, name, RTE_GRAPH_NAMESIZE) ==
> 0)
> -			return graph->id;
> +	graph = graph_find_by_name(name);
> +	if (graph)
> +		return graph->id;
> 
>  	return RTE_GRAPH_ID_INVALID;
>  }
> @@ -587,12 +617,10 @@ rte_graph_id_to_name(rte_graph_t id)  {
>  	struct graph *graph;
> 
> -	GRAPH_ID_CHECK(id);
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (graph->id == id)
> -			return graph->name;
> +	graph = graph_find_by_id(id);
> +	if (graph)
> +		return graph->name;
> 
> -fail:
>  	return NULL;
>  }
> 
> @@ -604,17 +632,13 @@ rte_graph_node_get(rte_graph_t gid, uint32_t nid)
>  	rte_graph_off_t off;
>  	rte_node_t count;
> 
> -	GRAPH_ID_CHECK(gid);
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (graph->id == gid) {
> -			rte_graph_foreach_node(count, off, graph->graph,
> -						node) {
> -				if (node->id == nid)
> -					return node;
> -			}
> -			break;
> +	graph = graph_find_by_id(gid);
> +	if (graph)
> +		rte_graph_foreach_node(count, off, graph->graph, node) {
> +			if (node->id == nid)
> +				return node;
>  		}
> -fail:
> +
>  	return NULL;
>  }
> 
> @@ -626,15 +650,11 @@ rte_graph_node_get_by_name(const char
> *graph_name, const char *node_name)
>  	rte_graph_off_t off;
>  	rte_node_t count;
> 
> -	STAILQ_FOREACH(graph, &graph_list, next)
> -		if (!strncmp(graph->name, graph_name,
> RTE_GRAPH_NAMESIZE)) {
> -			rte_graph_foreach_node(count, off, graph->graph,
> -						node) {
> -				if (!strncmp(node->name, node_name,
> -					     RTE_NODE_NAMESIZE))
> -					return node;
> -			}
> -			break;
> +	graph = graph_find_by_name(graph_name);
> +	if (graph)
> +		rte_graph_foreach_node(count, off, graph->graph, node) {
> +			if (!strncmp(node->name, node_name,
> RTE_NODE_NAMESIZE))
> +				return node;
>  		}
> 
>  	return NULL;
> @@ -713,13 +733,10 @@ rte_graph_export(const char *name, FILE *f)
>  	struct graph *graph;
>  	int rc = ENOENT;
> 
> -	STAILQ_FOREACH(graph, &graph_list, next) {
> -		if (strncmp(graph->name, name, RTE_GRAPH_NAMESIZE) ==
> 0) {
> -			rc = graph_to_dot(f, graph);
> -			goto end;
> -		}
> -	}
> -end:
> +	graph = graph_find_by_name(name);
> +	if (graph)
> +		rc = graph_to_dot(f, graph);
> +
>  	return -rc;
>  }
> 
> @@ -729,17 +746,16 @@ graph_scan_dump(FILE *f, rte_graph_t id, bool all)
>  	struct graph *graph;
> 
>  	RTE_VERIFY(f);
> -	GRAPH_ID_CHECK(id);
> 
> -	STAILQ_FOREACH(graph, &graph_list, next) {
> -		if (all == true) {
> +	if (all == true) {
> +		STAILQ_FOREACH(graph, &graph_list, next)
>  			graph_dump(f, graph);
> -		} else if (graph->id == id) {
> +	} else {
> +		graph = graph_find_by_id(id);
> +		if (graph)
>  			graph_dump(f, graph);
> -			return;
> -		}
>  	}
> -fail:
> +
>  	return;
>  }
> 
> @@ -758,7 +774,16 @@ rte_graph_list_dump(FILE *f)  rte_graph_t
>  rte_graph_max_count(void)
>  {
> -	return graph_id;
> +	struct graph *graph;
> +	rte_graph_t count = 0;
> +
> +	graph_spinlock_lock();
> +
> +	STAILQ_FOREACH(graph, &graph_list, next)
> +		count++;
> +
> +	graph_spinlock_unlock();
> +	return count;
>  }
> 
>  RTE_LOG_REGISTER_DEFAULT(rte_graph_logtype, INFO);
> --
> 2.32.1 (Apple Git-133)


  reply	other threads:[~2024-05-10  8:39 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-10  6:42 [PATCH v1] " Gongming Chen
2024-05-10  6:55 ` [PATCH v2] " Gongming Chen
2024-05-10  8:39   ` Kiran Kumar Kokkilagadda [this message]
2024-06-18 13:27     ` [EXTERNAL] " David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR18MB50717BEE1DF15249CE390878ACE72@PH0PR18MB5071.namprd18.prod.outlook.com \
    --to=kirankumark@marvell.com \
    --cc=chengm11@chinatelecom.cn \
    --cc=chengongming1900@outlook.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    --cc=ndabilpuram@marvell.com \
    --cc=rjarry@redhat.com \
    --cc=stable@dpdk.org \
    --cc=yanzhirun_163@163.com \
    --cc=yinf1@chinatelecom.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).