diff mbox

[dpdk-dev,v8,01/11] eal/linux: add interrupt vectors support in intr_handle

Message ID 1432198563-16334-2-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Cunming Liang May 21, 2015, 8:55 a.m. UTC 
  The patch adds interrupt vectors support in rte_intr_handle.
'vec_en' is set when interrupt vectors are detected and associated event fds are set.
Those event fds are stored in efds[].
'intr_vec' is reserved for device driver to initialize the vector mapping table.
When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.

Signed-off-by: Danny Zhou <danny.zhou@intel.com>
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
v7 changes:
 - add eptrs[], it's used to store the register rte_epoll_event instances.
 - add vec_en, to log the vector capability status.

v6 changes:
 - add mapping table between irq vector number and queue id.

v5 changes:
 - Create this new patch file for changed struct rte_intr_handle that
   other patches depend on, to avoid breaking git bisect.

 lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Neil Horman May 21, 2015, 10:32 a.m. UTC | #1 
On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
> The patch adds interrupt vectors support in rte_intr_handle.
> 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
> Those event fds are stored in efds[].
> 'intr_vec' is reserved for device driver to initialize the vector mapping table.
> When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
> 
> Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> ---
> v7 changes:
>  - add eptrs[], it's used to store the register rte_epoll_event instances.
>  - add vec_en, to log the vector capability status.
> 
> v6 changes:
>  - add mapping table between irq vector number and queue id.
> 
> v5 changes:
>  - Create this new patch file for changed struct rte_intr_handle that
>    other patches depend on, to avoid breaking git bisect.
> 
>  lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> index 6a159c7..27174df 100644
> --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> @@ -38,6 +38,8 @@
>  #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
>  #define _RTE_LINUXAPP_INTERRUPTS_H_
>  
> +#define RTE_MAX_RXTX_INTR_VEC_ID     32
> +
>  enum rte_intr_handle_type {
>  	RTE_INTR_HANDLE_UNKNOWN = 0,
>  	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
> @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
>  	RTE_INTR_HANDLE_MAX
>  };
>  
> +struct rte_epoll_event;
> +
>  /** Handle for interrupts. */
>  struct rte_intr_handle {
>  	union {
> @@ -57,6 +61,12 @@ struct rte_intr_handle {
>  	};
>  	int fd;	 /**< interrupt event file descriptor */
>  	enum rte_intr_handle_type type;  /**< handle type */
> +	uint32_t max_intr;               /**< max interrupt requested */
> +	uint32_t nb_efd;                 /**< number of available efds */
> +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
> +					 /**< intr vector epoll event ptr */
> +	int *intr_vec;                   /**< intr vector number array */
>  };
>  

This is going to be ABI breaking if this from test_interrupts.c:
static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];

is a plausible way of using this structure.  Even putting the data at the end of
the structure won't help, as the array indicies are off
Neil

