virtio-net: remove useless disable on freeze

RFC PATCH Applied Michael S. Tsirkin 4 participants 8 messages Apr 4, 2012 – May 31, 2012

Subsystems (5)

Networking Network drivers KVM virtio core +1 more subsystems

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2012-04-04 09:19 UTC #0 lore raw

RFC PATCH 1/1 virtio-net: remove useless disable on freeze

disable_cb is just an optimization: it can not guarantee that there are no callbacks.

I didn't yet figure out whether a callback in freeze will trigger a bug, but disable_cb won't address it in any case. So let's remove the useless calls as a first step.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/virtio_net.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

drivers/net/virtio_net.c +0 -5

index 019da01..971931e5 100644
@@ -1182,11 +1182,6 @@ static int virtnet_freeze(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
 
-	virtqueue_disable_cb(vi->rvq);
-	virtqueue_disable_cb(vi->svq);
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
-		virtqueue_disable_cb(vi->cvq);
-
 	netif_device_detach(vi->dev);
 	cancel_delayed_work_sync(&vi->refill);

Signature · 1 line

1.7.9.111.gf3fb0

Amit Shah

2012-05-03 10:59 UTC #1 lore raw

Quoted 2 lines · Michael S. Tsirkin

disable_cb is just an optimization: it
can not guarantee that there are no callbacks.

Even then, what's the harm in keeping it? If indeed there's an attempt to raise an interrupt after the host has been notified, it will be suppressed.

Also, disable_cb seems to be used elsewhere in the virtio_net.c file, to suit similar purposes.

Amit

Michael S. Tsirkin

author maintainer

2012-05-03 11:08 UTC #2 lore raw

Quoted 7 lines · Amit Shah

Quoted 2 lines · Michael S. Tsirkin
disable_cb is just an optimization: it
can not guarantee that there are no callbacks.
Even then, what's the harm in keeping it? If indeed there's an attempt to raise an interrupt after the host has been notified, it will be suppressed.

It won't. It's not a guarantee, e.g. with event index on it does nothing at all.

Quoted 4 lines

Also, disable_cb seems to be used elsewhere in the virtio_net.c file, to suit similar purposes.
Amit

Where?

Michael S. Tsirkin

author maintainer

2012-05-28 12:53 UTC #3 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

disable_cb is just an optimization: it can not guarantee that there are no callbacks.
I didn't yet figure out whether a callback in freeze will trigger a bug, but disable_cb won't address it in any case. So let's remove the useless calls as a first step.
Signed-off-by Michael S. Tsirkin <[email protected]>

Looks like this isn't in the 3.5 pull request -
just lost in the shuffle?
disable_cb is advisory so can't be relied upon.

Quoted 22 lines

---
 drivers/net/virtio_net.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

drivers/net/virtio_net.c +0 -5

index 019da01..971931e5 100644
@@ -1182,11 +1182,6 @@ static int virtnet_freeze(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
 
-	virtqueue_disable_cb(vi->rvq);
-	virtqueue_disable_cb(vi->svq);
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ))
-		virtqueue_disable_cb(vi->cvq);
-
 	netif_device_detach(vi->dev);
 	cancel_delayed_work_sync(&vi->refill);

Signature · 1 line

1.7.9.111.gf3fb0

Rusty Russell

Applied

2012-05-30 10:11 UTC #4 lore raw

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin
disable_cb is just an optimization: it can not guarantee that there are no callbacks.
I didn't yet figure out whether a callback in freeze will trigger a bug, but disable_cb won't address it in any case. So let's remove the useless calls as a first step.

Quoted 5 lines

Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>
Looks like this isn't in the 3.5 pull request -
just lost in the shuffle?
disable_cb is advisory so can't be relied upon.

I always (try to?) reply as I accept patches.

This one did slip by, but it's harmless so no need to push AFAICT.

Applied.

Thanks!
Rusty.

