[PATCH 0/4] tools: propagate bridge MTU to vif frontends

[PATCH 0/4] tools: propagate bridge MTU to vif frontends

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This is long missing functionality

Paul Durrant (4):
  tools/hotplug: add remove_from_bridge() and improve debug output
  tools/hotplug: combine add/online and remove/offline in vif-bridge...
  public/io/netif: specify MTU override node
  tools/hotplug: modify set_mtu() to inform the frontend via xenstore

 tools/hotplug/Linux/vif-bridge            | 20 +++------
 tools/hotplug/Linux/xen-network-common.sh | 51 +++++++++++++++++++----
 xen/include/public/io/netif.h             | 12 ++++++
 3 files changed, 60 insertions(+), 23 deletions(-)

-- 
2.20.1

[PATCH 1/4] tools/hotplug: add remove_from_bridge() and improve debug output

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This patch adds a remove_from_bridge() function into xen-network-common.sh
to partner with the existing add_to_bridge() function. The code in
add_to_bridge() is also slightly re-arranged to avoid duplication calls of
'ip link'.

Both add_to_bridge() and remove_from_bridge() will check if their bridge
manipulation operations are necessary and emit a log message if they are not.

NOTE: A call to remove_from_bridge() will be added by a subsequent patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/xen-network-common.sh | 37 ++++++++++++++++++-----
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 8dd3a62068..37e71cfa9c 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -126,19 +126,40 @@ add_to_bridge () {
     local bridge=$1
     local dev=$2
 
-    # Don't add $dev to $bridge if it's already on a bridge.
-    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
-	ip link set dev ${dev} up || true
-	return
-    fi
-    if which brctl >&/dev/null; then
-        brctl addif ${bridge} ${dev}
+    # Don't add $dev to $bridge if it's already on the bridge.
+    if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
+	log debug "adding $dev to bridge $bridge"
+	if which brctl >&/dev/null; then
+            brctl addif ${bridge} ${dev}
+	else
+            ip link set ${dev} master ${bridge}
+	fi
     else
-        ip link set ${dev} master ${bridge}
+	log debug "$dev already on bridge $bridge"
     fi
+
     ip link set dev ${dev} up
 }
 
+remove_from_bridge () {
+    local bridge=$1
+    local dev=$2
+
+    ip link set dev ${dev} down || :
+
+    # Don't remove $dev from $bridge if it's not on the bridge.
+    if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
+	log debug "removing $dev from bridge $bridge"
+	if which brctl >&/dev/null; then
+            brctl delif ${bridge} ${dev}
+	else
+            ip link set ${dev} nomaster
+	fi
+    else
+	log debug "$dev not on bridge $bridge"
+    fi
+}
+
 # Usage: set_mtu bridge dev
 set_mtu () {
     local bridge=$1
-- 
2.20.1

[PATCH 2/4] tools/hotplug: combine add/online and remove/offline in vif-bridge...

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

... as they are in vif-route.

The script is invoked with online/offline for vifs and add/remove for taps.
The operations that are necessary, however, are the same in both cases. This
patch therefore combines the cases.

The open-coded bridge removal code is also replaced with call to
remove_from_bridge().

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/vif-bridge | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
index e722090ca8..e1d7c49788 100644
--- a/tools/hotplug/Linux/vif-bridge
+++ b/tools/hotplug/Linux/vif-bridge
@@ -77,25 +77,17 @@ then
 fi
 
 case "$command" in
+    add)
+        ;&
     online)
         setup_virtual_bridge_port "$dev"
         set_mtu "$bridge" "$dev"
         add_to_bridge "$bridge" "$dev"
         ;;
-
+    remove)
+        ;&
     offline)
-        if which brctl >&/dev/null; then
-            do_without_error brctl delif "$bridge" "$dev"
-        else
-            do_without_error ip link set "$dev" nomaster
-        fi
-        do_without_error ifconfig "$dev" down
-        ;;
-
-    add)
-        setup_virtual_bridge_port "$dev"
-        set_mtu "$bridge" "$dev"
-        add_to_bridge "$bridge" "$dev"
+        remove_from_bridge "$bridge" "$dev"
         ;;
 esac
 
-- 
2.20.1

[PATCH 3/4] public/io/netif: specify MTU override node

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

