<html>
<head>
<base href="https://bugs.freedesktop.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEEDINFO "
title="NEEDINFO - Memory leak in drm_atomic.c eventually (few days) consuming all RAM (on at least one system configuration)"
href="https://bugs.freedesktop.org/show_bug.cgi?id=98420#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEEDINFO "
title="NEEDINFO - Memory leak in drm_atomic.c eventually (few days) consuming all RAM (on at least one system configuration)"
href="https://bugs.freedesktop.org/show_bug.cgi?id=98420">bug 98420</a>
from <span class="vcard"><a class="email" href="mailto:felix.monninger@gmail.com" title="Felix Monninger <felix.monninger@gmail.com>"> <span class="fn">Felix Monninger</span></a>
</span></b>
<pre>(In reply to Chris Wilson from <a href="show_bug.cgi?id=98420#c2">comment #2</a>)
<span class="quote">> Ah, I thought I sent this to dri-devel this morning:
>
> <a href="https://lists.freedesktop.org/archives/dri-devel/2016-October/122066.html">https://lists.freedesktop.org/archives/dri-devel/2016-October/122066.html</a>
>
> If you want to send a "I made this!" in reply (with sob), quite happy to
> adjust authorship.</span >
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]
<a href="https://github.com/torvalds/linux/commit/5f911905054a64cf8c7871fddd33f4af74f07a17">https://github.com/torvalds/linux/commit/5f911905054a64cf8c7871fddd33f4af74f07a17</a></pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
<li>You are the QA Contact for the bug.</li>
<li>You are on the CC list for the bug.</li>
</ul>
</body>
</html>