[1/2,RFC] : ethdev: add pre-defined meter policy API

  Currently, the flow meter policy does not support multiple actions
per color; also the allowed action types per color are very limited.
In addition, the policy cannot be pre-defined.

Due to the growing in flow actions offload abilities there is a potential
for the user to use variety of actions per color differently.
This new meter policy API comes to allow this potential in the most ethdev
common way using rte_flow action definition.
A list of rte_flow actions will be provided by the user per color
in order to create a meter policy.
In addition, the API forces to pre-define the policy before
the meters creation in order to allow sharing of single policy
with multiple meters efficiently.

meter_policy_id is added into struct rte_mtr_params.
So that it can get the policy during the meters creation.

Policy id 0 is default policy. Action per color as below:
green - no action, yellow - no action, red - drop

Allow coloring the packet using a new rte_flow_action_color
as could be done by the old policy API,

The next API function were added:
- rte_mtr_meter_policy_add
- rte_mtr_meter_policy_delete
- rte_mtr_meter_policy_update
- rte_mtr_meter_policy_validate
The next struct was changed:
- rte_mtr_params
- rte_mtr_capabilities
The next API was deleted:
- rte_mtr_policer_actions_update

Signed-off-by: Li Zhang <lizh@nvidia.com>
---
 lib/librte_ethdev/rte_flow.h       |  18 ++++
 lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
 lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
 lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
 4 files changed, 210 insertions(+), 74 deletions(-)
  

Hi Li and Matan,

Thank you for your proposal, some comments below.

I am also adding Jerin and Hemant to this thread, as they also participated in the definition of the rte_mtr API in 2017. Also Ajit expressed some interest in a previous email.

> -----Original Message-----
> From: Li Zhang <lizh@nvidia.com>
> Sent: Thursday, March 18, 2021 8:58 AM
> To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> Currently, the flow meter policy does not support multiple actions
> per color; also the allowed action types per color are very limited.
> In addition, the policy cannot be pre-defined.
> 
> Due to the growing in flow actions offload abilities there is a potential
> for the user to use variety of actions per color differently.
> This new meter policy API comes to allow this potential in the most ethdev
> common way using rte_flow action definition.
> A list of rte_flow actions will be provided by the user per color
> in order to create a meter policy.
> In addition, the API forces to pre-define the policy before
> the meters creation in order to allow sharing of single policy
> with multiple meters efficiently.
> 
> meter_policy_id is added into struct rte_mtr_params.
> So that it can get the policy during the meters creation.
> 
> Policy id 0 is default policy. Action per color as below:
> green - no action, yellow - no action, red - drop
> 
> Allow coloring the packet using a new rte_flow_action_color
> as could be done by the old policy API,
> 

The proposal essentially is to define the meter policy based on rte_flow actions rather than a reduced action set defined specifically just for meter object. This makes sense to me.

> The next API function were added:
> - rte_mtr_meter_policy_add
> - rte_mtr_meter_policy_delete
> - rte_mtr_meter_policy_update
> - rte_mtr_meter_policy_validate
> The next struct was changed:
> - rte_mtr_params
> - rte_mtr_capabilities
> The next API was deleted:
> - rte_mtr_policer_actions_update
> 
> Signed-off-by: Li Zhang <lizh@nvidia.com>
> ---
>  lib/librte_ethdev/rte_flow.h       |  18 ++++
>  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
>  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
>  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
>  4 files changed, 210 insertions(+), 74 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 669e677e91..5f38aa7fa4 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -31,6 +31,7 @@
>  #include <rte_ecpri.h>
>  #include <rte_mbuf.h>
>  #include <rte_mbuf_dyn.h>
> +#include <rte_meter.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
>  	 * See struct rte_flow_action_modify_field.
>  	 */
>  	RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> +
> +	/**
> +	 * Color the packet to reflect the meter color result.
> +	 *
> +	 * See struct rte_flow_action_color.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_COlOR,

Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.

>  };
> 
>  /**
> @@ -2828,6 +2836,16 @@ struct rte_flow_action_set_dscp {
>   */
>  struct rte_flow_shared_action;
> 
> +/**
> + * RTE_FLOW_ACTION_TYPE_COLOR
> + *
> + * The meter color should be set in the packet meta-data
> + * (i.e. struct rte_mbuf::sched::color).
> + */
> +struct rte_flow_action_color {
> +	enum rte_color color; /**< Green/Yellow/Red. */

I would avoid expanding the list of colors in the comment, as we might have to support more than 3 colors in the future. I would simply write: "Packet color."

> +};
> +
>  /**
>   * Field IDs for MODIFY_FIELD action.
>   */
> diff --git a/lib/librte_ethdev/rte_mtr.c b/lib/librte_ethdev/rte_mtr.c
> index 3073ac03f2..fccec3760b 100644
> --- a/lib/librte_ethdev/rte_mtr.c
> +++ b/lib/librte_ethdev/rte_mtr.c
> @@ -91,6 +91,40 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
>  		meter_profile_id, error);
>  }
> 
> +/* MTR meter policy validate */
> +int
> +rte_mtr_meter_policy_validate(uint16_t port_id,
> +	const struct rte_flow_action *actions[RTE_COLORS],
> +	struct rte_mtr_error *error)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	return RTE_MTR_FUNC(port_id, meter_policy_validate)(dev,
> +		actions, error);
> +}
> +
> +/* MTR meter policy add */
> +int
> +rte_mtr_meter_policy_add(uint16_t port_id,
> +	uint32_t policy_id,
> +	const struct rte_flow_action *actions[RTE_COLORS],
> +	struct rte_mtr_error *error)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	return RTE_MTR_FUNC(port_id, meter_policy_add)(dev,
> +		policy_id, actions, error);
> +}
> +
> +/** MTR meter policy delete */
> +int
> +rte_mtr_meter_policy_delete(uint16_t port_id,
> +	uint32_t policy_id,
> +	struct rte_mtr_error *error)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	return RTE_MTR_FUNC(port_id, meter_policy_delete)(dev,
> +		policy_id, error);
> +}
> +
>  /** MTR object create */
>  int
>  rte_mtr_create(uint16_t port_id,
> @@ -149,29 +183,28 @@ rte_mtr_meter_profile_update(uint16_t port_id,
>  		mtr_id, meter_profile_id, error);
>  }
> 
> -/** MTR object meter DSCP table update */
> +/** MTR object meter policy update */
>  int
> -rte_mtr_meter_dscp_table_update(uint16_t port_id,
> +rte_mtr_meter_policy_update(uint16_t port_id,
>  	uint32_t mtr_id,
> -	enum rte_color *dscp_table,
> +	uint32_t meter_policy_id,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> -	return RTE_MTR_FUNC(port_id, meter_dscp_table_update)(dev,
> -		mtr_id, dscp_table, error);
> +	return RTE_MTR_FUNC(port_id, meter_policy_update)(dev,
> +		mtr_id, meter_policy_id, error);
>  }
> 
> -/** MTR object policer action update */
> +/** MTR object meter DSCP table update */
>  int
> -rte_mtr_policer_actions_update(uint16_t port_id,
> +rte_mtr_meter_dscp_table_update(uint16_t port_id,
>  	uint32_t mtr_id,
> -	uint32_t action_mask,
> -	enum rte_mtr_policer_action *actions,
> +	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> -	return RTE_MTR_FUNC(port_id, policer_actions_update)(dev,
> -		mtr_id, action_mask, actions, error);
> +	return RTE_MTR_FUNC(port_id, meter_dscp_table_update)(dev,
> +		mtr_id, dscp_table, error);
>  }
> 
>  /** MTR object enabled stats update */
> diff --git a/lib/librte_ethdev/rte_mtr.h b/lib/librte_ethdev/rte_mtr.h
> index 916a09c5c3..07961f2777 100644
> --- a/lib/librte_ethdev/rte_mtr.h
> +++ b/lib/librte_ethdev/rte_mtr.h
> @@ -49,6 +49,7 @@
>  #include <rte_compat.h>
>  #include <rte_common.h>
>  #include <rte_meter.h>
> +#include <rte_flow.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -174,23 +175,6 @@ struct rte_mtr_meter_profile {
>  	};
>  };
> 
> -/**
> - * Policer actions
> - */
> -enum rte_mtr_policer_action {
> -	/** Recolor the packet as green. */
> -	MTR_POLICER_ACTION_COLOR_GREEN = 0,
> -
> -	/** Recolor the packet as yellow. */
> -	MTR_POLICER_ACTION_COLOR_YELLOW,
> -
> -	/** Recolor the packet as red. */
> -	MTR_POLICER_ACTION_COLOR_RED,
> -
> -	/** Drop the packet. */
> -	MTR_POLICER_ACTION_DROP,
> -};
> -
>  /**
>   * Parameters for each traffic metering & policing object
>   *
> @@ -232,13 +216,13 @@ struct rte_mtr_params {
>  	 */
>  	int meter_enable;
> 
> -	/** Policer actions (per meter output color). */
> -	enum rte_mtr_policer_action action[RTE_COLORS];
> -
>  	/** Set of stats counters to be enabled.
>  	 * @see enum rte_mtr_stats_type
>  	 */
>  	uint64_t stats_mask;
> +
> +	/** Meter policy ID. */
> +	uint32_t meter_policy_id;
>  };
> 
>  /**
> @@ -324,6 +308,13 @@ struct rte_mtr_capabilities {
>  	 */
>  	uint64_t meter_rate_max;
> 
> +	/**
> +	 * Maximum number of policy objects that can have.
> +	 * The value of 0 is invalid. Policy must be supported for meter.
> +	 * The maximum value is *n_max*.
> +	 */
> +	uint64_t meter_policy_n_max;
> +
>  	/**
>  	 * When non-zero, it indicates that color aware mode is supported
> for
>  	 * the srTCM RFC 2697 metering algorithm.
> @@ -342,18 +333,6 @@ struct rte_mtr_capabilities {
>  	 */
>  	int color_aware_trtcm_rfc4115_supported;
> 
> -	/** When non-zero, it indicates that the policer packet recolor
> actions
> -	 * are supported.
> -	 * @see enum rte_mtr_policer_action
> -	 */
> -	int policer_action_recolor_supported;
> -
> -	/** When non-zero, it indicates that the policer packet drop action is
> -	 * supported.
> -	 * @see enum rte_mtr_policer_action
> -	 */
> -	int policer_action_drop_supported;
> -
>  	/** Set of supported statistics counter types.
>  	 * @see enum rte_mtr_stats_type
>  	 */
> @@ -462,6 +441,94 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
>  	uint32_t meter_profile_id,
>  	struct rte_mtr_error *error);
> 
> +/**
> + * Policy id 0 is default policy.

I suggest you do not redundantly specify the value of the default policy ID in the comment. Replace by "Default policy ID."

> + * Action per color as below:
> + * green - no action, yellow - no action, red - drop

This does not make sense to me as the default policy. The default policy should be "no change", i.e. green -> green (no change), yellow -> yellow (no change), red -> red (no change).

I suggest we avoid the "no action" statement, as it might be confusing.

> + * It can be used without creating it by the rte_mtr_meter_policy_add
> function.
> + */
> +#define RTE_MTR_DEFAULT_POLICY_ID 0
> +
> +/**
> + * Check whether a meter policy can be created on a given port.
> + *
> + * The meter policy is validated for correctness and
> + * whether it could be accepted by the device given sufficient resources.
> + * The policy is checked against the current capability information
> + * meter_policy_n_max configuration.
> + * The policy may also optionally be validated against existing
> + * device policy resources.
> + * This function has no effect on the target device.
> + *
> + * @param[in] port_id
> + *   The port identifier of the Ethernet device.
> + * @param[in] policy_id
> + *   Policy identifier for the new meter policy.
> + * @param[in] actions
> + *   Associated action list per color.
> + *   list NULL is legal and means no special action.
> + *   (list terminated by the END action).
> + * @param[out] error
> + *   Error details. Filled in only on error, when not NULL.
> + *
> + * @return
> + *   0 on success, non-zero error code otherwise.
> + *
> + */
> +__rte_experimental
> +int
> +rte_mtr_meter_policy_validate(uint16_t port_id,
> +	uint32_t policy_id,
> +	const struct rte_flow_action *actions[RTE_COLORS],
> +	struct rte_mtr_error *error);
> +

This entire proposal is about defining a meter policy, yet there is no structure defining what exactly the meter policy is!

Please let's use the following structure for the policy parameters for the policy validate/add/delete API functions:

struct rte_mtr_meter_policy_params {
	struct rte_flow_action *actions[RTE_COLORS];
};

This would also give us the chance to document the policy concept in detail once and not repeat the same explanation over and over. For example, we need to explain clearly that actions[i] potentially represents a chain of rte_flow actions terminated by the END action, exactly as specified by the rte_flow API for the flow definition, and not just a single action. This is very important and yet not entirely clear from the above description.

This would also give us the chance to add more attributes to the meter policy later, if/when needed, with minimal impact to the API.

