dev Digest, Vol 318, Issue 122

dpdk-dev McDaniel, Timothy 1 participant 1 message Sep 22, 2020 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

McDaniel, Timothy

author

2020-09-22 15:55 UTC #1 lore raw

Thanks for the heads up

Tim

Quoted 494 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of [email protected]
Sent: Tuesday, September 22, 2020 10:45 AM
To: [email protected]
Subject: dev Digest, Vol 318, Issue 122
Send dev mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://mails.dpdk.org/listinfo/dev
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific than "Re: Contents of dev digest..."
Today's Topics:
1. Re: [PATCH v3 1/6] app/testpmd: fix missing verification of port id (Ferruh Yigit)
2. Re: [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set (Ferruh Yigit)
3. Re: [PATCH v3 5/6] app/testpmd: fix valid desc id check (Ferruh Yigit)
4. Re: [RFC v2 1/1] app/testpmd: distinguish ICMP identifier fields in packet (Ferruh Yigit)
5. Re: [PATCH v3] app/testpmd: fix the default RSS key configuration (Phil Yang)
----------------------------------------------------------------------
Message: 1
Date: Tue, 22 Sep 2020 15:49:36 +0100
From: Ferruh Yigit <[email protected]>
To: "Wei Hu (Xavier)" <[email protected]>, [email protected]
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH v3 1/6] app/testpmd: fix missing
verification of port id
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Quoted 8 lines · Wei Hu (Xavier)
From: Chengchang Tang <[email protected]>
To set Tx vlan offloads, it is required to stop port firstly. But before checking whether the port is stopped, the port id entered by the user is not checked for validity. When the port id is illegal, it would lead to a segmentation fault since it attempts to access a member of non-existent port.
Quoted 8 lines
This patch adds verification of port id in tx vlan offloads and remove duplicated check.
Fixes 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc [email protected]
Signed-off-by Chengchang Tang <[email protected]>
Signed-off-by Wei Hu (Xavier) <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>
------------------------------
Message: 2
Date: Tue, 22 Sep 2020 15:51:20 +0100
From: Ferruh Yigit <[email protected]>
To: "Wei Hu (Xavier)" <[email protected]>, [email protected]
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction
on txpkts set
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Quoted 12 lines · Wei Hu (Xavier)
From: Chengchang Tang <[email protected]>
Currently, if nb_txd is not set, the txpkts is not allowed to be set because the nb_txd is used to avoid the numer of segments exceed the Tx ring size and the default value of nb_txd is 0. And there is a bug that nb_txd is the global configuration for Tx ring size and the ring size could be changed by some command per queue. So these valid check is unreliable and introduced unnecessary constraints.
This patch adds a valid check function to use the real Tx ring size to check the validity of txpkts.
Quoted 7 lines
Fixes af75078fece3 ("first public release")
Cc [email protected]
Signed-off-by Chengchang Tang <[email protected]>
Signed-off-by Wei Hu (Xavier) <[email protected]>
---
  app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++---
-
Quoted 15 lines
  1 file changed, 38 insertions(+), 4 deletions(-)
app/test-pmd/config.c +5 -0
index 4e33208..882de2d 100644
@@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
  	printf("Split packet: %s\n", split);
  }
 
+static bool
+nb_segs_is_invalid(unsigned int nb_segs)
+{
+	uint16_t port_id;
+
app/test-pmd/config.c show_tx_pkt_segments(void) · line 3001
Quoted 10 lines
app/test-pmd/config.c +10 -0
+	RTE_ETH_FOREACH_DEV(port_id) {
+		struct rte_port *port = &ports[port_id];
+		uint16_t ring_size;
+		uint16_t queue_id;
+
+		/*
+		 * When configure the txq by rte_eth_tx_queue_setup with
+		 * nb_tx_desc being 0, it will use a default value provided by
+		 * PMDs to setup this txq. If the default value is 0, it will
+		 * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this
txq.
app/test-pmd/config.c show_tx_pkt_segments(void) · line 3010
Quoted 9 lines
app/test-pmd/config.c +9 -0
+		 */
+		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
+			if (port->nb_tx_desc[queue_id])
+				ring_size = port->nb_tx_desc[queue_id];
+			else if (port->dev_info.default_txportconf.ring_size)
+				ring_size =
+				    port->dev_info.default_txportconf.ring_size;
+			else
+				ring_size =
RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
app/test-pmd/config.c show_tx_pkt_segments(void) · line 3018
Quoted 8 lines
app/test-pmd/config.c +8 -0
+
+			if (ring_size < nb_segs) {
+				printf("nb segments per TX packets=%u >= TX "
+				       "queue(%u) ring_size=%u - ignored\n",
+					nb_segs, queue_id, ring_size);
+				return true;
+			}
+		}
What do you think calling 'rte_eth_rx_queue_info_get()' & 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
------------------------------
Message: 3
Date: Tue, 22 Sep 2020 15:53:04 +0100
From: Ferruh Yigit <[email protected]>
To: "Wei Hu (Xavier)" <[email protected]>, [email protected]
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH v3 5/6] app/testpmd: fix valid desc id
check
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Quoted 13 lines · Wei Hu (Xavier)
From: Chengchang Tang <[email protected]>
The number of desc is a per queue configuration. But in the check function, nb_txd & nb_rxd are used to check whether the desc_id is valid. nb_txd & nb_rxd are the global configuration of number of desc. If the queue configuration is changed by cmdline liks: "port config xx txq xx ring_size xxx", the real value will be changed.
This patch use the real value to check whether the desc_id is valid. And if these are not configured by user. It will use the default value to check it, since the rte_eth_rx_queue_setup & rte_eth_tx_queue_setup will use a default value to confiure the queue if nb_rx_desc or nb_tx_desc is zero.
Quoted 7 lines
Fixes af75078fece3 ("first public release")
Cc [email protected]
Signed-off-by Chengchang Tang <[email protected]>
Signed-off-by Wei Hu (Xavier) <[email protected]>
---
  app/test-pmd/config.c | 53
+++++++++++++++++++++++++++++++++++++++++----------
Quoted 27 lines
  1 file changed, 43 insertions(+), 10 deletions(-)
app/test-pmd/config.c +14 -2
index 882de2d..b7851c7 100644
@@ -1891,22 +1891,55 @@ tx_queue_id_is_invalid(queueid_t txq_id)
  }
 
  static int
