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