> +/**
> + * Meter policy add
> + *
> + * Create a new meter policy. The new policy
> + * is used to create single or multiple MTR objects.
> + *
> + * @param[in] port_id
> + *   The port identifier of the Ethernet device.
> + * @param[in] policy_id
> + *   Policy identifier for the new meter policy.
> + * @param[in] actions
> + *   Associated actions per color.
> + *   list NULL is legal and means no special action.
> + *   (list terminated by the END action).
> + * @param[out] error
> + *   Error details. Filled in only on error, when not NULL.
> + * @return
> + *   0 on success, non-zero error code otherwise.
> + */
> +__rte_experimental
> +int
> +rte_mtr_meter_policy_add(uint16_t port_id,
> +	uint32_t policy_id,
> +	const struct rte_flow_action *actions[RTE_COLORS],
> +	struct rte_mtr_error *error);
> +
> +/**
> + * Meter policy delete
> + *
> + * Delete an existing meter policy. This operation fails when there is
> + * currently at least one user (i.e. MTR object) of this policy.
> + *
> + * @param[in] port_id
> + *   The port identifier of the Ethernet device.
> + * @param[in] policy_id
> + *   Policy identifier.
> + * @param[out] error
> + *   Error details. Filled in only on error, when not NULL.
> + * @return
> + *   0 on success, non-zero error code otherwise.
> + */
> +__rte_experimental
> +int
> +rte_mtr_meter_policy_delete(uint16_t port_id,
> +	uint32_t policy_id,
> +	struct rte_mtr_error *error);
> +
>  /**
>   * MTR object create
>   *
> @@ -587,18 +654,14 @@ rte_mtr_meter_profile_update(uint16_t port_id,
>  	struct rte_mtr_error *error);
> 
>  /**
> - * MTR object DSCP table update
> + * MTR object meter policy update
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
>   * @param[in] mtr_id
>   *   MTR object ID. Needs to be valid.
> - * @param[in] dscp_table
> - *   When non-NULL: it points to a pre-allocated and pre-populated table
> with
> - *   exactly 64 elements providing the input color for each value of the
> - *   IPv4/IPv6 Differentiated Services Code Point (DSCP) input packet field.
> - *   When NULL: it is equivalent to setting this parameter to an “all-
> green”
> - *   populated table (i.e. table with all the 64 elements set to green color).
> + * @param[in] meter_policy_id
> + *   Meter policy ID for the current MTR object. Needs to be valid.
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> @@ -606,26 +669,24 @@ rte_mtr_meter_profile_update(uint16_t port_id,
>   */
>  __rte_experimental
>  int
> -rte_mtr_meter_dscp_table_update(uint16_t port_id,
> +rte_mtr_meter_policy_update(uint16_t port_id,
>  	uint32_t mtr_id,
> -	enum rte_color *dscp_table,
> +	uint32_t meter_policy_id,
>  	struct rte_mtr_error *error);
> 
>  /**
> - * MTR object policer actions update
> + * MTR object DSCP table update
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
>   * @param[in] mtr_id
>   *   MTR object ID. Needs to be valid.
> - * @param[in] action_mask
> - *   Bit mask indicating which policer actions need to be updated. One or
> more
> - *   policer actions can be updated in a single function invocation. To update
> - *   the policer action associated with color C, bit (1 << C) needs to be set in
> - *   *action_mask* and element at position C in the *actions* array needs to
> be
> - *   valid.
> - * @param[in] actions
> - *   Pre-allocated and pre-populated array of policer actions.
> + * @param[in] dscp_table
> + *   When non-NULL: it points to a pre-allocated and pre-populated table
> with
> + *   exactly 64 elements providing the input color for each value of the
> + *   IPv4/IPv6 Differentiated Services Code Point (DSCP) input packet field.
> + *   When NULL: it is equivalent to setting this parameter to an “all-
> green”
> + *   populated table (i.e. table with all the 64 elements set to green color).
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> @@ -633,10 +694,9 @@ rte_mtr_meter_dscp_table_update(uint16_t
> port_id,
>   */
>  __rte_experimental
>  int
> -rte_mtr_policer_actions_update(uint16_t port_id,
> +rte_mtr_meter_dscp_table_update(uint16_t port_id,
>  	uint32_t mtr_id,
> -	uint32_t action_mask,
> -	enum rte_mtr_policer_action *actions,
> +	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error);
> 
>  /**
> diff --git a/lib/librte_ethdev/rte_mtr_driver.h
> b/lib/librte_ethdev/rte_mtr_driver.h
> index a0ddc2b5f4..1ad8fb4c40 100644
> --- a/lib/librte_ethdev/rte_mtr_driver.h
> +++ b/lib/librte_ethdev/rte_mtr_driver.h
> @@ -41,6 +41,23 @@ typedef int (*rte_mtr_meter_profile_delete_t)(struct
> rte_eth_dev *dev,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR meter profile delete */
> 
> +typedef int (*rte_mtr_meter_policy_validate_t)(struct rte_eth_dev *dev,
> +	uint32_t policy_id,
> +	const struct rte_flow_action *actions[RTE_COLORS],
> +	struct rte_mtr_error *error);
> +/**< @internal MTR meter policy validate */
> +
> +typedef int (*rte_mtr_meter_policy_add_t)(struct rte_eth_dev *dev,
> +	uint32_t policy_id,
> +	const struct rte_flow_action *actions[RTE_COLORS],
> +	struct rte_mtr_error *error);
> +/**< @internal MTR meter policy add */
> +
> +typedef int (*rte_mtr_meter_policy_delete_t)(struct rte_eth_dev *dev,
> +	uint32_t policy_id,
> +	struct rte_mtr_error *error);
> +/**< @internal MTR meter policy delete */
> +
>  typedef int (*rte_mtr_create_t)(struct rte_eth_dev *dev,
>  	uint32_t mtr_id,
>  	struct rte_mtr_params *params,
> @@ -69,18 +86,17 @@ typedef int
> (*rte_mtr_meter_profile_update_t)(struct rte_eth_dev *dev,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object meter profile update */
> 
> -typedef int (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev
> *dev,
> +typedef int (*rte_mtr_meter_policy_update_t)(struct rte_eth_dev *dev,
>  	uint32_t mtr_id,
> -	enum rte_color *dscp_table,
> +	uint32_t meter_policy_id,
>  	struct rte_mtr_error *error);
> -/**< @internal MTR object meter DSCP table update */
> +/**< @internal MTR object meter policy update */
> 
> -typedef int (*rte_mtr_policer_actions_update_t)(struct rte_eth_dev *dev,
> +typedef int (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev
> *dev,
>  	uint32_t mtr_id,
> -	uint32_t action_mask,
> -	enum rte_mtr_policer_action *actions,
> +	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error);
> -/**< @internal MTR object policer action update*/
> +/**< @internal MTR object meter DSCP table update */
> 
>  typedef int (*rte_mtr_stats_update_t)(struct rte_eth_dev *dev,
>  	uint32_t mtr_id,
> @@ -124,14 +140,23 @@ struct rte_mtr_ops {
>  	/** MTR object meter DSCP table update */
>  	rte_mtr_meter_dscp_table_update_t meter_dscp_table_update;
> 
> -	/** MTR object policer action update */
> -	rte_mtr_policer_actions_update_t policer_actions_update;
> -
>  	/** MTR object enabled stats update */
>  	rte_mtr_stats_update_t stats_update;
> 
>  	/** MTR object stats read */
>  	rte_mtr_stats_read_t stats_read;
> +
> +	/** MTR meter policy validate */
> +	rte_mtr_meter_policy_validate_t meter_policy_validate;
> +
> +	/** MTR meter policy add */
> +	rte_mtr_meter_policy_add_t meter_policy_add;
> +
> +	/** MTR meter policy delete */
> +	rte_mtr_meter_policy_delete_t meter_policy_delete;
> +
> +	/** MTR object meter policy update */
> +	rte_mtr_meter_policy_update_t meter_policy_update;
>  };
> 
>  /**
> --
> 2.21.0

Regards,
Cristian
  

Hi Cristian

Thank you for your important review!
I agree with all your comments except one, please see inline.

From: Dumitrescu, Cristian
> Hi Li and Matan,
> 
> Thank you for your proposal, some comments below.
> 
> I am also adding Jerin and Hemant to this thread, as they also participated in
> the definition of the rte_mtr API in 2017. Also Ajit expressed some interest in a
> previous email.
> 
> > -----Original Message-----
> > From: Li Zhang <lizh@nvidia.com>
> > Sent: Thursday, March 18, 2021 8:58 AM
> > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> >
> > Currently, the flow meter policy does not support multiple actions per
> > color; also the allowed action types per color are very limited.
> > In addition, the policy cannot be pre-defined.
> >
> > Due to the growing in flow actions offload abilities there is a
> > potential for the user to use variety of actions per color differently.
> > This new meter policy API comes to allow this potential in the most
> > ethdev common way using rte_flow action definition.
> > A list of rte_flow actions will be provided by the user per color in
> > order to create a meter policy.
> > In addition, the API forces to pre-define the policy before the meters
> > creation in order to allow sharing of single policy with multiple
> > meters efficiently.
> >
> > meter_policy_id is added into struct rte_mtr_params.
> > So that it can get the policy during the meters creation.
> >
> > Policy id 0 is default policy. Action per color as below:
> > green - no action, yellow - no action, red - drop
> >
> > Allow coloring the packet using a new rte_flow_action_color as could
> > be done by the old policy API,
> >
> 
> The proposal essentially is to define the meter policy based on rte_flow actions
> rather than a reduced action set defined specifically just for meter object. This
> makes sense to me.
> 
> > The next API function were added:
> > - rte_mtr_meter_policy_add
> > - rte_mtr_meter_policy_delete
> > - rte_mtr_meter_policy_update
> > - rte_mtr_meter_policy_validate
> > The next struct was changed:
> > - rte_mtr_params
> > - rte_mtr_capabilities
> > The next API was deleted:
> > - rte_mtr_policer_actions_update
> >
> > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > ---
> >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> >  4 files changed, 210 insertions(+), 74 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -31,6 +31,7 @@
> >  #include <rte_ecpri.h>
> >  #include <rte_mbuf.h>
> >  #include <rte_mbuf_dyn.h>
> > +#include <rte_meter.h>
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> >        * See struct rte_flow_action_modify_field.
> >        */
> >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > +
> > +     /**
> > +      * Color the packet to reflect the meter color result.
> > +      *
> > +      * See struct rte_flow_action_color.
> > +      */
> > +     RTE_FLOW_ACTION_TYPE_COlOR,
> 
> Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> 
> >  };
> >
> >  /**
> > @@ -2828,6 +2836,16 @@ struct rte_flow_action_set_dscp {
> >   */
> >  struct rte_flow_shared_action;
> >
> > +/**
> > + * RTE_FLOW_ACTION_TYPE_COLOR
> > + *
> > + * The meter color should be set in the packet meta-data
> > + * (i.e. struct rte_mbuf::sched::color).
> > + */
> > +struct rte_flow_action_color {
> > +     enum rte_color color; /**< Green/Yellow/Red. */
> 
> I would avoid expanding the list of colors in the comment, as we might have to
> support more than 3 colors in the future. I would simply write: "Packet color."
> 
> > +};
> > +
> >  /**
> >   * Field IDs for MODIFY_FIELD action.
> >   */
> > diff --git a/lib/librte_ethdev/rte_mtr.c b/lib/librte_ethdev/rte_mtr.c
> > index 3073ac03f2..fccec3760b 100644
> > --- a/lib/librte_ethdev/rte_mtr.c
> > +++ b/lib/librte_ethdev/rte_mtr.c
> > @@ -91,6 +91,40 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
> >               meter_profile_id, error);  }
> >
> > +/* MTR meter policy validate */
> > +int
> > +rte_mtr_meter_policy_validate(uint16_t port_id,
> > +     const struct rte_flow_action *actions[RTE_COLORS],
> > +     struct rte_mtr_error *error)
> > +{
> > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > +     return RTE_MTR_FUNC(port_id, meter_policy_validate)(dev,
> > +             actions, error);
> > +}
> > +
> > +/* MTR meter policy add */
> > +int
> > +rte_mtr_meter_policy_add(uint16_t port_id,
> > +     uint32_t policy_id,
> > +     const struct rte_flow_action *actions[RTE_COLORS],
> > +     struct rte_mtr_error *error)
> > +{
> > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > +     return RTE_MTR_FUNC(port_id, meter_policy_add)(dev,
> > +             policy_id, actions, error); }
> > +
> > +/** MTR meter policy delete */
> > +int
> > +rte_mtr_meter_policy_delete(uint16_t port_id,
> > +     uint32_t policy_id,
> > +     struct rte_mtr_error *error)
> > +{
> > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > +     return RTE_MTR_FUNC(port_id, meter_policy_delete)(dev,
> > +             policy_id, error);
> > +}
> > +
> >  /** MTR object create */
> >  int
> >  rte_mtr_create(uint16_t port_id,
> > @@ -149,29 +183,28 @@ rte_mtr_meter_profile_update(uint16_t port_id,
> >               mtr_id, meter_profile_id, error);  }
> >
> > -/** MTR object meter DSCP table update */
> > +/** MTR object meter policy update */
> >  int
> > -rte_mtr_meter_dscp_table_update(uint16_t port_id,
> > +rte_mtr_meter_policy_update(uint16_t port_id,
> >       uint32_t mtr_id,
> > -     enum rte_color *dscp_table,
> > +     uint32_t meter_policy_id,
> >       struct rte_mtr_error *error)
> >  {
> >       struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > -     return RTE_MTR_FUNC(port_id, meter_dscp_table_update)(dev,
> > -             mtr_id, dscp_table, error);
> > +     return RTE_MTR_FUNC(port_id, meter_policy_update)(dev,
> > +             mtr_id, meter_policy_id, error);
> >  }
> >
> > -/** MTR object policer action update */
> > +/** MTR object meter DSCP table update */
> >  int
> > -rte_mtr_policer_actions_update(uint16_t port_id,
> > +rte_mtr_meter_dscp_table_update(uint16_t port_id,
> >       uint32_t mtr_id,
> > -     uint32_t action_mask,
> > -     enum rte_mtr_policer_action *actions,
> > +     enum rte_color *dscp_table,
> >       struct rte_mtr_error *error)
> >  {
> >       struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > -     return RTE_MTR_FUNC(port_id, policer_actions_update)(dev,
> > -             mtr_id, action_mask, actions, error);
> > +     return RTE_MTR_FUNC(port_id, meter_dscp_table_update)(dev,
> > +             mtr_id, dscp_table, error);
> >  }
> >
> >  /** MTR object enabled stats update */ diff --git
> > a/lib/librte_ethdev/rte_mtr.h b/lib/librte_ethdev/rte_mtr.h index
> > 916a09c5c3..07961f2777 100644
> > --- a/lib/librte_ethdev/rte_mtr.h
> > +++ b/lib/librte_ethdev/rte_mtr.h
> > @@ -49,6 +49,7 @@
> >  #include <rte_compat.h>
> >  #include <rte_common.h>
> >  #include <rte_meter.h>
> > +#include <rte_flow.h>
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> > @@ -174,23 +175,6 @@ struct rte_mtr_meter_profile {
> >       };
> >  };
> >
> > -/**
> > - * Policer actions
> > - */
> > -enum rte_mtr_policer_action {
> > -     /** Recolor the packet as green. */
> > -     MTR_POLICER_ACTION_COLOR_GREEN = 0,
> > -
> > -     /** Recolor the packet as yellow. */
> > -     MTR_POLICER_ACTION_COLOR_YELLOW,
> > -
> > -     /** Recolor the packet as red. */
> > -     MTR_POLICER_ACTION_COLOR_RED,
> > -
> > -     /** Drop the packet. */
> > -     MTR_POLICER_ACTION_DROP,
> > -};
> > -
> >  /**
> >   * Parameters for each traffic metering & policing object
> >   *
> > @@ -232,13 +216,13 @@ struct rte_mtr_params {
> >        */
> >       int meter_enable;
> >
> > -     /** Policer actions (per meter output color). */
> > -     enum rte_mtr_policer_action action[RTE_COLORS];
> > -
> >       /** Set of stats counters to be enabled.
> >        * @see enum rte_mtr_stats_type
> >        */
> >       uint64_t stats_mask;
> > +
> > +     /** Meter policy ID. */
> > +     uint32_t meter_policy_id;
> >  };
> >
> >  /**
> > @@ -324,6 +308,13 @@ struct rte_mtr_capabilities {
> >        */
> >       uint64_t meter_rate_max;
> >
> > +     /**
> > +      * Maximum number of policy objects that can have.
> > +      * The value of 0 is invalid. Policy must be supported for meter.
> > +      * The maximum value is *n_max*.
> > +      */
> > +     uint64_t meter_policy_n_max;
> > +
> >       /**
> >        * When non-zero, it indicates that color aware mode is
> > supported for
> >        * the srTCM RFC 2697 metering algorithm.
> > @@ -342,18 +333,6 @@ struct rte_mtr_capabilities {
> >        */
> >       int color_aware_trtcm_rfc4115_supported;
> >
> > -     /** When non-zero, it indicates that the policer packet recolor
> > actions
> > -      * are supported.
> > -      * @see enum rte_mtr_policer_action
> > -      */
> > -     int policer_action_recolor_supported;
> > -
> > -     /** When non-zero, it indicates that the policer packet drop action is
> > -      * supported.
> > -      * @see enum rte_mtr_policer_action
> > -      */
> > -     int policer_action_drop_supported;
> > -
> >       /** Set of supported statistics counter types.
> >        * @see enum rte_mtr_stats_type
> >        */
> > @@ -462,6 +441,94 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
> >       uint32_t meter_profile_id,
> >       struct rte_mtr_error *error);
> >
> > +/**
> > + * Policy id 0 is default policy.
> 
> I suggest you do not redundantly specify the value of the default policy ID in the
> comment. Replace by "Default policy ID."
> 
> > + * Action per color as below:
> > + * green - no action, yellow - no action, red - drop
> 
> This does not make sense to me as the default policy. The default policy should
> be "no change", i.e. green -> green (no change), yellow -> yellow (no change),
> red -> red (no change).

Can you explain why it doesn't make sense to you?

Meter with "no change" for all colors has no effect on the packets so it is redundant action which just costs performance and resources - probably never be used.

The most common usage for meter is to drop all the packets come above the defined rate limit - so it makes sense to take this behavior as default.


> I suggest we avoid the "no action" statement, as it might be confusing.

Maybe "do nothing" is better?


> > + * It can be used without creating it by the rte_mtr_meter_policy_add
> > function.
> > + */
> > +#define RTE_MTR_DEFAULT_POLICY_ID 0
> > +
> > +/**
> > + * Check whether a meter policy can be created on a given port.
> > + *
> > + * The meter policy is validated for correctness and
> > + * whether it could be accepted by the device given sufficient resources.
> > + * The policy is checked against the current capability information
> > + * meter_policy_n_max configuration.
> > + * The policy may also optionally be validated against existing
> > + * device policy resources.
> > + * This function has no effect on the target device.
> > + *
> > + * @param[in] port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param[in] policy_id
> > + *   Policy identifier for the new meter policy.
> > + * @param[in] actions
> > + *   Associated action list per color.
> > + *   list NULL is legal and means no special action.
> > + *   (list terminated by the END action).
> > + * @param[out] error
> > + *   Error details. Filled in only on error, when not NULL.
> > + *
> > + * @return
> > + *   0 on success, non-zero error code otherwise.
> > + *
> > + */
> > +__rte_experimental
> > +int
> > +rte_mtr_meter_policy_validate(uint16_t port_id,
> > +     uint32_t policy_id,
> > +     const struct rte_flow_action *actions[RTE_COLORS],
> > +     struct rte_mtr_error *error);
> > +
> 
> This entire proposal is about defining a meter policy, yet there is no structure
> defining what exactly the meter policy is!
> 
> Please let's use the following structure for the policy parameters for the policy
> validate/add/delete API functions:
> 
> struct rte_mtr_meter_policy_params {
>         struct rte_flow_action *actions[RTE_COLORS]; };
> 
> This would also give us the chance to document the policy concept in detail
> once and not repeat the same explanation over and over. For example, we
> need to explain clearly that actions[i] potentially represents a chain of rte_flow
> actions terminated by the END action, exactly as specified by the rte_flow API
> for the flow definition, and not just a single action. This is very important and
> yet not entirely clear from the above description.
> 
> This would also give us the chance to add more attributes to the meter policy
> later, if/when needed, with minimal impact to the API.
> 
> > +/**
> > + * Meter policy add
> > + *
> > + * Create a new meter policy. The new policy
> > + * is used to create single or multiple MTR objects.
> > + *
> > + * @param[in] port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param[in] policy_id
> > + *   Policy identifier for the new meter policy.
> > + * @param[in] actions
> > + *   Associated actions per color.
> > + *   list NULL is legal and means no special action.
> > + *   (list terminated by the END action).
> > + * @param[out] error
> > + *   Error details. Filled in only on error, when not NULL.
> > + * @return
> > + *   0 on success, non-zero error code otherwise.
> > + */
> > +__rte_experimental
> > +int
> > +rte_mtr_meter_policy_add(uint16_t port_id,
> > +     uint32_t policy_id,
> > +     const struct rte_flow_action *actions[RTE_COLORS],
> > +     struct rte_mtr_error *error);
> > +
> > +/**
> > + * Meter policy delete
> > + *
> > + * Delete an existing meter policy. This operation fails when there
> > +is
> > + * currently at least one user (i.e. MTR object) of this policy.
> > + *
> > + * @param[in] port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param[in] policy_id
> > + *   Policy identifier.
> > + * @param[out] error
> > + *   Error details. Filled in only on error, when not NULL.
> > + * @return
> > + *   0 on success, non-zero error code otherwise.
> > + */
> > +__rte_experimental
> > +int
> > +rte_mtr_meter_policy_delete(uint16_t port_id,
> > +     uint32_t policy_id,
> > +     struct rte_mtr_error *error);
> > +
> >  /**
> >   * MTR object create
> >   *
> > @@ -587,18 +654,14 @@ rte_mtr_meter_profile_update(uint16_t port_id,
> >       struct rte_mtr_error *error);
> >
> >  /**
> > - * MTR object DSCP table update
> > + * MTR object meter policy update
> >   *
> >   * @param[in] port_id
> >   *   The port identifier of the Ethernet device.
> >   * @param[in] mtr_id
> >   *   MTR object ID. Needs to be valid.
> > - * @param[in] dscp_table
> > - *   When non-NULL: it points to a pre-allocated and pre-populated table
> > with
> > - *   exactly 64 elements providing the input color for each value of the
> > - *   IPv4/IPv6 Differentiated Services Code Point (DSCP) input packet field.
> > - *   When NULL: it is equivalent to setting this parameter to an “all-
> > green”
> > - *   populated table (i.e. table with all the 64 elements set to green color).
> > + * @param[in] meter_policy_id
> > + *   Meter policy ID for the current MTR object. Needs to be valid.
> >   * @param[out] error
> >   *   Error details. Filled in only on error, when not NULL.
> >   * @return
> > @@ -606,26 +669,24 @@ rte_mtr_meter_profile_update(uint16_t port_id,
> >   */
> >  __rte_experimental
> >  int
> > -rte_mtr_meter_dscp_table_update(uint16_t port_id,
> > +rte_mtr_meter_policy_update(uint16_t port_id,
> >       uint32_t mtr_id,
> > -     enum rte_color *dscp_table,
> > +     uint32_t meter_policy_id,
> >       struct rte_mtr_error *error);
> >
> >  /**
> > - * MTR object policer actions update
> > + * MTR object DSCP table update
> >   *
> >   * @param[in] port_id
> >   *   The port identifier of the Ethernet device.
> >   * @param[in] mtr_id
> >   *   MTR object ID. Needs to be valid.
> > - * @param[in] action_mask
> > - *   Bit mask indicating which policer actions need to be updated. One or
> > more
> > - *   policer actions can be updated in a single function invocation. To update
> > - *   the policer action associated with color C, bit (1 << C) needs to be set in
> > - *   *action_mask* and element at position C in the *actions* array needs to
> > be
> > - *   valid.
> > - * @param[in] actions
> > - *   Pre-allocated and pre-populated array of policer actions.
> > + * @param[in] dscp_table
> > + *   When non-NULL: it points to a pre-allocated and pre-populated table
> > with
> > + *   exactly 64 elements providing the input color for each value of the
> > + *   IPv4/IPv6 Differentiated Services Code Point (DSCP) input packet field.
> > + *   When NULL: it is equivalent to setting this parameter to an “all-
> > green”
> > + *   populated table (i.e. table with all the 64 elements set to green color).
> >   * @param[out] error
> >   *   Error details. Filled in only on error, when not NULL.
> >   * @return
> > @@ -633,10 +694,9 @@ rte_mtr_meter_dscp_table_update(uint16_t
> > port_id,
> >   */
> >  __rte_experimental
> >  int
> > -rte_mtr_policer_actions_update(uint16_t port_id,
> > +rte_mtr_meter_dscp_table_update(uint16_t port_id,
> >       uint32_t mtr_id,
> > -     uint32_t action_mask,
> > -     enum rte_mtr_policer_action *actions,
> > +     enum rte_color *dscp_table,
> >       struct rte_mtr_error *error);
> >
> >  /**
> > diff --git a/lib/librte_ethdev/rte_mtr_driver.h
> > b/lib/librte_ethdev/rte_mtr_driver.h
> > index a0ddc2b5f4..1ad8fb4c40 100644
> > --- a/lib/librte_ethdev/rte_mtr_driver.h
> > +++ b/lib/librte_ethdev/rte_mtr_driver.h
> > @@ -41,6 +41,23 @@ typedef int
> > (*rte_mtr_meter_profile_delete_t)(struct
> > rte_eth_dev *dev,
> >       struct rte_mtr_error *error);
> >  /**< @internal MTR meter profile delete */
> >
> > +typedef int (*rte_mtr_meter_policy_validate_t)(struct rte_eth_dev *dev,
> > +     uint32_t policy_id,
> > +     const struct rte_flow_action *actions[RTE_COLORS],
> > +     struct rte_mtr_error *error);
> > +/**< @internal MTR meter policy validate */
> > +
> > +typedef int (*rte_mtr_meter_policy_add_t)(struct rte_eth_dev *dev,
> > +     uint32_t policy_id,
> > +     const struct rte_flow_action *actions[RTE_COLORS],
> > +     struct rte_mtr_error *error);
> > +/**< @internal MTR meter policy add */
> > +
> > +typedef int (*rte_mtr_meter_policy_delete_t)(struct rte_eth_dev *dev,
> > +     uint32_t policy_id,
> > +     struct rte_mtr_error *error);
> > +/**< @internal MTR meter policy delete */
> > +
> >  typedef int (*rte_mtr_create_t)(struct rte_eth_dev *dev,
> >       uint32_t mtr_id,
> >       struct rte_mtr_params *params,
> > @@ -69,18 +86,17 @@ typedef int
> > (*rte_mtr_meter_profile_update_t)(struct rte_eth_dev *dev,
> >       struct rte_mtr_error *error);
> >  /**< @internal MTR object meter profile update */
> >
> > -typedef int (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev
> > *dev,
> > +typedef int (*rte_mtr_meter_policy_update_t)(struct rte_eth_dev *dev,
> >       uint32_t mtr_id,
> > -     enum rte_color *dscp_table,
> > +     uint32_t meter_policy_id,
> >       struct rte_mtr_error *error);
> > -/**< @internal MTR object meter DSCP table update */
> > +/**< @internal MTR object meter policy update */
> >
> > -typedef int (*rte_mtr_policer_actions_update_t)(struct rte_eth_dev
> > *dev,
> > +typedef int (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev
> > *dev,
> >       uint32_t mtr_id,
> > -     uint32_t action_mask,
> > -     enum rte_mtr_policer_action *actions,
> > +     enum rte_color *dscp_table,
> >       struct rte_mtr_error *error);
> > -/**< @internal MTR object policer action update*/
> > +/**< @internal MTR object meter DSCP table update */
> >
> >  typedef int (*rte_mtr_stats_update_t)(struct rte_eth_dev *dev,
> >       uint32_t mtr_id,
> > @@ -124,14 +140,23 @@ struct rte_mtr_ops {
> >       /** MTR object meter DSCP table update */
> >       rte_mtr_meter_dscp_table_update_t meter_dscp_table_update;
> >
> > -     /** MTR object policer action update */
> > -     rte_mtr_policer_actions_update_t policer_actions_update;
> > -
> >       /** MTR object enabled stats update */
> >       rte_mtr_stats_update_t stats_update;
> >
> >       /** MTR object stats read */
> >       rte_mtr_stats_read_t stats_read;
> > +
> > +     /** MTR meter policy validate */
> > +     rte_mtr_meter_policy_validate_t meter_policy_validate;
> > +
> > +     /** MTR meter policy add */
> > +     rte_mtr_meter_policy_add_t meter_policy_add;
> > +
> > +     /** MTR meter policy delete */
> > +     rte_mtr_meter_policy_delete_t meter_policy_delete;
> > +
> > +     /** MTR object meter policy update */
> > +     rte_mtr_meter_policy_update_t meter_policy_update;
> >  };
> >
> >  /**
> > --
> > 2.21.0
> 
> Regards,
> Cristian
Thanks!
  

Hi All,

> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> Hi Cristian
> 
> Thank you for your important review!
> I agree with all your comments except one, please see inline.
> 
> From: Dumitrescu, Cristian
> > Hi Li and Matan,
> >
> > Thank you for your proposal, some comments below.
> >
> > I am also adding Jerin and Hemant to this thread, as they also participated in
> > the definition of the rte_mtr API in 2017. Also Ajit expressed some interest in
> a
> > previous email.
> >
> > > -----Original Message-----
> > > From: Li Zhang <lizh@nvidia.com>
> > > Sent: Thursday, March 18, 2021 8:58 AM
> > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com>
> > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > >
> > > Currently, the flow meter policy does not support multiple actions per
> > > color; also the allowed action types per color are very limited.
> > > In addition, the policy cannot be pre-defined.
> > >
> > > Due to the growing in flow actions offload abilities there is a
> > > potential for the user to use variety of actions per color differently.
> > > This new meter policy API comes to allow this potential in the most
> > > ethdev common way using rte_flow action definition.
> > > A list of rte_flow actions will be provided by the user per color in
> > > order to create a meter policy.
> > > In addition, the API forces to pre-define the policy before the meters
> > > creation in order to allow sharing of single policy with multiple
> > > meters efficiently.
> > >
> > > meter_policy_id is added into struct rte_mtr_params.
> > > So that it can get the policy during the meters creation.
> > >
> > > Policy id 0 is default policy. Action per color as below:
> > > green - no action, yellow - no action, red - drop
> > >
> > > Allow coloring the packet using a new rte_flow_action_color as could
> > > be done by the old policy API,
> > >
> >
> > The proposal essentially is to define the meter policy based on rte_flow
> actions
> > rather than a reduced action set defined specifically just for meter object.
> This
> > makes sense to me.
> >
> > > The next API function were added:
> > > - rte_mtr_meter_policy_add
> > > - rte_mtr_meter_policy_delete
> > > - rte_mtr_meter_policy_update
> > > - rte_mtr_meter_policy_validate
> > > The next struct was changed:
> > > - rte_mtr_params
> > > - rte_mtr_capabilities
> > > The next API was deleted:
> > > - rte_mtr_policer_actions_update
> > >
> > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > ---
> > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -31,6 +31,7 @@
> > >  #include <rte_ecpri.h>
> > >  #include <rte_mbuf.h>
> > >  #include <rte_mbuf_dyn.h>
> > > +#include <rte_meter.h>
> > >
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > >        * See struct rte_flow_action_modify_field.
> > >        */
> > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > +
> > > +     /**
> > > +      * Color the packet to reflect the meter color result.
> > > +      *
> > > +      * See struct rte_flow_action_color.
> > > +      */
> > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> >
> > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> >

Why do we need this action?
if it is to save the color it should be done by using mark/metadata
Or by the action of meter. For example you can see RTE_FLOW_ACTION_TYPE_SECURITY
Which if exist saves the session id to a dedicated mbuf field.

> > >  };
> > >
> > >  /**

[Snip]

> > I suggest you do not redundantly specify the value of the default policy ID in
> the
> > comment. Replace by "Default policy ID."
> >
> > > + * Action per color as below:
> > > + * green - no action, yellow - no action, red - drop
> >
> > This does not make sense to me as the default policy. The default policy
> should
> > be "no change", i.e. green -> green (no change), yellow -> yellow (no change),
> > red -> red (no change).
> 
> Can you explain why it doesn't make sense to you?
> 
> Meter with "no change" for all colors has no effect on the packets so it is
> redundant action which just costs performance and resources - probably never
> be used.
> 
> The most common usage for meter is to drop all the packets come above the
> defined rate limit - so it makes sense to take this behavior as default.
> 
> 
> > I suggest we avoid the "no action" statement, as it might be confusing.
> 
> Maybe "do nothing" is better?
> 
Maybe passthrough? Or in rte_flow passthru
 

> > > + * It can be used without creating it by the rte_mtr_meter_policy_add
> > > function.
> > > + */


Best,
Ori
  

On Thu, Mar 18, 2021 at 2:28 PM Li Zhang <lizh@nvidia.com> wrote:
>
> Currently, the flow meter policy does not support multiple actions
> per color; also the allowed action types per color are very limited.
> In addition, the policy cannot be pre-defined.
>
> Due to the growing in flow actions offload abilities there is a potential
> for the user to use variety of actions per color differently.
> This new meter policy API comes to allow this potential in the most ethdev
> common way using rte_flow action definition.
> A list of rte_flow actions will be provided by the user per color
> in order to create a meter policy.
> In addition, the API forces to pre-define the policy before
> the meters creation in order to allow sharing of single policy
> with multiple meters efficiently.
>
> meter_policy_id is added into struct rte_mtr_params.
> So that it can get the policy during the meters creation.
>
> Policy id 0 is default policy. Action per color as below:
> green - no action, yellow - no action, red - drop
>
> Allow coloring the packet using a new rte_flow_action_color
> as could be done by the old policy API,
>
> The next API function were added:
> - rte_mtr_meter_policy_add
> - rte_mtr_meter_policy_delete
> - rte_mtr_meter_policy_update
> - rte_mtr_meter_policy_validate
> The next struct was changed:
> - rte_mtr_params
> - rte_mtr_capabilities
> The next API was deleted:
> - rte_mtr_policer_actions_update
>
> Signed-off-by: Li Zhang <lizh@nvidia.com>
> ---
>  lib/librte_ethdev/rte_flow.h       |  18 ++++
>  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
>  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
>  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
>  4 files changed, 210 insertions(+), 74 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 669e677e91..5f38aa7fa4 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -31,6 +31,7 @@
>  #include <rte_ecpri.h>
>  #include <rte_mbuf.h>
>  #include <rte_mbuf_dyn.h>
> +#include <rte_meter.h>
>
>  #ifdef __cplusplus
>  extern "C" {
> @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
>          * See struct rte_flow_action_modify_field.
>          */
>         RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> +
> +       /**
> +        * Color the packet to reflect the meter color result.
> +        *
> +        * See struct rte_flow_action_color.
> +        */
> +       RTE_FLOW_ACTION_TYPE_COlOR,

Based on my understanding of this API,
1) Application creates the policy
2) Attachs the policy ID to meter object in params
If so, Why we need this new action?

>  };
>
>  /**
> @@ -2828,6 +2836,16 @@ struct rte_flow_action_set_dscp {
>   */
>  struct rte_flow_shared_action;
>
> +/**
> + * Meter policy add
> + *
> + * Create a new meter policy. The new policy
> + * is used to create single or multiple MTR objects.
> + *
> + * @param[in] port_id
> + *   The port identifier of the Ethernet device.
> + * @param[in] policy_id
> + *   Policy identifier for the new meter policy.
> + * @param[in] actions
> + *   Associated actions per color.
> + *   list NULL is legal and means no special action.
> + *   (list terminated by the END action).
> + * @param[out] error
> + *   Error details. Filled in only on error, when not NULL.
> + * @return
> + *   0 on success, non-zero error code otherwise.
> + */
> +__rte_experimental
> +int
> +rte_mtr_meter_policy_add(uint16_t port_id,


_create() may be better here instead of _add() as you have used _delete()

> +       uint32_t policy_id,
> +       const struct rte_flow_action *actions[RTE_COLORS],


1) Does this mean that MLX HW can support any rte_flow actions like,
if packet color is green do RTE_FLOW_ACTION_TYPE_SECURITY etc.


2) Is there any real-world use case other than using normal action
like pass or drop as it is used
in conjunction with meter object?


3) Marvell HW has the following policy actions
a) PASS
b) DROP
c) RED (Random early discard)

Both (a) and (c) are not in enumated as rte_flow_actions.

Should we take rte_flow_action or create meter-specific policy actions?
  

Hi Matan,

> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> Sent: Thursday, March 25, 2021 6:57 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit Khaparde <ajit.khaparde@broadcom.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>
> Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> Hi Cristian
> 
> Thank you for your important review!
> I agree with all your comments except one, please see inline.
> 
> From: Dumitrescu, Cristian
> > Hi Li and Matan,
> >
> > Thank you for your proposal, some comments below.
> >
> > I am also adding Jerin and Hemant to this thread, as they also participated
> in
> > the definition of the rte_mtr API in 2017. Also Ajit expressed some interest
> in a
> > previous email.
> >
> > > -----Original Message-----
> > > From: Li Zhang <lizh@nvidia.com>
> > > Sent: Thursday, March 18, 2021 8:58 AM
> > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Andrew
> > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com>
> > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > >
> > > Currently, the flow meter policy does not support multiple actions per
> > > color; also the allowed action types per color are very limited.
> > > In addition, the policy cannot be pre-defined.
> > >
> > > Due to the growing in flow actions offload abilities there is a
> > > potential for the user to use variety of actions per color differently.
> > > This new meter policy API comes to allow this potential in the most
> > > ethdev common way using rte_flow action definition.
> > > A list of rte_flow actions will be provided by the user per color in
> > > order to create a meter policy.
> > > In addition, the API forces to pre-define the policy before the meters
> > > creation in order to allow sharing of single policy with multiple
> > > meters efficiently.
> > >
> > > meter_policy_id is added into struct rte_mtr_params.
> > > So that it can get the policy during the meters creation.
> > >
> > > Policy id 0 is default policy. Action per color as below:
> > > green - no action, yellow - no action, red - drop
> > >
> > > Allow coloring the packet using a new rte_flow_action_color as could
> > > be done by the old policy API,
> > >
> >
> > The proposal essentially is to define the meter policy based on rte_flow
> actions
> > rather than a reduced action set defined specifically just for meter object.
> This
> > makes sense to me.
> >
> > > The next API function were added:
> > > - rte_mtr_meter_policy_add
> > > - rte_mtr_meter_policy_delete
> > > - rte_mtr_meter_policy_update
> > > - rte_mtr_meter_policy_validate
> > > The next struct was changed:
> > > - rte_mtr_params
> > > - rte_mtr_capabilities
> > > The next API was deleted:
> > > - rte_mtr_policer_actions_update
> > >
> > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > ---
> > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -31,6 +31,7 @@
> > >  #include <rte_ecpri.h>
> > >  #include <rte_mbuf.h>
> > >  #include <rte_mbuf_dyn.h>
> > > +#include <rte_meter.h>
> > >
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > >        * See struct rte_flow_action_modify_field.
> > >        */
> > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > +
> > > +     /**
> > > +      * Color the packet to reflect the meter color result.
> > > +      *
> > > +      * See struct rte_flow_action_color.
> > > +      */
> > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> >
> > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> >
> > >  };
> > >
> > >  /**
> > > @@ -2828,6 +2836,16 @@ struct rte_flow_action_set_dscp {
> > >   */
> > >  struct rte_flow_shared_action;
> > >
> > > +/**
> > > + * RTE_FLOW_ACTION_TYPE_COLOR
> > > + *
> > > + * The meter color should be set in the packet meta-data
> > > + * (i.e. struct rte_mbuf::sched::color).
> > > + */
> > > +struct rte_flow_action_color {
> > > +     enum rte_color color; /**< Green/Yellow/Red. */
> >
> > I would avoid expanding the list of colors in the comment, as we might
> have to
> > support more than 3 colors in the future. I would simply write: "Packet
> color."
> >
> > > +};
> > > +
> > >  /**
> > >   * Field IDs for MODIFY_FIELD action.
> > >   */
> > > diff --git a/lib/librte_ethdev/rte_mtr.c b/lib/librte_ethdev/rte_mtr.c
> > > index 3073ac03f2..fccec3760b 100644
> > > --- a/lib/librte_ethdev/rte_mtr.c
> > > +++ b/lib/librte_ethdev/rte_mtr.c
> > > @@ -91,6 +91,40 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
> > >               meter_profile_id, error);  }
> > >
> > > +/* MTR meter policy validate */
> > > +int
> > > +rte_mtr_meter_policy_validate(uint16_t port_id,
> > > +     const struct rte_flow_action *actions[RTE_COLORS],
> > > +     struct rte_mtr_error *error)
> > > +{
> > > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > +     return RTE_MTR_FUNC(port_id, meter_policy_validate)(dev,
> > > +             actions, error);
> > > +}
> > > +
> > > +/* MTR meter policy add */
> > > +int
> > > +rte_mtr_meter_policy_add(uint16_t port_id,
> > > +     uint32_t policy_id,
> > > +     const struct rte_flow_action *actions[RTE_COLORS],
> > > +     struct rte_mtr_error *error)
> > > +{
> > > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > +     return RTE_MTR_FUNC(port_id, meter_policy_add)(dev,
> > > +             policy_id, actions, error); }
> > > +
> > > +/** MTR meter policy delete */
> > > +int
> > > +rte_mtr_meter_policy_delete(uint16_t port_id,
> > > +     uint32_t policy_id,
> > > +     struct rte_mtr_error *error)
> > > +{
> > > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > +     return RTE_MTR_FUNC(port_id, meter_policy_delete)(dev,
> > > +             policy_id, error);
> > > +}
> > > +
> > >  /** MTR object create */
> > >  int
> > >  rte_mtr_create(uint16_t port_id,
> > > @@ -149,29 +183,28 @@ rte_mtr_meter_profile_update(uint16_t
> port_id,
> > >               mtr_id, meter_profile_id, error);  }
> > >
> > > -/** MTR object meter DSCP table update */
> > > +/** MTR object meter policy update */
> > >  int
> > > -rte_mtr_meter_dscp_table_update(uint16_t port_id,
> > > +rte_mtr_meter_policy_update(uint16_t port_id,
> > >       uint32_t mtr_id,
> > > -     enum rte_color *dscp_table,
> > > +     uint32_t meter_policy_id,
> > >       struct rte_mtr_error *error)
> > >  {
> > >       struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > -     return RTE_MTR_FUNC(port_id, meter_dscp_table_update)(dev,
> > > -             mtr_id, dscp_table, error);
> > > +     return RTE_MTR_FUNC(port_id, meter_policy_update)(dev,
> > > +             mtr_id, meter_policy_id, error);
> > >  }
> > >
> > > -/** MTR object policer action update */
> > > +/** MTR object meter DSCP table update */
> > >  int
> > > -rte_mtr_policer_actions_update(uint16_t port_id,
> > > +rte_mtr_meter_dscp_table_update(uint16_t port_id,
> > >       uint32_t mtr_id,
> > > -     uint32_t action_mask,
> > > -     enum rte_mtr_policer_action *actions,
> > > +     enum rte_color *dscp_table,
> > >       struct rte_mtr_error *error)
> > >  {
> > >       struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > -     return RTE_MTR_FUNC(port_id, policer_actions_update)(dev,
> > > -             mtr_id, action_mask, actions, error);
> > > +     return RTE_MTR_FUNC(port_id, meter_dscp_table_update)(dev,
> > > +             mtr_id, dscp_table, error);
> > >  }
> > >
> > >  /** MTR object enabled stats update */ diff --git
> > > a/lib/librte_ethdev/rte_mtr.h b/lib/librte_ethdev/rte_mtr.h index
> > > 916a09c5c3..07961f2777 100644
> > > --- a/lib/librte_ethdev/rte_mtr.h
> > > +++ b/lib/librte_ethdev/rte_mtr.h
> > > @@ -49,6 +49,7 @@
> > >  #include <rte_compat.h>
> > >  #include <rte_common.h>
> > >  #include <rte_meter.h>
> > > +#include <rte_flow.h>
> > >
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > > @@ -174,23 +175,6 @@ struct rte_mtr_meter_profile {
> > >       };
> > >  };
> > >
> > > -/**
> > > - * Policer actions
> > > - */
> > > -enum rte_mtr_policer_action {
> > > -     /** Recolor the packet as green. */
> > > -     MTR_POLICER_ACTION_COLOR_GREEN = 0,
> > > -
> > > -     /** Recolor the packet as yellow. */
> > > -     MTR_POLICER_ACTION_COLOR_YELLOW,
> > > -
> > > -     /** Recolor the packet as red. */
> > > -     MTR_POLICER_ACTION_COLOR_RED,
> > > -
> > > -     /** Drop the packet. */
> > > -     MTR_POLICER_ACTION_DROP,
> > > -};
> > > -
> > >  /**
> > >   * Parameters for each traffic metering & policing object
> > >   *
> > > @@ -232,13 +216,13 @@ struct rte_mtr_params {
> > >        */
> > >       int meter_enable;
> > >
> > > -     /** Policer actions (per meter output color). */
> > > -     enum rte_mtr_policer_action action[RTE_COLORS];
> > > -
> > >       /** Set of stats counters to be enabled.
> > >        * @see enum rte_mtr_stats_type
> > >        */
> > >       uint64_t stats_mask;
> > > +
> > > +     /** Meter policy ID. */
> > > +     uint32_t meter_policy_id;
> > >  };
> > >
> > >  /**
> > > @@ -324,6 +308,13 @@ struct rte_mtr_capabilities {
> > >        */
> > >       uint64_t meter_rate_max;
> > >
> > > +     /**
> > > +      * Maximum number of policy objects that can have.
> > > +      * The value of 0 is invalid. Policy must be supported for meter.
> > > +      * The maximum value is *n_max*.
> > > +      */
> > > +     uint64_t meter_policy_n_max;
> > > +
> > >       /**
> > >        * When non-zero, it indicates that color aware mode is
> > > supported for
> > >        * the srTCM RFC 2697 metering algorithm.
> > > @@ -342,18 +333,6 @@ struct rte_mtr_capabilities {
> > >        */
> > >       int color_aware_trtcm_rfc4115_supported;
> > >
> > > -     /** When non-zero, it indicates that the policer packet recolor
> > > actions
> > > -      * are supported.
> > > -      * @see enum rte_mtr_policer_action
> > > -      */
> > > -     int policer_action_recolor_supported;
> > > -
> > > -     /** When non-zero, it indicates that the policer packet drop action is
> > > -      * supported.
> > > -      * @see enum rte_mtr_policer_action
> > > -      */
> > > -     int policer_action_drop_supported;
> > > -
> > >       /** Set of supported statistics counter types.
> > >        * @see enum rte_mtr_stats_type
> > >        */
> > > @@ -462,6 +441,94 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
> > >       uint32_t meter_profile_id,
> > >       struct rte_mtr_error *error);
> > >
> > > +/**
> > > + * Policy id 0 is default policy.
> >
> > I suggest you do not redundantly specify the value of the default policy ID
> in the
> > comment. Replace by "Default policy ID."
> >
> > > + * Action per color as below:
> > > + * green - no action, yellow - no action, red - drop
> >
> > This does not make sense to me as the default policy. The default policy
> should
> > be "no change", i.e. green -> green (no change), yellow -> yellow (no
> change),
> > red -> red (no change).
> 
> Can you explain why it doesn't make sense to you?
> 
> Meter with "no change" for all colors has no effect on the packets so it is
> redundant action which just costs performance and resources - probably
> never be used.
> 

The mbuf::sched::color needs to be set for the packet, and the only way to do this is by applying the RTE_FLOW_ACTION_TYPE_COLOR Action, right? It would make sense that the default policy is to simply apply to the packet the color that the meter just computed for the current packet with no change, right?

> The most common usage for meter is to drop all the packets come above the
> defined rate limit - so it makes sense to take this behavior as default.
> 

I don't agree with this assertion either. One typical usage of the color is to accept all input packets from the user, either green, yellow or red in the absence of any congestion, and charge the user for this traffic; in case of congestion, as typically detected later (typically on scheduling and maybe on a different network node, depending on the application), the packet color is used to prioritize between packets, i.e. drop red packets first before dropping any yellow or green packets. In this case, there is no pre-defined "drop all red packets straight away" policy.

> 
> > I suggest we avoid the "no action" statement, as it might be confusing.
> 
> Maybe "do nothing" is better?
> 

Yes, makes sense to me.

> 
> > > + * It can be used without creating it by the rte_mtr_meter_policy_add
> > > function.
> > > + */
> > > +#define RTE_MTR_DEFAULT_POLICY_ID 0
> > > +
> > > +/**
> > > + * Check whether a meter policy can be created on a given port.
> > > + *
> > > + * The meter policy is validated for correctness and
> > > + * whether it could be accepted by the device given sufficient resources.
> > > + * The policy is checked against the current capability information
> > > + * meter_policy_n_max configuration.
> > > + * The policy may also optionally be validated against existing
> > > + * device policy resources.
> > > + * This function has no effect on the target device.
> > > + *
> > > + * @param[in] port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param[in] policy_id
> > > + *   Policy identifier for the new meter policy.
> > > + * @param[in] actions
> > > + *   Associated action list per color.
> > > + *   list NULL is legal and means no special action.
> > > + *   (list terminated by the END action).
> > > + * @param[out] error
> > > + *   Error details. Filled in only on error, when not NULL.
> > > + *
> > > + * @return
> > > + *   0 on success, non-zero error code otherwise.
> > > + *
> > > + */
> > > +__rte_experimental
> > > +int
> > > +rte_mtr_meter_policy_validate(uint16_t port_id,
> > > +     uint32_t policy_id,
> > > +     const struct rte_flow_action *actions[RTE_COLORS],
> > > +     struct rte_mtr_error *error);
> > > +
> >
> > This entire proposal is about defining a meter policy, yet there is no
> structure
> > defining what exactly the meter policy is!
> >
> > Please let's use the following structure for the policy parameters for the
> policy
> > validate/add/delete API functions:
> >
> > struct rte_mtr_meter_policy_params {
> >         struct rte_flow_action *actions[RTE_COLORS]; };
> >
> > This would also give us the chance to document the policy concept in detail
> > once and not repeat the same explanation over and over. For example, we
> > need to explain clearly that actions[i] potentially represents a chain of
> rte_flow
> > actions terminated by the END action, exactly as specified by the rte_flow
> API
> > for the flow definition, and not just a single action. This is very important
> and
> > yet not entirely clear from the above description.
> >
> > This would also give us the chance to add more attributes to the meter
> policy
> > later, if/when needed, with minimal impact to the API.
> >
> > > +/**
> > > + * Meter policy add
> > > + *
> > > + * Create a new meter policy. The new policy
> > > + * is used to create single or multiple MTR objects.
> > > + *
> > > + * @param[in] port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param[in] policy_id
> > > + *   Policy identifier for the new meter policy.
> > > + * @param[in] actions
> > > + *   Associated actions per color.
> > > + *   list NULL is legal and means no special action.
> > > + *   (list terminated by the END action).
> > > + * @param[out] error
> > > + *   Error details. Filled in only on error, when not NULL.
> > > + * @return
> > > + *   0 on success, non-zero error code otherwise.
> > > + */
> > > +__rte_experimental
> > > +int
> > > +rte_mtr_meter_policy_add(uint16_t port_id,
> > > +     uint32_t policy_id,
> > > +     const struct rte_flow_action *actions[RTE_COLORS],
> > > +     struct rte_mtr_error *error);
> > > +
> > > +/**
> > > + * Meter policy delete
> > > + *
> > > + * Delete an existing meter policy. This operation fails when there
> > > +is
> > > + * currently at least one user (i.e. MTR object) of this policy.
> > > + *
> > > + * @param[in] port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param[in] policy_id
> > > + *   Policy identifier.
> > > + * @param[out] error
> > > + *   Error details. Filled in only on error, when not NULL.
> > > + * @return
> > > + *   0 on success, non-zero error code otherwise.
> > > + */
> > > +__rte_experimental
> > > +int
> > > +rte_mtr_meter_policy_delete(uint16_t port_id,
> > > +     uint32_t policy_id,
> > > +     struct rte_mtr_error *error);
> > > +
> > >  /**
> > >   * MTR object create
> > >   *
> > > @@ -587,18 +654,14 @@ rte_mtr_meter_profile_update(uint16_t
> port_id,
> > >       struct rte_mtr_error *error);
> > >
> > >  /**
> > > - * MTR object DSCP table update
> > > + * MTR object meter policy update
> > >   *
> > >   * @param[in] port_id
> > >   *   The port identifier of the Ethernet device.
> > >   * @param[in] mtr_id
> > >   *   MTR object ID. Needs to be valid.
> > > - * @param[in] dscp_table
> > > - *   When non-NULL: it points to a pre-allocated and pre-populated table
> > > with
> > > - *   exactly 64 elements providing the input color for each value of the
> > > - *   IPv4/IPv6 Differentiated Services Code Point (DSCP) input packet
> field.
> > > - *   When NULL: it is equivalent to setting this parameter to an “all-
> > > green”
> > > - *   populated table (i.e. table with all the 64 elements set to green
> color).
> > > + * @param[in] meter_policy_id
> > > + *   Meter policy ID for the current MTR object. Needs to be valid.
> > >   * @param[out] error
> > >   *   Error details. Filled in only on error, when not NULL.
> > >   * @return
> > > @@ -606,26 +669,24 @@ rte_mtr_meter_profile_update(uint16_t
> port_id,
> > >   */
> > >  __rte_experimental
> > >  int
> > > -rte_mtr_meter_dscp_table_update(uint16_t port_id,
> > > +rte_mtr_meter_policy_update(uint16_t port_id,
> > >       uint32_t mtr_id,
> > > -     enum rte_color *dscp_table,
> > > +     uint32_t meter_policy_id,
> > >       struct rte_mtr_error *error);
> > >
> > >  /**
> > > - * MTR object policer actions update
> > > + * MTR object DSCP table update
> > >   *
> > >   * @param[in] port_id
> > >   *   The port identifier of the Ethernet device.
> > >   * @param[in] mtr_id
> > >   *   MTR object ID. Needs to be valid.
> > > - * @param[in] action_mask
> > > - *   Bit mask indicating which policer actions need to be updated. One or
> > > more
> > > - *   policer actions can be updated in a single function invocation. To
> update
> > > - *   the policer action associated with color C, bit (1 << C) needs to be set
> in
> > > - *   *action_mask* and element at position C in the *actions* array
> needs to
> > > be
> > > - *   valid.
> > > - * @param[in] actions
> > > - *   Pre-allocated and pre-populated array of policer actions.
> > > + * @param[in] dscp_table
> > > + *   When non-NULL: it points to a pre-allocated and pre-populated table
> > > with
> > > + *   exactly 64 elements providing the input color for each value of the
> > > + *   IPv4/IPv6 Differentiated Services Code Point (DSCP) input packet
> field.
> > > + *   When NULL: it is equivalent to setting this parameter to an “all-
> > > green”
> > > + *   populated table (i.e. table with all the 64 elements set to green
> color).
> > >   * @param[out] error
> > >   *   Error details. Filled in only on error, when not NULL.
> > >   * @return
> > > @@ -633,10 +694,9 @@ rte_mtr_meter_dscp_table_update(uint16_t
> > > port_id,
> > >   */
> > >  __rte_experimental
> > >  int
> > > -rte_mtr_policer_actions_update(uint16_t port_id,
> > > +rte_mtr_meter_dscp_table_update(uint16_t port_id,
> > >       uint32_t mtr_id,
> > > -     uint32_t action_mask,
> > > -     enum rte_mtr_policer_action *actions,
> > > +     enum rte_color *dscp_table,
> > >       struct rte_mtr_error *error);
> > >
> > >  /**
> > > diff --git a/lib/librte_ethdev/rte_mtr_driver.h
> > > b/lib/librte_ethdev/rte_mtr_driver.h
> > > index a0ddc2b5f4..1ad8fb4c40 100644
> > > --- a/lib/librte_ethdev/rte_mtr_driver.h
> > > +++ b/lib/librte_ethdev/rte_mtr_driver.h
> > > @@ -41,6 +41,23 @@ typedef int
> > > (*rte_mtr_meter_profile_delete_t)(struct
> > > rte_eth_dev *dev,
> > >       struct rte_mtr_error *error);
> > >  /**< @internal MTR meter profile delete */
> > >
> > > +typedef int (*rte_mtr_meter_policy_validate_t)(struct rte_eth_dev
> *dev,
> > > +     uint32_t policy_id,
> > > +     const struct rte_flow_action *actions[RTE_COLORS],
> > > +     struct rte_mtr_error *error);
> > > +/**< @internal MTR meter policy validate */
> > > +
> > > +typedef int (*rte_mtr_meter_policy_add_t)(struct rte_eth_dev *dev,
> > > +     uint32_t policy_id,
> > > +     const struct rte_flow_action *actions[RTE_COLORS],
> > > +     struct rte_mtr_error *error);
> > > +/**< @internal MTR meter policy add */
> > > +
> > > +typedef int (*rte_mtr_meter_policy_delete_t)(struct rte_eth_dev
> *dev,
> > > +     uint32_t policy_id,
> > > +     struct rte_mtr_error *error);
> > > +/**< @internal MTR meter policy delete */
> > > +
> > >  typedef int (*rte_mtr_create_t)(struct rte_eth_dev *dev,
> > >       uint32_t mtr_id,
> > >       struct rte_mtr_params *params,
> > > @@ -69,18 +86,17 @@ typedef int
> > > (*rte_mtr_meter_profile_update_t)(struct rte_eth_dev *dev,
> > >       struct rte_mtr_error *error);
> > >  /**< @internal MTR object meter profile update */
> > >
> > > -typedef int (*rte_mtr_meter_dscp_table_update_t)(struct
> rte_eth_dev
> > > *dev,
> > > +typedef int (*rte_mtr_meter_policy_update_t)(struct rte_eth_dev
> *dev,
> > >       uint32_t mtr_id,
> > > -     enum rte_color *dscp_table,
> > > +     uint32_t meter_policy_id,
> > >       struct rte_mtr_error *error);
> > > -/**< @internal MTR object meter DSCP table update */
> > > +/**< @internal MTR object meter policy update */
> > >
> > > -typedef int (*rte_mtr_policer_actions_update_t)(struct rte_eth_dev
> > > *dev,
> > > +typedef int (*rte_mtr_meter_dscp_table_update_t)(struct
> rte_eth_dev
> > > *dev,
> > >       uint32_t mtr_id,
> > > -     uint32_t action_mask,
> > > -     enum rte_mtr_policer_action *actions,
> > > +     enum rte_color *dscp_table,
> > >       struct rte_mtr_error *error);
> > > -/**< @internal MTR object policer action update*/
> > > +/**< @internal MTR object meter DSCP table update */
> > >
> > >  typedef int (*rte_mtr_stats_update_t)(struct rte_eth_dev *dev,
> > >       uint32_t mtr_id,
> > > @@ -124,14 +140,23 @@ struct rte_mtr_ops {
> > >       /** MTR object meter DSCP table update */
> > >       rte_mtr_meter_dscp_table_update_t meter_dscp_table_update;
> > >
> > > -     /** MTR object policer action update */
> > > -     rte_mtr_policer_actions_update_t policer_actions_update;
> > > -
> > >       /** MTR object enabled stats update */
> > >       rte_mtr_stats_update_t stats_update;
> > >
> > >       /** MTR object stats read */
> > >       rte_mtr_stats_read_t stats_read;
> > > +
> > > +     /** MTR meter policy validate */
> > > +     rte_mtr_meter_policy_validate_t meter_policy_validate;
> > > +
> > > +     /** MTR meter policy add */
> > > +     rte_mtr_meter_policy_add_t meter_policy_add;
> > > +
> > > +     /** MTR meter policy delete */
> > > +     rte_mtr_meter_policy_delete_t meter_policy_delete;
> > > +
> > > +     /** MTR object meter policy update */
> > > +     rte_mtr_meter_policy_update_t meter_policy_update;
> > >  };
> > >
> > >  /**
> > > --
> > > 2.21.0
> >
> > Regards,
> > Cristian
> Thanks!

Regards,
Cristian
  

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Monday, March 29, 2021 10:23 AM
> To: Matan Azrad <matan@nvidia.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Li Zhang <lizh@nvidia.com>; Dekel Peled
> <dekelp@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit Khaparde <ajit.khaparde@broadcom.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>
> Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> Hi All,
> 
> > -----Original Message-----
> > From: Matan Azrad <matan@nvidia.com>
> > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> >
> > Hi Cristian
> >
> > Thank you for your important review!
> > I agree with all your comments except one, please see inline.
> >
> > From: Dumitrescu, Cristian
> > > Hi Li and Matan,
> > >
> > > Thank you for your proposal, some comments below.
> > >
> > > I am also adding Jerin and Hemant to this thread, as they also participated
> in
> > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> interest in
> > a
> > > previous email.
> > >
> > > > -----Original Message-----
> > > > From: Li Zhang <lizh@nvidia.com>
> > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Andrew
> > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > > <cristian.dumitrescu@intel.com>
> > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > > >
> > > > Currently, the flow meter policy does not support multiple actions per
> > > > color; also the allowed action types per color are very limited.
> > > > In addition, the policy cannot be pre-defined.
> > > >
> > > > Due to the growing in flow actions offload abilities there is a
> > > > potential for the user to use variety of actions per color differently.
> > > > This new meter policy API comes to allow this potential in the most
> > > > ethdev common way using rte_flow action definition.
> > > > A list of rte_flow actions will be provided by the user per color in
> > > > order to create a meter policy.
> > > > In addition, the API forces to pre-define the policy before the meters
> > > > creation in order to allow sharing of single policy with multiple
> > > > meters efficiently.
> > > >
> > > > meter_policy_id is added into struct rte_mtr_params.
> > > > So that it can get the policy during the meters creation.
> > > >
> > > > Policy id 0 is default policy. Action per color as below:
> > > > green - no action, yellow - no action, red - drop
> > > >
> > > > Allow coloring the packet using a new rte_flow_action_color as could
> > > > be done by the old policy API,
> > > >
> > >
> > > The proposal essentially is to define the meter policy based on rte_flow
> > actions
> > > rather than a reduced action set defined specifically just for meter object.
> > This
> > > makes sense to me.
> > >
> > > > The next API function were added:
> > > > - rte_mtr_meter_policy_add
> > > > - rte_mtr_meter_policy_delete
> > > > - rte_mtr_meter_policy_update
> > > > - rte_mtr_meter_policy_validate
> > > > The next struct was changed:
> > > > - rte_mtr_params
> > > > - rte_mtr_capabilities
> > > > The next API was deleted:
> > > > - rte_mtr_policer_actions_update
> > > >
> > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > ---
> > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > @@ -31,6 +31,7 @@
> > > >  #include <rte_ecpri.h>
> > > >  #include <rte_mbuf.h>
> > > >  #include <rte_mbuf_dyn.h>
> > > > +#include <rte_meter.h>
> > > >
> > > >  #ifdef __cplusplus
> > > >  extern "C" {
> > > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > > >        * See struct rte_flow_action_modify_field.
> > > >        */
> > > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > > +
> > > > +     /**
> > > > +      * Color the packet to reflect the meter color result.
> > > > +      *
> > > > +      * See struct rte_flow_action_color.
> > > > +      */
> > > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> > >
> > > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> > >
> 
> Why do we need this action?

We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to set the packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order to tell the later stages of the pipeline what the packet color is.

> if it is to save the color it should be done by using mark/metadata

As stated in its description, the  RTE_FLOW_ACTION_TYPE_MARK action Is setting the mbuf::hash.fdir.hi field, which is used for a different purpose that is unrelated to the packet color, which has its own field within the mbuf.

> Or by the action of meter.

The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an action of the meter and meter only, right?

For example you can see
> RTE_FLOW_ACTION_TYPE_SECURITY
> Which if exist saves the session id to a dedicated mbuf field.
> 

The meter processing and action take place independently of the security API: it can be enabled when the security API is disabled and is not conditioned in any way by the security API. To be honest, I don't understand the connection with the security API that you are trying to make here.

> > > >  };
> > > >
> > > >  /**
> 
> [Snip]
> 
> > > I suggest you do not redundantly specify the value of the default policy ID
> in
> > the
> > > comment. Replace by "Default policy ID."
> > >
> > > > + * Action per color as below:
> > > > + * green - no action, yellow - no action, red - drop
> > >
> > > This does not make sense to me as the default policy. The default policy
> > should
> > > be "no change", i.e. green -> green (no change), yellow -> yellow (no
> change),
> > > red -> red (no change).
> >
> > Can you explain why it doesn't make sense to you?
> >
> > Meter with "no change" for all colors has no effect on the packets so it is
> > redundant action which just costs performance and resources - probably
> never
> > be used.
> >
> > The most common usage for meter is to drop all the packets come above
> the
> > defined rate limit - so it makes sense to take this behavior as default.
> >
> >
> > > I suggest we avoid the "no action" statement, as it might be confusing.
> >
> > Maybe "do nothing" is better?
> >
> Maybe passthrough? Or in rte_flow passthru
> 

No, we need to save the packet color in the packet mbuf (mbuf::sched:color), and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this.

> 
> > > > + * It can be used without creating it by the rte_mtr_meter_policy_add
> > > > function.
> > > > + */
> 
> 
> Best,
> Ori

Regards,
Cristian
  

Hi Jerin

Thanks for the review.
PSB

From: Jerin Jacob
> On Thu, Mar 18, 2021 at 2:28 PM Li Zhang <lizh@nvidia.com> wrote:
> >
> > Currently, the flow meter policy does not support multiple actions per
> > color; also the allowed action types per color are very limited.
> > In addition, the policy cannot be pre-defined.
> >
> > Due to the growing in flow actions offload abilities there is a
> > potential for the user to use variety of actions per color differently.
> > This new meter policy API comes to allow this potential in the most
> > ethdev common way using rte_flow action definition.
> > A list of rte_flow actions will be provided by the user per color in
> > order to create a meter policy.
> > In addition, the API forces to pre-define the policy before the meters
> > creation in order to allow sharing of single policy with multiple
> > meters efficiently.
> >
> > meter_policy_id is added into struct rte_mtr_params.
> > So that it can get the policy during the meters creation.
> >
> > Policy id 0 is default policy. Action per color as below:
> > green - no action, yellow - no action, red - drop
> >
> > Allow coloring the packet using a new rte_flow_action_color as could
> > be done by the old policy API,
> >
> > The next API function were added:
> > - rte_mtr_meter_policy_add
> > - rte_mtr_meter_policy_delete
> > - rte_mtr_meter_policy_update
> > - rte_mtr_meter_policy_validate
> > The next struct was changed:
> > - rte_mtr_params
> > - rte_mtr_capabilities
> > The next API was deleted:
> > - rte_mtr_policer_actions_update
> >
> > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > ---
> >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> >  4 files changed, 210 insertions(+), 74 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -31,6 +31,7 @@
> >  #include <rte_ecpri.h>
> >  #include <rte_mbuf.h>
> >  #include <rte_mbuf_dyn.h>
> > +#include <rte_meter.h>
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> >          * See struct rte_flow_action_modify_field.
> >          */
> >         RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > +
> > +       /**
> > +        * Color the packet to reflect the meter color result.
> > +        *
> > +        * See struct rte_flow_action_color.
> > +        */
> > +       RTE_FLOW_ACTION_TYPE_COlOR,
> 
> Based on my understanding of this API,
> 1) Application creates the policy
> 2) Attachs the policy ID to meter object in params If so, Why we need this new
> action?

Yes,
In the new policy API the meter actions will be defined by rte_flow actions.
The old policy API used rte_mtr actions: color green\color yellow\color red\drop.

This new rte_flow COLOR action comes to replace the old policy API "color" actions which set the color field in mbuf.


> >  };
> >
> >  /**
> > @@ -2828,6 +2836,16 @@ struct rte_flow_action_set_dscp {
> >   */
> >  struct rte_flow_shared_action;
> >
> > +/**
> > + * Meter policy add
> > + *
> > + * Create a new meter policy. The new policy
> > + * is used to create single or multiple MTR objects.
> > + *
> > + * @param[in] port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param[in] policy_id
> > + *   Policy identifier for the new meter policy.
> > + * @param[in] actions
> > + *   Associated actions per color.
> > + *   list NULL is legal and means no special action.
> > + *   (list terminated by the END action).
> > + * @param[out] error
> > + *   Error details. Filled in only on error, when not NULL.
> > + * @return
> > + *   0 on success, non-zero error code otherwise.
> > + */
> > +__rte_experimental
> > +int
> > +rte_mtr_meter_policy_add(uint16_t port_id,
> 
> 
> _create() may be better here instead of _add() as you have used _delete()

Yes!

> > +       uint32_t policy_id,
> > +       const struct rte_flow_action *actions[RTE_COLORS],
> 
> 
> 1) Does this mean that MLX HW can support any rte_flow actions like, if packet
> color is green do RTE_FLOW_ACTION_TYPE_SECURITY etc.

Theoretically yes, we can support most of the actions.
For the first stage we are going to support next: queue\RSS\port id\mark\tag\jump.

For example, user can select different queue\port id per color.

> 2) Is there any real-world use case other than using normal action like pass or
> drop as it is used in conjunction with meter object?

Yes, I wrote above.
 
> 3) Marvell HW has the following policy actions
> a) PASS
> b) DROP
> c) RED (Random early discard)
> 
> Both (a) and (c) are not in enumated as rte_flow_actions.

