DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
To: "Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>,
	Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>,
	David Marchand <david.marchand@redhat.com>,
	"Thomas Monjalon" <thomas@monjalon.net>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	"Bruce Richardson" <bruce.richardson@intel.com>
Cc: "John Daley (johndale)" <johndale@cisco.com>,
	Shahed Shaikh <shshaikh@marvell.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [RFC PATCH v3 2/3] eal: add mask and unmask interrupt APIs
Date: Wed, 17 Jul 2019 08:03:09 +0000	[thread overview]
Message-ID: <BYAPR18MB2424ED7AEB7544B114409F61C8C90@BYAPR18MB2424.namprd18.prod.outlook.com> (raw)
In-Reply-To: <MWHPR11MB1839FDD2957536FA5DDC2BF5BFC90@MWHPR11MB1839.namprd11.prod.outlook.com>

> > > -----Original Message-----
> > > From: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> > > Sent: Wednesday, July 17, 2019 11:26 AM
> > > To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; David
> > Marchand
> > > <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>;
> > > Ferruh Yigit <ferruh.yigit@intel.com>; Bruce Richardson
> > > <bruce.richardson@intel.com>
> > > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; John Daley
> > > (johndale) <johndale@cisco.com>; Shahed Shaikh
> > > <shshaikh@marvell.com>; dev@dpdk.org
> > > Subject: RE: [RFC PATCH v3 2/3] eal: add mask and unmask interrupt
> > > APIs
> > > > +rte_intr_mask(const struct rte_intr_handle *intr_handle) {
> > > > +	if (intr_handle && intr_handle->type == RTE_INTR_HANDLE_VDEV)
> > > > +		return 0;
> > > > +
> > > > +	if (!intr_handle || intr_handle->fd < 0 ||
> > > > +intr_handle->uio_cfg_fd <
> > > 0)
> > > > +		return -1;
> > > > +
> > > > +	switch (intr_handle->type){
> > > > +	/* Both masking and disabling are same for UIO */
> > > > +	case RTE_INTR_HANDLE_UIO:
> > > > +		if (uio_intr_disable(intr_handle))
> > > > +			return -1;
> > > > +		break;
> > > > +	case RTE_INTR_HANDLE_UIO_INTX:
> > > > +		if (uio_intx_intr_disable(intr_handle))
> > > > +			return -1;
> > > > +		break;
> > > > +	/* not used at this moment */
> > > > +	case RTE_INTR_HANDLE_ALARM:
> > > > +		return -1;
> > > > +#ifdef VFIO_PRESENT
> > > > +	case RTE_INTR_HANDLE_VFIO_MSIX:
> > > > +	case RTE_INTR_HANDLE_VFIO_MSI:
> > > > +		return 0;
> > >
> > > Isn't this a little confusing? It returns success, but irq is not masked.
> >
> > Yes. How about changing the API to rte_intr_ack()(Acknowledge the
> > interrupt)
> > Or something similar? i.e replace rte_intr_unmask() with
> > rte_intr_ack() for this use case.
> >
> 
> Not sure. I do not have a good suggestion here :-) Like to hear from
> David when he comes back, as he spent most time on this issue..

Sure. He is on vacation.
Any reason for thinking, rte_intr_ack()  may not be semantically correct?
I think, it is very much correct if there are no better suggestions.
Anyway it the name, From VFIO perspective, we know what is expected so I think it is fine.

> 
> Why not return -1 and let the caller deal with it?

If we make it as rte_intr_ack() no need to return -1 for MSIX-VFIO+Linux
as it is semantically correct.

> 
> Optimist view:
> Maintainers will see the error as vfio-pci + MSI/MSI_X is on
> everyone's test list. And it forces them to confront the issue. Do I
> really need unmask here, etc.

If we make it as ack then it fine as driver does not need to know the fine 
details.

> 
> Pessimist view:
> Wastes a lot of people's time. Potentially duplicate code like this
> everywhere.
> 
>   if (INTx) unmask();
> 
> BTW, are you targeting 19.08 or 19.11? Not sure how much change we can
> tolerate in 19.08.


19.08 as fundamentally it correct. Finer adjustment can made by existing
drivers if required in the testing phase.

It is trivial change as scope is limited to interrupt hander rte_intr_enable()
replacement with rte_intr_ack(). For MSIX case, it should be real NOP,
so I don't think there issue. It should be much better than the existing
state, where almost everything broken.

> Requirements for 19.08 seem to be...
> - Must fix the redhat bz (lost interrupt issue with qede + MSI/MSI-X)
> - Fix potentially similar issues in other drivers too?

Proposed patch will fix the above mentioned issues.

> 
> Thanks..
> -Hyong
> 
> > > As is, return code 0 means...
> > > - igb_uio: irq is masked for INTx, MSI, MSI-X
> > > - vfio-pci + INTx: irq is masked
> > > - vfio-pci + MSI/MSI-X: no changes
> > >
> > > Masking is useful only for INTx, IMO...
> > >
> > > Masking MSI/MSI-X via PCI-defined mechanisms (e.g. Mask bit in MSI-X
> > > Table) has no practical use for drivers. Handshaking/masking/unmasking
> is
> > > done via device/vendor specific ways, as needed. See all those
> > > ack/block/unblock/credit/... mechanisms used in various drivers/NICs to
> > > control interrupts their own way.
> > >
> > > A long time ago in early PCIe days, the linux kernel did auto-masking for
> > > MSI/MSI-X (i.e. mask before calling netdev irq handler). It was soon
> > removed
> > > as it was unnecessary overhead (expensive PIOs to NIC for every
> interrupt).
> > > Windows and FreeBSD do not do auto-masking either.
> >
> > rte_intr_ack() can abstract FreeBSD and Windows difference.
> >


  reply	other threads:[~2019-07-17  8:03 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-15 16:50 [dpdk-dev] [RFC PATCH] vfio: avoid re-installing irq handler 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
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 [this message]
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

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=BYAPR18MB2424ED7AEB7544B114409F61C8C90@BYAPR18MB2424.namprd18.prod.outlook.com \
    --to=jerinj@marvell.com \
    --cc=bruce.richardson@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).