Re: [PATCH 00/36] fix Rx and Tx queue state

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/22/2023 7:41 AM, David Marchand wrote:
> Hello,
> 
> On Fri, Sep 22, 2023 at 4:41 AM Jie Hai <haijie1@huawei.com> wrote:
>> On 2023/9/19 0:54, Ferruh Yigit wrote:
>>> On 9/8/2023 12:50 PM, David Marchand wrote:
>>>> On Fri, Sep 8, 2023 at 1:32 PM Jie Hai <haijie1@huawei.com> 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,
>>>>> for example,
>>>>
>>>> The driver is maintaining this state in dev_start / dev_stop and per
>>>> queue start/stop handlers.
>>>>
>>>>>
>>>>> [1] 5028f207a4fa ("app/testpmd: fix secondary process packet forwarding"
>>>>> [2] 141a520b35f7 ("app/testpmd: fix primary process not polling all queues")
>>>>>
>>>>> Therefore, the drivers need to modify the queue state in time
>>>>> according to the actual situation, especially when dev_start
>>>>> and dev_stop are called. see [3] for more information.
>>>>>
>>>>> [3] https://inbox.dpdk.org/dev/20230721160422.3848154-1-ferruh.yigit@amd.com/
>>>>>
>>>>> This patchset also resubmit the patch [2] and makes some fixes on the patch.
>>>>
>>>> I just had a quick look at some patches and I wonder if a better fix
>>>> would be at the ethdev level, rather than fixing all drivers.
>>>>
>>>>
>>>
>>> I came here to make the same comment,
>>>
>>> Jie, I forgot if we discuss this already but,
>>>
>>> does it work if we update queue state in 'rte_eth_dev_start()' &
>>> 'rte_eth_dev_stop()' when 'dev_start' & 'dev_stop' dev_ops succeeds?
>>>
>>> This solves the case driver forgets to update the queue state.
>>>
>>>
>> Hi, Furrh and David,
>>
>> It's OK for dev_stop, but not enough for dev_start.
>> Some drivers also support deferred_start.
>> Therefore, not all queues are in the start state after dev_start.
>>
>> If we want to get that correct queue state at the framework level, I
>> offer the following options:
>>
>> step 1. A capability(e.g. RTE_ETH_DEV_CAPA_DEFERRED_START) is added to
>> the framework, indicating whether the driver supports deferred_start.
>> The capability should be reported by the driver and user can get it by
>> rte_eth_dev_info_get().
>> step 2. All drivers that support deferred_start should report configuration
>> information about deferred_start through
>> rte_eth_rx_queue_info_get |rte_eth_tx_queue_info_get.
>> step 3.The framework updates the queue status based on the support and
>> configuration of deferred_start.
> 
> rte_eth_dev_start must only update the queue state if
> rx_deferred_start is unset (see struct
> rte_eth_rxconf::rx_deferred_start).
> And the queue state needs to be updated in ethdev
> rte_eth_dev_rx_queue_start/stop.
> 
> So I don't see where we need a new capability.
> 
> 

Hi David,

Problem seems 'rte_eth_rxconf::rx_deferred_start' is not stored in
ethdev level, so rte_eth_dev_start() can't use it.


And although application can request it, via rxconf, it is not clear if
driver supports it or if it will take into account, that is where a
capability flag can be useful, but it is a big change for this patch.