Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/08/03 14:07, Jason Wang wrote:
> On 2018年08月03日 12:04, Tonghao Zhang wrote:
>> On Fri, Aug 3, 2018 at 11:43 AM Jason Wang wrote:
>>>
>>> On 2018年08月03日 11:24, Tonghao Zhang wrote:
On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang
>> wrote:
>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
On 2018/08/02 17:18, Jason Wang wrote:
> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>> + struct vhost_virtqueue *rvq,
>>> + struct vhost_virtqueue *tvq,
>>> + bool rx)
>>> +{
>>> + struct socket *sock = rvq->private_data;
>>> +
>>> + if (rx)
>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>> + else if (sock && sk_has_rx_data(sock->sk))
>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>> + else {
>>> + /* On tx here, sock has no rx data, so we
>>> + * will wait for sock wakeup for rx, and
>>> + * vhost_enable_notify() is not needed. */
>> A possible case is we do have rx data but guest does not
>> refill the rx
>> queue. In this case we may lose notifications from guest.
> Yes, should consider this case. thanks.
I'm a bit confused. Isn't this covered by the previous
"else if (sock && sk_has_rx_data(...))" block?
>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>>> vhost_enble_notify() is false.
>>>
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (!rx)
+ vhost_net_enable_vq(net, vq);
>>> No need to enable rx virtqueue, if we are sure handle_rx()
>>> will be
>>> called soon.
>> If we disable rx virtqueue in handle_tx and don't send packets
>> from
>> guest anymore(handle_tx is not called), so we can wake up for
>> sock rx.
>> so the network is broken.
> Not sure I understand here. I mean is we schedule work for
> handle_rx(),
> there's no need to enable it since handle_rx() will do this for
> us.
Looks like in the last "else" block in
vhost_net_busy_poll_check() we
need to enable vq since in that case we have no rx data and
handle_rx()
is not scheduled.
>>> Yes.
>> So we will use the vhost_has_work() to check whether or not the
>> handle_rx is scheduled ?
>> If we use the vhost_has_work(), the work in the dev work_list may be
>> rx work, or tx work, right ?
> Yes. We can add a boolean to record whether or not we've called
> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> it was true.
so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during
busypoll"
may not consider the case: work is tx work in the dev work list.
>>> So two kinds of work, tx kick or tx wakeup.
>>>
>>> For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
>>> by not enabling kick if we found something is pending on txq. For tx
>>> wakeup, yes, the commit does not consider it. And that's why we want to
>>> disable tx wakeups during busy polling.
>> And in the handle_rx but not busy polling, the tx can wakeup anytime
>> and the tx work will be added to dev work list. In that case, if we
>> add
>> the rx poll to the queue, it is necessary ？ the commit be294a51a may
>> check whether the rx work is in the dev work list.
>
> I think the point this we don't poll rx during tx at that time. So if rx
> poll is interrupted, we should reschedule handle_rx(). After we poll rx
> on handle_tx(), we can try to optimize this on top.
That's true. We may be able to skip poll_queue in handle_rx/tx after
rx/tx busypolling is unified by this patch set.
--
Toshiaki Makita
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月03日 12:04, Tonghao Zhang wrote:
On Fri, Aug 3, 2018 at 11:43 AM Jason Wang wrote:
On 2018年08月03日 11:24, Tonghao Zhang wrote:
On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
On 2018年08月03日 10:51, Tonghao Zhang wrote:
On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
On 2018年08月02日 16:41, Toshiaki Makita wrote:
On 2018/08/02 17:18, Jason Wang wrote:
On 2018年08月01日 17:52, Tonghao Zhang wrote:
+static void vhost_net_busy_poll_check(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ struct socket *sock = rvq->private_data;
+
+ if (rx)
+ vhost_net_busy_poll_try_queue(net, tvq);
+ else if (sock && sk_has_rx_data(sock->sk))
+ vhost_net_busy_poll_try_queue(net, rvq);
+ else {
+ /* On tx here, sock has no rx data, so we
+ * will wait for sock wakeup for rx, and
+ * vhost_enable_notify() is not needed. */
A possible case is we do have rx data but guest does not refill the rx
queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
I'm a bit confused. Isn't this covered by the previous
"else if (sock && sk_has_rx_data(...))" block?
The problem is it does nothing if vhost_vq_avail_empty() is true and
vhost_enble_notify() is false.
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (!rx)
+ vhost_net_enable_vq(net, vq);
No need to enable rx virtqueue, if we are sure handle_rx() will be
called soon.
If we disable rx virtqueue in handle_tx and don't send packets from
guest anymore(handle_tx is not called), so we can wake up for sock rx.
so the network is broken.
Not sure I understand here. I mean is we schedule work for handle_rx(),
there's no need to enable it since handle_rx() will do this for us.
Looks like in the last "else" block in vhost_net_busy_poll_check() we
need to enable vq since in that case we have no rx data and handle_rx()
is not scheduled.
Yes.
So we will use the vhost_has_work() to check whether or not the
handle_rx is scheduled ?
If we use the vhost_has_work(), the work in the dev work_list may be
rx work, or tx work, right ?
Yes. We can add a boolean to record whether or not we've called
vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
it was true.
so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
may not consider the case: work is tx work in the dev work list.
So two kinds of work, tx kick or tx wakeup.
For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
by not enabling kick if we found something is pending on txq. For tx
wakeup, yes, the commit does not consider it. And that's why we want to
disable tx wakeups during busy polling.
And in the handle_rx but not busy polling, the tx can wakeup anytime
and the tx work will be added to dev work list. In that case, if we
add
the rx poll to the queue, it is necessary ？ the commit be294a51a may
check whether the rx work is in the dev work list.
I think the point this we don't poll rx during tx at that time. So if rx
poll is interrupted, we should reschedule handle_rx(). After we poll rx
on handle_tx(), we can try to optimize this on top.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/08/03 13:14, Tonghao Zhang wrote:
> On Fri, Aug 3, 2018 at 11:40 AM Toshiaki Makita
> wrote:
>>
>> On 2018/08/03 12:24, Tonghao Zhang wrote:
>>> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
On 2018年08月03日 10:51, Tonghao Zhang wrote:
> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>> On 2018/08/02 17:18, Jason Wang wrote:
On 2018年08月01日 17:52, Tonghao Zhang wrote:
>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>> + struct vhost_virtqueue *rvq,
>> + struct vhost_virtqueue *tvq,
>> + bool rx)
>> +{
>> + struct socket *sock = rvq->private_data;
>> +
>> + if (rx)
>> + vhost_net_busy_poll_try_queue(net, tvq);
>> + else if (sock && sk_has_rx_data(sock->sk))
>> + vhost_net_busy_poll_try_queue(net, rvq);
>> + else {
>> + /* On tx here, sock has no rx data, so we
>> + * will wait for sock wakeup for rx, and
>> + * vhost_enable_notify() is not needed. */
> A possible case is we do have rx data but guest does not refill the rx
> queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
>>> I'm a bit confused. Isn't this covered by the previous
>>> "else if (sock && sk_has_rx_data(...))" block?
>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>> vhost_enble_notify() is false.
>>
>>> +
>>> + cpu_relax();
>>> + }
>>> +
>>> + preempt_enable();
>>> +
>>> + if (!rx)
>>> + vhost_net_enable_vq(net, vq);
>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>> called soon.
> If we disable rx virtqueue in handle_tx and don't send packets from
> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> so the network is broken.
Not sure I understand here. I mean is we schedule work for handle_rx(),
there's no need to enable it since handle_rx() will do this for us.
>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>> need to enable vq since in that case we have no rx data and handle_rx()
>>> is not scheduled.
>>>
>> Yes.
> So we will use the vhost_has_work() to check whether or not the
> handle_rx is scheduled ?
> If we use the vhost_has_work(), the work in the dev work_list may be
> rx work, or tx work, right ?
Yes. We can add a boolean to record whether or not we've called
vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
it was true.
>>> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during
>>> busypoll"
>>> may not consider the case: work is tx work in the dev work list.
>>
>> Not sure what you are concerned but what I can say is that we need to
>> poll rx work if vhost_has_work() detects tx work in
>> vhost_net_rx_peek_head_len() since rx busypoll exits prematurely in that
>> case.
> In the handle_rx, when we busy poll, the vhost_has_work() return true,
> because the tx but not rx work is in the dev work list.
> and it is the most case, because tx work may be added to dev work list
> anytime(not during busy poll) when guest kick the vhost-net.
> so it is not necessary to add it., right ?
I'm lost.
What is the part you think is not needed?
1. When there is a tx work we exit rx busypoll.
2. When we exit rx busypoll by tx work, we poll rx work (so that we can
continue rx busypoll later).
--
Toshiaki Makita
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Fri, Aug 3, 2018 at 11:40 AM Toshiaki Makita
wrote:
>
> On 2018/08/03 12:24, Tonghao Zhang wrote:
> > On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
> >> On 2018年08月03日 10:51, Tonghao Zhang wrote:
> >>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> > On 2018/08/02 17:18, Jason Wang wrote:
> >> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> + struct vhost_virtqueue *rvq,
> + struct vhost_virtqueue *tvq,
> + bool rx)
> +{
> + struct socket *sock = rvq->private_data;
> +
> + if (rx)
> + vhost_net_busy_poll_try_queue(net, tvq);
> + else if (sock && sk_has_rx_data(sock->sk))
> + vhost_net_busy_poll_try_queue(net, rvq);
> + else {
> + /* On tx here, sock has no rx data, so we
> + * will wait for sock wakeup for rx, and
> + * vhost_enable_notify() is not needed. */
> >>> A possible case is we do have rx data but guest does not refill the rx
> >>> queue. In this case we may lose notifications from guest.
> >> Yes, should consider this case. thanks.
> > I'm a bit confused. Isn't this covered by the previous
> > "else if (sock && sk_has_rx_data(...))" block?
> The problem is it does nothing if vhost_vq_avail_empty() is true and
> vhost_enble_notify() is false.
>
> > +
> > + cpu_relax();
> > + }
> > +
> > + preempt_enable();
> > +
> > + if (!rx)
> > + vhost_net_enable_vq(net, vq);
> No need to enable rx virtqueue, if we are sure handle_rx() will be
> called soon.
> >>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>> so the network is broken.
> >> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >> there's no need to enable it since handle_rx() will do this for us.
> > Looks like in the last "else" block in vhost_net_busy_poll_check() we
> > need to enable vq since in that case we have no rx data and handle_rx()
> > is not scheduled.
> >
> Yes.
> >>> So we will use the vhost_has_work() to check whether or not the
> >>> handle_rx is scheduled ?
> >>> If we use the vhost_has_work(), the work in the dev work_list may be
> >>> rx work, or tx work, right ?
> >>
> >> Yes. We can add a boolean to record whether or not we've called
> >> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> >> it was true.
> > so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during
> > busypoll"
> > may not consider the case: work is tx work in the dev work list.
>
> Not sure what you are concerned but what I can say is that we need to
> poll rx work if vhost_has_work() detects tx work in
> vhost_net_rx_peek_head_len() since rx busypoll exits prematurely in that
> case.
In the handle_rx, when we busy poll, the vhost_has_work() return true,
because the tx but not rx work is in the dev work list.
and it is the most case, because tx work may be added to dev work list
anytime(not during busy poll) when guest kick the vhost-net.
so it is not necessary to add it., right ?
> >> So here's the needed changes:
> >>
> >> 1) Split the wakeup disabling to another patch
> >> 2) Squash the vhost_net_busy_poll_try_queue() and
> >> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
> >> duplicated checks.
> >> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
> >> poll_rx, this makes code more readable.
> > OK
> >> Thanks
> >>
> Thanks
> >>
> >
> >
>
> --
> Toshiaki Makita
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Fri, Aug 3, 2018 at 11:43 AM Jason Wang wrote:
>
>
>
> On 2018年08月03日 11:24, Tonghao Zhang wrote:
> > On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
> >>
> >>
> >> On 2018年08月03日 10:51, Tonghao Zhang wrote:
> >>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
>
> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> > On 2018/08/02 17:18, Jason Wang wrote:
> >> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> + struct vhost_virtqueue *rvq,
> + struct vhost_virtqueue *tvq,
> + bool rx)
> +{
> + struct socket *sock = rvq->private_data;
> +
> + if (rx)
> + vhost_net_busy_poll_try_queue(net, tvq);
> + else if (sock && sk_has_rx_data(sock->sk))
> + vhost_net_busy_poll_try_queue(net, rvq);
> + else {
> + /* On tx here, sock has no rx data, so we
> + * will wait for sock wakeup for rx, and
> + * vhost_enable_notify() is not needed. */
> >>> A possible case is we do have rx data but guest does not refill the rx
> >>> queue. In this case we may lose notifications from guest.
> >> Yes, should consider this case. thanks.
> > I'm a bit confused. Isn't this covered by the previous
> > "else if (sock && sk_has_rx_data(...))" block?
> The problem is it does nothing if vhost_vq_avail_empty() is true and
> vhost_enble_notify() is false.
>
> > +
> > + cpu_relax();
> > + }
> > +
> > + preempt_enable();
> > +
> > + if (!rx)
> > + vhost_net_enable_vq(net, vq);
> No need to enable rx virtqueue, if we are sure handle_rx() will be
> called soon.
> >>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>> so the network is broken.
> >> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >> there's no need to enable it since handle_rx() will do this for us.
> > Looks like in the last "else" block in vhost_net_busy_poll_check() we
> > need to enable vq since in that case we have no rx data and handle_rx()
> > is not scheduled.
> >
> Yes.
> >>> So we will use the vhost_has_work() to check whether or not the
> >>> handle_rx is scheduled ?
> >>> If we use the vhost_has_work(), the work in the dev work_list may be
> >>> rx work, or tx work, right ?
> >> Yes. We can add a boolean to record whether or not we've called
> >> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> >> it was true.
> > so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during
> > busypoll"
> > may not consider the case: work is tx work in the dev work list.
>
> So two kinds of work, tx kick or tx wakeup.
>
> For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
> by not enabling kick if we found something is pending on txq. For tx
> wakeup, yes, the commit does not consider it. And that's why we want to
> disable tx wakeups during busy polling.
And in the handle_rx but not busy polling, the tx can wakeup anytime
and the tx work will be added to dev work list. In that case, if we
add
the rx poll to the queue, it is necessary ？ the commit be294a51a may
check whether the rx work is in the dev work list.
> Thanks
>
> >
> >> So here's the needed changes:
> >>
> >> 1) Split the wakeup disabling to another patch
> >> 2) Squash the vhost_net_busy_poll_try_queue() and
> >> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
> >> duplicated checks.
> >> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
> >> poll_rx, this makes code more readable.
> > OK
> >> Thanks
> >>
> Thanks
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月03日 11:32, Toshiaki Makita wrote: On 2018/08/03 12:07, Jason Wang wrote: On 2018年08月02日 17:23, Jason Wang wrote: No need to enable rx virtqueue, if we are sure handle_rx() will be called soon. If we disable rx virtqueue in handle_tx and don't send packets from guest anymore(handle_tx is not called), so we can wake up for sock rx. so the network is broken. Not sure I understand here. I mean is we schedule work for handle_rx(), there's no need to enable it since handle_rx() will do this for us. Looks like in the last "else" block in vhost_net_busy_poll_check() we need to enable vq since in that case we have no rx data and handle_rx() is not scheduled. Rethink about this, looks not. We enable rx wakeups in this case, so if there's pending data, handle_rx() will be schedule after vhost_net_enable_vq(). You are right, but what I wanted to say is vhost_net_enable_vq() should be needed (I was talking about what would happen if vhost_net_enable_vq() were removed). Also, I think we should move vhost_net_enable_vq() from vhost_net_busy_poll() to this last "else" block because this is the case where rx wakeups is required. Anyway this part will be refactored so let's see what this code will look like in next version. I get your point. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/08/03 12:24, Tonghao Zhang wrote:
> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
>> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
On 2018年08月02日 16:41, Toshiaki Makita wrote:
> On 2018/08/02 17:18, Jason Wang wrote:
>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
+static void vhost_net_busy_poll_check(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ struct socket *sock = rvq->private_data;
+
+ if (rx)
+ vhost_net_busy_poll_try_queue(net, tvq);
+ else if (sock && sk_has_rx_data(sock->sk))
+ vhost_net_busy_poll_try_queue(net, rvq);
+ else {
+ /* On tx here, sock has no rx data, so we
+ * will wait for sock wakeup for rx, and
+ * vhost_enable_notify() is not needed. */
>>> A possible case is we do have rx data but guest does not refill the rx
>>> queue. In this case we may lose notifications from guest.
>> Yes, should consider this case. thanks.
> I'm a bit confused. Isn't this covered by the previous
> "else if (sock && sk_has_rx_data(...))" block?
The problem is it does nothing if vhost_vq_avail_empty() is true and
vhost_enble_notify() is false.
> +
> + cpu_relax();
> + }
> +
> + preempt_enable();
> +
> + if (!rx)
> + vhost_net_enable_vq(net, vq);
No need to enable rx virtqueue, if we are sure handle_rx() will be
called soon.
>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>> so the network is broken.
>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>> there's no need to enable it since handle_rx() will do this for us.
> Looks like in the last "else" block in vhost_net_busy_poll_check() we
> need to enable vq since in that case we have no rx data and handle_rx()
> is not scheduled.
>
Yes.
>>> So we will use the vhost_has_work() to check whether or not the
>>> handle_rx is scheduled ?
>>> If we use the vhost_has_work(), the work in the dev work_list may be
>>> rx work, or tx work, right ?
>>
>> Yes. We can add a boolean to record whether or not we've called
>> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
>> it was true.
> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
> may not consider the case: work is tx work in the dev work list.
Not sure what you are concerned but what I can say is that we need to
poll rx work if vhost_has_work() detects tx work in
vhost_net_rx_peek_head_len() since rx busypoll exits prematurely in that
case.
>> So here's the needed changes:
>>
>> 1) Split the wakeup disabling to another patch
>> 2) Squash the vhost_net_busy_poll_try_queue() and
>> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
>> duplicated checks.
>> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
>> poll_rx, this makes code more readable.
> OK
>> Thanks
>>
Thanks
>>
>
>
--
Toshiaki Makita
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/08/03 12:07, Jason Wang wrote: > On 2018年08月02日 17:23, Jason Wang wrote: >>> >> No need to enable rx virtqueue, if we are sure handle_rx() will be >> called soon. > If we disable rx virtqueue in handle_tx and don't send packets from > guest anymore(handle_tx is not called), so we can wake up for sock rx. > so the network is broken. Not sure I understand here. I mean is we schedule work for handle_rx(), there's no need to enable it since handle_rx() will do this for us. >>> Looks like in the last "else" block in vhost_net_busy_poll_check() we >>> need to enable vq since in that case we have no rx data and handle_rx() >>> is not scheduled. >>> > Rethink about this, looks not. We enable rx wakeups in this case, so if > there's pending data, handle_rx() will be schedule after > vhost_net_enable_vq(). You are right, but what I wanted to say is vhost_net_enable_vq() should be needed (I was talking about what would happen if vhost_net_enable_vq() were removed). Also, I think we should move vhost_net_enable_vq() from vhost_net_busy_poll() to this last "else" block because this is the case where rx wakeups is required. Anyway this part will be refactored so let's see what this code will look like in next version. -- Toshiaki Makita ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Fri, Aug 3, 2018 at 11:07 AM Jason Wang wrote:
>
>
>
> On 2018年08月03日 10:51, Tonghao Zhang wrote:
> > On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
> >>
> >>
> >> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> >>> On 2018/08/02 17:18, Jason Wang wrote:
> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> >> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> >> + struct vhost_virtqueue *rvq,
> >> + struct vhost_virtqueue *tvq,
> >> + bool rx)
> >> +{
> >> + struct socket *sock = rvq->private_data;
> >> +
> >> + if (rx)
> >> + vhost_net_busy_poll_try_queue(net, tvq);
> >> + else if (sock && sk_has_rx_data(sock->sk))
> >> + vhost_net_busy_poll_try_queue(net, rvq);
> >> + else {
> >> + /* On tx here, sock has no rx data, so we
> >> + * will wait for sock wakeup for rx, and
> >> + * vhost_enable_notify() is not needed. */
> > A possible case is we do have rx data but guest does not refill the rx
> > queue. In this case we may lose notifications from guest.
> Yes, should consider this case. thanks.
> >>> I'm a bit confused. Isn't this covered by the previous
> >>> "else if (sock && sk_has_rx_data(...))" block?
> >> The problem is it does nothing if vhost_vq_avail_empty() is true and
> >> vhost_enble_notify() is false.
> >>
> >>> +
> >>> + cpu_relax();
> >>> + }
> >>> +
> >>> + preempt_enable();
> >>> +
> >>> + if (!rx)
> >>> + vhost_net_enable_vq(net, vq);
> >> No need to enable rx virtqueue, if we are sure handle_rx() will be
> >> called soon.
> > If we disable rx virtqueue in handle_tx and don't send packets from
> > guest anymore(handle_tx is not called), so we can wake up for sock rx.
> > so the network is broken.
> Not sure I understand here. I mean is we schedule work for handle_rx(),
> there's no need to enable it since handle_rx() will do this for us.
> >>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
> >>> need to enable vq since in that case we have no rx data and handle_rx()
> >>> is not scheduled.
> >>>
> >> Yes.
> > So we will use the vhost_has_work() to check whether or not the
> > handle_rx is scheduled ?
> > If we use the vhost_has_work(), the work in the dev work_list may be
> > rx work, or tx work, right ?
>
> Yes. We can add a boolean to record whether or not we've called
> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> it was true.
so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
may not consider the case: work is tx work in the dev work list.
> So here's the needed changes:
>
> 1) Split the wakeup disabling to another patch
> 2) Squash the vhost_net_busy_poll_try_queue() and
> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
> duplicated checks.
> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
> poll_rx, this makes code more readable.
OK
> Thanks
>
> >> Thanks
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月03日 10:51, Tonghao Zhang wrote:
On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
On 2018年08月02日 16:41, Toshiaki Makita wrote:
On 2018/08/02 17:18, Jason Wang wrote:
On 2018年08月01日 17:52, Tonghao Zhang wrote:
+static void vhost_net_busy_poll_check(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ struct socket *sock = rvq->private_data;
+
+ if (rx)
+ vhost_net_busy_poll_try_queue(net, tvq);
+ else if (sock && sk_has_rx_data(sock->sk))
+ vhost_net_busy_poll_try_queue(net, rvq);
+ else {
+ /* On tx here, sock has no rx data, so we
+ * will wait for sock wakeup for rx, and
+ * vhost_enable_notify() is not needed. */
A possible case is we do have rx data but guest does not refill the rx
queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
I'm a bit confused. Isn't this covered by the previous
"else if (sock && sk_has_rx_data(...))" block?
The problem is it does nothing if vhost_vq_avail_empty() is true and
vhost_enble_notify() is false.
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (!rx)
+ vhost_net_enable_vq(net, vq);
No need to enable rx virtqueue, if we are sure handle_rx() will be
called soon.
If we disable rx virtqueue in handle_tx and don't send packets from
guest anymore(handle_tx is not called), so we can wake up for sock rx.
so the network is broken.
Not sure I understand here. I mean is we schedule work for handle_rx(),
there's no need to enable it since handle_rx() will do this for us.
Looks like in the last "else" block in vhost_net_busy_poll_check() we
need to enable vq since in that case we have no rx data and handle_rx()
is not scheduled.
Yes.
So we will use the vhost_has_work() to check whether or not the
handle_rx is scheduled ?
If we use the vhost_has_work(), the work in the dev work_list may be
rx work, or tx work, right ?
Yes. We can add a boolean to record whether or not we've called
vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
it was true.
So here's the needed changes:
1) Split the wakeup disabling to another patch
2) Squash the vhost_net_busy_poll_try_queue() and
vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
duplicated checks.
3) If possible, rename the boolean rx in vhost_net_busy_poll() to
poll_rx, this makes code more readable.
Thanks
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月02日 17:23, Jason Wang wrote: No need to enable rx virtqueue, if we are sure handle_rx() will be called soon. If we disable rx virtqueue in handle_tx and don't send packets from guest anymore(handle_tx is not called), so we can wake up for sock rx. so the network is broken. Not sure I understand here. I mean is we schedule work for handle_rx(), there's no need to enable it since handle_rx() will do this for us. Looks like in the last "else" block in vhost_net_busy_poll_check() we need to enable vq since in that case we have no rx data and handle_rx() is not scheduled. Yes. Thanks Rethink about this, looks not. We enable rx wakeups in this case, so if there's pending data, handle_rx() will be schedule after vhost_net_enable_vq(). Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Thu, Aug 2, 2018 at 5:23 PM Jason Wang wrote:
>
>
>
> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> > On 2018/08/02 17:18, Jason Wang wrote:
> >> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> + struct vhost_virtqueue *rvq,
> + struct vhost_virtqueue *tvq,
> + bool rx)
> +{
> + struct socket *sock = rvq->private_data;
> +
> + if (rx)
> + vhost_net_busy_poll_try_queue(net, tvq);
> + else if (sock && sk_has_rx_data(sock->sk))
> + vhost_net_busy_poll_try_queue(net, rvq);
> + else {
> + /* On tx here, sock has no rx data, so we
> + * will wait for sock wakeup for rx, and
> + * vhost_enable_notify() is not needed. */
> >>> A possible case is we do have rx data but guest does not refill the rx
> >>> queue. In this case we may lose notifications from guest.
> >> Yes, should consider this case. thanks.
> > I'm a bit confused. Isn't this covered by the previous
> > "else if (sock && sk_has_rx_data(...))" block?
>
> The problem is it does nothing if vhost_vq_avail_empty() is true and
> vhost_enble_notify() is false.
>
> >
> > +
> > + cpu_relax();
> > + }
> > +
> > + preempt_enable();
> > +
> > + if (!rx)
> > + vhost_net_enable_vq(net, vq);
> No need to enable rx virtqueue, if we are sure handle_rx() will be
> called soon.
> >>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>> so the network is broken.
> >> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >> there's no need to enable it since handle_rx() will do this for us.
> > Looks like in the last "else" block in vhost_net_busy_poll_check() we
> > need to enable vq since in that case we have no rx data and handle_rx()
> > is not scheduled.
> >
>
> Yes.
So we will use the vhost_has_work() to check whether or not the
handle_rx is scheduled ?
If we use the vhost_has_work(), the work in the dev work_list may be
rx work, or tx work, right ?
> Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月02日 17:57, Toshiaki Makita wrote:
On 2018/08/02 18:23, Jason Wang wrote:
On 2018年08月02日 16:41, Toshiaki Makita wrote:
On 2018/08/02 17:18, Jason Wang wrote:
On 2018年08月01日 17:52, Tonghao Zhang wrote:
+static void vhost_net_busy_poll_check(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ struct socket *sock = rvq->private_data;
+
+ if (rx)
+ vhost_net_busy_poll_try_queue(net, tvq);
+ else if (sock && sk_has_rx_data(sock->sk))
+ vhost_net_busy_poll_try_queue(net, rvq);
+ else {
+ /* On tx here, sock has no rx data, so we
+ * will wait for sock wakeup for rx, and
+ * vhost_enable_notify() is not needed. */
A possible case is we do have rx data but guest does not refill the rx
queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
I'm a bit confused. Isn't this covered by the previous
"else if (sock && sk_has_rx_data(...))" block?
The problem is it does nothing if vhost_vq_avail_empty() is true and
vhost_enble_notify() is false.
If vhost_enable_notify() is false, guest will eventually kicks vq, no?
Yes, you are right.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/08/02 18:23, Jason Wang wrote:
> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>> On 2018/08/02 17:18, Jason Wang wrote:
>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> + struct vhost_virtqueue *rvq,
> + struct vhost_virtqueue *tvq,
> + bool rx)
> +{
> + struct socket *sock = rvq->private_data;
> +
> + if (rx)
> + vhost_net_busy_poll_try_queue(net, tvq);
> + else if (sock && sk_has_rx_data(sock->sk))
> + vhost_net_busy_poll_try_queue(net, rvq);
> + else {
> + /* On tx here, sock has no rx data, so we
> + * will wait for sock wakeup for rx, and
> + * vhost_enable_notify() is not needed. */
A possible case is we do have rx data but guest does not refill the rx
queue. In this case we may lose notifications from guest.
>>> Yes, should consider this case. thanks.
>> I'm a bit confused. Isn't this covered by the previous
>> "else if (sock && sk_has_rx_data(...))" block?
>
> The problem is it does nothing if vhost_vq_avail_empty() is true and
> vhost_enble_notify() is false.
If vhost_enable_notify() is false, guest will eventually kicks vq, no?
--
Toshiaki Makita
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月02日 16:41, Toshiaki Makita wrote:
On 2018/08/02 17:18, Jason Wang wrote:
On 2018年08月01日 17:52, Tonghao Zhang wrote:
+static void vhost_net_busy_poll_check(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ struct socket *sock = rvq->private_data;
+
+ if (rx)
+ vhost_net_busy_poll_try_queue(net, tvq);
+ else if (sock && sk_has_rx_data(sock->sk))
+ vhost_net_busy_poll_try_queue(net, rvq);
+ else {
+ /* On tx here, sock has no rx data, so we
+ * will wait for sock wakeup for rx, and
+ * vhost_enable_notify() is not needed. */
A possible case is we do have rx data but guest does not refill the rx
queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
I'm a bit confused. Isn't this covered by the previous
"else if (sock && sk_has_rx_data(...))" block?
The problem is it does nothing if vhost_vq_avail_empty() is true and
vhost_enble_notify() is false.
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (!rx)
+ vhost_net_enable_vq(net, vq);
No need to enable rx virtqueue, if we are sure handle_rx() will be
called soon.
If we disable rx virtqueue in handle_tx and don't send packets from
guest anymore(handle_tx is not called), so we can wake up for sock rx.
so the network is broken.
Not sure I understand here. I mean is we schedule work for handle_rx(),
there's no need to enable it since handle_rx() will do this for us.
Looks like in the last "else" block in vhost_net_busy_poll_check() we
need to enable vq since in that case we have no rx data and handle_rx()
is not scheduled.
Yes.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018/08/02 17:18, Jason Wang wrote:
> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>> + struct vhost_virtqueue *rvq,
>>> + struct vhost_virtqueue *tvq,
>>> + bool rx)
>>> +{
>>> + struct socket *sock = rvq->private_data;
>>> +
>>> + if (rx)
>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>> + else if (sock && sk_has_rx_data(sock->sk))
>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>> + else {
>>> + /* On tx here, sock has no rx data, so we
>>> + * will wait for sock wakeup for rx, and
>>> + * vhost_enable_notify() is not needed. */
>>
>> A possible case is we do have rx data but guest does not refill the rx
>> queue. In this case we may lose notifications from guest.
> Yes, should consider this case. thanks.
I'm a bit confused. Isn't this covered by the previous
"else if (sock && sk_has_rx_data(...))" block?
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (!rx)
+ vhost_net_enable_vq(net, vq);
>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>> called soon.
>> If we disable rx virtqueue in handle_tx and don't send packets from
>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>> so the network is broken.
>
> Not sure I understand here. I mean is we schedule work for handle_rx(),
> there's no need to enable it since handle_rx() will do this for us.
Looks like in the last "else" block in vhost_net_busy_poll_check() we
need to enable vq since in that case we have no rx data and handle_rx()
is not scheduled.
--
Toshiaki Makita
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月01日 17:52, Tonghao Zhang wrote: + + cpu_relax(); + } + + preempt_enable(); + + if (!rx) + vhost_net_enable_vq(net, vq); No need to enable rx virtqueue, if we are sure handle_rx() will be called soon. If we disable rx virtqueue in handle_tx and don't send packets from guest anymore(handle_tx is not called), so we can wake up for sock rx. so the network is broken. Not sure I understand here. I mean is we schedule work for handle_rx(), there's no need to enable it since handle_rx() will do this for us. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On Wed, Aug 1, 2018 at 2:01 PM Jason Wang wrote:
>
>
>
> On 2018年08月01日 11:00, xiangxia.m@gmail.com wrote:
> > From: Tonghao Zhang
> >
> > 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.
> >
> > In the handle_tx, the busypoll will vhost_net_disable/enable_vq
> > because we have poll the sock. This can improve performance.
> > [This is suggested by Toshiaki Makita ]
> >
> > And when the sock receive skb, we should queue the poll if necessary.
> >
> > Signed-off-by: Tonghao Zhang
> > ---
> > drivers/vhost/net.c | 131
> >
> > 1 file changed, 91 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 32c1b52..5b45463 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -440,6 +440,95 @@ static void vhost_net_signal_used(struct
> > vhost_net_virtqueue *nvq)
> > nvq->done_idx = 0;
> > }
> >
> > +static int sk_has_rx_data(struct sock *sk)
> > +{
> > + struct socket *sock = sk->sk_socket;
> > +
> > + if (sock->ops->peek_len)
> > + return sock->ops->peek_len(sock);
> > +
> > + return skb_queue_empty(>sk_receive_queue);
> > +}
> > +
> > +static void vhost_net_busy_poll_try_queue(struct vhost_net *net,
> > + struct vhost_virtqueue *vq)
> > +{
> > + if (!vhost_vq_avail_empty(>dev, vq)) {
> > + vhost_poll_queue(>poll);
> > + } else if (unlikely(vhost_enable_notify(>dev, vq))) {
> > + vhost_disable_notify(>dev, vq);
> > + vhost_poll_queue(>poll);
> > + }
> > +}
> > +
> > +static void vhost_net_busy_poll_check(struct vhost_net *net,
> > + struct vhost_virtqueue *rvq,
> > + struct vhost_virtqueue *tvq,
> > + bool rx)
> > +{
> > + struct socket *sock = rvq->private_data;
> > +
> > + if (rx)
> > + vhost_net_busy_poll_try_queue(net, tvq);
> > + else if (sock && sk_has_rx_data(sock->sk))
> > + vhost_net_busy_poll_try_queue(net, rvq);
> > + else {
> > + /* On tx here, sock has no rx data, so we
> > + * will wait for sock wakeup for rx, and
> > + * vhost_enable_notify() is not needed. */
>
> A possible case is we do have rx data but guest does not refill the rx
> queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
> > + }
> > +}
> > +
> > +static void vhost_net_busy_poll(struct vhost_net *net,
> > + struct vhost_virtqueue *rvq,
> > + struct vhost_virtqueue *tvq,
> > + bool *busyloop_intr,
> > + bool rx)
> > +{
> > + unsigned long busyloop_timeout;
> > + unsigned long endtime;
> > + struct socket *sock;
> > + struct vhost_virtqueue *vq = rx ? tvq : rvq;
> > +
> > + mutex_lock_nested(>mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
> > + vhost_disable_notify(>dev, vq);
> > + sock = rvq->private_data;
> > +
> > + busyloop_timeout = rx ? rvq->busyloop_timeout:
> > + tvq->busyloop_timeout;
> > +
> > +
> > + /* Busypoll the sock, so don't need rx wakeups during it. */
> > + if (!rx)
> > + vhost_net_disable_vq(net, vq);
>
> Actually this piece of code is not a factoring out. I would suggest to
> add this in another patch, or on top of this series.
I will add this in another patch.
> > +
> > + preempt_disable();
> > + endtime = busy_clock() + busyloop_timeout;
> > +
> > + while (vhost_can_busy_poll(endtime)) {
> > + if (vhost_has_work(>dev)) {
> > + *busyloop_intr = true;
> > + break;
> > + }
> > +
> > + if ((sock && sk_has_rx_data(sock->sk) &&
> > + !vhost_vq_avail_empty(>dev, rvq)) ||
> > + !vhost_vq_avail_empty(>dev, tvq))
> > + break;
>
> Some checks were duplicated in vhost_net_busy_poll_check(). Need
> consider to unify them.
OK
> > +
> > + cpu_relax();
> > + }
> > +
> > + preempt_enable();
> > +
> > + if (!rx)
> > + vhost_net_enable_vq(net, vq);
>
> No need to enable rx virtqueue, if we are sure handle_rx() will be
> called soon.
If we disable rx virtqueue in handle_tx and don't send packets from
guest anymore(handle_tx is not called), so we can wake up for sock rx.
so the network is broken.
> > +
> > + vhost_net_busy_poll_check(net, rvq, tvq, rx);
>
> It looks to me just open code all check here is better and easier to be
> reviewed.
will be changed.
> Thanks
>
> > +
> > + mutex_unlock(>mutex);
> > +}
> > +
> > static int vhost_net_tx_get_vq_desc(struct
Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
On 2018年08月01日 11:00, xiangxia.m@gmail.com wrote:
From: Tonghao Zhang
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.
In the handle_tx, the busypoll will vhost_net_disable/enable_vq
because we have poll the sock. This can improve performance.
[This is suggested by Toshiaki Makita ]
And when the sock receive skb, we should queue the poll if necessary.
Signed-off-by: Tonghao Zhang
---
drivers/vhost/net.c | 131
1 file changed, 91 insertions(+), 40 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 32c1b52..5b45463 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -440,6 +440,95 @@ static void vhost_net_signal_used(struct
vhost_net_virtqueue *nvq)
nvq->done_idx = 0;
}
+static int sk_has_rx_data(struct sock *sk)
+{
+ struct socket *sock = sk->sk_socket;
+
+ if (sock->ops->peek_len)
+ return sock->ops->peek_len(sock);
+
+ return skb_queue_empty(>sk_receive_queue);
+}
+
+static void vhost_net_busy_poll_try_queue(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
+{
+ if (!vhost_vq_avail_empty(>dev, vq)) {
+ vhost_poll_queue(>poll);
+ } else if (unlikely(vhost_enable_notify(>dev, vq))) {
+ vhost_disable_notify(>dev, vq);
+ vhost_poll_queue(>poll);
+ }
+}
+
+static void vhost_net_busy_poll_check(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ struct socket *sock = rvq->private_data;
+
+ if (rx)
+ vhost_net_busy_poll_try_queue(net, tvq);
+ else if (sock && sk_has_rx_data(sock->sk))
+ vhost_net_busy_poll_try_queue(net, rvq);
+ else {
+ /* On tx here, sock has no rx data, so we
+* will wait for sock wakeup for rx, and
+* vhost_enable_notify() is not needed. */
A possible case is we do have rx data but guest does not refill the rx
queue. In this case we may lose notifications from guest.
+ }
+}
+
+static void vhost_net_busy_poll(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool *busyloop_intr,
+ bool rx)
+{
+ unsigned long busyloop_timeout;
+ unsigned long endtime;
+ struct socket *sock;
+ struct vhost_virtqueue *vq = rx ? tvq : rvq;
+
+ mutex_lock_nested(>mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+ vhost_disable_notify(>dev, vq);
+ sock = rvq->private_data;
+
+ busyloop_timeout = rx ? rvq->busyloop_timeout:
+ tvq->busyloop_timeout;
+
+
+ /* Busypoll the sock, so don't need rx wakeups during it. */
+ if (!rx)
+ vhost_net_disable_vq(net, vq);
Actually this piece of code is not a factoring out. I would suggest to
add this in another patch, or on top of this series.
+
+ preempt_disable();
+ endtime = busy_clock() + busyloop_timeout;
+
+ while (vhost_can_busy_poll(endtime)) {
+ if (vhost_has_work(>dev)) {
+ *busyloop_intr = true;
+ break;
+ }
+
+ if ((sock && sk_has_rx_data(sock->sk) &&
+!vhost_vq_avail_empty(>dev, rvq)) ||
+ !vhost_vq_avail_empty(>dev, tvq))
+ break;
Some checks were duplicated in vhost_net_busy_poll_check(). Need
consider to unify them.
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (!rx)
+ vhost_net_enable_vq(net, vq);
No need to enable rx virtqueue, if we are sure handle_rx() will be
called soon.
+
+ vhost_net_busy_poll_check(net, rvq, tvq, rx);
It looks to me just open code all check here is better and easier to be
reviewed.
Thanks
+
+ mutex_unlock(>mutex);
+}
+
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct vhost_net_virtqueue *nvq,
unsigned int *out_num, unsigned int *in_num,
@@ -753,16 +842,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq,
struct sock *sk)
return len;
}
-static int sk_has_rx_data(struct sock *sk)
-{
- struct socket *sock = sk->sk_socket;
-
- if (sock->ops->peek_len)
- return sock->ops->peek_len(sock);
-
- return skb_queue_empty(>sk_receive_queue);
-}
-
static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
bool
