[PATCH xserver] edid: Prune duplicates after adding modes from DDC

[Michel Dänzer]

On 23/02/17 06:46 AM, Chen, Xiaogang wrote:
>> From: Michel Dänzer [mailto:michel at daenzer.net]
>>
>> Multiple calls to xf86EdidMonitorSet (which can be triggered e.g. by running
>> xrandr) would potentially keep adding the same modes, causing the Monitor-
>>> Modes list to keep growing larger and using up more memory.
>>
>> Fix this by calling xf86PruneDuplicateModes after adding the modes returned
>> by xf86DDCGetModes. This removes any newly added modes which were
>> already in the Monitor->Modes list before, but keeps new modes which weren't
>> yet.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99521
>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
>> ---
>>
>> Xiaogang / Jorge, does this fix the memory leak you're seeing?
>>
> [XCHEN] Hi Michel: I tested your patch with same script I used before
> (while true; do xrandr --verbose; done), the memory leak is still
> there.

I stepped through the execution of the xf86PruneDuplicateModes call
added by my patch in gdb and verified that it works as intended,
eliminating the duplicate modes.

I suspect you're being mislead my the memleax tool: It reports any memory
which is allocated and not freed within a certain period of time (10
seconds by default). However, that's expected to happen here, not
necessarily a sign of a leak, since we're keeping the list of modes in
Monitor->Modes. (xf86PruneDuplicateModes may delete the old modes from
the previous xf86DDCGetModes call, not the new ones allocated in the
current call)


valgrind's vgdb support is more useful for analyzing this. Example
session without my patch:

Run Xorg in valgrind, e.g. something like

 sudo -b valgrind Xorg :1

Run a client in the background, to prevent the server from resetting
after each invocation of xrandr:

 DISPLAY=:1 xterm&

Determine the --pid argument to use for vgdb:

 sudo vgdb -l
 use --pid=15930 for /usr/bin/valgrind.bin Xorg

Get the baseline memory consumption:

 sudo vgdb --pid=15930 leak_check summary
 sending command leak_check summary to pid 15930
 ==15930== LEAK SUMMARY:
 ==15930==    definitely lost: 20,983 (+20,983) bytes in 87 (+87) blocks
 ==15930==    indirectly lost: 5,930 (+5,930) bytes in 92 (+92) blocks
 ==15930==      possibly lost: 5,802,594 (+5,802,594) bytes in 37,436 (+37,436) blocks
 ==15930==    still reachable: 10,608,319 (+10,608,391) bytes in 20,966 (+20,966) blocks
 ==15930==         suppressed: 0 (+0) bytes in 0 (+0) blocks
 ==15930== Reachable blocks (those to which a pointer was found) are not shown.
 ==15930== To see them, add 'reachable any' args to leak_check
 ==15930== 

Run xrandr and get the updated memory consumption:

 DISPLAY=:1 xrandr >/dev/null
 sudo vgdb --pid=15930 leak_check
 sending command leak_check to pid 15930
 ==15930== LEAK SUMMARY:
 ==15930==    definitely lost: 20,983 (+0) bytes in 87 (+0) blocks
 ==15930==    indirectly lost: 5,930 (+0) bytes in 92 (+0) blocks
 ==15930==      possibly lost: 5,802,594 (+0) bytes in 37,436 (+0) blocks
 ==15930==    still reachable: 10,610,170 (+1,851) bytes in 20,986 (+20) blocks
 ==15930==         suppressed: 0 (+0) bytes in 0 (+0) blocks
 ==15930== Reachable blocks (those to which a pointer was found) are not shown.
 ==15930== To see them, add 'reachable any' args to leak_check
 ==15930== 

The xrandr invocation caused reachable memory to grow by 1851 bytes in
20 blocks.

With my patch applied, this doesn't report any increase in reachable
memory[0], no matter how many times I run xrandr in the meantime.

[0] except for the first time, due to https://bugs.freedesktop.org/show_bug.cgi?id=99521#c26


Run "sudo vgdb --pid=<PID> help" to get some help about possible
commands, or see http://www.valgrind.org/docs/manual/mc-manual.html#mc
manual.monitor-commands for more information about valgrind's vgdb
support.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer