diff mbox

[dpdk-dev] net/virtio: fix fail to configure rxq interrupt

Message ID 1500434303-12975-1-git-send-email-jiayu.hu@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Yuanhan Liu
Headers

Checks

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

Commit Message

Hu, Jiayu July 19, 2017, 3:18 a.m. UTC 
  When use rte_eth_dev_configure() to enable rx queue interrupt for virtio
devices, virtio_configure_intr() isn't called to set up the interrupt
environment, which causes rx queue interrupt setup failed. This patch is
to fix this issue.

Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Yang, Zhiyong July 19, 2017, 3:23 a.m. UTC | #1 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jiayu Hu
> Sent: Wednesday, July 19, 2017 11:18 AM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; yliu@fridaylinux.org; Yao, Lei A
> <lei.a.yao@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/virtio: fix fail to configure rxq interrupt
> 
> When use rte_eth_dev_configure() to enable rx queue interrupt for virtio devices,
> virtio_configure_intr() isn't called to set up the interrupt environment, which
> causes rx queue interrupt setup failed. This patch is to fix this issue.
> 
> Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Reviewed-by: Zhiyong Yang <zhiyong.yang@intel.com >

Thanks
Zhiyong

> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00a3122..66656ed 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1688,6 +1688,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  		return -ENOTSUP;
>  	}
> 
> +	if (dev->data->dev_conf.intr_conf.rxq) {
> +		if (virtio_configure_intr(dev) < 0) {
> +			PMD_DRV_LOG(ERR, "failed to configure interrupt");
> +			return -ENOTSUP;
> +		}
> +	}
> +
>  	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>  		/* Enable vector (0) for Link State Intrerrupt */
>  		if (VTPCI_OPS(hw)->set_config_irq(hw, 0) ==
> --
> 2.7.4
Yuanhan Liu July 19, 2017, 11:08 a.m. UTC | #2 
On Wed, Jul 19, 2017 at 11:18:23AM +0800, Jiayu Hu wrote:
> When use rte_eth_dev_configure() to enable rx queue interrupt for virtio
> devices, virtio_configure_intr() isn't called to set up the interrupt
> environment, which causes rx queue interrupt setup failed. This patch is
> to fix this issue.

Hmm, how was this supposed to work in the begining when this feature
was introduced? Jianfeng?

	--yliu


> 
> Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00a3122..66656ed 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1688,6 +1688,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  		return -ENOTSUP;
>  	}
>  
> +	if (dev->data->dev_conf.intr_conf.rxq) {
> +		if (virtio_configure_intr(dev) < 0) {
> +			PMD_DRV_LOG(ERR, "failed to configure interrupt");
> +			return -ENOTSUP;
> +		}
> +	}
> +
>  	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>  		/* Enable vector (0) for Link State Intrerrupt */
>  		if (VTPCI_OPS(hw)->set_config_irq(hw, 0) ==
> -- 
> 2.7.4
Jianfeng Tan July 19, 2017, 4:03 p.m. UTC | #3 
On 7/19/2017 4:08 AM, Yuanhan Liu wrote:
> On Wed, Jul 19, 2017 at 11:18:23AM +0800, Jiayu Hu wrote:
>> When use rte_eth_dev_configure() to enable rx queue interrupt for virtio
>> devices, virtio_configure_intr() isn't called to set up the interrupt
>> environment, which causes rx queue interrupt setup failed. This patch is
>> to fix this issue.
> Hmm, how was this supposed to work in the begining when this feature
> was introduced? Jianfeng?

In the v17.05, virtio_configure_intr() will be called by 
virtio_dev_configure() if the request feature is changed. That's not 
correct.


>
> 	--yliu
>
>
>> Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>> ---
>>   drivers/net/virtio/virtio_ethdev.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 00a3122..66656ed 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1688,6 +1688,13 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   		return -ENOTSUP;
>>   	}
>>   
>> +	if (dev->data->dev_conf.intr_conf.rxq) {
>> +		if (virtio_configure_intr(dev) < 0) {
>> +			PMD_DRV_LOG(ERR, "failed to configure interrupt");
>> +			return -ENOTSUP;
>> +		}
>> +	}
>> +

Hi Jiayu,

I would expect this will not work for virtio pci devices (QEMU might 
crash). Could you double check that?

Thanks,
Jianfeng

