Re: [PATCH 10/10] vfio: ccw: Let user wait when busy on IO

[Pierre Morel]

On 25/04/2018 10:48, Cornelia Huck wrote:
> On Thu, 19 Apr 2018 16:48:13 +0200
> Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:
>
> > In the current implementation, we do not want to start a new SSCH
> > command before the last one ends.
> >
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/s390/cio/vfio_ccw_fsm.c     |  3 +++
> >   drivers/s390/cio/vfio_ccw_ops.c     | 21 ++++++++++++++++++++-
> >   drivers/s390/cio/vfio_ccw_private.h |  4 +++-
> >   3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index b77b8ad..4140292 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -195,6 +195,9 @@ static int fsm_irq(struct vfio_ccw_private *private)
> >   	if (private->io_trigger)
> >   		eventfd_signal(private->io_trigger, 1);
> >   
> > +	if (private->io_completion)
> > +		complete(private->io_completion);
> > +
> >   	return VFIO_CCW_STATE_IDLE;
> >   }
> >   
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> > index f0f4071..346532d 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -171,6 +171,8 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
> >   	struct vfio_ccw_private *private;
> >   	struct ccw_io_region *region;
> >   	union scsw *scsw;
> > +	int max_retries = 5;
> > +	DECLARE_COMPLETION_ONSTACK(completion);
> >   
> >   	if (*ppos + count > sizeof(*region))
> >   		return -EINVAL;
> > @@ -185,7 +187,24 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
> >   	if ((scsw->cmd.fctl & SCSW_FCTL_START_FUNC) != SCSW_FCTL_START_FUNC)
> >   		return -EINVAL;
> >   
> > -	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_SSCH_REQ);
> > +	do {
> > +		switch (private->state) {
> > +		case VFIO_CCW_STATE_BUSY:
> > +			private->io_completion = &completion;
> > +			wait_for_completion(&completion);
> > +			break;
> > +		case VFIO_CCW_STATE_IDLE:
> > +			if (!vfio_ccw_fsm_event(private,
> > +						VFIO_CCW_EVENT_SSCH_REQ))
> > +				return count;
> > +			break;
> > +		default:
> > +			return -EBUSY;
> > +		}
> > +	} while (max_retries--);
> I really don't think we want to go there. If we are busy, generate an
> indication to that respect, but don't retry. My preferred approach
> would be to keep the "we're busy" times as small as possible and let
> the host channel subsystem handle any further races. We can't make that
> bulletproof anyway, so no reason to make life more difficult for us.

OK, clear.

Thanks

Pierre

> > +
> > +	if (max_retries <= 0)
> > +		return -EBUSY;
> >   	if (region->ret_code != 0)
> >   		return region->ret_code;
> >   
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> > index dbef727..7cca078 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -39,6 +39,7 @@ struct vfio_ccw_private {
> >   	struct subchannel	*sch;
> >   	int			state;
> >   	struct completion	*completion;
> > +	struct completion	*io_completion;
> >   	atomic_t		avail;
> >   	struct mdev_device	*mdev;
> >   	struct notifier_block	nb;
> > @@ -93,12 +94,13 @@ enum vfio_ccw_event {
> >   typedef int (fsm_func_t)(struct vfio_ccw_private *);
> >   extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
> >   
> > -static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
> > +static inline int vfio_ccw_fsm_event(struct vfio_ccw_private *private,
> >   				     int event)
> >   {
> >   	mutex_lock(&private->state_mutex);
> >   	private->state = vfio_ccw_jumptable[private->state][event](private);
> >   	mutex_unlock(&private->state_mutex);
> > +	return private->io_region.ret_code;
> >   }
> >   
> >   extern struct workqueue_struct *vfio_ccw_work_q;


--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany