Xen devel - Feb 2013 - [PATCH] xen: netback: remove redundant xenvif_put

netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
a xenvif_put(). As callers of netbk_fatal_tx_err should only
have one reference to the vif at this time, then the xenvif_put
in netbk_fatal_tx_err is one too many.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 drivers/net/xen-netback/netback.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 2b9520c..c23b9ec 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -893,7 +893,6 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
 {
 	netdev_err(vif->dev, "fatal error; disabling device\n");
 	xenvif_carrier_off(vif);
-	xenvif_put(vif);
 }
 
 static int netbk_count_requests(struct xenvif *vif,
-- 
1.8.1.2

On Mon, Feb 18, 2013 at 8:29 PM, Andrew Jones <drjones@redhat.com> wrote:
> netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
> a xenvif_put(). As callers of netbk_fatal_tx_err should only
> have one reference to the vif at this time, then the xenvif_put
> in netbk_fatal_tx_err is one too many.
>
>
Hmm... we had a discussion on this not long ago.

http://marc.info/?l=xen-devel&m=136084174026977&w=2

Do you actually experiencing problem?


Wei.


> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  drivers/net/xen-netback/netback.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/xen-netback/netback.c
> b/drivers/net/xen-netback/netback.c
> index 2b9520c..c23b9ec 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -893,7 +893,6 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
>  {
>         netdev_err(vif->dev, "fatal error; disabling
device\n");
>         xenvif_carrier_off(vif);
> -       xenvif_put(vif);
>  }
>
>  static int netbk_count_requests(struct xenvif *vif,
> --
> 1.8.1.2
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

From: Andrew Jones <drjones@redhat.com>
Date: Mon, 18 Feb 2013 21:29:20 +0100
> netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
> a xenvif_put(). As callers of netbk_fatal_tx_err should only
> have one reference to the vif at this time, then the xenvif_put
> in netbk_fatal_tx_err is one too many.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
Applied.

>>> On 19.02.13 at 06:53, David Miller <davem@davemloft.net>
wrote:
> From: Andrew Jones <drjones@redhat.com>
> Date: Mon, 18 Feb 2013 21:29:20 +0100
> 
>> netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
>> a xenvif_put(). As callers of netbk_fatal_tx_err should only
>> have one reference to the vif at this time, then the xenvif_put
>> in netbk_fatal_tx_err is one too many.
>> 
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> Applied.
But this is wrong from all we can tell, we discussed this before
(Wei pointed to the discussion in an earlier reply). The core of
it is that the put here parallels the one in netbk_tx_err(), and
the one in xenvif_carrier_off() matches the get from
xenvif_connect() (which normally would be done on the path
coming through xenvif_disconnect()).

And anyway - shouldn''t changes to netback require an ack from
Ian?

Jan

On Tue, 19 Feb 2013 08:03:49 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 19.02.13 at 06:53, David Miller <davem@davemloft.net>
wrote:
> > From: Andrew Jones <drjones@redhat.com>
> > Date: Mon, 18 Feb 2013 21:29:20 +0100
> > 
> >> netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
> >> a xenvif_put(). As callers of netbk_fatal_tx_err should only
> >> have one reference to the vif at this time, then the xenvif_put
> >> in netbk_fatal_tx_err is one too many.
> >> 
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> > 
> > Applied.
> 
> But this is wrong from all we can tell, we discussed this before
> (Wei pointed to the discussion in an earlier reply). The core of
> it is that the put here parallels the one in netbk_tx_err(), and
> the one in xenvif_carrier_off() matches the get from
> xenvif_connect() (which normally would be done on the path
> coming through xenvif_disconnect()).
I see the balance described by Ian in [1] now. Sorry that I missed
that previous discussion and generated this noise.

[1] http://marc.info/?l=xen-devel&m=136084174026977&w=2

drew
> 
> And anyway - shouldn''t changes to netback require an ack from
> Ian?
> 
> Jan
>

On Tue, 2013-02-19 at 08:03 +0000, Jan Beulich wrote:
> >>> On 19.02.13 at 06:53, David Miller <davem@davemloft.net>
wrote:
> > From: Andrew Jones <drjones@redhat.com>
> > Date: Mon, 18 Feb 2013 21:29:20 +0100
> > 
> >> netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
> >> a xenvif_put(). As callers of netbk_fatal_tx_err should only
> >> have one reference to the vif at this time, then the xenvif_put
> >> in netbk_fatal_tx_err is one too many.
> >> 
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> > 
> > Applied.
> 
> But this is wrong from all we can tell,
Yes, please can this be reverted.
>  we discussed this before
> (Wei pointed to the discussion in an earlier reply). The core of
> it is that the put here parallels the one in netbk_tx_err(), and
> the one in xenvif_carrier_off() matches the get from
> xenvif_connect() (which normally would be done on the path
> coming through xenvif_disconnect()).
Perhaps Andrew was looking at the tree before "xen-netback: correctly
return errors from netbk_count_requests()" which fixed a different case
of a double put which may have appeared to be fixed by this change too.

Ian.
> And anyway - shouldn''t changes to netback require an ack from
> Ian?
> 
> Jan
>

On Tue, 2013-02-19 at 08:03 +0000, Jan Beulich wrote:
> The core of
> it is that the put here parallels the one in netbk_tx_err(), and
> the one in xenvif_carrier_off() matches the get from
> xenvif_connect() (which normally would be done on the path
> coming through xenvif_disconnect()). 
A few people have made this mistake already, perhaps this helps make it
more obvious what is going on...

8<--------------------------------

From 7b93077e2cda5881b594d9c7e733e617df87d7c0 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 19 Feb 2013 10:46:12 +0000
Subject: [PATCH] xen/netback: refactor xenvif_carrier_on from xenvif_connect

Several people have been confused by the vif reference count taken by
xenvif_connect() being dropped in xenvif_carrier_off(). Factor out bringing
the carrier up (and the associated reference grab) to try and make the
relationship more obvious.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/interface.c |   49 +++++++++++++++++++---------------
 1 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c
b/drivers/net/xen-netback/interface.c
index d984141..059d726 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -307,6 +307,32 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t
domid,
 	return vif;
 }
 
+static void xenvif_carrier_on(struct xenvif *vif)
+{
+	xenvif_get(vif);
+
+	rtnl_lock();
+	if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
+		dev_set_mtu(vif->dev, ETH_DATA_LEN);
+	netdev_update_features(vif->dev);
+	netif_carrier_on(vif->dev);
+	if (netif_running(vif->dev))
+		xenvif_up(vif);
+	rtnl_unlock();
+}
+
+void xenvif_carrier_off(struct xenvif *vif)
+{
+	struct net_device *dev = vif->dev;
+
+	rtnl_lock();
+	netif_carrier_off(dev); /* discard queued packets */
+	if (netif_running(dev))
+		xenvif_down(vif);
+	rtnl_unlock();
+	xenvif_put(vif);
+}
+
 int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 		   unsigned long rx_ring_ref, unsigned int evtchn)
 {
@@ -328,16 +354,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long
tx_ring_ref,
 	vif->irq = err;
 	disable_irq(vif->irq);
 
-	xenvif_get(vif);
-
-	rtnl_lock();
-	if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
-		dev_set_mtu(vif->dev, ETH_DATA_LEN);
-	netdev_update_features(vif->dev);
-	netif_carrier_on(vif->dev);
-	if (netif_running(vif->dev))
-		xenvif_up(vif);
-	rtnl_unlock();
+	xenvif_carrier_on(vif);
 
 	return 0;
 err_unmap:
@@ -346,18 +363,6 @@ err:
 	return err;
 }
 
-void xenvif_carrier_off(struct xenvif *vif)
-{
-	struct net_device *dev = vif->dev;
-
-	rtnl_lock();
-	netif_carrier_off(dev); /* discard queued packets */
-	if (netif_running(dev))
-		xenvif_down(vif);
-	rtnl_unlock();
-	xenvif_put(vif);
-}
-
 void xenvif_disconnect(struct xenvif *vif)
 {
 	if (netif_carrier_ok(vif->dev))
-- 
1.7.2.5

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Tue, 19 Feb 2013 08:58:44 +0000
> On Tue, 2013-02-19 at 08:03 +0000, Jan Beulich wrote:
>> >>> On 19.02.13 at 06:53, David Miller
<davem@davemloft.net> wrote:
>> > From: Andrew Jones <drjones@redhat.com>
>> > Date: Mon, 18 Feb 2013 21:29:20 +0100
>> > 
>> >> netbk_fatal_tx_err() calls xenvif_carrier_off(), which does
>> >> a xenvif_put(). As callers of netbk_fatal_tx_err should only
>> >> have one reference to the vif at this time, then the
xenvif_put
>> >> in netbk_fatal_tx_err is one too many.
>> >> 
>> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > 
>> > Applied.
>> 
>> But this is wrong from all we can tell,
> 
> Yes, please can this be reverted.
Done and I''ve annotated the revert commit message with as much
information as possible.

On Tue, 2013-02-19 at 18:06 +0000, David Miller wrote:
> From: Ian Campbell <Ian.Campbell@citrix.com>
> Date: Tue, 19 Feb 2013 08:58:44 +0000
> 
> > On Tue, 2013-02-19 at 08:03 +0000, Jan Beulich wrote:
> >> >>> On 19.02.13 at 06:53, David Miller
<davem@davemloft.net> wrote:
> >> > From: Andrew Jones <drjones@redhat.com>
> >> > Date: Mon, 18 Feb 2013 21:29:20 +0100
> >> > 
> >> >> netbk_fatal_tx_err() calls xenvif_carrier_off(), which
does
> >> >> a xenvif_put(). As callers of netbk_fatal_tx_err should
only
> >> >> have one reference to the vif at this time, then the
xenvif_put
> >> >> in netbk_fatal_tx_err is one too many.
> >> >> 
> >> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> > 
> >> > Applied.
> >> 
> >> But this is wrong from all we can tell,
> > 
> > Yes, please can this be reverted.
> 
> Done and I''ve annotated the revert commit message with as much
> information as possible.
Thanks, 629821d9b looks good to me.

May be worth considering the patch in
<1361281636.1051.100.camel@zakaz.uk.xensource.com> if we get many more
of these queries...

Ian.