[PATCH 0/1] net/ena: devargs api change

[PATCH 0/1] net/ena: devargs api change

[shaibran]

From: Shai Brandes <shaibran@amazon.com>

Hi everyone, this is an API change to the ena PMD devargs that removes
the 3 devargs that control the user setting for the LLQ policy
and replaces it with a new shared devarg that implements the same logic.

Shai Brandes (1):
  net/ena: restructure the llq policy user setting

 doc/guides/nics/ena.rst                |  25 ++----
 doc/guides/rel_notes/release_24_07.rst |   8 ++
 drivers/net/ena/ena_ethdev.c           | 104 +++++++++----------------
 drivers/net/ena/ena_ethdev.h           |   3 -
 4 files changed, 51 insertions(+), 89 deletions(-)

-- 
2.17.1

[PATCH 1/1] net/ena: restructure the llq policy user setting

[shaibran]

From: Shai Brandes <shaibran@amazon.com>

Replaced `enable_llq`, `normal_llq_hdr` and `large_llq_hdr`
devargs with a new shared devarg named `llq_policy` that
implements the same logic and accepts the following values:
0 - Disable LLQ.
    Use with extreme caution as it leads to a huge performance
    degradation on AWS instances from 6th generation onwards.
1 - Accept device recommended LLQ policy (Default).
    Device can recommend normal or large LLQ policy.
2 - Enforce normal LLQ policy.
3 - Enforce large LLQ policy.
    Required for packets with header that exceed 96 bytes on
    AWS instances prior to 5th generation.

Signed-off-by: Shai Brandes <shaibran@amazon.com>
Reviewed-by: Amit Bernstein <amitbern@amazon.com>
---
 doc/guides/nics/ena.rst                |  25 ++----
 doc/guides/rel_notes/release_24_07.rst |   8 ++
 drivers/net/ena/ena_ethdev.c           | 104 +++++++++----------------
 drivers/net/ena/ena_ethdev.h           |   3 -
 4 files changed, 51 insertions(+), 89 deletions(-)

diff --git a/doc/guides/nics/ena.rst b/doc/guides/nics/ena.rst
index 2b105834a0..8f693ac3c9 100644
--- a/doc/guides/nics/ena.rst
+++ b/doc/guides/nics/ena.rst
@@ -107,15 +107,15 @@ Configuration
 Runtime Configuration
 ^^^^^^^^^^^^^^^^^^^^^
 
-   * **large_llq_hdr** (default 0)
+   * **llq_policy** (default 1)
 
-     Enables or disables usage of large LLQ headers. This option will have
-     effect only if the device also supports large LLQ headers. Otherwise, the
-     default value will be used.
-
-   * **normal_llq_hdr** (default 0)
-
-     Enforce normal LLQ policy.
+      Controls whether use device recommended header policy or override it.
+      0 - Disable LLQ.
+         **Use with extreme caution as it leads to a huge performance
+           degradation on AWS instances from 6th generation onwards.**
+      1 - Accept device recommended LLQ policy (Default).
+      2 - Enforce normal LLQ policy.
+      3 - Enforce large LLQ policy.
 
    * **miss_txc_to** (default 5)
 
@@ -126,15 +126,6 @@ Runtime Configuration
      timer service. Setting this parameter to 0 disables this feature. Maximum
      allowed value is 60 seconds.
 
