> From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > Sent: Thursday, 5 January 2023 23.06 > > On Thu, Jan 05, 2023 at 10:34:55PM +0100, Morten Brørup wrote: > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > Sent: Thursday, 5 January 2023 21.58 > > > > > > On Thu, Jan 05, 2023 at 09:27:12AM -0800, Tyler Retzlaff wrote: > > > > On Thu, Jan 05, 2023 at 09:23:49AM -0800, Tyler Retzlaff wrote: > > > > > > > oh! not a problem. i'm very keen to catch any mistakes, > thought > > > i had > > > > > > > missed something. > > > > > > > > > > > > I think we should move all bit-related functions together. > > > > > > Please could you add another patch to your series > > > > > > moving "ms1b"/"bsf"/"fls" functions in this file? > > > > > > > > > > > > > > > > > > > > > > okay, so there is already a rte_bitops.h. i guess everything > should > > > go > > > > > there including the leading/trailing count functions instead of > > > adding a > > > > > new header. > > > > > > > > > > i'll introduce a new patch to the series that gathers the > existing > > > > > functions into rte_bitops.h and place the new functions there > too. > > > > > > > > > > thanks > > > > > > > > just as a further follow up, you do understand that this is > > > technically > > > > an api break? > > > > > > > > moving functions from rte_common.h to rte_bitops.h will make > > > translation > > > > units that included only rte_common.h but used these functions > will > > > > fail to compile without being updated to include rte_bitops.h. > > > > > > > > anyway, i'll submit v3 with this change anyway. > > > > > > so when attempting to do this it became immediately obvious that > moving > > > just the bit op functions out is going to create a circular > dependency > > > between rte_common.h, rte_bitops.h > > > > > > once the bit ops are moved out of common there are still other > inline > > > functions that remain in comman that require bringing bitops back > in, > > > but bitops depends on common. > > > > > > my compromise will be to break log2 and pow2 inline functions into > > > their > > > own files to break the cycle (common no longer depends on bitops). > i'll > > > submit patches for this but it ends up touching a lot more of the > > > tree to add back includes for log/pow inline use. > > > > > > alternatively i can just not move the remaining bit manipulation > > > functions, let me know which is preferred. > > > > It seems that no perfect solution exists, so we will have to live > with a compromise. Here is another proposal for a compromise, for yours > and Thomas's consideration: > > > > I noticed that rte_bitops.h is mainly for setting/getting bits, used > for accessing hardware. > > > > Your functions are mathematical functions, and so are the similar > functions in rte_common.h (which is why it makes sense to keep them > together with yours). If we cannot clean up rte_common.h by moving them > out, perhaps we should accept the current situation (until we find a > way to move them out) and just add your mathematical functions where > the existing mathematical functions reside, i.e. in rte_common.h. > > > > This proposal only makes the existing mess slightly larger; it > doesn't create a new kind of mess. > > so i fudged around a bit to see if i could get a happy medium. i ended > up with this. > > remove include of rte_debug.h from rte_bitops.h > > * had to remove the RTE_ASSERT from existing rte_bitops.h functions > * this breaks a good piece of the cycle debug -> log -> common -> > bitops -> debug > * deal breaker? i don't think it was right that we were getting all > of log, common just for using bitops anyway. > > move pow2 functions from rte_common.h -> rte_pow2ops.h > * new header includes rte_bitops.h > > move log2 functions from rte_common.h -> rte_log2ops.h > * new header includes rte_bitops.h, rte_pow2ops.h > > include rte_bitops.h, rte_pow2ops.h and rte_log2ops.h back into > rte_common.h > > * this is done to reduce the impact of compatibility break by > continuing to expose the pow2/log2/bitops via rte_common.h > > so we end up with 3 standalone headers, where the whole tree builds > without having to add a pile of includes for the new headers. we can > later deprecate the exposure of the inline functions when including > rte_common.h > > * one caveat is that there was some contamination coming in via the > removed rte_debug.h where rte_bitops.h was used. so technically > a break of api too. > > objections? > > if this is no good i'll just fold my new functions into rte_common.h > and > leave the mess for the next person, though i am trying not to do that. > > thanks for the discussion.
Here's some long term thinking: EAL has grown into a trashcan where too much is thrown in. It should only be a thin shim to abstract the underlying hardware and O/S environment. A step in that direction could be splitting the current EAL into a true EAL and a Utils library. Not now, but perhaps some day in a rosy future. Your proposal effectively makes rte_common.h even bigger by including rte_bitops.h (which was intended for accessing hardware). I am not sure it is a step in the right direction. On the other hand, introducing yet another header file for bit-mathematical functions is probably worse than adding them to rte_bitops.h. I can't come up with something good myself, but I lean towards simply adding your functions to rte_common.h and live with the existing mess. If you think your proposal is better, I will not object. I'm only voicing my thoughts. @Thomas may have another perspective on the matter. Thanks for all the work you put into this. -Morten
