Re: [ISSUE] Memleak in LED sysfs on heavy usage
From: Jacek Anaszewski
Date: Fri Sep 16 2016 - 17:08:52 EST
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?
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