Make virtio_net support carrier detection

On Saturday 14 March 2009 05:31:00 David Miller wrote:
> You can set netif_carrier_on() until you are blue in the face,
> but until you hook up the ethtool link indication operation
> NetworkManager won't see it.

Um, dude, you already have that patch in your net-next tree.  See below.
This one goes on top.

> I don't understand what all of this hob-knobbing is about,
> Pantelis's patch was perfect, appropriate, and should have
> gone straight in to net-2.6 and probably -stable too.
> 
> Is this some kind of control issue Rusty?  It's the only
> explanation I can come up with :-))

I don't think it was straightforward at all.  Current virtio_net "cards"
don't support carrier detection.  We reported that (correctly) to userspace,
like every other driver which doesn't support carrier detect.

So why is virtio_net different?  Or should all devices which can't detect
carrier set it to on, so older NetworkManagers work?

This may be obvious to you.  It's not to me.
Rusty.

commit 9f4d26d0f3016cf8813977d624751b94465fa317
Author: Mark McLoughlin <markmc@redhat.com>
Date:   Mon Jan 19 17:09:49 2009 -0800

    virtio_net: add link status handling
    
    Allow the host to inform us that the link is down by adding
    a VIRTIO_NET_F_STATUS which indicates that device status is
    available in virtio_net config.
    
    This is currently useful for simulating link down conditions
    (e.g. using proposed qemu 'set_link' monitor command) but
    would also be needed if we were to support device assignment
    via virtio.
    
    Signed-off-by: Mark McLoughlin <markmc@redhat.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (added future masking)
    Signed-off-by: David S. Miller <davem@davemloft.net>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> I don't think it was straightforward at all.  Current virtio_net "cards"
> don't support carrier detection.  We reported that (correctly) to userspace,
> like every other driver which doesn't support carrier detect.
>
> So why is virtio_net different?  Or should all devices which can't detect
> carrier set it to on, so older NetworkManagers work?
>

"The difference between theory and practice is not important in theory
but very important in practice"

The practice is:

1) In real wired network cards, carrier is "off by default" (until you plug
a cable) and for laptops it could well be off most of the time (this
is the case here).
So if a "real hardware" driver reports "on" by default it could be wrong
often enough to cause serious frustration to the user (having to wait for
a timeout possibly).

So, real hardware drivers cannot afford to lie that the carrier is "on".
Only the user can know.

In the server case where carrier is normally "on", you either don't want to
use NetworkManager, or you would configure the link explicitly in NM anyway.


2) In virtio_net, network is "on by default", so the only problem by
reporting "on"
instead of "I have no idea" would be with a future qemu version, *if* the user
does "set_link off" in qemu monitor while using NetworkManager (but using
this feature would require a kernel update anyway comparing to what is
there now). For the extremely common case of someone testing a livecd
of a new distro release in existing qemu/kvm, they get the expected results.


3) There is a real cost of reporting "I have no idea" when the truth is
"it is on unless you do something to turn it off explicitly, oh and you 'll
need a future version of qemu (i.e., the "virtual hardware") for that too".


4) Not supporting carrier detection / reporting is arguably a "virtual
hardware bug".
Just try to sell a real hardware card these days without this feature :)
Not to mention that unlike real hardware, adding such small features to
virtio_net has a near-zero cost.

Working around this "virtual hardware bug" (my patch) while it is being fixed
properly (in both the driver and the hardware at the same time), is reasonable
imho because reporting "on" will be correct 100% of the time with
current versions
of qemu and most of the time with future versions as well.

Mark's commit alone achieves the desired effect of reporting carrier
status correctly
but only in the case of newer (yet unreleased?) qemu because of the

+       if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
+               return;

inside virtnet_update_status(). So, your patch adds the explicit
netif_carrier_on
in the probe function which is essentially replicating the behavior you get with
my patch for current versions of qemu / virtio "hardware", you just have to wait
another 3 months for 2.6.30 (!!) to get this.

