Re: [PATCH V2] lib/sbitmap: kill 'depth' from sbitmap_word

[John Garry <john.garry@huawei.com>]

On 10/01/2022 07:29, Ming Lei wrote:
> Only the last sbitmap_word can have different depth, and all the others
> must have same depth of 1U << sb->shift, so not necessary to store it in
> sbitmap_word, and it can be retrieved easily and efficiently by adding
> one internal helper of __map_depth(sb, index).
> 
> Remove 'depth' field from sbitmap_word, then the annotation of
> ____cacheline_aligned_in_smp for 'word' isn't needed any more.
> 
> Not see performance effect when running high parallel IOPS test on
> null_blk.
> 
> This way saves us one cacheline(usually 64 words) per each sbitmap_word.
> 
> Cc: Martin Wilck <martin.wilck@suse.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Reviewed-by: John Garry <john.garry@huawei.com>

> ---
> V2:
> 	- remove the annotation of ____cacheline_aligned_in_smp for 'word'
> 	as suggested by Jens
> 
>   include/linux/sbitmap.h | 17 ++++++++++-------
>   lib/sbitmap.c           | 34 ++++++++++++++--------------------
>   2 files changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/sbitmap.h b/include/linux/sbitmap.h
> index fc0357a6e19b..3754dc45f890 100644
> --- a/include/linux/sbitmap.h
> +++ b/include/linux/sbitmap.h
> @@ -27,15 +27,10 @@ struct seq_file;
>    * struct sbitmap_word - Word in a &struct sbitmap.
>    */
>   struct sbitmap_word {
> -	/**
> -	 * @depth: Number of bits being used in @word/@cleared
> -	 */
> -	unsigned long depth;
> -
>   	/**
>   	 * @word: word holding free bits
>   	 */
> -	unsigned long word ____cacheline_aligned_in_smp;
> +	unsigned long word;
>   
>   	/**
>   	 * @cleared: word holding cleared bits
> @@ -164,6 +159,14 @@ struct sbitmap_queue {
>   int sbitmap_init_node(struct sbitmap *sb, unsigned int depth, int shift,
>   		      gfp_t flags, int node, bool round_robin, bool alloc_hint);
>   
> +/* sbitmap internal helper */
> +static inline unsigned int __map_depth(const struct sbitmap *sb, int index)
> +{
> +	if (index == sb->map_nr - 1)

Do you think that unlikely may be useful?

> +		return sb->depth - (index << sb->shift);
> +	return 1U << sb->shift;
> +}
> +