[xserver] xfree86: fix Xorg memory leak when run xrandr

[Chen, Xiaogang]

Hi Michel, Peter:



Thank you for your review. I understand your concerns. I used memleax to monitor memory alloc/dealloc(https://github.com/WuBingzheng/memleax) by attaching it to Xorg process. This tool can record call stacks that lead to memory allocated but the memory blocks have not been release yet. Its stack monitoring result is attached here.



Inside function xf86DDCGetModes some DisplayMode are allocated(such as by DDCModesFromEstablished(), etc). To be honest, I do not know why it does that, but I do not find these DisplayMode(s) got used afterword, and they are not release either afterword. It is the reason I dealloc these DisplayMode at xf86EdidMonitorSet().



To verify that, I ran the script: while true; do xrandr --verbose; done



With original code, by using top or htop, I sought the virtual memory consumption of Xorg process or system memory consumption keep going up(1MB every 1-2 second) when running the script above. The memleax pointed to xf86DDCGetModes has allocated some memory(DisplayMode) and these memory blocks are not released so far.



After delete these DisplayMode the memory consumption of Xorg(and system used mem) keeps very stable when running same script above. And I did not see any regression so far.



Please correct me if I did not understand why inside xf86DDCGetMode some DisplayMode are allocated. Thanks.



Xiaogang





-----Original Message-----
From: Michel Dänzer [mailto:michel at daenzer.net]
Sent: Monday, February 20, 2017 8:05 PM
To: Xiaogang Chen <chenxiaogang888 at gmail.com>
Cc: xorg-devel at lists.x.org; Chen, Xiaogang <Xiaogang.Chen at amd.com>
Subject: Re: [xserver] xfree86: fix Xorg memory leak when run xrandr



On 21/02/17 03:55 AM, Xiaogang Chen wrote:

> xf86DDCGetModes generates some DisplayMods that are not used and not released after.

> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=99521

>

> Signed-off-by: Xiaogang Chen <Xiaogang.Chen at amd.com<mailto:Xiaogang.Chen at amd.com>>

> ---

>  hw/xfree86/modes/xf86EdidModes.c | 17 +++++------------

>  1 file changed, 5 insertions(+), 12 deletions(-)

>

> diff --git a/hw/xfree86/modes/xf86EdidModes.c

> b/hw/xfree86/modes/xf86EdidModes.c

> index f0e1e97..915b291 100644

> --- a/hw/xfree86/modes/xf86EdidModes.c

> +++ b/hw/xfree86/modes/xf86EdidModes.c

> @@ -1201,18 +1201,11 @@ xf86EdidMonitorSet(int scrnIndex, MonPtr Monitor, xf86MonPtr DDC)

>          /* look for last Mode */

>          Mode = Modes;

>

> -        while (Mode->next)

> -            Mode = Mode->next;

> +        /*release these video modes that are not used after*/

> +        while (Mode) {

> +           xf86DeleteMode(&Modes, Mode);

> +           Mode = Modes;

> +       }

>

> -        /* add to MonPtr */

> -        if (Monitor->Modes) {

> -            Monitor->Last->next = Modes;

> -            Modes->prev = Monitor->Last;

> -            Monitor->Last = Mode;

> -        }

> -        else {

> -            Monitor->Modes = Modes;

> -            Monitor->Last = Mode;

> -        }

>      }



This doesn't make sense to me I'm afraid. The intention of the existing code is to append all modes in the Modes list to the Monitor->Modes list. You're changing it to just delete all modes in the Modes list instead.





I'm not sure how this function could be responsible for the memory leaks you're seeing. Can you provide valgrind output about those leaks?





--

Earthling Michel Dänzer               |               http://www.amd.com

Libre software enthusiast             |             Mesa and X developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170221/0626e174/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Xorg-Mem-leak-Memleax-monitor.rtf
Type: application/rtf
Size: 23322 bytes
Desc: Xorg-Mem-leak-Memleax-monitor.rtf
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170221/0626e174/attachment-0001.rtf>