[PATCH 1/5] net/ixgbevf: unregister irq handler when other interrupts not allowed.

[PATCH 1/5] net/ixgbevf: unregister irq handler when other interrupts not allowed.

[Tonghao Zhang]

When we bind the ixgbe VF (e.g 82599 card) to igb_uio and enable the
rx-interrupt, there will be more than one epoll_wait on intr_handle.fd.
One is in "eal-intr-thread" thread, and the others are in the thread
which call the "rte_epoll_wait". The problem is that sometiems
"eal-intr-thread" thread will process the rx interrupt, and then
rte_epoll_wait can't get the event any more, and the packets may be lost.

We should unregister the status interrupt handler in "eal-intr-thread"
thread and the ixgbe pf is in the same case.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 43e0132..e67389f 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -5049,6 +5049,15 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	}
 	ixgbevf_configure_msix(dev);
 
+	if (!rte_intr_allow_others(intr_handle)) {
+		rte_intr_callback_unregister(intr_handle,
+					     ixgbevf_dev_interrupt_handler,
+					     dev);
+		if (dev->data->dev_conf.intr_conf.lsc != 0)
+			PMD_INIT_LOG(INFO, "lsc won't enable because of"
+				     " no intr multiplex");
+	}
+
 	/* When a VF port is bound to VFIO-PCI, only miscellaneous interrupt
 	 * is mapped to VFIO vector 0 in eth_ixgbevf_dev_init( ).
 	 * If previous VFIO interrupt mapping setting in eth_ixgbevf_dev_init( )
@@ -5091,6 +5100,12 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 
 	ixgbe_dev_clear_queues(dev);
 
+	if (!rte_intr_allow_others(intr_handle))
+		/* resume to the default handler */
+		rte_intr_callback_register(intr_handle,
+					   ixgbevf_dev_interrupt_handler,
+					   (void *)dev);
+
 	/* Clean datapath event and queue/vec mapping */
 	rte_intr_efd_disable(intr_handle);
 	if (intr_handle->intr_vec != NULL) {
-- 
1.8.3.1

[PATCH 2/5] net/ixgbevf: set the inter-interrupt interval for EITR.

[Tonghao Zhang]

Set EITR interval as default. This patch can improve the performance
when we enable the rx-intrrupt to process the packets because we hope
rx-intrrupt reduce CPU.

The 200us value of EITR makes the performance better with the low CPU.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index e67389f..b20cab9 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -5780,6 +5780,9 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 		if (vector_idx < base + intr_handle->nb_efd - 1)
 			vector_idx++;
 	}
+
+	IXGBE_WRITE_REG(hw, IXGBE_VTEITR(IXGBE_MISC_VEC_ID),
+			IXGBE_EITR_INTERVAL_US(200) | IXGBE_EITR_CNT_WDIS);
 }
 
 /**
-- 
1.8.3.1

[PATCH 3/5] net/ixgbevf: save IXGBE_VTEIMS to intr->mask for performance.

[Tonghao Zhang]

If dpdk APPs call the rte_eth_dev_rx_intr_enable or
rte_eth_dev_rx_intr_disable frequently, and ixgbe vf will read
the IXGBE_VTEIMS register everytime. We can optimize the driver
function. The patch save the IXGBE_VTEIMS to mask to avoid read
frequently.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 55 +++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index b20cab9..e929235 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -241,8 +241,8 @@ static int ixgbevf_dev_link_update(struct rte_eth_dev *dev,
 static void ixgbevf_dev_stop(struct rte_eth_dev *dev);
 static void ixgbevf_dev_close(struct rte_eth_dev *dev);
 static int  ixgbevf_dev_reset(struct rte_eth_dev *dev);
-static void ixgbevf_intr_disable(struct ixgbe_hw *hw);
-static void ixgbevf_intr_enable(struct ixgbe_hw *hw);
+static void ixgbevf_intr_disable(struct rte_eth_dev *dev);
+static void ixgbevf_intr_enable(struct rte_eth_dev *dev);
 static int ixgbevf_dev_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *stats);
 static void ixgbevf_dev_stats_reset(struct rte_eth_dev *dev);
@@ -1636,7 +1636,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev *eth_dev)
 	ixgbevf_dev_stats_reset(eth_dev);
 
 	/* Disable the interrupts for VF */
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(eth_dev);
 
 	hw->mac.num_rar_entries = 128; /* The MAX of the underlying PF */
 	diag = hw->mac.ops.reset_hw(hw);
@@ -1705,7 +1705,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev *eth_dev)
 	rte_intr_callback_register(intr_handle,
 				   ixgbevf_dev_interrupt_handler, eth_dev);
 	rte_intr_enable(intr_handle);
-	ixgbevf_intr_enable(hw);
+	ixgbevf_intr_enable(eth_dev);
 
 	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x mac.type=%s",
 		     eth_dev->data->port_id, pci_dev->id.vendor_id,
@@ -1738,7 +1738,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = NULL;
 
 	/* Disable the interrupts for VF */
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(eth_dev);
 
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
@@ -4924,19 +4924,32 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
  * Virtual Function operations
  */
 static void
