From: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
To: "Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>,
David Marchand <david.marchand@redhat.com>,
Thomas Monjalon <thomas@monjalon.net>,
"Ferruh Yigit" <ferruh.yigit@intel.com>,
Alejandro Lucero <alejandro.lucero@netronome.com>,
Anatoly Burakov <anatoly.burakov@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"John Daley (johndale)" <johndale@cisco.com>,
Shahed Shaikh <shshaikh@marvell.com>,
"Nithin Kumar Dabilpuram" <ndabilpuram@marvell.com>
Subject: Re: [dpdk-dev] [RFC PATCH] vfio: avoid re-installing irq handler
Date: Tue, 16 Jul 2019 09:56:12 +0000 [thread overview]
Message-ID: <BYAPR18MB242493E031B8418394E507FAC8CE0@BYAPR18MB2424.namprd18.prod.outlook.com> (raw)
In-Reply-To: <MWHPR11MB1839BA8141F4822C2CAAB603BFCE0@MWHPR11MB1839.namprd11.prod.outlook.com>
> -----Original Message-----
> From: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Sent: Tuesday, July 16, 2019 1:19 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; David Marchand
> <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Alejandro
> Lucero <alejandro.lucero@netronome.com>; Anatoly Burakov
> <anatoly.burakov@intel.com>
> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Shahed
> Shaikh <shshaikh@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>
> Subject: RE: [RFC PATCH] vfio: avoid re-installing irq handler
>
> > -----Original Message-----
> > From: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> [...]
> > > > > A rough patch for the approach mentioned earlier. It is only for
> > > discussion.
> > > > > http://mails.dpdk.org/archives/dev/2019-July/138113.html
> > > > >
> > > > > To try this out, first revert the following then apply.
> > > > > commit 89aac60e0be9 ("vfio: fix interrupts race condition")
> > > >
> > > > Yes. This patch has to be to reverted. It changes the existing
> > > > interrupt behavior and does not address the MSIX case as well.
> > > >
> > > > I think, The clean fix would be to introduce rte_intr_mask() and
> > > > rte_intr_unmask() by abstracting the INTX and MSIX differences And
> > > > let qede driver call it as needed.
> > > >
> > > > Thoughts?
> > >
> > > Hi,
> >
> > Hi Hyong,
> >
> > >
> > > You are proposing these?
> > > - Add rte_intr_mask_intx, rte_intr_unmask_intx.
> > > No APIs for masking MSI/MSI-X as vfio-pci does not support that.
> > > - Modify PMD irq handlers to use rte_intr_unmask_intx as necessary.
> >
> > No, introduce the rte_intr_mask() and rte_intr_unmask().
> > For MSIX + Linux VFIO, That API can return -ENOSUP as Linux VFIO+MSIX
> > is not supporting.
> > Another platform/eal may support it.
> >
>
> These generic names would invite people to use API, only to see it fail, since
> it only works with INTx..
It works for all non VFIO MSIx case. VFIO MSIx case it NOP(Yes, No need to return error in that case)
>
> > Mask and unmask is operation is known to all IRQ controllers.
> > So, IMO, As far as abstraction is concerned it will be good fit.
> >
> > > That might be too intrusive. And too much work for the sake of INTx..
> > > Anyone really using/needing INTx these days? :-)
> >
> > Yup. Mask needs to called only for only qede INTx. Looks like qede Has
> > MSIX and INTX separate handler. So this mask can go to qede INTx
> >
> > >
> > > The following drivers call rte_intr_enable from their irq handlers.
> > > So with explicit rte_intr_unmask_intx, all these would need to do
> > > "if using intx, unmask"?
> > >
> > > atlantic, avp, axgbe, bnx2x, e1000, fm10k, ice, ixgbe, nfp, qede,
> > > sfc,
> > > vmxnet3
> >
> > No change on these PMDs.
> >
>
> Why is that?
>
> These drivers potentially have the same "lost" interrupt issue mentioned in
> the original redhat bz (qede + MSI). I *think* this observation led David to
> address them all through vfio changes, rather than fixing qede alone.
>
> You want to introduce unmask API and use it only for qede in this cycle, and
> ask respective maintainers to fix their drivers in 19.11?
Changing the rte_intr_enable() to rte_intr_unmask() is trivial on the places
Where existing drivers enable as unmask.
If we understand it correctly:
In case of non VFIO MSIX(INTx) and UIO
-------------------------------------------------
AK1) Kernel receives interrupt
AK2) Kernel _mask_ the interrupt
AK3) Kernel notify the use space
On usersapce:
AU1) Driver specific interrupt handler invoked
AU2) Handle the driver specific interrupt
AU3) Call rte_intr_enable(), it will intern call VFIO_IRQ_SET_ACTION_UNMASK using VFIO_DEVICE_SET_IRQS to unmask the interrupt.
In case of VFIO MSIX(INTx)
------------------------------------
BK1) Kernel receives interrupt.
BK2) Kernel notify the use space
On usersapce:
BU1) Driver specific interrupt handler invoked
BU2) Handle the driver specific interrupt
BU3) Call rte_intr_enable(), it will intern call VFIO_IRQ_SET_ACTION_TRIGGER using VFIO_DEVICE_SET_IRQS to unmask the interrupt.
VFIO_IRQ_SET_ACTION_TRIGGER: is the nasty one, it will free the existing interrupt handler and request new handler etc.
Which is the source of the actual race conditional problem.
Ideally BU3 can be just NOP. Since we need to keep the same interrupt handler for both UIO and MSIX(I *think*)
DPDK tends to use rte_intr_enable() which can work for AU3/BU3 as well.
So we need,
A light weight primitive, which unmask the AK2 incase of VFIO INTx by not overriding
The meaning of normal rte_intr_enable() which suppose not use for MSIX interrupt in action due to racy behavior of VFIO_IRQ_SET_ACTION_TRIGGER
Replacing AU3 and BU3 as rte_intr_unmask() would fix problem. Where rte_intr_unmask() for BU3 is just NOP.
next prev parent reply other threads:[~2019-07-16 9:57 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-15 16:50 Jerin Jacob Kollanukkaran
2019-07-16 5:58 ` Hyong Youb Kim (hyonkim)
2019-07-16 6:47 ` Jerin Jacob Kollanukkaran
2019-07-16 7:49 ` Hyong Youb Kim (hyonkim)
2019-07-16 9:56 ` Jerin Jacob Kollanukkaran [this message]
2019-07-16 6:46 ` [dpdk-dev] [RFC PATCH] eal: add mask and unmask interrupt apis Nithin Dabilpuram
2019-07-16 7:01 ` [dpdk-dev] [RFC PATCH v2] " Nithin Dabilpuram
2019-07-16 16:44 ` [dpdk-dev] [RFC PATCH v3 1/3] vfio: revert change that does intr eventfd setup at probe Nithin Dabilpuram
2019-07-16 16:44 ` [dpdk-dev] [RFC PATCH v3 2/3] eal: add mask and unmask interrupt APIs Nithin Dabilpuram
2019-07-17 5:55 ` Hyong Youb Kim (hyonkim)
2019-07-17 6:14 ` Jerin Jacob Kollanukkaran
2019-07-17 7:09 ` Hyong Youb Kim (hyonkim)
2019-07-17 8:03 ` Jerin Jacob Kollanukkaran
2019-07-17 8:45 ` Hyong Youb Kim (hyonkim)
2019-07-17 9:20 ` Jerin Jacob Kollanukkaran
2019-07-17 9:51 ` Hyong Youb Kim (hyonkim)
2019-07-17 10:43 ` Jerin Jacob Kollanukkaran
2019-07-17 11:06 ` Hyong Youb Kim (hyonkim)
2019-07-17 11:16 ` Jerin Jacob Kollanukkaran
2019-07-17 12:04 ` Nithin Kumar Dabilpuram
2019-07-16 16:44 ` [dpdk-dev] [RFC PATCH v3 3/3] drivers/net: use unmask API in interrupt handlers Nithin Dabilpuram
2019-07-17 6:01 ` Hyong Youb Kim (hyonkim)
2019-07-17 7:47 ` Nithin Kumar Dabilpuram
2019-07-16 20:06 ` [dpdk-dev] [RFC PATCH v3 1/3] vfio: revert change that does intr eventfd setup at probe Stephen Hemminger
-- strict thread matches above, loose matches on Subject: below --
2019-07-15 15:58 [dpdk-dev] [RFC PATCH] vfio: avoid re-installing irq handler Hyong Youb Kim
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=BYAPR18MB242493E031B8418394E507FAC8CE0@BYAPR18MB2424.namprd18.prod.outlook.com \
--to=jerinj@marvell.com \
--cc=alejandro.lucero@netronome.com \
--cc=anatoly.burakov@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=hyonkim@cisco.com \
--cc=johndale@cisco.com \
--cc=ndabilpuram@marvell.com \
--cc=shshaikh@marvell.com \
--cc=thomas@monjalon.net \
/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).