> Please also add the links to the relevant capabilities: @see struct
> rte_tm_node_capabilities::XYZ.
This is already added for packet_mode field in descrption of struct
rte_tm_shaper_params similar to struct rte_tm_wred_params.
Do you see anything missing ?
On Wed, Apr 22, 2020 at 05:01:46PM +0530, Nithin Dabilpuram wrote:
> On Wed, Apr 22, 2020 at 10:10:47AM +0000, Dumitrescu, Cristian wrote:
> > External Email
> >
> > ----------------------------------------------------------------------
> > Hi Nithin,
> >
> > > -----Original Message-----
> > > From: Nithin Dabilpuram <nithind1...@gmail.com>
> > > Sent: Wednesday, April 22, 2020 9:00 AM
> > > To: Singh, Jasvinder <jasvinder.si...@intel.com>; Dumitrescu, Cristian
> > > <cristian.dumitre...@intel.com>; Thomas Monjalon
> > > <tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>; Andrew
> > > Rybchenko <arybche...@solarflare.com>
> > > Cc: dev@dpdk.org; jer...@marvell.com; kka...@marvell.com; Nithin
> > > Dabilpuram <ndabilpu...@marvell.com>
> > > Subject: [PATCH v3] ethdev: add tm support for shaper config in pkt mode
> > >
> > > From: Nithin Dabilpuram <ndabilpu...@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 <ndabilpu...@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?
> I see error only because of v3 changes in node cap. There seems to be some gcc
> issue with order of initializations only when next field is union. I tried
> both
> gcc 4.8.5 and 7.3.1
>
> struct test {
> int a;
> char b;
> int c;
> union {
> int d;
> };
>
> };
>
> struct test ds1 = {
> .a = 10,
> .b = 20,
> {
> .d = 40,
> }
> };
>
> int main(void)
> {
> printf("ds1.a %d\n", ds1.a);
> }
> [root@hyd1588t430 dpdk]# make a
> cc a.c -o a
> a.c:20:2: warning: braces around scalar initializer [enabled by default]
> {
> ^
> a.c:20:2: warning: (near initialization for ‘ds1.c’) [enabled by default]
> a.c:21:3: error: field name not in record or union initializer
> .d = 40,
> ^
> a.c:21:3: error: (near initialization for ‘ds1.c’)
> make: *** [a] Error 1
>
> I don't see any issue if 'd' is not inside a union.
>
> Similar issue is reported in kernel and similar fix was applied of
> initializing all the fields.
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_3033041_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=Ygf72TMZqO2D2qgWdhga8M61LRznr54ORYKCZ5iQG7w&s=n6pWrzBSgTCaznkjfH4UXjJS_CvBpX2_uY-CJ-LZcd4&e=
>
>
> >
> > 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.
> Ack.
> >
> > 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".
> Ack. My bad, I misunderstood earlier comment. Will fix in v4.
> >
> > <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?
> Ack.
> >
> > Please also add the links to the relevant capabilities: @see struct
> > rte_tm_node_capabilities::XYZ.
> Ack.
> >
> >
> > Regards,
> > Cristian
