[v7,14/18] vdpa/ifc: add interrupt and handle for virtio blk
Checks
Commit Message
Create a thread to poll and relay config space change interrupt.
Use VHOST_USER_SLAVE_CONFIG_CHANGE_MSG to info qemu.
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
drivers/vdpa/ifc/ifcvf_vdpa.c | 112 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
Comments
> -----Original Message-----
> From: Pei, Andy <andy.pei@intel.com>
> Sent: Wednesday, April 27, 2022 4:30 PM
> To: dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Cao,
> Gang <gang.cao@intel.com>; Liu, Changpeng <changpeng.liu@intel.com>
> Subject: [PATCH v7 14/18] vdpa/ifc: add interrupt and handle for virtio
> blk
Better be: vdpa/ifc: add interrupt handling for config space
>
> Create a thread to poll and relay config space change interrupt.
> Use VHOST_USER_SLAVE_CONFIG_CHANGE_MSG to info qemu.
Inform QEMU. You don't need to save words in commit log. The commit log
should be as detailed as possible to make readers understand quickly what
the commit is doing :)
>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
> drivers/vdpa/ifc/ifcvf_vdpa.c | 112
> ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 112 insertions(+)
>
> diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
> index 5a8cf1c..0e94e1f 100644
> --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
> +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
> @@ -53,7 +53,9 @@ struct ifcvf_internal {
> int vfio_group_fd;
> int vfio_dev_fd;
> pthread_t tid; /* thread for notify relay */
> + pthread_t intr_tid; /* thread for intr relay */
Thread for virtio-blk config space change interrupt relay
> int epfd;
> + int csc_fd;
csc_epfd
> int vid;
> struct rte_vdpa_device *vdev;
> uint16_t max_queues;
> @@ -558,6 +560,107 @@ struct rte_vdpa_dev_info {
> return 0;
> }
>
> +static void
> +virtio_interrupt_handler(struct ifcvf_internal *internal)
> +{
> + int vid = internal->vid;
> + int ret;
> +
> + ret = rte_vhost_slave_config_change(vid, 1);
> + if (ret)
> + DRV_LOG(ERR, "failed to notify the guest about configuration
> space change.");
> +}
> +
> +static void *
> +intr_relay(void *arg)
> +{
> + struct ifcvf_internal *internal = (struct ifcvf_internal *)arg;
> + struct epoll_event csc_event;
> + struct epoll_event ev;
> + uint64_t buf;
> + int nbytes;
> + int csc_fd, csc_val = 0;
> +
> + csc_fd = epoll_create(1);
> + if (csc_fd < 0) {
> + DRV_LOG(ERR, "failed to create epoll for config space
> change.");
> + return NULL;
> + }
> +
> + ev.events = EPOLLIN | EPOLLPRI | EPOLLRDHUP | EPOLLHUP;
> + ev.data.fd = rte_intr_fd_get(internal->pdev->intr_handle);
> + if (epoll_ctl(csc_fd, EPOLL_CTL_ADD,
> + rte_intr_fd_get(internal->pdev->intr_handle), &ev) < 0) {
> + DRV_LOG(ERR, "epoll add error: %s", strerror(errno));
> + return NULL;
Close the epfd and set to -1 if err.
> + }
> +
> + internal->csc_fd = csc_fd;
> +
> + for (;;) {
> + csc_val = epoll_wait(csc_fd, &csc_event, 1, -1);
> + if (csc_val < 0) {
> + if (errno == EINTR)
> + continue;
> + DRV_LOG(ERR, "epoll_wait return fail\n");
Save '\n', it's not needed for DRV_LOG. Please check other DRV_LOGs
> + return NULL;
> + } else if (csc_val == 0) {
> + continue;
> + } else {
> + /* csc_val > 0 */
> + nbytes = read(csc_event.data.fd, &buf, 8);
> + if (nbytes < 0) {
> + if (errno == EINTR || errno == EWOULDBLOCK)
EAGAIN should also be this case?
> + continue;
> + DRV_LOG(ERR, "Error reading from file
> descriptor %d: %s\n",
> + csc_event.data.fd,
> + strerror(errno));
> + return NULL;
> + } else if (nbytes == 0) {
> + DRV_LOG(ERR, "Read nothing from file
> descriptor %d\n",
> + csc_event.data.fd);
> + continue;
> + } else {
> + virtio_interrupt_handler(internal);
> + }
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static int
> +setup_intr_relay(struct ifcvf_internal *internal)
> +{
> + int ret;
> +
> + ret = pthread_create(&internal->intr_tid, NULL, intr_relay,
> + (void *)internal);
EAL API: rte_ctrl_thread_create, will be preferred.
> + if (ret) {
> + DRV_LOG(ERR, "failed to create notify relay pthread.");
> + return -1;
> + }
> + return 0;
> +}
> +
> +static int
> +unset_intr_relay(struct ifcvf_internal *internal)
> +{
> + void *status;
> +
> + if (internal->intr_tid) {
> + pthread_cancel(internal->intr_tid);
> + pthread_join(internal->intr_tid, &status);
> + }
> + internal->intr_tid = 0;
> +
> + if (internal->csc_fd >= 0)
> + close(internal->csc_fd);
> + internal->csc_fd = -1;
> +
> + return 0;
> +}
> +
> static int
> update_datapath(struct ifcvf_internal *internal)
> {
> @@ -584,10 +687,16 @@ struct rte_vdpa_dev_info {
> if (ret)
> goto err;
>
> + ret = setup_intr_relay(internal);
> + if (ret)
> + goto err;
> +
But this is not needed for net, right? As I said, we should
include validation for net also.
Thanks,
Chenbo
> rte_atomic32_set(&internal->running, 1);
> } else if (rte_atomic32_read(&internal->running) &&
> (!rte_atomic32_read(&internal->started) ||
> !rte_atomic32_read(&internal->dev_attached))) {
> + ret = unset_intr_relay(internal);
> +
> ret = unset_notify_relay(internal);
> if (ret)
> goto err;
> @@ -880,6 +989,9 @@ struct rte_vdpa_dev_info {
> /* stop the direct IO data path */
> unset_notify_relay(internal);
> vdpa_ifcvf_stop(internal);
> +
> + unset_intr_relay(internal);
> +
> vdpa_disable_vfio_intr(internal);
>
> ret = rte_vhost_host_notifier_ctrl(vid, RTE_VHOST_QUEUE_ALL, false);
> --
> 1.8.3.1
HI Chenbo,
Thanks for your reply.
My reply is inline.
> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Friday, May 13, 2022 10:53 AM
> To: Pei, Andy <andy.pei@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Cao, Gang <gang.cao@intel.com>; Liu,
> Changpeng <changpeng.liu@intel.com>
> Subject: RE: [PATCH v7 14/18] vdpa/ifc: add interrupt and handle for virtio
> blk
>
> > -----Original Message-----
> > From: Pei, Andy <andy.pei@intel.com>
> > Sent: Wednesday, April 27, 2022 4:30 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com;
> > Cao, Gang <gang.cao@intel.com>; Liu, Changpeng
> > <changpeng.liu@intel.com>
> > Subject: [PATCH v7 14/18] vdpa/ifc: add interrupt and handle for
> > virtio blk
>
> Better be: vdpa/ifc: add interrupt handling for config space
>
Sure. I will fix it in next version.
> >
> > Create a thread to poll and relay config space change interrupt.
> > Use VHOST_USER_SLAVE_CONFIG_CHANGE_MSG to info qemu.
>
> Inform QEMU. You don't need to save words in commit log. The commit log
> should be as detailed as possible to make readers understand quickly what
> the commit is doing :)
>
Sure. I will fix it in next version.
> >
> > Signed-off-by: Andy Pei <andy.pei@intel.com>
> > ---
> > drivers/vdpa/ifc/ifcvf_vdpa.c | 112
> > ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 112 insertions(+)
> >
> > diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c
> > b/drivers/vdpa/ifc/ifcvf_vdpa.c index 5a8cf1c..0e94e1f 100644
> > --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
> > +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
> > @@ -53,7 +53,9 @@ struct ifcvf_internal {
> > int vfio_group_fd;
> > int vfio_dev_fd;
> > pthread_t tid; /* thread for notify relay */
> > + pthread_t intr_tid; /* thread for intr relay */
>
> Thread for virtio-blk config space change interrupt relay
>
Sure.
> > int epfd;
> > + int csc_fd;
>
> csc_epfd
>
OK.
> > int vid;
> > struct rte_vdpa_device *vdev;
> > uint16_t max_queues;
> > @@ -558,6 +560,107 @@ struct rte_vdpa_dev_info {
> > return 0;
> > }
> >
> > +static void
> > +virtio_interrupt_handler(struct ifcvf_internal *internal) {
> > + int vid = internal->vid;
> > + int ret;
> > +
> > + ret = rte_vhost_slave_config_change(vid, 1);
> > + if (ret)
> > + DRV_LOG(ERR, "failed to notify the guest about configuration
> > space change.");
> > +}
> > +
> > +static void *
> > +intr_relay(void *arg)
> > +{
> > + struct ifcvf_internal *internal = (struct ifcvf_internal *)arg;
> > + struct epoll_event csc_event;
> > + struct epoll_event ev;
> > + uint64_t buf;
> > + int nbytes;
> > + int csc_fd, csc_val = 0;
> > +
> > + csc_fd = epoll_create(1);
> > + if (csc_fd < 0) {
> > + DRV_LOG(ERR, "failed to create epoll for config space
> > change.");
> > + return NULL;
> > + }
> > +
> > + ev.events = EPOLLIN | EPOLLPRI | EPOLLRDHUP | EPOLLHUP;
> > + ev.data.fd = rte_intr_fd_get(internal->pdev->intr_handle);
> > + if (epoll_ctl(csc_fd, EPOLL_CTL_ADD,
> > + rte_intr_fd_get(internal->pdev->intr_handle), &ev) < 0) {
> > + DRV_LOG(ERR, "epoll add error: %s", strerror(errno));
> > + return NULL;
>
> Close the epfd and set to -1 if err.
>
I check other epoll usage in DPDK, it seems most usage do not close the epfd and set to -1.
I am not sure whether it is needed.
> > + }
> > +
> > + internal->csc_fd = csc_fd;
> > +
> > + for (;;) {
> > + csc_val = epoll_wait(csc_fd, &csc_event, 1, -1);
> > + if (csc_val < 0) {
> > + if (errno == EINTR)
> > + continue;
> > + DRV_LOG(ERR, "epoll_wait return fail\n");
>
> Save '\n', it's not needed for DRV_LOG. Please check other DRV_LOGs
>
OK
> > + return NULL;
> > + } else if (csc_val == 0) {
> > + continue;
> > + } else {
> > + /* csc_val > 0 */
> > + nbytes = read(csc_event.data.fd, &buf, 8);
> > + if (nbytes < 0) {
> > + if (errno == EINTR || errno == EWOULDBLOCK)
>
> EAGAIN should also be this case?
>
Yes, it will be add in next version.
> > + continue;
> > + DRV_LOG(ERR, "Error reading from file
> > descriptor %d: %s\n",
> > + csc_event.data.fd,
> > + strerror(errno));
> > + return NULL;
> > + } else if (nbytes == 0) {
> > + DRV_LOG(ERR, "Read nothing from file
> > descriptor %d\n",
> > + csc_event.data.fd);
> > + continue;
> > + } else {
> > + virtio_interrupt_handler(internal);
> > + }
> > + }
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int
> > +setup_intr_relay(struct ifcvf_internal *internal) {
> > + int ret;
> > +
> > + ret = pthread_create(&internal->intr_tid, NULL, intr_relay,
> > + (void *)internal);
>
> EAL API: rte_ctrl_thread_create, will be preferred.
>
Sure, I will use " rte_ctrl_thread_create " in next version.
> > + if (ret) {
> > + DRV_LOG(ERR, "failed to create notify relay pthread.");
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
> > +static int
> > +unset_intr_relay(struct ifcvf_internal *internal) {
> > + void *status;
> > +
> > + if (internal->intr_tid) {
> > + pthread_cancel(internal->intr_tid);
> > + pthread_join(internal->intr_tid, &status);
> > + }
> > + internal->intr_tid = 0;
> > +
> > + if (internal->csc_fd >= 0)
> > + close(internal->csc_fd);
> > + internal->csc_fd = -1;
> > +
> > + return 0;
> > +}
> > +
> > static int
> > update_datapath(struct ifcvf_internal *internal) { @@ -584,10
> > +687,16 @@ struct rte_vdpa_dev_info {
> > if (ret)
> > goto err;
> >
> > + ret = setup_intr_relay(internal);
> > + if (ret)
> > + goto err;
> > +
>
> But this is not needed for net, right? As I said, we should include validation
> for net also.
>
> Thanks,
> Chenbo
>
For net device, especially the harden virtio device, fabric plug in or out will cause config change.
I think net device may also need this interrupt, but I am not sure.
> > rte_atomic32_set(&internal->running, 1);
> > } else if (rte_atomic32_read(&internal->running) &&
> > (!rte_atomic32_read(&internal->started) ||
> > !rte_atomic32_read(&internal->dev_attached))) {
> > + ret = unset_intr_relay(internal);
> > +
> > ret = unset_notify_relay(internal);
> > if (ret)
> > goto err;
> > @@ -880,6 +989,9 @@ struct rte_vdpa_dev_info {
> > /* stop the direct IO data path */
> > unset_notify_relay(internal);
> > vdpa_ifcvf_stop(internal);
> > +
> > + unset_intr_relay(internal);
> > +
> > vdpa_disable_vfio_intr(internal);
> >
> > ret = rte_vhost_host_notifier_ctrl(vid, RTE_VHOST_QUEUE_ALL,
> false);
> > --
> > 1.8.3.1
>
@@ -53,7 +53,9 @@ struct ifcvf_internal {
int vfio_group_fd;
int vfio_dev_fd;
pthread_t tid; /* thread for notify relay */
+ pthread_t intr_tid; /* thread for intr relay */
int epfd;
+ int csc_fd;
int vid;
struct rte_vdpa_device *vdev;
uint16_t max_queues;
@@ -558,6 +560,107 @@ struct rte_vdpa_dev_info {
return 0;
}
+static void
+virtio_interrupt_handler(struct ifcvf_internal *internal)
+{
+ int vid = internal->vid;
+ int ret;
+
+ ret = rte_vhost_slave_config_change(vid, 1);
+ if (ret)
+ DRV_LOG(ERR, "failed to notify the guest about configuration space change.");
+}
+
+static void *
+intr_relay(void *arg)
+{
+ struct ifcvf_internal *internal = (struct ifcvf_internal *)arg;
+ struct epoll_event csc_event;
+ struct epoll_event ev;
+ uint64_t buf;
+ int nbytes;
+ int csc_fd, csc_val = 0;
+
+ csc_fd = epoll_create(1);
+ if (csc_fd < 0) {
+ DRV_LOG(ERR, "failed to create epoll for config space change.");
+ return NULL;
+ }
+
+ ev.events = EPOLLIN | EPOLLPRI | EPOLLRDHUP | EPOLLHUP;
+ ev.data.fd = rte_intr_fd_get(internal->pdev->intr_handle);
+ if (epoll_ctl(csc_fd, EPOLL_CTL_ADD,
+ rte_intr_fd_get(internal->pdev->intr_handle), &ev) < 0) {
+ DRV_LOG(ERR, "epoll add error: %s", strerror(errno));
+ return NULL;
+ }
+
+ internal->csc_fd = csc_fd;
+
+ for (;;) {
+ csc_val = epoll_wait(csc_fd, &csc_event, 1, -1);
+ if (csc_val < 0) {
+ if (errno == EINTR)
+ continue;
+ DRV_LOG(ERR, "epoll_wait return fail\n");
+ return NULL;
+ } else if (csc_val == 0) {
+ continue;
+ } else {
+ /* csc_val > 0 */
+ nbytes = read(csc_event.data.fd, &buf, 8);
+ if (nbytes < 0) {
+ if (errno == EINTR || errno == EWOULDBLOCK)
+ continue;
+ DRV_LOG(ERR, "Error reading from file descriptor %d: %s\n",
+ csc_event.data.fd,
+ strerror(errno));
+ return NULL;
+ } else if (nbytes == 0) {
+ DRV_LOG(ERR, "Read nothing from file descriptor %d\n",
+ csc_event.data.fd);
+ continue;
+ } else {
+ virtio_interrupt_handler(internal);
+ }
+ }
+ }
+
+ return NULL;
+}
+
+static int
+setup_intr_relay(struct ifcvf_internal *internal)
+{
+ int ret;
+
+ ret = pthread_create(&internal->intr_tid, NULL, intr_relay,
+ (void *)internal);
+ if (ret) {
+ DRV_LOG(ERR, "failed to create notify relay pthread.");
+ return -1;
+ }
+ return 0;
+}
+
+static int
+unset_intr_relay(struct ifcvf_internal *internal)
+{
+ void *status;
+
+ if (internal->intr_tid) {
+ pthread_cancel(internal->intr_tid);
+ pthread_join(internal->intr_tid, &status);
+ }
+ internal->intr_tid = 0;
+
+ if (internal->csc_fd >= 0)
+ close(internal->csc_fd);
+ internal->csc_fd = -1;
+
+ return 0;
+}
+
static int
update_datapath(struct ifcvf_internal *internal)
{
@@ -584,10 +687,16 @@ struct rte_vdpa_dev_info {
if (ret)
goto err;
+ ret = setup_intr_relay(internal);
+ if (ret)
+ goto err;
+
rte_atomic32_set(&internal->running, 1);
} else if (rte_atomic32_read(&internal->running) &&
(!rte_atomic32_read(&internal->started) ||
!rte_atomic32_read(&internal->dev_attached))) {
+ ret = unset_intr_relay(internal);
+
ret = unset_notify_relay(internal);
if (ret)
goto err;
@@ -880,6 +989,9 @@ struct rte_vdpa_dev_info {
/* stop the direct IO data path */
unset_notify_relay(internal);
vdpa_ifcvf_stop(internal);
+
+ unset_intr_relay(internal);
+
vdpa_disable_vfio_intr(internal);
ret = rte_vhost_host_notifier_ctrl(vid, RTE_VHOST_QUEUE_ALL, false);