Virtualization - May 2016 - [PATCH RFC kernel] balloon: speed up inflating/deflating process

> > > > > This can be pre-initialized, correct?
> > > >
> > > > pre-initialized? I am not quite understand your mean.
> > >
> > > I think you can maintain sg as part of device state and init sg
with the
> bitmap.
> > >
> >
> > I got it.
> >
> > > > > This is grossly inefficient if you only requested a
single page.
> > > > > And it's also allocating memory very aggressively
without ever
> > > > > telling the host what is going on.
> > > >
> > > > If only requested a single page, there is no need  to send
the
> > > > entire page bitmap, This RFC patch has already considered
about this.
> > >
> > > where's that addressed in code?
> > >
> >
> > By record the start_pfn and end_pfn.
> >
> > The start_pfn & end_pfn will be updated in set_page_bitmap() and
will
> > be used in the function tell_host():
> >
> > ----------------------------------------------------------------------
> > -----------
> > +static void set_page_bitmap(struct virtio_balloon *vb, struct page
> > +*page) {
> > +	unsigned int i;
> > +	unsigned long *bitmap = vb->page_bitmap;
> > +	unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > +
> > +	for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > +		set_bit(balloon_pfn + i, bitmap);
> 
> BTW, there's a page size value in header so there is no longer need to
set
> multiple bits per page.
Yes, you are right.
> 
> > +	if (balloon_pfn < vb->start_pfn)
> > +		vb->start_pfn = balloon_pfn;
> > +	if (balloon_pfn > vb->end_pfn)
> > +		vb->end_pfn = balloon_pfn;
> > +}
> 
> Sounds good, but you also need to limit by allocated bitmap size.
Why should we limit the page bitmap size? Is it no good to send a large page
bitmap?
or to save the memory used for page bitmap? Or some other
reason?
> 
> >
> > +		unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > +		struct scatterlist sg[5];
> > +
> > +		start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > +		end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > +		bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> sizeof(long);
> > +
> > +		sg_init_table(sg, 5);
> > +		sg_set_buf(&sg[0], &flags, sizeof(flags));
> > +		sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > +		sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > +		sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > +		sg_set_buf(&sg[4], vb->page_bitmap +
> > +				 (start_pfn / BITS_PER_LONG), bmap_len);
> 
> Looks wrong. start_pfn should start at offset 0 I think ...
I don't know what is wrong here, could you tell me why?

Thanks!

Liang

On Tue, May 24, 2016 at 02:36:08PM +0000, Li, Liang Z
wrote:
> > > > > > This can be pre-initialized, correct?
> > > > >
> > > > > pre-initialized? I am not quite understand your mean.
> > > >
> > > > I think you can maintain sg as part of device state and init
sg with the
> > bitmap.
> > > >
> > >
> > > I got it.
> > >
> > > > > > This is grossly inefficient if you only requested
a single page.
> > > > > > And it's also allocating memory very
aggressively without ever
> > > > > > telling the host what is going on.
> > > > >
> > > > > If only requested a single page, there is no need  to
send the
> > > > > entire page bitmap, This RFC patch has already
considered about this.
> > > >
> > > > where's that addressed in code?
> > > >
> > >
> > > By record the start_pfn and end_pfn.
> > >
> > > The start_pfn & end_pfn will be updated in set_page_bitmap()
and will
> > > be used in the function tell_host():
> > >
> > >
----------------------------------------------------------------------
> > > -----------
> > > +static void set_page_bitmap(struct virtio_balloon *vb, struct
page
> > > +*page) {
> > > +	unsigned int i;
> > > +	unsigned long *bitmap = vb->page_bitmap;
> > > +	unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > > +
> > > +	for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > > +		set_bit(balloon_pfn + i, bitmap);
> > 
> > BTW, there's a page size value in header so there is no longer
need to set
> > multiple bits per page.
> 
> Yes, you are right.
> 
> > 
> > > +	if (balloon_pfn < vb->start_pfn)
> > > +		vb->start_pfn = balloon_pfn;
> > > +	if (balloon_pfn > vb->end_pfn)
> > > +		vb->end_pfn = balloon_pfn;
> > > +}
> > 
> > Sounds good, but you also need to limit by allocated bitmap size.
> 
> Why should we limit the page bitmap size? Is it no good to send a large
page bitmap?
> or to save the memory used for page bitmap? Or some other reason?
To save memory. First allocating a large bitmap can fail, second this is
pinned memory that is wasted - it's unused most of the time while guest
is running.
> > 
> > >
> > > +		unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > +		struct scatterlist sg[5];
> > > +
> > > +		start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > +		end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > +		bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > sizeof(long);
> > > +
> > > +		sg_init_table(sg, 5);
> > > +		sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > +		sg_set_buf(&sg[1], &start_pfn, sizeof(start_pfn));
> > > +		sg_set_buf(&sg[2], &page_shift, sizeof(page_shift));
> > > +		sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > +		sg_set_buf(&sg[4], vb->page_bitmap +
> > > +				 (start_pfn / BITS_PER_LONG), bmap_len);
> > 
> > Looks wrong. start_pfn should start at offset 0 I think ...
> 
> I don't know what is wrong here, could you tell me why?
> 
> Thanks!
> 
> Liang
start_pfn should mean "bit 0 in bitmap refers to pfn X".
So it does not make sense to also add it as offset within bitmap.

