Re: [Xen-devel] [PATCH] xen-netfront: remove warning when unloading module
On Mon, Nov 20, 2017 at 12:17:11PM +0100, Juergen Gross wrote:
> On 20/11/17 11:49, Wei Liu wrote:
> > CC netfront maintainers.
> >
> > On Mon, Nov 20, 2017 at 11:41:09AM +0100, Eduardo Otubo wrote:
> >> When unloading module xen_netfront from guest, dmesg would output
> >> warning messages like below:
> >>
> >> [ 105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use!
> >> [ 105.236839] deferring g.e. 0x903 (pfn 0x35805)
> >>
> >> This problem relies on netfront and netback being out of sync. By the time
> >> netfront revokes the g.e.'s netback didn't have enough time to free all of
> >> them, hence displaying the warnings on dmesg.
> >>
> >> The trick here is to make netfront to wait until netback frees all the
> >> g.e.'s
> >> and only then continue to cleanup for the module removal, and this is done
> >> by
> >> manipulating both device states.
> >>
> >> Signed-off-by: Eduardo Otubo <ot...@redhat.com>
> >> ---
> >> drivers/net/xen-netfront.c | 11 +++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> >> index 8b8689c6d887..b948e2a1ce40 100644
> >> --- a/drivers/net/xen-netfront.c
> >> +++ b/drivers/net/xen-netfront.c
> >> @@ -2130,6 +2130,17 @@ static int xennet_remove(struct xenbus_device *dev)
> >>
> >>dev_dbg(>dev, "%s\n", dev->nodename);
> >>
> >> + xenbus_switch_state(dev, XenbusStateClosing);
> >> + while (xenbus_read_driver_state(dev->otherend) != XenbusStateClosing){
> >> + cpu_relax();
> >> + schedule();
> >> + }
> >> + xenbus_switch_state(dev, XenbusStateClosed);
> >> + while (dev->xenbus_state != XenbusStateClosed){
> >> + cpu_relax();
> >> + schedule();
> >> + }
>
> I really don't like the busy waits.
>
> Can't you use e.g. a wait queue and wait_event_interruptible() instead?
I thought about using these, but I don't think the busy waits here are much of a
problem because it's just unloading a kernel module, not a very repetitive
action. But yes I can go for this approach on v2.
>
> BTW: what happens if the device is already in closed state if you enter
> xennet_remove()? In case this is impossible, please add a comment to
> indicate you've thought about that case.
Looks like this is the same problem Paul Durrant mentioned on his comment. I'll
work on this as well on v2.
Thanks for the review and the help on IRC :-)
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-netfront: remove warning when unloading module
On Mon, Nov 20, 2017 at 10:55:55AM +, Paul Durrant wrote:
> > -Original Message-
> > From: Eduardo Otubo [mailto:ot...@redhat.com]
> > Sent: 20 November 2017 10:41
> > To: xen-de...@lists.xenproject.org
> > Cc: net...@vger.kernel.org; Paul Durrant <paul.durr...@citrix.com>; Wei
> > Liu <wei.l...@citrix.com>; linux-ker...@vger.kernel.org;
> > vkuzn...@redhat.com; cav...@redhat.com; che...@redhat.com;
> > mga...@redhat.com; Eduardo Otubo <ot...@redhat.com>
> > Subject: [PATCH] xen-netfront: remove warning when unloading module
> >
> > When unloading module xen_netfront from guest, dmesg would output
> > warning messages like below:
> >
> > [ 105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use!
> > [ 105.236839] deferring g.e. 0x903 (pfn 0x35805)
> >
> > This problem relies on netfront and netback being out of sync. By the time
> > netfront revokes the g.e.'s netback didn't have enough time to free all of
> > them, hence displaying the warnings on dmesg.
> >
> > The trick here is to make netfront to wait until netback frees all the
> > g.e.'s
> > and only then continue to cleanup for the module removal, and this is done
> > by
> > manipulating both device states.
> >
> > Signed-off-by: Eduardo Otubo <ot...@redhat.com>
> > ---
> > drivers/net/xen-netfront.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > index 8b8689c6d887..b948e2a1ce40 100644
> > --- a/drivers/net/xen-netfront.c
> > +++ b/drivers/net/xen-netfront.c
> > @@ -2130,6 +2130,17 @@ static int xennet_remove(struct xenbus_device
> > *dev)
> >
> > dev_dbg(>dev, "%s\n", dev->nodename);
> >
> > + xenbus_switch_state(dev, XenbusStateClosing);
> > + while (xenbus_read_driver_state(dev->otherend) !=
> > XenbusStateClosing){
> > + cpu_relax();
> > + schedule();
> > + }
> > + xenbus_switch_state(dev, XenbusStateClosed);
> > + while (dev->xenbus_state != XenbusStateClosed){
> > + cpu_relax();
> > + schedule();
> > + }
> > +
>
> Waitiing for closing should be ok but waiting for closed is risky. As soon as
> a backend is in the closed state then a toolstack can completely remove the
> backend xenstore area, resulting a state of XenbusStateUnknown, which would
> cause your second loop to spin forever.
>
> Paul
Well, that's a scenario I didn't foresee. I'll come up with a solution in order
avoid this problem. Thanks for the review.
>
> > xennet_disconnect_backend(info);
> >
> > unregister_netdev(info->netdev);
> > --
> > 2.13.6
>
--
Eduardo Otubo
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen-netfront: remove warning when unloading module
When unloading module xen_netfront from guest, dmesg would output
warning messages like below:
[ 105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use!
[ 105.236839] deferring g.e. 0x903 (pfn 0x35805)
This problem relies on netfront and netback being out of sync. By the time
netfront revokes the g.e.'s netback didn't have enough time to free all of
them, hence displaying the warnings on dmesg.
The trick here is to make netfront to wait until netback frees all the g.e.'s
and only then continue to cleanup for the module removal, and this is done by
manipulating both device states.
Signed-off-by: Eduardo Otubo <ot...@redhat.com>
---
drivers/net/xen-netfront.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 8b8689c6d887..b948e2a1ce40 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2130,6 +2130,17 @@ static int xennet_remove(struct xenbus_device *dev)
dev_dbg(>dev, "%s\n", dev->nodename);
+ xenbus_switch_state(dev, XenbusStateClosing);
+ while (xenbus_read_driver_state(dev->otherend) != XenbusStateClosing){
+ cpu_relax();
+ schedule();
+ }
+ xenbus_switch_state(dev, XenbusStateClosed);
+ while (dev->xenbus_state != XenbusStateClosed){
+ cpu_relax();
+ schedule();
+ }
+
xennet_disconnect_backend(info);
unregister_netdev(info->netdev);
--
2.13.6
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-netfront, xen-netback: Use correct minimum MTU values
On Mon, Oct 16, 2017 at 03:20:32PM +0200, Mohammed Gamal wrote:
> RFC791 specifies the minimum MTU to be 68, while xen-net{front|back}
> drivers use a minimum value of 0.
>
> When set MTU to 0~67 with xen_net{front|back} driver, the network
> will become unreachable immediately, the guest can no longer be pinged.
>
> xen_net{front|back} should not allow the user to set this value which causes
> network problems.
>
> Reported-by: Chen Shi <che...@redhat.com>
> Signed-off-by: Mohammed Gamal <mga...@redhat.com>
> ---
> drivers/net/xen-netback/interface.c | 2 +-
> drivers/net/xen-netfront.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/xen-netback/interface.c
> b/drivers/net/xen-netback/interface.c
> index ee8ed9da..4491ca5 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -486,7 +486,7 @@ struct xenvif *xenvif_alloc(struct device *parent,
> domid_t domid,
>
> dev->tx_queue_len = XENVIF_QUEUE_LENGTH;
>
> - dev->min_mtu = 0;
> + dev->min_mtu = ETH_MIN_MTU;
> dev->max_mtu = ETH_MAX_MTU - VLAN_ETH_HLEN;
>
> /*
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 523387e..8b8689c 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1316,7 +1316,7 @@ static struct net_device *xennet_create_dev(struct
> xenbus_device *dev)
> netdev->features |= netdev->hw_features;
>
> netdev->ethtool_ops = _ethtool_ops;
> - netdev->min_mtu = 0;
> + netdev->min_mtu = ETH_MIN_MTU;
> netdev->max_mtu = XEN_NETIF_MAX_TX_SIZE;
> SET_NETDEV_DEV(netdev, >dev);
>
> --
> 1.8.3.1
>
Acked-by: Eduardo Otubo <ot...@redhat.com>
--
Eduardo Otubo
Senior Software Engineer @ RedHat
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
