Re: [dpdk-dev] [PATCH] app/testpmd: fix vdev socket initialization

["Phil Yang (Arm Technology China)" <Phil.Yang@arm.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, October 15, 2018 6:42 PM
> To: Phil Yang (Arm Technology China) <Phil.Yang@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; anatoly.burakov@intel.com
> Subject: Re: [PATCH] app/testpmd: fix vdev socket initialization
> 
> On 10/15/2018 10:51 AM, Phil Yang (Arm Technology China) wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Saturday, October 13, 2018 1:13 AM
> >> To: Phil Yang (Arm Technology China) <Phil.Yang@arm.com>;
> >> dev@dpdk.org
> >> Cc: nd <nd@arm.com>; anatoly.burakov@intel.com
> >> Subject: Re: [PATCH] app/testpmd: fix vdev socket initialization
> >>
> >> On 10/12/2018 10:34 AM, phil.yang@arm.com wrote:
> >>> The cmdline settings of port-numa-config and rxring-numa-config have
> >>> been flushed by the following init_config. If we don't configure the
> >>> port-numa-config, the virtual device will allocate the device ports
> >>> to socket 0. It will cause failure when the socket 0 is unavailable.
> >>>
> >>> eg:
> >>> testpmd -l <cores from socket 1> --vdev net_pcap0,iface=lo
> >>> --socket-mem=64 -- --numa --port-numa-config="(0,1)"
> >>> --ring-numa-config="(0,1,1),(0,2,1)" -i
> >>>
> >>> ...
> >>> Configuring Port 0 (socket 0)
> >>> Failed to setup RX queue:No mempool allocation on the socket 0
> >>> EAL: Error - exiting with code: 1
> >>>   Cause: Start ports failed
> >>>
> >>> Fix by allocate the devices port to the first available socket or
> >>> the socket configured in port-numa-config.
> >>
> >> I confirm this fixes the issue, by making vdev to allocate from
> >> available socket instead of hardcoded socket 0, overall this make sense.
> >>
> >> But currently there is no way to request mempool form "socket 0" if
> >> only cores from "socket 1" provided in "-l", even with
> >> "port-numa-config" and "rxring- numa-config".
> >> Both this behavior and the problem this patch fixes caused by patch:
> >> Commit dbfb8ec7094c ("app/testpmd: optimize mbuf pool allocation")
> >>
> >> It is good to have optimized mempool allocation but I think this
> >> shouldn't limit the tool. If user wants mempools from specific socket, let it
> have.
> >>
> >> What about changing the default behavior to:
> >> 1- Allocate mempool only from socket that coremask provided (current
> >> approach)
> >> 2- Plus, allocate mempool from sockets of attached devices (this is
> >> alternative solution to this patch, your solution seems better for
> >> virtual devices but for physical devices allocating from socket it
> >> connects can be better)
> >> 3- Plus, allocate mempool from sockets provided in "port-numa-config"
> >> and "rxring-numa-config"
> >>
> >> What do you think?
> >
> > Hi Ferruh,
> >
> > Totally agreed with your suggestion.
> >
> > As I understand, allocating mempool from sockets of attached devices will
> enable the cross NUMA scenario for Testpmd.
> 
> Yes it will.
> 
> >
> > Below is my fix for physic port mempool allocate issue. So, is it better to
> separate it into a new patch on the top of this one or rework this one by adding
> below fix? I prefer to add a new one because the current patch has already fixed
> two defects. Anyway, I will follow your comment.
> 
> +1 to separate it into a new patch, so I will check existing patch.
> 
> Below looks good only not sure if is should be in `set_default_fwd_ports_config`?
> Or perhaps `set_default_fwd_lcores_config`?
Hi Ferruh,

IMO, 'set_default_fwd_lcores_config' is aiming to update sockets info and core related info according to the -l <core list> or -c <core mask> input.
So, go through the attached devices to update ports' socket info in 'set_default_fwd_ports_config' is reasonable.

I think the initialization process go through like below in Testpmd.

                                         /    1.  'set_default_fwd_lcore_config'   update core related info
'set_def_fwd_config'   —  2. 'set_default_peer_eth_addrs '  update port address
                |                       \	    3. 'set_default_fwd_ports_config '  update port (or devices) related info
                V
'launch_args_parse'    —  update port-numa-config settings
                |
                V                 /     1. allocate mempool for each available socket which recorded in socket_ids[]
       'init_config'       —    2. 'init_fwd_streams' update port->socket_id info according to the port-numa-config
                :
          start_port

Once this patch applied. This socket_ids[] update order will affect the default socket of mempool allocation.
e.g. :  socket_ids[0] = -l <core list>
           socket_ids[1] = <attached devices socket id> when it is not the socket listed in <core list>
For virtual devices, the default socket is socket_ids[0]. For physic devices, the default socket will be socket_id[1].

> 
> And port-numa-config and rxring-numa-config still not covered.
Those configurations have been initialed in launch_args_parse' and operate the forwarding streams in 'init_fwd_streams'.  So the patch covered those configurations.

Thanks
Phil Yang
> 
> >
> >    565 static void
> >    566 set_default_fwd_ports_config(void)
> >    567 {
> >    568 ›   portid_t pt_id;
> >    569 ›   int i = 0;
> >    570
> >    571 ›   RTE_ETH_FOREACH_DEV(pt_id) {
> >    572 ›   ›   fwd_ports_ids[i++] = pt_id;
> >    573
> > +  574 ›   ›   /* Update sockets info according to the attached device */
> > +  575 ›   ›   int socket_id = rte_eth_dev_socket_id(pt_id);
> > +  576 ›   ›   if (socket_id >= 0 && new_socket_id(pt_id)) {
> > +  577 ›   ›   ›   if (num_sockets >= RTE_MAX_NUMA_NODES) {
> > +  578 ›   ›   ›   ›   rte_exit(EXIT_FAILURE,
> > +  579 ›   ›   ›   ›   ›    "Total sockets greater than %u\n",
> > +  580 ›   ›   ›   ›   ›    RTE_MAX_NUMA_NODES);
> > +  581 ›   ›   ›   }
> > +  582 ›   ›   ›   socket_ids[num_sockets++] = socket_id;
> > +  583 ›   ›   }
> > +  584 ›   }
> > +  585
> >    586 ›   nb_cfg_ports = nb_ports;
> >    587 ›   nb_fwd_ports = nb_ports;
> >    588 }
> >
> > Thanks
> > Phil Yang
> >
> >>
> >>
> >>>
> >>> Fixes: 487f9a5 ("app/testpmd: fix NUMA structures initialization")
> >>>
> >>> Signed-off-by: Phil Yang <phil.yang@arm.com>
> >>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> >>
> >> <...>