HI Chenbo,
Thanks for your reply.
My reply is inline.
> -----Original Message-----
> From: Xia, Chenbo <chenbo....@intel.com>
> Sent: Friday, May 13, 2022 10:53 AM
> To: Pei, Andy <andy....@intel.com>; dev@dpdk.org
> Cc: maxime.coque...@redhat.com; Cao, Gang <gang....@intel.com>; Liu,
> Changpeng <changpeng....@intel.com>
> Subject: RE: [PATCH v7 14/18] vdpa/ifc: add interrupt and handle for virtio
> blk
>
> > -----Original Message-----
> > From: Pei, Andy <andy....@intel.com>
> > Sent: Wednesday, April 27, 2022 4:30 PM
> > To: dev@dpdk.org
> > Cc: Xia, Chenbo <chenbo....@intel.com>; maxime.coque...@redhat.com;
> > Cao, Gang <gang....@intel.com>; Liu, Changpeng
> > <changpeng....@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....@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
>