So, I still propose my version for 2.6.29 / -stable and then mark's
commit applies
on top of that just fine (just don't delete the netif_carrier_on)  and
adds the feature
of the user being able to set the status to on / off explicitly if
they have a new version
of qemu / virtio while still doing the "right thing" for current versions.

Therefore, I guess I should resend my patch to netdev with reworked comments
to reflect this discussion. Hope that is ok with you too.

Pantelis

>
> commit 9f4d26d0f3016cf8813977d624751b94465fa317
> Author: Mark McLoughlin <markmc@redhat.com>
> Date:   Mon Jan 19 17:09:49 2009 -0800
>
>    virtio_net: add link status handling
>
>    Allow the host to inform us that the link is down by adding
>    a VIRTIO_NET_F_STATUS which indicates that device status is
>    available in virtio_net config.
>
>    This is currently useful for simulating link down conditions
>    (e.g. using proposed qemu 'set_link' monitor command) but
>    would also be needed if we were to support device assignment
>    via virtio.
>
>    Signed-off-by: Mark McLoughlin <markmc@redhat.com>
>    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (added future masking)
>    Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 30ae6d9..9b33d6e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -42,6 +42,7 @@ struct virtnet_info
>        struct virtqueue *rvq, *svq;
>        struct net_device *dev;
>        struct napi_struct napi;
> +       unsigned int status;
>
>        /* The skb we couldn't send because buffers were full. */
>        struct sk_buff *last_xmit_skb;
> @@ -611,6 +612,7 @@ static struct ethtool_ops virtnet_ethtool_ops = {
>        .set_tx_csum = virtnet_set_tx_csum,
>        .set_sg = ethtool_op_set_sg,
>        .set_tso = ethtool_op_set_tso,
> +       .get_link = ethtool_op_get_link,
>  };
>
>  #define MIN_MTU 68
> @@ -636,6 +638,41 @@ static const struct net_device_ops virtnet_netdev = {
>  #endif
>  };
>
> +static void virtnet_update_status(struct virtnet_info *vi)
> +{
> +       u16 v;
> +
> +       if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
> +               return;
> +
> +       vi->vdev->config->get(vi->vdev,
> +                             offsetof(struct virtio_net_config, status),
> +                             &v, sizeof(v));
> +
> +       /* Ignore unknown (future) status bits */
> +       v &= VIRTIO_NET_S_LINK_UP;
> +
> +       if (vi->status == v)
> +               return;
> +
> +       vi->status = v;
> +
> +       if (vi->status & VIRTIO_NET_S_LINK_UP) {
> +               netif_carrier_on(vi->dev);
> +               netif_wake_queue(vi->dev);
> +       } else {
> +               netif_carrier_off(vi->dev);
> +               netif_stop_queue(vi->dev);
> +       }
> +}
> +
> +static void virtnet_config_changed(struct virtio_device *vdev)
> +{
> +       struct virtnet_info *vi = vdev->priv;
> +
> +       virtnet_update_status(vi);
> +}
> +
>  static int virtnet_probe(struct virtio_device *vdev)
>  {
>        int err;
> @@ -738,6 +775,9 @@ static int virtnet_probe(struct virtio_device *vdev)
>                goto unregister;
>        }
>
> +       vi->status = VIRTIO_NET_S_LINK_UP;
> +       virtnet_update_status(vi);
> +
>        pr_debug("virtnet: registered device %s\n", dev->name);
>        return 0;
>
> @@ -793,7 +833,7 @@ static unsigned int features[] = {
>        VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
>        VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
>        VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */
> -       VIRTIO_NET_F_MRG_RXBUF,
> +       VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS,
>        VIRTIO_F_NOTIFY_ON_EMPTY,
>  };
>
> @@ -805,6 +845,7 @@ static struct virtio_driver virtio_net = {
>        .id_table =     id_table,
>        .probe =        virtnet_probe,
>        .remove =       __devexit_p(virtnet_remove),
> +       .config_changed = virtnet_config_changed,
>  };
>
>  static int __init init(void)
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 5cdd0aa..f76bd4a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -21,11 +21,16 @@
>  #define VIRTIO_NET_F_HOST_ECN  13      /* Host can handle TSO[6] w/ ECN in. */
>  #define VIRTIO_NET_F_HOST_UFO  14      /* Host can handle UFO in. */
>  #define VIRTIO_NET_F_MRG_RXBUF 15      /* Host can merge receive buffers. */
> +#define VIRTIO_NET_F_STATUS    16      /* virtio_net_config.status available */
> +
> +#define VIRTIO_NET_S_LINK_UP   1       /* Link is up */
>
>  struct virtio_net_config
>  {
>        /* The config defining mac address (if VIRTIO_NET_F_MAC) */
>        __u8 mac[6];
> +       /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
> +       __u16 status;
>  } __attribute__((packed));
>
>  /* This is the first element of the scatter-gather list.  If you don't
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Mar 14, 2009 at 12:40 PM, Pantelis Koukousoulas
<pktoss@gmail.com> wrote:
>> I don't think it was straightforward at all.  Current virtio_net "cards"
>> don't support carrier detection.  We reported that (correctly) to userspace,
>> like every other driver which doesn't support carrier detect.
>>
>> So why is virtio_net different?  Or should all devices which can't detect
>> carrier set it to on, so older NetworkManagers work?
>>
>
[snip]
>
> So, I still propose my version for 2.6.29 / -stable and then mark's
> commit applies
> on top of that just fine (just don't delete the netif_carrier_on)  and
> adds the feature
> of the user being able to set the status to on / off explicitly if
> they have a new version
> of qemu / virtio while still doing the "right thing" for current versions.
>
> Therefore, I guess I should resend my patch to netdev with reworked comments
> to reflect this discussion. Hope that is ok with you too.
>
> Pantelis

Or, we can merge the extra netif_carrier_on (what your patch did) to
mark's commit
and just add that full thing to 2.6.29 (as 2 patches probably because
of git, but
one next to the other please for bisection / readability reasons).

I just tested this solution with a self-made livecd, it applies
cleanly and works fine :)
(it might be ~42 lines instead of 2 but it is still trivial enough to
be confident that
nothing will break).

This way we get the expected result for every combination of current and future
versions of qemu / kernel without ever having one "lie" to the other or innocent
users becoming frustrated.

Again:
 (1) My version: smallest, only takes into account current "hardware",
might lead
   to a surprise for someone that uses a future qemu with 2.6.29, does
set_link off
   and expects to see "operation not supported" in ethtool instead of
"Link detected: yes".

 (2) Mark's commit: proper implementation (does both the hardware and
driver part),
       but will only work right for future qemu versions.

 (3) The 2 merged: work for all combinations of current and future
kernel / qemu.
   42 lines instead of 2 but just as unlikely to cause breakage.


I 'd be happy with either (3) in 2.6.29, or (1) in 2.6.29 and (2) in 2.6.30.

Just please don't select to "do nothing". That would be wrong :)

Pantelis
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Saturday 14 March 2009 21:10:43 Pantelis Koukousoulas wrote:
> Therefore, I guess I should resend my patch to netdev with reworked comments
> to reflect this discussion. Hope that is ok with you too.

Already sent to Linus and -stable, with Dave's Ack.

Tho he hasn't applied it (yet?)

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Mar 16, 2009 at 12:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Saturday 14 March 2009 21:10:43 Pantelis Koukousoulas wrote:
>> Therefore, I guess I should resend my patch to netdev with reworked comments
>> to reflect this discussion. Hope that is ok with you too.
>
> Already sent to Linus and -stable, with Dave's Ack.
>
> Tho he hasn't applied it (yet?)
>
> Thanks,
> Rusty.
>

Great, I 'll just wait then :)

Thanks,
Pantelis
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Sat, 14 Mar 2009 10:49:42 +1030

> On Saturday 14 March 2009 05:31:00 David Miller wrote:
> > You can set netif_carrier_on() until you are blue in the face,
> > but until you hook up the ethtool link indication operation
> > NetworkManager won't see it.
> 
> Um, dude, you already have that patch in your net-next tree.  See below.
> This one goes on top.

Ahh, I didn't comprehend this bit.

I overreacted, that's for sure, sorry.

> Or should all devices which can't detect carrier set it to on, so
> older NetworkManagers work?

It is my opinion that this is the only correct behavior.

If you don't know, it's on.  Because "on" means "usable".  And you
can't fault NetworkManager or any other tool for making that kind of
decision.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday 16 March 2009 13:28:06 David Miller wrote:
> > Or should all devices which can't detect carrier set it to on, so
> > older NetworkManagers work?
> 
> It is my opinion that this is the only correct behavior.
> 
> If you don't know, it's on.  Because "on" means "usable".  And you
> can't fault NetworkManager or any other tool for making that kind of
> decision.

Yes, I agree that NM has to assume unknown == on.  But I still think the
kernel is right as it is to return -EOPNOTSUPP and let userspace deal with it.

"Let's fix broken userspace in the kernel" arguments make me nervous ;)

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 16 Mar 2009 13:43:13 +1030

> On Monday 16 March 2009 13:28:06 David Miller wrote:
> > > Or should all devices which can't detect carrier set it to on, so
> > > older NetworkManagers work?
> > 
> > It is my opinion that this is the only correct behavior.
> > 
> > If you don't know, it's on.  Because "on" means "usable".  And you
> > can't fault NetworkManager or any other tool for making that kind of
> > decision.
> 
> Yes, I agree that NM has to assume unknown == on.  But I still think the
> kernel is right as it is to return -EOPNOTSUPP and let userspace deal with it.
> 
> "Let's fix broken userspace in the kernel" arguments make me nervous ;)

This is right from one perspective.

But from another, -EOPNOTSUPP means "half-written driver, kick maintainer"
:-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html