[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
On Tue, Feb 23, 2016 at 02:10:57AM +, Zhang, Helin wrote:
>
>
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Michael Qiu
> > Sent: Friday, January 29, 2016 1:58 PM
> > To: dev at dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
> >
> > Currently, ixgbe vf and pf will disable interrupt twice in stop stage and
> > uninit
> > stage. It will cause an error:
> >
> > testpmd> quit
> >
> > Shutting down port 0...
> > Stopping ports...
> > Done
> > Closing ports...
> > EAL: Error disabling MSI-X interrupts for fd 26
> > Done
> >
> > Becasue the interrupt already been disabled in stop stage.
> > Since it is enabled in init stage, better remove from stop stage.
> >
> > Fixes: 0eb609239efd ("ixgbe: enable Rx queue interrupts for PF and VF")
> >
> > Signed-off-by: Michael Qiu
> Acked-by: Helin Zhang
>
applied to dpdk-next-net/rel_16_04
/Bruce
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Michael Qiu
> Sent: Friday, January 29, 2016 1:58 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
>
> Currently, ixgbe vf and pf will disable interrupt twice in stop stage and
> uninit
> stage. It will cause an error:
>
> testpmd> quit
>
> Shutting down port 0...
> Stopping ports...
> Done
> Closing ports...
> EAL: Error disabling MSI-X interrupts for fd 26
> Done
>
> Becasue the interrupt already been disabled in stop stage.
> Since it is enabled in init stage, better remove from stop stage.
>
> Fixes: 0eb609239efd ("ixgbe: enable Rx queue interrupts for PF and VF")
>
> Signed-off-by: Michael Qiu
Acked-by: Helin Zhang
> ---
> v2 --> v1:
> fix error in commit log word "interrupte"
>
> drivers/net/ixgbe/ixgbe_ethdev.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 4c4c6df..a561f8d 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2192,9 +2192,6 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> /* disable interrupts */
> ixgbe_disable_intr(hw);
>
> - /* disable intr eventfd mapping */
> - rte_intr_disable(intr_handle);
> -
> /* reset the NIC */
> ixgbe_pf_reset_hw(hw);
> hw->adapter_stopped = 0;
> @@ -3898,9 +3895,6 @@ ixgbevf_dev_stop(struct rte_eth_dev *dev)
>
> ixgbe_dev_clear_queues(dev);
>
> - /* disable intr eventfd mapping */
> - rte_intr_disable(intr_handle);
> -
> /* Clean datapath event and queue/vec mapping */
> rte_intr_efd_disable(intr_handle);
> if (intr_handle->intr_vec != NULL) {
> --
> 1.9.3
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
Hi Michael > > On 2016/2/2 19:03, Ananyev, Konstantin wrote: > > > > [...] > > I don't think i40e miss it, because it not the right please to disable > interrupt. > because all interrupts are enabled in init stage. > > Actually, ixgbe enable the interrupt in init stage, but in dev_start, it > disable it > first and re-enable, so it just the same with doing nothing about > interrupt. > > Just think below: > > 1. start the port.(interrupt already enabled in init stage, disable --> > re-enable) > 2. stop the port.(disable interrupt) > 3. start port again(Try to disable, but failed, already disabled) > > Would you think the code has issue? > >>> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls > >>> dev_close(), > >>> which calls dev_stop(). So I think the disabling can be done only in > >>> dev_stop(). > >>> All others can make use of dev_stop to disable the interrupt. > >> As I said, if it is in dev_stop, it will has issue when dev_start --> > >> dev_stop --> dev_start, this also could applied in i40e and fm10k. If > >> you want to put it in dev_stop, better to remove enable interrupts in > >> init stage, and only put it in dev_start. > > We can't remove enabling interrupt at init stage and put it only in > > dev_start(). > > That means PF couldn't handle interrupts from VF till dev_start() will be > > executed on PF > > - which could never happen. > > For same reason we can't disable all interrupts in dev_stop(). > > See: http://dpdk.org/ml/archives/dev/2015-November/027238.html > > Hi, Konstantin > > Yes, you are right. > > So the only way to fix this issue should remove it in dev_stop(), and > left it in uinit() stage, which my patch does. > > Am I right? Yes, I think so. PF should be able to receive MBOX interrupts after dev_stop(). Konstantin > > Thanks, > Michael > > Konstantin > > > >> Thanks, > >> Michael > >>> Regards, > >>> Helin > >>> > Thanks, > Michael > > > Maybe we can follow fm10k's style. > > > >> On other hand, if we remove it in dev_stop, any side effect? In ixgbe > >> start, it will always disable it first and then re-enable it, so it's > >> safe. > > I think you mean we can disable intr anyway even if it has been > > disabled. > Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable > interrupts, and > if we try disable twice, it will return and error. > That's why I mean we need a flag to show the interrupts stats. If it > already > disabled, we do not need call in to kernel. just return and give a > warning > message. > > Thanks, > Michael > > > Sounds more like why we don't > > need this patch :) > > > >> Thanks, > >> Michael > >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
On 2016/2/2 19:03, Ananyev, Konstantin wrote: > [...] I don't think i40e miss it, because it not the right please to disable interrupt. because all interrupts are enabled in init stage. Actually, ixgbe enable the interrupt in init stage, but in dev_start, it disable it first and re-enable, so it just the same with doing nothing about interrupt. Just think below: 1. start the port.(interrupt already enabled in init stage, disable --> re-enable) 2. stop the port.(disable interrupt) 3. start port again(Try to disable, but failed, already disabled) Would you think the code has issue? >>> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(), >>> which calls dev_stop(). So I think the disabling can be done only in >>> dev_stop(). >>> All others can make use of dev_stop to disable the interrupt. >> As I said, if it is in dev_stop, it will has issue when dev_start --> >> dev_stop --> dev_start, this also could applied in i40e and fm10k. If >> you want to put it in dev_stop, better to remove enable interrupts in >> init stage, and only put it in dev_start. > We can't remove enabling interrupt at init stage and put it only in > dev_start(). > That means PF couldn't handle interrupts from VF till dev_start() will be > executed on PF > - which could never happen. > For same reason we can't disable all interrupts in dev_stop(). > See: http://dpdk.org/ml/archives/dev/2015-November/027238.html Hi, Konstantin Yes, you are right. So the only way to fix this issue should remove it in dev_stop(), and left it in uinit() stage, which my patch does. Am I right? Thanks, Michael > Konstantin > >> Thanks, >> Michael >>> Regards, >>> Helin >>> Thanks, Michael > Maybe we can follow fm10k's style. > >> On other hand, if we remove it in dev_stop, any side effect? In ixgbe >> start, it will always disable it first and then re-enable it, so it's >> safe. > I think you mean we can disable intr anyway even if it has been disabled. Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, and if we try disable twice, it will return and error. That's why I mean we need a flag to show the interrupts stats. If it already disabled, we do not need call in to kernel. just return and give a warning message. Thanks, Michael > Sounds more like why we don't > need this patch :) > >> Thanks, >> Michael >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Qiu, Michael > Sent: Tuesday, February 02, 2016 2:57 AM > To: Zhang, Helin; Lu, Wenzhuo; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice > > On 2/2/2016 10:14 AM, Zhang, Helin wrote: > > > >> -Original Message- > >> From: Qiu, Michael > >> Sent: Tuesday, February 2, 2016 10:07 AM > >> To: Lu, Wenzhuo; dev at dpdk.org > >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin > >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > >> > >> [+cc helin] > >> > >> On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: > >>> Hi Michael, > >>> > >>>> -Original Message- > >>>> From: Qiu, Michael > >>>> Sent: Monday, February 1, 2016 4:05 PM > >>>> To: Lu, Wenzhuo; dev at dpdk.org > >>>> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming > >>>> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > >>>> > >>>> On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > >>>>> Hi Michael, > >>>>> > >>>>>> -Original Message- > >>>>>> From: Qiu, Michael > >>>>>> Sent: Friday, January 29, 2016 1:58 PM > >>>>>> To: dev at dpdk.org > >>>>>> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, > >>>>>> Michael > >>>>>> Subject: [PATCH v2] ixgbe: Fix disable interrupt twice > >>>>>> > >>>>>> Currently, ixgbe vf and pf will disable interrupt twice in stop > >>>>>> stage and uninit stage. It will cause an error: > >>>>>> > >>>>>> testpmd> quit > >>>>>> > >>>>>> Shutting down port 0... > >>>>>> Stopping ports... > >>>>>> Done > >>>>>> Closing ports... > >>>>>> EAL: Error disabling MSI-X interrupts for fd 26 > >>>>>> Done > >>>>>> > >>>>>> Becasue the interrupt already been disabled in stop stage. > >>>>>> Since it is enabled in init stage, better remove from stop stage. > >>>>> I'm afraid it's not a good idea to just remove the intr_disable from > >> dev_stop. > >>>>> I think dev_stop have the chance to be used independently with > >>>>> dev_unint. In > >>>> this scenario, we still need intr_disable, right? > >>>>> Maybe what we need is some check before we disable the intr:) > >>>> Yes, indeed we need some check in disable intr, but it need > >>>> additional fields in "struct rte_intr_handle", and it's much saft to > >>>> do so, but as I check i40e/fm10k code, only ixgbe disable it in > >>>> dev_stop(). > >>> I found fm10k doesn't enable intr in dev_start. So, I think it's OK. But > >>> i40e > >> enables intr in dev_start. > >>> To my opinion, it's more like i40e misses the intr_disable in dev_stop. > >> I don't think i40e miss it, because it not the right please to disable > >> interrupt. > >> because all interrupts are enabled in init stage. > >> > >> Actually, ixgbe enable the interrupt in init stage, but in dev_start, it > >> disable it > >> first and re-enable, so it just the same with doing nothing about > >> interrupt. > >> > >> Just think below: > >> > >> 1. start the port.(interrupt already enabled in init stage, disable --> > >> re-enable) > >> 2. stop the port.(disable interrupt) > >> 3. start port again(Try to disable, but failed, already disabled) > >> > >> Would you think the code has issue? > > [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(), > > which calls dev_stop(). So I think the disabling can be done only in > > dev_stop(). > > All others can make use of dev_stop to disable the interrupt. > > As I said, if it is in dev_stop, it will has issue when dev_start --> > dev_stop --> dev_start, this also could applied in i40e and fm10k. If > you want to put it in dev_stop, better to remove enable interrupts in > init stage, and only put it in dev_start. We can't remove enabling interrupt at init stage and put it only in dev_start(). That means PF couldn't handle interrupts from VF till dev_start() will be executed on PF - which could never happen. For same reason we can't disable all interrupts in dev_stop(). See: http://dpdk.org/ml/archives/dev/2015-November/027238.html Konstantin > > Thanks, > Michael > > Regards, > > Helin > > > >> Thanks, > >> Michael > >> > >>> Maybe we can follow fm10k's style. > >>> > >>>> On other hand, if we remove it in dev_stop, any side effect? In ixgbe > >>>> start, it will always disable it first and then re-enable it, so it's > >>>> safe. > >>> I think you mean we can disable intr anyway even if it has been disabled. > >> Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable > >> interrupts, and > >> if we try disable twice, it will return and error. > >> That's why I mean we need a flag to show the interrupts stats. If it > >> already > >> disabled, we do not need call in to kernel. just return and give a warning > >> message. > >> > >> Thanks, > >> Michael > >> > >>> Sounds more like why we don't > >>> need this patch :) > >>> > >>>> Thanks, > >>>> Michael > >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
On 2/2/2016 11:07 AM, Zhang, Helin wrote: > >> -Original Message- >> From: Qiu, Michael >> Sent: Tuesday, February 2, 2016 10:57 AM >> To: Zhang, Helin ; Lu, Wenzhuo >> ; dev at dpdk.org >> Cc: Zhou, Danny ; Liu, Yong ; >> Liang, Cunming >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice >> >> On 2/2/2016 10:14 AM, Zhang, Helin wrote: -Original Message- From: Qiu, Michael Sent: Tuesday, February 2, 2016 10:07 AM To: Lu, Wenzhuo; dev at dpdk.org Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice [+cc helin] On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: > Hi Michael, > >> -Original Message- >> From: Qiu, Michael >> Sent: Monday, February 1, 2016 4:05 PM >> To: Lu, Wenzhuo; dev at dpdk.org >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice >> >> On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: >>> Hi Michael, >>> -Original Message- From: Qiu, Michael Sent: Friday, January 29, 2016 1:58 PM To: dev at dpdk.org Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, Michael Subject: [PATCH v2] ixgbe: Fix disable interrupt twice Currently, ixgbe vf and pf will disable interrupt twice in stop stage and uninit stage. It will cause an error: testpmd> quit Shutting down port 0... Stopping ports... Done Closing ports... EAL: Error disabling MSI-X interrupts for fd 26 Done Becasue the interrupt already been disabled in stop stage. Since it is enabled in init stage, better remove from stop stage. >>> I'm afraid it?s not a good idea to just remove the intr_disable >>> from dev_stop. >>> I think dev_stop have the chance to be used independently with >>> dev_unint. In >> this scenario, we still need intr_disable, right? >>> Maybe what we need is some check before we disable the intr:) >> Yes, indeed we need some check in disable intr, but it need >> additional fields in "struct rte_intr_handle", and it's much saft >> to do so, but as I check i40e/fm10k code, only ixgbe disable it in >> dev_stop(). > I found fm10k doesn?t enable intr in dev_start. So, I think it's OK. > But i40e enables intr in dev_start. > To my opinion, it's more like i40e misses the intr_disable in dev_stop. I don't think i40e miss it, because it not the right please to disable interrupt. because all interrupts are enabled in init stage. Actually, ixgbe enable the interrupt in init stage, but in dev_start, it disable it first and re-enable, so it just the same with doing nothing about >> interrupt. Just think below: 1. start the port.(interrupt already enabled in init stage, disable --> re-enable) 2. stop the port.(disable interrupt) 3. start port again(Try to disable, but failed, already disabled) Would you think the code has issue? >>> [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls >>> dev_close(), which calls dev_stop(). So I think the disabling can be done >>> only in >> dev_stop(). >>> All others can make use of dev_stop to disable the interrupt. >> As I said, if it is in dev_stop, it will has issue when dev_start --> >> dev_stop --> >> dev_start, this also could applied in i40e and fm10k. If you want to put it >> in >> dev_stop, better to remove enable interrupts in init stage, and only put it >> in >> dev_start. > Oh, yes, you are talking about the refactoring. That's good, and reasonable. > Please do more validation with LSC, mailbox, rx interrupts, to make sure there > is no issue introduced. I have no plan to do code refactor, it includes lots of validation, and will influence many components, time is limited for 2.3. I would like keep it in uninit and remove it from stop, this only affect ixgbe, and I have done validation for it. Thanks, Michael > Thanks, > Helin > >> Thanks, >> Michael >>> Regards, >>> Helin >>> Thanks, Michael > Maybe we can follow fm10k's style. > >> On other hand, if we remove it in dev_stop, any side effect? In >> ixgbe start, it will always disable it first and then re-enable it, so >> it's safe. > I think you mean we can disable intr anyway even if it has been disabled. Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, and if we try disable twice, it will return and error. That's why I mean we need a flag to show the interrupts stats. If it already disabled, we do not need call in to kernel. just return and give a warning message. Thanks, Michael > Sounds more like why we
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
> -Original Message- > From: Qiu, Michael > Sent: Tuesday, February 2, 2016 10:57 AM > To: Zhang, Helin ; Lu, Wenzhuo > ; dev at dpdk.org > Cc: Zhou, Danny ; Liu, Yong ; > Liang, Cunming > Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > > On 2/2/2016 10:14 AM, Zhang, Helin wrote: > > > >> -Original Message- > >> From: Qiu, Michael > >> Sent: Tuesday, February 2, 2016 10:07 AM > >> To: Lu, Wenzhuo; dev at dpdk.org > >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin > >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > >> > >> [+cc helin] > >> > >> On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: > >>> Hi Michael, > >>> > -Original Message- > From: Qiu, Michael > Sent: Monday, February 1, 2016 4:05 PM > To: Lu, Wenzhuo; dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming > Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > > On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > > Hi Michael, > > > >> -Original Message- > >> From: Qiu, Michael > >> Sent: Friday, January 29, 2016 1:58 PM > >> To: dev at dpdk.org > >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, > >> Michael > >> Subject: [PATCH v2] ixgbe: Fix disable interrupt twice > >> > >> Currently, ixgbe vf and pf will disable interrupt twice in stop > >> stage and uninit stage. It will cause an error: > >> > >> testpmd> quit > >> > >> Shutting down port 0... > >> Stopping ports... > >> Done > >> Closing ports... > >> EAL: Error disabling MSI-X interrupts for fd 26 > >> Done > >> > >> Becasue the interrupt already been disabled in stop stage. > >> Since it is enabled in init stage, better remove from stop stage. > > I'm afraid it's not a good idea to just remove the intr_disable > > from > >> dev_stop. > > I think dev_stop have the chance to be used independently with > > dev_unint. In > this scenario, we still need intr_disable, right? > > Maybe what we need is some check before we disable the intr:) > Yes, indeed we need some check in disable intr, but it need > additional fields in "struct rte_intr_handle", and it's much saft > to do so, but as I check i40e/fm10k code, only ixgbe disable it in > dev_stop(). > >>> I found fm10k doesn't enable intr in dev_start. So, I think it's OK. > >>> But i40e > >> enables intr in dev_start. > >>> To my opinion, it's more like i40e misses the intr_disable in dev_stop. > >> I don't think i40e miss it, because it not the right please to disable > >> interrupt. > >> because all interrupts are enabled in init stage. > >> > >> Actually, ixgbe enable the interrupt in init stage, but in dev_start, > >> it disable it first and re-enable, so it just the same with doing nothing > >> about > interrupt. > >> > >> Just think below: > >> > >> 1. start the port.(interrupt already enabled in init stage, disable > >> --> > >> re-enable) > >> 2. stop the port.(disable interrupt) > >> 3. start port again(Try to disable, but failed, already disabled) > >> > >> Would you think the code has issue? > > [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls > > dev_close(), which calls dev_stop(). So I think the disabling can be done > > only in > dev_stop(). > > All others can make use of dev_stop to disable the interrupt. > > As I said, if it is in dev_stop, it will has issue when dev_start --> > dev_stop --> > dev_start, this also could applied in i40e and fm10k. If you want to put it in > dev_stop, better to remove enable interrupts in init stage, and only put it in > dev_start. Oh, yes, you are talking about the refactoring. That's good, and reasonable. Please do more validation with LSC, mailbox, rx interrupts, to make sure there is no issue introduced. Thanks, Helin > > Thanks, > Michael > > Regards, > > Helin > > > >> Thanks, > >> Michael > >> > >>> Maybe we can follow fm10k's style. > >>> > On other hand, if we remove it in dev_stop, any side effect? In > ixgbe start, it will always disable it first and then re-enable it, so > it's safe. > >>> I think you mean we can disable intr anyway even if it has been disabled. > >> Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable > >> interrupts, and if we try disable twice, it will return and error. > >> That's why I mean we need a flag to show the interrupts stats. If it > >> already disabled, we do not need call in to kernel. just return and > >> give a warning message. > >> > >> Thanks, > >> Michael > >> > >>> Sounds more like why we don't > >>> need this patch :) > >>> > Thanks, > Michael > >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
On 2/2/2016 10:14 AM, Zhang, Helin wrote: > >> -Original Message- >> From: Qiu, Michael >> Sent: Tuesday, February 2, 2016 10:07 AM >> To: Lu, Wenzhuo; dev at dpdk.org >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice >> >> [+cc helin] >> >> On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: >>> Hi Michael, >>> -Original Message- From: Qiu, Michael Sent: Monday, February 1, 2016 4:05 PM To: Lu, Wenzhuo; dev at dpdk.org Cc: Zhou, Danny; Liu, Yong; Liang, Cunming Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > Hi Michael, > >> -Original Message- >> From: Qiu, Michael >> Sent: Friday, January 29, 2016 1:58 PM >> To: dev at dpdk.org >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, >> Michael >> Subject: [PATCH v2] ixgbe: Fix disable interrupt twice >> >> Currently, ixgbe vf and pf will disable interrupt twice in stop >> stage and uninit stage. It will cause an error: >> >> testpmd> quit >> >> Shutting down port 0... >> Stopping ports... >> Done >> Closing ports... >> EAL: Error disabling MSI-X interrupts for fd 26 >> Done >> >> Becasue the interrupt already been disabled in stop stage. >> Since it is enabled in init stage, better remove from stop stage. > I'm afraid it?s not a good idea to just remove the intr_disable from >> dev_stop. > I think dev_stop have the chance to be used independently with > dev_unint. In this scenario, we still need intr_disable, right? > Maybe what we need is some check before we disable the intr:) Yes, indeed we need some check in disable intr, but it need additional fields in "struct rte_intr_handle", and it's much saft to do so, but as I check i40e/fm10k code, only ixgbe disable it in dev_stop(). >>> I found fm10k doesn?t enable intr in dev_start. So, I think it's OK. But >>> i40e >> enables intr in dev_start. >>> To my opinion, it's more like i40e misses the intr_disable in dev_stop. >> I don't think i40e miss it, because it not the right please to disable >> interrupt. >> because all interrupts are enabled in init stage. >> >> Actually, ixgbe enable the interrupt in init stage, but in dev_start, it >> disable it >> first and re-enable, so it just the same with doing nothing about interrupt. >> >> Just think below: >> >> 1. start the port.(interrupt already enabled in init stage, disable --> >> re-enable) >> 2. stop the port.(disable interrupt) >> 3. start port again(Try to disable, but failed, already disabled) >> >> Would you think the code has issue? > [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(), > which calls dev_stop(). So I think the disabling can be done only in > dev_stop(). > All others can make use of dev_stop to disable the interrupt. As I said, if it is in dev_stop, it will has issue when dev_start --> dev_stop --> dev_start, this also could applied in i40e and fm10k. If you want to put it in dev_stop, better to remove enable interrupts in init stage, and only put it in dev_start. Thanks, Michael > Regards, > Helin > >> Thanks, >> Michael >> >>> Maybe we can follow fm10k's style. >>> On other hand, if we remove it in dev_stop, any side effect? In ixgbe start, it will always disable it first and then re-enable it, so it's safe. >>> I think you mean we can disable intr anyway even if it has been disabled. >> Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, >> and >> if we try disable twice, it will return and error. >> That's why I mean we need a flag to show the interrupts stats. If it already >> disabled, we do not need call in to kernel. just return and give a warning >> message. >> >> Thanks, >> Michael >> >>> Sounds more like why we don't >>> need this patch :) >>> Thanks, Michael >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
Hi Michael, Acked-by: Wenzhuo Lu > -Original Message- > From: Qiu, Michael > Sent: Tuesday, February 2, 2016 10:07 AM > To: Lu, Wenzhuo; dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin > Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > > [+cc helin] > > On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: > > Hi Michael, > > > >> -Original Message- > >> From: Qiu, Michael > >> Sent: Monday, February 1, 2016 4:05 PM > >> To: Lu, Wenzhuo; dev at dpdk.org > >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming > >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > >> > >> On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > >>> Hi Michael, > >>> > -Original Message- > From: Qiu, Michael > Sent: Friday, January 29, 2016 1:58 PM > To: dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, > Michael > Subject: [PATCH v2] ixgbe: Fix disable interrupt twice > > Currently, ixgbe vf and pf will disable interrupt twice in stop > stage and uninit stage. It will cause an error: > > testpmd> quit > > Shutting down port 0... > Stopping ports... > Done > Closing ports... > EAL: Error disabling MSI-X interrupts for fd 26 > Done > > Becasue the interrupt already been disabled in stop stage. > Since it is enabled in init stage, better remove from stop stage. > >>> I'm afraid it's not a good idea to just remove the intr_disable from > >>> dev_stop. > >>> I think dev_stop have the chance to be used independently with > >>> dev_unint. In > >> this scenario, we still need intr_disable, right? > >>> Maybe what we need is some check before we disable the intr:) > >> Yes, indeed we need some check in disable intr, but it need > >> additional fields in "struct rte_intr_handle", and it's much saft to > >> do so, but as I check i40e/fm10k code, only ixgbe disable it in dev_stop(). > > I found fm10k doesn't enable intr in dev_start. So, I think it's OK. But > > i40e > enables intr in dev_start. > > To my opinion, it's more like i40e misses the intr_disable in dev_stop. > > I don't think i40e miss it, because it not the right please to disable > interrupt. > because all interrupts are enabled in init stage. > > Actually, ixgbe enable the interrupt in init stage, but in dev_start, it > disable it first > and re-enable, so it just the same with doing nothing about interrupt. > > Just think below: > > 1. start the port.(interrupt already enabled in init stage, disable --> > re-enable) > 2. stop the port.(disable interrupt) > 3. start port again(Try to disable, but failed, already disabled) > > Would you think the code has issue? Got your point. So, dev_start/stop will not influence the state of intr enabling/disabling. The intr will be enabled/disabled during dev_init/unint. Thanks. > > Thanks, > Michael > > > Maybe we can follow fm10k's style. > > > >> On other hand, if we remove it in dev_stop, any side effect? In ixgbe > >> start, it will always disable it first and then re-enable it, so it's safe. > > I think you mean we can disable intr anyway even if it has been disabled. > > Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, > and if > we try disable twice, it will return and error. > That's why I mean we need a flag to show the interrupts stats. If it already > disabled, we do not need call in to kernel. just return and give a warning > message. > > Thanks, > Michael > > > Sounds more like why we don't > > need this patch :) > > > >> Thanks, > >> Michael > >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
> -Original Message- > From: Qiu, Michael > Sent: Tuesday, February 2, 2016 10:07 AM > To: Lu, Wenzhuo; dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Zhang, Helin > Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > > [+cc helin] > > On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: > > Hi Michael, > > > >> -Original Message- > >> From: Qiu, Michael > >> Sent: Monday, February 1, 2016 4:05 PM > >> To: Lu, Wenzhuo; dev at dpdk.org > >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming > >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > >> > >> On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > >>> Hi Michael, > >>> > -Original Message- > From: Qiu, Michael > Sent: Friday, January 29, 2016 1:58 PM > To: dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, > Michael > Subject: [PATCH v2] ixgbe: Fix disable interrupt twice > > Currently, ixgbe vf and pf will disable interrupt twice in stop > stage and uninit stage. It will cause an error: > > testpmd> quit > > Shutting down port 0... > Stopping ports... > Done > Closing ports... > EAL: Error disabling MSI-X interrupts for fd 26 > Done > > Becasue the interrupt already been disabled in stop stage. > Since it is enabled in init stage, better remove from stop stage. > >>> I'm afraid it's not a good idea to just remove the intr_disable from > dev_stop. > >>> I think dev_stop have the chance to be used independently with > >>> dev_unint. In > >> this scenario, we still need intr_disable, right? > >>> Maybe what we need is some check before we disable the intr:) > >> Yes, indeed we need some check in disable intr, but it need > >> additional fields in "struct rte_intr_handle", and it's much saft to > >> do so, but as I check i40e/fm10k code, only ixgbe disable it in dev_stop(). > > I found fm10k doesn't enable intr in dev_start. So, I think it's OK. But > > i40e > enables intr in dev_start. > > To my opinion, it's more like i40e misses the intr_disable in dev_stop. > > I don't think i40e miss it, because it not the right please to disable > interrupt. > because all interrupts are enabled in init stage. > > Actually, ixgbe enable the interrupt in init stage, but in dev_start, it > disable it > first and re-enable, so it just the same with doing nothing about interrupt. > > Just think below: > > 1. start the port.(interrupt already enabled in init stage, disable --> > re-enable) > 2. stop the port.(disable interrupt) > 3. start port again(Try to disable, but failed, already disabled) > > Would you think the code has issue? [Zhang, Helin] in ixgbe PMD, it can be seen that uninit() calls dev_close(), which calls dev_stop(). So I think the disabling can be done only in dev_stop(). All others can make use of dev_stop to disable the interrupt. Regards, Helin > > Thanks, > Michael > > > Maybe we can follow fm10k's style. > > > >> On other hand, if we remove it in dev_stop, any side effect? In ixgbe > >> start, it will always disable it first and then re-enable it, so it's safe. > > I think you mean we can disable intr anyway even if it has been disabled. > > Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, > and > if we try disable twice, it will return and error. > That's why I mean we need a flag to show the interrupts stats. If it already > disabled, we do not need call in to kernel. just return and give a warning > message. > > Thanks, > Michael > > > Sounds more like why we don't > > need this patch :) > > > >> Thanks, > >> Michael > >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
[+cc helin] On 2/2/2016 9:03 AM, Lu, Wenzhuo wrote: > Hi Michael, > >> -Original Message- >> From: Qiu, Michael >> Sent: Monday, February 1, 2016 4:05 PM >> To: Lu, Wenzhuo; dev at dpdk.org >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming >> Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice >> >> On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: >>> Hi Michael, >>> -Original Message- From: Qiu, Michael Sent: Friday, January 29, 2016 1:58 PM To: dev at dpdk.org Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, Michael Subject: [PATCH v2] ixgbe: Fix disable interrupt twice Currently, ixgbe vf and pf will disable interrupt twice in stop stage and uninit stage. It will cause an error: testpmd> quit Shutting down port 0... Stopping ports... Done Closing ports... EAL: Error disabling MSI-X interrupts for fd 26 Done Becasue the interrupt already been disabled in stop stage. Since it is enabled in init stage, better remove from stop stage. >>> I'm afraid it?s not a good idea to just remove the intr_disable from >>> dev_stop. >>> I think dev_stop have the chance to be used independently with dev_unint. In >> this scenario, we still need intr_disable, right? >>> Maybe what we need is some check before we disable the intr:) >> Yes, indeed we need some check in disable intr, but it need additional >> fields in >> "struct rte_intr_handle", and it's much saft to do so, but as I check >> i40e/fm10k >> code, only ixgbe disable it in dev_stop(). > I found fm10k doesn?t enable intr in dev_start. So, I think it's OK. But i40e > enables intr in dev_start. > To my opinion, it's more like i40e misses the intr_disable in dev_stop. I don't think i40e miss it, because it not the right please to disable interrupt. because all interrupts are enabled in init stage. Actually, ixgbe enable the interrupt in init stage, but in dev_start, it disable it first and re-enable, so it just the same with doing nothing about interrupt. Just think below: 1. start the port.(interrupt already enabled in init stage, disable --> re-enable) 2. stop the port.(disable interrupt) 3. start port again(Try to disable, but failed, already disabled) Would you think the code has issue? Thanks, Michael > Maybe we can follow fm10k's style. > >> On other hand, if we remove it in dev_stop, any side effect? In ixgbe start, >> it will >> always disable it first and then re-enable it, so it's safe. > I think you mean we can disable intr anyway even if it has been disabled. Actually, we couldn't, DPDK call VFIO ioctl to kernel to disable interrupts, and if we try disable twice, it will return and error. That's why I mean we need a flag to show the interrupts stats. If it already disabled, we do not need call in to kernel. just return and give a warning message. Thanks, Michael > Sounds more like why we don't > need this patch :) > >> Thanks, >> Michael >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
Hi Michael, > -Original Message- > From: Qiu, Michael > Sent: Monday, February 1, 2016 4:05 PM > To: Lu, Wenzhuo; dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming > Subject: Re: [PATCH v2] ixgbe: Fix disable interrupt twice > > On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > > Hi Michael, > > > >> -Original Message- > >> From: Qiu, Michael > >> Sent: Friday, January 29, 2016 1:58 PM > >> To: dev at dpdk.org > >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, Michael > >> Subject: [PATCH v2] ixgbe: Fix disable interrupt twice > >> > >> Currently, ixgbe vf and pf will disable interrupt twice in stop stage > >> and uninit stage. It will cause an error: > >> > >> testpmd> quit > >> > >> Shutting down port 0... > >> Stopping ports... > >> Done > >> Closing ports... > >> EAL: Error disabling MSI-X interrupts for fd 26 > >> Done > >> > >> Becasue the interrupt already been disabled in stop stage. > >> Since it is enabled in init stage, better remove from stop stage. > > I'm afraid it's not a good idea to just remove the intr_disable from > > dev_stop. > > I think dev_stop have the chance to be used independently with dev_unint. In > this scenario, we still need intr_disable, right? > > Maybe what we need is some check before we disable the intr:) > > Yes, indeed we need some check in disable intr, but it need additional fields > in > "struct rte_intr_handle", and it's much saft to do so, but as I check > i40e/fm10k > code, only ixgbe disable it in dev_stop(). I found fm10k doesn't enable intr in dev_start. So, I think it's OK. But i40e enables intr in dev_start. To my opinion, it's more like i40e misses the intr_disable in dev_stop. Maybe we can follow fm10k's style. > > On other hand, if we remove it in dev_stop, any side effect? In ixgbe start, > it will > always disable it first and then re-enable it, so it's safe. I think you mean we can disable intr anyway even if it has been disabled. Sounds more like why we don't need this patch :) > > Thanks, > Michael > >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
On 1/29/2016 4:07 PM, Lu, Wenzhuo wrote: > Hi Michael, > >> -Original Message- >> From: Qiu, Michael >> Sent: Friday, January 29, 2016 1:58 PM >> To: dev at dpdk.org >> Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, Michael >> Subject: [PATCH v2] ixgbe: Fix disable interrupt twice >> >> Currently, ixgbe vf and pf will disable interrupt twice in stop stage and >> uninit >> stage. It will cause an error: >> >> testpmd> quit >> >> Shutting down port 0... >> Stopping ports... >> Done >> Closing ports... >> EAL: Error disabling MSI-X interrupts for fd 26 >> Done >> >> Becasue the interrupt already been disabled in stop stage. >> Since it is enabled in init stage, better remove from stop stage. > I'm afraid it?s not a good idea to just remove the intr_disable from dev_stop. > I think dev_stop have the chance to be used independently with dev_unint. In > this scenario, we still need intr_disable, right? > Maybe what we need is some check before we disable the intr:) Yes, indeed we need some check in disable intr, but it need additional fields in "struct rte_intr_handle", and it's much saft to do so, but as I check i40e/fm10k code, only ixgbe disable it in dev_stop(). On other hand, if we remove it in dev_stop, any side effect? In ixgbe start, it will always disable it first and then re-enable it, so it's safe. Thanks, Michael >
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
Currently, ixgbe vf and pf will disable interrupt twice in
stop stage and uninit stage. It will cause an error:
testpmd> quit
Shutting down port 0...
Stopping ports...
Done
Closing ports...
EAL: Error disabling MSI-X interrupts for fd 26
Done
Becasue the interrupt already been disabled in stop stage.
Since it is enabled in init stage, better remove from
stop stage.
Fixes: 0eb609239efd ("ixgbe: enable Rx queue interrupts for PF and VF")
Signed-off-by: Michael Qiu
---
v2 --> v1:
fix error in commit log word "interrupte"
drivers/net/ixgbe/ixgbe_ethdev.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 4c4c6df..a561f8d 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2192,9 +2192,6 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
/* disable interrupts */
ixgbe_disable_intr(hw);
- /* disable intr eventfd mapping */
- rte_intr_disable(intr_handle);
-
/* reset the NIC */
ixgbe_pf_reset_hw(hw);
hw->adapter_stopped = 0;
@@ -3898,9 +3895,6 @@ ixgbevf_dev_stop(struct rte_eth_dev *dev)
ixgbe_dev_clear_queues(dev);
- /* disable intr eventfd mapping */
- rte_intr_disable(intr_handle);
-
/* Clean datapath event and queue/vec mapping */
rte_intr_efd_disable(intr_handle);
if (intr_handle->intr_vec != NULL) {
--
1.9.3
[dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
Hi Michael, > -Original Message- > From: Qiu, Michael > Sent: Friday, January 29, 2016 1:58 PM > To: dev at dpdk.org > Cc: Zhou, Danny; Liu, Yong; Liang, Cunming; Lu, Wenzhuo; Qiu, Michael > Subject: [PATCH v2] ixgbe: Fix disable interrupt twice > > Currently, ixgbe vf and pf will disable interrupt twice in stop stage and > uninit > stage. It will cause an error: > > testpmd> quit > > Shutting down port 0... > Stopping ports... > Done > Closing ports... > EAL: Error disabling MSI-X interrupts for fd 26 > Done > > Becasue the interrupt already been disabled in stop stage. > Since it is enabled in init stage, better remove from stop stage. I'm afraid it's not a good idea to just remove the intr_disable from dev_stop. I think dev_stop have the chance to be used independently with dev_unint. In this scenario, we still need intr_disable, right? Maybe what we need is some check before we disable the intr:)
