| Message ID | 1361219360-29176-1-git-send-email-drjones@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)