-ixgbevf_intr_disable(struct ixgbe_hw *hw)
+ixgbevf_intr_disable(struct rte_eth_dev *dev)
 {
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
+	struct ixgbe_hw *hw =
+		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
 	PMD_INIT_FUNC_TRACE();
 
 	/* Clear interrupt mask to stop from interrupts being generated */
 	IXGBE_WRITE_REG(hw, IXGBE_VTEIMC, IXGBE_VF_IRQ_CLEAR_MASK);
 
 	IXGBE_WRITE_FLUSH(hw);
+
+	/* Clear mask value. */
+	intr->mask = 0;
 }
 
 static void
-ixgbevf_intr_enable(struct ixgbe_hw *hw)
+ixgbevf_intr_enable(struct rte_eth_dev *dev)
 {
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
+	struct ixgbe_hw *hw =
+		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
 	PMD_INIT_FUNC_TRACE();
 
 	/* VF enable interrupt autoclean */
@@ -4945,6 +4958,9 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, IXGBE_VF_IRQ_ENABLE_MASK);
 
 	IXGBE_WRITE_FLUSH(hw);
+
+	/* Save IXGBE_VTEIMS value to mask. */
+	intr->mask = IXGBE_VF_IRQ_ENABLE_MASK;
 }
 
 static int
@@ -5070,7 +5086,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	rte_intr_enable(intr_handle);
 
 	/* Re-enable interrupt for VF */
-	ixgbevf_intr_enable(hw);
+	ixgbevf_intr_enable(dev);
 
 	return 0;
 }
@@ -5084,7 +5100,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(dev);
 
 	hw->adapter_stopped = 1;
 	ixgbe_stop_adapter(hw);
@@ -5582,17 +5598,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
-	uint32_t mask;
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	struct ixgbe_hw *hw =
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	uint32_t vec = IXGBE_MISC_VEC_ID;
 
-	mask = IXGBE_READ_REG(hw, IXGBE_VTEIMS);
 	if (rte_intr_allow_others(intr_handle))
 		vec = IXGBE_RX_VEC_START;
-	mask |= (1 << vec);
+	intr->mask |= (1 << vec);
 	RTE_SET_USED(queue_id);
-	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, mask);
+	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
 
 	rte_intr_enable(intr_handle);
 
@@ -5602,19 +5618,19 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 static int
 ixgbevf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id)
 {
-	uint32_t mask;
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	struct ixgbe_hw *hw =
 		IXGBE_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;
 	uint32_t vec = IXGBE_MISC_VEC_ID;
 
-	mask = IXGBE_READ_REG(hw, IXGBE_VTEIMS);
 	if (rte_intr_allow_others(intr_handle))
 		vec = IXGBE_RX_VEC_START;
-	mask &= ~(1 << vec);
+	intr->mask &= ~(1 << vec);
 	RTE_SET_USED(queue_id);
-	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, mask);
+	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
 
 	return 0;
 }
@@ -8170,7 +8186,7 @@ static void ixgbevf_mbx_process(struct rte_eth_dev *dev)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(dev);
 
 	/* read-on-clear nic registers here */
 	eicr = IXGBE_READ_REG(hw, IXGBE_VTEICR);
@@ -8187,7 +8203,6 @@ static void ixgbevf_mbx_process(struct rte_eth_dev *dev)
 static int
 ixgbevf_dev_interrupt_action(struct rte_eth_dev *dev)
 {
-	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 
@@ -8196,7 +8211,7 @@ static void ixgbevf_mbx_process(struct rte_eth_dev *dev)
 		intr->flags &= ~IXGBE_FLAG_MAILBOX;
 	}
 
-	ixgbevf_intr_enable(hw);
+	ixgbevf_intr_enable(dev);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.

[Tonghao Zhang]

When we bind the ixgbevf to vfio and call the rte_eth_dev_rx_intr_enable
and rte_eth_dev_rx_intr_disable frequently, the interrupt setting
(msi_set_mask_bit) will take more CPU as show below. rte_intr_enable
call the ioctl to map the fd to interrupts frequently.

perf top:
5.45%  [kernel]   [k] msi_set_mask_bit

It is unnecessary to call the rte_intr_enable in
ixgbe_dev_rx_queue_intr_enable. because the fds has been mapped to
interrupt and not unmapped in ixgbe_dev_rx_queue_intr_disable.

This patch add checks for using VFIO.  With the patch, msi_set_mask_bit is
not listed in perl any more. Any suggestion will be welcome.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index e929235..79e4097 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -5610,7 +5610,9 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_SET_USED(queue_id);
 	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
 
-	rte_intr_enable(intr_handle);
+	if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
+	    intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
+		rte_intr_enable(intr_handle);
 
 	return 0;
 }
@@ -5659,7 +5661,10 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 		mask &= (1 << (queue_id - 32));
 		IXGBE_WRITE_REG(hw, IXGBE_EIMS_EX(1), mask);
 	}
-	rte_intr_enable(intr_handle);
+
+	if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
+	    intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
+		rte_intr_enable(intr_handle);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 5/5] vhost: add reconnect thread name for client mode.

[Tonghao Zhang]

This patch adds the name for vhost-user reconnect thread.
It can help us to know whether the thread is running.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/librte_vhost/socket.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index d44a0f1..c2e34e0 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -433,6 +433,7 @@ struct vhost_user_reconnect_list {
 vhost_user_reconnect_init(void)
 {
 	int ret;
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	ret = pthread_mutex_init(&reconn_list.mutex, NULL);
 	if (ret < 0) {
@@ -449,6 +450,13 @@ struct vhost_user_reconnect_list {
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"failed to destroy reconnect mutex");
 		}
+	} else {
+		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
+			 "vhost-reconn");
+
+		if (rte_thread_setname(reconn_tid, thread_name))
+			RTE_LOG(DEBUG, VHOST_CONFIG,
+			"Failed to set thread name for vhost-user reconnect");
 	}
 
 	return ret;
