diff mbox

[dpdk-dev,v3] eal: add counter size for efd clean

Message ID 1503540656-104421-1-git-send-email-jingjing.wu@intel.com (mailing list archive)
State Accepted, archived
Headers show

Checks

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

Commit Message

Jingjing Wu Aug. 24, 2017, 2:10 a.m. UTC
For virtual device, the rte_intr_handle struct is
initialized by the virtual device driver, including
the event fd assignment. If the event fd need to be
read for clean, an argument is required for the proper
event fd read.

This patch adds efd_counter_size in rte_intr_handle
struct to tell the rx interrupt process the read size.

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---

v3 change:
 - rework commit log

v2 change:
 - fix typo and reword commit log

 drivers/net/virtio/virtio_user/virtio_user_dev.c      |  2 ++
 lib/librte_eal/linuxapp/eal/eal_interrupts.c          | 19 ++++++++++++-------
 .../linuxapp/eal/include/exec-env/rte_interrupts.h    |  1 +
 3 files changed, 15 insertions(+), 7 deletions(-)

Comments

Jianfeng Tan Sept. 4, 2017, 2:49 a.m. UTC | #1
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Thursday, August 24, 2017 10:11 AM
> To: Tan, Jianfeng
> Cc: Wu, Jingjing; dev@dpdk.org
> Subject: [PATCH v3] eal: add counter size for efd clean
> 
> For virtual device, the rte_intr_handle struct is
> initialized by the virtual device driver, including
> the event fd assignment. If the event fd need to be
> read for clean, an argument is required for the proper
> event fd read.
> 
> This patch adds efd_counter_size in rte_intr_handle
> struct to tell the rx interrupt process the read size.
> 
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks,
Jianfeng

