On 05/09/2017 01:40 AM, Michael S. Tsirkin wrote:
On Sun, May 07, 2017 at 04:19:28AM +0000, Wang, Wei W wrote:
On 05/06/2017 06:26 AM, Michael S. Tsirkin wrote:
On Thu, Apr 27, 2017 at 02:31:49PM +0800, Wei Wang wrote:
On 04/27/2017 07:20 AM, Michael S. Tsirkin wrote:
On Wed, Apr 26, 2017 at 11:03:34AM +0000, Wang, Wei W wrote:
Hi Michael, could you please give some feedback?
I'm sorry, I'm not sure feedback on what you are requesting.
Oh, just some trivial things (e.g. use a field in the header,
hdr->chunks to indicate the number of chunks in the payload) that
wasn't confirmed.
I will prepare the new version with fixing the agreed issues, and we
can continue to discuss those parts if you still find them improper.
The interface looks reasonable now, even though there's a way to
make it even simpler if we can limit chunk size to 2G (in fact 4G -
1). Do you think we can live with this limitation?
Yes, I think we can. So, is it good to change to use the previous
64-bit chunk format (52-bit base + 12-bit size)?
This isn't what I meant. virtio ring has descriptors with a 64 bit address and 32 bit
size.
If size < 4g is not a significant limitation, why not just use that to pass
address/size in a standard s/g list, possibly using INDIRECT?
OK, I see your point, thanks. Post the two options here for an analysis:
Option1 (what we have now):
struct virtio_balloon_page_chunk {
__le64 chunk_num;
struct virtio_balloon_page_chunk_entry entry[];
};
Option2:
struct virtio_balloon_page_chunk {
__le64 chunk_num;
struct scatterlist entry[];
};
This isn't what I meant really :) I meant vring_desc.
OK. Repost the code change:
Option2:
struct virtio_balloon_page_chunk {
__le64 chunk_num;
struct ving_desc entry[];
};
We pre-allocate a table of desc, and each desc is used to hold a chunk.
In that case, the virtqueue_add() function, which deals with sg, is not
usable for us. We may need to add a new one,
virtqueue_add_indirect_desc(),
to add a pre-allocated indirect descriptor table to vring.
I don't have an issue to change it to Option2, but I would prefer Option1,
because I think there is no be obvious difference between the two options,
while Option1 appears to have little advantages here:
1) "struct virtio_balloon_page_chunk_entry" has smaller size than
"struct scatterlist", so the same size of allocated page chunk buffer
can hold more entry[] using Option1;
2) INDIRECT needs on demand kmalloc();
Within alloc_indirect? We can fix that with a separate patch.
3) no 4G size limit;
Do you see lots of >=4g chunks in practice?
It wouldn't be much in practice, but we still need the extra code to
handle the case - break larger chunks into less-than 4g ones.
Best,
Wei