Stephen Rothwell

2012-05-31 08:35 UTC #5 lore raw

Hi all,

Quoted 17 lines · On Wed, 30 May 2012 19:41:47 +0930 Rusty Russell <[email protected]> wrote:

Quoted 9 lines · Michael S. Tsirkin
Quoted 8 lines · Michael S. Tsirkin
disable_cb is just an optimization: it can not guarantee that there are no callbacks.
I didn't yet figure out whether a callback in freeze will trigger a bug, but disable_cb won't address it in any case. So let's remove the useless calls as a first step.
Quoted 5 lines
Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>
Looks like this isn't in the 3.5 pull request -
just lost in the shuffle?
disable_cb is advisory so can't be relied upon.

Quoted 5 lines

I always (try to?) reply as I accept patches.
This one did slip by, but it's harmless so no need to push AFAICT.
Applied.

This patch exists in two trees in linux-next already ... Davem's net tree (so presumably he will send it to Linus shortly) and Michael's vhost tree (is that tree needed any more?). Presumably it is now also in the rr tree?

Signature · 2 lines

Cheers,
Stephen Rothwell                    [email protected]

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Michael S. Tsirkin

author maintainer

2012-05-31 08:47 UTC #6 lore raw

Quoted 22 lines · Stephen Rothwell

Hi all,
Quoted 19 lines · On Wed, 30 May 2012 19:41:47 +0930 Rusty Russell <[email protected]> wrote:
Quoted 9 lines · Michael S. Tsirkin
Quoted 8 lines · Michael S. Tsirkin
disable_cb is just an optimization: it can not guarantee that there are no callbacks.
I didn't yet figure out whether a callback in freeze will trigger a bug, but disable_cb won't address it in any case. So let's remove the useless calls as a first step.
Quoted 5 lines
Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>
Looks like this isn't in the 3.5 pull request -
just lost in the shuffle?
disable_cb is advisory so can't be relied upon.
I always (try to?) reply as I accept patches.

Quoted 7 lines

Quoted 3 lines
This one did slip by, but it's harmless so no need to push AFAICT.
Applied.
This patch exists in two trees in linux-next already ... Davem's net tree (so presumably he will send it to Linus shortly) and Michael's vhost tree (is that tree needed any more?).

Yes and I dropped the patch from there, just did not push yet.

Quoted 6 lines

Presumably it is now also in the rr
tree?
Signature · 2 lines
Cheers,
Stephen Rothwell                    [email protected]

Michael S. Tsirkin

author maintainer

2012-05-31 09:04 UTC #7 lore raw

Quoted 25 lines · Michael S. Tsirkin

Quoted 24 lines · Stephen Rothwell
Hi all,
Quoted 21 lines · On Wed, 30 May 2012 19:41:47 +0930 Rusty Russell <[email protected]> wrote:
Quoted 9 lines · Michael S. Tsirkin
Quoted 8 lines · Michael S. Tsirkin
disable_cb is just an optimization: it can not guarantee that there are no callbacks.
I didn't yet figure out whether a callback in freeze will trigger a bug, but disable_cb won't address it in any case. So let's remove the useless calls as a first step.
Quoted 5 lines
Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>
Looks like this isn't in the 3.5 pull request -
just lost in the shuffle?
disable_cb is advisory so can't be relied upon.
I always (try to?) reply as I accept patches.
This one did slip by, but it's harmless so no need to push AFAICT.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Applied.
This patch exists in two trees in linux-next already ... Davem's net tree (so presumably he will send it to Linus shortly) and Michael's vhost tree (is that tree needed any more?).
Yes and I dropped the patch from there, just did not push yet.

pushed.
There's usually not too much in my tree but it helps me a lot that it's in linux-next.

Quoted 8 lines

Quoted 6 lines
Presumably it is now also in the rr
tree?
Signature · 2 lines
Cheers,
Stephen Rothwell                    [email protected]