[dpdk-dev] eal: fix rte_intr_dp_is_en() check

  When binding a device to igb_uio with intr_conf.rxq set to 1, nb_efd
is 1 (for link event) but rte_intr_dp_is_en() will still return true.
rte_intr_dp_is_en() should also consider intr_handle type in addition
to nb_efd.

Signed-off-by: Yong Wang <yongwang@vmware.com>
---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Hi Yong,

rte_intr_dp_is_en() returns true when rte_intr_efd_enable() (the way to 
enable data-path interrupt) sets a number of event fds.
In this case, "intr_conf.rxq=1" configuration causes "nb_efd=1". The 
value comes from RTE_MIN($nb_efd, 1) from data-path, but not from link 
event.
Per link event, you shouldn't use rte_intr_dp_is_en() as the indication.
As igb_uio only has a single vector, when the conflict(both intr_rxq and 
intr_lsc turn on) happens, the intr_rxq has high priority than intr_lsc 
as default PMD behavior.
Reference as PG 3.1.9 note in 
http://dpdk.org/doc/guides/prog_guide/env_abstraction_layer.html

Regards,
Cunming

On 7/15/2016 8:36 AM, Yong Wang wrote:
> When binding a device to igb_uio with intr_conf.rxq set to 1, nb_efd
> is 1 (for link event) but rte_intr_dp_is_en() will still return true.
> rte_intr_dp_is_en() should also consider intr_handle type in addition
> to nb_efd.
>
> Signed-off-by: Yong Wang <yongwang@vmware.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_interrupts.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> index 47a3b20..71f63e9 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> @@ -1200,7 +1200,8 @@ rte_intr_efd_disable(struct rte_intr_handle *intr_handle)
>   int
>   rte_intr_dp_is_en(struct rte_intr_handle *intr_handle)
>   {
> -	return !(!intr_handle->nb_efd);
> +	return (!(!intr_handle->nb_efd) &&
> +		(intr_handle->type == RTE_INTR_HANDLE_VFIO_MSIX));
>   }
>   
>   int
  

Hi Yong,

I think the interrupt management should be simpler.
If you want to invest some time to rework this API, you
are very welcome.


2016-07-18 14:19, Liang, Cunming:
> Hi Yong,
> 
> rte_intr_dp_is_en() returns true when rte_intr_efd_enable() (the way to 
> enable data-path interrupt) sets a number of event fds.
> In this case, "intr_conf.rxq=1" configuration causes "nb_efd=1". The 
> value comes from RTE_MIN($nb_efd, 1) from data-path, but not from link 
> event.
> Per link event, you shouldn't use rte_intr_dp_is_en() as the indication.
> As igb_uio only has a single vector, when the conflict(both intr_rxq and 
> intr_lsc turn on) happens, the intr_rxq has high priority than intr_lsc 
> as default PMD behavior.
> Reference as PG 3.1.9 note in 
> http://dpdk.org/doc/guides/prog_guide/env_abstraction_layer.html
> 
> Regards,
> Cunming
> 
> On 7/15/2016 8:36 AM, Yong Wang wrote:
> > When binding a device to igb_uio with intr_conf.rxq set to 1, nb_efd
> > is 1 (for link event) but rte_intr_dp_is_en() will still return true.
> > rte_intr_dp_is_en() should also consider intr_handle type in addition
> > to nb_efd.
> >
> > Signed-off-by: Yong Wang <yongwang@vmware.com>
  

> On Jul 18, 2016, at 2:21 AM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> Hi Yong,
> 
> I think the interrupt management should be simpler.
> If you want to invest some time to rework this API, you
> are very welcome.
> 
> 
> 2016-07-18 14:19, Liang, Cunming:
>> Hi Yong,
>> 
>> rte_intr_dp_is_en() returns true when rte_intr_efd_enable() (the way to 
>> enable data-path interrupt) sets a number of event fds.
>> In this case, "intr_conf.rxq=1" configuration causes "nb_efd=1". The 
>> value comes from RTE_MIN($nb_efd, 1) from data-path, but not from link 
>> event.
>> Per link event, you shouldn't use rte_intr_dp_is_en() as the indication.
>> As igb_uio only has a single vector, when the conflict(both intr_rxq and 
>> intr_lsc turn on) happens, the intr_rxq has high priority than intr_lsc 
>> as default PMD behavior.
>> Reference as PG 3.1.9 note in 
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__dpdk.org_doc_guides_prog-5Fguide_env-5Fabstraction-5Flayer.html&d=CwICAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=44mSO5N5yEs4CeCdtQE0xt0F7J0p67_mApYVAzyYms0&m=xFO005GdZMA9a6IC2kvhKYaXW7Rhl3dlcom5HfOSy2g&s=yERFYJTe6TBWFCltmR3xikmxkLKutbot5BfFxK30tZ0&e= 
>> 
>> Regards,
>> Cunning

