> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Monday, September 23, 2019 2:06 PM
> To: Singh, Jasvinder <jasvinder.si...@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.dumitre...@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.dumitre...@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
