Re: [PATCH 2/5] docs: include ramblock.h in the memory API docs

[Peter Xu <peterx@redhat.com>]

On Thu, Mar 07, 2024 at 06:11:02PM +0000, Alex Bennée wrote:
> The RAMBlock concept is fairly central to RAM-like MemoryRegions so
> lets update the structure documentation and include in the docs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/memory.rst   |  1 +
>  include/exec/ramblock.h | 76 +++++++++++++++++++++++++++--------------
>  2 files changed, 52 insertions(+), 25 deletions(-)
> 
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 69c5e3f914a..ed24708fce3 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -369,4 +369,5 @@ callbacks are called:
>  API Reference
>  -------------
>  
> +.. kernel-doc:: include/exec/ramblock.h
>  .. kernel-doc:: include/exec/memory.h
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 848915ea5bf..eb2416b6f66 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -24,68 +24,94 @@
>  #include "qemu/rcu.h"
>  #include "exec/ramlist.h"
>  
> +/**
> + * struct RAMBlock - represents a chunk of RAM
> + *
> + * RAMBlocks can be backed by allocated RAM or a file-descriptor. See
> + * @flags for the details. For the purposes of migration various book
> + * keeping and dirty state tracking elements are also tracked in this
> + * structure.
> + */
>  struct RAMBlock {
> +    /** @rcu: used for lazy free under RCU */
>      struct rcu_head rcu;
> +    /** @mr: parent MemoryRegion the block belongs to */
>      struct MemoryRegion *mr;
> +    /** @host: pointer to host address of RAM */
>      uint8_t *host;
> -    uint8_t *colo_cache; /* For colo, VM's ram cache */
> +    /** @colo_cache: For colo, VM's ram cache */
> +    uint8_t *colo_cache;
> +    /** @offset: offset into host backing store??? or guest address space? */

I think it's the first, or to be explicit, "ram_addr_t address space"?

>      ram_addr_t offset;
> +    /** @used_length: amount of store used */
>      ram_addr_t used_length;
> +    /** @max_length: for blocks that can be resized the max possible */
>      ram_addr_t max_length;
> +    /** @resized: callback notifier when block resized  */
>      void (*resized)(const char*, uint64_t length, void *host);
> +    /** @flags: see RAM_* flags in memory.h  */
>      uint32_t flags;
> -    /* Protected by the BQL.  */
> +    /** @idstr: Protected by the BQL.  */

Hmm, I think RCU should be enough to read an idstr?  Maybe as simple as:

@idstr: Name of the ramblock

?

>      char idstr[256];
> -    /* RCU-enabled, writes protected by the ramlist lock */
> +    /**
> +     * @next: next RAMBlock, RCU-enabled, writes protected by the
> +     * ramlist lock
> +     */
>      QLIST_ENTRY(RAMBlock) next;
> +    /** @ramblock_notifiers: list of RAMBlockNotifier notifiers */
>      QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
> +    /** @fd: fd of backing store if used */

Can also add: "For anonymous RAMBlocks, it's always -1".

>      int fd;
> +    /** @fd_offset: offset into the fd based backing store */
>      uint64_t fd_offset;
> +    /** @page_size: ideal page size of backing store*/

"ideal" might be a bit ambiguous.  How about "backend page size"?

For anon, it's always PAGE_SIZE, for file, it's the one reported in
fstatfs().  But this might be too verbose.

>      size_t page_size;
> -    /* dirty bitmap used during migration */
> +    /** @bmap:  dirty bitmap used during migration */
>      unsigned long *bmap;
>  
>      /*
>       * Below fields are only used by mapped-ram migration
>       */
> -    /* bitmap of pages present in the migration file */
> +
> +    /** @file_bmap: bitmap of pages present in the migration file  */

Can append "(only used in mapped-ram migrations)".  This may also apply to
below two fields.

>      unsigned long *file_bmap;
> -    /*
> -     * offset in the file pages belonging to this ramblock are saved,
> -     * used only during migration to a file.
> -     */
> +    /** @bitmap_offset: offset in the migration file of the bitmaps */

s/bitmaps/bitmap/, as there's only one for each rb.

>      off_t bitmap_offset;
> +    /** @pages_offset: offset in the migration file of the pages */
>      uint64_t pages_offset;
>  
> -    /* bitmap of already received pages in postcopy */
> +    /** @receivedmap: bitmap of already received pages in postcopy */
>      unsigned long *receivedmap;
>  
> -    /*
> -     * bitmap to track already cleared dirty bitmap.  When the bit is
> -     * set, it means the corresponding memory chunk needs a log-clear.
> -     * Set this up to non-NULL to enable the capability to postpone
> -     * and split clearing of dirty bitmap on the remote node (e.g.,
> -     * KVM).  The bitmap will be set only when doing global sync.
> +    /**
> +     * @clear_bmap: bitmap to track already cleared dirty bitmap. When
> +     * the bit is set, it means the corresponding memory chunk needs a
> +     * log-clear. Set this up to non-NULL to enable the capability to
> +     * postpone and split clearing of dirty bitmap on the remote node
> +     * (e.g., KVM). The bitmap will be set only when doing global
> +     * sync.
>       *
>       * It is only used during src side of ram migration, and it is
>       * protected by the global ram_state.bitmap_mutex.
>       *
>       * NOTE: this bitmap is different comparing to the other bitmaps
>       * in that one bit can represent multiple guest pages (which is
> -     * decided by the `clear_bmap_shift' variable below).  On
> +     * decided by the @clear_bmap_shift variable below).  On
>       * destination side, this should always be NULL, and the variable
> -     * `clear_bmap_shift' is meaningless.
> +     * @clear_bmap_shift is meaningless.
>       */
>      unsigned long *clear_bmap;
> +    /** @clear_bmap_shift: number pages each @clear_bmap bit represents */
>      uint8_t clear_bmap_shift;
>  
> -    /*
> -     * RAM block length that corresponds to the used_length on the migration
> -     * source (after RAM block sizes were synchronized). Especially, after
> -     * starting to run the guest, used_length and postcopy_length can differ.
> -     * Used to register/unregister uffd handlers and as the size of the received
> -     * bitmap. Receiving any page beyond this length will bail out, as it
> -     * could not have been valid on the source.
> +    /**
> +     * @postcopy_length: RAM block length that corresponds to the
> +     * @used_length on the migration source (after RAM block sizes
> +     * were synchronized). Especially, after starting to run the
> +     * guest, @used_length and @postcopy_length can differ. Used to
> +     * register/unregister uffd handlers and as the size of the
> +     * received bitmap. Receiving any page beyond this length will
> +     * bail out, as it could not have been valid on the source.
>       */
>      ram_addr_t postcopy_length;
>  };
> -- 
> 2.39.2
> 

-- 
Peter Xu