RE: [PATCH v7 14/18] vdpa/ifc: add interrupt and handle for virtio blk

["Pei, Andy" <andy.pei@intel.com>]

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
>