From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hannes Reinecke Subject: Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers) Date: Thu, 17 Sep 2015 08:51:54 +0200 Message-ID: <55FA630A.4020707@suse.de> References: <1442449758-14594-1-git-send-email-s-anna@ti.com> <1442449758-14594-2-git-send-email-s-anna@ti.com> <20150917082425-mutt-send-email-mst@redhat.com> Reply-To: open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20150917082425-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: "Michael S. Tsirkin" , Suman Anna Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, JBottomley-O3H1v1f1dlM@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Id: linux-scsi@vger.kernel.org On 09/17/2015 07:33 AM, Michael S. Tsirkin wrote: > On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote: >> The virtio core uses a static ida named virtio_index_ida for >> assigning index numbers to virtio devices during registration. >> The ida core may allocate some internal idr cache layers and >> an ida bitmap upon any ida allocation, and all these layers are >> truely freed only upon the ida destruction. The virtio_index_ida >> is not destroyed at present, leading to a memory leak when using >> the virtio core as a module and atleast one virtio device is >> registered and unregistered. >> >> Fix this by invoking ida_destroy() in the virtio core module >> exit. >> >> Cc: "Michael S. Tsirkin" >> Signed-off-by: Suman Anna >=20 > Interesting. > Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c > or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c? >=20 > If no, why not? >=20 > One doesn't generally expect to have to free global variables. > Maybe we should forbid DEFINE_IDA in modules? >=20 > James, could you comment on this please? >=20 Well, looking at the code 'ida_destroy' only need to be called if you want/need to do a general cleanup. It shouldn't be required if you do correct reference counting on your objects, and call idr_remove() on each of them. Unless I'm misreading something. Seems like a topic for KS; Johannes had a larger patchset recently to clean up idr, which run into very much the same issues. Cheers, Hannes --=20 Dr. Hannes Reinecke zSeries & Storage hare-l3A5Bk7waGM@public.gmane.org +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg GF: J. Hawn, J. Guild, F. Imend=C3=B6rffer, HRB 16746 (AG N=C3=BCrnberg) --=20 You received this message because you are subscribed to the Google Groups "= open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org Visit this group at http://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753702AbbIQGv7 (ORCPT ); Thu, 17 Sep 2015 02:51:59 -0400 Received: from mx2.suse.de ([195.135.220.15]:39234 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753447AbbIQGv5 (ORCPT ); Thu, 17 Sep 2015 02:51:57 -0400 Subject: Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers) To: "Michael S. Tsirkin" , Suman Anna References: <1442449758-14594-1-git-send-email-s-anna@ti.com> <1442449758-14594-2-git-send-email-s-anna@ti.com> <20150917082425-mutt-send-email-mst@redhat.com> Cc: linux-scsi@vger.kernel.org, JBottomley@Odin.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, open-iscsi@googlegroups.com From: Hannes Reinecke Message-ID: <55FA630A.4020707@suse.de> Date: Thu, 17 Sep 2015 08:51:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150917082425-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/17/2015 07:33 AM, Michael S. Tsirkin wrote: > On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote: >> The virtio core uses a static ida named virtio_index_ida for >> assigning index numbers to virtio devices during registration. >> The ida core may allocate some internal idr cache layers and >> an ida bitmap upon any ida allocation, and all these layers are >> truely freed only upon the ida destruction. The virtio_index_ida >> is not destroyed at present, leading to a memory leak when using >> the virtio core as a module and atleast one virtio device is >> registered and unregistered. >> >> Fix this by invoking ida_destroy() in the virtio core module >> exit. >> >> Cc: "Michael S. Tsirkin" >> Signed-off-by: Suman Anna > > Interesting. > Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c > or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c? > > If no, why not? > > One doesn't generally expect to have to free global variables. > Maybe we should forbid DEFINE_IDA in modules? > > James, could you comment on this please? > Well, looking at the code 'ida_destroy' only need to be called if you want/need to do a general cleanup. It shouldn't be required if you do correct reference counting on your objects, and call idr_remove() on each of them. Unless I'm misreading something. Seems like a topic for KS; Johannes had a larger patchset recently to clean up idr, which run into very much the same issues. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)