On Mon, Jan 28, 2013 at 6:18 PM, Brian Paul <bri...@vmware.com> wrote: > On 01/27/2013 06:52 PM, Marek Olšák wrote: >> >> Module: Mesa >> Branch: master >> Commit: 87592cff57feef29565150b9203e220b50623f30 >> URL: >> http://cgit.freedesktop.org/mesa/mesa/commit/?id=87592cff57feef29565150b9203e220b50623f30 >> >> Author: Marek Olšák<mar...@gmail.com> >> Date: Mon Jan 28 02:47:24 2013 +0100 >> >> gallium/u_upload_mgr: fix a serious memory leak >> >> It can eat all memory and crash in a matter of minutes with r600g. >> >> --- >> >> src/gallium/auxiliary/util/u_upload_mgr.c | 3 +-- >> 1 files changed, 1 insertions(+), 2 deletions(-) >> >> diff --git a/src/gallium/auxiliary/util/u_upload_mgr.c >> b/src/gallium/auxiliary/util/u_upload_mgr.c >> index 47d39af..6859751 100644 >> --- a/src/gallium/auxiliary/util/u_upload_mgr.c >> +++ b/src/gallium/auxiliary/util/u_upload_mgr.c >> @@ -167,7 +167,7 @@ enum pipe_error u_upload_alloc( struct u_upload_mgr >> *upload, >> * sure the caller doesn't get garbage values. >> */ >> *out_offset = ~0; >> - *outbuf = NULL; >> + pipe_resource_reference(outbuf, NULL); >> *ptr = NULL; >> >> /* Make sure we have enough space in the upload buffer >> @@ -189,7 +189,6 @@ enum pipe_error u_upload_alloc( struct u_upload_mgr >> *upload, >> PIPE_TRANSFER_UNSYNCHRONIZED, >> &upload->transfer); >> if (!upload->map) { >> - pipe_resource_reference(outbuf, NULL); >> upload->transfer = NULL; >> return PIPE_ERROR_OUT_OF_MEMORY; >> } >> > > I'm not 100% sure this is right either. > > When I was working on this last week, all the calls to u_upload_alloc() that > I was looking at were setting the outbuf variable to NULL before the call. > But per r600g (and others?) that's not true.
u_upload_data also calls u_upload_alloc, so all calls to u_upload_data should be checked as well. > > With your change above, will it ever be the case that the incoming outbuf > will have a refcount=1? If so, it seems we'd be mistakenly deallocating the > buffer at this point. > > I _think_ that the refcount will always be >1 since it should be a pointer > to the u_upload_mgr::buffer buffer. But I haven't completely convinced > myself of that. > > The safest solution might be to restore the original code (before my patch) > regarding outbuf. No, it's alright. u_upload_alloc shouldn't usually fail and outbuf is always unreferenced later in the code. That has always been the expected behavior. r600g sets outbuf to a member of r600_context, which can be non-NULL, and it expects the buffer to be unreferenced in u_upload_data. The original code might have lower overhead in the case of no error though. Marek _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
