Re: [dpdk-dev] [PATCH 1/2] ethdev: add packet mode in meter profile structure

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

> -----Original Message-----
> From: Li Zhang <lizh@nvidia.com>
> Sent: Thursday, April 1, 2021 7:19 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> dekelp@nvidia.com; Ori Kam <orika@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Liron Himi
> <lironh@marvell.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>
> Subject: RE: [PATCH 1/2] ethdev: add packet mode in meter profile structure
> 
> Hi Dumitrescu and Liron,
> 
> I add the check in softnet and mvpp2 drivers in the following patch.
> Please help review it.
> https://patchwork.dpdk.org/project/dpdk/list/?series=16036
> 

As stated in a different email, it is difficult to review this API change, as it is split across (at least) two patch sets, one with the API changes and another one with the driver error checks, also as stated before the numbering of some patches does not seem right; difficult to tell if I am reviewing the latest version or a superseded one. 

Could you please create a unified V2 that contains all patches in one place? Thank you!

> Thanks!
> Regards,
> Li Zhang
> > -----Original Message-----
> > From: Li Zhang
> > Sent: Thursday, April 1, 2021 9:42 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > dekelp@nvidia.com; Ori Kam <orika@nvidia.com>; Slava Ovsiienko
> > <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> > Shuler <shahafs@nvidia.com>; NBU-Contact-Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar
> Yanai
> > <roniba@nvidia.com>
> > Subject: RE: [PATCH 1/2] ethdev: add packet mode in meter profile
> structure
> >
> > Hi Dumitrescu,
> >
> > Thank you for your comments.
> > I will add the necessary checks in V2 patches.
> >
> > Regards,
> > Li Zhang
> >
> > > -----Original Message-----
> > > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Sent: Thursday, April 1, 2021 12:27 AM
> > > To: Li Zhang <lizh@nvidia.com>; dekelp@nvidia.com; Ori Kam
> > > <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Matan
> > > Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; NBU-
> > > Contact-Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > > <andrew.rybchenko@oktetlabs.ru>
> > > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar
> > > Yanai <roniba@nvidia.com>
> > > Subject: RE: [PATCH 1/2] ethdev: add packet mode in meter profile
> > > structure
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Hi Li,
> > >
> > > > -----Original Message-----
> > > > From: Li Zhang <lizh@nvidia.com>
> > > > Sent: Wednesday, March 31, 2021 9:54 AM
> > > > To: dekelp@nvidia.com; orika@nvidia.com; viacheslavo@nvidia.com;
> > > > matan@nvidia.com; shahafs@nvidia.com; Dumitrescu, Cristian
> > > > <cristian.dumitrescu@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > > Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > > Cc: dev@dpdk.org; rasland@nvidia.com; roniba@nvidia.com
> > > > Subject: [PATCH 1/2] ethdev: add packet mode in meter profile
> > > > structure
> > > >
> > > > Currently meter algorithms only supports rate is bytes per second(BPS).
> > > > Add packet_mode flag in meter profile parameters data structure.
> > > > So that it can meter traffic by packet per second.
> > > >
> > > > When packet_mode is 0, the profile rates and bucket sizes are
> > > > specified in bytes per second and bytes when packet_mode is not 0,
> > > > the profile rates and bucket sizes are specified in packets and
> > > > packets per second.
> > > >
> > > > The below structure will be extended:
> > > > rte_mtr_meter_profile
> > > > rte_mtr_capabilities
> > > >
> > > > Signed-off-by: Li Zhang <lizh@nvidia.com>
> > > > ---
> > > >  doc/guides/rel_notes/release_21_05.rst |  7 ++
> > > >  lib/librte_ethdev/rte_mtr.h            | 89 ++++++++++++++++++++++----
> > > >  2 files changed, 85 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/doc/guides/rel_notes/release_21_05.rst
> > > > b/doc/guides/rel_notes/release_21_05.rst
> > > > index 3c76148b11..c9c15794ad 100644
> > > > --- a/doc/guides/rel_notes/release_21_05.rst
> > > > +++ b/doc/guides/rel_notes/release_21_05.rst
> > > > @@ -119,6 +119,13 @@ New Features
> > > >    * Added command to display Rx queue used descriptor count.
> > > >      ``show port (port_id) rxq (queue_id) desc used count``
> > > >
> > > > +* **Added support for meter PPS profile.**
> > > > +
> > > > +  Currently meter algorithms only supports bytes per second(BPS).
> > > > +  Add packet_mode in the meter profile parameters data structures
> > > > + to support packet per second (PPS) mode.
> > > > +  So that it can meter traffic by packet per second.
> > > > +  Packet_mode must be 0 when it is bytes mode.
> > > >
> > > >  Removed Items
> > > >  -------------
> > > > diff --git a/lib/librte_ethdev/rte_mtr.h
> > > > b/lib/librte_ethdev/rte_mtr.h index e1dc59eb06..52611a90f5 100644
> > > > --- a/lib/librte_ethdev/rte_mtr.h
> > > > +++ b/lib/librte_ethdev/rte_mtr.h
> > > > @@ -133,46 +133,71 @@ struct rte_mtr_meter_profile {
> > > >       union {
> > > >               /** Items only valid when *alg* is set to srTCM - RFC 2697. */
> > > >               struct {
> > > > -                     /** Committed Information Rate (CIR)
> > > > (bytes/second). */
> > > > +                     /**
> > > > +                      * Committed Information Rate (CIR)
> > > > +                      * (bytes per second or packets per second).
> > > > +                      */
> > > >                       uint64_t cir;
> > > >
> > > > -                     /** Committed Burst Size (CBS) (bytes). */
> > > > +                     /** Committed Burst Size (CBS) (bytes or
> > > > + packets). */
> > > >                       uint64_t cbs;
> > > >
> > > > -                     /** Excess Burst Size (EBS) (bytes). */
> > > > +                     /** Excess Burst Size (EBS) (bytes or packets).
> > > > + */
> > > >                       uint64_t ebs;
> > > >               } srtcm_rfc2697;
> > > >
> > > >               /** Items only valid when *alg* is set to trTCM - RFC 2698. */
> > > >               struct {
> > > > -                     /** Committed Information Rate (CIR)
> > > > (bytes/second). */
> > > > +                     /**
> > > > +                      * Committed Information Rate (CIR)
> > > > +                      * (bytes per second or packets per second).
> > > > +                      */
> > > >                       uint64_t cir;
> > > >
> > > > -                     /** Peak Information Rate (PIR) (bytes/second). */
> > > > +                     /**
> > > > +                      * Peak Information Rate (PIR)
> > > > +                      * (bytes per second or packets per second).
> > > > +                      */
> > > >                       uint64_t pir;
> > > >
> > > > -                     /** Committed Burst Size (CBS) (byes). */
> > > > +                     /** Committed Burst Size (CBS) (bytes or
> > > > + packets). */
> > > >                       uint64_t cbs;
> > > >
> > > > -                     /** Peak Burst Size (PBS) (bytes). */
> > > > +                     /** Peak Burst Size (PBS) (bytes or packets).
> > > > + */
> > > >                       uint64_t pbs;
> > > >               } trtcm_rfc2698;
> > > >
> > > >               /** Items only valid when *alg* is set to trTCM - RFC 4115. */
> > > >               struct {
> > > > -                     /** Committed Information Rate (CIR)
> > > > (bytes/second). */
> > > > +                     /**
> > > > +                      * Committed Information Rate (CIR)
> > > > +                      * (bytes per second or packets per second).
> > > > +                      */
> > > >                       uint64_t cir;
> > > >
> > > > -                     /** Excess Information Rate (EIR) (bytes/second). */
> > > > +                     /**
> > > > +                      * Excess Information Rate (EIR)
> > > > +                      * (bytes per second or packets per second).
> > > > +                      */
> > > >                       uint64_t eir;
> > > >
> > > > -                     /** Committed Burst Size (CBS) (byes). */
> > > > +                     /** Committed Burst Size (CBS) (bytes or
> > > > + packets). */
> > > >                       uint64_t cbs;
> > > >
> > > > -                     /** Excess Burst Size (EBS) (bytes). */
> > > > +                     /** Excess Burst Size (EBS) (bytes or packets).
> > > > + */
> > > >                       uint64_t ebs;
> > > >               } trtcm_rfc4115;
> > > >       };
> > > > +
> > > > +     /**
> > > > +      * When zero, the byte mode is enabled for the current
> > > > + profile, so
> > > > the
> > > > +      * *rate* and *size* fields are specified in bytes per second
> > > > +      * and bytes, respectively.
> > > > +      * When non-zero, the packet mode is enabled for the current
> > > > profile,
> > > > +      * so the *rate* and *size* fields are specified in packets per
> second
> > > > +      * and packets, respectively.
> > > > +      */
> > > > +     int packet_mode;
> > > >  };
> > > >
> > > >  /**
> > > > @@ -333,6 +358,48 @@ struct rte_mtr_capabilities {
> > > >        */
> > > >       int color_aware_trtcm_rfc4115_supported;
> > > >
> > > > +     /**
> > > > +      * srTCM rfc2697 byte mode supported.
> > > > +      * When non-zero, it indicates that byte mode is supported for
> > > > +      * the srTCM RFC 2697 metering algorithm.
> > > > +      */
> > > > +     int srtcm_rfc2697_byte_mode_supported;
> > > > +
> > > > +     /**
> > > > +      * srTCM rfc2697 packet mode supported.
> > > > +      * When non-zero, it indicates that packet mode is supported for
> > > > +      * the srTCM RFC 2697 metering algorithm.
> > > > +      */
> > > > +     int srtcm_rfc2697_packet_mode_supported;
> > > > +
> > > > +     /**
> > > > +      * trTCM rfc2698 byte mode supported.
> > > > +      * When non-zero, it indicates that byte mode is supported for
> > > > +      * the trTCM RFC 2698 metering algorithm.
> > > > +      */
> > > > +     int trtcm_rfc2698_byte_mode_supported;
> > > > +
> > > > +     /**
> > > > +      * trTCM rfc2698 packet mode supported.
> > > > +      * When non-zero, it indicates that packet mode is supported for
> > > > +      * the trTCM RFC 2698 metering algorithm.
> > > > +      */
> > > > +     int trtcm_rfc2698_packet_mode_supported;
> > > > +
> > > > +     /**
> > > > +      * trTCM rfc4115 byte mode supported.
> > > > +      * When non-zero, it indicates that byte mode is supported for
> > > > +      * the trTCM RFC 4115 metering algorithm.
> > > > +      */
> > > > +     int trtcm_rfc4115_byte_mode_supported;
> > > > +
> > > > +     /**
> > > > +      * trTCM rfc4115 packet mode supported.
> > > > +      * When non-zero, it indicates that packet mode is supported for
> > > > +      * the trTCM RFC 4115 metering algorithm.
> > > > +      */
> > > > +     int trtcm_rfc4115_packet_mode_supported;
> > > > +
> > > >       /** Set of supported statistics counter types.
> > > >        * @see enum rte_mtr_stats_type
> > > >        */
> > > > --
> > > > 2.27.0
> > >
> > > These API updates look good to me, but please add the necessary checks
> > > to the existing drivers implementing the rte_mtr API to makes sure
> > > that profiles with packet_mode set to TRUE are rejected.
> > >
> > > For the Soft NIC driver, this is a simple check in file
> > > drivers/net/softnic/rte_eth_softnic_meter.c, function
> meter_profile_check.
> > > Thanks!
> > >
> > > Regards,
> > > Cristian