>>   	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>   		/* Enable vector (0) for Link State Intrerrupt */
>>   		if (VTPCI_OPS(hw)->set_config_irq(hw, 0) ==
>> -- 
>> 2.7.4
Yuanhan Liu July 31, 2017, 2:30 a.m. UTC | #4 
On Wed, Jul 19, 2017 at 11:18:23AM +0800, Jiayu Hu wrote:
> When use rte_eth_dev_configure() to enable rx queue interrupt for virtio
> devices, virtio_configure_intr() isn't called to set up the interrupt
> environment, which causes rx queue interrupt setup failed. This patch is
> to fix this issue.
> 
> Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>

Applied to dpdk-next-virtio.

Thanks.

	--yliu
Yuanhan Liu July 31, 2017, 3:47 a.m. UTC | #5 
On Mon, Jul 31, 2017 at 02:47:13AM +0000, Hu, Jiayu wrote:
> Hi Yuanhan,
> 
> When we run l3fwd-power in VM with applying this patch in VM DPDK, QEMU will crash. I think this patch can't solve the problem correctly but I haven't figured out the reason. I am so sorry about it.

It's okay. But you should reply earlier here, so that I would not apply it.

Regarding the crash, I believe Jianfeng had met a similar issue before. IIRC,
it's about the order of event fd setups (or something like it). You might
want to contact him for more details.

And it's dropped.

	--yliu

> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > Sent: Monday, July 31, 2017 10:30
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>; Yao, Lei A
> > <lei.a.yao@intel.com>; stable@dpdk.org
> > Subject: Re: [PATCH] net/virtio: fix fail to configure rxq interrupt
> > 
> > On Wed, Jul 19, 2017 at 11:18:23AM +0800, Jiayu Hu wrote:
> > > When use rte_eth_dev_configure() to enable rx queue interrupt for virtio
> > > devices, virtio_configure_intr() isn't called to set up the interrupt
> > > environment, which causes rx queue interrupt setup failed. This patch is
> > > to fix this issue.
> > >
> > > Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > 
> > Applied to dpdk-next-virtio.
> > 
> > Thanks.
> > 
> > 	--yliu
Hu, Jiayu July 31, 2017, 7:28 a.m. UTC | #6 
Hi Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Monday, July 31, 2017 11:47 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH] net/virtio: fix fail to configure rxq interrupt
> 
> On Mon, Jul 31, 2017 at 02:47:13AM +0000, Hu, Jiayu wrote:
> > Hi Yuanhan,
> >
> > When we run l3fwd-power in VM with applying this patch in VM DPDK,
> QEMU will crash. I think this patch can't solve the problem correctly but I
> haven't figured out the reason. I am so sorry about it.
> 
> It's okay. But you should reply earlier here, so that I would not apply it.
> 
> Regarding the crash, I believe Jianfeng had met a similar issue before. IIRC,
> it's about the order of event fd setups (or something like it). You might
> want to contact him for more details.

The root cause is that the interrupt should be set up before DRIVER_OK
is sent to QEMU. Otherwise, QEMU will crash.

The simplest way to enable RXQ interrupt after devices are initialized is to
re-init them via virtio_init_devices(), rather than to call virtio_configure_intr()
directly. This is also what Jianfeng does originally.

BRs,
Jiayu
> 
> And it's dropped.
> 
> 	--yliu
> 
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > > Sent: Monday, July 31, 2017 10:30
> > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>; Yao, Lei A
> > > <lei.a.yao@intel.com>; stable@dpdk.org
> > > Subject: Re: [PATCH] net/virtio: fix fail to configure rxq interrupt
> > >
> > > On Wed, Jul 19, 2017 at 11:18:23AM +0800, Jiayu Hu wrote:
> > > > When use rte_eth_dev_configure() to enable rx queue interrupt for
> virtio
> > > > devices, virtio_configure_intr() isn't called to set up the interrupt
> > > > environment, which causes rx queue interrupt setup failed. This patch is
> > > > to fix this issue.
> > > >
> > > > Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > >
> > > Applied to dpdk-next-virtio.
> > >
> > > Thanks.
> > >
> > > 	--yliu
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 00a3122..66656ed 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1688,6 +1688,13 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 		return -ENOTSUP;
 	}
 
+	if (dev->data->dev_conf.intr_conf.rxq) {
+		if (virtio_configure_intr(dev) < 0) {
+			PMD_DRV_LOG(ERR, "failed to configure interrupt");
+			return -ENOTSUP;
+		}
+	}
+
 	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		/* Enable vector (0) for Link State Intrerrupt */
 		if (VTPCI_OPS(hw)->set_config_irq(hw, 0) ==