-- 
1.8.3.1

Re: [PATCH 5/5] vhost: add reconnect thread name for client mode.

[Yuanhan Liu]

On Fri, Jan 05, 2018 at 06:10:39AM -0800, Tonghao Zhang wrote:
> This patch adds the name for vhost-user reconnect thread.
> It can help us to know whether the thread is running.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  lib/librte_vhost/socket.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index d44a0f1..c2e34e0 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -433,6 +433,7 @@ struct vhost_user_reconnect_list {
>  vhost_user_reconnect_init(void)
>  {
>  	int ret;
> +	char thread_name[RTE_MAX_THREAD_NAME_LEN];
>  
>  	ret = pthread_mutex_init(&reconn_list.mutex, NULL);
>  	if (ret < 0) {
> @@ -449,6 +450,13 @@ struct vhost_user_reconnect_list {
>  			RTE_LOG(ERR, VHOST_CONFIG,
>  				"failed to destroy reconnect mutex");
>  		}
> +	} else {
> +		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> +			 "vhost-reconn");
> +
> +		if (rte_thread_setname(reconn_tid, thread_name))
> +			RTE_LOG(DEBUG, VHOST_CONFIG,
> +			"Failed to set thread name for vhost-user reconnect");

Applied to dpdk-next-virtio, with the intendation fixed.

I have also shortten the log a bit "failed to set reconnect thread name".
There is no need to specify "vhost-user" again since VHOST_CONFIG tells
you it comes from vhost-user.

Thanks.

	--yliu

Re: [PATCH 5/5] vhost: add reconnect thread name for client mode.

[Yuanhan Liu]

On Tue, Jan 09, 2018 at 09:12:43PM +0800, Yuanhan Liu wrote:
> On Fri, Jan 05, 2018 at 06:10:39AM -0800, Tonghao Zhang wrote:
> > This patch adds the name for vhost-user reconnect thread.
> > It can help us to know whether the thread is running.
> > 
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  lib/librte_vhost/socket.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> > index d44a0f1..c2e34e0 100644
> > --- a/lib/librte_vhost/socket.c
> > +++ b/lib/librte_vhost/socket.c
> > @@ -433,6 +433,7 @@ struct vhost_user_reconnect_list {
> >  vhost_user_reconnect_init(void)
> >  {
> >  	int ret;
> > +	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> >  
> >  	ret = pthread_mutex_init(&reconn_list.mutex, NULL);
> >  	if (ret < 0) {
> > @@ -449,6 +450,13 @@ struct vhost_user_reconnect_list {
> >  			RTE_LOG(ERR, VHOST_CONFIG,
> >  				"failed to destroy reconnect mutex");
> >  		}
> > +	} else {
> > +		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> > +			 "vhost-reconn");
> > +
> > +		if (rte_thread_setname(reconn_tid, thread_name))
> > +			RTE_LOG(DEBUG, VHOST_CONFIG,
> > +			"Failed to set thread name for vhost-user reconnect");
> 
> Applied to dpdk-next-virtio, with the intendation fixed.

Note that I have just applied this patch only. The rest of this patchset
belongs to the ixgbe PMD driver, which need another maintainer for review.

That also means, it's better if you could send them sperately next time
when they are not related. More specifically, in this case, one patch
for vhost-user, another patchset for all the rest ixgbe changes.

	--yliu

Re: [PATCH 5/5] vhost: add reconnect thread name for client mode.

[Tonghao Zhang]

Thanks for your review.

On Tue, Jan 9, 2018 at 9:40 PM, Yuanhan Liu <yliu@fridaylinux.org> wrote:
> On Tue, Jan 09, 2018 at 09:12:43PM +0800, Yuanhan Liu wrote:
>> On Fri, Jan 05, 2018 at 06:10:39AM -0800, Tonghao Zhang wrote:
>> > This patch adds the name for vhost-user reconnect thread.
>> > It can help us to know whether the thread is running.
>> >
>> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> > ---
>> >  lib/librte_vhost/socket.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
>> > index d44a0f1..c2e34e0 100644
>> > --- a/lib/librte_vhost/socket.c
>> > +++ b/lib/librte_vhost/socket.c
>> > @@ -433,6 +433,7 @@ struct vhost_user_reconnect_list {
>> >  vhost_user_reconnect_init(void)
>> >  {
>> >     int ret;
>> > +   char thread_name[RTE_MAX_THREAD_NAME_LEN];
>> >
>> >     ret = pthread_mutex_init(&reconn_list.mutex, NULL);
>> >     if (ret < 0) {
>> > @@ -449,6 +450,13 @@ struct vhost_user_reconnect_list {
>> >                     RTE_LOG(ERR, VHOST_CONFIG,
>> >                             "failed to destroy reconnect mutex");
>> >             }
>> > +   } else {
>> > +           snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
>> > +                    "vhost-reconn");
>> > +
>> > +           if (rte_thread_setname(reconn_tid, thread_name))
>> > +                   RTE_LOG(DEBUG, VHOST_CONFIG,
>> > +                   "Failed to set thread name for vhost-user reconnect");
>>
>> Applied to dpdk-next-virtio, with the intendation fixed.
>
> Note that I have just applied this patch only. The rest of this patchset
> belongs to the ixgbe PMD driver, which need another maintainer for review.
>
> That also means, it's better if you could send them sperately next time
> when they are not related. More specifically, in this case, one patch
> for vhost-user, another patchset for all the rest ixgbe changes.
>
>         --yliu

Re: [PATCH 1/5] net/ixgbevf: unregister irq handler when other interrupts not allowed.

[Xing, Beilei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
> Sent: Friday, January 5, 2018 10:11 PM
> To: dev@dpdk.org
> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Subject: [dpdk-dev] [PATCH 1/5] net/ixgbevf: unregister irq handler when
> other interrupts not allowed.
> 
> When we bind the ixgbe VF (e.g 82599 card) to igb_uio and enable the rx-
> interrupt, there will be more than one epoll_wait on intr_handle.fd.
> One is in "eal-intr-thread" thread, and the others are in the thread which call
> the "rte_epoll_wait". The problem is that sometiems "eal-intr-thread"
> thread will process the rx interrupt, and then rte_epoll_wait can't get the
> event any more, and the packets may be lost.
> 
> We should unregister the status interrupt handler in "eal-intr-thread"
> thread and the ixgbe pf is in the same case.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

The patch looks OK for me, the commit log can be reworded, for example, "we" can be removed.
Apart from that:
Acked-by: Beilei Xing <beilei.xing@intel.com>
 
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 43e0132..e67389f 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -5049,6 +5049,15 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	}
>  	ixgbevf_configure_msix(dev);
> 
> +	if (!rte_intr_allow_others(intr_handle)) {
> +		rte_intr_callback_unregister(intr_handle,
> +					     ixgbevf_dev_interrupt_handler,
> +					     dev);
> +		if (dev->data->dev_conf.intr_conf.lsc != 0)
> +			PMD_INIT_LOG(INFO, "lsc won't enable because of"
> +				     " no intr multiplex");
> +	}
> +
>  	/* When a VF port is bound to VFIO-PCI, only miscellaneous interrupt
>  	 * is mapped to VFIO vector 0 in eth_ixgbevf_dev_init( ).
>  	 * If previous VFIO interrupt mapping setting in
> eth_ixgbevf_dev_init( ) @@ -5091,6 +5100,12 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> 
>  	ixgbe_dev_clear_queues(dev);
> 
> +	if (!rte_intr_allow_others(intr_handle))
> +		/* resume to the default handler */
> +		rte_intr_callback_register(intr_handle,
> +					   ixgbevf_dev_interrupt_handler,
> +					   (void *)dev);
> +
>  	/* Clean datapath event and queue/vec mapping */
>  	rte_intr_efd_disable(intr_handle);
>  	if (intr_handle->intr_vec != NULL) {
> --
> 1.8.3.1

Re: [PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.

[Xing, Beilei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
> Sent: Friday, January 5, 2018 10:11 PM
> To: dev@dpdk.org
> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Subject: [dpdk-dev] [PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.

The patch is not only for ixgbevf, but also for ixgbe, right?
so how about changing the title with net/ixgbe started?

> When we bind the ixgbevf to vfio and call the rte_eth_dev_rx_intr_enable
> and rte_eth_dev_rx_intr_disable frequently, the interrupt setting
> (msi_set_mask_bit) will take more CPU as show below. rte_intr_enable call
> the ioctl to map the fd to interrupts frequently.
> 
> perf top:
> 5.45%  [kernel]   [k] msi_set_mask_bit
> 
> It is unnecessary to call the rte_intr_enable in
> ixgbe_dev_rx_queue_intr_enable. because the fds has been mapped to
> interrupt and not unmapped in ixgbe_dev_rx_queue_intr_disable.
> 
> This patch add checks for using VFIO.  With the patch, msi_set_mask_bit is
> not listed in perl any more. Any suggestion will be welcome.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index e929235..79e4097 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -5610,7 +5610,9 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev
> *dev, bool on)
>  	RTE_SET_USED(queue_id);
>  	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
> 
> -	rte_intr_enable(intr_handle);
> +	if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
> +	    intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
> +		rte_intr_enable(intr_handle);

For igb_uio, did you check if it's necessary to call rte_intr_enable every time? Since rte interrupt is not disabled during ixgbevf_dev_rx_queue_intr_disable.

> 
>  	return 0;
>  }
> @@ -5659,7 +5661,10 @@ static void ixgbevf_set_vfta_all(struct
> rte_eth_dev *dev, bool on)
>  		mask &= (1 << (queue_id - 32));
>  		IXGBE_WRITE_REG(hw, IXGBE_EIMS_EX(1), mask);
>  	}
> -	rte_intr_enable(intr_handle);
> +
> +	if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
> +	    intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
> +		rte_intr_enable(intr_handle);

The same comment as above.

> 
>  	return 0;
>  }
> --
> 1.8.3.1

Re: [PATCH 3/5] net/ixgbevf: save IXGBE_VTEIMS to intr->mask for performance.

[Xing, Beilei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
> Sent: Friday, January 5, 2018 10:11 PM
> To: dev@dpdk.org
> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Subject: [dpdk-dev] [PATCH 3/5] net/ixgbevf: save IXGBE_VTEIMS to intr-
> >mask for performance.
> 
> If dpdk APPs call the rte_eth_dev_rx_intr_enable or
> rte_eth_dev_rx_intr_disable frequently, and ixgbe vf will read the
> IXGBE_VTEIMS register everytime. We can optimize the driver function. The
> patch save the IXGBE_VTEIMS to mask to avoid read frequently.

"We can optimize the driver function" can be removed.

> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Acked-by: Beilei Xing <beilei.xing@intel.com>

Re: [PATCH 2/5] net/ixgbevf: set the inter-interrupt interval for EITR.

[Xing, Beilei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
> Sent: Friday, January 5, 2018 10:11 PM
> To: dev@dpdk.org
> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Subject: [dpdk-dev] [PATCH 2/5] net/ixgbevf: set the inter-interrupt interval
> for EITR.
> 
> Set EITR interval as default. This patch can improve the performance when
> we enable the rx-intrrupt to process the packets because we hope rx-
> intrrupt reduce CPU.
> 
> The 200us value of EITR makes the performance better with the low CPU.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index e67389f..b20cab9 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -5780,6 +5780,9 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev
> *dev, bool on)
>  		if (vector_idx < base + intr_handle->nb_efd - 1)
>  			vector_idx++;
>  	}
> +
> +	IXGBE_WRITE_REG(hw, IXGBE_VTEITR(IXGBE_MISC_VEC_ID),
> +			IXGBE_EITR_INTERVAL_US(200) |

Is it possible to configure interval via DPDK configuration? Just like CONFIG_RTE_LIBRTE_I40E_ITR_INTERVAL for i40e.
Maybe the value is good for you but not suitable for other users.

Besides, seems the patches in the patchset have no dependency, needn't put them into one patchset, and then the Acked patch can be applied quickly.

> IXGBE_EITR_CNT_WDIS);
>  }
> 
>  /**
> --
> 1.8.3.1

Re: [PATCH 1/5] net/ixgbevf: unregister irq handler when other interrupts not allowed.

[Tonghao Zhang]

On Thu, Jan 11, 2018 at 2:55 PM, Xing, Beilei <beilei.xing@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
>> Sent: Friday, January 5, 2018 10:11 PM
>> To: dev@dpdk.org
>> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> Subject: [dpdk-dev] [PATCH 1/5] net/ixgbevf: unregister irq handler when
>> other interrupts not allowed.
>>
>> When we bind the ixgbe VF (e.g 82599 card) to igb_uio and enable the rx-
>> interrupt, there will be more than one epoll_wait on intr_handle.fd.
>> One is in "eal-intr-thread" thread, and the others are in the thread which call
>> the "rte_epoll_wait". The problem is that sometiems "eal-intr-thread"
>> thread will process the rx interrupt, and then rte_epoll_wait can't get the
>> event any more, and the packets may be lost.
>>
>> We should unregister the status interrupt handler in "eal-intr-thread"
>> thread and the ixgbe pf is in the same case.
>>
>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> The patch looks OK for me, the commit log can be reworded, for example, "we" can be removed.
Yes

> Apart from that:
> Acked-by: Beilei Xing <beilei.xing@intel.com>
Thanks for your reviews. I will repost v2
>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 43e0132..e67389f 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -5049,6 +5049,15 @@ static int
>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>       }
>>       ixgbevf_configure_msix(dev);
>>
>> +     if (!rte_intr_allow_others(intr_handle)) {
>> +             rte_intr_callback_unregister(intr_handle,
>> +                                          ixgbevf_dev_interrupt_handler,
>> +                                          dev);
>> +             if (dev->data->dev_conf.intr_conf.lsc != 0)
>> +                     PMD_INIT_LOG(INFO, "lsc won't enable because of"
>> +                                  " no intr multiplex");
>> +     }
>> +
>>       /* When a VF port is bound to VFIO-PCI, only miscellaneous interrupt
>>        * is mapped to VFIO vector 0 in eth_ixgbevf_dev_init( ).
>>        * If previous VFIO interrupt mapping setting in
>> eth_ixgbevf_dev_init( ) @@ -5091,6 +5100,12 @@ static int
>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>
>>       ixgbe_dev_clear_queues(dev);
>>
>> +     if (!rte_intr_allow_others(intr_handle))
>> +             /* resume to the default handler */
>> +             rte_intr_callback_register(intr_handle,
>> +                                        ixgbevf_dev_interrupt_handler,
>> +                                        (void *)dev);
>> +
>>       /* Clean datapath event and queue/vec mapping */
>>       rte_intr_efd_disable(intr_handle);
>>       if (intr_handle->intr_vec != NULL) {
>> --
>> 1.8.3.1
>

Re: [PATCH 2/5] net/ixgbevf: set the inter-interrupt interval for EITR.

[Tonghao Zhang]

On Thu, Jan 11, 2018 at 3:26 PM, Xing, Beilei <beilei.xing@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
>> Sent: Friday, January 5, 2018 10:11 PM
>> To: dev@dpdk.org
>> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> Subject: [dpdk-dev] [PATCH 2/5] net/ixgbevf: set the inter-interrupt interval
>> for EITR.
>>
>> Set EITR interval as default. This patch can improve the performance when
>> we enable the rx-intrrupt to process the packets because we hope rx-
>> intrrupt reduce CPU.
>>
>> The 200us value of EITR makes the performance better with the low CPU.
>>
>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index e67389f..b20cab9 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -5780,6 +5780,9 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev
>> *dev, bool on)
>>               if (vector_idx < base + intr_handle->nb_efd - 1)
>>                       vector_idx++;
>>       }
>> +
>> +     IXGBE_WRITE_REG(hw, IXGBE_VTEITR(IXGBE_MISC_VEC_ID),
>> +                     IXGBE_EITR_INTERVAL_US(200) |
>
> Is it possible to configure interval via DPDK configuration? Just like CONFIG_RTE_LIBRTE_I40E_ITR_INTERVAL for i40e.
> Maybe the value is good for you but not suitable for other users.

Thanks  for you tips.
>
> Besides, seems the patches in the patchset have no dependency, needn't put them into one patchset, and then the Acked patch can be applied quickly.
>
>> IXGBE_EITR_CNT_WDIS);
>>  }
>>
>>  /**
>> --
>> 1.8.3.1
>

Re: [PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.

[Tonghao Zhang]

On Thu, Jan 11, 2018 at 3:06 PM, Xing, Beilei <beilei.xing@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
>> Sent: Friday, January 5, 2018 10:11 PM
>> To: dev@dpdk.org
>> Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> Subject: [dpdk-dev] [PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.
>
> The patch is not only for ixgbevf, but also for ixgbe, right?
> so how about changing the title with net/ixgbe started?
>
OK

>> When we bind the ixgbevf to vfio and call the rte_eth_dev_rx_intr_enable
>> and rte_eth_dev_rx_intr_disable frequently, the interrupt setting
>> (msi_set_mask_bit) will take more CPU as show below. rte_intr_enable call
>> the ioctl to map the fd to interrupts frequently.
>>
>> perf top:
>> 5.45%  [kernel]   [k] msi_set_mask_bit
>>
>> It is unnecessary to call the rte_intr_enable in
>> ixgbe_dev_rx_queue_intr_enable. because the fds has been mapped to
>> interrupt and not unmapped in ixgbe_dev_rx_queue_intr_disable.
>>
>> This patch add checks for using VFIO.  With the patch, msi_set_mask_bit is
>> not listed in perl any more. Any suggestion will be welcome.
>>
>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index e929235..79e4097 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -5610,7 +5610,9 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev
>> *dev, bool on)
>>       RTE_SET_USED(queue_id);
>>       IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
>>
>> -     rte_intr_enable(intr_handle);
>> +     if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
>> +         intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
>> +             rte_intr_enable(intr_handle);
>
> For igb_uio, did you check if it's necessary to call rte_intr_enable every time? Since rte interrupt is not disabled during ixgbevf_dev_rx_queue_intr_disable.

good, remove the rte_intr_enable directly, because intr has been
disable/enable via writing the register in
ixgbevf_dev_rx_queue_intr_disable/enable.
>>
>>       return 0;
>>  }
>> @@ -5659,7 +5661,10 @@ static void ixgbevf_set_vfta_all(struct
>> rte_eth_dev *dev, bool on)
>>               mask &= (1 << (queue_id - 32));
>>               IXGBE_WRITE_REG(hw, IXGBE_EIMS_EX(1), mask);
>>       }
>> -     rte_intr_enable(intr_handle);
>> +
>> +     if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
>> +         intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
>> +             rte_intr_enable(intr_handle);
>
> The same comment as above.
>
>>
>>       return 0;
>>  }
>> --
>> 1.8.3.1
>

Re: [PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.

[Dai, Wei]

Hi, Tonghao
Thanks for your patch.
It looks that same change can be applied to ixgbe_dev_interrupt_action( )
and ixgbe_dev_interrupt_delayed_handler( ).
Anyway, you can test all these changes with example/l3fwd-power. 
-Wei

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> Sent: Thursday, January 11, 2018 3:06 PM
> To: Tonghao Zhang <xiangxia.m.yue@gmail.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/5] net/ixgbevf: add check for
> rte_intr_enable.
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
> > Sent: Friday, January 5, 2018 10:11 PM
> > To: dev@dpdk.org
> > Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > Subject: [dpdk-dev] [PATCH 4/5] net/ixgbevf: add check for
> rte_intr_enable.
> 
> The patch is not only for ixgbevf, but also for ixgbe, right?
> so how about changing the title with net/ixgbe started?
> 
> > When we bind the ixgbevf to vfio and call the
> > rte_eth_dev_rx_intr_enable and rte_eth_dev_rx_intr_disable frequently,
> > the interrupt setting
> > (msi_set_mask_bit) will take more CPU as show below. rte_intr_enable
> > call the ioctl to map the fd to interrupts frequently.
> >
> > perf top:
> > 5.45%  [kernel]   [k] msi_set_mask_bit
> >
> > It is unnecessary to call the rte_intr_enable in
> > ixgbe_dev_rx_queue_intr_enable. because the fds has been mapped to
> > interrupt and not unmapped in ixgbe_dev_rx_queue_intr_disable.
> >
> > This patch add checks for using VFIO.  With the patch,
> > msi_set_mask_bit is not listed in perl any more. Any suggestion will be
> welcome.
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index e929235..79e4097 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -5610,7 +5610,9 @@ static void ixgbevf_set_vfta_all(struct
> > rte_eth_dev *dev, bool on)
> >  	RTE_SET_USED(queue_id);
> >  	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
> >
> > -	rte_intr_enable(intr_handle);
> > +	if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
> > +	    intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
> > +		rte_intr_enable(intr_handle);
> 
> For igb_uio, did you check if it's necessary to call rte_intr_enable every time?
> Since rte interrupt is not disabled during ixgbevf_dev_rx_queue_intr_disable.
> 
> >
> >  	return 0;
> >  }
> > @@ -5659,7 +5661,10 @@ static void ixgbevf_set_vfta_all(struct
> > rte_eth_dev *dev, bool on)
> >  		mask &= (1 << (queue_id - 32));
> >  		IXGBE_WRITE_REG(hw, IXGBE_EIMS_EX(1), mask);
> >  	}
> > -	rte_intr_enable(intr_handle);
> > +
> > +	if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
> > +	    intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
> > +		rte_intr_enable(intr_handle);
> 
> The same comment as above.
> 
> >
> >  	return 0;
> >  }
> > --
> > 1.8.3.1

Re: [PATCH 4/5] net/ixgbevf: add check for rte_intr_enable.

[Tonghao Zhang]

On Fri, Jan 12, 2018 at 8:10 PM, Dai, Wei <wei.dai@intel.com> wrote:
> Hi, Tonghao
> Thanks for your patch.
> It looks that same change can be applied to ixgbe_dev_interrupt_action( )
> and ixgbe_dev_interrupt_delayed_handler( ).
Yes, but the irq (e.g mailbox irq) is not  same as rx interrupt
handled frequently.
and it will not affect the performance.

 > Anyway, you can test all these changes with example/l3fwd-power.
> -Wei
I tested them with  example/l3fwd-power and our apps. It is ok.
I will send v3 to you.

>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
>> Sent: Thursday, January 11, 2018 3:06 PM
>> To: Tonghao Zhang <xiangxia.m.yue@gmail.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 4/5] net/ixgbevf: add check for
>> rte_intr_enable.
>>
>>
>>
>> > -----Original Message-----
>> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tonghao Zhang
>> > Sent: Friday, January 5, 2018 10:11 PM
>> > To: dev@dpdk.org
>> > Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> > Subject: [dpdk-dev] [PATCH 4/5] net/ixgbevf: add check for
>> rte_intr_enable.
>>
>> The patch is not only for ixgbevf, but also for ixgbe, right?
>> so how about changing the title with net/ixgbe started?
>>
>> > When we bind the ixgbevf to vfio and call the
>> > rte_eth_dev_rx_intr_enable and rte_eth_dev_rx_intr_disable frequently,
>> > the interrupt setting
>> > (msi_set_mask_bit) will take more CPU as show below. rte_intr_enable
>> > call the ioctl to map the fd to interrupts frequently.
>> >
>> > perf top:
>> > 5.45%  [kernel]   [k] msi_set_mask_bit
>> >
>> > It is unnecessary to call the rte_intr_enable in
>> > ixgbe_dev_rx_queue_intr_enable. because the fds has been mapped to
>> > interrupt and not unmapped in ixgbe_dev_rx_queue_intr_disable.
>> >
>> > This patch add checks for using VFIO.  With the patch,
>> > msi_set_mask_bit is not listed in perl any more. Any suggestion will be
>> welcome.
>> >
>> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> > ---
>> >  drivers/net/ixgbe/ixgbe_ethdev.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> > b/drivers/net/ixgbe/ixgbe_ethdev.c
>> > index e929235..79e4097 100644
>> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> > @@ -5610,7 +5610,9 @@ static void ixgbevf_set_vfta_all(struct
>> > rte_eth_dev *dev, bool on)
>> >     RTE_SET_USED(queue_id);
>> >     IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
>> >
>> > -   rte_intr_enable(intr_handle);
>> > +   if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
>> > +       intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
>> > +           rte_intr_enable(intr_handle);
>>
>> For igb_uio, did you check if it's necessary to call rte_intr_enable every time?
>> Since rte interrupt is not disabled during ixgbevf_dev_rx_queue_intr_disable.
>>
>> >
>> >     return 0;
>> >  }
>> > @@ -5659,7 +5661,10 @@ static void ixgbevf_set_vfta_all(struct
>> > rte_eth_dev *dev, bool on)
>> >             mask &= (1 << (queue_id - 32));
>> >             IXGBE_WRITE_REG(hw, IXGBE_EIMS_EX(1), mask);
>> >     }
>> > -   rte_intr_enable(intr_handle);
>> > +
>> > +   if (intr_handle->type == RTE_INTR_HANDLE_UIO ||
>> > +       intr_handle->type == RTE_INTR_HANDLE_UIO_INTX)
>> > +           rte_intr_enable(intr_handle);
>>
>> The same comment as above.
>>
>> >
>> >     return 0;
>> >  }
>> > --
>> > 1.8.3.1
>

[PATCH 3/5] net/ixgbevf: save IXGBE_VTEIMS to intr->mask for performance.

[Tonghao Zhang]

If dpdk APPs call the rte_eth_dev_rx_intr_enable or
rte_eth_dev_rx_intr_disable frequently, and ixgbe vf will read
the IXGBE_VTEIMS register everytime. We can optimize the driver
function. The patch save the IXGBE_VTEIMS to mask to avoid read
frequently.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 55 +++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index b20cab9..e929235 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -241,8 +241,8 @@ static int ixgbevf_dev_link_update(struct rte_eth_dev *dev,
 static void ixgbevf_dev_stop(struct rte_eth_dev *dev);
 static void ixgbevf_dev_close(struct rte_eth_dev *dev);
 static int  ixgbevf_dev_reset(struct rte_eth_dev *dev);
-static void ixgbevf_intr_disable(struct ixgbe_hw *hw);
-static void ixgbevf_intr_enable(struct ixgbe_hw *hw);
+static void ixgbevf_intr_disable(struct rte_eth_dev *dev);
+static void ixgbevf_intr_enable(struct rte_eth_dev *dev);
 static int ixgbevf_dev_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *stats);
 static void ixgbevf_dev_stats_reset(struct rte_eth_dev *dev);
@@ -1636,7 +1636,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev *eth_dev)
 	ixgbevf_dev_stats_reset(eth_dev);
 
 	/* Disable the interrupts for VF */
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(eth_dev);
 
 	hw->mac.num_rar_entries = 128; /* The MAX of the underlying PF */
 	diag = hw->mac.ops.reset_hw(hw);
@@ -1705,7 +1705,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev *eth_dev)
 	rte_intr_callback_register(intr_handle,
 				   ixgbevf_dev_interrupt_handler, eth_dev);
 	rte_intr_enable(intr_handle);
-	ixgbevf_intr_enable(hw);
+	ixgbevf_intr_enable(eth_dev);
 
 	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x mac.type=%s",
 		     eth_dev->data->port_id, pci_dev->id.vendor_id,
@@ -1738,7 +1738,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = NULL;
 
 	/* Disable the interrupts for VF */
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(eth_dev);
 
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
@@ -4924,19 +4924,32 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
  * Virtual Function operations
  */
 static void
-ixgbevf_intr_disable(struct ixgbe_hw *hw)
+ixgbevf_intr_disable(struct rte_eth_dev *dev)
 {
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
+	struct ixgbe_hw *hw =
+		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
 	PMD_INIT_FUNC_TRACE();
 
 	/* Clear interrupt mask to stop from interrupts being generated */
 	IXGBE_WRITE_REG(hw, IXGBE_VTEIMC, IXGBE_VF_IRQ_CLEAR_MASK);
 
 	IXGBE_WRITE_FLUSH(hw);
+
+	/* Clear mask value. */
+	intr->mask = 0;
 }
 
 static void
-ixgbevf_intr_enable(struct ixgbe_hw *hw)
+ixgbevf_intr_enable(struct rte_eth_dev *dev)
 {
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
+	struct ixgbe_hw *hw =
+		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
 	PMD_INIT_FUNC_TRACE();
 
 	/* VF enable interrupt autoclean */
@@ -4945,6 +4958,9 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, IXGBE_VF_IRQ_ENABLE_MASK);
 
 	IXGBE_WRITE_FLUSH(hw);
+
+	/* Save IXGBE_VTEIMS value to mask. */
+	intr->mask = IXGBE_VF_IRQ_ENABLE_MASK;
 }
 
 static int
@@ -5070,7 +5086,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	rte_intr_enable(intr_handle);
 
 	/* Re-enable interrupt for VF */
-	ixgbevf_intr_enable(hw);
+	ixgbevf_intr_enable(dev);
 
 	return 0;
 }