There is currently no documentation to state what MTU a frontend should
adertise to its network stack. It has however long been assumed that the
default value of 1500 is correct.

This patch specifies a mechanism to allow the tools to set the MTU via a
xenstore node in the frontend area and states that the absence of that node
means the frontend should assume an MTU of 1500 octets.

NOTE: The Windows PV frontend has used an MTU sampled from the xenstore
      node specified in this patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Juergen Gross <jgross@suse.com>
---
 xen/include/public/io/netif.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 9fcf91a2fe..00dd258712 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -204,6 +204,18 @@
  * present).
  */
 
+/*
+ * MTU
+ * ===
+ *
+ * The toolstack may set a value of MTU for the frontend by setting the
+ * /local/domain/<domid>/device/vif/<vif>/mtu node with the MTU value in
+ * octets. If this node is absent the frontend should assume an MTU value
+ * of 1500 octets. A frontend is also at liberty to ignore this value so
+ * it is only suitable for informing the frontend that a packet payload
+ * >1500 octets is permitted.
+ */
+
 /*
  * Hash types
  * ==========
-- 
2.20.1

[PATCH 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

set_mtu() currently sets the backend vif MTU but does not inform the frontend
what it is. This patch adds code to write the MTU into a xenstore node. See
netif.h for a specification of the node.

NOTE: There is also a small modification replacing '$mtu' with '${mtu}'
      for style consistency.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 tools/hotplug/Linux/vif-bridge            |  2 +-
 tools/hotplug/Linux/xen-network-common.sh | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
index e1d7c49788..b99cc82a21 100644
--- a/tools/hotplug/Linux/vif-bridge
+++ b/tools/hotplug/Linux/vif-bridge
@@ -81,7 +81,7 @@ case "$command" in
         ;&
     online)
         setup_virtual_bridge_port "$dev"
-        set_mtu "$bridge" "$dev"
+        set_mtu "$bridge" "$dev" "$type_if"
         add_to_bridge "$bridge" "$dev"
         ;;
     remove)
diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 37e71cfa9c..24fc42d9cf 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -164,9 +164,21 @@ remove_from_bridge () {
 set_mtu () {
     local bridge=$1
     local dev=$2
+    local type_if=$3
+
     mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
     if [ -n "$mtu" ] && [ "$mtu" -gt 0 ]
     then
-            ip link set dev ${dev} mtu $mtu || :
+            ip link set dev ${dev} mtu ${mtu} || :
+    fi
+
+    if [ ${type_if} = vif ]
+    then
+       dev_=${dev#vif}
+       domid=${dev_%.*}
+       devid=${dev_#*.}
+
+       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
+       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
     fi
 }
-- 
2.20.1

Re: [PATCH 3/4] public/io/netif: specify MTU override node

[Jürgen Groß]

On 30.07.20 21:48, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> There is currently no documentation to state what MTU a frontend should
> adertise to its network stack. It has however long been assumed that the
> default value of 1500 is correct.
> 
> This patch specifies a mechanism to allow the tools to set the MTU via a
> xenstore node in the frontend area and states that the absence of that node
> means the frontend should assume an MTU of 1500 octets.
> 
> NOTE: The Windows PV frontend has used an MTU sampled from the xenstore
>        node specified in this patch.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Can you please update docs/misc/xenstore-paths.pandoc accordingly?
With that you can have my:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

RE: [PATCH 3/4] public/io/netif: specify MTU override node

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 03 August 2020 06:09
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>
> Subject: RE: [EXTERNAL] [PATCH 3/4] public/io/netif: specify MTU override node
> 
> On 30.07.20 21:48, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > There is currently no documentation to state what MTU a frontend should
> > adertise to its network stack. It has however long been assumed that the
> > default value of 1500 is correct.
> >
> > This patch specifies a mechanism to allow the tools to set the MTU via a
> > xenstore node in the frontend area and states that the absence of that node
> > means the frontend should assume an MTU of 1500 octets.
> >
> > NOTE: The Windows PV frontend has used an MTU sampled from the xenstore
> >        node specified in this patch.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> Can you please update docs/misc/xenstore-paths.pandoc accordingly?

Sure. Given the path is for use by tools then it should indeed be documented there as well.

> With that you can have my:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 

Thanks,

  Paul

> 
> Juergen