* Re: [PATCH] net/virtio-net: reverse unregistering on exit
From: Michael S. Tsirkin @ 2017-08-22 18:13 UTC (permalink / raw)
To: Pierre Morel; +Cc: virtualization
In-Reply-To: <1503413992-29558-2-git-send-email-pmorel@linux.vnet.ibm.com>
On Tue, Aug 22, 2017 at 04:59:52PM +0200, Pierre Morel wrote:
> unregister_virtio_driver should be done before the unregistering of
> the hotplug state machine callbacks, otherwise the state machine still
> holds some instance states at that time.
>
> Let's first unregister the virtio_net_driver first and then the hotplug
> state machine callbacks.
>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
I'm having a deja vu.
Wasn't this fixed by
commit cfa0ebc9d6d6308564f5174ecb655b9d504b2be5
Author: Andrew Jones <drjones@redhat.com>
Date: Mon Jul 24 15:38:32 2017 +0200
virtio-net: fix module unloading
already?
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 143d8a9..c042ffd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2734,9 +2734,9 @@ module_init(virtio_net_driver_init);
>
> static __exit void virtio_net_driver_exit(void)
> {
> + unregister_virtio_driver(&virtio_net_driver);
> cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> cpuhp_remove_multi_state(virtionet_online);
> - unregister_virtio_driver(&virtio_net_driver);
> }
> module_exit(virtio_net_driver_exit);
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: David Miller @ 2017-08-22 18:01 UTC (permalink / raw)
To: mst; +Cc: netdev, willemdebruijn.kernel, den, virtualization
In-Reply-To: <20170822204015-mutt-send-email-mst@kernel.org>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 22 Aug 2017 20:55:56 +0300
> Which reminds me that skb_linearize in net core seems to be
> fundamentally racy - I suspect that if skb is cloned, and someone is
> trying to use the shared frags while another thread calls skb_linearize,
> we get some use after free bugs which likely mostly go undetected
> because the corrupted packets mostly go on wire and get dropped
> by checksum code.
Indeed, it does assume that the skb from which the clone was made
never has it's geometry changed.
I don't think even the TCP retransmit queue has this guarantee.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Michael S. Tsirkin @ 2017-08-22 17:55 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, Willem de Bruijn, Koichiro Den,
virtualization
In-Reply-To: <64d451ae-9944-e978-5a05-54bb1a62aaad@redhat.com>
On Tue, Aug 22, 2017 at 10:50:41AM +0800, Jason Wang wrote:
> > Perhaps the descriptor pool should also be
> > revised to allow out of order completions. Then there is no need to
> > copy zerocopy packets whenever they may experience delay.
>
> Yes, but as replied in the referenced thread, windows driver may treat out
> of order completion as a bug.
That would be a windows driver bug then, but I don't think it makes this
assumption. What the referenced thread
(https://patchwork.kernel.org/patch/3787671/) is saying is that host
must use any buffers made available on a tx vq within a reasonable
timeframe otherwise windows guests panic.
Ideally we would detect that a packet is actually experiencing delay and
trigger the copy at that point e.g. by calling skb_linearize. But it
isn't easy to track these packets though and even harder to do a data
copy without races.
Which reminds me that skb_linearize in net core seems to be
fundamentally racy - I suspect that if skb is cloned, and someone is
trying to use the shared frags while another thread calls skb_linearize,
we get some use after free bugs which likely mostly go undetected
because the corrupted packets mostly go on wire and get dropped
by checksum code.
--
MST
^ 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-22 17:19 UTC (permalink / raw)
To: Koichiro Den; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <1503410668.8694.14.camel@klaipeden.com>
>> > > /* Don't wait up for transmitted skbs to be freed. */
>> > > if (!use_napi) {
>> > > + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
>> > > + struct ubuf_info *uarg;
>> > > + uarg = skb_shinfo(skb)->destructor_arg;
>> > > + if (uarg->callback)
>> > > + uarg->callback(uarg, true);
>> > > + skb_shinfo(skb)->destructor_arg = NULL;
>> > > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
>> > > + }
>> >
>> > Instead of open coding, this can use skb_zcopy_clear.
>>
>> It is not correct to only send the zerocopy completion here, as
>> the skb will still be shared with the nic. The only safe approach
>> to notifying early is to create a copy with skb_orphan_frags_rx.
>> That will call skb_zcopy_clear(skb, false) internally. But the
>> copy will be very expensive.
> I noticed this email after my last post. I cannot not imagine how it could be
> unsafe in virtio case. Sorry could you explain a bit more?
A guest process sends a packet with MSG_ZEROCOPY to the
virtio-net device. As soon as the process receives the completion
notification, it is allowed to reuse the memory backing the packet.
A call to skb_zcopy_clear in virtio-net start_xmit will notify the
process that it is allowed to reuse the memory. But the user pages
are still linked into the skb frags and are about to be shared with
the host os.
>> Is the deadlock you refer to the case where tx interrupts are
>> disabled, so transmit completions are only handled in start_xmit
>> and somehow the socket(s) are unable to send new data? The
>> question is what is blocking them. If it is zerocopy, it may be a
>> too low optmem_max or hitting the per-user locked pages limit.
>> We may have to keep interrupts enabled when zerocopy skbs
>> are in flight.
> Even if we keep interrupts enabled, We miss the completion without start_xmit.
> And it's also likely that the next start_xmit depends on the completion itself
> as I described in my last post.
>
^ 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-22 17:16 UTC (permalink / raw)
To: Koichiro Den; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <1503409339.8694.12.camel@klaipeden.com>
>> > An issue of the referenced patch is that sndbuf could be smaller than low
>> > watermark.
> We cannot determine the low watermark properly because of not only sndbuf size
> issue but also the fact that the upper vhost-net cannot directly see how much
> descriptor is currently available at the virtio-net tx queue. It depends on
> multiqueue settings or other senders which are also using the same tx queue.
> Note that in the latter case if they constantly transmitting, the deadlock could
> not occur(*), however if it has just temporarily fulfill some portion of the
> pool in the mean time, then the low watermark cannot be helpful.
> (*: That is because it's reliable enough in the sense I mention below.)
>
> Keep in this in mind, let me briefly describe the possible deadlock I mentioned:
> (1). vhost-net on L1 guest has nothing to do sendmsg until the upper layer sets
> new descriptors, which depends only on the vhost-net zcopy callback and adding
> newly used descriptors.
> (2). vhost-net callback depends on the skb freeing on the xmit path only.
> (3). the xmit path depends (possibly only) on the vhost-net sendmsg.
> As you see, it's enough to bring about the situation above that L1 virtio-net
> reaches its limit earlier than the L0 host processing. The vhost-net pool could
> be almost full or empty, whatever.
Thanks for the context. This issue is very similar to the one that used to
exist when running out of transmit descriptors, before the removal of
the timer and introduction of skb_orphan in start_xmit.
To make sure that I understand correctly, let me paraphrase:
A. guest socket cannot send because it exhausted its sk budget (sndbuf, tsq, ..)
B. budget is not freed up until guest receives tx completion for this flow
C. tx completion is held back on the host side in vhost_zerocopy_signal_used
behind the completion for an unrelated skb
D. unrelated packet is delayed somewhere in the host stackf zerocopy
completions.
e.g., netem
The issue that is specific to vhost-net zerocopy is that (C) enforces strict
ordering of transmit completions causing head of line blocking behind
vhost-net zerocopy callbacks.
This is a different problem from
C1. tx completion is delayed until guest sends another packet and
triggers free_old_xmit_skb
Both in host and guest, zerocopy packets should never be able to loop
to a receive path where they can cause unbounded delay.
The obvious cases of latency are queueing, like netem. That leads
to poor performance for unrelated flows, but I don't see how this
could cause deadlock.
^ permalink raw reply
* [PATCH] net/virtio-net: reverse unregistering on exit
From: Pierre Morel @ 2017-08-22 14:59 UTC (permalink / raw)
To: virtualization; +Cc: mst
In-Reply-To: <1503413992-29558-1-git-send-email-pmorel@linux.vnet.ibm.com>
unregister_virtio_driver should be done before the unregistering of
the hotplug state machine callbacks, otherwise the state machine still
holds some instance states at that time.
Let's first unregister the virtio_net_driver first and then the hotplug
state machine callbacks.
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 143d8a9..c042ffd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2734,9 +2734,9 @@ module_init(virtio_net_driver_init);
static __exit void virtio_net_driver_exit(void)
{
+ unregister_virtio_driver(&virtio_net_driver);
cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
cpuhp_remove_multi_state(virtionet_online);
- unregister_virtio_driver(&virtio_net_driver);
}
module_exit(virtio_net_driver_exit);
--
2.7.4
^ permalink raw reply related
* [PATCH] net/virtio-net: reverse unregistering on exit
From: Pierre Morel @ 2017-08-22 14:59 UTC (permalink / raw)
To: virtualization; +Cc: mst
Hi,
I got a problem with virtio-net in a QEMU guest.
When virtio-net is used as a module in a guest and the network is
activated, removing the virtio-net modules produces a kernel error:
[ 2375.624949] Last Breaking-Event-Address:
[ 2375.624954] [<00000000001453d4>] __cpuhp_remove_state+0x84/0x1a8
[ 2375.624959] ---[ end trace cc9fd68b89f5235a ]---
[ 2375.624966] Error: Removing state 163 which has instances left.
[ 2375.624985] ------------[ cut here ]------------
The problem is produced by an inversion in the order of unregistering
the driver and the hotplug state machine callbacks in virtio_net_driver_exit.
This patch solves the problem by first unregistering the virtio_net_driver
before unregistering the hotplug state machine callbacks.
Best regards,
Pierre Morel
Pierre Morel (1):
net/virtio-net: reverse unregistering on exit
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH] vhost: fix end of range for access_ok
From: Koichiro Den @ 2017-08-22 14:49 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: netdev, David Miller, kvm, virtualization
In-Reply-To: <1503344576-8141-1-git-send-email-mst@redhat.com>
On Mon, 2017-08-21 at 22:45 +0300, Michael S. Tsirkin wrote:
> During access_ok checks, addr increases as we iterate over the data
> structure, thus addr + len - 1 will point beyond the end of region we
> are translating. Harmless since we then verify that the region covers
> addr, but let's not waste cpu cycles.
>
> Reported-by: Koichiro Den <den@klaipeden.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Lightly tested, would appreciate an ack from reporter.
>
> drivers/vhost/vhost.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e4613a3..ecd70e4 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1176,7 +1176,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
> {
> const struct vhost_umem_node *node;
> struct vhost_umem *umem = vq->iotlb;
> - u64 s = 0, size, orig_addr = addr;
> + u64 s = 0, size, orig_addr = addr, last = addr + len - 1;
>
> if (vhost_vq_meta_fetch(vq, addr, len, type))
> return true;
> @@ -1184,7 +1184,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
> while (len > s) {
> node = vhost_umem_interval_tree_iter_first(&umem->umem_tree,
> addr,
> - addr + len - 1);
> + last);
> if (node == NULL || node->start > addr) {
> vhost_iotlb_miss(vq, addr, access);
> return false;
Michael, Thank you for this one.
Acked-by: Koichiro Den <den@klaipeden.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC 2/3] virtio-iommu: device probing and operations
From: Jean-Philippe Brucker @ 2017-08-22 14:19 UTC (permalink / raw)
To: Tian, Kevin, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: cdall@linaro.org, lorenzo.pieralisi@arm.com, mst@redhat.com,
marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com,
Auger Eric, robin.murphy@arm.com
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D190D7CF52@SHSMSX101.ccr.corp.intel.com>
On 22/08/17 07:24, Tian, Kevin wrote:
>>> (sorry to pick up this old thread, as the .tex one is not good for review
>>> and this thread provides necessary background for IOASID).
>>>
>>> Hi, Jean,
>>>
>>> I'd like to hear more clarification regarding the relationship between
>>> IOASID and PASID. When reading back above explanation, it looks
>>> confusing to me now (though I might get the meaning months ago :/).
>>> At least Intel VT-d only understands PASID (or you can think IOASID
>>> =PASID). There is no such layered address space concept. Then for
>>> map/unmap type request, do you intend to steal some PASIDs for
>>> that purpose on such architecture (since IOASID is a mandatory field
>>> in map/unmap request)?
>>
>> IOASID is a logical ID, it isn't used by hardware. The address space
>> concept in virtio-iommu allows to group endpoints together, so that they
>> have the same address space. I thought it was pretty much the same as
>> "domains" in VT-d? In any case, it is the same as domains in Linux. An
>> IOASID provides a handle for communication between virtio-iommu device
>> and
>> driver, but unlike PASID, the IOASID number doesn't mean anything outside
>> of virtio-iommu.
>
> Thanks. It's clear to me then.
>
> btw does it make more sense to use "domain id" instead of "IO address
> space id"? For one, when talking about layered address spaces
> usually parent address space is a superset of all child address spaces
> which doesn't apply to this case, since either anonymous address
> space or PASID-tagged address spaces are completely isolated. Instead> 'domain' is a more inclusive terminology to embrace multiple address
> spaces. For two, 'domain' is better aligned to software terminology (e.g.
> iommu_domain) is easier for people to catch up. :-)
I do agree that the naming isn't great. I didn't use "domain" for various
reasons (it also has a different meanings in ARM) but I keep regretting
it. As there is no virtio-iommu code upstream yet, it is still possible to
change this one.
I find that "address space" was a good fit for the baseline device, but
the name doesn't scale. When introducing PASIDs, the address space moves
one level down in the programming model. It is contexts, anonymous or
PASID-tagged, that should be called address spaces. I was considering
replacing it with "domain", "container", "partition"...
Even though I don't want to use too much Linux terminology (virtio isn't
just Linux), "domain" is better fitted, somewhat neutral, and gets the
point across. A domain has one or more input address spaces and a single
output address space.
When introducing nested translation to virtio-iommu (for the guest to have
virtual machines itself), there will be one or more intermediate address
spaces. Domains will be nested, with the terminology "parent domain" and
"child domain". I only briefly looked at a programming model for this but
I think we can nest virtio-iommus without much hassle.
If there is no objection the next version will use "domain" in place of
"address_space". The change is quite invasive at this point, but I believe
that it will makes things more clear down the road.
>> I haven't introduced PASIDs in public virtio-iommu documents yet, but the
>> way I intend it, PASID != IOASID. We will still have a logical address
>> space identified by IOASID, that can contain multiple contexts identified
>> by PASID. At the moment, after the ATTACH request, an address space
>> contains a single anonymous context (NO PASID) that can be managed with
>> MAP/UNMAP requests. With virtio-iommu v0.4, structures look like the
>> following. The NO PASID context is implicit.
>>
>> address space context
>> endpoint ----. .- mapping
>> endpoint ----+---- IOASID -------- NO PASID ----+- mapping
>> endpoint ----' '- mapping
>>
>> I'd like to add a flag to ATTACH that says "don't create a default
>> anonymous context, I'll handle contexts myself". Then a new "ADD_TABLE"
>> request to handle contexts. When creating a context, the guest decides if
>> it wants to manage it via MAP/UNMAP requests (and a new "context" field),
>> or instead manage mappings itself by allocating a page directory and use
>> INVALIDATE requests.
>>
>> address space context
>> endpoint ----. .- mapping
>> endpoint ----+---- IOASID ----+--- NO PASID ----+- mapping
>> endpoint ----' | '- mapping
>> +--- PASID 0 ---- pgd
>> | ...
>> '--- PASID N ---- pgd
>>
>> In this example the guest chose to still have an anonymous context that
>> uses MAP/UNMAP, along with a few PASID contexts with their own page
>> tables.
>>
>
> Above explanation is a good background. Is it useful to include it
> in current spec? Though SVM support is not planned now, adding
> such background could help build a full story for IOASID concept.
I think introducing this explanation when PASIDs are added to the spec is
good enough. Right now it would look like clutter.
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH 1/2] vhost: remove the possible fruitless search on iotlb prefetch
From: Koichiro Den @ 2017-08-22 14:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kvm, virtualization
In-Reply-To: <20170821223039-mutt-send-email-mst@kernel.org>
On Mon, 2017-08-21 at 22:45 +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 19, 2017 at 03:41:14PM +0900, Koichiro Den wrote:
> > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > ---
> > drivers/vhost/vhost.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index e4613a3c362d..93e909afc1c3 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1184,7 +1184,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
> > while (len > s) {
> > node = vhost_umem_interval_tree_iter_first(&umem-
> > >umem_tree,
> > addr,
> > - addr + len - 1);
> > + addr + len - s -
> > 1);
> > if (node == NULL || node->start > addr) {
> > vhost_iotlb_miss(vq, addr, access);
> > return false;
>
> This works but it probably makes sense to just refactor the code to make end
> of
> range a variable. I posted a patch like this, pls take a look.
>
> > --
> > 2.9.4
> >
Ok, thanks.
_______________________________________________
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: Koichiro Den @ 2017-08-22 14:04 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-LO0rP1A_hrxqx_1HYEv6qyk=Sc=D7iH7cPh2YXDm=cYQ@mail.gmail.com>
On Tue, 2017-08-22 at 08:11 -0400, Willem de Bruijn wrote:
> On Sun, Aug 20, 2017 at 4:49 PM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > On Sat, Aug 19, 2017 at 2:38 AM, Koichiro Den <den@klaipeden.com> wrote:
> > > Facing the possible unbounded delay relying on freeing on xmit path,
> > > we also better to invoke and clear the upper layer zerocopy callback
> > > beforehand to keep them from waiting for unbounded duration in vain.
> >
> > Good point.
> >
> > > For instance, this removes the possible deadlock in the case that the
> > > upper layer is a zerocopy-enabled vhost-net.
> > > This does not apply if napi_tx is enabled since it will be called in
> > > reasonale time.
> >
> > Indeed. Btw, I am gathering data to eventually make napi the default
> > mode. But that is taking some time.
> >
> > >
> > > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > > ---
> > > drivers/net/virtio_net.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 4302f313d9a7..f7deaa5b7b50 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > > struct net_device *dev)
> > >
> > > /* Don't wait up for transmitted skbs to be freed. */
> > > if (!use_napi) {
> > > + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> > > + struct ubuf_info *uarg;
> > > + uarg = skb_shinfo(skb)->destructor_arg;
> > > + if (uarg->callback)
> > > + uarg->callback(uarg, true);
> > > + skb_shinfo(skb)->destructor_arg = NULL;
> > > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> > > + }
> >
> > Instead of open coding, this can use skb_zcopy_clear.
>
> It is not correct to only send the zerocopy completion here, as
> the skb will still be shared with the nic. The only safe approach
> to notifying early is to create a copy with skb_orphan_frags_rx.
> That will call skb_zcopy_clear(skb, false) internally. But the
> copy will be very expensive.
I noticed this email after my last post. I cannot not imagine how it could be
unsafe in virtio case. Sorry could you explain a bit more?
>
> Is the deadlock you refer to the case where tx interrupts are
> disabled, so transmit completions are only handled in start_xmit
> and somehow the socket(s) are unable to send new data? The
> question is what is blocking them. If it is zerocopy, it may be a
> too low optmem_max or hitting the per-user locked pages limit.
> We may have to keep interrupts enabled when zerocopy skbs
> are in flight.
Even if we keep interrupts enabled, We miss the completion without start_xmit.
And it's also likely that the next start_xmit depends on the completion itself
as I described in my last post.
_______________________________________________
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: Koichiro Den @ 2017-08-22 13:42 UTC (permalink / raw)
To: Willem de Bruijn, Jason Wang
Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-L-9QnxV2kM_j2BxVk=h7i3wvMZxG-3_=0Q=jDPgeT=VQ@mail.gmail.com>
On Mon, 2017-08-21 at 23:10 -0400, Willem de Bruijn wrote:
> > > > Interesting, deadlock could be treated as a a radical case of the
> > > > discussion
> > > > here https://patchwork.kernel.org/patch/3787671/.
> > > >
> > > > git grep tells more similar skb_orphan() cases. Do we need to change
> > > > them
> > > > all (or part)?
> > >
> > > Most skb_orphan calls are not relevant to the issue of transmit delay.
> >
> >
> > Yes, but at least we should audit the ones in drivers/net.
>
> Do you mean other virtual device driver transmit paths, like xen,
> specifically?
>
> > > > Actually, we may meet similar issues at many other places (e.g netem).
> > >
> > > Netem is an interesting case. Because it is intended to mimic network
> > > delay, at least in the case where it calls skb_orphan, it may make
> > > sense to release all references, including calling skb_zcopy_clear.
> > >
> > > In general, zerocopy reverts to copy on all paths that may cause
> > > unbounded delay due to another process. Guarding against delay
> > > induced by the administrator is infeasible. It is always possible to
> > > just pause the nic. Netem is one instance of that, and not unbounded.
> >
> >
> > The problem is, admin may only delay the traffic in e.g one interface, but
> > it actually delay or stall all traffic inside a VM.
>
> Understood. Ideally we can remove the HoL blocking cause of this,
> itself.
>
> > > > Need
> > > > to consider a complete solution for this. Figuring out all places that
> > > > could
> > > > delay a packet is a method.
> > >
> > > The issue described in the referenced patch seems like head of line
> > > blocking between two flows. If one flow delays zerocopy descriptor
> > > release from the vhost-net pool, it blocks all subsequent descriptors
> > > in that pool from being released, also delaying other flows that use
> > > the same descriptor pool. If the pool is empty, all transmission stopped.
> > >
> > > Reverting to copy tx when the pool reaches a low watermark, as the
> > > patch does, fixes this.
> >
> >
> > An issue of the referenced patch is that sndbuf could be smaller than low
> > watermark.
We cannot determine the low watermark properly because of not only sndbuf size
issue but also the fact that the upper vhost-net cannot directly see how much
descriptor is currently available at the virtio-net tx queue. It depends on
multiqueue settings or other senders which are also using the same tx queue.
Note that in the latter case if they constantly transmitting, the deadlock could
not occur(*), however if it has just temporarily fulfill some portion of the
pool in the mean time, then the low watermark cannot be helpful.
(*: That is because it's reliable enough in the sense I mention below.)
Keep in this in mind, let me briefly describe the possible deadlock I mentioned:
(1). vhost-net on L1 guest has nothing to do sendmsg until the upper layer sets
new descriptors, which depends only on the vhost-net zcopy callback and adding
newly used descriptors.
(2). vhost-net callback depends on the skb freeing on the xmit path only.
(3). the xmit path depends (possibly only) on the vhost-net sendmsg.
As you see, it's enough to bring about the situation above that L1 virtio-net
reaches its limit earlier than the L0 host processing. The vhost-net pool could
be almost full or empty, whatever.
Also note that relaxing the restriction (2) (and thus remove the dependency (3)
-> (1)) seems quite natural but it needs to be reliable enough to never miss the
last trigger if it's based on interruption. We might need to do some
modification on virtio interruption mitigation in none tx napi case but it's too
costly and not fair to cover the upper layer peculiarity.
So I think drivers which holds characteristics such as (2) is worth checking
whether we should revise.
> >
> > > Perhaps the descriptor pool should also be
> > > revised to allow out of order completions. Then there is no need to
> > > copy zerocopy packets whenever they may experience delay.
> >
> >
> > Yes, but as replied in the referenced thread, windows driver may treat out
> > of order completion as a bug.
>
> Interesting. I missed that. Perhaps the zerocopy optimization
> could be gated on guest support for out of order completions.
>
> > > On the point of counting copy vs zerocopy: the new msg_zerocopy
> > > variant of ubuf_info has a field to record whether a deep copy was
> > > made. This can be used with vhost-net zerocopy, too.
> >
> >
> > Just to make sure I understand. It's still not clear to me how to reuse this
> > for vhost-net, e.g zerocopy flag is in a union which is not used by
> > vhost_net.
>
> True, but that is not set in stone. I went back and forth on that when
> preparing fix 0a4a060bb204 ("sock: fix zerocopy_success regression
> with msg_zerocopy"). The field can be moved outside the union and
> initialized in the other zerocopy paths.
_______________________________________________
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-22 12:11 UTC (permalink / raw)
To: Koichiro Den; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-LYLsQNt_5yYZ+PUwv04ZwaJUXn-YakH=tuh0TV+BG7iA@mail.gmail.com>
On Sun, Aug 20, 2017 at 4:49 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Sat, Aug 19, 2017 at 2:38 AM, Koichiro Den <den@klaipeden.com> wrote:
>> Facing the possible unbounded delay relying on freeing on xmit path,
>> we also better to invoke and clear the upper layer zerocopy callback
>> beforehand to keep them from waiting for unbounded duration in vain.
>
> Good point.
>
>> For instance, this removes the possible deadlock in the case that the
>> upper layer is a zerocopy-enabled vhost-net.
>> This does not apply if napi_tx is enabled since it will be called in
>> reasonale time.
>
> Indeed. Btw, I am gathering data to eventually make napi the default
> mode. But that is taking some time.
>
>>
>> Signed-off-by: Koichiro Den <den@klaipeden.com>
>> ---
>> drivers/net/virtio_net.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4302f313d9a7..f7deaa5b7b50 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>>
>> /* Don't wait up for transmitted skbs to be freed. */
>> if (!use_napi) {
>> + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
>> + struct ubuf_info *uarg;
>> + uarg = skb_shinfo(skb)->destructor_arg;
>> + if (uarg->callback)
>> + uarg->callback(uarg, true);
>> + skb_shinfo(skb)->destructor_arg = NULL;
>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
>> + }
>
> Instead of open coding, this can use skb_zcopy_clear.
It is not correct to only send the zerocopy completion here, as
the skb will still be shared with the nic. The only safe approach
to notifying early is to create a copy with skb_orphan_frags_rx.
That will call skb_zcopy_clear(skb, false) internally. But the
copy will be very expensive.
Is the deadlock you refer to the case where tx interrupts are
disabled, so transmit completions are only handled in start_xmit
and somehow the socket(s) are unable to send new data? The
question is what is blocking them. If it is zerocopy, it may be a
too low optmem_max or hitting the per-user locked pages limit.
We may have to keep interrupts enabled when zerocopy skbs
are in flight.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-08-22 11:47 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <CAF=yD-L-9QnxV2kM_j2BxVk=h7i3wvMZxG-3_=0Q=jDPgeT=VQ@mail.gmail.com>
On 2017年08月22日 11:10, Willem de Bruijn wrote:
>>>> Interesting, deadlock could be treated as a a radical case of the
>>>> discussion
>>>> here https://patchwork.kernel.org/patch/3787671/.
>>>>
>>>> git grep tells more similar skb_orphan() cases. Do we need to change them
>>>> all (or part)?
>>> Most skb_orphan calls are not relevant to the issue of transmit delay.
>>
>> Yes, but at least we should audit the ones in drivers/net.
> Do you mean other virtual device driver transmit paths, like xen,
> specifically?
Git grep does not show skb_orphan() was used for xen for me. But looking
at cxgb4/sge.c which seems to call skb_orphan() for large packet and
reclaim transmitted packets when:
- doing ndo_start_xmit()
- or a timer.
>>>> Actually, we may meet similar issues at many other places (e.g netem).
>>> Netem is an interesting case. Because it is intended to mimic network
>>> delay, at least in the case where it calls skb_orphan, it may make
>>> sense to release all references, including calling skb_zcopy_clear.
>>>
>>> In general, zerocopy reverts to copy on all paths that may cause
>>> unbounded delay due to another process. Guarding against delay
>>> induced by the administrator is infeasible. It is always possible to
>>> just pause the nic. Netem is one instance of that, and not unbounded.
>>
>> The problem is, admin may only delay the traffic in e.g one interface, but
>> it actually delay or stall all traffic inside a VM.
> Understood. Ideally we can remove the HoL blocking cause of this,
> itself.
>
>>>> Need
>>>> to consider a complete solution for this. Figuring out all places that
>>>> could
>>>> delay a packet is a method.
>>> The issue described in the referenced patch seems like head of line
>>> blocking between two flows. If one flow delays zerocopy descriptor
>>> release from the vhost-net pool, it blocks all subsequent descriptors
>>> in that pool from being released, also delaying other flows that use
>>> the same descriptor pool. If the pool is empty, all transmission stopped.
>>>
>>> Reverting to copy tx when the pool reaches a low watermark, as the
>>> patch does, fixes this.
>>
>> An issue of the referenced patch is that sndbuf could be smaller than low
>> watermark.
>>
>>> Perhaps the descriptor pool should also be
>>> revised to allow out of order completions. Then there is no need to
>>> copy zerocopy packets whenever they may experience delay.
>>
>> Yes, but as replied in the referenced thread, windows driver may treat out
>> of order completion as a bug.
> Interesting. I missed that. Perhaps the zerocopy optimization
> could be gated on guest support for out of order completions.
Yes, we may plan to explicitly notify driver about out of order in
future virtio.
>
>>> On the point of counting copy vs zerocopy: the new msg_zerocopy
>>> variant of ubuf_info has a field to record whether a deep copy was
>>> made. This can be used with vhost-net zerocopy, too.
>>
>> Just to make sure I understand. It's still not clear to me how to reuse this
>> for vhost-net, e.g zerocopy flag is in a union which is not used by
>> vhost_net.
> True, but that is not set in stone. I went back and forth on that when
> preparing fix 0a4a060bb204 ("sock: fix zerocopy_success regression
> with msg_zerocopy"). The field can be moved outside the union and
> initialized in the other zerocopy paths.
Ok. I see.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drm: virtio: constify drm_fb_helper_funcs
From: Daniel Vetter @ 2017-08-22 6:35 UTC (permalink / raw)
To: Arvind Yadav
Cc: airlied, linux-kernel, dri-devel, virtualization, daniel.vetter
In-Reply-To: <60db0ba6ddce08444299533e97c9adec3b3ef1bb.1503311145.git.arvind.yadav.cs@gmail.com>
On Mon, Aug 21, 2017 at 04:37:32PM +0530, Arvind Yadav wrote:
> drm_fb_helper_funcs are not supposed to change at runtime.
> All functions working with drm_fb_helper_funcs provided
> by <drm/drm_fb_helper.h> work with const drm_fb_helper_funcs.
> So mark the non-const structs as const.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
> drivers/gpu/drm/virtio/virtgpu_fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
> index 33df067..61f33ec 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fb.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
> @@ -309,7 +309,7 @@ static int virtio_gpu_fbdev_destroy(struct drm_device *dev,
>
> return 0;
> }
> -static struct drm_fb_helper_funcs virtio_gpu_fb_helper_funcs = {
> +static const struct drm_fb_helper_funcs virtio_gpu_fb_helper_funcs = {
I have this patch already from someone else. Please create your patches
against linux-next to avoid this kind of duplicated work.
Thanks, Daniel
> .fb_probe = virtio_gpufb_create,
> };
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* RE: [RFC 2/3] virtio-iommu: device probing and operations
From: Tian, Kevin @ 2017-08-22 6:24 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: cdall@linaro.org, lorenzo.pieralisi@arm.com, mst@redhat.com,
marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com,
robin.murphy@arm.com
In-Reply-To: <454095c4-cae5-ad52-a459-5c9e2cce4047@arm.com>
> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> Sent: Monday, August 21, 2017 8:00 PM
>
> On 21/08/17 08:59, Tian, Kevin wrote:
> >> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> >> Sent: Monday, April 24, 2017 11:06 PM
> >>>>>> 1. Attach device
> >>>>>> ----------------
> >>>>>>
> >>>>>> struct virtio_iommu_req_attach {
> >>>>>> le32 address_space;
> >>>>>> le32 device;
> >>>>>> le32 flags/reserved;
> >>>>>> };
> >>>>>>
> >>>>>> Attach a device to an address space. 'address_space' is an identifier
> >>>>>> unique to the guest. If the address space doesn't exist in the
> IOMMU
> >>>>>
> >>>>> Based on your description this address space ID is per operation
> right?
> >>>>> MAP/UNMAP and page-table sharing should have different ID
> spaces...
> >>>>
> >>>> I think it's simpler if we keep a single IOASID space per virtio-iommu
> >>>> device, because the maximum number of address spaces (described
> by
> >>>> ioasid_bits) might be a restriction of the pIOMMU. For page-table
> >> sharing
> >>>> you still need to define which devices will share a page directory using
> >>>> ATTACH requests, though that interface is not set in stone.
> >>>
> >>> got you. yes VM is supposed to consume less IOASIDs than physically
> >>> available. It doesn’t hurt to have one IOASID space for both IOVA
> >>> map/unmap usages (one IOASID per device) and SVM usages (multiple
> >>> IOASIDs per device). The former is digested by software and the latter
> >>> will be bound to hardware.
> >>>
> >>
> >> Hmm, I'm using address space indexed by IOASID for "classic" IOMMU,
> and
> >> then contexts indexed by PASID when talking about SVM. So in my mind
> an
> >> address space can have multiple sub-address-spaces (contexts). Number
> of
> >> IOASIDs is a limitation of the pIOMMU, and number of PASIDs is a
> >> limitation of the device. Therefore attaching devices to address spaces
> >> would update the number of available contexts in that address space.
> The
> >> terminology is not ideal, and I'd be happy to change it for something
> more
> >> clear.
> >>
> >
> > (sorry to pick up this old thread, as the .tex one is not good for review
> > and this thread provides necessary background for IOASID).
> >
> > Hi, Jean,
> >
> > I'd like to hear more clarification regarding the relationship between
> > IOASID and PASID. When reading back above explanation, it looks
> > confusing to me now (though I might get the meaning months ago :/).
> > At least Intel VT-d only understands PASID (or you can think IOASID
> > =PASID). There is no such layered address space concept. Then for
> > map/unmap type request, do you intend to steal some PASIDs for
> > that purpose on such architecture (since IOASID is a mandatory field
> > in map/unmap request)?
>
> IOASID is a logical ID, it isn't used by hardware. The address space
> concept in virtio-iommu allows to group endpoints together, so that they
> have the same address space. I thought it was pretty much the same as
> "domains" in VT-d? In any case, it is the same as domains in Linux. An
> IOASID provides a handle for communication between virtio-iommu device
> and
> driver, but unlike PASID, the IOASID number doesn't mean anything outside
> of virtio-iommu.
Thanks. It's clear to me then.
btw does it make more sense to use "domain id" instead of "IO address
space id"? For one, when talking about layered address spaces
usually parent address space is a superset of all child address spaces
which doesn't apply to this case, since either anonymous address
space or PASID-tagged address spaces are completely isolated. Instead
'domain' is a more inclusive terminology to embrace multiple address
spaces. For two, 'domain' is better aligned to software terminology (e.g.
iommu_domain) is easier for people to catch up. :-)
>
> I haven't introduced PASIDs in public virtio-iommu documents yet, but the
> way I intend it, PASID != IOASID. We will still have a logical address
> space identified by IOASID, that can contain multiple contexts identified
> by PASID. At the moment, after the ATTACH request, an address space
> contains a single anonymous context (NO PASID) that can be managed with
> MAP/UNMAP requests. With virtio-iommu v0.4, structures look like the
> following. The NO PASID context is implicit.
>
> address space context
> endpoint ----. .- mapping
> endpoint ----+---- IOASID -------- NO PASID ----+- mapping
> endpoint ----' '- mapping
>
> I'd like to add a flag to ATTACH that says "don't create a default
> anonymous context, I'll handle contexts myself". Then a new "ADD_TABLE"
> request to handle contexts. When creating a context, the guest decides if
> it wants to manage it via MAP/UNMAP requests (and a new "context" field),
> or instead manage mappings itself by allocating a page directory and use
> INVALIDATE requests.
>
> address space context
> endpoint ----. .- mapping
> endpoint ----+---- IOASID ----+--- NO PASID ----+- mapping
> endpoint ----' | '- mapping
> +--- PASID 0 ---- pgd
> | ...
> '--- PASID N ---- pgd
>
> In this example the guest chose to still have an anonymous context that
> uses MAP/UNMAP, along with a few PASID contexts with their own page
> tables.
>
Above explanation is a good background. Is it useful to include it
in current spec? Though SVM support is not planned now, adding
such background could help build a full story for IOASID concept.
Thanks
Kevin
_______________________________________________
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-22 3:10 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <64d451ae-9944-e978-5a05-54bb1a62aaad@redhat.com>
>>> Interesting, deadlock could be treated as a a radical case of the
>>> discussion
>>> here https://patchwork.kernel.org/patch/3787671/.
>>>
>>> git grep tells more similar skb_orphan() cases. Do we need to change them
>>> all (or part)?
>>
>> Most skb_orphan calls are not relevant to the issue of transmit delay.
>
>
> Yes, but at least we should audit the ones in drivers/net.
Do you mean other virtual device driver transmit paths, like xen,
specifically?
>>> Actually, we may meet similar issues at many other places (e.g netem).
>>
>> Netem is an interesting case. Because it is intended to mimic network
>> delay, at least in the case where it calls skb_orphan, it may make
>> sense to release all references, including calling skb_zcopy_clear.
>>
>> In general, zerocopy reverts to copy on all paths that may cause
>> unbounded delay due to another process. Guarding against delay
>> induced by the administrator is infeasible. It is always possible to
>> just pause the nic. Netem is one instance of that, and not unbounded.
>
>
> The problem is, admin may only delay the traffic in e.g one interface, but
> it actually delay or stall all traffic inside a VM.
Understood. Ideally we can remove the HoL blocking cause of this,
itself.
>>> Need
>>> to consider a complete solution for this. Figuring out all places that
>>> could
>>> delay a packet is a method.
>>
>> The issue described in the referenced patch seems like head of line
>> blocking between two flows. If one flow delays zerocopy descriptor
>> release from the vhost-net pool, it blocks all subsequent descriptors
>> in that pool from being released, also delaying other flows that use
>> the same descriptor pool. If the pool is empty, all transmission stopped.
>>
>> Reverting to copy tx when the pool reaches a low watermark, as the
>> patch does, fixes this.
>
>
> An issue of the referenced patch is that sndbuf could be smaller than low
> watermark.
>
>> Perhaps the descriptor pool should also be
>> revised to allow out of order completions. Then there is no need to
>> copy zerocopy packets whenever they may experience delay.
>
>
> Yes, but as replied in the referenced thread, windows driver may treat out
> of order completion as a bug.
Interesting. I missed that. Perhaps the zerocopy optimization
could be gated on guest support for out of order completions.
>> On the point of counting copy vs zerocopy: the new msg_zerocopy
>> variant of ubuf_info has a field to record whether a deep copy was
>> made. This can be used with vhost-net zerocopy, too.
>
>
> Just to make sure I understand. It's still not clear to me how to reuse this
> for vhost-net, e.g zerocopy flag is in a union which is not used by
> vhost_net.
True, but that is not set in stone. I went back and forth on that when
preparing fix 0a4a060bb204 ("sock: fix zerocopy_success regression
with msg_zerocopy"). The field can be moved outside the union and
initialized in the other zerocopy paths.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Jason Wang @ 2017-08-22 2:50 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <CAF=yD-LZ4=WAYfUtY7xRWi50FRSkrcOa+b7uc46xRnC4sbDCzQ@mail.gmail.com>
On 2017年08月21日 23:41, Willem de Bruijn wrote:
> On Mon, Aug 21, 2017 at 8:33 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2017年08月19日 14:38, Koichiro Den wrote:
>>> Facing the possible unbounded delay relying on freeing on xmit path,
>>> we also better to invoke and clear the upper layer zerocopy callback
>>> beforehand to keep them from waiting for unbounded duration in vain.
>>> For instance, this removes the possible deadlock in the case that the
>>> upper layer is a zerocopy-enabled vhost-net.
>>> This does not apply if napi_tx is enabled since it will be called in
>>> reasonale time.
>>>
>>> Signed-off-by: Koichiro Den <den@klaipeden.com>
>>> ---
>>> drivers/net/virtio_net.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 4302f313d9a7..f7deaa5b7b50 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
>>> struct net_device *dev)
>>> /* Don't wait up for transmitted skbs to be freed. */
>>> if (!use_napi) {
>>> + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
>>> + struct ubuf_info *uarg;
>>> + uarg = skb_shinfo(skb)->destructor_arg;
>>> + if (uarg->callback)
>>> + uarg->callback(uarg, true);
>>> + skb_shinfo(skb)->destructor_arg = NULL;
>>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
>>> + }
>>> skb_orphan(skb);
>>> nf_reset(skb);
>>> }
>>
>>
>> Interesting, deadlock could be treated as a a radical case of the discussion
>> here https://patchwork.kernel.org/patch/3787671/.
>>
>> git grep tells more similar skb_orphan() cases. Do we need to change them
>> all (or part)?
> Most skb_orphan calls are not relevant to the issue of transmit delay.
Yes, but at least we should audit the ones in drivers/net.
>
>> Actually, we may meet similar issues at many other places (e.g netem).
> Netem is an interesting case. Because it is intended to mimic network
> delay, at least in the case where it calls skb_orphan, it may make
> sense to release all references, including calling skb_zcopy_clear.
>
> In general, zerocopy reverts to copy on all paths that may cause
> unbounded delay due to another process. Guarding against delay
> induced by the administrator is infeasible. It is always possible to
> just pause the nic. Netem is one instance of that, and not unbounded.
The problem is, admin may only delay the traffic in e.g one interface,
but it actually delay or stall all traffic inside a VM.
>
>> Need
>> to consider a complete solution for this. Figuring out all places that could
>> delay a packet is a method.
> The issue described in the referenced patch seems like head of line
> blocking between two flows. If one flow delays zerocopy descriptor
> release from the vhost-net pool, it blocks all subsequent descriptors
> in that pool from being released, also delaying other flows that use
> the same descriptor pool. If the pool is empty, all transmission stopped.
>
> Reverting to copy tx when the pool reaches a low watermark, as the
> patch does, fixes this.
An issue of the referenced patch is that sndbuf could be smaller than
low watermark.
> Perhaps the descriptor pool should also be
> revised to allow out of order completions. Then there is no need to
> copy zerocopy packets whenever they may experience delay.
Yes, but as replied in the referenced thread, windows driver may treat
out of order completion as a bug.
>
> On the point of counting copy vs zerocopy: the new msg_zerocopy
> variant of ubuf_info has a field to record whether a deep copy was
> made. This can be used with vhost-net zerocopy, too.
Just to make sure I understand. It's still not clear to me how to reuse
this for vhost-net, e.g zerocopy flag is in a union which is not used by
vhost_net.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v14 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-08-21 20:22 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <599699AF.1090705@intel.com>
On Fri, Aug 18, 2017 at 03:39:27PM +0800, Wei Wang wrote:
> On 08/18/2017 10:22 AM, Michael S. Tsirkin wrote:
> > +static void send_balloon_page_sg(struct virtio_balloon *vb,
> > + struct virtqueue *vq,
> > + void *addr,
> > + uint32_t size)
> > +{
> > + unsigned int len;
> > + int ret;
> > +
> > + do {
> > + ret = add_one_sg(vq, addr, size);
> > + virtqueue_kick(vq);
> > + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > + /*
> > + * It is uncommon to see the vq is full, because the sg is sent
> > + * one by one and the device is able to handle it in time. But
> > + * if that happens, we go back to retry after an entry gets
> > + * released.
> > + */
> > Why send one by one though? Why not batch some s/gs and wait for all
> > of them to be completed? If memory if fragmented, waiting every time is
> > worse than what we have now (VIRTIO_BALLOON_ARRAY_PFNS_MAX at a time).
> >
>
> OK, I'll do batching in some fashion.
>
>
> Best,
> Wei
>
>
btw you need to address the build errors that kbot has found.
--
MST
^ permalink raw reply
* Re: [PATCH 1/2] vhost: remove the possible fruitless search on iotlb prefetch
From: Michael S. Tsirkin @ 2017-08-21 19:45 UTC (permalink / raw)
To: Koichiro Den; +Cc: netdev, kvm, virtualization
In-Reply-To: <20170819064114.27219-1-den@klaipeden.com>
On Sat, Aug 19, 2017 at 03:41:14PM +0900, Koichiro Den wrote:
> Signed-off-by: Koichiro Den <den@klaipeden.com>
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e4613a3c362d..93e909afc1c3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1184,7 +1184,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
> while (len > s) {
> node = vhost_umem_interval_tree_iter_first(&umem->umem_tree,
> addr,
> - addr + len - 1);
> + addr + len - s - 1);
> if (node == NULL || node->start > addr) {
> vhost_iotlb_miss(vq, addr, access);
> return false;
This works but it probably makes sense to just refactor the code to make end of
range a variable. I posted a patch like this, pls take a look.
> --
> 2.9.4
>
^ permalink raw reply
* [PATCH] vhost: fix end of range for access_ok
From: Michael S. Tsirkin @ 2017-08-21 19:45 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, netdev, Koichiro Den, virtualization, David Miller
During access_ok checks, addr increases as we iterate over the data
structure, thus addr + len - 1 will point beyond the end of region we
are translating. Harmless since we then verify that the region covers
addr, but let's not waste cpu cycles.
Reported-by: Koichiro Den <den@klaipeden.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Lightly tested, would appreciate an ack from reporter.
drivers/vhost/vhost.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e4613a3..ecd70e4 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1176,7 +1176,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
{
const struct vhost_umem_node *node;
struct vhost_umem *umem = vq->iotlb;
- u64 s = 0, size, orig_addr = addr;
+ u64 s = 0, size, orig_addr = addr, last = addr + len - 1;
if (vhost_vq_meta_fetch(vq, addr, len, type))
return true;
@@ -1184,7 +1184,7 @@ static int iotlb_access_ok(struct vhost_virtqueue *vq,
while (len > s) {
node = vhost_umem_interval_tree_iter_first(&umem->umem_tree,
addr,
- addr + len - 1);
+ last);
if (node == NULL || node->start > addr) {
vhost_iotlb_miss(vq, addr, access);
return false;
--
MST
^ permalink raw reply related
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-08-21 15:41 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, Koichiro Den,
Michael S. Tsirkin
In-Reply-To: <5352c98a-fa48-fcf9-c062-9986a317a1b0@redhat.com>
On Mon, Aug 21, 2017 at 8:33 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年08月19日 14:38, Koichiro Den wrote:
>>
>> Facing the possible unbounded delay relying on freeing on xmit path,
>> we also better to invoke and clear the upper layer zerocopy callback
>> beforehand to keep them from waiting for unbounded duration in vain.
>> For instance, this removes the possible deadlock in the case that the
>> upper layer is a zerocopy-enabled vhost-net.
>> This does not apply if napi_tx is enabled since it will be called in
>> reasonale time.
>>
>> Signed-off-by: Koichiro Den <den@klaipeden.com>
>> ---
>> drivers/net/virtio_net.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4302f313d9a7..f7deaa5b7b50 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
>> struct net_device *dev)
>> /* Don't wait up for transmitted skbs to be freed. */
>> if (!use_napi) {
>> + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
>> + struct ubuf_info *uarg;
>> + uarg = skb_shinfo(skb)->destructor_arg;
>> + if (uarg->callback)
>> + uarg->callback(uarg, true);
>> + skb_shinfo(skb)->destructor_arg = NULL;
>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
>> + }
>> skb_orphan(skb);
>> nf_reset(skb);
>> }
>
>
>
> Interesting, deadlock could be treated as a a radical case of the discussion
> here https://patchwork.kernel.org/patch/3787671/.
>
> git grep tells more similar skb_orphan() cases. Do we need to change them
> all (or part)?
Most skb_orphan calls are not relevant to the issue of transmit delay.
> Actually, we may meet similar issues at many other places (e.g netem).
Netem is an interesting case. Because it is intended to mimic network
delay, at least in the case where it calls skb_orphan, it may make
sense to release all references, including calling skb_zcopy_clear.
In general, zerocopy reverts to copy on all paths that may cause
unbounded delay due to another process. Guarding against delay
induced by the administrator is infeasible. It is always possible to
just pause the nic. Netem is one instance of that, and not unbounded.
> Need
> to consider a complete solution for this. Figuring out all places that could
> delay a packet is a method.
The issue described in the referenced patch seems like head of line
blocking between two flows. If one flow delays zerocopy descriptor
release from the vhost-net pool, it blocks all subsequent descriptors
in that pool from being released, also delaying other flows that use
the same descriptor pool. If the pool is empty, all transmission stopped.
Reverting to copy tx when the pool reaches a low watermark, as the
patch does, fixes this. Perhaps the descriptor pool should also be
revised to allow out of order completions. Then there is no need to
copy zerocopy packets whenever they may experience delay.
On the point of counting copy vs zerocopy: the new msg_zerocopy
variant of ubuf_info has a field to record whether a deep copy was
made. This can be used with vhost-net zerocopy, too.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/2] vhost-net: revert vhost_exceeds_maxpend logic to its original
From: Koichiro Den @ 2017-08-21 13:32 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: netdev, kvm, virtualization
In-Reply-To: <047bb207-cec6-ece5-470f-340d7c5ee7b7@redhat.com>
On Mon, 2017-08-21 at 20:40 +0800, Jason Wang wrote:
>
> On 2017年08月21日 11:06, Jason Wang wrote:
> >
> >
> > On 2017年08月19日 14:41, Koichiro Den wrote:
> > > To depend on vq.num and the usage of VHOST_MAX_PEND is not succinct
> > > and in some case unexpected, so revert its logic part only.
> >
> > Hi:
> >
> > Could you explain a little bit more on the case that is was not
> > sufficent?
> >
> > Thanks
>
> Just have another thought.
>
> I wonder whether or not just use ulimit(memlock) is better here. It
> looks more flexible.
>
> Thanks
It sounds a better approach, though at present I have got no idea how much
portion it should be.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/2] vhost-net: revert vhost_exceeds_maxpend logic to its original
From: Koichiro Den @ 2017-08-21 13:30 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: netdev, kvm, virtualization
In-Reply-To: <0cbc33d6-eb1e-cea4-4cdf-a8a88932da62@redhat.com>
On Mon, 2017-08-21 at 11:06 +0800, Jason Wang wrote:
>
> On 2017年08月19日 14:41, Koichiro Den wrote:
> > To depend on vq.num and the usage of VHOST_MAX_PEND is not succinct
> > and in some case unexpected, so revert its logic part only.
>
> Hi:
>
> Could you explain a little bit more on the case that is was not sufficent?
It's just because vq.num could theoretically be around VHOST_MAX_PEND, that
could lead to an unexpected situation. Plus, I thought its name and usage is not
intrinsic. Its actual functioning in normal vq.num == 256 case is I think good
enough. Thanks.
>
> Thanks
>
> >
> > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > ---
> > drivers/vhost/net.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 06d044862e58..99cf99b308a7 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -433,11 +433,15 @@ static int vhost_net_tx_get_vq_desc(struct vhost_net
> > *net,
> >
> > static bool vhost_exceeds_maxpend(struct vhost_net *net)
> > {
> > + int num_pends;
> > struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> > struct vhost_virtqueue *vq = &nvq->vq;
> >
> > - return (nvq->upend_idx + vq->num - VHOST_MAX_PEND) % UIO_MAXIOV
> > - == nvq->done_idx;
> > + num_pends = likely(nvq->upend_idx >= nvq->done_idx) ?
> > + (nvq->upend_idx - nvq->done_idx) :
> > + (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx);
> > +
> > + return num_pends > VHOST_MAX_PEND;
> > }
> >
> > /* Expects to be always run from workqueue - which acts as
>
>
_______________________________________________
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: Koichiro Den @ 2017-08-21 12:58 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: netdev, virtualization
In-Reply-To: <5352c98a-fa48-fcf9-c062-9986a317a1b0@redhat.com>
On Mon, 2017-08-21 at 20:33 +0800, Jason Wang wrote:
>
> On 2017年08月19日 14:38, Koichiro Den wrote:
> > Facing the possible unbounded delay relying on freeing on xmit path,
> > we also better to invoke and clear the upper layer zerocopy callback
> > beforehand to keep them from waiting for unbounded duration in vain.
> > For instance, this removes the possible deadlock in the case that the
> > upper layer is a zerocopy-enabled vhost-net.
> > This does not apply if napi_tx is enabled since it will be called in
> > reasonale time.
> >
> > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > ---
> > drivers/net/virtio_net.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4302f313d9a7..f7deaa5b7b50 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> >
> > /* Don't wait up for transmitted skbs to be freed. */
> > if (!use_napi) {
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> > + struct ubuf_info *uarg;
> > + uarg = skb_shinfo(skb)->destructor_arg;
> > + if (uarg->callback)
> > + uarg->callback(uarg, true);
> > + skb_shinfo(skb)->destructor_arg = NULL;
> > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> > + }
> > skb_orphan(skb);
> > nf_reset(skb);
> > }
>
>
> Interesting, deadlock could be treated as a a radical case of the
> discussion here https://patchwork.kernel.org/patch/3787671/.
Thanks for letting me know this one. I am going to read it.
>
> git grep tells more similar skb_orphan() cases. Do we need to change
> them all (or part)?
Maybe, even though it seems less likely that we may meet it than the one I
described as an example, such as virtio-net sandwiched between vhost-net.
>
> Actually, we may meet similar issues at many other places (e.g netem).
> Need to consider a complete solution for this. Figuring out all places
> that could delay a packet is a method.
Okay I will do it and post the result and a suggestion if possible. Thanks.
>
> Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
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