>  #endif /* _RTE_LINUXAPP_INTERRUPTS_H_ */
> -- 
> 1.8.1.4
> 
>
Neil Horman May 21, 2015, 5:58 p.m. UTC | #2 
On Thu, May 21, 2015 at 10:43:00AM -0700, Stephen Hemminger wrote:
> On Thu, 21 May 2015 06:32:02 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
> > > The patch adds interrupt vectors support in rte_intr_handle.
> > > 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
> > > Those event fds are stored in efds[].
> > > 'intr_vec' is reserved for device driver to initialize the vector mapping table.
> > > When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
> > > 
> > > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > ---
> > > v7 changes:
> > >  - add eptrs[], it's used to store the register rte_epoll_event instances.
> > >  - add vec_en, to log the vector capability status.
> > > 
> > > v6 changes:
> > >  - add mapping table between irq vector number and queue id.
> > > 
> > > v5 changes:
> > >  - Create this new patch file for changed struct rte_intr_handle that
> > >    other patches depend on, to avoid breaking git bisect.
> > > 
> > >  lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > index 6a159c7..27174df 100644
> > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > @@ -38,6 +38,8 @@
> > >  #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
> > >  #define _RTE_LINUXAPP_INTERRUPTS_H_
> > >  
> > > +#define RTE_MAX_RXTX_INTR_VEC_ID     32
> > > +
> > >  enum rte_intr_handle_type {
> > >  	RTE_INTR_HANDLE_UNKNOWN = 0,
> > >  	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
> > > @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
> > >  	RTE_INTR_HANDLE_MAX
> > >  };
> > >  
> > > +struct rte_epoll_event;
> > > +
> > >  /** Handle for interrupts. */
> > >  struct rte_intr_handle {
> > >  	union {
> > > @@ -57,6 +61,12 @@ struct rte_intr_handle {
> > >  	};
> > >  	int fd;	 /**< interrupt event file descriptor */
> > >  	enum rte_intr_handle_type type;  /**< handle type */
> > > +	uint32_t max_intr;               /**< max interrupt requested */
> > > +	uint32_t nb_efd;                 /**< number of available efds */
> > > +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> > > +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > > +					 /**< intr vector epoll event ptr */
> > > +	int *intr_vec;                   /**< intr vector number array */
> > >  };
> > >    
> > 
> > This is going to be ABI breaking if this from test_interrupts.c:
> > static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];
> > 
> > is a plausible way of using this structure.  Even putting the data at the end of
> > the structure won't help, as the array indicies are off
> 
> This needs to go in 2.0 and 2.0 has to have new ABI anyway.
> 
We've already released 2.0, I think you mean 2.1, but 2.1 can't have a new ABI
because we didn't announce it in 1.8.  The earliest we can update the ABI
(according to the ABI docs) at this point is 2.2, since we need to announce the
change in 2.1, then make it in 2.2

Neil
Stephen Hemminger May 21, 2015, 6:21 p.m. UTC | #3 
On Thu, 21 May 2015 13:58:46 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:

> On Thu, May 21, 2015 at 10:43:00AM -0700, Stephen Hemminger wrote:
> > On Thu, 21 May 2015 06:32:02 -0400
> > Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > > On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
> > > > The patch adds interrupt vectors support in rte_intr_handle.
> > > > 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
> > > > Those event fds are stored in efds[].
> > > > 'intr_vec' is reserved for device driver to initialize the vector mapping table.
> > > > When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
> > > > 
> > > > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > > ---
> > > > v7 changes:
> > > >  - add eptrs[], it's used to store the register rte_epoll_event instances.
> > > >  - add vec_en, to log the vector capability status.
> > > > 
> > > > v6 changes:
> > > >  - add mapping table between irq vector number and queue id.
> > > > 
> > > > v5 changes:
> > > >  - Create this new patch file for changed struct rte_intr_handle that
> > > >    other patches depend on, to avoid breaking git bisect.
> > > > 
> > > >  lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > index 6a159c7..27174df 100644
> > > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > @@ -38,6 +38,8 @@
> > > >  #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
> > > >  #define _RTE_LINUXAPP_INTERRUPTS_H_
> > > >  
> > > > +#define RTE_MAX_RXTX_INTR_VEC_ID     32
> > > > +
> > > >  enum rte_intr_handle_type {
> > > >  	RTE_INTR_HANDLE_UNKNOWN = 0,
> > > >  	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
> > > > @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
> > > >  	RTE_INTR_HANDLE_MAX
> > > >  };
> > > >  
> > > > +struct rte_epoll_event;
> > > > +
> > > >  /** Handle for interrupts. */
> > > >  struct rte_intr_handle {
> > > >  	union {
> > > > @@ -57,6 +61,12 @@ struct rte_intr_handle {
> > > >  	};
> > > >  	int fd;	 /**< interrupt event file descriptor */
> > > >  	enum rte_intr_handle_type type;  /**< handle type */
> > > > +	uint32_t max_intr;               /**< max interrupt requested */
> > > > +	uint32_t nb_efd;                 /**< number of available efds */
> > > > +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> > > > +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > > > +					 /**< intr vector epoll event ptr */
> > > > +	int *intr_vec;                   /**< intr vector number array */
> > > >  };
> > > >    
> > > 
> > > This is going to be ABI breaking if this from test_interrupts.c:
> > > static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];
> > > 
> > > is a plausible way of using this structure.  Even putting the data at the end of
> > > the structure won't help, as the array indicies are off
> > 
> > This needs to go in 2.0 and 2.0 has to have new ABI anyway.
> > 
> We've already released 2.0, I think you mean 2.1, but 2.1 can't have a new ABI
> because we didn't announce it in 1.8.  The earliest we can update the ABI
> (according to the ABI docs) at this point is 2.2, since we need to announce the
> change in 2.1, then make it in 2.2
> 
> Neil
> 

