Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Cornelia Huck 
On Tue, 5 Jun 2018 16:21:03 +0200
Pierre Morel  wrote:

> On 05/06/2018 15:35, Cornelia Huck wrote:
> > On Tue, 5 Jun 2018 15:10:11 +0200
> > Pierre Morel  wrote:
> >  
> >> On 05/06/2018 13:38, Cornelia Huck wrote:  
> >>> On Fri, 25 May 2018 12:21:14 +0200
> >>> Pierre Morel  wrote:
> >>> 
>  We use mutex around the FSM function call to make the FSM
>  event handling and state change atomic.  
> >>> I'm still not really clear as to what this mutex is supposed to
> >>> serialize:
> >>>
> >>> - Modification of the state?
> >>> - Any calls in the state machine?
> >>> - A combination? (That would imply that we only deal with the state in
> >>> the state machine.)  
> >> yes to all  
> > But wouldn't that imply that you need to either take the mutex if you
> > do something dependent on the state, or fire an event in that case?  
> 
> Yes, if it is not I forgot something important (like I did in patch 10)
> vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.
> 
> I have some cases where I do not respect this.
> This is false and I must handle this with a new private variable,
> this is where I test the state after having fired an event.
> I will need to change this, in quiesce, reset, probe and open (others?).

But isn't that inherently racy? Even if you check the return code from
the state machine change, it might have changed in the meantime again.

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Cornelia Huck 
On Tue, 5 Jun 2018 16:21:03 +0200
Pierre Morel  wrote:

> On 05/06/2018 15:35, Cornelia Huck wrote:
> > On Tue, 5 Jun 2018 15:10:11 +0200
> > Pierre Morel  wrote:
> >  
> >> On 05/06/2018 13:38, Cornelia Huck wrote:  
> >>> On Fri, 25 May 2018 12:21:14 +0200
> >>> Pierre Morel  wrote:
> >>> 
>  We use mutex around the FSM function call to make the FSM
>  event handling and state change atomic.  
> >>> I'm still not really clear as to what this mutex is supposed to
> >>> serialize:
> >>>
> >>> - Modification of the state?
> >>> - Any calls in the state machine?
> >>> - A combination? (That would imply that we only deal with the state in
> >>> the state machine.)  
> >> yes to all  
> > But wouldn't that imply that you need to either take the mutex if you
> > do something dependent on the state, or fire an event in that case?  
> 
> Yes, if it is not I forgot something important (like I did in patch 10)
> vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.
> 
> I have some cases where I do not respect this.
> This is false and I must handle this with a new private variable,
> this is where I test the state after having fired an event.
> I will need to change this, in quiesce, reset, probe and open (others?).

But isn't that inherently racy? Even if you check the return code from
the state machine change, it might have changed in the meantime again.

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Pierre Morel 

On 05/06/2018 15:35, Cornelia Huck wrote:

On Tue, 5 Jun 2018 15:10:11 +0200
Pierre Morel  wrote:


On 05/06/2018 13:38, Cornelia Huck wrote:

On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel  wrote:
  

We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
the state machine.)

yes to all

But wouldn't that imply that you need to either take the mutex if you
do something dependent on the state, or fire an event in that case?


Yes, if it is not I forgot something important (like I did in patch 10)
vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.

I have some cases where I do not respect this.
This is false and I must handle this with a new private variable,
this is where I test the state after having fired an event.
I will need to change this, in quiesce, reset, probe and open (others?).




  

Signed-off-by: Pierre Morel 
---
   drivers/s390/cio/vfio_ccw_drv.c | 3 +--
   drivers/s390/cio/vfio_ccw_private.h | 3 +++
   2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
   
   	private = container_of(work, struct vfio_ccw_private, io_work);

vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-   if (private->mdev)
-   private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?

It is moved to fsm_irq() which is called under mutex.
fsm_irq() returns VFIO_CCW_STATE_IDLE.

So, should that go into another patch?


I will see if I can put it inside the patch 01/10 moving state change 
out of IRQ context.



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

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Pierre Morel 

On 05/06/2018 15:35, Cornelia Huck wrote:

On Tue, 5 Jun 2018 15:10:11 +0200
Pierre Morel  wrote:


On 05/06/2018 13:38, Cornelia Huck wrote:

On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel  wrote:
  

We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
the state machine.)

yes to all

But wouldn't that imply that you need to either take the mutex if you
do something dependent on the state, or fire an event in that case?


Yes, if it is not I forgot something important (like I did in patch 10)
vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.

I have some cases where I do not respect this.
This is false and I must handle this with a new private variable,
this is where I test the state after having fired an event.
I will need to change this, in quiesce, reset, probe and open (others?).




  

Signed-off-by: Pierre Morel 
---
   drivers/s390/cio/vfio_ccw_drv.c | 3 +--
   drivers/s390/cio/vfio_ccw_private.h | 3 +++
   2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
   
   	private = container_of(work, struct vfio_ccw_private, io_work);

vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-   if (private->mdev)
-   private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?

It is moved to fsm_irq() which is called under mutex.
fsm_irq() returns VFIO_CCW_STATE_IDLE.

So, should that go into another patch?


I will see if I can put it inside the patch 01/10 moving state change 
out of IRQ context.



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

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Cornelia Huck 
On Tue, 5 Jun 2018 15:10:11 +0200
Pierre Morel  wrote:

> On 05/06/2018 13:38, Cornelia Huck wrote:
> > On Fri, 25 May 2018 12:21:14 +0200
> > Pierre Morel  wrote:
> >  
> >> We use mutex around the FSM function call to make the FSM
> >> event handling and state change atomic.  
> > I'm still not really clear as to what this mutex is supposed to
> > serialize:
> >
> > - Modification of the state?
> > - Any calls in the state machine?
> > - A combination? (That would imply that we only deal with the state in
> >the state machine.)  
> 
> yes to all

But wouldn't that imply that you need to either take the mutex if you
do something dependent on the state, or fire an event in that case?

> 
> >  
> >> Signed-off-by: Pierre Morel 
> >> ---
> >>   drivers/s390/cio/vfio_ccw_drv.c | 3 +--
> >>   drivers/s390/cio/vfio_ccw_private.h | 3 +++
> >>   2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c 
> >> b/drivers/s390/cio/vfio_ccw_drv.c
> >> index 6b7112e..98951d5 100644
> >> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct 
> >> *work)
> >>   
> >>private = container_of(work, struct vfio_ccw_private, io_work);
> >>vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> >> -  if (private->mdev)
> >> -  private->state = VFIO_CCW_STATE_IDLE;  
> > Looks like an unrelated change? If you want to do all state changes
> > under the mutex, that should rather be moved than deleted, shouldn't it?  
> 
> It is moved to fsm_irq() which is called under mutex.
> fsm_irq() returns VFIO_CCW_STATE_IDLE.

So, should that go into another patch?

> 
> >  
> >>   }
> >>   
> >>   static void vfio_ccw_sch_event_todo(struct work_struct *work)  
> 
>

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Cornelia Huck 
On Tue, 5 Jun 2018 15:10:11 +0200
Pierre Morel  wrote:

> On 05/06/2018 13:38, Cornelia Huck wrote:
> > On Fri, 25 May 2018 12:21:14 +0200
> > Pierre Morel  wrote:
> >  
> >> We use mutex around the FSM function call to make the FSM
> >> event handling and state change atomic.  
> > I'm still not really clear as to what this mutex is supposed to
> > serialize:
> >
> > - Modification of the state?
> > - Any calls in the state machine?
> > - A combination? (That would imply that we only deal with the state in
> >the state machine.)  
> 
> yes to all

But wouldn't that imply that you need to either take the mutex if you
do something dependent on the state, or fire an event in that case?

> 
> >  
> >> Signed-off-by: Pierre Morel 
> >> ---
> >>   drivers/s390/cio/vfio_ccw_drv.c | 3 +--
> >>   drivers/s390/cio/vfio_ccw_private.h | 3 +++
> >>   2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c 
> >> b/drivers/s390/cio/vfio_ccw_drv.c
> >> index 6b7112e..98951d5 100644
> >> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct 
> >> *work)
> >>   
> >>private = container_of(work, struct vfio_ccw_private, io_work);
> >>vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> >> -  if (private->mdev)
> >> -  private->state = VFIO_CCW_STATE_IDLE;  
> > Looks like an unrelated change? If you want to do all state changes
> > under the mutex, that should rather be moved than deleted, shouldn't it?  
> 
> It is moved to fsm_irq() which is called under mutex.
> fsm_irq() returns VFIO_CCW_STATE_IDLE.

So, should that go into another patch?

> 
> >  
> >>   }
> >>   
> >>   static void vfio_ccw_sch_event_todo(struct work_struct *work)  
> 
>

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Pierre Morel 

On 05/06/2018 13:38, Cornelia Huck wrote:

On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel  wrote:


We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
   the state machine.)


yes to all




Signed-off-by: Pierre Morel 
---
  drivers/s390/cio/vfio_ccw_drv.c | 3 +--
  drivers/s390/cio/vfio_ccw_private.h | 3 +++
  2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  
  	private = container_of(work, struct vfio_ccw_private, io_work);

vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-   if (private->mdev)
-   private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?


It is moved to fsm_irq() which is called under mutex.
fsm_irq() returns VFIO_CCW_STATE_IDLE.




  }
  
  static void vfio_ccw_sch_event_todo(struct work_struct *work)



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

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Pierre Morel 

On 05/06/2018 13:38, Cornelia Huck wrote:

