Re: [PATCH v5 2/6] kho: support incremental entry publication in kho_block

[Mike Rapoport]

Hi,

> To allow LUO sessions to be frozen and committed into the outgoing KHO
> subtree from userspace ahead of reboot, kho_block needs to support
> publishing entries one by one into a live serialized block chain while
> new blocks may still be preallocated or shrunk for uncommitted sessions.
> 
> Currently, kho_block assumes outgoing block sets are populated in a
> single batch during reboot() before the chain is exposed: preallocated
> blocks are linked onto the chain immediately even while empty
> (ser->count == 0), and reserving a slot increments ser->count before the
> entry is populated.
> 
> Address this by keeping preallocated outgoing blocks unlinked in
> kho_block_add(), linking blocks lazily only as entries are written. Add
> kho_block_set_it_append() to copy each entry into its slot before
> publishing the updated ser->count and next-block link via
> smp_store_release(), paired with smp_load_acquire() in
> kho_block_set_it_read_entry().
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> ---
>  include/linux/kho_block.h     |  1 +
>  kernel/liveupdate/kho_block.c | 86 +++++++++++++++++++++++++++++++----
>  2 files changed, 77 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/kho_block.h b/include/linux/kho_block.h
> index 93a7cc2be5f51..a9e696ef3e08e 100644
> --- a/include/linux/kho_block.h
> +++ b/include/linux/kho_block.h
> @@ -100,6 +100,7 @@ static inline bool kho_block_set_is_empty(struct kho_block_set *bs)
>  
>  void kho_block_set_it_init(struct kho_block_set_it *it, struct kho_block_set *bs);
>  void *kho_block_set_it_reserve_entry(struct kho_block_set_it *it);
> +void *kho_block_set_it_append(struct kho_block_set_it *it, const void *src);
>  void *kho_block_set_it_read_entry(struct kho_block_set_it *it);
>  void *kho_block_set_it_prev(struct kho_block_set_it *it);
>  
> diff --git a/kernel/liveupdate/kho_block.c b/kernel/liveupdate/kho_block.c
> index 0d2a342ef4228..2813c6fd593c4 100644
> --- a/kernel/liveupdate/kho_block.c
> +++ b/kernel/liveupdate/kho_block.c
> @@ -16,6 +16,12 @@
>   * array of serialized entries. Multiple blocks are linked together via a
>   * physical pointer in the header, forming a linked list that can be easily
>   * traversed in both the current and the next kernel.
> + *
> + * Outgoing blocks preallocated via kho_block_set_grow() remain unlinked until
> + * entries are written into them. Callers publishing entries incrementally into
> + * a live KHO subtree can use kho_block_set_it_append(), which populates each
> + * entry before exposing the updated block count or next-block link via
> + * release-store ordering.


