Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Cristian,

> > > 18/12/2018 14:19, Dumitrescu, Cristian:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 18/12/2018 12:18, Dumitrescu, Cristian:
> > > > > > > > I replied in v3 that it should stay in rte_meter.h.
> > > > > > > > You can include rte_meter.h in ethdev.
> > > > > > >
> > > > > > > OK, thanks Thomas, makes sense to me as well.
> > > > > > >
> > > > > >
> > > > > > Thomas,
> > > > > >
> > > > > > I agree with your input, but just want to make sure we are on the
> > same
> > > > > page:
> > > > > >
> > > > > > Besides including rte_meter.h in ethdev (which you are fine with), we
> > > > > would also need to include rte_meter.h in mbuf.
> > > > > >
> > > > > > Are you OK with this as well?
> > > > >
> > > > > Why do we need rte_meter.h in mbuf?
> > > > >
> > > >
> > > > You probably looked at V2 only, but in V3 we have functions to set/get
> > the color within the mbuf->hash.sched field.
> > > >
> > > > For space compression reasons, the mbuf->hash.sched stores the color
> > on 8-bit variable, while for the outside world the set/get functions
> > > work with the 32-bit enum type. We do same thing in other places in DPDK,
> > such as rte_crypto_op, etc.
> > >
> > > So it's a different discussion.
> > > We need to review this v3 and check how relevant this mbuf API is.
> > >
> > > If the API is accepted, yes the include should not be an issue.
> >
> > Personally, I don't think it is a good idea to add extra dependency for
> > librte_mbuf.
> > I'd prefer either to keep rte_color definition inside librte_mbuf,
> > or move corresponding function definitions out of it.
> > Konstantin
> 
> Konstantin,
> 
> 	As you see, the number of options is limited, and none of them is perfect:
> 
> 	1/ color enum in EAL/common/include: still my favorite, as it does not create any new library dependencies, and it is already used
> to store lots of similar generic items

As I remember, we don't put network specific things into EAL, but probably you have different examples?

> 	2/ color enum in rte_meter.h: results in creating new librte_mbuf dependency to librte_meter
> 	3/ color enum in rte_mbuf.h: results in creating new librte_meter dependency to librte_mbuf (yes, currently librte_meter does not
> depend on librte_mbuf)

4/ Keep color as uint8_t in rte_mbuf and 'move enum rte_color' definition and related functions in rte_meter.

My preference is either 3) or 4).
Konstantin