Re: [dpdk-dev] [PATCH v2 00/15] sched: subport level configuration of pipe nodes

["Singh, Jasvinder" <jasvinder.singh@intel.com>]

> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Monday, September 23, 2019 2:06 PM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH v2 00/15] sched: subport level configuration of pipe nodes
> 
> Hi Jasvinder,
> 
> > -----Original Message-----
> > From: Singh, Jasvinder
> > Sent: Monday, September 9, 2019 11:05 AM
> > To: dev@dpdk.org
> > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Subject: [PATCH v2 00/15] sched: subport level configuration of pipe
> > nodes
> >
> > This patchset refactors the dpdk qos sched library to allow subport
> > level configuration flexibility of the pipe nodes.
> >
> > Currently, all parameters for the pipe nodes (subscribers)
> > configuration are part of the port level structure which forces all
> > groups of subscribers (pipes) in different subports to have similar
> > configurations in terms of their number, queue sizes, traffic-classes,
> > etc.
> >
> > The new implementation moves pipe nodes configuration parameters from
> > port level to subport level structure. This allows different subports
> > of the same port to have different configuration for the pipe nodes,
> > for examples- number of pipes, queue sizes, pipe profiles, etc.
> >
> > In order to keep the implementation complexity under control, all
> > pipes within the same subport share the same configuration for queue
> > sizes.
> >
> > v2:
> > - fix qsize parsing in sample application
> > - fix checkpatch warnings
> >
> > Jasvinder Singh (15):
> >   sched: add pipe config params to subport struct
> >   sched: modify internal structs for config flexibility
> >   sched: remove pipe params config from port level
> >   shced: add pipe config to subport level
> >   sched: modify pipe functions for config flexibility
> >   sched: modify pkt enqueue for config flexibility
> >   sched: update memory compute to support flexiblity
> >   sched: update grinder functions for config flexibility
> >   sched: update pkt dequeue for flexible config
> >   sched: update queue stats read for config flexibility
> >   test/sched: modify tests for subport config flexibility
> >   net/softnic: add subport config flexibility to TM
> >   ip_pipeline: add subport config flexibility to TM
> >   examples/qos_sched: add subport configuration flexibility
> >   sched: remove redundant code
> >
> >  app/test/test_sched.c                    |   35 +-
> >  doc/guides/rel_notes/release_19_11.rst   |    6 +-
> >  drivers/net/softnic/rte_eth_softnic_tm.c |   51 +-
> >  examples/ip_pipeline/cli.c               |   71 +-
> >  examples/ip_pipeline/tmgr.c              |   23 +-
> >  examples/ip_pipeline/tmgr.h              |    7 +-
> >  examples/qos_sched/app_thread.c          |    4 +-
> >  examples/qos_sched/cfg_file.c            |  229 ++--
> >  examples/qos_sched/init.c                |   54 +-
> >  examples/qos_sched/main.h                |    1 +
> >  examples/qos_sched/profile.cfg           |    5 +-
> >  examples/qos_sched/profile_ov.cfg        |    5 +-
> >  examples/qos_sched/stats.c               |   36 +-
> >  lib/librte_sched/Makefile                |    2 +-
> >  lib/librte_sched/meson.build             |    2 +-
> >  lib/librte_sched/rte_sched.c             | 1400 +++++++++++++---------
> >  lib/librte_sched/rte_sched.h             |  114 +-
> >  17 files changed, 1183 insertions(+), 862 deletions(-)
> >
> > --
> > 2.21.0
> 
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> 
> A few comments/suggestion, mainly on some API name changes:
> 
> 1. We need a good comment explaining the difference between
> rte_sched_port_params::n_max_pipes_per_subport and
> rte_sched_subport_params::n_pipes_per_subport. The former reserves a fixed
> number of bits in struct rte_mbuf::sched.queue_id for the pipe_id for all the
> subports of the same port, while the latter provides a mechanism to
> enable/allocate fewer pipes for each subport, as needed, with the benefit of
> avoiding memory allocation for the queues of the pipes that are not really
> needed. Another way to look at it is to say all subports have the same number
> of pipes (n_max_pipes_per_subport), but some of these pipes might not be
> implemented by all the subports. 

I will improve doxygen comments as suggested above, thanks.

Maybe we should name the port parameter
> as n_pipes_per_subport and the subport parameter as
> n_pipes_per_subport_enabled?

Will rename these parameters as suggested.

> PS: I did check the critical functions rte_sched_port_qindex() and
> rte_sched_port_pkt_read_tree_path(), and they look good to me.
> 
> 2. The rte_sched_PORT_pipe_profile_add() should probably be now renamed
> as rte_sched_SUBPORT_pipe_profile_add(), right?

Thanks for spotting this :)

> 3. The port parameter n_max_pipe_profiles does not make sense to me, as we
> are now introducing the n_pipe_profiles subport parameter. Is this a code
> leftover or is this used to simplify the memory allocation? Assuming the latter,
> my vote is to remove it.

I will remove this field from the port structure. 

Thank you once again for review and comments, will fix above in v3 version.
Jasvinder