Toshiaki Makita
2018-Jul-23 09:57 UTC
[PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote:> From: Tonghao Zhang <xiangxia.m.yue at gmail.com> > > Factor out generic busy polling logic and will be > used for in tx path in the next patch. And with the patch, > qemu can set differently the busyloop_timeout for rx queue. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > ---...> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net, > + struct vhost_virtqueue *rvq, > + struct vhost_virtqueue *tvq, > + bool rx) > +{ > + struct socket *sock = rvq->private_data; > + > + if (rx) { > + if (!vhost_vq_avail_empty(&net->dev, tvq)) { > + vhost_poll_queue(&tvq->poll); > + } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) { > + vhost_disable_notify(&net->dev, tvq); > + vhost_poll_queue(&tvq->poll); > + } > + } else if ((sock && sk_has_rx_data(sock->sk)) && > + !vhost_vq_avail_empty(&net->dev, rvq)) { > + vhost_poll_queue(&rvq->poll);Now we wait for vq_avail for rx as well, I think you cannot skip vhost_enable_notify() on tx. Probably you might want to do: } else if (sock && sk_has_rx_data(sock->sk)) { if (!vhost_vq_avail_empty(&net->dev, rvq)) { vhost_poll_queue(&rvq->poll); } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) { vhost_disable_notify(&net->dev, rvq); vhost_poll_queue(&rvq->poll); } } Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx? -- Toshiaki Makita
Tonghao Zhang
2018-Jul-23 12:43 UTC
[PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> wrote:> > On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue at gmail.com> > > > > Factor out generic busy polling logic and will be > > used for in tx path in the next patch. And with the patch, > > qemu can set differently the busyloop_timeout for rx queue. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > > --- > ... > > +static void vhost_net_busy_poll_vq_check(struct vhost_net *net, > > + struct vhost_virtqueue *rvq, > > + struct vhost_virtqueue *tvq, > > + bool rx) > > +{ > > + struct socket *sock = rvq->private_data; > > + > > + if (rx) { > > + if (!vhost_vq_avail_empty(&net->dev, tvq)) { > > + vhost_poll_queue(&tvq->poll); > > + } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) { > > + vhost_disable_notify(&net->dev, tvq); > > + vhost_poll_queue(&tvq->poll); > > + } > > + } else if ((sock && sk_has_rx_data(sock->sk)) && > > + !vhost_vq_avail_empty(&net->dev, rvq)) { > > + vhost_poll_queue(&rvq->poll); > > Now we wait for vq_avail for rx as well, I think you cannot skip > vhost_enable_notify() on tx. Probably you might want to do:I think vhost_enable_notify is needed.> } else if (sock && sk_has_rx_data(sock->sk)) { > if (!vhost_vq_avail_empty(&net->dev, rvq)) { > vhost_poll_queue(&rvq->poll); > } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) { > vhost_disable_notify(&net->dev, rvq); > vhost_poll_queue(&rvq->poll); > } > }As Jason review as before, we only want rx kick when packet is pending at socket but we're out of available buffers. So we just enable notify, but not poll it ? } else if ((sock && sk_has_rx_data(sock->sk)) && !vhost_vq_avail_empty(&net->dev, rvq)) { vhost_poll_queue(&rvq->poll); else { vhost_enable_notify(&net->dev, rvq); }> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?I cant find why it is better, if necessary, we can do it.> -- > Toshiaki Makita >
Tonghao Zhang
2018-Jul-23 17:31 UTC
[PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita <toshiaki.makita1 at gmail.com> wrote:> > On 18/07/23 (?) 21:43, Tonghao Zhang wrote: > > On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita > > <makita.toshiaki at lab.ntt.co.jp> wrote: > >> > >> On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote: > >>> From: Tonghao Zhang <xiangxia.m.yue at gmail.com> > >>> > >>> Factor out generic busy polling logic and will be > >>> used for in tx path in the next patch. And with the patch, > >>> qemu can set differently the busyloop_timeout for rx queue. > >>> > >>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > >>> --- > >> ... > >>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net, > >>> + struct vhost_virtqueue *rvq, > >>> + struct vhost_virtqueue *tvq, > >>> + bool rx) > >>> +{ > >>> + struct socket *sock = rvq->private_data; > >>> + > >>> + if (rx) { > >>> + if (!vhost_vq_avail_empty(&net->dev, tvq)) { > >>> + vhost_poll_queue(&tvq->poll); > >>> + } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) { > >>> + vhost_disable_notify(&net->dev, tvq); > >>> + vhost_poll_queue(&tvq->poll); > >>> + } > >>> + } else if ((sock && sk_has_rx_data(sock->sk)) && > >>> + !vhost_vq_avail_empty(&net->dev, rvq)) { > >>> + vhost_poll_queue(&rvq->poll); > >> > >> Now we wait for vq_avail for rx as well, I think you cannot skip > >> vhost_enable_notify() on tx. Probably you might want to do: > > I think vhost_enable_notify is needed. > > > >> } else if (sock && sk_has_rx_data(sock->sk)) { > >> if (!vhost_vq_avail_empty(&net->dev, rvq)) { > >> vhost_poll_queue(&rvq->poll); > >> } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) { > >> vhost_disable_notify(&net->dev, rvq); > >> vhost_poll_queue(&rvq->poll); > >> } > >> } > > As Jason review as before, we only want rx kick when packet is pending at > > socket but we're out of available buffers. So we just enable notify, > > but not poll it ? > > > > } else if ((sock && sk_has_rx_data(sock->sk)) && > > !vhost_vq_avail_empty(&net->dev, rvq)) { > > vhost_poll_queue(&rvq->poll); > > else { > > vhost_enable_notify(&net->dev, rvq); > > } > > When vhost_enable_notify() returns true the avail becomes non-empty > while we are enabling notify. We may delay the rx process if we don't > check the return value of vhost_enable_notify().I got it thanks.> >> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx? > > I cant find why it is better, if necessary, we can do it. > > The reason is pretty simple... we are busypolling the socket so we don't > need rx wakeups during it?OK, but one question, how about rx? do we use the vhost_net_disable_vq/vhost_net_ensable_vq on rx ?> -- > Toshiaki Makita
Toshiaki Makita
2018-Jul-24 02:53 UTC
[PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/07/24 2:31, Tonghao Zhang wrote:> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita > <toshiaki.makita1 at gmail.com> wrote: >> >> On 18/07/23 (?) 21:43, Tonghao Zhang wrote: >>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita >>> <makita.toshiaki at lab.ntt.co.jp> wrote: >>>> >>>> On 2018/07/22 3:04, xiangxia.m.yue at gmail.com wrote: >>>>> From: Tonghao Zhang <xiangxia.m.yue at gmail.com> >>>>> >>>>> Factor out generic busy polling logic and will be >>>>> used for in tx path in the next patch. And with the patch, >>>>> qemu can set differently the busyloop_timeout for rx queue. >>>>> >>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> >>>>> --- >>>> ... >>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net, >>>>> + struct vhost_virtqueue *rvq, >>>>> + struct vhost_virtqueue *tvq, >>>>> + bool rx) >>>>> +{ >>>>> + struct socket *sock = rvq->private_data; >>>>> + >>>>> + if (rx) { >>>>> + if (!vhost_vq_avail_empty(&net->dev, tvq)) { >>>>> + vhost_poll_queue(&tvq->poll); >>>>> + } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) { >>>>> + vhost_disable_notify(&net->dev, tvq); >>>>> + vhost_poll_queue(&tvq->poll); >>>>> + } >>>>> + } else if ((sock && sk_has_rx_data(sock->sk)) && >>>>> + !vhost_vq_avail_empty(&net->dev, rvq)) { >>>>> + vhost_poll_queue(&rvq->poll); >>>> >>>> Now we wait for vq_avail for rx as well, I think you cannot skip >>>> vhost_enable_notify() on tx. Probably you might want to do: >>> I think vhost_enable_notify is needed. >>> >>>> } else if (sock && sk_has_rx_data(sock->sk)) { >>>> if (!vhost_vq_avail_empty(&net->dev, rvq)) { >>>> vhost_poll_queue(&rvq->poll); >>>> } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) { >>>> vhost_disable_notify(&net->dev, rvq); >>>> vhost_poll_queue(&rvq->poll); >>>> } >>>> } >>> As Jason review as before, we only want rx kick when packet is pending at >>> socket but we're out of available buffers. So we just enable notify, >>> but not poll it ? >>> >>> } else if ((sock && sk_has_rx_data(sock->sk)) && >>> !vhost_vq_avail_empty(&net->dev, rvq)) { >>> vhost_poll_queue(&rvq->poll); >>> else { >>> vhost_enable_notify(&net->dev, rvq); >>> } >> >> When vhost_enable_notify() returns true the avail becomes non-empty >> while we are enabling notify. We may delay the rx process if we don't >> check the return value of vhost_enable_notify(). > I got it thanks. >>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx? >>> I cant find why it is better, if necessary, we can do it. >> >> The reason is pretty simple... we are busypolling the socket so we don't >> need rx wakeups during it? > OK, but one question, how about rx? do we use the > vhost_net_disable_vq/vhost_net_ensable_vq on rx ?If we are busypolling the sock tx buf? I'm not sure if polling it improves the performance. -- Toshiaki Makita
Reasonably Related Threads
- [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
- [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
- [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
- [PATCH net-next v7 0/4] net: vhost: improve performance when enable busyloop
- [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()