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.
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/
(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!
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.
(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.
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
-- 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.