-- 
MST

> On Tue, May 24, 2016 at 02:36:08PM +0000, Li, Liang Z wrote:
> > > > > > > This can be pre-initialized, correct?
> > > > > >
> > > > > > pre-initialized? I am not quite understand your
mean.
> > > > >
> > > > > I think you can maintain sg as part of device state and
init sg
> > > > > with the
> > > bitmap.
> > > > >
> > > >
> > > > I got it.
> > > >
> > > > > > > This is grossly inefficient if you only
requested a single page.
> > > > > > > And it's also allocating memory very
aggressively without
> > > > > > > ever telling the host what is going on.
> > > > > >
> > > > > > If only requested a single page, there is no need 
to send the
> > > > > > entire page bitmap, This RFC patch has already
considered about
> this.
> > > > >
> > > > > where's that addressed in code?
> > > > >
> > > >
> > > > By record the start_pfn and end_pfn.
> > > >
> > > > The start_pfn & end_pfn will be updated in
set_page_bitmap() and
> > > > will be used in the function tell_host():
> > > >
> > > >
------------------------------------------------------------------
> > > > ----
> > > > -----------
> > > > +static void set_page_bitmap(struct virtio_balloon *vb,
struct
> > > > +page
> > > > +*page) {
> > > > +	unsigned int i;
> > > > +	unsigned long *bitmap = vb->page_bitmap;
> > > > +	unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > > > +
> > > > +	for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > > > +		set_bit(balloon_pfn + i, bitmap);
> > >
> > > BTW, there's a page size value in header so there is no
longer need
> > > to set multiple bits per page.
> >
> > Yes, you are right.
> >
> > >
> > > > +	if (balloon_pfn < vb->start_pfn)
> > > > +		vb->start_pfn = balloon_pfn;
> > > > +	if (balloon_pfn > vb->end_pfn)
> > > > +		vb->end_pfn = balloon_pfn;
> > > > +}
> > >
> > > Sounds good, but you also need to limit by allocated bitmap size.
> >
> > Why should we limit the page bitmap size? Is it no good to send a
large
> page bitmap?
> > or to save the memory used for page bitmap? Or some other reason?
> 
> To save memory. First allocating a large bitmap can fail, second this is
pinned
> memory that is wasted - it's unused most of the time while guest is
running.
> 
> > >
> > > >
> > > > +		unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > > +		struct scatterlist sg[5];
> > > > +
> > > > +		start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > > +		end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > > +		bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > > sizeof(long);
> > > > +
> > > > +		sg_init_table(sg, 5);
> > > > +		sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > > +		sg_set_buf(&sg[1], &start_pfn,
sizeof(start_pfn));
> > > > +		sg_set_buf(&sg[2], &page_shift,
sizeof(page_shift));
> > > > +		sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > > +		sg_set_buf(&sg[4], vb->page_bitmap +
> > > > +				 (start_pfn / BITS_PER_LONG), bmap_len);
> > >
> > > Looks wrong. start_pfn should start at offset 0 I think ...
> >
> > I don't know what is wrong here, could you tell me why?
> >
> > Thanks!
> >
> > Liang
> 
> start_pfn should mean "bit 0 in bitmap refers to pfn X".
> So it does not make sense to also add it as offset within bitmap.
> 
> --
> MST

