net/ixgbe: fix repeated Rx packet buffer shrink for FDIR

Enabling Flow Director reduces RXPBSIZE to reserve SRAM for the filter table. Clearing the last FDIR rule set the mode back to none without restoring that buffer, so the next flow create ran configure again and subtracted the same amount a second time.

Keep the FDIR mode after rules are removed, and subtract RXPBSIZE only once until the next hardware reset.

---
 drivers/net/intel/ixgbe/ixgbe_ethdev.c |  7 +++++--
 drivers/net/intel/ixgbe/ixgbe_ethdev.h |  1 +
 drivers/net/intel/ixgbe/ixgbe_fdir.c   | 21 ++++++++++++++-------
 drivers/net/intel/ixgbe/ixgbe_flow.c   |  1 -
 4 files changed, 20 insertions(+), 10 deletions(-)

index cbb7b4831c..ce5b4c51f5 100644
@@ -1372,6 +1372,7 @@ static int ixgbe_fdir_filter_init(struct rte_eth_dev *eth_dev)
 	}
 	fdir_info->n_flows = 0;
 	fdir_info->mask_added = FALSE;
+	fdir_info->rx_pb_reduced = false;
 
 	/* drop queue is always fixed */
 	IXGBE_DEV_FDIR_CONF(eth_dev)->drop_queue = IXGBE_FDIR_DROP_QUEUE;

@@ -2541,6 +2542,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
 	struct ixgbe_macsec_setting *macsec_setting =
 		IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
+	struct ixgbe_hw_fdir_info *info =
+		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(adapter);
 
 	/*
 	 * This function calls into the base driver, which in turn will use

@@ -2628,9 +2631,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	/* Configure DCB hw */
 	ixgbe_configure_dcb(dev);
 
