diff mbox

[dpdk-dev] eal: fix max number of interrupt request

Message ID 1486670383-5286-1-git-send-email-qi.z.zhang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Qi Zhang Feb. 9, 2017, 7:59 p.m. UTC
The max number of interrupt request is possible
be changed after rte_intr_callback_register, so
in get_max_intr, we need to check if nessesary to
update the max_intr.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Thomas Monjalon Feb. 10, 2017, 10:18 a.m. UTC | #1
2017-02-09 14:59, Qi Zhang:
> The max number of interrupt request is possible
> be changed after rte_intr_callback_register, so
> in get_max_intr, we need to check if nessesary to
> update the max_intr.

So you are using rte_intr_enable() to update the max_intr field
in the case of VFIO_MSIX.
What about MSI, INTX and UIO cases?
Qi Zhang Feb. 13, 2017, 1:16 a.m. UTC | #2
Hi Thomas:

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 10, 2017 6:19 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [PATCH] eal: fix max number of interrupt request
> 
> 2017-02-09 14:59, Qi Zhang:
> > The max number of interrupt request is possible be changed after
> > rte_intr_callback_register, so in get_max_intr, we need to check if
> > nessesary to update the max_intr.
> 
> So you are using rte_intr_enable() to update the max_intr field in the case of
> VFIO_MSIX.
> What about MSI, INTX and UIO cases?

My thought is, even without my fix, VFIO_MSIX is already the only case that try to modify max_intr field 
In get_max_intr, we have:
				if (!src->intr_handle.max_intr)
                        src->intr_handle.max_intr = 1;
                else if (src->intr_handle.max_intr > RTE_MAX_RXTX_INTR_VEC_ID)
                        src->intr_handle.max_intr
                                = RTE_MAX_RXTX_INTR_VEC_ID + 1;
So my patch just follow this and fix some problem.

Another option is I can use a local variable that assigned by max_intr with boundary check, so get_max_intr can be totally removed and max_intr in intr_source will not be modified.

To me both fix are not perfect, I think the problem is in rte_intr_callback_register we just save a copy of the pci_dev->intr_handle but not the address point, so we are missing some mechanism to sync them.
But since we have tight schedule on the 17.02 release and this issue does cause some example code can't work, so we need to a fix it first, we may consider improve the mechanism later.

Thanks
Qi
Thomas Monjalon Feb. 13, 2017, 9:28 p.m. UTC | #3
2017-02-13 01:16, Zhang, Qi Z:
> Hi Thomas:
> 
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2017-02-09 14:59, Qi Zhang:
> > > The max number of interrupt request is possible be changed after
> > > rte_intr_callback_register, so in get_max_intr, we need to check if
> > > nessesary to update the max_intr.
> > 
> > So you are using rte_intr_enable() to update the max_intr field in the case of
> > VFIO_MSIX.
> > What about MSI, INTX and UIO cases?
> 
> My thought is, even without my fix, VFIO_MSIX is already the only case that try to modify max_intr field 
> In get_max_intr, we have:
> 				if (!src->intr_handle.max_intr)
>                         src->intr_handle.max_intr = 1;
>                 else if (src->intr_handle.max_intr > RTE_MAX_RXTX_INTR_VEC_ID)
>                         src->intr_handle.max_intr
>                                 = RTE_MAX_RXTX_INTR_VEC_ID + 1;
> So my patch just follow this and fix some problem.
> 
> Another option is I can use a local variable that assigned by max_intr with boundary check, so get_max_intr can be totally removed and max_intr in intr_source will not be modified.
> 
> To me both fix are not perfect, I think the problem is in rte_intr_callback_register we just save a copy of the pci_dev->intr_handle but not the address point, so we are missing some mechanism to sync them.
> But since we have tight schedule on the 17.02 release and this issue does cause some example code can't work, so we need to a fix it first, we may consider improve the mechanism later.
> 
> Thanks
> Qi

Applied with this title: "vfio: fix maximum number of interrupt for MSI-X"

Please check how to document this behaviour and make it consistent
with other types of interrupts.
Qi Zhang Feb. 14, 2017, 12:13 a.m. UTC | #4
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, February 14, 2017 5:29 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [PATCH] eal: fix max number of interrupt request
> 
> 2017-02-13 01:16, Zhang, Qi Z:
> > Hi Thomas:
> >
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2017-02-09 14:59, Qi Zhang:
> > > > The max number of interrupt request is possible be changed after
> > > > rte_intr_callback_register, so in get_max_intr, we need to check
> > > > if nessesary to update the max_intr.
> > >
> > > So you are using rte_intr_enable() to update the max_intr field in
> > > the case of VFIO_MSIX.
> > > What about MSI, INTX and UIO cases?
> >
> > My thought is, even without my fix, VFIO_MSIX is already the only case
> > that try to modify max_intr field In get_max_intr, we have:
> > 				if (!src->intr_handle.max_intr)
> >                         src->intr_handle.max_intr = 1;
> >                 else if (src->intr_handle.max_intr >
> RTE_MAX_RXTX_INTR_VEC_ID)
> >                         src->intr_handle.max_intr
> >                                 = RTE_MAX_RXTX_INTR_VEC_ID +
> 1; So my
> > patch just follow this and fix some problem.
> >
> > Another option is I can use a local variable that assigned by max_intr with
> boundary check, so get_max_intr can be totally removed and max_intr in
> intr_source will not be modified.
> >
> > To me both fix are not perfect, I think the problem is in
> rte_intr_callback_register we just save a copy of the pci_dev->intr_handle
> but not the address point, so we are missing some mechanism to sync them.
> > But since we have tight schedule on the 17.02 release and this issue does
> cause some example code can't work, so we need to a fix it first, we may
> consider improve the mechanism later.
> >
> > Thanks
> > Qi
> 
> Applied with this title: "vfio: fix maximum number of interrupt for MSI-X"
> 
> Please check how to document this behaviour and make it consistent with
> other types of interrupts.

OK, I will check.

Thanks
Qi
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index b5b3f2b..92a19cb 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -287,6 +287,8 @@  get_max_intr(const struct rte_intr_handle *intr_handle)
 		if (src->intr_handle.fd != intr_handle->fd)
 			continue;
 
+		if (src->intr_handle.max_intr < intr_handle->max_intr)
+			src->intr_handle.max_intr = intr_handle->max_intr;
 		if (!src->intr_handle.max_intr)
 			src->intr_handle.max_intr = 1;
 		else if (src->intr_handle.max_intr > RTE_MAX_RXTX_INTR_VEC_ID)