On 9/25/2026 11:47 AM, Burakov, Anatoly wrote:
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.
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):
diff --git a/drivers/net/intel/ixgbe/ixgbe_fdir.c
b/drivers/net/intel/ixgbe/ixgbe_fdir.c
index b32dc542874..9f48a27cb3a 100644
--- a/drivers/net/intel/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/intel/ixgbe/ixgbe_fdir.c
@@ -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;
}
diff --git a/drivers/net/intel/ixgbe/ixgbe_flow.c
b/drivers/net/intel/ixgbe/ixgbe_flow.c
index 6868893d46a..da05e61e8b4 100644
--- a/drivers/net/intel/ixgbe/ixgbe_flow.c
+++ b/drivers/net/intel/ixgbe/ixgbe_flow.c
@@ -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