+	/* reset_hw restored RXPBSIZE; FDIR must subtract again */
+	info->rx_pb_reduced = false;
 	if (fdir_conf->mode != RTE_FDIR_MODE_NONE) {
-		struct ixgbe_hw_fdir_info *info =
-			IXGBE_DEV_PRIVATE_TO_FDIR_INFO(adapter);
 		err = ixgbe_fdir_configure(adapter, fdir_conf, &info->mask);
 		if (err)
 			goto error;

index ba826b6d8d..495a6ddd22 100644
@@ -200,6 +200,7 @@ struct ixgbe_hw_fdir_info {
 	struct rte_hash *hash_handle; /* cuckoo hash handler */
 	uint32_t n_flows;
 	bool mask_added; /* If already got mask from consistent filter */
+	bool rx_pb_reduced; /* RXPBSIZE already reduced for FDIR */
 };
 
 struct ixgbe_rte_flow_rss_conf {

index b32dc54287..ca96ea85d7 100644
@@ -560,8 +560,10 @@ ixgbe_fdir_configure(struct ixgbe_adapter *adapter,
 		const struct ixgbe_hw_fdir_mask *fdir_mask)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(adapter);
+	struct ixgbe_hw_fdir_info *info =
+			IXGBE_DEV_PRIVATE_TO_FDIR_INFO(adapter);
 	int err;
-	uint32_t fdirctrl, pbsize;
+	uint32_t fdirctrl;
 	int i;
 	enum rte_fdir_mode mode = fdir_conf->mode;
 

@@ -591,11 +593,18 @@ ixgbe_fdir_configure(struct ixgbe_adapter *adapter,
 	/*
 	 * Before enabling Flow Director, the Rx Packet Buffer size
 	 * must be reduced.  The new value is the current size minus
-	 * flow director memory usage size.
+	 * flow director memory usage size.  Do this only once; a later
+	 * re-configure (e.g. after a failed filter program) must not
+	 * subtract again.
 	 */
-	pbsize = (1 << (PBALLOC_SIZE_SHIFT + (fdirctrl & FDIRCTRL_PBALLOC_MASK)));
-	IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0),
-	    (IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0)) - pbsize));
+	if (!info->rx_pb_reduced) {
+		uint32_t pbsize = (1 << (PBALLOC_SIZE_SHIFT +
+					(fdirctrl & FDIRCTRL_PBALLOC_MASK)));
+
+		IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0),
+		    (IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0)) - pbsize));
+		info->rx_pb_reduced = true;
+	}
 
 	/*
 	 * The defaults in the HW for RX PB 1-7 are not zero and so should be

@@ -1359,7 +1368,6 @@ ixgbe_fdir_filter_restore(struct rte_eth_dev *dev)
 int
 ixgbe_clear_all_fdir_filter(struct rte_eth_dev *dev)
 {
-	struct rte_eth_fdir_conf *fdir_conf = IXGBE_DEV_FDIR_CONF(dev);
 	struct ixgbe_hw_fdir_info *fdir_info =
 		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
 	struct ixgbe_fdir_filter *fdir_filter;

@@ -1384,7 +1392,6 @@ ixgbe_clear_all_fdir_filter(struct rte_eth_dev *dev)
 	fdir_info->mask = (struct ixgbe_hw_fdir_mask){0};
 	fdir_info->flex_bytes_offset = 0;
 	fdir_info->mask_added = FALSE;
-	fdir_conf->mode = RTE_FDIR_MODE_NONE;
 
 	if (had_flows)
 		ret = ixgbe_fdir_flush(dev);

index 6868893d46..1e79ef8b28 100644
@@ -3164,7 +3164,6 @@ ixgbe_flow_destroy(struct rte_eth_dev *dev,
 				fdir_info->mask_added = false;
 				fdir_info->mask = (struct ixgbe_hw_fdir_mask){0};
 				fdir_info->flex_bytes_offset = 0;
-				fdir_conf->mode = RTE_FDIR_MODE_NONE;
 			}
 		}
 		break;

2.53.0

Reviewing this patch, I'm wondering if this approach is actually the best way to fix this. For example, after this change, if flow director is used and then all rules removed, the flow director mode is still preserved, possibly preventing use of other features that conflict with the old fdir mode.

I wonder if a cleaner fix - which should be shorter too - might be to instead just restore the RXPBSIZE value back when the fdir mode is set to NONE. This gives a clean combination of (enable + set RXPBSIZE0) and (disable + reset RXPBSIZE).

What do you think?

/Bruce

A general comment: instead of reducing/bringing things back and storing a flag noting whether we did, I would rather do the following:

0) store default rx pb size at init
1) on enabling FDIR, recalculate using that value minus FDIR table size
2) on disabling FDIR[*], restore the default
3) similarly, on enable/disable VMDq, recalculate and/or reset

[*] there is no "disable FDIR" call, only fdir flush which just flushes the FDIR tables but does not actually disable FDIR. arguably, we should convert it to "disable FDIR" by flushing FDIR *and* writing 0 to FDIRCTRL *and* restoring rx pb size to defaults. naturally, after running fdir disable function, FDIR will need to be reconfigured for next FDIR flow and get rx pb size recalculated again.

So, a bit of a refactor, but I think that would make way more sense.

Thanks,
Anatoly

I asked an AI to implement a fix based on this, and here's what it came up with, it is roughly what I would like to see instead (obviously, please review/rework as appropriate e.g. to properly support VMDq as well):

index b32dc542874..9f48a27cb3a 100644
@@ -101,7 +101,6 @@ static int fdir_write_perfect_filter_82599(struct ixgbe_hw *hw,
 static int fdir_add_signature_filter_82599(struct ixgbe_hw *hw,
                union ixgbe_atr_input *input, u8 queue, uint32_t fdircmd,
                uint32_t fdirhash);
-static int ixgbe_fdir_flush(struct rte_eth_dev *dev);
 
 /**
  * This function is based on ixgbe_fdir_enable_82599() in 

base/ixgbe_82599.c.
@@ -554,6 +553,20 @@ ixgbe_set_fdir_flex_conf(struct ixgbe_adapter *adapter,

        return 0;

}

+static void
+ixgbe_fdir_disable(struct ixgbe_hw *hw)
+{
+ uint32_t rx_pb_size;
+ int i;
+

+       IXGBE_WRITE_REG(hw, IXGBE_FDIRCTRL, 0);
+       rx_pb_size = (uint32_t)hw->mac.rx_pb_size << IXGBE_RXPBSIZE_SHIFT;
+       IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0), rx_pb_size);
+       for (i = 1; i < 8; i++)

+ IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(i), 0);
+ IXGBE_WRITE_FLUSH(hw);
+}
+
int
ixgbe_fdir_configure(struct ixgbe_adapter *adapter,
const struct rte_eth_fdir_conf *fdir_conf,
@@ -561,7 +574,7 @@ ixgbe_fdir_configure(struct ixgbe_adapter *adapter,

 {
        struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(adapter);
        int err;
-       uint32_t fdirctrl, pbsize;
+       uint32_t fdirctrl, pbsize, rx_pb_size;
        int i;
        enum rte_fdir_mode mode = fdir_conf->mode;

@@ -589,13 +602,14 @@ ixgbe_fdir_configure(struct ixgbe_adapter *adapter,

                return err;

        /*
-        * Before enabling Flow Director, the Rx Packet Buffer size
-        * must be reduced.  The new value is the current size minus
-        * flow director memory usage size.
+        * Before enabling Flow Director, the Rx Packet Buffer size must be
+        * reduced. The new value is the default size minus flow director
+        * memory usage size.
         */

