Re: [dpdk-dev] [EXT] RE: [PATCH v3] ethdev: add tm support for shaper config in pkt mode

[Nithin Dabilpuram <ndabilpuram@marvell.com>]

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