diff mbox series

[1/2] app/testpmd: change rule type

Message ID 20230222141139.3233715-1-elibr@nvidia.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [1/2] app/testpmd: change rule type |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Eli Britstein Feb. 22, 2023, 2:11 p.m. UTC 
Change rule type to be uintptr_t (instead of currently uint32_t) to be
able to accomodate larger IDs, as a pre-step towards allowing user-id
to flows.

Signed-off-by: Eli Britstein <elibr@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 12 ++++++------
 app/test-pmd/config.c       | 34 ++++++++++++++++++----------------
 app/test-pmd/testpmd.h      | 10 +++++-----
 3 files changed, 29 insertions(+), 27 deletions(-)

Comments

Ferruh Yigit March 1, 2023, 1 a.m. UTC | #1 
On 2/22/2023 2:11 PM, Eli Britstein wrote:
> Change rule type to be uintptr_t (instead of currently uint32_t) to be
> able to accomodate larger IDs, as a pre-step towards allowing user-id
> to flows.
> 

No objection to extend id storage type, but I am not clear why allowing
user-id justifies this, will user insert id more than uint32_max, or is
there some intention to partition variable for user provided ids etc?
Can you please elaborate?

> Signed-off-by: Eli Britstein <elibr@nvidia.com>
> ---
>  app/test-pmd/cmdline_flow.c | 12 ++++++------
>  app/test-pmd/config.c       | 34 ++++++++++++++++++----------------
>  app/test-pmd/testpmd.h      | 10 +++++-----
>  3 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 9309607f11..a2709e8aa9 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -1085,16 +1085,16 @@ struct buffer {
>  			uint8_t *data;
>  		} vc; /**< Validate/create arguments. */
>  		struct {
> -			uint32_t *rule;
> -			uint32_t rule_n;
> +			uintptr_t *rule;

Why not using 'uint64_t'?

As far as I know 'uintptr_t' is logically to hold pointer values, it is
good for usage like:
``
void *val;
uintptr_t ptr = (uintptr_t)val;
``

Having 'uintptr_t' as type for pointer looks confusing to me.

> +			uintptr_t rule_n;

Similarly, why not 'uint64_t' or 'size_t'?

Variable name suggest it is storing a count, if so 'size_t' is more
suitable logically (functionally I guess both are same).

Same comments apply for all below type changes.
diff mbox series

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 9309607f11..a2709e8aa9 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -1085,16 +1085,16 @@  struct buffer {
 			uint8_t *data;
 		} vc; /**< Validate/create arguments. */
 		struct {
-			uint32_t *rule;
-			uint32_t rule_n;
+			uintptr_t *rule;
+			uintptr_t rule_n;
 		} destroy; /**< Destroy arguments. */
 		struct {
 			char file[128];
 			bool mode;
-			uint32_t rule;
+			uintptr_t rule;
 		} dump; /**< Dump arguments. */
 		struct {
-			uint32_t rule;
+			uintptr_t rule;
 			struct rte_flow_action action;
 		} query; /**< Query arguments. */
 		struct {
@@ -9683,7 +9683,7 @@  parse_qo_destroy(struct context *ctx, const struct token *token,
 		 void *buf, unsigned int size)
 {
 	struct buffer *out = buf;
-	uint32_t *flow_id;
+	uintptr_t *flow_id;
 
 	/* Token name must match. */
 	if (parse_default(ctx, token, str, len, NULL, 0) < 0)
@@ -10899,7 +10899,7 @@  comp_rule_id(struct context *ctx, const struct token *token,
 	port = &ports[ctx->port];
 	for (pf = port->flow_list; pf != NULL; pf = pf->next) {
 		if (buf && i == ent)
-			return snprintf(buf, size, "%u", pf->id);
+			return snprintf(buf, size, "%"PRIu64, pf->id);
 		++i;
 	}
 	if (buf)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 4121c5c9bb..167cb246c5 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2723,7 +2723,7 @@  port_queue_flow_create(portid_t port_id, queueid_t queue_id,
 		flow = rte_flow_async_create_by_index(port_id, queue_id, &op_attr, pt->table,
 			rule_idx, actions, actions_idx, job, &error);
 	if (!flow) {
-		uint32_t flow_id = pf->id;
+		uintptr_t flow_id = pf->id;
 		port_queue_flow_destroy(port_id, queue_id, true, 1, &flow_id);
 		free(job);
 		return port_flow_complain(&error);
@@ -2734,14 +2734,14 @@  port_queue_flow_create(portid_t port_id, queueid_t queue_id,
 	pf->flow = flow;
 	job->pf = pf;
 	port->flow_list = pf;
-	printf("Flow rule #%u creation enqueued\n", pf->id);
+	printf("Flow rule #%"PRIu64" creation enqueued\n", pf->id);
 	return 0;
 }
 
 /** Enqueue number of destroy flow rules operations. */
 int
 port_queue_flow_destroy(portid_t port_id, queueid_t queue_id,
-			bool postpone, uint32_t n, const uint32_t *rule)
+			bool postpone, uint32_t n, const uintptr_t *rule)
 {
 	struct rte_flow_op_attr op_attr = { .postpone = postpone };
 	struct rte_port *port;
@@ -2788,7 +2788,8 @@  port_queue_flow_destroy(portid_t port_id, queueid_t queue_id,
 				ret = port_flow_complain(&error);
 				continue;
 			}
-			printf("Flow rule #%u destruction enqueued\n", pf->id);
+			printf("Flow rule #%"PRIu64" destruction enqueued\n",
+			       pf->id);
 			*tmp = pf->next;
 			break;
 		}
@@ -3087,7 +3088,7 @@  port_queue_flow_push(portid_t port_id, queueid_t queue_id)
 /** Pull queue operation results from the queue. */
 static int
 port_queue_aged_flow_destroy(portid_t port_id, queueid_t queue_id,
-			     const uint32_t *rule, int nb_flows)
+			     const uintptr_t *rule, int nb_flows)
 {
 	struct rte_port *port = &ports[port_id];
 	struct rte_flow_op_result *res;
@@ -3150,7 +3151,7 @@  port_queue_flow_aged(portid_t port_id, uint32_t queue_id, uint8_t destroy)
 {
 	void **contexts;
 	int nb_context, total = 0, idx;
-	uint32_t *rules = NULL;
+	uintptr_t *rules = NULL;
 	struct rte_port *port;
 	struct rte_flow_error error;
 	enum age_action_context_type *type;
@@ -3206,7 +3207,7 @@  port_queue_flow_aged(portid_t port_id, uint32_t queue_id, uint8_t destroy)
 		switch (*type) {
 		case ACTION_AGE_CONTEXT_TYPE_FLOW:
 			ctx.pf = container_of(type, struct port_flow, age_type);
-			printf("%-20s\t%" PRIu32 "\t%" PRIu32 "\t%" PRIu32
+			printf("%-20s\t%" PRIuPTR "\t%" PRIu32 "\t%" PRIu32
 								 "\t%c%c%c\t\n",
 			       "Flow",
 			       ctx.pf->id,
@@ -3354,13 +3355,13 @@  port_flow_create(portid_t port_id,
 	port->flow_list = pf;
 	if (tunnel_ops->enabled)
 		port_flow_tunnel_offload_cmd_release(port_id, tunnel_ops, pft);
-	printf("Flow rule #%u created\n", pf->id);
+	printf("Flow rule #%"PRIu64" created\n", pf->id);
 	return 0;
 }
 
 /** Destroy a number of flow rules. */
 int
-port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule)
+port_flow_destroy(portid_t port_id, uint32_t n, const uintptr_t *rule)
 {
 	struct rte_port *port;
 	struct port_flow **tmp;
@@ -3389,7 +3390,7 @@  port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule)
 				ret = port_flow_complain(&error);
 				continue;
 			}
-			printf("Flow rule #%u destroyed\n", pf->id);
+			printf("Flow rule #%"PRIu64" destroyed\n", pf->id);
 			*tmp = pf->next;
 			free(pf);
 			break;
@@ -3434,7 +3435,7 @@  port_flow_flush(portid_t port_id)
 
 /** Dump flow rules. */
 int
-port_flow_dump(portid_t port_id, bool dump_all, uint32_t rule_id,
+port_flow_dump(portid_t port_id, bool dump_all, uintptr_t rule_id,
 		const char *file_name)
 {
 	int ret = 0;
@@ -3463,7 +3464,8 @@  port_flow_dump(portid_t port_id, bool dump_all, uint32_t rule_id,
 			}
 		}
 		if (found == false) {
-			fprintf(stderr, "Failed to dump to flow %d\n", rule_id);
+			fprintf(stderr, "Failed to dump to flow %"PRIu64"\n",
+				rule_id);
 			return -EINVAL;
 		}
 	}
@@ -3493,7 +3495,7 @@  port_flow_dump(portid_t port_id, bool dump_all, uint32_t rule_id,
 
 /** Query a flow rule. */
 int
-port_flow_query(portid_t port_id, uint32_t rule,
+port_flow_query(portid_t port_id, uintptr_t rule,
 		const struct rte_flow_action *action)
 {
 	struct rte_flow_error error;
@@ -3515,7 +3517,7 @@  port_flow_query(portid_t port_id, uint32_t rule,
 		if (pf->id == rule)
 			break;
 	if (!pf) {
-		fprintf(stderr, "Flow rule #%u not found\n", rule);
+		fprintf(stderr, "Flow rule #%"PRIu64" not found\n", rule);
 		return -ENOENT;
 	}
 	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
@@ -3622,7 +3624,7 @@  port_flow_aged(portid_t port_id, uint8_t destroy)
 		switch (*type) {
 		case ACTION_AGE_CONTEXT_TYPE_FLOW:
 			ctx.pf = container_of(type, struct port_flow, age_type);
-			printf("%-20s\t%" PRIu32 "\t%" PRIu32 "\t%" PRIu32
+			printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
 								 "\t%c%c%c\t\n",
 			       "Flow",
 			       ctx.pf->id,
@@ -3700,7 +3702,7 @@  port_flow_list(portid_t port_id, uint32_t n, const uint32_t *group)
 		const struct rte_flow_action *action = pf->rule.actions;
 		const char *name;
 
-		printf("%" PRIu32 "\t%" PRIu32 "\t%" PRIu32 "\t%c%c%c\t",
+		printf("%" PRIu64 "\t%" PRIu32 "\t%" PRIu32 "\t%c%c%c\t",
 		       pf->id,
 		       pf->rule.attr->group,
 		       pf->rule.attr->priority,
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 329a6378a1..ba29d97293 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -215,7 +215,7 @@  struct port_table {
 struct port_flow {
 	struct port_flow *next; /**< Next flow in list. */
 	struct port_flow *tmp; /**< Temporary linking. */
-	uint32_t id; /**< Flow rule ID. */
+	uintptr_t id; /**< Flow rule ID. */
 	struct rte_flow *flow; /**< Opaque flow object returned by PMD. */
 	struct rte_flow_conv_rule rule; /**< Saved flow rule description. */
 	enum age_action_context_type age_type; /**< Age action context type. */
@@ -948,7 +948,7 @@  int port_queue_flow_create(portid_t port_id, queueid_t queue_id,
 			   const struct rte_flow_item *pattern,
 			   const struct rte_flow_action *actions);
 int port_queue_flow_destroy(portid_t port_id, queueid_t queue_id,
-			    bool postpone, uint32_t n, const uint32_t *rule);
+			    bool postpone, uint32_t n, const uintptr_t *rule);
 int port_queue_action_handle_create(portid_t port_id, uint32_t queue_id,
 			bool postpone, uint32_t id,
 			const struct rte_flow_indir_action_conf *conf,
@@ -984,11 +984,11 @@  int port_action_handle_query(portid_t port_id, uint32_t id);
 void update_age_action_context(const struct rte_flow_action *actions,
 		     struct port_flow *pf);
 int mcast_addr_pool_destroy(portid_t port_id);
-int port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule);
+int port_flow_destroy(portid_t port_id, uint32_t n, const uintptr_t *rule);
 int port_flow_flush(portid_t port_id);
 int port_flow_dump(portid_t port_id, bool dump_all,
-			uint32_t rule, const char *file_name);
-int port_flow_query(portid_t port_id, uint32_t rule,
+			uintptr_t rule, const char *file_name);
+int port_flow_query(portid_t port_id, uintptr_t rule,
 		    const struct rte_flow_action *action);
 void port_flow_list(portid_t port_id, uint32_t n, const uint32_t *group);
 void port_flow_aged(portid_t port_id, uint8_t destroy);