(a) is in, just don't specify any action.

Can you explain what is "Random early discard"?
How did you specify it in the old\current meter policy API?


Note, that after the policy actions the device should continue to do the rest of the actions in the flow (after meter) if no termination action in the policy color.

> 
> Should we take rte_flow_action or create meter-specific policy actions?

This patch removes the meter-specific policy actions.
You need to use rte_flow action.

By the way, can you help to adjust Marvell driver to the change?
  

From: Dumitrescu, Cristian
> Hi Matan,
> 
> > -----Original Message-----
> > From: Matan Azrad <matan@nvidia.com>
> > Sent: Thursday, March 25, 2021 6:57 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> > <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> > <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> > Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> > <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit
> > Khaparde <ajit.khaparde@broadcom.com>
> > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar
> > Yanai <roniba@nvidia.com>
> > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > API
> >
> > Hi Cristian
> >
> > Thank you for your important review!
> > I agree with all your comments except one, please see inline.
> >
> > From: Dumitrescu, Cristian
> > > Hi Li and Matan,
> > >
> > > Thank you for your proposal, some comments below.
> > >
> > > I am also adding Jerin and Hemant to this thread, as they also
> > > participated
> > in
> > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> > > interest
> > in a
> > > previous email.
> > >
> > > > -----Original Message-----
> > > > From: Li Zhang <lizh@nvidia.com>
> > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Andrew
> > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > > <cristian.dumitrescu@intel.com>
> > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > > > API
> > > >
> > > > Currently, the flow meter policy does not support multiple actions
> > > > per color; also the allowed action types per color are very limited.
> > > > In addition, the policy cannot be pre-defined.
> > > >
> > > > Due to the growing in flow actions offload abilities there is a
> > > > potential for the user to use variety of actions per color differently.
> > > > This new meter policy API comes to allow this potential in the
> > > > most ethdev common way using rte_flow action definition.
> > > > A list of rte_flow actions will be provided by the user per color
> > > > in order to create a meter policy.
> > > > In addition, the API forces to pre-define the policy before the
> > > > meters creation in order to allow sharing of single policy with
> > > > multiple meters efficiently.
> > > >
> > > > meter_policy_id is added into struct rte_mtr_params.
> > > > So that it can get the policy during the meters creation.
> > > >
> > > > Policy id 0 is default policy. Action per color as below:
> > > > green - no action, yellow - no action, red - drop
> > > >
> > > > Allow coloring the packet using a new rte_flow_action_color as
> > > > could be done by the old policy API,
> > > >
> > >
> > > The proposal essentially is to define the meter policy based on
> > > rte_flow
> > actions
> > > rather than a reduced action set defined specifically just for meter object.
> > This
> > > makes sense to me.
> > >
> > > > The next API function were added:
> > > > - rte_mtr_meter_policy_add
> > > > - rte_mtr_meter_policy_delete
> > > > - rte_mtr_meter_policy_update
> > > > - rte_mtr_meter_policy_validate
> > > > The next struct was changed:
> > > > - rte_mtr_params
> > > > - rte_mtr_capabilities
> > > > The next API was deleted:
> > > > - rte_mtr_policer_actions_update
> > > >
> > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > ---
> > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > >  4 files changed, 210 insertions(
<snip>
> > > > +/**
> > > > + * Policy id 0 is default policy.
> > >
> > > I suggest you do not redundantly specify the value of the default
> > > policy ID
> > in the
> > > comment. Replace by "Default policy ID."
> > >
> > > > + * Action per color as below:
> > > > + * green - no action, yellow - no action, red - drop
> > >
> > > This does not make sense to me as the default policy. The default
> > > policy
> > should
> > > be "no change", i.e. green -> green (no change), yellow -> yellow
> > > (no
> > change),
> > > red -> red (no change).
> >
> > Can you explain why it doesn't make sense to you?
> >
> > Meter with "no change" for all colors has no effect on the packets so
> > it is redundant action which just costs performance and resources -
> > probably never be used.
> >
> 
> The mbuf::sched::color needs to be set for the packet, and the only way to do
> this is by applying the RTE_FLOW_ACTION_TYPE_COLOR Action, right? It would
> make sense that the default policy is to simply apply to the packet the color
> that the meter just computed for the current packet with no change, right?

I don't think so.
When we are working with HW offloads (this is the main goal of rte_flow and this meter API) the motivation is to do the actions directly in the NIC HW.
Moving the color information to the SW is like doing "partial offload".


> > The most common usage for meter is to drop all the packets come above
> > the defined rate limit - so it makes sense to take this behavior as default.
> >
> 
> I don't agree with this assertion either. One typical usage of the color is to
> accept all input packets from the user, either green, yellow or red in the
> absence of any congestion, and charge the user for this traffic; in case of
> congestion, as typically detected later (typically on scheduling and maybe on a
> different network node, depending on the application), the packet color is used
> to prioritize between packets, i.e. drop red packets first before dropping any
> yellow or green packets. In this case, there is no pre-defined "drop all red
> packets straight away" policy.


I familiar with a lot of meter users(at least 5 applications) in the industry, no one use the color actions. 
All of them drop red packets and continue to the next flow actions(after meter) otherwise.


If you insist, we can define 2 default IDs...

> >
> > > I suggest we avoid the "no action" statement, as it might be confusing.
> >
> > Maybe "do nothing" is better?
> >
> 
> Yes, makes sense to me.

<snip>
  

On Tue, Mar 30, 2021 at 2:01 AM Matan Azrad <matan@nvidia.com> wrote:
>
> Hi Jerin
>
> Thanks for the review.
> PSB
>
> From: Jerin Jacob
> > On Thu, Mar 18, 2021 at 2:28 PM Li Zhang <lizh@nvidia.com> wrote:
> > >
> > > Currently, the flow meter policy does not support multiple actions per
> > > color; also the allowed action types per color are very limited.
> > > In addition, the policy cannot be pre-defined.
> > >
> > > Due to the growing in flow actions offload abilities there is a
> > > potential for the user to use variety of actions per color differently.
> > > This new meter policy API comes to allow this potential in the most
> > > ethdev common way using rte_flow action definition.
> > > A list of rte_flow actions will be provided by the user per color in
> > > order to create a meter policy.
> > > In addition, the API forces to pre-define the policy before the meters
> > > creation in order to allow sharing of single policy with multiple
> > > meters efficiently.
> > >
> > > meter_policy_id is added into struct rte_mtr_params.
> > > So that it can get the policy during the meters creation.
> > >
> > > Policy id 0 is default policy. Action per color as below:
> > > green - no action, yellow - no action, red - drop
> > >
> > > Allow coloring the packet using a new rte_flow_action_color as could
> > > be done by the old policy API,
> > >
> > > The next API function were added:
> > > - rte_mtr_meter_policy_add
> > > - rte_mtr_meter_policy_delete
> > > - rte_mtr_meter_policy_update
> > > - rte_mtr_meter_policy_validate
> > > The next struct was changed:
> > > - rte_mtr_params
> > > - rte_mtr_capabilities
> > > The next API was deleted:
> > > - rte_mtr_policer_actions_update
> > >
> > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > ---
> > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -31,6 +31,7 @@
> > >  #include <rte_ecpri.h>
> > >  #include <rte_mbuf.h>
> > >  #include <rte_mbuf_dyn.h>
> > > +#include <rte_meter.h>
> > >
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > >          * See struct rte_flow_action_modify_field.
> > >          */
> > >         RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > +
> > > +       /**
> > > +        * Color the packet to reflect the meter color result.
> > > +        *
> > > +        * See struct rte_flow_action_color.
> > > +        */
> > > +       RTE_FLOW_ACTION_TYPE_COlOR,
> >
> > Based on my understanding of this API,
> > 1) Application creates the policy
> > 2) Attachs the policy ID to meter object in params If so, Why we need this new
> > action?
>
> Yes,
> In the new policy API the meter actions will be defined by rte_flow actions.
> The old policy API used rte_mtr actions: color green\color yellow\color red\drop.
>
> This new rte_flow COLOR action comes to replace the old policy API "color" actions which set the color field in mbuf.