@@ -5084,7 +5100,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(dev);
 
 	hw->adapter_stopped = 1;
 	ixgbe_stop_adapter(hw);
@@ -5582,17 +5598,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
-	uint32_t mask;
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	struct ixgbe_hw *hw =
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	uint32_t vec = IXGBE_MISC_VEC_ID;
 
-	mask = IXGBE_READ_REG(hw, IXGBE_VTEIMS);
 	if (rte_intr_allow_others(intr_handle))
 		vec = IXGBE_RX_VEC_START;
-	mask |= (1 << vec);
+	intr->mask |= (1 << vec);
 	RTE_SET_USED(queue_id);
-	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, mask);
+	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
 
 	rte_intr_enable(intr_handle);
 
@@ -5602,19 +5618,19 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 static int
 ixgbevf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id)
 {
-	uint32_t mask;
+	struct ixgbe_interrupt *intr =
+		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	struct ixgbe_hw *hw =
 		IXGBE_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;
 	uint32_t vec = IXGBE_MISC_VEC_ID;
 
-	mask = IXGBE_READ_REG(hw, IXGBE_VTEIMS);
 	if (rte_intr_allow_others(intr_handle))
 		vec = IXGBE_RX_VEC_START;
-	mask &= ~(1 << vec);
+	intr->mask &= ~(1 << vec);
 	RTE_SET_USED(queue_id);
-	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, mask);
+	IXGBE_WRITE_REG(hw, IXGBE_VTEIMS, intr->mask);
 
 	return 0;
 }
@@ -8170,7 +8186,7 @@ static void ixgbevf_mbx_process(struct rte_eth_dev *dev)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
-	ixgbevf_intr_disable(hw);
+	ixgbevf_intr_disable(dev);
 
 	/* read-on-clear nic registers here */
 	eicr = IXGBE_READ_REG(hw, IXGBE_VTEICR);
@@ -8187,7 +8203,6 @@ static void ixgbevf_mbx_process(struct rte_eth_dev *dev)
 static int
 ixgbevf_dev_interrupt_action(struct rte_eth_dev *dev)
 {
-	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 
@@ -8196,7 +8211,7 @@ static void ixgbevf_mbx_process(struct rte_eth_dev *dev)
 		intr->flags &= ~IXGBE_FLAG_MAILBOX;
 	}
 
-	ixgbevf_intr_enable(hw);
+	ixgbevf_intr_enable(dev);
 
 	return 0;
 }
-- 
1.8.3.1