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

[Yuanhan Liu]

On Wed, Sep 16, 2015 at 10:06:56AM +0800, Yuanhan Liu wrote:
> 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.

Oops, I just found we can do that while editing the commit log (IIRC,
I had a similar try long time ago, and it seems it didn't work).

        --yliu