OK. Looks good to me. I think, it is better to change to
RTE_FLOW_ACTION_TYPE_METER_COLOR to
denote it as meter-specific action.

>
>
> > >  };
> > >
> > >  /**
> > > @@ -2828,6 +2836,16 @@ struct rte_flow_action_set_dscp {
> > >   */
> > >  struct rte_flow_shared_action;
> > >
> > > +/**
> > > + * Meter policy add
> > > + *
> > > + * Create a new meter policy. The new policy
> > > + * is used to create single or multiple MTR objects.
> > > + *
> > > + * @param[in] port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param[in] policy_id
> > > + *   Policy identifier for the new meter policy.
> > > + * @param[in] actions
> > > + *   Associated actions per color.
> > > + *   list NULL is legal and means no special action.
> > > + *   (list terminated by the END action).
> > > + * @param[out] error
> > > + *   Error details. Filled in only on error, when not NULL.
> > > + * @return
> > > + *   0 on success, non-zero error code otherwise.
> > > + */
> > > +__rte_experimental
> > > +int
> > > +rte_mtr_meter_policy_add(uint16_t port_id,
> >
> >
> > _create() may be better here instead of _add() as you have used _delete()
>
> Yes!

OK

>
> > > +       uint32_t policy_id,
> > > +       const struct rte_flow_action *actions[RTE_COLORS],
> >
> >
> > 1) Does this mean that MLX HW can support any rte_flow actions like, if packet
> > color is green do RTE_FLOW_ACTION_TYPE_SECURITY etc.
>
> Theoretically yes, we can support most of the actions.
> For the first stage we are going to support next: queue\RSS\port id\mark\tag\jump.
>
> For example, user can select different queue\port id per color.


