[4/4] ethdev: add traffic manager query function
Checks
Commit Message
Add function to allow querying a node in the scheduler tree. Returns
the parameters as were given to the add function. Adding this function
allows apps to just query the hierarchy rather than having to maintain
their own copies of it internally.
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
lib/ethdev/ethdev_trace.h | 16 +++++++++++
lib/ethdev/ethdev_trace_points.c | 3 ++
lib/ethdev/rte_tm.c | 25 +++++++++++++++++
lib/ethdev/rte_tm.h | 48 ++++++++++++++++++++++++++++++++
lib/ethdev/rte_tm_driver.h | 12 ++++++++
5 files changed, 104 insertions(+)
Comments
On 8/6/2024 4:24 PM, Bruce Richardson wrote:
> +/**
> + * Return information about a traffic management node
> + *
> + * Return information about a hierarchy node, using the same format of parameters
> + * as was passed to the rte_rm_node_add() function.
> + * Each of the "out" parameters pointers (except error) may be passed as NULL if the
> + * information is not needed by the caller. For example, to one may check if a node id
> + * is in use by:
> + *
> + * struct rte_tm_error error;
> + * int ret = rte_tm_node_query(port, node_id, NULL, NULL, NULL, NULL, NULL, &error);
> + * if (ret == ENOENT) ...
> + *
> + * @param[in] port_id
> + * The port identifier of the Ethernet device.
> + * @param[in] node_id
> + * Node ID. Should be a valid node id.
> + * @param[out] parent_node_id
> + * Parent node ID.
> + * @param[out] priority
> + * Node priority. The highest node priority is zero. Used by the SP algorithm
> + * running on the parent of the current node for scheduling this child node.
> + * @param[out] weight
> + * Node weight. The node weight is relative to the weight sum of all siblings
> + * that have the same priority. The lowest weight is one. Used by the WFQ
> + * algorithm running on the parent of the current node for scheduling this
> + * child node.
> + * @param[out] level_id
> + * The node level in the scheduler hierarchy.
> + * @param[out] params
> + * Node parameters, as would be used when creating the node.
> + * @param[out] error
> + * Error details. Filled in only on error, when not NULL.
> + * @return
> + * 0 on success, non-zero error code otherwise.
> + * -EINVAL - port or node id value is invalid
> + * -ENOENT - no node exists with the provided id
> + */
> +int
> +rte_tm_node_query(uint16_t port_id,
> + uint32_t node_id,
> + uint32_t *parent_node_id,
> + uint32_t *priority,
> + uint32_t *weight,
> + uint32_t *level_id,
> + struct rte_tm_node_params *params,
> + struct rte_tm_error *error);
> +
>
No objection to get an TM node query API overall, but it would be good
to get more comment on the what correct API should be, we are missing it.
Both because it is not discussed much, and it is first release, better
to add this API as experimental.
Also we should have an implementation in driver and a sample application
usage (testpmd?) with new API. Are these planned separately for this
release, or can it be available part of next version of this patch?
Finally, does it worth documenting this in release notes, as just a
query API I am not sure if this a notable feature, but just a reminder.
On Sun, Sep 22, 2024 at 05:26:30PM +0100, Ferruh Yigit wrote:
> On 8/6/2024 4:24 PM, Bruce Richardson wrote:
> > +/**
> > + * Return information about a traffic management node
> > + *
> > + * Return information about a hierarchy node, using the same format of parameters
> > + * as was passed to the rte_rm_node_add() function.
> > + * Each of the "out" parameters pointers (except error) may be passed as NULL if the
> > + * information is not needed by the caller. For example, to one may check if a node id
> > + * is in use by:
> > + *
> > + * struct rte_tm_error error;
> > + * int ret = rte_tm_node_query(port, node_id, NULL, NULL, NULL, NULL, NULL, &error);
> > + * if (ret == ENOENT) ...
> > + *
> > + * @param[in] port_id
> > + * The port identifier of the Ethernet device.
> > + * @param[in] node_id
> > + * Node ID. Should be a valid node id.
> > + * @param[out] parent_node_id
> > + * Parent node ID.
> > + * @param[out] priority
> > + * Node priority. The highest node priority is zero. Used by the SP algorithm
> > + * running on the parent of the current node for scheduling this child node.
> > + * @param[out] weight
> > + * Node weight. The node weight is relative to the weight sum of all siblings
> > + * that have the same priority. The lowest weight is one. Used by the WFQ
> > + * algorithm running on the parent of the current node for scheduling this
> > + * child node.
> > + * @param[out] level_id
> > + * The node level in the scheduler hierarchy.
> > + * @param[out] params
> > + * Node parameters, as would be used when creating the node.
> > + * @param[out] error
> > + * Error details. Filled in only on error, when not NULL.
> > + * @return
> > + * 0 on success, non-zero error code otherwise.
> > + * -EINVAL - port or node id value is invalid
> > + * -ENOENT - no node exists with the provided id
> > + */
> > +int
> > +rte_tm_node_query(uint16_t port_id,
> > + uint32_t node_id,
> > + uint32_t *parent_node_id,
> > + uint32_t *priority,
> > + uint32_t *weight,
> > + uint32_t *level_id,
> > + struct rte_tm_node_params *params,
> > + struct rte_tm_error *error);
> > +
> >
>
> No objection to get an TM node query API overall, but it would be good
> to get more comment on the what correct API should be, we are missing it.
> Both because it is not discussed much, and it is first release, better
> to add this API as experimental.
>
Yes, good point, it should be added to version.map file and marked as
experimental. Happy to take any feedback on what the API should be.
> Also we should have an implementation in driver and a sample application
> usage (testpmd?) with new API. Are these planned separately for this
> release, or can it be available part of next version of this patch?
>
I have an implemented for ice driver in [1]. On testpmd side, I never
thought to do so, because it was more for me when writing test code around
the APIs than for actual end-users. [As I explain, this API should save app
developers the work of storing a copy of the TM hierarchy in the app code
too]. However, I think it may be no harm to do a testpmd call for it, it
may be useful for debugging.
[1] https://patches.dpdk.org/project/dpdk/patch/20240812152815.1132697-2-bruce.richardson@intel.com/
> Finally, does it worth documenting this in release notes, as just a
> query API I am not sure if this a notable feature, but just a reminder.
>
Don't think it requires an RN item.
Will perhaps do a new revision of this patchset without this final patch,
and then submit this patch as a separate one for tracking. I don't think
the other patches to mark things const should be blocked by discussion on
this.
Regards,
/Bruce
On Mon, Oct 07, 2024 at 12:04:37PM +0100, Bruce Richardson wrote:
> On Sun, Sep 22, 2024 at 05:26:30PM +0100, Ferruh Yigit wrote:
> > On 8/6/2024 4:24 PM, Bruce Richardson wrote:
> > > +/**
> > > + * Return information about a traffic management node
> > > + *
> > > + * Return information about a hierarchy node, using the same format of parameters
> > > + * as was passed to the rte_rm_node_add() function.
> > > + * Each of the "out" parameters pointers (except error) may be passed as NULL if the
> > > + * information is not needed by the caller. For example, to one may check if a node id
> > > + * is in use by:
> > > + *
> > > + * struct rte_tm_error error;
> > > + * int ret = rte_tm_node_query(port, node_id, NULL, NULL, NULL, NULL, NULL, &error);
> > > + * if (ret == ENOENT) ...
> > > + *
> > > + * @param[in] port_id
> > > + * The port identifier of the Ethernet device.
> > > + * @param[in] node_id
> > > + * Node ID. Should be a valid node id.
> > > + * @param[out] parent_node_id
> > > + * Parent node ID.
> > > + * @param[out] priority
> > > + * Node priority. The highest node priority is zero. Used by the SP algorithm
> > > + * running on the parent of the current node for scheduling this child node.
> > > + * @param[out] weight
> > > + * Node weight. The node weight is relative to the weight sum of all siblings
> > > + * that have the same priority. The lowest weight is one. Used by the WFQ
> > > + * algorithm running on the parent of the current node for scheduling this
> > > + * child node.
> > > + * @param[out] level_id
> > > + * The node level in the scheduler hierarchy.
> > > + * @param[out] params
> > > + * Node parameters, as would be used when creating the node.
> > > + * @param[out] error
> > > + * Error details. Filled in only on error, when not NULL.
> > > + * @return
> > > + * 0 on success, non-zero error code otherwise.
> > > + * -EINVAL - port or node id value is invalid
> > > + * -ENOENT - no node exists with the provided id
> > > + */
> > > +int
> > > +rte_tm_node_query(uint16_t port_id,
> > > + uint32_t node_id,
> > > + uint32_t *parent_node_id,
> > > + uint32_t *priority,
> > > + uint32_t *weight,
> > > + uint32_t *level_id,
> > > + struct rte_tm_node_params *params,
> > > + struct rte_tm_error *error);
> > > +
> > >
> >
> > No objection to get an TM node query API overall, but it would be good
> > to get more comment on the what correct API should be, we are missing it.
> > Both because it is not discussed much, and it is first release, better
> > to add this API as experimental.
> >
>
> Yes, good point, it should be added to version.map file and marked as
> experimental. Happy to take any feedback on what the API should be.
>
> > Also we should have an implementation in driver and a sample application
> > usage (testpmd?) with new API. Are these planned separately for this
> > release, or can it be available part of next version of this patch?
> >
> I have an implemented for ice driver in [1]. On testpmd side, I never
> thought to do so, because it was more for me when writing test code around
> the APIs than for actual end-users. [As I explain, this API should save app
> developers the work of storing a copy of the TM hierarchy in the app code
> too]. However, I think it may be no harm to do a testpmd call for it, it
> may be useful for debugging.
>
> [1] https://patches.dpdk.org/project/dpdk/patch/20240812152815.1132697-2-bruce.richardson@intel.com/
>
> > Finally, does it worth documenting this in release notes, as just a
> > query API I am not sure if this a notable feature, but just a reminder.
> >
>
> Don't think it requires an RN item.
>
> Will perhaps do a new revision of this patchset without this final patch,
> and then submit this patch as a separate one for tracking. I don't think
> the other patches to mark things const should be blocked by discussion on
> this.
>
This patch is now split off into separate series including driver update
and testpmd command:
https://patches.dpdk.org/project/dpdk/list/?series=33342
Regards,
/Bruce
@@ -1903,6 +1903,22 @@ RTE_TRACE_POINT(
rte_trace_point_emit_int(ret);
)
+RTE_TRACE_POINT(
+ rte_tm_trace_node_query,
+ RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t node_id,
+ uint32_t *parent_node_id, uint32_t *priority,
+ uint32_t *weight, uint32_t *level_id,
+ struct rte_tm_node_params *params, int ret),
+ rte_trace_point_emit_u16(port_id);
+ rte_trace_point_emit_u32(node_id);
+ rte_trace_point_emit_ptr(parent_node_id);
+ rte_trace_point_emit_ptr(priority);
+ rte_trace_point_emit_ptr(weight);
+ rte_trace_point_emit_ptr(level_id);
+ rte_trace_point_emit_ptr(params);
+ rte_trace_point_emit_int(ret);
+)
+
RTE_TRACE_POINT(
rte_tm_trace_node_delete,
RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t node_id, int ret),
@@ -694,6 +694,9 @@ RTE_TRACE_POINT_REGISTER(rte_tm_trace_mark_vlan_dei,
RTE_TRACE_POINT_REGISTER(rte_tm_trace_node_add,
lib.ethdev.tm.node_add)
+RTE_TRACE_POINT_REGISTER(rte_tm_trace_node_query,
+ lib.ethdev.tm.node_query)
+
RTE_TRACE_POINT_REGISTER(rte_tm_trace_node_capabilities_get,
lib.ethdev.tm.node_capabilities_get)
@@ -301,6 +301,31 @@ int rte_tm_node_add(uint16_t port_id,
return ret;
}
+int rte_tm_node_query(uint16_t port_id,
+ uint32_t node_id,
+ uint32_t *parent_node_id,
+ uint32_t *priority,
+ uint32_t *weight,
+ uint32_t *level_id,
+ struct rte_tm_node_params *params,
+ struct rte_tm_error *error)
+{
+ struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+ int ret;
+
+ if (dev == NULL)
+ return -EINVAL;
+
+ ret = RTE_TM_FUNC(port_id, node_query)(dev,
+ node_id, parent_node_id, priority, weight, level_id,
+ params, error);
+
+ rte_tm_trace_node_query(port_id, node_id, parent_node_id, priority,
+ weight, level_id, params, ret);
+
+ return ret;
+}
+
/* Delete node from traffic manager hierarchy */
int rte_tm_node_delete(uint16_t port_id,
uint32_t node_id,
@@ -1599,6 +1599,54 @@ rte_tm_node_add(uint16_t port_id,
const struct rte_tm_node_params *params,
struct rte_tm_error *error);
+/**
+ * Return information about a traffic management node
+ *
+ * Return information about a hierarchy node, using the same format of parameters
+ * as was passed to the rte_rm_node_add() function.
+ * Each of the "out" parameters pointers (except error) may be passed as NULL if the
+ * information is not needed by the caller. For example, to one may check if a node id
+ * is in use by:
+ *
+ * struct rte_tm_error error;
+ * int ret = rte_tm_node_query(port, node_id, NULL, NULL, NULL, NULL, NULL, &error);
+ * if (ret == ENOENT) ...
+ *
+ * @param[in] port_id
+ * The port identifier of the Ethernet device.
+ * @param[in] node_id
+ * Node ID. Should be a valid node id.
+ * @param[out] parent_node_id
+ * Parent node ID.
+ * @param[out] priority
+ * Node priority. The highest node priority is zero. Used by the SP algorithm
+ * running on the parent of the current node for scheduling this child node.
+ * @param[out] weight
+ * Node weight. The node weight is relative to the weight sum of all siblings
+ * that have the same priority. The lowest weight is one. Used by the WFQ
+ * algorithm running on the parent of the current node for scheduling this
+ * child node.
+ * @param[out] level_id
+ * The node level in the scheduler hierarchy.
+ * @param[out] params
+ * Node parameters, as would be used when creating the node.
+ * @param[out] error
+ * Error details. Filled in only on error, when not NULL.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ * -EINVAL - port or node id value is invalid
+ * -ENOENT - no node exists with the provided id
+ */
+int
+rte_tm_node_query(uint16_t port_id,
+ uint32_t node_id,
+ uint32_t *parent_node_id,
+ uint32_t *priority,
+ uint32_t *weight,
+ uint32_t *level_id,
+ struct rte_tm_node_params *params,
+ struct rte_tm_error *error);
+
/**
* Traffic manager node delete
*
@@ -119,6 +119,16 @@ typedef int (*rte_tm_node_resume_t)(struct rte_eth_dev *dev,
uint32_t node_id,
struct rte_tm_error *error);
+/** @internal Traffic manager node query */
+typedef int (*rte_tm_node_query_t)(const struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t *parent_node_id,
+ uint32_t *priority,
+ uint32_t *weight,
+ uint32_t *level_id,
+ struct rte_tm_node_params *params,
+ struct rte_tm_error *error);
+
/** @internal Traffic manager hierarchy commit */
typedef int (*rte_tm_hierarchy_commit_t)(struct rte_eth_dev *dev,
int clear_on_fail,
@@ -248,6 +258,8 @@ struct rte_tm_ops {
rte_tm_node_suspend_t node_suspend;
/** Traffic manager node resume */
rte_tm_node_resume_t node_resume;
+ /** Traffic manager node resume */
+ rte_tm_node_query_t node_query;
/** Traffic manager hierarchy commit */
rte_tm_hierarchy_commit_t hierarchy_commit;