Re: [ISSUE] Memleak in LED sysfs on heavy usage

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 09/16/2016 11:08 PM, Jacek Anaszewski wrote:
>
> On 09/16/2016 09:44 PM, Greg KH wrote:
>> On Fri, Sep 16, 2016 at 08:49:44PM +0200, Jacek Anaszewski wrote:
>>> On 09/16/2016 04:39 PM, Greg KH wrote:
>>>> On Fri, Sep 16, 2016 at 04:32:39PM +0200, Jacek Anaszewski wrote:
>>>>> On 09/16/2016 04:06 PM, Greg KH wrote:
>>>>>> On Fri, Sep 16, 2016 at 03:41:09PM +0200, Jacek Anaszewski wrote:
>>>>>>> On 09/16/2016 02:08 PM, Daniel Gorsulowski wrote:
>>>>>>>> Hi Jacek,
>>>>>>>>
>>>>>>>> Am 16.09.2016 um 13:25 schrieb Jacek Anaszewski:
>>>>>>>>> On 09/16/2016 10:15 AM, Daniel Gorsulowski wrote:
>>>>>>>>>> Hi Jacek,
>>>>>>>>>>
>>>>>>>>>> Am 16.09.2016 um 09:31 schrieb Jacek Anaszewski:
>>>>>>>>>>> Hi Daniel,
>>>>>>>>>>>
>>>>>>>>>>> On 09/12/2016 10:50 AM, Daniel Gorsulowski wrote:
>>>>>>>>>>>> Hello!
>>>>>>>>>>>>
>>>>>>>>>>>> Please consider if I made something wrong, sending this
>>>>>>>>>>>> issue. This is
>>>>>>>>>>>> my first contact to the LKML.
>>>>>>>>>>>> By mistake, I accessed an LED via /sys/class/leds subsystem
>>>>>>>>>>>> very
>>>>>>>>>>>> fast in
>>>>>>>>>>>> an user application. I figured out, that the free user memory
>>>>>>>>>>>> decreased
>>>>>>>>>>>> constantly. So I tried to analyze the Problem and wrote a litte
>>>>>>>>>>>> script:
>>>>>>>>>>>>
>>>>>>>>>>>> #!/bin/sh
>>>>>>>>>>>> while [ 1 ]; do
>>>>>>>>>>>>         echo 1 > /sys/class/leds/2a_service_yellow/brightness
>>>>>>>>>>>>         echo 0 > /sys/class/leds/2a_service_yellow/brightness
>>>>>>>>>>>> done
>>>>>>>>>>>>
>>>>>>>>>>>> And voila, I was able to reproduce the problem.
>>>>>>>>>>>> So I add a bit more debugging:
>>>>>>>>>>>>
>>>>>>>>>>>> #!/bin/sh
>>>>>>>>>>>> cnt=0
>>>>>>>>>>>> while [ 1 ]; do
>>>>>>>>>>>>         if [ `expr $cnt % 1000` -eq 0 ]; then
>>>>>>>>>>>>                 free | grep Mem: | cut -d' ' -f25
>>>>>>>>>>>>         fi
>>>>>>>>>>>>         echo 1 > /sys/class/leds/2a_service_yellow/brightness
>>>>>>>>>>>>         echo 0 > /sys/class/leds/2a_service_yellow/brightness
>>>>>>>>>>>>         let "cnt++"
>>>>>>>>>>>> done
>>>>>>>>>>>>
>>>>>>>>>>>> And huh? No memory is eaten anymore. So it looks like, the
>>>>>>>>>>>> problem
>>>>>>>>>>>> only
>>>>>>>>>>>> occours on heavy (fast) usage of /sys/class/leds subsystem.
>>>>>>>>>>>>
>>>>>>>>>>>> I rewrote the script and toggled a GPIO pin, but there was
>>>>>>>>>>>> no problem
>>>>>>>>>>>> recognizable.
>>>>>>>>>>>
>>>>>>>>>>> I've been unable to reproduce the problem with leds-aat1290
>>>>>>>>>>> driver
>>>>>>>>>>> and Samsung M0 board. It must be driver specific issue.
>>>>>>>>>>> What driver did you use?
>>>>>>>>>>>
>>>>>>>>>> I defined LEDS_GPIO and so I'm using leds-gpio driver.
>>>>>>>>>> danielg@debby:~/opt/prj/ti-linux-kernel$ cat .config | grep
>>>>>>>>>> LEDS | grep
>>>>>>>>>> -v "^# "
>>>>>>>>>> CONFIG_INPUT_LEDS=y
>>>>>>>>>> CONFIG_NEW_LEDS=y
>>>>>>>>>> CONFIG_LEDS_CLASS=y
>>>>>>>>>> CONFIG_LEDS_GPIO=y
>>>>>>>>>> CONFIG_LEDS_TRIGGERS=y
>>>>>>>>>> CONFIG_LEDS_TRIGGER_TIMER=y
>>>>>>>>>> CONFIG_LEDS_TRIGGER_ONESHOT=y
>>>>>>>>>> CONFIG_LEDS_TRIGGER_HEARTBEAT=y
>>>>>>>>>> CONFIG_LEDS_TRIGGER_GPIO=y
>>>>>>>>>> CONFIG_LEDS_TRIGGER_DEFAULT_ON=y
>>>>>>>>>> CONFIG_LEDS_TRIGGER_TRANSIENT=y
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Unfortunately I am still unable to reproduce the problem with
>>>>>>>>> leds-gpio.
>>>>>>>>> I'm not observing any heavy usage with your test case:
>>>>>>>>>
>>>>>>>>> ~#free
>>>>>>>>>               total       used       free     shared    buffers
>>>>>>>>> cached
>>>>>>>>> Mem:       1028092      61364     966728          0
>>>>>>>>> 8416      22396
>>>>>>>>> -/+ buffers/cache:      30552     997540
>>>>>>>>> Swap:            0          0          0
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Actually you didn't give any numbers. What kernel version are
>>>>>>>>> you using?
>>>>>>>>>
>>>>>>>> As I wrote, the problems occurred in vanilla 4.6 kernel, but
>>>>>>>> also in 4.4
>>>>>>>> kernel (with PREEMPT-RT Patchset).
>>>>>>>
>>>>>>> Heh, funny coincidence. I was testing this on recent linux-leds.git,
>>>>>>> for-next branch and was not able to detect the issue. It started to
>>>>>>> appear after resetting HEAD to 4.8-rc2 base. Finally it turned out
>>>>>>> that what fixes the issue is the most recent commit [1].
>>>>>>>
>>>>>>> Further investigation revealed that this is kobject_uevent_env(),
>>>>>>> called from led_trigger_set(), which causes memory leaks when called
>>>>>>> with high frequency.
>>>>>>
>>>>>> Really?  Where in kobject_uevent_env() is the memory leak?
>>>>>
>>>>> I'll chase it down when and will let you know. This may be
>>>>> non-trivial issue as it suffices to add "sleep 0.1" between
>>>>> brightness setting operations to prevent it.
>>>>
>>>> Why are you abusing uevents for flashing an LED?  Please don't do that,
>>>> it's not what that interface is for at all.
>>>
>>> It is called in a result of setting brightness value to LED_OFF,
>>> which also removes registered trigger if any.
>>
>> So every time the LED goes off a uevent happens?  That's not a good
>> design.
>
> We had a bug, but it was fixed with recent commit. Now the uevent
> will happen when LED goes off only if trigger has been set.
>
>>> The rationale for calling kobject_uevent_env() is given in the
>>> relevant commit message:
>>>
>>> commit 52c47742f79d9240f90af9a6722fe8bb3fa8c0f9
>>> Author: Colin Cross <ccross@android.com>
>>> Date:   Mon Aug 27 09:31:49 2012 +0800
>>>
>>>     leds: triggers: send uevent when changing triggers
>>>
>>>     Some triggers create sysfs files when they are enabled. Send a
>>> uevent
>>>     "change" notification whenever the trigger is changed to allow
>>> userspace
>>>     processes such as udev to modify permissions on the new files.
>>>
>>>     A change notification will also be sent during registration of
>>> led class
>>>     devices or led triggers if the default trigger of an led class
>>> device
>>>     is found.
>>
>> If a sysfs file is removed, then I could see a change event being ok.
>> But that's not what this patch does, it ALWAYS sends a uevent, even if
>> nothing changed!
>
> Yes, the function needs to be fixed. I'll submit relevant patch soon.
>
>> Please fix that, otherwise you are going to really annoy userspace tools
>> with this.
>>
>> But even then, I don't see how the uevent code has a memory leak with
>> this, do you?

On x86_64 memory usage grows by ~50kB, and than gets back to the value
from before launching the stress test. It doesn't start to grow again.

On exynos4412-trats2 board memory usage grows by 240Mb and then it
gradually drops, so this is certainly not a memory leak, but some
other platform specific memory management related issue.

It will perform further investigation, in a spare time.

> Not at the moment, but the fact is that the issue ceases to occur
> after commenting the line out.
>
>> And why aren't you checking the return value of
>> kobject_uevent_env()?
>
> Will fix.
>

-- 
Best regards,
Jacek Anaszewski