OK.

>
> > 2) Is there any real-world use case other than using normal action like pass or
> > drop as it is used in conjunction with meter object?
>
> Yes, I wrote above.
>
> > 3) Marvell HW has the following policy actions
> > a) PASS
> > b) DROP
> > c) RED (Random early discard)
> >
> > Both (a) and (c) are not in enumated as rte_flow_actions.
>
> (a) is in, just don't specify any action.
>
> Can you explain what is "Random early discard"?

https://en.wikipedia.org/wiki/Random_early_detection

> How did you specify it in the old\current meter policy API?

It can not. We are planning to add support for the meter in the new cnxk driver.
Planning change the meter API based on the driver patch.

>
>
> Note, that after the policy actions the device should continue to do the rest of the actions in the flow (after meter) if no termination action in the policy color.
>
> >
> > Should we take rte_flow_action or create meter-specific policy actions?
>
> This patch removes the meter-specific policy actions.
> You need to use rte_flow action.
>
> By the way, can you help to adjust Marvell driver to the change?

The current driver(octeontx2) is not using meter.



>
  

Hi Matan,

> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> Sent: Monday, March 29, 2021 9:44 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit Khaparde <ajit.khaparde@broadcom.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>
> Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> 
> 
> From: Dumitrescu, Cristian
> > Hi Matan,
> >
> > > -----Original Message-----
> > > From: Matan Azrad <matan@nvidia.com>
> > > Sent: Thursday, March 25, 2021 6:57 AM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> > > <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> > > <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> > > Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> > > <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Andrew
> > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> > > <jerinjacobk@gmail.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>;
> > Ajit
> > > Khaparde <ajit.khaparde@broadcom.com>
> > > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar
> > > Yanai <roniba@nvidia.com>
> > > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > > API
> > >
> > > Hi Cristian
> > >
> > > Thank you for your important review!
> > > I agree with all your comments except one, please see inline.
> > >
> > > From: Dumitrescu, Cristian
> > > > Hi Li and Matan,
> > > >
> > > > Thank you for your proposal, some comments below.
> > > >
> > > > I am also adding Jerin and Hemant to this thread, as they also
> > > > participated
> > > in
> > > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> > > > interest
> > > in a
> > > > previous email.
> > > >
> > > > > -----Original Message-----
> > > > > From: Li Zhang <lizh@nvidia.com>
> > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > Andrew
> > > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > > > <cristian.dumitrescu@intel.com>
> > > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > > > > API
> > > > >
> > > > > Currently, the flow meter policy does not support multiple actions
> > > > > per color; also the allowed action types per color are very limited.
> > > > > In addition, the policy cannot be pre-defined.
> > > > >
> > > > > Due to the growing in flow actions offload abilities there is a
> > > > > potential for the user to use variety of actions per color differently.
> > > > > This new meter policy API comes to allow this potential in the
> > > > > most ethdev common way using rte_flow action definition.
> > > > > A list of rte_flow actions will be provided by the user per color
> > > > > in order to create a meter policy.
> > > > > In addition, the API forces to pre-define the policy before the
> > > > > meters creation in order to allow sharing of single policy with
> > > > > multiple meters efficiently.
> > > > >
> > > > > meter_policy_id is added into struct rte_mtr_params.
> > > > > So that it can get the policy during the meters creation.
> > > > >
> > > > > Policy id 0 is default policy. Action per color as below:
> > > > > green - no action, yellow - no action, red - drop
> > > > >
> > > > > Allow coloring the packet using a new rte_flow_action_color as
> > > > > could be done by the old policy API,
> > > > >
> > > >
> > > > The proposal essentially is to define the meter policy based on
> > > > rte_flow
> > > actions
> > > > rather than a reduced action set defined specifically just for meter
> object.
> > > This
> > > > makes sense to me.
> > > >
> > > > > The next API function were added:
> > > > > - rte_mtr_meter_policy_add
> > > > > - rte_mtr_meter_policy_delete
> > > > > - rte_mtr_meter_policy_update
> > > > > - rte_mtr_meter_policy_validate
> > > > > The next struct was changed:
> > > > > - rte_mtr_params
> > > > > - rte_mtr_capabilities
> > > > > The next API was deleted:
> > > > > - rte_mtr_policer_actions_update
> > > > >
> > > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > > ---
> > > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++-------
> --
> > > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > > >  4 files changed, 210 insertions(
> <snip>
> > > > > +/**
> > > > > + * Policy id 0 is default policy.
> > > >
> > > > I suggest you do not redundantly specify the value of the default
> > > > policy ID
> > > in the
> > > > comment. Replace by "Default policy ID."
> > > >
> > > > > + * Action per color as below:
> > > > > + * green - no action, yellow - no action, red - drop
> > > >
> > > > This does not make sense to me as the default policy. The default
> > > > policy
> > > should
> > > > be "no change", i.e. green -> green (no change), yellow -> yellow
> > > > (no
> > > change),
> > > > red -> red (no change).
> > >
> > > Can you explain why it doesn't make sense to you?
> > >
> > > Meter with "no change" for all colors has no effect on the packets so
> > > it is redundant action which just costs performance and resources -
> > > probably never be used.
> > >
> >
> > The mbuf::sched::color needs to be set for the packet, and the only way to
> do
> > this is by applying the RTE_FLOW_ACTION_TYPE_COLOR Action, right? It
> would
> > make sense that the default policy is to simply apply to the packet the color
> > that the meter just computed for the current packet with no change, right?
> 
> I don't think so.
> When we are working with HW offloads (this is the main goal of rte_flow and
> this meter API) the motivation is to do the actions directly in the NIC HW.
> Moving the color information to the SW is like doing "partial offload".
> 

Sorry, Matan, but as the bulk of the packets are passed by the NIC to the CPU as opposed to being returned to the network without the CPU ever seeing them, the rte_flow API is a partial offload API, not a full offload API, not sure why we disagree here.

Just to make sure we are on the same page and not getting round in circles: I think you agree that we should have this action RTE_FLOW_ACTION_TYPE_COLOR to setup the color for the CPU to see in mbuf::sched:color, but you don't agree this action should be part of the default policy, did I get your position correctly?

> 
> > > The most common usage for meter is to drop all the packets come above
> > > the defined rate limit - so it makes sense to take this behavior as default.
> > >
> >
> > I don't agree with this assertion either. One typical usage of the color is to
> > accept all input packets from the user, either green, yellow or red in the
> > absence of any congestion, and charge the user for this traffic; in case of
> > congestion, as typically detected later (typically on scheduling and maybe
> on a
> > different network node, depending on the application), the packet color is
> used
> > to prioritize between packets, i.e. drop red packets first before dropping
> any
> > yellow or green packets. In this case, there is no pre-defined "drop all red
> > packets straight away" policy.
> 
> 
> I familiar with a lot of meter users(at least 5 applications) in the industry, no
> one use the color actions.
> All of them drop red packets and continue to the next flow actions(after
> meter) otherwise.
> 

I politely but firmly disagree. None of the apps that I have seen is dropping the red packets straight away, they simply use the color as an indication of the packet drop priority at a later stage in the pipeline when congestion is detected. 

> 
> If you insist, we can define 2 default IDs...
> 

Maybe we should not have any pre-registered policies at all?

For the user's convenience, we could provide the configuration parameters for some of the common policies, like the ones mentioned here, in the API and let the users decide which one(s), if any, they want to register?

> > >
> > > > I suggest we avoid the "no action" statement, as it might be confusing.
> > >
> > > Maybe "do nothing" is better?
> > >
> >
> > Yes, makes sense to me.
> 
> <snip>

Regards,
Cristian
  

Hi Cristian,

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@nvidia.com>
> > Hi All,
> >
> > > -----Original Message-----
> > > From: Matan Azrad <matan@nvidia.com>
> > > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > >
> > > Hi Cristian
> > >
> > > Thank you for your important review!
> > > I agree with all your comments except one, please see inline.
> > >
> > > From: Dumitrescu, Cristian
> > > > Hi Li and Matan,
> > > >
> > > > Thank you for your proposal, some comments below.
> > > >
> > > > I am also adding Jerin and Hemant to this thread, as they also
> participated
> > in
> > > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> > interest in
> > > a
> > > > previous email.
> > > >
> > > > > -----Original Message-----
> > > > > From: Li Zhang <lizh@nvidia.com>
> > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com; Singh,
> > > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Andrew
> > > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu, Cristian
> > > > > <cristian.dumitrescu@intel.com>
> > > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> > > > >
> > > > > Currently, the flow meter policy does not support multiple actions per
> > > > > color; also the allowed action types per color are very limited.
> > > > > In addition, the policy cannot be pre-defined.
> > > > >
> > > > > Due to the growing in flow actions offload abilities there is a
> > > > > potential for the user to use variety of actions per color differently.
> > > > > This new meter policy API comes to allow this potential in the most
> > > > > ethdev common way using rte_flow action definition.
> > > > > A list of rte_flow actions will be provided by the user per color in
> > > > > order to create a meter policy.
> > > > > In addition, the API forces to pre-define the policy before the meters
> > > > > creation in order to allow sharing of single policy with multiple
> > > > > meters efficiently.
> > > > >
> > > > > meter_policy_id is added into struct rte_mtr_params.
> > > > > So that it can get the policy during the meters creation.
> > > > >
> > > > > Policy id 0 is default policy. Action per color as below:
> > > > > green - no action, yellow - no action, red - drop
> > > > >
> > > > > Allow coloring the packet using a new rte_flow_action_color as could
> > > > > be done by the old policy API,
> > > > >
> > > >
> > > > The proposal essentially is to define the meter policy based on rte_flow
> > > actions
> > > > rather than a reduced action set defined specifically just for meter object.
> > > This
> > > > makes sense to me.
> > > >
> > > > > The next API function were added:
> > > > > - rte_mtr_meter_policy_add
> > > > > - rte_mtr_meter_policy_delete
> > > > > - rte_mtr_meter_policy_update
> > > > > - rte_mtr_meter_policy_validate
> > > > > The next struct was changed:
> > > > > - rte_mtr_params
> > > > > - rte_mtr_capabilities
> > > > > The next API was deleted:
> > > > > - rte_mtr_policer_actions_update
> > > > >
> > > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > > ---
> > > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++---------
> > > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4 100644
> > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > @@ -31,6 +31,7 @@
> > > > >  #include <rte_ecpri.h>
> > > > >  #include <rte_mbuf.h>
> > > > >  #include <rte_mbuf_dyn.h>
> > > > > +#include <rte_meter.h>
> > > > >
> > > > >  #ifdef __cplusplus
> > > > >  extern "C" {
> > > > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > > > >        * See struct rte_flow_action_modify_field.
> > > > >        */
> > > > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > > > +
> > > > > +     /**
> > > > > +      * Color the packet to reflect the meter color result.
> > > > > +      *
> > > > > +      * See struct rte_flow_action_color.
> > > > > +      */
> > > > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> > > >
> > > > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> > > >
> >
> > Why do we need this action?
> 
> We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to set the
> packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order to
> tell the later stages of the pipeline what the packet color is.
> 
> > if it is to save the color it should be done by using mark/metadata
> 
> As stated in its description, the  RTE_FLOW_ACTION_TYPE_MARK action Is
> setting the mbuf::hash.fdir.hi field, which is used for a different purpose that is
> unrelated to the packet color, which has its own field within the mbuf.
> 

Agree,

> > Or by the action of meter.
> 
> The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an action
> of the meter and meter only, right?
> 
> For example you can see
> > RTE_FLOW_ACTION_TYPE_SECURITY
> > Which if exist saves the session id to a dedicated mbuf field.
> >
> 
> The meter processing and action take place independently of the security API: it
> can be enabled when the security API is disabled and is not conditioned in any
> way by the security API. To be honest, I don't understand the connection with
> the security API that you are trying to make here.
> 

Sorry for not being clear,
I didn’t mean use the security action, what I meant is just like the security action
which when given, it will saves the session and pass it to the SW in dedicated member in the 
mbuf, the same with meter if the meter action is present then
the PMD should know to save the color value and extract it to the correct mbuf member.

Does that make sense?


> > > > >  };
> > > > >
> > > > >  /**
> >
> > [Snip]
> >
> > > > I suggest you do not redundantly specify the value of the default policy ID
> > in
> > > the
> > > > comment. Replace by "Default policy ID."
> > > >
> > > > > + * Action per color as below:
> > > > > + * green - no action, yellow - no action, red - drop
> > > >
> > > > This does not make sense to me as the default policy. The default policy
> > > should
> > > > be "no change", i.e. green -> green (no change), yellow -> yellow (no
> > change),
> > > > red -> red (no change).
> > >
> > > Can you explain why it doesn't make sense to you?
> > >
> > > Meter with "no change" for all colors has no effect on the packets so it is
> > > redundant action which just costs performance and resources - probably
> > never
> > > be used.
> > >
> > > The most common usage for meter is to drop all the packets come above
> > the
> > > defined rate limit - so it makes sense to take this behavior as default.
> > >
> > >
> > > > I suggest we avoid the "no action" statement, as it might be confusing.
> > >
> > > Maybe "do nothing" is better?
> > >
> > Maybe passthrough? Or in rte_flow passthru
> >
> 
> No, we need to save the packet color in the packet mbuf (mbuf::sched:color),
> and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this.
> 

Please see my comment above.
The saving of color will be done automatically.

> >
> > > > > + * It can be used without creating it by the rte_mtr_meter_policy_add
> > > > > function.
> > > > > + */
> >
> >
> > Best,
> > Ori
> 
> Regards,
> Cristian

