Re: [dpdk-dev] [Linuxarm] Re: [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx and Tx

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Friday, April 9, 2021 14:10
> To: linuxarm@openeuler.org; dev <dev@dpdk.org>; Li, Xiaoyun
> <xiaoyun.li@intel.com>
> Subject: Re: [Linuxarm] Re: [PATCH 2/3] app/testpmd: remove forwarding
> config from parsing Rx and Tx
> 
> 
> 
> 在 2021/4/2 10:33, Li, Xiaoyun 写道:
> >
> >
> >> -----Original Message-----
> >> From: oulijun <oulijun@huawei.com>
> >> Sent: Friday, April 2, 2021 09:45
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from
> >> parsing Rx and Tx
> >>
> >>
> >>
> >> 在 2021/3/29 9:53, Li, Xiaoyun 写道:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: oulijun <oulijun@huawei.com>
> >>>> Sent: Thursday, March 25, 2021 11:04
> >>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >>>> <ferruh.yigit@intel.com>
> >>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >>>> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from
> >>>> parsing Rx and Tx
> >>>>
> >>>>
> >>>>
> >>>> 在 2021/3/24 9:44, Li, Xiaoyun 写道:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: oulijun <oulijun@huawei.com>
> >>>>>> Sent: Wednesday, March 24, 2021 09:01
> >>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >>>>>> <ferruh.yigit@intel.com>
> >>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >>>>>> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config
> >>>>>> from parsing Rx and Tx
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> 在 2021/3/23 15:50, Li, Xiaoyun 写道:
> >>>>>>> Hi
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Lijun Ou <oulijun@huawei.com>
> >>>>>>>> Sent: Friday, March 5, 2021 18:22
> >>>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
> >>>>>>>> linuxarm@openeuler.org
> >>>>>>>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from
> >>>>>>>> parsing Rx and Tx
> >>>>>>>>
> >>>>>>>> From: Huisong Li <lihuisong@huawei.com>
> >>>>>>>>
> >>>>>>>
> >>>>>>> The commit message should be more simple and avoids grammar
> >> mistakes.
> >>>>>>>
> >>>>>> All right, I will simply it.
> >>>>>>>> The "fwd_config_setup()" function does release and apply for
> >>>>>>>> memory of forwarding flows, and re-establish these streams when
> >>>>>>>> rxq/txq or rxd/txd is changed. The function is also called by
> >>>>>>>> "start_packet_forwarding()" when user executes "start" cmd.
> >>>>>>>> All changes for rxq/txq or rxd/txd can be updated uniformly
> >>>>>>>> when this command is executed. Therefore, it is a little
> >>>>>>>> redundant in the
> >>>>>> "cmd_config_rx_tx_parsed"
> >>>>>>>> function.
> >>>>>>>
> >>>>>>> It's not redundant. This command may configure number of rxq/txq.
> >>>>>>> So the
> >>>>>> fwd streams map may change.
> >>>>>>> Then it's common to check the fwd streams after this command
> >>>>>>> using "show
> >>>>>> config fwd".
> >>>>>>> If you remove this fwd stream update, users can't get the
> >>>>>>> correct new fwd
> >>>>>> streams until they start the traffic.
> >>>>>>> But they may change a lot of things and want to check if the
> >>>>>>> setting is correct
> >>>>>> before they start the traffic.
> >>>>>>>
> >>>>>> Yes, you are right. It's really unfriendly.
> >>>>>>>>
> >>>>>>>> In addition, the forwarding stream under one TC is configured
> >>>>>>>> based on number of queues allocated to TC. And number of queues
> >>>>>>>> allocated to TC is updated after calling  "rte_eth_dev_configure"
> >>>>>>>> again. If the number of queues is reduced after configuring the
> >>>>>>>> DCB, and then, release and apply for stream memory, and
> >>>>>>>> reinitialize the forwarding stream under the DCB mode based on
> >>>>>>>> the old TC
> >>>> information.
> >>>>>>>> As a result, null pointer may be accessed.
> >>>>>>>
> >>>>>>> I think you should add "rte_eth_dev_configure " into
> >>>>>>> dcb_fwd_config_setup()
> >>>>>> before rte_eth_dev_get_dcb_info().
> >>>>>>>
> >>>>>>> And the commit message should be similar like the following:
> >>>>>>> Segment fault might happen after configuring queue number to
> >>>>>>> less because
> >>>>>> dcb_fwd_config_setup setup dcb based on old dcb info.
> >>>>>>> And dcb info can only update after rte_eth_dev_configure().
> >>>>>>> So this patch adds rte_eth_dev_configure() before
> >>>>>>> rte_eth_dev_get_dcb_info()
> >>>>>> to get updated dcb info to fix this issue.
> >>>>>>>
> >>>>>> Thank you for your advice. But the above adjustments may still
> >>>>>> not work for some drivers. The mapping between queues and TCs in
> >>>>>> these drivers is updated in the dev_start stage.
> >>>>>>
> >>>>>> I have an idea. We can move fwd_config_setup() to start_port(),
> >>>>>> which is called by main() and after starting ports This not only
> >>>>>> solves the segment fault, but also does not have the problem you
> >>>>>> mentioned above. I
> >>>> test it and it is ok.
> >>>>>>
> >>>>>> What do you think, xiaoyun?
> >>>>>
> >>>>> How can you fix the issue I mentioned?
> >>>>> You still need to start port first to see the updated fwd config.
> >>>> Yes. But it can make sure that users get the correct new fwd
> >>>> streams before executing the 'start' command.
> >>>>> And for those drivers, why does the mapping has to be updated in
> >>>>> dev_start
> >>>> stage?
> >>>>> What does it need? Can't it be moved to dev_configure?
> >>>> The framework does not require that the configuration parameters
> >>>> transferred by the dev_configure API must be updated in the
> >>>> interface of driver. The driver can  verify the correctness of
> >>>> these parameters in the interface, and then complete the update in
> >>>> the dev_start stage. It depends on the design and need of the
> >>>> driver. Maybe it's more appropriate to put it in dev_configure, but
> >>>> now we can't ask all these
> >> drivers to modify the operation.
> >>>
> >>> I still think it's driver's responsibility to configure DCB in dev_configure.
> >>>
> >>> Calling fwd_config_setup here is because this command changes queue
> >> number. Users just change queue number so want to check fwd setup.
> >> It's a normal and reasonable behavior.
> >>> Starting port will be called in many situations. I don't think it's
> >>> appropriate to
> >> call fwd_config_setup every time calling port_start. And you can't
> >> expect users know the fwd setup only change after port_start.
> >>>
> >>> These are only my thoughts. If anyone else agrees you, they can give you
> ack.
> >>>>>>
> >> @Ferruh, what do you think?
> >> Currently, both ixgbe and i40e also have this problem. Even if
> >> testpmd can be modified according to Xiaoyun's suggestion, and the
> >> driver is not modified, this problem still exists in some drivers, such as ixgbe
> and hns3.
> >
> > NO. I40e and ixgbe DON'T have the issue.
> > I40e tc mapping is done in dev_configure.
> > ixgbe tc mapping is always fixed due to hw design. It only needs dev_configure
> to get dcb_conf info. No matter you configure dcb or not, the mapping is always
> the same. You can check ixgbe_dev_get_dcb_info().
> >
> > So for i40e and ixgbe, you only need to add dev_configure in
> dcb_fwd_config_setup().
> >
> Hi, Xiaoyun,
> 
> The fwd_config_setup() is called when the port is in the stop state and start
> state.
> When we run "start" cmd, all port are in the start state.  Namely,  we need to
> decide whether to call rte_eth_dev_configure() based on the state of all ports
> before rte_eth_dev_get_dcb_info() in dcb_fwd_config_setup().
> Modify as follows:
> @@ -1834,6 +1834,7 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>                  return;
>          }
> 
> +       fwd_config_setup();
>          init_port_config();
> 
>          cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); diff --git a/app/test-
> pmd/config.c b/app/test-pmd/config.c index bd6ee8b..5981b50 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3179,7 +3179,24 @@ dcb_fwd_config_setup(void)
>          uint16_t nb_rx_queue, nb_tx_queue;
>          uint16_t i, j, k, sm_id = 0;
>          uint16_t total_tc_num = 0;
> +       struct rte_port *port;
>          uint8_t tc = 0;
> +       portid_t pid;
> +       int ret;
> +
> +       if (all_ports_stopped()) {
> +               /* re-configure the device after changing queue numbers. */
> +               for (pid = 0; pid < nb_fwd_ports; pid++) {
> +                       port = &ports[pid];
> +                       ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +                                                   &port->dev_conf);
> +                       if (ret < 0) {
> +                               printf("Failed to re-configure port %d,
> ret = %d.\n",
> +                                       pid, ret);
> +                               return;
> +                       }
> +               }
> +       }
> 
>          cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>          cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
> 
> But there's still no way to get away with the segment fault for ixgbe and i40e.
> 1/ For ixgbe, tc mapping is always fixed after DCB is configured.  If the number
> of queues is reduced, segment fault occur.
>      Does it mean that the number of queues cannot be modified for ixgbe after
> DCB is configured?
> 2/ For i40e, it appears that the mapping between tc and queue is not updated
> after changing queue numbers. From i40e_vsi_config_tc(), its update seems to
> depend only on tc_map changes.
> 
> please check it?