Then just skip 2.1 (or make it a trivial doc change only dummy release),
and call it 2.2.

I guess we need to proactively say every .x release will have new ABI.
Sorry, this is a project under development.
Neil Horman May 22, 2015, 12:05 a.m. UTC | #4 
On Thu, May 21, 2015 at 11:14:00AM -0700, Stephen Hemminger wrote:
> On Thu, 21 May 2015 13:58:46 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Thu, May 21, 2015 at 10:43:00AM -0700, Stephen Hemminger wrote:
> > > On Thu, 21 May 2015 06:32:02 -0400
> > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > 
> > > > On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
> > > > > The patch adds interrupt vectors support in rte_intr_handle.
> > > > > 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
> > > > > Those event fds are stored in efds[].
> > > > > 'intr_vec' is reserved for device driver to initialize the vector mapping table.
> > > > > When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
> > > > > 
> > > > > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > > > ---
> > > > > v7 changes:
> > > > >  - add eptrs[], it's used to store the register rte_epoll_event instances.
> > > > >  - add vec_en, to log the vector capability status.
> > > > > 
> > > > > v6 changes:
> > > > >  - add mapping table between irq vector number and queue id.
> > > > > 
> > > > > v5 changes:
> > > > >  - Create this new patch file for changed struct rte_intr_handle that
> > > > >    other patches depend on, to avoid breaking git bisect.
> > > > > 
> > > > >  lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > index 6a159c7..27174df 100644
> > > > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > @@ -38,6 +38,8 @@
> > > > >  #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
> > > > >  #define _RTE_LINUXAPP_INTERRUPTS_H_
> > > > >  
> > > > > +#define RTE_MAX_RXTX_INTR_VEC_ID     32
> > > > > +
> > > > >  enum rte_intr_handle_type {
> > > > >  	RTE_INTR_HANDLE_UNKNOWN = 0,
> > > > >  	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
> > > > > @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
> > > > >  	RTE_INTR_HANDLE_MAX
> > > > >  };
> > > > >  
> > > > > +struct rte_epoll_event;
> > > > > +
> > > > >  /** Handle for interrupts. */
> > > > >  struct rte_intr_handle {
> > > > >  	union {
> > > > > @@ -57,6 +61,12 @@ struct rte_intr_handle {
> > > > >  	};
> > > > >  	int fd;	 /**< interrupt event file descriptor */
> > > > >  	enum rte_intr_handle_type type;  /**< handle type */
> > > > > +	uint32_t max_intr;               /**< max interrupt requested */
> > > > > +	uint32_t nb_efd;                 /**< number of available efds */
> > > > > +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> > > > > +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > > > > +					 /**< intr vector epoll event ptr */
> > > > > +	int *intr_vec;                   /**< intr vector number array */
> > > > >  };
> > > > >    
> > > > 
> > > > This is going to be ABI breaking if this from test_interrupts.c:
> > > > static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];
> > > > 
> > > > is a plausible way of using this structure.  Even putting the data at the end of
> > > > the structure won't help, as the array indicies are off
> > > 
> > > This needs to go in 2.0 and 2.0 has to have new ABI anyway.
> > > 
> > We've already released 2.0, I think you mean 2.1, but 2.1 can't have a new ABI
> > because we didn't announce it in 1.8.  The earliest we can update the ABI
> > (according to the ABI docs) at this point is 2.2, since we need to announce the
> > change in 2.1, then make it in 2.2
> > 
> > Neil
> > 
> 
> Then just skip 2.1 (or make it a trivial doc change only dummy release),
> and call it 2.2.
> 
> I guess we need to proactively say every .x release will have new ABI.
> Sorry, this is a project under development.
> 
Sorry, NAK.  I didn't go through all the trouble of creating an ABI
infrastructure just to throw it out the window on some rubber stamp.  We decided
on the rules, we need to stick to them.  We have large projects that rely on
DPDK now (OVS primarily), and we owe it to them to not just go completely throw
out the ABI every release.  We have a process for doing it, lets follow it.

