Re: [dpdk-dev] [PATCH] net/iavf: remove interrupt handler

["Wu, Jingjing" <jingjing.wu@intel.com>]

> > -----Original Message-----
> > From: Zhang, RobinX <robinx.zhang@intel.com>
> > Sent: Friday, July 23, 2021 3:47 PM
> > To: dev@dpdk.org
> > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Guo,
> > Junfeng <junfeng.guo@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>;
> > Zhang, RobinX <robinx.zhang@intel.com>
> > Subject: [PATCH] net/iavf: remove interrupt handler
> 
> As you are not going to remove interrupt handler for all the cases, the title is
> misleading Better replace it with "enable interrupt polling"
> 
> >
> > For VF hosted by Intel 700 series NICs, internal rx interrupt and
> > adminq interrupt share the same source, that cause a lot cpu cycles be
> > wasted on interrupt handler on rx path.
> >
> > The patch disable pci interrupt and remove the interrupt handler,
> > replace it with a low frequency(50ms) interrupt polling daemon which
> > is implemtented by registering an alarm callback periodly.
> >
> > The virtual channel capability bit VIRTCHNL_VF_OFFLOAD_WB_ON_ITR can
> > be used to negotiate if iavf PMD needs to enable background alarm or
> > not, so ideally this change will not impact the case hosted by Intel 800 series
> NICS.
> >
> > Suggested-by: Jingjing Wu <jingjing.wu@intel.com>
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> 
As it is a kind of problem solving, can it be sent to dpdk-stable too?

