On 9/27/2022 9:59 PM, Chautru, Nicolas wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
Hi Ferruh,
Thanks for your comment.
To be totally honest I don't yet see how your suggestion would be better, but I
quite possibly miss something. I did not reply in line with your comments so
that to try to be clearer and avoid spreading the argument to much. Ray and
Bruce feel free to chime in as well.
First to state the obvious: Nothing will change the fact that in case new enums
are being added in DPDK, and if the application doesn't change, then user would
not be able to interpret any such additional status/capability (backward
compatible only feature parity and still ABI compliant) which is totally
accepted as fine and up to the user, but the intention is at least not to have
adverse effect even when they don’t update their code for such new features
(notably in case they just use an older PMD not supporting such new features as
a basic typical example in the ecosystem). I think we agree on that problematic.
In term of history of not using MAX value for enum, I believe there is already
well documented and you agree with the reasoning of why we had to move away
from this [1]. Not just cosmetically where the max value is called an enum or a
#define but to have application making hardcoded assumption on the possible
maximum range for such enum notably when sizing array. The only caveat being
that at the time, the community did spot the weakness but did not come to an
agreement with regards to the best way to manage this moving forward.
In case your point is purely cosmetic to rename the PADDED_MAX value from the
enum to a #define (both public) I don't see how this would make thing clearer
by obfuscating the fact it is genuinely a padded value and to have that value
directly related to the enum structure. Also note that there is already an
actual max value defined for these enums (but kept private on purpose) which is
used by the lib/bbdev functions to restrict usage to what is actually supported
in the given implementation (distinct from padded max value).
Arguably the only concern I could understand in your message would be this one " my
concern was if user assumes all values valid until PADDED_MAX and tries to iterate array
until that value".
But really the fact that it is indeed a padded value implies fairly explicitly
that we have padded the supported enums with placeholders enums not yet
defined. That is fairly tautological! I cannot see how it could confuse anyone.
That is indeed to avoid such confusion that we went on that direction to expose
a public future-proof padded maximum value.
Then looking at usage in practice: when integrating the bbdev api with higher
level SW stacks (such as FlexRAN reference sw or 3rd party stacks) I don’t see
how any of this theoretical concerns you raised would be relevant for any of
these very cases (enqueue status, new capability etc...). The only genuine
concern was sizing array based on MAX value being not ABI compliant.
I cannot think of any code in the application presently deployed or future that
would then do what you are concerned about and cause an issue, and we
definitely don’t do such things in any example for bbdev-test or in FlexRAN
reference code provided to the ecosystem. The application would already have a
default case when an enum being provided has no matching application, or more
accurately in practice they would purely not look for these and hence these
would be ignored seamlessly.
Thanks again for the discussion. I wish this had happened earlier (we only
discussed this with Ray and Bruce while you were still at Intel), let me know
what you think.
It may be more generally good moving forward to come to a general agreement at
your technical forum level to avoid confusion. When we discussed earlier we
came to the conclusion that the DPDK community had well documented what not to
do to avoid ABI breakage but not necessarily what are the best alternatives.
Hopefully such future discussion should not delay this serie to be applied but
still let me know.
Hi Nic,
I believe it is more clear/safe to convert to SIZE_MAX macros, although
it is not a blocker.
Anyway, I am not sure about the value of continuing this discussion,
perhaps it is better to clarify the guidance for similar case with ABI
maintainer and techboard, so it can proceed according to the decision.
Thanks,
ferruh
Thanks again
[1]
* lib: will fix extending some enum/define breaking the ABI. There are
multiple
samples in DPDK that enum/define terminated with a ``.*MAX.*`` value
which is
used by iterators, and arrays holding these values are sized with this
``.*MAX.*`` value. So extending this enum/define increases the
``.*MAX.*``
value which increases the size of the array and depending on how/where
the
array is used this may break the ABI.
``RTE_ETH_FLOW_MAX`` is one sample of the mentioned case, adding a new
flow
type will break the ABI because of ``flex_mask[RTE_ETH_FLOW_MAX]`` array
usage in following public struct hierarchy:
``rte_eth_fdir_flex_conf -> rte_eth_fdir_conf -> rte_eth_conf (in the
middle)``.
Need to identify this kind of usages and fix in 20.11, otherwise this
blocks
us extending existing enum/define.
One solution can be using a fixed size array instead of ``.*MAX.*``
value.
-----Original Message-----
From: Chautru, Nicolas
Sent: Saturday, September 24, 2022 9:35 AM
To: 'Akhil Goyal' <gak...@marvell.com>; dev@dpdk.org; Maxime Coquelin
<maxime.coque...@redhat.com>; ferruh.yi...@xilinx.com; Ray Kinsella
<m...@ashroe.eu>
Cc: 'Akhil Goyal' <gak...@marvell.com>; Chautru, Nicolas
<nicolas.chau...@intel.com>; 'tho...@monjalon.net'
<tho...@monjalon.net>; 'Ray Kinsella' <m...@ashroe.eu>;
't...@redhat.com' <t...@redhat.com>; Richardson, Bruce
<bruce.richard...@intel.com>; 'david.march...@redhat.com'
<david.march...@redhat.com>; step...@networkplumber.org; Zhang,
Mingshan <mingshan.zh...@intel.com>; 'hemant.agra...@nxp.com'
<hemant.agra...@nxp.com>
Subject: RE: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and
status information
Hi Ferruh, Ray, Akhil,
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@amd.com>
Sent: Friday, September 23, 2022 4:28 PM
To: Akhil Goyal <gak...@marvell.com>; Nicolas Chautru
<nicolas.chau...@intel.com>; dev@dpdk.org; tho...@monjalon.net;
hemant.agra...@nxp.com; Ray Kinsella <m...@ashroe.eu>
Cc: maxime.coque...@redhat.com; t...@redhat.com;
bruce.richard...@intel.com; david.march...@redhat.com;
step...@networkplumber.org; mingshan.zh...@intel.com
Subject: Re: [EXT] [PATCH v7 6/7] bbdev: add queue related warning
and status information
On 9/21/2022 8:21 PM, Akhil Goyal wrote:
diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
ed528b8..b7ecf94 100644
--- a/lib/bbdev/rte_bbdev.h
+++ b/lib/bbdev/rte_bbdev.h
@@ -224,6 +224,19 @@ struct rte_bbdev_queue_conf {
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 superdes a previous
+ one
Spell check - supersedes.
+ */
+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 */
+ RTE_BBDEV_ENQ_STATUS_PADDED_MAX = 6, /**<
Maximum enq
status number including padding */
Are we ok to have this kind of padding across DPDK for all the
enums to avoid
ABI issues?
@Ray, @Thomas: any thoughts?
This kind of usage can prevent ABI tool warning, and can fix issues
caused by application using returned enum as index [1].
But I think it is still problematic in case application tries to
walk through till MAX, that is a common usage [2], user may miss that
this
is PADDED.
Hi Ferruh,
I don’t believe that case can happen here. Even if application was using an
undefined index, the related functions are protected for that :
See rte_bbdev_enqueue_status_str(enum rte_bbdev_enqueue_status status)
The reason for having padded max, is that the application may use this for
array sizing if required without concern, we will never return a value that
would exceeds this.
In the other direction that is not a problem either since application (even it
needs to store thigs in array) can used the padded version.
Note that this discussed with Ray notably as a BKM.
Overall exchanging enum values between library and application is
possible trouble for binary compatibility. If application and
library uses enum values independently, this is OK.
Since enum cases not deleted but added in new version of the
libraries, more problematic usage is passing enum value from library
to application, and bbdev seems doing this in a few places.
I don’t see a case where it is a genuine issue.
An enum is being reported from library, even if due to future enum insertion
there is a new enum reported between 2 ABI changes, that would still be
within bounds.
With current approach PADDED_MAX usage is more like #define usage,
it is not dynamic but a hardcoded value that is used as array size value.
Not providing a MAX enum case restricts the application, application
can't use it as size of an array or can't use it walk through
related array, usage reduces to if/switch comparisons.
It can use the padded_max to size application array. Even if application was
walking through these, there is no adverse effect.
Although this may not be most convenient for application, it can
provide safer usage for binary compatibility.
@Nic, what do you think provide these PADDED_MAX as #define
SIZE_MAX
macros?
With this application still can allocate a relevant array with
correct size, or know the size of library provided array, but can't
use it to iterate on these arrays.
That would be back to how it was done before which made things very
inflexible and prevented to change these enums between ABIs versions.
This change was highlighted and discussed many months ago and flagged in
the deprecation notice in previous release for that very reason.
Ray, can you please chime in since you know best.
Thanks Ferruh,
Nic
[1]
--------------- library old version ---------------------------- enum
type {
CASE_A,
CASE_B,
CASE_MAX,
};
struct data {
enum type type;
union {
type specific struct
};
};
int api_get(struct data *data);
--------------- application ----------------------------
struct data data;
int array[CASE_MAX];
api_get(&data);
foo(array[data.type]);
--------------- library NEW version ----------------------------
enum type {
CASE_A,
CASE_B,
CASE_C,
CASE_D,
CASE_MAX,
};
When application is NOT recompiled but using new version of the
library, values 'CASE_C' & 'CASE_D' will crash application, so this
will create a ABI compatibility issue.
Note: In the past I have claimed that application should check
'CASE_MAX', instead of using returned value directly as index, but
this is refused by argument that we can't control the application and
should play safe assuming application behaves wrong.
[2]
--------------- library ----------------------------
enum type {
CASE_NONE,
CASE_A,
CASE_B,
CASE_C,
CASE_D,
CASE_PADDED_MAX = 666,
};
--------------- application ----------------------------
for (int i = CASE_NONE; i < CASE_PADDED_MAX; i++)
fragile_init(i);
---
