lkml.org 
[lkml]   [2010]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: request_firmware API exhaust memory
    From
    On Mon, Apr 26, 2010 at 6:19 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
    > On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote:
    >> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@gmail.com> 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/

    \
     
     \ /
      Last update: 2010-04-26 19:01    [W:0.027 / U:31.740 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site