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

Commit Message

Paul Durrant Aug. 3, 2020, 12:49 p.m. UTC

From: Paul Durrant <[e-mail]>

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 <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
---
 tools/hotplug/Linux/vif-bridge            |  2 +-
 tools/hotplug/Linux/xen-network-common.sh | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

Comments

Ian Jackson Aug. 4, 2020, 11:13 a.m. UTC | #1

Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> From: Paul Durrant <[e-mail]>
> 
> 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 <[e-mail]>

Reviewed-by: Ian Jackson <[e-mail]>

> 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}

It's surprising to me that this code doesn't have the xenbus path
already in some variable.  But I guess from the fact that you've added
this code, that it doesn't.

Ian.

Paul Durrant Aug. 4, 2020, 11:20 a.m. UTC | #2

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 04 August 2020 12:14
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; Paul Durrant <[e-mail]>; Wei Liu <[e-mail]>
> Subject: Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> 
> Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > From: Paul Durrant <[e-mail]>
> >
> > 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 <[e-mail]>
> 
> Reviewed-by: Ian Jackson <[e-mail]>
> 

Thanks.

> > 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}
> 
> It's surprising to me that this code doesn't have the xenbus path
> already in some variable.  But I guess from the fact that you've added
> this code, that it doesn't.
> 

It is set, but set to the backend path. For safety I guess it's probably best if I use a local in this instance. Can I keep your R-b
with such a change?

  Paul

> Ian.

Ian Jackson Aug. 4, 2020, 11:35 a.m. UTC | #3

Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> > -----Original Message-----
> > From: Ian Jackson <[e-mail]>
> > Sent: 04 August 2020 12:14
> > To: Paul Durrant <[e-mail]>
> > Cc: [e-mail]; Paul Durrant <[e-mail]>; Wei Liu <[e-mail]>
> > Subject: Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> > 
> > Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> > xenstore"):
> > > +       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
> > > +       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
> > 
> > It's surprising to me that this code doesn't have the xenbus path
> > already in some variable.  But I guess from the fact that you've added
> > this code, that it doesn't.
> 
> It is set, but set to the backend path. For safety I guess it's probably best if I use a local in this instance. Can I keep your R-b
> with such a change?

Oh, wow.  I hadn't realised that.  I take back my earlier R-b :-).

Can you please use a different variable name for the frontend path ?

...

Actually.

This shouldn't be in the frontend at all, should it ?  In general the
backend writes to the backend and the frontend to the frontend.

So maybe I need to take back my R-b of
  [PATCH v2 3/4] public/io/netif: specify MTU override node

Sorry for the confusion.  I seem rather undercaffienated today.

Ian.

Paul Durrant Aug. 4, 2020, 1:31 p.m. UTC | #4

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 04 August 2020 12:35
> To: [e-mail]
> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Wei Liu' <[e-mail]>
> Subject: RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> 
> Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > > -----Original Message-----
> > > From: Ian Jackson <[e-mail]>
> > > Sent: 04 August 2020 12:14
> > > To: Paul Durrant <[e-mail]>
> > > Cc: [e-mail]; Paul Durrant <[e-mail]>; Wei Liu <[e-mail]>
> > > Subject: Re: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore
> > >
> > > Paul Durrant writes ("[PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> > > xenstore"):
> > > > +       XENBUS_PATH="/local/domain/$domid/device/vif/$devid"
> > > > +       xenstore_write "$XENBUS_PATH/mtu" ${mtu}
> > >
> > > It's surprising to me that this code doesn't have the xenbus path
> > > already in some variable.  But I guess from the fact that you've added
> > > this code, that it doesn't.
> >
> > It is set, but set to the backend path. For safety I guess it's probably best if I use a local in
> this instance. Can I keep your R-b
> > with such a change?
> 
> Oh, wow.  I hadn't realised that.  I take back my earlier R-b :-).
> 
> Can you please use a different variable name for the frontend path ?
> 

OK.

> ...
> 
> Actually.
> 
> This shouldn't be in the frontend at all, should it ?  In general the
> backend writes to the backend and the frontend to the frontend.
> 
> So maybe I need to take back my R-b of
>   [PATCH v2 3/4] public/io/netif: specify MTU override node
> 
> Sorry for the confusion.  I seem rather undercaffienated today.
> 

Too late. The xenstore node has been used by Windows frontends for the best part of a decade so we can't practically change the
path. Another way would be to also modify netback to simply echo the value from backend into frontend, but that seems rather
pointless.

Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it sets to 1492 in libxl__device_nic_setdefault() but
never writes it into xenstore. There is even a comment:

/* nic->mtu = */

in libxl__nic_from_xenstore() which implies it should have been there, but isn't.
I still think picking up the MTU from the bridge is the better way though. 

  Paul

> Ian.

Ian Jackson Aug. 5, 2020, 9:30 a.m. UTC | #5

Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> > -----Original Message-----
> > From: Ian Jackson <[e-mail]>
...
> > Actually.
> > 
> > This shouldn't be in the frontend at all, should it ?  In general the
> > backend writes to the backend and the frontend to the frontend.
> > 
> > So maybe I need to take back my R-b of
> >   [PATCH v2 3/4] public/io/netif: specify MTU override node
> > 
> > Sorry for the confusion.  I seem rather undercaffienated today.
> 
> Too late. The xenstore node has been used by Windows frontends for the best part of a decade so we can't practically change the
> path. Another way would be to also modify netback to simply echo the value from backend into frontend, but that seems rather
> pointless.

Hmm.  How does this interact with driver domains ?  I think a driver
domain might not have write access to this node.

Is there a value we can store in it that won't break these Windows
frontends, that libxl in the toolstack domain could write, before the
hotplug script runs in the driver domain ?

> Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it sets to 1492 in libxl__device_nic_setdefault() but
> never writes it into xenstore. There is even a comment:
> 
> /* nic->mtu = */
> 
> in libxl__nic_from_xenstore() which implies it should have been there, but isn't.
> I still think picking up the MTU from the bridge is the better way though. 

I agree that the default should come from the bridge.  Ideally there
would be a way to override it in the config.

Thanks,
Ian.

