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

Pierre Morel
2018-04-25T14:00:05Z

On 25/04/2018 10:48, Cornelia Huck wrote:
> On Thu, 19 Apr 2018 16:48:13 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> 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@linux.vnet.ibm.com>
>> ---
>> 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