net/tap: set queue started and stopped

  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/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)
  

> On Jul 9, 2018, at 3:20 PM, Eads, Gage <gage.eads@intel.com> wrote:
> 
> 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/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index df396bfde..c3250da63 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -641,12 +641,22 @@ tap_link_set_up(struct rte_eth_dev *dev)
> static int
> tap_dev_start(struct rte_eth_dev *dev)
> {
> -	int err;
> +	int err, i;
> 
> 	err = tap_intr_handle_set(dev, 1);
> 	if (err)
> 		return err;
> -	return tap_link_set_up(dev);
> +
> +	err = tap_link_set_up(dev);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++)
> +		dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> +	for (i = 0; i < dev->data->nb_rx_queues; i++)
> +		dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> +
> +	return err;
> }
> 
> /* This function gets called when the current port gets stopped.
> @@ -654,6 +664,13 @@ tap_dev_start(struct rte_eth_dev *dev)
> static void
> tap_dev_stop(struct rte_eth_dev *dev)
> {
> +	int i;
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++)
> +		dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
> +	for (i = 0; i < dev->data->nb_rx_queues; i++)
> +		dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
> +
> 	tap_intr_handle_set(dev, 0);
> 	tap_link_set_down(dev);
> }
> @@ -1342,6 +1359,37 @@ tap_rss_hash_update(struct rte_eth_dev *dev,
> 	return 0;
> }
> 
> +static int
> +tap_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;

We need to verify the rx_queue_id is valid before setting the state.

if (rx_queue_id < dev->data>nb_rx_queues)
	dev->data->rx_queue_state[rx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
else
	return -1;

This needs to be done for each of these routines.

> +
> +	return 0;
> +}
> +
> +static int
> +tap_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
> +tap_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
> +tap_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              = tap_dev_start,
> 	.dev_stop               = tap_dev_stop,
> @@ -1350,6 +1398,10 @@ static const struct eth_dev_ops ops = {
> 	.dev_infos_get          = tap_dev_info,
> 	.rx_queue_setup         = tap_rx_queue_setup,
> 	.tx_queue_setup         = tap_tx_queue_setup,
> +	.rx_queue_start         = tap_rx_queue_start,
> +	.tx_queue_start         = tap_tx_queue_start,
> +	.rx_queue_stop          = tap_rx_queue_stop,
> +	.tx_queue_stop          = tap_tx_queue_stop,
> 	.rx_queue_release       = tap_rx_queue_release,
> 	.tx_queue_release       = tap_tx_queue_release,
> 	.flow_ctrl_get          = tap_flow_ctrl_get,
> -- 
> 2.13.6
> 

Regards,
Keith
  

<snip>

> >
> > +static int
> > +tap_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;
> 
> We need to verify the rx_queue_id is valid before setting the state.
> 
> if (rx_queue_id < dev->data>nb_rx_queues)
> 	dev->data->rx_queue_state[rx_queue_id] =
> RTE_ETH_QUEUE_STATE_STARTED;
> else
> 	return -1;
> 
> This needs to be done for each of these routines.
> 

The ethdev layer function (rte_eth_dev_{rx, tx}_queue_{start, stop}) already does the queue ID bounds check -- do you prefer to duplicate it here?

Thanks,
Gage
  

> On Jul 9, 2018, at 4:51 PM, Eads, Gage <gage.eads@intel.com> wrote:
> 
> <snip>
> 
>>> 
>>> +static int
>>> +tap_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;
>> 
>> We need to verify the rx_queue_id is valid before setting the state.
>> 
>> if (rx_queue_id < dev->data>nb_rx_queues)
>> 	dev->data->rx_queue_state[rx_queue_id] =
>> RTE_ETH_QUEUE_STATE_STARTED;
>> else
>> 	return -1;
>> 
>> This needs to be done for each of these routines.
>> 
> 
> The ethdev layer function (rte_eth_dev_{rx, tx}_queue_{start, stop}) already does the queue ID bounds check -- do you prefer to duplicate it here?

I looked in ixgb driver and it was checking I then assumed needed it. I should check in the ethdev layer. We do not need to duplicate more checks.

Thanks for spotting that one.

> 
> Thanks,
> Gage

Regards,
Keith
  

> On Jul 9, 2018, at 3:20 PM, Eads, Gage <gage.eads@intel.com> wrote:
> 
> 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>

Acked-by Keith Wiles <keith.wiles@intel.com>

Regards,
Keith
  

> On Jul 9, 2018, at 5:00 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
> 
> 
> 
>> On Jul 9, 2018, at 4:51 PM, Eads, Gage <gage.eads@intel.com> wrote:
>> 
>> <snip>
>> 
>>>> 
>>>> +static int
>>>> +tap_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;
>>> 
>>> We need to verify the rx_queue_id is valid before setting the state.
>>> 
>>> if (rx_queue_id < dev->data>nb_rx_queues)
>>> 	dev->data->rx_queue_state[rx_queue_id] =
>>> RTE_ETH_QUEUE_STATE_STARTED;
>>> else
>>> 	return -1;
>>> 
>>> This needs to be done for each of these routines.
>>> 
>> 
>> The ethdev layer function (rte_eth_dev_{rx, tx}_queue_{start, stop}) already does the queue ID bounds check -- do you prefer to duplicate it here?
> 
> I looked in ixgb driver and it was checking I then assumed needed it. I should check in the ethdev layer. We do not need to duplicate more checks.
> 
> Thanks for spotting that one.

Looks like a number of the Intel drivers check the queue_id in the PMD :-(

> 
>> 
>> Thanks,
>> Gage
> 
> Regards,
> Keith

Regards,
Keith
  

> -----Original Message-----
> From: Wiles, Keith
> Sent: Monday, July 9, 2018 5:07 PM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/tap: set queue started and stopped
> 
> 
> 
> > On Jul 9, 2018, at 5:00 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
> >
> >
> >
> >> On Jul 9, 2018, at 4:51 PM, Eads, Gage <gage.eads@intel.com> wrote:
> >>
> >> <snip>
> >>
> >>>>
> >>>> +static int
> >>>> +tap_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;
> >>>
> >>> We need to verify the rx_queue_id is valid before setting the state.
> >>>
> >>> if (rx_queue_id < dev->data>nb_rx_queues)
> >>> 	dev->data->rx_queue_state[rx_queue_id] =
> >>> RTE_ETH_QUEUE_STATE_STARTED; else
> >>> 	return -1;
> >>>
> >>> This needs to be done for each of these routines.
> >>>
> >>
> >> The ethdev layer function (rte_eth_dev_{rx, tx}_queue_{start, stop}) already
> does the queue ID bounds check -- do you prefer to duplicate it here?
> >
> > I looked in ixgb driver and it was checking I then assumed needed it. I should
> check in the ethdev layer. We do not need to duplicate more checks.
> >
> > Thanks for spotting that one.
> 
> Looks like a number of the Intel drivers check the queue_id in the PMD :-(

Well, these queue start/stop functions shouldn't be called in the main loop, so a redundant check should be fine performance-wise. Perhaps not ok from a code maintenance perspective.

> 
> >
> >>
> >> Thanks,
> >> Gage
> >
> > Regards,
> > Keith
> 
> Regards,
> Keith
  

On 7/9/2018 11:01 PM, Wiles, Keith wrote:
> 
> 
>> On Jul 9, 2018, at 3:20 PM, Eads, Gage <gage.eads@intel.com> wrote:
>>
>> 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>
> 
> Acked-by Keith Wiles <keith.wiles@intel.com>

Applied to dpdk-next-net/master, thanks.