From patchwork Mon Jul 18 03:57:30 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Krishna Kumar X-Patchwork-Id: 105158 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 173B2B6F89 for ; Mon, 18 Jul 2011 14:01:31 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756282Ab1GRD5p (ORCPT ); Sun, 17 Jul 2011 23:57:45 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:47413 "EHLO e23smtp07.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756223Ab1GRD5o (ORCPT ); Sun, 17 Jul 2011 23:57:44 -0400 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [202.81.31.247]) by e23smtp07.au.ibm.com (8.14.4/8.13.1) with ESMTP id p6I3vXSM007319 for ; Mon, 18 Jul 2011 13:57:33 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p6I3v30l1151138 for ; Mon, 18 Jul 2011 13:57:03 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p6I3vWoW004725 for ; Mon, 18 Jul 2011 13:57:32 +1000 Received: from krkumar2.in.ibm.com ([9.79.207.6]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p6I3vUTd004681; Mon, 18 Jul 2011 13:57:30 +1000 From: Krishna Kumar To: mst@redhat.com Cc: netdev@vger.kernel.org, shemminger@vyatta.com, davem@davemloft.net, Krishna Kumar Date: Mon, 18 Jul 2011 09:27:30 +0530 Message-Id: <20110718035730.16001.77240.sendpatchset@krkumar2.in.ibm.com> Subject: Re: [PATCH] Fix panic in virtnet_remove Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org "Michael S. Tsirkin" wrote on 07/17/2011 03:42:15 PM: > > modprobe -r virtio_net panics in free_netdev() as the > > dev is already freed in the newly introduced virtnet_free > > (commit 3fa2a1df9094). > > Good catch, thanks! > > > Since virtnet_remove doesn't require > > dev after unregister, > > I'm not sure that true: vi used just above the line you remove > is I think the struct virtnet_info that is allocated > as part of that structure. You are right, the dev cannot be freed in the destructor. > > I am removing the free_netdev call > > in virtnet_remove instead of in virtnet_free (which seems > > to be the right place to free the dev). Confirmed that > > the panic is fixed with this patch. > > This might be just because the memory isn't reused. > Try enabling slab poisoning, I think you'll observe some problems. > > Do we absolutely have to have a destructor? > Can't we move the per cpu counter free from > virtnet_free to virtnet_remove, and get rid of > virtnet_free completely? I see some other drivers doing that, e.g. xennet_remove: ... free_percpu(info->stats); free_netdev(info->netdev); ... How about this patch (compile tested only)? Signed-off-by: Krishna Kumar --- drivers/net/virtio_net.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) -- 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 -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c --- org/drivers/net/virtio_net.c 2011-07-18 09:14:02.000000000 +0530 +++ new/drivers/net/virtio_net.c 2011-07-18 09:16:35.000000000 +0530 @@ -705,14 +705,6 @@ static void virtnet_netpoll(struct net_d } #endif -static void virtnet_free(struct net_device *dev) -{ - struct virtnet_info *vi = netdev_priv(dev); - - free_percpu(vi->stats); - free_netdev(dev); -} - static int virtnet_open(struct net_device *dev) { struct virtnet_info *vi = netdev_priv(dev); @@ -959,7 +951,6 @@ static int virtnet_probe(struct virtio_d /* Set up network device as normal. */ dev->netdev_ops = &virtnet_netdev; dev->features = NETIF_F_HIGHDMA; - dev->destructor = virtnet_free; SET_ETHTOOL_OPS(dev, &virtnet_ethtool_ops); SET_NETDEV_DEV(dev, &vdev->dev); @@ -1122,6 +1113,7 @@ static void __devexit virtnet_remove(str while (vi->pages) __free_pages(get_a_page(vi, GFP_KERNEL), 0); + free_percpu(vi->stats); free_netdev(vi->dev); }