- pbsize = (1 << (PBALLOC_SIZE_SHIFT + (fdirctrl & FDIRCTRL_PBALLOC_MASK)));
- IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0),
- (IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0)) - pbsize));
+ pbsize = 1 << (PBALLOC_SIZE_SHIFT +
+ (fdirctrl & FDIRCTRL_PBALLOC_MASK));
+ rx_pb_size = (uint32_t)hw->mac.rx_pb_size << IXGBE_RXPBSIZE_SHIFT;
+ IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0), rx_pb_size - pbsize);

/*
* The defaults in the HW for RX PB 1-7 are not zero and so
should be
@@ -609,21 +623,25 @@ ixgbe_fdir_configure(struct ixgbe_adapter *adapter,

        err = ixgbe_fdir_set_input_mask(adapter, fdir_mask, mode);
        if (err < 0) {
                PMD_INIT_LOG(ERR, " Error on setting FD mask");
-               return err;
+               goto error;
        }
        err = ixgbe_set_fdir_flex_conf(adapter, &fdir_conf->flex_conf,
                                       &fdirctrl);
        if (err < 0) {
                PMD_INIT_LOG(ERR, " Error on setting FD flexible 
arguments.");
-               return err;
+               goto error;
        }

        err = fdir_enable_82599(hw, fdirctrl);
        if (err < 0) {
                PMD_INIT_LOG(ERR, " Error on enabling FD.");
-               return err;
+               goto error;
        }
        return 0;

+
+error:
+ ixgbe_fdir_disable(hw);
+ return err;
}

/*
@@ -1180,28 +1198,6 @@ ixgbe_fdir_filter_program(struct ixgbe_adapter *adapter,

        return err;

}

-static int
-ixgbe_fdir_flush(struct rte_eth_dev *dev)
-{
- struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
- struct ixgbe_hw_fdir_info *info =
- IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
- int ret;
-
- ret = ixgbe_reinit_fdir_tables_82599(hw);
- if (ret < 0) {
- PMD_INIT_LOG(ERR, "Failed to re-initialize FD table.");
- return ret;
- }
-

-       info->f_add = 0;
-       info->f_remove = 0;
-       info->add = 0;
-       info->remove = 0;

-
- return ret;
-}
-
#define FDIRENTRIES_NUM_SHIFT 10
void
ixgbe_fdir_info_get(struct rte_eth_dev *dev, struct rte_eth_fdir_info
*fdir_info)
@@ -1360,13 +1356,26 @@ int
ixgbe_clear_all_fdir_filter(struct rte_eth_dev *dev)

 {
        struct rte_eth_fdir_conf *fdir_conf = IXGBE_DEV_FDIR_CONF(dev);
+       struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
        struct ixgbe_hw_fdir_info *fdir_info =
                IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
        struct ixgbe_fdir_filter *fdir_filter;
-       bool had_flows;
-       int ret = 0;
+       int ret;

- had_flows = (fdir_info->n_flows != 0);
+ if (fdir_conf->mode != RTE_FDIR_MODE_NONE) {
+ ret = ixgbe_reinit_fdir_tables_82599(hw);
+ if (ret < 0) {
+ PMD_INIT_LOG(ERR, "Failed to re-initialize FD table.");
+ return ret;
+ }
+

+               fdir_info->f_add = 0;
+               fdir_info->f_remove = 0;
+               fdir_info->add = 0;
+               fdir_info->remove = 0;

+
+ ixgbe_fdir_disable(hw);
+ }

        /* flush flow director */
        rte_hash_reset(fdir_info->hash_handle);
