On 9/16/2026 1:18 PM, Anatoly Burakov wrote:
Use the new flow graph API and the common parsing framework to implement
flow parser for flow director.
The FDIR flow tracking is moved inside the new engine, and the FDIR code is
refactored to not mix software tracking with HW writes.
Signed-off-by: Anatoly Burakov<[email protected]>
---
<snip>
+ hash_handle = rte_hash_create(&hash_params);
- /* drop queue is always fixed */
- IXGBE_DEV_FDIR_CONF(eth_dev)->drop_queue = IXGBE_FDIR_DROP_QUEUE;
+ if (hash_handle == NULL) {
+ PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
+ rte_hash_free(hash_handle);
no need to free NULL
+ ci_refcount_release(&state->ref);
+ return NULL;
+ }
+
+ state->hash_handle = hash_handle;
+ state->mask_conf.mode = RTE_FDIR_MODE_NONE;
+
+ /* drop queue is always fixed */
+ IXGBE_DEV_PRIVATE_TO_FDIR_CONF(adapter)->drop_queue =
IXGBE_FDIR_DROP_QUEUE;
+ }
- return 0;
+ return state;
}
<snip>
+/* hardware loses its flow director setup across a stop/start cycle
*/ +void +ixgbe_fdir_hw_invalidate(struct rte_eth_dev *dev) { - int
ret; + struct ixgbe_adapter *adapter = IXGBE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+ struct ixgbe_fdir_state *state = &adapter->fdir_state;
- ret = rte_hash_lookup(fdir_info->hash_handle, (const void *)key);
- if (ret < 0)
- return NULL;
-
- return fdir_info->hash_map[ret];
+ if (state != NULL) {
nit: can the state be NULL at all?
+ state->mask_conf.hw_configured = false;
+ state->mask_conf.mask_programmed = false;
+ }
}
<snip>
+
+static int
+ixgbe_flow_fdir_flow_unregister(struct ci_flow *flow, struct rte_flow_error
*error)
+{
+ struct ixgbe_fdir_flow *fdir_flow = (struct ixgbe_fdir_flow *)flow;
+ struct ixgbe_fdir_priv *priv = flow->engine_priv;
+ struct ixgbe_fdir_state *state = priv->state;
+ int ret;
+
+ ret = ixgbe_fdir_table_del(state->hash_handle, fdir_flow);
+ if (ret == -ENOENT) {
+ return rte_flow_error_set(error, ENOENT,
+ RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
+ "Flow director filter is missing from the filter
table");
+ }
+
+ ixgbe_fdir_mask_release(&state->mask_conf);
Do we also need to check if there are any other rules and, if there are
none, reset hw_configured as well as global_fdir_conf->mode, and restore
all HW state, such as the adjusted packet buffer (pballoc from
ixgbe_fdir_configure())?
I mean, should we do a similar cleanup to ixgbe_flow_fdir_flow_uninstall()?
We can end up in a situation where ixgbe_fdir_filter_program() fails
while installing the first flow, after the HW and masks have already
been configured. Based on the ci_flow_create() logic, if the
installation fails, only ci_flow_unregister() is called. This leaves us
in an inconsistent state where we have no flows installed, but FDIR
remains configured.
+
+ return 0;
+}
+
+static int
+ixgbe_flow_fdir_configure_hw(struct ixgbe_adapter *adapter,
+ struct ixgbe_fdir_mask_state *mask_state,
+ struct rte_flow_error *error)
+{
+ struct rte_eth_fdir_conf *global_fdir_conf =
IXGBE_DEV_PRIVATE_TO_FDIR_CONF(adapter);
+ struct rte_eth_fdir_conf local_fdir_conf = *global_fdir_conf;
+ int ret;
+
+ local_fdir_conf.mode = mask_state->mode;
+
+ ret = ixgbe_fdir_configure(adapter, &local_fdir_conf,
&mask_state->mask);
There is a problem with calling ixgbe_fdir_configure() multiple times.
This call changes the HW state:
/* ixgbe_fdir.c:ixgbe_fdir_configure() */
IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0),
(IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0)) - pbsize));
With every invocation, IXGBE_RXPBSIZE(0) is decreased by pbsize.
Could we configure FDIR only once during device start, without
configuring it with the first rule? Instead, for the first rule, we
could just call ixgbe_fdir_set_input_mask() / ixgbe_set_fdir_flex_conf().
+ if (ret != 0) {
+ return rte_flow_error_set(error, -ret,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
+ "Failed to configure flow director");
+ }
+
+ global_fdir_conf->mode = mask_state->mode;
+ mask_state->hw_configured = true;
+
+ return 0;
+}
+
+static int
+ixgbe_flow_fdir_program_mask(struct ixgbe_adapter *adapter,
+ struct ixgbe_fdir_mask_state *mask_state,
+ struct rte_flow_error *error)
+{
+ struct ixgbe_hw_fdir_info *global_fdir_info =
IXGBE_DEV_PRIVATE_TO_FDIR_INFO(adapter);
+ int ret;
+
+ if (mask_state->mask.flex_bytes_mask != 0) {
+ ret = ixgbe_fdir_set_flexbytes_offset(adapter,
mask_state->flex_bytes_offset);
+ if (ret != 0) {
+ return rte_flow_error_set(error, -ret,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
+ "Failed to set flex bytes offset");
+ }
+ }
+
+ ret = ixgbe_fdir_set_input_mask(adapter, &mask_state->mask,
mask_state->mode);
+ if (ret != 0) {
+ return rte_flow_error_set(error, -ret,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
+ "Failed to set input mask");
+ }
+
+ /* record what is now in hardware for ixgbe_fdir_info_get() */
+ global_fdir_info->mask = mask_state->mask;
+ global_fdir_info->flex_bytes_offset = mask_state->flex_bytes_offset;
+ mask_state->mask_programmed = true;
+
+ return 0;
+}
+
+static int
+ixgbe_flow_fdir_flow_install(struct ci_flow *flow,
+ struct rte_flow_error *error)
+{
+ struct ixgbe_adapter *adapter =
IXGBE_DEV_PRIVATE_TO_ADAPTER(flow->dev_data->dev_private);
+ struct ixgbe_fdir_flow *fdir_flow = (struct ixgbe_fdir_flow *)flow;
+ struct ixgbe_fdir_priv *priv = flow->engine_priv;
+ struct ixgbe_fdir_mask_state *mask_conf = &priv->state->mask_conf;
+ int ret;
+
+ if (!mask_conf->hw_configured) {
+ ret = ixgbe_flow_fdir_configure_hw(adapter, mask_conf, error);
+ if (ret != 0)
+ return ret;
return rte_flow_error_set() instead?
+ }
+
+ if (!mask_conf->mask_programmed) {
the previous invocation of the ixgbe_flow_fdir_configure_hw() have
already programmed mask
(->ixgbe_fdir_configure->ixgbe_fdir_set_input_mask). Do we really need
to do this one more time?
+ ret = ixgbe_flow_fdir_program_mask(adapter, mask_conf, error);
+ if (ret != 0)
do we need to call ixgbe_fdir_hw_invalidate() on failure?
+ return ret;
+ }
+
+ ret = ixgbe_fdir_filter_program(adapter, &fdir_flow->rule,
fdir_flow->queue,
+ fdir_flow->fdircmd_flags, fdir_flow->fdirhash);
+ if (ret != 0) {
+ return rte_flow_error_set(error, -ret,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
+ "Failed to program flow director filter");
+ }
+
+ return 0;
+}
+
+static int
+ixgbe_flow_fdir_flow_uninstall(struct ci_flow *flow,
+ struct rte_flow_error *error)
+{
+ struct ixgbe_adapter *adapter =
IXGBE_DEV_PRIVATE_TO_ADAPTER(flow->dev_data->dev_private);
+ struct rte_eth_fdir_conf *global_fdir_conf =
IXGBE_DEV_PRIVATE_TO_FDIR_CONF(adapter);
+ struct ixgbe_fdir_flow *fdir_flow = (struct ixgbe_fdir_flow *)flow;
+ struct ixgbe_fdir_priv *priv = flow->engine_priv;
+ struct ixgbe_fdir_mask_state *mask_conf = &priv->state->mask_conf;
+ int ret;
+
+ ret = ixgbe_fdir_filter_clear(adapter, fdir_flow->fdirhash);
+ if (ret != 0) {
+ return rte_flow_error_set(error, -ret,
+ RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
+ "Failed to remove flow director filter");
+ }
+
+ /* unregister has not run yet, so this filter is still counted */
+ if (mask_conf->ref.count > 1)
+ return 0;
+
+ mask_conf->hw_configured = false;
+ mask_conf->mask_programmed = false;
+ global_fdir_conf->mode = RTE_FDIR_MODE_NONE;
+
+ ret = ixgbe_fdir_reset_tables(adapter);
+ if (ret != 0) {
+ return rte_flow_error_set(error, -ret,
+ RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
+ "Failed to reset flow director tables");
Would it be worth doing this in a separate function, such as
ixgbe_flow_fdir_configure_hw(), and also resetting the masks to their
defaults?
+ }
+
+ return 0;
+}
<snip>
--
Regards,
Vladimir