Best,
Ori
  

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Thursday, April 1, 2021 2:14 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Matan Azrad
> <matan@nvidia.com>; Li Zhang <lizh@nvidia.com>; Dekel Peled
> <dekelp@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit Khaparde <ajit.khaparde@broadcom.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>
> Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy API
> 
> Hi Cristian,
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@nvidia.com>
> > > Hi All,
> > >
> > > > -----Original Message-----
> > > > From: Matan Azrad <matan@nvidia.com>
> > > > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> API
> > > >
> > > > Hi Cristian
> > > >
> > > > Thank you for your important review!
> > > > I agree with all your comments except one, please see inline.
> > > >
> > > > From: Dumitrescu, Cristian
> > > > > Hi Li and Matan,
> > > > >
> > > > > Thank you for your proposal, some comments below.
> > > > >
> > > > > I am also adding Jerin and Hemant to this thread, as they also
> > participated
> > > in
> > > > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> > > interest in
> > > > a
> > > > > previous email.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Li Zhang <lizh@nvidia.com>
> > > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > > To: dekelp@nvidia.com; orika@nvidia.com;
> viacheslavo@nvidia.com;
> > > > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com;
> Singh,
> > > > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > Andrew
> > > > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu,
> Cristian
> > > > > > <cristian.dumitrescu@intel.com>
> > > > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> API
> > > > > >
> > > > > > Currently, the flow meter policy does not support multiple actions
> per
> > > > > > color; also the allowed action types per color are very limited.
> > > > > > In addition, the policy cannot be pre-defined.
> > > > > >
> > > > > > Due to the growing in flow actions offload abilities there is a
> > > > > > potential for the user to use variety of actions per color differently.
> > > > > > This new meter policy API comes to allow this potential in the most
> > > > > > ethdev common way using rte_flow action definition.
> > > > > > A list of rte_flow actions will be provided by the user per color in
> > > > > > order to create a meter policy.
> > > > > > In addition, the API forces to pre-define the policy before the
> meters
> > > > > > creation in order to allow sharing of single policy with multiple
> > > > > > meters efficiently.
> > > > > >
> > > > > > meter_policy_id is added into struct rte_mtr_params.
> > > > > > So that it can get the policy during the meters creation.
> > > > > >
> > > > > > Policy id 0 is default policy. Action per color as below:
> > > > > > green - no action, yellow - no action, red - drop
> > > > > >
> > > > > > Allow coloring the packet using a new rte_flow_action_color as
> could
> > > > > > be done by the old policy API,
> > > > > >
> > > > >
> > > > > The proposal essentially is to define the meter policy based on
> rte_flow
> > > > actions
> > > > > rather than a reduced action set defined specifically just for meter
> object.
> > > > This
> > > > > makes sense to me.
> > > > >
> > > > > > The next API function were added:
> > > > > > - rte_mtr_meter_policy_add
> > > > > > - rte_mtr_meter_policy_delete
> > > > > > - rte_mtr_meter_policy_update
> > > > > > - rte_mtr_meter_policy_validate
> > > > > > The next struct was changed:
> > > > > > - rte_mtr_params
> > > > > > - rte_mtr_capabilities
> > > > > > The next API was deleted:
> > > > > > - rte_mtr_policer_actions_update
> > > > > >
> > > > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > > > ---
> > > > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++----
> -----
> > > > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > > > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4
> 100644
> > > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > > @@ -31,6 +31,7 @@
> > > > > >  #include <rte_ecpri.h>
> > > > > >  #include <rte_mbuf.h>
> > > > > >  #include <rte_mbuf_dyn.h>
> > > > > > +#include <rte_meter.h>
> > > > > >
> > > > > >  #ifdef __cplusplus
> > > > > >  extern "C" {
> > > > > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > > > > >        * See struct rte_flow_action_modify_field.
> > > > > >        */
> > > > > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > > > > +
> > > > > > +     /**
> > > > > > +      * Color the packet to reflect the meter color result.
> > > > > > +      *
> > > > > > +      * See struct rte_flow_action_color.
> > > > > > +      */
> > > > > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> > > > >
> > > > > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> > > > >
> > >
> > > Why do we need this action?
> >
> > We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to
> set the
> > packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order
> to
> > tell the later stages of the pipeline what the packet color is.
> >
> > > if it is to save the color it should be done by using mark/metadata
> >
> > As stated in its description, the  RTE_FLOW_ACTION_TYPE_MARK action Is
> > setting the mbuf::hash.fdir.hi field, which is used for a different purpose
> that is
> > unrelated to the packet color, which has its own field within the mbuf.
> >
> 
> Agree,
> 
> > > Or by the action of meter.
> >
> > The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an
> action
> > of the meter and meter only, right?
> >
> > For example you can see
> > > RTE_FLOW_ACTION_TYPE_SECURITY
> > > Which if exist saves the session id to a dedicated mbuf field.
> > >
> >
> > The meter processing and action take place independently of the security
> API: it
> > can be enabled when the security API is disabled and is not conditioned in
> any
> > way by the security API. To be honest, I don't understand the connection
> with
> > the security API that you are trying to make here.
> >
> 
> Sorry for not being clear,
> I didn’t mean use the security action, what I meant is just like the security
> action
> which when given, it will saves the session and pass it to the SW in dedicated
> member in the
> mbuf, the same with meter if the meter action is present then
> the PMD should know to save the color value and extract it to the correct
> mbuf member.
> 
> Does that make sense?
> 

Yes, I does, I guess we are using the same principle for the proposed RTE_FLOW_ACTION_TYPE_COLOR action, right?

> 
> > > > > >  };
> > > > > >
> > > > > >  /**
> > >
> > > [Snip]
> > >
> > > > > I suggest you do not redundantly specify the value of the default
> policy ID
> > > in
> > > > the
> > > > > comment. Replace by "Default policy ID."
> > > > >
> > > > > > + * Action per color as below:
> > > > > > + * green - no action, yellow - no action, red - drop
> > > > >
> > > > > This does not make sense to me as the default policy. The default
> policy
> > > > should
> > > > > be "no change", i.e. green -> green (no change), yellow -> yellow (no
> > > change),
> > > > > red -> red (no change).
> > > >
> > > > Can you explain why it doesn't make sense to you?
> > > >
> > > > Meter with "no change" for all colors has no effect on the packets so it
> is
> > > > redundant action which just costs performance and resources -
> probably
> > > never
> > > > be used.
> > > >
> > > > The most common usage for meter is to drop all the packets come
> above
> > > the
> > > > defined rate limit - so it makes sense to take this behavior as default.
> > > >
> > > >
> > > > > I suggest we avoid the "no action" statement, as it might be
> confusing.
> > > >
> > > > Maybe "do nothing" is better?
> > > >
> > > Maybe passthrough? Or in rte_flow passthru
> > >
> >
> > No, we need to save the packet color in the packet mbuf
> (mbuf::sched:color),
> > and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this.
> >
> 
> Please see my comment above.
> The saving of color will be done automatically.

The "automatically" part might be the problem for some apps, like the one Matan is describing.

If for example all that user wants is to drop all the RED packets immediately with no need to consider the packet color later in the CPU app, then the user policy is simply: [GREEN => pass-through; YELLOW => pass-through; RED => drop] with no need to use the RTE_FLOW_ACTION_TYPE_COLOR action to set the color in the mbuf::sched::color field, as the app does not need to know the color. This is the use-case described by Matan.

If for example the user utilizes the packet color on the CPU app as the packet drop priority in case of congestion, then the user policy is simply: [GREEN => color; YELLOW => color; RED => color], so the RTE_FLOW_ACTION_TYPE_COLOR action is used to set the color in the mbuf::sched:color field. This is the use-case I described in earlier emails.

So it is probably the best option to have the user to explicitly set / not set the RTE_FLOW_ACTION_TYPE_COLOR action as opposed to have this action always executed automatically/implicitly.

Makes sense?

> 
> > >
> > > > > > + * It can be used without creating it by the
> rte_mtr_meter_policy_add
> > > > > > function.
> > > > > > + */
> > >
> > >
> > > Best,
> > > Ori
> >
> > Regards,
> > Cristian
> 
> Best,
> Ori

