DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Li, Xiaoyun" <xiaoyun.li@intel.com>
To: oulijun <oulijun@huawei.com>, "Yigit, Ferruh" <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"linuxarm@openeuler.org" <linuxarm@openeuler.org>
Subject: Re: [dpdk-dev] [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx and Tx
Date: Fri, 2 Apr 2021 02:33:30 +0000	[thread overview]
Message-ID: <DM4PR11MB553462192EADBBC0D93D39DF997A9@DM4PR11MB5534.namprd11.prod.outlook.com> (raw)
In-Reply-To: <c7645662-f47d-8683-8105-f270b2b04157@huawei.com>



> -----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().

> >>>>
> >>>>>>
> >>>>>> 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
> >>>>>
> >>>>> .
> >>>>>
> >>> .
> >>>
> > .
> >

  reply	other threads:[~2021-04-02  2:33 UTC|newest]

Thread overview: 85+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-05 10:22 [dpdk-dev] [PATCH 0/3] Fixes for testpmd Lijun Ou
2021-03-05 10:22 ` [dpdk-dev] [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test Lijun Ou
2021-03-23  8:55   ` Li, Xiaoyun
2021-03-23 14:17     ` oulijun
2021-03-23 14:17     ` oulijun
2021-03-24  1:51       ` Li, Xiaoyun
2021-04-10  1:12       ` Ferruh Yigit
2021-04-10  2:53         ` oulijun
2021-03-23 14:18     ` oulijun
2021-03-24  2:03       ` Li, Xiaoyun
2021-03-24 13:40         ` [dpdk-dev] [Linuxarm] " oulijun
2021-03-25  2:21           ` Li, Xiaoyun
2021-03-27  3:53             ` oulijun
2021-04-10  0:56             ` Ferruh Yigit
2021-04-10  2:58               ` oulijun
2021-04-12  7:52                 ` Ferruh Yigit
2021-04-12 12:20                   ` oulijun
2021-03-05 10:22 ` [dpdk-dev] [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx and Tx Lijun Ou
2021-03-23  7:50   ` Li, Xiaoyun
2021-03-24  1:00     ` oulijun
2021-03-24  1:44       ` Li, Xiaoyun
2021-03-25  3:03         ` oulijun
2021-03-29  1:53           ` Li, Xiaoyun
2021-04-02  1:44             ` oulijun
2021-04-02  2:33               ` Li, Xiaoyun [this message]
2021-04-09  6:09                 ` [dpdk-dev] [Linuxarm] " oulijun
2021-04-09  7:08                   ` Li, Xiaoyun
2021-04-10  3:02                     ` oulijun
2021-04-12 10:35   ` [dpdk-dev] " Ferruh Yigit
2021-04-12 12:20     ` oulijun
2021-03-05 10:22 ` [dpdk-dev] [PATCH 3/3] app/testpmd: use of Rx/Tx in testpmd Lijun Ou
2021-03-23  3:12   ` Li, Xiaoyun
2021-03-23  3:17     ` Li, Xiaoyun
2021-03-23  7:25       ` Andrew Rybchenko
2021-03-23  7:46         ` Tu, Lijuan
2021-03-23  7:51           ` oulijun
2021-03-23  7:58             ` Li, Xiaoyun
2021-03-23 10:42               ` Ferruh Yigit
2021-03-23  7:59             ` Tu, Lijuan
2021-03-20  1:19 ` [dpdk-dev] [PATCH 0/3] Fixes for testpmd Min Hu (Connor)
2021-04-20  7:31 ` [dpdk-dev] [PATCH V2 0/7] modifications about DCB forwarding configuration Huisong Li
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 1/7] app/testpmd: fix forward lcores number when DCB test Huisong Li
2021-04-27  9:20     ` Li, Xiaoyun
2021-04-27 13:49       ` Huisong Li
2021-04-28  2:13         ` Li, Xiaoyun
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 2/7] app/testpmd: fix DCB forwarding configuration Huisong Li
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 3/7] app/testpmd: fix a segment fault when DCB test Huisong Li
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 4/7] app/testpmd: add check for support of reporting DCB info Huisong Li
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 5/7] app/testpmd: move position of verifying DCB test Huisong Li
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 6/7] app/testpmd: add forwarding config in start port Huisong Li
2021-04-20  7:32   ` [dpdk-dev] [PATCH V2 7/7] app/testpmd: remove redundant fwd streams initialization Huisong Li
2021-04-20  7:35 ` [dpdk-dev] [PATCH 0/3] Fixes for testpmd Huisong Li
2021-04-20  9:00 ` [dpdk-dev] [PATCH V3 0/7] modifications about DCB forwarding configuration Huisong Li
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 1/7] app/testpmd: fix forward lcores number when DCB test Huisong Li
2021-04-27 10:18     ` Li, Xiaoyun
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 2/7] app/testpmd: fix DCB forwarding configuration Huisong Li
2021-04-27 10:59     ` Li, Xiaoyun
2021-04-27 14:13       ` Huisong Li
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB test Huisong Li
2021-04-27 11:19     ` Li, Xiaoyun
2021-04-27 14:10       ` Huisong Li
2021-04-28  2:02         ` Li, Xiaoyun
2021-04-28  2:26           ` Huisong Li
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 4/7] app/testpmd: add check for support of reporting DCB info Huisong Li
2021-04-27 11:26     ` Li, Xiaoyun
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 5/7] app/testpmd: move position of verifying DCB test Huisong Li
2021-04-27 11:33     ` Li, Xiaoyun
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 6/7] app/testpmd: add forwarding config in start port Huisong Li
2021-04-27 11:43     ` Li, Xiaoyun
2021-04-28  2:14       ` Huisong Li
2021-04-20  9:01   ` [dpdk-dev] [PATCH V3 7/7] app/testpmd: remove redundant fwd streams initialization Huisong Li
2021-04-27 11:50     ` Li, Xiaoyun
2021-04-28  6:40 ` [dpdk-dev] [PATCH V4 0/7] modifications about DCB forwarding configuration Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 1/7] app/testpmd: fix forward lcores number when DCB test Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 2/7] app/testpmd: fix DCB forwarding configuration Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 3/7] app/testpmd: fix a segment fault when DCB test Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 4/7] app/testpmd: add check for support of reporting DCB info Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 5/7] app/testpmd: move position of verifying DCB test Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 6/7] app/testpmd: add forwarding configuration when config DCB Huisong Li
2021-04-28  6:40   ` [dpdk-dev] [PATCH V4 7/7] app/testpmd: remove redundant fwd streams initialization Huisong Li
2021-04-28  7:01   ` [dpdk-dev] [PATCH V4 0/7] modifications about DCB forwarding configuration Li, Xiaoyun
2021-04-29 16:13     ` Ferruh Yigit
  -- strict thread matches above, loose matches on Subject: below --
2021-03-05  0:55 [dpdk-dev] [PATCH 0/3] testpmd updates Lijun Ou
2021-03-05  0:55 ` [dpdk-dev] [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx and Tx Lijun Ou
2021-03-05  3:21   ` Li, Xiaoyun
2021-03-05  6:12     ` oulijun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM4PR11MB553462192EADBBC0D93D39DF997A9@DM4PR11MB5534.namprd11.prod.outlook.com \
    --to=xiaoyun.li@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=linuxarm@openeuler.org \
    --cc=oulijun@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).