[Bug 98420] Memory leak in drm_atomic.c eventually (few days) consuming all RAM (on at least one system configuration)

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98420

--- Comment #3 from Felix Monninger <felix.monninger at gmail.com> ---
(In reply to Chris Wilson from comment #2)
> Ah, I thought I sent this to dri-devel this morning:
> 
> https://lists.freedesktop.org/archives/dri-devel/2016-October/122066.html
> 
> If you want to send a "I made this!" in reply (with sob), quite happy to
> adjust authorship.

I made this! :)

Thank you both Ville and Chris for your advice and the refactor. I will add the
modified patch including signed-off-by to this report here, as I'm having
trouble replying to the dri-devel thread as I was not subscribed to it at the
time of posting. Please look over it to see if I did it correctly according to
Documentation/SubmittingPatches (I left your signed-off-by in the line below
mine and added a [<name>: <foo>] comment informing about your refactoring of
the patch in between (I hope that's ok/allowed; if not, feel free to change it.
I see the kernel is quite strict on traceability of ownership)).

Two minor comments on the modifications you did:
- The NULL check "if (new_blob) drm_property_unreference_blob(new_blob);" is
theoretically unneccessary as it is done inside "...unreference_blob()",
however I would leave it in, too, for clarity and ease of understanding what is
being done. There has been a commit removing these checks (which has been
generated automatically by a script) though, see [1].
- Because this check is for (new_blob != NULL) and not for (blob_id != 0), the
code now assumes that a new_blob that originally is equal to NULL (in the case
blob_id == 0) is never set to something non-NULL inside
drm_atomic_replace_property_blob() and thus afterwards erroneously
unreferenced. Maybe we should change it to "if (blob_id != 0)
drm_property_unreference_blob(new_blob);", then it behaves correctly in any
case, even though we the control flow with twice the same if looks a little bit
silly. (I am not familiar with the specifics of the Kernel style. If by
convention only the first pointer argument to a function is ever reassigned,
this is no problem.)

Thank you again for your help, I had a lot of fun in investigating this!
Felix


[1]
https://github.com/torvalds/linux/commit/5f911905054a64cf8c7871fddd33f4af74f07a17

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx-bugs/attachments/20161025/60b4ba19/attachment.html>