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.
