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

[Chen, Xiaogang]

> -----Original Message-----
> From: Michel Dänzer [mailto:michel at daenzer.net]
> Sent: Thursday, March 09, 2017 2:25 AM
> To: Chen, Xiaogang <Xiaogang.Chen at amd.com>
> Cc: xorg-devel at lists.x.org; jorge_monteagudo at hotmail.com
> Subject: Re: [PATCH xserver] edid: Prune duplicates after adding modes from
> DDC
> 
> 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)
[XCHEN] I have same understanding on memleax tool: it reports memory allocated and not freed in a certain time. It just gives clues where the memory leaks may happen.
We claim there is memory leak by using top or htop cmd to see Xorg virtual memory space size or system memory consumption. They keep increasing with or without your patch.
> 
> 
> 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