Regards,
Cristian
  

Hi Cristian,


> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> 
> Hi Ori,
> 
> > -----Original Message-----
> > From: Ori Kam <orika@nvidia.com>
> > Hi Cristian,
> >
> > > -----Original Message-----
> > > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Hi Ori,
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@nvidia.com>
> > > > Hi All,
> > > >
> > > > > -----Original Message-----
> > > > > From: Matan Azrad <matan@nvidia.com>
> > > > > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > API
> > > > >
> > > > > Hi Cristian
> > > > >
> > > > > Thank you for your important review!
> > > > > I agree with all your comments except one, please see inline.
> > > > >
> > > > > From: Dumitrescu, Cristian
> > > > > > Hi Li and Matan,
> > > > > >
> > > > > > Thank you for your proposal, some comments below.
> > > > > >
> > > > > > I am also adding Jerin and Hemant to this thread, as they also
> > > participated
> > > > in
> > > > > > the definition of the rte_mtr API in 2017. Also Ajit expressed some
> > > > interest in
> > > > > a
> > > > > > previous email.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Li Zhang <lizh@nvidia.com>
> > > > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > > > To: dekelp@nvidia.com; orika@nvidia.com;
> > viacheslavo@nvidia.com;
> > > > > > > matan@nvidia.com; shahafs@nvidia.com; lironh@marvell.com;
> > Singh,
> > > > > > > Jasvinder <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > > Andrew
> > > > > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu,
> > Cristian
> > > > > > > <cristian.dumitrescu@intel.com>
> > > > > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > API
> > > > > > >
> > > > > > > Currently, the flow meter policy does not support multiple actions
> > per
> > > > > > > color; also the allowed action types per color are very limited.
> > > > > > > In addition, the policy cannot be pre-defined.
> > > > > > >
> > > > > > > Due to the growing in flow actions offload abilities there is a
> > > > > > > potential for the user to use variety of actions per color differently.
> > > > > > > This new meter policy API comes to allow this potential in the most
> > > > > > > ethdev common way using rte_flow action definition.
> > > > > > > A list of rte_flow actions will be provided by the user per color in
> > > > > > > order to create a meter policy.
> > > > > > > In addition, the API forces to pre-define the policy before the
> > meters
> > > > > > > creation in order to allow sharing of single policy with multiple
> > > > > > > meters efficiently.
> > > > > > >
> > > > > > > meter_policy_id is added into struct rte_mtr_params.
> > > > > > > So that it can get the policy during the meters creation.
> > > > > > >
> > > > > > > Policy id 0 is default policy. Action per color as below:
> > > > > > > green - no action, yellow - no action, red - drop
> > > > > > >
> > > > > > > Allow coloring the packet using a new rte_flow_action_color as
> > could
> > > > > > > be done by the old policy API,
> > > > > > >
> > > > > >
> > > > > > The proposal essentially is to define the meter policy based on
> > rte_flow
> > > > > actions
> > > > > > rather than a reduced action set defined specifically just for meter
> > object.
> > > > > This
> > > > > > makes sense to me.
> > > > > >
> > > > > > > The next API function were added:
> > > > > > > - rte_mtr_meter_policy_add
> > > > > > > - rte_mtr_meter_policy_delete
> > > > > > > - rte_mtr_meter_policy_update
> > > > > > > - rte_mtr_meter_policy_validate
> > > > > > > The next struct was changed:
> > > > > > > - rte_mtr_params
> > > > > > > - rte_mtr_capabilities
> > > > > > > The next API was deleted:
> > > > > > > - rte_mtr_policer_actions_update
> > > > > > >
> > > > > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > > > > ---
> > > > > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > > > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > > > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++----
> > -----
> > > > > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > > > > >  4 files changed, 210 insertions(+), 74 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > > > b/lib/librte_ethdev/rte_flow.h index 669e677e91..5f38aa7fa4
> > 100644
> > > > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > > > @@ -31,6 +31,7 @@
> > > > > > >  #include <rte_ecpri.h>
> > > > > > >  #include <rte_mbuf.h>
> > > > > > >  #include <rte_mbuf_dyn.h>
> > > > > > > +#include <rte_meter.h>
> > > > > > >
> > > > > > >  #ifdef __cplusplus
> > > > > > >  extern "C" {
> > > > > > > @@ -2236,6 +2237,13 @@ enum rte_flow_action_type {
> > > > > > >        * See struct rte_flow_action_modify_field.
> > > > > > >        */
> > > > > > >       RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> > > > > > > +
> > > > > > > +     /**
> > > > > > > +      * Color the packet to reflect the meter color result.
> > > > > > > +      *
> > > > > > > +      * See struct rte_flow_action_color.
> > > > > > > +      */
> > > > > > > +     RTE_FLOW_ACTION_TYPE_COlOR,
> > > > > >
> > > > > > Typo here, it should be RTE_FLOW_ACTION_TYPE_COLOR.
> > > > > >
> > > >
> > > > Why do we need this action?
> > >
> > > We need this new proposed RTE_FLOW_ACTION_TYPE_COLOR action to
> > set the
> > > packet color in the packet mbuf (i.e. in the mbuf::sched:color field) in order
> > to
> > > tell the later stages of the pipeline what the packet color is.
> > >
> > > > if it is to save the color it should be done by using mark/metadata
> > >
> > > As stated in its description, the  RTE_FLOW_ACTION_TYPE_MARK action Is
> > > setting the mbuf::hash.fdir.hi field, which is used for a different purpose
> > that is
> > > unrelated to the packet color, which has its own field within the mbuf.
> > >
> >
> > Agree,
> >
> > > > Or by the action of meter.
> > >
> > > The new proposed RTE_FLOW_ACTION_TYPE_COLOR action is indeed an
> > action
> > > of the meter and meter only, right?
> > >
> > > For example you can see
> > > > RTE_FLOW_ACTION_TYPE_SECURITY
> > > > Which if exist saves the session id to a dedicated mbuf field.
> > > >
> > >
> > > The meter processing and action take place independently of the security
> > API: it
> > > can be enabled when the security API is disabled and is not conditioned in
> > any
> > > way by the security API. To be honest, I don't understand the connection
> > with
> > > the security API that you are trying to make here.
> > >
> >
> > Sorry for not being clear,
> > I didn’t mean use the security action, what I meant is just like the security
> > action
> > which when given, it will saves the session and pass it to the SW in dedicated
> > member in the
> > mbuf, the same with meter if the meter action is present then
> > the PMD should know to save the color value and extract it to the correct
> > mbuf member.
> >
> > Does that make sense?
> >
> 
> Yes, I does, I guess we are using the same principle for the proposed
> RTE_FLOW_ACTION_TYPE_COLOR action, right?
> 

Just to make sure we are talking about the same thing.
I don't think we should add any new action for this.
My idea is that when the PMD will see the action RTE_FLOW_ACTION_TYPE_METER,
it will send the traffic to the meter, and will also in the of case the packet being
routed to the application will update the color member in the mbuf with color value.

> >
> > > > > > >  };
> > > > > > >
> > > > > > >  /**
> > > >
> > > > [Snip]
> > > >
> > > > > > I suggest you do not redundantly specify the value of the default
> > policy ID
> > > > in
> > > > > the
> > > > > > comment. Replace by "Default policy ID."
> > > > > >
> > > > > > > + * Action per color as below:
> > > > > > > + * green - no action, yellow - no action, red - drop
> > > > > >
> > > > > > This does not make sense to me as the default policy. The default
> > policy
> > > > > should
> > > > > > be "no change", i.e. green -> green (no change), yellow -> yellow (no
> > > > change),
> > > > > > red -> red (no change).
> > > > >
> > > > > Can you explain why it doesn't make sense to you?
> > > > >
> > > > > Meter with "no change" for all colors has no effect on the packets so it
> > is
> > > > > redundant action which just costs performance and resources -
> > probably
> > > > never
> > > > > be used.
> > > > >
> > > > > The most common usage for meter is to drop all the packets come
> > above
> > > > the
> > > > > defined rate limit - so it makes sense to take this behavior as default.
> > > > >
> > > > >
> > > > > > I suggest we avoid the "no action" statement, as it might be
> > confusing.
> > > > >
> > > > > Maybe "do nothing" is better?
> > > > >
> > > > Maybe passthrough? Or in rte_flow passthru
> > > >
> > >
> > > No, we need to save the packet color in the packet mbuf
> > (mbuf::sched:color),
> > > and the RTE_FLOW_ACTION_TYPE_PASSTHRU action is not doing this.
> > >
> >
> > Please see my comment above.
> > The saving of color will be done automatically.
> 
> The "automatically" part might be the problem for some apps, like the one
> Matan is describing.
> 
> If for example all that user wants is to drop all the RED packets immediately
> with no need to consider the packet color later in the CPU app, then the user
> policy is simply: [GREEN => pass-through; YELLOW => pass-through; RED =>
> drop] with no need to use the RTE_FLOW_ACTION_TYPE_COLOR action to set
> the color in the mbuf::sched::color field, as the app does not need to know the
> color. This is the use-case described by Matan.
> 
> If for example the user utilizes the packet color on the CPU app as the packet
> drop priority in case of congestion, then the user policy is simply: [GREEN =>
> color; YELLOW => color; RED => color], so the
> RTE_FLOW_ACTION_TYPE_COLOR action is used to set the color in the
> mbuf::sched:color field. This is the use-case I described in earlier emails.
> 
> So it is probably the best option to have the user to explicitly set / not set the
> RTE_FLOW_ACTION_TYPE_COLOR action as opposed to have this action always
> executed automatically/implicitly.
> 
> Makes sense?
> 

I think I understand what you are saying but I don't agree,
First I assume that the meter color is dynamic field, if not the
answer will need some small change.
lets look at the use cases:
1. The first case of direct drop:
The PMD  sees that the dynamic color was not registered so it knows
not to same or update the field.
2. Second case the app register the color field and the pmd
knows that it needs to save it when meter action was set.

In any case the worst that will happen is that the PMD will store a value that
will not be checked.

Also adding a color action makes sense only if I can match on it later on, and for that
we have registers tags,mark and metadata.

Another approach is to totally remove the policy and just create mtr_color_item,
Then when application issues the meter action the color will be set to the color_item
Which the application will be able to match on. 
basically the application will create the policy using rte flow group with rules that matches
the color.

This is the way I really think it should be done, but we can move to this way later on.

 

> >
> > > >
> > > > > > > + * It can be used without creating it by the
> > rte_mtr_meter_policy_add
> > > > > > > function.
> > > > > > > + */
> > > >
> > > >
> > > > Best,
> > > > Ori
> > >
> > > Regards,
> > > Cristian
> >
> > Best,
> > Ori
> 
> Regards,
> Cristian

Best,
Ori
  

Hi Cristian

From: Dumitrescu, Cristian
> Hi Matan,
> 
> > -----Original Message-----
> > From: Matan Azrad <matan@nvidia.com>
> > Sent: Monday, March 29, 2021 9:44 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> > <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> > <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf
> > Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> > <jerinjacobk@gmail.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Ajit
> > Khaparde <ajit.khaparde@broadcom.com>
> > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar
> > Yanai <roniba@nvidia.com>
> > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter policy
> > API
> >
> >
> >
> > From: Dumitrescu, Cristian
> > > Hi Matan,
> > >
> > > > -----Original Message-----
> > > > From: Matan Azrad <matan@nvidia.com>
> > > > Sent: Thursday, March 25, 2021 6:57 AM
> > > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Li Zhang
> > > > <lizh@nvidia.com>; Dekel Peled <dekelp@nvidia.com>; Ori Kam
> > > > <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> > > > Shahaf Shuler <shahafs@nvidia.com>; lironh@marvell.com; Singh,
> > > > Jasvinder <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Andrew
> > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> > > > <jerinjacobk@gmail.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>;
> > > Ajit
> > > > Khaparde <ajit.khaparde@broadcom.com>
> > > > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar
> > > > Yanai <roniba@nvidia.com>
> > > > Subject: RE: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter
> > > > policy API
> > > >
> > > > Hi Cristian
> > > >
> > > > Thank you for your important review!
> > > > I agree with all your comments except one, please see inline.
> > > >
> > > > From: Dumitrescu, Cristian
> > > > > Hi Li and Matan,
> > > > >
> > > > > Thank you for your proposal, some comments below.
> > > > >
> > > > > I am also adding Jerin and Hemant to this thread, as they also
> > > > > participated
> > > > in
> > > > > the definition of the rte_mtr API in 2017. Also Ajit expressed
> > > > > some interest
> > > > in a
> > > > > previous email.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Li Zhang <lizh@nvidia.com>
> > > > > > Sent: Thursday, March 18, 2021 8:58 AM
> > > > > > To: dekelp@nvidia.com; orika@nvidia.com;
> > > > > > viacheslavo@nvidia.com; matan@nvidia.com; shahafs@nvidia.com;
> > > > > > lironh@marvell.com; Singh, Jasvinder
> > > > > > <jasvinder.singh@intel.com>; Thomas Monjalon
> > > > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > > Andrew
> > > > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Dumitrescu,
> > > > > > Cristian <cristian.dumitrescu@intel.com>
> > > > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > > > Subject: [PATCH 1/2] [RFC]: ethdev: add pre-defined meter
> > > > > > policy API
> > > > > >
> > > > > > Currently, the flow meter policy does not support multiple
> > > > > > actions per color; also the allowed action types per color are very
> limited.
> > > > > > In addition, the policy cannot be pre-defined.
> > > > > >
> > > > > > Due to the growing in flow actions offload abilities there is
> > > > > > a potential for the user to use variety of actions per color differently.
> > > > > > This new meter policy API comes to allow this potential in the
> > > > > > most ethdev common way using rte_flow action definition.
> > > > > > A list of rte_flow actions will be provided by the user per
> > > > > > color in order to create a meter policy.
> > > > > > In addition, the API forces to pre-define the policy before
> > > > > > the meters creation in order to allow sharing of single policy
> > > > > > with multiple meters efficiently.
> > > > > >
> > > > > > meter_policy_id is added into struct rte_mtr_params.
> > > > > > So that it can get the policy during the meters creation.
> > > > > >
> > > > > > Policy id 0 is default policy. Action per color as below:
> > > > > > green - no action, yellow - no action, red - drop
> > > > > >
> > > > > > Allow coloring the packet using a new rte_flow_action_color as
> > > > > > could be done by the old policy API,
> > > > > >
> > > > >
> > > > > The proposal essentially is to define the meter policy based on
> > > > > rte_flow
> > > > actions
> > > > > rather than a reduced action set defined specifically just for
> > > > > meter
> > object.
> > > > This
> > > > > makes sense to me.
> > > > >
> > > > > > The next API function were added:
> > > > > > - rte_mtr_meter_policy_add
> > > > > > - rte_mtr_meter_policy_delete
> > > > > > - rte_mtr_meter_policy_update
> > > > > > - rte_mtr_meter_policy_validate The next struct was changed:
> > > > > > - rte_mtr_params
> > > > > > - rte_mtr_capabilities
> > > > > > The next API was deleted:
> > > > > > - rte_mtr_policer_actions_update
> > > > > >
> > > > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > > > ---
> > > > > >  lib/librte_ethdev/rte_flow.h       |  18 ++++
> > > > > >  lib/librte_ethdev/rte_mtr.c        |  55 ++++++++--
> > > > > >  lib/librte_ethdev/rte_mtr.h        | 166 ++++++++++++++++++++-------
> > --
> > > > > >  lib/librte_ethdev/rte_mtr_driver.h |  45 ++++++--
> > > > > >  4 files changed, 210 insertions(
> > <snip>
> > > > > > +/**
> > > > > > + * Policy id 0 is default policy.
> > > > >
> > > > > I suggest you do not redundantly specify the value of the
> > > > > default policy ID
> > > > in the
> > > > > comment. Replace by "Default policy ID."
> > > > >
> > > > > > + * Action per color as below:
> > > > > > + * green - no action, yellow - no action, red - drop
> > > > >
> > > > > This does not make sense to me as the default policy. The
> > > > > default policy
> > > > should
> > > > > be "no change", i.e. green -> green (no change), yellow ->
> > > > > yellow (no
> > > > change),
> > > > > red -> red (no change).
> > > >
> > > > Can you explain why it doesn't make sense to you?
> > > >
> > > > Meter with "no change" for all colors has no effect on the packets
> > > > so it is redundant action which just costs performance and
> > > > resources - probably never be used.
> > > >
> > >
> > > The mbuf::sched::color needs to be set for the packet, and the only
> > > way to
> > do
> > > this is by applying the RTE_FLOW_ACTION_TYPE_COLOR Action, right? It
> > would
> > > make sense that the default policy is to simply apply to the packet
> > > the color that the meter just computed for the current packet with no
> change, right?
> >
> > I don't think so.
> > When we are working with HW offloads (this is the main goal of
> > rte_flow and this meter API) the motivation is to do the actions directly in the
> NIC HW.
> > Moving the color information to the SW is like doing "partial offload".
> >
> 
> Sorry, Matan, but as the bulk of the packets are passed by the NIC to the CPU as
> opposed to being returned to the network without the CPU ever seeing them,
> the rte_flow API is a partial offload API, not a full offload API, not sure why we
> disagree here.
> 

I said that the direction is to do "full offload", for sure "partial offload" is exists too, using MARK, METADATA, etc.. actions.
Example for "full offload" is all the hairpin features, added lastly.


> Just to make sure we are on the same page and not getting round in circles: I
> think you agree that we should have this action
> RTE_FLOW_ACTION_TYPE_COLOR to setup the color for the CPU to see in
> mbuf::sched:color, but you don't agree this action should be part of the default
> policy, did I get your position correctly?

Yes.

> >
> > > > The most common usage for meter is to drop all the packets come
> > > > above the defined rate limit - so it makes sense to take this behavior as
> default.
> > > >
> > >
> > > I don't agree with this assertion either. One typical usage of the
> > > color is to accept all input packets from the user, either green,
> > > yellow or red in the absence of any congestion, and charge the user
> > > for this traffic; in case of congestion, as typically detected later
> > > (typically on scheduling and maybe
> > on a
> > > different network node, depending on the application), the packet
> > > color is
> > used
> > > to prioritize between packets, i.e. drop red packets first before
> > > dropping
> > any
> > > yellow or green packets. In this case, there is no pre-defined "drop
> > > all red packets straight away" policy.
> >
> >
> > I familiar with a lot of meter users(at least 5 applications) in the
> > industry, no one use the color actions.
> > All of them drop red packets and continue to the next flow
> > actions(after
> > meter) otherwise.
> >
> 
> I politely but firmly disagree. None of the apps that I have seen is dropping the
> red packets straight away, they simply use the color as an indication of the
> packet drop priority at a later stage in the pipeline when congestion is
> detected.
> 
> >
> > If you insist, we can define 2 default IDs...
> >
> 
> Maybe we should not have any pre-registered policies at all?
> 

OK, for that.

> For the user's convenience, we could provide the configuration parameters for
> some of the common policies, like the ones mentioned here, in the API and let
> the users decide which one(s), if any, they want to register?

Good direction
I think it is better to give these 2 examples in the policy create\validate description or in example application,
Then the user just can copy it to create a policy....

> > > >
> > > > > I suggest we avoid the "no action" statement, as it might be confusing.
> > > >
> > > > Maybe "do nothing" is better?
> > > >
> > >
> > > Yes, makes sense to me.
> >
> > <snip>
> 
> Regards,
> Cristian
  

On Thu, 18 Mar 2021 10:58:14 +0200
Li Zhang <lizh@nvidia.com> wrote:

> +
> +	/**
> +	 * Color the packet to reflect the meter color result.
> +	 *
> +	 * See struct rte_flow_action_color.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_COlOR,

Why the odd use of lower case here? Shouldn't it be RTE_FLOW_ACTION_TYPE_COLOR
  

Hi Stephen

Thanks your comments.
We already change it in the following series and please take a look:
https://patchwork.dpdk.org/project/dpdk/list/?series=16524
+	/**
+	 * Color the packet to reflect the meter color result.
+	 * Set the meter color in the mbuf to the selected color.
+	 *
+	 * See struct rte_flow_action_meter_color.
+	 */
+	RTE_FLOW_ACTION_TYPE_METER_COLOR,

Regards,
Li Zhang
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, April 21, 2021 1:56 AM
> To: Li Zhang <lizh@nvidia.com>
> Cc: dekelp@nvidia.com; Ori Kam <orika@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; lironh@marvell.com;
> jasvinder.singh@intel.com; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Cristian Dumitrescu
> <cristian.dumitrescu@intel.com>; dev@dpdk.org; Raslan Darawsheh
> <rasland@nvidia.com>; Roni Bar Yanai <roniba@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] [RFC]: ethdev: add pre-defined meter
> policy API
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 18 Mar 2021 10:58:14 +0200
> Li Zhang <lizh@nvidia.com> wrote:
> 
> > +
> > +     /**
> > +      * Color the packet to reflect the meter color result.
> > +      *
> > +      * See struct rte_flow_action_color.
> > +      */
> > +     RTE_FLOW_ACTION_TYPE_COlOR,
> 
> Why the odd use of lower case here? Shouldn't it be
> RTE_FLOW_ACTION_TYPE_COLOR