* Re: [PATCH] [RFC] virtio: Limit the retries on a virtio device reset
From: Pierre Morel @ 2017-08-24 17:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Cornelia Huck, virtualization
In-Reply-To: <20170824170725-mutt-send-email-mst@kernel.org>
On 24/08/2017 16:12, Michael S. Tsirkin wrote:
> On Thu, Aug 24, 2017 at 02:16:11PM +0200, Pierre Morel wrote:
>> On 24/08/2017 13:07, Cornelia Huck wrote:
>>> On Wed, 23 Aug 2017 18:33:02 +0200
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>
>>>> Reseting a device can sometime fail, even a virtual device.
>>>> If the device is not reseted after a while the driver should
>>>> abandon the retries.
>>>> This is the change proposed for the modern virtio_pci.
>>>>
>>>> More generally, when this happens,the virtio driver can set the
>>>> VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
>>>>
>>>> The virtio core can test if the reset was succesful by testing
>>>> this flag after a reset.
>>>>
>>>> This behavior is backward compatible with existing drivers.
>>>> This behavior seems to me compatible with Virtio-1.0 specifications,
>>>> Chapters 2.1 Device Status Field.
>>>> There I definitively need your opinion: Is it right?
>>>
>>> Will have to double check with the spec.
>>>
>>>>
>>>> This patch also lead to another question:
>>>> do we care if a device provided by the hypervisor is buggy?
>>>
>>> Getting into a hang because of a broken device is not nice, but I'm not
>>> sure we need to plan for this. Have you seen this in the wild?
>>
>> Yes, with virtio-pci on S390.
>
> And what triggered this?
Buggy zPCI QEMU device we are currently put right
> I don't think we can recover from a failed reset in all cases.
I do not think so too.
The device must be abandoned.
Too dangerous to be used.
Normaly the hypervisor should not be buggy. But... nobdy's perfect
>
>>
>>>
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> ---
>>>> drivers/virtio/virtio.c | 4 ++++
>>>> drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
>>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>> index 48230a5..6255dc4 100644
>>>> --- a/drivers/virtio/virtio.c
>>>> +++ b/drivers/virtio/virtio.c
>>>> @@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
>>>> /* We always start by resetting the device, in case a previous
>>>> * driver messed it up. This also tests that code path a little. */
>>>> dev->config->reset(dev);
>>>> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
>>>> + return -EIO;
>>>> /* Acknowledge that we've seen the device. */
>>>> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>>>> @@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
>>>> /* We always start by resetting the device, in case a previous
>>>> * driver messed it up. */
>>>> dev->config->reset(dev);
>>>> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
>>>> + return -EIO;
>>>
>>> virtio-ccw prior to rev 2 won't ever see this (as the read command did
>>> not exist then), but this is not really a problem.
>>>
>>>> /* Acknowledge that we've seen the device. */
>>>> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>>>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>>>> index 2555d80..bfc5fc1 100644
>>>> --- a/drivers/virtio/virtio_pci_modern.c
>>>> +++ b/drivers/virtio/virtio_pci_modern.c
>>>> @@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
>>>> static void vp_reset(struct virtio_device *vdev)
>>>> {
>>>> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>>>> + int retry_count = 10;
>>>
>>> When you're touching this anyway, it would be a good time to add an
>>> extra blank line :)
>>
>> Yes, I like blank lines too.
>>
>>>
>>>> /* 0 status means a reset. */
>>>> vp_iowrite8(0, &vp_dev->common->device_status);
>>>> /* After writing 0 to device_status, the driver MUST wait for a read of
>>>> @@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
>>>> * This will flush out the status write, and flush in device writes,
>>>> * including MSI-X interrupts, if any.
>>>> */
>>>> - while (vp_ioread8(&vp_dev->common->device_status))
>>>> + while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
>>>> msleep(1);
>>>> + /* If the read did not return 0 before the timeout consider that
>>>> + * the device failed.
>>>> + */
>>>> + if (retry_count <= 0) {
>>>> + virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
>>>> + return;
>>>> + }
>
> I'm not sure what's the right approach by I don't really like this one:
> - an arbitrary number of retries looks wrong. why 10?
I fear that at this moment we can not rely on a lot of information on
the device. An arbitrary value may not be so bad.
But I agree 10 can be discussed :). It is just a convenient value for
testing.
Something leading to a waiting time of around some seconds would be more
appropriate I think.
> - doing this on probe might be reasonable but any other reset
> is expected to actually reset the device
We are handling virtual devices.
If we consider that if one reset works the next reset will take the same
path and work, we do not have to.
But... not completely sure, bugs can hide everywhere.
> - we'll have to spread these tests all over the place.
I counted 19 places where to check if the reset went OK.
None of them touch the device anymore after reset and just free driver's
resources.
So that if reset failed, nothing goes wrong, no device access, but the
probability that the next probe fail is high. (If it ever succeed).
> Allowing reset to fail would be better.
May be I did not understand what you mean.
Testing the flag or a return value is as expensive.
Of course the implementation is a mater of taste.
I notice two other things to do:
- May be adding a warning would be fine too.
- Virtio_ccw may add a fail flag when allocation of CCW failed.
I did not find anything to do for virtio_mmio or legacy virtio_pci.
Regards,
Pierre
>
>
>>>> + virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>>>
>>> Adding ACK here seems wrong?
>>
>> Exact, I forgot to remove this from a previous test.
>> I wait a little and post a v2
>>
>> Thanks for reviewing.
>>
>> Pierre
>>
>>>
>>>> /* Flush pending VQ/configuration callbacks. */
>>>> vp_synchronize_vectors(vdev);
>>>> }
>>>
>>
>>
>> --
>> Pierre Morel
>> Linux/KVM/QEMU in Böblingen - Germany
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio_pci: fix cpu affinity support
From: Christoph Hellwig @ 2017-08-24 16:07 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization, stable, yasu.isimatu
Commit 0b0f9dc5 ("Revert "virtio_pci: use shared interrupts for
virtqueues"") removed the adjustment of the pre_vectors for the virtio
MSI-X vector allocation which was added in commit fb5e31d9 ("virtio:
allow drivers to request IRQ affinity when creating VQs"). This will
lead to an incorrect assignment of MSI-X vectors, and potential
deadlocks when offlining cpus.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes: 0b0f9dc5 ("Revert "virtio_pci: use shared interrupts for virtqueues")
Reported-by: YASUAKI ISHIMATSU <yasu.isimatu@gmail.com>
Cc: stable@vger.kernel.org
---
drivers/virtio/virtio_pci_common.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 007a4f366086..1c4797e53f68 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -107,6 +107,7 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
const char *name = dev_name(&vp_dev->vdev.dev);
+ unsigned flags = PCI_IRQ_MSIX;
unsigned i, v;
int err = -ENOMEM;
@@ -126,10 +127,13 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
GFP_KERNEL))
goto error;
+ if (desc) {
+ flags |= PCI_IRQ_AFFINITY;
+ desc->pre_vectors++; /* virtio config vector */
+ }
+
err = pci_alloc_irq_vectors_affinity(vp_dev->pci_dev, nvectors,
- nvectors, PCI_IRQ_MSIX |
- (desc ? PCI_IRQ_AFFINITY : 0),
- desc);
+ nvectors, flags, desc);
if (err < 0)
goto error;
vp_dev->msix_enabled = 1;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] [RFC] virtio: Limit the retries on a virtio device reset
From: Michael S. Tsirkin @ 2017-08-24 14:19 UTC (permalink / raw)
To: Pierre Morel; +Cc: cohuck, virtualization
In-Reply-To: <1503505982-29568-1-git-send-email-pmorel@linux.vnet.ibm.com>
On Wed, Aug 23, 2017 at 06:33:02PM +0200, Pierre Morel wrote:
> Reseting a device can sometime fail, even a virtual device.
> If the device is not reseted after a while the driver should
> abandon the retries.
> This is the change proposed for the modern virtio_pci.
>
> More generally, when this happens,the virtio driver can set the
> VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
>
> The virtio core can test if the reset was succesful by testing
> this flag after a reset.
>
> This behavior is backward compatible with existing drivers.
> This behavior seems to me compatible with Virtio-1.0 specifications,
> Chapters 2.1 Device Status Field.
> There I definitively need your opinion: Is it right?
>
> This patch also lead to another question:
> do we care if a device provided by the hypervisor is buggy?
>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
So I think this is not the best place to start to add error recovery.
It should be much more common to have a situation where device gets
broken while it's being used. Spec has a NEEDS_RESET flag for this.
I think we should start by coding up that support in all virtio drivers.
As a next step, we can add more code to detect unexpected behaviour by
the host and mark device as broken. Then we can do more things by
looking at the broken flag.
> ---
> drivers/virtio/virtio.c | 4 ++++
> drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 48230a5..6255dc4 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
> /* We always start by resetting the device, in case a previous
> * driver messed it up. This also tests that code path a little. */
> dev->config->reset(dev);
> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
> + return -EIO;
>
> /* Acknowledge that we've seen the device. */
> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> @@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
> /* We always start by resetting the device, in case a previous
> * driver messed it up. */
> dev->config->reset(dev);
> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
> + return -EIO;
>
> /* Acknowledge that we've seen the device. */
> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 2555d80..bfc5fc1 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
> static void vp_reset(struct virtio_device *vdev)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> + int retry_count = 10;
> /* 0 status means a reset. */
> vp_iowrite8(0, &vp_dev->common->device_status);
> /* After writing 0 to device_status, the driver MUST wait for a read of
> @@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
> * This will flush out the status write, and flush in device writes,
> * including MSI-X interrupts, if any.
> */
> - while (vp_ioread8(&vp_dev->common->device_status))
> + while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
> msleep(1);
> + /* If the read did not return 0 before the timeout consider that
> + * the device failed.
> + */
> + if (retry_count <= 0) {
> + virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
> + return;
> + }
> + virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> /* Flush pending VQ/configuration callbacks. */
> vp_synchronize_vectors(vdev);
> }
> --
> 2.3.0
^ permalink raw reply
* Re: [PATCH] [RFC] virtio: Limit the retries on a virtio device reset
From: Michael S. Tsirkin @ 2017-08-24 14:12 UTC (permalink / raw)
To: Pierre Morel; +Cc: Cornelia Huck, virtualization
In-Reply-To: <af4aa628-3583-50b3-1007-4c8b6d4e2183@linux.vnet.ibm.com>
On Thu, Aug 24, 2017 at 02:16:11PM +0200, Pierre Morel wrote:
> On 24/08/2017 13:07, Cornelia Huck wrote:
> > On Wed, 23 Aug 2017 18:33:02 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >
> > > Reseting a device can sometime fail, even a virtual device.
> > > If the device is not reseted after a while the driver should
> > > abandon the retries.
> > > This is the change proposed for the modern virtio_pci.
> > >
> > > More generally, when this happens,the virtio driver can set the
> > > VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
> > >
> > > The virtio core can test if the reset was succesful by testing
> > > this flag after a reset.
> > >
> > > This behavior is backward compatible with existing drivers.
> > > This behavior seems to me compatible with Virtio-1.0 specifications,
> > > Chapters 2.1 Device Status Field.
> > > There I definitively need your opinion: Is it right?
> >
> > Will have to double check with the spec.
> >
> > >
> > > This patch also lead to another question:
> > > do we care if a device provided by the hypervisor is buggy?
> >
> > Getting into a hang because of a broken device is not nice, but I'm not
> > sure we need to plan for this. Have you seen this in the wild?
>
> Yes, with virtio-pci on S390.
And what triggered this?
I don't think we can recover from a failed reset in all cases.
>
> >
> > >
> > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> > > ---
> > > drivers/virtio/virtio.c | 4 ++++
> > > drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
> > > 2 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index 48230a5..6255dc4 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
> > > /* We always start by resetting the device, in case a previous
> > > * driver messed it up. This also tests that code path a little. */
> > > dev->config->reset(dev);
> > > + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
> > > + return -EIO;
> > > /* Acknowledge that we've seen the device. */
> > > virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> > > @@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
> > > /* We always start by resetting the device, in case a previous
> > > * driver messed it up. */
> > > dev->config->reset(dev);
> > > + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
> > > + return -EIO;
> >
> > virtio-ccw prior to rev 2 won't ever see this (as the read command did
> > not exist then), but this is not really a problem.
> >
> > > /* Acknowledge that we've seen the device. */
> > > virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index 2555d80..bfc5fc1 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
> > > static void vp_reset(struct virtio_device *vdev)
> > > {
> > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > + int retry_count = 10;
> >
> > When you're touching this anyway, it would be a good time to add an
> > extra blank line :)
>
> Yes, I like blank lines too.
>
> >
> > > /* 0 status means a reset. */
> > > vp_iowrite8(0, &vp_dev->common->device_status);
> > > /* After writing 0 to device_status, the driver MUST wait for a read of
> > > @@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
> > > * This will flush out the status write, and flush in device writes,
> > > * including MSI-X interrupts, if any.
> > > */
> > > - while (vp_ioread8(&vp_dev->common->device_status))
> > > + while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
> > > msleep(1);
> > > + /* If the read did not return 0 before the timeout consider that
> > > + * the device failed.
> > > + */
> > > + if (retry_count <= 0) {
> > > + virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
> > > + return;
> > > + }
I'm not sure what's the right approach by I don't really like this one:
- an arbitrary number of retries looks wrong. why 10?
- doing this on probe might be reasonable but any other reset
is expected to actually reset the device
- we'll have to spread these tests all over the place.
Allowing reset to fail would be better.
> > > + virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >
> > Adding ACK here seems wrong?
>
> Exact, I forgot to remove this from a previous test.
> I wait a little and post a v2
>
> Thanks for reviewing.
>
> Pierre
>
> >
> > > /* Flush pending VQ/configuration callbacks. */
> > > vp_synchronize_vectors(vdev);
> > > }
> >
>
>
> --
> Pierre Morel
> Linux/KVM/QEMU in Böblingen - Germany
^ 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-24 13:50 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-KSek+LmZu0X0TXetmFEQ59iF3NpjZ4KugbwLo1BGfhaA@mail.gmail.com>
On Wed, Aug 23, 2017 at 11:28:24PM -0400, Willem de Bruijn wrote:
> >> > * as a generic solution, if we were to somehow overcome the safety issue, track
> >> > the delay and do copy if some threshold is reached could be an answer, but it's
> >> > hard for now.> * so things like the current vhost-net implementation of deciding whether or not
> >> > to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
> >> > practical compromise.
> >>
> >> The fragility of this mechanism is another argument for switching to tx napi
> >> as default.
> >>
> >> Is there any more data about the windows guest issues when completions
> >> are not queued within a reasonable timeframe? What is this timescale and
> >> do we really need to work around this.
> >
> > I think it's pretty large, many milliseconds.
> >
> > But I wonder what do you mean by "work around". Using buffers within
> > limited time frame sounds like a reasonable requirement to me.
>
> Vhost-net zerocopy delays completions until the skb is really
> sent.
This is fundamental in any solution. Guest/application can not
write over a memory buffer as long as hardware might be reading it.
> Traffic shaping can introduce msec timescale latencies.
>
> The delay may actually be a useful signal. If the guest does not
> orphan skbs early, TSQ will throttle the socket causing host
> queue build up.
>
> But, if completions are queued in-order, unrelated flows may be
> throttled as well. Allowing out of order completions would resolve
> this HoL blocking.
We can allow out of order, no guests that follow virtio spec
will break. But this won't help in all cases
- a single slow flow can occupy the whole ring, you will not
be able to make any new buffers available for the fast flow
- what host considers a single flow can be multiple flows for guest
There are many other examples.
> > Neither
> > do I see why would using tx interrupts within guest be a work around -
> > AFAIK windows driver uses tx interrupts.
>
> It does not address completion latency itself. What I meant was
> that in an interrupt-driver model, additional starvation issues,
> such as the potential deadlock raised at the start of this thread,
> or the timer delay observed before packets were orphaned in
> virtio-net in commit b0c39dbdc204, are mitigated.
>
> Specifically, it breaks the potential deadlock where sockets are
> blocked waiting for completions (to free up budget in sndbuf, tsq, ..),
> yet completion handling is blocked waiting for a new packet to
> trigger free_old_xmit_skbs from start_xmit.
This talk of potential deadlock confuses me - I think you mean we would
deadlock if we did not orphan skbs in !use_napi - is that right? If you
mean that you can drop skb orphan and this won't lead to a deadlock if
free skbs upon a tx interrupt, I agree, for sure.
> >> That is the only thing keeping us from removing the HoL blocking in vhost-net zerocopy.
> >
> > 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.
For example, we could set ndo_tx_timeout within guest. Then
if tx queue is stopped for too long, a watchdog would fire.
We worked around most of the issues by introducing guest/host
copy. This copy, done by vhost-net, allows us to pretend that
a not-nic backend (e.g. a qdisc) is a nic (virtio-net).
This way you can both do traffic shaping in host with
unbounded latencies and limit latency from guest point of view.
Cost is both data copies and loss of end to end credit accounting.
Changing Linux as a host to limit latencies while not doing copies will
not be an easy task but that's the only fix that comes to mind.
--
MST
^ permalink raw reply
* Re: [PATCH] [RFC] virtio: Limit the retries on a virtio device reset
From: Pierre Morel @ 2017-08-24 12:16 UTC (permalink / raw)
To: Cornelia Huck; +Cc: mst, virtualization
In-Reply-To: <20170824130746.25f80368.cohuck@redhat.com>
On 24/08/2017 13:07, Cornelia Huck wrote:
> On Wed, 23 Aug 2017 18:33:02 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> Reseting a device can sometime fail, even a virtual device.
>> If the device is not reseted after a while the driver should
>> abandon the retries.
>> This is the change proposed for the modern virtio_pci.
>>
>> More generally, when this happens,the virtio driver can set the
>> VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
>>
>> The virtio core can test if the reset was succesful by testing
>> this flag after a reset.
>>
>> This behavior is backward compatible with existing drivers.
>> This behavior seems to me compatible with Virtio-1.0 specifications,
>> Chapters 2.1 Device Status Field.
>> There I definitively need your opinion: Is it right?
>
> Will have to double check with the spec.
>
>>
>> This patch also lead to another question:
>> do we care if a device provided by the hypervisor is buggy?
>
> Getting into a hang because of a broken device is not nice, but I'm not
> sure we need to plan for this. Have you seen this in the wild?
Yes, with virtio-pci on S390.
>
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>> drivers/virtio/virtio.c | 4 ++++
>> drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index 48230a5..6255dc4 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
>> /* We always start by resetting the device, in case a previous
>> * driver messed it up. This also tests that code path a little. */
>> dev->config->reset(dev);
>> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
>> + return -EIO;
>>
>> /* Acknowledge that we've seen the device. */
>> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> @@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
>> /* We always start by resetting the device, in case a previous
>> * driver messed it up. */
>> dev->config->reset(dev);
>> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
>> + return -EIO;
>
> virtio-ccw prior to rev 2 won't ever see this (as the read command did
> not exist then), but this is not really a problem.
>
>>
>> /* Acknowledge that we've seen the device. */
>> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index 2555d80..bfc5fc1 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
>> @@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
>> static void vp_reset(struct virtio_device *vdev)
>> {
>> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> + int retry_count = 10;
>
> When you're touching this anyway, it would be a good time to add an
> extra blank line :)
Yes, I like blank lines too.
>
>> /* 0 status means a reset. */
>> vp_iowrite8(0, &vp_dev->common->device_status);
>> /* After writing 0 to device_status, the driver MUST wait for a read of
>> @@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
>> * This will flush out the status write, and flush in device writes,
>> * including MSI-X interrupts, if any.
>> */
>> - while (vp_ioread8(&vp_dev->common->device_status))
>> + while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
>> msleep(1);
>> + /* If the read did not return 0 before the timeout consider that
>> + * the device failed.
>> + */
>> + if (retry_count <= 0) {
>> + virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
>> + return;
>> + }
>> + virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>
> Adding ACK here seems wrong?
Exact, I forgot to remove this from a previous test.
I wait a little and post a v2
Thanks for reviewing.
Pierre
>
>> /* Flush pending VQ/configuration callbacks. */
>> vp_synchronize_vectors(vdev);
>> }
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] [RFC] virtio: Limit the retries on a virtio device reset
From: Cornelia Huck @ 2017-08-24 11:07 UTC (permalink / raw)
To: Pierre Morel; +Cc: mst, virtualization
In-Reply-To: <1503505982-29568-1-git-send-email-pmorel@linux.vnet.ibm.com>
On Wed, 23 Aug 2017 18:33:02 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> Reseting a device can sometime fail, even a virtual device.
> If the device is not reseted after a while the driver should
> abandon the retries.
> This is the change proposed for the modern virtio_pci.
>
> More generally, when this happens,the virtio driver can set the
> VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
>
> The virtio core can test if the reset was succesful by testing
> this flag after a reset.
>
> This behavior is backward compatible with existing drivers.
> This behavior seems to me compatible with Virtio-1.0 specifications,
> Chapters 2.1 Device Status Field.
> There I definitively need your opinion: Is it right?
Will have to double check with the spec.
>
> This patch also lead to another question:
> do we care if a device provided by the hypervisor is buggy?
Getting into a hang because of a broken device is not nice, but I'm not
sure we need to plan for this. Have you seen this in the wild?
>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
> drivers/virtio/virtio.c | 4 ++++
> drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 48230a5..6255dc4 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
> /* We always start by resetting the device, in case a previous
> * driver messed it up. This also tests that code path a little. */
> dev->config->reset(dev);
> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
> + return -EIO;
>
> /* Acknowledge that we've seen the device. */
> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> @@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
> /* We always start by resetting the device, in case a previous
> * driver messed it up. */
> dev->config->reset(dev);
> + if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
> + return -EIO;
virtio-ccw prior to rev 2 won't ever see this (as the read command did
not exist then), but this is not really a problem.
>
> /* Acknowledge that we've seen the device. */
> virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 2555d80..bfc5fc1 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
> static void vp_reset(struct virtio_device *vdev)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> + int retry_count = 10;
When you're touching this anyway, it would be a good time to add an
extra blank line :)
> /* 0 status means a reset. */
> vp_iowrite8(0, &vp_dev->common->device_status);
> /* After writing 0 to device_status, the driver MUST wait for a read of
> @@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
> * This will flush out the status write, and flush in device writes,
> * including MSI-X interrupts, if any.
> */
> - while (vp_ioread8(&vp_dev->common->device_status))
> + while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
> msleep(1);
> + /* If the read did not return 0 before the timeout consider that
> + * the device failed.
> + */
> + if (retry_count <= 0) {
> + virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
> + return;
> + }
> + virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
Adding ACK here seems wrong?
> /* Flush pending VQ/configuration callbacks. */
> vp_synchronize_vectors(vdev);
> }
^ 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-24 4:34 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-KSek+LmZu0X0TXetmFEQ59iF3NpjZ4KugbwLo1BGfhaA@mail.gmail.com>
On Wed, Aug 23, 2017 at 11:28:24PM -0400, Willem de Bruijn wrote:
> >> > * as a generic solution, if we were to somehow overcome the safety issue, track
> >> > the delay and do copy if some threshold is reached could be an answer, but it's
> >> > hard for now.> * so things like the current vhost-net implementation of deciding whether or not
> >> > to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
> >> > practical compromise.
> >>
> >> The fragility of this mechanism is another argument for switching to tx napi
> >> as default.
> >>
> >> Is there any more data about the windows guest issues when completions
> >> are not queued within a reasonable timeframe? What is this timescale and
> >> do we really need to work around this.
> >
> > I think it's pretty large, many milliseconds.
> >
> > But I wonder what do you mean by "work around". Using buffers within
> > limited time frame sounds like a reasonable requirement to me.
>
> Vhost-net zerocopy delays completions until the skb is really
> sent. Traffic shaping can introduce msec timescale latencies.
>
> The delay may actually be a useful signal. If the guest does not
> orphan skbs early, TSQ will throttle the socket causing host
> queue build up.
>
> But, if completions are queued in-order, unrelated flows may be
> throttled as well. Allowing out of order completions would resolve
> this HoL blocking.
There's no issue with out of order. It does not break any guests AFAIK.
> > Neither
> > do I see why would using tx interrupts within guest be a work around -
> > AFAIK windows driver uses tx interrupts.
>
> It does not address completion latency itself. What I meant was
> that in an interrupt-driver model, additional starvation issues,
> such as the potential deadlock raised at the start of this thread,
> or the timer delay observed before packets were orphaned in
> virtio-net in commit b0c39dbdc204, are mitigated.
>
> Specifically, it breaks the potential deadlock where sockets are
> blocked waiting for completions (to free up budget in sndbuf, tsq, ..),
> yet completion handling is blocked waiting for a new packet to
> trigger free_old_xmit_skbs from start_xmit.
>
> >> That is the only thing keeping us from removing the HoL blocking in vhost-net zerocopy.
> >
> > We don't enable network watchdog on virtio but we could and maybe
> > should.
>
> Can you elaborate?
^ 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-24 3:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <20170824014553-mutt-send-email-mst@kernel.org>
>> > * as a generic solution, if we were to somehow overcome the safety issue, track
>> > the delay and do copy if some threshold is reached could be an answer, but it's
>> > hard for now.> * so things like the current vhost-net implementation of deciding whether or not
>> > to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
>> > practical compromise.
>>
>> The fragility of this mechanism is another argument for switching to tx napi
>> as default.
>>
>> Is there any more data about the windows guest issues when completions
>> are not queued within a reasonable timeframe? What is this timescale and
>> do we really need to work around this.
>
> I think it's pretty large, many milliseconds.
>
> But I wonder what do you mean by "work around". Using buffers within
> limited time frame sounds like a reasonable requirement to me.
Vhost-net zerocopy delays completions until the skb is really
sent. Traffic shaping can introduce msec timescale latencies.
The delay may actually be a useful signal. If the guest does not
orphan skbs early, TSQ will throttle the socket causing host
queue build up.
But, if completions are queued in-order, unrelated flows may be
throttled as well. Allowing out of order completions would resolve
this HoL blocking.
> Neither
> do I see why would using tx interrupts within guest be a work around -
> AFAIK windows driver uses tx interrupts.
It does not address completion latency itself. What I meant was
that in an interrupt-driver model, additional starvation issues,
such as the potential deadlock raised at the start of this thread,
or the timer delay observed before packets were orphaned in
virtio-net in commit b0c39dbdc204, are mitigated.
Specifically, it breaks the potential deadlock where sockets are
blocked waiting for completions (to free up budget in sndbuf, tsq, ..),
yet completion handling is blocked waiting for a new packet to
trigger free_old_xmit_skbs from start_xmit.
>> That is the only thing keeping us from removing the HoL blocking in vhost-net zerocopy.
>
> We don't enable network watchdog on virtio but we could and maybe
> should.
Can you elaborate?
^ 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-23 22:57 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Koichiro Den, virtualization
In-Reply-To: <CAF=yD-+U_aWxSmPPY8v8t=JO0MRZ+N+DzVJTAMKYQ5F=pE1PfA@mail.gmail.com>
On Wed, Aug 23, 2017 at 11:20:45AM -0400, Willem de Bruijn wrote:
> > Please let me make sure if I understand it correctly:
> > * always do copy with skb_orphan_frags_rx as Willem mentioned in the earlier
> > post, before the xmit_skb as opposed to my original patch, is safe but too
> > costly so cannot be adopted.
>
> One more point about msg_zerocopy in the guest. This does add new allocation
> limits on optmem and locked pages rlimit.
>
> Hitting these should be extremely rare. The tcp small queues limit normally
> throttles well before this.
>
> Virtio-net is an exception because it breaks the tsq signal by calling
> skb_orphan before transmission.
>
> As a result hitting these limits is more likely here. But, in this edge case the
> sendmsg call will not block, either, but fail with -ENOBUFS. The caller can
> send without zerocopy to make forward progress and
> trigger free_old_xmit_skbs from start_xmit.
>
> > * as a generic solution, if we were to somehow overcome the safety issue, track
> > the delay and do copy if some threshold is reached could be an answer, but it's
> > hard for now.> * so things like the current vhost-net implementation of deciding whether or not
> > to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
> > practical compromise.
>
> The fragility of this mechanism is another argument for switching to tx napi
> as default.
>
> Is there any more data about the windows guest issues when completions
> are not queued within a reasonable timeframe? What is this timescale and
> do we really need to work around this.
I think it's pretty large, many milliseconds.
But I wonder what do you mean by "work around". Using buffers within
limited time frame sounds like a reasonable requirement to me. Neither
do I see why would using tx interrupts within guest be a work around -
AFAIK windows driver uses tx interrupts.
> That is the only thing keeping us from removing the HoL blocking in vhost-net zerocopy.
We don't enable network watchdog on virtio but we could and maybe
should.
--
MST
^ permalink raw reply
* [PATCH] [RFC] virtio: Limit the retries on a virtio device reset
From: Pierre Morel @ 2017-08-23 16:33 UTC (permalink / raw)
To: virtualization; +Cc: cohuck, mst
Reseting a device can sometime fail, even a virtual device.
If the device is not reseted after a while the driver should
abandon the retries.
This is the change proposed for the modern virtio_pci.
More generally, when this happens,the virtio driver can set the
VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
The virtio core can test if the reset was succesful by testing
this flag after a reset.
This behavior is backward compatible with existing drivers.
This behavior seems to me compatible with Virtio-1.0 specifications,
Chapters 2.1 Device Status Field.
There I definitively need your opinion: Is it right?
This patch also lead to another question:
do we care if a device provided by the hypervisor is buggy?
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
drivers/virtio/virtio.c | 4 ++++
drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 48230a5..6255dc4 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
/* We always start by resetting the device, in case a previous
* driver messed it up. This also tests that code path a little. */
dev->config->reset(dev);
+ if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
+ return -EIO;
/* Acknowledge that we've seen the device. */
virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
@@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
/* We always start by resetting the device, in case a previous
* driver messed it up. */
dev->config->reset(dev);
+ if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
+ return -EIO;
/* Acknowledge that we've seen the device. */
virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 2555d80..bfc5fc1 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
static void vp_reset(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ int retry_count = 10;
/* 0 status means a reset. */
vp_iowrite8(0, &vp_dev->common->device_status);
/* After writing 0 to device_status, the driver MUST wait for a read of
@@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
* This will flush out the status write, and flush in device writes,
* including MSI-X interrupts, if any.
*/
- while (vp_ioread8(&vp_dev->common->device_status))
+ while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
msleep(1);
+ /* If the read did not return 0 before the timeout consider that
+ * the device failed.
+ */
+ if (retry_count <= 0) {
+ virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
+ return;
+ }
+ virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
/* Flush pending VQ/configuration callbacks. */
vp_synchronize_vectors(vdev);
}
--
2.3.0
^ 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-23 15:20 UTC (permalink / raw)
To: Koichiro Den; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <1503498504.8694.26.camel@klaipeden.com>
> Please let me make sure if I understand it correctly:
> * always do copy with skb_orphan_frags_rx as Willem mentioned in the earlier
> post, before the xmit_skb as opposed to my original patch, is safe but too
> costly so cannot be adopted.
One more point about msg_zerocopy in the guest. This does add new allocation
limits on optmem and locked pages rlimit.
Hitting these should be extremely rare. The tcp small queues limit normally
throttles well before this.
Virtio-net is an exception because it breaks the tsq signal by calling
skb_orphan before transmission.
As a result hitting these limits is more likely here. But, in this edge case the
sendmsg call will not block, either, but fail with -ENOBUFS. The caller can
send without zerocopy to make forward progress and
trigger free_old_xmit_skbs from start_xmit.
> * as a generic solution, if we were to somehow overcome the safety issue, track
> the delay and do copy if some threshold is reached could be an answer, but it's
> hard for now.> * so things like the current vhost-net implementation of deciding whether or not
> to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
> practical compromise.
The fragility of this mechanism is another argument for switching to tx napi
as default.
Is there any more data about the windows guest issues when completions
are not queued within a reasonable timeframe? What is this timescale and
do we really need to work around this. That is the only thing keeping us from
removing the HoL blocking in vhost-net zerocopy.
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Koichiro Den @ 2017-08-23 14:47 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: Network Development, Willem de Bruijn, virtualization
In-Reply-To: <1503498504.8694.26.camel@klaipeden.com>
On Wed, 2017-08-23 at 23:28 +0900, Koichiro Den wrote:
> On Tue, 2017-08-22 at 20:55 +0300, Michael S. Tsirkin wrote:
> > 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.
> >
>
> Please let me make sure if I understand it correctly:
> * always do copy with skb_orphan_frags_rx as Willem mentioned in the earlier
> post, before the xmit_skb as opposed to my original patch, is safe but too
> costly so cannot be adopted.
> * as a generic solution, if we were to somehow overcome the safety issue,
> track
> the delay and do copy if some threshold is reached could be an answer, but
> it's
> hard for now.
> * so things like the current vhost-net implementation of deciding whether or
> not
> to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
> practical compromise.
<- I forgot to mention the max pend checking part.
>
> 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-23 14:28 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: Network Development, Willem de Bruijn, virtualization
In-Reply-To: <20170822204015-mutt-send-email-mst@kernel.org>
On Tue, 2017-08-22 at 20:55 +0300, Michael S. Tsirkin wrote:
> 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.
>
Please let me make sure if I understand it correctly:
* always do copy with skb_orphan_frags_rx as Willem mentioned in the earlier
post, before the xmit_skb as opposed to my original patch, is safe but too
costly so cannot be adopted.
* as a generic solution, if we were to somehow overcome the safety issue, track
the delay and do copy if some threshold is reached could be an answer, but it's
hard for now.
* so things like the current vhost-net implementation of deciding whether or not
to do zerocopy beforehand referring the zerocopy tx error ratio is a point of
practical compromise.
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-23 14:26 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-JyLEUgUgeH5TjRUj2wz9CjRkAx1HUyXRxCDyT5iHx-sg@mail.gmail.com>
On Tue, 2017-08-22 at 13:19 -0400, Willem de Bruijn wrote:
> > > > > /* 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.
> >
Thanks for the explanation, I misinterpreted the "nic" part, now it's clear.
Sorry about bothering.
_______________________________________________
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-23 14:24 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin
In-Reply-To: <CAF=yD-+AOnYafiwAf+v+fhyg_0fi-6LdQSnPYcoAcngJqYs9dg@mail.gmail.com>
On Tue, 2017-08-22 at 13:16 -0400, Willem de Bruijn wrote:
> > > > 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.
Thanks for the wrap-up. I see all the points now and also that C1 should not
cause deadlock.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] [RFC] virtio-iommu version 0.4
From: Auger Eric @ 2017-08-23 13:55 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, robin.murphy,
eric.auger.pro
In-Reply-To: <20170804181927.12148-1-jean-philippe.brucker@arm.com>
Hi Jean-Philippe,
On 04/08/2017 20:19, Jean-Philippe Brucker wrote:
> This is the continuation of my proposal for virtio-iommu, the para-
> virtualized IOMMU. Here is a summary of the changes since last time [1]:
>
> * The virtio-iommu document now resembles an actual specification. It is
> split into a formal description of the virtio device, and implementation
> notes. Please find sources and binaries at [2].
>
> * Added a probe request to describe to the guest different properties that
> do not fit in firmware or in the virtio config space. This is a
> necessary stepping stone for extending the virtio-iommu.
>
> * There is a working Qemu prototype [3], thanks to Eric Auger and Bharat
> Bhushan.
>
> You can find the Linux driver and kvmtool device at [4] and [5]. I
> plan to rework driver and kvmtool device slightly before sending the
> patches.
>
> To understand the virtio-iommu, I advise to first read introduction and
> motivation, then skim through implementation notes and finally look at the
> device specification.
>
> I wasn't sure how to organize the review. For those who prefer to comment
> inline, I attached v0.4 of device-operations.tex and topology.tex+MSI.tex
> to this thread. They are the biggest chunks of the document. But LaTeX
> isn't very pleasant to read, so you can simply send a list of comments in
> relation to section numbers and a few words of context, we'll manage.
Please find some comments/questions below:
2.6.7:1
I do not understand the footnode #6 sentence: 'Without a specific
definition of the ACK requirements for a given property type, it simply
means that the driver read all fields of that property."
2.6.7:2
what if the driver has not provided a big enough buffer and the device
cannot report all supported properties?
2.6.8.2:
Couldn't we use the same terminology as iommu_resv_type in iommu.h?
the negative form is not the easiest to understand for me.
Why F_MSI?
2.6.8.2.1
Multiple overlapping RESV_MEM properties are merged together. Device
requirement? if same types I assume?
what if the device is a physical assigned device. does the device report
the host doorbell or the guest doorbell, may be worth to clarify.
3.1.1 last paragraph
you talk about device ID but this is a terminology only known by ARM I
think. Actually it is introduced later in 3.1.2.
3.1.2
About ACPI integration. Isn't IORT ARM specific? Will other
architectures use that table? In IORT we also have Named Component node
which look pretty the same. Could you elaborate of the difference?
Device Object name: isn't it the path to the LNR0005 in the namespace?
Thanks
Eric
>
> ---
> Version numbers 0.1-0.4 are arbitrary. I'm hoping they allow to compare
> more easily differences since the RFC (see [6]), but haven't been made
> public so far. This is the first public posting since initial proposal
> [1], and the following describes all changes.
>
> ## v0.1 ##
>
> Content is the same as the RFC, but formatted to LaTeX. 'make' generates
> one PDF and one HTML document.
>
> ## v0.2 ##
>
> Add introductions, improve topology example and firmware description based
> on feedback and a number of useful discussions.
>
> ## v0.3 ##
>
> Add normative sections (MUST, SHOULD, etc). Clarify some things, tighten
> the device and driver behaviour. Unmap semantics are consolidated; they
> are now closer to VFIO Type1 v2 semantics.
>
> ## v0.4 ##
>
> Introduce PROBE requests. They provide per-endpoint information to the
> driver that couldn't be described otherwise.
>
> For the moment, they allow to handle MSIs on x86 virtual platforms (see
> 3.2). To do that we communicate reserved IOVA regions, that will also be
> useful for describing regions that cannot be mapped for a given endpoint,
> for instance addresses that correspond to a PCI bridge window.
>
> Introducing such a large framework for this tiny feature may seem
> overkill, but it is needed for future extensions of the virtio-iommu and I
> believe it really is worth the effort.
>
> ## Future ##
>
> Other extensions are in preparation. I won't detail them here because v0.4
> already is a lot to digest, but in short, building on top of PROBE:
>
> * First, since the IOMMU is paravirtualized, the device can expose some
> properties of the physical topology to the guest, and let it allocate
> resources more efficiently. For example, when the virtio-iommu manages
> both physical and emulated endpoints, with different underlying IOMMUs,
> we now have a way to describe multiple page and block granularities,
> instead of forcing the guest to use the most restricted one for all
> endpoints. This will most likely be in v0.5.
>
> * Then on top of that, a major improvement will describe hardware
> acceleration features available to the guest. There is what I call "Page
> Table Handover" (or simply, from the host POV, "Nested"), the ability
> for the guest to manipulate its own page tables instead of sending
> MAP/UNMAP requests to the host. This, along with IO Page Fault
> reporting, will also permit SVM virtualization on different platforms.
>
> Thanks,
> Jean
>
> [1] http://www.spinics.net/lists/kvm/msg147990.html
> [2] git://linux-arm.org/virtio-iommu.git branch viommu/v0.4
> http://www.linux-arm.org/git?p=virtio-iommu.git;a=blob;f=dist/v0.4/virtio-iommu-v0.4.pdf
> I reiterate the disclaimers: don't use this document as a reference,
> it's a draft. It's also not an OASIS document yet. It may be riddled
> with mistakes. As this is a working draft, it is unstable and I do not
> guarantee backward compatibility of future versions.
> [3] https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00004.html
> [4] git://linux-arm.org/linux-jpb.git virtio-iommu/v0.4
> Warning: UAPI headers have changed! They didn't follow the spec,
> please update. (Use branch v0.1, that has the old headers, for the
> Qemu prototype [3])
> [5] git://linux-arm.org/kvmtool-jpb.git virtio-iommu/v0.4
> Warning: command-line has changed! Use --viommu vfio[,opts] and
> --viommu virtio[,opts] to instantiate a device.
> [6] http://www.linux-arm.org/git?p=virtio-iommu.git;a=tree;f=dist/diffs
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
^ permalink raw reply
* Re: [RFC] virtio-iommu version 0.4
From: Jean-Philippe Brucker @ 2017-08-23 10:01 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, eric.auger,
robin.murphy, eric.auger.pro
In-Reply-To: <20170804181927.12148-1-jean-philippe.brucker@arm.com>
On 04/08/17 19:19, Jean-Philippe Brucker wrote:
> Other extensions are in preparation. I won't detail them here because v0.4
> already is a lot to digest, but in short, building on top of PROBE:
>
> * First, since the IOMMU is paravirtualized, the device can expose some
> properties of the physical topology to the guest, and let it allocate
> resources more efficiently. For example, when the virtio-iommu manages
> both physical and emulated endpoints, with different underlying IOMMUs,
> we now have a way to describe multiple page and block granularities,
> instead of forcing the guest to use the most restricted one for all
> endpoints. This will most likely be in v0.5.
In order to extend requests with PASIDs and (later) nested mode, I intend
to rename "address_space" field to "domain", since it is a lot more
precise about what the field is referring to and the current name would
make these extensions confusing. Please find the rationale at [1].
"ioasid_bits" will be "domain_bits" and "VIRTIO_IOMMU_F_IOASID_BITS" will
be "VIRTIO_IOMMU_F_DOMAIN_BITS".
For those that had time to read this version, do you have other comments
and suggestions about v0.4? Otherwise it is the only update I have for
v0.5 (along with fine-grained address range and page size properties from
the quoted text) and I will send it soon.
In particular, please tell me now if you see the need for other
destructive changes like this one. They will be impossible to introduce
once a driver or device is upstream.
Thanks,
Jean
[1] https://www.spinics.net/lists/kvm/msg154573.html
^ permalink raw reply
* Re: [PATCH] net/virtio-net: reverse unregistering on exit
From: Pierre Morel @ 2017-08-23 8:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20170822211736-mutt-send-email-mst@kernel.org>
On 22/08/2017 20:17, Michael S. Tsirkin wrote:
> On Tue, Aug 22, 2017 at 04:59:51PM +0200, Pierre Morel wrote:
>> 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 ]------------
>
> Which kernel do you use?
A too old one , 4.12.0 , I forgot to check on linux-next.
The problem has indeed been solved in between.
Sorry for the noise.
>
>
>> 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
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] net/virtio-net: reverse unregistering on exit
From: Pierre Morel @ 2017-08-23 8:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20170822211245-mutt-send-email-mst@kernel.org>
On 22/08/2017 20:13, Michael S. Tsirkin wrote:
> 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?
hum... yes, sorry
>
>> ---
>> 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
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
_______________________________________________
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: Tian, Kevin @ 2017-08-23 2:23 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,
Auger Eric, robin.murphy@arm.com
In-Reply-To: <03eabc9f-5db1-0eb9-b91a-c47da9d1a29b@arm.com>
> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> Sent: Tuesday, August 22, 2017 10:19 PM
>
> 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.
>
Sounds good to me. Thanks.
^ 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 18:39 UTC (permalink / raw)
To: Eric Dumazet
Cc: willemdebruijn.kernel, netdev, den, virtualization, David Miller
In-Reply-To: <1503426508.2499.47.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Aug 22, 2017 at 11:28:28AM -0700, Eric Dumazet wrote:
> On Tue, 2017-08-22 at 11:01 -0700, David Miller wrote:
> > 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.
>
> TCP retransmit makes sure to avoid that.
>
> if (skb_unclone(skb, GFP_ATOMIC))
> return -ENOMEM;
>
> ( Before cloning again skb )
>
>
I'm pretty sure not all users of skb_clone or generally
__pskb_pull_tail are careful like this.
E.g. skb_cow_data actually intentionally pulls pages when
skb is cloned.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Eric Dumazet @ 2017-08-22 18:28 UTC (permalink / raw)
To: David Miller; +Cc: willemdebruijn.kernel, mst, netdev, den, virtualization
In-Reply-To: <20170822.110108.343109469263087166.davem@davemloft.net>
On Tue, 2017-08-22 at 11:01 -0700, David Miller wrote:
> 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.
TCP retransmit makes sure to avoid that.
if (skb_unclone(skb, GFP_ATOMIC))
return -ENOMEM;
( Before cloning again skb )
^ permalink raw reply
* Re: [PATCH] net/virtio-net: reverse unregistering on exit
From: Michael S. Tsirkin @ 2017-08-22 18:17 UTC (permalink / raw)
To: Pierre Morel; +Cc: virtualization
In-Reply-To: <1503413992-29558-1-git-send-email-pmorel@linux.vnet.ibm.com>
On Tue, Aug 22, 2017 at 04:59:51PM +0200, Pierre Morel wrote:
> 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 ]------------
Which kernel do you use?
> 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] 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
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