Re: [PATCH 30/36] net/sfc: fix Rx and Tx queue state

[Jie Hai <haijie1@huawei.com>]

On 2023/9/8 20:01, Andrew Rybchenko wrote:
> On 9/8/23 14:28, Jie Hai wrote:
>> The DPDK framework reports the queue state, which is stored in
>> dev->data->tx_queue_state and dev->data->rx_queue_state. The
>> state is maintained by the driver. Users may determine whether
>> a queue participates in packet forwarding based on the state.
>> Therefore, the driver needs to modify the queue state in time
>> according to the actual situation.
>>
>> Fixes: 9ad9ff476cac ("ethdev: add queue state in queried queue 
>> information")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> ---
>>   drivers/net/sfc/sfc_repr.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/net/sfc/sfc_repr.c b/drivers/net/sfc/sfc_repr.c
>> index 6c7727d56980..278e37477530 100644
>> --- a/drivers/net/sfc/sfc_repr.c
>> +++ b/drivers/net/sfc/sfc_repr.c
>> @@ -263,6 +263,7 @@ static int
>>   sfc_repr_dev_start(struct rte_eth_dev *dev)
>>   {
>>       struct sfc_repr *sr = sfc_repr_by_eth_dev(dev);
>> +    uint16_t i;
>>       int ret;
>>       sfcr_info(sr, "entry");
>> @@ -274,6 +275,11 @@ sfc_repr_dev_start(struct rte_eth_dev *dev)
>>       if (ret != 0)
>>           goto fail_start;
>> +    for (i = 0; i < dev->data->nb_rx_queues; i++)
>> +        dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
>> +    for (i = 0; i < dev->data->nb_tx_queues; i++)
>> +        dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> 
> May be I miss something, but the driver does when queue is
> really started in sfc_rx_qstart() and sfc_tx_qstart().
> Also the patch is wrong in general in the case of deferred
> start queues.
> 
> Same for stop.
Hi, Andrew Rybchenko,

Thanks for your review.

There are two ops implementations,
one in file sfc_ethdev.c(eg. sfc_dev_start)
and the other in file sfc_repr.c (eg. sfc_repr_dev_start).
This patch is for the second one, which does not support deferred start, 
please see 'sfc_repr_rx_qcheck_conf' and 'sfc_repr_tx_qcheck_conf'.

The function process of ‘sfc_repr_dev_start’and ‘sfc_repr_dev_stop’ is 
as follows:
sfc_repr_dev_start
   -->sfc_repr_start
     -->sfc_repr_proxy_start_repr
       -->sfc_repr_proxy_start
       	-->sfc_repr_proxy_rxq_start
       	  -->sfc_rx_qstart -- RTE_ETH_QUEUE_STATE_STARTED
       	-->sfc_repr_proxy_txq_start

sfc_repr_dev_stop
   -->sfc_repr_stop
     -->sfc_repr_proxy_stop_repr
       -->sfc_repr_proxy_stop
       	-->sfc_repr_proxy_rxq_stop
       	  -->sfc_rx_qstop -- RTE_ETH_QUEUE_STATE_STOPPED
       	-->sfc_repr_proxy_txq_stop

Since the Rx has been modified, Maybe I should modify the TX queue 
status separately in 'sfc_repr_proxy_txq_start/stop'.

--
Best regards,
Jie Hai
> 
>> +
>>       sfcr_info(sr, "done");
>>       return 0;
>> @@ -338,6 +344,7 @@ static int
>>   sfc_repr_dev_stop(struct rte_eth_dev *dev)
>>   {
>>       struct sfc_repr *sr = sfc_repr_by_eth_dev(dev);
>> +    uint16_t i;
>>       int ret;
>>       sfcr_info(sr, "entry");
>> @@ -352,6 +359,11 @@ sfc_repr_dev_stop(struct rte_eth_dev *dev)
>>       sfc_repr_unlock(sr);
>> +    for (i = 0; i < dev->data->nb_rx_queues; i++)
>> +        dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
>> +    for (i = 0; i < dev->data->nb_tx_queues; i++)
>> +        dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
>> +
>>       sfcr_info(sr, "done");
>>       return 0;
> 
> .