> No need to add me as the author for this patch but, you can add a reference
> to the original i40e commit to explain you implement the same logic.
> 
> > Signed-off-by: Robin Zhang <robinx.zhang@intel.com>
> > ---
> >  drivers/net/iavf/iavf.h        |  3 +++
> >  drivers/net/iavf/iavf_ethdev.c | 37
> > ++++++++++++++--------------------
> >  drivers/net/iavf/iavf_vchnl.c  | 11 ++++------
> >  3 files changed, 22 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h index
> > b3bd078111..771f3b79d7 100644
> > --- a/drivers/net/iavf/iavf.h
> > +++ b/drivers/net/iavf/iavf.h
> > @@ -69,6 +69,8 @@
> >  #define IAVF_QUEUE_ITR_INTERVAL_DEFAULT 32 /* 32 us */
> >  #define IAVF_QUEUE_ITR_INTERVAL_MAX     8160 /* 8160 us */
> >
> > +#define IAVF_ALARM_INTERVAL 50000 /* us */
> > +
> >  /* The overhead from MTU to max frame size.
> >   * Considering QinQ packet, the VLAN tag needs to be counted twice.
> >   */
> > @@ -372,6 +374,7 @@ int iavf_config_irq_map_lv(struct iavf_adapter
> > *adapter, uint16_t num,  void iavf_add_del_all_mac_addr(struct
> > iavf_adapter *adapter, bool add);  int iavf_dev_link_update(struct
> > rte_eth_dev *dev,  __rte_unused int wait_to_complete);
> > +void iavf_dev_alarm_handler(void *param);
> >  int iavf_query_stats(struct iavf_adapter *adapter,
> >      struct virtchnl_eth_stats **pstats);  int
> > iavf_config_promisc(struct iavf_adapter *adapter, bool enable_unicast,
> > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > b/drivers/net/iavf/iavf_ethdev.c index
> > 41382c6d66..bbe5b3ddb1 100644
> > --- a/drivers/net/iavf/iavf_ethdev.c
> > +++ b/drivers/net/iavf/iavf_ethdev.c
> > @@ -16,6 +16,7 @@
> >  #include <rte_interrupts.h>
> >  #include <rte_debug.h>
> >  #include <rte_pci.h>
> > +#include <rte_alarm.h>
> >  #include <rte_atomic.h>
> >  #include <rte_eal.h>
> >  #include <rte_ether.h>
> > @@ -692,9 +693,9 @@ static int iavf_config_rx_queues_irqs(struct
> > rte_eth_dev *dev,
> >   */
> >  vf->msix_base = IAVF_MISC_VEC_ID;
> >
> > -/* set ITR to max */
> > +/* set ITR to default */
> >  interval = iavf_calc_itr_interval(
> > -IAVF_QUEUE_ITR_INTERVAL_MAX);
> > +IAVF_QUEUE_ITR_INTERVAL_DEFAULT);
> >  IAVF_WRITE_REG(hw, IAVF_VFINT_DYN_CTL01,
> >         IAVF_VFINT_DYN_CTL01_INTENA_MASK |
> >         (IAVF_ITR_INDEX_DEFAULT <<
> > @@ -853,9 +854,8 @@ iavf_dev_start(struct rte_eth_dev *dev)
> > PMD_DRV_LOG(ERR, "configure irq failed");  goto err_queue;  }
> > -/* re-enable intr again, because efd assign may change */
> > +/* only enable interrupt in rx interrupt mode */
> >  if (dev->data->dev_conf.intr_conf.rxq != 0) {
> > -rte_intr_disable(intr_handle);  rte_intr_enable(intr_handle);  }
> >
> > @@ -889,6 +889,9 @@ iavf_dev_stop(struct rte_eth_dev *dev)
> >
> >  PMD_INIT_FUNC_TRACE();
> >
> > +if (dev->data->dev_conf.intr_conf.rxq != 0)
> > +rte_intr_disable(intr_handle);
> > +
> >  if (adapter->stopped == 1)
> >  return 0;
> >
> > @@ -1669,8 +1672,6 @@ iavf_dev_rx_queue_intr_enable(struct
> rte_eth_dev
> > *dev, uint16_t queue_id)
> >
> >  IAVF_WRITE_FLUSH(hw);
> >
> > -rte_intr_ack(&pci_dev->intr_handle);
> > -
> >  return 0;
> >  }
> >
> > @@ -2201,8 +2202,8 @@ iavf_disable_irq0(struct iavf_hw *hw)
> > IAVF_WRITE_FLUSH(hw);  }
> >
> > -static void
> > -iavf_dev_interrupt_handler(void *param)
> > +void
> > +iavf_dev_alarm_handler(void *param)
> >  {
> >  struct rte_eth_dev *dev = (struct rte_eth_dev *)param;  struct
> > iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > @@ -2212,6 +2213,9 @@ iavf_dev_interrupt_handler(void *param)
> > iavf_handle_virtchnl_msg(dev);
> >
> >  iavf_enable_irq0(hw);
> > +
> > +rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
> > +  iavf_dev_alarm_handler, dev);
> >  }
> >
> >  static int
> > @@ -2314,13 +2318,8 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
> > rte_ether_addr_copy((struct rte_ether_addr *)hw->mac.addr,
> > &eth_dev->data->mac_addrs[0]);
> >
> > -/* register callback func to eal lib */
> > -rte_intr_callback_register(&pci_dev->intr_handle,
> > -   iavf_dev_interrupt_handler,
> > -   (void *)eth_dev);
> > -
> > -/* enable uio intr after callback register */
> > -rte_intr_enable(&pci_dev->intr_handle);
> > +rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
> > +  iavf_dev_alarm_handler, eth_dev);
> >
> >  /* configure and enable device interrupt */  iavf_enable_irq0(hw); @@
> > -2341,7 +2340,6 @@ iavf_dev_close(struct rte_eth_dev *dev)  {  struct
> > iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >  struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); -struct
> > rte_intr_handle *intr_handle = &pci_dev->intr_handle;  struct
> > iavf_adapter *adapter =
> > IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> >  struct iavf_info *vf =
> > IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > @@ -2364,12 +2362,7 @@ iavf_dev_close(struct rte_eth_dev *dev)
> > iavf_config_promisc(adapter, false, false);
> >
> >  iavf_shutdown_adminq(hw);
> > -/* disable uio intr before callback unregister */
> > -rte_intr_disable(intr_handle);
> > -
> > -/* unregister callback func from eal lib */
> > -rte_intr_callback_unregister(intr_handle,
> > -     iavf_dev_interrupt_handler, dev);
> > +rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
> >  iavf_disable_irq0(hw);
> >
> >  if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_QOS) diff --git
> > a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> > 06dc663947..af19dd6412 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -13,6 +13,7 @@
> >  #include <rte_common.h>
> >
> >  #include <rte_debug.h>
> > +#include <rte_alarm.h>
> >  #include <rte_atomic.h>
> >  #include <rte_eal.h>
> >  #include <rte_ether.h>
> > @@ -1663,7 +1664,6 @@ iavf_request_queues(struct iavf_adapter
> > *adapter, uint16_t num)  {  struct rte_eth_dev *dev =
> > adapter->eth_dev;  struct iavf_info *vf =
> > IAVF_DEV_PRIVATE_TO_VF(adapter); -struct rte_pci_device *pci_dev =
> > RTE_ETH_DEV_TO_PCI(dev);  struct virtchnl_vf_res_request vfres;
> > struct iavf_cmd_info args;  uint16_t num_queue_pairs; @@ -1687,13
> > +1687,10 @@ iavf_request_queues(struct iavf_adapter *adapter, uint16_t
> > num)  args.out_buffer = vf->aq_resp;  args.out_size = IAVF_AQ_BUF_SZ;
> >
> > -/*
> > - * disable interrupt to avoid the admin queue message to be read
> > - * before iavf_read_msg_from_pf.
> > - */
> > -rte_intr_disable(&pci_dev->intr_handle);
> > +rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
> >  err = iavf_execute_vf_cmd(adapter, &args);
> > -rte_intr_enable(&pci_dev->intr_handle);
> > +rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
> > +  iavf_dev_alarm_handler, dev);
> >  if (err) {
> >  PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
> > return err;
> > --
> > 2.25.1
>