* Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable interrupts in handler
2018-04-04 17:12 ` Jankowski, Konrad
@ 2018-04-06 15:47 ` Zhang, Helin
2018-04-06 15:52 ` Zhang, Helin
2018-04-10 13:26 ` Zhang, Qi Z
2 siblings, 0 replies; 7+ messages in thread
From: Zhang, Helin @ 2018-04-06 15:47 UTC (permalink / raw)
To: Jankowski, Konrad, Zhang, Qi Z, Dai, Wei, Xing, Beilei, Wu,
Jingjing, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jankowski, Konrad
> Sent: Thursday, April 5, 2018 1:12 AM
> To: Zhang, Qi Z; Dai, Wei; Xing, Beilei; Wu, Jingjing; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable interrupts
> in handler
>
> Hi Zhang,
>
> If you look at the source of interrupt handlers in both the igb_uio and
> uio_pci_generic drivers (look for irqreturn_t type), you will see they can
> disable interrupts immediately after receipt of one.
> It's up to the user to make sure they're re-enabled. Also please compare with
> code in i40e_ethdev.c, which still does this correctly - there's an explicit
> rte_intr_enable(dev->intr_handle) call at the end of
> i40e_dev_interrupt_handler().
> Probably a cleaner approach would be to leave them disabled as is, but only
> enable them for a once-off receipt when sending an AdminQ message, maybe
> that was the assumption here. However I've added some tracing to
> igbuio_pci_irqcontrol() and I'm sure this isn't happening on my system.
> (nothing is enabling those interrupts post device init). Looks like code path
> might be different with the newest igb_uio driver and MSI enabled, but the
> current code will still not work for all cases (like with uio_pci_generic).
Hi Konrad
Please don't use uio_pci_generic, as it is not supported by X710/XL710/XXV710 series,
as below. With VFIO, do you see similar issue?
http://www.dpdk.org/doc/guides/rel_notes/known_issues.html#uio-pci-generic-module-bind-failed-in-x710-xl710-xxv710
> There can also be cases when you have and older igb_uio driver which disables
> interrupts in all cases and running it with with a new DPDK. (vendor provided
> compiled driver) I think for full compatibility we need to keep re-enabling
> those interrupts.
>
> Regards,
> Konrad
>
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, March 28, 2018 4:37 AM
> To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
>
> Hi Jankowski:
>
> > -----Original Message-----
> > From: Jankowski, KonradX
> > Sent: Thursday, February 15, 2018 2:33 AM
> > To: Dai, Wei <wei.dai@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > Cc: Jankowski, KonradX <konradx.jankowski@intel.com>
> > Subject: [PATCH] net/i40evf: regression fix - reenable interrupts in
> > handler
> >
> > Commit 66b8304f removed the rte_intr_enable() call from
> > i40evf_dev_interrupt_handler() as a "bonus". On one of my systems this
> > causes the AdminQ messages to stop beeing delivered to the VF. This
> > results in unability to initialize and use the port. With this patch it works
> again.
> >
> > System in question:
> > Wind River OVP6 running kernel 3.10.58-ovp-rt58-WR6.0.0.13_preempt-rt
> >
> > Signed-off-by: Konrad Jankowski <konrad.jankowski@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev_vf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index fd003fe..b927a35 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -1404,6 +1404,7 @@ i40evf_dev_interrupt_handler(void *param)
> >
> > done:
> > i40evf_enable_irq0(hw);
> > + rte_intr_enable(dev->intr_handle);'
>
> Would you explain more about why the patch fix the issue?
> Usually we will not accept a fix just because it work but not understand the
> root cause.
>
> Regards
> Qi
>
> > }
> >
> > static int
> > --
> > 2.5.5
>
> --------------------------------------------------------------
> Intel Research and Development Ireland Limited Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
>
>
> This e-mail and any attachments may contain confidential material for the sole
> use of the intended recipient(s). Any review or distribution by others is strictly
> prohibited. If you are not the intended recipient, please contact the sender
> and delete all copies.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable interrupts in handler
2018-04-04 17:12 ` Jankowski, Konrad
2018-04-06 15:47 ` Zhang, Helin
@ 2018-04-06 15:52 ` Zhang, Helin
2018-04-10 13:26 ` Zhang, Qi Z
2 siblings, 0 replies; 7+ messages in thread
From: Zhang, Helin @ 2018-04-06 15:52 UTC (permalink / raw)
To: Jankowski, Konrad, Zhang, Qi Z, Dai, Wei, Xing, Beilei, Wu,
Jingjing, dev
> -----Original Message-----
> From: Zhang, Helin
> Sent: Friday, April 6, 2018 11:48 PM
> To: Jankowski, Konrad; Zhang, Qi Z; Dai, Wei; Xing, Beilei; Wu, Jingjing;
> dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jankowski, Konrad
> > Sent: Thursday, April 5, 2018 1:12 AM
> > To: Zhang, Qi Z; Dai, Wei; Xing, Beilei; Wu, Jingjing; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable
> > interrupts in handler
> >
> > Hi Zhang,
> >
> > If you look at the source of interrupt handlers in both the igb_uio
> > and uio_pci_generic drivers (look for irqreturn_t type), you will see
> > they can disable interrupts immediately after receipt of one.
> > It's up to the user to make sure they're re-enabled. Also please
> > compare with code in i40e_ethdev.c, which still does this correctly -
> > there's an explicit
> > rte_intr_enable(dev->intr_handle) call at the end of
> > i40e_dev_interrupt_handler().
> > Probably a cleaner approach would be to leave them disabled as is, but
> > only enable them for a once-off receipt when sending an AdminQ
> > message, maybe that was the assumption here. However I've added some
> > tracing to
> > igbuio_pci_irqcontrol() and I'm sure this isn't happening on my system.
> > (nothing is enabling those interrupts post device init). Looks like
> > code path might be different with the newest igb_uio driver and MSI
> > enabled, but the current code will still not work for all cases (like with
> uio_pci_generic).
>
> Hi Konrad
> Please don't use uio_pci_generic, as it is not supported by X710/XL710/XXV710
> series, as below. With VFIO, do you see similar issue?
> http://www.dpdk.org/doc/guides/rel_notes/known_issues.html#uio-pci-
> generic-module-bind-failed-in-x710-xl710-xxv710
>
> > There can also be cases when you have and older igb_uio driver which
> > disables interrupts in all cases and running it with with a new DPDK.
> > (vendor provided compiled driver) I think for full compatibility we
> > need to keep re-enabling those interrupts.
> >
> > Regards,
> > Konrad
> >
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Wednesday, March 28, 2018 4:37 AM
> > To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> > <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts
> > in handler
> >
> > Hi Jankowski:
> >
> > > -----Original Message-----
> > > From: Jankowski, KonradX
> > > Sent: Thursday, February 15, 2018 2:33 AM
> > > To: Dai, Wei <wei.dai@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > > Cc: Jankowski, KonradX <konradx.jankowski@intel.com>
> > > Subject: [PATCH] net/i40evf: regression fix - reenable interrupts in
> > > handler
Better title might be needed, with start of 'fix'. Also note that 'Fixes' line is
needed, and 'Cc' line might be needed.
> > >
> > > Commit 66b8304f removed the rte_intr_enable() call from
> > > i40evf_dev_interrupt_handler() as a "bonus". On one of my systems
> > > this causes the AdminQ messages to stop beeing delivered to the VF.
> > > This results in unability to initialize and use the port. With this
> > > patch it works
> > again.
Also please fix the checkpatch warnings on patchwork. Thanks!
/Helin
> > >
> > > System in question:
> > > Wind River OVP6 running kernel
> > > 3.10.58-ovp-rt58-WR6.0.0.13_preempt-rt
> > >
> > > Signed-off-by: Konrad Jankowski <konrad.jankowski@intel.com>
> > > ---
> > > drivers/net/i40e/i40e_ethdev_vf.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > > b/drivers/net/i40e/i40e_ethdev_vf.c
> > > index fd003fe..b927a35 100644
> > > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > > @@ -1404,6 +1404,7 @@ i40evf_dev_interrupt_handler(void *param)
> > >
> > > done:
> > > i40evf_enable_irq0(hw);
> > > + rte_intr_enable(dev->intr_handle);'
> >
> > Would you explain more about why the patch fix the issue?
> > Usually we will not accept a fix just because it work but not
> > understand the root cause.
> >
> > Regards
> > Qi
> >
> > > }
> > >
> > > static int
> > > --
> > > 2.5.5
> >
> > --------------------------------------------------------------
> > Intel Research and Development Ireland Limited Registered in Ireland
> > Registered Office: Collinstown Industrial Park, Leixlip, County
> > Kildare Registered Number: 308263
> >
> >
> > This e-mail and any attachments may contain confidential material for
> > the sole use of the intended recipient(s). Any review or distribution
> > by others is strictly prohibited. If you are not the intended
> > recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable interrupts in handler
2018-04-04 17:12 ` Jankowski, Konrad
2018-04-06 15:47 ` Zhang, Helin
2018-04-06 15:52 ` Zhang, Helin
@ 2018-04-10 13:26 ` Zhang, Qi Z
2018-04-10 14:06 ` Jankowski, Konrad
2 siblings, 1 reply; 7+ messages in thread
From: Zhang, Qi Z @ 2018-04-10 13:26 UTC (permalink / raw)
To: Jankowski, Konrad, Dai, Wei, Xing, Beilei, Wu, Jingjing, dev
Hi Konrad:
Sorry for late reply.
> -----Original Message-----
> From: Jankowski, Konrad
> Sent: Thursday, April 5, 2018 1:12 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Dai, Wei <wei.dai@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
>
> Hi Zhang,
>
> If you look at the source of interrupt handlers in both the igb_uio and
> uio_pci_generic drivers (look for irqreturn_t type), you will see they can
> disable interrupts immediately after receipt of one.
Do you mean
if (udev->mode == RTE_INTR_MODE_LEGACY &&
!pci_check_and_mask_intx(udev->pdev))
return IRQ_NONE;
so you are using legacy interrupt mode?
According i40e datasheet (chapter 7.5.1), legacy interrupt mode is only supported by PF,
So that's why we have rte_intr_enable in i40e_dev_interrupt_handler.
But for VF, it only support msi-x interrupt mode.
Regards
Qi
> It's up to the user to make sure they're re-enabled. Also please compare with
> code in i40e_ethdev.c, which still does this correctly - there's an explicit
> rte_intr_enable(dev->intr_handle) call at the end of
> i40e_dev_interrupt_handler().
> Probably a cleaner approach would be to leave them disabled as is, but only
> enable them for a once-off receipt when sending an AdminQ message, maybe
> that was the assumption here. However I've added some tracing to
> igbuio_pci_irqcontrol() and I'm sure this isn't happening on my system.
> (nothing is enabling those interrupts post device init). Looks like code path
> might be different with the newest igb_uio driver and MSI enabled, but the
> current code will still not work for all cases (like with uio_pci_generic).
> There can also be cases when you have and older igb_uio driver which disables
> interrupts in all cases and running it with with a new DPDK. (vendor provided
> compiled driver) I think for full compatibility we need to keep re-enabling
> those interrupts.
>
> Regards,
> Konrad
>
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, March 28, 2018 4:37 AM
> To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
>
> Hi Jankowski:
>
> > -----Original Message-----
> > From: Jankowski, KonradX
> > Sent: Thursday, February 15, 2018 2:33 AM
> > To: Dai, Wei <wei.dai@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > Cc: Jankowski, KonradX <konradx.jankowski@intel.com>
> > Subject: [PATCH] net/i40evf: regression fix - reenable interrupts in
> > handler
> >
> > Commit 66b8304f removed the rte_intr_enable() call from
> > i40evf_dev_interrupt_handler() as a "bonus". On one of my systems this
> > causes the AdminQ messages to stop beeing delivered to the VF. This
> > results in unability to initialize and use the port. With this patch it works
> again.
> >
> > System in question:
> > Wind River OVP6 running kernel 3.10.58-ovp-rt58-WR6.0.0.13_preempt-rt
> >
> > Signed-off-by: Konrad Jankowski <konrad.jankowski@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev_vf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index fd003fe..b927a35 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -1404,6 +1404,7 @@ i40evf_dev_interrupt_handler(void *param)
> >
> > done:
> > i40evf_enable_irq0(hw);
> > + rte_intr_enable(dev->intr_handle);'
>
> Would you explain more about why the patch fix the issue?
> Usually we will not accept a fix just because it work but not understand the
> root cause.
>
> Regards
> Qi
>
> > }
> >
> > static int
> > --
> > 2.5.5
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40evf: regression fix - reenable interrupts in handler
2018-04-10 13:26 ` Zhang, Qi Z
@ 2018-04-10 14:06 ` Jankowski, Konrad
0 siblings, 0 replies; 7+ messages in thread
From: Jankowski, Konrad @ 2018-04-10 14:06 UTC (permalink / raw)
To: Zhang, Qi Z, Dai, Wei, Xing, Beilei, Wu, Jingjing, dev
Hi Qi,
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, April 10, 2018 2:26 PM
> To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts in handler
>
> Hi Konrad:
> Sorry for late reply.
>
> > -----Original Message-----
> > From: Jankowski, Konrad
> > Sent: Thursday, April 5, 2018 1:12 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Dai, Wei <wei.dai@intel.com>;
> > Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts
> > in handler
> >
> > Hi Zhang,
> >
> > If you look at the source of interrupt handlers in both the igb_uio
> > and uio_pci_generic drivers (look for irqreturn_t type), you will see
> > they can disable interrupts immediately after receipt of one.
>
> Do you mean
>
> if (udev->mode == RTE_INTR_MODE_LEGACY &&
> !pci_check_and_mask_intx(udev->pdev))
> return IRQ_NONE;
>
> so you are using legacy interrupt mode?
>
> According i40e datasheet (chapter 7.5.1), legacy interrupt mode is only
> supported by PF, So that's why we have rte_intr_enable in
> i40e_dev_interrupt_handler.
> But for VF, it only support msi-x interrupt mode.
>
> Regards
> Qi
OK I see. Still the old version of igb_uio disables interrupts in both cases, MSI or legacy.
That version comes provided with windriver OVP6, and that's the only version that
actually works on this system (back from DPDK 1.6 accrording to rte_version.h).
If we're ok with not supporting this system, I'm ok with dropping this patch.
(I'll just use it locally myself)
Cheers,
Konrad
> > It's up to the user to make sure they're re-enabled. Also please
> > compare with code in i40e_ethdev.c, which still does this correctly -
> > there's an explicit
> > rte_intr_enable(dev->intr_handle) call at the end of
> > i40e_dev_interrupt_handler().
> > Probably a cleaner approach would be to leave them disabled as is, but
> > only enable them for a once-off receipt when sending an AdminQ
> > message, maybe that was the assumption here. However I've added some
> > tracing to
> > igbuio_pci_irqcontrol() and I'm sure this isn't happening on my system.
> > (nothing is enabling those interrupts post device init). Looks like
> > code path might be different with the newest igb_uio driver and MSI
> > enabled, but the current code will still not work for all cases (like with
> uio_pci_generic).
> > There can also be cases when you have and older igb_uio driver which
> > disables interrupts in all cases and running it with with a new DPDK.
> > (vendor provided compiled driver) I think for full compatibility we
> > need to keep re-enabling those interrupts.
> >
> > Regards,
> > Konrad
> >
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Wednesday, March 28, 2018 4:37 AM
> > To: Jankowski, Konrad <konrad.jankowski@intel.com>; Dai, Wei
> > <wei.dai@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH] net/i40evf: regression fix - reenable interrupts
> > in handler
> >
> > Hi Jankowski:
> >
> > > -----Original Message-----
> > > From: Jankowski, KonradX
> > > Sent: Thursday, February 15, 2018 2:33 AM
> > > To: Dai, Wei <wei.dai@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > > Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> > > Cc: Jankowski, KonradX <konradx.jankowski@intel.com>
> > > Subject: [PATCH] net/i40evf: regression fix - reenable interrupts in
> > > handler
> > >
> > > Commit 66b8304f removed the rte_intr_enable() call from
> > > i40evf_dev_interrupt_handler() as a "bonus". On one of my systems
> > > this causes the AdminQ messages to stop beeing delivered to the VF.
> > > This results in unability to initialize and use the port. With this
> > > patch it works
> > again.
> > >
> > > System in question:
> > > Wind River OVP6 running kernel
> > > 3.10.58-ovp-rt58-WR6.0.0.13_preempt-rt
> > >
> > > Signed-off-by: Konrad Jankowski <konrad.jankowski@intel.com>
> > > ---
> > > drivers/net/i40e/i40e_ethdev_vf.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > > b/drivers/net/i40e/i40e_ethdev_vf.c
> > > index fd003fe..b927a35 100644
> > > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > > @@ -1404,6 +1404,7 @@ i40evf_dev_interrupt_handler(void *param)
> > >
> > > done:
> > > i40evf_enable_irq0(hw);
> > > + rte_intr_enable(dev->intr_handle);'
> >
> > Would you explain more about why the patch fix the issue?
> > Usually we will not accept a fix just because it work but not
> > understand the root cause.
> >
> > Regards
> > Qi
> >
> > > }
> > >
> > > static int
> > > --
> > > 2.5.5
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
^ permalink raw reply [flat|nested] 7+ messages in thread