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

[Jacek Anaszewski]

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@xxxxxxxxxxx>
> > > 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