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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, December 19, 2018 10:49 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; david.marchand@redhat.com;
> olivier.matz@6wind.com
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
> 
> 
> 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?

Examples in librte_eal/common/include: random, reciprocal, bitmap, alarm, branch prediction, debug, log, fbarray, hexdump, keep alive, string functions, tailq, etc.

You might agree with me these are utilities that are not even used by EAL, therefor not really part of EAL.

Color would logically belong to librte_net, but librte_net currently depends on mbuf, which IMO is totally incorrect, as network protocols existed before DPD mbuf came into existence.

> 
> > 	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.

Yes, 4/ is what we are implementing in V5.

> 
> My preference is either 3) or 4).
> Konstantin