[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [UNIKRAFT PATCH 00/18] Allocator statistics
 
 
Hi! I'm back with the review for the 2nd part of this patch series. The first thing is that, unfortunately, this series can no longer be applied because of removing the `THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.` line from: `lib/ukalloc/include/uk/alloc.h`, `lib/ukalloc/include/uk/alloc_impl.h` and `lib/nolibc/malloc.c`. Looks good overall, but I have a few more comments for the 10th and 17th patch. I will leave them there.  Best wishes, Laurentiu
 
 
 Hi Simon,
  I was delegated to review only a part of this patch series (patches from 1 to 9). Great work overall, but I have some comments which I will leave inline at the corresponding patches(1, 5, 6 and 7).
  Best wishes, Laurentiu
  
 This patch series introduces allocator statistics. With minimal 
instrumentation, allocators can report counters on allocation 
and freeing events that happened. 
For example, the following could be queried per allocator: 
 
 last_alloc_size:         230 B /* size of the last allocation */ 
 max_alloc_size:         4096 B /* biggest satisfied allocation size */ 
 min_alloc_size:           12 B /* smallest satisfied allocation size */ 
 tot_nb_allocs:            75   /* total number of satisfied allocations */ 
 tot_nb_frees:             22   /* total number of satisfied free operations */ 
 cur_nb_allocs:            53   /* current number of active allocations */ 
 max_nb_allocs:            53   /* maximum number of active allocations */ 
 max_mem_use:          218023 B /* maximum amount of memory that was in use */ 
 cur_mem_use:          217088 B /* current amount of memory that is in use */ 
 nb_enomem:                 0   /* number of times failing allocation requests */ 
 
Note: The meaning of `CONFIG_IFSTAT` changes with this patch series. 
The configuration option originally raised wrong expectations while it 
it just provided an API to query the amount of available memory from 
an allocator. `CONFIG_IFSTAT` is now enabling these detailed allocator 
statistics. 
 
Note: Since this patch series changes some details on the internal ukalloc 
API (`<uk/alloc_impl.h>`), external memory allocators (like TLSF, mimalloc) 
need to be adopted as well. For retrieving statistics from them, they also 
need to be instrumented to count alloc and free events. 
 
The first four commits are doing minor corrections of the API headers. 
Afterwards, the statistics API is introduced. Additionally, the last 
commits introduce a one consolidated global allocator statistics, as 
well as a per library allocator statistics. `lib/nolibc` had to 
instrumented in order to support per-library statistics properly. 
Similar to `nolibc`, `newlib` and `musl` need to be instrumented, too to 
properly support per-library statistics for libc allocation interfaces: 
malloc(), calloc(), free(), etc. 
 
Simon Kuenzer (18): 
  lib/ukalloc: Correct BSD license header 
  lib/ukalloc: Move ifpages comment 
  lib/ukalloc: `extern C {` at the beginning of <uk/alloc.h> 
  lib/ukalloc: Move `uk_zalloc()` after `uk_calloc()` 
  lib/ukalloc: Introduce `uk_alloc_maxalloc()` and make 
    `uk_alloc_availmem()` available 
  lib/ukalloc: Introduce `uk_alloc_pavail()`, `uk_alloc_pmaxalloc()` 
  lib/ukalloc: Wrappers for "fee memory" interfaces 
  lib/ukalloc: Iterator helper for allocators 
  lib/ukalloc: Introduce `uk_alloc_availmem_total(), 
    `uk_alloc_pavail_total()` 
  lib/ukalloc: Allocator statistics 
  lib/ukalloc: Global statistics 
  lib/ukallocbbuddy: Instrumentation for statistics 
  lib/ukallocregion: Internal functions as `static` 
  lib/ukallocregion: Instrumentation for statistics 
  lib/ukalloc: Per-library allocation statistics 
  lib/ukalloc: Iterator for per-library statistics 
  lib/ukalloc: Use Unikraft internal types 
  lib/nolibc: Enable per-library allocator statistics 
 
 lib/nolibc/Makefile.uk                   |   1 - 
 lib/nolibc/include/stdlib.h              |  39 ++- 
 lib/ukalloc/Config.uk                    |  29 ++- 
 lib/ukalloc/Makefile.uk                  |   5 + 
 lib/ukalloc/alloc.c                      | 225 ++++++++++------ 
 lib/ukalloc/exportsyms.uk                |   9 + 
 lib/ukalloc/include/uk/alloc.h           | 174 +++++++++---- 
 lib/ukalloc/include/uk/alloc_impl.h      | 179 +++++++++++-- 
 lib/ukalloc/libstats.c                   | 311 +++++++++++++++++++++++ 
 lib/ukalloc/libstats.ld                  |   9 + 
 lib/ukalloc/libstats.localsyms.uk        |   2 + 
 lib/{nolibc/malloc.c => ukalloc/stats.c} |  56 ++-- 
 lib/ukallocbbuddy/bbuddy.c               |  50 +++- 
 lib/ukallocregion/region.c               |  54 +++- 
 plat/common/include/pci/pci_bus.h        |   2 + 
 plat/common/memory.c                     |   2 + 
 plat/xen/gnttab.c                        |   1 + 
 plat/xen/memory.c                        |   2 + 
 plat/xen/x86/gnttab.c                    |   1 + 
 19 files changed, 952 insertions(+), 199 deletions(-) 
 create mode 100644 lib/ukalloc/libstats.c 
 create mode 100644 lib/ukalloc/libstats.ld 
 create mode 100644 lib/ukalloc/libstats.localsyms.uk 
 rename lib/{nolibc/malloc.c => ukalloc/stats.c} (62%) 
 
--  
2.20.1 
 
  
  
 
    
     |