-rx_desc_id_is_invalid(uint16_t rxdesc_id)
+rx_desc_id_is_invalid(portid_t port_id, queueid_t rxq_id, uint16_t rxdesc_id)
  {
-	if (rxdesc_id < nb_rxd)
+	struct rte_port *port = &ports[port_id];
+	uint16_t ring_size;
+
+	/*
+	 * When configure the rxq by rte_eth_rx_queue_setup with nb_rx_desc
+	 * being 0, it will use a default value provided by PMDs to setup this
+	 * rxq. If the default value is 0, it will use the
+	 * RTE_ETH_DEV_FALLBACK_RX_RINGSIZE to setup this rxq.
+	 */
+	if (port->nb_rx_desc[rxq_id])
+		ring_size = port->nb_rx_desc[rxq_id];
+	else if (port->dev_info.default_rxportconf.ring_size)
+		ring_size = port->dev_info.default_rxportconf.ring_size;
app/test-pmd/config.c tx_queue_id_is_invalid(queueid_t txq_id) · line 1916
Quoted 10 lines
app/test-pmd/config.c +6 -2
+	else
+		ring_size =  RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+
+	if (rxdesc_id < ring_size)
  		return 0;
-	printf("Invalid RX descriptor %d (must be < nb_rxd=%d)\n",
-	       rxdesc_id, nb_rxd);
+	printf("Invalid RX descriptor %d (must be < ring_size=%d)\n",
+	       rxdesc_id, ring_size);
  	return 1;
+1 to fix, but similar comment as previous patch, can 'rte_eth_rx_queue_info_get()' be used to detect the 'ring_size'?
------------------------------
Message: 4
Date: Tue, 22 Sep 2020 16:38:13 +0100
From: Ferruh Yigit <[email protected]>
To: Li Zhang <[email protected]>, [email protected], [email protected],
[email protected], [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [dpdk-dev] [RFC v2 1/1] app/testpmd: distinguish ICMP
identifier fields in packet
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Quoted 16 lines · Li Zhang
From: lizh <[email protected]>
Ability to distinguish ICMP identifier fields in packets. Dstinguish ICMP sequence number field too.
Already supports ICMP code and type fields in current version. Existing fields in ICMP header contain the required information. ICMP header already is supported and no code change in RTE FLOW. Extend testpmd CLI to include the fields of ident and sequence number. One example:
flow create 0 ingress pattern eth / ipv4 /
icmp code is 1 ident is 5 seq is 6 /
end actions count / queue index 0 / end
The ICMP packet with code 1, identifier 5 and
sequence number 6 will be matched.
It will implement action counter and forward to queue 0.
Overall looks good. @Ori, any objection?
@Li, is there any PMD implementation planned to use these icmp fields?
Quoted 28 lines
Signed-off-by Li Zhang <[email protected]>
---
  app/test-pmd/cmdline_flow.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)
app/test-pmd/cmdline_flow.c +4 -0
index 6263d307ed..6e04d538ea 100644
@@ -143,6 +143,8 @@ enum index {
  	ITEM_ICMP,
  	ITEM_ICMP_TYPE,
  	ITEM_ICMP_CODE,
+	ITEM_ICMP_IDENT,
+	ITEM_ICMP_SEQ,
  	ITEM_UDP,
  	ITEM_UDP_SRC,
  	ITEM_UDP_DST,
@@ -893,6 +895,8 @@ static const enum index item_ipv6[] = {
  static const enum index item_icmp[] = {
  	ITEM_ICMP_TYPE,
  	ITEM_ICMP_CODE,
+	ITEM_ICMP_IDENT,
+	ITEM_ICMP_SEQ,
  	ITEM_NEXT,
  	ZERO,
  };
app/test-pmd/cmdline_flow.c static const struct token token_list[] = { · line 2203
Quoted 7 lines
app/test-pmd/cmdline_flow.c +4 -0
@@ -2193,6 +2197,20 @@ static const struct token token_list[] = {
  		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp,
  					     hdr.icmp_code)),
  	},
+	[ITEM_ICMP_IDENT] = {
+		.name = "ident",
+		.help = "ICMP packet identifier",
+		.next = NEXT(item_icmp, NEXT_ENTRY(UNSIGNED),
item_param),
app/test-pmd/cmdline_flow.c static const struct token token_list[] = { · line 2210
Quoted 7 lines
app/test-pmd/cmdline_flow.c +7 -0
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp,
+					     hdr.icmp_ident)),
+	},
+	[ITEM_ICMP_SEQ] = {
+		.name = "seq",
+		.help = "ICMP packet sequence number",
+		.next = NEXT(item_icmp, NEXT_ENTRY(UNSIGNED),
item_param),
app/test-pmd/cmdline_flow.c static const struct token token_list[] = { · line 2216
Quoted 7 lines
app/test-pmd/cmdline_flow.c +3 -0
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_icmp,
+					     hdr.icmp_seq_nb)),
+	},
  	[ITEM_UDP] = {
  		.name = "udp",
  		.help = "match UDP header",
------------------------------
Message: 5
Date: Tue, 22 Sep 2020 15:44:29 +0000
From: Phil Yang <[email protected]>
To: oulijun <[email protected]>, "[email protected]"
<[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>
Cc: "[email protected]" <[email protected]>, "[email protected]"
	<[email protected]>, nd <[email protected]>, nd <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS
	key configuration
Message-ID:
	<[email protected]
urprd08.prod.outlook.com>
Content-Type: text/plain; charset="us-ascii"
Quoted 21 lines · dev <[email protected]> On Behalf Of Lijun Ou writes:
Quoted 13 lines
Quoted 2 lines
Subject: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration
Hi Lijun,
Please fix the coding style issues.
"Must be a reply to the first patch (--in-reply-to)."
Quoted 3 lines
When a user runs a flow create cmd to configure an RSS rule with specifying the empty rss actions in testpmd, this mean
Quoted 8 lines
                                                                                                     ^^^ means
Quoted 2 lines
that the flow gets the default RSS functions from the valid NIC default RSS hash key. However, the testpmd is not set
^^^ is set xxx incorrectly
Quoted 2 lines
the default RSS key incorrectly when RSS key is not specified in flow create cmd.
Use the NIC valid default RSS key instead of the testpmd dummy RSS key in
Quoted 8 lines
the flow configuration when the RSS key is not specified in the flow rule. If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the default key.
Quoted 2 lines
Is that good to put it in this way? Because I think it is not a bug, your patch
offers an approach to update the default testpmd RSS key.
Quoted 1 line
Do you have any better advice? or don't use my approach? I think the
No, I think you misunderstood me. I agree with your proposal and your patch looks good to me.
My suggestion is to reword the commit message to highlight that you mare making testpmd use the valid NIC RSS key as the default flow RSS key in this patch.
In my perspective, if you don't specify any RSS key in your flow rule, it should allow any available RSS key work as the default key.
So use a dummy RSS key is correct as well.
Quoted 15 lines
previous methods are easy to misunderstand.Can we use the NUL KEY solution and fix the problem that the length is 0 and the RSS key is not NULL ?
Thanks
Lijun Ou
Quoted 1 line
I would also suggest making the commit message shorter and move the
test result into the cover letter.
Quoted 7 lines
Because the checkepatch tool is not happy with the hex dumped below.
http://mails.dpdk.org/archives/test-report/2020-September/151395.html
Thanks,
Phil
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
the cmdline as flows:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
Quoted 12 lines
Quoted 12 lines
20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end
3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-udp udp
RSS key:
74657374706D6427732064656661756C74205253532068617368206B65792C20 6F
Quoted 10 lines
Quoted 9 lines
Quoted 9 lines
76657272696465
Now, it uses rte_eth_dev_rss_hash_conf_get to correctly the default rss key. the cmdline and result as flows:
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F2
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines
0C
6A42B73BBEAC01FA
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end
testpmd> show port 0 rss-hash key
RSS functions:
all ipv4-udp udp
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
Quoted 10 lines
Quoted 10 lines
20C6A42B73BBEAC01FA
Fixes ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
Cc [email protected]
Signed-off-by Lijun Ou <[email protected]>
---
V2->V3:
-fix checkpatch warning.
Quoted 41 lines
Quoted 7 lines
app/test-
Quoted 7 lines
V1->V2:
(quoted diff) +0 -2
-fix the commit.
---
  app/test-pmd/cmdline_flow.c | 8 ++++++++
  1 file changed, 8 insertions(+)
app/test- renamed from app/test-pmd/cmdline_flow.c +0 -0
pmd/cmdline_flow.c
Quoted 33 lines
Quoted 26 lines
index 6263d30..e6648da 100644
app/test-pmd/cmdline_flow.c +8 -0
@@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const
 struct token *token,
  		action_rss_data->queue[i] = i;
  	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
  	    ctx->port != (portid_t)RTE_PORT_ALL) {
+		struct rte_eth_rss_conf rss_conf = {0};
  		struct rte_eth_dev_info info;
  		int ret2;
@@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const
 struct token *token,
  		action_rss_data->conf.key_len =
  			RTE_MIN(sizeof(action_rss_data->key),
  				info.hash_key_size);
+
+		rss_conf.rss_key_len = sizeof(action_rss_data->key);
+		rss_conf.rss_key = action_rss_data->key;
+		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port,
 &rss_conf);
+		if (ret2 != 0)
+			return ret2;
+		action_rss_data->conf.key = rss_conf.rss_key;
  	}
Quoted 4 lines
action->conf = &action_rss_data->conf;
return ret;
--
2.7.4
.
End of dev Digest, Vol 318, Issue 122
*************************************