Re: [PATCH v3 10/19] net/ixgbe: reimplement FDIR parser

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

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<anatoly.burakov@intel.com>
> ---
<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