> -----Original Message-----
> From: oulijun <ouli...@huawei.com>
> Sent: Friday, April 2, 2021 09:45
> To: Li, Xiaoyun <xiaoyun...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>
> Cc: dev@dpdk.org; linux...@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 <ouli...@huawei.com>
> >> Sent: Thursday, March 25, 2021 11:04
> >> To: Li, Xiaoyun <xiaoyun...@intel.com>; Yigit, Ferruh
> >> <ferruh.yi...@intel.com>
> >> Cc: dev@dpdk.org; linux...@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 <ouli...@huawei.com>
> >>>> Sent: Wednesday, March 24, 2021 09:01
> >>>> To: Li, Xiaoyun <xiaoyun...@intel.com>; Yigit, Ferruh
> >>>> <ferruh.yi...@intel.com>
> >>>> Cc: dev@dpdk.org; linux...@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 <ouli...@huawei.com>
> >>>>>> Sent: Friday, March 5, 2021 18:22
> >>>>>> To: Yigit, Ferruh <ferruh.yi...@intel.com>
> >>>>>> Cc: Li, Xiaoyun <xiaoyun...@intel.com>; dev@dpdk.org;
> >>>>>> linux...@openeuler.org
> >>>>>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from
> >>>>>> parsing Rx and Tx
> >>>>>>
> >>>>>> From: Huisong Li <lihuis...@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().
> >>>>
> >>>>>>
> >>>>>> 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: sta...@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Huisong Li <lihuis...@huawei.com>
> >>>>>> Signed-off-by: Lijun Ou <ouli...@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
> >>>>>
> >>>>> .
> >>>>>
> >>> .
> >>>
> > .
> >