> ---
> 
> v3 change:
>  - rework commit log
> 
> v2 change:
>  - fix typo and reword commit log
> 
>  drivers/net/virtio/virtio_user/virtio_user_dev.c      |  2 ++
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c          | 19 ++++++++++++-------
>  .../linuxapp/eal/include/exec-env/rte_interrupts.h    |  1 +
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 7941271..906d7a2 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -270,6 +270,8 @@ virtio_user_fill_intr_handle(struct virtio_user_dev
> *dev)
>  	eth_dev->intr_handle->nb_efd = dev->max_queue_pairs;
>  	eth_dev->intr_handle->max_intr = dev->max_queue_pairs + 1;
>  	eth_dev->intr_handle->type = RTE_INTR_HANDLE_VDEV;
> +	/* For virtio vdev, no need to read counter for clean */
> +	eth_dev->intr_handle->efd_counter_size = 0;
>  	if (dev->vhostfd >= 0)
>  		eth_dev->intr_handle->fd = dev->vhostfd;
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> index 3e9ac41..7de39c3 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> @@ -914,7 +914,7 @@ static void
>  eal_intr_proc_rxtx_intr(int fd, const struct rte_intr_handle *intr_handle)
>  {
>  	union rte_intr_read_buffer buf;
> -	int bytes_read = 1;
> +	int bytes_read = 0;
>  	int nbytes;
> 
>  	switch (intr_handle->type) {
> @@ -930,11 +930,9 @@ eal_intr_proc_rxtx_intr(int fd, const struct
> rte_intr_handle *intr_handle)
>  		break;
>  #endif
>  	case RTE_INTR_HANDLE_VDEV:
> -		/* for vdev, fd points to:
> -		 * a. eventfd which does not need to read out;
> -		 * b. datapath fd which needs PMD to read out.
> -		 */
> -		return;
> +		bytes_read = intr_handle->efd_counter_size;
> +		/* For vdev, number of bytes to read is set by driver */
> +		break;
>  	case RTE_INTR_HANDLE_EXT:
>  		return;
>  	default:
> @@ -947,6 +945,8 @@ eal_intr_proc_rxtx_intr(int fd, const struct
> rte_intr_handle *intr_handle)
>  	 * read out to clear the ready-to-be-read flag
>  	 * for epoll_wait.
>  	 */
> +	if (bytes_read == 0)
> +		return;
>  	do {
>  		nbytes = read(fd, &buf, bytes_read);
>  		if (nbytes < 0) {
> @@ -1206,7 +1206,12 @@ rte_intr_efd_enable(struct rte_intr_handle
> *intr_handle, uint32_t nb_efd)
>  		intr_handle->nb_efd   = n;
>  		intr_handle->max_intr = NB_OTHER_INTR + n;
>  	} else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
> -		/* do nothing, and let vdev driver to initialize this struct */
> +		/* only check, initialization would be done in vdev driver.*/
> +		if (intr_handle->efd_counter_size >
> +		    sizeof(union rte_intr_read_buffer)) {
> +			RTE_LOG(ERR, EAL, "the efd_counter_size is
> oversized");
> +			return -EINVAL;
> +		}
>  	} else {
>  		intr_handle->efds[0]  = intr_handle->fd;
>  		intr_handle->nb_efd   = RTE_MIN(nb_efd, 1U);
> 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 6daffeb..fd4ea6c 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
> @@ -93,6 +93,7 @@ struct rte_intr_handle {
>  	enum rte_intr_handle_type type;  /**< handle type */
>  	uint32_t max_intr;             /**< max interrupt requested */
>  	uint32_t nb_efd;               /**< number of available efd(event fd) */
> +	uint8_t efd_counter_size;      /**< size of efd counter, used for vdev
> */
>  	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 */
> --
> 2.4.11
Thomas Monjalon Oct. 11, 2017, 1:11 p.m. UTC | #2
Hi,

24/08/2017 04:10, Jingjing Wu:
>         } else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
> -               /* do nothing, and let vdev driver to initialize this struct */
> +               /* only check, initialization would be done in vdev driver.*/
> +               if (intr_handle->efd_counter_size >
> +                   sizeof(union rte_intr_read_buffer)) {
> +                       RTE_LOG(ERR, EAL, "the efd_counter_size is oversized");
> +                       return -EINVAL;
> +               }

How interrupts are working with other buses?

Is it something we should manage in bus drivers code?
Jingjing Wu Oct. 16, 2017, 10:29 a.m. UTC | #3
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 11, 2017 9:11 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>;
> shreyansh.jain@nxp.com; hemant.agrawal@nxp.com;
> santosh.shukla@caviumnetworks.com; Tomasz Duszynski <tdu@semihalf.com>;
> Jacek Siuda <jck@semihalf.com>; jerin.jacob@caviumnetworks.com
> Subject: Re: [dpdk-dev] [PATCH v3] eal: add counter size for efd clean
> 
> Hi,
> 
> 24/08/2017 04:10, Jingjing Wu:
> >         } else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
> > -               /* do nothing, and let vdev driver to initialize this struct */
> > +               /* only check, initialization would be done in vdev driver.*/
> > +               if (intr_handle->efd_counter_size >
> > +                   sizeof(union rte_intr_read_buffer)) {
> > +                       RTE_LOG(ERR, EAL, "the efd_counter_size is oversized");
> > +                       return -EINVAL;
> > +               }
> 
> How interrupts are working with other buses?
> 
> Is it something we should manage in bus drivers code?

Any plan to move interrupts from EAL to bus?
Thomas Monjalon Oct. 16, 2017, 10:45 a.m. UTC | #4
16/10/2017 12:29, Wu, Jingjing:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Hi,
> > 
> > 24/08/2017 04:10, Jingjing Wu:
> > >         } else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
> > > -               /* do nothing, and let vdev driver to initialize this struct */
> > > +               /* only check, initialization would be done in vdev driver.*/
> > > +               if (intr_handle->efd_counter_size >
> > > +                   sizeof(union rte_intr_read_buffer)) {
> > > +                       RTE_LOG(ERR, EAL, "the efd_counter_size is oversized");
> > > +                       return -EINVAL;
> > > +               }
> > 
> > How interrupts are working with other buses?
> > 
> > Is it something we should manage in bus drivers code?
> 
> Any plan to move interrupts from EAL to bus?

It is an open question :)
Jingjing Wu Oct. 24, 2017, 1:38 p.m. UTC | #5
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, October 16, 2017 6:46 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>; shreyansh.jain@nxp.com;
> hemant.agrawal@nxp.com; santosh.shukla@caviumnetworks.com; Tomasz Duszynski
> <tdu@semihalf.com>; Jacek Siuda <jck@semihalf.com>;
> jerin.jacob@caviumnetworks.com
> Subject: Re: [dpdk-dev] [PATCH v3] eal: add counter size for efd clean
> 
> 16/10/2017 12:29, Wu, Jingjing:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Hi,
> > >
> > > 24/08/2017 04:10, Jingjing Wu:
> > > >         } else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
> > > > -               /* do nothing, and let vdev driver to initialize this struct */
> > > > +               /* only check, initialization would be done in vdev driver.*/
> > > > +               if (intr_handle->efd_counter_size >
> > > > +                   sizeof(union rte_intr_read_buffer)) {
> > > > +                       RTE_LOG(ERR, EAL, "the efd_counter_size is
> oversized");
> > > > +                       return -EINVAL;
> > > > +               }
> > >
> > > How interrupts are working with other buses?
> > >
> > > Is it something we should manage in bus drivers code?
> >
> > Any plan to move interrupts from EAL to bus?
> 
> It is an open question :)