Thanks Cunming and Thomas for the feedback.  I agree with Thomas that the rx queue interrupt APIs could be simplified and it’s not clear to me if the API user needs to be concerned about rte_intr_cap_multiple(), rte_intr_allow_others(), rte_intr_dp_is_en(), etc. if all he needs is to enable intr with certain # of vectors and map the vectors to either efds or callbacks.  I feel it’s simpler just returning the number of vectors the system can support if it cannot satisfy nb_efd vectors and let the application decide how should this be handled. The link event and rxq interrupt init/uninit path also looks quite different and some unification will be helpful.

The API documentation can also use some more clarification.  For example, without reading the program guide, it’s easy to miss the fact that when the device is bound to UIO, the implementation will try to enable rxq instead of lsc when both intr_conf.lsc and intr_conf.rxq is set to 1. I can imagine there are cases where an application would prefer to enable link event instead of rxq interrupt in such cases and currently there is no easy way to achieve that.  Or is there any particular reason such a preference is chosen?

>> 
>> On 7/15/2016 8:36 AM, Yong Wang wrote:
>>> When binding a device to igb_uio with intr_conf.rxq set to 1, nb_efd
>>> is 1 (for link event) but rte_intr_dp_is_en() will still return true.
>>> rte_intr_dp_is_en() should also consider intr_handle type in addition
>>> to nb_efd.
>>> 
>>> Signed-off-by: Yong Wang <yongwang@vmware.com>
>
  

Hi Yong,

> -----Original Message-----

> From: Yong Wang [mailto:yongwang@vmware.com]

> Sent: Wednesday, July 20, 2016 5:59 AM

> To: Thomas Monjalon <thomas.monjalon@6wind.com>

> Cc: dev@dpdk.org; Liang, Cunming <cunming.liang@intel.com>;

> david.marchand@6wind.com

> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_intr_dp_is_en() check

> 

> 

> > On Jul 18, 2016, at 2:21 AM, Thomas Monjalon <thomas.monjalon@6wind.com>

> wrote:

> >

> > Hi Yong,

> >

> > I think the interrupt management should be simpler.

> > If you want to invest some time to rework this API, you

> > are very welcome.

> >

> >

> > 2016-07-18 14:19, Liang, Cunming:

> >> Hi Yong,

> >>

> >> rte_intr_dp_is_en() returns true when rte_intr_efd_enable() (the way to

> >> enable data-path interrupt) sets a number of event fds.

> >> In this case, "intr_conf.rxq=1" configuration causes "nb_efd=1". The

> >> value comes from RTE_MIN($nb_efd, 1) from data-path, but not from link

> >> event.

> >> Per link event, you shouldn't use rte_intr_dp_is_en() as the indication.

> >> As igb_uio only has a single vector, when the conflict(both intr_rxq and

> >> intr_lsc turn on) happens, the intr_rxq has high priority than intr_lsc

> >> as default PMD behavior.

> >> Reference as PG 3.1.9 note in

> >> https://urldefense.proofpoint.com/v2/url?u=http-

> 3A__dpdk.org_doc_guides_prog-5Fguide_env-5Fabstraction-

> 5Flayer.html&d=CwICAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-

> uEs&r=44mSO5N5yEs4CeCdtQE0xt0F7J0p67_mApYVAzyYms0&m=xFO005GdZMA

> 9a6IC2kvhKYaXW7Rhl3dlcom5HfOSy2g&s=yERFYJTe6TBWFCltmR3xikmxkLKutbot

> 5BfFxK30tZ0&e=

> >>

> >> Regards,

> >> Cunning

> 

> Thanks Cunming and Thomas for the feedback.  I agree with Thomas that the rx

> queue interrupt APIs could be simplified and it’s not clear to me if the API user

> needs to be concerned about rte_intr_cap_multiple(), rte_intr_allow_others(),

> rte_intr_dp_is_en(), etc. if all he needs is to enable intr with certain # of vectors

> and map the vectors to either efds or callbacks.  I feel it’s simpler just returning