Neil
Stephen Hemminger May 22, 2015, 4:52 p.m. UTC | #5 
On Fri, 22 May 2015 00:05:36 +0000
Neil Horman <nhorman@tuxdriver.com> wrote:

> On Thu, May 21, 2015 at 11:14:00AM -0700, Stephen Hemminger wrote:
> > On Thu, 21 May 2015 13:58:46 -0400
> > Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > > On Thu, May 21, 2015 at 10:43:00AM -0700, Stephen Hemminger wrote:
> > > > On Thu, 21 May 2015 06:32:02 -0400
> > > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > 
> > > > > On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
> > > > > > The patch adds interrupt vectors support in rte_intr_handle.
> > > > > > 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
> > > > > > Those event fds are stored in efds[].
> > > > > > 'intr_vec' is reserved for device driver to initialize the vector mapping table.
> > > > > > When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
> > > > > > 
> > > > > > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > > > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > > > > ---
> > > > > > v7 changes:
> > > > > >  - add eptrs[], it's used to store the register rte_epoll_event instances.
> > > > > >  - add vec_en, to log the vector capability status.
> > > > > > 
> > > > > > v6 changes:
> > > > > >  - add mapping table between irq vector number and queue id.
> > > > > > 
> > > > > > v5 changes:
> > > > > >  - Create this new patch file for changed struct rte_intr_handle that
> > > > > >    other patches depend on, to avoid breaking git bisect.
> > > > > > 
> > > > > >  lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > > index 6a159c7..27174df 100644
> > > > > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > > @@ -38,6 +38,8 @@
> > > > > >  #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
> > > > > >  #define _RTE_LINUXAPP_INTERRUPTS_H_
> > > > > >  
> > > > > > +#define RTE_MAX_RXTX_INTR_VEC_ID     32
> > > > > > +
> > > > > >  enum rte_intr_handle_type {
> > > > > >  	RTE_INTR_HANDLE_UNKNOWN = 0,
> > > > > >  	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
> > > > > > @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
> > > > > >  	RTE_INTR_HANDLE_MAX
> > > > > >  };
> > > > > >  
> > > > > > +struct rte_epoll_event;
> > > > > > +
> > > > > >  /** Handle for interrupts. */
> > > > > >  struct rte_intr_handle {
> > > > > >  	union {
> > > > > > @@ -57,6 +61,12 @@ struct rte_intr_handle {
> > > > > >  	};
> > > > > >  	int fd;	 /**< interrupt event file descriptor */
> > > > > >  	enum rte_intr_handle_type type;  /**< handle type */
> > > > > > +	uint32_t max_intr;               /**< max interrupt requested */
> > > > > > +	uint32_t nb_efd;                 /**< number of available efds */
> > > > > > +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> > > > > > +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > > > > > +					 /**< intr vector epoll event ptr */
> > > > > > +	int *intr_vec;                   /**< intr vector number array */
> > > > > >  };
> > > > > >    
> > > > > 
> > > > > This is going to be ABI breaking if this from test_interrupts.c:
> > > > > static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];
> > > > > 
> > > > > is a plausible way of using this structure.  Even putting the data at the end of
> > > > > the structure won't help, as the array indicies are off
> > > > 
> > > > This needs to go in 2.0 and 2.0 has to have new ABI anyway.
> > > > 
> > > We've already released 2.0, I think you mean 2.1, but 2.1 can't have a new ABI
> > > because we didn't announce it in 1.8.  The earliest we can update the ABI
> > > (according to the ABI docs) at this point is 2.2, since we need to announce the
> > > change in 2.1, then make it in 2.2
> > > 
> > > Neil
> > > 
> > 
> > Then just skip 2.1 (or make it a trivial doc change only dummy release),
> > and call it 2.2.
> > 
> > I guess we need to proactively say every .x release will have new ABI.
> > Sorry, this is a project under development.
> > 
> Sorry, NAK.  I didn't go through all the trouble of creating an ABI
> infrastructure just to throw it out the window on some rubber stamp.  We decided
> on the rules, we need to stick to them.  We have large projects that rely on
> DPDK now (OVS primarily), and we owe it to them to not just go completely throw
> out the ABI every release.  We have a process for doing it, lets follow it.
> 
> Neil
> 