-   * **enable_llq** (default 1)
-
-     Determines whenever the driver should use the LLQ (if it's available) or
-     not.
-
-     **NOTE: On the 6th generation AWS instances disabling LLQ may lead to a
-     huge performance degradation. In general disabling LLQ is highly not
-     recommended!**
-
    * **control_poll_interval** (default 0)
 
      Enable polling-based functionality of the admin queues,
diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
index e68a53d757..1fa678864d 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -73,6 +73,12 @@ New Features
   ``bpf_obj_get()`` for an xskmap pinned (by the AF_XDP DP) inside the
   container.
 
+* **Updated Amazon ena (Elastic Network Adapter) net driver.**
+
+  * Modified the PMD API that controls the LLQ header policy.
+    Replaced ``enable_llq``, ``normal_llq_hdr`` and ``large_llq_hdr`` devargs
+    with a new shared devarg ``llq_policy`` that keeps the same logic.
+
 * **Update Tap PMD driver.**
 
   * Updated to support up to 8 queues when used by secondary process.
@@ -117,6 +123,8 @@ API Changes
    This section is a comment. Do not overwrite or remove it.
    Also, make sure to start the actual text at the margin.
    =======================================================
+* drivers/net/ena: Removed ``enable_llq``, ``normal_llq_hdr`` and ``large_llq_hdr`` devargs
+  and replaced it with a new shared devarg ``llq_policy`` that keeps the same logic.
 
 
 ABI Changes
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 66fc287faf..e3c2696ae1 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -81,18 +81,27 @@ struct ena_stats {
 	ENA_STAT_ENTRY(stat, srd)
 
 /* Device arguments */
-#define ENA_DEVARG_LARGE_LLQ_HDR "large_llq_hdr"
-#define ENA_DEVARG_NORMAL_LLQ_HDR "normal_llq_hdr"
+
+/* Controls whether to disable LLQ, use device recommended header policy
+ * or overriding the device recommendation.
+ * 0 - Disable LLQ.
+ *     Use with extreme caution as it leads to a huge performance
+ *     degradation on AWS instances from 6th generation onwards.
+ * 1 - Accept device recommended LLQ policy (Default).
+ *     Device can recommend normal or large LLQ policy.
+ * 2 - Enforce normal LLQ policy.
+ * 3 - Enforce large LLQ policy.
+ *     Required for packets with header that exceed 96 bytes on
+ *     AWS instances prior to 5th generation.
+ */
+ #define ENA_DEVARG_LLQ_POLICY "llq_policy"
+
+
 /* Timeout in seconds after which a single uncompleted Tx packet should be
  * considered as a missing.
  */
 #define ENA_DEVARG_MISS_TXC_TO "miss_txc_to"
-/*
- * Controls whether LLQ should be used (if available). Enabled by default.
- * NOTE: It's highly not recommended to disable the LLQ, as it may lead to a
- * huge performance degradation on 6th generation AWS instances.
- */
-#define ENA_DEVARG_ENABLE_LLQ "enable_llq"
+
 /*
  * Controls the period of time (in milliseconds) between two consecutive inspections of
  * the control queues when the driver is in poll mode and not using interrupts.
@@ -296,9 +305,9 @@ static int ena_xstats_get_by_id(struct rte_eth_dev *dev,
 				const uint64_t *ids,
 				uint64_t *values,
 				unsigned int n);
-static int ena_process_bool_devarg(const char *key,
-				   const char *value,
-				   void *opaque);
+static int ena_process_llq_policy_devarg(const char *key,
+					 const char *value,
+					 void *opaque);
 static int ena_parse_devargs(struct ena_adapter *adapter,
 			     struct rte_devargs *devargs);
 static void ena_copy_customer_metrics(struct ena_adapter *adapter,
@@ -314,7 +323,6 @@ static int ena_rx_queue_intr_disable(struct rte_eth_dev *dev,
 static int ena_configure_aenq(struct ena_adapter *adapter);
 static int ena_mp_primary_handle(const struct rte_mp_msg *mp_msg,
 				 const void *peer);
-static ena_llq_policy ena_define_llq_hdr_policy(struct ena_adapter *adapter);
 static bool ena_use_large_llq_hdr(struct ena_adapter *adapter, uint8_t recommended_entry_size);
 
 static const struct eth_dev_ops ena_dev_ops = {
@@ -2292,9 +2300,6 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
 
 	/* Assign default devargs values */
 	adapter->missing_tx_completion_to = ENA_TX_TIMEOUT;
-	adapter->enable_llq = true;
-	adapter->use_large_llq_hdr = false;
-	adapter->use_normal_llq_hdr = false;
 
 	/* Get user bypass */
 	rc = ena_parse_devargs(adapter, pci_dev->device.devargs);
@@ -2302,7 +2307,6 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
 		PMD_INIT_LOG(CRIT, "Failed to parse devargs\n");
 		goto err;
 	}
-	adapter->llq_header_policy = ena_define_llq_hdr_policy(adapter);
 
 	rc = ena_com_allocate_customer_metrics_buffer(ena_dev);
 	if (rc != 0) {
@@ -3736,44 +3740,29 @@ static int ena_process_uint_devarg(const char *key,
 	return 0;
 }
 
-static int ena_process_bool_devarg(const char *key,
-				   const char *value,
-				   void *opaque)
+static int ena_process_llq_policy_devarg(const char *key, const char *value, void *opaque)
 {
 	struct ena_adapter *adapter = opaque;
-	bool bool_value;
+	uint32_t policy;
 
-	/* Parse the value. */
-	if (strcmp(value, "1") == 0) {
-		bool_value = true;
-	} else if (strcmp(value, "0") == 0) {
-		bool_value = false;
+	policy = strtoul(value, NULL, DECIMAL_BASE);
+	if (policy < ENA_LLQ_POLICY_LAST) {
+		adapter->llq_header_policy = policy;
 	} else {
-		PMD_INIT_LOG(ERR,
-			"Invalid value: '%s' for key '%s'. Accepted: '0' or '1'\n",
-			value, key);
+		PMD_INIT_LOG(ERR, "Invalid value: '%s' for key '%s'. valid [0-3]\n", value, key);
 		return -EINVAL;
 	}
-
-	/* Now, assign it to the proper adapter field. */
-	if (strcmp(key, ENA_DEVARG_LARGE_LLQ_HDR) == 0)
-		adapter->use_large_llq_hdr = bool_value;
-	else if (strcmp(key, ENA_DEVARG_NORMAL_LLQ_HDR) == 0)
-		adapter->use_normal_llq_hdr = bool_value;
-	else if (strcmp(key, ENA_DEVARG_ENABLE_LLQ) == 0)
-		adapter->enable_llq = bool_value;
-
+	PMD_DRV_LOG(INFO,
+		"LLQ policy is %u [0 - disabled, 1 - device recommended, 2 - normal, 3 - large]\n",
+		adapter->llq_header_policy);
 	return 0;
 }
 
-static int ena_parse_devargs(struct ena_adapter *adapter,
-			     struct rte_devargs *devargs)
+static int ena_parse_devargs(struct ena_adapter *adapter, struct rte_devargs *devargs)
 {
 	static const char * const allowed_args[] = {
-		ENA_DEVARG_LARGE_LLQ_HDR,
-		ENA_DEVARG_NORMAL_LLQ_HDR,
+		ENA_DEVARG_LLQ_POLICY,
 		ENA_DEVARG_MISS_TXC_TO,
-		ENA_DEVARG_ENABLE_LLQ,
 		ENA_DEVARG_CONTROL_PATH_POLL_INTERVAL,
 		NULL,
 	};
@@ -3785,27 +3774,17 @@ static int ena_parse_devargs(struct ena_adapter *adapter,
 
 	kvlist = rte_kvargs_parse(devargs->args, allowed_args);
 	if (kvlist == NULL) {
-		PMD_INIT_LOG(ERR, "Invalid device arguments: %s\n",
-			devargs->args);
+		PMD_INIT_LOG(ERR, "Invalid device arguments: %s\n", devargs->args);
 		return -EINVAL;
 	}
-
-	rc = rte_kvargs_process(kvlist, ENA_DEVARG_LARGE_LLQ_HDR,
-		ena_process_bool_devarg, adapter);
-	if (rc != 0)
-		goto exit;
-	rc = rte_kvargs_process(kvlist, ENA_DEVARG_NORMAL_LLQ_HDR,
-		ena_process_bool_devarg, adapter);
+	rc = rte_kvargs_process(kvlist, ENA_DEVARG_LLQ_POLICY,
+		ena_process_llq_policy_devarg, adapter);
 	if (rc != 0)
 		goto exit;
 	rc = rte_kvargs_process(kvlist, ENA_DEVARG_MISS_TXC_TO,
 		ena_process_uint_devarg, adapter);
 	if (rc != 0)
 		goto exit;
-	rc = rte_kvargs_process(kvlist, ENA_DEVARG_ENABLE_LLQ,
-		ena_process_bool_devarg, adapter);
-	if (rc != 0)
-		goto exit;
 	rc = rte_kvargs_process(kvlist, ENA_DEVARG_CONTROL_PATH_POLL_INTERVAL,
 		ena_process_uint_devarg, adapter);
 	if (rc != 0)
@@ -4029,9 +4008,7 @@ RTE_PMD_REGISTER_PCI(net_ena, rte_ena_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ena, pci_id_ena_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ena, "* igb_uio | uio_pci_generic | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_ena,
-	ENA_DEVARG_LARGE_LLQ_HDR "=<0|1> "
-	ENA_DEVARG_NORMAL_LLQ_HDR "=<0|1> "
-	ENA_DEVARG_ENABLE_LLQ "=<0|1> "
+	ENA_DEVARG_LLQ_POLICY "=<0|1|2|3> "
 	ENA_DEVARG_MISS_TXC_TO "=<uint>"
 	ENA_DEVARG_CONTROL_PATH_POLL_INTERVAL "=<0-1000>");
 RTE_LOG_REGISTER_SUFFIX(ena_logtype_init, init, NOTICE);
@@ -4219,17 +4196,6 @@ ena_mp_primary_handle(const struct rte_mp_msg *mp_msg, const void *peer)
 	return rte_mp_reply(&mp_rsp, peer);
 }
 
-static ena_llq_policy ena_define_llq_hdr_policy(struct ena_adapter *adapter)
-{
-	if (!adapter->enable_llq)
-		return ENA_LLQ_POLICY_DISABLED;
-	if (adapter->use_large_llq_hdr)
-		return ENA_LLQ_POLICY_LARGE;
-	if (adapter->use_normal_llq_hdr)
-		return ENA_LLQ_POLICY_NORMAL;
-	return ENA_LLQ_POLICY_RECOMMENDED;
-}
-
 static bool ena_use_large_llq_hdr(struct ena_adapter *adapter, uint8_t recommended_entry_size)
 {
 	if (adapter->llq_header_policy == ENA_LLQ_POLICY_LARGE) {
diff --git a/drivers/net/ena/ena_ethdev.h b/drivers/net/ena/ena_ethdev.h
index 7d82d222ce..fe7d4a2d65 100644
--- a/drivers/net/ena/ena_ethdev.h
+++ b/drivers/net/ena/ena_ethdev.h
@@ -337,9 +337,6 @@ struct ena_adapter {
 	uint32_t active_aenq_groups;
 
 	bool trigger_reset;
-	bool enable_llq;
-	bool use_large_llq_hdr;
-	bool use_normal_llq_hdr;
 	ena_llq_policy llq_header_policy;
 
 	uint32_t last_tx_comp_qid;
-- 
2.17.1

Re: [PATCH 1/1] net/ena: restructure the llq policy user setting

[Ferruh Yigit]

On 6/6/2024 2:33 PM, shaibran@amazon.com wrote:
> From: Shai Brandes <shaibran@amazon.com>
> 
> Replaced `enable_llq`, `normal_llq_hdr` and `large_llq_hdr`
> devargs with a new shared devarg named `llq_policy` that
> implements the same logic and accepts the following values:
> 0 - Disable LLQ.
>     Use with extreme caution as it leads to a huge performance
>     degradation on AWS instances from 6th generation onwards.
> 1 - Accept device recommended LLQ policy (Default).
>     Device can recommend normal or large LLQ policy.
> 2 - Enforce normal LLQ policy.
> 3 - Enforce large LLQ policy.
>     Required for packets with header that exceed 96 bytes on
>     AWS instances prior to 5th generation.
> 
> Signed-off-by: Shai Brandes <shaibran@amazon.com>
> Reviewed-by: Amit Bernstein <amitbern@amazon.com>
>

Hi Shai,

This patch changes device parameters and impacts end user.
Although this is not part of ABI policy, and we don't have an explicit
policy around it, but since it may impact end user experience, would you
be OK to postpone this patch to v24.11 release, where ABI break is planned?

RE: [PATCH 1/1] net/ena: restructure the llq policy user setting

[Brandes, Shai]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

Sure, thanks!

בתאריך 5 ביולי 2024 20:32,‏ Ferruh Yigit <ferruh.yigit@amd.com> כתב:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On 6/6/2024 2:33 PM, shaibran@amazon.com wrote:
> From: Shai Brandes <shaibran@amazon.com>
>
> Replaced `enable_llq`, `normal_llq_hdr` and `large_llq_hdr`
> devargs with a new shared devarg named `llq_policy` that
> implements the same logic and accepts the following values:
> 0 - Disable LLQ.
>     Use with extreme caution as it leads to a huge performance
>     degradation on AWS instances from 6th generation onwards.
> 1 - Accept device recommended LLQ policy (Default).
>     Device can recommend normal or large LLQ policy.
> 2 - Enforce normal LLQ policy.
> 3 - Enforce large LLQ policy.
>     Required for packets with header that exceed 96 bytes on
>     AWS instances prior to 5th generation.
>
> Signed-off-by: Shai Brandes <shaibran@amazon.com>
> Reviewed-by: Amit Bernstein <amitbern@amazon.com>
>

Hi Shai,

This patch changes device parameters and impacts end user.
Although this is not part of ABI policy, and we don't have an explicit
policy around it, but since it may impact end user experience, would you
be OK to postpone this patch to v24.11 release, where ABI break is planned?




[-- Attachment #2: Type: text/html, Size: 2262 bytes --]