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

[Jason Wang]

On 09/24/2015 01:57 PM, Yuanhan Liu wrote:
> On Thu, Sep 24, 2015 at 01:34:31PM +0800, Jason Wang wrote:
>>
>> Some nitpicks and comments. If you plan to send another version, please
>> consider to fix them.
> I will, but I'd like to hold a while before getting more comments from
> Michael; I don't want to repost a whole new version too often for minor
> changes.
>
> BTW, Michael, is this patchset being ready for merge? If not, please
> kindly tell me what is wrong so that I can fix them. TBH, I'd really
> like to see it be merged soon, as I'm gonna have holidays soon for
> two weeks start from this Saturday. I could still reply emails, even
> make patches during that, but I guess I only have limited time for that.
>
>> Reviewed-by: Jason Wang <address@hidden>
> Thank you!
>
>> Btw, it's better to extend current vhost-user unit test to support
>> multiqueue. Please consider this on top this series.
> Yeah, I will. But, may I do that later? (And if possible, after the
> merge?)
>

Yes, of course :)

>>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>>> index 43db9b4..cfc9d41 100644
>>> --- a/docs/specs/vhost-user.txt
>>> +++ b/docs/specs/vhost-user.txt
>>> @@ -135,6 +135,21 @@ As older slaves don't support negotiating protocol 
>>> features,
>>>  a feature bit was dedicated for this purpose:
>>>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
>>>  

[...]

>>>  
>>>  static void vhost_user_cleanup(NetClientState *nc)
>>>  {
>>>      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
>>>  
>>> -    vhost_user_stop(s);
>>> +    if (s->vhost_net) {
>>> +        vhost_net_cleanup(s->vhost_net);
>>> +        s->vhost_net = NULL;
>>> +    }
>>> +
>>>      qemu_purge_queued_packets(nc);
>>>  }
>>>  
>>> @@ -95,59 +136,61 @@ static NetClientInfo net_vhost_user_info = {
>>>          .has_ufo = vhost_user_has_ufo,
>>>  };
>>>  
>>> -static void net_vhost_link_down(VhostUserState *s, bool link_down)
>>> -{
>>> -    s->nc.link_down = link_down;
>>> -
>>> -    if (s->nc.peer) {
>>> -        s->nc.peer->link_down = link_down;
>>> -    }
>>> -
>>> -    if (s->nc.info->link_status_changed) {
>>> -        s->nc.info->link_status_changed(&s->nc);
>>> -    }
>>> -
>>> -    if (s->nc.peer && s->nc.peer->info->link_status_changed) {
>>> -        s->nc.peer->info->link_status_changed(s->nc.peer);
>>> -    }
>>> -}
>>> -
>>>  static void net_vhost_user_event(void *opaque, int event)
>>>  {
>>> -    VhostUserState *s = opaque;
>>> +    const char *name = opaque;
>>> +    NetClientState *ncs[MAX_QUEUE_NUM];
>>> +    VhostUserState *s;
>>> +    Error *err = NULL;
>>> +    int queues;
>>>  
>>> +    queues = qemu_find_net_clients_except(name, ncs,
>>> +                                          NET_CLIENT_OPTIONS_KIND_NIC,
>>> +                                          MAX_QUEUE_NUM);
>>> +    s = DO_UPCAST(VhostUserState, nc, ncs[0]);
>>>      switch (event) {
>>>      case CHR_EVENT_OPENED:
>>> -        vhost_user_start(s);
>>> -        net_vhost_link_down(s, false);
>>> +        if (vhost_user_start(queues, ncs) < 0) {
>>> +            exit(1);
>> How about report a specific error instead of just abort here?
> There is an error report at vhost_user_start():
>
>                 error_report("you are asking more queues than "
>                              "supported: %d\n", max_queues);
>
> Or, do you mean something else?
>
>
>       --yliu

Not sure, but we have error_abort and error_fatal. Markus may know more
about this.


[...]