Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support

[Yuanhan Liu]

On Tue, Sep 15, 2015 at 09:02:07AM -0600, Eric Blake wrote:
> On 09/15/2015 01:10 AM, Yuanhan Liu wrote:
> > From: Changchun Ouyang <address@hidden>
> > 
> > This patch is initially based a patch from Nikolay Nikolaev.
> > 
> > Here is the latest version for adding vhost-user multiple queue support,
> > by creating a nc and vhost_net pair for each queue.
> 
> The phrase "Here is the latest version" doesn't make much sense in the
> long term in git (that is, a year from now, we won't care how many
> preliminary versions there were, just about the version that got
> committed; and if anything in git changes after that point, it is no
> longer the latest version).

Yeah, good point.

> 
> > 
> > Qemu exits if find that the backend can't support number of requested
> > queues(by providing queues=# option). The max number is queried by a
> 
> space before ( in English.

Michael reminded me behore, and sorry for making such mistake again.

And thanks for other typo corrections.

> 
> > new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol
> > feature VHOST_USER_PROTOCOL_F_MQ is present first.
> > 
> > The max queue check is done at vhost-user initiation stage. We initiate
> > one queue first, which, in the meantime, also gets the max_queues the
> > backend supports.
> > 
> > In older version, it was reported that some messages are sent more times
> > than necessary. Here we came an agreement with Michael that we could
> > categorize vhost user messages to 2 types: none-vring specific messages,
> 
> s/none/non/
> 
> > which should be sent only once, and vring specific messages, which should
> > be sent per queue.
> > 
> > Here I introduced a helper function vhost_user_one_time_request(), which
> > lists following messages as none-vring specific messages:
> 
> s/none/non/
> 
> > 
> >         VHOST_USER_GET_FEATURES
> >         VHOST_USER_SET_FEATURES
> >         VHOST_USER_GET_PROTOCOL_FEATURES
> >         VHOST_USER_SET_PROTOCOL_FEATURES
> >         VHOST_USER_SET_OWNER
> >         VHOST_USER_RESET_DEVICE
> >         VHOST_USER_SET_MEM_TABLE
> >         VHOST_USER_GET_QUEUE_NUM
> > 
> > For above messages, we simply ignore them when they are not sent the first
> > time.
> > 
> 
> Up to here is mostly fine for the commit message.  Meanwhile...
> 
> > v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
> >     once only, and invoke qemu_find_net_clients_except() at the handler
> >     to gather all related ncs, for initiating all queues. Which addresses
> >     a hidden bug in older verion in a more QEMU-like way.
> > 
> > v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
> >     value from net->nc->queue_index.
> 
> ...this chunk here is useful only on the mailing list, and not in git,
> and therefore, should appear...

Sorry that I may disagree here.

First of all, it does no harm at all to include few old version
descriptions. Instead, it may give some hints to the reader how
the patch evolves. Meanwhile, you will find they are quite common
in some well known open source projects, such as linux kernel:

    address@hidden ~/linux]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
    10828

You can find them at qemu project (though not that common), too:

    address@hidden ~/qemu]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
    390

So, if it's a qemu community specific culture, I could do what
you suggested.

If not, I'd like to put them into the commit log, as putting it
outside the commit log gives unnecessary extra burden to patch
author when he need update several version change information
in a patch set: he has to format the patch set first, and add
them one by one by editing those patches.

Thanks.

        --yliu