From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: Nithin Dabilpuram <nithind1988@gmail.com>,
"Singh, Jasvinder" <jasvinder.singh@intel.com>,
Thomas Monjalon <thomas@monjalon.net>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
Andrew Rybchenko <arybchenko@solarflare.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"jerinj@marvell.com" <jerinj@marvell.com>,
"kkanas@marvell.com" <kkanas@marvell.com>,
Nithin Dabilpuram <ndabilpuram@marvell.com>
Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add tm support for shaper config in pkt mode
Date: Wed, 22 Apr 2020 10:10:47 +0000 [thread overview]
Message-ID: <BYAPR11MB293540BCDE702352487137ECEBD20@BYAPR11MB2935.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200422075948.10051-1-nithind1988@gmail.com>
Hi Nithin,
> -----Original Message-----
> From: Nithin Dabilpuram <nithind1988@gmail.com>
> Sent: Wednesday, April 22, 2020 9:00 AM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kkanas@marvell.com; Nithin
> Dabilpuram <ndabilpuram@marvell.com>
> Subject: [PATCH v3] ethdev: add tm support for shaper config in pkt mode
>
> From: Nithin Dabilpuram <ndabilpuram@marvell.com>
>
> Some NIC hardware support shaper to work in packet mode i.e
> shaping or ratelimiting traffic is in packets per second (PPS) as
> opposed to default bytes per second (BPS). Hence this patch
> adds support to configure shared or private shaper in packet mode,
> provide rate in PPS and add related tm capabilities in port/level/node
> capability structures.
>
> This patch also updates tm port/level/node capability structures with
> exiting features of scheduler wfq packet mode, scheduler wfq byte mode
> and private/shared shaper byte mode.
>
> SoftNIC PMD is also updated with new capabilities.
>
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
>
> v2..v3:
> - Fix typo's
> - Add shaper_shared_(packet, byte)_mode_supported in level and node cap
> - Fix comment in pkt_length_adjust.
> - Move rte_eth_softnic_tm.c capability update to patch 1/4 to
> avoid compilations issues in node and level cap array in softnicpmd.
> ../drivers/net/softnic/rte_eth_softnic_tm.c:782:3: warning: braces around
> scalar initializer
> {.nonleaf = {
> ../drivers/net/softnic/rte_eth_softnic_tm.c:782:3: note: (near initialization
> for ‘tm_node_cap[0].shaper_shared_byte_mode_supported’)
> ../drivers/net/softnic/rte_eth_softnic_tm.c:782:4: error: field name not in
> record or union initializer
> {.nonleaf = {
>
> v1..v2:
> - Add seperate capability for shaper and scheduler pktmode and bytemode.
> - Add packet_mode field in struct rte_tm_shaper_params to indicate
> packet mode shaper profile.
>
>
> drivers/net/softnic/rte_eth_softnic_tm.c | 65 ++++++++++
> lib/librte_ethdev/rte_tm.h | 196
> ++++++++++++++++++++++++++++++-
> 2 files changed, 259 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c
> b/drivers/net/softnic/rte_eth_softnic_tm.c
> index 80a470c..344819f 100644
> --- a/drivers/net/softnic/rte_eth_softnic_tm.c
> +++ b/drivers/net/softnic/rte_eth_softnic_tm.c
<snip>...
The softnic changes logically belong to a separate patch. I understand you are getting some build warnings, but it does not make sense to me. Are you sure there is no other way to avoid them?
You are not checking that packet_mode is set to 0. Please add a check in function shaper_profile_check() (file rte_eth_softnic_tm.c) to verify that packet_mode is always set to 0.
Jasvinder, any other changes we need in Soft NIC?
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index f9c0cf3..b3865af 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
I am now fine with your additions to this file, with some minor exceptions listed below. Thank you!
> @@ -250,6 +250,23 @@ struct rte_tm_capabilities {
> */
> uint64_t shaper_private_rate_max;
>
> + /** Shaper private packet mode supported. When non-zero, this
> parameter
> + * indicates that there is at least one node that can be configured
> + * with packet mode in it's private shaper. When shaper is configured
Recurring typo: it's -> its
Please search for all occurrences of "it's", all should be replaced with either "its" or "it is", we should have absolutely no occurrence of "it's".
<snip>...
>
> @@ -860,6 +1034,11 @@ struct rte_tm_token_bucket {
> * Dual rate shapers use both the committed and the peak token buckets.
> The
> * rate of the peak bucket has to be bigger than zero, as well as greater than
> * or equal to the rate of the committed bucket.
> + *
> + * @see struct
> rte_tm_capabilities::shaper_private_packet_mode_supported
> + * @see struct rte_tm_capabilities::shaper_private_byte_mode_supported
> + * @see struct
> rte_tm_capabilities::shaper_shared_packet_mode_supported
> + * @see struct rte_tm_capabilities::shaper_shared_byte_mode_supported
> */
> struct rte_tm_shaper_params {
> /** Committed token bucket */
> @@ -872,8 +1051,19 @@ struct rte_tm_shaper_params {
> * purpose of shaping. Can be used to correct the packet length with
> * the framing overhead bytes that are also consumed on the wire
> (e.g.
> * RTE_TM_ETH_FRAMING_OVERHEAD_FCS).
> + * This field is ignored when the profile enables packet mode.
> */
> int32_t pkt_length_adjust;
> +
> + /** When zero, the private or shared shaper that is associated to this
> + * profile works in byte mode and hence *rate* and *size* fields in
> + * both token bucket configurations are specified in bytes per second
> + * and bytes respectively.
> + * When non-zero, that private or shared shaper works in packet
> mode and
> + * hence *rate* and *size* fields in both token bucket configurations
> + * are specified in packets per second and packets respectively.
> + */
> + int packet_mode;
> };
I would like to simplify this comment a bit. The reference to a shaper being associated with a profile might be confusing, as some people might incorrectly read there is a 1:1 association between shaper and profile, etc; this is described in mode details in the comment at the top of this structure. I would avoid any mentions of objects outside of the current structure.
I would phrase it like: "When zero, the byte mode is enabled for the current profile, so the *rate* and *size* fields in both the committed and peak token buckets 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 in both the committed and peak token buckets are specified in packets per second and packets, respectively. ". Is this OK with you?
Please also add the links to the relevant capabilities: @see struct rte_tm_node_capabilities::XYZ.
Regards,
Cristian
next prev parent reply other threads:[~2020-04-22 10:10 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 16:00 [dpdk-dev] [PATCH 1/2] ethdev: add tm cap for private shaper packet mode Nithin Dabilpuram
2020-03-30 16:00 ` [dpdk-dev] [PATCH 2/2] app/testpmd: add tm non leaf node pktmode command Nithin Dabilpuram
2020-04-07 7:30 ` [dpdk-dev] [PATCH 1/2] ethdev: add tm cap for private shaper packet mode Nithin Dabilpuram
2020-04-07 16:31 ` Dumitrescu, Cristian
2020-04-07 17:21 ` [dpdk-dev] [EXT] " Nithin Dabilpuram
2020-04-10 11:45 ` Dumitrescu, Cristian
2020-04-10 11:56 ` Nithin Dabilpuram
2020-04-11 11:44 ` [dpdk-dev] [PATCH v2 1/4] ethdev: add tm support for shaper config in pkt mode Nithin Dabilpuram
2020-04-11 11:44 ` [dpdk-dev] [PATCH v2 2/4] drivers/net: update tm capability for existing pmds Nithin Dabilpuram
2020-04-11 11:44 ` [dpdk-dev] [PATCH v2 3/4] app/testpmd: add tm cmd for non leaf and shaper pktmode Nithin Dabilpuram
2020-04-11 11:44 ` [dpdk-dev] [PATCH v2 4/4] net/octeontx2: support tm length adjust and pkt mode Nithin Dabilpuram
2020-04-16 13:48 ` [dpdk-dev] [PATCH v2 1/4] ethdev: add tm support for shaper config in " Ferruh Yigit
2020-04-21 5:11 ` [dpdk-dev] [EXT] " Nithin Dabilpuram
2020-04-21 9:30 ` [dpdk-dev] " Dumitrescu, Cristian
2020-04-21 9:58 ` Nithin Dabilpuram
2020-04-21 10:23 ` Dumitrescu, Cristian
2020-04-21 11:55 ` [dpdk-dev] [EXT] " Nithin Dabilpuram
2020-04-22 7:59 ` [dpdk-dev] [PATCH v3] " Nithin Dabilpuram
2020-04-22 7:59 ` [dpdk-dev] [PATCH v3 2/4] drivers/net: update tm capability for existing pmds Nithin Dabilpuram
2020-04-22 7:59 ` [dpdk-dev] [PATCH v3 3/4] app/testpmd: add tm cmd for non leaf and shaper pktmode Nithin Dabilpuram
2020-04-22 7:59 ` [dpdk-dev] [PATCH v3 4/4] net/octeontx2: support tm length adjust and pkt mode Nithin Dabilpuram
2020-04-22 8:09 ` [dpdk-dev] [PATCH v3] ethdev: add tm support for shaper config in " Nithin Dabilpuram
2020-04-22 12:18 ` Singh, Jasvinder
2020-04-22 17:21 ` [dpdk-dev] [EXT] " Nithin Dabilpuram
2020-04-22 10:10 ` Dumitrescu, Cristian [this message]
2020-04-22 11:31 ` Nithin Dabilpuram
2020-04-22 11:49 ` Nithin Dabilpuram
2020-04-22 11:59 ` Dumitrescu, Cristian
2020-04-22 12:01 ` Dumitrescu, Cristian
2020-04-22 8:05 ` [dpdk-dev] [PATCH v3 1/4] " Nithin Dabilpuram
2020-04-22 17:21 ` [dpdk-dev] [PATCH v4 " Nithin Dabilpuram
2020-04-22 17:21 ` [dpdk-dev] [PATCH v4 2/4] drivers/net: update tm capability for existing pmds Nithin Dabilpuram
2020-04-22 17:21 ` [dpdk-dev] [PATCH v4 3/4] app/testpmd: add tm cmd for non leaf and shaper pktmode Nithin Dabilpuram
2020-04-22 17:21 ` [dpdk-dev] [PATCH v4 4/4] net/octeontx2: support tm length adjust and pkt mode Nithin Dabilpuram
2020-04-24 10:28 ` [dpdk-dev] [PATCH v4 1/4] ethdev: add tm support for shaper config in " Dumitrescu, Cristian
2020-04-25 20:09 ` Ferruh Yigit
2020-04-27 9:19 ` Dumitrescu, Cristian
2020-04-27 16:12 ` Ferruh Yigit
2020-04-27 16:28 ` Dumitrescu, Cristian
2020-04-28 15:30 ` Thomas Monjalon
2020-04-28 17:35 ` Dumitrescu, Cristian
2020-04-27 16:29 ` Jerin Jacob
2020-04-27 16:49 ` Ferruh Yigit
2020-04-27 16:59 ` Jerin Jacob
2020-04-28 11:51 ` [dpdk-dev] [EXT] " Nithin Dabilpuram
2020-04-28 13:56 ` Ferruh Yigit
2020-04-28 14:06 ` [dpdk-dev] " Ferruh Yigit
2020-04-28 14:45 ` Bruce Richardson
2020-04-28 15:04 ` Luca Boccassi
2020-04-28 15:54 ` Thomas Monjalon
2020-04-29 8:45 ` Dumitrescu, Cristian
2020-04-29 9:03 ` Bruce Richardson
2020-05-01 10:27 ` Ferruh Yigit
2020-05-01 13:16 ` [dpdk-dev] [EXT] " Nithin Dabilpuram
2020-08-25 16:59 ` Ferruh Yigit
2020-09-07 11:12 ` Nithin Dabilpuram
2020-09-14 13:01 ` Ferruh Yigit
2020-05-01 13:18 ` [dpdk-dev] " Jerin Jacob
2020-05-05 8:01 ` Ray Kinsella
2020-04-28 15:42 ` Ray Kinsella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BYAPR11MB293540BCDE702352487137ECEBD20@BYAPR11MB2935.namprd11.prod.outlook.com \
--to=cristian.dumitrescu@intel.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=jasvinder.singh@intel.com \
--cc=jerinj@marvell.com \
--cc=kkanas@marvell.com \
--cc=ndabilpuram@marvell.com \
--cc=nithind1988@gmail.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).