Re: [PATCH v2 1/2] eal: provide leading and trailing zero bit count abstraction

[Thomas Monjalon <thomas@monjalon.net>]

06/01/2023 02:04, Tyler Retzlaff:
> On Fri, Jan 06, 2023 at 12:10:45AM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Thursday, 5 January 2023 23.06
> > > 
> > > 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.
> 
> it's hard when we are starting with something monolithic and trying to
> split it. you always end up with these iterative transitions toward what
> you want because you have to keep some kind of compat.
> 
> > 
> > @Thomas may have another perspective on the matter.
> 
> Thomas any last word after this back and forth?  This change isn't
> necessarily blocking me but is a distraction i'd like to finish/offload.

I think it is better to move all in rte_bitops.h
rte_common must have almost zero dependency.
If log2 and pow2 are based on bitops operations, it is not a bad fit for rte_bitops.h.