Re: [dpdk-dev] [PATCH v3 05/13] net/mlx5: swap items/actions validations for e-switch rules

[Yongseok Koh <yskoh@mellanox.com>]

On Thu, Nov 01, 2018 at 05:19:26AM -0700, Slava Ovsiienko wrote:
> The rule validation function for E-Switch checks item list first,
> then action list is checked. This patch swaps the validation order,
> now actions are checked first. This is preparation for validation
> function update with VXLAN tunnel actions. VXLAN decapsulation
> action requires to check the items in special way. We could do
> this special check in the single item check pass if the action
> flags were gathered before. This is the reason to swap the
> item/actions checking loops.
> 
> Suggested-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>

Thanks

>  drivers/net/mlx5/mlx5_flow_tcf.c | 260 +++++++++++++++++++--------------------
>  1 file changed, 130 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
> index 55c77e3..50f3bd1 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -1174,6 +1174,136 @@ struct pedit_parser {
>  	ret = flow_tcf_validate_attributes(attr, error);
>  	if (ret < 0)
>  		return ret;
> +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
> +		unsigned int i;
> +		uint64_t current_action_flag = 0;
> +
> +		switch (actions->type) {
> +		case RTE_FLOW_ACTION_TYPE_VOID:
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_PORT_ID:
> +			current_action_flag = MLX5_FLOW_ACTION_PORT_ID;
> +			if (!actions->conf)
> +				break;
> +			conf.port_id = actions->conf;
> +			if (conf.port_id->original)
> +				i = 0;
> +			else
> +				for (i = 0; ptoi[i].ifindex; ++i)
> +					if (ptoi[i].port_id == conf.port_id->id)
> +						break;
> +			if (!ptoi[i].ifindex)
> +				return rte_flow_error_set
> +					(error, ENODEV,
> +					 RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> +					 conf.port_id,
> +					 "missing data to convert port ID to"
> +					 " ifindex");
> +			port_id_dev = &rte_eth_devices[conf.port_id->id];
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_JUMP:
> +			current_action_flag = MLX5_FLOW_ACTION_JUMP;
> +			if (!actions->conf)
> +				break;
> +			conf.jump = actions->conf;
> +			if (attr->group >= conf.jump->group)
> +				return rte_flow_error_set
> +					(error, ENOTSUP,
> +					 RTE_FLOW_ERROR_TYPE_ACTION,
> +					 actions,
> +					 "can jump only to a group forward");
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_DROP:
> +			current_action_flag = MLX5_FLOW_ACTION_DROP;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_COUNT:
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_OF_POP_VLAN:
> +			current_action_flag = MLX5_FLOW_ACTION_OF_POP_VLAN;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN:
> +			current_action_flag = MLX5_FLOW_ACTION_OF_PUSH_VLAN;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID:
> +			if (!(action_flags & MLX5_FLOW_ACTION_OF_PUSH_VLAN))
> +				return rte_flow_error_set
> +					(error, ENOTSUP,
> +					 RTE_FLOW_ERROR_TYPE_ACTION, actions,
> +					 "vlan modify is not supported,"
> +					 " set action must follow push action");
> +			current_action_flag = MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP:
> +			if (!(action_flags & MLX5_FLOW_ACTION_OF_PUSH_VLAN))
> +				return rte_flow_error_set
> +					(error, ENOTSUP,
> +					 RTE_FLOW_ERROR_TYPE_ACTION, actions,
> +					 "vlan modify is not supported,"
> +					 " set action must follow push action");
> +			current_action_flag = MLX5_FLOW_ACTION_OF_SET_VLAN_PCP;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_IPV4_SRC;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_IPV4_DST;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_IPV6_SRC;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_IPV6_DST;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_TP_SRC;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_TP_DST;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_TTL:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_TTL;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_DEC_TTL:
> +			current_action_flag = MLX5_FLOW_ACTION_DEC_TTL;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_MAC_SRC:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_MAC_SRC;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_SET_MAC_DST:
> +			current_action_flag = MLX5_FLOW_ACTION_SET_MAC_DST;
> +			break;
> +		default:
> +			return rte_flow_error_set(error, ENOTSUP,
> +						  RTE_FLOW_ERROR_TYPE_ACTION,
> +						  actions,
> +						  "action not supported");
> +		}
> +		if (current_action_flag & MLX5_TCF_CONFIG_ACTIONS) {
> +			if (!actions->conf)
> +				return rte_flow_error_set(error, EINVAL,
> +						RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> +						actions,
> +						"action configuration not set");
> +		}
> +		if ((current_action_flag & MLX5_TCF_PEDIT_ACTIONS) &&
> +		    pedit_validated)
> +			return rte_flow_error_set(error, ENOTSUP,
> +						  RTE_FLOW_ERROR_TYPE_ACTION,
> +						  actions,
> +						  "set actions should be "
> +						  "listed successively");
> +		if ((current_action_flag & ~MLX5_TCF_PEDIT_ACTIONS) &&
> +		    (action_flags & MLX5_TCF_PEDIT_ACTIONS))
> +			pedit_validated = 1;
> +		if ((current_action_flag & MLX5_TCF_FATE_ACTIONS) &&
> +		    (action_flags & MLX5_TCF_FATE_ACTIONS))
> +			return rte_flow_error_set(error, EINVAL,
> +						  RTE_FLOW_ERROR_TYPE_ACTION,
> +						  actions,
> +						  "can't have multiple fate"
> +						  " actions");
> +		action_flags |= current_action_flag;
> +	}
>  	for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
>  		unsigned int i;
>  
> @@ -1375,136 +1505,6 @@ struct pedit_parser {
>  						  NULL, "item not supported");
>  		}
>  	}
> -	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
> -		unsigned int i;
> -		uint64_t current_action_flag = 0;
> -
> -		switch (actions->type) {
> -		case RTE_FLOW_ACTION_TYPE_VOID:
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_PORT_ID:
> -			current_action_flag = MLX5_FLOW_ACTION_PORT_ID;
> -			if (!actions->conf)
> -				break;
> -			conf.port_id = actions->conf;
> -			if (conf.port_id->original)
> -				i = 0;
> -			else
> -				for (i = 0; ptoi[i].ifindex; ++i)
> -					if (ptoi[i].port_id == conf.port_id->id)
> -						break;
> -			if (!ptoi[i].ifindex)
> -				return rte_flow_error_set
> -					(error, ENODEV,
> -					 RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> -					 conf.port_id,
> -					 "missing data to convert port ID to"
> -					 " ifindex");
> -			port_id_dev = &rte_eth_devices[conf.port_id->id];
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_JUMP:
> -			current_action_flag = MLX5_FLOW_ACTION_JUMP;
> -			if (!actions->conf)
> -				break;
> -			conf.jump = actions->conf;
> -			if (attr->group >= conf.jump->group)
> -				return rte_flow_error_set
> -					(error, ENOTSUP,
> -					 RTE_FLOW_ERROR_TYPE_ACTION,
> -					 actions,
> -					 "can jump only to a group forward");
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_DROP:
> -			current_action_flag = MLX5_FLOW_ACTION_DROP;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_COUNT:
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_OF_POP_VLAN:
> -			current_action_flag = MLX5_FLOW_ACTION_OF_POP_VLAN;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN:
> -			current_action_flag = MLX5_FLOW_ACTION_OF_PUSH_VLAN;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID:
> -			if (!(action_flags & MLX5_FLOW_ACTION_OF_PUSH_VLAN))
> -				return rte_flow_error_set
> -					(error, ENOTSUP,
> -					 RTE_FLOW_ERROR_TYPE_ACTION, actions,
> -					 "vlan modify is not supported,"
> -					 " set action must follow push action");
> -			current_action_flag = MLX5_FLOW_ACTION_OF_SET_VLAN_VID;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP:
> -			if (!(action_flags & MLX5_FLOW_ACTION_OF_PUSH_VLAN))
> -				return rte_flow_error_set
> -					(error, ENOTSUP,
> -					 RTE_FLOW_ERROR_TYPE_ACTION, actions,
> -					 "vlan modify is not supported,"
> -					 " set action must follow push action");
> -			current_action_flag = MLX5_FLOW_ACTION_OF_SET_VLAN_PCP;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_IPV4_SRC;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_IPV4_DST;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_IPV6_SRC;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_IPV6_DST;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_TP_SRC:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_TP_SRC;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_TP_DST:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_TP_DST;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_TTL:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_TTL;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_DEC_TTL:
> -			current_action_flag = MLX5_FLOW_ACTION_DEC_TTL;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_MAC_SRC:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_MAC_SRC;
> -			break;
> -		case RTE_FLOW_ACTION_TYPE_SET_MAC_DST:
> -			current_action_flag = MLX5_FLOW_ACTION_SET_MAC_DST;
> -			break;
> -		default:
> -			return rte_flow_error_set(error, ENOTSUP,
> -						  RTE_FLOW_ERROR_TYPE_ACTION,
> -						  actions,
> -						  "action not supported");
> -		}
> -		if (current_action_flag & MLX5_TCF_CONFIG_ACTIONS) {
> -			if (!actions->conf)
> -				return rte_flow_error_set(error, EINVAL,
> -						RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> -						actions,
> -						"action configuration not set");
> -		}
> -		if ((current_action_flag & MLX5_TCF_PEDIT_ACTIONS) &&
> -		    pedit_validated)
> -			return rte_flow_error_set(error, ENOTSUP,
> -						  RTE_FLOW_ERROR_TYPE_ACTION,
> -						  actions,
> -						  "set actions should be "
> -						  "listed successively");
> -		if ((current_action_flag & ~MLX5_TCF_PEDIT_ACTIONS) &&
> -		    (action_flags & MLX5_TCF_PEDIT_ACTIONS))
> -			pedit_validated = 1;
> -		if ((current_action_flag & MLX5_TCF_FATE_ACTIONS) &&
> -		    (action_flags & MLX5_TCF_FATE_ACTIONS))
> -			return rte_flow_error_set(error, EINVAL,
> -						  RTE_FLOW_ERROR_TYPE_ACTION,
> -						  actions,
> -						  "can't have multiple fate"
> -						  " actions");
> -		action_flags |= current_action_flag;
> -	}
>  	if ((action_flags & MLX5_TCF_PEDIT_ACTIONS) &&
>  	    (action_flags & MLX5_FLOW_ACTION_DROP))
>  		return rte_flow_error_set(error, ENOTSUP,
> -- 
> 1.8.3.1
>