Hi Thomas, In absence of Ray (I did not see email from him for a some time) can you please advise on best option so that as to move on. I can either keep as is based on initial review with Ray, or replace _PADDED_MAX to _SIZE_MAX macro as suggested by Ferruh. I am happy either way as long as we are able to move forward. There is no full consensus but not strong opinion either from anyone. Thanks, Nic
> -----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
