Re: [dpdk-dev] [PATCH v2] app/testpmd: support non contiguous socket ids

[Shahaf Shuler <shahafs@mellanox.com>]

--Shahaf


> -----Original Message-----
> From: Wu, Jingjing [mailto:jingjing.wu@intel.com]
> Sent: Monday, May 8, 2017 3:54 AM
> To: Shahaf Shuler <shahafs@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>;
> stable@dpdk.org
> Subject: RE: [PATCH v2] app/testpmd: support non contiguous socket ids
> 
> 
> 
> > -----Original Message-----
> > From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> > Sent: Sunday, May 7, 2017 2:06 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>;
> > stable@dpdk.org
> > Subject: RE: [PATCH v2] app/testpmd: support non contiguous socket ids
> >
> > Saturday, May 6, 2017 4:41 AM, Wu, Jingjing:
> > > >
> > > > The test assumes the socket ids are contiguous. This is not
> > > > necessarily the case on all servers and may cause mempool creation to
> fail.
> > > >
> > > > Fixing it by detecting the list of valid socket ids and use it for
> > > > the mempool creation.
> > > >
> > > > Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> > > >
> > > > CC: stable@dpdk.org
> > > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > ---
> > > > on v2:
> > > >  * fix minor typo on commit message : be->by.
> > > > ---
> > > >  app/test-pmd/parameters.c | 38 ++++++++++++++++++++++++++++-
> ----
> > > -----
> > > >  app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++-
> -----
> > > ---
> > > >  app/test-pmd/testpmd.h    |  4 +++-
> > > >  3 files changed, 60 insertions(+), 20 deletions(-)
> > > >
> >
> > [..]
> >
> > > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > > dfe64427d..a556a8aff 100644
> > > > --- a/app/test-pmd/testpmd.c
> > > > +++ b/app/test-pmd/testpmd.c
> >
> > [..]
> >
> > > > +/*
> > > >   * Setup default configuration.
> > > >   */
> > > >  static void
> > > > @@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
> > > >
> > > >  	nb_lc = 0;
> > > >  	for (i = 0; i < RTE_MAX_LCORE; i++) {
> > > > -		sock_num = rte_lcore_to_socket_id(i) + 1;
> > > > -		if (sock_num > max_socket) {
> > > > -			if (sock_num > RTE_MAX_NUMA_NODES)
> > > > -				rte_exit(EXIT_FAILURE, "Total sockets
> > > greater
> > > > than %u\n", RTE_MAX_NUMA_NODES);
> > > > -			max_socket = sock_num;
> > > > +		sock_num = rte_lcore_to_socket_id(i);
> > > +1 is missed?
> >
> > I don't think so.
> > On previous implementation this logic was meant to find the max_socket.
> > max_socket was the first invalid socket_id and was used, for example :
> >
> > if (socket_id >= max_socket) {
> >
> > or
> >
> > for (i = 0; i < max_socket; i++)
> >
> > now, on above logic, we list the valid socket id. Therefore
> > rte_lcore_to_socket_id return a valid id and not need to add 1.
> >
> >
> OK, but at list the following check "if (sock_num >
> RTE_MAX_NUMA_NODES)"
> Should be "if (sock_num +1 > RTE_MAX_NUMA_NODES)", right?
> 

Right, i prefer the following though, what do you think?

       for (i = 0; i < RTE_MAX_LCORE; i++) {
                sock_num = rte_lcore_to_socket_id(i);
                if (new_socket_id(sock_num)) {
                        if (num_sockets >= RTE_MAX_NUMA_NODES) {
                                rte_exit(EXIT_FAILURE,
                                         "Total sockets greater than %u\n",
                                         RTE_MAX_NUMA_NODES);
                        }
                        socket_ids[num_sockets++] = sock_num;
                }

> One more thing, if this patch is fixing a bug, I think "fix" should be added in
> title.

OK, I guess I can change the commit title and message.

BTW note that there is a V3 with Thomas requested changes.
If we agree on above I can submit a v4 with the last updates.


> 
> Thanks
> Jingjing