| Message ID | 1441697927-16456-6-git-send-email-yuanhan.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
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. We do this in set_netdev() instead of setting it in each kind of netdev. > net->dev.nvqs = 2; > net->dev.vqs = net->vqs; > > @@ -196,11 +203,6 @@ fail: > return NULL; > } > > -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) > -{ > - net->dev.vq_index = vq_index; > -} > - > static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, > bool set) > { > @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, > goto err; > } > > - for (i = 0; i < total_queues; i++) { > - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2); > - } > - > r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true); > if (r < 0) { > error_report("Error binding guest notifier: %d", -r); > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 93dcecd..2d6bbe5 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, > /* We don't provide a receive callback */ > s->nc.receive_disabled = 1; > s->chr = chr; > + nc->queue_index = 0; > Fail to understand why this is needed. It will be set to correct value in set_netdev(). > qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); >
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? > > > net->dev.nvqs = 2; > > net->dev.vqs = net->vqs; > > > > @@ -196,11 +203,6 @@ fail: > > return NULL; > > } > > > > -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) > > -{ > > - net->dev.vq_index = vq_index; > > -} > > - > > static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, > > bool set) > > { > > @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, > > goto err; > > } > > > > - for (i = 0; i < total_queues; i++) { > > - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2); > > - } > > - > > r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true); > > if (r < 0) { > > error_report("Error binding guest notifier: %d", -r); > > diff --git a/net/vhost-user.c b/net/vhost-user.c > > index 93dcecd..2d6bbe5 100644 > > --- a/net/vhost-user.c > > +++ b/net/vhost-user.c > > @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, > > /* We don't provide a receive callback */ > > s->nc.receive_disabled = 1; > > s->chr = chr; > > + nc->queue_index = 0; > > > > Fail to understand why this is needed. It will be set to correct value > in set_netdev(). Right, it's unnecessary. BTW, May I ask you a quick question: why set_netdev is invoked after tap init, while it is invoked before vhost-user init? --yliu > > > qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); > >
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. But a question is that why need we do this at so early stage? ( Even before its peers is connected.) > >>> net->dev.nvqs = 2; >>> net->dev.vqs = net->vqs; >>> >>> @@ -196,11 +203,6 @@ fail: >>> return NULL; >>> } >>> >>> -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) >>> -{ >>> - net->dev.vq_index = vq_index; >>> -} >>> - >>> static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, >>> bool set) >>> { >>> @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, >>> goto err; >>> } >>> >>> - for (i = 0; i < total_queues; i++) { >>> - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2); >>> - } >>> - >>> r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true); >>> if (r < 0) { >>> error_report("Error binding guest notifier: %d", -r); >>> diff --git a/net/vhost-user.c b/net/vhost-user.c >>> index 93dcecd..2d6bbe5 100644 >>> --- a/net/vhost-user.c >>> +++ b/net/vhost-user.c >>> @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, >>> /* We don't provide a receive callback */ >>> s->nc.receive_disabled = 1; >>> s->chr = chr; >>> + nc->queue_index = 0; >>> >> Fail to understand why this is needed. It will be set to correct value >> in set_netdev(). > Right, it's unnecessary. > > BTW, May I ask you a quick question: why set_netdev is invoked after > tap init, while it is invoked before vhost-user init? > > --yliu I think the reason is that you must initialize all netdevs before a device tries to connect them. >>> qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); >>>
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? > > > > >>> net->dev.nvqs = 2; > >>> net->dev.vqs = net->vqs; > >>> > >>> @@ -196,11 +203,6 @@ fail: > >>> return NULL; > >>> } > >>> > >>> -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) > >>> -{ > >>> - net->dev.vq_index = vq_index; > >>> -} > >>> - > >>> static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, > >>> bool set) > >>> { > >>> @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, > >>> goto err; > >>> } > >>> > >>> - for (i = 0; i < total_queues; i++) { > >>> - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2); > >>> - } > >>> - > >>> r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true); > >>> if (r < 0) { > >>> error_report("Error binding guest notifier: %d", -r); > >>> diff --git a/net/vhost-user.c b/net/vhost-user.c > >>> index 93dcecd..2d6bbe5 100644 > >>> --- a/net/vhost-user.c > >>> +++ b/net/vhost-user.c > >>> @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, > >>> /* We don't provide a receive callback */ > >>> s->nc.receive_disabled = 1; > >>> s->chr = chr; > >>> + nc->queue_index = 0; > >>> > >> Fail to understand why this is needed. It will be set to correct value > >> in set_netdev(). > > Right, it's unnecessary. > > > > BTW, May I ask you a quick question: why set_netdev is invoked after > > tap init, while it is invoked before vhost-user init? > > > > --yliu > > I think the reason is that you must initialize all netdevs before a > device tries to connect them. Thanks! --yliu
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?
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. 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
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
On Thu, Sep 10, 2015 at 02:54:02PM +0800, Jason Wang wrote: > > > 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 doubt that will work, for the socket is given at cmd line by "-chardev" option. You can image that it's not flexible when you want to have 10 queues, or more. > > > > > 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. Good. > Then I believe you will need to set queue_index in > vhost_user initialization code? Yes. I then will go this way if no objection. --yliu
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); + net->dev.nvqs = 2; net->dev.vqs = net->vqs; @@ -196,11 +203,6 @@ fail: return NULL; } -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) -{ - net->dev.vq_index = vq_index; -} - static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, bool set) { @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, goto err; } - for (i = 0; i < total_queues; i++) { - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2); - } - r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true); if (r < 0) { error_report("Error binding guest notifier: %d", -r); diff --git a/net/vhost-user.c b/net/vhost-user.c index 93dcecd..2d6bbe5 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, /* We don't provide a receive callback */ s->nc.receive_disabled = 1; s->chr = chr; + nc->queue_index = 0; qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
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(-)