> the number of vectors the system can support if it cannot satisfy nb_efd vectors

> and let the application decide how should this be handled. 


It's welcome to make a simpler intr API. At the beginning of introducing rxq intr, it
didn't attract much attention, the coordination problem of EAL intr thread remains.

Status quo:

- The 3 APIs you talked is the helper function for PMD developer, not for end user.
   EAL provides efds/vectors mapping, hides vfio/uio difference. The causes/vectors
   mapping is handled by PMD. Then it's not necessary for the end user to aware
   intr vector, just take aware of event by queue id.

   Your idea on exposing vectors to end user is an option. In terms of it, the APP
   has to understand the causes of vectors, misc(e.g. lsc, mb, etc) vs non-misc(rxq).
   In particular case(e.g. mb), it seems APP is not the right person to turn on/off.
   If all are acceptable, ethdev can provide new API for causes to vector mapping.

   There's one note for replacing the 3 API provided to PMD. PMD is blind
   to the user space IO infrastructure, vfio or uio. The way for PMD to configure
   mapping for legacy/MSI(e.g. uio case) and MSIX(e.g. vfio case) isn't the same.

>The link event and rxq

> interrupt init/uninit path also looks quite different and some unification will be

> helpful.


- 'intr_conf' and interrupt thread for misc
   To turn on lsc intr, it requires 'intr_conf.lsc=1'. It cases ethdev to register handler
   for further processing in the interrupt thread. It's not necessary for rxq intr.

   Probably a clean way is not to handle device external interrupt event in EAL
   interrupt thread (intr mb may have some problem). The EAL interrupt thread is
   only used to postpone the delay execution or other background interrupt
   (e.g. alarm). Then misc/non-misc can be combined,  while requiring APP to
   detect the interrupt causes. 

> 

> The API documentation can also use some more clarification.  For example,

> without reading the program guide, it’s easy to miss the fact that when the

> device is bound to UIO, the implementation will try to enable rxq instead of lsc

> when both intr_conf.lsc and intr_conf.rxq is set to 1. I can imagine there are cases

> where an application would prefer to enable link event instead of rxq interrupt in

> such cases and currently there is no easy way to achieve that.  Or is there any

> particular reason such a preference is chosen?


In uio, if do care lsc. The suggestion is to only turn on intr_conf.lsc. It's just a default
behavior definition for the single vector tradeoff. Which don't want to introduce
another configuration. The uio msix patch was expected to upstream on that time...
The uio single vector problem probably will remain for a long time. So welcome to
rework these stuffs. 

Thanks,
Cunming
 
> 

> >>

> >> On 7/15/2016 8:36 AM, Yong Wang wrote:

> >>> When binding a device to igb_uio with intr_conf.rxq set to 1, nb_efd

> >>> is 1 (for link event) but rte_intr_dp_is_en() will still return true.

> >>> rte_intr_dp_is_en() should also consider intr_handle type in addition

> >>> to nb_efd.

> >>>

> >>> Signed-off-by: Yong Wang <yongwang@vmware.com>

> >
  

2016-07-20 07:03, Liang, Cunming:
>    Probably a clean way is not to handle device external interrupt event in EAL
>    interrupt thread (intr mb may have some problem). The EAL interrupt thread is
>    only used to postpone the delay execution or other background interrupt
>    (e.g. alarm). Then misc/non-misc can be combined,  while requiring APP to
>    detect the interrupt causes. 

I am not sure it was a good idea to have a thread for the link interrupt.
It may be simpler and cleaner to let the application do the pthread_create.
  

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, July 20, 2016 4:42 PM
> To: Liang, Cunming <cunming.liang@intel.com>
> Cc: Yong Wang <yongwang@vmware.com>; dev@dpdk.org;
> david.marchand@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_intr_dp_is_en() check
> 
> 2016-07-20 07:03, Liang, Cunming:
> >    Probably a clean way is not to handle device external interrupt event in EAL
> >    interrupt thread (intr mb may have some problem). The EAL interrupt thread
> is
> >    only used to postpone the delay execution or other background interrupt
> >    (e.g. alarm). Then misc/non-misc can be combined,  while requiring APP to
> >    detect the interrupt causes.
> 
> I am not sure it was a good idea to have a thread for the link interrupt.
> It may be simpler and cleaner to let the application do the pthread_create.
The EAL intr thread is reserved for all interrupt before, is not design for link interrupt
on the first day. Personally I vote to remove link interrupt from interrupt thread.
-Cunming