* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-29 22:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <20170829233920-mutt-send-email-mst@kernel.org>
On Tue, Aug 29, 2017 at 4:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 03:53:08PM -0400, Willem de Bruijn wrote:
>> On Tue, Aug 29, 2017 at 3:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Tue, Aug 29, 2017 at 03:35:38PM -0400, Willem de Bruijn wrote:
>> >> By the way, I have had an unrelated patch outstanding for a while
>> >> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
>> >> command. Will send that as RFC.
>> >
>> > Oh nice.
>>
>> Great :)
>>
>> > One needs to be careful about locking there which is why
>> > no devices support that yet.
>>
>> I originally wrote it based on the virtnet_reset function introduced
>> for xdp. Calling this from virtnet_config_changed_work is non trivial,
>> as virtnet_freeze_down waits until no config worker is running.
>>
>> Otherwise, I could not find any constraints on when freeze may be
>> called, and it largely follows the same path. I hope I didn't miss anything.
>
> The issue is that on freeze processes are not running so we
> generally know no new packets will arrive (might be wrong
> for bridging, then it's a bug). On device error you must
> prevent new skbs from coming in, etc.
Thanks a lot for the quick review. I had indeed not yet figured out
which invariants freeze can depend on that are not universal. Same
for the virtnet_reset call from the .ndo_xdp. Will need to take a much
better look at that.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-08-30 1:45 UTC (permalink / raw)
To: Willem de Bruijn, Michael S. Tsirkin
Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-+Wk9sc9dXMUq1+x_hh=3ThTXa6BnZkygP3tgVpjbp93g@mail.gmail.com>
On 2017年08月30日 03:35, Willem de Bruijn wrote:
> On Fri, Aug 25, 2017 at 9:03 PM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Fri, Aug 25, 2017 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Fri, Aug 25, 2017 at 06:44:36PM -0400, Willem de Bruijn wrote:
>>>>>>>>> We don't enable network watchdog on virtio but we could and maybe
>>>>>>>>> should.
>>>>>>>> Can you elaborate?
>>>>>>> The issue is that holding onto buffers for very long times makes guests
>>>>>>> think they are stuck. This is funamentally because from guest point of
>>>>>>> view this is a NIC, so it is supposed to transmit things out in
>>>>>>> a timely manner. If host backs the virtual NIC by something that is not
>>>>>>> a NIC, with traffic shaping etc introducing unbounded latencies,
>>>>>>> guest will be confused.
>>>>>> That assumes that guests are fragile in this regard. A linux guest
>>>>>> does not make such assumptions.
>>>>> Yes it does. Examples above:
>>>>> > > - a single slow flow can occupy the whole ring, you will not
>>>>> > > be able to make any new buffers available for the fast flow
>>>> Oh, right. Though those are due to vring_desc pool exhaustion
>>>> rather than an upper bound on latency of any single packet.
>>>>
>>>> Limiting the number of zerocopy packets in flight to some fraction
>>>> of the ring ensures that fast flows can always grab a slot.
>>>> Running
>>>> out of ubuf_info slots reverts to copy, so indirectly does this. But
>>>> I read it correclty the zerocopy pool may be equal to or larger than
>>>> the descriptor pool. Should we refine the zcopy_used test
>>>>
>>>> (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
>>>>
>>>> to also return false if the number of outstanding ubuf_info is greater
>>>> than, say, vq->num >> 1?
>>>
>>> We'll need to think about where to put the threshold, but I think it's
>>> a good idea.
>>>
>>> Maybe even a fixed number, e.g. max(vq->num >> 1, X) to limit host
>>> resources.
>>>
>>> In a sense it still means once you run out of slots zcopt gets disabled possibly permanently.
>>>
>>> Need to experiment with some numbers.
>> I can take a stab with two flows, one delayed in a deep host qdisc
>> queue. See how this change affects the other flow and also how
>> sensitive that is to the chosen threshold value.
> Incomplete results at this stage, but I do see this correlation between
> flows. It occurs even while not running out of zerocopy descriptors,
> which I cannot yet explain.
>
> Running two threads in a guest, each with a udp socket, each
> sending up to 100 datagrams, or until EAGAIN, every msec.
>
> Sender A sends 1B datagrams.
> Sender B sends VHOST_GOODCOPY_LEN, which is enough
> to trigger zcopy_used in vhost net.
>
> A local receive process on the host receives both flows. To avoid
> a deep copy when looping the packet onto the receive path,
> changed skb_orphan_frags_rx to always return false (gross hack).
>
> The flow with the larger packets is redirected through netem on ifb0:
>
> modprobe ifb
> ip link set dev ifb0 up
> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>
> tc qdisc add dev tap0 ingress
> tc filter add dev tap0 parent ffff: protocol ip \
> u32 match ip dport 8000 0xffff \
> action mirred egress redirect dev ifb0
>
> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>
> no filter
> rx.A: ~840,000
> rx.B: ~840,000
Just to make sure I understand the case here. What did rx.B mean here? I
thought all traffic sent by Sender B has been redirected to ifb0?
>
> limit 1
> rx.A: ~500,000
> rx.B: ~3100
> ifb0: 3273 sent, 371141 dropped
>
> limit 100
> rx.A: ~9000
> rx.B: ~4200
> ifb0: 4630 sent, 1491 dropped
>
> limit 1000
> rx.A: ~6800
> rx.B: ~4200
> ifb0: 4651 sent, 0 dropped
>
> Sender B is always correctly rate limited to 1 MBps or less. With a
> short queue, it ends up dropping a lot and sending even less.
>
> When a queue builds up for sender B, sender A throughput is strongly
> correlated with queue length. With queue length 1, it can send almost
> at unthrottled speed. But even at limit 100 its throughput is on the
> same order as sender B.
>
> What is surprising to me is that this happens even though the number
> of ubuf_info in use at limit 100 is around 100 at all times. In other words,
> it does not exhaust the pool.
>
> When forcing zcopy_used to be false for all packets, this effect of
> sender A throughput being correlated with sender B does not happen.
>
> no filter
> rx.A: ~850,000
> rx.B: ~850,000
>
> limit 100
> rx.A: ~850,000
> rx.B: ~4200
> ifb0: 4518 sent, 876182 dropped
>
> Also relevant is that with zerocopy, the sender processes back off
> and report the same count as the receiver. Without zerocopy,
> both senders send at full speed, even if only 4200 packets from flow
> B arrive at the receiver.
>
> This is with the default virtio_net driver, so without napi-tx.
What kind of qdisc do you use in guest? I suspect we should use
something which could do fair queueing (e.g sfq).
>
> It appears that the zerocopy notifications are pausing the guest.
> Will look at that now.
Another factor is the tx interrupt coalescing parameters of ifb0, maybe
we should disable it during the test.
Thanks
>
> By the way, I have had an unrelated patch outstanding for a while
> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
> command. Will send that as RFC.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-30 3:11 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <b8893b72-4d09-2492-0d31-5135286e6874@redhat.com>
On Tue, Aug 29, 2017 at 9:45 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年08月30日 03:35, Willem de Bruijn wrote:
>>
>> On Fri, Aug 25, 2017 at 9:03 PM, Willem de Bruijn
>> <willemdebruijn.kernel@gmail.com> wrote:
>>>
>>> On Fri, Aug 25, 2017 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com>
>>> wrote:
>>>>
>>>> On Fri, Aug 25, 2017 at 06:44:36PM -0400, Willem de Bruijn wrote:
>>>>>>>>>>
>>>>>>>>>> We don't enable network watchdog on virtio but we could and maybe
>>>>>>>>>> should.
>>>>>>>>>
>>>>>>>>> Can you elaborate?
>>>>>>>>
>>>>>>>> The issue is that holding onto buffers for very long times makes
>>>>>>>> guests
>>>>>>>> think they are stuck. This is funamentally because from guest point
>>>>>>>> of
>>>>>>>> view this is a NIC, so it is supposed to transmit things out in
>>>>>>>> a timely manner. If host backs the virtual NIC by something that is
>>>>>>>> not
>>>>>>>> a NIC, with traffic shaping etc introducing unbounded latencies,
>>>>>>>> guest will be confused.
>>>>>>>
>>>>>>> That assumes that guests are fragile in this regard. A linux guest
>>>>>>> does not make such assumptions.
>>>>>>
>>>>>> Yes it does. Examples above:
>>>>>> > > - a single slow flow can occupy the whole ring, you will
>>>>>> not
>>>>>> > > be able to make any new buffers available for the fast
>>>>>> flow
>>>>>
>>>>> Oh, right. Though those are due to vring_desc pool exhaustion
>>>>> rather than an upper bound on latency of any single packet.
>>>>>
>>>>> Limiting the number of zerocopy packets in flight to some fraction
>>>>> of the ring ensures that fast flows can always grab a slot.
>>>>> Running
>>>>> out of ubuf_info slots reverts to copy, so indirectly does this. But
>>>>> I read it correclty the zerocopy pool may be equal to or larger than
>>>>> the descriptor pool. Should we refine the zcopy_used test
>>>>>
>>>>> (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
>>>>>
>>>>> to also return false if the number of outstanding ubuf_info is greater
>>>>> than, say, vq->num >> 1?
>>>>
>>>>
>>>> We'll need to think about where to put the threshold, but I think it's
>>>> a good idea.
>>>>
>>>> Maybe even a fixed number, e.g. max(vq->num >> 1, X) to limit host
>>>> resources.
>>>>
>>>> In a sense it still means once you run out of slots zcopt gets disabled
>>>> possibly permanently.
>>>>
>>>> Need to experiment with some numbers.
>>>
>>> I can take a stab with two flows, one delayed in a deep host qdisc
>>> queue. See how this change affects the other flow and also how
>>> sensitive that is to the chosen threshold value.
>>
>> Incomplete results at this stage, but I do see this correlation between
>> flows. It occurs even while not running out of zerocopy descriptors,
>> which I cannot yet explain.
>>
>> Running two threads in a guest, each with a udp socket, each
>> sending up to 100 datagrams, or until EAGAIN, every msec.
>>
>> Sender A sends 1B datagrams.
>> Sender B sends VHOST_GOODCOPY_LEN, which is enough
>> to trigger zcopy_used in vhost net.
>>
>> A local receive process on the host receives both flows. To avoid
>> a deep copy when looping the packet onto the receive path,
>> changed skb_orphan_frags_rx to always return false (gross hack).
>>
>> The flow with the larger packets is redirected through netem on ifb0:
>>
>> modprobe ifb
>> ip link set dev ifb0 up
>> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>>
>> tc qdisc add dev tap0 ingress
>> tc filter add dev tap0 parent ffff: protocol ip \
>> u32 match ip dport 8000 0xffff \
>> action mirred egress redirect dev ifb0
>>
>> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>>
>> no filter
>> rx.A: ~840,000
>> rx.B: ~840,000
>
>
> Just to make sure I understand the case here. What did rx.B mean here? I
> thought all traffic sent by Sender B has been redirected to ifb0?
It has been, but the packet still arrives at the destination socket.
IFB is a special virtual device that applies traffic shaping and
then reinjects it back at the point it was intercept by mirred.
rx.B is indeed arrival rate at the receiver, similar to rx.A.
>>
>> limit 1
>> rx.A: ~500,000
>> rx.B: ~3100
>> ifb0: 3273 sent, 371141 dropped
>>
>> limit 100
>> rx.A: ~9000
>> rx.B: ~4200
>> ifb0: 4630 sent, 1491 dropped
>>
>> limit 1000
>> rx.A: ~6800
>> rx.B: ~4200
>> ifb0: 4651 sent, 0 dropped
>>
>> Sender B is always correctly rate limited to 1 MBps or less. With a
>> short queue, it ends up dropping a lot and sending even less.
>>
>> When a queue builds up for sender B, sender A throughput is strongly
>> correlated with queue length. With queue length 1, it can send almost
>> at unthrottled speed. But even at limit 100 its throughput is on the
>> same order as sender B.
>>
>> What is surprising to me is that this happens even though the number
>> of ubuf_info in use at limit 100 is around 100 at all times. In other
>> words,
>> it does not exhaust the pool.
>>
>> When forcing zcopy_used to be false for all packets, this effect of
>> sender A throughput being correlated with sender B does not happen.
>>
>> no filter
>> rx.A: ~850,000
>> rx.B: ~850,000
>>
>> limit 100
>> rx.A: ~850,000
>> rx.B: ~4200
>> ifb0: 4518 sent, 876182 dropped
>>
>> Also relevant is that with zerocopy, the sender processes back off
>> and report the same count as the receiver. Without zerocopy,
>> both senders send at full speed, even if only 4200 packets from flow
>> B arrive at the receiver.
>>
>> This is with the default virtio_net driver, so without napi-tx.
>
>
> What kind of qdisc do you use in guest? I suspect we should use something
> which could do fair queueing (e.g sfq).
Or fq. The test was using the default, pfifo_fast.
This particular two flow test probably would not be affected,
as something else is delaying both flows equally once some
completions are delayed.
One obvious candidate would be hitting VHOST_MAX_PEND.
But I instrumented that and handle_tx is never throttled by
vhost_exceeds_maxpend.
Btw, vhost_exceeds_maxpend implements almost what I
suggested earlier and was planning to test here: ensure that
only part of the descriptor pool is filled with zerocopy requests.
Only, it currently breaks out of the loop when the max is
reached. I think that we should move it into the main
zcopy_used calculation, so that hitting this threshold reverts to
copy-based transmission, instead of delaying all tx until
zerocopy budget opens up.
>>
>> It appears that the zerocopy notifications are pausing the guest.
>> Will look at that now.
>
>
> Another factor is the tx interrupt coalescing parameters of ifb0, maybe we
> should disable it during the test.
>
> Thanks
>
>
>>
>> By the way, I have had an unrelated patch outstanding for a while
>> to have virtio-net support the VIRTIO_CONFIG_S_NEEDS_RESET
>> command. Will send that as RFC.
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH v2 1/7] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Juergen Gross @ 2017-08-30 7:33 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Yang Zhang, xen-devel, Boris Ostrovsky
Cc: Jeremy Fitzhardinge, rkrcmar, kvm, mst, peterz, Pan Xinhui,
virtualization, H. Peter Anvin, Alok Kataria, wanpeng.li, x86,
Ingo Molnar, Kees Cook, Chris Wright, Andy Lutomirski, dmatlack,
tglx, Quan Xu, linux-doc, linux-kernel, pbonzini,
Kirill A. Shutemov
In-Reply-To: <20170829135548.GG32175@char.us.oracle.com>
On 29/08/17 15:55, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 29, 2017 at 11:46:35AM +0000, Yang Zhang wrote:
>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called in
>> idle path which will polling for a while before we enter the real idle
>> state.
>>
>> In virtualization, idle path includes several heavy operations
>> includes timer access(LAPIC timer or TSC deadline timer) which will hurt
>> performance especially for latency intensive workload like message
>> passing task. The cost is mainly come from the vmexit which is a
>> hardware context switch between VM and hypervisor. Our solution is to
>> poll for a while and do not enter real idle path if we can get the
>> schedule event during polling.
>>
>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>> reduce the useless poll.
>>
>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>> Cc: Chris Wright <chrisw@sous-sol.org>
>> Cc: Alok Kataria <akataria@vmware.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> Cc: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>
> Adding xen-devel.
>
> Juergen, we really should replace Jeremy's name with xen-devel or
> your name..
I wouldn't mind being added. What does Jeremy think of being removed?
> Wasn't there an patch by you that took some of the
> mainternship over it?
I added include/linux/hypervisor.h to the PARAVIRT section and offered
to maintain it in case the PARAVIRT maintainers didn't want to.
Juergen
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-31 14:30 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-+Wk9sc9dXMUq1+x_hh=3ThTXa6BnZkygP3tgVpjbp93g@mail.gmail.com>
On Tue, Aug 29, 2017 at 3:35 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, Aug 25, 2017 at 9:03 PM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Fri, Aug 25, 2017 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Fri, Aug 25, 2017 at 06:44:36PM -0400, Willem de Bruijn wrote:
>>>> >> >> > We don't enable network watchdog on virtio but we could and maybe
>>>> >> >> > should.
>>>> >> >>
>>>> >> >> Can you elaborate?
>>>> >> >
>>>> >> > The issue is that holding onto buffers for very long times makes guests
>>>> >> > think they are stuck. This is funamentally because from guest point of
>>>> >> > view this is a NIC, so it is supposed to transmit things out in
>>>> >> > a timely manner. If host backs the virtual NIC by something that is not
>>>> >> > a NIC, with traffic shaping etc introducing unbounded latencies,
>>>> >> > guest will be confused.
>>>> >>
>>>> >> That assumes that guests are fragile in this regard. A linux guest
>>>> >> does not make such assumptions.
>>>> >
>>>> > Yes it does. Examples above:
>>>> > > > - a single slow flow can occupy the whole ring, you will not
>>>> > > > be able to make any new buffers available for the fast flow
>>>>
>>>> Oh, right. Though those are due to vring_desc pool exhaustion
>>>> rather than an upper bound on latency of any single packet.
>>>>
>>>> Limiting the number of zerocopy packets in flight to some fraction
>>>> of the ring ensures that fast flows can always grab a slot.
>>>> Running
>>>> out of ubuf_info slots reverts to copy, so indirectly does this. But
>>>> I read it correclty the zerocopy pool may be equal to or larger than
>>>> the descriptor pool. Should we refine the zcopy_used test
>>>>
>>>> (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
>>>>
>>>> to also return false if the number of outstanding ubuf_info is greater
>>>> than, say, vq->num >> 1?
>>>
>>>
>>> We'll need to think about where to put the threshold, but I think it's
>>> a good idea.
>>>
>>> Maybe even a fixed number, e.g. max(vq->num >> 1, X) to limit host
>>> resources.
>>>
>>> In a sense it still means once you run out of slots zcopt gets disabled possibly permanently.
>>>
>>> Need to experiment with some numbers.
>>
>> I can take a stab with two flows, one delayed in a deep host qdisc
>> queue. See how this change affects the other flow and also how
>> sensitive that is to the chosen threshold value.
>
> Incomplete results at this stage, but I do see this correlation between
> flows. It occurs even while not running out of zerocopy descriptors,
> which I cannot yet explain.
>
> Running two threads in a guest, each with a udp socket, each
> sending up to 100 datagrams, or until EAGAIN, every msec.
>
> Sender A sends 1B datagrams.
> Sender B sends VHOST_GOODCOPY_LEN, which is enough
> to trigger zcopy_used in vhost net.
>
> A local receive process on the host receives both flows. To avoid
> a deep copy when looping the packet onto the receive path,
> changed skb_orphan_frags_rx to always return false (gross hack).
>
> The flow with the larger packets is redirected through netem on ifb0:
>
> modprobe ifb
> ip link set dev ifb0 up
> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>
> tc qdisc add dev tap0 ingress
> tc filter add dev tap0 parent ffff: protocol ip \
> u32 match ip dport 8000 0xffff \
> action mirred egress redirect dev ifb0
>
> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>
> no filter
> rx.A: ~840,000
> rx.B: ~840,000
>
> limit 1
> rx.A: ~500,000
> rx.B: ~3100
> ifb0: 3273 sent, 371141 dropped
>
> limit 100
> rx.A: ~9000
> rx.B: ~4200
> ifb0: 4630 sent, 1491 dropped
>
> limit 1000
> rx.A: ~6800
> rx.B: ~4200
> ifb0: 4651 sent, 0 dropped
>
> Sender B is always correctly rate limited to 1 MBps or less. With a
> short queue, it ends up dropping a lot and sending even less.
>
> When a queue builds up for sender B, sender A throughput is strongly
> correlated with queue length. With queue length 1, it can send almost
> at unthrottled speed. But even at limit 100 its throughput is on the
> same order as sender B.
>
> What is surprising to me is that this happens even though the number
> of ubuf_info in use at limit 100 is around 100 at all times. In other words,
> it does not exhaust the pool.
>
> When forcing zcopy_used to be false for all packets, this effect of
> sender A throughput being correlated with sender B does not happen.
>
> no filter
> rx.A: ~850,000
> rx.B: ~850,000
>
> limit 100
> rx.A: ~850,000
> rx.B: ~4200
> ifb0: 4518 sent, 876182 dropped
>
> Also relevant is that with zerocopy, the sender processes back off
> and report the same count as the receiver. Without zerocopy,
> both senders send at full speed, even if only 4200 packets from flow
> B arrive at the receiver.
>
> This is with the default virtio_net driver, so without napi-tx.
>
> It appears that the zerocopy notifications are pausing the guest.
> Will look at that now.
It was indeed as simple as that. With 256 descriptors, queuing even
a hundred or so packets causes the guest to stall the device as soon
as the qdisc is installed.
Adding this check
+ in_use = nvq->upend_idx - nvq->done_idx;
+ if (nvq->upend_idx < nvq->done_idx)
+ in_use += UIO_MAXIOV;
+
+ if (in_use > (vq->num >> 2))
+ zcopy_used = false;
Has the desired behavior of reverting zerocopy requests to copying.
Without this change, the result is, as previously reported, throughput
dropping to hundreds of packets per second on both flows.
With the change, pps as observed for a few seconds at handle_tx is
zerocopy=165 copy=168435
zerocopy=0 copy=168500
zerocopy=65 copy=168535
Both flows continue to send at more or less normal rate, with only
sender B observing massive drops at the netem.
With the queue removed the rate reverts to
zerocopy=58878 copy=110239
zerocopy=58833 copy=110207
This is not a 50/50 split, which implies that some packets from the large
packet flow are still converted to copying. Without the change the rate
without queue was 80k zerocopy vs 80k copy, so this choice of
(vq->num >> 2) appears too conservative.
However, testing with (vq->num >> 1) was not as effective at mitigating
stalls. I did not save that data, unfortunately. Can run more tests on fine
tuning this variable, if the idea sounds good.
I also compiled qemu with 1024 descriptors in the tx ring instead of 256.
In that case the test
if (unlikely(vhost_exceeds_maxpend(net)))
break;
which is
return (nvq->upend_idx + vq->num - VHOST_MAX_PEND) % UIO_MAXIOV
== nvq->done_idx;
Is hit before my proposed change, again stalling the device instead of
reverting to copying.
^ permalink raw reply
* virtio_net: ethtool supported link modes
From: Radu Rendec via Virtualization @ 2017-08-31 17:04 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: Michael S. Tsirkin
Hello,
Looking at the code in virtnet_set_link_ksettings, it seems the speed
and duplex can be set to any valid value. The driver will "remember"
them and report them back in virtnet_get_link_ksettings.
However, the supported link modes (link_modes.supported in struct
ethtool_link_ksettings) is always 0, indicating that no speed/duplex
setting is supported.
Does it make more sense to set (at least a few of) the supported link
modes, such as 10baseT_Half ... 10000baseT_Full?
I would expect to see consistency between what is reported in
link_modes.supported and what can actually be set. Could you please
share your opinion on this?
Thank you,
Radu Rendec
^ permalink raw reply
* [PATCH] virtgpu: remove redundant task_comm copying
From: Michał Mirosław @ 2017-08-31 18:06 UTC (permalink / raw)
To: David Airlie, Gerd Hoffmann; +Cc: dri-devel, virtualization
Remove redundant task name copying because:
1. task->comm is already NUL-terminated
2. virtio_gpu_context_create() trims passed dbgname anyway
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/gpu/drm/virtio/virtgpu_kms.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 1e1c90b30d4a..6f5c4a8c0ba8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -272,20 +272,18 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
struct virtio_gpu_device *vgdev = dev->dev_private;
struct virtio_gpu_fpriv *vfpriv;
uint32_t id;
- char dbgname[64], tmpname[TASK_COMM_LEN];
+ char dbgname[TASK_COMM_LEN];
/* can't create contexts without 3d renderer */
if (!vgdev->has_virgl_3d)
return 0;
- get_task_comm(tmpname, current);
- snprintf(dbgname, sizeof(dbgname), "%s", tmpname);
- dbgname[63] = 0;
/* allocate a virt GPU context for this opener */
vfpriv = kzalloc(sizeof(*vfpriv), GFP_KERNEL);
if (!vfpriv)
return -ENOMEM;
+ get_task_comm(dbgname, current);
virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname, &id);
vfpriv->ctx_id = id;
--
2.11.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-09-01 3:08 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <CAF=yD-KUoW6hxZtpAmyVrJXCY+=Fq1FOcbD3h=HmDQaPoC1MLg@mail.gmail.com>
On 2017年08月30日 11:11, Willem de Bruijn wrote:
> On Tue, Aug 29, 2017 at 9:45 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2017年08月30日 03:35, Willem de Bruijn wrote:
>>> On Fri, Aug 25, 2017 at 9:03 PM, Willem de Bruijn
>>> <willemdebruijn.kernel@gmail.com> wrote:
>>>> On Fri, Aug 25, 2017 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com>
>>>> wrote:
>>>>> On Fri, Aug 25, 2017 at 06:44:36PM -0400, Willem de Bruijn wrote:
[...]
>>> Incomplete results at this stage, but I do see this correlation between
>>> flows. It occurs even while not running out of zerocopy descriptors,
>>> which I cannot yet explain.
>>>
>>> Running two threads in a guest, each with a udp socket, each
>>> sending up to 100 datagrams, or until EAGAIN, every msec.
>>>
>>> Sender A sends 1B datagrams.
>>> Sender B sends VHOST_GOODCOPY_LEN, which is enough
>>> to trigger zcopy_used in vhost net.
>>>
>>> A local receive process on the host receives both flows. To avoid
>>> a deep copy when looping the packet onto the receive path,
>>> changed skb_orphan_frags_rx to always return false (gross hack).
>>>
>>> The flow with the larger packets is redirected through netem on ifb0:
>>>
>>> modprobe ifb
>>> ip link set dev ifb0 up
>>> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>>>
>>> tc qdisc add dev tap0 ingress
>>> tc filter add dev tap0 parent ffff: protocol ip \
>>> u32 match ip dport 8000 0xffff \
>>> action mirred egress redirect dev ifb0
>>>
>>> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>>>
>>> no filter
>>> rx.A: ~840,000
>>> rx.B: ~840,000
>>
>> Just to make sure I understand the case here. What did rx.B mean here? I
>> thought all traffic sent by Sender B has been redirected to ifb0?
> It has been, but the packet still arrives at the destination socket.
> IFB is a special virtual device that applies traffic shaping and
> then reinjects it back at the point it was intercept by mirred.
>
> rx.B is indeed arrival rate at the receiver, similar to rx.A.
>
I see, then ifb looks pretty fit to the test.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-09-01 3:25 UTC (permalink / raw)
To: Willem de Bruijn, Michael S. Tsirkin
Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-+AjQLLUKdvnrwd2tqFtw4Hm81cR7WUJd65oLnziNGM8A@mail.gmail.com>
On 2017年08月31日 22:30, Willem de Bruijn wrote:
>> Incomplete results at this stage, but I do see this correlation between
>> flows. It occurs even while not running out of zerocopy descriptors,
>> which I cannot yet explain.
>>
>> Running two threads in a guest, each with a udp socket, each
>> sending up to 100 datagrams, or until EAGAIN, every msec.
>>
>> Sender A sends 1B datagrams.
>> Sender B sends VHOST_GOODCOPY_LEN, which is enough
>> to trigger zcopy_used in vhost net.
>>
>> A local receive process on the host receives both flows. To avoid
>> a deep copy when looping the packet onto the receive path,
>> changed skb_orphan_frags_rx to always return false (gross hack).
>>
>> The flow with the larger packets is redirected through netem on ifb0:
>>
>> modprobe ifb
>> ip link set dev ifb0 up
>> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>>
>> tc qdisc add dev tap0 ingress
>> tc filter add dev tap0 parent ffff: protocol ip \
>> u32 match ip dport 8000 0xffff \
>> action mirred egress redirect dev ifb0
>>
>> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>>
>> no filter
>> rx.A: ~840,000
>> rx.B: ~840,000
>>
>> limit 1
>> rx.A: ~500,000
>> rx.B: ~3100
>> ifb0: 3273 sent, 371141 dropped
>>
>> limit 100
>> rx.A: ~9000
>> rx.B: ~4200
>> ifb0: 4630 sent, 1491 dropped
>>
>> limit 1000
>> rx.A: ~6800
>> rx.B: ~4200
>> ifb0: 4651 sent, 0 dropped
>>
>> Sender B is always correctly rate limited to 1 MBps or less. With a
>> short queue, it ends up dropping a lot and sending even less.
>>
>> When a queue builds up for sender B, sender A throughput is strongly
>> correlated with queue length. With queue length 1, it can send almost
>> at unthrottled speed. But even at limit 100 its throughput is on the
>> same order as sender B.
>>
>> What is surprising to me is that this happens even though the number
>> of ubuf_info in use at limit 100 is around 100 at all times. In other words,
>> it does not exhaust the pool.
>>
>> When forcing zcopy_used to be false for all packets, this effect of
>> sender A throughput being correlated with sender B does not happen.
>>
>> no filter
>> rx.A: ~850,000
>> rx.B: ~850,000
>>
>> limit 100
>> rx.A: ~850,000
>> rx.B: ~4200
>> ifb0: 4518 sent, 876182 dropped
>>
>> Also relevant is that with zerocopy, the sender processes back off
>> and report the same count as the receiver. Without zerocopy,
>> both senders send at full speed, even if only 4200 packets from flow
>> B arrive at the receiver.
>>
>> This is with the default virtio_net driver, so without napi-tx.
>>
>> It appears that the zerocopy notifications are pausing the guest.
>> Will look at that now.
> It was indeed as simple as that. With 256 descriptors, queuing even
> a hundred or so packets causes the guest to stall the device as soon
> as the qdisc is installed.
>
> Adding this check
>
> + in_use = nvq->upend_idx - nvq->done_idx;
> + if (nvq->upend_idx < nvq->done_idx)
> + in_use += UIO_MAXIOV;
> +
> + if (in_use > (vq->num >> 2))
> + zcopy_used = false;
>
> Has the desired behavior of reverting zerocopy requests to copying.
>
> Without this change, the result is, as previously reported, throughput
> dropping to hundreds of packets per second on both flows.
>
> With the change, pps as observed for a few seconds at handle_tx is
>
> zerocopy=165 copy=168435
> zerocopy=0 copy=168500
> zerocopy=65 copy=168535
>
> Both flows continue to send at more or less normal rate, with only
> sender B observing massive drops at the netem.
>
> With the queue removed the rate reverts to
>
> zerocopy=58878 copy=110239
> zerocopy=58833 copy=110207
>
> This is not a 50/50 split, which impliesTw that some packets from the large
> packet flow are still converted to copying. Without the change the rate
> without queue was 80k zerocopy vs 80k copy, so this choice of
> (vq->num >> 2) appears too conservative.
>
> However, testing with (vq->num >> 1) was not as effective at mitigating
> stalls. I did not save that data, unfortunately. Can run more tests on fine
> tuning this variable, if the idea sounds good.
Looks like there're still two cases were left:
1) sndbuf is not INT_MAX
2) tx napi is used for virtio-net
1) could be a corner case, and for 2) what your suggest here may not
solve the issue since it still do in order completion.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: virtio_net: ethtool supported link modes
From: Jason Wang @ 2017-09-01 3:36 UTC (permalink / raw)
To: Radu Rendec, virtualization, netdev, linux-kernel; +Cc: Michael S. Tsirkin
In-Reply-To: <1504199044.22080.11.camel@arista.com>
On 2017年09月01日 01:04, Radu Rendec wrote:
> Hello,
>
> Looking at the code in virtnet_set_link_ksettings, it seems the speed
> and duplex can be set to any valid value. The driver will "remember"
> them and report them back in virtnet_get_link_ksettings.
>
> However, the supported link modes (link_modes.supported in struct
> ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> setting is supported.
>
> Does it make more sense to set (at least a few of) the supported link
> modes, such as 10baseT_Half ... 10000baseT_Full?
>
> I would expect to see consistency between what is reported in
> link_modes.supported and what can actually be set. Could you please
> share your opinion on this?
I think the may make sense only if there's a hardware implementation for
virtio. And we probably need to extend virtio spec for adding new commands.
Thanks
>
> Thank you,
> Radu Rendec
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH v2 1/7] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Yang Zhang @ 2017-09-01 6:50 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, jgross, Boris Ostrovsky
Cc: Jeremy Fitzhardinge, rkrcmar, kvm, mst, peterz, Pan Xinhui,
virtualization, H. Peter Anvin, Alok Kataria, wanpeng.li, x86,
Ingo Molnar, Kees Cook, Chris Wright, Andy Lutomirski, dmatlack,
tglx, Quan Xu, linux-doc, linux-kernel, pbonzini,
Kirill A. Shutemov
In-Reply-To: <20170829135548.GG32175@char.us.oracle.com>
On 2017/8/29 21:55, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 29, 2017 at 11:46:35AM +0000, Yang Zhang wrote:
>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called in
>> idle path which will polling for a while before we enter the real idle
>> state.
>>
>> In virtualization, idle path includes several heavy operations
>> includes timer access(LAPIC timer or TSC deadline timer) which will hurt
>> performance especially for latency intensive workload like message
>> passing task. The cost is mainly come from the vmexit which is a
>> hardware context switch between VM and hypervisor. Our solution is to
>> poll for a while and do not enter real idle path if we can get the
>> schedule event during polling.
>>
>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>> reduce the useless poll.
>>
>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>> Cc: Chris Wright <chrisw@sous-sol.org>
>> Cc: Alok Kataria <akataria@vmware.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> Cc: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>
> Adding xen-devel.
>
> Juergen, we really should replace Jeremy's name with xen-devel or
> your name.. Wasn't there an patch by you that took some of the
> mainternship over it?
Hi Konard, I didn't test it in Xen side since i don't have the
environment but i can add it for Xen in next version if you think it is
useful to Xen as well.
>
>> ---
>> arch/x86/include/asm/paravirt.h | 5 +++++
>> arch/x86/include/asm/paravirt_types.h | 6 ++++++
>> arch/x86/kernel/paravirt.c | 6 ++++++
>> 3 files changed, 17 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>> index 9ccac19..6d46760 100644
>> --- a/arch/x86/include/asm/paravirt.h
>> +++ b/arch/x86/include/asm/paravirt.h
>> @@ -202,6 +202,11 @@ static inline unsigned long long paravirt_read_pmc(int counter)
>>
>> #define rdpmcl(counter, val) ((val) = paravirt_read_pmc(counter))
>>
>> +static inline void paravirt_idle_poll(void)
>> +{
>> + PVOP_VCALL0(pv_idle_ops.poll);
>> +}
>> +
>> static inline void paravirt_alloc_ldt(struct desc_struct *ldt, unsigned entries)
>> {
>> PVOP_VCALL2(pv_cpu_ops.alloc_ldt, ldt, entries);
>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>> index 9ffc36b..cf45726 100644
>> --- a/arch/x86/include/asm/paravirt_types.h
>> +++ b/arch/x86/include/asm/paravirt_types.h
>> @@ -324,6 +324,10 @@ struct pv_lock_ops {
>> struct paravirt_callee_save vcpu_is_preempted;
>> } __no_randomize_layout;
>>
>> +struct pv_idle_ops {
>> + void (*poll)(void);
>> +} __no_randomize_layout;
>> +
>> /* This contains all the paravirt structures: we get a convenient
>> * number for each function using the offset which we use to indicate
>> * what to patch. */
>> @@ -334,6 +338,7 @@ struct paravirt_patch_template {
>> struct pv_irq_ops pv_irq_ops;
>> struct pv_mmu_ops pv_mmu_ops;
>> struct pv_lock_ops pv_lock_ops;
>> + struct pv_idle_ops pv_idle_ops;
>> } __no_randomize_layout;
>>
>> extern struct pv_info pv_info;
>> @@ -343,6 +348,7 @@ struct paravirt_patch_template {
>> extern struct pv_irq_ops pv_irq_ops;
>> extern struct pv_mmu_ops pv_mmu_ops;
>> extern struct pv_lock_ops pv_lock_ops;
>> +extern struct pv_idle_ops pv_idle_ops;
>>
>> #define PARAVIRT_PATCH(x) \
>> (offsetof(struct paravirt_patch_template, x) / sizeof(void *))
>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
>> index bc0a849..1b5b247 100644
>> --- a/arch/x86/kernel/paravirt.c
>> +++ b/arch/x86/kernel/paravirt.c
>> @@ -128,6 +128,7 @@ static void *get_call_destination(u8 type)
>> #ifdef CONFIG_PARAVIRT_SPINLOCKS
>> .pv_lock_ops = pv_lock_ops,
>> #endif
>> + .pv_idle_ops = pv_idle_ops,
>> };
>> return *((void **)&tmpl + type);
>> }
>> @@ -312,6 +313,10 @@ struct pv_time_ops pv_time_ops = {
>> .steal_clock = native_steal_clock,
>> };
>>
>> +struct pv_idle_ops pv_idle_ops = {
>> + .poll = paravirt_nop,
>> +};
>> +
>> __visible struct pv_irq_ops pv_irq_ops = {
>> .save_fl = __PV_IS_CALLEE_SAVE(native_save_fl),
>> .restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl),
>> @@ -471,3 +476,4 @@ struct pv_mmu_ops pv_mmu_ops __ro_after_init = {
>> EXPORT_SYMBOL (pv_mmu_ops);
>> EXPORT_SYMBOL_GPL(pv_info);
>> EXPORT_SYMBOL (pv_irq_ops);
>> +EXPORT_SYMBOL (pv_idle_ops);
>> --
>> 1.8.3.1
>>
--
Yang
Alibaba Cloud Computing
^ permalink raw reply
* [PATCH net] vhost_net: correctly check tx avail during rx busy polling
From: Jason Wang @ 2017-09-01 9:02 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, linux-kernel, kvm, virtualization
We check tx avail through vhost_enable_notify() in the past which is
wrong since it only checks whether or not guest has filled more
available buffer since last avail idx synchronization which was just
done by vhost_vq_avail_empty() before. What we really want is checking
pending buffers in the avail ring. Fix this by calling
vhost_vq_avail_empty() instead.
This issue could be noticed by doing netperf TCP_RR benchmark as
client from guest (but not host). With this fix, TCP_RR from guest to
localhost restores from 1375.91 trans per sec to 55235.28 trans per
sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
Fixes: 030881372460 ("vhost_net: basic polling support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
- The patch is needed for -stable
---
drivers/vhost/net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 06d0448..1b68253 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -634,7 +634,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
preempt_enable();
- if (vhost_enable_notify(&net->dev, vq))
+ if (!vhost_vq_avail_empty(&net->dev, vq))
vhost_poll_queue(&vq->poll);
mutex_unlock(&vq->mutex);
--
2.7.4
^ permalink raw reply related
* Re: virtio_net: ethtool supported link modes
From: Radu Rendec via Virtualization @ 2017-09-01 12:01 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <38286395-683f-f285-aaa5-68f0e8b68675@redhat.com>
On Fri, 2017-09-01 at 11:36 +0800, Jason Wang wrote:
>
> On 2017年09月01日 01:04, Radu Rendec wrote:
> > Hello,
> >
> > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > and duplex can be set to any valid value. The driver will "remember"
> > them and report them back in virtnet_get_link_ksettings.
> >
> > However, the supported link modes (link_modes.supported in struct
> > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > setting is supported.
> >
> > Does it make more sense to set (at least a few of) the supported link
> > modes, such as 10baseT_Half ... 10000baseT_Full?
> >
> > I would expect to see consistency between what is reported in
> > link_modes.supported and what can actually be set. Could you please
> > share your opinion on this?
>
> I think the may make sense only if there's a hardware implementation for
> virtio. And we probably need to extend virtio spec for adding new commands.
So you're saying that the "hardware" should provide the supported link
modes (e.g. by using feature bits at the virtio layer) and the
virtio_net driver should just translate them and expose them as
link_modes.supported?
Then for consistency, I assume setting speed/duplex via ethtool should
also go into the virtio layer (currently virtio_net seems to just store
them for future retrieval via ethtool).
Thanks,
Radu
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: virtio_net: ethtool supported link modes
From: Michael S. Tsirkin @ 2017-09-01 15:43 UTC (permalink / raw)
To: Radu Rendec; +Cc: netdev, virtio-dev, linux-kernel, virtualization
In-Reply-To: <1504199044.22080.11.camel@arista.com>
On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> Hello,
>
> Looking at the code in virtnet_set_link_ksettings, it seems the speed
> and duplex can be set to any valid value. The driver will "remember"
> them and report them back in virtnet_get_link_ksettings.
>
> However, the supported link modes (link_modes.supported in struct
> ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> setting is supported.
>
> Does it make more sense to set (at least a few of) the supported link
> modes, such as 10baseT_Half ... 10000baseT_Full?
>
> I would expect to see consistency between what is reported in
> link_modes.supported and what can actually be set. Could you please
> share your opinion on this?
>
> Thank you,
> Radu Rendec
I would like to know more about why this is desirable.
We used not to support the modes at all, but it turned out
some tools are confused by this: e.g. people would try to
bond virtio with a hardware device, tools would see
a mismatch in speed and features between bonded devices
and get confused.
See
commit 16032be56c1f66770da15cb94f0eb366c37aff6e
Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Wed Feb 3 04:04:37 2016 +0100
virtio_net: add ethtool support for set and get of settings
as well as the discussion around it
https://www.spinics.net/lists/netdev/msg362111.html
If you think we need to add more hacks like this, a stronger
motivation than "to see consistency" would be needed.
--
MST
^ permalink raw reply
* Re: [PATCH net] vhost_net: correctly check tx avail during rx busy polling
From: Michael S. Tsirkin @ 2017-09-01 15:51 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1504256570-3488-1-git-send-email-jasowang@redhat.com>
On Fri, Sep 01, 2017 at 05:02:50PM +0800, Jason Wang wrote:
> We check tx avail through vhost_enable_notify() in the past which is
> wrong since it only checks whether or not guest has filled more
> available buffer since last avail idx synchronization which was just
> done by vhost_vq_avail_empty() before. What we really want is checking
> pending buffers in the avail ring.
These are rx buffers, right? I'm not even sure why do we need to poll
for them. Running out of rx buffers is a slow path.
> Fix this by calling
> vhost_vq_avail_empty() instead.
>
> This issue could be noticed by doing netperf TCP_RR benchmark as
> client from guest (but not host). With this fix, TCP_RR from guest to
> localhost restores from 1375.91 trans per sec to 55235.28 trans per
> sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
>
> Fixes: 030881372460 ("vhost_net: basic polling support")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> - The patch is needed for -stable
> ---
> drivers/vhost/net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 06d0448..1b68253 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -634,7 +634,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
In fact why does it poll the ring at all? I thought this function's
job is to poll the socket, isn't it?
>
> preempt_enable();
>
> - if (vhost_enable_notify(&net->dev, vq))
> + if (!vhost_vq_avail_empty(&net->dev, vq))
> vhost_poll_queue(&vq->poll);
> mutex_unlock(&vq->mutex);
Adding more contex:
mutex_lock(&vq->mutex);
vhost_disable_notify(&net->dev, vq);
preempt_disable();
endtime = busy_clock() + vq->busyloop_timeout;
while (vhost_can_busy_poll(&net->dev, endtime) &&
!sk_has_rx_data(sk) &&
vhost_vq_avail_empty(&net->dev, vq))
cpu_relax();
preempt_enable();
if (vhost_enable_notify(&net->dev, vq))
vhost_poll_queue(&vq->poll);
mutex_unlock(&vq->mutex);
len = peek_head_len(rvq, sk);
If you drop this we'll exit the function with notifications
disabled. Seems wrong to me.
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-09-01 16:15 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <5ef7fcf3-d4f0-be16-6ddb-724d954cfc68@redhat.com>
On Thu, Aug 31, 2017 at 11:25 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年08月31日 22:30, Willem de Bruijn wrote:
>>>
>>> Incomplete results at this stage, but I do see this correlation between
>>> flows. It occurs even while not running out of zerocopy descriptors,
>>> which I cannot yet explain.
>>>
>>> Running two threads in a guest, each with a udp socket, each
>>> sending up to 100 datagrams, or until EAGAIN, every msec.
>>>
>>> Sender A sends 1B datagrams.
>>> Sender B sends VHOST_GOODCOPY_LEN, which is enough
>>> to trigger zcopy_used in vhost net.
>>>
>>> A local receive process on the host receives both flows. To avoid
>>> a deep copy when looping the packet onto the receive path,
>>> changed skb_orphan_frags_rx to always return false (gross hack).
>>>
>>> The flow with the larger packets is redirected through netem on ifb0:
>>>
>>> modprobe ifb
>>> ip link set dev ifb0 up
>>> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>>>
>>> tc qdisc add dev tap0 ingress
>>> tc filter add dev tap0 parent ffff: protocol ip \
>>> u32 match ip dport 8000 0xffff \
>>> action mirred egress redirect dev ifb0
>>>
>>> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>>>
>>> no filter
>>> rx.A: ~840,000
>>> rx.B: ~840,000
>>>
>>> limit 1
>>> rx.A: ~500,000
>>> rx.B: ~3100
>>> ifb0: 3273 sent, 371141 dropped
>>>
>>> limit 100
>>> rx.A: ~9000
>>> rx.B: ~4200
>>> ifb0: 4630 sent, 1491 dropped
>>>
>>> limit 1000
>>> rx.A: ~6800
>>> rx.B: ~4200
>>> ifb0: 4651 sent, 0 dropped
>>>
>>> Sender B is always correctly rate limited to 1 MBps or less. With a
>>> short queue, it ends up dropping a lot and sending even less.
>>>
>>> When a queue builds up for sender B, sender A throughput is strongly
>>> correlated with queue length. With queue length 1, it can send almost
>>> at unthrottled speed. But even at limit 100 its throughput is on the
>>> same order as sender B.
>>>
>>> What is surprising to me is that this happens even though the number
>>> of ubuf_info in use at limit 100 is around 100 at all times. In other
>>> words,
>>> it does not exhaust the pool.
>>>
>>> When forcing zcopy_used to be false for all packets, this effect of
>>> sender A throughput being correlated with sender B does not happen.
>>>
>>> no filter
>>> rx.A: ~850,000
>>> rx.B: ~850,000
>>>
>>> limit 100
>>> rx.A: ~850,000
>>> rx.B: ~4200
>>> ifb0: 4518 sent, 876182 dropped
>>>
>>> Also relevant is that with zerocopy, the sender processes back off
>>> and report the same count as the receiver. Without zerocopy,
>>> both senders send at full speed, even if only 4200 packets from flow
>>> B arrive at the receiver.
>>>
>>> This is with the default virtio_net driver, so without napi-tx.
>>>
>>> It appears that the zerocopy notifications are pausing the guest.
>>> Will look at that now.
>>
>> It was indeed as simple as that. With 256 descriptors, queuing even
>> a hundred or so packets causes the guest to stall the device as soon
>> as the qdisc is installed.
>>
>> Adding this check
>>
>> + in_use = nvq->upend_idx - nvq->done_idx;
>> + if (nvq->upend_idx < nvq->done_idx)
>> + in_use += UIO_MAXIOV;
>> +
>> + if (in_use > (vq->num >> 2))
>> + zcopy_used = false;
>>
>> Has the desired behavior of reverting zerocopy requests to copying.
>>
>> Without this change, the result is, as previously reported, throughput
>> dropping to hundreds of packets per second on both flows.
>>
>> With the change, pps as observed for a few seconds at handle_tx is
>>
>> zerocopy=165 copy=168435
>> zerocopy=0 copy=168500
>> zerocopy=65 copy=168535
>>
>> Both flows continue to send at more or less normal rate, with only
>> sender B observing massive drops at the netem.
>>
>> With the queue removed the rate reverts to
>>
>> zerocopy=58878 copy=110239
>> zerocopy=58833 copy=110207
>>
>> This is not a 50/50 split, which impliesTw that some packets from the
>> large
>> packet flow are still converted to copying. Without the change the rate
>> without queue was 80k zerocopy vs 80k copy, so this choice of
>> (vq->num >> 2) appears too conservative.
>>
>> However, testing with (vq->num >> 1) was not as effective at mitigating
>> stalls. I did not save that data, unfortunately. Can run more tests on
>> fine
>> tuning this variable, if the idea sounds good.
>
>
> Looks like there're still two cases were left:
To be clear, this patch is not intended to fix all issues. It is a small
improvement to avoid HoL blocking due to queued zerocopy skbs.
The trade-off is that reverting to copying in these cases increases
cycle cost. I think that that is a trade-off worth making compared to
the alternative drop in throughput. It probably would be good to be
able to measure this without kernel instrumentation: export
counters similar to net->tx_zcopy_err and net->tx_packets (though
without reset to zero, as in vhost_net_tx_packet).
> 1) sndbuf is not INT_MAX
You mean the case where the device stalls, later zerocopy notifications
are queued, but these are never cleaned in free_old_xmit_skbs,
because it requires a start_xmit and by now the (only) socket is out of
descriptors?
A watchdog would help somewhat. With tx-napi, this case cannot occur,
either, as free_old_xmit_skbs no longer depends on a call to start_xmit.
> 2) tx napi is used for virtio-net
I am not aware of any issue specific to the use of tx-napi?
> 1) could be a corner case, and for 2) what your suggest here may not solve
> the issue since it still do in order completion.
Somewhat tangential, but it might also help to break the in-order
completion processing in vhost_zerocopy_signal_used. Complete
all descriptors between done_idx and upend_idx. done_idx should
then only be forward to the oldest still not-completed descriptor.
In the test I ran, where the oldest descriptors are held in a queue and
all newer ones are tail-dropped, this would avoid blocking a full ring
of completions, when only a small number (or 1) is actually delayed.
Dynamic switching between copy and zerocopy using zcopy_used
already returns completions out-of-order, so this is not a huge leap.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-09-01 16:17 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <CAF=yD-JREJqpsRdCi31qo1QNFEE2DDCaTGYw1f1waPgtov8WEg@mail.gmail.com>
>>> This is not a 50/50 split, which impliesTw that some packets from the
>>> large
>>> packet flow are still converted to copying. Without the change the rate
>>> without queue was 80k zerocopy vs 80k copy, so this choice of
>>> (vq->num >> 2) appears too conservative.
>>>
>>> However, testing with (vq->num >> 1) was not as effective at mitigating
>>> stalls. I did not save that data, unfortunately. Can run more tests on
>>> fine
>>> tuning this variable, if the idea sounds good.
>>
>>
>> Looks like there're still two cases were left:
>
> To be clear, this patch is not intended to fix all issues. It is a small
> improvement to avoid HoL blocking due to queued zerocopy skbs.
>
> The trade-off is that reverting to copying in these cases increases
> cycle cost. I think that that is a trade-off worth making compared to
> the alternative drop in throughput. It probably would be good to be
> able to measure this without kernel instrumentation: export
> counters similar to net->tx_zcopy_err and net->tx_packets (though
> without reset to zero, as in vhost_net_tx_packet).
>
>> 1) sndbuf is not INT_MAX
>
> You mean the case where the device stalls, later zerocopy notifications
> are queued, but these are never cleaned in free_old_xmit_skbs,
> because it requires a start_xmit and by now the (only) socket is out of
> descriptors?
Typo, sorry. I meant out of sndbuf.
> A watchdog would help somewhat. With tx-napi, this case cannot occur,
> either, as free_old_xmit_skbs no longer depends on a call to start_xmit.
>
>> 2) tx napi is used for virtio-net
>
> I am not aware of any issue specific to the use of tx-napi?
>
>> 1) could be a corner case, and for 2) what your suggest here may not solve
>> the issue since it still do in order completion.
>
> Somewhat tangential, but it might also help to break the in-order
> completion processing in vhost_zerocopy_signal_used. Complete
> all descriptors between done_idx and upend_idx. done_idx should
> then only be forward to the oldest still not-completed descriptor.
>
> In the test I ran, where the oldest descriptors are held in a queue and
> all newer ones are tail-dropped, this would avoid blocking a full ring
> of completions, when only a small number (or 1) is actually delayed.
>
> Dynamic switching between copy and zerocopy using zcopy_used
> already returns completions out-of-order, so this is not a huge leap.
^ permalink raw reply
* Re: virtio_net: ethtool supported link modes
From: Radu Rendec via Virtualization @ 2017-09-01 16:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, virtio-dev, linux-kernel, virtualization
In-Reply-To: <20170901183517-mutt-send-email-mst@kernel.org>
On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > and duplex can be set to any valid value. The driver will "remember"
> > them and report them back in virtnet_get_link_ksettings.
> >
> > However, the supported link modes (link_modes.supported in struct
> > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > setting is supported.
> >
> > Does it make more sense to set (at least a few of) the supported link
> > modes, such as 10baseT_Half ... 10000baseT_Full?
> >
> > I would expect to see consistency between what is reported in
> > link_modes.supported and what can actually be set. Could you please
> > share your opinion on this?
>
> I would like to know more about why this is desirable.
>
> We used not to support the modes at all, but it turned out
> some tools are confused by this: e.g. people would try to
> bond virtio with a hardware device, tools would see
> a mismatch in speed and features between bonded devices
> and get confused.
>
> See
>
> commit 16032be56c1f66770da15cb94f0eb366c37aff6e
> Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Date: Wed Feb 3 04:04:37 2016 +0100
>
> virtio_net: add ethtool support for set and get of settings
>
>
> as well as the discussion around it
> https://www.spinics.net/lists/netdev/msg362111.html
Thanks for pointing these out. It is much more clear now why modes
support is implemented the way it is and what the expectations are.
> If you think we need to add more hacks like this, a stronger
> motivation than "to see consistency" would be needed.
The use case behind my original question is very simple:
* Net device is queried via ethtool for supported modes.
* Supported modes are presented to user.
* User can configure any of the supported modes.
This is done transparently to the net device type (driver), so it
actually makes sense for physical NICs.
This alone of course is not a good enough motivation to modify the
driver. And it can be easily addressed in user-space at the application
level by testing for the driver.
I was merely trying to avoid driver-specific workarounds (i.e. keep the
application driver agnostic) and wondered if "advertising" supported
modes through ethtool made any sense and/or would be a desirable change
from the driver perspective. I believe I have my answers now.
Thanks,
Radu
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: virtio_net: ethtool supported link modes
From: Michael S. Tsirkin @ 2017-09-01 17:45 UTC (permalink / raw)
To: Radu Rendec; +Cc: netdev, virtio-dev, linux-kernel, virtualization
In-Reply-To: <1504282793.12952.17.camel@arista.com>
On Fri, Sep 01, 2017 at 05:19:53PM +0100, Radu Rendec wrote:
> On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> > > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > > and duplex can be set to any valid value. The driver will "remember"
> > > them and report them back in virtnet_get_link_ksettings.
> > >
> > > However, the supported link modes (link_modes.supported in struct
> > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > > setting is supported.
> > >
> > > Does it make more sense to set (at least a few of) the supported link
> > > modes, such as 10baseT_Half ... 10000baseT_Full?
> > >
> > > I would expect to see consistency between what is reported in
> > > link_modes.supported and what can actually be set. Could you please
> > > share your opinion on this?
> >
> > I would like to know more about why this is desirable.
> >
> > We used not to support the modes at all, but it turned out
> > some tools are confused by this: e.g. people would try to
> > bond virtio with a hardware device, tools would see
> > a mismatch in speed and features between bonded devices
> > and get confused.
> >
> > See
> >
> > commit 16032be56c1f66770da15cb94f0eb366c37aff6e
> > Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> > Date: Wed Feb 3 04:04:37 2016 +0100
> >
> > virtio_net: add ethtool support for set and get of settings
> >
> >
> > as well as the discussion around it
> > https://www.spinics.net/lists/netdev/msg362111.html
>
> Thanks for pointing these out. It is much more clear now why modes
> support is implemented the way it is and what the expectations are.
>
> > If you think we need to add more hacks like this, a stronger
> > motivation than "to see consistency" would be needed.
>
> The use case behind my original question is very simple:
> * Net device is queried via ethtool for supported modes.
> * Supported modes are presented to user.
> * User can configure any of the supported modes.
Since this has no effect on virtio, isn't presenting
"no supported modes" to user the right thing to do?
> This is done transparently to the net device type (driver), so it
> actually makes sense for physical NICs.
>
> This alone of course is not a good enough motivation to modify the
> driver. And it can be easily addressed in user-space at the application
> level by testing for the driver.
I think you might want to special-case no supported modes.
Special-casing virtio is probably best avoided.
> I was merely trying to avoid driver-specific workarounds (i.e. keep the
> application driver agnostic)
I think that's the right approach. So if driver does not present
any supported modes this probably means it is not necessary
to display or program any.
> and wondered if "advertising" supported
> modes through ethtool made any sense and/or would be a desirable change
> from the driver perspective. I believe I have my answers now.
>
> Thanks,
> Radu
^ permalink raw reply
* Call for Papers --- ICITS'18 --- Ecuador,,, Deadline: September 12
From: Marle @ 2017-09-02 18:23 UTC (permalink / raw)
To: virtualization
[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]
*** Proceedings published by Springer and indexed by ISI, Scopus, EI-Compendex, DBLP, etc.
---------------------------------------------------------------------------------------------------
ICITS'18 - The 2018 International Conference on Information Technology & Systems
Peninsula de Santa Elena, Ecuador, 10 - 12 January 2018
http://www.icits.me/
-------------------------------------------------------------------------------------------------
SCOPE
ICITS'18 - The 2018 International Conference on Information Technology & Systems (http://www.icits.me/), to be held at Península de Santa Elena, Ecuador, 10 - 12 January 2018, is an international forum for researchers and practitioners to present and discuss the most recent innovations, trends, results, experiences and concerns in the several perspectives of Information Technology & Systems.
We are pleased to invite you to submit your papers to ICITS'18. They can be written in English, Spanish or Portuguese. All submissions will be reviewed on the basis of relevance, originality, importance and clarity.
TOPICS
Submitted papers should be related with one or more of the main themes proposed for the Conference:
A) Information and Knowledge Management (IKM);
B) Organizational Models and Information Systems (OMIS);
C) Software and Systems Modeling (SSM);
D) Software Systems, Architectures, Applications and Tools (SSAAT);
E) Multimedia Systems and Applications (MSA);
F) Computer Networks, Mobility and Pervasive Systems (CNMPS);
G) Intelligent and Decision Support Systems (IDSS);
H) Big Data Analytics and Applications (BDAA);
I) Human-Computer Interaction (HCI);
J) Ethics, Computers and Security (ECS)
K) Health Informatics (HIS);
L) Information Technologies in Education (ITE);
SUBMISSION AND DECISION
Submitted papers (until 10-page limit) must comply with the format of Advances in Intelligent Systems and Computing series (see Instructions for Authors at Springer Website or download a DOC example) be written in English, Spanish or Portuguese, must not have been published before, not be under review for any other conference or publication and not include any information leading to the authors’ identification. Therefore, the authors’ names, affiliations and bibliographic references should not be included in the version for evaluation by the Scientific Committee. This information should only be included in the camera-ready version, saved in Word or Latex format and also in PDF format. These files must be accompanied by the Consent to Publication form filled out, in a ZIP file, and uploaded at the conference management system.
All papers will be subjected to a “double-blind review” by at least two members of the Scientific Committee.
Based on Scientific Committee evaluation, a paper can be rejected or accepted by the Conference Chairs. In the later case, it can be accepted as paper or poster.
The authors of papers accepted as posters must build and print a poster to be exhibited during the Conference. This poster must follow an A1 or A2 vertical format. The Conference can includes Work Sessions where these posters are presented and orally discussed, with a 7 minute limit per poster.
The authors of accepted papers will have 15 minutes to present their work in a Conference Work Session; approximately 5 minutes of discussion will follow each presentation.
PUBLICATION AND INDEXING
To ensure that an accepted paper is published, at least one of the authors must be fully registered by the 20th of October 2017, and the paper must comply with the suggested layout and page-limit (until 10 pages). Additionally, all recommended changes must be addressed by the authors before they submit the camera-ready version.
No more than one paper per registration will be published. An extra fee must be paid for publication of additional papers, with a maximum of one additional paper per registration. One registration permits only the participation of one author in the conference.
Papers written in English and accepted and registered will be published in Proceedings by Springer, in a book of the Advances in Intelligent Systems and Computing series, will be submitted for indexation by ISI, EI-Compendex, SCOPUS and DBLP, among others, and will be available in the SpringerLink Digital Library.
Papers written in Spanish or Portuguese and accepted and registered will be published in Proceedings by AISTI and will be submitted for evaluation and possible indexation by ISI, EI-Compendex and SCOPUS.
The authors of the best selected papers will be invited to extend them for publication in international journals indexed by ISI, SCOPUS, EI-Compendex and DBLP, among others.
IMPORTANT DATES
Paper Submission: September 12, 2017
Notification of Acceptance: October 13, 2017
Payment of Registration, to ensure the inclusion of an accepted paper in the conference proceedings: October 20, 2017.
Camera-ready Submission: October 20, 2017
---------
ICITS'18 website: http://www.icits.me/
---------
----------------------------------------------
---
This email has been checked for viruses by AVG.
http://www.avg.com
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] vhost_net: correctly check tx avail during rx busy polling
From: Jason Wang @ 2017-09-04 2:51 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170901184531-mutt-send-email-mst@kernel.org>
On 2017年09月01日 23:51, Michael S. Tsirkin wrote:
> On Fri, Sep 01, 2017 at 05:02:50PM +0800, Jason Wang wrote:
>> We check tx avail through vhost_enable_notify() in the past which is
>> wrong since it only checks whether or not guest has filled more
>> available buffer since last avail idx synchronization which was just
>> done by vhost_vq_avail_empty() before. What we really want is checking
>> pending buffers in the avail ring.
> These are rx buffers, right? I'm not even sure why do we need to poll
> for them. Running out of rx buffers is a slow path.
Actually it polls for tx buffer here. I admit the code (or probably the
variable name) is confusing here.
>
>> Fix this by calling
>> vhost_vq_avail_empty() instead.
>>
>> This issue could be noticed by doing netperf TCP_RR benchmark as
>> client from guest (but not host). With this fix, TCP_RR from guest to
>> localhost restores from 1375.91 trans per sec to 55235.28 trans per
>> sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
>>
>> Fixes: 030881372460 ("vhost_net: basic polling support")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> - The patch is needed for -stable
>> ---
>> drivers/vhost/net.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 06d0448..1b68253 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -634,7 +634,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
> In fact why does it poll the ring at all? I thought this function's
> job is to poll the socket, isn't it?
Tx notification is disabled to try to avoid vmexits, so we poll tx avail
buffers too.
>
>
>>
>> preempt_enable();
>>
>> - if (vhost_enable_notify(&net->dev, vq))
>> + if (!vhost_vq_avail_empty(&net->dev, vq))
>> vhost_poll_queue(&vq->poll);
>> mutex_unlock(&vq->mutex);
>
> Adding more contex:
>
> mutex_lock(&vq->mutex);
> vhost_disable_notify(&net->dev, vq);
>
> preempt_disable();
> endtime = busy_clock() + vq->busyloop_timeout;
>
> while (vhost_can_busy_poll(&net->dev, endtime) &&
> !sk_has_rx_data(sk) &&
> vhost_vq_avail_empty(&net->dev, vq))
> cpu_relax();
>
> preempt_enable();
>
> if (vhost_enable_notify(&net->dev, vq))
> vhost_poll_queue(&vq->poll);
> mutex_unlock(&vq->mutex);
>
> len = peek_head_len(rvq, sk);
>
>
> If you drop this we'll exit the function with notifications
> disabled. Seems wrong to me.
Yes, will fix this in V2.
Thanks
>
>>
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-09-04 3:03 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <CAF=yD-LZ4Abn8CFSQyuNnYSRZkHx4FTzK4hSi9WQaHO-28rsHQ@mail.gmail.com>
On 2017年09月02日 00:17, Willem de Bruijn wrote:
>>>> This is not a 50/50 split, which impliesTw that some packets from the
>>>> large
>>>> packet flow are still converted to copying. Without the change the rate
>>>> without queue was 80k zerocopy vs 80k copy, so this choice of
>>>> (vq->num >> 2) appears too conservative.
>>>>
>>>> However, testing with (vq->num >> 1) was not as effective at mitigating
>>>> stalls. I did not save that data, unfortunately. Can run more tests on
>>>> fine
>>>> tuning this variable, if the idea sounds good.
>>>
>>> Looks like there're still two cases were left:
>> To be clear, this patch is not intended to fix all issues. It is a small
>> improvement to avoid HoL blocking due to queued zerocopy skbs.
Right, just want to see if there's anything left.
>>
>> The trade-off is that reverting to copying in these cases increases
>> cycle cost. I think that that is a trade-off worth making compared to
>> the alternative drop in throughput. It probably would be good to be
>> able to measure this without kernel instrumentation: export
>> counters similar to net->tx_zcopy_err and net->tx_packets (though
>> without reset to zero, as in vhost_net_tx_packet).
I think it's acceptable if extra cycles were spent if we detect HOL anyhow.
>>
>>> 1) sndbuf is not INT_MAX
>> You mean the case where the device stalls, later zerocopy notifications
>> are queued, but these are never cleaned in free_old_xmit_skbs,
>> because it requires a start_xmit and by now the (only) socket is out of
>> descriptors?
> Typo, sorry. I meant out of sndbuf.
I mean e.g for tun. If its sndbuf is smaller than e.g (vq->num >> 1) *
$pkt_size and if all packet were held by some modules, limitation like
vq->num >> 1 won't work since we hit sudbuf before it.
>
>> A watchdog would help somewhat. With tx-napi, this case cannot occur,
>> either, as free_old_xmit_skbs no longer depends on a call to start_xmit.
>>
>>> 2) tx napi is used for virtio-net
>> I am not aware of any issue specific to the use of tx-napi?
Might not be clear here, I mean e.g virtio_net (tx-napi) in guest +
vhost_net (zerocopy) in host. In this case, even if we switch to
datacopy if ubuf counts exceeds vq->num >> 1, we still complete tx
buffers in order, tx interrupt could be delayed for indefinite time.
>>
>>> 1) could be a corner case, and for 2) what your suggest here may not solve
>>> the issue since it still do in order completion.
>> Somewhat tangential, but it might also help to break the in-order
>> completion processing in vhost_zerocopy_signal_used. Complete
>> all descriptors between done_idx and upend_idx. done_idx should
>> then only be forward to the oldest still not-completed descriptor.
>>
>> In the test I ran, where the oldest descriptors are held in a queue and
>> all newer ones are tail-dropped,
Do you mean the descriptors were tail-dropped by vhost?
>> this would avoid blocking a full ring
>> of completions, when only a small number (or 1) is actually delayed.
>>
>> Dynamic switching between copy and zerocopy using zcopy_used
>> already returns completions out-of-order, so this is not a huge leap.
Yes.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] x86/paravirt: remove no longer used paravirt functions
From: Juergen Gross @ 2017-09-04 10:25 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
With removal of lguest some of the paravirt functions are no longer
needed. Remove them.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/desc.h | 3 +--
arch/x86/include/asm/paravirt.h | 37 -----------------------------------
arch/x86/include/asm/paravirt_types.h | 9 ---------
arch/x86/include/asm/pgtable.h | 27 ++++---------------------
arch/x86/include/asm/special_insns.h | 10 +++++-----
arch/x86/kernel/paravirt.c | 5 -----
arch/x86/kvm/vmx.c | 2 +-
arch/x86/mm/pgtable.c | 7 +------
arch/x86/xen/enlighten_pv.c | 2 --
arch/x86/xen/mmu_pv.c | 2 --
10 files changed, 12 insertions(+), 92 deletions(-)
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index d0a21b12dd58..f6630e73843c 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -128,7 +128,6 @@ static inline int desc_empty(const void *ptr)
#define load_ldt(ldt) asm volatile("lldt %0"::"m" (ldt))
#define store_gdt(dtr) native_store_gdt(dtr)
-#define store_idt(dtr) native_store_idt(dtr)
#define store_tr(tr) (tr = native_store_tr())
#define load_TLS(t, cpu) native_load_tls(t, cpu)
@@ -248,7 +247,7 @@ static inline void native_store_gdt(struct desc_ptr *dtr)
asm volatile("sgdt %0":"=m" (*dtr));
}
-static inline void native_store_idt(struct desc_ptr *dtr)
+static inline void store_idt(struct desc_ptr *dtr)
{
asm volatile("sidt %0":"=m" (*dtr));
}
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 9ccac1926587..c5e3e4d6ac16 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -71,11 +71,6 @@ static inline void write_cr3(unsigned long x)
PVOP_VCALL1(pv_mmu_ops.write_cr3, x);
}
-static inline unsigned long __read_cr4(void)
-{
- return PVOP_CALL0(unsigned long, pv_cpu_ops.read_cr4);
-}
-
static inline void __write_cr4(unsigned long x)
{
PVOP_VCALL1(pv_cpu_ops.write_cr4, x);
@@ -228,10 +223,6 @@ static inline void set_ldt(const void *addr, unsigned entries)
{
PVOP_VCALL2(pv_cpu_ops.set_ldt, addr, entries);
}
-static inline void store_idt(struct desc_ptr *dtr)
-{
- PVOP_VCALL1(pv_cpu_ops.store_idt, dtr);
-}
static inline unsigned long paravirt_store_tr(void)
{
return PVOP_CALL0(unsigned long, pv_cpu_ops.store_tr);
@@ -365,12 +356,6 @@ static inline void paravirt_release_p4d(unsigned long pfn)
PVOP_VCALL1(pv_mmu_ops.release_p4d, pfn);
}
-static inline void pte_update(struct mm_struct *mm, unsigned long addr,
- pte_t *ptep)
-{
- PVOP_VCALL3(pv_mmu_ops.pte_update, mm, addr, ptep);
-}
-
static inline pte_t __pte(pteval_t val)
{
pteval_t ret;
@@ -472,28 +457,6 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
PVOP_VCALL4(pv_mmu_ops.set_pte_at, mm, addr, ptep, pte.pte);
}
-static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
- pmd_t *pmdp, pmd_t pmd)
-{
- if (sizeof(pmdval_t) > sizeof(long))
- /* 5 arg words */
- pv_mmu_ops.set_pmd_at(mm, addr, pmdp, pmd);
- else
- PVOP_VCALL4(pv_mmu_ops.set_pmd_at, mm, addr, pmdp,
- native_pmd_val(pmd));
-}
-
-static inline void set_pud_at(struct mm_struct *mm, unsigned long addr,
- pud_t *pudp, pud_t pud)
-{
- if (sizeof(pudval_t) > sizeof(long))
- /* 5 arg words */
- pv_mmu_ops.set_pud_at(mm, addr, pudp, pud);
- else
- PVOP_VCALL4(pv_mmu_ops.set_pud_at, mm, addr, pudp,
- native_pud_val(pud));
-}
-
static inline void set_pmd(pmd_t *pmdp, pmd_t pmd)
{
pmdval_t val = native_pmd_val(pmd);
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 9ffc36bfe4cd..680624a13c69 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -107,7 +107,6 @@ struct pv_cpu_ops {
unsigned long (*read_cr0)(void);
void (*write_cr0)(unsigned long);
- unsigned long (*read_cr4)(void);
void (*write_cr4)(unsigned long);
#ifdef CONFIG_X86_64
@@ -119,8 +118,6 @@ struct pv_cpu_ops {
void (*load_tr_desc)(void);
void (*load_gdt)(const struct desc_ptr *);
void (*load_idt)(const struct desc_ptr *);
- /* store_gdt has been removed. */
- void (*store_idt)(struct desc_ptr *);
void (*set_ldt)(const void *desc, unsigned entries);
unsigned long (*store_tr)(void);
void (*load_tls)(struct thread_struct *t, unsigned int cpu);
@@ -248,12 +245,6 @@ struct pv_mmu_ops {
void (*set_pte_at)(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pteval);
void (*set_pmd)(pmd_t *pmdp, pmd_t pmdval);
- void (*set_pmd_at)(struct mm_struct *mm, unsigned long addr,
- pmd_t *pmdp, pmd_t pmdval);
- void (*set_pud_at)(struct mm_struct *mm, unsigned long addr,
- pud_t *pudp, pud_t pudval);
- void (*pte_update)(struct mm_struct *mm, unsigned long addr,
- pte_t *ptep);
pte_t (*ptep_modify_prot_start)(struct mm_struct *mm, unsigned long addr,
pte_t *ptep);
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 77037b6f1caa..bb8e9ea7deb4 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -43,8 +43,6 @@ extern struct mm_struct *pgd_page_get_mm(struct page *page);
#else /* !CONFIG_PARAVIRT */
#define set_pte(ptep, pte) native_set_pte(ptep, pte)
#define set_pte_at(mm, addr, ptep, pte) native_set_pte_at(mm, addr, ptep, pte)
-#define set_pmd_at(mm, addr, pmdp, pmd) native_set_pmd_at(mm, addr, pmdp, pmd)
-#define set_pud_at(mm, addr, pudp, pud) native_set_pud_at(mm, addr, pudp, pud)
#define set_pte_atomic(ptep, pte) \
native_set_pte_atomic(ptep, pte)
@@ -75,8 +73,6 @@ extern struct mm_struct *pgd_page_get_mm(struct page *page);
#define pte_clear(mm, addr, ptep) native_pte_clear(mm, addr, ptep)
#define pmd_clear(pmd) native_pmd_clear(pmd)
-#define pte_update(mm, addr, ptep) do { } while (0)
-
#define pgd_val(x) native_pgd_val(x)
#define __pgd(x) native_make_pgd(x)
@@ -965,31 +961,18 @@ static inline void native_set_pte_at(struct mm_struct *mm, unsigned long addr,
native_set_pte(ptep, pte);
}
-static inline void native_set_pmd_at(struct mm_struct *mm, unsigned long addr,
- pmd_t *pmdp , pmd_t pmd)
+static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
+ pmd_t *pmdp, pmd_t pmd)
{
native_set_pmd(pmdp, pmd);
}
-static inline void native_set_pud_at(struct mm_struct *mm, unsigned long addr,
- pud_t *pudp, pud_t pud)
+static inline void set_pud_at(struct mm_struct *mm, unsigned long addr,
+ pud_t *pudp, pud_t pud)
{
native_set_pud(pudp, pud);
}
-#ifndef CONFIG_PARAVIRT
-/*
- * Rules for using pte_update - it must be called after any PTE update which
- * has not been done using the set_pte / clear_pte interfaces. It is used by
- * shadow mode hypervisors to resynchronize the shadow page tables. Kernel PTE
- * updates should either be sets, clears, or set_pte_atomic for P->P
- * transitions, which means this hook should only be called for user PTEs.
- * This hook implies a P->P protection or access change has taken place, which
- * requires a subsequent TLB flush.
- */
-#define pte_update(mm, addr, ptep) do { } while (0)
-#endif
-
/*
* We only update the dirty/accessed state if we set
* the dirty bit by hand in the kernel, since the hardware
@@ -1017,7 +1000,6 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
pte_t *ptep)
{
pte_t pte = native_ptep_get_and_clear(ptep);
- pte_update(mm, addr, ptep);
return pte;
}
@@ -1044,7 +1026,6 @@ static inline void ptep_set_wrprotect(struct mm_struct *mm,
unsigned long addr, pte_t *ptep)
{
clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
- pte_update(mm, addr, ptep);
}
#define flush_tlb_fix_spurious_fault(vma, address) do { } while (0)
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 9efaabf5b54b..a24dfcf79f4a 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -135,6 +135,11 @@ static inline void native_wbinvd(void)
extern asmlinkage void native_load_gs_index(unsigned);
+static inline unsigned long __read_cr4(void)
+{
+ return native_read_cr4();
+}
+
#ifdef CONFIG_PARAVIRT
#include <asm/paravirt.h>
#else
@@ -173,11 +178,6 @@ static inline void write_cr3(unsigned long x)
native_write_cr3(x);
}
-static inline unsigned long __read_cr4(void)
-{
- return native_read_cr4();
-}
-
static inline void __write_cr4(unsigned long x)
{
native_write_cr4(x);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index bc0a849589bb..0b338ec7e9d2 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -330,7 +330,6 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
.set_debugreg = native_set_debugreg,
.read_cr0 = native_read_cr0,
.write_cr0 = native_write_cr0,
- .read_cr4 = native_read_cr4,
.write_cr4 = native_write_cr4,
#ifdef CONFIG_X86_64
.read_cr8 = native_read_cr8,
@@ -346,7 +345,6 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
.set_ldt = native_set_ldt,
.load_gdt = native_load_gdt,
.load_idt = native_load_idt,
- .store_idt = native_store_idt,
.store_tr = native_store_tr,
.load_tls = native_load_tls,
#ifdef CONFIG_X86_64
@@ -414,8 +412,6 @@ struct pv_mmu_ops pv_mmu_ops __ro_after_init = {
.set_pte = native_set_pte,
.set_pte_at = native_set_pte_at,
.set_pmd = native_set_pmd,
- .set_pmd_at = native_set_pmd_at,
- .pte_update = paravirt_nop,
.ptep_modify_prot_start = __ptep_modify_prot_start,
.ptep_modify_prot_commit = __ptep_modify_prot_commit,
@@ -427,7 +423,6 @@ struct pv_mmu_ops pv_mmu_ops __ro_after_init = {
.pmd_clear = native_pmd_clear,
#endif
.set_pud = native_set_pud,
- .set_pud_at = native_set_pud_at,
.pmd_val = PTE_IDENT,
.make_pmd = PTE_IDENT,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6ef2940119b..d7762e779952 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5163,7 +5163,7 @@ static void vmx_set_constant_host_state(struct vcpu_vmx *vmx)
vmcs_write16(HOST_SS_SELECTOR, __KERNEL_DS); /* 22.2.4 */
vmcs_write16(HOST_TR_SELECTOR, GDT_ENTRY_TSS*8); /* 22.2.4 */
- native_store_idt(&dt);
+ store_idt(&dt);
vmcs_writel(HOST_IDTR_BASE, dt.address); /* 22.2.4 */
vmx->host_idt_base = dt.address;
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 508a708eb9a6..942391b5b639 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -426,10 +426,8 @@ int ptep_set_access_flags(struct vm_area_struct *vma,
{
int changed = !pte_same(*ptep, entry);
- if (changed && dirty) {
+ if (changed && dirty)
*ptep = entry;
- pte_update(vma->vm_mm, address, ptep);
- }
return changed;
}
@@ -486,9 +484,6 @@ int ptep_test_and_clear_young(struct vm_area_struct *vma,
ret = test_and_clear_bit(_PAGE_BIT_ACCESSED,
(unsigned long *) &ptep->pte);
- if (ret)
- pte_update(vma->vm_mm, addr, ptep);
-
return ret;
}
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 811e4ddb3f37..a204a95200d3 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1056,7 +1056,6 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
.read_cr0 = xen_read_cr0,
.write_cr0 = xen_write_cr0,
- .read_cr4 = native_read_cr4,
.write_cr4 = xen_write_cr4,
#ifdef CONFIG_X86_64
@@ -1091,7 +1090,6 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
.alloc_ldt = xen_alloc_ldt,
.free_ldt = xen_free_ldt,
- .store_idt = native_store_idt,
.store_tr = xen_store_tr,
.write_ldt_entry = xen_write_ldt_entry,
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index cab28cf2cffb..5f61b7e2e6b2 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2430,8 +2430,6 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
.flush_tlb_single = xen_flush_tlb_single,
.flush_tlb_others = xen_flush_tlb_others,
- .pte_update = paravirt_nop,
-
.pgd_alloc = xen_pgd_alloc,
.pgd_free = xen_pgd_free,
--
2.12.3
^ permalink raw reply related
* Re: virtio_net: ethtool supported link modes
From: Radu Rendec via Virtualization @ 2017-09-04 14:59 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, virtio-dev, linux-kernel, virtualization
In-Reply-To: <20170901204222-mutt-send-email-mst@kernel.org>
On Fri, 2017-09-01 at 20:45 +0300, Michael S. Tsirkin wrote:
> On Fri, Sep 01, 2017 at 05:19:53PM +0100, Radu Rendec wrote:
> > On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> > > > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > > > and duplex can be set to any valid value. The driver will "remember"
> > > > them and report them back in virtnet_get_link_ksettings.
> > > >
> > > > However, the supported link modes (link_modes.supported in struct
> > > > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > > > setting is supported.
> > > >
> > > > Does it make more sense to set (at least a few of) the supported link
> > > > modes, such as 10baseT_Half ... 10000baseT_Full?
> > > >
> > > > I would expect to see consistency between what is reported in
> > > > link_modes.supported and what can actually be set. Could you please
> > > > share your opinion on this?
> >
> > The use case behind my original question is very simple:
> > * Net device is queried via ethtool for supported modes.
> > * Supported modes are presented to user.
> > * User can configure any of the supported modes.
>
> Since this has no effect on virtio, isn't presenting
> "no supported modes" to user the right thing to do?
Yes, that makes sense.
> > This is done transparently to the net device type (driver), so it
> > actually makes sense for physical NICs.
> >
> > This alone of course is not a good enough motivation to modify the
> > driver. And it can be easily addressed in user-space at the application
> > level by testing for the driver.
>
> I think you might want to special-case no supported modes.
> Special-casing virtio is probably best avoided.
>
> > I was merely trying to avoid driver-specific workarounds (i.e. keep the
> > application driver agnostic)
>
> I think that's the right approach. So if driver does not present
> any supported modes this probably means it is not necessary
> to display or program any.
Yes, apparently it boils down to special-casing no supported modes.
This avoids both modifying virtio and special-casing virtio, and keeps
the application driver-agnostic at the same time.
Thanks for all the feedback. It was very helpful in figuring out the
right approach. I really appreciate it.
Radu
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH net V2] vhost_net: correctly check tx avail during rx busy polling
From: Jason Wang @ 2017-09-05 1:22 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: linux-kernel, kvm, virtualization
We check tx avail through vhost_enable_notify() in the past which is
wrong since it only checks whether or not guest has filled more
available buffer since last avail idx synchronization which was just
done by vhost_vq_avail_empty() before. What we really want is checking
pending buffers in the avail ring. Fix this by calling
vhost_vq_avail_empty() instead.
This issue could be noticed by doing netperf TCP_RR benchmark as
client from guest (but not host). With this fix, TCP_RR from guest to
localhost restores from 1375.91 trans per sec to 55235.28 trans per
sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
Fixes: 030881372460 ("vhost_net: basic polling support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
- The patch is needed for -stable
- Changes from V1: enable vq notification when needed
---
drivers/vhost/net.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 06d0448..1c75572 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -634,8 +634,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
preempt_enable();
- if (vhost_enable_notify(&net->dev, vq))
+ if (!vhost_vq_avail_empty(&net->dev, vq))
vhost_poll_queue(&vq->poll);
+ else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+ vhost_disable_notify(&net->dev, vq);
+ vhost_poll_queue(&vq->poll);
+ }
+
mutex_unlock(&vq->mutex);
len = peek_head_len(rvq, sk);
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox