diff mbox series

net/pcap: set queue started and stopped

Message ID 20180709202159.18726-1-gage.eads@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/pcap: set queue started and stopped |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Eads, Gage July 9, 2018, 8:21 p.m. UTC 
  Set the rx and tx queue state appropriately when the queues or device are
started or stopped.

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Ferruh Yigit July 18, 2018, 9:13 a.m. UTC | #1 
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?

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

>  		goto status_down;
>  	}
>
Eads, Gage July 18, 2018, 2:17 p.m. UTC | #2 
> -----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.

> 
> >
> > 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.
Ferruh Yigit July 18, 2018, 2:25 p.m. UTC | #3 
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?

> 
>>
>>>
>>> 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.
>
Eads, Gage July 18, 2018, 4:04 p.m. UTC | #4 
> -----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.

> >
> >>
> >>>
> >>> 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.
> >
Ferruh Yigit July 18, 2018, 4:06 p.m. UTC | #5 
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.
>>>
>
diff mbox series

Patch

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;
+
 		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;
 		goto status_down;
 	}
 
@@ -643,6 +649,38 @@  eth_tx_queue_setup(struct rte_eth_dev *dev,
 	return 0;
 }
 
+static int
+eth_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id)
+{
+	dev->data->rx_queue_state[rx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
+
+	return 0;
+}
+
+static int
+eth_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
+{
+	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
+
+	return 0;
+}
+
+static int
+eth_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id)
+{
+	dev->data->rx_queue_state[rx_queue_id] = RTE_ETH_QUEUE_STATE_STOPPED;
+
+	return 0;
+}
+
+static int
+eth_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id)
+{
+	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STOPPED;
+
+	return 0;
+}
+
 static const struct eth_dev_ops ops = {
 	.dev_start = eth_dev_start,
 	.dev_stop = eth_dev_stop,
@@ -651,6 +689,10 @@  static const struct eth_dev_ops ops = {
 	.dev_infos_get = eth_dev_info,
 	.rx_queue_setup = eth_rx_queue_setup,
 	.tx_queue_setup = eth_tx_queue_setup,
+	.rx_queue_start = eth_rx_queue_start,
+	.tx_queue_start = eth_tx_queue_start,
+	.rx_queue_stop = eth_rx_queue_stop,
+	.tx_queue_stop = eth_tx_queue_stop,
 	.rx_queue_release = eth_queue_release,
 	.tx_queue_release = eth_queue_release,
 	.link_update = eth_link_update,