Re: [dpdk-dev] [PATCH] net/pcap: set queue started and stopped

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

On 7/18/2018 5:04 PM, Eads, Gage wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, July 18, 2018 9:25 AM
>> To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
>> Subject: Re: [PATCH] net/pcap: set queue started and stopped
>>
>> On 7/18/2018 3:17 PM, Eads, Gage wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Wednesday, July 18, 2018 4:14 AM
>>>> To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
>>>> Subject: Re: [PATCH] net/pcap: set queue started and stopped
>>>>
>>>> On 7/9/2018 9:21 PM, Gage Eads wrote:
>>>>> Set the rx and tx queue state appropriately when the queues or
>>>>> device are started or stopped.
>>>>
>>>> Is there a specific reason to enable these dev_ops, if so can you
>>>> please document in commit log?
>>>
>>> Yes, the purpose of the patch is to enable the rte_eth_dev_{rx,
>> tx}_queue_{start, stop} functions for the PCAP PMD. I'll update the message in
>> v2.
>>
>> I guess that part is clear :) I was asking if there is a higher level reason to enable
>> queue start/stop on these PMDs?
>> Is there some specific usecase not working for you when these are not enabled?
>>
> 
> We have an application that uses the start/stop functions for deferred queue starting, and runs with a variety of PMDs. Even though the PCAP PMD doesn't have any notion "deferred start", some of the other PMDs we use do.
> 
> We've also got some local changes that, if RTE_LIBRTE_ETHDEV_DEBUG is true, will return an error if you try to receive or transmit from a queue whose state is STOPPED. Without the PCAP patch, this debug check fails. We're looking into submitting that debug code in the future, but in the meantime we wanted to make the PCAP compliant with the start/stop ethdev functions.

Got it, thanks for clarification.

> 
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Gage Eads <gage.eads@intel.com>
>>>>> ---
>>>>>  drivers/net/pcap/rte_eth_pcap.c | 42
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/pcap/rte_eth_pcap.c
>>>>> b/drivers/net/pcap/rte_eth_pcap.c index 6bd4a7d79..21e466bcd 100644
>>>>> --- a/drivers/net/pcap/rte_eth_pcap.c
>>>>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>>>>> @@ -430,6 +430,10 @@ eth_dev_start(struct rte_eth_dev *dev)
>>>>>  				return -1;
>>>>>  			rx->pcap = tx->pcap;
>>>>>  		}
>>>>> +
>>>>> +		dev->data->tx_queue_state[0] =
>>>> RTE_ETH_QUEUE_STATE_STARTED;
>>>>> +		dev->data->rx_queue_state[0] =
>>>> RTE_ETH_QUEUE_STATE_STARTED;
>>>>
>>>> pcap also supports multiple queue, instead of hardcoding the queue 0
>>>> it can be possible to iterate through dev->data->nb_rx_queues,
>>>> dev->data-
>>>>> nb_tx_queues.
>>>>
>>>> And I think it is not good to set this in "internals->single_iface"
>>>> condition, it is better to do these assignments just above
>>>> "status_up" after all queues initialized.
>>>>
>>>>> +
>>>>>  		goto status_up;
>>>>>  	}
>>>>>
>>>>> @@ -490,6 +494,8 @@ eth_dev_stop(struct rte_eth_dev *dev)
>>>>>  		pcap_close(tx->pcap);
>>>>>  		tx->pcap = NULL;
>>>>>  		rx->pcap = NULL;
>>>>> +		dev->data->tx_queue_state[0] =
>>>> RTE_ETH_QUEUE_STATE_STOPPED;
>>>>> +		dev->data->rx_queue_state[0] =
>>>> RTE_ETH_QUEUE_STATE_STOPPED;
>>>>
>>>> same here, just above "status_down" is better place and by using
>>>> dev->data->nb_[r/t]x_queues
>>>
>>> Agreed, I will move the started and stopped assignments as you suggested.
>>>
>