DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Steve Yang <stevex.yang@intel.com>, dev@dpdk.org
Cc: jia.guo@intel.com, haiyue.wang@intel.com, qiming.yang@intel.com,
	beilei.xing@intel.com, orika@nvidia.com, murphyx.yang@intel.com
Subject: Re: [dpdk-dev] [RFC v2 2/6] net/i40e: define the mirror filter parser
Date: Fri, 19 Feb 2021 12:56:52 +0000
Message-ID: <a9202452-0260-ff60-39eb-b4568979e0a5@intel.com> (raw)
In-Reply-To: <20201103082809.41149-3-stevex.yang@intel.com>

On 11/3/2020 8:28 AM, Steve Yang wrote:
> Define the sample filter parser for mirror, it will divide to two phases,
> the one is sample attributions pattern parsing, and the mirror config
> will be filled in according to pattern type VF/PF/VLAN when sample ratio
> is 1.
> The another is sample action parsing that the port id of mirror config
> will be filled in according to action type VF/PF.
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>

<...>

> +#define GET_VLAN_ID_FROM_TCI(vlan_item, default_vid) \
> +	((vlan_item) ? ntohs(vlan_item->tci) & 0x0fff : (default_vid))
> +

'ntohs' usage is breaking the windows build (i40e is now build for windows) 
because of missing header, can you please prefer:
  I40E_NTOHS()

<...>

> +	if (attr->group) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_GROUP,
> +				   attr, "Not support group.");
> +		return -rte_errno;
> +	}
> +	if (attr->transfer) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER,
> +				   attr, "Not support group.");

copy/paste error in the error msg.

<...>

> +static int
> +i40e_flow_parse_sample_action(struct rte_eth_dev *dev,
> +			      const struct rte_flow_action *actions,
> +			      struct rte_flow_error *error,
> +			      union i40e_filter_t *filter)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	const struct rte_flow_action *act;
> +	struct i40e_mirror_rule_conf *mirror_config =	&filter->mirror_conf;
> +	uint16_t *dst_vsi_seid = &mirror_config->dst_vsi_seid;
> +	uint32_t index = 0;
> +
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_SAMPLE) {
> +		rte_flow_error_set(error, EINVAL,
> +			RTE_FLOW_ERROR_TYPE_ACTION, act,
> +			"Not supported action.");
> +		return -rte_errno;
> +	}
> +

Is above check required, isn't this function called only if the action type is 
'sample'

> +	if (((const struct rte_flow_action_sample *)act->conf)->ratio != 1) {
> +		rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION_CONF, act,
> +				"Invalid ratio for mirror action");

So, only "ratio == 1" is supported, better to highlight this in the error message.

> +			return -rte_errno;
> +	}
> +
> +	index++;
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +
> +	if (act->type == RTE_FLOW_ACTION_TYPE_PORT_ID) {
> +		const struct rte_flow_action_port_id *conf =
> +			(const struct rte_flow_action_port_id *)act->conf;
> +
> +		if (!conf->id) {
> +			*dst_vsi_seid = pf->main_vsi_seid;
> +		} else if (conf->id <= pf->vf_num) {
> +			*dst_vsi_seid = pf->vfs[conf->id - 1].vsi->seid;
> +		} else {
> +			rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION_CONF, act,
> +				"Invalid port id mirror action");
> +			return -rte_errno;
> +		}
> +	}
> +
> +	/* Check if the next non-void item is END */
> +	index++;
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_END) {
> +		rte_flow_error_set(error, EINVAL,
> +			RTE_FLOW_ERROR_TYPE_ACTION, act,
> +			"Only support pf or vf parameter item.");

So, only 'PORT_ID' is supported.

Can you please add comment on both "i40e_flow_parse_sample_attr_pattern" & 
"i40e_flow_parse_sample_action" to document these restrictions, or expectation? 
It both helps to understand what to expect without diving deep into the code and 
documents intention which can help finding any possible coding error.


  parent reply	other threads:[~2021-02-19 12:56 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-14  8:41 [dpdk-dev] [PATCH v1 0/8] use generic flow command to re-realize mirror rule SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 1/8] ethdev: support the mirror action for flow SimonX Lu
2020-10-14 12:07   ` Ori Kam
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 2/8] app/testpmd: support action mirror for flow command SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 3/8] net/ixgbe: add mirror rule config and extend flow filter type SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 4/8] net/ixgbe: define the mirror filter paser SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 5/8] net/ixgbe: use generic flow command to re-realize mirror rule SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 6/8] net/i40e: add mirror rule config and export add/del rule APIs SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 7/8] net/i40e: define the mirror filter paser SimonX Lu
2020-10-14  8:41 ` [dpdk-dev] [PATCH v1 8/8] net/i40e: use generic flow command to re-realize mirror rule SimonX Lu
     [not found] ` <20201103082809.41149-1-stevex.yang@intel.com>
2020-11-03  8:28   ` [dpdk-dev] [RFC v2 1/6] net/i40e: add mirror rule config and add/del rule APIs Steve Yang
2020-11-03  8:28   ` [dpdk-dev] [RFC v2 2/6] net/i40e: define the mirror filter parser Steve Yang
2021-02-18 18:59     ` Thomas Monjalon
2021-02-19 12:56     ` Ferruh Yigit [this message]
2020-11-03  8:28   ` [dpdk-dev] [RFC v2 3/6] net/i40e: use generic flow command to re-realize mirror rule Steve Yang
2020-11-03  8:28   ` [dpdk-dev] [RFC v2 4/6] net/ixgbe: add mirror rule config and add/del rule APIs Steve Yang
2020-11-03  8:28   ` [dpdk-dev] [RFC v2 5/6] net/ixgbe: define the mirror filter parser Steve Yang
2021-02-19 12:57     ` Ferruh Yigit
2020-11-03  8:28   ` [dpdk-dev] [RFC v2 6/6] net/ixgbe: use flow sample to re-realize mirror rule Steve Yang
2021-02-19 12:59     ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a9202452-0260-ff60-39eb-b4568979e0a5@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=beilei.xing@intel.com \
    --cc=dev@dpdk.org \
    --cc=haiyue.wang@intel.com \
    --cc=jia.guo@intel.com \
    --cc=murphyx.yang@intel.com \
    --cc=orika@nvidia.com \
    --cc=qiming.yang@intel.com \
    --cc=stevex.yang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git