On Mon, Jul 25, 2011 at 5:25 AM, Zhi Yong Wu <zwu.kernel@xxxxxxxxx> wrote:
> On Fri, Jul 22, 2011 at 6:54 PM, Stefan Hajnoczi <stefanha@xxxxxxxxx> wrote:
>> On Fri, Jul 22, 2011 at 10:20 AM, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx> wrote:
>>> +static void bdrv_block_timer(void *opaque)
>>> +{
>>> + BlockDriverState *bs = opaque;
>>> + BlockQueue *queue = bs->block_queue;
>>> + uint64_t intval = 1;
>>> +
>>> + while (!QTAILQ_EMPTY(&queue->requests)) {
>>> + BlockIORequest *request;
>>> + int ret;
>>> +
>>> + request = QTAILQ_FIRST(&queue->requests);
>>> + QTAILQ_REMOVE(&queue->requests, request, entry);
>>> +
>>> + ret = queue->handler(request);
>>
>> Please remove the function pointer and call qemu_block_queue_handler()
>> directly. This indirection is not needed and makes it harder to
>> follow the code.
> Should it keep the same style with other queue implemetations such as
> network queue? As you have known, network queue has one queue handler.
You mean net/queue.c:queue->deliver? There are two deliver functions,
qemu_deliver_packet() and qemu_vlan_deliver_packet(), which is why a
function pointer is necessary.
In this case there is only one handler function so the indirection is
not necessary.
>>
>>> + if (ret == 0) {
>>> + QTAILQ_INSERT_HEAD(&queue->requests, request, entry);
>>> + break;
>>> + }
>>> +
>>> + qemu_free(request);
>>> + }
>>> +
>>> + qemu_mod_timer(bs->block_timer, (intval * 1000000000) + qemu_get_clock_ns(vm_clock));
>>
>> intval is always 1. The timer should be set to the next earliest deadline.
> pls see bdrv_aio_readv/writev:
> + /* throttling disk read I/O */
> + if (bs->io_limits != NULL) {
> + if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
> + ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
> + sector_num, qiov, nb_sectors, cb, opaque);
> + qemu_mod_timer(bs->block_timer, wait_time +
> qemu_get_clock_ns(vm_clock));
>
> The timer has been modified when the blk request is enqueued.
The current algorithm seems to be:
1. Queue requests that exceed the limit and set the timer.
2. Dequeue all requests when the timer fires.
3. Set 1s periodic timer.
Why is the timer set up as a periodic 1 second timer in bdrv_block_timer()?
>>> + bs->slice_start[is_write] = real_time;
>>> +
>>> + bs->io_disps->bytes[is_write] = 0;
>>> + if (bs->io_limits->bps[2]) {
>>> + bs->io_disps->bytes[!is_write] = 0;
>>> + }
>>
>> All previous data should be discarded when a new time slice starts:
>> bs->io_disps->bytes[IO_LIMIT_READ] = 0;
>> bs->io_disps->bytes[IO_LIMIT_WRITE] = 0;
> If only bps_rd is specified, bs->io_disps->bytes[IO_LIMIT_WRITE] will
> never be used; i think that it should not be cleared here. right?
I think there is no advantage in keeping slices separate for
read/write. The code would be simpler and work the same if there is
only one slice and past history is cleared when a new slice starts.
Stefan
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
