Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init

[Jason Wang <jasowang@redhat.com>]

On 09/10/2015 02:18 PM, Yuanhan Liu wrote:
> On Thu, Sep 10, 2015 at 01:52:30PM +0800, Jason Wang wrote:
>>
>> On 09/10/2015 01:17 PM, Yuanhan Liu wrote:
>>> On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
>>>>>
>>>>> On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
>>>>>>> On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
>>>>>>>>> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
>>>>>>>>>>> So that we could use the `vq_index' as well in the vhost_net_init
>>>>>>>>>>> stage, which is required when adding vhost-user multiple-queue support,
>>>>>>>>>>> where we need the vq_index to indicate which queue pair we are gonna
>>>>>>>>>>> initiate.
>>>>>>>>>>>
>>>>>>>>>>> vhost-user has no multiple queue support yet, hence no queue_index set
>>>>>>>>>>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
>>>>>>>>>>> will be set properly soon in the next patch.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  hw/net/vhost_net.c | 16 +++++++---------
>>>>>>>>>>>  net/vhost-user.c   |  1 +
>>>>>>>>>>>  2 files changed, 8 insertions(+), 9 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>>>>>>>> index f9441e9..141b557 100644
>>>>>>>>>>> --- a/hw/net/vhost_net.c
>>>>>>>>>>> +++ b/hw/net/vhost_net.c
>>>>>>>>>>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
>>>>>>>>>>>      }
>>>>>>>>>>>  }
>>>>>>>>>>>  
>>>>>>>>>>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
>>>>>>>>>>> +{
>>>>>>>>>>> +    net->dev.vq_index = vq_index;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>>  struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>>>>>>>>>>  {
>>>>>>>>>>>      int r;
>>>>>>>>>>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>>>>>>>>>>      }
>>>>>>>>>>>      net->nc = options->net_backend;
>>>>>>>>>>>  
>>>>>>>>>>> +    vhost_net_set_vq_index(net, net->nc->queue_index * 2);
>>>>>>>>>>> +
>>>>>>>>> This breaks vhost kernel multiqueue since queue_index was not
>>>>>>>>> initialized at this time.
>>>>>>> Right, thanks for pointing it out.
>>>>>>>
>>>>>>>>> We do this in set_netdev() instead of setting
>>>>>>>>> it in each kind of netdev.
>>>>>>> Can we move it to net_init_tap() for setting the right queue_index
>>>>>>> for each nc?
>>>>>>>
>>>>>>> Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
>>>>>>> vhost-user mq support), another one at vhost_net_start(for vhost kernel
>>>>>>> mq support)?
>>>>>>>
>>>>>>> Or, do you have better ideas?
>>>>> I think setting queue_index in net_init_tap() looks ok.
>>> Good to know.
>>>
>>>>> But a question
>>>>> is that why need we do this at so early stage? ( Even before its peers
>>>>> is connected.)
>>> For vhost-user multiple queues support, we will invoke vhost_net_init()
>>> N times for each queue pair, and hence we need to distinguish which
>>> queue it is while sending messages like VHOST_SET_VRING_CALL for
>>> initializing corresponding queue pair.
>>>
>>> Does that make sense to you?
>>>
>> Not sure. Since current codes works for vhost-kernel. (vhost_net_init()
>> was also called N times). We don't want to break existed vhost-kernel
>> API when developing multiqueue. For each virtqueue TX/RX pair, we have
>> one vhost net device and it has no knowledge for the others (which was
>> hide by qemu). So VHOST_SET_VRING_CALL works without any change here.
>>
>> For the case here, since you still have multiple instances of vhost_net
>> structure. Maybe the vhost-user backend can distinguish form this?
> Yeah, I guess that's the difference between vhost-user and vhost-kernel.
> Vhost-kernel opens a char device(/dev/vhost-net) for each vhost_dev,
> hence it's distinguishable. But for vhost-user, all vhost_dev share one
> char device(a socket) for communication, hence, it's not distinguishable.

How about using individual socket in this case? This seems can also
minimize the changes of backend.

>
> I was thinking maybe we could export vhost_net_set_vq_index() and invoke
> it at net/vhost-user.c, so that we break nothing, and in the meantime,
> it keeps the logic inside vhost-user.
>
> What do you think?
>
> 	--yliu
>

Sounds work. Then I believe you will need to set queue_index in
vhost_user initialization code?

Thanks