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

[Nithin Dabilpuram <ndabilpuram@marvell.com>]

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://patchwork.kernel.org/patch/3033041/

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