No.
After users config queue numbers using cmd "port config all rxq 4". It's normal that they may want to check what new fwd config will be like using "show config fwd"
So you should call fwd_config_setup() in cmd_config_rx_tx_parsed(). The ports are in stopped state.

dcb_fwd_config_setup() will call rte_eth_dev_get_dcb_info() at first to get the dcb info first.
The segment fault happens because rte_eth_dev_get_dcb_info() get the wrong info based on old queue numbers (new queue number is small).

For ixgbe, The fixed mapping means for maximum queue numbers for example 128 queues. Each queue and tc mapping is fixed to fill to hw. Fixed values. It's just you enable which queues or not.
You can change queue number as you want. In dev_start stage, tc will actually take effect. get_dcb_info() only needs rx_adv_conf.dcb_rx_conf which info is included in dev_conf.
So for ixgbe, you only need to call dev_configure to update the new dcb_rx_conf for get_dcb_info().

For i40e, dcb_setup is in dev_configure stage. i40e_dev_get_dcb_info() will get the correct info too.

> >>>>>>
> >>>>>>>>
> >>>>>>>> Like:
> >>>>>>>> set nbcore 4
> >>>>>>>> port stop all
> >>>>>>>> port config 0 dcb vt off 4 pfc on port start all port stop all
> >>>>>>>> port config all rxq 8 port config all txq 8
> >>>>>>>>
> >>>>>>>> At the moment, a segmentation fault occurs.
> >>>>>>>>
> >>>>>>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration
> >>>>>>>> settings")
> >>>>>>>> Cc: stable@dpdk.org
> >>>>>>>>
> >>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>>>>>> ---
> >>>>>>>> V1->V2:
> >>>>>>>> - use stream instead of flow
> >>>>>>>> ---
> >>>>>>>>      app/test-pmd/cmdline.c | 2 --
> >>>>>>>>      1 file changed, 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> >>>>>>>> index 4df0c32..e316f5c 100644
> >>>>>>>> --- a/app/test-pmd/cmdline.c
> >>>>>>>> +++ b/app/test-pmd/cmdline.c
> >>>>>>>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void
> >> *parsed_result,
> >>>>>>>>      		return;
> >>>>>>>>      	}
> >>>>>>>>
> >>>>>>>> -	fwd_config_setup();
> >>>>>>>> -
> >>>>>>>>      	init_port_config();
> >>>>>>>>
> >>>>>>>>      	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> >>>>>>>> --
> >>>>>>>> 2.7.4
> >>>>>>>
> >>>>>>> .
> >>>>>>>
> >>>>> .
> >>>>>
> >>> .
> >>>
> > _______________________________________________
> > Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
> > email to linuxarm-leave@openeuler.org
> >