Re: [PATCH v9 5/5] virtio-balloon: VIRTIO_BALLOON_F_MISC_VQ

[Michael S. Tsirkin]

On Thu, Apr 27, 2017 at 02:33:44PM +0800, Wei Wang wrote:
> On 04/14/2017 01:08 AM, Michael S. Tsirkin wrote:
> > On Thu, Apr 13, 2017 at 05:35:08PM +0800, Wei Wang wrote:
> > > Add a new vq, miscq, to handle miscellaneous requests between the device
> > > and the driver.
> > > 
> > > This patch implemnts the VIRTIO_BALLOON_MISCQ_INQUIRE_UNUSED_PAGES
> > implements
> > 
> > > request sent from the device.
> > Commands are sent from host and handled on guest.
> > In fact how is this so different from stats?
> > How about reusing the stats vq then? You can use one buffer
> > for stats and one buffer for commands.
> > 
> 
> The meaning of the two vqs is a little different. statq is used for
> reporting statistics, while miscq is intended to be used to handle
> miscellaneous requests from the guest or host

misc just means "anything goes". If you want it to mean
"commands" name it so.

> (I think it can
> also be used the other way around in the future when other
> new features are added which need the guest to send requests
> and the host to provide responses).
> 
> I would prefer to have them separate, because:
> If we plan to combine them, we need to put the previous statq
> related implementation under miscq with a new command (I think
> we can't combine them without using commands to distinguish
> the two features).

Right.

> In this way, an old driver won't work with a new QEMU or a new
> driver won't work with an old QEMU. Would this be considered
> as an issue here?

Compatibility is and should always be handled using
feature flags.  There's a feature flag for this, isn't it?

> 
> 
> > 
> > > +	miscq_out_hdr->flags = 0;
> > > +
> > > +	for_each_populated_zone(zone) {
> > > +		for (order = MAX_ORDER - 1; order > 0; order--) {
> > > +			for (migratetype = 0; migratetype < MIGRATE_TYPES;
> > > +			     migratetype++) {
> > > +				do {
> > > +					ret = inquire_unused_page_block(zone,
> > > +						order, migratetype, &page);
> > > +					if (!ret) {
> > > +						pfn = (u64)page_to_pfn(page);
> > > +						add_one_chunk(vb, vq,
> > > +							PAGE_CHUNK_TYPE_UNUSED,
> > > +							pfn,
> > > +							(u64)(1 << order));
> > > +					}
> > > +				} while (!ret);
> > > +			}
> > > +		}
> > > +	}
> > > +	miscq_out_hdr->flags |= VIRTIO_BALLOON_MISCQ_F_COMPLETE;
> > And where is miscq_out_hdr used? I see no add_outbuf anywhere.
> > 
> > Things like this should be passed through function parameters
> > and not stuffed into device structure, fields should be
> > initialized before use and not where we happen to
> > have the data handy.
> > 
> 
> miscq_out_hdr is linear with the payload (i.e. kmalloc(hdr+payload) ).
> It is the same as the use of statq - one request in-flight each time.
> 
> 
> > 
> > Also, _F_ is normally a bit number, you use it as a value here.
> > 
> It intends to be a bit number. Bit 0 of flags to indicate the completion
> of handling the request.