I meant, that close and ship existing 2.1 code base early and open 2.2 early
to keep things rolling. But in general this project needs x.x.y releases
with ABI stability, and just admit that x.x releases will not have stable ABI.
That is reality now.

A lot of the ABI problem is that the code does not do a good job of hiding.
And also does not sepearte driver ABI from user ABI. There are things like
structure of PCI and interrupt handles that the user from library point
of view should not care about, but drivers will need to.
Neil Horman May 27, 2015, 10:33 a.m. UTC | #6 
On Fri, May 22, 2015 at 09:52:06AM -0700, Stephen Hemminger wrote:
> On Fri, 22 May 2015 00:05:36 +0000
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Thu, May 21, 2015 at 11:14:00AM -0700, Stephen Hemminger wrote:
> > > On Thu, 21 May 2015 13:58:46 -0400
> > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > 
> > > > On Thu, May 21, 2015 at 10:43:00AM -0700, Stephen Hemminger wrote:
> > > > > On Thu, 21 May 2015 06:32:02 -0400
> > > > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > 
> > > > > > On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
> > > > > > > The patch adds interrupt vectors support in rte_intr_handle.
> > > > > > > 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
> > > > > > > Those event fds are stored in efds[].
> > > > > > > 'intr_vec' is reserved for device driver to initialize the vector mapping table.
> > > > > > > When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
> > > > > > > 
> > > > > > > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > > > > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > > > > > ---
> > > > > > > v7 changes:
> > > > > > >  - add eptrs[], it's used to store the register rte_epoll_event instances.
> > > > > > >  - add vec_en, to log the vector capability status.
> > > > > > > 
> > > > > > > v6 changes:
> > > > > > >  - add mapping table between irq vector number and queue id.
> > > > > > > 
> > > > > > > v5 changes:
> > > > > > >  - Create this new patch file for changed struct rte_intr_handle that
> > > > > > >    other patches depend on, to avoid breaking git bisect.
> > > > > > > 
> > > > > > >  lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > > > index 6a159c7..27174df 100644
> > > > > > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > > > > > > @@ -38,6 +38,8 @@
> > > > > > >  #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
> > > > > > >  #define _RTE_LINUXAPP_INTERRUPTS_H_
> > > > > > >  
> > > > > > > +#define RTE_MAX_RXTX_INTR_VEC_ID     32
> > > > > > > +
> > > > > > >  enum rte_intr_handle_type {
> > > > > > >  	RTE_INTR_HANDLE_UNKNOWN = 0,
> > > > > > >  	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
> > > > > > > @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
> > > > > > >  	RTE_INTR_HANDLE_MAX
> > > > > > >  };
> > > > > > >  
> > > > > > > +struct rte_epoll_event;
> > > > > > > +
> > > > > > >  /** Handle for interrupts. */
> > > > > > >  struct rte_intr_handle {
> > > > > > >  	union {
> > > > > > > @@ -57,6 +61,12 @@ struct rte_intr_handle {
> > > > > > >  	};
> > > > > > >  	int fd;	 /**< interrupt event file descriptor */
> > > > > > >  	enum rte_intr_handle_type type;  /**< handle type */
> > > > > > > +	uint32_t max_intr;               /**< max interrupt requested */
> > > > > > > +	uint32_t nb_efd;                 /**< number of available efds */
> > > > > > > +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> > > > > > > +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > > > > > > +					 /**< intr vector epoll event ptr */
> > > > > > > +	int *intr_vec;                   /**< intr vector number array */
> > > > > > >  };
> > > > > > >    
> > > > > > 
> > > > > > This is going to be ABI breaking if this from test_interrupts.c:
> > > > > > static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];
> > > > > > 
> > > > > > is a plausible way of using this structure.  Even putting the data at the end of
> > > > > > the structure won't help, as the array indicies are off
> > > > > 
> > > > > This needs to go in 2.0 and 2.0 has to have new ABI anyway.
> > > > > 
> > > > We've already released 2.0, I think you mean 2.1, but 2.1 can't have a new ABI
> > > > because we didn't announce it in 1.8.  The earliest we can update the ABI
> > > > (according to the ABI docs) at this point is 2.2, since we need to announce the
> > > > change in 2.1, then make it in 2.2
> > > > 
> > > > Neil
> > > > 
> > > 
> > > Then just skip 2.1 (or make it a trivial doc change only dummy release),
> > > and call it 2.2.
> > > 
> > > I guess we need to proactively say every .x release will have new ABI.
> > > Sorry, this is a project under development.
> > > 
> > Sorry, NAK.  I didn't go through all the trouble of creating an ABI
> > infrastructure just to throw it out the window on some rubber stamp.  We decided
> > on the rules, we need to stick to them.  We have large projects that rely on
> > DPDK now (OVS primarily), and we owe it to them to not just go completely throw
> > out the ABI every release.  We have a process for doing it, lets follow it.
> > 
> > Neil
> > 
> 
> I meant, that close and ship existing 2.1 code base early and open 2.2 early
> to keep things rolling. But in general this project needs x.x.y releases
> with ABI stability, and just admit that x.x releases will not have stable ABI.
> That is reality now.
> 
I'm  not opposed to doing that, though the purpose of the proposed cadence was
to give sufficient notice to downstream consumers of DPDK that ABI changes were
comming.  As long as the time delta beweeen 2.X and 2.X+1 is sufficient for
consumers to have time to react and update their applications I'm ok with it
(which I know is subjective, but I'm willing to experiment there).

> A lot of the ABI problem is that the code does not do a good job of hiding.
> And also does not sepearte driver ABI from user ABI. There are things like
> structure of PCI and interrupt handles that the user from library point
> of view should not care about, but drivers will need to.
> 
I agree.  I had hoped that implementing an ABI process would help drive
improvements in this area, but it hasn't seemed to yet.

Neil

> 
>
Cunming Liang May 29, 2015, 8:56 a.m. UTC | #7 
Hi Neil,

On 5/22/2015 1:58 AM, Neil Horman wrote:
> On Thu, May 21, 2015 at 10:43:00AM -0700, Stephen Hemminger wrote:
>> On Thu, 21 May 2015 06:32:02 -0400
>> Neil Horman <nhorman@tuxdriver.com> wrote:
>>
>>> On Thu, May 21, 2015 at 04:55:53PM +0800, Cunming Liang wrote:
>>>> The patch adds interrupt vectors support in rte_intr_handle.
>>>> 'vec_en' is set when interrupt vectors are detected and associated event fds are set.
>>>> Those event fds are stored in efds[].
>>>> 'intr_vec' is reserved for device driver to initialize the vector mapping table.
>>>> When the event fds add to a specified epoll instance, 'elist' will hold the rte_epoll_event object pointer.
>>>>
>>>> Signed-off-by: Danny Zhou <danny.zhou@intel.com>
>>>> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
>>>> ---
>>>> v7 changes:
>>>>   - add eptrs[], it's used to store the register rte_epoll_event instances.
>>>>   - add vec_en, to log the vector capability status.
>>>>
>>>> v6 changes:
>>>>   - add mapping table between irq vector number and queue id.
>>>>
>>>> v5 changes:
>>>>   - Create this new patch file for changed struct rte_intr_handle that
>>>>     other patches depend on, to avoid breaking git bisect.
>>>>
>>>>   lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
>>>> index 6a159c7..27174df 100644
>>>> --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
>>>> +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
>>>> @@ -38,6 +38,8 @@
>>>>   #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
>>>>   #define _RTE_LINUXAPP_INTERRUPTS_H_
>>>>   
>>>> +#define RTE_MAX_RXTX_INTR_VEC_ID     32
>>>> +
>>>>   enum rte_intr_handle_type {
>>>>   	RTE_INTR_HANDLE_UNKNOWN = 0,
>>>>   	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
>>>> @@ -48,6 +50,8 @@ enum rte_intr_handle_type {
>>>>   	RTE_INTR_HANDLE_MAX
>>>>   };
>>>>   
>>>> +struct rte_epoll_event;
>>>> +
>>>>   /** Handle for interrupts. */
>>>>   struct rte_intr_handle {
>>>>   	union {
>>>> @@ -57,6 +61,12 @@ struct rte_intr_handle {
>>>>   	};
>>>>   	int fd;	 /**< interrupt event file descriptor */
>>>>   	enum rte_intr_handle_type type;  /**< handle type */
>>>> +	uint32_t max_intr;               /**< max interrupt requested */
>>>> +	uint32_t nb_efd;                 /**< number of available efds */
>>>> +	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
>>>> +	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
>>>> +					 /**< intr vector epoll event ptr */
>>>> +	int *intr_vec;                   /**< intr vector number array */
>>>>   };
>>>>     
>>> This is going to be ABI breaking if this from test_interrupts.c:
>>> static struct rte_intr_handle intr_handles[TEST_INTERRUPT_HANDLE_MAX];
>>>
>>> is a plausible way of using this structure.  Even putting the data at the end of
>>> the structure won't help, as the array indicies are off
>> This needs to go in 2.0 and 2.0 has to have new ABI anyway.
>>
> We've already released 2.0, I think you mean 2.1, but 2.1 can't have a new ABI
> because we didn't announce it in 1.8.  The earliest we can update the ABI
> (according to the ABI docs) at this point is 2.2, since we need to announce the
> change in 2.1, then make it in 2.2
>
> Neil
>
I'll follow your guidance to send a separate patch to announce the ABI 
changes in this release.
For this code patch series, I propose to turn off the whole feature by 
default so as to avoid ABI broken in this release.
On next release v2.2, I'll send another cleanup patch to remove the 
feature macro.
In this way, we either won't block the feature code review or won't 
break ABI.
For users who are still going to use the feature in v2.1, they shall 
make sure aware of the impact of ABI changes and take risk to turn on 
the feature manually.
The v9 patch will include this part. Does it sound good to you?

Thanks,
Steve
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
index 6a159c7..27174df 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
@@ -38,6 +38,8 @@ 
 #ifndef _RTE_LINUXAPP_INTERRUPTS_H_
 #define _RTE_LINUXAPP_INTERRUPTS_H_
 
+#define RTE_MAX_RXTX_INTR_VEC_ID     32
+
 enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_UNKNOWN = 0,
 	RTE_INTR_HANDLE_UIO,      /**< uio device handle */
@@ -48,6 +50,8 @@  enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_MAX
 };
 
+struct rte_epoll_event;
+
 /** Handle for interrupts. */
 struct rte_intr_handle {
 	union {
@@ -57,6 +61,12 @@  struct rte_intr_handle {
 	};
 	int fd;	 /**< interrupt event file descriptor */
 	enum rte_intr_handle_type type;  /**< handle type */
+	uint32_t max_intr;               /**< max interrupt requested */
+	uint32_t nb_efd;                 /**< number of available efds */
+	int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
+	struct rte_epoll_event *elist[RTE_MAX_RXTX_INTR_VEC_ID];
+					 /**< intr vector epoll event ptr */
+	int *intr_vec;                   /**< intr vector number array */
 };
 
 #endif /* _RTE_LINUXAPP_INTERRUPTS_H_ */