Re: [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:

So the way I see it, there are several issues:

- internal wait - forces multiple APIs like kick/kick_sync
   note how kick_sync can fail but your code never checks return code
- need to re-write the last descriptor - might not work
   for alternative layouts which always expose descriptors
   immediately

Probably it wasn't clear. Please let me explain the two functions here:

1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
grabs a desc from the vq and inserts it to the chain tail (which is indexed by prev_id, probably better to call it tail_id). Then, the new added desc becomes the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc when it's
added to the chain, and set when another desc comes to follow later.

2) virtqueue_add_chain(vq, head_id,..): expose the chain to the other end.

So, if people want to add a desc and immediately expose it to the other end,
i.e. build a single desc chain, they can just add and expose:

virtqueue_add_chain_desc(..);
virtqueue_add_chain(..,head_id);

Would you see any issues here?


- some kind of iterator type would be nicer instead of
   maintaining head/prev explicitly

Why would we need to iterate the chain? I think it would be simpler to use
a wrapper struct:

struct virtqueue_desc_chain {
    unsigned int head;  // head desc id of the chain
    unsigned int tail;     // tail desc id of the chain
}

The new desc will be put to desc[tail].next, and we don't need to walk
from the head desc[head].next when inserting a new desc to the chain, right?



As for the use, it would be better to do

if (!add_next(vq, ...)) {
	add_last(vq, ...)
	kick
	wait
}

"!add_next(vq, ...)" means that the vq is full? If so, what would add_last() do then?


Using VIRTQUEUE_DESC_ID_INIT seems to avoid a branch in the driver, but
in fact it merely puts the branch in the virtio code.


Actually it wasn't intended to improve performance. It is used to indicate the "init" state of the chain. So, when virtqueue_add_chain_desc(, head_id,..) finds head id=INIT, it will assign the grabbed desc id to &head_id. In some sense, it is equivalent to add_first().

Do you have a different opinion here?

Best,
Wei






[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux