Bug 525890

The virtio-net patches are merged upstream now. Maybe we want to pull in all of the updates instead of just the three requested ones?

The commit ids of the patches mentioned previously are


3c1b27d5043086a485f8526353ae9fe37bfa1065
48925e372f04f5e35fec6269127c62b2c71ab794
0aea51c37fc5868cd723f670af9056c2ef694fee


From the other commits, I think we should definitely pull the virtio_blk commit by Christoph Hellwig (f1b0ef062602713c2c7cfa12362d5d90ed01c5f6).

I'm just listing out the other virtio-related commits below. No preference either way.

commit b3f24698a7faa6e9d8a14124cfdc25353fc8ca19
Author: Rusty Russell <rusty.au>
Date:   Thu Sep 24 09:59:19 2009 -0600

    virtio_net: formalize skb_vnet_hdr
    

commit b0c39dbdc204006ef3558a66716ff09797619778
Author: Rusty Russell <rusty.au>
Date:   Thu Sep 24 09:59:19 2009 -0600

    virtio_net: don't free buffers in xmit ring

commit 8958f574dbe7e41cc54df0df1accc861bb9f6be8
Author: Rusty Russell <rusty.au>
Date:   Thu Sep 24 09:59:18 2009 -0600

    virtio_net: return NETDEV_TX_BUSY instead of queueing an extra skb.

commit 2b5bbe3b8bee8b38bdc27dd9c0270829b6eb7eeb
Author: Rusty Russell <rusty.au>
Date:   Thu Sep 24 09:59:17 2009 -0600

    virtio_net: skb_orphan() and nf_reset() in xmit path.

commit f1b0ef062602713c2c7cfa12362d5d90ed01c5f6
Author: Christoph Hellwig <hch>
Date:   Thu Sep 17 19:57:42 2009 +0200

    virtio_blk: add support for cache flush

ommit 3ca4f5ca73057a617f9444a91022d7127041970a
Author: Fernando Luis Vazquez Cao <fernando.co.jp>
Date:   Fri Jul 31 15:25:56 2009 +0900

    virtio: add virtio IDs file

commit 3a20210dc26bbfff3bbb48bb22d2846240b71d8f
Author: Fernando Luis Vazquez Cao <fernando.co.jp>
Date:   Fri Jul 31 12:09:05 2009 +0900

    virtio: get rid of redundant VIRTIO_ID_9P definition

commit f68d24082e22ccee3077d11aeb6dc5354f0ca7f1
Author: Rusty Russell <rusty.au>
Date:   Wed Sep 23 22:26:29 2009 -0600

    virtio_pci: minor MSI-X cleanups

Okay, I just looked at:

git log v2.6.31/..linus/master drivers/virtio/ drivers/net/virtio_net.c drivers/block/virtio_blk.c 

and here's my take on it:

---
virtio-net: Allow UFO feature to be set and advertised.

  needs:
   udpv6: Handle large incoming UDP/IPv6 packets and support software UFO
   udpv4: Handle large incoming UDP/IPv4 packets and support software UFO

  It would be an performance improvement for bulk UDP transfers, but
  I'd prefer to see an explicit RFE for UFO.

netdev: convert pseudo drivers to netdev_tx_t

  needs:
   netdev: change transmit to limited range type

  No great benefit to this.

netdev: drivers should make ethtool_ops const
const: make block_device_operations const

  No great benefit to either of these.

virtio_pci: minor MSI-X cleanups

  No great benefit to this either; fairly substantial patch. Would
  only pull this in if it makes backporting another patch easier.

virtio: add virtio IDs file

  No benefit

virtio_blk: add support for cache flush

  We don't have support for this in the F-12 version of qemu. I'd
  prefer to see this split out into a separate RFE and pull it in
  explicitly (post F-12 GA) if/when we update F-12 to qemu-0.12.