@@ -1386,8 +1395,5 @@ ixgbe_clear_all_fdir_filter(struct rte_eth_dev *dev)
        fdir_info->mask_added = FALSE;
        fdir_conf->mode = RTE_FDIR_MODE_NONE;

- if (had_flows)
- ret = ixgbe_fdir_flush(dev);
-
- return ret;
+ return 0;
}

index 6868893d46a..da05e61e8b4 100644
@@ -3157,15 +3157,15 @@ ixgbe_flow_destroy(struct rte_eth_dev *dev,
                memcpy(&fdir_rule,
                        &fdir_rule_ptr->filter_info,
                        sizeof(struct ixgbe_fdir_rule));
-               ret = ixgbe_fdir_filter_program(adapter, fdir_conf, &fdir_rule, TRUE, FALSE);
+               if (fdir_info->n_flows == 1)
+                       ret = ixgbe_clear_all_fdir_filter(dev);
+               else
+                       ret = ixgbe_fdir_filter_program(adapter, fdir_conf,
+                                       &fdir_rule, TRUE, FALSE);
                if (!ret) {
                        rte_free(fdir_rule_ptr);
-                       if (fdir_info->n_flows > 0 && --(fdir_info->n_flows) == 0) {
-                               fdir_info->mask_added = false;
-                               fdir_info->mask = (struct ixgbe_hw_fdir_mask){0};
-                               fdir_info->flex_bytes_offset = 0;
-                               fdir_conf->mode = RTE_FDIR_MODE_NONE;
-                       }
+                       if (fdir_info->n_flows > 1)
+                               fdir_info->n_flows--;
                }
                break;
        case RTE_ETH_FILTER_L2_TUNNEL:

Thanks,
Anatoly

Hi Anatoly, Bruce,

Thanks for the review. Agreed, keeping the FDIR mode after the last rule is gone is wrong (a later rule with another mode fails with "Conflict with existing fdir mode"). I'll drop the flag and do v2 along the lines of the draft: RXPBSIZE(0) computed from the default size on enable, restored on disable, and FDIR disabled on the configure error path and when the first rule fails after FDIR was just enabled.

Unless you object, I plan to:

1. Use hw->mac.rx_pb_size as the default size (512 on 82599, 384 on X540/X550/E610) instead of saving RXPBSIZE at init.

2. Leave VMDq out: ixgbe_fdir_configure() rejects all DCB mq modes, and the non-DCB VMDq/SR-IOV paths don't write RXPBSIZE, so PB0 is the only buffer whenever FDIR can be enabled.

3. Not address the fact that FDIR enable/disable from the flow API changes RXPBSIZE/FDIRCTRL while Rx is enabled if the port is started. Enabling on the first rule already works this way today; restoring the buffer on the last rule removal adds the disable side. dev_start only touches these registers with Rx disabled.

I'll send v2 rebased on next-net-intel in the next few days.

Thanks

Enabling Flow Director reduces RXPBSIZE(0) to reserve packet buffer memory for the filter table. The new size was computed from the current register value, and nothing restored it when FDIR state was reset. After the last FDIR rule was removed, or when the first rule failed after FDIR had been enabled, the next rule created enabled FDIR again and reduced the buffer a second time.

Compute RXPBSIZE(0) from the default packet buffer size, and disable FDIR and restore the buffer when the last rule is removed, on flush, and on the enable error paths.

---
v2:
- Drop the rx_pb_reduced flag and keep resetting the FDIR mode to NONE
when the last rule is removed (Bruce).
- Compute RXPBSIZE(0) from hw->mac.rx_pb_size, and disable FDIR and
restore the buffer when the last rule is removed or on flush, based
on Anatoly's draft.
- Disable FDIR again if the first rule fails after enabling it.
- Do not fail flush if re-initializing the FDIR tables fails, as FDIR
is disabled right after.
- Add Fixes tag for the first-rule failure path.
- Compile tested only.
- Author name follows the pending .mailmap update: <https://patches.dpdk.org/project/dpdk/patch/[email protected]/>

 drivers/net/intel/ixgbe/ixgbe_ethdev.h |  1 +
 drivers/net/intel/ixgbe/ixgbe_fdir.c   | 70 +++++++++++++-------------
 drivers/net/intel/ixgbe/ixgbe_flow.c   | 34 ++++++++-----
 3 files changed, 58 insertions(+), 47 deletions(-)

index 2e9d166e29..dfc8fce1a8 100644
@@ -700,6 +700,7 @@ void ixgbe_filterlist_flush(struct rte_eth_dev *dev);
 int ixgbe_fdir_configure(struct rte_eth_dev *dev,
 			 const struct rte_eth_fdir_conf *fdir_conf,
 			 const struct ixgbe_hw_fdir_mask *fdir_mask);
+void ixgbe_fdir_disable(struct ixgbe_hw *hw);
 int ixgbe_fdir_set_input_mask(struct ixgbe_adapter *adapter,
 			      const struct ixgbe_hw_fdir_mask *mask,
 			      enum rte_fdir_mode mode);

index ff56d7b279..a8d1b269c7 100644
@@ -2625,6 +2625,7 @@ ixgbe_fdir_flow_program(struct rte_eth_dev *dev,
 	struct rte_eth_fdir_conf local_fdir_conf = *fdir_conf;
 	struct ixgbe_hw_fdir_info *fdir_info =
 			IXGBE_DEV_PRIVATE_TO_FDIR_INFO(adapter);
+	bool fdir_enabled = false;
 	int ret;
 
 	if (fdir_rule->queue >= dev->data->nb_rx_queues) {

@@ -2647,6 +2648,7 @@ ixgbe_fdir_flow_program(struct rte_eth_dev *dev,
 				RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				NULL, "Failed to configure fdir mode");
 		}
+		fdir_enabled = true;
 	} else if (fdir_conf->mode != fdir_rule->mode) {
 		return rte_flow_error_set(error, EINVAL,
 			RTE_FLOW_ERROR_TYPE_UNSPECIFIED,

@@ -2657,17 +2659,25 @@ ixgbe_fdir_flow_program(struct rte_eth_dev *dev,
 	ret = ixgbe_fdir_process_rule(adapter, fdir_info, fdir_rule,
 		first_mask, error);
 	if (ret)
-		return ret;
+		goto error;
 
 	/* Program the filter */
 	ret = ixgbe_fdir_filter_program(adapter, &local_fdir_conf,
 			fdir_rule, FALSE, FALSE);
-	if (ret)
-		return rte_flow_error_set(error, EINVAL,
+	if (ret) {
+		ret = rte_flow_error_set(error, EINVAL,
 			RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 			NULL, "Failed to add fdir filter");
+		goto error;
+	}
 
 	return 0;
+
+error:
+	/* FDIR mode is only recorded on success, so undo the enable */
+	if (fdir_enabled)
+		ixgbe_fdir_disable(IXGBE_DEV_PRIVATE_TO_HW(adapter));
+	return ret;
 }
 
 /* Flow actions check specific to RSS filter */

@@ -3159,16 +3169,16 @@ ixgbe_flow_destroy(struct rte_eth_dev *dev,
 	case RTE_ETH_FILTER_FDIR:
 		fdir_rule_ptr = (struct ixgbe_fdir_rule_ele *)pmd_flow->rule;
 		fdir_rule = fdir_rule_ptr->filter_info;
-		ret = ixgbe_fdir_filter_program(adapter, fdir_conf, &fdir_rule, TRUE, FALSE);
-		if (!ret) {
-			rte_free(fdir_rule_ptr);
-			if (fdir_info->n_flows > 0 && --(fdir_info->n_flows) == 0) {
-				fdir_info->mask_added = false;
-				fdir_info->mask = (struct ixgbe_hw_fdir_mask){0};
-				fdir_info->flex_bytes_offset = 0;
-				fdir_conf->mode = RTE_FDIR_MODE_NONE;
-			}
+		if (fdir_info->n_flows == 1) {
+			ret = ixgbe_clear_all_fdir_filter(dev);
+		} else {
+			ret = ixgbe_fdir_filter_program(adapter, fdir_conf,
+					&fdir_rule, TRUE, FALSE);
+			if (!ret && fdir_info->n_flows > 0)
+				fdir_info->n_flows--;
 		}
+		if (!ret)
+			rte_free(fdir_rule_ptr);
 		break;
 	case RTE_ETH_FILTER_L2_TUNNEL:
 		l2_tn_filter_ptr = (struct ixgbe_eth_l2_tunnel_conf_ele *)

2.53.0

Applied to dpdk-next-net-intel

/Bruce