[PATCH] xen: netback: remove redundant xenvif_put

[PATCH] xen: netback: remove redundant xenvif_put

[Andrew Jones]

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

Re: [PATCH] xen: netback: remove redundant xenvif_put

[Wei Liu]

[-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 2340 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] xen: netback: remove redundant xenvif_put

[David Miller]

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.

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

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[Andrew Jones]

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
> 

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

[Andrew Jones]

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
> 

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

[Ian Campbell]

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
> 

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

[Ian Campbell]

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

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

[David Miller]

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.

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

[Ian Campbell]

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.