On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel  wrote:


We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
   the state machine.)


yes to all




Signed-off-by: Pierre Morel 
---
  drivers/s390/cio/vfio_ccw_drv.c | 3 +--
  drivers/s390/cio/vfio_ccw_private.h | 3 +++
  2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  
  	private = container_of(work, struct vfio_ccw_private, io_work);

vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-   if (private->mdev)
-   private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?


It is moved to fsm_irq() which is called under mutex.
fsm_irq() returns VFIO_CCW_STATE_IDLE.




  }
  
  static void vfio_ccw_sch_event_todo(struct work_struct *work)



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

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Cornelia Huck 
On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel  wrote:

> We use mutex around the FSM function call to make the FSM
> event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
  the state machine.)

> 
> Signed-off-by: Pierre Morel 
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 3 +--
>  drivers/s390/cio/vfio_ccw_private.h | 3 +++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 6b7112e..98951d5 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>   private = container_of(work, struct vfio_ccw_private, io_work);
>   vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> - if (private->mdev)
> - private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?

>  }
>  
>  static void vfio_ccw_sch_event_todo(struct work_struct *work)

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-06-05 Thread Cornelia Huck 
On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel  wrote:

> We use mutex around the FSM function call to make the FSM
> event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
  the state machine.)

> 
> Signed-off-by: Pierre Morel 
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 3 +--
>  drivers/s390/cio/vfio_ccw_private.h | 3 +++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 6b7112e..98951d5 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>   private = container_of(work, struct vfio_ccw_private, io_work);
>   vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> - if (private->mdev)
> - private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?

>  }
>  
>  static void vfio_ccw_sch_event_todo(struct work_struct *work)

[PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-05-25 Thread Pierre Morel 
We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

Signed-off-by: Pierre Morel 
---
 drivers/s390/cio/vfio_ccw_drv.c | 3 +--
 drivers/s390/cio/vfio_ccw_private.h | 3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 
private = container_of(work, struct vfio_ccw_private, io_work);
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-   if (private->mdev)
-   private->state = VFIO_CCW_STATE_IDLE;
 }
 
 static void vfio_ccw_sch_event_todo(struct work_struct *work)
@@ -118,6 +116,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
return -ENOMEM;
private->sch = sch;
dev_set_drvdata(>dev, private);
+   mutex_init(>state_mutex);
 
spin_lock_irq(sch->lock);
private->state = VFIO_CCW_STATE_NOT_OPER;
diff --git a/drivers/s390/cio/vfio_ccw_private.h 
b/drivers/s390/cio/vfio_ccw_private.h
index 6c74f73..241176c 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -51,6 +51,7 @@ struct vfio_ccw_private {
struct eventfd_ctx  *io_trigger;
struct work_struct  io_work;
struct work_struct  event_work;
+   struct mutexstate_mutex;
 } __aligned(8);
 
 extern int vfio_ccw_mdev_reg(struct subchannel *sch);
@@ -92,7 +93,9 @@ 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,
 int event)
 {
+   mutex_lock(>state_mutex);
private->state = vfio_ccw_jumptable[private->state][event](private);
+   mutex_unlock(>state_mutex);
 }
 
 extern struct workqueue_struct *vfio_ccw_work_q;
-- 
2.7.4

[PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

2018-05-25 Thread Pierre Morel 
We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

Signed-off-by: Pierre Morel 
---
 drivers/s390/cio/vfio_ccw_drv.c | 3 +--
 drivers/s390/cio/vfio_ccw_private.h | 3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 
private = container_of(work, struct vfio_ccw_private, io_work);
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-   if (private->mdev)
-   private->state = VFIO_CCW_STATE_IDLE;
 }
 
 static void vfio_ccw_sch_event_todo(struct work_struct *work)
@@ -118,6 +116,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
return -ENOMEM;
private->sch = sch;
dev_set_drvdata(>dev, private);
+   mutex_init(>state_mutex);
 
spin_lock_irq(sch->lock);
private->state = VFIO_CCW_STATE_NOT_OPER;
diff --git a/drivers/s390/cio/vfio_ccw_private.h 
b/drivers/s390/cio/vfio_ccw_private.h
index 6c74f73..241176c 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -51,6 +51,7 @@ struct vfio_ccw_private {
struct eventfd_ctx  *io_trigger;
struct work_struct  io_work;
struct work_struct  event_work;
+   struct mutexstate_mutex;
 } __aligned(8);
 
 extern int vfio_ccw_mdev_reg(struct subchannel *sch);
@@ -92,7 +93,9 @@ 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,
 int event)
 {
+   mutex_lock(>state_mutex);
private->state = vfio_ccw_jumptable[private->state][event](private);
+   mutex_unlock(>state_mutex);
 }
 
 extern struct workqueue_struct *vfio_ccw_work_q;
-- 
2.7.4