From: Jie Hai <haijie1@huawei.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Thomas Monjalon <thomas@monjalon.net>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: "lihuisong (C)" <lihuisong@huawei.com>,
David Marchand <david.marchand@redhat.com>
Subject: Re: [PATCH v2 1/8] lib/ethdev: update Rx and Tx queue status
Date: Sat, 7 Oct 2023 16:36:37 +0800 [thread overview]
Message-ID: <0e4b8a39-9c57-b645-4fc3-f80451481f7f@huawei.com> (raw)
In-Reply-To: <f0aab038-662d-4110-a3a3-1f5fe222338d@amd.com>
On 2023/9/28 21:15, Ferruh Yigit wrote:
> On 9/28/2023 8:42 AM, Jie Hai wrote:
>> The DPDK framework reports the queue status, which is stored in
>> 'dev->data->tx_queue_state' and 'dev->data->rx_queue_state'.The
>> state is currently maintained by the drivers. Users may determine
>> whether a queue participates in packet forwarding based on the
>> state. However, not all drivers correctly report the queue status.
>> This may cause forwarding problems.
>>
>> Since it is difficult to modify the queue status of all drivers,
>> we consider updating the queue status at the framework level.
>> Some drivers support queues for hairpin, leaving status updating
>> for such queues to the drivers. Some drivers support
>> 'deferred_start'. Assume that all drivers that support
>> 'deferred_start' can obtain the configuration through
>> 'rte_eth_tx_queue_info_get' and 'rte_eth_rx_queue_info_get'. So
>> we can directly update the queue status in 'rte_eth_dev_start'
>> and 'rte_eth_dev_stop'.
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> ---
>> lib/ethdev/rte_ethdev.c | 40 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 40 insertions(+)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 0840d2b5942a..e3aaa71eba9e 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1641,6 +1641,9 @@ rte_eth_dev_start(uint16_t port_id)
>> struct rte_eth_dev_info dev_info;
>> int diag;
>> int ret, ret_stop;
>> + uint16_t i;
>> + struct rte_eth_rxq_info rxq_info;
>> + struct rte_eth_txq_info txq_info;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> dev = &rte_eth_devices[port_id];
>> @@ -1697,6 +1700,30 @@ rte_eth_dev_start(uint16_t port_id)
>> (*dev->dev_ops->link_update)(dev, 0);
>> }
>>
>> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> + if (rte_eth_dev_is_rx_hairpin_queue(dev, i))
>> + continue;
>> +
>> + memset(&rxq_info, 0, sizeof(rxq_info));
>> + ret = rte_eth_rx_queue_info_get(port_id, i, &rxq_info);
>> + if (ret == 0 && rxq_info.conf.rx_deferred_start != 0)
>> + dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
>> + else
>> + dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
>> + }
>> +
>> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> + if (rte_eth_dev_is_tx_hairpin_queue(dev, i))
>> + continue;
>> +
>> + memset(&txq_info, 0, sizeof(txq_info));
>> + ret = rte_eth_tx_queue_info_get(port_id, i, &txq_info);
>> + if (ret == 0 && txq_info.conf.tx_deferred_start != 0)
>> + dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
>> + else
>> + dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
>> + }
>> +
>
> Hi Jie,
>
> I am not sure about adding queue_info_get() calls a dependency to
> start(), since start() is a mandatory API, I am concerned about possible
> side affects.
> Like if rte_eth_rx_queue_info_get() fails,Yes, unless we let rte_eth_rx|tx_queue_info_get a mandatory API.
> Or event though deferred_start is set, can application first call
> rx_queue_start(), later start(), like:
> start()
> rx_queue_start()
> stop()
> rx_queue_start()
> start()
start() --> deferred_start is confugured, the queue is stopped
rx_queue_start() --> the queue is started
stop() --> all queues are stopped
rx_queue_start() --> not supported, port should starts first
start()
If we change the order as:
start()
rx_queue_start()
stop()
start()
The last status of the queue for different drivers may be different.
Most drivers starts all queues except the queue setting deferred_start.
For sfc driver, all queues are started and the status of the queue
setting deferred_start will be reported as stopped, which is not correct.
That's the point, drivers have their own private processing for
different sequences of deferred_start, start|stop and queue_start|stop.
> Or even applications sets deferred_start, PMD be ignoring without
> reflecting that to conf.rx_deferred_start, etc...
Supppose the app sets the deferred_start,
whether the driver verifies the support the configuration or not,
whether the driver issues the configuration to the hardware or not,
whether the driver saves and reports the configuration or not,
Different driver implementations vary.
For the above three cases, pay attention to the following:
1) Y-Y-Y That's what we need.
2) Y-Y-N That's what we should add.
3) N-N-Y This will lead to wrong information(e.g.ice_rxtx.c mlx5_txq.c
mlx5_rxq.c)
4) N-N-N That's what we need, too.
>
>
> Anyway, intention was to move common task, setting queue state, to the
> ethdev layer, but because of the deferred_start, in rte_eth_dev_start()
> we don't really know the queue state.
>
> We can try to move deferred state information to the ethdev, but that is
> similar to setting queue state in the driver, that is why perhaps better
> to leave setting state to drivers, as done in the first version of the set.
> @Jie, @David, what do you think?
>
I support V1. Whoever changes the queue status is responsible for
reporting the queue status.
That will maintains the specificity of the drive treatment.
>
> And, @Jie, can you also check the rx_queue_start() & tx_queue_start()
> dev_ops of the drivers sets the queue state?
> If missing on some drivers, that needs to be added too.
>
I have checked already, all drivers support rx_queue_start() &
tx_queue_start() set the queue state.
>
>> /* expose selection of PMD fast-path functions */
>> eth_dev_fp_ops_setup(rte_eth_fp_ops + port_id, dev);
>>
>> @@ -1708,6 +1735,7 @@ int
>> rte_eth_dev_stop(uint16_t port_id)
>> {
>> struct rte_eth_dev *dev;
>> + uint16_t i;
>> int ret;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> @@ -1731,6 +1759,18 @@ rte_eth_dev_stop(uint16_t port_id)
>> dev->data->dev_started = 0;
>> rte_ethdev_trace_stop(port_id, ret);
>>
>> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> + if (rte_eth_dev_is_rx_hairpin_queue(dev, i))
>> + continue;
>> + dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
>> + }
>> +
>> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> + if (rte_eth_dev_is_tx_hairpin_queue(dev, i))
>> + continue;
>> + dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
>> + }
>> +
>> return ret;
>> }
>>
>
next prev parent reply other threads:[~2023-10-07 8:36 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-08 11:28 [PATCH 00/36] fix Rx and Tx queue state Jie Hai
2023-09-08 11:28 ` [PATCH 01/36] net/axgbe: " Jie Hai
2023-09-08 11:28 ` [PATCH 02/36] net/af_packet: " Jie Hai
2023-09-08 11:28 ` [PATCH 03/36] net/af_xdp: " Jie Hai
2023-09-08 11:28 ` [PATCH 04/36] net/avp: " Jie Hai
2023-09-08 11:28 ` [PATCH 05/36] net/bnx2x: " Jie Hai
2023-09-08 11:28 ` [PATCH 06/36] net/bnxt: " Jie Hai
2023-09-08 11:28 ` [PATCH 07/36] net/bonding: " Jie Hai
2023-09-08 11:28 ` [PATCH 08/36] net/cxgbe: " Jie Hai
2023-09-08 11:28 ` [PATCH 09/36] net/dpaa: " Jie Hai
2023-09-16 10:07 ` Hemant Agrawal
2023-09-08 11:28 ` [PATCH 10/36] net/dpaa2: " Jie Hai
2023-09-16 10:07 ` Hemant Agrawal
2023-09-08 11:28 ` [PATCH 11/36] net/e1000: " Jie Hai
2023-09-08 11:28 ` [PATCH 12/36] net/ena: " Jie Hai
2023-09-08 11:28 ` [PATCH 13/36] net/enetc: " Jie Hai
2023-09-08 11:28 ` [PATCH 14/36] net/enic: " Jie Hai
2023-09-08 11:28 ` [PATCH 15/36] net/hinic: " Jie Hai
2023-09-08 11:28 ` [PATCH 16/36] net/ipn3ke: " Jie Hai
2023-09-10 2:56 ` Xu, Rosen
2023-09-08 11:28 ` [PATCH 17/36] net/memif: " Jie Hai
2023-09-08 11:28 ` [PATCH 18/36] net/mana: " Jie Hai
2023-09-08 11:28 ` [PATCH 19/36] net/mlx4: " Jie Hai
2023-09-08 11:28 ` [PATCH 20/36] net/mvneta: " Jie Hai
2023-09-08 11:28 ` [PATCH 21/36] net/mvpp2: " Jie Hai
2023-09-08 11:28 ` [PATCH 22/36] net/netvsc: " Jie Hai
2023-09-08 11:28 ` [PATCH 23/36] net/nfp: " Jie Hai
2023-09-11 1:45 ` Chaoyong He
2023-09-08 11:28 ` [PATCH 24/36] net/ngbe: " Jie Hai
2023-09-08 11:28 ` [PATCH 25/36] net/null: " Jie Hai
2023-09-08 11:28 ` [PATCH 26/36] net/octeon_ep: " Jie Hai
2023-09-08 11:28 ` [PATCH 27/36] net/octeontx: " Jie Hai
2023-11-02 9:59 ` [EXT] " Harman Kalra
2023-11-02 12:34 ` Jie Hai
2023-09-08 11:28 ` [PATCH 28/36] net/pfe: " Jie Hai
2023-09-08 11:28 ` [PATCH 29/36] net/ring: " Jie Hai
2023-09-08 11:28 ` [PATCH 30/36] net/sfc: " Jie Hai
2023-09-08 12:01 ` Andrew Rybchenko
2023-09-12 2:39 ` Jie Hai
2023-09-08 11:28 ` [PATCH 31/36] net/softnic: " Jie Hai
2023-09-18 11:24 ` Dumitrescu, Cristian
2023-09-08 11:28 ` [PATCH 32/36] net/txgbe: " Jie Hai
2023-09-08 11:28 ` [PATCH 33/36] net/vhost: " Jie Hai
2023-09-08 11:28 ` [PATCH 34/36] net/virtio: " Jie Hai
2023-09-08 11:29 ` [PATCH 35/36] net/vmxnet3: " Jie Hai
2023-09-08 11:29 ` [PATCH 36/36] app/testpmd: fix primary process not polling all queues Jie Hai
2023-09-08 11:50 ` [PATCH 00/36] fix Rx and Tx queue state David Marchand
2023-09-18 16:54 ` Ferruh Yigit
2023-09-22 2:41 ` Jie Hai
2023-09-22 6:41 ` David Marchand
2023-09-26 13:59 ` Jie Hai
2023-09-28 12:51 ` Ferruh Yigit
2023-09-28 7:42 ` [PATCH v2 0/8] " Jie Hai
2023-09-28 7:42 ` [PATCH v2 1/8] lib/ethdev: update Rx and Tx queue status Jie Hai
2023-09-28 9:24 ` lihuisong (C)
2023-09-28 13:15 ` Ferruh Yigit
2023-10-07 8:36 ` Jie Hai [this message]
2023-10-16 11:23 ` Ferruh Yigit
2023-09-28 7:42 ` [PATCH v2 2/8] net/cpfl: support getting queue information Jie Hai
2023-10-01 16:04 ` Ali Alnubani
2023-09-28 7:43 ` [PATCH v2 3/8] net/enetc: save deferred start configuratin for queues Jie Hai
2023-09-28 7:43 ` [PATCH v2 4/8] net/enetc: support getting queue information Jie Hai
2023-09-28 7:43 ` [PATCH v2 5/8] net/failsafe: " Jie Hai
2023-09-28 7:43 ` [PATCH v2 6/8] net/fm10k: " Jie Hai
2023-09-28 7:43 ` [PATCH v2 7/8] net/idpf: " Jie Hai
2023-09-28 7:43 ` [PATCH v2 8/8] app/testpmd: fix primary process not polling all queues Jie Hai
2023-10-01 16:08 ` [PATCH v2 0/8] fix Rx and Tx queue state Ali Alnubani
2023-10-16 11:51 ` [PATCH 00/36] " Ferruh Yigit
2023-10-16 12:01 ` Ferruh Yigit
2023-10-17 14:11 ` Thomas Monjalon
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=0e4b8a39-9c57-b645-4fc3-f80451481f7f@huawei.com \
--to=haijie1@huawei.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=lihuisong@huawei.com \
--cc=thomas@monjalon.net \
/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).