virtio_net: formalize skb_vnet_hdr

  This is just Rusty cleaning the mergeable receive buffers support up
  a bit. No great benefit apart from easing backporting.

virtio_net: skb_orphan() and nf_reset() in xmit path
virtio_net: return NETDEV_TX_BUSY instead of queueing an extra skb
virtio_net: don't free buffers in xmit ring

  This code is complex and we've seen bugs related to it before in
  Fedora. I'm worried about the complexity here and whether it depends
  on other changes in 2.6.32. I'm also not entirely clear what the big
  benefit for us would be. I'd like some input from Herbert before
  pulling this in.

virtio: make add_buf return capacity remaining

  Seems to allow a minor performance improvement whereby we don't
  re-check the ring for available space. Dubious that this is of
  more than theoretical benefit, and the logic changes are tricky
  enough that I wouldn't be sure it's zero-risk.

virtio_net: avoid (most) NETDEV_TX_BUSY by stopping queue early.
virtio_net: Check for room in the vq before adding buffer

  Looks related to both of the above. Hairy stuff. Not entirely clear
  it would be a huge benefit.

---

Summary:

  - UFO support would be nice to have, but involves core networking bits.
    I'd like to see an explicit RFE for this

  - We do want VIRTIO_BLK_F_FLUSH for qemu-0.12, but I think it's best to
    split that out into a separate bug and deal with it post GA

  - Amit's main request is about optimizing how virtio_net handles the
    ring full condition. I might be missing something, but it looks to
    me like it would be a fairly small speedup on a relatively little
    hit condition (from my profiling, at least). The logic changes are
    complex, in areas that we've had bugs before and, at a glance, I'm
    not 100% sure they're not tied up in core networking changes around
    NETDEV_TX_BUSY, nf_reset() and skb_orphan(). My instinct is to not
    pull these in unless we have benchmarks showing a speedup, in which
    case we (e.g. Herbert :-) should take a look at the entangled changes

just my €0.02c

(In reply to comment #3)

> virtio_blk: add support for cache flush
> 
>   We don't have support for this in the F-12 version of qemu. I'd
>   prefer to see this split out into a separate RFE and pull it in
>   explicitly (post F-12 GA) if/when we update F-12 to qemu-0.12.

This makes a virtio using F12 guest safe for the default qemu cache mode once qemu is fixed. If and old qemu is used it does not cause any chance.

A clear vote for yes from me as this enables F12 to actually be used safely with virtio on any host.  Getting this out ASAP is a good thing.

(In reply to comment #4)
> (In reply to comment #3)
> 
> > virtio_blk: add support for cache flush
> > 
> >   We don't have support for this in the F-12 version of qemu. I'd
> >   prefer to see this split out into a separate RFE and pull it in
> >   explicitly (post F-12 GA) if/when we update F-12 to qemu-0.12.
> 
> This makes a virtio using F12 guest safe for the default qemu cache mode once
> qemu is fixed. If and old qemu is used it does not cause any chance.
> 
> A clear vote for yes from me as this enables F12 to actually be used safely
> with virtio on any host.  Getting this out ASAP is a good thing. 

Actually, yes - we shouldn't wait until qemu in F12 has support for this; e.g. the kernel on F12 GA install images should include this for guest installs

Filed as bug #526869

(In reply to comment #3)

> virtio_net: skb_orphan() and nf_reset() in xmit path
> virtio_net: return NETDEV_TX_BUSY instead of queueing an extra skb
> virtio_net: don't free buffers in xmit ring
> 
>   This code is complex and we've seen bugs related to it before in
>   Fedora. I'm worried about the complexity here and whether it depends
>   on other changes in 2.6.32. I'm also not entirely clear what the big
>   benefit for us would be. I'd like some input from Herbert before
>   pulling this in.

There was at least one regression from this stuff:

  http://bugzilla.kernel.org/show_bug.cgi?id=14378

Okay, i don't think there are any objections to waiting until 2.6.32 for this stuff, so closing