Re: [dpdk-dev] [PATCH v3 2/3] eal: add ack interrupt API

From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: Nithin Dabilpuram <ndabilpuram@marvell.com>,
	Hyong Youb Kim <hyonkim@cisco.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: jerinj@marvell.com, John Daley <johndale@cisco.com>,
	Shahed Shaikh <shshaikh@marvell.com>,
	dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3 2/3] eal: add ack interrupt API
Date: Thu, 18 Jul 2019 14:13:18 +0100	[thread overview]
Message-ID: <b65d019d-a0ef-adea-8217-af4664b4544a@intel.com> (raw)
In-Reply-To: <20190718084628.134290-3-ndabilpuram@marvell.com>

On 18-Jul-19 9:46 AM, Nithin Dabilpuram wrote:
> Add new ack interrupt API to avoid using
> VFIO_IRQ_SET_ACTION_TRIGGER(rte_intr_enable()) for
> acking interrupt purpose for VFIO based interrupt handlers.
> This implementation is specific to Linux.
> 
> Using rte_intr_enable() for acking interrupt has below issues
> 
>   * Time consuming to do for every interrupt received as it will
>     free_irq() followed by request_irq() and all other initializations
>   * A race condition because of a window between free_irq() and
>     request_irq() with packet reception still on and device still
>     enabled and would throw warning messages like below.
>     [158764.159833] do_IRQ: 9.34 No irq handler for vector
> 
> In this patch, rte_intr_ack() is a no-op for VFIO_MSIX/VFIO_MSI interrupts
> as they are edge triggered and kernel would not mask the interrupt before
> delivering the event to userspace and we don't need to ack.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Tested-by: Shahed Shaikh <shshaikh@marvell.com>
> ---

<snip>

>    */
>   int rte_intr_disable(const struct rte_intr_handle *intr_handle);
>   
> +/**
> + * It acks an interrupt raised for the specified handle.
> + *
> + * Call this function to ack an interrupt from interrupt
> + * handler either from application or driver, so that
> + * new interrupts are raised.
> + *
> + * @note PMD generally calls this function at the end of its IRQ callback.
> + *    Internally, it unmasks the interrupt if possible. For INTx, unmasking
> + *    is required as the interrupt is auto-masked prior to invoking
> + *    callback. For MSI/MSI-X, unmasking is typically not needed as the
> + *    interrupt is not auto-masked. In fact, for interrupt handle types
> + *    VFIO_MSIX and VFIO_MSI, this function is no-op.

I've added this comment in previous revision's discussion, but i'll copy 
it here as well.

Does the user of this API even cares about these details? I would think 
that it would be easier to just mandate calling this function at the end 
of each interrupt callback, regardless of which interrupt mode is used.

IMO internal details (about no-ops and such) are better explained in the 
implementation, not in the public-facing API header.

-- 
Thanks,
Anatoly