| Message ID | 20210315165312.22453-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] docs/devel: expand style section of memory management | expand |
On 15/03/2021 17.57, Peter Maydell wrote: > On Mon, 15 Mar 2021 at 16:53, Alex Bennée <alex.bennee@linaro.org> wrote: >> -Prefer g_new(T, n) instead of g_malloc(sizeof(T) ``*`` n) for the following >> +Care should be taken to avoid introducing places where the guest could >> +trigger an exit. For example using ``g_malloc`` on start-up is fine >> +if the result of a failure is going to be a fatal exit anyway. There >> +may be some start-up cases where failing is unreasonable (for example >> +speculatively loading debug symbols). >> + >> +However if we are doing an allocation because of something the guest >> +has done we should never trigger an exit. The code may deal with this >> +by trying to allocate less memory and continue or re-designed to allocate >> +buffers on start-up. > > I think this is overly strong. We want to avoid malloc-or-die for > cases where the guest gets to decide how big the allocation is; > but if we're doing a single small fixed-size allocation that happens > to be triggered by a guest action we should be OK to g_malloc() that > I think. I agree with Peter. If the host is so much out-of-memory that we even can't allocate some few bytes anymore (let's say less than 4k), the system is pretty much dead anyway and it might be better to terminate the program immediately instead of continuing with the out-of-memory situation. Thomas
On Mon, Mar 15, 2021 at 06:04:10PM +0100, Thomas Huth wrote: > On 15/03/2021 17.57, Peter Maydell wrote: > > On Mon, 15 Mar 2021 at 16:53, Alex Bennée <alex.bennee@linaro.org> wrote: > > > -Prefer g_new(T, n) instead of g_malloc(sizeof(T) ``*`` n) for the following > > > +Care should be taken to avoid introducing places where the guest could > > > +trigger an exit. For example using ``g_malloc`` on start-up is fine > > > +if the result of a failure is going to be a fatal exit anyway. There > > > +may be some start-up cases where failing is unreasonable (for example > > > +speculatively loading debug symbols). > > > + > > > +However if we are doing an allocation because of something the guest > > > +has done we should never trigger an exit. The code may deal with this > > > +by trying to allocate less memory and continue or re-designed to allocate > > > +buffers on start-up. > > > > I think this is overly strong. We want to avoid malloc-or-die for > > cases where the guest gets to decide how big the allocation is; > > but if we're doing a single small fixed-size allocation that happens > > to be triggered by a guest action we should be OK to g_malloc() that > > I think. > > I agree with Peter. If the host is so much out-of-memory that we even can't > allocate some few bytes anymore (let's say less than 4k), the system is > pretty much dead anyway and it might be better to terminate the program > immediately instead of continuing with the out-of-memory situation. On a Linux host you're almost certainly not going to see g_malloc fail for small allocations at least. Instead at some point the host will be under enough memory pressure that the OOM killer activates and reaps arbitrary processes based on some criteria it has, freeing up memory for malloc to succeed (unless OOM killer picked you as the victim). Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Mar 15, 2021 at 06:04:10PM +0100, Thomas Huth wrote: >> On 15/03/2021 17.57, Peter Maydell wrote: >> > On Mon, 15 Mar 2021 at 16:53, Alex Bennée <alex.bennee@linaro.org> wrote: >> > > -Prefer g_new(T, n) instead of g_malloc(sizeof(T) ``*`` n) for the following >> > > +Care should be taken to avoid introducing places where the guest could >> > > +trigger an exit. For example using ``g_malloc`` on start-up is fine >> > > +if the result of a failure is going to be a fatal exit anyway. There >> > > +may be some start-up cases where failing is unreasonable (for example >> > > +speculatively loading debug symbols). >> > > + >> > > +However if we are doing an allocation because of something the guest >> > > +has done we should never trigger an exit. The code may deal with this >> > > +by trying to allocate less memory and continue or re-designed to allocate >> > > +buffers on start-up. >> > >> > I think this is overly strong. We want to avoid malloc-or-die for >> > cases where the guest gets to decide how big the allocation is; >> > but if we're doing a single small fixed-size allocation that happens >> > to be triggered by a guest action we should be OK to g_malloc() that >> > I think. >> >> I agree with Peter. If the host is so much out-of-memory that we even can't >> allocate some few bytes anymore (let's say less than 4k), the system is >> pretty much dead anyway and it might be better to terminate the program >> immediately instead of continuing with the out-of-memory situation. > > On a Linux host you're almost certainly not going to see g_malloc > fail for small allocations at least. Instead at some point the host > will be under enough memory pressure that the OOM killer activates > and reaps arbitrary processes based on some criteria it has, freeing > up memory for malloc to succeed (unless OOM killer picked you as the > victim). OK how about this wording: Please note that ``g_malloc`` will exit on allocation failure, so there is no need to test for failure (as you would have to with ``malloc``). Generally using ``g_malloc`` on start-up is fine as the result of a failure to allocate memory is going to be a fatal exit anyway. There may be some start-up cases where failing is unreasonable (for example speculatively loading a large debug symbol table). Care should be taken to avoid introducing places where the guest could trigger an exit by causing a large allocation. For small allocations, of the order of 4k, a failure to allocate is likely indicative of an overloaded host and allowing ``g_malloc`` to ``exit`` is a reasonable approach. However for larger allocations where we could realistically fall-back to a smaller one if need be we should use functions like ``g_try_new`` and check the result. For example this is valid approach for a time/space trade-off like ``tlb_mmu_resize_locked`` in the SoftMMU TLB code. > > Regards, > Daniel -- Alex Bennée
On Mon, Mar 15, 2021 at 05:54:17PM +0000, Alex Bennée wrote: > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Mon, Mar 15, 2021 at 06:04:10PM +0100, Thomas Huth wrote: > >> On 15/03/2021 17.57, Peter Maydell wrote: > >> > On Mon, 15 Mar 2021 at 16:53, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > > -Prefer g_new(T, n) instead of g_malloc(sizeof(T) ``*`` n) for the following > >> > > +Care should be taken to avoid introducing places where the guest could > >> > > +trigger an exit. For example using ``g_malloc`` on start-up is fine > >> > > +if the result of a failure is going to be a fatal exit anyway. There > >> > > +may be some start-up cases where failing is unreasonable (for example > >> > > +speculatively loading debug symbols). > >> > > + > >> > > +However if we are doing an allocation because of something the guest > >> > > +has done we should never trigger an exit. The code may deal with this > >> > > +by trying to allocate less memory and continue or re-designed to allocate > >> > > +buffers on start-up. > >> > > >> > I think this is overly strong. We want to avoid malloc-or-die for > >> > cases where the guest gets to decide how big the allocation is; > >> > but if we're doing a single small fixed-size allocation that happens > >> > to be triggered by a guest action we should be OK to g_malloc() that > >> > I think. > >> > >> I agree with Peter. If the host is so much out-of-memory that we even can't > >> allocate some few bytes anymore (let's say less than 4k), the system is > >> pretty much dead anyway and it might be better to terminate the program > >> immediately instead of continuing with the out-of-memory situation. > > > > On a Linux host you're almost certainly not going to see g_malloc > > fail for small allocations at least. Instead at some point the host > > will be under enough memory pressure that the OOM killer activates > > and reaps arbitrary processes based on some criteria it has, freeing > > up memory for malloc to succeed (unless OOM killer picked you as the > > victim). > > OK how about this wording: > > Please note that ``g_malloc`` will exit on allocation failure, so > there is no need to test for failure (as you would have to with > ``malloc``). Generally using ``g_malloc`` on start-up is fine as the > result of a failure to allocate memory is going to be a fatal exit > anyway. There may be some start-up cases where failing is unreasonable > (for example speculatively loading a large debug symbol table). > > Care should be taken to avoid introducing places where the guest could > trigger an exit by causing a large allocation. For small allocations, > of the order of 4k, a failure to allocate is likely indicative of an > overloaded host and allowing ``g_malloc`` to ``exit`` is a reasonable > approach. However for larger allocations where we could realistically > fall-back to a smaller one if need be we should use functions like > ``g_try_new`` and check the result. For example this is valid approach > for a time/space trade-off like ``tlb_mmu_resize_locked`` in the > SoftMMU TLB code. Fine with me Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Mar 15, 2021 at 06:04:10PM +0100, Thomas Huth wrote: >> On 15/03/2021 17.57, Peter Maydell wrote: >> > On Mon, 15 Mar 2021 at 16:53, Alex Bennée <alex.bennee@linaro.org> wrote: >> > > -Prefer g_new(T, n) instead of g_malloc(sizeof(T) ``*`` n) for the following >> > > +Care should be taken to avoid introducing places where the guest could >> > > +trigger an exit. For example using ``g_malloc`` on start-up is fine >> > > +if the result of a failure is going to be a fatal exit anyway. There >> > > +may be some start-up cases where failing is unreasonable (for example >> > > +speculatively loading debug symbols). >> > > + >> > > +However if we are doing an allocation because of something the guest >> > > +has done we should never trigger an exit. The code may deal with this >> > > +by trying to allocate less memory and continue or re-designed to allocate >> > > +buffers on start-up. >> > >> > I think this is overly strong. We want to avoid malloc-or-die for >> > cases where the guest gets to decide how big the allocation is; >> > but if we're doing a single small fixed-size allocation that happens >> > to be triggered by a guest action we should be OK to g_malloc() that >> > I think. >> >> I agree with Peter. If the host is so much out-of-memory that we even can't >> allocate some few bytes anymore (let's say less than 4k), the system is >> pretty much dead anyway and it might be better to terminate the program >> immediately instead of continuing with the out-of-memory situation. > > On a Linux host you're almost certainly not going to see g_malloc > fail for small allocations at least. Instead at some point the host > will be under enough memory pressure that the OOM killer activates > and reaps arbitrary processes based on some criteria it has, freeing > up memory for malloc to succeed (unless OOM killer picked you as the > victim). This happens even for large allocations. In a prior iteration of the "When it's okay to treat OOM as fatal?" discussion[1], I showed that Linux malloc() and g_malloc() happily give me a terabyte of memory I don't have in 1024 chunks of 1 GiB each. I just reran the test, same results. See also [2]. [1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg04229.html [2] http://turnoff.us/geek/bad-malloc/
diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 8b0bdb3570..823fa6f209 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -385,17 +385,35 @@ avoided. Low level memory management =========================== -Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign +Use of the ``malloc/free/realloc/calloc/valloc/memalign/posix_memalign`` APIs is not allowed in the QEMU codebase. Instead of these routines, -use the GLib memory allocation routines g_malloc/g_malloc0/g_new/ -g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree -APIs. +use the GLib memory allocation routines +``g_malloc/g_malloc0/g_new/g_new0/g_realloc/g_free`` +or QEMU's ``qemu_memalign/qemu_blockalign/qemu_vfree`` APIs. -Please note that g_malloc will exit on allocation failure, so there -is no need to test for failure (as you would have to with malloc). -Calling g_malloc with a zero size is valid and will return NULL. +Please note that ``g_malloc`` will exit on allocation failure, so there +is no need to test for failure (as you would have to with ``malloc``). -Prefer g_new(T, n) instead of g_malloc(sizeof(T) ``*`` n) for the following +Care should be taken to avoid introducing places where the guest could +trigger an exit. For example using ``g_malloc`` on start-up is fine +if the result of a failure is going to be a fatal exit anyway. There +may be some start-up cases where failing is unreasonable (for example +speculatively loading debug symbols). + +However if we are doing an allocation because of something the guest +has done we should never trigger an exit. The code may deal with this +by trying to allocate less memory and continue or re-designed to allocate +buffers on start-up. + +If the lifetime of the allocation is within the function and there are +multiple exist paths you can also improve the readability of the code +by using ``g_autofree`` and related annotations. See :ref:`autofree-ref` +for more details. + + +Calling ``g_malloc`` with a zero size is valid and will return NULL. + +Prefer ``g_new(T, n)`` instead of ``g_malloc(sizeof(T) * n)`` for the following reasons: * It catches multiplication overflowing size_t; @@ -409,8 +427,8 @@ Declarations like are acceptable, though. -Memory allocated by qemu_memalign or qemu_blockalign must be freed with -qemu_vfree, since breaking this will cause problems on Win32. +Memory allocated by ``qemu_memalign`` or ``qemu_blockalign`` must be freed with +``qemu_vfree``, since breaking this will cause problems on Win32. String manipulation =================== @@ -485,6 +503,8 @@ In addition, QEMU assumes that the compiler does not use the latitude given in C99 and C11 to treat aspects of signed '<<' as undefined, as documented in the GNU Compiler Collection manual starting at version 4.0. +.. _autofree-ref: + Automatic memory deallocation =============================
This aims to provide a bit more guidance for those who take on one of our "clean up memory allocation" bite-sized tasks. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- docs/devel/style.rst | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) -- 2.20.1