>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -93,9 +99,7 @@ static int kho_block_add(struct kho_block_set *bs,
>  	list_add_tail(&block->list, &bs->blocks);
>  	bs->nblocks++;
>  
> -	if (last)
> -		last->ser->next = virt_to_phys(ser);
> -	else
> +	if (!last)
>  		bs->head_pa = virt_to_phys(ser);


>  
>  	return 0;
> @@ -129,14 +133,15 @@ static void kho_block_set_shrink_one(struct kho_block_set *bs)
>  	last = list_last_entry(&bs->blocks, struct kho_block, list);
>  	list_del(&last->list);
>  	bs->nblocks--;
> -	kho_block_free_ser(bs, last->ser);
> -	kfree(last);
>  
>  	new_last = list_last_entry_or_null(&bs->blocks, struct kho_block, list);
>  	if (new_last)
> -		new_last->ser->next = 0;
> +		WRITE_ONCE(new_last->ser->next, 0);

Why this is needed? Docs explicitly state that "Synchronization and
locking are the responsibility of the caller".

>  	else
>  		bs->head_pa = 0;
> +
> +	kho_block_free_ser(bs, last->ser);
> +	kfree(last);

This change seems unnecessary.

>  }
>  
>  /**
> @@ -144,9 +149,9 @@ static void kho_block_set_shrink_one(struct kho_block_set *bs)
>   * @bs:    The block set.
>   * @count: The target number of valid entries to accommodate.
>   *
> - * Dynamically preallocates and links preserved memory blocks if the target
> - * entry count exceeds the current total capacity of the set, ensuring they
> - * are available during serialization/deserialization.
> + * Dynamically preallocates preserved memory blocks if the target entry count
> + * exceeds the current total capacity of the set, ensuring they are available
> + * during serialization/deserialization.
>   *
>   * Context: Caller must hold a lock protecting the block set.
>   * Return: 0 on success, or a negative errno on failure.
> @@ -318,6 +323,7 @@ void kho_block_set_clear(struct kho_block_set *bs)
>  	struct kho_block *block;
>  
>  	list_for_each_entry(block, &bs->blocks, list) {
> +		block->ser->next = 0;
>  		block->ser->count = 0;
>  		memset(block->ser + 1, 0, KHO_BLOCK_SIZE - sizeof(*block->ser));
>  	}
> @@ -348,6 +354,7 @@ void kho_block_set_it_init(struct kho_block_set_it *it, struct kho_block_set *bs
>   */
>  void *kho_block_set_it_reserve_entry(struct kho_block_set_it *it)
>  {
> +	struct kho_block *prev = NULL;
>  	void *entry;
>  
>  	if (!it->block)
> @@ -356,15 +363,73 @@ void *kho_block_set_it_reserve_entry(struct kho_block_set_it *it)
>  	if (it->i == it->bs->count_per_block) {
>  		if (list_is_last(&it->block->list, &it->bs->blocks))
>  			return NULL;
> +		prev = it->block;
>  		it->block = list_next_entry(it->block, list);
>  		it->i = 0;
>  	}
>  
> +	if (!it->i)
> +		it->block->ser->next = 0;
> +
>  	entry = kho_block_entry(it, it->i++);
>  	it->block->ser->count = it->i;
> +	if (prev)
> +		prev->ser->next = virt_to_phys(it->block->ser);

kernel-doc should mention that this function now links blocks.

And I believe there should be a paragraph about the new flow in the DOC:
section.

> +
>  	return entry;
>  }
>  
> +/**
> + * kho_block_set_it_append - Write and publish an entry at the next slot.
> + * @it:  The block iterator.
> + * @src: Entry data of size @it->bs->entry_size to copy into the slot.
> + *
> + * Copies @src into the next available slot before updating the block's entry
> + * count or linking a newly occupied block to the previous block, so concurrent
> + * readers never observe an uninitialized slot or an empty block.
> + *
> + * Return: A pointer to the written entry slot, or NULL if the block set's
> + * capacity is fully exhausted.
> + */
> +void *kho_block_set_it_append(struct kho_block_set_it *it, const void *src)
> +{
> +	struct kho_block *prev = NULL;
> +	void *dst;
> +
> +	if (!it->block)
> +		return NULL;
> +
> +	if (it->i == it->bs->count_per_block) {
> +		if (list_is_last(&it->block->list, &it->bs->blocks))
> +			return NULL;
> +		prev = it->block;
> +		it->block = list_next_entry(it->block, list);
> +		it->i = 0;
> +	}
> +
> +	if (!it->i)
> +		WRITE_ONCE(it->block->ser->next, 0);
> +
> +	dst = kho_block_entry(it, it->i++);
> +	memcpy(dst, src, it->bs->entry_size);
> +
> +	/*
> +	 * Ensure the serialized entry is visible before exposing the
> +	 * incremented count to concurrent readers.
> +	 */
> +	smp_store_release(&it->block->ser->count, it->i);
> +	if (prev) {
> +		/*
> +		 * Ensure the new block's first entry and header are visible
> +		 * before linking prev->ser->next for concurrent readers.
> +		 */
> +		smp_store_release(&prev->ser->next,
> +				  virt_to_phys(it->block->ser));

Until now the locking model was "the caller is responsible to hold a
lock".

Using WRITE_ONCE() and smp_store_release() instead of plain assignments
implies that it is not anymore, or at least not always.

This should be a well documented separate change.

-- 
Sincerely yours,
Mike.