[v2,2/3] eal: add new rte color definition
Checks
Commit Message
Added new rte_color definition in eal to
consolidate color definition which is currently replicated
in various places such as rte_meter.h, rte_tm.h and rte_mtr.h
Removed rte_tm_color and rte_mtr_color and used the new color
definition in rte_tm* and rte_mtr* files.
Updated softnic and testpmd to use new color definition.
Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
app/test-pmd/cmdline_mtr.c | 47 +++++++++++----------
app/test-pmd/cmdline_tm.c | 23 +++++-----
drivers/net/softnic/rte_eth_softnic_flow.c | 10 +++--
drivers/net/softnic/rte_eth_softnic_meter.c | 28 ++++++------
drivers/net/softnic/rte_eth_softnic_tm.c | 31 ++++++++------
lib/librte_eal/common/Makefile | 2 +-
lib/librte_eal/common/include/rte_color.h | 18 ++++++++
lib/librte_ethdev/rte_mtr.c | 2 +-
lib/librte_ethdev/rte_mtr.h | 21 +++------
lib/librte_ethdev/rte_mtr_driver.h | 2 +-
lib/librte_ethdev/rte_tm.h | 25 ++++-------
lib/librte_meter/rte_meter.h | 7 +++
12 files changed, 118 insertions(+), 98 deletions(-)
create mode 100644 lib/librte_eal/common/include/rte_color.h
Comments
Hi Reshma,
> -----Original Message-----
> From: Pattan, Reshma
> Sent: Friday, December 7, 2018 2:32 PM
> To: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>
> Subject: [PATCH v2 2/3] eal: add new rte color definition
>
> Added new rte_color definition in eal to
> consolidate color definition which is currently replicated
> in various places such as rte_meter.h, rte_tm.h and rte_mtr.h
>
> Removed rte_tm_color and rte_mtr_color and used the new color
> definition in rte_tm* and rte_mtr* files.
>
> Updated softnic and testpmd to use new color definition.
>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
> app/test-pmd/cmdline_mtr.c | 47 +++++++++++----------
> app/test-pmd/cmdline_tm.c | 23 +++++-----
> drivers/net/softnic/rte_eth_softnic_flow.c | 10 +++--
> drivers/net/softnic/rte_eth_softnic_meter.c | 28 ++++++------
> drivers/net/softnic/rte_eth_softnic_tm.c | 31 ++++++++------
> lib/librte_eal/common/Makefile | 2 +-
> lib/librte_eal/common/include/rte_color.h | 18 ++++++++
> lib/librte_ethdev/rte_mtr.c | 2 +-
> lib/librte_ethdev/rte_mtr.h | 21 +++------
> lib/librte_ethdev/rte_mtr_driver.h | 2 +-
> lib/librte_ethdev/rte_tm.h | 25 ++++-------
> lib/librte_meter/rte_meter.h | 7 +++
> 12 files changed, 118 insertions(+), 98 deletions(-)
> create mode 100644 lib/librte_eal/common/include/rte_color.h
>
This patch should come first in the set, i.e. before the redefinition of the sched field in struct rte_mbuf, as it defines the format for the new sched color field.
At this point, we should not remove the usage of rte_meter_color, rte_mtr_color, rte_mtr_color in any of the files using them, as this will break people's apps. We should first deprecate them by adding a deprecation note in the current release and then in one of the next releases (hopefully 19.5) remove them completely by replacing with the new rte_color, as you do in this patch. So, for now, please do not do this replacement in any of the above files. Bottom line: remove (for now) the above changes in the .c files of test-pmd, softnic, librte_ethdev; keep the above changes in the .h files such as rte_meter.h, rte_tm.h, rte_mtr.h. Makes sense? This patch should be very small :)
<snip>
> diff --git a/lib/librte_eal/common/include/rte_color.h
> b/lib/librte_eal/common/include/rte_color.h
> new file mode 100644
> index 000000000..f4387071b
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_color.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _RTE_COLOR_H_
> +#define _RTE_COLOR_H_
> +
> +/**
> + * Color
> + */
> +enum rte_color {
> + RTE_COLOR_GREEN = 0, /**< Green */
> + RTE_COLOR_YELLOW, /**< Yellow */
> + RTE_COLOR_RED, /**< Red */
> + RTE_COLORS /**< Number of colors */
> +};
> +
> +#endif /* _RTE_COLOR_H_ */
Please add the C++ pattern at the beginning and end of this file:
#ifndef __INCLUDE_RTE_COLOR__
#define __INCLUDE_RTE_COLOR__
#ifdef __cplusplus
extern "C" {
#endif
#ifdef __cplusplus
}
#endif
#endif
<snip>
> diff --git a/lib/librte_ethdev/rte_mtr.h b/lib/librte_ethdev/rte_mtr.h
> index c4819b274..113db06a9 100644
> --- a/lib/librte_ethdev/rte_mtr.h
> +++ b/lib/librte_ethdev/rte_mtr.h
> @@ -76,21 +76,12 @@
> #include <stdint.h>
> #include <rte_compat.h>
> #include <rte_common.h>
> +#include <rte_color.h>
>
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> -/**
> - * Color
> - */
> -enum rte_mtr_color {
> - RTE_MTR_GREEN = 0, /**< Green */
> - RTE_MTR_YELLOW, /**< Yellow */
> - RTE_MTR_RED, /**< Red */
> - RTE_MTR_COLORS /**< Number of colors. */
> -};
> -
We should not use rte_color yet, we should simply create alias of current rte_mtr_color enum and values to the new rte_color enum and enum values, just like you do it below for rte_meter.h. So please remove the rest of the changes for now.
<snip>
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index 646ef3880..95f322313 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -51,6 +51,7 @@
> #include <stdint.h>
>
> #include <rte_common.h>
> +#include <rte_color.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -115,16 +116,6 @@ extern "C" {
> */
> #define RTE_TM_NODE_LEVEL_ID_ANY UINT32_MAX
>
> -/**
> - * Color
> - */
> -enum rte_tm_color {
> - RTE_TM_GREEN = 0, /**< Green */
> - RTE_TM_YELLOW, /**< Yellow */
> - RTE_TM_RED, /**< Red */
> - RTE_TM_COLORS /**< Number of colors */
> -};
> -
We should not use rte_color yet, we should simply create alias of current rte_tm_color enum and values to the new rte_color enum and enum values, just like you do it below for rte_meter.h. So please remove the rest of the changes for now.
<snip>
> diff --git a/lib/librte_meter/rte_meter.h b/lib/librte_meter/rte_meter.h
> index 58a051583..e6187c49d 100644
> --- a/lib/librte_meter/rte_meter.h
> +++ b/lib/librte_meter/rte_meter.h
> @@ -21,6 +21,7 @@ extern "C" {
>
> #include <stdint.h>
>
> +#include <rte_color.h>
> /*
> * Application Programmer's Interface (API)
> *
> @@ -34,6 +35,12 @@ enum rte_meter_color {
> e_RTE_METER_COLORS /**< Number of available colors */
> };
>
> +/* New rte_color is defined and used to deprecate rte_meter_color soon.
> */
> +#define rte_meter_color rte_color
> +#define e_RTE_METER_GREEN RTE_COLOR_GREEN
> +#define e_RTE_METER_YELLOW RTE_COLOR_YELLOW
> +#define e_RTE_METER_RED RTE_COLOR_RED
> +
> /** srTCM parameters per metered traffic flow. The CIR, CBS and EBS
> parameters only
> count bytes of IP packets and do not include link specific headers. At least
> one of
> the CBS or EBS parameters has to be greater than zero. */
> --
> 2.17.1
The changes for rte_meter.h are perfect, please do the same to rte_mtr.h and rte_tm.h.
Regards,
Cristian
Hi Reshma,
> diff --git a/lib/librte_eal/common/include/rte_color.h b/lib/librte_eal/common/include/rte_color.h
> new file mode 100644
> index 000000000..f4387071b
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_color.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _RTE_COLOR_H_
> +#define _RTE_COLOR_H_
> +
> +/**
> + * Color
> + */
> +enum rte_color {
> + RTE_COLOR_GREEN = 0, /**< Green */
> + RTE_COLOR_YELLOW, /**< Yellow */
> + RTE_COLOR_RED, /**< Red */
> + RTE_COLORS /**< Number of colors */
> +};
Does it really belong to EAL?
Konstantin
> +
> +#endif /* _RTE_COLOR_H_ */
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, December 14, 2018 11:36 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> Hi Reshma,
>
> > diff --git a/lib/librte_eal/common/include/rte_color.h
> b/lib/librte_eal/common/include/rte_color.h
> > new file mode 100644
> > index 000000000..f4387071b
> > --- /dev/null
> > +++ b/lib/librte_eal/common/include/rte_color.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation
> > + */
> > +
> > +#ifndef _RTE_COLOR_H_
> > +#define _RTE_COLOR_H_
> > +
> > +/**
> > + * Color
> > + */
> > +enum rte_color {
> > + RTE_COLOR_GREEN = 0, /**< Green */
> > + RTE_COLOR_YELLOW, /**< Yellow */
> > + RTE_COLOR_RED, /**< Red */
> > + RTE_COLORS /**< Number of colors */
> > +};
>
> Does it really belong to EAL?
> Konstantin
>
Why not?
It needs to be visible to multiple libraries: ethdev, meter, sched, as well as drivers. We'd like to avoid adding more complexity to dependencies between libraries.
It is very generic. EAL common/include is currently the place to put generic data structures, functions, algs, etc that are widely used by DPDK libraries. Lots of similar examples are easy to find in this folder.
Where else would you put it?
> > +
> > +#endif /* _RTE_COLOR_H_ */
On 2018-12-15 00:35, Ananyev, Konstantin wrote:
> Hi Reshma,
>
>> diff --git a/lib/librte_eal/common/include/rte_color.h b/lib/librte_eal/common/include/rte_color.h
>> new file mode 100644
>> index 000000000..f4387071b
>> --- /dev/null
>> +++ b/lib/librte_eal/common/include/rte_color.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2018 Intel Corporation
>> + */
>> +
>> +#ifndef _RTE_COLOR_H_
>> +#define _RTE_COLOR_H_
>> +
>> +/**
>> + * Color
>> + */
>> +enum rte_color {
>> + RTE_COLOR_GREEN = 0, /**< Green */
>> + RTE_COLOR_YELLOW, /**< Yellow */
>> + RTE_COLOR_RED, /**< Red */
>> + RTE_COLORS /**< Number of colors */
>> +};
>
> Does it really belong to EAL?
> Konstantin
>
If this is supposed to be a generic type, we definitely need
RTE_COLOR_BLACK as well, or RTE_COLOR_VERY_VERY_DARK_GREY.
/Batman
> -----Original Message-----
> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> Sent: Saturday, December 15, 2018 2:16 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; dev@dpdk.org; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; jerin.jacob@caviumnetworks.com; Singh,
> Jasvinder <jasvinder.singh@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> On 2018-12-15 00:35, Ananyev, Konstantin wrote:
> > Hi Reshma,
> >
> >> diff --git a/lib/librte_eal/common/include/rte_color.h
> >> b/lib/librte_eal/common/include/rte_color.h
> >> new file mode 100644
> >> index 000000000..f4387071b
> >> --- /dev/null
> >> +++ b/lib/librte_eal/common/include/rte_color.h
> >> @@ -0,0 +1,18 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2018 Intel Corporation */
> >> +
> >> +#ifndef _RTE_COLOR_H_
> >> +#define _RTE_COLOR_H_
> >> +
> >> +/**
> >> + * Color
> >> + */
> >> +enum rte_color {
> >> + RTE_COLOR_GREEN = 0, /**< Green */
> >> + RTE_COLOR_YELLOW, /**< Yellow */
> >> + RTE_COLOR_RED, /**< Red */
> >> + RTE_COLORS /**< Number of colors */ };
> >
> > Does it really belong to EAL?
> > Konstantin
> >
>
> If this is supposed to be a generic type, we definitely need RTE_COLOR_BLACK
> as well, or RTE_COLOR_VERY_VERY_DARK_GREY.
>
Ok, I can add RTE_COLOR_BLACK after RTE_COLOR_RED now.
Thanks,
Reshma
> -----Original Message-----
> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> Sent: Saturday, December 15, 2018 2:16 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; dev@dpdk.org; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; jerin.jacob@caviumnetworks.com; Singh,
> Jasvinder <jasvinder.singh@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> On 2018-12-15 00:35, Ananyev, Konstantin wrote:
> > Hi Reshma,
> >
> >> diff --git a/lib/librte_eal/common/include/rte_color.h
> b/lib/librte_eal/common/include/rte_color.h
> >> new file mode 100644
> >> index 000000000..f4387071b
> >> --- /dev/null
> >> +++ b/lib/librte_eal/common/include/rte_color.h
> >> @@ -0,0 +1,18 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2018 Intel Corporation
> >> + */
> >> +
> >> +#ifndef _RTE_COLOR_H_
> >> +#define _RTE_COLOR_H_
> >> +
> >> +/**
> >> + * Color
> >> + */
> >> +enum rte_color {
> >> + RTE_COLOR_GREEN = 0, /**< Green */
> >> + RTE_COLOR_YELLOW, /**< Yellow */
> >> + RTE_COLOR_RED, /**< Red */
> >> + RTE_COLORS /**< Number of colors */
> >> +};
> >
> > Does it really belong to EAL?
> > Konstantin
> >
>
> If this is supposed to be a generic type, we definitely need
> RTE_COLOR_BLACK as well, or RTE_COLOR_VERY_VERY_DARK_GREY.
>
> /Batman
Hi Mattias,
The packet color values of (green, yellow, red) are not my invention, they are part of the IETF DiffServ foundation and standardized by a long list of RFCs, with just a few of them listed below:
RFC 2697 - srTCM
RFC 2698 - trTCM
RFC 4115 - trTCM
RFC 2597 - Assured Forwarding
It is also easy to check in the documentation from Cisco, Juniper and other router vendors that the packet color values used for traffic metering and policing are always (green, yellow, red).
So, bottom line:
1. This is a formal as well as a de facto standard, to me it makes no sense for DPDK to do it differently. Why not align to the formal and de facto industry standards?
2. DPDK already recognized this in a number of its APIs, such as: rte_meter, ethdev rte_mtr, ethdev rte_tm, rte_sched. This patch is simply a cosmetic consolidation of the packet color definition for easier integration of all these API, it does not propose any API change. If you want to change any of these APIs, please describe the motivation and send a separate patch for review.
Regards,
Cristian
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Saturday, December 15, 2018 12:16 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
>
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Friday, December 14, 2018 11:36 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
> >
> > Hi Reshma,
> >
> > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > b/lib/librte_eal/common/include/rte_color.h
> > > new file mode 100644
> > > index 000000000..f4387071b
> > > --- /dev/null
> > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > @@ -0,0 +1,18 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright(c) 2018 Intel Corporation
> > > + */
> > > +
> > > +#ifndef _RTE_COLOR_H_
> > > +#define _RTE_COLOR_H_
> > > +
> > > +/**
> > > + * Color
> > > + */
> > > +enum rte_color {
> > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > + RTE_COLOR_RED, /**< Red */
> > > + RTE_COLORS /**< Number of colors */
> > > +};
> >
> > Does it really belong to EAL?
> > Konstantin
> >
>
> Why not?
>
> It needs to be visible to multiple libraries: ethdev, meter, sched, as well as drivers. We'd like to avoid adding more complexity to
> dependencies between libraries.
>
> It is very generic. EAL common/include is currently the place to put generic data structures, functions, algs, etc that are widely used by
> DPDK libraries. Lots of similar examples are easy to find in this folder.
I don't think it is *that* generic to be in EAL.
Yes it is used by few libs, ethdev and by softnic PMD,
but it doesn't look as core dpdk thing to me.
>
> Where else would you put it?
If it defines format of rte_mbuf fileds,
then probably new .h inside librte_mbuf is a good place.
Other alternatives would be rte_ethdev or rte_net.
Konstantin
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, December 17, 2018 11:21 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
>
>
> > -----Original Message-----
> > From: Dumitrescu, Cristian
> > Sent: Saturday, December 15, 2018 12:16 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan, Reshma
> > <reshma.pattan@intel.com>; dev@dpdk.org;
> > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > definition
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Friday, December 14, 2018 11:36 PM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > <jasvinder.singh@intel.com>
> > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > definition
> > >
> > > Hi Reshma,
> > >
> > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > b/lib/librte_eal/common/include/rte_color.h
> > > > new file mode 100644
> > > > index 000000000..f4387071b
> > > > --- /dev/null
> > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > @@ -0,0 +1,18 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(c) 2018 Intel Corporation */
> > > > +
> > > > +#ifndef _RTE_COLOR_H_
> > > > +#define _RTE_COLOR_H_
> > > > +
> > > > +/**
> > > > + * Color
> > > > + */
> > > > +enum rte_color {
> > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > + RTE_COLOR_RED, /**< Red */
> > > > + RTE_COLORS /**< Number of colors */ };
> > >
> > > Does it really belong to EAL?
> > > Konstantin
> > >
> >
> > Why not?
> >
> > It needs to be visible to multiple libraries: ethdev, meter, sched, as
> > well as drivers. We'd like to avoid adding more complexity to dependencies
> between libraries.
> >
> > It is very generic. EAL common/include is currently the place to put
> > generic data structures, functions, algs, etc that are widely used by DPDK
> libraries. Lots of similar examples are easy to find in this folder.
>
> I don't think it is *that* generic to be in EAL.
> Yes it is used by few libs, ethdev and by softnic PMD,
> but it doesn't look as core dpdk thing to me.
>
> >
> > Where else would you put it?
>
> If it defines format of rte_mbuf fileds, then probably new .h inside librte_mbuf is
> a good place.
> Other alternatives would be rte_ethdev or rte_net.
After going through the lib/Makefile dependencies, I see we can have rte_color.h in eal or mbuf library only.
Cannot keep it inside ethdev or net libraries because these two libraries already have dependency on mbuf library, so cannot create loop dependency.
Snippet
1) DEPDIRS-librte_eal := librte_kvargs
2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
DEPDIRS-librte_ethdev += librte_mbuf
DEPDIRS-librte_ethdev += librte_kvargs
DEPDIRS-librte_ethdev += librte_cmdline
4) DEPDIRS-librte_net := librte_mbuf librte_eal
5) DEPDIRS-librte_meter := librte_eal
Thanks,
Reshma
> -----Original Message-----
> From: Pattan, Reshma
> Sent: Monday, December 17, 2018 5:15 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Dumitrescu,
> Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
>
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Monday, December 17, 2018 11:21 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Pattan, Reshma
> > <reshma.pattan@intel.com>; dev@dpdk.org;
> > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
> >
> >
> >
> > > -----Original Message-----
> > > From: Dumitrescu, Cristian
> > > Sent: Saturday, December 15, 2018 12:16 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan,
> Reshma
> > > <reshma.pattan@intel.com>; dev@dpdk.org;
> > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > <jasvinder.singh@intel.com>
> > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > definition
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Friday, December 14, 2018 11:36 PM
> > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > > <jasvinder.singh@intel.com>
> > > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > > definition
> > > >
> > > > Hi Reshma,
> > > >
> > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > new file mode 100644
> > > > > index 000000000..f4387071b
> > > > > --- /dev/null
> > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > @@ -0,0 +1,18 @@
> > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > +
> > > > > +#ifndef _RTE_COLOR_H_
> > > > > +#define _RTE_COLOR_H_
> > > > > +
> > > > > +/**
> > > > > + * Color
> > > > > + */
> > > > > +enum rte_color {
> > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > + RTE_COLOR_RED, /**< Red */
> > > > > + RTE_COLORS /**< Number of colors */ };
> > > >
> > > > Does it really belong to EAL?
> > > > Konstantin
> > > >
> > >
> > > Why not?
> > >
> > > It needs to be visible to multiple libraries: ethdev, meter, sched, as
> > > well as drivers. We'd like to avoid adding more complexity to
> dependencies
> > between libraries.
> > >
> > > It is very generic. EAL common/include is currently the place to put
> > > generic data structures, functions, algs, etc that are widely used by DPDK
> > libraries. Lots of similar examples are easy to find in this folder.
> >
> > I don't think it is *that* generic to be in EAL.
> > Yes it is used by few libs, ethdev and by softnic PMD,
> > but it doesn't look as core dpdk thing to me.
> >
> > >
> > > Where else would you put it?
> >
> > If it defines format of rte_mbuf fileds, then probably new .h inside
> librte_mbuf is
> > a good place.
> > Other alternatives would be rte_ethdev or rte_net.
>
> After going through the lib/Makefile dependencies, I see we can have
> rte_color.h in eal or mbuf library only.
> Cannot keep it inside ethdev or net libraries because these two libraries
> already have dependency on mbuf library, so cannot create loop
> dependency.
>
> Snippet
>
> 1) DEPDIRS-librte_eal := librte_kvargs
>
> 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
>
> 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
> DEPDIRS-librte_ethdev += librte_mbuf
> DEPDIRS-librte_ethdev += librte_kvargs
> DEPDIRS-librte_ethdev += librte_cmdline
>
> 4) DEPDIRS-librte_net := librte_mbuf librte_eal
>
> 5) DEPDIRS-librte_meter := librte_eal
>
> Thanks,
> Reshma
Yes, I wound not mind to put this header file in librte_net, it makes sense to me. But librte_net depends on librte_mbuf, so then librte_net is not an option.
The only two options are librte_eal and librte_mbuf. Between these two, my vote was librte_eal (as we already have plenty of similar items in librte_eal/common/include) instead of librte_mbuf, as to me the packet color is not related to how DPDK decides to pick its packet meta-data.
To me, librte_eal/common/include is still the best option, but I guess I can live with librte_mbuf in case Konstantin has a hard opinion on it.
What is your choice, Konstantin?
On 2018-12-17 10:41, Dumitrescu, Cristian wrote:
>
>
>> -----Original Message-----
>> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
>> Sent: Saturday, December 15, 2018 2:16 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan, Reshma
>> <reshma.pattan@intel.com>; dev@dpdk.org; Dumitrescu, Cristian
>> <cristian.dumitrescu@intel.com>; jerin.jacob@caviumnetworks.com; Singh,
>> Jasvinder <jasvinder.singh@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>>
>> On 2018-12-15 00:35, Ananyev, Konstantin wrote:
>>> Hi Reshma,
>>>
>>>> diff --git a/lib/librte_eal/common/include/rte_color.h
>> b/lib/librte_eal/common/include/rte_color.h
>>>> new file mode 100644
>>>> index 000000000..f4387071b
>>>> --- /dev/null
>>>> +++ b/lib/librte_eal/common/include/rte_color.h
>>>> @@ -0,0 +1,18 @@
>>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>>> + * Copyright(c) 2018 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef _RTE_COLOR_H_
>>>> +#define _RTE_COLOR_H_
>>>> +
>>>> +/**
>>>> + * Color
>>>> + */
>>>> +enum rte_color {
>>>> + RTE_COLOR_GREEN = 0, /**< Green */
>>>> + RTE_COLOR_YELLOW, /**< Yellow */
>>>> + RTE_COLOR_RED, /**< Red */
>>>> + RTE_COLORS /**< Number of colors */
>>>> +};
>>>
>>> Does it really belong to EAL?
>>> Konstantin
>>>
>>
>> If this is supposed to be a generic type, we definitely need
>> RTE_COLOR_BLACK as well, or RTE_COLOR_VERY_VERY_DARK_GREY.
>>
>> /Batman
>
> Hi Mattias,
>
> The packet color values of (green, yellow, red) are not my invention, they are part of the IETF DiffServ foundation and standardized by a long list of RFCs, with just a few of them listed below:
>
> RFC 2697 - srTCM
> RFC 2698 - trTCM
> RFC 4115 - trTCM
> RFC 2597 - Assured Forwarding
>
> It is also easy to check in the documentation from Cisco, Juniper and other router vendors that the packet color values used for traffic metering and policing are always (green, yellow, red).
>
> So, bottom line:
> 1. This is a formal as well as a de facto standard, to me it makes no sense for DPDK to do it differently. Why not align to the formal and de facto industry standards?
> 2. DPDK already recognized this in a number of its APIs, such as: rte_meter, ethdev rte_mtr, ethdev rte_tm, rte_sched. This patch is simply a cosmetic consolidation of the packet color definition for easier integration of all these API, it does not propose any API change. If you want to change any of these APIs, please describe the motivation and send a separate patch for review.
>
It was a joke. Also, I'm not actually Batman. Sorry for the confusion.
17/12/2018 19:51, Dumitrescu, Cristian:
> From: Pattan, Reshma
> > From: Ananyev, Konstantin
> > > From: Dumitrescu, Cristian
> > > > From: Ananyev, Konstantin
> > > > > > --- /dev/null
> > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > +enum rte_color {
> > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > >
> > > > > Does it really belong to EAL?
> > > > > Konstantin
> > > >
> > > > Why not?
> > > >
> > > > It needs to be visible to multiple libraries: ethdev, meter, sched, as
> > > > well as drivers. We'd like to avoid adding more complexity to
> > dependencies
> > > between libraries.
> > > >
> > > > It is very generic. EAL common/include is currently the place to put
> > > > generic data structures, functions, algs, etc that are widely used by DPDK
> > > libraries. Lots of similar examples are easy to find in this folder.
> > >
> > > I don't think it is *that* generic to be in EAL.
> > > Yes it is used by few libs, ethdev and by softnic PMD,
> > > but it doesn't look as core dpdk thing to me.
> > >
> > > >
> > > > Where else would you put it?
> > >
> > > If it defines format of rte_mbuf fileds, then probably new .h inside
> > librte_mbuf is
> > > a good place.
> > > Other alternatives would be rte_ethdev or rte_net.
> >
> > After going through the lib/Makefile dependencies, I see we can have
> > rte_color.h in eal or mbuf library only.
> > Cannot keep it inside ethdev or net libraries because these two libraries
> > already have dependency on mbuf library, so cannot create loop
> > dependency.
> >
> > Snippet
> >
> > 1) DEPDIRS-librte_eal := librte_kvargs
> >
> > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> >
> > 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
> > DEPDIRS-librte_ethdev += librte_mbuf
> > DEPDIRS-librte_ethdev += librte_kvargs
> > DEPDIRS-librte_ethdev += librte_cmdline
> >
> > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> >
> > 5) DEPDIRS-librte_meter := librte_eal
> >
> > Thanks,
> > Reshma
>
> Yes, I wound not mind to put this header file in librte_net, it makes sense to me. But librte_net depends on librte_mbuf, so then librte_net is not an option.
>
> The only two options are librte_eal and librte_mbuf. Between these two, my vote was librte_eal (as we already have plenty of similar items in librte_eal/common/include) instead of librte_mbuf, as to me the packet color is not related to how DPDK decides to pick its packet meta-data.
>
> To me, librte_eal/common/include is still the best option, but I guess I can live with librte_mbuf in case Konstantin has a hard opinion on it.
>
> What is your choice, Konstantin?
I replied in v3 that it should stay in rte_meter.h.
You can include rte_meter.h in ethdev.
The other option, agreed by Reshma, is to add black color ;)
Note: I did not see this discussion on v2 because the versions are
not in the same thread. Have I already asked to use --in-reply-to please?
On 12/17/2018 7:36 PM, Mattias Rönnblom wrote:
> On 2018-12-17 10:41, Dumitrescu, Cristian wrote:
>>
>>
>>> -----Original Message-----
>>> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
>>> Sent: Saturday, December 15, 2018 2:16 PM
>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan, Reshma
>>> <reshma.pattan@intel.com>; dev@dpdk.org; Dumitrescu, Cristian
>>> <cristian.dumitrescu@intel.com>; jerin.jacob@caviumnetworks.com; Singh,
>>> Jasvinder <jasvinder.singh@intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>>>
>>> On 2018-12-15 00:35, Ananyev, Konstantin wrote:
>>>> Hi Reshma,
>>>>
>>>>> diff --git a/lib/librte_eal/common/include/rte_color.h
>>> b/lib/librte_eal/common/include/rte_color.h
>>>>> new file mode 100644
>>>>> index 000000000..f4387071b
>>>>> --- /dev/null
>>>>> +++ b/lib/librte_eal/common/include/rte_color.h
>>>>> @@ -0,0 +1,18 @@
>>>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>>>> + * Copyright(c) 2018 Intel Corporation
>>>>> + */
>>>>> +
>>>>> +#ifndef _RTE_COLOR_H_
>>>>> +#define _RTE_COLOR_H_
>>>>> +
>>>>> +/**
>>>>> + * Color
>>>>> + */
>>>>> +enum rte_color {
>>>>> + RTE_COLOR_GREEN = 0, /**< Green */
>>>>> + RTE_COLOR_YELLOW, /**< Yellow */
>>>>> + RTE_COLOR_RED, /**< Red */
>>>>> + RTE_COLORS /**< Number of colors */
>>>>> +};
>>>>
>>>> Does it really belong to EAL?
>>>> Konstantin
>>>>
>>>
>>> If this is supposed to be a generic type, we definitely need
>>> RTE_COLOR_BLACK as well, or RTE_COLOR_VERY_VERY_DARK_GREY.
>>>
>>> /Batman
>>
>> Hi Mattias,
>>
>> The packet color values of (green, yellow, red) are not my invention, they are part of the IETF DiffServ foundation and standardized by a long list of RFCs, with just a few of them listed below:
>>
>> RFC 2697 - srTCM
>> RFC 2698 - trTCM
>> RFC 4115 - trTCM
>> RFC 2597 - Assured Forwarding
>>
>> It is also easy to check in the documentation from Cisco, Juniper and other router vendors that the packet color values used for traffic metering and policing are always (green, yellow, red).
>>
>> So, bottom line:
>> 1. This is a formal as well as a de facto standard, to me it makes no sense for DPDK to do it differently. Why not align to the formal and de facto industry standards?
>> 2. DPDK already recognized this in a number of its APIs, such as: rte_meter, ethdev rte_mtr, ethdev rte_tm, rte_sched. This patch is simply a cosmetic consolidation of the packet color definition for easier integration of all these API, it does not propose any API change. If you want to change any of these APIs, please describe the motivation and send a separate patch for review.
>>
>
> It was a joke. Also, I'm not actually Batman. Sorry for the confusion.
This is hilarious :), although I am a bit disappointed to not have
RTE_COLOR_VERY_VERY_DARK_GREY
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Monday, December 17, 2018 6:51 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder <jasvinder.singh@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
>
>
> > -----Original Message-----
> > From: Pattan, Reshma
> > Sent: Monday, December 17, 2018 5:15 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Dumitrescu,
> > Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Monday, December 17, 2018 11:21 AM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Pattan, Reshma
> > > <reshma.pattan@intel.com>; dev@dpdk.org;
> > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > <jasvinder.singh@intel.com>
> > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Dumitrescu, Cristian
> > > > Sent: Saturday, December 15, 2018 12:16 AM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan,
> > Reshma
> > > > <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > > <jasvinder.singh@intel.com>
> > > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > > definition
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ananyev, Konstantin
> > > > > Sent: Friday, December 14, 2018 11:36 PM
> > > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > > Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > > > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > > > <jasvinder.singh@intel.com>
> > > > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > > > definition
> > > > >
> > > > > Hi Reshma,
> > > > >
> > > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > > new file mode 100644
> > > > > > index 000000000..f4387071b
> > > > > > --- /dev/null
> > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > @@ -0,0 +1,18 @@
> > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > > +
> > > > > > +#ifndef _RTE_COLOR_H_
> > > > > > +#define _RTE_COLOR_H_
> > > > > > +
> > > > > > +/**
> > > > > > + * Color
> > > > > > + */
> > > > > > +enum rte_color {
> > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > >
> > > > > Does it really belong to EAL?
> > > > > Konstantin
> > > > >
> > > >
> > > > Why not?
> > > >
> > > > It needs to be visible to multiple libraries: ethdev, meter, sched, as
> > > > well as drivers. We'd like to avoid adding more complexity to
> > dependencies
> > > between libraries.
> > > >
> > > > It is very generic. EAL common/include is currently the place to put
> > > > generic data structures, functions, algs, etc that are widely used by DPDK
> > > libraries. Lots of similar examples are easy to find in this folder.
> > >
> > > I don't think it is *that* generic to be in EAL.
> > > Yes it is used by few libs, ethdev and by softnic PMD,
> > > but it doesn't look as core dpdk thing to me.
> > >
> > > >
> > > > Where else would you put it?
> > >
> > > If it defines format of rte_mbuf fileds, then probably new .h inside
> > librte_mbuf is
> > > a good place.
> > > Other alternatives would be rte_ethdev or rte_net.
> >
> > After going through the lib/Makefile dependencies, I see we can have
> > rte_color.h in eal or mbuf library only.
> > Cannot keep it inside ethdev or net libraries because these two libraries
> > already have dependency on mbuf library, so cannot create loop
> > dependency.
> >
> > Snippet
> >
> > 1) DEPDIRS-librte_eal := librte_kvargs
> >
> > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> >
> > 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
> > DEPDIRS-librte_ethdev += librte_mbuf
> > DEPDIRS-librte_ethdev += librte_kvargs
> > DEPDIRS-librte_ethdev += librte_cmdline
> >
> > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> >
> > 5) DEPDIRS-librte_meter := librte_eal
> >
> > Thanks,
> > Reshma
>
> Yes, I wound not mind to put this header file in librte_net, it makes sense to me. But librte_net depends on librte_mbuf, so then librte_net is
> not an option.
>
> The only two options are librte_eal and librte_mbuf. Between these two, my vote was librte_eal (as we already have plenty of similar items
> in librte_eal/common/include) instead of librte_mbuf, as to me the packet color is not related to how DPDK decides to pick its packet meta-
> data.
>
> To me, librte_eal/common/include is still the best option, but I guess I can live with librte_mbuf in case Konstantin has a hard opinion on it.
>
> What is your choice, Konstantin?
If to choose between EAL and mbuf - I would choose mbuf,
that what I stated in my previous mail, see above.
BTW, I probably missing something, but why rte_net is not an option?
What circular dependency you are talking about?
Konstantin
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, December 18, 2018 10:15 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
>
>
> > -----Original Message-----
> > From: Dumitrescu, Cristian
> > Sent: Monday, December 17, 2018 6:51 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > definition
> >
> >
> >
> > > -----Original Message-----
> > > From: Pattan, Reshma
> > > Sent: Monday, December 17, 2018 5:15 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Dumitrescu,
> > > Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > <jasvinder.singh@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > definition
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Monday, December 17, 2018 11:21 AM
> > > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Pattan,
> > > > Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > > <jasvinder.singh@intel.com>
> > > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > > definition
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Dumitrescu, Cristian
> > > > > Sent: Saturday, December 15, 2018 12:16 AM
> > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pattan,
> > > Reshma
> > > > > <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > > > <jasvinder.singh@intel.com>
> > > > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > > > definition
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ananyev, Konstantin
> > > > > > Sent: Friday, December 14, 2018 11:36 PM
> > > > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > > > Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > > > > > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > > > > > <jasvinder.singh@intel.com>
> > > > > > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > > > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > > > > > definition
> > > > > >
> > > > > > Hi Reshma,
> > > > > >
> > > > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000..f4387071b
> > > > > > > --- /dev/null
> > > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > > @@ -0,0 +1,18 @@
> > > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > > > +
> > > > > > > +#ifndef _RTE_COLOR_H_
> > > > > > > +#define _RTE_COLOR_H_
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * Color
> > > > > > > + */
> > > > > > > +enum rte_color {
> > > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > > >
> > > > > > Does it really belong to EAL?
> > > > > > Konstantin
> > > > > >
> > > > >
> > > > > Why not?
> > > > >
> > > > > It needs to be visible to multiple libraries: ethdev, meter,
> > > > > sched, as well as drivers. We'd like to avoid adding more
> > > > > complexity to
> > > dependencies
> > > > between libraries.
> > > > >
> > > > > It is very generic. EAL common/include is currently the place to
> > > > > put generic data structures, functions, algs, etc that are
> > > > > widely used by DPDK
> > > > libraries. Lots of similar examples are easy to find in this folder.
> > > >
> > > > I don't think it is *that* generic to be in EAL.
> > > > Yes it is used by few libs, ethdev and by softnic PMD, but it
> > > > doesn't look as core dpdk thing to me.
> > > >
> > > > >
> > > > > Where else would you put it?
> > > >
> > > > If it defines format of rte_mbuf fileds, then probably new .h
> > > > inside
> > > librte_mbuf is
> > > > a good place.
> > > > Other alternatives would be rte_ethdev or rte_net.
> > >
> > > After going through the lib/Makefile dependencies, I see we can have
> > > rte_color.h in eal or mbuf library only.
> > > Cannot keep it inside ethdev or net libraries because these two
> > > libraries already have dependency on mbuf library, so cannot create
> > > loop dependency.
> > >
> > > Snippet
> > >
> > > 1) DEPDIRS-librte_eal := librte_kvargs
> > >
> > > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > >
> > > 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool
> > > librte_ring DEPDIRS-librte_ethdev += librte_mbuf
> > > DEPDIRS-librte_ethdev += librte_kvargs DEPDIRS-librte_ethdev +=
> > > librte_cmdline
> > >
> > > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> > >
> > > 5) DEPDIRS-librte_meter := librte_eal
> > >
> > > Thanks,
> > > Reshma
> >
> > Yes, I wound not mind to put this header file in librte_net, it makes
> > sense to me. But librte_net depends on librte_mbuf, so then librte_net is not
> an option.
> >
> > The only two options are librte_eal and librte_mbuf. Between these
> > two, my vote was librte_eal (as we already have plenty of similar
> > items in librte_eal/common/include) instead of librte_mbuf, as to me the
> packet color is not related to how DPDK decides to pick its packet meta- data.
> >
> > To me, librte_eal/common/include is still the best option, but I guess I can live
> with librte_mbuf in case Konstantin has a hard opinion on it.
> >
> > What is your choice, Konstantin?
>
> If to choose between EAL and mbuf - I would choose mbuf, that what I stated in
> my previous mail, see above.
> BTW, I probably missing something, but why rte_net is not an option?
> What circular dependency you are talking about?
> Konstantin
>
Since librte_net has mbuf in its dependent list as below from lib/Makefile.
i.e. DEPDIRS-librte_net := librte_mbuf librte_eal
So now, If we move rte_color.h to librte_net, then need to add librte_net to mbuf dependency list(as we are using rte_color.h in rte_mbuf.h)
Current mbuf dependency list is
DEPDIRS-librte_mbuf := librte_eal librte_mempool
The new will be
DEPDIRS-librte_mbuf := librte_eal librte_mempool librte_net
So this will create circular dependency, I think this is not allowed in DPDK right?
If my understanding is correct, librte_net is not the correct place. Please clarify.
Thanks,
Reshma
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, December 17, 2018 11:11 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> 17/12/2018 19:51, Dumitrescu, Cristian:
> > From: Pattan, Reshma
> > > From: Ananyev, Konstantin
> > > > From: Dumitrescu, Cristian
> > > > > From: Ananyev, Konstantin
> > > > > > > --- /dev/null
> > > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > > +enum rte_color {
> > > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > > >
> > > > > > Does it really belong to EAL?
> > > > > > Konstantin
> > > > >
> > > > > Why not?
> > > > >
> > > > > It needs to be visible to multiple libraries: ethdev, meter, sched, as
> > > > > well as drivers. We'd like to avoid adding more complexity to
> > > dependencies
> > > > between libraries.
> > > > >
> > > > > It is very generic. EAL common/include is currently the place to put
> > > > > generic data structures, functions, algs, etc that are widely used by
> DPDK
> > > > libraries. Lots of similar examples are easy to find in this folder.
> > > >
> > > > I don't think it is *that* generic to be in EAL.
> > > > Yes it is used by few libs, ethdev and by softnic PMD,
> > > > but it doesn't look as core dpdk thing to me.
> > > >
> > > > >
> > > > > Where else would you put it?
> > > >
> > > > If it defines format of rte_mbuf fileds, then probably new .h inside
> > > librte_mbuf is
> > > > a good place.
> > > > Other alternatives would be rte_ethdev or rte_net.
> > >
> > > After going through the lib/Makefile dependencies, I see we can have
> > > rte_color.h in eal or mbuf library only.
> > > Cannot keep it inside ethdev or net libraries because these two libraries
> > > already have dependency on mbuf library, so cannot create loop
> > > dependency.
> > >
> > > Snippet
> > >
> > > 1) DEPDIRS-librte_eal := librte_kvargs
> > >
> > > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > >
> > > 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool
> librte_ring
> > > DEPDIRS-librte_ethdev += librte_mbuf
> > > DEPDIRS-librte_ethdev += librte_kvargs
> > > DEPDIRS-librte_ethdev += librte_cmdline
> > >
> > > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> > >
> > > 5) DEPDIRS-librte_meter := librte_eal
> > >
> > > Thanks,
> > > Reshma
> >
> > Yes, I wound not mind to put this header file in librte_net, it makes sense
> to me. But librte_net depends on librte_mbuf, so then librte_net is not an
> option.
> >
> > The only two options are librte_eal and librte_mbuf. Between these two,
> my vote was librte_eal (as we already have plenty of similar items in
> librte_eal/common/include) instead of librte_mbuf, as to me the packet
> color is not related to how DPDK decides to pick its packet meta-data.
> >
> > To me, librte_eal/common/include is still the best option, but I guess I can
> live with librte_mbuf in case Konstantin has a hard opinion on it.
> >
> > What is your choice, Konstantin?
>
> I replied in v3 that it should stay in rte_meter.h.
Strange, I did not see this reply from you ...
> You can include rte_meter.h in ethdev.
OK, thanks Thomas, makes sense to me as well.
>
> The other option, agreed by Reshma, is to add black color ;)
>
> Note: I did not see this discussion on v2 because the versions are
> not in the same thread. Have I already asked to use --in-reply-to please?
>
>
Hi Reshma,
> > > > > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000..f4387071b
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > @@ -0,0 +1,18 @@
> > > > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > > > > +
> > > > > > > > +#ifndef _RTE_COLOR_H_
> > > > > > > > +#define _RTE_COLOR_H_
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * Color
> > > > > > > > + */
> > > > > > > > +enum rte_color {
> > > > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > > > >
> > > > > > > Does it really belong to EAL?
> > > > > > > Konstantin
> > > > > > >
> > > > > >
> > > > > > Why not?
> > > > > >
> > > > > > It needs to be visible to multiple libraries: ethdev, meter,
> > > > > > sched, as well as drivers. We'd like to avoid adding more
> > > > > > complexity to
> > > > dependencies
> > > > > between libraries.
> > > > > >
> > > > > > It is very generic. EAL common/include is currently the place to
> > > > > > put generic data structures, functions, algs, etc that are
> > > > > > widely used by DPDK
> > > > > libraries. Lots of similar examples are easy to find in this folder.
> > > > >
> > > > > I don't think it is *that* generic to be in EAL.
> > > > > Yes it is used by few libs, ethdev and by softnic PMD, but it
> > > > > doesn't look as core dpdk thing to me.
> > > > >
> > > > > >
> > > > > > Where else would you put it?
> > > > >
> > > > > If it defines format of rte_mbuf fileds, then probably new .h
> > > > > inside
> > > > librte_mbuf is
> > > > > a good place.
> > > > > Other alternatives would be rte_ethdev or rte_net.
> > > >
> > > > After going through the lib/Makefile dependencies, I see we can have
> > > > rte_color.h in eal or mbuf library only.
> > > > Cannot keep it inside ethdev or net libraries because these two
> > > > libraries already have dependency on mbuf library, so cannot create
> > > > loop dependency.
> > > >
> > > > Snippet
> > > >
> > > > 1) DEPDIRS-librte_eal := librte_kvargs
> > > >
> > > > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > > >
> > > > 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool
> > > > librte_ring DEPDIRS-librte_ethdev += librte_mbuf
> > > > DEPDIRS-librte_ethdev += librte_kvargs DEPDIRS-librte_ethdev +=
> > > > librte_cmdline
> > > >
> > > > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> > > >
> > > > 5) DEPDIRS-librte_meter := librte_eal
> > > >
> > > > Thanks,
> > > > Reshma
> > >
> > > Yes, I wound not mind to put this header file in librte_net, it makes
> > > sense to me. But librte_net depends on librte_mbuf, so then librte_net is not
> > an option.
> > >
> > > The only two options are librte_eal and librte_mbuf. Between these
> > > two, my vote was librte_eal (as we already have plenty of similar
> > > items in librte_eal/common/include) instead of librte_mbuf, as to me the
> > packet color is not related to how DPDK decides to pick its packet meta- data.
> > >
> > > To me, librte_eal/common/include is still the best option, but I guess I can live
> > with librte_mbuf in case Konstantin has a hard opinion on it.
> > >
> > > What is your choice, Konstantin?
> >
> > If to choose between EAL and mbuf - I would choose mbuf, that what I stated in
> > my previous mail, see above.
> > BTW, I probably missing something, but why rte_net is not an option?
> > What circular dependency you are talking about?
> > Konstantin
> >
>
> Since librte_net has mbuf in its dependent list as below from lib/Makefile.
> i.e. DEPDIRS-librte_net := librte_mbuf librte_eal
>
> So now, If we move rte_color.h to librte_net, then need to add librte_net to mbuf dependency list(as we are using rte_color.h in
> rte_mbuf.h)
>
> Current mbuf dependency list is
> DEPDIRS-librte_mbuf := librte_eal librte_mempool
>
> The new will be
> DEPDIRS-librte_mbuf := librte_eal librte_mempool librte_net
>
> So this will create circular dependency, I think this is not allowed in DPDK right?
I understand that part, but why rte_color definitions have to be visible by rte_mbuf?
Do you refer it in rte_mbuf functions?
Konstantin
>
> If my understanding is correct, librte_net is not the correct place. Please clarify.
Hi,
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, December 18, 2018 10:38 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> Hi Reshma,
>
> > > > > > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > > > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000..f4387071b
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > @@ -0,0 +1,18 @@
> > > > > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > > > > > +
> > > > > > > > > +#ifndef _RTE_COLOR_H_
> > > > > > > > > +#define _RTE_COLOR_H_
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * Color
> > > > > > > > > + */
> > > > > > > > > +enum rte_color {
> > > > > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > > > > >
> > > > > > > > Does it really belong to EAL?
> > > > > > > > Konstantin
> > > > > > > >
> > > > > > >
> > > > > > > Why not?
> > > > > > >
> > > > > > > It needs to be visible to multiple libraries: ethdev, meter,
> > > > > > > sched, as well as drivers. We'd like to avoid adding more
> > > > > > > complexity to
> > > > > dependencies
> > > > > > between libraries.
> > > > > > >
> > > > > > > It is very generic. EAL common/include is currently the
> > > > > > > place to put generic data structures, functions, algs, etc
> > > > > > > that are widely used by DPDK
> > > > > > libraries. Lots of similar examples are easy to find in this folder.
> > > > > >
> > > > > > I don't think it is *that* generic to be in EAL.
> > > > > > Yes it is used by few libs, ethdev and by softnic PMD, but it
> > > > > > doesn't look as core dpdk thing to me.
> > > > > >
> > > > > > >
> > > > > > > Where else would you put it?
> > > > > >
> > > > > > If it defines format of rte_mbuf fileds, then probably new .h
> > > > > > inside
> > > > > librte_mbuf is
> > > > > > a good place.
> > > > > > Other alternatives would be rte_ethdev or rte_net.
> > > > >
> > > > > After going through the lib/Makefile dependencies, I see we can
> > > > > have rte_color.h in eal or mbuf library only.
> > > > > Cannot keep it inside ethdev or net libraries because these two
> > > > > libraries already have dependency on mbuf library, so cannot
> > > > > create loop dependency.
> > > > >
> > > > > Snippet
> > > > >
> > > > > 1) DEPDIRS-librte_eal := librte_kvargs
> > > > >
> > > > > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > > > >
> > > > > 3)DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool
> > > > > librte_ring DEPDIRS-librte_ethdev += librte_mbuf
> > > > > DEPDIRS-librte_ethdev += librte_kvargs DEPDIRS-librte_ethdev +=
> > > > > librte_cmdline
> > > > >
> > > > > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> > > > >
> > > > > 5) DEPDIRS-librte_meter := librte_eal
> > > > >
> > > > > Thanks,
> > > > > Reshma
> > > >
> > > > Yes, I wound not mind to put this header file in librte_net, it
> > > > makes sense to me. But librte_net depends on librte_mbuf, so then
> > > > librte_net is not
> > > an option.
> > > >
> > > > The only two options are librte_eal and librte_mbuf. Between these
> > > > two, my vote was librte_eal (as we already have plenty of similar
> > > > items in librte_eal/common/include) instead of librte_mbuf, as to
> > > > me the
> > > packet color is not related to how DPDK decides to pick its packet meta-
> data.
> > > >
> > > > To me, librte_eal/common/include is still the best option, but I
> > > > guess I can live
> > > with librte_mbuf in case Konstantin has a hard opinion on it.
> > > >
> > > > What is your choice, Konstantin?
> > >
> > > If to choose between EAL and mbuf - I would choose mbuf, that what I
> > > stated in my previous mail, see above.
> > > BTW, I probably missing something, but why rte_net is not an option?
> > > What circular dependency you are talking about?
> > > Konstantin
> > >
> >
> > Since librte_net has mbuf in its dependent list as below from lib/Makefile.
> > i.e. DEPDIRS-librte_net := librte_mbuf librte_eal
> >
> > So now, If we move rte_color.h to librte_net, then need to add
> > librte_net to mbuf dependency list(as we are using rte_color.h in
> > rte_mbuf.h)
> >
> > Current mbuf dependency list is
> > DEPDIRS-librte_mbuf := librte_eal librte_mempool
> >
> > The new will be
> > DEPDIRS-librte_mbuf := librte_eal librte_mempool librte_net
> >
> > So this will create circular dependency, I think this is not allowed in DPDK right?
>
> I understand that part, but why rte_color definitions have to be visible by
> rte_mbuf?
> Do you refer it in rte_mbuf functions?
Oh yes, in 2nd patch of this patchset we have added new set/get functions in librte_mbuf, there ,we are referring the rte_color.
I am sorry I would have been more explicit in my earlier mail.
Thanks,
Reshma
> -----Original Message-----
> From: Pattan, Reshma
> Sent: Tuesday, December 18, 2018 10:41 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> Hi,
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Tuesday, December 18, 2018 10:38 AM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> > jerin.jacob@caviumnetworks.com; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color
> > definition
> >
> > Hi Reshma,
> >
> > > > > > > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000..f4387071b
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > > @@ -0,0 +1,18 @@
> > > > > > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > > > > > > +
> > > > > > > > > > +#ifndef _RTE_COLOR_H_ #define _RTE_COLOR_H_
> > > > > > > > > > +
> > > > > > > > > > +/**
> > > > > > > > > > + * Color
> > > > > > > > > > + */
> > > > > > > > > > +enum rte_color {
> > > > > > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > > > > > >
> > > > > > > > > Does it really belong to EAL?
> > > > > > > > > Konstantin
> > > > > > > > >
> > > > > > > >
> > > > > > > > Why not?
> > > > > > > >
> > > > > > > > It needs to be visible to multiple libraries: ethdev,
> > > > > > > > meter, sched, as well as drivers. We'd like to avoid
> > > > > > > > adding more complexity to
> > > > > > dependencies
> > > > > > > between libraries.
> > > > > > > >
> > > > > > > > It is very generic. EAL common/include is currently the
> > > > > > > > place to put generic data structures, functions, algs, etc
> > > > > > > > that are widely used by DPDK
> > > > > > > libraries. Lots of similar examples are easy to find in this folder.
> > > > > > >
> > > > > > > I don't think it is *that* generic to be in EAL.
> > > > > > > Yes it is used by few libs, ethdev and by softnic PMD, but
> > > > > > > it doesn't look as core dpdk thing to me.
> > > > > > >
> > > > > > > >
> > > > > > > > Where else would you put it?
> > > > > > >
> > > > > > > If it defines format of rte_mbuf fileds, then probably new
> > > > > > > .h inside
> > > > > > librte_mbuf is
> > > > > > > a good place.
> > > > > > > Other alternatives would be rte_ethdev or rte_net.
> > > > > >
> > > > > > After going through the lib/Makefile dependencies, I see we
> > > > > > can have rte_color.h in eal or mbuf library only.
> > > > > > Cannot keep it inside ethdev or net libraries because these
> > > > > > two libraries already have dependency on mbuf library, so
> > > > > > cannot create loop dependency.
> > > > > >
> > > > > > Snippet
> > > > > >
> > > > > > 1) DEPDIRS-librte_eal := librte_kvargs
> > > > > >
> > > > > > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > > > > >
> > > > > > 3)DEPDIRS-librte_ethdev := librte_net librte_eal
> > > > > > librte_mempool librte_ring DEPDIRS-librte_ethdev +=
> > > > > > librte_mbuf DEPDIRS-librte_ethdev += librte_kvargs
> > > > > > DEPDIRS-librte_ethdev += librte_cmdline
> > > > > >
> > > > > > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> > > > > >
> > > > > > 5) DEPDIRS-librte_meter := librte_eal
> > > > > >
> > > > > > Thanks,
> > > > > > Reshma
> > > > >
> > > > > Yes, I wound not mind to put this header file in librte_net, it
> > > > > makes sense to me. But librte_net depends on librte_mbuf, so
> > > > > then librte_net is not
> > > > an option.
> > > > >
> > > > > The only two options are librte_eal and librte_mbuf. Between
> > > > > these two, my vote was librte_eal (as we already have plenty of
> > > > > similar items in librte_eal/common/include) instead of
> > > > > librte_mbuf, as to me the
> > > > packet color is not related to how DPDK decides to pick its packet
> > > > meta-
> > data.
> > > > >
> > > > > To me, librte_eal/common/include is still the best option, but I
> > > > > guess I can live
> > > > with librte_mbuf in case Konstantin has a hard opinion on it.
> > > > >
> > > > > What is your choice, Konstantin?
> > > >
> > > > If to choose between EAL and mbuf - I would choose mbuf, that what
> > > > I stated in my previous mail, see above.
> > > > BTW, I probably missing something, but why rte_net is not an option?
> > > > What circular dependency you are talking about?
> > > > Konstantin
> > > >
> > >
> > > Since librte_net has mbuf in its dependent list as below from lib/Makefile.
> > > i.e. DEPDIRS-librte_net := librte_mbuf librte_eal
> > >
> > > So now, If we move rte_color.h to librte_net, then need to add
> > > librte_net to mbuf dependency list(as we are using rte_color.h in
> > > rte_mbuf.h)
> > >
> > > Current mbuf dependency list is
> > > DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > >
> > > The new will be
> > > DEPDIRS-librte_mbuf := librte_eal librte_mempool librte_net
> > >
> > > So this will create circular dependency, I think this is not allowed in DPDK
> right?
> >
> > I understand that part, but why rte_color definitions have to be
> > visible by rte_mbuf?
> > Do you refer it in rte_mbuf functions?
>
>
> Oh yes, in 2nd patch of this patchset we have added new set/get functions in
> librte_mbuf, there ,we are referring the rte_color.
> I am sorry I would have been more explicit in my earlier mail.
>
I guess Reshma is referring to 2nd patch of the v3 version which is the latest version of this series.
Patchwork: https://patches.dpdk.org/patch/48788/
Mail list: https://mails.dpdk.org/archives/dev/2018-December/120848.html
> > 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?
Regards,
Cristian
> > > > > > > > > > > diff --git a/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > > b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000000000..f4387071b
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/lib/librte_eal/common/include/rte_color.h
> > > > > > > > > > > @@ -0,0 +1,18 @@
> > > > > > > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > > > > > > + * Copyright(c) 2018 Intel Corporation */
> > > > > > > > > > > +
> > > > > > > > > > > +#ifndef _RTE_COLOR_H_ #define _RTE_COLOR_H_
> > > > > > > > > > > +
> > > > > > > > > > > +/**
> > > > > > > > > > > + * Color
> > > > > > > > > > > + */
> > > > > > > > > > > +enum rte_color {
> > > > > > > > > > > + RTE_COLOR_GREEN = 0, /**< Green */
> > > > > > > > > > > + RTE_COLOR_YELLOW, /**< Yellow */
> > > > > > > > > > > + RTE_COLOR_RED, /**< Red */
> > > > > > > > > > > + RTE_COLORS /**< Number of colors */ };
> > > > > > > > > >
> > > > > > > > > > Does it really belong to EAL?
> > > > > > > > > > Konstantin
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Why not?
> > > > > > > > >
> > > > > > > > > It needs to be visible to multiple libraries: ethdev,
> > > > > > > > > meter, sched, as well as drivers. We'd like to avoid
> > > > > > > > > adding more complexity to
> > > > > > > dependencies
> > > > > > > > between libraries.
> > > > > > > > >
> > > > > > > > > It is very generic. EAL common/include is currently the
> > > > > > > > > place to put generic data structures, functions, algs, etc
> > > > > > > > > that are widely used by DPDK
> > > > > > > > libraries. Lots of similar examples are easy to find in this folder.
> > > > > > > >
> > > > > > > > I don't think it is *that* generic to be in EAL.
> > > > > > > > Yes it is used by few libs, ethdev and by softnic PMD, but
> > > > > > > > it doesn't look as core dpdk thing to me.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Where else would you put it?
> > > > > > > >
> > > > > > > > If it defines format of rte_mbuf fileds, then probably new
> > > > > > > > .h inside
> > > > > > > librte_mbuf is
> > > > > > > > a good place.
> > > > > > > > Other alternatives would be rte_ethdev or rte_net.
> > > > > > >
> > > > > > > After going through the lib/Makefile dependencies, I see we
> > > > > > > can have rte_color.h in eal or mbuf library only.
> > > > > > > Cannot keep it inside ethdev or net libraries because these
> > > > > > > two libraries already have dependency on mbuf library, so
> > > > > > > cannot create loop dependency.
> > > > > > >
> > > > > > > Snippet
> > > > > > >
> > > > > > > 1) DEPDIRS-librte_eal := librte_kvargs
> > > > > > >
> > > > > > > 2)DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > > > > > >
> > > > > > > 3)DEPDIRS-librte_ethdev := librte_net librte_eal
> > > > > > > librte_mempool librte_ring DEPDIRS-librte_ethdev +=
> > > > > > > librte_mbuf DEPDIRS-librte_ethdev += librte_kvargs
> > > > > > > DEPDIRS-librte_ethdev += librte_cmdline
> > > > > > >
> > > > > > > 4) DEPDIRS-librte_net := librte_mbuf librte_eal
> > > > > > >
> > > > > > > 5) DEPDIRS-librte_meter := librte_eal
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Reshma
> > > > > >
> > > > > > Yes, I wound not mind to put this header file in librte_net, it
> > > > > > makes sense to me. But librte_net depends on librte_mbuf, so
> > > > > > then librte_net is not
> > > > > an option.
> > > > > >
> > > > > > The only two options are librte_eal and librte_mbuf. Between
> > > > > > these two, my vote was librte_eal (as we already have plenty of
> > > > > > similar items in librte_eal/common/include) instead of
> > > > > > librte_mbuf, as to me the
> > > > > packet color is not related to how DPDK decides to pick its packet
> > > > > meta-
> > > data.
> > > > > >
> > > > > > To me, librte_eal/common/include is still the best option, but I
> > > > > > guess I can live
> > > > > with librte_mbuf in case Konstantin has a hard opinion on it.
> > > > > >
> > > > > > What is your choice, Konstantin?
> > > > >
> > > > > If to choose between EAL and mbuf - I would choose mbuf, that what
> > > > > I stated in my previous mail, see above.
> > > > > BTW, I probably missing something, but why rte_net is not an option?
> > > > > What circular dependency you are talking about?
> > > > > Konstantin
> > > > >
> > > >
> > > > Since librte_net has mbuf in its dependent list as below from lib/Makefile.
> > > > i.e. DEPDIRS-librte_net := librte_mbuf librte_eal
> > > >
> > > > So now, If we move rte_color.h to librte_net, then need to add
> > > > librte_net to mbuf dependency list(as we are using rte_color.h in
> > > > rte_mbuf.h)
> > > >
> > > > Current mbuf dependency list is
> > > > DEPDIRS-librte_mbuf := librte_eal librte_mempool
> > > >
> > > > The new will be
> > > > DEPDIRS-librte_mbuf := librte_eal librte_mempool librte_net
> > > >
> > > > So this will create circular dependency, I think this is not allowed in DPDK
> > right?
> > >
> > > I understand that part, but why rte_color definitions have to be
> > > visible by rte_mbuf?
> > > Do you refer it in rte_mbuf functions?
> >
> >
> > Oh yes, in 2nd patch of this patchset we have added new set/get functions in
> > librte_mbuf, there ,we are referring the rte_color.
> > I am sorry I would have been more explicit in my earlier mail.
> >
>
> I guess Reshma is referring to 2nd patch of the v3 version which is the latest version of this series.
> Patchwork: https://patches.dpdk.org/patch/48788/
> Mail list: https://mails.dpdk.org/archives/dev/2018-December/120848.html
>
Thanks for clarification.
In theory inside mbuf color is just uint8_t, so I think you can keep mrte_mbuf.h as it is,
and have rte_color and realted function definitions in different place (rte_net/rte_meter).
Another possible option is to create a new .h file inside librte_mbuf (rte_mbuf_meter.h or so),
put related struct/enum/function definitions into it and include it from rte_mbuf.h
Konstantin
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?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, December 18, 2018 12:38 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>;
> 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
>
> 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.
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.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, December 18, 2018 1:39 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> 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
>
> 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
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, December 18, 2018 5:07 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> 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
>
>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Tuesday, December 18, 2018 1:39 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>;
> > 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
> >
> > 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
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)
Personally, I can live with any of these options.
Thomas,
It would be good to have your input as well, Reshma just sent V4 implementing your proposal based on having the color enum defined in rte_meter.h, which gets included into rte_mbuf.h.
We need to make progress for RC1 deadline.
Thanks,
Cristian
18/12/2018 20:34, Dumitrescu, Cristian:
> From: Ananyev, Konstantin
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 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
> 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)
>
> Personally, I can live with any of these options.
>
> Thomas,
>
> It would be good to have your input as well, Reshma just sent V4 implementing your proposal based on having the color enum defined in rte_meter.h, which gets included into rte_mbuf.h.
We need a decision from Olivier, mbuf maintainer.
> We need to make progress for RC1 deadline.
I'm afraid 19.02 is not the right release to change the mbuf.
This kind of decision usually takes several weeks.
Cc the technical board to get more opinions.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, December 18, 2018 8:20 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; 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; techboard@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/3] eal: add new rte color definition
>
> 18/12/2018 20:34, Dumitrescu, Cristian:
> > From: Ananyev, Konstantin
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 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
> > 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)
> >
> > Personally, I can live with any of these options.
> >
> > Thomas,
> >
> > It would be good to have your input as well, Reshma just sent V4
> implementing your proposal based on having the color enum defined in
> rte_meter.h, which gets included into rte_mbuf.h.
>
> We need a decision from Olivier, mbuf maintainer.
>
> > We need to make progress for RC1 deadline.
>
> I'm afraid 19.02 is not the right release to change the mbuf.
> This kind of decision usually takes several weeks.
> Cc the technical board to get more opinions.
>
Hi Thomas,
The only change we are planning to do on the mbuf is reformatting of the sched field, according to the deprecation note that was already acked by Olivier and many others:
http://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst#n52
We are planning to send V5 that should simplify the solution and avoid any such issues. Specifically, what we are going to do in V5 is:
1/ Make sure the only mbuf changes are related to the reformatting of the sched field, precisely implementing the above deprecation note;
2/ Include rte_meter.h into some ethdev headers, which you already agreed with
3/ Avoid adding any header to librte_eal/common/include
Are you OK with this?
Hopefully this should remove any roadblocks and any further delays for this patch set.
Regards,
Cristian
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
19/12/2018 11:47, Dumitrescu, Cristian:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 18/12/2018 20:34, Dumitrescu, Cristian:
> > > From: Ananyev, Konstantin
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 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
> > > 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)
> > >
> > > Personally, I can live with any of these options.
> > >
> > > Thomas,
> > >
> > > It would be good to have your input as well, Reshma just sent V4
> > implementing your proposal based on having the color enum defined in
> > rte_meter.h, which gets included into rte_mbuf.h.
> >
> > We need a decision from Olivier, mbuf maintainer.
> >
> > > We need to make progress for RC1 deadline.
> >
> > I'm afraid 19.02 is not the right release to change the mbuf.
> > This kind of decision usually takes several weeks.
> > Cc the technical board to get more opinions.
> >
>
> Hi Thomas,
>
> The only change we are planning to do on the mbuf is reformatting of the sched field, according to the deprecation note that was already acked by Olivier and many others:
> http://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst#n52
>
> We are planning to send V5 that should simplify the solution and avoid any such issues. Specifically, what we are going to do in V5 is:
> 1/ Make sure the only mbuf changes are related to the reformatting of the sched field, precisely implementing the above deprecation note;
> 2/ Include rte_meter.h into some ethdev headers, which you already agreed with
> 3/ Avoid adding any header to librte_eal/common/include
>
> Are you OK with this?
>
> Hopefully this should remove any roadblocks and any further delays for this patch set.
Yes I am OK.
Obvioulsy, I would be more confortable if having an ack from Olivier.
> 18/12/2018 20:34, Dumitrescu, Cristian:
> > From: Ananyev, Konstantin
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 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
> > 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)
> >
> > Personally, I can live with any of these options.
> >
> > Thomas,
> >
> > It would be good to have your input as well, Reshma just sent V4 implementing your proposal based on having the color enum
> defined in rte_meter.h, which gets included into rte_mbuf.h.
>
> We need a decision from Olivier, mbuf maintainer.
>
> > We need to make progress for RC1 deadline.
>
> I'm afraid 19.02 is not the right release to change the mbuf.
> This kind of decision usually takes several weeks.
> Cc the technical board to get more opinions.
I general, I don't see any problem with these changes for 19.02.
As I can see,, they only affect rte_mbuf sched filed format/interpretation.
All other fields meaning/layout remain the same.
Konstantin
> -----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
@@ -9,6 +9,7 @@
#include <rte_ethdev.h>
#include <rte_flow.h>
#include <rte_mtr.h>
+#include <rte_color.h>
#include "testpmd.h"
#include "cmdline_mtr.h"
@@ -74,7 +75,7 @@ parse_uint(uint64_t *value, const char *str)
}
static int
-parse_dscp_table_entries(char *str, enum rte_mtr_color **dscp_table)
+parse_dscp_table_entries(char *str, enum rte_color **dscp_table)
{
char *token;
int i = 0;
@@ -84,21 +85,21 @@ parse_dscp_table_entries(char *str, enum rte_mtr_color **dscp_table)
return 0;
/* Allocate memory for dscp table */
- *dscp_table = (enum rte_mtr_color *)malloc(MAX_DSCP_TABLE_ENTRIES *
- sizeof(enum rte_mtr_color));
+ *dscp_table = (enum rte_color *)malloc(MAX_DSCP_TABLE_ENTRIES *
+ sizeof(enum rte_color));
if (*dscp_table == NULL)
return -1;
while (1) {
if (strcmp(token, "G") == 0 ||
strcmp(token, "g") == 0)
- *dscp_table[i++] = RTE_MTR_GREEN;
+ *dscp_table[i++] = RTE_COLOR_GREEN;
else if (strcmp(token, "Y") == 0 ||
strcmp(token, "y") == 0)
- *dscp_table[i++] = RTE_MTR_YELLOW;
+ *dscp_table[i++] = RTE_COLOR_YELLOW;
else if (strcmp(token, "R") == 0 ||
strcmp(token, "r") == 0)
- *dscp_table[i++] = RTE_MTR_RED;
+ *dscp_table[i++] = RTE_COLOR_RED;
else {
free(*dscp_table);
return -1;
@@ -117,7 +118,7 @@ parse_dscp_table_entries(char *str, enum rte_mtr_color **dscp_table)
static int
parse_meter_color_str(char *c_str, uint32_t *use_prev_meter_color,
- enum rte_mtr_color **dscp_table)
+ enum rte_color **dscp_table)
{
char *token;
uint64_t previous_mtr_color = 0;
@@ -182,20 +183,20 @@ parse_policer_action_string(char *p_str, uint32_t action_mask,
return -1;
if (g_color == 0 && (action_mask & 0x1)) {
- actions[RTE_MTR_GREEN] = action;
+ actions[RTE_COLOR_GREEN] = action;
g_color = 1;
} else if (y_color == 0 && (action_mask & 0x2)) {
- actions[RTE_MTR_YELLOW] = action;
+ actions[RTE_COLOR_YELLOW] = action;
y_color = 1;
} else
- actions[RTE_MTR_RED] = action;
+ actions[RTE_COLOR_RED] = action;
}
return 0;
}
static int
parse_multi_token_string(char *t_str, uint16_t *port_id,
- uint32_t *mtr_id, enum rte_mtr_color **dscp_table)
+ uint32_t *mtr_id, enum rte_color **dscp_table)
{
char *token;
uint64_t val;
@@ -782,7 +783,7 @@ static void cmd_create_port_meter_parsed(void *parsed_result,
uint32_t shared = res->shared;
uint32_t use_prev_meter_color = 0;
uint16_t port_id = res->port_id;
- enum rte_mtr_color *dscp_table = NULL;
+ enum rte_color *dscp_table = NULL;
char *c_str = res->meter_input_color;
int ret;
@@ -808,11 +809,11 @@ static void cmd_create_port_meter_parsed(void *parsed_result,
else
params.meter_enable = 0;
- params.action[RTE_MTR_GREEN] =
+ params.action[RTE_COLOR_GREEN] =
string_to_policer_action(res->g_action);
- params.action[RTE_MTR_YELLOW] =
+ params.action[RTE_COLOR_YELLOW] =
string_to_policer_action(res->y_action);
- params.action[RTE_MTR_RED] =
+ params.action[RTE_COLOR_RED] =
string_to_policer_action(res->r_action);
params.stats_mask = res->statistics_mask;
@@ -1134,7 +1135,7 @@ static void cmd_set_port_meter_dscp_table_parsed(void *parsed_result,
{
struct cmd_set_port_meter_dscp_table_result *res = parsed_result;
struct rte_mtr_error error;
- enum rte_mtr_color *dscp_table = NULL;
+ enum rte_color *dscp_table = NULL;
char *t_str = res->token_string;
uint32_t mtr_id = 0;
uint16_t port_id;
@@ -1245,7 +1246,7 @@ static void cmd_set_port_meter_policer_action_parsed(void *parsed_result,
}
/* Allocate memory for policer actions */
- actions = (enum rte_mtr_policer_action *)malloc(RTE_MTR_COLORS *
+ actions = (enum rte_mtr_policer_action *)malloc(RTE_COLORS *
sizeof(enum rte_mtr_policer_action));
if (actions == NULL) {
printf("Memory for policer actions not allocated (error)\n");
@@ -1426,22 +1427,22 @@ static void cmd_show_port_meter_stats_parsed(void *parsed_result,
/* Display stats */
if (stats_mask & RTE_MTR_STATS_N_PKTS_GREEN)
printf("\tPkts G: %" PRIu64 "\n",
- stats.n_pkts[RTE_MTR_GREEN]);
+ stats.n_pkts[RTE_COLOR_GREEN]);
if (stats_mask & RTE_MTR_STATS_N_BYTES_GREEN)
printf("\tBytes G: %" PRIu64 "\n",
- stats.n_bytes[RTE_MTR_GREEN]);
+ stats.n_bytes[RTE_COLOR_GREEN]);
if (stats_mask & RTE_MTR_STATS_N_PKTS_YELLOW)
printf("\tPkts Y: %" PRIu64 "\n",
- stats.n_pkts[RTE_MTR_YELLOW]);
+ stats.n_pkts[RTE_COLOR_YELLOW]);
if (stats_mask & RTE_MTR_STATS_N_BYTES_YELLOW)
printf("\tBytes Y: %" PRIu64 "\n",
- stats.n_bytes[RTE_MTR_YELLOW]);
+ stats.n_bytes[RTE_COLOR_YELLOW]);
if (stats_mask & RTE_MTR_STATS_N_PKTS_RED)
printf("\tPkts R: %" PRIu64 "\n",
- stats.n_pkts[RTE_MTR_RED]);
+ stats.n_pkts[RTE_COLOR_RED]);
if (stats_mask & RTE_MTR_STATS_N_BYTES_RED)
printf("\tBytes R: %" PRIu64 "\n",
- stats.n_bytes[RTE_MTR_RED]);
+ stats.n_bytes[RTE_COLOR_RED]);
if (stats_mask & RTE_MTR_STATS_N_PKTS_DROPPED)
printf("\tPkts DROPPED: %" PRIu64 "\n",
stats.n_pkts_dropped);
@@ -9,6 +9,7 @@
#include <rte_ethdev.h>
#include <rte_flow.h>
#include <rte_tm.h>
+#include <rte_color.h>
#include "testpmd.h"
#include "cmdline_tm.h"
@@ -296,7 +297,7 @@ static void cmd_show_port_tm_cap_parsed(void *parsed_result,
printf("cap.cman_wred_context_shared_n_contexts_per_node_max %" PRIu32
"\n", cap.cman_wred_context_shared_n_contexts_per_node_max);
- for (i = 0; i < RTE_TM_COLORS; i++) {
+ for (i = 0; i < RTE_COLORS; i++) {
printf("cap.mark_vlan_dei_supported %" PRId32 "\n",
cap.mark_vlan_dei_supported[i]);
printf("cap.mark_ip_ecn_tcp_supported %" PRId32 "\n",
@@ -642,22 +643,22 @@ static void cmd_show_port_tm_node_stats_parsed(void *parsed_result,
stats.n_bytes);
if (stats_mask & RTE_TM_STATS_N_PKTS_GREEN_DROPPED)
printf("\tPkts dropped (green): %" PRIu64 "\n",
- stats.leaf.n_pkts_dropped[RTE_TM_GREEN]);
+ stats.leaf.n_pkts_dropped[RTE_COLOR_GREEN]);
if (stats_mask & RTE_TM_STATS_N_PKTS_YELLOW_DROPPED)
printf("\tPkts dropped (yellow): %" PRIu64 "\n",
- stats.leaf.n_pkts_dropped[RTE_TM_YELLOW]);
+ stats.leaf.n_pkts_dropped[RTE_COLOR_YELLOW]);
if (stats_mask & RTE_TM_STATS_N_PKTS_RED_DROPPED)
printf("\tPkts dropped (red): %" PRIu64 "\n",
- stats.leaf.n_pkts_dropped[RTE_TM_RED]);
+ stats.leaf.n_pkts_dropped[RTE_COLOR_RED]);
if (stats_mask & RTE_TM_STATS_N_BYTES_GREEN_DROPPED)
printf("\tBytes dropped (green): %" PRIu64 "\n",
- stats.leaf.n_bytes_dropped[RTE_TM_GREEN]);
+ stats.leaf.n_bytes_dropped[RTE_COLOR_GREEN]);
if (stats_mask & RTE_TM_STATS_N_BYTES_YELLOW_DROPPED)
printf("\tBytes dropped (yellow): %" PRIu64 "\n",
- stats.leaf.n_bytes_dropped[RTE_TM_YELLOW]);
+ stats.leaf.n_bytes_dropped[RTE_COLOR_YELLOW]);
if (stats_mask & RTE_TM_STATS_N_BYTES_RED_DROPPED)
printf("\tBytes dropped (red): %" PRIu64 "\n",
- stats.leaf.n_bytes_dropped[RTE_TM_RED]);
+ stats.leaf.n_bytes_dropped[RTE_COLOR_RED]);
if (stats_mask & RTE_TM_STATS_N_PKTS_QUEUED)
printf("\tPkts queued: %" PRIu64 "\n",
stats.leaf.n_pkts_queued);
@@ -1267,7 +1268,7 @@ static void cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
{
struct cmd_add_port_tm_node_wred_profile_result *res = parsed_result;
struct rte_tm_wred_params wp;
- enum rte_tm_color color;
+ enum rte_color color;
struct rte_tm_error error;
uint32_t wred_profile_id = res->wred_profile_id;
portid_t port_id = res->port_id;
@@ -1280,7 +1281,7 @@ static void cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
memset(&error, 0, sizeof(struct rte_tm_error));
/* WRED Params (Green Color)*/
- color = RTE_TM_GREEN;
+ color = RTE_COLOR_GREEN;
wp.red_params[color].min_th = res->min_th_g;
wp.red_params[color].max_th = res->max_th_g;
wp.red_params[color].maxp_inv = res->maxp_inv_g;
@@ -1288,14 +1289,14 @@ static void cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
/* WRED Params (Yellow Color)*/
- color = RTE_TM_YELLOW;
+ color = RTE_COLOR_YELLOW;
wp.red_params[color].min_th = res->min_th_y;
wp.red_params[color].max_th = res->max_th_y;
wp.red_params[color].maxp_inv = res->maxp_inv_y;
wp.red_params[color].wq_log2 = res->wq_log2_y;
/* WRED Params (Red Color)*/
- color = RTE_TM_RED;
+ color = RTE_COLOR_RED;
wp.red_params[color].min_th = res->min_th_r;
wp.red_params[color].max_th = res->max_th_r;
wp.red_params[color].maxp_inv = res->maxp_inv_r;
@@ -12,6 +12,7 @@
#include <rte_flow.h>
#include <rte_flow_driver.h>
#include <rte_tailq.h>
+#include <rte_color.h>
#include "rte_eth_softnic_internals.h"
#include "rte_eth_softnic.h"
@@ -1624,11 +1625,14 @@ flow_rule_action_get(struct pmd_internals *softnic,
/* RTE_TABLE_ACTION_METER */
rule_action->mtr.mtr[0].meter_profile_id = meter_profile_id;
rule_action->mtr.mtr[0].policer[e_RTE_METER_GREEN] =
- softnic_table_action_policer(m->params.action[RTE_MTR_GREEN]);
+ softnic_table_action_policer
+ (m->params.action[RTE_COLOR_GREEN]);
rule_action->mtr.mtr[0].policer[e_RTE_METER_YELLOW] =
- softnic_table_action_policer(m->params.action[RTE_MTR_YELLOW]);
+ softnic_table_action_policer
+ (m->params.action[RTE_COLOR_YELLOW]);
rule_action->mtr.mtr[0].policer[e_RTE_METER_RED] =
- softnic_table_action_policer(m->params.action[RTE_MTR_RED]);
+ softnic_table_action_policer
+ (m->params.action[RTE_COLOR_RED]);
rule_action->mtr.tc_mask = 1;
rule_action->action_mask |= 1 << RTE_TABLE_ACTION_MTR;
break;
@@ -8,6 +8,7 @@
#include <rte_mtr.h>
#include <rte_mtr_driver.h>
+#include <rte_color.h>
#include "rte_eth_softnic_internals.h"
@@ -458,7 +459,7 @@ pmd_mtr_meter_profile_update(struct rte_eth_dev *dev,
static int
pmd_mtr_meter_dscp_table_update(struct rte_eth_dev *dev,
uint32_t mtr_id,
- enum rte_mtr_color *dscp_table,
+ enum rte_color *dscp_table,
struct rte_mtr_error *error)
{
struct pmd_internals *p = dev->data->dev_private;
@@ -536,7 +537,7 @@ pmd_mtr_policer_actions_update(struct rte_eth_dev *dev,
NULL,
"Invalid actions");
- for (i = 0; i < RTE_MTR_COLORS; i++) {
+ for (i = 0; i < RTE_COLORS; i++) {
if (action_mask & (1 << i)) {
if (actions[i] != MTR_POLICER_ACTION_COLOR_GREEN &&
actions[i] != MTR_POLICER_ACTION_COLOR_YELLOW &&
@@ -560,7 +561,7 @@ pmd_mtr_policer_actions_update(struct rte_eth_dev *dev,
memcpy(&action, &m->flow->action, sizeof(action));
/* Set action */
- for (i = 0; i < RTE_MTR_COLORS; i++)
+ for (i = 0; i < RTE_COLORS; i++)
if (action_mask & (1 << i))
action.mtr.mtr[0].policer[i] =
softnic_table_action_policer(actions[i]);
@@ -588,7 +589,7 @@ pmd_mtr_policer_actions_update(struct rte_eth_dev *dev,
}
/* Meter: Update policer actions */
- for (i = 0; i < RTE_MTR_COLORS; i++)
+ for (i = 0; i < RTE_COLORS; i++)
if (action_mask & (1 << i))
m->params.action[i] = actions[i];
@@ -618,15 +619,17 @@ mtr_stats_convert(struct softnic_mtr *m,
if (in->n_packets_valid) {
uint32_t i;
- for (i = 0; i < RTE_MTR_COLORS; i++) {
+ for (i = 0; i < RTE_COLORS; i++) {
if (m->params.action[i] == MTR_POLICER_ACTION_COLOR_GREEN)
- out->n_pkts[RTE_MTR_GREEN] += in->n_packets[i];
+ out->n_pkts[RTE_COLOR_GREEN] +=
+ in->n_packets[i];
if (m->params.action[i] == MTR_POLICER_ACTION_COLOR_YELLOW)
- out->n_pkts[RTE_MTR_YELLOW] += in->n_packets[i];
+ out->n_pkts[RTE_COLOR_YELLOW] +=
+ in->n_packets[i];
if (m->params.action[i] == MTR_POLICER_ACTION_COLOR_RED)
- out->n_pkts[RTE_MTR_RED] += in->n_packets[i];
+ out->n_pkts[RTE_COLOR_RED] += in->n_packets[i];
if (m->params.action[i] == MTR_POLICER_ACTION_DROP)
out->n_pkts_dropped += in->n_packets[i];
@@ -638,15 +641,16 @@ mtr_stats_convert(struct softnic_mtr *m,
if (in->n_bytes_valid) {
uint32_t i;
- for (i = 0; i < RTE_MTR_COLORS; i++) {
+ for (i = 0; i < RTE_COLORS; i++) {
if (m->params.action[i] == MTR_POLICER_ACTION_COLOR_GREEN)
- out->n_bytes[RTE_MTR_GREEN] += in->n_bytes[i];
+ out->n_bytes[RTE_COLOR_GREEN] += in->n_bytes[i];
if (m->params.action[i] == MTR_POLICER_ACTION_COLOR_YELLOW)
- out->n_bytes[RTE_MTR_YELLOW] += in->n_bytes[i];
+ out->n_bytes[RTE_COLOR_YELLOW] +=
+ in->n_bytes[i];
if (m->params.action[i] == MTR_POLICER_ACTION_COLOR_RED)
- out->n_bytes[RTE_MTR_RED] += in->n_bytes[i];
+ out->n_bytes[RTE_COLOR_RED] += in->n_bytes[i];
if (m->params.action[i] == MTR_POLICER_ACTION_DROP)
out->n_bytes_dropped += in->n_bytes[i];
@@ -8,6 +8,7 @@
#include <rte_malloc.h>
#include <rte_string_fns.h>
+#include <rte_color.h>
#include "rte_eth_softnic_internals.h"
#include "rte_eth_softnic.h"
@@ -1204,7 +1205,7 @@ wred_profile_check(struct rte_eth_dev *dev,
struct rte_tm_error *error)
{
struct tm_wred_profile *wp;
- enum rte_tm_color color;
+ enum rte_color color;
/* WRED profile ID must not be NONE. */
if (wred_profile_id == RTE_TM_WRED_PROFILE_ID_NONE)
@@ -1240,7 +1241,7 @@ wred_profile_check(struct rte_eth_dev *dev,
rte_strerror(ENOTSUP));
/* min_th <= max_th, max_th > 0 */
- for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
+ for (color = RTE_COLOR_GREEN; color < RTE_COLORS; color++) {
uint32_t min_th = profile->red_params[color].min_th;
uint32_t max_th = profile->red_params[color].max_th;
@@ -2218,10 +2219,10 @@ wred_profiles_set(struct rte_eth_dev *dev)
struct pmd_internals *p = dev->data->dev_private;
struct rte_sched_port_params *pp = &p->soft.tm.params.port_params;
uint32_t tc_id;
- enum rte_tm_color color;
+ enum rte_color color;
for (tc_id = 0; tc_id < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; tc_id++)
- for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
+ for (color = RTE_COLOR_GREEN; color < RTE_COLORS; color++) {
struct rte_red_params *dst =
&pp->red_params[tc_id][color];
struct tm_wred_profile *src_wp =
@@ -3058,9 +3059,9 @@ read_port_stats(struct rte_eth_dev *dev,
s.n_pkts_tc[id] - s.n_pkts_tc_dropped[id];
nr->stats.n_bytes +=
s.n_bytes_tc[id] - s.n_bytes_tc_dropped[id];
- nr->stats.leaf.n_pkts_dropped[RTE_TM_GREEN] +=
+ nr->stats.leaf.n_pkts_dropped[RTE_COLOR_GREEN] +=
s.n_pkts_tc_dropped[id];
- nr->stats.leaf.n_bytes_dropped[RTE_TM_GREEN] +=
+ nr->stats.leaf.n_bytes_dropped[RTE_COLOR_GREEN] +=
s.n_bytes_tc_dropped[id];
}
}
@@ -3105,9 +3106,9 @@ read_subport_stats(struct rte_eth_dev *dev,
s.n_pkts_tc[tc_id] - s.n_pkts_tc_dropped[tc_id];
ns->stats.n_bytes +=
s.n_bytes_tc[tc_id] - s.n_bytes_tc_dropped[tc_id];
- ns->stats.leaf.n_pkts_dropped[RTE_TM_GREEN] +=
+ ns->stats.leaf.n_pkts_dropped[RTE_COLOR_GREEN] +=
s.n_pkts_tc_dropped[tc_id];
- ns->stats.leaf.n_bytes_dropped[RTE_TM_GREEN] +=
+ ns->stats.leaf.n_bytes_dropped[RTE_COLOR_GREEN] +=
s.n_bytes_tc_dropped[tc_id];
}
@@ -3162,8 +3163,9 @@ read_pipe_stats(struct rte_eth_dev *dev,
/* Stats accumulate */
np->stats.n_pkts += s.n_pkts - s.n_pkts_dropped;
np->stats.n_bytes += s.n_bytes - s.n_bytes_dropped;
- np->stats.leaf.n_pkts_dropped[RTE_TM_GREEN] += s.n_pkts_dropped;
- np->stats.leaf.n_bytes_dropped[RTE_TM_GREEN] +=
+ np->stats.leaf.n_pkts_dropped[RTE_COLOR_GREEN] +=
+ s.n_pkts_dropped;
+ np->stats.leaf.n_bytes_dropped[RTE_COLOR_GREEN] +=
s.n_bytes_dropped;
np->stats.leaf.n_pkts_queued = qlen;
}
@@ -3222,8 +3224,9 @@ read_tc_stats(struct rte_eth_dev *dev,
/* Stats accumulate */
nt->stats.n_pkts += s.n_pkts - s.n_pkts_dropped;
nt->stats.n_bytes += s.n_bytes - s.n_bytes_dropped;
- nt->stats.leaf.n_pkts_dropped[RTE_TM_GREEN] += s.n_pkts_dropped;
- nt->stats.leaf.n_bytes_dropped[RTE_TM_GREEN] +=
+ nt->stats.leaf.n_pkts_dropped[RTE_COLOR_GREEN] +=
+ s.n_pkts_dropped;
+ nt->stats.leaf.n_bytes_dropped[RTE_COLOR_GREEN] +=
s.n_bytes_dropped;
nt->stats.leaf.n_pkts_queued = qlen;
}
@@ -3281,8 +3284,8 @@ read_queue_stats(struct rte_eth_dev *dev,
/* Stats accumulate */
nq->stats.n_pkts += s.n_pkts - s.n_pkts_dropped;
nq->stats.n_bytes += s.n_bytes - s.n_bytes_dropped;
- nq->stats.leaf.n_pkts_dropped[RTE_TM_GREEN] += s.n_pkts_dropped;
- nq->stats.leaf.n_bytes_dropped[RTE_TM_GREEN] +=
+ nq->stats.leaf.n_pkts_dropped[RTE_COLOR_GREEN] += s.n_pkts_dropped;
+ nq->stats.leaf.n_bytes_dropped[RTE_COLOR_GREEN] +=
s.n_bytes_dropped;
nq->stats.leaf.n_pkts_queued = qlen;
@@ -17,7 +17,7 @@ INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
INC += rte_malloc.h rte_keepalive.h rte_time.h
INC += rte_service.h rte_service_component.h
INC += rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h
-INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h
+INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h rte_color.h
GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
new file mode 100644
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#ifndef _RTE_COLOR_H_
+#define _RTE_COLOR_H_
+
+/**
+ * Color
+ */
+enum rte_color {
+ RTE_COLOR_GREEN = 0, /**< Green */
+ RTE_COLOR_YELLOW, /**< Yellow */
+ RTE_COLOR_RED, /**< Red */
+ RTE_COLORS /**< Number of colors */
+};
+
+#endif /* _RTE_COLOR_H_ */
@@ -153,7 +153,7 @@ rte_mtr_meter_profile_update(uint16_t port_id,
int __rte_experimental
rte_mtr_meter_dscp_table_update(uint16_t port_id,
uint32_t mtr_id,
- enum rte_mtr_color *dscp_table,
+ enum rte_color *dscp_table,
struct rte_mtr_error *error)
{
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
@@ -76,21 +76,12 @@
#include <stdint.h>
#include <rte_compat.h>
#include <rte_common.h>
+#include <rte_color.h>
#ifdef __cplusplus
extern "C" {
#endif
-/**
- * Color
- */
-enum rte_mtr_color {
- RTE_MTR_GREEN = 0, /**< Green */
- RTE_MTR_YELLOW, /**< Yellow */
- RTE_MTR_RED, /**< Red */
- RTE_MTR_COLORS /**< Number of colors. */
-};
-
/**
* Statistics counter type
*/
@@ -125,10 +116,10 @@ enum rte_mtr_stats_type {
*/
struct rte_mtr_stats {
/** Number of packets passed by the policer (per color). */
- uint64_t n_pkts[RTE_MTR_COLORS];
+ uint64_t n_pkts[RTE_COLORS];
/** Number of bytes passed by the policer (per color). */
- uint64_t n_bytes[RTE_MTR_COLORS];
+ uint64_t n_bytes[RTE_COLORS];
/** Number of packets dropped by the policer. */
uint64_t n_pkts_dropped;
@@ -260,7 +251,7 @@ struct rte_mtr_params {
* at least one yellow or red color element, then the color aware mode
* is configured.
*/
- enum rte_mtr_color *dscp_table;
+ enum rte_color *dscp_table;
/** Non-zero to enable the meter, zero to disable the meter at the time
* of MTR object creation. Ignored when the meter profile indicated by
@@ -270,7 +261,7 @@ struct rte_mtr_params {
int meter_enable;
/** Policer actions (per meter output color). */
- enum rte_mtr_policer_action action[RTE_MTR_COLORS];
+ enum rte_mtr_policer_action action[RTE_COLORS];
/** Set of stats counters to be enabled.
* @see enum rte_mtr_stats_type
@@ -636,7 +627,7 @@ rte_mtr_meter_profile_update(uint16_t port_id,
int __rte_experimental
rte_mtr_meter_dscp_table_update(uint16_t port_id,
uint32_t mtr_id,
- enum rte_mtr_color *dscp_table,
+ enum rte_color *dscp_table,
struct rte_mtr_error *error);
/**
@@ -70,7 +70,7 @@ typedef int (*rte_mtr_meter_profile_update_t)(struct rte_eth_dev *dev,
typedef int (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev *dev,
uint32_t mtr_id,
- enum rte_mtr_color *dscp_table,
+ enum rte_color *dscp_table,
struct rte_mtr_error *error);
/**< @internal MTR object meter DSCP table update */
@@ -51,6 +51,7 @@
#include <stdint.h>
#include <rte_common.h>
+#include <rte_color.h>
#ifdef __cplusplus
extern "C" {
@@ -115,16 +116,6 @@ extern "C" {
*/
#define RTE_TM_NODE_LEVEL_ID_ANY UINT32_MAX
-/**
- * Color
- */
-enum rte_tm_color {
- RTE_TM_GREEN = 0, /**< Green */
- RTE_TM_YELLOW, /**< Yellow */
- RTE_TM_RED, /**< Red */
- RTE_TM_COLORS /**< Number of colors */
-};
-
/**
* Node statistics counter type
*/
@@ -179,12 +170,12 @@ struct rte_tm_node_stats {
/** Number of packets dropped by current leaf node per each
* color.
*/
- uint64_t n_pkts_dropped[RTE_TM_COLORS];
+ uint64_t n_pkts_dropped[RTE_COLORS];
/** Number of bytes dropped by current leaf node per each
* color.
*/
- uint64_t n_bytes_dropped[RTE_TM_COLORS];
+ uint64_t n_bytes_dropped[RTE_COLORS];
/** Number of packets currently waiting in the packet queue of
* current leaf node.
@@ -435,16 +426,16 @@ struct rte_tm_capabilities {
uint32_t cman_wred_context_shared_n_contexts_per_node_max;
/** Support for VLAN DEI packet marking (per color). */
- int mark_vlan_dei_supported[RTE_TM_COLORS];
+ int mark_vlan_dei_supported[RTE_COLORS];
/** Support for IPv4/IPv6 ECN marking of TCP packets (per color). */
- int mark_ip_ecn_tcp_supported[RTE_TM_COLORS];
+ int mark_ip_ecn_tcp_supported[RTE_COLORS];
/** Support for IPv4/IPv6 ECN marking of SCTP packets (per color). */
- int mark_ip_ecn_sctp_supported[RTE_TM_COLORS];
+ int mark_ip_ecn_sctp_supported[RTE_COLORS];
/** Support for IPv4/IPv6 DSCP packet marking (per color). */
- int mark_ip_dscp_supported[RTE_TM_COLORS];
+ int mark_ip_dscp_supported[RTE_COLORS];
/** Set of supported dynamic update operations.
* @see enum rte_tm_dynamic_update_type
@@ -861,7 +852,7 @@ struct rte_tm_red_params {
*/
struct rte_tm_wred_params {
/** One set of RED parameters per packet color */
- struct rte_tm_red_params red_params[RTE_TM_COLORS];
+ struct rte_tm_red_params red_params[RTE_COLORS];
/** When non-zero, the *min_th* and *max_th* thresholds are specified
* in packets (WRED packet mode). When zero, the *min_th* and *max_th*
@@ -21,6 +21,7 @@ extern "C" {
#include <stdint.h>
+#include <rte_color.h>
/*
* Application Programmer's Interface (API)
*
@@ -34,6 +35,12 @@ enum rte_meter_color {
e_RTE_METER_COLORS /**< Number of available colors */
};
+/* New rte_color is defined and used to deprecate rte_meter_color soon. */
+#define rte_meter_color rte_color
+#define e_RTE_METER_GREEN RTE_COLOR_GREEN
+#define e_RTE_METER_YELLOW RTE_COLOR_YELLOW
+#define e_RTE_METER_RED RTE_COLOR_RED
+
/** srTCM parameters per metered traffic flow. The CIR, CBS and EBS parameters only
count bytes of IP packets and do not include link specific headers. At least one of
the CBS or EBS parameters has to be greater than zero. */