On Fri, Apr 14, 2017 at 04:37:52PM +0800, Wei Wang wrote:
On 04/14/2017 12:34 AM, Michael S. Tsirkin wrote:I would say add the extra code there too. Or maybe we can avoid
On Thu, Apr 13, 2017 at 05:35:05PM +0800, Wei Wang wrote:Right. bitmap is the way to gather pages to chunk.
So we don't need the bitmap to talk to host, it is just
a data structure we chose to maintain lists of pages, right?
It's only needed in the balloon page case.
For the unused page case, we don't need it, since the free
page blocks are already chunks.
OK as far as it goes but you need much better isolation for it."#define PAGE_CHUNK_TYPE_UNUSED 1" is added in another patch.
Build a data structure with APIs such as _init, _cleanup, _add, _clear,
_find_first, _find_next.
Completely unrelated to pages, it just maintains bits.
Then use it here.
static int oom_pages = OOM_VBALLOON_DEFAULT_PAGES;Doesn't look like you are ever adding more types in this
module_param(oom_pages, int, S_IRUSR | S_IWUSR);
MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
@@ -50,6 +54,10 @@ MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
static struct vfsmount *balloon_mnt;
#endif
+/* Types of pages to chunk */
+#define PAGE_CHUNK_TYPE_BALLOON 0
+
patchset. Pls keep code simple, generalize it later.
adding it altogether.
Types of page to chunk are treated differently. Different types of pageSo just combine the two message formats and then it'll all be easier?
chunks are sent to the host via different protocols.
1) PAGE_CHUNK_TYPE_BALLOON: Ballooned (i.e. inflated/deflated) pages
to chunk. For the ballooned type, it uses the basic chunk msg format:
virtio_balloon_page_chunk_hdr +
virtio_balloon_page_chunk * MAX_PAGE_CHUNKS
2) PAGE_CHUNK_TYPE_UNUSED: unused pages to chunk. It uses this miscq msg
format:
miscq_hdr +
virtio_balloon_page_chunk_hdr +
virtio_balloon_page_chunk * MAX_PAGE_CHUNKS
The chunk msg is actually the payload of the miscq msg.
Agree, thanks.And miscq hdr. In fact just let compiler do the math - something like:Sounds good, thanks.+#define MAX_PAGE_CHUNKS 4096This is an order-4 allocation. I'd make it 4095 and then it's
an order-3 one.
I think it would be better to make it 4090. Leave some space for the hdr
as well.
(8 * PAGE_SIZE - sizeof(hdr)) / sizeof(chunk)
OK. The miscq msg is
I skimmed explanation of algorithms below but please make sure
code speaks for itself and add comments inline to document it.
Whenever you answered me inline this is where you want to
try to make code clearer and add comments.
Also, pls find ways to abstract the data structure so we don't
need to deal with its internals all over the code.
....
Well just pass the correct pointer in.For the unused page chunk case, it follows its own protocol:{Moving back to before the header? How can this make sense?
struct scatterlist sg;
+ struct virtio_balloon_page_chunk_hdr *hdr;
+ void *buf;
unsigned int len;
- sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
+ switch (type) {
+ case PAGE_CHUNK_TYPE_BALLOON:
+ hdr = vb->balloon_page_chunk_hdr;
+ len = 0;
+ break;
+ default:
+ dev_warn(&vb->vdev->dev, "%s: chunk %d of unknown pages\n",
+ __func__, type);
+ return;
+ }
- /* We should always be able to add one buffer to an empty queue. */
- virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
- virtqueue_kick(vq);
+ buf = (void *)hdr - len;
It works fine since len is 0, so just buf = hdr.
miscq_hdr + payload(chunk msg).
"buf = (void *)hdr - len" moves the buf pointer to the miscq_hdr, to send
the entire miscq msg.
OK, will take your suggestion:Please check the patch for implementing the unused page chunk,Exactly. And all this pointer math is very messy. Please look for ways
it will be clear. If necessary, I can put "buf = (void *)hdr - len" from
that patch.
to clean it. It's generally easy to fill structures:
struct foo *foo = kmalloc(..., sizeof(*foo) + n * sizeof(foo->a[0]));
for (i = 0; i < n; ++i)
foo->a[i] = b;
this is the kind of code that's easy to understand and it's
obvious there are no overflows and no info leaks here.
It's rather confusing. Try to pass # of chunks aroundhdr->chunks tells the host how many chunks are there in the payload.+ len += sizeof(struct virtio_balloon_page_chunk_hdr);Why zero it here after device used it? Better to zero before use.
+ len += hdr->chunks * sizeof(struct virtio_balloon_page_chunk);
+ sg_init_table(&sg, 1);
+ sg_set_buf(&sg, buf, len);
+ if (!virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL)) {
+ virtqueue_kick(vq);
+ if (busy_wait)
+ while (!virtqueue_get_buf(vq, &len) &&
+ !virtqueue_is_broken(vq))
+ cpu_relax();
+ else
+ wait_event(vb->acked, virtqueue_get_buf(vq, &len));
+ hdr->chunks = 0;
After the device use it, it is ready to zero it.
in some other way.
Not in bytes actually. base is a base pfn, which is the starting addressAre both size and base in bytes then?what is the "unit" you referred to?+ }what are the units here? Looks like it's in 4kbyte units?
+}
+
+static void add_one_chunk(struct virtio_balloon *vb, struct virtqueue *vq,
+ int type, u64 base, u64 size)
This is the function to add one chunk, base pfn and size of the chunk are
supplied to the function.
But you do not send them to host as is, you shift them for some reason
before sending them to host.
That's a waste on systems with large page sizes, and it does notA bit in the bitmap corresponds to a pfn of a balloon page(4KB).+ if (zero >= end)Still not so what does a bit refer to? page or 4kbytes?
+ chunk_size = end - one;
+ else
+ chunk_size = zero - one;
+
+ if (chunk_size)
+ add_one_chunk(vb, vq, PAGE_CHUNK_TYPE_BALLOON,
+ pfn_start + one, chunk_size);
I think it should be a page.
look like you handle that case correctly.