| Message ID | 1524149293-12658-4-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 19 Apr 2018 16:48:06 +0200 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > The Sub channel event callback is threaded using workqueues. > The work uses the FSM introducing the VFIO_CCW_EVENT_SCH_EVENT > event. I don't think this is a good name; after all, all of the events are events for the subchannel :) This seems to be more of a "we need to update the schib" event... VFIO_CCW_EVENT_SCHIB_CHANGED? _SCH_CHANGED? _UPDATE_NEEDED? Tbh, I'm not quite sure this makes sense for me yet... will continue reading, but this probably needs a 'why'. > The update of the SCHIB is now done inside the FSM function. > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > --- > drivers/s390/cio/vfio_ccw_drv.c | 33 +++++++++++++-------------------- > drivers/s390/cio/vfio_ccw_fsm.c | 23 +++++++++++++++++++++++ > drivers/s390/cio/vfio_ccw_private.h | 3 +++ > 3 files changed, 39 insertions(+), 20 deletions(-)
On 25/04/2018 10:25, Cornelia Huck wrote: > On Thu, 19 Apr 2018 16:48:06 +0200 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >> The Sub channel event callback is threaded using workqueues. >> The work uses the FSM introducing the VFIO_CCW_EVENT_SCH_EVENT >> event. > I don't think this is a good name; after all, all of the events are > events for the subchannel :) > > This seems to be more of a "we need to update the schib" event... > VFIO_CCW_EVENT_SCHIB_CHANGED? _SCH_CHANGED? _UPDATE_NEEDED? > > Tbh, I'm not quite sure this makes sense for me yet... will continue > reading, but this probably needs a 'why'. SCHIB_CHANGED or something like this sounds better indeed. :) > >> The update of the SCHIB is now done inside the FSM function. >> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_drv.c | 33 +++++++++++++-------------------- >> drivers/s390/cio/vfio_ccw_fsm.c | 23 +++++++++++++++++++++++ >> drivers/s390/cio/vfio_ccw_private.h | 3 +++ >> 3 files changed, 39 insertions(+), 20 deletions(-)
On Thu, 26 Apr 2018 14:59:54 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Pierre Morel <pmorel@linux.vnet.ibm.com> [2018-04-19 16:48:06 +0200]: > > > The Sub channel event callback is threaded using workqueues. > > The work uses the FSM introducing the VFIO_CCW_EVENT_SCH_EVENT > > event. > > The update of the SCHIB is now done inside the FSM function. > > > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > > --- > > drivers/s390/cio/vfio_ccw_drv.c | 33 +++++++++++++-------------------- > > drivers/s390/cio/vfio_ccw_fsm.c | 23 +++++++++++++++++++++++ > > drivers/s390/cio/vfio_ccw_private.h | 3 +++ > > 3 files changed, 39 insertions(+), 20 deletions(-) > > > > @@ -171,28 +181,11 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch) > > static int vfio_ccw_sch_event(struct subchannel *sch, int process) > > { > > struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); > > - unsigned long flags; > > > > - spin_lock_irqsave(sch->lock, flags); > > if (!device_is_registered(&sch->dev)) > > - goto out_unlock; > > - > > - if (work_pending(&sch->todo_work)) > > - goto out_unlock; > Just realized that this has a bug in the orignal implementation. For > error out this should return -EAGAIN. We'd need a separated fix on > this. Indeed. Will you send a patch, or should I hack something up? > > > - > > - if (cio_update_schib(sch)) { > > - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER); > > - goto out_unlock; > > - } > > - > > - private = dev_get_drvdata(&sch->dev); > > - if (private->state == VFIO_CCW_STATE_NOT_OPER) { > > - private->state = private->mdev ? VFIO_CCW_STATE_IDLE : > > - VFIO_CCW_STATE_STANDBY; > > - } > This hunk was toatally removed, and this is fine because? > > > - > > -out_unlock: > > - spin_unlock_irqrestore(sch->lock, flags); > > + return -1; > -1 is not a valid code. -ENODEV looks more fitting, if we decide to go with this rework. > > > + WARN_ON(work_pending(&private->event_work)); > > + queue_work(vfio_ccw_work_q, &private->event_work); > > > > return 0; > > } I'm wondering why this should always be done via a workqueue. It seems the other subchannel types try to do as much as possible immediately? (And returning -EAGAIN already triggers the css code to schedule another call later.)
On 30/04/2018 17:28, Cornelia Huck wrote: > On Thu, 26 Apr 2018 14:59:54 +0800 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >> * Pierre Morel <pmorel@linux.vnet.ibm.com> [2018-04-19 16:48:06 +0200]: >> >>> The Sub channel event callback is threaded using workqueues. >>> The work uses the FSM introducing the VFIO_CCW_EVENT_SCH_EVENT >>> event. >>> The update of the SCHIB is now done inside the FSM function. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >>> --- >>> drivers/s390/cio/vfio_ccw_drv.c | 33 +++++++++++++-------------------- >>> drivers/s390/cio/vfio_ccw_fsm.c | 23 +++++++++++++++++++++++ >>> drivers/s390/cio/vfio_ccw_private.h | 3 +++ >>> 3 files changed, 39 insertions(+), 20 deletions(-) >>> >>> @@ -171,28 +181,11 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch) >>> static int vfio_ccw_sch_event(struct subchannel *sch, int process) >>> { >>> struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); >>> - unsigned long flags; >>> >>> - spin_lock_irqsave(sch->lock, flags); >>> if (!device_is_registered(&sch->dev)) >>> - goto out_unlock; >>> - >>> - if (work_pending(&sch->todo_work)) >>> - goto out_unlock; >> Just realized that this has a bug in the orignal implementation. For >> error out this should return -EAGAIN. We'd need a separated fix on >> this. > Indeed. Will you send a patch, or should I hack something up? > >>> - >>> - if (cio_update_schib(sch)) { >>> - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER); >>> - goto out_unlock; >>> - } >>> - >>> - private = dev_get_drvdata(&sch->dev); >>> - if (private->state == VFIO_CCW_STATE_NOT_OPER) { >>> - private->state = private->mdev ? VFIO_CCW_STATE_IDLE : >>> - VFIO_CCW_STATE_STANDBY; >>> - } >> This hunk was toatally removed, and this is fine because? The first part is moved to fsm_sch_event() The second part disapear per design as state changes are done inside the FSM. >> >>> - >>> -out_unlock: >>> - spin_unlock_irqrestore(sch->lock, flags); >>> + return -1; >> -1 is not a valid code. > -ENODEV looks more fitting, if we decide to go with this rework. :) yes, forgot the -1 from the first tests. > >>> + WARN_ON(work_pending(&private->event_work)); >>> + queue_work(vfio_ccw_work_q, &private->event_work); >>> >>> return 0; >>> } > I'm wondering why this should always be done via a workqueue. It seems > the other subchannel types try to do as much as possible immediately? Doing things inside the top half is not very friendly with the system. The goal of the patch is to build a clean atomic state machine. Allowing the use of mutexes insures atomicity. I notice that I forgot to point this out in the cover letter although it is one of the design key. I will update the cover letter. > (And returning -EAGAIN already triggers the css code to schedule > another call later.) Yes, if(work_pending()) return -EAGAIN Thanks for the review Pierre >
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index f1b158c..8a91eee 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -77,6 +77,15 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) private->state = VFIO_CCW_STATE_IDLE; } +static void vfio_ccw_sch_event_todo(struct work_struct *work) +{ + struct vfio_ccw_private *private; + + private = container_of(work, struct vfio_ccw_private, event_work); + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_SCH_EVENT); +} + + /* * Css driver callbacks */ @@ -125,6 +134,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) goto out_disable; INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo); + INIT_WORK(&private->event_work, vfio_ccw_sch_event_todo); atomic_set(&private->avail, 1); private->state = VFIO_CCW_STATE_STANDBY; @@ -171,28 +181,11 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch) static int vfio_ccw_sch_event(struct subchannel *sch, int process) { struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); - unsigned long flags; - spin_lock_irqsave(sch->lock, flags); if (!device_is_registered(&sch->dev)) - goto out_unlock; - - if (work_pending(&sch->todo_work)) - goto out_unlock; - - if (cio_update_schib(sch)) { - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER); - goto out_unlock; - } - - private = dev_get_drvdata(&sch->dev); - if (private->state == VFIO_CCW_STATE_NOT_OPER) { - private->state = private->mdev ? VFIO_CCW_STATE_IDLE : - VFIO_CCW_STATE_STANDBY; - } - -out_unlock: - spin_unlock_irqrestore(sch->lock, flags); + return -1; + WARN_ON(work_pending(&private->event_work)); + queue_work(vfio_ccw_work_q, &private->event_work); return 0; } diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index 2f3108d..13751b4 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -183,6 +183,24 @@ static int fsm_irq(struct vfio_ccw_private *private, } /* + * Got a sub-channel event . + */ +static int fsm_sch_event(struct vfio_ccw_private *private, + enum vfio_ccw_event event) +{ + unsigned long flags; + int ret = private->state; + struct subchannel *sch = private->sch; + + spin_lock_irqsave(sch->lock, flags); + if (cio_update_schib(sch)) + ret = VFIO_CCW_STATE_NOT_OPER; + spin_unlock_irqrestore(sch->lock, flags); + + return ret; +} + +/* * Device statemachine */ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { @@ -190,25 +208,30 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_nop, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq, + [VFIO_CCW_EVENT_SCH_EVENT] = fsm_nop, }, [VFIO_CCW_STATE_STANDBY] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_SCH_EVENT] = fsm_sch_event, }, [VFIO_CCW_STATE_IDLE] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_SCH_EVENT] = fsm_sch_event, }, [VFIO_CCW_STATE_BOXED] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_SCH_EVENT] = fsm_sch_event, }, [VFIO_CCW_STATE_BUSY] = { [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy, [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq, + [VFIO_CCW_EVENT_SCH_EVENT] = fsm_sch_event, }, }; diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index f526b18..3284e64 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -33,6 +33,7 @@ * @scsw: scsw info * @io_trigger: eventfd ctx for signaling userspace I/O results * @io_work: work for deferral process of I/O handling + * @event_work: work for deferral process of sub-channel event */ struct vfio_ccw_private { struct subchannel *sch; @@ -49,6 +50,7 @@ struct vfio_ccw_private { struct eventfd_ctx *io_trigger; struct work_struct io_work; + struct work_struct event_work; } __aligned(8); extern int vfio_ccw_mdev_reg(struct subchannel *sch); @@ -76,6 +78,7 @@ enum vfio_ccw_event { VFIO_CCW_EVENT_NOT_OPER, VFIO_CCW_EVENT_IO_REQ, VFIO_CCW_EVENT_INTERRUPT, + VFIO_CCW_EVENT_SCH_EVENT, /* last element! */ NR_VFIO_CCW_EVENTS };
The Sub channel event callback is threaded using workqueues. The work uses the FSM introducing the VFIO_CCW_EVENT_SCH_EVENT event. The update of the SCHIB is now done inside the FSM function. Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> --- drivers/s390/cio/vfio_ccw_drv.c | 33 +++++++++++++-------------------- drivers/s390/cio/vfio_ccw_fsm.c | 23 +++++++++++++++++++++++ drivers/s390/cio/vfio_ccw_private.h | 3 +++ 3 files changed, 39 insertions(+), 20 deletions(-)