[v10,6/7] bbdev: add queue related warning and status information
Checks
Commit Message
This allows to expose more information with regards to any
queue related failure and warning which cannot be supported
in existing API.
Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
app/test-bbdev/test_bbdev_perf.c | 2 ++
lib/bbdev/rte_bbdev.c | 19 +++++++++++++++
lib/bbdev/rte_bbdev.h | 41 ++++++++++++++++++++++++++++++++
lib/bbdev/version.map | 1 +
4 files changed, 63 insertions(+)
Comments
Looking at this patch because I have been alerted about the ABI compat handling.
I see some details that should have been caught in earlier reviews.
30/09/2022 20:46, Nicolas Chautru:
> +/*
> + * Maximum size to be used to manage the enum rte_bbdev_enqueue_status including padding for future
This line is long.
It is always better to split lines logically,
for instance here, before "including".
> + * enum insertion
It could be made clear that the real enum size is smaller or equal.
> + */
> +#define RTE_BBDEV_ENQ_STATUS_SIZE_MAX 6
[...]
> +enum rte_bbdev_enqueue_status {
> + RTE_BBDEV_ENQ_STATUS_NONE, /**< Nothing to report */
> + RTE_BBDEV_ENQ_STATUS_QUEUE_FULL, /**< Not enough room in queue */
> + RTE_BBDEV_ENQ_STATUS_RING_FULL, /**< Not enough room in ring */
> + RTE_BBDEV_ENQ_STATUS_INVALID_OP, /**< Operation was rejected as invalid */
> +};
A comment is missing at the end of the enum to remind updating the MAX.
But the big question is why do we need this "MAX" value?
The guideline is to avoid using such MAX value for long term compatibility.
[...]
> +/**
> + * Converts queue status from enum to string
Should be imperative form: "Convert".
A dot is missing at the end of the sentence.
> + *
> + * @param status
> + * Queue status as enum
> + *
> + * @returns
> + * Queue status as string or NULL if op_type is invalid
It is not aligned with above parameter.
Choose an indentation format and keep it consistent.
[...]
> # added in 22.11
> rte_bbdev_device_status_str;
> + rte_bbdev_enqueue_status_str;
> rte_bbdev_enqueue_fft_ops;
> rte_bbdev_dequeue_fft_ops;
It is not alphabetical order.
Hi Thomas,
I will update all your comments below today, thanks.
The one where we need your confirmation is specifically this comment from your, I believe we discussed but good to make sure we are all aligned:
> But the big question is why do we need this "MAX" value?
> The guideline is to avoid using such MAX value for long term compatibility.
This is not a _MAX enum but a _SIZE_MAX for array related to that enum. Note that the actual max value of the enum exists but is used a private macro.
The distinction is that the application cannot make any assumptions on what is the maximum enum value (ie. we don't want enum with MAX value, that is not future proof has captured in doc).
But the application can make some assumption on the sizing of array based on such an enum. The difference being the padding which allows for the enum growth without breaking ABI or application.
The previous name was _PADDED_MAX to make it clear this not a max enum but a padded value. Then more recenrtly the consensus in the community was to change this to _SIZE_MAX to be arguably more explicit this is to be used for array size. The comments I believe also make it clear this is not a MAX enum.
Does that make sense and do you agree this is best consensus so far to move forward?
Thanks Thomas,
Nic
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, October 3, 2022 1:29 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; gakhil@marvell.com; maxime.coquelin@redhat.com;
> trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org; Zhang, Mingshan
> <mingshan.zhang@intel.com>; hemant.agrawal@nxp.com
> Subject: Re: [PATCH v10 6/7] bbdev: add queue related warning and status
> information
>
> Looking at this patch because I have been alerted about the ABI compat
> handling.
> I see some details that should have been caught in earlier reviews.
>
> 30/09/2022 20:46, Nicolas Chautru:
>
> > +/*
> > + * Maximum size to be used to manage the enum
> > +rte_bbdev_enqueue_status including padding for future
>
> This line is long.
> It is always better to split lines logically, for instance here, before "including".
>
> > + * enum insertion
>
> It could be made clear that the real enum size is smaller or equal.
>
> > + */
> > +#define RTE_BBDEV_ENQ_STATUS_SIZE_MAX 6
> [...]
> > +enum rte_bbdev_enqueue_status {
> > + RTE_BBDEV_ENQ_STATUS_NONE, /**< Nothing to report */
> > + RTE_BBDEV_ENQ_STATUS_QUEUE_FULL, /**< Not enough room in
> queue */
> > + RTE_BBDEV_ENQ_STATUS_RING_FULL, /**< Not enough room in
> ring */
> > + RTE_BBDEV_ENQ_STATUS_INVALID_OP, /**< Operation was
> rejected as invalid */
> > +};
>
> A comment is missing at the end of the enum to remind updating the MAX.
>
> But the big question is why do we need this "MAX" value?
> The guideline is to avoid using such MAX value for long term compatibility.
>
> [...]
> > +/**
> > + * Converts queue status from enum to string
>
> Should be imperative form: "Convert".
> A dot is missing at the end of the sentence.
>
> > + *
> > + * @param status
> > + * Queue status as enum
> > + *
> > + * @returns
> > + * Queue status as string or NULL if op_type is invalid
>
> It is not aligned with above parameter.
> Choose an indentation format and keep it consistent.
>
> [...]
> > # added in 22.11
> > rte_bbdev_device_status_str;
> > + rte_bbdev_enqueue_status_str;
> > rte_bbdev_enqueue_fft_ops;
> > rte_bbdev_dequeue_fft_ops;
>
> It is not alphabetical order.
>
>
03/10/2022 18:39, Chautru, Nicolas:
> Hi Thomas,
>
> I will update all your comments below today, thanks.
Please do a self review of other patches (I reviewed only this one),
I suspect there are a lot of other details to improve in other places.
> The one where we need your confirmation is specifically this comment from your, I believe we discussed but good to make sure we are all aligned:
Yes let's be clear.
> > But the big question is why do we need this "MAX" value?
> > The guideline is to avoid using such MAX value for long term compatibility.
>
> This is not a _MAX enum but a _SIZE_MAX for array related to that enum. Note that the actual max value of the enum exists but is used a private macro.
> The distinction is that the application cannot make any assumptions on what is the maximum enum value (ie. we don't want enum with MAX value, that is not future proof has captured in doc).
> But the application can make some assumption on the sizing of array based on such an enum. The difference being the padding which allows for the enum growth without breaking ABI or application.
> The previous name was _PADDED_MAX to make it clear this not a max enum but a padded value. Then more recenrtly the consensus in the community was to change this to _SIZE_MAX to be arguably more explicit this is to be used for array size. The comments I believe also make it clear this is not a MAX enum.
>
> Does that make sense and do you agree this is best consensus so far to move forward?
It makes a lot of sense to me because this is what I proposed 2 or 3 years ago
to the techboard when we approached different general solutions.
But it has been said that it is not ideal for 2 reasons I think:
- there is a compatibility breakage when we reach the maximum
- there is no assumption on how the padding is filled
That's why the preferred way should be to avoid having any maximum value.
For instance, array allocation and iteration could be done in the API.
Anyway I'm fine with your solution for now.
@@ -4360,6 +4360,8 @@ get_bbdev_queue_stats(uint16_t dev_id, uint16_t queue_id,
stats->dequeued_count = q_stats->dequeued_count;
stats->enqueue_err_count = q_stats->enqueue_err_count;
stats->dequeue_err_count = q_stats->dequeue_err_count;
+ stats->enqueue_warning_count = q_stats->enqueue_warning_count;
+ stats->dequeue_warning_count = q_stats->dequeue_warning_count;
stats->acc_offload_cycles = q_stats->acc_offload_cycles;
return 0;
@@ -721,6 +721,8 @@ get_stats_from_queues(struct rte_bbdev *dev, struct rte_bbdev_stats *stats)
stats->dequeued_count += q_stats->dequeued_count;
stats->enqueue_err_count += q_stats->enqueue_err_count;
stats->dequeue_err_count += q_stats->dequeue_err_count;
+ stats->enqueue_warn_count += q_stats->enqueue_warn_count;
+ stats->dequeue_warn_count += q_stats->dequeue_warn_count;
}
rte_bbdev_log_debug("Got stats on %u", dev->data->dev_id);
}
@@ -1163,3 +1165,20 @@ rte_bbdev_device_status_str(enum rte_bbdev_device_status status)
rte_bbdev_log(ERR, "Invalid device status");
return NULL;
}
+
+const char *
+rte_bbdev_enqueue_status_str(enum rte_bbdev_enqueue_status status)
+{
+ static const char * const enq_sta_string[] = {
+ "RTE_BBDEV_ENQ_STATUS_NONE",
+ "RTE_BBDEV_ENQ_STATUS_QUEUE_FULL",
+ "RTE_BBDEV_ENQ_STATUS_RING_FULL",
+ "RTE_BBDEV_ENQ_STATUS_INVALID_OP",
+ };
+
+ if (status < sizeof(enq_sta_string) / sizeof(char *))
+ return enq_sta_string[status];
+
+ rte_bbdev_log(ERR, "Invalid enqueue status");
+ return NULL;
+}
@@ -35,6 +35,12 @@ extern "C" {
#define RTE_BBDEV_MAX_DEVS 128 /**< Max number of devices */
#endif
+/*
+ * Maximum size to be used to manage the enum rte_bbdev_enqueue_status including padding for future
+ * enum insertion
+ */
+#define RTE_BBDEV_ENQ_STATUS_SIZE_MAX 6
+
/** Flags indicate current state of BBDEV device */
enum rte_bbdev_state {
RTE_BBDEV_UNUSED,
@@ -223,6 +229,20 @@ rte_bbdev_queue_start(uint16_t dev_id, uint16_t queue_id);
int
rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
+/**
+ * Flags indicate the reason why a previous enqueue may not have
+ * consumed all requested operations
+ * In case of multiple reasons the latter supersedes a previous one.
+ * The related macro RTE_BBDEV_ENQ_STATUS_SIZE_MAX can be used as an absolute maximum for
+ * notably sizing array while allowing for future enumeration insertion.
+ */
+enum rte_bbdev_enqueue_status {
+ RTE_BBDEV_ENQ_STATUS_NONE, /**< Nothing to report */
+ RTE_BBDEV_ENQ_STATUS_QUEUE_FULL, /**< Not enough room in queue */
+ RTE_BBDEV_ENQ_STATUS_RING_FULL, /**< Not enough room in ring */
+ RTE_BBDEV_ENQ_STATUS_INVALID_OP, /**< Operation was rejected as invalid */
+};
+
/**
* Flags indicate the status of the device
*/
@@ -246,6 +266,12 @@ struct rte_bbdev_stats {
uint64_t enqueue_err_count;
/** Total error count on operations dequeued */
uint64_t dequeue_err_count;
+ /** Total warning count on operations enqueued */
+ uint64_t enqueue_warn_count;
+ /** Total warning count on operations dequeued */
+ uint64_t dequeue_warn_count;
+ /** Total enqueue status count based on rte_bbdev_enqueue_status enum */
+ uint64_t enqueue_status_count[RTE_BBDEV_ENQ_STATUS_SIZE_MAX];
/** CPU cycles consumed by the (HW/SW) accelerator device to offload
* the enqueue request to its internal queues.
* - For a HW device this is the cycles consumed in MMIO write
@@ -386,6 +412,7 @@ struct rte_bbdev_queue_data {
void *queue_private; /**< Driver-specific per-queue data */
struct rte_bbdev_queue_conf conf; /**< Current configuration */
struct rte_bbdev_stats queue_stats; /**< Queue statistics */
+ enum rte_bbdev_enqueue_status enqueue_status; /**< Enqueue status when op is rejected */
bool started; /**< Queue state */
};
@@ -938,6 +965,20 @@ __rte_experimental
const char*
rte_bbdev_device_status_str(enum rte_bbdev_device_status status);
+/**
+ * Converts queue status from enum to string
+ *
+ * @param status
+ * Queue status as enum
+ *
+ * @returns
+ * Queue status as string or NULL if op_type is invalid
+ *
+ */
+__rte_experimental
+const char*
+rte_bbdev_enqueue_status_str(enum rte_bbdev_enqueue_status status);
+
#ifdef __cplusplus
}
#endif
@@ -45,6 +45,7 @@ EXPERIMENTAL {
# added in 22.11
rte_bbdev_device_status_str;
+ rte_bbdev_enqueue_status_str;
rte_bbdev_enqueue_fft_ops;
rte_bbdev_dequeue_fft_ops;
rte_bbdev_fft_op_alloc_bulk;