| Message ID | 12441.1521709552@nyx |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] virtio-net: Fix operstate for virtio when no VIRTIO_NET_F_STATUS | expand |
On Thu, Mar 22, 2018 at 09:05:52AM +0000, Jay Vosburgh wrote: > The operstate update logic will leave an interface in the > default UNKNOWN operstate if the interface carrier state never changes > from the default carrier up state set at creation. This includes the > case of an explicit call to netif_carrier_on, as the carrier on to on > transition has no effect on operstate. > > This affects virtio-net for the case that the virtio peer does > not support VIRTIO_NET_F_STATUS (the feature that provides carrier state > updates). Without this feature, the virtio specification states that > "the link should be assumed active," so, logically, the operstate should > be UP instead of UNKNOWN. This has impact on user space applications > that use the operstate to make availability decisions for the interface. > > Resolve this by changing the virtio probe logic slightly to call > netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS > cases, and then the existing call to netif_carrier_on for the "without" > case will cause an operstate transition. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Ben Hutchings <ben@decadent.org.uk> > Fixes: 167c25e4c550 ("virtio-net: init link state correctly") I'd say that's an abuse of this notation. openstate was UNKNOWN even before that fix. > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > > I considered resolving this by changing linkwatch_init_dev to > unconditionally call rfc2863_policy, as that would always set operstate > for all interfaces. > > This would not have any impact on most cases (as most drivers > call netif_carrier_off during probe), except for the loopback device, > which currently has an operstate of UNKNOWN (because it never does any > carrier state transitions). This change would add a round trip on the > dev_base_lock for every loopback device creation, which could have a > negative impact when creating many loopback devices, e.g., when > concurrently creating large numbers of containers. > > > 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 23374603e4d9..7b187ec7411e 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -2857,8 +2857,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > /* Assume link up if device can't report link status, > otherwise get link status from config. */ > + netif_carrier_off(dev); > if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) { > - netif_carrier_off(dev); > schedule_work(&vi->config_work); > } else { > vi->status = VIRTIO_NET_S_LINK_UP; > -- > 2.14.1
Michael S. Tsirkin <mst@redhat.com> wrote: >On Thu, Mar 22, 2018 at 09:05:52AM +0000, Jay Vosburgh wrote: >> The operstate update logic will leave an interface in the >> default UNKNOWN operstate if the interface carrier state never changes >> from the default carrier up state set at creation. This includes the >> case of an explicit call to netif_carrier_on, as the carrier on to on >> transition has no effect on operstate. >> >> This affects virtio-net for the case that the virtio peer does >> not support VIRTIO_NET_F_STATUS (the feature that provides carrier state >> updates). Without this feature, the virtio specification states that >> "the link should be assumed active," so, logically, the operstate should >> be UP instead of UNKNOWN. This has impact on user space applications >> that use the operstate to make availability decisions for the interface. >> >> Resolve this by changing the virtio probe logic slightly to call >> netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS >> cases, and then the existing call to netif_carrier_on for the "without" >> case will cause an operstate transition. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: Ben Hutchings <ben@decadent.org.uk> >> Fixes: 167c25e4c550 ("virtio-net: init link state correctly") > >I'd say that's an abuse of this notation. openstate was UNKNOWN >even before that fix. I went back to the commit that added the dependency on VIRTIO_NET_F_STATUS (and that this patch would thus apply on top of). If that's an issue, I can resubmit without it. -J >> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > >Acked-by: Michael S. Tsirkin <mst@redhat.com> > > >> --- >> >> I considered resolving this by changing linkwatch_init_dev to >> unconditionally call rfc2863_policy, as that would always set operstate >> for all interfaces. >> >> This would not have any impact on most cases (as most drivers >> call netif_carrier_off during probe), except for the loopback device, >> which currently has an operstate of UNKNOWN (because it never does any >> carrier state transitions). This change would add a round trip on the >> dev_base_lock for every loopback device creation, which could have a >> negative impact when creating many loopback devices, e.g., when >> concurrently creating large numbers of containers. >> >> >> 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 23374603e4d9..7b187ec7411e 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -2857,8 +2857,8 @@ static int virtnet_probe(struct virtio_device *vdev) >> >> /* Assume link up if device can't report link status, >> otherwise get link status from config. */ >> + netif_carrier_off(dev); >> if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) { >> - netif_carrier_off(dev); >> schedule_work(&vi->config_work); >> } else { >> vi->status = VIRTIO_NET_S_LINK_UP; >> -- >> 2.14.1
On Thu, Mar 22, 2018 at 12:02:10PM +0000, Jay Vosburgh wrote: > Michael S. Tsirkin <mst@redhat.com> wrote: > > >On Thu, Mar 22, 2018 at 09:05:52AM +0000, Jay Vosburgh wrote: > >> The operstate update logic will leave an interface in the > >> default UNKNOWN operstate if the interface carrier state never changes > >> from the default carrier up state set at creation. This includes the > >> case of an explicit call to netif_carrier_on, as the carrier on to on > >> transition has no effect on operstate. > >> > >> This affects virtio-net for the case that the virtio peer does > >> not support VIRTIO_NET_F_STATUS (the feature that provides carrier state > >> updates). Without this feature, the virtio specification states that > >> "the link should be assumed active," so, logically, the operstate should > >> be UP instead of UNKNOWN. This has impact on user space applications > >> that use the operstate to make availability decisions for the interface. > >> > >> Resolve this by changing the virtio probe logic slightly to call > >> netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS > >> cases, and then the existing call to netif_carrier_on for the "without" > >> case will cause an operstate transition. > >> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >> Cc: Jason Wang <jasowang@redhat.com> > >> Cc: Ben Hutchings <ben@decadent.org.uk> > >> Fixes: 167c25e4c550 ("virtio-net: init link state correctly") > > > >I'd say that's an abuse of this notation. openstate was UNKNOWN > >even before that fix. > > I went back to the commit that added the dependency on > VIRTIO_NET_F_STATUS (and that this patch would thus apply on top of). > If that's an issue, I can resubmit without it. > > -J The patch can be trivially backported to any version that has virtio. The issue was present since the original version of virtio. VIRTIO_NET_F_STATUS fixed it for new devices. So the tag is incorrectly blaming a partial fix for not being a full one. Also, I think it's more appropriate for net-next - it's a minor ABI change (previously presence of VIRTIO_NET_F_STATUS could be detected by looking at operstate, now it can't). Hopefully this makes more apps work than it breaks. So yes, pls repost without Fixes and with net-next unless davem can make the change himself. > >> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > > > >Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > > >> --- > >> > >> I considered resolving this by changing linkwatch_init_dev to > >> unconditionally call rfc2863_policy, as that would always set operstate > >> for all interfaces. > >> > >> This would not have any impact on most cases (as most drivers > >> call netif_carrier_off during probe), except for the loopback device, > >> which currently has an operstate of UNKNOWN (because it never does any > >> carrier state transitions). This change would add a round trip on the > >> dev_base_lock for every loopback device creation, which could have a > >> negative impact when creating many loopback devices, e.g., when > >> concurrently creating large numbers of containers. > >> > >> > >> 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 23374603e4d9..7b187ec7411e 100644 > >> --- a/drivers/net/virtio_net.c > >> +++ b/drivers/net/virtio_net.c > >> @@ -2857,8 +2857,8 @@ static int virtnet_probe(struct virtio_device *vdev) > >> > >> /* Assume link up if device can't report link status, > >> otherwise get link status from config. */ > >> + netif_carrier_off(dev); > >> if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) { > >> - netif_carrier_off(dev); > >> schedule_work(&vi->config_work); > >> } else { > >> vi->status = VIRTIO_NET_S_LINK_UP; > >> -- > >> 2.14.1
Michael S. Tsirkin <mst@redhat.com> wrote: >On Thu, Mar 22, 2018 at 12:02:10PM +0000, Jay Vosburgh wrote: >> Michael S. Tsirkin <mst@redhat.com> wrote: >> >> >On Thu, Mar 22, 2018 at 09:05:52AM +0000, Jay Vosburgh wrote: >> >> The operstate update logic will leave an interface in the >> >> default UNKNOWN operstate if the interface carrier state never changes >> >> from the default carrier up state set at creation. This includes the >> >> case of an explicit call to netif_carrier_on, as the carrier on to on >> >> transition has no effect on operstate. >> >> >> >> This affects virtio-net for the case that the virtio peer does >> >> not support VIRTIO_NET_F_STATUS (the feature that provides carrier state >> >> updates). Without this feature, the virtio specification states that >> >> "the link should be assumed active," so, logically, the operstate should >> >> be UP instead of UNKNOWN. This has impact on user space applications >> >> that use the operstate to make availability decisions for the interface. >> >> >> >> Resolve this by changing the virtio probe logic slightly to call >> >> netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS >> >> cases, and then the existing call to netif_carrier_on for the "without" >> >> case will cause an operstate transition. >> >> >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> >> Cc: Jason Wang <jasowang@redhat.com> >> >> Cc: Ben Hutchings <ben@decadent.org.uk> >> >> Fixes: 167c25e4c550 ("virtio-net: init link state correctly") >> > >> >I'd say that's an abuse of this notation. openstate was UNKNOWN >> >even before that fix. >> >> I went back to the commit that added the dependency on >> VIRTIO_NET_F_STATUS (and that this patch would thus apply on top of). >> If that's an issue, I can resubmit without it. >> >> -J > >The patch can be trivially backported to any version that has virtio. > >The issue was present since the original version of virtio. >VIRTIO_NET_F_STATUS fixed it for new devices. >So the tag is incorrectly blaming a partial fix for not being a full >one. > >Also, I think it's more appropriate for net-next - it's a >minor ABI change (previously presence of VIRTIO_NET_F_STATUS >could be detected by looking at operstate, now it can't). >Hopefully this makes more apps work than it breaks. > >So yes, pls repost without Fixes and with net-next unless >davem can make the change himself. Reposting with requested changes. -J >> >> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> >> > >> >Acked-by: Michael S. Tsirkin <mst@redhat.com> >> > >> > >> >> --- >> >> >> >> I considered resolving this by changing linkwatch_init_dev to >> >> unconditionally call rfc2863_policy, as that would always set operstate >> >> for all interfaces. >> >> >> >> This would not have any impact on most cases (as most drivers >> >> call netif_carrier_off during probe), except for the loopback device, >> >> which currently has an operstate of UNKNOWN (because it never does any >> >> carrier state transitions). This change would add a round trip on the >> >> dev_base_lock for every loopback device creation, which could have a >> >> negative impact when creating many loopback devices, e.g., when >> >> concurrently creating large numbers of containers. >> >> >> >> >> >> 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 23374603e4d9..7b187ec7411e 100644 >> >> --- a/drivers/net/virtio_net.c >> >> +++ b/drivers/net/virtio_net.c >> >> @@ -2857,8 +2857,8 @@ static int virtnet_probe(struct virtio_device *vdev) >> >> >> >> /* Assume link up if device can't report link status, >> >> otherwise get link status from config. */ >> >> + netif_carrier_off(dev); >> >> if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) { >> >> - netif_carrier_off(dev); >> >> schedule_work(&vi->config_work); >> >> } else { >> >> vi->status = VIRTIO_NET_S_LINK_UP; >> >> -- >> >> 2.14.1
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 23374603e4d9..7b187ec7411e 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2857,8 +2857,8 @@ static int virtnet_probe(struct virtio_device *vdev) /* Assume link up if device can't report link status, otherwise get link status from config. */ + netif_carrier_off(dev); if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) { - netif_carrier_off(dev); schedule_work(&vi->config_work); } else { vi->status = VIRTIO_NET_S_LINK_UP;
The operstate update logic will leave an interface in the default UNKNOWN operstate if the interface carrier state never changes from the default carrier up state set at creation. This includes the case of an explicit call to netif_carrier_on, as the carrier on to on transition has no effect on operstate. This affects virtio-net for the case that the virtio peer does not support VIRTIO_NET_F_STATUS (the feature that provides carrier state updates). Without this feature, the virtio specification states that "the link should be assumed active," so, logically, the operstate should be UP instead of UNKNOWN. This has impact on user space applications that use the operstate to make availability decisions for the interface. Resolve this by changing the virtio probe logic slightly to call netif_carrier_off for both the "with" and "without" VIRTIO_NET_F_STATUS cases, and then the existing call to netif_carrier_on for the "without" case will cause an operstate transition. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Cc: Ben Hutchings <ben@decadent.org.uk> Fixes: 167c25e4c550 ("virtio-net: init link state correctly") Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> --- I considered resolving this by changing linkwatch_init_dev to unconditionally call rfc2863_policy, as that would always set operstate for all interfaces. This would not have any impact on most cases (as most drivers call netif_carrier_off during probe), except for the loopback device, which currently has an operstate of UNKNOWN (because it never does any carrier state transitions). This change would add a round trip on the dev_base_lock for every loopback device creation, which could have a negative impact when creating many loopback devices, e.g., when concurrently creating large numbers of containers. drivers/net/virtio_net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)