Re: [Qemu-devel] [PATCH -V3 09/32] virtio-9p: Implement P9_TWRITE/ Thread model in QEMU

[Avi Kivity <avi@redhat.com>]

On 03/30/2010 04:13 PM, Anthony Liguori wrote:
> On 03/30/2010 05:24 AM, Avi Kivity wrote:
>> On 03/30/2010 12:23 AM, Anthony Liguori wrote:
>>>> It's not sufficient.  If you have a single thread that runs both 
>>>> live migrations and timers, then timers will be backlogged behind 
>>>> live migration, or you'll have to yield often.  This is regardless 
>>>> of the locking model (and of course having threads without fixing 
>>>> the locking is insufficient as well, live migration accesses guest 
>>>> memory so it needs the big qemu lock).
>>>
>>>
>>> But what's the solution?  Sending every timer in a separate thread?  
>>> We'll hit the same problem if we implement an arbitrary limit to 
>>> number of threads.
>>
>> A completion that's expected to take a couple of microseconds at most 
>> can live in the iothread.  A completion that's expected to take a 
>> couple of milliseconds wants its own thread.  We'll have to think 
>> about anything in between.
>>
>> vnc and migration can perform large amounts of work in a single 
>> completion; they're limited only by the socket send rate and our 
>> internal rate-limiting which are both outside our control.  Most 
>> device timers are O(1).  virtio completions probably fall into the 
>> annoying "have to think about it" department.
>
> I think it may make more sense to have vcpu completions vs. io thread 
> completions and make vcpu completions target short lived operations.

vcpu completions make sense when you can tell that a completion will 
cause an interrupt injection and you have a good idea which cpu will be 
interrupted.

>
>>>>> What I'm skeptical of, is whether converting virtio-9p or qcow2 to 
>>>>> handle each request in a separate thread is really going to 
>>>>> improve things. 
>>>>
>>>> Currently qcow2 isn't even fullly asynchronous, so it can't fail to 
>>>> improve things.
>>>
>>> Unless it introduces more data corruptions which is my concern with 
>>> any significant change to qcow2.
>>
>> It's possible to move qcow2 to a thread without any significant 
>> change to it (simply run the current code in its own thread, 
>> protected by a mutex).  Further changes would be very incremental.
>
> But that offers no advantage to what we have which fails the 
> proof-by-example that threading makes the situation better. 

It has an advantage, qcow2 is currently synchronous in parts:

block/qcow2-cluster.c:    ret = bdrv_write(s->hd, (cluster_offset >> 9) 
+ n_start,
block/qcow2.c:        bdrv_write(s->hd, (meta.cluster_offset >> 9) + num 
- 1, buf, 1);
block/qcow2.c:        bdrv_write(bs, sector_num, buf, s->cluster_sectors);
block/qcow2-cluster.c:                    ret = 
bdrv_read(bs->backing_hd, sector_num, buf, n1);
block/qcow2-cluster.c:        ret = bdrv_read(s->hd, coffset >> 9, 
s->cluster_data, nb_csectors);

> To convert qcow2 to be threaded, I think you would have to wrap the 
> whole thing in a lock, then convert the current asynchronous functions 
> to synchronous (that's the whole point, right).  At this point, you've 
> regressed performance because you can only handle one read/write 
> outstanding at a given time.  So now you have to make the locking more 
> granular but because we do layered block devices, you've got to make 
> most of the core block driver functions thread safe.

Not at all.  The first conversion will be to keep the current code as 
is, operating asynchronously, but running in its own thread.  It will 
still support multiple outstanding requests using the current state 
machine code; the synchronous parts will be remain synchronous relative 
to the block device, but async relative to everything else.  The second 
stage will convert the state machine code to threaded code.  This is 
more difficult but not overly so - turn every dependency list into a mutex.

> Once you get basic data operations concurrent, which I expect won't be 
> so bad, to get an improvement over the current code, you have to allow 
> simultaneous access to metadata which is where I think the vast 
> majority of the complexity will come from.

I have no plans to do that, all I want is qcow2 not to block vcpus.  
btw, I don't think it's all that complicated, it's simple to lock 
individual L2 blocks and the L1 block.

> You could argue that we stick qcow2 into a thread and stop there and 
> that fixes the problems with synchronous data access.  If that's the 
> argument, then let's not even bother doing at the qcow layer, let's 
> just switch the block aio emulation to use a dedicated thread.

That's certainly the plan for vmdk and friends which are today useless.  
qcow2 deserves better treatment.

>>> Sticking the VNC server in it's own thread would be fine.  Trying to 
>>> make the VNC server multithreaded though would be problematic.
>>
>> Why would it be problematic?  Each client gets its own threads, they 
>> don't interact at all do they?
>
> Dealing with locking of the core display which each client uses for 
> rendering.  Things like CopyRect will get ugly quickly.Ultimately, 
> this comes down to a question of lock granularity and thread 
> granularity.  I don't think it's a good idea to start with the 
> assumption that we want extremely fine granularity.  There's certainly 
> very low hanging fruit with respect to threading.

Not familiar with the code, but doesn't vnc access the display core 
through an API?  Slap a lot onto that.

>>
>> I meant, exposing qemu core to the threads instead of pretending they 
>> aren't there.  I'm not familiar with 9p so don't hold much of an 
>> opinion, but didn't you say you need threads in order to handle async 
>> syscalls?  That may not be the deep threading we're discussing here.
>>
>> btw, IIUC currently disk hotunplug will stall a guest, no?  We need 
>> async aio_flush().
>
> But aio_flush() never takes a very long time, right :-)
>
> We had this discussion in the past re: live migration because we do an 
> aio_flush() in the critical stage.
>

Live migration will stall a guest anyway.  It doesn't matter if 
aio_flush blocks for a few ms, since the final stage will dominate it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.