Durrant, Paul Aug. 5, 2020, 9:44 a.m. UTC | #6

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 05 August 2020 10:31
> To: [e-mail]
> Cc: [e-mail]; Durrant, Paul <[e-mail]>; 'Wei Liu' <[e-mail]>
> Subject: RE: [EXTERNAL] [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > > -----Original Message-----
> > > From: Ian Jackson <[e-mail]>
> ...
> > > Actually.
> > >
> > > This shouldn't be in the frontend at all, should it ?  In general the
> > > backend writes to the backend and the frontend to the frontend.
> > >
> > > So maybe I need to take back my R-b of
> > >   [PATCH v2 3/4] public/io/netif: specify MTU override node
> > >
> > > Sorry for the confusion.  I seem rather undercaffienated today.
> >
> > Too late. The xenstore node has been used by Windows frontends for the best part of a decade so we
> can't practically change the
> > path. Another way would be to also modify netback to simply echo the value from backend into
> frontend, but that seems rather
> > pointless.
> 
> Hmm.  How does this interact with driver domains ?  I think a driver
> domain might not have write access to this node.
> 

That's a good point; I think we will also need to actually write it from libxl first in that case.

> Is there a value we can store in it that won't break these Windows
> frontends, that libxl in the toolstack domain could write, before the
> hotplug script runs in the driver domain ?
> 
> > Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it sets to 1492 in
> libxl__device_nic_setdefault() but
> > never writes it into xenstore. There is even a comment:
> >
> > /* nic->mtu = */
> >
> > in libxl__nic_from_xenstore() which implies it should have been there, but isn't.
> > I still think picking up the MTU from the bridge is the better way though.
> 
> I agree that the default should come from the bridge.  Ideally there
> would be a way to override it in the config.
> 

Well, I guess we address the driver domain issue in this way too... I will add a patch to libxl to write the libxl_device_nic mtu value into xenstore, in both backend (where it should always have been) and frontend. I think the current setting of 1492 can be changed to 1500 safely (since nothing appears to currently use that value). The hotplug script should then have sufficient access to update, and a subsequent patch can add a mechanism to set the value from the config.

  Paul

Ian Jackson Aug. 5, 2020, 10:13 a.m. UTC | #7

Durrant, Paul writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore"):
> > -----Original Message-----
> > From: Ian Jackson <[e-mail]>
...
> Well, I guess we address the driver domain issue in this way
> too... I will add a patch to libxl to write the libxl_device_nic mtu
> value into xenstore,

Do you mean libxl in dom0 or libxl in the driver domain ?

libxl contains code that runs in both contexts.

See device_hotplug in libxl_device.c, in particular notice
    if (aodev->dev->backend_domid != domid) {

If you want the mtu to be read from the bridge, it can only be
determined by the driver domain, because the bridge is in the driver
domain.

In v2 of this series you arrange for the hotplug script to copy the
mtu from the bridge into the frontend path.  That won't work because
the hotplug script can't write to that xenstore node because (unlike a
domo0 backend) a driver domain backend doesn't have write access to
the frontend so can't create a new node there.

ISTM that it is correct that it is the hotplug script that does this
interface setup.  If it weren't for this erroneous use of the frontend
path I think the right design would be:
  * toolstack libxl reads the config file to find whether there is an MTU
  * toolstack libxl writes mtu node in backend iff one was in config
    (and leaves the node absent otherwise)
  * driver domain libxl runs hotplug script
  * driver domain hotplug script looks for mtu in backend; if there
    isn't one, it gets the value from the bridge and writes it to
    the backend in xenstore
  * driver domain backend driver reads mtu value from backend path
  * guest domain frontend driver reads mtu value from backend path
  * on domain save/migrate, toolstack libxl will record the mtu
    value as the actual configuration so that the migrated domain
    will get the same mtu

I don't think I understand what (in these kind of terms) you are
proposing, in order to support the frontends that want to read the mtu
from the frontend.

>  I think the current setting of 1492 can be changed to 1500 safely
> (since nothing appears to currently use that value).

Right, that seems correct to me.

> The hotplug script should then have sufficient access to update, and
> a subsequent patch can add a mechanism to set the value from the
> config.

I think what I am missing is how this "subsequent patch" would work ?
Ie what design are we aiming for, that we are now implementing part
of ?

Ian.

Durrant, Paul Aug. 5, 2020, 10:42 a.m. UTC | #8

> -----Original Message-----
> From: Ian Jackson <[e-mail]>
> Sent: 05 August 2020 11:13
> To: Durrant, Paul <[e-mail]>
> Cc: [e-mail]; [e-mail]; 'Wei Liu' <[e-mail]>
> Subject: RE: [EXTERNAL] [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Durrant, Paul writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via
> xenstore"):
> > > -----Original Message-----
> > > From: Ian Jackson <[e-mail]>
> ...
> > Well, I guess we address the driver domain issue in this way
> > too... I will add a patch to libxl to write the libxl_device_nic mtu
> > value into xenstore,
> 
> Do you mean libxl in dom0 or libxl in the driver domain ?
> 
> libxl contains code that runs in both contexts.
> 
> See device_hotplug in libxl_device.c, in particular notice
>     if (aodev->dev->backend_domid != domid) {
> 
> If you want the mtu to be read from the bridge, it can only be
> determined by the driver domain, because the bridge is in the driver
> domain.
> 
> In v2 of this series you arrange for the hotplug script to copy the
> mtu from the bridge into the frontend path.  That won't work because
> the hotplug script can't write to that xenstore node because (unlike a
> domo0 backend) a driver domain backend doesn't have write access to
> the frontend so can't create a new node there.
> 
> ISTM that it is correct that it is the hotplug script that does this
> interface setup.  If it weren't for this erroneous use of the frontend
> path I think the right design would be:
>   * toolstack libxl reads the config file to find whether there is an MTU
>   * toolstack libxl writes mtu node in backend iff one was in config
>     (and leaves the node absent otherwise)

This is where the 'subsequent patch' comes in (see the end of the email)...

>   * driver domain libxl runs hotplug script
>   * driver domain hotplug script looks for mtu in backend; if there
>     isn't one, it gets the value from the bridge and writes it to
>     the backend in xenstore
>   * driver domain backend driver reads mtu value from backend path
>   * guest domain frontend driver reads mtu value from backend path
>   * on domain save/migrate, toolstack libxl will record the mtu
>     value as the actual configuration so that the migrated domain
>     will get the same mtu
> 

That sounds right.

> I don't think I understand what (in these kind of terms) you are
> proposing, in order to support the frontends that want to read the mtu
> from the frontend.

We need some way for creating the frontend node such that the driver domain has write access. Presumably there is a suitable place in the toolstack instance of libxl to do this. This would mean we either need to write a sentinel 'invalid' value or write the default value. In the default case we could still leave the backend node absent so the hotplug script will still know whether or not a value was set in the cfg.

> 
> >  I think the current setting of 1492 can be changed to 1500 safely
> > (since nothing appears to currently use that value).
> 
> Right, that seems correct to me.
> 
> > The hotplug script should then have sufficient access to update, and
> > a subsequent patch can add a mechanism to set the value from the
> > config.
> 
> I think what I am missing is how this "subsequent patch" would work ?
> Ie what design are we aiming for, that we are now implementing part
> of ?

The subsequent patch would be one that actually acquires the mtu value from the vif config, and adds documentation to xl-network-configuration.5.pod, since this is currently missing.

  Paul

> 
> Ian.

Patch

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
 }