Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753945Ab0DZQ7j (ORCPT ); Mon, 26 Apr 2010 12:59:39 -0400 Received: from mail-bw0-f219.google.com ([209.85.218.219]:33188 "EHLO mail-bw0-f219.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753473Ab0DZQ7h convert rfc822-to-8bit (ORCPT ); Mon, 26 Apr 2010 12:59:37 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=PvL2YsflUs6YuiqkJHD3n8bxgG8vwiUPYl3X0D1Ldze+LyUv4m91XynmVvCj8HbFJz jXNIcOcspif7gVcailB4GCTR2Jm7DCnjo2hNmlQMAJN0fx5r2WmAEB6GaU8wVPDSADtd K5kn9q6dPrxkoJIfTTx+J5hWkXal7FP/uOB3Y= MIME-Version: 1.0 In-Reply-To: <1272295140.2434.8.camel@yio.site> References: <20100419145934.GA10893@kroah.com> <20100425163711.GA20196@kroah.com> <20100425193658.GA24039@kroah.com> <1272295140.2434.8.camel@yio.site> Date: Mon, 26 Apr 2010 19:59:35 +0300 Message-ID: Subject: Re: request_firmware API exhaust memory From: Tomas Winkler To: Kay Sievers Cc: Greg KH , Johannes Berg , David Woodhouse , "Rafael J. Wysocki" , Emmanuel Grumbach , linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4453 Lines: 88 On Mon, Apr 26, 2010 at 6:19 PM, Kay Sievers wrote: > On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote: >> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler wrote: >> > Said thing is that I don't see where the memory goes.... Anyhow I will >> > try to run valgrin on udev just to be sure. >> >> Nah, that memory would be freed, if you kill all udev processes, which >> it doesn't. >> >> The many udev worker processes you see for a few seconds was caused by >> udevd handling events with TIMEOUT= set special. We need to make sure, >> that firmware events run immediately and don't wait for other >> processes to finish. The logic who does that was always creating a new >> worker. I changed this now, but this will not affect the underlying >> problem you are seeing, it will just make the udev workers not grow in >> a timeframe of less than 10 seconds. The change is here: >>   http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=665ee17def2caa6811ae032ae68ebf8239a18cf8 >> but as mentioned, this change is unrelated to the memory leak you are seeing. >> >> > I'll be glad If someone can run my simple driver I posted and confirm >> > that sees the same problem >> >> I can confirm that memory gets lost. I suspect for some reason the >> firmware does not get properly cleaned up. If you increase the size of >> the firmware image, it will leak memory much faster. > > I guess, the assumption that vfree() will free pages which are allocated > by custom code, and not by vmalloc(), is not true. > > The attached changes seem to fix the issue for me. > > The custom page array mangling was introduced by David as an optimization > with commit 6e03a201bbe8137487f340d26aa662110e324b20 and this should be > checked, and if needed be fixed. > > Cheers, > Kay > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 985da11..fe4e872 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -162,7 +162,7 @@ static ssize_t firmware_loading_store(struct device *dev, >                        mutex_unlock(&fw_lock); >                        break; >                } > -               vfree(fw_priv->fw->data); > +               vunmap(fw_priv->fw->data); >                fw_priv->fw->data = NULL; >                for (i = 0; i < fw_priv->nr_pages; i++) >                        __free_page(fw_priv->pages[i]); > @@ -176,7 +176,7 @@ static ssize_t firmware_loading_store(struct device *dev, >                break; >        case 0: >                if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) { > -                       vfree(fw_priv->fw->data); > +                       vunmap(fw_priv->fw->data); >                        fw_priv->fw->data = vmap(fw_priv->pages, >                                                 fw_priv->nr_pages, >                                                 0, PAGE_KERNEL_RO); > @@ -184,9 +184,6 @@ static ssize_t firmware_loading_store(struct device *dev, >                                dev_err(dev, "%s: vmap() failed\n", __func__); >                                goto err; >                        } > -                       /* Pages will be freed by vfree() */ > -                       fw_priv->page_array_size = 0; > -                       fw_priv->nr_pages = 0; >                        complete(&fw_priv->completion); >                        clear_bit(FW_STATUS_LOADING, &fw_priv->status); >                        break; > @@ -578,7 +575,7 @@ release_firmware(const struct firmware *fw) >                        if (fw->data == builtin->data) >                                goto free_fw; >                } > -               vfree(fw->data); > +               vunmap(fw->data); >        free_fw: >                kfree(fw); >        } Thanks for your effort I will test it tomorrow Tomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/