Re: [dpdk-dev] [PATCH v3 1/5] ethdev: add rx interrupt enable/disable functions

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Feb 19, 2015 at 07:58:38AM +0000, Zhou, Danny wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Tuesday, February 17, 2015 11:55 PM
> > To: Zhou, Danny
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 1/5] ethdev: add rx interrupt enable/disable functions
> > 
> > On Tue, Feb 17, 2015 at 09:47:15PM +0800, Zhou Danny wrote:
> > > v3 changes
> > > - Add return value for interrupt enable/disable functions
> > >
> > > Add two dev_ops functions to enable and disable rx queue interrupts
> > >
> > > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > > Tested-by: Yong Liu <yong.liu@intel.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 43 ++++++++++++++++++++++++++++++++
> > >  lib/librte_ether/rte_ethdev.h | 57 +++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 100 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > > index ea3a1fb..d27469a 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -2825,6 +2825,49 @@ _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > >  	}
> > >  	rte_spinlock_unlock(&rte_eth_dev_cb_lock);
> > >  }
> > > +
> > > +int
> > > +rte_eth_dev_rx_queue_intr_enable(uint8_t port_id,
> > > +				uint16_t queue_id)
> > > +{
> > > +	struct rte_eth_dev *dev;
> > > +
> > > +	if (port_id >= nb_ports) {
> > > +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > > +		return (-ENODEV);
> > > +	}
> > > +
> > > +	dev = &rte_eth_devices[port_id];
> > > +	if (dev == NULL) {
> > > +		PMD_DEBUG_TRACE("Invalid port device\n");
> > > +		return (-ENODEV);
> > > +	}
> > > +
> > > +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_intr_enable, -ENOTSUP);
> > > +	return (*dev->dev_ops->rx_queue_intr_enable)(dev, queue_id);
> > > +}
> > > +
> > > +int
> > > +rte_eth_dev_rx_queue_intr_disable(uint8_t port_id,
> > > +				uint16_t queue_id)
> > > +{
> > > +	struct rte_eth_dev *dev;
> > > +
> > > +	if (port_id >= nb_ports) {
> > > +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > > +		return (-ENODEV);
> > > +	}
> > > +
> > > +	dev = &rte_eth_devices[port_id];
> > > +	if (dev == NULL) {
> > > +		PMD_DEBUG_TRACE("Invalid port device\n");
> > > +		return (-ENODEV);
> > > +	}
> > > +
> > > +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_intr_disable, -ENOTSUP);
> > > +	return (*dev->dev_ops->rx_queue_intr_disable)(dev, queue_id);
> > > +}
> > > +
> > >  #ifdef RTE_NIC_BYPASS
> > >  int rte_eth_dev_bypass_init(uint8_t port_id)
> > >  {
> > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> > > index 84160c3..0f320a9 100644
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > @@ -848,6 +848,8 @@ struct rte_eth_fdir {
> > >  struct rte_intr_conf {
> > >  	/** enable/disable lsc interrupt. 0 (default) - disable, 1 enable */
> > >  	uint16_t lsc;
> > > +	/** enable/disable rxq interrupt. 0 (default) - disable, 1 enable */
> > > +	uint16_t rxq;
> > >  };
> > >
> > >  /**
> > > @@ -1109,6 +1111,14 @@ typedef int (*eth_tx_queue_setup_t)(struct rte_eth_dev *dev,
> > >  				    const struct rte_eth_txconf *tx_conf);
> > >  /**< @internal Setup a transmit queue of an Ethernet device. */
> > >
> > > +typedef int (*eth_rx_enable_intr_t)(struct rte_eth_dev *dev,
> > > +				    uint16_t rx_queue_id);
> > > +/**< @internal Enable interrupt of a receive queue of an Ethernet device. */
> > > +
> > > +typedef int (*eth_rx_disable_intr_t)(struct rte_eth_dev *dev,
> > > +				    uint16_t rx_queue_id);
> > > +/**< @internal Disable interrupt of a receive queue of an Ethernet device. */
> > > +
> > >  typedef void (*eth_queue_release_t)(void *queue);
> > >  /**< @internal Release memory resources allocated by given RX/TX queue. */
> > >
> > > @@ -1445,6 +1455,8 @@ struct eth_dev_ops {
> > >  	eth_queue_start_t          tx_queue_start;/**< Start TX for a queue.*/
> > >  	eth_queue_stop_t           tx_queue_stop;/**< Stop TX for a queue.*/
> > >  	eth_rx_queue_setup_t       rx_queue_setup;/**< Set up device RX queue.*/
> > > +	eth_rx_enable_intr_t       rx_queue_intr_enable; /**< Enable Rx queue interrupt. */
> > > +	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt.*/
> > Put these at the end of eth_dev_ops if you want to avoid breaking ABI
> 
> I purposely add those two APIs at current position to ensure all rxq related APIs are declared together
> in eth_dev_ops. Anyway, moving them to the end is ok to me for the reason of ABI, though the code looks
> a little bit ugly.
> 
Right, pretty isn't a reason to break ABI as noted in the release notes doc

Neil

> > 
> > >  	eth_queue_release_t        rx_queue_release;/**< Release RX queue.*/
> > >  	eth_rx_queue_count_t       rx_queue_count; /**< Get Rx queue count. */
> > >  	eth_rx_descriptor_done_t   rx_descriptor_done;  /**< Check rxd DD bit */
> > > @@ -2811,6 +2823,51 @@ void _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > >  				enum rte_eth_event_type event);
> > >
> > >  /**
> > > + * When there is no rx packet coming in Rx Queue for a long time, we can
> > > + * sleep lcore related to RX Queue for power saving, and enable rx interrupt
> > > + * to be triggered when rx packect arrives.
> > > + *
> > > + * The rte_eth_dev_rx_queue_intr_enable() function enables rx queue
> > > + * interrupt on specific rx queue of a port.
> > > + *
> > > + * @param port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param queue_id
> > > + *   The index of the receive queue from which to retrieve input packets.
> > > + *   The value must be in the range [0, nb_rx_queue - 1] previously supplied
> > > + *   to rte_eth_dev_configure().
> > > + * @return
> > > + *   - (0) if successful.
> > > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support
> > > + *     that operation.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + */
> > > +int rte_eth_dev_rx_queue_intr_enable(uint8_t port_id,
> > > +				uint16_t queue_id);
> > > +
> > > +/**
> > > + * When lcore wakes up from rx interrupt indicating packet coming, disable rx
> > > + * interrupt and returns to polling mode.
> > > + *
> > > + * The rte_eth_dev_rx_queue_intr_disable() function disables rx queue
> > > + * interrupt on specific rx queue of a port.
> > > + *
> > > + * @param port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param queue_id
> > > + *   The index of the receive queue from which to retrieve input packets.
> > > + *   The value must be in the range [0, nb_rx_queue - 1] previously supplied
> > > + *   to rte_eth_dev_configure().
> > > + * @return
> > > + *   - (0) if successful.
> > > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support
> > > + *     that operation.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + */
> > > +int rte_eth_dev_rx_queue_intr_disable(uint8_t port_id,
> > > +				uint16_t queue_id);
> > > +
> > > +/**
> > >   * Turn on the LED on the Ethernet device.
> > >   * This function turns on the LED on the Ethernet device.
> > >   *
> > > --
> > > 1.8.1.4
> > >
> > >
>