* Re: [Xen-devel] [PATCH] xen: netback: remove redundant xenvif_put
2013-02-19 8:03 ` [Xen-devel] " Jan Beulich
@ 2013-02-19 8:58 ` Andrew Jones
2013-02-19 8:58 ` Ian Campbell
2013-02-19 13:47 ` Ian Campbell
2 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2013-02-19 8:58 UTC (permalink / raw)
To: Jan Beulich
Cc: David Miller, ian.campbell, xen-devel, linux-kernel, netdev, liuw
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xen-devel] [PATCH] xen: netback: remove redundant xenvif_put
2013-02-19 8:03 ` [Xen-devel] " Jan Beulich
2013-02-19 8:58 ` Andrew Jones
@ 2013-02-19 8:58 ` Ian Campbell
2013-02-19 18:06 ` David Miller
2013-02-19 13:47 ` Ian Campbell
2 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2013-02-19 8:58 UTC (permalink / raw)
To: Jan Beulich
Cc: David Miller, drjones@redhat.com, xen-devel@lists.xensource.com,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xen-devel] [PATCH] xen: netback: remove redundant xenvif_put
2013-02-19 8:58 ` Ian Campbell
@ 2013-02-19 18:06 ` David Miller
2013-02-20 9:23 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2013-02-19 18:06 UTC (permalink / raw)
To: Ian.Campbell; +Cc: JBeulich, drjones, xen-devel, linux-kernel, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xen-devel] [PATCH] xen: netback: remove redundant xenvif_put
2013-02-19 18:06 ` David Miller
@ 2013-02-20 9:23 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2013-02-20 9:23 UTC (permalink / raw)
To: David Miller
Cc: JBeulich@suse.com, drjones@redhat.com,
xen-devel@lists.xensource.com, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xen-devel] [PATCH] xen: netback: remove redundant xenvif_put
2013-02-19 8:03 ` [Xen-devel] " Jan Beulich
2013-02-19 8:58 ` Andrew Jones
2013-02-19 8:58 ` Ian Campbell
@ 2013-02-19 13:47 ` Ian Campbell
2 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2013-02-19 13:47 UTC (permalink / raw)
To: Jan Beulich
Cc: David Miller, drjones@redhat.com, xen-devel@lists.xensource.com,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 9+ messages in thread