DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Qiu, Michael" <michael.qiu@intel.com>
To: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: Fix disable interrupt twice
Date: Tue, 2 Feb 2016 02:06:55 +0000	[thread overview]
Message-ID: <533710CFB86FA344BFBF2D6802E6028622F28A4D@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC0909034266D1@shsmsx102.ccr.corp.intel.com>

[+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@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@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
>


  reply	other threads:[~2016-02-02  2:07 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-29  5:51 [dpdk-dev] [PATCH] " Michael Qiu
2016-01-29  5:58 ` [dpdk-dev] [PATCH v2] " Michael Qiu
2016-01-29  8:07   ` Lu, Wenzhuo
2016-02-01  8:05     ` Qiu, Michael
2016-02-02  1:03       ` Lu, Wenzhuo
2016-02-02  2:06         ` Qiu, Michael [this message]
2016-02-02  2:14           ` Zhang, Helin
2016-02-02  2:57             ` Qiu, Michael
2016-02-02  3:07               ` Zhang, Helin
2016-02-02  3:15                 ` Qiu, Michael
2016-02-02 11:03               ` Ananyev, Konstantin
2016-02-19  8:07                 ` Qiu, Michael
2016-02-19 15:14                   ` Ananyev, Konstantin
2016-02-02  2:26           ` Lu, Wenzhuo
2016-02-23  2:10   ` Zhang, Helin
2016-02-26 14:39     ` Bruce Richardson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=533710CFB86FA344BFBF2D6802E6028622F28A4D@SHSMSX101.ccr.corp.intel.com \
    --to=michael.qiu@intel.com \
    --cc=dev@dpdk.org \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).