Re: [dpdk-dev] [RFC PATCH v3 2/3] eal: add mask and unmask interrupt APIs

[Jerin Jacob Kollanukkaran <jerinj@marvell.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.
> >