Bug 103991 - Memoryleak in Xorg when using xrandr
Summary: Memoryleak in Xorg when using xrandr
Status: RESOLVED MOVED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: Other Linux (All)
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-30 08:22 UTC by Luc
Modified: 2018-12-13 22:38 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
patch proposal solving the memory leak (4.55 KB, patch)
2017-11-30 08:22 UTC, Luc
no flags Details | Splinter Review

Description Luc 2017-11-30 08:22:38 UTC
Created attachment 135827 [details] [review]
patch proposal solving the memory leak

In Xorg there is a memory leak each time xrandr is used.
This is easy to test by issuing following command:

$ while true ; do xrandr --verbose > /dev/null ; done

This will gradually let X take more memory until the oom killer pops in.
Reason is that each time the Monitor->Modes list grows, without pruning
duplicates.

In attachment a patch is added which solves the issue.
This was found and patched on the 1.19.5 Xserver version.
Comment 1 Olivier Fourdan 2017-11-30 08:29:52 UTC
Could be a duplicate of bug 99521 fixed with commit fdc79fe72

commit fdc79fe72bc0b97776df2c3a664076c60e08a87c
Author: Michel Dänzer <michel.daenzer@amd.com>
Date:   Thu Mar 9 17:34:55 2017 +0900

    edid: Prune duplicates after adding modes from DDC
    
    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 makes sure there's only one instance
    of each unique mode in the list.
    
    v2:
    * Replace semicolon with {} for empty for loop (Emil Velikov)
    * Slightly tweak commit log to avoid minor inaccuracy about what
      xf86PruneDuplicateModes does
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99521
    Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
    Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Otherwise, best way to submit patches is to send them to xorg-devel mailing list, it's unlikely people will review patches in bugzilla for Xorg.

For more details, please see https://www.x.org/wiki/Development/Documentation/SubmittingPatches/
Comment 2 Michel Dänzer 2017-11-30 09:03:41 UTC
(In reply to Olivier Fourdan from comment #1)
> Could be a duplicate of bug 99521 fixed with commit fdc79fe72

It is.

Note that in the meantime I've come to think that it would be better to just replace the old modes with the new ones, instead of combining them somehow. Luc, if you want to make a patch to that effect based on Git master and send it to the xorg-devel mailing list for review, that would be great!
Comment 3 Luc 2017-11-30 15:13:57 UTC
Yes, no problem.
I suppose you mean that the old ones are removed and then replaced with the new discovered ones?

Remark that to prune the duplicates was not enough: I also removed the tracking of the last in the list, as this tracking does not work correctly causing parts of the list to go unlinked. Thus even with pruning the duplicates I had still a memory leak...

I checked the other patch, and there the last pointer is still kept, so I'm not sure the leak is then fixed in all cases.
Comment 4 Michel Dänzer 2017-11-30 15:26:34 UTC
(In reply to Luc from comment #3)
> Yes, no problem.
> I suppose you mean that the old ones are removed and then replaced with the
> new discovered ones?

Exactly.


> I checked the other patch, and there the last pointer is still kept, so I'm
> not sure the leak is then fixed in all cases.

I think it should be fine; Monitor->Last is updated after pruning duplicate modes.

If you think the Last pointer should still be removed with the changes above, please do it in a separate patch.
Comment 5 Luc 2017-11-30 16:33:42 UTC
Hi Michel,

When diving deeper in the code, throwing them all away is probably not a good idea.
In case a config file is loaded, these are added there as well. I suppose that we should not delete these...
This is the path doing this:
HandleConfigFile->ConfigLayout->ConfigScreen->ConfigMonitor
Comment 6 GitLab Migration User 2018-12-13 22:38:26 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/xserver/issues/526.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.