> > > > > > > This is grossly inefficient if you only
requested a single page.
> > > > > > > And it's also allocating memory very
aggressively without
> > > > > > > ever telling the host what is going on.
> > > > > >
> > > > > > If only requested a single page, there is no need 
to send the
> > > > > > entire page bitmap, This RFC patch has already
considered about
> this.
> > > > >
> > > > > where's that addressed in code?
> > > > >
> > > >
> > > > By record the start_pfn and end_pfn.
> > > >
> > > > The start_pfn & end_pfn will be updated in
set_page_bitmap() and
> > > > will be used in the function tell_host():
> > > >
> > > >
------------------------------------------------------------------
> > > > ----
> > > > -----------
> > > > +static void set_page_bitmap(struct virtio_balloon *vb,
struct
> > > > +page
> > > > +*page) {
> > > > +	unsigned int i;
> > > > +	unsigned long *bitmap = vb->page_bitmap;
> > > > +	unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > > > +
> > > > +	for (i = 0; i < VIRTIO_BALLOON_PAGES_PER_PAGE; i++)
> > > > +		set_bit(balloon_pfn + i, bitmap);
> > >
> > > BTW, there's a page size value in header so there is no
longer need
> > > to set multiple bits per page.
> >
> > Yes, you are right.
> >
> > >
> > > > +	if (balloon_pfn < vb->start_pfn)
> > > > +		vb->start_pfn = balloon_pfn;
> > > > +	if (balloon_pfn > vb->end_pfn)
> > > > +		vb->end_pfn = balloon_pfn;
> > > > +}
> > >
> > > Sounds good, but you also need to limit by allocated bitmap size.
> >
> > Why should we limit the page bitmap size? Is it no good to send a
large
> page bitmap?
> > or to save the memory used for page bitmap? Or some other reason?
> 
> To save memory. First allocating a large bitmap can fail, second this is
pinned
> memory that is wasted - it's unused most of the time while guest is
running.
Make sense.
> 
> > >
> > > >
> > > > +		unsigned long start_pfn, end_pfn, flags = 0, bmap_len;
> > > > +		struct scatterlist sg[5];
> > > > +
> > > > +		start_pfn = rounddown(vb->start_pfn, BITS_PER_LONG);
> > > > +		end_pfn = roundup(vb->end_pfn, BITS_PER_LONG);
> > > > +		bmap_len = (end_pfn - start_pfn) / BITS_PER_LONG *
> > > sizeof(long);
> > > > +
> > > > +		sg_init_table(sg, 5);
> > > > +		sg_set_buf(&sg[0], &flags, sizeof(flags));
> > > > +		sg_set_buf(&sg[1], &start_pfn,
sizeof(start_pfn));
> > > > +		sg_set_buf(&sg[2], &page_shift,
sizeof(page_shift));
> > > > +		sg_set_buf(&sg[3], &bmap_len, sizeof(bmap_len));
> > > > +		sg_set_buf(&sg[4], vb->page_bitmap +
> > > > +				 (start_pfn / BITS_PER_LONG), bmap_len);
> > >
> > > Looks wrong. start_pfn should start at offset 0 I think ...
> >
> > I don't know what is wrong here, could you tell me why?
> >
> > Thanks!
> >
> > Liang
> 
> start_pfn should mean "bit 0 in bitmap refers to pfn X".
> So it does not make sense to also add it as offset within bitmap.
I got it, but in my code, it's correct, because the page_bitmap is
range from pfn 0 to max_pfn. 
It should be changed if we are going to use a small page bitmap.

Thanks!

Liang

> 
> --
> MST