Re: [Qemu-devel] [PATCH 0/3] vfio-ccw: support hsch/csch (kernel part)

[Cornelia Huck]

On Wed, 19 Dec 2018 15:17:19 +0100
Halil Pasic <address@hidden> wrote:

> On Wed, 19 Dec 2018 12:54:42 +0100
> Cornelia Huck <address@hidden> wrote:
> 
> > On Fri, 7 Dec 2018 17:54:23 +0100
> > Halil Pasic <address@hidden> wrote:
> >   
> > > On Fri, 7 Dec 2018 11:05:29 +0100
> > > Cornelia Huck <address@hidden> wrote:
> > >   
> > > > > > I think most of the sorting-out-the-operations stuff should be done 
> > > > > > by
> > > > > > the hardware itself, and we should not really try to enforce 
> > > > > > anything
> > > > > > special in our vfio code.
> > > > > >         
> > > > > 
> > > > > Sounds very reasonable to me. Does this mean you are against Pierre's
> > > > > '[PATCH v3 6/6] vfio: ccw: serialize the write system calls' as it 
> > > > > does
> > > > > not let HW sort out stuff, but enforces sequencing?      
> > > > 
> > > > I have not yet had time to look at that, sorry.
> > > >     
> > > > > 
> > > > >       
> > > > > > For your example, it might be best if a hsch is always accepted and
> > > > > > send on towards the hardware.        
> > > > > 
> > > > > Nod.
> > > > >       
> > > > > > Probably best to reflect back -EAGAIN if
> > > > > > we're currently processing another instruction from another vcpu, so
> > > > > > that the user space caller can retry.        
> > > > > 
> > > > > Hm, not sure how this works together with your previous sentence.     
> > > > >  
> > > > 
> > > > The software layering. We have the kernel layer
> > > > (drivers/s390/cio/vfio_ccw_*) that interacts with the hardware more or
> > > > less directly, and the QEMU layer, which does some writes on regions.
> > > > In the end, the goal is to act on behalf of the guest issuing a
> > > > ssch/hsch/csch, which is from the guest's view a single instruction. We
> > > > should not have the individual "instructions" compete with each other
> > > > so that they run essentially in parallel (kernel layer), but we should
> > > > also not try to impose an artificial ordering as to when instructions
> > > > executed by different vcpus are executed (QEMU layer). Therefore, don't
> > > > try to run an instruction in the kernel when another one is in progress
> > > > for the same subchannel (exclusivity in the kernel), but retry in QEMU
> > > > if needed (no ordering between vcpus imposed).
> > > > 
> > > > In short, don't create strange concurrency issues in the "instruction"
> > > > handling, but make it possible to execute instructions in a
> > > > non-predictable order if the guest does not care about enforcing
> > > > ordering on its side.
> > > >     
> > > 
> > > I'm neither sold on this, nor am I violently opposing it. Will try to
> > > meditate on it some more if any spare cycles arise. Currently I don't
> > > see the benefit of the non-predictable order over plain FCFS. For
> > > example, let's assume we have a ssch "instruction" that 1 second to
> > > complete. Since normally ssch instruction  does not have to process the
> > > channel program, and is thus kind of a constant time operation (now we
> > > do the translation and the pinning as a part of the "instruction), our
> > > strange guest gets jumpy and does a csch after T+0.2s. And on T+0.8 in
> > > desperation follows the whole up with a hsch. If I understand your
> > > proposal correctly, both userspace handlers would spin on -EAGAIN until
> > > T+1. When ssch is done the csch and the hsch would race for who can
> > > be the next. I don't quite get the value of that.  
> > 
> > What would happen on real hardware for such a guest? I would expect
> > that the csch and the hsch would be executed in a random order as well.
> >   
> 
> Yes, they would be executed in random order, but would not wait until the
> ssch is done (and especially not wait until the channel program gets
> translated). AFAIR bot cancel the start function immediately -- if any
> pending.
> 
> Furthermore the point where the race is decided is changing the function
> control bits -- the update needs to be an interlocked one obviously.
> 
> What I want to say, there is no merit in waiting -- one second in the
> example. At some point it needs to be decided who is considered first,
> and artificially procrastinating this decision does not do us any good,
> because we may end up with otherwise unlikely behavior.

You've really lost me here :( I fear you're criticizing something I
don't want to implement; I'll write some code, that should make things
much easier to discuss.

> 
> > My point is that it is up to the guest to impose an order on the
> > execution of instructions, if wanted. We should not try to guess
> > anything; I think that would make the implementation needlessly complex.
> >   
> 
> I'm not for guessing stuff, but rather for sticking to the architecture.
> 
> > >   
> > > > >       
> > > > > > Same for ssch, if another ssch is
> > > > > > already being processed. We *could* reflect cc 2 if the fctl
> > > > > > bit is already set, but that won't do for csch, so it is
> > > > > > probably best to have the hardware figure that out in any case.
> > > > > >         
> > > > > 
> > > > > We just need to be careful about avoiding races if we let hw sort
> > > > > out things. If an ssch is issued with the start function pending
> > > > > the correct response is cc 2.       
> > > > 
> > > > But sending it on to the hardware will give us that cc 2, no?
> > > >     
> > > > >       
> > > > > > If I read the code correctly, we currently reflect -EBUSY and
> > > > > > not -EAGAIN if we get a ssch request while already processing
> > > > > > another one. QEMU hands that back to the guest as a cc 2, which
> > > > > > is not 100% correct. In practice, we don't see this with Linux
> > > > > > guests due to locking.   
> > > > > 
> > > > > Nod, does not happen because of BQL. We currently do the
> > > > > user-space counterpart of vfio_ccw_mdev_write() in BQL context or
> > > > > (i.e. we hold BQL until translation is done and our host ssch()
> > > > > comes back)?      
> > > > 
> > > > The Linux kernel uses the subchannel lock to enforce exclusivity for
> > > > subchannel instructions, so we won't see Linux guests issue
> > > > instructions on different vcpus in parallel, that's what I meant.
> > > >    
> > > 
> > > That is cool. Yet I think the situation with the BQL is relevant.
> > > Because while BQL is held, not only IO instructions on a single
> > > vfio-ccw device are mutually exclusive. AFAIU no other instruction
> > > QEMU instruction handler can engage. And a store subchannel for
> > > device A having to wait until the translation for the start
> > > subchannel on device B is done is not the most scary thing I can
> > > imagine.   
> > 
> > Yes. But we still need to be able to cope with a userspace that does
> > not give us those guarantees.
> >   
> 
> I agree. The point I was trying to make is not that 'We are good, because
> qemu takes care of it!' on the contrary, I wanted to give voice to my
> concern that a guest that has a couple of vfio-ccw devices in use could
> experience performance problems because vfio-ccw holds BQL for long.

TBH, I have no idea how this will scale to many vfio-ccw devices.

> 
> > > > > 
> > > > > I think -EBUSY is the correct response for ssch while start
> > > > > pending set. I think we set start pending in QEMU before we issue
> > > > > 'start command/io request' to the kernel. I don't think -EAGAIN
> > > > > is a good idea. AFAIU we would expect user-space to loop on
> > > > > -EAGAIN e.g. at least until the processing of a 'start command'
> > > > > is done and the (fist) ssch by the host is issued. And then
> > > > > what?  Translate the second channel program issue the second ssch
> > > > > in the host and probably get a non-zero cc? Or return -EBUSY? Or
> > > > > keep returning -EAGAIN?      
> > > > 
> > > > My idea was:
> > > > - return -EAGAIN if we're already processing a channel instruction
> > > > - continue returning -EBUSY etc. if the instruction gets the
> > > > respective return code from the hardware
> > > > 
> > > > So, the second ssch would first get a -EAGAIN and then a -EBUSY if
> > > > the first ssch is done, but the subchannel is still doing the start
> > > > function. Just as you would expect when you do a ssch while your
> > > > last request has not finished yet.
> > > >     
> > > 
> > > But before you can issue the second ssch you have to do the
> > > translation for it. And we must assume the IO corresponding to the
> > > first ssch is not done yet -- so we still need the translated channel
> > > program of the first ssch.   
> > 
> > Yes, we need to be able to juggle different translated channel programs
> > if we don't consider this part of the "instruction execution". But if
> > we return -EAGAIN if the code is currently doing that translation, we
> > should be fine, no?
> >   
> 
> As long as you return -EAGAIN we are fine. But AFAIU you proposed to
> do that until the I/O is submitted to the HW subchannel via ssch(). But
> that is not the case I'm talking about here. We have already translated
> the channel program for the first request, submitted it via ssch() and
> are awaiting an interrupt that tells us the I/O is done. While waiting
> for this interrupt we get a new ssch request. I understood, you don't
> want to give -EAGAIN for this one, but make the ssch decide. The problem
> is you still need the old translated channel program for the interrupt
> handling, and at the same time you need the new channel program
> translated as well, before doing the ssch for it in the host.

Why? You're not doing anything with that second ssch at all, it returns
before translation is started.

> 
> > > That is if we insist on doing the -EBUSY
> > > based on a return code from the hardware. I'm not sure we end up with
> > > a big simplification from making the "instructions" mutex on vfio-ccw
> > > device level in kernel as proposed above.   
> > 
> > I'm not sure we're not talking past each other here...   
> 
> I'm afraid we do.
> 
> > the "translate
> > and issue instruction" part should be mutually exclusive; I just don't
> > want to return -EBUSY, but -EAGAIN, so that userspace knows it should
> > try again.
> >   
> 
> I got it. But I wanted to point out, that we need the old channel program
> *beyond* the "translate and issue instruction".

Of course. But I don't want to start a new channel program.

> 
> > > But I'm not against it. If
> > > you have the time to write the patches I will find time to review
> > > them.  
> > 
> > Probably only on the new year...  
> 
> I think the stuff is better discussed with code at hand. I'm happy to
> continue this discussion if you think it is useful to you. Otherwise I
> suggest do it the way you think is the best, and I will try  to find and
> to point out the problems, if any.

I'll try to post something in January. Have a nice holiday break :)