-----Original Message-----
From: Akhil Goyal <gak...@marvell.com>
Sent: Thursday, September 29, 2022 11:33 AM
To: Ferruh Yigit <ferruh.yi...@amd.com>; Chautru, Nicolas
<nicolas.chau...@intel.com>; dev@dpdk.org; Maxime Coquelin
<maxime.coque...@redhat.com>; ferruh.yi...@xilinx.com; Ray Kinsella
<m...@ashroe.eu>
Cc: tho...@monjalon.net; t...@redhat.com; Richardson, Bruce
<bruce.richard...@intel.com>; david.march...@redhat.com;
step...@networkplumber.org; Zhang, Mingshan
<mingshan.zh...@intel.com>; hemant.agra...@nxp.com
Subject: RE: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and status
information
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.
I agree with Ferruh's comment for converting to SIZE_MAX macros.
However, it is not a strong comment from my side.
Moving to techboard would mean this patchset would skip the RC1 window.
I believe as Ray is the maintainer and go to person for ABI related issues.
I believe if he can take a look at the suggestion and provide ack/nack to
whichever Approach would be fine and we can go ahead in that direction.
I would like to close this as soon as possible. There are a lot of patches to be
blocked on this series.
Regards,
Akhil
