[2/2,RFC] : ethdev: manage meter API object handles by the drivers

  Currently, all the meter objects are managed by the user IDs:
meter, profile and policy.
Hence, each PMD should manage data-structure in order to
map each API ID to the private PMD management structure.

From the application side, it has all the picture how meter
is going to be assigned to flows and can easily use direct
mapping even when the meter handler is provided by the PMDs.

Also, this is the approach of the rte_flow API handles:
the flow handle and the shared action handle
is provided by the PMDs.

Use drivers handlers in order to manage all the meter API objects.

The following API will be changed:
- rte_mtr_meter_profile_add
- rte_mtr_meter_profile_delete
- rte_mtr_meter_policy_validate
- rte_mtr_meter_policy_add
- rte_mtr_meter_policy_delete
- rte_mtr_create
- rte_mtr_destroy
- rte_mtr_meter_disable
- rte_mtr_meter_enable
- rte_mtr_meter_profile_update
- rte_mtr_meter_policy_update
- rte_mtr_meter_dscp_table_update
- rte_mtr_stats_update
- rte_mtr_stats_read
The next struct will be changed:
- rte_flow_action_meter
- rte_mtr_params

Signed-off-by: Li Zhang <lizh@nvidia.com>
---
 lib/librte_ethdev/rte_flow.h       |   9 ++-
 lib/librte_ethdev/rte_mtr.c        |  77 ++++++++++++----------
 lib/librte_ethdev/rte_mtr.h        | 102 +++++++++++++++--------------
 lib/librte_ethdev/rte_mtr_driver.h |  36 +++++-----
 4 files changed, 122 insertions(+), 102 deletions(-)
  

Hi Li and Matan,

> -----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 2/2] [RFC]: ethdev: manage meter API object handles by the
> drivers
> 
> Currently, all the meter objects are managed by the user IDs:
> meter, profile and policy.
> Hence, each PMD should manage data-structure in order to
> map each API ID to the private PMD management structure.
> 
> From the application side, it has all the picture how meter
> is going to be assigned to flows and can easily use direct
> mapping even when the meter handler is provided by the PMDs.
> 
> Also, this is the approach of the rte_flow API handles:
> the flow handle and the shared action handle
> is provided by the PMDs.
> 
> Use drivers handlers in order to manage all the meter API objects.
> 

This seems to be take 2 of the discussion that we already had  in this thread: https://mails.dpdk.org/archives/dev/2021-March/200710.html, so apologies for mostly summarizing my previous feedback here.

I am against this proposal because:
1. We already discussed this topic of user-provided handles vs. driver-provided handles at length on this exact email list back in 2017, when we first introduced this API, and I don't see any real reason to revisit the decision we took then.
2. For me, it is more natural and it also helps the application to simplify its data structures if the user provides its own IDs rather than the user having to deal with the IDs provided by the driver.
3. It is much easier and portable to pass numeric and string-based IDs around (e.g. between processes) as opposed to pointer-based IDs, as pointers are only valid in one address space and not in others. There are several DPDK APIs that moved away from pointer handles to string IDs.
4. The mapping of user IDs to internal pointers within the driver is IMO not a big issue in terms of memory footprint or API call rate. Matan also confirmed this in the above thread when saying tis is not about either driver memory footprint or API call speed, as this mapping is easy to optimize.

And last but not least, this change obviously propagates in every API function, so it would result in big churn in API, all drivers and all apps (including testpmd, etc) implementing it (for IMO no real benefit). Yes, this API is experimental and therefore we can operate changes in it, but I'd rather see incremental and converging improvements rather than this.

If you guys insist with this proposal, I would like to get more opinions from other vendors and contributors from within our DPDK community.

> The following API will be changed:
> - rte_mtr_meter_profile_add
> - rte_mtr_meter_profile_delete
> - rte_mtr_meter_policy_validate
> - rte_mtr_meter_policy_add
> - rte_mtr_meter_policy_delete
> - rte_mtr_create
> - rte_mtr_destroy
> - rte_mtr_meter_disable
> - rte_mtr_meter_enable
> - rte_mtr_meter_profile_update
> - rte_mtr_meter_policy_update
> - rte_mtr_meter_dscp_table_update
> - rte_mtr_stats_update
> - rte_mtr_stats_read
> The next struct will be changed:
> - rte_flow_action_meter
> - rte_mtr_params
> 
> Signed-off-by: Li Zhang <lizh@nvidia.com>
> ---
>  lib/librte_ethdev/rte_flow.h       |   9 ++-
>  lib/librte_ethdev/rte_mtr.c        |  77 ++++++++++++----------
>  lib/librte_ethdev/rte_mtr.h        | 102 +++++++++++++++--------------
>  lib/librte_ethdev/rte_mtr_driver.h |  36 +++++-----
>  4 files changed, 122 insertions(+), 102 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 5f38aa7fa4..6d2b86592d 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2480,6 +2480,13 @@ struct rte_flow_action_port_id {
>  	uint32_t id; /**< DPDK port ID. */
>  };
> 
> +/**
> + * Opaque type returned after successfully creating a meter.
> + *
> + * This handle can be used to manage the related meter (e.g. to destroy it).
> + */
> +struct rte_mtr;
> +
>  /**
>   * RTE_FLOW_ACTION_TYPE_METER
>   *
> @@ -2489,7 +2496,7 @@ struct rte_flow_action_port_id {
>   * next item with their color set by the MTR object.
>   */
>  struct rte_flow_action_meter {
> -	uint32_t mtr_id; /**< MTR object ID created with rte_mtr_create().
> */
> +	struct rte_mtr *mtr; /**< MTR object created with rte_mtr_create().
> */
>  };
> 
>  /**
> diff --git a/lib/librte_ethdev/rte_mtr.c b/lib/librte_ethdev/rte_mtr.c
> index fccec3760b..e407c6f956 100644
> --- a/lib/librte_ethdev/rte_mtr.c
> +++ b/lib/librte_ethdev/rte_mtr.c
> @@ -57,6 +57,19 @@ rte_mtr_ops_get(uint16_t port_id, struct
> rte_mtr_error *error)
>  	ops->func;					\
>  })
> 
> +#define RTE_MTR_FUNC_PTR(port_id, func)			\
> +({							\
> +	const struct rte_mtr_ops *ops =			\
> +		rte_mtr_ops_get(port_id, error);	\
> +	if (ops == NULL)				\
> +		return NULL;				\
> +							\
> +	if (ops->func == NULL)				\
> +		return NULL;				\
> +							\
> +	ops->func;					\
> +})
> +
>  /* MTR capabilities get */
>  int
>  rte_mtr_capabilities_get(uint16_t port_id,
> @@ -69,26 +82,25 @@ rte_mtr_capabilities_get(uint16_t port_id,
>  }
> 
>  /* MTR meter profile add */
> -int
> +struct rte_mtr_profile *
>  rte_mtr_meter_profile_add(uint16_t port_id,
> -	uint32_t meter_profile_id,
>  	struct rte_mtr_meter_profile *profile,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> -	return RTE_MTR_FUNC(port_id, meter_profile_add)(dev,
> -		meter_profile_id, profile, error);
> +	return RTE_MTR_FUNC_PTR(port_id, meter_profile_add)(dev,
> +		profile, error);
>  }
> 
>  /** MTR meter profile delete */
>  int
>  rte_mtr_meter_profile_delete(uint16_t port_id,
> -	uint32_t meter_profile_id,
> +	struct rte_mtr_profile *profile,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, meter_profile_delete)(dev,
> -		meter_profile_id, error);
> +		profile, error);
>  }
> 
>  /* MTR meter policy validate */
> @@ -103,126 +115,123 @@ rte_mtr_meter_policy_validate(uint16_t port_id,
>  }
> 
>  /* MTR meter policy add */
> -int
> +struct rte_mtr_policy *
>  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);
> +	return RTE_MTR_FUNC_PTR(port_id, meter_policy_add)(dev,
> +		actions, error);
>  }
> 
>  /** MTR meter policy delete */
>  int
>  rte_mtr_meter_policy_delete(uint16_t port_id,
> -	uint32_t policy_id,
> +	struct rte_mtr_policy *policy,
>  	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);
> +		policy, error);
>  }
> 
>  /** MTR object create */
> -int
> +struct rte_mtr *
>  rte_mtr_create(uint16_t port_id,
> -	uint32_t mtr_id,
>  	struct rte_mtr_params *params,
>  	int shared,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> -	return RTE_MTR_FUNC(port_id, create)(dev,
> -		mtr_id, params, shared, error);
> +	return RTE_MTR_FUNC_PTR(port_id, create)(dev, params, shared,
> error);
>  }
> 
>  /** MTR object destroy */
>  int
>  rte_mtr_destroy(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, destroy)(dev,
> -		mtr_id, error);
> +		mtr, error);
>  }
> 
>  /** MTR object meter enable */
>  int
>  rte_mtr_meter_enable(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, meter_enable)(dev,
> -		mtr_id, error);
> +		mtr, error);
>  }
> 
>  /** MTR object meter disable */
>  int
>  rte_mtr_meter_disable(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, meter_disable)(dev,
> -		mtr_id, error);
> +		mtr, error);
>  }
> 
>  /** MTR object meter profile update */
>  int
>  rte_mtr_meter_profile_update(uint16_t port_id,
> -	uint32_t mtr_id,
> -	uint32_t meter_profile_id,
> +	struct rte_mtr *mtr,
> +	struct rte_mtr_profile *profile,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, meter_profile_update)(dev,
> -		mtr_id, meter_profile_id, error);
> +		mtr, profile, error);
>  }
> 
>  /** MTR object meter policy update */
>  int
>  rte_mtr_meter_policy_update(uint16_t port_id,
> -	uint32_t mtr_id,
> -	uint32_t meter_policy_id,
> +	struct rte_mtr *mtr,
> +	struct rte_mtr_policy *policy,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, meter_policy_update)(dev,
> -		mtr_id, meter_policy_id, error);
> +		mtr, policy, error);
>  }
> 
>  /** MTR object meter DSCP table update */
>  int
>  rte_mtr_meter_dscp_table_update(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	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, meter_dscp_table_update)(dev,
> -		mtr_id, dscp_table, error);
> +		mtr, dscp_table, error);
>  }
> 
>  /** MTR object enabled stats update */
>  int
>  rte_mtr_stats_update(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	uint64_t stats_mask,
>  	struct rte_mtr_error *error)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, stats_update)(dev,
> -		mtr_id, stats_mask, error);
> +		mtr, stats_mask, error);
>  }
> 
>  /** MTR object stats read */
>  int
>  rte_mtr_stats_read(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_stats *stats,
>  	uint64_t *stats_mask,
>  	int clear,
> @@ -230,5 +239,5 @@ rte_mtr_stats_read(uint16_t port_id,
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	return RTE_MTR_FUNC(port_id, stats_read)(dev,
> -		mtr_id, stats, stats_mask, clear, error);
> +		mtr, stats, stats_mask, clear, error);
>  }
> diff --git a/lib/librte_ethdev/rte_mtr.h b/lib/librte_ethdev/rte_mtr.h
> index 07961f2777..2b20e55079 100644
> --- a/lib/librte_ethdev/rte_mtr.h
> +++ b/lib/librte_ethdev/rte_mtr.h
> @@ -181,8 +181,8 @@ struct rte_mtr_meter_profile {
>   * @see enum rte_mtr_stats_type
>   */
>  struct rte_mtr_params {
> -	/** Meter profile ID. */
> -	uint32_t meter_profile_id;
> +	/** Meter profile. */
> +	struct rte_mtr_profile *profile;
> 
>  	/** Meter input color in case of MTR object chaining. When non-
> zero: if
>  	 * a previous MTR object is enabled in the same flow, then the color
> @@ -221,8 +221,8 @@ struct rte_mtr_params {
>  	 */
>  	uint64_t stats_mask;
> 
> -	/** Meter policy ID. */
> -	uint32_t meter_policy_id;
> +	/** Meter policy. */
> +	struct rte_mtr_policy *policy;
>  };
> 
>  /**
> @@ -395,28 +395,32 @@ rte_mtr_capabilities_get(uint16_t port_id,
>  	struct rte_mtr_capabilities *cap,
>  	struct rte_mtr_error *error);
> 
> +/**
> + * Opaque type returned after successfully creating a profile.
> + *
> + * This handle can be used to manage the related profile (e.g. to destroy it).
> + */
> +struct rte_mtr_profile;
> +
>  /**
>   * Meter profile add
>   *
> - * Create a new meter profile with ID set to *meter_profile_id*. The new
> profile
> + * Create a new meter profile. The new profile
>   * is used to create one or several MTR objects.
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
> - * @param[in] meter_profile_id
> - *   ID for the new meter profile. Needs to be unused by any of the existing
> - *   meter profiles added for the current port.
>   * @param[in] profile
>   *   Meter profile parameters. Needs to be pre-allocated and valid.
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> - *   0 on success, non-zero error code otherwise.
> + *   A valid handle in case of success, NULL otherwise and rte_errno is set
> + *   to the positive version of one of the error codes.
>   */
>  __rte_experimental
> -int
> +struct rte_mtr_profile *
>  rte_mtr_meter_profile_add(uint16_t port_id,
> -	uint32_t meter_profile_id,
>  	struct rte_mtr_meter_profile *profile,
>  	struct rte_mtr_error *error);
> 
> @@ -428,8 +432,8 @@ rte_mtr_meter_profile_add(uint16_t port_id,
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
> - * @param[in] meter_profile_id
> - *   Meter profile ID. Needs to be the valid.
> + * @param[in] profile
> + *   Meter profile pointer. Needs to be the valid.
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> @@ -438,16 +442,15 @@ rte_mtr_meter_profile_add(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_profile_delete(uint16_t port_id,
> -	uint32_t meter_profile_id,
> +	struct rte_mtr_profile *profile,
>  	struct rte_mtr_error *error);
> 
>  /**
> - * Policy id 0 is default policy.
> - * Action per color as below:
> - * green - no action, yellow - no action, red - drop
> - * It can be used without creating it by the rte_mtr_meter_policy_add
> function.
> + * Opaque type returned after successfully creating a policy.
> + *
> + * This handle can be used to manage the related policy (e.g. to destroy it).
>   */
> -#define RTE_MTR_DEFAULT_POLICY_ID 0
> +struct rte_mtr_policy;
> 
>  /**
>   * Check whether a meter policy can be created on a given port.
> @@ -478,7 +481,6 @@ rte_mtr_meter_profile_delete(uint16_t port_id,
>  __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);
> 
> @@ -490,8 +492,6 @@ rte_mtr_meter_policy_validate(uint16_t port_id,
>   *
>   * @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.
> @@ -499,12 +499,12 @@ rte_mtr_meter_policy_validate(uint16_t port_id,
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> - *   0 on success, non-zero error code otherwise.
> + *   A valid handle in case of success, NULL otherwise and rte_errno is set
> + *   to the positive version of one of the error codes.
>   */
>  __rte_experimental
> -int
> +struct rte_mtr_policy *
>  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);
> 
> @@ -516,8 +516,8 @@ rte_mtr_meter_policy_add(uint16_t port_id,
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
> - * @param[in] policy_id
> - *   Policy identifier.
> + * @param[in] policy
> + *   Policy pointer. Needs to be valid.
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> @@ -526,20 +526,28 @@ rte_mtr_meter_policy_add(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_policy_delete(uint16_t port_id,
> -	uint32_t policy_id,
> +	struct rte_mtr_policy *policy,
>  	struct rte_mtr_error *error);
> 
> +/**
> + * Opaque type returned after successfully creating a meter.
> + *
> + * This handle can be used to manage the related meter (e.g. to destroy it).
> + */
> +struct rte_mtr;
> +
>  /**
>   * MTR object create
>   *
>   * Create a new MTR object for the current port. This object is run as part of
>   * associated flow action for traffic metering and policing.
> + * Policy pointer NULL is default policy.
> + * Action per color as below:
> + * green - no action, yellow - no action, red - drop
> + * It can be used without creating it by the rte_mtr_meter_policy_add
> function.
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
> - * @param[in] mtr_id
> - *   MTR object ID. Needs to be unused by any of the existing MTR objects.
> - *   created for the current port.
>   * @param[in] params
>   *   MTR object params. Needs to be pre-allocated and valid.
>   * @param[in] shared
> @@ -548,14 +556,14 @@ rte_mtr_meter_policy_delete(uint16_t port_id,
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
>   * @return
> - *   0 on success, non-zero error code otherwise.
> + *   A valid handle in case of success, NULL otherwise and rte_errno is set
> + *   to the positive version of one of the error codes.
>   *
>   * @see enum rte_flow_action_type::RTE_FLOW_ACTION_TYPE_METER
>   */
>  __rte_experimental
> -int
> +struct rte_mtr *
>  rte_mtr_create(uint16_t port_id,
> -	uint32_t mtr_id,
>  	struct rte_mtr_params *params,
>  	int shared,
>  	struct rte_mtr_error *error);
> @@ -568,8 +576,8 @@ rte_mtr_create(uint16_t port_id,
>   *
>   * @param[in] port_id
>   *   The port identifier of the Ethernet device.
> - * @param[in] mtr_id
> - *   MTR object ID. Needs to be valid.
> + * @param[in] mtr
> + *   MTR pointer. Needs to be valid.
>   *   created for the current port.
>   * @param[out] error
>   *   Error details. Filled in only on error, when not NULL.
> @@ -579,7 +587,7 @@ rte_mtr_create(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_destroy(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error);
> 
>  /**
> @@ -607,7 +615,7 @@ rte_mtr_destroy(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_disable(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error);
> 
>  /**
> @@ -629,7 +637,7 @@ rte_mtr_meter_disable(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_enable(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error);
> 
>  /**
> @@ -649,8 +657,8 @@ rte_mtr_meter_enable(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_profile_update(uint16_t port_id,
> -	uint32_t mtr_id,
> -	uint32_t meter_profile_id,
> +	struct rte_mtr *mtr,
> +	struct rte_mtr_profile *profile,
>  	struct rte_mtr_error *error);
> 
>  /**
> @@ -660,8 +668,6 @@ rte_mtr_meter_profile_update(uint16_t port_id,
>   *   The port identifier of the Ethernet device.
>   * @param[in] mtr_id
>   *   MTR object ID. Needs to be valid.
> - * @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
> @@ -670,8 +676,8 @@ rte_mtr_meter_profile_update(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_policy_update(uint16_t port_id,
> -	uint32_t mtr_id,
> -	uint32_t meter_policy_id,
> +	struct rte_mtr *mtr,
> +	struct rte_mtr_policy *policy,
>  	struct rte_mtr_error *error);
> 
>  /**
> @@ -695,7 +701,7 @@ rte_mtr_meter_policy_update(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_meter_dscp_table_update(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error);
> 
> @@ -720,7 +726,7 @@ rte_mtr_meter_dscp_table_update(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_stats_update(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	uint64_t stats_mask,
>  	struct rte_mtr_error *error);
> 
> @@ -752,7 +758,7 @@ rte_mtr_stats_update(uint16_t port_id,
>  __rte_experimental
>  int
>  rte_mtr_stats_read(uint16_t port_id,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_stats *stats,
>  	uint64_t *stats_mask,
>  	int clear,
> diff --git a/lib/librte_ethdev/rte_mtr_driver.h
> b/lib/librte_ethdev/rte_mtr_driver.h
> index 1ad8fb4c40..d7a8853b51 100644
> --- a/lib/librte_ethdev/rte_mtr_driver.h
> +++ b/lib/librte_ethdev/rte_mtr_driver.h
> @@ -30,82 +30,80 @@ typedef int (*rte_mtr_capabilities_get_t)(struct
> rte_eth_dev *dev,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR capabilities get */
> 
> -typedef int (*rte_mtr_meter_profile_add_t)(struct rte_eth_dev *dev,
> -	uint32_t meter_profile_id,
> +typedef struct rte_mtr_profile *(*rte_mtr_meter_profile_add_t)
> +	(struct rte_eth_dev *dev,
>  	struct rte_mtr_meter_profile *profile,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR meter profile add */
> 
>  typedef int (*rte_mtr_meter_profile_delete_t)(struct rte_eth_dev *dev,
> -	uint32_t meter_profile_id,
> +	struct rte_mtr_profile *profile,
>  	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,
> +typedef struct rte_mtr_policy *(*rte_mtr_meter_policy_add_t)
> +	(struct rte_eth_dev *dev,
>  	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_policy *policy,
>  	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,
> +typedef struct rte_mtr *(*rte_mtr_create_t)(struct rte_eth_dev *dev,
>  	struct rte_mtr_params *params,
>  	int shared,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object create */
> 
>  typedef int (*rte_mtr_destroy_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object destroy */
> 
>  typedef int (*rte_mtr_meter_enable_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object meter enable */
> 
>  typedef int (*rte_mtr_meter_disable_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object meter disable */
> 
>  typedef int (*rte_mtr_meter_profile_update_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> -	uint32_t meter_profile_id,
> +	struct rte_mtr *mtr,
> +	struct rte_mtr_profile *profile,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object meter profile update */
> 
>  typedef int (*rte_mtr_meter_policy_update_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> -	uint32_t meter_policy_id,
> +	struct rte_mtr *mtr,
> +	struct rte_mtr_policy *policy,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object meter policy update */
> 
>  typedef int (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev
> *dev,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object meter DSCP table update */
> 
>  typedef int (*rte_mtr_stats_update_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	uint64_t stats_mask,
>  	struct rte_mtr_error *error);
>  /**< @internal MTR object enabled stats update */
> 
>  typedef int (*rte_mtr_stats_read_t)(struct rte_eth_dev *dev,
> -	uint32_t mtr_id,
> +	struct rte_mtr *mtr,
>  	struct rte_mtr_stats *stats,
>  	uint64_t *stats_mask,
>  	int clear,
> --
> 2.21.0

Regards,
Cristian
  

Hi Cristian

From: Dumitrescu, Cristian
> Hi Li and Matan,
> 
> > -----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 2/2] [RFC]: ethdev: manage meter API object handles by
> > the drivers
> >
> > Currently, all the meter objects are managed by the user IDs:
> > meter, profile and policy.
> > Hence, each PMD should manage data-structure in order to map each API
> > ID to the private PMD management structure.
> >
> > From the application side, it has all the picture how meter is going
> > to be assigned to flows and can easily use direct mapping even when
> > the meter handler is provided by the PMDs.
> >
> > Also, this is the approach of the rte_flow API handles:
> > the flow handle and the shared action handle is provided by the PMDs.
> >
> > Use drivers handlers in order to manage all the meter API objects.
> >
> 
> This seems to be take 2 of the discussion that we already had  in this thread:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmails.dp
> dk.org%2Farchives%2Fdev%2F2021-
> March%2F200710.html&amp;data=04%7C01%7Cmatan%40nvidia.com%7Cab0
> e3cc77b9e4101344e08d8ee434bbe%7C43083d15727340c1b7db39efd9ccc17a%
> 7C0%7C0%7C637521320105450617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> amp;sdata=94bFRICfGEzk5s53MRUvFMQe5ZlhP2Tmnu82hwUytc4%3D&amp;re
> served=0, so apologies for mostly summarizing my previous feedback here.
> 
> I am against this proposal because:
> 1. We already discussed this topic of user-provided handles vs. driver-provided
> handles at length on this exact email list back in 2017, when we first introduced
> this API, and I don't see any real reason to revisit the decision we took then.

Why not?
There is more experiences\usages now.
New drivers added the support and also now scalability is growing and growing....


> 2. For me, it is more natural and it also helps the application to simplify its data
> structures if the user provides its own IDs rather than the user having to deal
> with the IDs provided by the driver.

Generally I don't think other flow DPDK APIs align with your feelings here, see rte_flow object and rte_flow_shared_action.

Specifically for meter:
	- here, meter is HW\driver offload where performance\rate either for meter creation\deletion or for the actual data-path is very important especially when we talk on very big numbers, so "natural" has less importance here.
	  We need to think on the global solution for application ->API->driver. in meter feature, the user has the ability to manage the IDs better than the PMDs for the most of the use-cases:
			1. meter per flow: just save the driver handle in the app flow context.
			2. meter per VM\USER flows\rte_flow group\any other context grouped multiple flows: just save the driver handle in the app context.
	If PMD need to map the IDs, it is more complex for sure, requires more memory and more lookup time.

	- I'm not sure it is natural for all the use-cases, sometimes generating unique ID may complex the app.


> 3. It is much easier and portable to pass numeric and string-based IDs around
> (e.g. between processes) as opposed to pointer-based IDs, as pointers are only
> valid in one address space and not in others. There are several DPDK APIs that
> moved away from pointer handles to string IDs.

Yes, I agree here generally.
But again, since meter is used only by rte_flow, it is better to align the same handle mechanism.

> 4. The mapping of user IDs to internal pointers within the driver is IMO not a
> big issue in terms of memory footprint or API call rate. Matan also confirmed
> this in the above thread when saying tis is not about either driver memory
> footprint or API call speed, as this mapping is easy to optimize.

Yes, it is not very big deal, but still costs more than the new suggestion, especially in big scale.

> And last but not least, this change obviously propagates in every API function,
> so it would result in big churn in API, all drivers and all apps (including testpmd,
> etc) implementing it (for IMO no real benefit). Yes, this API is experimental and
> therefore we can operate changes in it, but I'd rather see incremental and
> converging improvements rather than this.

Yes, it changes all API, but very small part in each, will be very easy to align all the current dpdk components to use this concept. 

> If you guys insist with this proposal, I would like to get more opinions from
> other vendors and contributors from within our DPDK community.


Yes, more opinions are very welcomed.

Thanks
  

On Thu, Mar 25, 2021 at 1:21 AM Matan Azrad <matan@nvidia.com> wrote:
>
> Hi Cristian
>
> From: Dumitrescu, Cristian
> > Hi Li and Matan,
> >
> > > -----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 2/2] [RFC]: ethdev: manage meter API object handles by
> > > the drivers
> > >
> > > Currently, all the meter objects are managed by the user IDs:
> > > meter, profile and policy.
> > > Hence, each PMD should manage data-structure in order to map each API
> > > ID to the private PMD management structure.
> > >
> > > From the application side, it has all the picture how meter is going
> > > to be assigned to flows and can easily use direct mapping even when
> > > the meter handler is provided by the PMDs.
> > >
> > > Also, this is the approach of the rte_flow API handles:
> > > the flow handle and the shared action handle is provided by the PMDs.
> > >
> > > Use drivers handlers in order to manage all the meter API objects.
> > >
> >
> > This seems to be take 2 of the discussion that we already had  in this thread:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmails.dp
> > dk.org%2Farchives%2Fdev%2F2021-
> > March%2F200710.html&amp;data=04%7C01%7Cmatan%40nvidia.com%7Cab0
> > e3cc77b9e4101344e08d8ee434bbe%7C43083d15727340c1b7db39efd9ccc17a%
> > 7C0%7C0%7C637521320105450617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
> > C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> > amp;sdata=94bFRICfGEzk5s53MRUvFMQe5ZlhP2Tmnu82hwUytc4%3D&amp;re
> > served=0, so apologies for mostly summarizing my previous feedback here.
> >
> > I am against this proposal because:
> > 1. We already discussed this topic of user-provided handles vs. driver-provided
> > handles at length on this exact email list back in 2017, when we first introduced
> > this API, and I don't see any real reason to revisit the decision we took then.
>
> Why not?
> There is more experiences\usages now.
> New drivers added the support and also now scalability is growing and growing....
>
>
> > 2. For me, it is more natural and it also helps the application to simplify its data
> > structures if the user provides its own IDs rather than the user having to deal
> > with the IDs provided by the driver.
>
> Generally I don't think other flow DPDK APIs align with your feelings here, see rte_flow object and rte_flow_shared_action.
>
> Specifically for meter:
>         - here, meter is HW\driver offload where performance\rate either for meter creation\deletion or for the actual data-path is very important especially when we talk on very big numbers, so "natural" has less importance here.
>           We need to think on the global solution for application ->API->driver. in meter feature, the user has the ability to manage the IDs better than the PMDs for the most of the use-cases:
>                         1. meter per flow: just save the driver handle in the app flow context.
>                         2. meter per VM\USER flows\rte_flow group\any other context grouped multiple flows: just save the driver handle in the app context.
>         If PMD need to map the IDs, it is more complex for sure, requires more memory and more lookup time.
>
>         - I'm not sure it is natural for all the use-cases, sometimes generating unique ID may complex the app.
>
>
> > 3. It is much easier and portable to pass numeric and string-based IDs around
> > (e.g. between processes) as opposed to pointer-based IDs, as pointers are only
> > valid in one address space and not in others. There are several DPDK APIs that
> > moved away from pointer handles to string IDs.
Pardon my ignorance..
But which DPDK APIs moved to string IDs from pointer handles?

>
> Yes, I agree here generally.
> But again, since meter is used only by rte_flow, it is better to align the same handle mechanism.
I don't want to say - do this because rte_flow uses a pointer.
I don't have a strong opinion for one over the other. In the end the
logic can be adapted one way or the other.
But having implemented rte_flow support in the PMD, I think it is a
good idea to avoid the duplication of meter_id to pointer based handle
conversion and bookkeeping logic in the application and the PMD.

>
> > 4. The mapping of user IDs to internal pointers within the driver is IMO not a
> > big issue in terms of memory footprint or API call rate. Matan also confirmed
> > this in the above thread when saying tis is not about either driver memory
> > footprint or API call speed, as this mapping is easy to optimize.
>
> Yes, it is not very big deal, but still costs more than the new suggestion, especially in big scale.
>
> > And last but not least, this change obviously propagates in every API function,
> > so it would result in big churn in API, all drivers and all apps (including testpmd,
> > etc) implementing it (for IMO no real benefit). Yes, this API is experimental and
> > therefore we can operate changes in it, but I'd rather see incremental and
> > converging improvements rather than this.
>
> Yes, it changes all API, but very small part in each, will be very easy to align all the current dpdk components to use this concept.
>
> > If you guys insist with this proposal, I would like to get more opinions from
> > other vendors and contributors from within our DPDK community.
>
>
> Yes, more opinions are very welcomed.
>
> Thanks
  

On Thu, Mar 25, 2021 at 1:51 PM Matan Azrad <matan@nvidia.com> wrote:
>
> Hi Cristian
>
> From: Dumitrescu, Cristian
> > Hi Li and Matan,
> >
> > > -----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 2/2] [RFC]: ethdev: manage meter API object handles by
> > > the drivers
> > >

>
> Yes, it changes all API, but very small part in each, will be very easy to align all the current dpdk components to use this concept.
>
> > If you guys insist with this proposal, I would like to get more opinions from
> > other vendors and contributors from within our DPDK community.
>
>
> Yes, more opinions are very welcomed.

This point was discussed to the core in the initial meter RFC.

IMO, IDs helps in managing meter objects more clean way with lookup
cost. especially, If a non DPDK application, managing the meter ID
via some sort of IPC.

Considering existing APIs and drivers are implemented with ID scheme,
I think, it would be better to give some performance data for
changing the scheme.

I was under impression that, Typical number of MAX meter HW objects
could be around 16k something and the number of meter objects
created and destroyed per second will be very low. (In the order to
16k ops per second).

Could you share the practical number of meter HW objects in MLX SoCs
and the number of operations are you are envisioning?

>
> Thanks
  

Hi Ajit

Looks like you agree with this RFC to move meter objects to be managed by PMD pointers.

Thanks for the review!

From: Ajit Khaparde
> On Thu, Mar 25, 2021 at 1:21 AM Matan Azrad <matan@nvidia.com> wrote:
> >
> > Hi Cristian
> >
> > From: Dumitrescu, Cristian
> > > Hi Li and Matan,
> > >
> > > > -----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 2/2] [RFC]: ethdev: manage meter API object handles by
> > > > the drivers
> > > >
> > > > Currently, all the meter objects are managed by the user IDs:
> > > > meter, profile and policy.
> > > > Hence, each PMD should manage data-structure in order to map each API
> > > > ID to the private PMD management structure.
> > > >
> > > > From the application side, it has all the picture how meter is going
> > > > to be assigned to flows and can easily use direct mapping even when
> > > > the meter handler is provided by the PMDs.
> > > >
> > > > Also, this is the approach of the rte_flow API handles:
> > > > the flow handle and the shared action handle is provided by the PMDs.
> > > >
> > > > Use drivers handlers in order to manage all the meter API objects.
> > > >
> > >
> > > This seems to be take 2 of the discussion that we already had  in this thread:
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmails.dp
> > > dk.org%2Farchives%2Fdev%2F2021-
> > >
> March%2F200710.html&amp;data=04%7C01%7Cmatan%40nvidia.com%7Cab0
> > >
> e3cc77b9e4101344e08d8ee434bbe%7C43083d15727340c1b7db39efd9ccc17a%
> > >
> 7C0%7C0%7C637521320105450617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
> > >
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> > >
> amp;sdata=94bFRICfGEzk5s53MRUvFMQe5ZlhP2Tmnu82hwUytc4%3D&amp;re
> > > served=0, so apologies for mostly summarizing my previous feedback here.
> > >
> > > I am against this proposal because:
> > > 1. We already discussed this topic of user-provided handles vs. driver-
> provided
> > > handles at length on this exact email list back in 2017, when we first
> introduced
> > > this API, and I don't see any real reason to revisit the decision we took then.
> >
> > Why not?
> > There is more experiences\usages now.
> > New drivers added the support and also now scalability is growing and
> growing....
> >
> >
> > > 2. For me, it is more natural and it also helps the application to simplify its
> data
> > > structures if the user provides its own IDs rather than the user having to
> deal
> > > with the IDs provided by the driver.
> >
> > Generally I don't think other flow DPDK APIs align with your feelings here, see
> rte_flow object and rte_flow_shared_action.
> >
> > Specifically for meter:
> >         - here, meter is HW\driver offload where performance\rate either for
> meter creation\deletion or for the actual data-path is very important especially
> when we talk on very big numbers, so "natural" has less importance here.
> >           We need to think on the global solution for application ->API->driver. in
> meter feature, the user has the ability to manage the IDs better than the PMDs
> for the most of the use-cases:
> >                         1. meter per flow: just save the driver handle in the app flow
> context.
> >                         2. meter per VM\USER flows\rte_flow group\any other context
> grouped multiple flows: just save the driver handle in the app context.
> >         If PMD need to map the IDs, it is more complex for sure, requires more
> memory and more lookup time.
> >
> >         - I'm not sure it is natural for all the use-cases, sometimes generating
> unique ID may complex the app.
> >
> >
> > > 3. It is much easier and portable to pass numeric and string-based IDs
> around
> > > (e.g. between processes) as opposed to pointer-based IDs, as pointers are
> only
> > > valid in one address space and not in others. There are several DPDK APIs
> that
> > > moved away from pointer handles to string IDs.
> Pardon my ignorance..
> But which DPDK APIs moved to string IDs from pointer handles?
> 
> >
> > Yes, I agree here generally.
> > But again, since meter is used only by rte_flow, it is better to align the same
> handle mechanism.
> I don't want to say - do this because rte_flow uses a pointer.
> I don't have a strong opinion for one over the other. In the end the
> logic can be adapted one way or the other.
> But having implemented rte_flow support in the PMD, I think it is a
> good idea to avoid the duplication of meter_id to pointer based handle
> conversion and bookkeeping logic in the application and the PMD.
> 
> >
> > > 4. The mapping of user IDs to internal pointers within the driver is IMO not
> a
> > > big issue in terms of memory footprint or API call rate. Matan also
> confirmed
> > > this in the above thread when saying tis is not about either driver memory
> > > footprint or API call speed, as this mapping is easy to optimize.
> >
> > Yes, it is not very big deal, but still costs more than the new suggestion,
> especially in big scale.
> >
> > > And last but not least, this change obviously propagates in every API
> function,
> > > so it would result in big churn in API, all drivers and all apps (including
> testpmd,
> > > etc) implementing it (for IMO no real benefit). Yes, this API is experimental
> and
> > > therefore we can operate changes in it, but I'd rather see incremental and
> > > converging improvements rather than this.
> >
> > Yes, it changes all API, but very small part in each, will be very easy to align
> all the current dpdk components to use this concept.
> >
> > > If you guys insist with this proposal, I would like to get more opinions from
> > > other vendors and contributors from within our DPDK community.
> >
> >
> > Yes, more opinions are very welcomed.
> >
> > Thanks
  

Hi Jerin

Thanks for the review.
PSB

From: Jerin Jacob
> On Thu, Mar 25, 2021 at 1:51 PM Matan Azrad <matan@nvidia.com> wrote:
> >
> > Hi Cristian
> >
> > From: Dumitrescu, Cristian
> > > Hi Li and Matan,
> > >
> > > > -----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 2/2] [RFC]: ethdev: manage meter API object
> > > > handles by the drivers
> > > >
> 
> >
> > Yes, it changes all API, but very small part in each, will be very easy to align
> all the current dpdk components to use this concept.
> >
> > > If you guys insist with this proposal, I would like to get more
> > > opinions from other vendors and contributors from within our DPDK
> community.
> >
> >
> > Yes, more opinions are very welcomed.
> 
> This point was discussed to the core in the initial meter RFC.
> 
> IMO, IDs helps in managing meter objects more clean way with lookup cost.

Although it is not the main topic here I ask:
What is the difference between pointer to ID for the cost topic? Both are unique IDs actually....

> especially, If a non DPDK application, managing the meter ID via some sort of
> IPC.
> 
> Considering existing APIs and drivers are implemented with ID scheme, I think,
> it would be better to give some performance data for changing the scheme.
> 
> I was under impression that, Typical number of MAX meter HW objects could
> be around 16k something and the number of meter objects created and
> destroyed per second will be very low. (In the order to 16k ops per second)
> Could you share the practical number of meter HW objects in MLX SoCs and
> the number of operations are you are envisioning?

We are talking about 4M HW meters in the next version of mlx5 driver.
The rate of flows(with meter action) creation\deletion can arrive to 200K-300K per second and we are working hard to improve it more.

Can you please give also your opinion about the owner of the ID\pointer?
The main goal of this patch is to move the owner from the application to the driver....




 
> >
> > Thanks
  

On Tue, Mar 30, 2021 at 1:40 AM Matan Azrad <matan@nvidia.com> wrote:
>
> Hi Jerin


Hi Matan

>
> Thanks for the review.
> PSB
>
> From: Jerin Jacob
> > On Thu, Mar 25, 2021 at 1:51 PM Matan Azrad <matan@nvidia.com> wrote:
> > >
> > > Hi Cristian
> > >
> > > From: Dumitrescu, Cristian
> > > > Hi Li and Matan,
> > > >
> > > > > -----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 2/2] [RFC]: ethdev: manage meter API object
> > > > > handles by the drivers
> > > > >
> >
> > >
> > > Yes, it changes all API, but very small part in each, will be very easy to align
> > all the current dpdk components to use this concept.
> > >
> > > > If you guys insist with this proposal, I would like to get more
> > > > opinions from other vendors and contributors from within our DPDK
> > community.
> > >
> > >
> > > Yes, more opinions are very welcomed.
> >
> > This point was discussed to the core in the initial meter RFC.
> >
> > IMO, IDs helps in managing meter objects more clean way with lookup cost.
>
> Although it is not the main topic here I ask:
> What is the difference between pointer to ID for the cost topic? Both are unique IDs actually....

Once is defined by app and another one by implemention.


>
> > especially, If a non DPDK application, managing the meter ID via some sort of
> > IPC.
> >
> > Considering existing APIs and drivers are implemented with ID scheme, I think,
> > it would be better to give some performance data for changing the scheme.
> >
> > I was under impression that, Typical number of MAX meter HW objects could
> > be around 16k something and the number of meter objects created and
> > destroyed per second will be very low. (In the order to 16k ops per second)
> > Could you share the practical number of meter HW objects in MLX SoCs and
> > the number of operations are you are envisioning?
>
> We are talking about 4M HW meters in the next version of mlx5 driver.
> The rate of flows(with meter action) creation\deletion can arrive to 200K-300K per second and we are working hard to improve it more.
>
> Can you please give also your opinion about the owner of the ID\pointer?

Just to clarify, When you have 4M HW meters, At least, you have will
4M registers or 4M context memory kind
of an interface between HW<->SW set/get meter-specific objects. Or Is
it some kind of emulation over SW?

If it 4M HW meters then ID scheme makes sense.


> The main goal of this patch is to move the owner from the application to the driver....
>
>
>
>
>
> > >
> > > Thanks