OK. How about the patch on current interrupt? I think it is OK, right :)
Jingjing Wu Oct. 31, 2017, 1:25 p.m. UTC | #6
Ping

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Tuesday, October 24, 2017 9:39 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>; shreyansh.jain@nxp.com;
> hemant.agrawal@nxp.com; santosh.shukla@caviumnetworks.com; Tomasz Duszynski
> <tdu@semihalf.com>; Jacek Siuda <jck@semihalf.com>;
> jerin.jacob@caviumnetworks.com
> Subject: RE: [dpdk-dev] [PATCH v3] eal: add counter size for efd clean
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Monday, October 16, 2017 6:46 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>; shreyansh.jain@nxp.com;
> > hemant.agrawal@nxp.com; santosh.shukla@caviumnetworks.com; Tomasz Duszynski
> > <tdu@semihalf.com>; Jacek Siuda <jck@semihalf.com>;
> > jerin.jacob@caviumnetworks.com
> > Subject: Re: [dpdk-dev] [PATCH v3] eal: add counter size for efd clean
> >
> > 16/10/2017 12:29, Wu, Jingjing:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Hi,
> > > >
> > > > 24/08/2017 04:10, Jingjing Wu:
> > > > >         } else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
> > > > > -               /* do nothing, and let vdev driver to initialize this struct */
> > > > > +               /* only check, initialization would be done in vdev driver.*/
> > > > > +               if (intr_handle->efd_counter_size >
> > > > > +                   sizeof(union rte_intr_read_buffer)) {
> > > > > +                       RTE_LOG(ERR, EAL, "the efd_counter_size is
> > oversized");
> > > > > +                       return -EINVAL;
> > > > > +               }
> > > >
> > > > How interrupts are working with other buses?
> > > >
> > > > Is it something we should manage in bus drivers code?
> > >
> > > Any plan to move interrupts from EAL to bus?
> >
> > It is an open question :)
> 
> OK. How about the patch on current interrupt? I think it is OK, right :)
Thomas Monjalon Nov. 7, 2017, 12:41 a.m. UTC | #7
04/09/2017 04:49, Tan, Jianfeng:
> From: Wu, Jingjing
> > 
> > For virtual device, the rte_intr_handle struct is
> > initialized by the virtual device driver, including
> > the event fd assignment. If the event fd need to be
> > read for clean, an argument is required for the proper
> > event fd read.
> > 
> > This patch adds efd_counter_size in rte_intr_handle
> > struct to tell the rx interrupt process the read size.
> > 
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> 
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

I am not convinced at all by the whole interrupt management codebase.
But waiting longer for applying a patch will not improve the situation :)

Applied, thanks

Reworded title:
	eal/linux: add interrupt counter size for vdev
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 7941271..906d7a2 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -270,6 +270,8 @@  virtio_user_fill_intr_handle(struct virtio_user_dev *dev)
 	eth_dev->intr_handle->nb_efd = dev->max_queue_pairs;
 	eth_dev->intr_handle->max_intr = dev->max_queue_pairs + 1;
 	eth_dev->intr_handle->type = RTE_INTR_HANDLE_VDEV;
+	/* For virtio vdev, no need to read counter for clean */
+	eth_dev->intr_handle->efd_counter_size = 0;
 	if (dev->vhostfd >= 0)
 		eth_dev->intr_handle->fd = dev->vhostfd;
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 3e9ac41..7de39c3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -914,7 +914,7 @@  static void
 eal_intr_proc_rxtx_intr(int fd, const struct rte_intr_handle *intr_handle)
 {
 	union rte_intr_read_buffer buf;
-	int bytes_read = 1;
+	int bytes_read = 0;
 	int nbytes;
 
 	switch (intr_handle->type) {
@@ -930,11 +930,9 @@  eal_intr_proc_rxtx_intr(int fd, const struct rte_intr_handle *intr_handle)
 		break;
 #endif
 	case RTE_INTR_HANDLE_VDEV:
-		/* for vdev, fd points to:
-		 * a. eventfd which does not need to read out;
-		 * b. datapath fd which needs PMD to read out.
-		 */
-		return;
+		bytes_read = intr_handle->efd_counter_size;
+		/* For vdev, number of bytes to read is set by driver */
+		break;
 	case RTE_INTR_HANDLE_EXT:
 		return;
 	default:
@@ -947,6 +945,8 @@  eal_intr_proc_rxtx_intr(int fd, const struct rte_intr_handle *intr_handle)
 	 * read out to clear the ready-to-be-read flag
 	 * for epoll_wait.
 	 */
+	if (bytes_read == 0)
+		return;
 	do {
 		nbytes = read(fd, &buf, bytes_read);
 		if (nbytes < 0) {
@@ -1206,7 +1206,12 @@  rte_intr_efd_enable(struct rte_intr_handle *intr_handle, uint32_t nb_efd)
 		intr_handle->nb_efd   = n;
 		intr_handle->max_intr = NB_OTHER_INTR + n;
 	} else if (intr_handle->type == RTE_INTR_HANDLE_VDEV) {
-		/* do nothing, and let vdev driver to initialize this struct */
+		/* only check, initialization would be done in vdev driver.*/
+		if (intr_handle->efd_counter_size >
+		    sizeof(union rte_intr_read_buffer)) {
+			RTE_LOG(ERR, EAL, "the efd_counter_size is oversized");
+			return -EINVAL;
+		}
 	} else {
 		intr_handle->efds[0]  = intr_handle->fd;
 		intr_handle->nb_efd   = RTE_MIN(nb_efd, 1U);
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 6daffeb..fd4ea6c 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
@@ -93,6 +93,7 @@  struct rte_intr_handle {
 	enum rte_intr_handle_type type;  /**< handle type */
 	uint32_t max_intr;             /**< max interrupt requested */
 	uint32_t nb_efd;               /**< number of available efd(event fd) */
+	uint8_t efd_counter_size;      /**< size of efd counter, used for vdev */
 	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 */